amadan
release: close three residuals from re-review — unreadable clock also taints, honest Save comment, deterministic monotonic-SetAt test
An existing-but-unreadable clock file (EACCES, EISDIR, an I/O error — not os.ErrNotExist) still fell through LoadFirstSeen untainted, since the taint was only set inside the "read succeeded, parse failed" branch. Now any os.ReadFile failure other than not-exist taints the clock too, so ProductionBake fails toward baking for that case as well. Corrected ProductionBake's taint comment: Save never clears st.tainted in memory: the taint holds for the lifetime of this load, and it's the NEXT LoadFirstSeen (of the file Save rewrote) that comes back clean. TestSetProductionMonotonicSetAtOnRapidFlip now loops three flips and requires every consecutive pair strictly increasing, closing the vacuous pass a single before/after pair had if the two calls happened to straddle a truncated-second boundary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VioXzogWf1Xi4Rswz2B6EK
3 files changed,
+55
−27
internal/release/bake.go+14 −4internal/release/bake_test.go+15 −0internal/release/production_test.go+26 −23
diff --git a/internal/release/bake.go b/internal/release/bake.go| index 76536775e..8b8d0c311 100644 |
| --- a/internal/release/bake.go |
| +++ b/internal/release/bake.go |
| @@ -24,6 +24,7 @@ package release |
| import ( |
| "encoding/json" |
| + "errors" |
| "fmt" |
| "math/rand" |
| "os" |
| @@ -181,6 +182,12 @@ func LoadFirstSeen(path string) *FirstSeen { |
| if err := json.Unmarshal(raw, st); err != nil { |
| st.tainted = true |
| } |
| + } else if !errors.Is(err, os.ErrNotExist) { |
| + // The file EXISTS but this process could not read it (EACCES, |
| + // EISDIR, an I/O error) — a different failure than "no file", and |
| + // one this box cannot tell apart from a bucket object it can't |
| + // trust either. Same taint, same fail-toward-baking direction. |
| + st.tainted = true |
| } |
| if st.Channels == nil { |
| st.Channels = map[string]map[string]time.Time{} |
| @@ -433,10 +440,13 @@ func (st *FirstSeen) ObserveProduction(acct, app string, flag *ProductionFlag, n |
| // baked head to protect. |
| func (st *FirstSeen) ProductionBake(acct, app string, flagErr error, hasBakeableHead bool, holds map[string]time.Duration, now time.Time) bool { |
| if st.tainted { |
| - // This box's own clock file read but did not parse — untrustworthy |
| - // in a way absence never is. Fail toward baking for every held |
| - // head, same direction as flagErr, until the next Save overwrites |
| - // it with something this binary wrote. |
| + // This box's own clock file existed but couldn't be read or didn't |
| + // parse — untrustworthy in a way absence never is. Fail toward |
| + // baking for every held head, same direction as flagErr, for the |
| + // lifetime of THIS load: tainted is set once, in LoadFirstSeen, and |
| + // nothing clears it in memory afterward. Save rewrites the file |
| + // from this struct's current (now-valid, in-memory) content, so the |
| + // NEXT LoadFirstSeen of that path starts clean. |
| return true |
| } |
| if flagErr != nil { |
diff --git a/internal/release/bake_test.go b/internal/release/bake_test.go| index f597123a7..7465509cb 100644 |
| --- a/internal/release/bake_test.go |
| +++ b/internal/release/bake_test.go |
| @@ -591,4 +591,19 @@ func TestCorruptClockFailsTowardBakingNotAwayFromIt(t *testing.T) { |
| if fresh.ProductionBake("bab", "kass", nil, true, DefaultHolds, base) { |
| t.Fatal("a fresh box with no clock file baked by default") |
| } |
| + |
| + // A file that EXISTS but can't be read at all (not "did not parse") is |
| + // a different os.ReadFile failure mode than os.ErrNotExist — a |
| + // directory sitting at the clock path is the easiest way to reproduce |
| + // it portably (EISDIR), and review found the first cut of this fix only |
| + // tainted the "read OK, parse failed" branch, leaving this one to |
| + // silently fail open. |
| + dirPath := filepath.Join(t.TempDir(), "clock.json") |
| + if err := os.Mkdir(dirPath, 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + unreadable := LoadFirstSeen(dirPath) |
| + if !unreadable.ProductionBake("bab", "kass", nil, true, DefaultHolds, base) { |
| + t.Fatal("an unreadable (directory-at-path) clock file did not fail toward baking") |
| + } |
| } |
diff --git a/internal/release/production_test.go b/internal/release/production_test.go| index 50b86c9ab..2ae0866fc 100644 |
| --- a/internal/release/production_test.go |
| +++ b/internal/release/production_test.go |
| @@ -206,32 +206,35 @@ func TestProductionFlagRefusesTamperedPayload(t *testing.T) { |
| // itself writes — for tests that hand-build payloads. |
| func timeNowTestHelper() time.Time { return time.Now().UTC().Truncate(time.Second) } |
| -// Task-4-review fix 5 (Minor, promoted): SetAt is truncated to the second, |
| -// so two flips within the same wall-clock second would otherwise share a |
| -// set_at — and the box-side replay floor (bake.go's NewestSetAt) treats an |
| -// equal set_at as nothing new, silently swallowing the second flip on every |
| -// box that already saw the first. Two back-to-back SetProduction calls (as |
| -// fast as a test loop runs them, i.e. very likely within one truncated |
| -// second) must still produce strictly increasing set_ats. |
| +// Task-4-review fix 5 (Minor, promoted), tightened by re-review: SetAt is |
| +// truncated to the second, so two flips within the same wall-clock second |
| +// would otherwise share a set_at — and the box-side replay floor (bake.go's |
| +// NewestSetAt) treats an equal set_at as nothing new, silently swallowing |
| +// the second flip on every box that already saw the first. A single |
| +// before/after pair is vacuous if the two calls happen to straddle a |
| +// second boundary (it would pass with or without the bump). Looping several |
| +// rapid flips and requiring EVERY consecutive pair to be strictly |
| +// increasing removes that escape: the bump guarantees strict monotonicity |
| +// regardless of whether any given pair lands in the same truncated second |
| +// or crosses one, so this holds deterministically with the fix in place. |
| func TestSetProductionMonotonicSetAtOnRapidFlip(t *testing.T) { |
| s := NewDirStore(t.TempDir()) |
| ctx := context.Background() |
| priv, pub := testKeys(t) |
| - if err := SetProduction(ctx, s, priv, "bab", "kass", true, "paul@keymail.dev"); err != nil { |
| - t.Fatalf("SetProduction on: %v", err) |
| - } |
| - f1, err := GetProduction(ctx, s, pub, "bab", "kass") |
| - if err != nil || f1 == nil { |
| - t.Fatalf("GetProduction after first flip: %v, %v", f1, err) |
| - } |
| - if err := SetProduction(ctx, s, priv, "bab", "kass", false, "paul@keymail.dev"); err != nil { |
| - t.Fatalf("SetProduction off: %v", err) |
| - } |
| - f2, err := GetProduction(ctx, s, pub, "bab", "kass") |
| - if err != nil || f2 == nil { |
| - t.Fatalf("GetProduction after second flip: %v, %v", f2, err) |
| - } |
| - if !f2.SetAt.After(f1.SetAt) { |
| - t.Fatalf("second flip's set_at %v not strictly after first's %v (same-second collision not bumped)", f2.SetAt, f1.SetAt) |
| + |
| + var prev time.Time |
| + for i := 0; i < 3; i++ { |
| + if err := SetProduction(ctx, s, priv, "bab", "kass", i%2 == 0, "paul@keymail.dev"); err != nil { |
| + t.Fatalf("SetProduction flip %d: %v", i, err) |
| + } |
| + f, err := GetProduction(ctx, s, pub, "bab", "kass") |
| + if err != nil || f == nil { |
| + t.Fatalf("GetProduction after flip %d: %v, %v", i, f, err) |
| + } |
| + if i > 0 && !f.SetAt.After(prev) { |
| + t.Fatalf("flip %d's set_at %v not strictly after flip %d's %v (same-second collision not bumped)", |
| + i, f.SetAt, i-1, prev) |
| + } |
| + prev = f.SetAt |
| } |
| } |