amadan
host: fix review findings on per-box snapshot keys (#158)
Finding 1: TestTwoBoxesSnapshotWithoutColliding didn't prove the generation prune is scoped per-box — with one generation per box it never crossed snapshotKeep, so a globally-scoped prune mutation stayed green. Extend the test with four more interleaved fingerprint-changing passes per box (aggregate 10 generations, each box's own count 5, under snapshotKeep=8 individually but over it combined) and assert both prefixes still list exactly 5. Verified by temporarily re-collapsing the prune's list call to the global "control/registry-snapshots/" prefix: the extended test fails; reverted and confirmed green again. Finding 2: the OVERWRITING guard's comment still claimed it "makes the moment loud... where a rehearsal will see it" for the same-name standby-restore case. Under per-box keys that's false in every reachable case — readSnapshotMeta reads only this box's own key, whose sole writer stamps meta.Box = box, so prior.Box != box cannot occur, including when a rebuilt standby reuses the dead box's name. Comment now says so plainly and points at the DR runbook's Step 0 ordering rule as the actual owner of that hazard. Comparison logic unchanged, per the reviewer's explicit instruction.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+51
−8
internal/host/snapshot_loop.go+14 −8internal/host/snapshot_loop_test.go+37 −0
diff --git a/internal/host/snapshot_loop.go b/internal/host/snapshot_loop.go| index 247cd541b..e75bb1569 100644 |
| --- a/internal/host/snapshot_loop.go |
| +++ b/internal/host/snapshot_loop.go |
| @@ -105,14 +105,20 @@ func registrySnapshotPass(ctx context.Context, s release.Store, reg *registry.Re |
| if _, err := s.Put(ctx, genKey, body, ""); err != nil && !errors.Is(err, release.ErrCASConflict) { |
| return fmt.Errorf("snapshot: put %s: %w", genKey, err) |
| } |
| - // Overwriting a sidecar another box wrote is the cold-standby clobber |
| - // (runbook step 0): a freshly bootstrapped standby whose agent starts |
| - // before the operator restores will replace the dead box's snapshot with |
| - // its own empty registry, and every self-consistency check downstream |
| - // (bytes, created_at) still passes. Nothing here can safely decide whose |
| - // registry is authoritative — a legitimate promotion looks identical — |
| - // so this does not refuse the write; it makes the moment loud, in the |
| - // agent log, where a rehearsal will see it. |
| + // This guard predates per-box keys and is retained for the shared-key |
| + // era and for hand-placed sidecars only (a sidecar copied or edited by |
| + // an operator with a Box field that does not match this box). Under |
| + // per-box keys it reads ONLY this box's own key, and the only writer |
| + // that ever lands there stamps meta.Box = box — so prior.Box != box |
| + // cannot occur through normal operation, INCLUDING the same-name |
| + // standby-restore case the runbook worries about (a rebuilt standby |
| + // reusing the dead box's machine-id before restoring: same name means |
| + // prior.Box already equals box, so this stays silent). That clobber — |
| + // the freshly bootstrapped standby's agent starting and writing its own |
| + // empty registry over the dead box's last-good snapshot before the |
| + // operator restores it — is NOT detected here; the DR runbook's Step 0 |
| + // ordering rule (restore before the agent is allowed to snapshot) owns |
| + // that hazard instead. |
| if prior := readSnapshotMeta(ctx, s, box); prior != nil && prior.Box != "" && prior.Box != box { |
| log.Printf("agent: registry snapshot: OVERWRITING a snapshot written by box %q (this box is %q, prior created_at %s) — if this box is a standby that has not been restored yet, STOP and preserve the prior snapshot", |
| prior.Box, box, prior.CreatedAt.Format(time.RFC3339)) |
diff --git a/internal/host/snapshot_loop_test.go b/internal/host/snapshot_loop_test.go| index a762be914..cce0fdae7 100644 |
| --- a/internal/host/snapshot_loop_test.go |
| +++ b/internal/host/snapshot_loop_test.go |
| @@ -3,6 +3,7 @@ package host |
| import ( |
| "context" |
| "encoding/json" |
| + "fmt" |
| "path/filepath" |
| "testing" |
| "time" |
| @@ -280,6 +281,42 @@ func TestTwoBoxesSnapshotWithoutColliding(t *testing.T) { |
| if err != nil || len(gens) != 1 { |
| t.Fatalf("annie generations = %v, %v; want exactly 1", gens, err) |
| } |
| + |
| + // Prove the prune is scoped per box, not global. Interleave four more |
| + // fingerprint-changing passes for each box (registry content varies |
| + // each round so the fingerprint gate always writes). After this, each |
| + // box has 5 generations of its own (1 above + 4 here) — under |
| + // snapshotKeep (8), so a correctly per-box-scoped prune leaves both at |
| + // exactly 5. But the AGGREGATE across both boxes is 10, which crosses |
| + // snapshotKeep — so a prune that (wrongly) lists the GLOBAL |
| + // "control/registry-snapshots/" prefix would prune down to 8 total, |
| + // eating into one or both boxes' tails. This is exactly the mutation |
| + // the reviewer flagged: snapshotGenPrefix(box) collapsed back to a |
| + // shared "control/registry-snapshots/" would pass every other test in |
| + // this file (each of which uses only one box) but must fail here. |
| + for i := 0; i < 4; i++ { |
| + at := now.Add(time.Duration(i+1) * time.Minute) |
| + if err := regA.Upsert(registry.Route{Host: fmt.Sprintf("annie-%d.example.com", i), Kind: "service", Addr: "10.0.1.1:3000", Status: "ready"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := registrySnapshotPass(context.Background(), s, regA, "annie", at, &fpA); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := regB.Upsert(registry.Route{Host: fmt.Sprintf("betty-%d.example.com", i), Kind: "service", Addr: "10.0.2.1:3000", Status: "ready"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := registrySnapshotPass(context.Background(), s, regB, "betty", at, &fpB); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + gensA, err := s.List(context.Background(), "control/registry-snapshots/annie/") |
| + if err != nil || len(gensA) != 5 { |
| + t.Fatalf("annie generations after interleaving = %v, %v; want exactly 5 (per-box prune must not see betty's writes)", gensA, err) |
| + } |
| + gensB, err := s.List(context.Background(), "control/registry-snapshots/betty/") |
| + if err != nil || len(gensB) != 5 { |
| + t.Fatalf("betty generations after interleaving = %v, %v; want exactly 5 (per-box prune must not see annie's writes)", gensB, err) |
| + } |
| } |
| func TestSeedSnapshotFingerprintEmptyWhenNoSidecar(t *testing.T) { |