amadan

carlos / platform Public

release: bake ratchet review fixes — zero-bake pinning, HoldsForApp baselines, changed contract

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 7faa473cd3f424e273850b334b5243d894a963dc parent ad40a59
2 files changed, +132 −11
  • internal/release/bake.go +40 −11
  • internal/release/bake_test.go +92 −0
diff --git a/internal/release/bake.go b/internal/release/bake.go
index e871c7f26..24a44f1db 100644
--- a/internal/release/bake.go
+++ b/internal/release/bake.go
@@ -583,6 +583,16 @@ func (f *FirstSeen) checkHoldPromotion(mem *holdMemory, now time.Time) bool {
// A channel whose BakeWindow() fails to parse is skipped: old memory is
// kept (fail closed) and this never prevents any other channel in the same
// pipeline from being observed.
+//
+// Never call this with a pipeline that failed verification — like Observe
+// and ObserveProduction, memory only ever ticks for a signed object. p ==
+// nil must mean genuinely, verifiably absent (the object deleted); a read
+// or verify FAILURE is a different thing and must fail toward the longest
+// window instead, never be handed to this function as nil — passing a
+// corrupt read as nil would let it cheaply masquerade as deletion and
+// inherit the calm "never loosens from absence" path above instead of
+// failing closed. (Enforcing that at the call site is Task 5's job; this
+// function's contract is documented here so that call site gets it right.)
func (f *FirstSeen) ObservePipeline(acct, app string, p *Pipeline, now time.Time) bool {
if f.Holds == nil {
f.Holds = map[string]*holdMemory{}
@@ -617,7 +627,17 @@ func (f *FirstSeen) ObservePipeline(acct, app string, p *Pipeline, now time.Time
mem.Window = w
mem.Pending = 0
mem.PendingAt = time.Time{}
- } else if mem.Pending != w {
+ } else if mem.PendingAt.IsZero() || mem.Pending != w {
+ // PendingAt.IsZero() must stand on its own here, not
+ // just mem.Pending != w: a declared window of exactly
+ // ZERO is a legal value, and Pending's own zero value
+ // is indistinguishable from "nothing pending" — without
+ // this, a fresh channel whose first-ever declared bake
+ // is 0 would never stamp PendingAt (mem.Pending, freshly
+ // zeroed, already equals w=0), so checkHoldPromotion's
+ // PendingAt.IsZero() guard would short-circuit forever
+ // and the channel would sit pinned at its fail-closed
+ // baseline for the life of the box.
mem.Pending = w
mem.PendingAt = now
}
@@ -631,24 +651,33 @@ func (f *FirstSeen) ObservePipeline(acct, app string, p *Pipeline, now time.Time
}
// HoldsForApp returns the ENFORCED per-channel bake window map for a
-// pipelined app — one entry per channel this box has actually latched
-// memory for via ObservePipeline (a channel p declares but this box has
-// never observed simply has no entry yet). nil when p == nil, so the
-// caller falls back to DefaultHolds entirely, the pre-pipeline behaviour
-// for an app with no pipeline at all. Channels absent from the returned
-// map fall to HoldFor's 72h fail-closed default when the caller passes
-// this map to HoldFor — which lands on exactly the same baseline
-// ObservePipeline would have latched for that channel on its first
-// sighting, so the two paths agree.
+// pipelined app: one entry per channel p declares, using this box's
+// ratchet memory where it has any and HoldFor(DefaultHolds, name) — the
+// exact same fail-closed baseline ObservePipeline would latch on that
+// channel's first sighting — for a channel not yet observed. The two
+// paths therefore always agree, including for a rung name like "edge"
+// whose baseline is 0: synthesizing it here (rather than leaving the
+// channel absent from the map) matters precisely because HoldFor's OWN
+// unknown-channel fallback is a hardcoded 72h, which would silently
+// contradict a 0 baseline if this function ever left it out. nil when p
+// == nil, so the caller falls back to DefaultHolds entirely — the
+// pre-pipeline behaviour for an app with no pipeline at all.
+//
+// Never call this with a pipeline that failed verification, same
+// prohibition as ObservePipeline: p == nil must mean genuinely absent, not
+// "read or verify failed" — a verify failure must fail toward the longest
+// window at the call site instead (Task 5), never reach here as nil.
func (f *FirstSeen) HoldsForApp(acct, app string, p *Pipeline) map[string]time.Duration {
if p == nil {
return nil
}
out := map[string]time.Duration{}
for _, ch := range p.Channels {
+ w := HoldFor(DefaultHolds, ch.Name)
if mem := f.Holds[channelClockKey(acct, app, ch.Name)]; mem != nil {
- out[ch.Name] = mem.Window
+ w = mem.Window
}
+ out[ch.Name] = w
}
return out
}
diff --git a/internal/release/bake_test.go b/internal/release/bake_test.go
index 9ef5e203a..ed65b0af1 100644
--- a/internal/release/bake_test.go
+++ b/internal/release/bake_test.go
@@ -683,3 +683,95 @@ func TestHoldMemorySurvivesSaveLoadAndOldBinaryDrop(t *testing.T) {
t.Fatalf("legacy clock parse = %v, holds %v", err, legacy.Holds)
}
}
+
+// TestHoldMemoryZeroBakeRatchetsIn is the review fix for the critical bug:
+// Pending's zero value is indistinguishable from "nothing pending", and a
+// declared bake of exactly ZERO is a legal value — without PendingAt as
+// its own sentinel, a brand-new channel with no bake declared would latch
+// its 72h fail-closed baseline and NEVER ratchet down, because
+// mem.Pending (freshly zeroed) already equals w=0 on the very first call.
+func TestHoldMemoryZeroBakeRatchetsIn(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ now := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC)
+ pipe := func(setAt time.Time) *Pipeline {
+ return &Pipeline{Schema: 1, Channels: []PipelineChannel{
+ {Name: "dev"}, {Name: "live"}}, SetAt: setAt}
+ }
+ f.ObservePipeline("bae", "web", pipe(now), now)
+ if h := f.HoldsForApp("bae", "web", pipe(now)); h["dev"] != 72*time.Hour || h["live"] != 72*time.Hour {
+ t.Fatalf("first-sighting windows = %v; want fail-closed 72h baseline on both channels", h)
+ }
+ later := now.Add(72*time.Hour + time.Second)
+ if !f.ObservePipeline("bae", "web", pipe(now), later) {
+ t.Fatal("re-observe past the elapsed baseline did not report changed")
+ }
+ h := f.HoldsForApp("bae", "web", pipe(now))
+ if h["dev"] != 0 || h["live"] != 0 {
+ t.Fatalf("post-ratchet windows = %v; want 0 (declared zero bake) on both channels", h)
+ }
+}
+
+// TestHoldsForAppBeforeObserveReturnsBaselines is the review fix for
+// HoldsForApp's fallback-agreement claim: before any ObservePipeline call,
+// every declared channel must still report the exact baseline
+// ObservePipeline would have latched on first sighting — including a rung
+// name like "edge" whose baseline is 0, which HoldFor's own unknown-channel
+// fallback (72h) would silently get wrong if left out of the map.
+func TestHoldsForAppBeforeObserveReturnsBaselines(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ p := &Pipeline{Schema: 1, Channels: []PipelineChannel{
+ {Name: "edge"}, {Name: "custom"}}, SetAt: time.Now()}
+ h := f.HoldsForApp("bae", "web", p)
+ if h["edge"] != 0 {
+ t.Fatalf("edge baseline = %v; want 0 (rung default) before any observation", h["edge"])
+ }
+ if h["custom"] != 72*time.Hour {
+ t.Fatalf("custom baseline = %v; want 72h fail-closed before any observation", h["custom"])
+ }
+}
+
+// TestObservePipelineChangedContract pins the `changed` return value —
+// PassFilter's sole trigger for a changed-gated Save — across the cases
+// that matter: a genuine first sighting, a replay that changes nothing, a
+// pending promotion actually landing, and p == nil with nothing to
+// promote.
+func TestObservePipelineChangedContract(t *testing.T) {
+ now := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC)
+
+ t.Run("first sighting reports changed", func(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ p := &Pipeline{Schema: 1, Channels: []PipelineChannel{{Name: "stable", Bake: "96h"}}, SetAt: now}
+ if !f.ObservePipeline("bae", "web", p, now) {
+ t.Fatal("first sighting did not report changed")
+ }
+ })
+
+ t.Run("replay of the same object with nothing pending reports no change", func(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ // 96h >= the 72h baseline: tightens immediately, no pending created.
+ p := &Pipeline{Schema: 1, Channels: []PipelineChannel{{Name: "stable", Bake: "96h"}}, SetAt: now}
+ f.ObservePipeline("bae", "web", p, now)
+ if f.ObservePipeline("bae", "web", p, now.Add(time.Hour)) {
+ t.Fatal("replay of the identical object (nothing pending) reported changed")
+ }
+ })
+
+ t.Run("pending promotion firing reports changed", func(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ // 1h < the 72h baseline: goes pending at the first sighting.
+ p := &Pipeline{Schema: 1, Channels: []PipelineChannel{{Name: "stable", Bake: "1h"}}, SetAt: now}
+ f.ObservePipeline("bae", "web", p, now)
+ if !f.ObservePipeline("bae", "web", p, now.Add(72*time.Hour+time.Second)) {
+ t.Fatal("pending promotion elapsing did not report changed")
+ }
+ })
+
+ t.Run("p == nil with nothing to promote reports no change", func(t *testing.T) {
+ f := &FirstSeen{Jitter: 0}
+ p := &Pipeline{Schema: 1, Channels: []PipelineChannel{{Name: "stable", Bake: "96h"}}, SetAt: now}
+ f.ObservePipeline("bae", "web", p, now) // tightens only, nothing pending
+ if f.ObservePipeline("bae", "web", nil, now.Add(time.Hour)) {
+ t.Fatal("p == nil with nothing pending reported changed")
+ }
+ })
+}