Skip to main content

max / makenotwork

sando: pin referenced artifacts in the node release gc too MNW@68491209 pinned the host store and left `gc_remote_releases` a bare count over mtime. Same defect, weaker instance: every promote and rollback rsyncs from the host store, so a node directory this evicts is re-pushed rather than lost. What it costs is a full rsync of a bundle the node already had, at the worst moment -- mid-rollback, with a tier already failing. `deploy_node` takes the pinned set and threads it to `deploy_remote`. The node mirrors the host's directory name, so `retention::pinned_dirs` already returns the right strings and nothing is recomputed per node. The parameter is `Option<&PinnedReleases>`, and the two cases are not the same: `None` means the caller could not determine the set and the gc is skipped rather than run blind, `Some(none())` means there is genuinely nothing to protect. The promote and the operator rollback fail if the pin query fails, matching `build::publish`, since neither has shipped bytes yet. The canary rollback degrades to `None` instead -- it runs when a tier is already failing, and refusing it over a failed SELECT is the worse outcome. The script carries pinned names in the positional parameters rather than a here-doc through `grep -v`: grep exits 1 when it selects no lines, which happens both when nothing is pinned and when everything is, and would abort under `set -e` for two perfectly normal cases. `case` has no exit status to trip over and matches whole names rather than prefixes. Six tests drive the real script through LocalExec rather than asserting on its text: count parity when nothing is pinned, the done condition, all pinned, missing store, shell metacharacters in names, and prefix matching. Closes sando b904e4f3.
Author: Max Johnson <me@maxj.phd> · 2026-08-25 19:21 UTC
Signed with PGP, not checked
Commit: bdf03f596397a286250f3f7373fb9b4cb1d0c712
Parent: b090fa4
4 files changed, +336 insertions, -13 deletions
@@ -269,11 +269,24 @@
269 269 ///
270 270 /// `primary_bin` is only used for logging — every file present in the staged
271 271 /// dir gets shipped.
272 + ///
273 + /// `pinned` protects the node's own `releases/` from the gc that runs after a
274 + /// successful remote deploy. The node mirrors the host's directory name, so the
275 + /// set is the host's ([`crate::retention::pinned_dirs`]) with nothing recomputed
276 + /// per node. `None` says the caller could not determine it and the gc is skipped
277 + /// rather than run blind; `Some(PinnedReleases::none())` says there is genuinely
278 + /// nothing deployed to protect. The distinction matters — the first is ignorance
279 + /// and the second is knowledge — which is why this is an `Option` and not an
280 + /// empty set standing in for both.
281 + ///
282 + /// Unused on the `ssh_target=local` path: that deploy is a symlink swap over a
283 + /// store the host gc already owns.
272 284 pub async fn deploy_node(
273 285 executor: &dyn Executor,
274 286 placement: Placement<'_>,
275 287 version: &str,
276 288 primary_bin: &str,
289 + pinned: Option<&PinnedReleases>,
277 290 ) -> Result<PathBuf> {
278 291 let node = placement.node();
279 292 let staged_release_dir = placement.bundle();
@@ -303,6 +316,7 @@
303 316 release_id,
304 317 staged_release_dir,
305 318 primary_bin,
319 + pinned,
306 320 )
307 321 .await
308 322 }
@@ -488,6 +502,7 @@
488 502 release_id: &str,
489 503 staged_release_dir: &Path,
490 504 primary_bin: &str,
505 + pinned: Option<&PinnedReleases>,
491 506 ) -> Result<PathBuf> {
492 507 let release_root = &node.release_root;
493 508 let service = &node.service_name;
@@ -667,8 +682,22 @@
667 682 .context(FailureStage::AtOrAfterSwap)?;
668 683 }
669 684
670 - if let Err(e) = gc_remote_releases(executor, release_root).await {
671 - tracing::warn!(error = %e, "remote release GC failed (non-fatal)");
685 + // No pinned set means the caller could not determine what is referenced,
686 + // and a gc that cannot tell is the exact failure this parameter exists to
687 + // stop. Skipping costs disk on the node; running blind costs the artifact a
688 + // rollback resolves to. `finalize_local_release` takes the same position by
689 + // refusing to publish at all when the pin query fails.
690 + match pinned {
691 + Some(pinned) => {
692 + if let Err(e) = gc_remote_releases(executor, release_root, pinned).await {
693 + tracing::warn!(error = %e, "remote release GC failed (non-fatal)");
694 + }
695 + }
696 + None => tracing::warn!(
697 + node = %node.name,
698 + "remote release GC skipped: the pinned set is unknown, and a gc that \
699 + cannot see what is referenced is what stranded the host store twice"
700 + ),
672 701 }
673 702
674 703 Ok(PathBuf::from(release_root)
@@ -983,18 +1012,73 @@
983 1012 Ok(())
984 1013 }
985 1014
986 - async fn gc_remote_releases(executor: &dyn Executor, release_root: &str) -> Result<()> {
987 - // `ls -t` orders by mtime desc. Skip the first N, rm the rest. `xargs -r`
988 - // is a no-op when stdin is empty (avoids `rm` complaining).
989 - let script = format!(
1015 + /// Trim a node's `releases/` to the pinned set plus the [`RELEASES_TO_KEEP`]
1016 + /// newest of what is left.
1017 + ///
1018 + /// The node mirrors the host's directory name (see [`deploy_node`]), so the same
1019 + /// [`PinnedReleases`] the host gc subtracts is the right set here — nothing has
1020 + /// to be recomputed per node.
1021 + ///
1022 + /// Weaker instance of the host defect: every promote and rollback rsyncs from
1023 + /// the host store, so a node directory evicted here is re-pushed rather than
1024 + /// lost. What it costs is a full rsync of a bundle the node already had, at the
1025 + /// worst moment — mid-rollback, with a tier already failing.
1026 + ///
1027 + /// Pinning is applied before the count, matching [`gc_local_releases`]: a pinned
1028 + /// directory is not a candidate, so it cannot occupy one of the count's slots.
1029 + async fn gc_remote_releases(
1030 + executor: &dyn Executor,
1031 + release_root: &str,
1032 + pinned: &PinnedReleases,
1033 + ) -> Result<()> {
1034 + run_checked(
1035 + executor,
1036 + &remote_gc_script(release_root, pinned),
1037 + "remote release gc",
1038 + )
1039 + .await
1040 + .map(|_| ())
1041 + }
1042 +
1043 + /// The remote gc as shell.
1044 + ///
1045 + /// Split out so the script is testable without an executor: it is the half of
1046 + /// this that can be wrong in a way `rm -rf` makes expensive.
1047 + ///
1048 + /// `ls -1t` orders by mtime desc. Pinned names are carried in the positional
1049 + /// parameters rather than a here-doc piped through `grep -v`, because `grep`
1050 + /// exits 1 when it selects no lines — which happens on both edges that matter
1051 + /// (nothing pinned, or everything pinned) and would abort the script under
1052 + /// `set -e` for the two cases that are perfectly normal. Comparing with `case`
1053 + /// has no exit status to trip over, and matches whole names rather than
1054 + /// substrings, which a `grep -F` without `-x` would not.
1055 + fn remote_gc_script(release_root: &str, pinned: &PinnedReleases) -> String {
1056 + let pins = pinned
1057 + .sorted_names()
1058 + .into_iter()
1059 + .map(sh_quote)
1060 + .collect::<Vec<_>>()
1061 + .join(" ");
1062 + // `set --` with no operands unsets the positional parameters, which is
1063 + // exactly what a nothing-pinned gc wants: the `for` below then iterates zero
1064 + // times and every directory is a candidate. Written as one branch because
1065 + // `set -- ` with an empty expansion is the same statement.
1066 + let set_pins = format!("set -- {pins}");
1067 + format!(
990 1068 "set -e; cd {root}/releases 2>/dev/null || exit 0; \
991 - ls -1t | tail -n +{keep_plus_one} | xargs -r -I{{}} rm -rf -- {{}}",
1069 + {set_pins}; \
1070 + n=0; \
1071 + ls -1t | while IFS= read -r d; do \
1072 + for p in \"$@\"; do \
1073 + if [ \"$d\" = \"$p\" ]; then continue 2; fi; \
1074 + done; \
1075 + n=$((n+1)); \
1076 + if [ \"$n\" -le {keep} ]; then continue; fi; \
1077 + rm -rf -- \"$d\"; \
1078 + done",
992 1079 root = sh_quote(release_root),
993 - keep_plus_one = RELEASES_TO_KEEP + 1,
994 - );
995 - run_checked(executor, &script, "remote release gc")
996 - .await
997 - .map(|_| ())
1080 + keep = RELEASES_TO_KEEP,
1081 + )
998 1082 }
999 1083
1000 1084 #[cfg(test)]
@@ -1614,6 +1698,180 @@
1614 1698 }
1615 1699 }
1616 1700
1701 + // ---- remote gc ----
1702 + //
1703 + // Driven through `LocalExec`, so these run the real shell the node runs
1704 + // rather than asserting on the script's text. The script is the half of the
1705 + // remote gc that can be wrong, and it is wrong with `rm -rf`.
1706 +
1707 + /// `releases/` with `total` dirs named `v00..`, oldest first by mtime.
1708 + async fn releases_by_age(root: &Path, total: usize) -> Vec<String> {
1709 + let releases = root.join("releases");
1710 + tokio::fs::create_dir_all(&releases).await.unwrap();
1711 + let mut names = Vec::new();
1712 + for i in 0..total {
1713 + let name = format!("v{i:02}");
1714 + let dir = releases.join(&name);
1715 + tokio::fs::create_dir(&dir).await.unwrap();
1716 + // A file inside, so a deletion is visible as more than an empty dir.
1717 + tokio::fs::write(dir.join("makenotwork"), b"x")
1718 + .await
1719 + .unwrap();
1720 + let f = std::fs::File::open(&dir).unwrap();
1721 + let when =
1722 + SystemTime::UNIX_EPOCH + std::time::Duration::from_secs(1_700_000_000 + i as u64);
1723 + f.set_times(std::fs::FileTimes::new().set_modified(when))
1724 + .unwrap();
1725 + names.push(name);
1726 + }
1727 + names
1728 + }
1729 +
1730 + /// Parity with the count-only script this replaced: nothing pinned, newest
1731 + /// `RELEASES_TO_KEEP` survive. If this drifts the change was not a
1732 + /// refinement of the old behaviour but a replacement of it.
1733 + #[tokio::test]
1734 + async fn gc_remote_releases_keeps_last_n_by_mtime_when_nothing_is_pinned() {
1735 + let tmp = tempfile::tempdir().unwrap();
1736 + let root = tmp.path();
1737 + let total = RELEASES_TO_KEEP + 3;
1738 + let names = releases_by_age(root, total).await;
1739 +
1740 + gc_remote_releases(&local_executor(), root.to_str().unwrap(), &no_pins())
1741 + .await
1742 + .unwrap();
1743 +
1744 + let releases = root.join("releases");
1745 + for name in names.iter().take(total - RELEASES_TO_KEEP) {
1746 + assert!(!releases.join(name).exists(), "expected pruned: {name}");
1747 + }
1748 + for name in names.iter().skip(total - RELEASES_TO_KEEP) {
1749 + assert!(releases.join(name).exists(), "expected to survive: {name}");
1750 + }
1751 + }
1752 +
1753 + /// The done condition, on the node: the dirs a tier's current and previous
1754 + /// artifacts name survive even when they are the oldest on disk and well
1755 + /// past the count. Same shape as the host-store test above, which is the
1756 + /// point — the two stores now answer the same question the same way.
1757 + #[tokio::test]
1758 + async fn gc_remote_releases_never_evicts_a_pinned_dir() {
1759 + let tmp = tempfile::tempdir().unwrap();
1760 + let root = tmp.path();
1761 + let total = RELEASES_TO_KEEP + 3;
1762 + let names = releases_by_age(root, total).await;
1763 +
1764 + let pinned: PinnedReleases = [names[0].clone(), names[1].clone()].into_iter().collect();
1765 + gc_remote_releases(&local_executor(), root.to_str().unwrap(), &pinned)
1766 + .await
1767 + .unwrap();
1768 +
1769 + let releases = root.join("releases");
1770 + for name in [&names[0], &names[1]] {
1771 + assert!(
1772 + releases.join(name).exists(),
1773 + "a referenced artifact was evicted from the node: {name}"
1774 + );
1775 + }
1776 + // And pinning does not spend the count's slots, again matching the host.
1777 + let unpinned: Vec<_> = names.iter().filter(|n| !pinned.contains(n)).collect();
1778 + let cut = unpinned.len() - RELEASES_TO_KEEP;
1779 + for name in unpinned.iter().take(cut) {
1780 + assert!(!releases.join(name).exists(), "expected pruned: {name}");
1781 + }
1782 + for name in unpinned.iter().skip(cut) {
1783 + assert!(releases.join(name).exists(), "expected to survive: {name}");
1784 + }
1785 + }
1786 +
1787 + /// Every dir pinned means the loop deletes nothing and the script still
1788 + /// exits 0. Worth its own test because the obvious implementation of this
1789 + /// filter is `grep -v`, which exits 1 when it selects no lines and would
1790 + /// have failed the deploy here under `set -e`.
1791 + #[tokio::test]
1792 + async fn gc_remote_releases_succeeds_when_everything_is_pinned() {
1793 + let tmp = tempfile::tempdir().unwrap();
1794 + let root = tmp.path();
1795 + let names = releases_by_age(root, RELEASES_TO_KEEP + 3).await;
1796 + let pinned: PinnedReleases = names.iter().cloned().collect();
1797 +
1798 + gc_remote_releases(&local_executor(), root.to_str().unwrap(), &pinned)
1799 + .await
1800 + .unwrap();
1801 +
1802 + let releases = root.join("releases");
1803 + for name in &names {
1804 + assert!(releases.join(name).exists(), "expected to survive: {name}");
1805 + }
1806 + }
1807 +
1808 + /// A `releases/` that does not exist is not an error: a node's first deploy
1809 + /// creates the dir, and gc runs on the same path.
1810 + #[tokio::test]
1811 + async fn gc_remote_releases_is_a_noop_when_the_store_is_missing() {
1812 + let tmp = tempfile::tempdir().unwrap();
1813 + gc_remote_releases(&local_executor(), tmp.path().to_str().unwrap(), &no_pins())
1814 + .await
1815 + .unwrap();
1816 + }
1817 +
1818 + /// Names reach the script as positional parameters, so a name that looks
1819 + /// like shell must be compared whole rather than expanded or split. None of
1820 + /// these can be a digest16, but the pre-identity names are version strings
1821 + /// and the pinned set is data read out of a database.
1822 + #[tokio::test]
1823 + async fn gc_remote_releases_quotes_pinned_names() {
1824 + let tmp = tempfile::tempdir().unwrap();
1825 + let root = tmp.path();
1826 + let releases = root.join("releases");
1827 + tokio::fs::create_dir_all(&releases).await.unwrap();
1828 + let awkward = ["a b", "x'y", "*"];
1829 + for name in awkward {
1830 + tokio::fs::create_dir(releases.join(name)).await.unwrap();
1831 + }
1832 + // Enough newer dirs that the count alone would evict all three.
1833 + let filler: Vec<String> = (0..=RELEASES_TO_KEEP).map(|i| format!("f{i}")).collect();
1834 + for name in &filler {
1835 + tokio::fs::create_dir(releases.join(name)).await.unwrap();
1836 + }
1837 +
1838 + let pinned: PinnedReleases = awkward.iter().map(|s| (*s).to_string()).collect();
1839 + gc_remote_releases(&local_executor(), root.to_str().unwrap(), &pinned)
1840 + .await
1841 + .unwrap();
1842 +
1843 + for name in awkward {
1844 + assert!(releases.join(name).exists(), "expected to survive: {name}");
1845 + }
1846 + }
1847 +
1848 + /// A pinned name matches a whole directory name, never a prefix of one.
1849 + /// `case`-with-globbing or a `grep -F` without `-x` would keep `v0` and
1850 + /// `v01` both because one contains the other, quietly widening the pinned
1851 + /// set past what the database said.
1852 + #[tokio::test]
1853 + async fn gc_remote_releases_matches_whole_names_not_prefixes() {
1854 + let tmp = tempfile::tempdir().unwrap();
1855 + let root = tmp.path();
1856 + let names = releases_by_age(root, RELEASES_TO_KEEP + 3).await;
1857 +
1858 + // Pin the oldest by an exact name; its neighbours share the prefix.
1859 + let pinned: PinnedReleases = [names[0].clone()].into_iter().collect();
1860 + gc_remote_releases(&local_executor(), root.to_str().unwrap(), &pinned)
1861 + .await
1862 + .unwrap();
1863 +
1864 + let releases = root.join("releases");
1865 + assert!(
1866 + releases.join(&names[0]).exists(),
1867 + "the pinned dir was evicted"
1868 + );
1869 + assert!(
1870 + !releases.join(&names[1]).exists(),
1871 + "a dir sharing the pinned name's prefix was treated as pinned"
1872 + );
1873 + }
1874 +
1617 1875 #[tokio::test]
1618 1876 async fn gc_local_releases_noop_when_releases_dir_missing() {
1619 1877 let tmp = tempfile::tempdir().unwrap();
@@ -1649,7 +1907,7 @@
1649 1907 );
1650 1908
1651 1909 let placement = Placement::check(&node, &staged, None).expect("both sides silent");
1652 - let result = deploy_node(&executor, placement, "0.0.1", "server").await;
1910 + let result = deploy_node(&executor, placement, "0.0.1", "server", Some(&no_pins())).await;
1653 1911 let err = result.expect_err("deploy to unreachable host should fail");
1654 1912 let msg = format!("{err:#}");
1655 1913 // Don't pin exact wording, just that the failure is attributed (ssh /
@@ -1694,6 +1952,7 @@
1694 1952 Placement::check(&node, &staged, None).unwrap(),
1695 1953 "0.0.1",
1696 1954 "server",
1955 + Some(&no_pins()),
1697 1956 )
1698 1957 .await
1699 1958 .unwrap();
@@ -2168,6 +2427,7 @@
2168 2427 Placement::check(&node, &staged, None).unwrap(),
2169 2428 "0.0.1",
2170 2429 "server",
2430 + Some(&no_pins()),
2171 2431 )
2172 2432 .await
2173 2433 .unwrap_err();
@@ -2307,6 +2567,7 @@
2307 2567 Placement::check(&node, &staged, None).unwrap(),
2308 2568 "0.9.0",
2309 2569 "makenotwork",
2570 + Some(&no_pins()),
2310 2571 )
2311 2572 .await
2312 2573 .expect("deploy_remote should succeed against the fake");
@@ -2342,6 +2603,7 @@
2342 2603 Placement::check(&node, &staged, None).unwrap(),
2343 2604 "0.9.0",
2344 2605 "makenotwork",
2606 + Some(&no_pins()),
2345 2607 )
2346 2608 .await
2347 2609 .expect_err("rsync failure must fail the deploy");
@@ -2372,6 +2634,7 @@
2372 2634 Placement::check(&node, &staged, None).unwrap(),
2373 2635 "0.9.0",
2374 2636 "makenotwork",
2637 + Some(&no_pins()),
2375 2638 )
2376 2639 .await
2377 2640 .expect_err("arch mismatch must fail the deploy");
@@ -2401,6 +2664,7 @@
2401 2664 Placement::check(&node, &staged, None).unwrap(),
2402 2665 "0.9.0",
2403 2666 "makenotwork",
2667 + Some(&no_pins()),
2404 2668 )
2405 2669 .await
2406 2670 .unwrap();
@@ -2432,6 +2696,7 @@
2432 2696 Placement::check(&node, &staged, None).unwrap(),
2433 2697 "0.9.0",
2434 2698 "makenotwork",
2699 + Some(&no_pins()),
2435 2700 )
2436 2701 .await
2437 2702 .unwrap();
@@ -2474,6 +2739,7 @@
2474 2739 Placement::check(&node, &staged, None).unwrap(),
2475 2740 "0.9.0",
2476 2741 "makenotwork",
2742 + Some(&no_pins()),
2477 2743 )
2478 2744 .await
2479 2745 .expect_err("a bad companion must fail the deploy");
@@ -2528,6 +2794,7 @@
2528 2794 Placement::check(&node, &staged, None).unwrap(),
2529 2795 "0.9.0",
2530 2796 "makenotwork",
2797 + Some(&no_pins()),
2531 2798 )
2532 2799 .await
2533 2800 .unwrap();
@@ -64,6 +64,18 @@
64 64 pub fn is_empty(&self) -> bool {
65 65 self.0.is_empty()
66 66 }
67 +
68 + /// The pinned names in a stable order.
69 + ///
70 + /// Sorted rather than in hash order because the remote gc embeds these in a
71 + /// shell script: an unordered set would rewrite the script text on every
72 + /// deploy with no change of meaning, and a script that differs run to run is
73 + /// one nobody can diff against the last one that worked.
74 + pub fn sorted_names(&self) -> Vec<&str> {
75 + let mut names: Vec<&str> = self.0.iter().map(String::as_str).collect();
76 + names.sort_unstable();
77 + names
78 + }
67 79 }
68 80
69 81 impl FromIterator<String> for PinnedReleases {
@@ -467,6 +467,16 @@
467 467 let bundles =
468 468 promotion::bundles_for_nodes(&s, &previous_str, &target_nodes, &staged_dir, None, None)
469 469 .await?;
470 + // The node gc's pinned set, read before the first node is touched. An
471 + // operator-driven rollback has not started shipping bytes yet, so failing
472 + // here is free and matches the host store's rule: never gc without knowing
473 + // what is referenced. The canary rollback inside a promote takes the softer
474 + // line (skip the gc, roll back anyway) because by then a tier is already
475 + // failing.
476 + let pinned = crate::retention::pinned_dirs(&s.pool, &s.cfg.id)
477 + .await
478 + .map_err(crate::error::Error::Other)?;
479 +
470 480 for (i, (node, node_bundle, node_bundle_platform, _)) in bundles.iter().enumerate() {
471 481 let executor = s
472 482 .executors
@@ -481,6 +491,7 @@
481 491 placement,
482 492 &previous_str,
483 493 s.cfg.primary_bin(),
494 + Some(&pinned),
484 495 )
485 496 .await
486 497 {
@@ -240,6 +240,23 @@
240 240 .map_err(crate::error::Error::Db)?
241 241 .flatten();
242 242
243 + // Which release dirs the node gc must not touch. Read once, before the
244 + // loop: it is the same set for every node (each mirrors the host's
245 + // directory name) and reading it per node would let it change mid-rollout.
246 + //
247 + // Read BEFORE the first deploy on purpose. `tier_state` still holds the
248 + // pre-promote current/previous here, which is exactly the pair a canary
249 + // rollback needs to find on the node. The build being shipped now protects
250 + // itself: it is the newest directory by mtime the moment it lands, so the
251 + // count covers it until `tier_state` catches up.
252 + //
253 + // A failure to read it fails the promote, matching the host store's rule
254 + // (`build::publish` propagates the same error rather than gc'ing blind).
255 + // Nothing has been deployed at this point, so failing here is free.
256 + let pinned = crate::retention::pinned_dirs(&s.pool, &s.cfg.id)
257 + .await
258 + .map_err(crate::error::Error::Other)?;
259 +
243 260 // 3. Deploy to each node. Sequential canary is the only policy
244 261 // implemented in v0; parallel is a one-line change once we trust the
245 262 // sequential path. Track the nodes already flipped to the new version so
@@ -294,6 +311,7 @@
294 311 placement,
295 312 &version_str,
296 313 s.cfg.primary_bin(),
314 + Some(&pinned),
297 315 )
298 316 .await;
299 317 let finished = chrono::Utc::now().to_rfc3339();
@@ -865,6 +883,20 @@
865 883 }
866 884 };
867 885
886 + // The node gc's pinned set. Unlike the promote path this cannot fail the
887 + // operation: a rollback is what saves a tier that is already failing, and
888 + // refusing to run it because a SELECT failed would be the worse outcome by
889 + // far. So a read failure degrades to `None`, which skips the node gc and
890 + // deploys anyway.
891 + let pinned = match crate::retention::pinned_dirs(&s.pool, &s.cfg.id).await {
892 + Ok(p) => Some(p),
893 + Err(e) => {
894 + tracing::warn!(tier = %tier, error = %e,
895 + "canary rollback: could not read the pinned set; rolling back with the node gc skipped");
896 + None
897 + }
898 + };
899 +
868 900 let mut report = RollbackReport::default();
869 901 for (node, node_bundle, node_bundle_platform, _) in &bundles {
870 902 let executor = s
@@ -895,6 +927,7 @@
895 927 placement,
896 928 prev_version,
897 929 s.cfg.primary_bin(),
930 + pinned.as_ref(),
898 931 )
899 932 .await
900 933 {