amadan
release: review fixes — a narrowed pass may not repair a tainted clock, and an unreadable flag says so
Five items from the task-5 review. IMPORTANT 1: a failed GetProduction was completely silent. It re-imposes the full window on every held head of the app, and the likeliest cause is not tampering: on a GetObject-without-ListBucket fleet — the same IAM shape the missing-pointer branch above already handles — a GET of an ABSENT key answers 403, not 404, so the spec's default-OFF silently inverts to production fleet-wide with no diagnostics at all. Now logged per app, in the function's existing die-free-but-log style. IMPORTANT 2: the taint-forced Save could be executed by the canary fast loop, which reads no production flags. It would have overwritten the corrupt clock with EMPTY production memory and cleared the taint that was failing closed, before anything re-observed the flags; the hourly pass would then load clean-but-empty, and with the bucket flag also deleted the latch would be gone. A narrowed pass (extra != nil) now never Saves a tainted clock — not the forced Save and not an ordinary changed Save. A pass that fetched no flags cannot repair what it would overwrite, and the canary observations it skips re-occur within seconds. The residual is written down where the code is: even the full pass only re-observes flags for apps with a currently resolved held head, so an app whose pointer is simultaneously unreadable keeps its latch out of the repaired file until the pointer heals — triple tamper, inside the box-file boundary LoadFirstSeen already accepts. Both are pinned by TestNarrowedPassNeverRepairsATaintedClock, and both new tests were mutation-checked: reverting the guard makes it fail with the file rewritten to channels-only and no production key at all, which is exactly the loss it exists to prevent. Minor: TestPassProductionOffStillRatchets pins the dangerous direction end to end on the adopt path (on, held, off, STILL held) and kills the hasBakeableHead-constant-false mutant, which the package previously survived. The err == nil guard's comment no longer claims to be load-bearing — it is the second lock on a door ObserveProduction's nil-check already holds. Dropped a dead setup Pass in TestCanaryFastLoopPaysNoProductionRead whose "not vacuous" comment had drifted onto the wrong statement, and adoptHost's ignore doc now covers the declared-off-and-past-the-ratchet case.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VioXzogWf1Xi4Rswz2B6EK
2 files changed,
+153
−20
internal/release/adopt.go+52 −14internal/release/adopt_test.go+101 −6
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index 8a9e3a652..8816459ba 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -224,6 +224,18 @@ func PassFilter(ctx context.Context, reg *registry.Registry, s Store, pub ed2551 |
| // be able to shorten a window by breaking the object. |
| f, err := GetProduction(ctx, s, pub, ak.acct, ak.app) |
| prod[ak] = prodState{flag: f, err: err} |
| + if err != nil { |
| + // SAY SO. This failure re-imposes the full window on every held |
| + // head of the app, and the most likely cause is not tampering at |
| + // all: on a GetObject-without-ListBucket fleet (the same IAM shape |
| + // the missing-pointer branch above already handles) a GET of an |
| + // ABSENT key answers 403, not 404 — so the spec's default-OFF |
| + // silently inverts to production for every app with no flag. A |
| + // 72h hold nobody can see the reason for is the worst version of |
| + // this feature; log it like every other per-app problem here and |
| + // carry on. |
| + fmt.Printf(" %s/%s: production flag unreadable (%v) — treating as production, holds apply\n", ak.acct, ak.app, err) |
| + } |
| } |
| // Tick the bake clock: record every channel head this box hasn't seen at |
| @@ -240,14 +252,7 @@ func PassFilter(ctx context.Context, reg *registry.Registry, s Store, pub ed2551 |
| // for days on a fleet that happens to see no new channel head, and the |
| // legacy file would look safe to delete long before it actually is (see |
| // LoadFirstSeenMigrating and BakeDir's doc comments, 2026-08-03 review). |
| - // |
| - // bake.tainted forces it for a different reason: the clock file existed |
| - // but did not read or parse, so every held head on this box is failing |
| - // toward baking until that file is replaced (ProductionBake). Without |
| - // this the corrupt file would survive every changed-gated pass and the |
| - // box would bake forever; with it, one pass rewrites the file from this |
| - // struct's valid in-memory content and the NEXT load starts clean. |
| - changed := bake.migrated || bake.tainted |
| + changed := bake.migrated |
| for k, ptr := range pointers { |
| if ptr != nil && bake.Observe(k.acct, k.app, k.channel, ptr.Version, now) { |
| changed = true |
| @@ -258,14 +263,46 @@ func PassFilter(ctx context.Context, reg *registry.Registry, s Store, pub ed2551 |
| // That is what makes prodMemory's "a mixed-version rollout costs at most |
| // one pass of this memory" true — a binary that wiped the latch re-latches |
| // it from the bucket and honors it in the SAME pass, never a pass later. |
| - // Only err == nil reaches the memory: it must only ever tick for signed |
| - // documents (a failed read is a verdict input, never an observation). |
| + // |
| + // The err == nil guard is the second lock on a door ObserveProduction's |
| + // own nil-check already holds (GetProduction returns a nil flag on every |
| + // error path, and a nil flag is a no-op there). It stays because the rule |
| + // it states — the memory only ever ticks for VERIFIED documents — is |
| + // worth being true at the call site too, not only at the callee. |
| for ak, ps := range prod { |
| if ps.err == nil && bake.ObserveProduction(ak.acct, ak.app, ps.flag, now) { |
| changed = true |
| } |
| } |
| - if changed { |
| + // bake.tainted forces a Save for a different reason than migrated: the |
| + // clock file existed but did not read or parse, so every held head on |
| + // this box is failing toward baking until that file is replaced |
| + // (ProductionBake). Left alone, the corrupt file would survive every |
| + // changed-gated pass and the box would bake forever. |
| + // |
| + // But ONLY a full pass may perform that repair. A NARROWED pass |
| + // (extra != nil — the canary fast loop, every 2-3 seconds) resolves only |
| + // channels with no hold, so it fetched no production flags at all: its |
| + // in-memory Production map is EMPTY, not repaired. Letting it write would |
| + // overwrite the corrupt file with that emptiness AND clear the taint that |
| + // is currently failing closed, all before anything re-observed the flags |
| + // — and if the bucket flag had also been deleted, the latch would be gone |
| + // for good. So a tainted clock on a narrowed pass skips the Save entirely, |
| + // forced or ordinary: a pass that fetched no flags cannot repair what it |
| + // would overwrite. Nothing is lost — the canary observations it would |
| + // have recorded re-occur within seconds, and the hourly pass does the |
| + // repair with the flags actually in hand. |
| + // |
| + // Residual, accepted: even the full pass only re-observes flags for apps |
| + // with a currently-resolved HELD head, so an app whose channel pointer is |
| + // simultaneously unreadable keeps its latch out of the repaired file |
| + // until that pointer heals. That is triple-tamper territory (corrupt |
| + // clock file AND unreadable pointer AND deleted flag) and sits inside the |
| + // box-file trust boundary LoadFirstSeen already documents as accepted. |
| + switch { |
| + case bake.tainted && extra != nil: |
| + // skip: see above. |
| + case changed || bake.tainted: |
| if err := bake.Save(); err != nil { |
| return fmt.Errorf("record bake clock: %v", err) |
| } |
| @@ -360,9 +397,10 @@ func RouteAccount(rt registry.Route) string { |
| // string; see NagExpiredCanaries for who sets it and why. |
| // |
| // ignore is the app-level bake verdict from ProductionBake — the production |
| -// flag's doing, never a publisher field. True means this app is not declared |
| -// production on this box, so the supply-chain window does not apply to it; |
| -// the caller computes it once per route from the flag it read this pass. |
| +// flag's doing, never a publisher field. True means the supply-chain window |
| +// does not CURRENTLY apply to this app on this box (not declared production, |
| +// or declared off and past the ratchet); the caller computes it once per |
| +// route from the flag it read this pass. |
| func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir string, bake *FirstSeen, poisoned map[string]bool, rt registry.Route, ptr *ChannelPointer, restart func(host string) error, now time.Time, nag io.Writer, ignore bool) { |
| if ptr == nil { |
| return |
diff --git a/internal/release/adopt_test.go b/internal/release/adopt_test.go| index a0bccc390..962ab16f3 100644 |
| --- a/internal/release/adopt_test.go |
| +++ b/internal/release/adopt_test.go |
| @@ -901,12 +901,8 @@ func TestCanaryFastLoopPaysNoProductionRead(t *testing.T) { |
| t.Fatalf("canary-only pass read the production flag %d times, want 0 — the fast loop must not pay bucket reads for a window canary/* does not have", s.n) |
| } |
| - // Not vacuous: the full pass, which does resolve a held head, reads it — |
| - // once, for two channels and three routes. |
| - if err := Pass(ctx, reg, dir, pub, opt, func(string) error { return nil }); err != nil { |
| - t.Fatalf("Pass: %v", err) |
| - } |
| - s.n = 0 |
| + // Not vacuous: the full pass, which DOES resolve a held head, reads it — |
| + // once, for two canary channels, a stable one and three routes. |
| if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| t.Fatalf("Pass: %v", err) |
| } |
| @@ -914,3 +910,102 @@ func TestCanaryFastLoopPaysNoProductionRead(t *testing.T) { |
| t.Fatalf("full pass read the production flag %d times, want exactly 1 per (account, app)", s.n) |
| } |
| } |
| + |
| +// A NARROWED pass (the canary fast loop) must never repair a tainted clock: |
| +// it fetched no production flags, so its Production map is empty, and writing |
| +// it would replace the corrupt file with that emptiness AND clear the taint |
| +// that is currently failing closed — before anything re-observed the flags. |
| +// The full pass, which holds the flags, does the repair. |
| +func TestNarrowedPassNeverRepairsATaintedClock(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + opt := t.TempDir() |
| + reg := testRegistry(t) |
| + |
| + shipRelease(t, s, "bab", "myapp", "v1", adoptPlatform, []byte("bin")) |
| + for _, ch := range []string{"canary/x", "stable"} { |
| + if err := Promote(ctx, s, priv, "bab", "myapp", ch, "v1"); err != nil { |
| + t.Fatalf("Promote %s: %v", ch, err) |
| + } |
| + } |
| + if err := SetProduction(ctx, s, priv, "bab", "myapp", true, "test"); err != nil { |
| + t.Fatal(err) |
| + } |
| + addRoute(t, reg, "cx.example.com", "myapp", "canary/x", false) |
| + addRoute(t, reg, "s.example.com", "myapp", "stable", false) |
| + |
| + if err := os.WriteFile(FirstSeenPath(opt), []byte("{not json"), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if err := PassFilter(ctx, reg, s, pub, opt, func(string) error { return nil }, isCanaryTestRoute); err != nil { |
| + t.Fatalf("PassFilter: %v", err) |
| + } |
| + if raw, err := os.ReadFile(FirstSeenPath(opt)); err != nil { |
| + t.Fatal(err) |
| + } else if json.Valid(raw) { |
| + t.Fatalf("the canary fast loop overwrote a tainted clock — a pass that fetched no flags cannot repair what it would overwrite; file is now %q", raw) |
| + } |
| + |
| + // The hourly pass has the flags in hand, so it repairs — and the app's |
| + // production state survives, re-latched from the bucket in that same pass. |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatalf("Pass: %v", err) |
| + } |
| + st := LoadFirstSeen(FirstSeenPath(opt)) |
| + if st.tainted { |
| + t.Fatalf("the full pass did not repair the tainted clock") |
| + } |
| + if mem := st.Production[prodKey("bab", "myapp")]; mem == nil || !mem.WasProduction { |
| + t.Fatalf("production memory = %+v, want the latch re-established by the repairing pass", mem) |
| + } |
| +} |
| + |
| +// The off-ratchet, end to end on the adopt path: flipping production OFF does |
| +// not release a held head immediately — the off is honored only after the |
| +// stable window has elapsed on this box's own sighting of it (spec §4). This |
| +// is the dangerous direction, so it is pinned here and not only in bake_test: |
| +// it is also what fails if ProductionBake's hasBakeableHead carve-out is ever |
| +// passed a constant false from this call site. |
| +func TestPassProductionOffStillRatchets(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + opt := t.TempDir() |
| + reg := testRegistry(t) |
| + |
| + shipRelease(t, s, "bab", "myapp", "v1", adoptPlatform, []byte("bin")) |
| + if err := Promote(ctx, s, priv, "bab", "myapp", "stable", "v1"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := SetProduction(ctx, s, priv, "bab", "myapp", true, "test"); err != nil { |
| + t.Fatal(err) |
| + } |
| + addRoute(t, reg, "h.example.com", "myapp", "", false) |
| + |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatal(err) |
| + } |
| + if v, _ := LiveVersion(opt, "h.example.com"); v != "" { |
| + t.Fatalf("live = %q, want held (declared production)", v) |
| + } |
| + |
| + // Flip it off. An operator who can flip the flag must not thereby have a |
| + // same-minute bypass of the window their held head is sitting in. |
| + if err := SetProduction(ctx, s, priv, "bab", "myapp", false, "test"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Pass(ctx, reg, s, pub, opt, func(string) error { return nil }); err != nil { |
| + t.Fatal(err) |
| + } |
| + if v, _ := LiveVersion(opt, "h.example.com"); v != "" { |
| + t.Fatalf("live = %q, want STILL held — production: false must ratchet through the stable window, never release a held head on sight", v) |
| + } |
| + // And the box recorded the off-sighting, which is what the ratchet is |
| + // measured from — its own clock, never the flag's timestamp. |
| + mem := LoadFirstSeen(FirstSeenPath(opt)).Production[prodKey("bab", "myapp")] |
| + if mem == nil || mem.OffFirstSeen.IsZero() || !mem.WasProduction { |
| + t.Fatalf("production memory = %+v, want a latched app with an off-sighting recorded", mem) |
| + } |
| +} |