amadan
adopt fix round: checked rollback, structural no-wake, bounded restart flap
The start is now gated on the rollback having HAPPENED, not attempted: PointLive's error was silently discarded, and in the documented link-present-staged-version-gone state the symlink still names the refused build — a start there would launch it. The failed rollback is printed, the summary line stops claiming it, and the caps restore keys on it too. makeStartRoute refuses Hibernate rows and the converged-daemon branch no longer reaches the start at all (its failed restart was carlos-app@'s, not the named unit's) — never-wake is structural again, not argued. The fast sweep bounds the honest flap the fix created: a failed restart holds its host out of the sweep on a growing window (30s doubling to 15m, per host, keyed on the failed version so a recovery promote starts fresh; serial bumps bypass the hold, success clears it). Before the fix a broken canary head self-terminated after two ticks by stamping the lie; after it, unbounded, it was a service cycle every 2 seconds. Revert-checked both ways: gate removed reproduces the 6-cycles-in-6-ticks incident, start removed reproduces the lying stamp. The second-pass test's fake is now stateful (try-restart no-ops on the unit the failed restart left down), so reverting the start fix fails the suite instead of sliding past it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+438
−92
internal/host/agent.go+149 −41internal/host/agent_test.go+135 −6internal/release/adopt.go+91 −40internal/release/adopt_test.go+63 −5
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 1747f5053..eee8ad464 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -915,6 +915,19 @@ const ( |
| fastFullClamp = 5 * time.Second |
| fastBackoffBase = 2 * time.Second |
| fastBackoffCap = 5 * time.Minute |
| + |
| + // The restart-failure hold (noteRestartFailed) starts higher and caps |
| + // higher than the staging backoff above, because what it bounds is not |
| + // a retry's COST to this box but a service CYCLE on a tenant's process: |
| + // re-attempting a version whose restart just failed repoints, performs a |
| + // REAL restart of the running old service, fails, rolls back and starts |
| + // it again — retrying that 2 seconds later is near-certain to do it all |
| + // again. 30s → 15m doubling means a persistently broken head costs a |
| + // handful of honest cycles in its first half hour and then four an hour |
| + // from this loop, with the hourly floors' one-each on top — instead of |
| + // one every other second for as long as the broken head stands. |
| + fastRestartBackoffBase = 30 * time.Second |
| + fastRestartBackoffCap = 15 * time.Minute |
| ) |
| // fastDeps is one fastConvergeLoop's state and seams — a struct so the tick |
| @@ -933,7 +946,19 @@ type fastDeps struct { |
| lastFull time.Time |
| notBefore map[string]time.Time |
| step map[string]time.Duration |
| - serialBroken bool // last ReadSerial failed and was logged; reset on success |
| + restartHold map[string]*restartFailure // hosts held out of the sweep after a failed restart; see noteRestartFailed |
| + serialBroken bool // last ReadSerial failed and was logged; reset on success |
| +} |
| + |
| +// restartFailure is one host's cross-pass memo of a version its restart |
| +// refused: which version failed, the current hold step, and when the sweep |
| +// may try that host again. Kept on the loop (fastDeps), never in |
| +// release.Pass — the pass stays stateless; this loop is the caller whose |
| +// cadence needs the bound. |
| +type restartFailure struct { |
| + ver string |
| + step time.Duration |
| + notBefore time.Time |
| } |
| // fastPassOpts is the option set EVERY fast-converge pass carries — the |
| @@ -1021,6 +1046,19 @@ func (d *fastDeps) fastMembers(now time.Time) []string { |
| if !release.Adoptable(rt) || seen[rt.Host] { |
| continue |
| } |
| + // A host inside its restart-failure hold is outside the sweep |
| + // entirely — canary or unstaged. Sweeping it again at this cadence |
| + // would repoint, cycle the RUNNING old service through a restart |
| + // that is going to fail, roll back and start it again, every 2-3 |
| + // seconds for as long as the broken head stands (before the |
| + // rollback-start fix that flap self-terminated after two ticks — by |
| + // stamping the lie instead). The hourly floors still retry held |
| + // hosts; a console promote's serial bump still converges them |
| + // immediately (fullPass is not membership-gated); and a restart that |
| + // SUCCEEDS clears the hold at once (observeRestart). |
| + if d.restartHeld(rt.Host, now) { |
| + continue |
| + } |
| if isCanaryRoute(rt) { |
| seen[rt.Host] = true |
| members = append(members, rt.Host) |
| @@ -1063,6 +1101,64 @@ func (d *fastDeps) noteUnstaged(host string, now time.Time) { |
| // backoffFor reports host's current backoff step (zero when none recorded). |
| func (d *fastDeps) backoffFor(host string) time.Duration { return d.step[host] } |
| +// noteRestartFailed records that host's restart onto ver just failed, and |
| +// holds the host out of the sweep for a growing window: same version failing |
| +// again doubles the step (capped), a DIFFERENT version starts the hold fresh |
| +// at the base — a new promotion is new work, not a continuation of the old |
| +// failure. Keyed per host with the version recorded, rather than per |
| +// (host, version) pairs accumulating: one live symlink per host means one |
| +// version under attempt at a time, and the map stays bounded by the fleet. |
| +func (d *fastDeps) noteRestartFailed(host, ver string, now time.Time) { |
| + if d.restartHold == nil { |
| + d.restartHold = map[string]*restartFailure{} |
| + } |
| + m := d.restartHold[host] |
| + if m == nil || m.ver != ver { |
| + m = &restartFailure{ver: ver, step: fastRestartBackoffBase} |
| + } else if m.step *= 2; m.step > fastRestartBackoffCap { |
| + m.step = fastRestartBackoffCap |
| + } |
| + m.notBefore = now.Add(m.step) |
| + d.restartHold[host] = m |
| +} |
| + |
| +// restartHeld reports whether host is inside its restart-failure hold. |
| +func (d *fastDeps) restartHeld(host string, now time.Time) bool { |
| + m := d.restartHold[host] |
| + return m != nil && now.Before(m.notBefore) |
| +} |
| + |
| +// observeRestart wraps the restart dispatch so this loop SEES its outcomes — |
| +// the seam the restart-failure hold feeds on. On an error the version under |
| +// attempt is read from the live symlink (adoptHost repoints BEFORE it |
| +// consults the callback, so at callback time the symlink names the version |
| +// being tried; the read failing degrades the memo to host-keyed, which only |
| +// makes the hold slightly coarser). On success the hold is cleared: the host |
| +// is converged, or a recovery promote just landed, and either way there is |
| +// nothing left to hold it out of the sweep for. |
| +// |
| +// The wrapper never SUPPRESSES a restart — suppression inside the callback |
| +// would return nil to adoptHost, which would stamp SetVersions over a |
| +// process nothing cycled: the exact lie this whole mechanism exists to |
| +// prevent. The hold gates sweep MEMBERSHIP (fastMembers) instead, so a held |
| +// host is simply not repointed at this cadence at all. |
| +// |
| +// Single-goroutine by construction, like every other fastDeps map: the |
| +// callback runs inside PassFilter/Pass inside fastTick, on the loop's own |
| +// goroutine. |
| +func (d *fastDeps) observeRestart(restart func(string) error) func(string) error { |
| + return func(host string) error { |
| + err := restart(host) |
| + if err != nil { |
| + ver, _ := release.LiveVersion(d.optDir, host) |
| + d.noteRestartFailed(host, ver, time.Now()) |
| + } else { |
| + delete(d.restartHold, host) |
| + } |
| + return err |
| + } |
| +} |
| + |
| // fastTick is one fast-converge tick — two independent jobs (spec §3-§4): |
| // |
| // 1. Serial check: one ReadSerial. The FIRST successful read only seeds |
| @@ -1176,7 +1272,12 @@ func fastConvergeLoop(reg *registry.Registry, svc *activator.Service) { |
| } |
| dd := dataDir() |
| optDir := release.OptDir() |
| - restart := makeRestartRoute(reg, systemctlTryRestart, systemctlTryRestartUnit, bounceFn(svc)) |
| + // d is built empty first so the restart dispatch can be wrapped in its |
| + // observer (observeRestart): a failed restart holds its host out of the |
| + // sweep instead of the sweep re-cycling the service every tick. The |
| + // remaining fields are filled in right below. |
| + d := &fastDeps{reg: reg, store: store, pub: pub, optDir: optDir} |
| + restart := d.observeRestart(makeRestartRoute(reg, systemctlTryRestart, systemctlTryRestartUnit, bounceFn(svc))) |
| // Built once, used by both pass shapes below — see fastPassOpts for the |
| // DO-NOT list and why the stage requester is per-loop, not per-tick. |
| opts := fastPassOpts(dd, stageRequester(dd), restart, makeStartRoute(reg, systemctlStartUnit)) |
| @@ -1187,40 +1288,41 @@ func fastConvergeLoop(reg *registry.Registry, svc *activator.Service) { |
| restartCache := map[[2]string]string{} |
| restartSeed(ctx, reg, store, dd, restartCache) |
| restartBounce := bounceFn(svc) |
| - d := &fastDeps{ |
| - reg: reg, store: store, pub: pub, optDir: optDir, |
| - restart: restart, passOpts: opts, |
| - // The serial-triggered full pass set, in spec order: mint routes |
| - // (reconcile), then reconcile aliases, then stage them (adopt), then deliver |
| - // their config. Each error is one log line, never fatal — the hourly floors |
| - // stand underneath every leg. |
| - fullPass: func(ctx context.Context) { |
| - if err := reconcileSerialized(ctx, store, reg, provisionTickConfig(os.Getenv)); err != nil { |
| - log.Printf("agent: fast converge reconcile: %v", err) |
| - } |
| - if err := domains.ReconcileAliases(ctx, store, reg, time.Now); err != nil { |
| - log.Printf("agent: fast converge alias pass: %v", err) |
| - } |
| - if err := release.Pass(ctx, reg, store, pub, optDir, restart, opts...); err != nil { |
| - log.Printf("agent: fast converge adopt pass: %v", err) |
| - } |
| - if priv != nil { |
| - if err := syncExecSerialized(ctx, reg, store, priv, dd, svc); err != nil { |
| - log.Printf("agent: fast converge config sync: %v", err) |
| - } |
| - } |
| - }, |
| - // The restart leg: this process's own children bounced here, everything |
| - // unit-shaped relayed to a root pass. Deliberately NOT inside fullPass |
| - // above — see fastTick's comment for both reasons. |
| - restartLeg: func(ctx context.Context) { |
| - routes, err := reg.List() |
| - if err != nil { |
| - log.Printf("agent: restart check: list routes: %v", err) |
| - return |
| + d.restart = restart |
| + d.passOpts = opts |
| + // The serial-triggered full pass set, in spec order: mint routes |
| + // (reconcile), then reconcile aliases, then stage them (adopt), then deliver |
| + // their config. Each error is one log line, never fatal — the hourly floors |
| + // stand underneath every leg. Not membership-gated by the restart hold: |
| + // a serial bump is an explicit console action, bounded by whoever keeps |
| + // making it — and it is what lets a recovery promote through the console |
| + // converge immediately even mid-hold. |
| + d.fullPass = func(ctx context.Context) { |
| + if err := reconcileSerialized(ctx, store, reg, provisionTickConfig(os.Getenv)); err != nil { |
| + log.Printf("agent: fast converge reconcile: %v", err) |
| + } |
| + if err := domains.ReconcileAliases(ctx, store, reg, time.Now); err != nil { |
| + log.Printf("agent: fast converge alias pass: %v", err) |
| + } |
| + if err := release.Pass(ctx, reg, store, pub, optDir, restart, opts...); err != nil { |
| + log.Printf("agent: fast converge adopt pass: %v", err) |
| + } |
| + if priv != nil { |
| + if err := syncExecSerialized(ctx, reg, store, priv, dd, svc); err != nil { |
| + log.Printf("agent: fast converge config sync: %v", err) |
| } |
| - restartRequestLeg(ctx, routes, store, dd, restartBounce, restartCache) |
| - }, |
| + } |
| + } |
| + // The restart leg: this process's own children bounced here, everything |
| + // unit-shaped relayed to a root pass. Deliberately NOT inside fullPass |
| + // above — see fastTick's comment for both reasons. |
| + d.restartLeg = func(ctx context.Context) { |
| + routes, err := reg.List() |
| + if err != nil { |
| + log.Printf("agent: restart check: list routes: %v", err) |
| + return |
| + } |
| + restartRequestLeg(ctx, routes, store, dd, restartBounce, restartCache) |
| } |
| log.Printf("agent: fast converge loop armed (2s + jitter: serial poll + canary/* + unstaged routes)") |
| for { |
| @@ -2097,15 +2199,21 @@ func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), tryR |
| // over. (The converged-daemon rollback is caps.Restore's business, not |
| // this callback's — see release.WithStartUnit.) |
| // |
| -// This cannot wake anything: it fires only after a try-restart of the named |
| -// unit returned non-zero, and try-restart exits 0 on a loaded-but-stopped |
| -// unit — so a deliberately stopped service never produces the error that |
| -// reaches this, and a unit whose name resolves to nothing gives systemctl |
| -// start nothing to bring up either. |
| +// This cannot wake anything, twice over. By ARGUMENT: it fires only after a |
| +// try-restart of the named unit returned non-zero, and try-restart exits 0 |
| +// on a loaded-but-stopped unit — so a deliberately stopped service never |
| +// produces the error that reaches this, and a unit whose name resolves to |
| +// nothing gives systemctl start nothing to bring up either. And by |
| +// STRUCTURE: a Hibernate-flagged route starts nothing here, full stop. That |
| +// guard costs nothing real — provision refuses the addr-with-hibernate |
| +// shape, so no legitimate named-unit route hibernates, and the only |
| +// hibernating+Unit row is the misconfiguration restart.go already documents |
| +// — and it turns "an adopt path cannot wake a sleeper" back into a property |
| +// of the code's shape rather than of an argument about systemd exit codes. |
| func makeStartRoute(reg *registry.Registry, startUnit func(unit string) error) func(host string) error { |
| return func(host string) error { |
| rt, err := reg.Get(host) |
| - if err != nil || rt == nil || rt.Unit == "" { |
| + if err != nil || rt == nil || rt.Hibernate || rt.Unit == "" { |
| return nil |
| } |
| return startUnit(rt.Unit) |
diff --git a/internal/host/agent_test.go b/internal/host/agent_test.go| index 01e5816c8..6c314a9d2 100644 |
| --- a/internal/host/agent_test.go |
| +++ b/internal/host/agent_test.go |
| @@ -848,11 +848,12 @@ func TestAdoptPassRefusesToStampAVersionAWrongUnitDidNotStart(t *testing.T) { |
| } |
| // TestMakeStartRouteStartsOnlyANamedUnit pins the one-arm shape of the |
| -// rollback-start dispatch: only a route that NAMES its unit has anything for |
| -// it to start. An exec route, a unit-backed route with no name, and a host |
| -// the registry cannot answer for all start NOTHING and return nil — starting |
| -// carlos-app@<host> or an activator child from the rollback path could wake |
| -// a sleeper, which no adopt path may do. |
| +// rollback-start dispatch: only an AWAKE route that NAMES its unit has |
| +// anything for it to start. An exec route, a unit-backed route with no name, |
| +// a host the registry cannot answer for, and any Hibernate-flagged row all |
| +// start NOTHING and return nil — starting carlos-app@<host>, an activator |
| +// child, or a sleeping row's unit from the rollback path could wake a |
| +// sleeper, which no adopt path may do. |
| func TestMakeStartRouteStartsOnlyANamedUnit(t *testing.T) { |
| reg := restartTestRegistry(t) |
| if err := reg.Upsert(registry.Route{ |
| @@ -872,9 +873,20 @@ func TestMakeStartRouteStartsOnlyANamedUnit(t *testing.T) { |
| }); err != nil { |
| t.Fatalf("upsert unit.test: %v", err) |
| } |
| + // A hibernating row carrying a unit name is the misconfiguration |
| + // restart.go documents (provision refuses addr-with-hibernate, so no |
| + // legitimate named-unit route sleeps) — and it is exactly the row the |
| + // STRUCTURAL guard exists for: even here, the rollback-start starts |
| + // nothing. |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "sleeper.test", Kind: "instance", App: "a", Status: "ready", |
| + Unit: "sleeper.service", Hibernate: true, |
| + }); err != nil { |
| + t.Fatalf("upsert sleeper.test: %v", err) |
| + } |
| var units []string |
| start := makeStartRoute(reg, func(u string) error { units = append(units, u); return nil }) |
| - for _, h := range []string{"console.test", "exec.test", "unit.test", "ghost.test"} { |
| + for _, h := range []string{"console.test", "exec.test", "unit.test", "ghost.test", "sleeper.test"} { |
| if err := start(h); err != nil { |
| t.Fatalf("start %s: %v", h, err) |
| } |
| @@ -1348,6 +1360,123 @@ func TestFastSweepBouncesAWarmHibernatingCanary(t *testing.T) { |
| } |
| } |
| +// TestRestartHoldGrowsCapsAndResetsOnANewVersion pins the memo arithmetic |
| +// behind the flap bound: same version failing again doubles the hold (to a |
| +// cap), a different version starts fresh — a recovery promote is new work, |
| +// not a continuation of the old failure. |
| +func TestRestartHoldGrowsCapsAndResetsOnANewVersion(t *testing.T) { |
| + d := &fastDeps{} |
| + t0 := time.Now() |
| + d.noteRestartFailed("h", "v2", t0) |
| + if !d.restartHeld("h", t0.Add(fastRestartBackoffBase-time.Second)) { |
| + t.Fatal("first failure must hold for the base step") |
| + } |
| + if d.restartHeld("h", t0.Add(fastRestartBackoffBase+time.Second)) { |
| + t.Fatal("the hold must expire after its step") |
| + } |
| + d.noteRestartFailed("h", "v2", t0) |
| + if !d.restartHeld("h", t0.Add(2*fastRestartBackoffBase-time.Second)) { |
| + t.Fatal("a second failure on the SAME version must double the step") |
| + } |
| + for i := 0; i < 12; i++ { |
| + d.noteRestartFailed("h", "v2", t0) |
| + } |
| + if d.restartHeld("h", t0.Add(fastRestartBackoffCap+time.Second)) { |
| + t.Fatalf("the step must cap at %v", fastRestartBackoffCap) |
| + } |
| + d.noteRestartFailed("h", "v3", t0) |
| + if d.restartHeld("h", t0.Add(fastRestartBackoffBase+time.Second)) { |
| + t.Fatal("a DIFFERENT version must reset the hold to the base step") |
| + } |
| +} |
| + |
| +// TestFastSweepBoundsARestartFlap is the incident this bound exists for: a |
| +// canary head whose restart persistently fails, on a box whose fast loop can |
| +// actually reach the restart. Before the rollback-start fix that flap |
| +// self-terminated after two ticks — by stamping the lie (try-restart exits 0 |
| +// on the unit the failed restart left down). With the fix, each sweep pass |
| +// is a REAL cycle of the running old service: repoint, restart (kills it), |
| +// fail, roll back, start it again. Unbounded, that is a service cycle every |
| +// 2 seconds for as long as the broken head stands. The hold bounds it: the |
| +// host leaves the sweep for a growing window, a post-hold retry is one cycle |
| +// (not one per tick), and a restart that finally succeeds clears the hold at |
| +// once. |
| +func TestFastSweepBoundsARestartFlap(t *testing.T) { |
| + env := newFastFixture(t) |
| + ctx := context.Background() |
| + plat := runtime.GOOS + "-" + runtime.GOARCH |
| + |
| + for _, v := range []string{"old", "new"} { |
| + m := release.Manifest{ |
| + App: "fastapp", Version: v, Kind: "binary", |
| + Artifacts: map[string]release.Artifact{plat: {}}, |
| + } |
| + if err := release.Ship(ctx, env.store, "bab", m, map[string][]byte{plat: []byte("build " + v)}); err != nil { |
| + t.Fatalf("Ship %s: %v", v, err) |
| + } |
| + } |
| + env.addRoute(t, "flap.test", "canary/dev") |
| + env.stageLive(t, "flap.test", "old") |
| + if err := release.Promote(ctx, env.store, env.priv, "bab", "fastapp", "canary/dev", "new"); err != nil { |
| + t.Fatalf("Promote: %v", err) |
| + } |
| + |
| + calls := 0 |
| + fail := true |
| + base := func(string) error { |
| + calls++ |
| + if fail { |
| + return errors.New("unit would not come back") |
| + } |
| + return nil |
| + } |
| + d := env.deps(nil) |
| + restart := d.observeRestart(base) |
| + d.restart = restart |
| + d.passOpts = fastPassOpts(env.dataDir, func(host, ver string, err error) {}, restart, nil) |
| + |
| + // Tick 1: the sweep attempts the head, the restart fails, the pass rolls |
| + // back — and the failure is memo'd. |
| + t0 := time.Now() |
| + fastTick(ctx, d, t0) |
| + if calls != 1 { |
| + t.Fatalf("tick 1: restart calls = %d, want 1", calls) |
| + } |
| + if v, _ := release.LiveVersion(env.optDir, "flap.test"); v != "old" { |
| + t.Fatalf("live after the failed tick = %q, want old (rolled back)", v) |
| + } |
| + |
| + // Ticks inside the hold: the host is outside the sweep, the service is |
| + // NOT cycled again. Without the hold every one of these is a real kill. |
| + for i := 1; i <= 5; i++ { |
| + fastTick(ctx, d, t0.Add(time.Duration(i)*fastTickEvery)) |
| + } |
| + if calls != 1 { |
| + t.Fatalf("held ticks: restart calls = %d, want still 1 — the sweep must not cycle a failing service every tick", calls) |
| + } |
| + |
| + // Past the hold: exactly one more attempt, which fails again and doubles |
| + // the step. |
| + fastTick(ctx, d, t0.Add(fastRestartBackoffBase+2*time.Second)) |
| + if calls != 2 { |
| + t.Fatalf("post-hold tick: restart calls = %d, want 2", calls) |
| + } |
| + |
| + // The head is fixed now (a recovery promote, say): the next allowed |
| + // retry succeeds, adopts, and clears the hold at once. |
| + fail = false |
| + fastTick(ctx, d, t0.Add(3*fastRestartBackoffBase+4*time.Second)) |
| + if calls != 3 { |
| + t.Fatalf("recovery tick: restart calls = %d, want 3", calls) |
| + } |
| + if v, _ := release.LiveVersion(env.optDir, "flap.test"); v != "new" { |
| + t.Fatalf("live after recovery = %q, want new", v) |
| + } |
| + if d.restartHeld("flap.test", t0.Add(24*time.Hour)) { |
| + t.Fatal("a SUCCESSFUL restart must clear the hold") |
| + } |
| +} |
| + |
| // ---- store selection: bucket, dir, or the fleet store door (spec F5) ------- |
| // doorHit is one request the fake door saw, reduced to the parts these tests |
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index 8ce506205..86eabc5ed 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -181,7 +181,10 @@ func OnStageBlocked(fn func(host, ver string, err error)) PassOption { |
| // returns false without starting anything when the host is not awake, and |
| // `systemctl try-restart` is defined as a no-op on a unit that is not |
| // running. Nothing on either path can start a process. A callback that could |
| -// start one belongs on the awake loop only. |
| +// start one belongs on the awake loop only. (WithStartUnit narrows that |
| +// invariant without breaking it: the ONE start this package can now perform |
| +// fires only after this callback RETURNED AN ERROR — and no stopped unit's |
| +// try-restart produces one, so a sleeper cannot arm it. See WithStartUnit.) |
| // |
| // DO NOT pass this from RunAdopt (carlos-adopt.service). That process is not |
| // the activator's parent: the exec children belong to the agent, so its only |
| @@ -208,13 +211,16 @@ func RestartHibernating(restart func(host string) error) PassOption { |
| // header the named-unit arm's returned error exists to prevent, now over a |
| // service that is not running at all. |
| // |
| -// start fires ONLY on that rollback path, and only when there WAS a |
| -// previous version to roll back to: a first adoption whose restart fails |
| -// has nothing restored to start — the symlink still names the refused |
| -// build, and starting it would run that build. A start that also fails |
| -// changes nothing about the refusal (the version stays poisoned and |
| -// unstamped) but is printed, because at that point the box is genuinely |
| -// broken and an operator must not read the rollback line as "recovered". |
| +// start fires ONLY on that rollback path, and only when the rollback |
| +// actually HAPPENED: a first adoption whose restart fails has no previous |
| +// version, and a rollback PointLive itself refused (the old version's |
| +// directory gone — materialize.go's "link present, staged version gone" |
| +// state) rolled nothing — in both the symlink still names the refused |
| +// build, and starting there would run the exact build the failure refused. |
| +// A start that also fails changes nothing about the refusal (the version |
| +// stays poisoned and unstamped) but is printed, because at that point the |
| +// box is most likely serving nothing and an operator must not read the |
| +// rollback line as "recovered". |
| // |
| // This cannot WAKE a hibernating route, and the reasoning is the contract: |
| // the path is reached only after a restart that was ATTEMPTED and returned |
| @@ -224,9 +230,10 @@ func RestartHibernating(restart func(host string) error) PassOption { |
| // would not come back, or the name resolves to nothing (in which case the |
| // start reaches nothing either). The bounce and carlos-app@ arms swallow |
| // their failures and return nil, so a sleeping exec child can never route |
| -// here. The converged-daemon arm (caps.Restart) can, but its rollback's |
| -// caps.Restore owns re-binding what it changed, and the callback below |
| -// starts nothing for such a route anyway: |
| +// here. The converged-daemon arm (caps.Restart) is excluded at the guard |
| +// itself: the restart that failed there was carlos-app@<host>'s, its |
| +// rollback belongs to caps.Restore, and starting the route's NAMED unit |
| +// there would start a unit that pass never touched. |
| // |
| // start must start ONLY a route that NAMES its unit (registry Unit column) |
| // and return nil for everything else — internal/host's makeStartRoute is |
| @@ -270,8 +277,11 @@ func stageBlocked(err error) bool { |
| // hibernate-flagged hosts a restart callback too, because the flag means "may |
| // sleep", not "is asleep" — a warm exec child must not go on serving a binary |
| // its live symlink no longer names. It never makes the pass wake anything; |
| -// the option only accepts a callback that cannot start a process. See its doc |
| -// comment. Everything else above holds unchanged: sleepers still go last, |
| +// the option only accepts a callback that cannot start a process — and the |
| +// start WithStartUnit adds is armed only by that callback returning an error, |
| +// which a sleeper's exit-0 try-restart never does (see WithStartUnit). See |
| +// its doc comment. Everything else above holds unchanged: sleepers still go |
| +// last, |
| // still poison a bad build, still have no health gate of their own. |
| func Pass(ctx context.Context, reg *registry.Registry, s Store, pub ed25519.PublicKey, optDir string, restart func(host string) error, opts ...PassOption) error { |
| return PassFilter(ctx, reg, s, pub, optDir, restart, nil, opts...) |
| @@ -623,7 +633,11 @@ func RouteAccount(rt registry.Route) string { |
| // and record, never wake, never health-gate. A hibernating host WITH that |
| // option set arrives here with a non-nil callback and takes the ordinary |
| // path — safe because the option only accepts a callback that cannot start a |
| -// process, so "never woken" survives the callback being consulted. |
| +// process, so "never woken" survives the callback being consulted. The one |
| +// start this function itself can perform (cfg.startUnit, the restart-failure |
| +// rollback below) does not weaken that: it is armed only by that callback |
| +// RETURNING AN ERROR, which a stopped unit's exit-0 try-restart never |
| +// produces — WithStartUnit's doc carries the full argument. |
| // cfg is the call's PassOptions, already applied: cfg.nag nil (the ordinary |
| // case) means the canary-TTL nag below never fires, never even builds its |
| // string, and cfg.capsOps nil means the capability converge never runs at |
| @@ -821,7 +835,8 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| // guarantees it); the nil check is here anyway, because a nil deref |
| // on this path would panic the root adopt pass for the whole box. |
| var rerr error |
| - if converged && plan.Set != nil && plan.Set.Daemon { |
| + daemonRestart := converged && plan.Set != nil && plan.Set.Daemon |
| + if daemonRestart { |
| rerr = caps.Restart(*cfg.capsOps, plan) |
| } else { |
| rerr = restart(rt.Host) |
| @@ -831,9 +846,23 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| // the host keeps running what it ran. The symlink goes back |
| // FIRST: caps.Restore may start the service, and it must start |
| // on the old binary. |
| + // |
| + // The repoint's error is CHECKED, where it was once discarded: |
| + // "link present, staged version gone" is a documented real state |
| + // (materialize.go — an operator's /opt cleanup, a partially |
| + // restored volume), and in it PointLive refuses and the symlink |
| + // STILL NAMES ver. Everything below that claims a rollback — the |
| + // restore, the summary line, above all the start — must key on |
| + // whether the rollback actually HAPPENED, not on whether one was |
| + // attempted. |
| poisoned[ver] = true |
| + rolledBack := false |
| if cur != "" { |
| - _ = PointLive(optDir, rt.Host, RouteAccount(rt), cur, rt.App) |
| + if perr := PointLive(optDir, rt.Host, RouteAccount(rt), cur, rt.App); perr != nil { |
| + fmt.Printf(" %s: could not roll the symlink back to %s: %v\n", rt.Host, cur, perr) |
| + } else { |
| + rolledBack = true |
| + } |
| } |
| // The rule is one line long: restore the unit shape that matches |
| // whatever the symlink NOW names. |
| @@ -844,9 +873,13 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| // empty snapshot, which it reads as "this route had no generated |
| // drop-ins" and honours by REMOVING the ones this pass wrote, |
| // leaving the live version running with no capability shape at |
| - // all until the hourly grant-drift pass rebuilds it. |
| + // all until the hourly grant-drift pass rebuilds it. A rollback |
| + // that itself FAILED lands in the same place for the other |
| + // reason: the symlink still names ver, and putting the previous |
| + // version's drop-ins under it would be exactly the shape/binary |
| + // mismatch this whole branch exists to prevent. |
| rolled := "" |
| - if converged && cur != "" { |
| + if converged && rolledBack { |
| // Bytes are not a binding: Restore re-runs the socket |
| // sequence itself when the socket half moves, or the MX |
| // stays off :25 after a rollback. |
| @@ -857,33 +890,51 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| rolled = " and restored the previous version's capability drop-ins" |
| } |
| } else if converged { |
| - rolled = "; there is no previous version to roll back to, so its capability drop-ins stand as converged" |
| + if cur == "" { |
| + rolled = "; there is no previous version to roll back to, so its capability drop-ins stand as converged" |
| + } else { |
| + rolled = "; its capability drop-ins stand as converged with the version the symlink still names" |
| + } |
| + } |
| + action := "rolled the symlink back" |
| + if cur != "" && !rolledBack { |
| + action = "could not roll the symlink back" |
| } |
| - fmt.Printf(" %s: update to %s failed: %v — rolled the symlink back%s\n", rt.Host, ver, rerr, rolled) |
| - // Start the service again on the version the symlink NOW names. |
| - // The rollback above put the bytes back; without this the box is |
| - // left with a correct symlink and no process — and because |
| - // try-restart is a no-op on a unit that is not running (which is |
| - // what lets a deliberately-stopped service stay stopped), the |
| - // NEXT pass's restart would exit 0, SetVersions would stamp the |
| - // build that has never run, and the header would claim it over a |
| - // service that is down. Start, not try-restart: the whole premise |
| - // here is that the unit is not running. Gated on cur != "" — with |
| - // no previous version nothing was rolled back, the symlink still |
| - // names ver, and starting it would run the build whose restart |
| - // just failed. AFTER caps.Restore above, deliberately: the drop-in |
| - // shape must match the binary before anything execs it, and on a |
| - // unit Restore's socket sequence already brought up, start is an |
| - // exit-0 no-op. |
| + fmt.Printf(" %s: update to %s failed: %v — %s%s\n", rt.Host, ver, rerr, action, rolled) |
| + // Start the service again on the version the symlink NOW names — |
| + // but ONLY when that is the restored one. The rollback above put |
| + // the bytes back; without this the box is left with a correct |
| + // symlink and no process — and because try-restart is a no-op on |
| + // a unit that is not running (which is what lets a |
| + // deliberately-stopped service stay stopped), the NEXT pass's |
| + // restart would exit 0, SetVersions would stamp the build that |
| + // has never run, and the header would claim it over a service |
| + // that is down. Start, not try-restart: the whole premise here is |
| + // that the unit is not running. |
| + // |
| + // Gated on rolledBack, not cur != "": a rollback that FAILED (the |
| + // old version's directory gone — PointLive refuses) leaves the |
| + // symlink naming ver, and a start there would launch the exact |
| + // build whose restart just failed. And not for a converged daemon |
| + // (daemonRestart): the restart that failed there was |
| + // carlos-app@<host>'s, via caps.Restart; its rollback belongs to |
| + // caps.Restore above, and this callback would start rt.Unit — a |
| + // unit that pass never touched. AFTER caps.Restore, deliberately: |
| + // the drop-in shape must match the binary before anything execs |
| + // it, and on a unit Restore's socket sequence already brought up, |
| + // start is an exit-0 no-op. |
| // |
| // A start that also fails changes nothing about the refusal — the |
| // version stays poisoned and unstamped — but it is printed, |
| - // because at that point the box is genuinely broken and the |
| - // operator needs to know that rather than reading the rollback |
| - // line as "recovered". |
| - if cfg.startUnit != nil && cur != "" { |
| + // because at that point the box is very likely serving nothing |
| + // and the operator must not read the rollback line as |
| + // "recovered". "May": a try-restart that failed without stopping |
| + // anything (a permission-denied systemctl) fails its start the |
| + // same way while the old process still serves — the two errors on |
| + // the line tell them apart. |
| + if cfg.startUnit != nil && rolledBack && !daemonRestart { |
| if serr := cfg.startUnit(rt.Host); serr != nil { |
| - fmt.Printf(" %s: rolled back to %s but could not start it: %v — this host is serving nothing\n", |
| + fmt.Printf(" %s: rolled back to %s but could not start it: %v — this host may be serving nothing\n", |
| rt.Host, cur, serr) |
| } |
| } |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index 998a50032..ae317849b 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -2747,9 +2747,13 @@ func TestPassProductionOffStillRatchets(t *testing.T) { |
| // ---- WithStartUnit: the rollback must not leave the service down ---- |
| // startUnitFixture is the shared setup of the WithStartUnit tests: one |
| -// named-unit-shaped route live on "old" (staged locally, symlink pointed), |
| -// with "new" promoted at the channel head. canary/* so no bake window |
| -// applies. |
| +// plain instance route live on "old" (staged locally, symlink pointed), with |
| +// "new" promoted at the channel head; canary/* so no bake window applies. |
| +// The route carries no Unit column — this package never reads it (the |
| +// named-unit shape lives in internal/host's dispatch). What these tests |
| +// model is the CALLBACK CONTRACT: a restart that fails the way a named |
| +// unit's does (returns the error), and a start capability handed in through |
| +// WithStartUnit. |
| func startUnitFixture(t *testing.T, host string) (context.Context, *registry.Registry, Store, ed25519.PublicKey, string) { |
| t.Helper() |
| s := NewDirStore(t.TempDir()) |
| @@ -2814,8 +2818,21 @@ func TestSecondPassOverAStillBrokenBuildDoesNotStamp(t *testing.T) { |
| host := "console.example.com" |
| ctx, reg, s, pub, opt := startUnitFixture(t, host) |
| - restart := func(string) error { return errors.New("still broken") } |
| - start := func(string) error { return nil } |
| + // The fake is STATEFUL, and the state is the whole defect narrative: a |
| + // failed restart leaves the unit DOWN, and try-restart on a not-running |
| + // unit is an exit-0 NO-OP. With the fix reverted (no start on rollback) |
| + // pass 1 leaves running=false, pass 2's restart returns nil, and |
| + // SetVersions stamps "new" — this test's assertion fires. An |
| + // unconditionally-erroring fake could not fail on that revert. |
| + running := true |
| + restart := func(string) error { |
| + if !running { |
| + return nil // try-restart is a no-op on a stopped unit |
| + } |
| + running = false // the failed restart left it down |
| + return errors.New("still broken") |
| + } |
| + start := func(string) error { running = true; return nil } |
| for i := 0; i < 2; i++ { |
| if err := Pass(ctx, reg, s, pub, opt, restart, WithStartUnit(start)); err != nil { |
| t.Fatalf("pass %d: %v", i, err) |
| @@ -2887,6 +2904,47 @@ func TestSuccessfulRestartNeverConsultsStart(t *testing.T) { |
| } |
| } |
| +// TestFailedRollbackStartsNothing: the start is gated on the rollback |
| +// having HAPPENED, not on a previous version existing. "Link present, |
| +// staged version gone" is a documented real state (materialize.go — an |
| +// operator's /opt cleanup, a partially restored volume): PointLive refuses |
| +// to repoint at a version directory that is not there, the symlink still |
| +// names the refused build, and a start would launch exactly that build. |
| +// The refusal is also no longer silent — the pass says the rollback failed. |
| +func TestFailedRollbackStartsNothing(t *testing.T) { |
| + host := "console.example.com" |
| + ctx, reg, s, pub, opt := startUnitFixture(t, host) |
| + |
| + // The operator's cleanup: old's staged directory is gone, so the |
| + // rollback's PointLive will refuse. The live symlink still dangles at |
| + // old, which is how adoptHost sees cur = "old" and attempts the update. |
| + if err := os.RemoveAll(filepath.Dir(VersionPath(opt, "bab", "old", "console"))); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + restart := func(string) error { return errors.New("Job for console.service failed") } |
| + start := func(h string) error { |
| + t.Fatalf("start consulted for %q after a rollback that DID NOT HAPPEN — the symlink still names the refused build", h) |
| + return nil |
| + } |
| + var out bytes.Buffer |
| + withCapturedStdout(t, &out, func() { |
| + if err := Pass(ctx, reg, s, pub, opt, restart, WithStartUnit(start)); err != nil { |
| + t.Fatalf("Pass: %v", err) |
| + } |
| + }) |
| + if !strings.Contains(out.String(), "could not roll the symlink back") { |
| + t.Fatalf("printed %q, want the failed rollback reported instead of swallowed", out.String()) |
| + } |
| + rt, err := reg.Get(host) |
| + if err != nil || rt == nil { |
| + t.Fatalf("get %s: %+v, %v", host, rt, err) |
| + } |
| + if rt.Version == "new" { |
| + t.Fatal("a build whose restart failed was stamped") |
| + } |
| +} |
| + |
| // TestFirstAdoptionRestartFailureStartsNothing: with no previous version |
| // (cur == "") the symlink could not be rolled back — it still names the |
| // version whose restart just failed — so there is no RESTORED version to |