Skip to main content

max / makenotwork

Stop reporting a safe rollback as a production incident Found by the 0.11.3 deploy on 2026-08-01. The promote to prod failed its pre-swap config check; the canary rollback then failed the same way, and Sando logged "canary rollback FAILED for node; it remains on the new version -- manual intervention needed". That was false. The rollback failed BEFORE the symlink swap, which by its own contract leaves current intact, so the node was still on the old version and needed nothing. The message sends an operator to do surgery on a healthy production box, which is the worst direction for a deploy tool to be wrong in. The stage was already known and thrown away: every pre-swap step wrapped its error with the words "current symlink left intact", where only a human could read it. It is now a FailureStage attached as anyhow context and recovered with stage_of, so the reporting layer can act on it. rollback_deployed_nodes returns a RollbackReport instead of a count, because "the rollback failed" and "the node is on the new version" are different claims and the caller has to tell them apart. Three outcomes: restored, already on the previous version, indeterminate. A tier is consistent when nothing is indeterminate -- including when every rollback "failed" before its swap. That also fixes the summary line, where restored=0 of=1 read as total failure when the accurate reading was that nothing needed restoring. An unannotated error is indeterminate, never assumed safe. Guessing the other way would reintroduce this bug pointing at the dangerous side. Also, the cause of that deploy failure: the node's env file had been rewritten 0600, and the pre-swap check is the only thing that reads it as the deploy user (systemd loads EnvironmentFile= as root), so the running service was fine while the next deploy was already broken. It surfaced as a bare "Permission denied" from inside a generated script. A readability probe now runs first and names the user, the mode and the owner.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-01 16:56 UTC
Signed with PGP, not checked
Commit: ef1bc433e44e06f1b80d6e675451635942c37bd7
Parent: 8d03885
3 files changed, +422 insertions, -57 deletions
M sando/daemon/src/deploy.rs +220 -9
@@ -192,6 +192,55 @@
192 192 .await
193 193 }
194 194
195 + /// Where a node deploy failed, relative to the symlink swap.
196 + ///
197 + /// The distinction is the whole difference between "nothing happened" and "go
198 + /// look at production now", and it used to be carried only in the wording of a
199 + /// `.context()` string, which meant the reporting layer could not act on it. It
200 + /// reported every rollback failure as though the node were stranded on the new
201 + /// version — including the case where the node had never left the old one,
202 + /// which is the safe case and the common one.
203 + ///
204 + /// Attached as `anyhow` context, so it both reads correctly in the error chain
205 + /// and can be recovered with `stage_of`.
206 + #[derive(Debug, Clone, Copy, PartialEq, Eq)]
207 + pub enum FailureStage {
208 + /// Failed before the swap ran. `current` still points at the old release
209 + /// and the service was never restarted, so the node is on the OLD version.
210 + /// Nothing is stranded and nothing needs doing.
211 + BeforeSwap,
212 + /// Failed at or after the swap. The node's version is not knowable from
213 + /// here: the swap script rolls `current` back if the restart fails, but a
214 + /// failure between the two, or in a companion after the server is already
215 + /// live, can leave the node on either version.
216 + AtOrAfterSwap,
217 + }
218 +
219 + impl std::fmt::Display for FailureStage {
220 + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
221 + match self {
222 + Self::BeforeSwap => {
223 + f.write_str("current symlink left intact; node is on the previous version")
224 + }
225 + Self::AtOrAfterSwap => {
226 + f.write_str("the symlink swap had already run; node version is indeterminate")
227 + }
228 + }
229 + }
230 + }
231 +
232 + /// Recover the [`FailureStage`] from a deploy error's context chain.
233 + ///
234 + /// `None` means the error predates the stage annotation or came from somewhere
235 + /// that does not set one. Callers must treat that as indeterminate rather than
236 + /// as safe: guessing "before the swap" would reintroduce the bug in the
237 + /// opposite, worse direction.
238 + pub fn stage_of(err: &anyhow::Error) -> Option<FailureStage> {
239 + // anyhow's own downcast_ref searches attached context values, not just the
240 + // source chain, which is where a `.context(FailureStage::…)` lands.
241 + err.downcast_ref::<FailureStage>().copied()
242 + }
243 +
195 244 async fn reset_local_current(
196 245 executor: &dyn Executor,
197 246 release_root: &Path,
@@ -230,7 +279,8 @@
230 279 &format!("set -e; mkdir -p {q}", q = sh_quote(&release_dir)),
231 280 "creating remote release dir",
232 281 )
233 - .await?;
282 + .await
283 + .context(FailureStage::BeforeSwap)?;
234 284
235 285 tracing::info!(node = %node.name, version, primary = %primary_bin, "deploy: rsync release dir");
236 286 // Rsync the whole staged dir (binaries + every release_contents entry).
@@ -247,7 +297,8 @@
247 297 &SyncOpts::release_mirror(),
248 298 )
249 299 .await
250 - .context("rsync failed (current symlink left intact)")?;
300 + .context("rsync failed")
301 + .context(FailureStage::BeforeSwap)?;
251 302
252 303 // Verify the bundle on the node against its own MANIFEST before the swap
253 304 // (invariant 3, wiki [[release-artifact-identity]]). The MANIFEST shipped in
@@ -263,7 +314,8 @@
263 314 "verifying bundle digest on node",
264 315 )
265 316 .await
266 - .context("node-side bundle verification failed (current symlink left intact)")?;
317 + .context("node-side bundle verification failed")
318 + .context(FailureStage::BeforeSwap)?;
267 319
268 320 // Fail closed on a wrong-architecture binary before the symlink swap. The
269 321 // "never cross-compile" rule is enforced at build time (build_host check),
@@ -278,9 +330,8 @@
278 330 "verifying binary arch matches node",
279 331 )
280 332 .await
281 - .context(
282 - "deployed binary architecture does not match the target node (current symlink left intact)",
283 - )?;
333 + .context("deployed binary architecture does not match the target node")
334 + .context(FailureStage::BeforeSwap)?;
284 335
285 336 // Config-drift guard (opt-in per node). Runs the freshly-rsynced binary in
286 337 // config-only mode with the node's env sourced, BEFORE the swap, so a
@@ -292,7 +343,8 @@
292 343 tracing::info!(node = %node.name, version, "deploy: pre-swap config check");
293 344 check_target_config(executor, &deployed_bin, env_file)
294 345 .await
295 - .context("pre-swap config check failed (current symlink left intact)")?;
346 + .context("pre-swap config check failed")
347 + .context(FailureStage::BeforeSwap)?;
296 348 }
297 349
298 350 tracing::info!(node = %node.name, version, "deploy: symlink swap + service reload");
@@ -306,7 +358,8 @@
306 358 &swap_and_restart,
307 359 "symlink swap + systemctl reload-or-restart",
308 360 )
309 - .await?;
361 + .await
362 + .context(FailureStage::AtOrAfterSwap)?;
310 363
311 364 // Companion services (opt-in per node): install each from the just-rsynced
312 365 // bundle and restart its unit via the node-side wrapper, AFTER the server is
@@ -327,7 +380,8 @@
327 380 "companion {} deploy failed (server already swapped)",
328 381 c.name
329 382 )
330 - })?;
383 + })
384 + .context(FailureStage::AtOrAfterSwap)?;
331 385 }
332 386
333 387 if let Err(e) = gc_remote_releases(executor, release_root).await {
@@ -374,6 +428,34 @@
374 428 deployed_bin: &str,
375 429 env_file: &str,
376 430 ) -> Result<()> {
431 + // Readability first, as its own step with its own message.
432 + //
433 + // The env file is read by this check AS THE DEPLOY USER, and it is the only
434 + // thing that does. systemd loads `EnvironmentFile=` as root before dropping
435 + // to `User=`, so the running service does not care about the mode — which
436 + // means a file rewritten 0600 breaks the next deploy while the current one
437 + // keeps serving, and the breakage is invisible until someone ships. That is
438 + // exactly how prod deploy 0.11.3 failed on 2026-08-01.
439 + //
440 + // Without this step the operator gets `bash: line 9: <file>: Permission
441 + // denied` out of a generated script and has to reverse-engineer which user
442 + // and which file. Naming the user, the mode and the owner turns that into a
443 + // one-line read.
444 + let probe = readability_probe_script(env_file);
445 + if let Ok(Err(e)) = tokio::time::timeout(
446 + std::time::Duration::from_secs(20),
447 + run_checked(executor, &probe, "env file readability"),
448 + )
449 + .await
450 + {
451 + return Err(e).context(format!(
452 + "the deploy user cannot read {env_file}. systemd reads EnvironmentFile= as root, so \
453 + the running service is unaffected and this breaks only deploys. Expected mode 0640 \
454 + owned root:<service user> (see sando/deploy/bootstrap-node.sh); something that \
455 + rewrote the file likely did so with a 077 umask"
456 + ));
457 + }
458 +
377 459 let script = config_check_script(env_file, deployed_bin);
378 460 let fut = run_checked(executor, &script, "pre-swap config check");
379 461 match tokio::time::timeout(std::time::Duration::from_secs(20), fut).await {
@@ -385,6 +467,27 @@
385 467 }
386 468 }
387 469
470 + /// Assert the deploy user can read `env_file`, reporting who it is and what the
471 + /// file actually looks like when it cannot.
472 + ///
473 + /// `stat` output is best-effort: a node without it (or a file that does not
474 + /// exist) still gets the identity line, which is the half an operator cannot
475 + /// derive from the failure on their own.
476 + fn readability_probe_script(env_file: &str) -> String {
477 + format!(
478 + "if [ ! -e {env} ]; then\n\
479 + \techo \"{env_disp}: does not exist on this node\" >&2; exit 1\n\
480 + fi\n\
481 + if [ ! -r {env} ]; then\n\
482 + \techo \"cannot read {env_disp} as $(id -un) (groups: $(id -Gn))\" >&2\n\
483 + \tstat -c 'actual: mode %a owner %U:%G' {env} >&2 2>/dev/null || true\n\
484 + \texit 1\n\
485 + fi\n",
486 + env = sh_quote(env_file),
487 + env_disp = env_file,
488 + )
489 + }
490 +
388 491 /// Shell that loads `env_file` with systemd `EnvironmentFile=` semantics, then
389 492 /// runs `bin` under `MNW_CHECK_CONFIG=1`.
390 493 ///
@@ -544,6 +647,114 @@
544 647 use std::sync::{Arc, Mutex as StdMutex};
545 648 use std::time::SystemTime;
546 649
650 + // ---- failure stage ----
651 + //
652 + // The 2026-08-01 prod deploy failed its pre-swap config check, and the
653 + // rollback then failed the same way — which left the node safely on the old
654 + // version, and was reported as "it remains on the new version, manual
655 + // intervention needed". These pin the distinction the reporting layer now
656 + // depends on.
657 +
658 + #[test]
659 + fn a_pre_swap_failure_is_recoverable_as_such() {
660 + let e = anyhow::anyhow!("Permission denied")
661 + .context("pre-swap config check failed")
662 + .context(FailureStage::BeforeSwap);
663 + assert_eq!(stage_of(&e), Some(FailureStage::BeforeSwap));
664 + // The reason survives alongside the stage; the stage does not replace it.
665 + let rendered = format!("{e:#}");
666 + assert!(
667 + rendered.contains("pre-swap config check failed"),
668 + "{rendered}"
669 + );
670 + assert!(rendered.contains("Permission denied"), "{rendered}");
671 + }
672 +
673 + #[test]
674 + fn a_post_swap_failure_is_recoverable_as_such() {
675 + let e = anyhow::anyhow!("unit failed to start")
676 + .context("companion x deploy failed (server already swapped)")
677 + .context(FailureStage::AtOrAfterSwap);
678 + assert_eq!(stage_of(&e), Some(FailureStage::AtOrAfterSwap));
679 + }
680 +
681 + #[test]
682 + fn an_unannotated_failure_has_no_stage() {
683 + // Must be None, not a default. A caller seeing None has to treat the
684 + // node as indeterminate; inferring "before the swap" would reintroduce
685 + // the original bug pointing the other way, which is the dangerous way.
686 + let e = anyhow::anyhow!("something older, from before stages existed");
687 + assert_eq!(stage_of(&e), None);
688 + }
689 +
690 + // ---- env file readability probe ----
691 +
692 + #[tokio::test]
693 + async fn readability_probe_passes_on_a_readable_file() {
694 + let tmp = tempfile::tempdir().unwrap();
695 + let f = tmp.path().join("ok.env");
696 + tokio::fs::write(&f, "A=1\n").await.unwrap();
697 + let script = readability_probe_script(&f.to_string_lossy());
698 + let out = run_checked(&local_executor(), &script, "probe").await;
699 + assert!(out.is_ok(), "{:?}", out.err().map(|e| format!("{e:#}")));
700 + }
701 +
702 + #[tokio::test]
703 + async fn readability_probe_names_the_user_and_mode_when_unreadable() {
704 + // Root can read anything, so a mode-based test would pass spuriously
705 + // there. Skip rather than assert something false. No libc dependency
706 + // for one probe: a 0-mode temp file is readable iff we are root.
707 + let probe_dir = tempfile::tempdir().unwrap();
708 + let probe_file = probe_dir.path().join("root-check");
709 + tokio::fs::write(&probe_file, "x").await.unwrap();
710 + tokio::fs::set_permissions(
711 + &probe_file,
712 + std::os::unix::fs::PermissionsExt::from_mode(0o000),
713 + )
714 + .await
715 + .unwrap();
716 + if tokio::fs::read(&probe_file).await.is_ok() {
717 + return; // running as root
718 + }
719 + let tmp = tempfile::tempdir().unwrap();
720 + let f = tmp.path().join("locked.env");
721 + tokio::fs::write(&f, "A=1\n").await.unwrap();
722 + tokio::fs::set_permissions(&f, std::os::unix::fs::PermissionsExt::from_mode(0o000))
723 + .await
724 + .unwrap();
725 +
726 + let script = readability_probe_script(&f.to_string_lossy());
727 + let err = run_checked(&local_executor(), &script, "probe")
728 + .await
729 + .expect_err("an unreadable file must fail the probe");
730 + let msg = format!("{err:#}");
731 + // The two things the raw bash error does not tell you.
732 + assert!(msg.contains("cannot read"), "{msg}");
733 + assert!(msg.contains("mode 0") || msg.contains("mode "), "{msg}");
734 + }
735 +
736 + #[tokio::test]
737 + async fn readability_probe_distinguishes_missing_from_unreadable() {
738 + let tmp = tempfile::tempdir().unwrap();
739 + let missing = tmp.path().join("nope.env");
740 + let script = readability_probe_script(&missing.to_string_lossy());
741 + let err = run_checked(&local_executor(), &script, "probe")
742 + .await
743 + .expect_err("a missing file must fail the probe");
744 + let msg = format!("{err:#}");
745 + assert!(msg.contains("does not exist"), "{msg}");
746 + }
747 +
748 + #[test]
749 + fn the_two_stages_read_differently() {
750 + // These strings end up in an operator's terminal during an incident.
751 + let before = FailureStage::BeforeSwap.to_string();
752 + let after = FailureStage::AtOrAfterSwap.to_string();
753 + assert!(before.contains("previous version"), "{before}");
754 + assert!(after.contains("indeterminate"), "{after}");
755 + assert_ne!(before, after);
756 + }
757 +
547 758 /// A LocalExec granted the default node capabilities (deploy + restart).
548 759 fn local_executor() -> LocalExec {
549 760 LocalExec::new(CapabilitySet::from_tokens(
M sando/daemon/src/routes/mod.rs +62 -5
@@ -871,7 +871,7 @@
871 871
872 872 #[cfg(test)]
873 873 mod tests {
874 - use super::promotion::{rollback_deployed_nodes, unsatisfied_gates};
874 + use super::promotion::{RollbackReport, rollback_deployed_nodes, unsatisfied_gates};
875 875 use super::*;
876 876 use crate::config::Config;
877 877 use crate::topology::{BackupConfig, CanaryPolicy, Gate, Node, RepoConfig, Tier, Topology};
@@ -1776,8 +1776,12 @@
1776 1776 state.executors = std::sync::Arc::new(execs);
1777 1777
1778 1778 let refs: Vec<&Node> = nodes.iter().collect();
1779 - let restored = rollback_deployed_nodes(&state, &tid("a"), &refs, "1.0.0").await;
1780 - assert_eq!(restored, 2, "both deployed nodes should be restored");
1779 + let report = rollback_deployed_nodes(&state, &tid("a"), &refs, "1.0.0").await;
1780 + assert_eq!(report.restored, 2, "both deployed nodes should be restored");
1781 + assert!(
1782 + report.is_consistent(),
1783 + "nothing should be indeterminate: {report:?}"
1784 + );
1781 1785
1782 1786 for n in &nodes {
1783 1787 let cur = tokio::fs::read_link(std::path::Path::new(&n.release_root).join("current"))
@@ -1810,11 +1814,64 @@
1810 1814 companions: Vec::new(),
1811 1815 };
1812 1816 let state = test_state().await; // no versions row for "9.9.9"
1813 - let restored = rollback_deployed_nodes(&state, &tid("a"), &[&node], "9.9.9").await;
1817 + let report = rollback_deployed_nodes(&state, &tid("a"), &[&node], "9.9.9").await;
1814 1818 assert_eq!(
1815 - restored, 0,
1819 + report.restored, 0,
1816 1820 "no artifact to roll back to -> nothing restored, no panic"
1817 1821 );
1822 + assert_eq!(
1823 + report.touched(),
1824 + 1,
1825 + "the node must be accounted for somewhere"
1826 + );
1827 + // No rollback could even be attempted, so the node's version is not
1828 + // knowable here. That must read as indeterminate, not as safe.
1829 + assert_eq!(report.indeterminate, 1);
1830 + assert!(!report.is_consistent());
1831 + }
1832 +
1833 + /// The 2026-08-01 prod incident, as a unit test on the reporting shape.
1834 + ///
1835 + /// A rollback that fails before the symlink swap leaves the node on the
1836 + /// version it was already running — the one being rolled back to. The old
1837 + /// code counted that as "not restored" and reported it as `restored=0 of=1`
1838 + /// plus "it remains on the new version — manual intervention needed", which
1839 + /// sent an operator to inspect a production box that was entirely fine.
1840 + #[test]
1841 + fn a_rollback_that_failed_before_the_swap_is_not_an_incident() {
1842 + let report = RollbackReport {
1843 + restored: 0,
1844 + already_on_previous: 1,
1845 + indeterminate: 0,
1846 + };
1847 + assert_eq!(report.touched(), 1);
1848 + assert!(
1849 + report.is_consistent(),
1850 + "a node that never left the previous version is not split-brain"
1851 + );
1852 +
1853 + // Contrast: the same zero restored, but the swap had run. This one does
1854 + // warrant a human, and the two must not report the same way.
1855 + let real = RollbackReport {
1856 + restored: 0,
1857 + already_on_previous: 0,
1858 + indeterminate: 1,
1859 + };
1860 + assert_eq!(real.touched(), 1);
1861 + assert!(!real.is_consistent());
1862 + }
1863 +
1864 + /// A genuine split-brain still reports as one: some nodes back on the old
1865 + /// version, one stranded.
1866 + #[test]
1867 + fn a_mixed_outcome_is_inconsistent_if_any_node_is_unknown() {
1868 + let report = RollbackReport {
1869 + restored: 2,
1870 + already_on_previous: 1,
1871 + indeterminate: 1,
1872 + };
1873 + assert_eq!(report.touched(), 4);
1874 + assert!(!report.is_consistent());
1818 1875 }
1819 1876
1820 1877 // ---- FleetFake: a multi-node promote across recorded fake executors ----
M sando/daemon/src/routes/promotion.rs +140 -43
@@ -270,13 +270,34 @@
270 270 let touched = deployed.len();
271 271 match prev_version.as_deref() {
272 272 Some(prev) => {
273 - let restored = rollback_deployed_nodes(&s, &target.name, &deployed, prev).await;
274 - tracing::warn!(
275 - tier = %target.name, restored, of = touched,
276 - from = %version, to = prev,
277 - "canary failed mid-rollout; rolled touched nodes back to the previous version",
278 - );
279 - if restored > 0
273 + let report = rollback_deployed_nodes(&s, &target.name, &deployed, prev).await;
274 + // Say what is true, and say it differently when nothing is
275 + // wrong. `restored=0 of=1` read as total failure when the
276 + // accurate reading was "0 needed restoring" — the same
277 + // confusion as the per-node message, one level up.
278 + if report.is_consistent() {
279 + tracing::warn!(
280 + tier = %target.name,
281 + restored = report.restored,
282 + already_on_previous = report.already_on_previous,
283 + of = report.touched(),
284 + from = %version, to = prev,
285 + "canary failed mid-rollout; every touched node is on the previous \
286 + version and the tier is consistent",
287 + );
288 + } else {
289 + tracing::error!(
290 + tier = %target.name,
291 + restored = report.restored,
292 + already_on_previous = report.already_on_previous,
293 + indeterminate = report.indeterminate,
294 + of = report.touched(),
295 + from = %version, to = prev,
296 + "canary failed mid-rollout and the tier is NOT consistent; some nodes \
297 + have an indeterminate version",
298 + );
299 + }
300 + if report.restored > 0
280 301 && let Ok(prev_v) = crate::domain::Version::parse(prev)
281 302 {
282 303 crate::events::emit(
@@ -288,16 +309,22 @@
288 309 },
289 310 );
290 311 }
291 - // If every touched node was restored, the tier is consistent on
292 - // `prev` — clear any stale flag. Otherwise it is genuinely
293 - // split-brain: record exactly how, for /state.
294 - if restored == touched {
312 + // The tier is consistent when no node's version is unknown —
313 + // which includes the case where a rollback "failed" before
314 + // the swap and so left the node on `prev` already. Flagging
315 + // that as partial would put a permanent scare on /state for
316 + // a fleet that is entirely on one version.
317 + if report.is_consistent() {
295 318 clear_partial(&s, &target.name).await;
296 319 } else {
297 320 set_partial(&s, &target.name, &format!(
298 - "canary rollback incomplete: {restored}/{touched} nodes restored to {prev}; \
299 - {} may still be on {version} — manual check needed",
300 - touched - restored,
321 + "canary rollback incomplete: {indeterminate} of {touched} node(s) have an \
322 + indeterminate version and may be on {version}; {restored} restored to \
323 + {prev}, {already} were never swapped — manual check needed",
324 + touched = report.touched(),
325 + indeterminate = report.indeterminate,
326 + restored = report.restored,
327 + already = report.already_on_previous,
301 328 )).await;
302 329 }
303 330 }
@@ -472,48 +499,94 @@
472 499 })))
473 500 }
474 501
502 + /// What a canary rollback actually left behind, per node.
503 + ///
504 + /// Three outcomes, not two, and conflating the middle one with the last is the
505 + /// bug this type exists to prevent: a rollback that fails *before* the symlink
506 + /// swap leaves the node exactly where it already was, on the previous version.
507 + /// Reporting that as "stranded on the new version, manual intervention needed"
508 + /// sends an operator to do surgery on a healthy production box.
509 + #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
510 + pub(super) struct RollbackReport {
511 + /// Put back on the previous version by a successful redeploy.
512 + pub(super) restored: usize,
513 + /// Rollback failed before the swap, so the node never left the previous
514 + /// version. Nothing is stranded and nothing needs doing.
515 + pub(super) already_on_previous: usize,
516 + /// Rollback failed at or after the swap, or could not be attempted at all.
517 + /// The node's version is not knowable from here. This is the only outcome
518 + /// that warrants a human.
519 + pub(super) indeterminate: usize,
520 + }
521 +
522 + impl RollbackReport {
523 + /// Every touched node accounted for.
524 + pub(super) fn touched(self) -> usize {
525 + self.restored + self.already_on_previous + self.indeterminate
526 + }
527 +
528 + /// True when no node is in an unknown state, whether or not every rollback
529 + /// "succeeded". A tier whose rollbacks all failed before the swap is
530 + /// consistent on the previous version and is not an incident.
531 + pub(super) fn is_consistent(self) -> bool {
532 + self.indeterminate == 0
533 + }
534 + }
535 +
475 536 /// After a canary node fails mid-promote, restore the nodes already flipped to
476 537 /// the new version back to `prev_version`, leaving the tier consistent (all on
477 538 /// the old version) rather than split-brain. Best-effort: every node is
478 539 /// attempted; a per-node failure is logged but never propagated (the promote is
479 - /// already failing). Returns how many nodes were successfully restored. Returns
480 - /// 0 (with an error log) when the previous version has no recorded artifact to
481 - /// roll back to.
540 + /// already failing).
541 + ///
542 + /// Returns a [`RollbackReport`] rather than a bare count, because "the rollback
543 + /// failed" is not the same claim as "the node is on the new version" and the
544 + /// caller has to be able to tell them apart. When the previous version has no
545 + /// recorded artifact, no rollback can be attempted and every touched node is
546 + /// reported indeterminate.
482 547 pub(super) async fn rollback_deployed_nodes(
483 548 s: &AppState,
484 549 tier: &crate::domain::TierId,
485 550 nodes: &[&crate::topology::Node],
486 551 prev_version: &str,
487 - ) -> usize {
488 - let bin: Option<(String,)> = match sqlx::query_as(
489 - "SELECT artifact_path FROM versions WHERE version = ?",
490 - )
491 - .bind(prev_version)
492 - .fetch_optional(&s.pool)
493 - .await
494 - {
495 - Ok(b) => b,
496 - Err(e) => {
497 - tracing::error!(tier = %tier, prev = prev_version, error = %e,
498 - "canary rollback: looking up the previous artifact failed; nodes left on the new version");
499 - return 0;
500 - }
501 - };
552 + ) -> RollbackReport {
553 + let bin: Option<(String,)> =
554 + match sqlx::query_as("SELECT artifact_path FROM versions WHERE version = ?")
555 + .bind(prev_version)
556 + .fetch_optional(&s.pool)
557 + .await
558 + {
559 + Ok(b) => b,
560 + Err(e) => {
561 + tracing::error!(tier = %tier, prev = prev_version, error = %e,
562 + "canary rollback: looking up the previous artifact failed; no rollback attempted");
563 + return RollbackReport {
564 + indeterminate: nodes.len(),
565 + ..Default::default()
566 + };
567 + }
568 + };
502 569 let Some((bin,)) = bin else {
503 570 tracing::error!(tier = %tier, prev = prev_version, nodes = nodes.len(),
504 - "canary rollback: previous version has no artifact_path; nodes left on the new version");
505 - return 0;
571 + "canary rollback: previous version has no artifact_path; no rollback attempted");
572 + return RollbackReport {
573 + indeterminate: nodes.len(),
574 + ..Default::default()
575 + };
506 576 };
507 577 let Some(staged_dir) = std::path::PathBuf::from(&bin)
508 578 .parent()
509 579 .map(std::path::Path::to_path_buf)
510 580 else {
511 581 tracing::error!(tier = %tier, prev = prev_version,
512 - "canary rollback: previous artifact_path has no parent dir; nodes left on the new version");
513 - return 0;
582 + "canary rollback: previous artifact_path has no parent dir; no rollback attempted");
583 + return RollbackReport {
584 + indeterminate: nodes.len(),
585 + ..Default::default()
586 + };
514 587 };
515 588
516 - let mut restored = 0usize;
589 + let mut report = RollbackReport::default();
517 590 for node in nodes {
518 591 let executor = s
519 592 .executors
@@ -530,17 +603,41 @@
530 603 .await
531 604 {
532 605 Ok(_) => {
533 - restored += 1;
606 + report.restored += 1;
534 607 tracing::warn!(tier = %tier, node = %node.name, version = prev_version,
535 608 "canary rollback: node restored to the previous version");
536 609 }
537 - Err(e) => tracing::error!(
538 - tier = %tier, node = %node.name, version = prev_version, error = %format!("{e:#}"),
539 - "canary rollback FAILED for node; it remains on the new version — manual intervention needed",
540 - ),
610 + // A rollback is itself a deploy, so it fails at a stage too. Failing
611 + // before the swap means it never touched `current` — the node is
612 + // still on the version it was already running, which is the one we
613 + // were rolling back TO. That is the intended end state reached by a
614 + // different route, not an incident.
615 + Err(e) => match crate::deploy::stage_of(&e) {
616 + Some(crate::deploy::FailureStage::BeforeSwap) => {
617 + report.already_on_previous += 1;
618 + tracing::warn!(
619 + tier = %tier, node = %node.name, version = prev_version,
620 + error = %format!("{e:#}"),
621 + "canary rollback did not run, and did not need to: it failed before the \
622 + symlink swap, so the node is already on the previous version",
623 + );
624 + }
625 + // Unannotated errors land here deliberately. Guessing "safe"
626 + // would reintroduce the original bug in the worse direction.
627 + stage => {
628 + report.indeterminate += 1;
629 + tracing::error!(
630 + tier = %tier, node = %node.name, version = prev_version,
631 + error = %format!("{e:#}"),
632 + stage = ?stage,
633 + "canary rollback FAILED for node at or after the symlink swap; its version \
634 + is indeterminate — manual intervention needed",
635 + );
636 + }
637 + },
541 638 }
542 639 }
543 - restored
640 + report
544 641 }
545 642
546 643 /// Flag a tier as left in a partial / mixed-version state, with a human-readable