amadan
host: the journal carries the promote it belongs to, so recovery answers the right one
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QCUCpfxbCyEAVQ2m51dH5
4 files changed,
+69
−10
internal/host/systemapply.go+18 −8internal/host/systemapply_test.go+30 −0internal/host/systemstate.go+14 −0internal/host/systemstate_test.go+7 −2
diff --git a/internal/host/systemapply.go b/internal/host/systemapply.go| index dc4757ec5..0be0eff27 100644 |
| --- a/internal/host/systemapply.go |
| +++ b/internal/host/systemapply.go |
| @@ -171,6 +171,11 @@ func (a *systemApplier) file(name string) string { return filepath.Join(a.rootDi |
| // caller should exit non-zero on (an apply that rolled back, a rollback |
| // that failed, a broken box); refusals and no-ops are recorded in |
| // result.json and return nil. |
| +// |
| +// A run whose RECOVERY rolled the box back exits 0, unlike a rollback |
| +// inside install: the result and the heartbeat carry the truth, the |
| +// journal is closed, and re-firing OnFailure= over a finished recovery |
| +// would only start it again with nothing left to do. |
| func (a *systemApplier) Run(ctx context.Context, recover bool) error { |
| if err := os.MkdirAll(a.rootDir(), 0o755); err != nil { |
| return err |
| @@ -272,13 +277,16 @@ func (a *systemApplier) install(ctx context.Context, req systemRequest, p *relea |
| if err != nil { |
| return a.record(req, fmt.Errorf("%w: candidate bootstrap: %v", errSystemRefused, err)) |
| } |
| - // Only the unit set is kept: a rollback re-runs this bootstrap, so that |
| - // what it puts back is derived from the binary it is putting back. |
| + // Only the unit set is taken from the return: a rollback picks this |
| + // staging up as it stands, plan.json and all, rather than exec'ing the |
| + // previous binary again on a box that is already unhealthy. |
| _, prevUnits, err := a.stagePlan(ctx, systemPrevPath(a.optDir), prevDir) |
| if err != nil { |
| return a.record(req, fmt.Errorf("%w: carlos.prev bootstrap: %v", errSystemRefused, err)) |
| } |
| - j := systemJournal{Phase: systemPhaseInstalling, Target: target, PrevBuild: a.prevBuild(ctx), |
| + // PromotedAt rides along for the whole transaction: it is half of the |
| + // tuple a recovery has to answer for. |
| + j := systemJournal{Phase: systemPhaseInstalling, Target: target, PromotedAt: req.PromotedAt, PrevBuild: a.prevBuild(ctx), |
| UnitsAdded: unitsMinus(newUnits, prevUnits), UnitsRemoved: unitsMinus(prevUnits, newUnits), StartedAt: a.ops.now().UTC()} |
| if err := a.journal(j); err != nil { |
| return err |
| @@ -735,12 +743,14 @@ func (a *systemApplier) recoverIfNeeded(ctx context.Context) (bool, error) { |
| if err != nil || j == nil { |
| return false, err |
| } |
| - // The result belongs to the apply being recovered, not to whatever the |
| - // agent has asked for since. The request is used only when it is that |
| - // same apply — then it carries the real promoted_at. |
| + // The result belongs to the promote being recovered, which the journal |
| + // names in full. The request is used only when it is that same promote, |
| + // version and promoted_at both: a re-promote of the same version is a |
| + // new tuple, and recording this rollback against it would refuse the |
| + // operator's retry before it started. |
| req, _ := a.readRequest() |
| - if req.Version != j.Target { |
| - req = systemRequest{Version: j.Target, PromotedAt: j.StartedAt} |
| + if req.Version != j.Target || !req.PromotedAt.Equal(j.promoted()) { |
| + req = systemRequest{Version: j.Target, PromotedAt: j.promoted()} |
| } |
| a.ops.logf("system apply: recovering an apply of %s interrupted at %s", j.Target, j.Phase) |
| if j.Phase == systemPhaseBackingUp { |
diff --git a/internal/host/systemapply_test.go b/internal/host/systemapply_test.go| index c27f30492..698de007d 100644 |
| --- a/internal/host/systemapply_test.go |
| +++ b/internal/host/systemapply_test.go |
| @@ -876,6 +876,36 @@ func TestRecoveryAnswersTheJournalsVersionNotALaterRequest(t *testing.T) { |
| } |
| } |
| +func TestRecoveryAnswersTheInterruptedPromoteNotTheRePromote(t *testing.T) { |
| + // v2 was interrupted; the operator re-promoted the same v2. The |
| + // recovery's result must name the promote it recovered, or the retry |
| + // arrives at a box that has already recorded it as rolled back. |
| + f, first := applyHappy(t) |
| + os.WriteFile(systemPrevPath(f.a.optDir), []byte("#!/bin/sh\necho carlos v1\n"), 0o755) |
| + writeJSONDurable(filepath.Join(systemRootDir(f.a.optDir), "journal.json"), |
| + systemJournal{Phase: systemPhaseUnits, Target: "v2", PromotedAt: first.PromotedAt, PrevBuild: "v1", StartedAt: f.now}, 0o600) |
| + time.Sleep(1100 * time.Millisecond) // promoted_at has second resolution |
| + again := f.promote("v2", []byte("#!/bin/sh\necho carlos v2\n"), false) |
| + if again.PromotedAt.Equal(first.PromotedAt) { |
| + t.Fatal("fixture: the re-promote must move promoted_at") |
| + } |
| + if err := f.a.Run(context.Background(), true); err != nil { |
| + t.Fatal(err) |
| + } |
| + r := f.result() |
| + if r == nil || r.Outcome != systemOutcomeRolledBack || !r.matches("stable", "v2", first.PromotedAt) { |
| + t.Fatalf("result = %+v, want the promote the journal names (%s)", r, first.PromotedAt) |
| + } |
| + // And the re-promote is a tuple of its own: the terminal-tuple guard |
| + // does not refuse it. |
| + if err := f.a.Run(context.Background(), false); err != nil { |
| + t.Fatal(err) |
| + } |
| + if r := f.result(); r == nil || r.Outcome != systemOutcomeApplied || !r.matches("stable", "v2", again.PromotedAt) { |
| + t.Fatalf("result = %+v, the operator's re-promote must be applied", r) |
| + } |
| +} |
| + |
| func TestRecoveryFromGatedRollsBackWhenTheBinaryIsNotTheGatedOne(t *testing.T) { |
| // The journal says v2 passed its gate, but v1 is on disk: whatever put |
| // it there, this is a pre-gate state and going forward would record an |
diff --git a/internal/host/systemstate.go b/internal/host/systemstate.go| index 9f4548e3d..4b565ed78 100644 |
| --- a/internal/host/systemstate.go |
| +++ b/internal/host/systemstate.go |
| @@ -92,15 +92,29 @@ type systemSeen map[string]time.Time |
| // systemJournal is the open transaction. Written before every box-changing |
| // step, removed last; a run that finds one recovers before anything else. |
| +// Target and PromotedAt are the promote it belongs to: the same tuple the |
| +// result is keyed by, so a recovery answers for the apply that was cut off |
| +// and not for whatever the agent has asked for since. |
| type systemJournal struct { |
| Phase string `json:"phase"` |
| Target string `json:"target"` |
| + PromotedAt time.Time `json:"promoted_at"` |
| PrevBuild string `json:"prev_build"` |
| UnitsAdded []string `json:"units_added,omitempty"` |
| UnitsRemoved []string `json:"units_removed,omitempty"` |
| StartedAt time.Time `json:"started_at"` |
| } |
| +// promoted is the promote the journal is an apply of. A journal written by |
| +// a build from before the field existed has only StartedAt to offer, which |
| +// is what recovery keyed results by then. |
| +func (j *systemJournal) promoted() time.Time { |
| + if j.PromotedAt.IsZero() { |
| + return j.StartedAt |
| + } |
| + return j.PromotedAt |
| +} |
| + |
| var systemVersionRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`) |
| // validSystemVersion is what makes the cache path derivable: no leading dot, |
diff --git a/internal/host/systemstate_test.go b/internal/host/systemstate_test.go| index b7ea13993..2ad07b42c 100644 |
| --- a/internal/host/systemstate_test.go |
| +++ b/internal/host/systemstate_test.go |
| @@ -107,14 +107,19 @@ func TestSystemStateReadersRoundTrip(t *testing.T) { |
| if !seen2["stable"].Equal(at) { |
| t.Fatalf("seen = %v", seen2) |
| } |
| - j := systemJournal{Phase: systemPhaseInstalled, Target: "v1", PrevBuild: "v0", UnitsAdded: []string{"a.path"}, StartedAt: at} |
| + j := systemJournal{Phase: systemPhaseInstalled, Target: "v1", PromotedAt: at.Add(-time.Hour), PrevBuild: "v0", UnitsAdded: []string{"a.path"}, StartedAt: at} |
| if err := writeJSONDurable(filepath.Join(root, "journal.json"), j, 0o600); err != nil { |
| t.Fatal(err) |
| } |
| j2, err := readSystemJournal(opt) |
| - if err != nil || j2.Phase != systemPhaseInstalled || j2.UnitsAdded[0] != "a.path" { |
| + if err != nil || j2.Phase != systemPhaseInstalled || j2.UnitsAdded[0] != "a.path" || !j2.promoted().Equal(at.Add(-time.Hour)) { |
| t.Fatalf("journal = %+v err=%v", j2, err) |
| } |
| + // A journal from before the field existed still names its apply. |
| + old := &systemJournal{Phase: systemPhaseInstalled, Target: "v1", StartedAt: at} |
| + if !old.promoted().Equal(at) { |
| + t.Fatalf("promoted = %v, want the started_at a journal without promoted_at has", old.promoted()) |
| + } |
| applied := systemApplied{Version: "v1", Channel: "stable", At: at} |
| if err := writeJSONDurable(filepath.Join(root, "applied.json"), applied, 0o644); err != nil { |
| t.Fatal(err) |