amadan
release: a retry's fresh stamp is judged against the guarded pointer
Final review F7 (Plan 1 D1). PromoteOnLadder checked timestamp freshness against its first pointer read; a concurrent retry that renewed the same version before sg.promote's read passed the version guard and the signer could reuse that pointer's second-truncated PromotedAt, leaving a refusing box blocked behind a successful retry. With expectVersion set, sg.promote now waits (ctx-aware, capped at 2 s) until now-truncated is strictly after the PromotedAt of the pointer whose etag guards the write, and refuses if the cap is not enough (clock skew) rather than write a stamp that is not newer. The earlier wait in PromoteOnLadder is gone. Tests: TestPromoteRetryStampsAfterTheGuardedPointer, TestConcurrentRetriesNeverShareAStamp (both fail with the wait disabled).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+112
−13
internal/release/release.go+36 −13internal/release/release_test.go+76 −0
diff --git a/internal/release/release.go b/internal/release/release.go| index a542170f6..ac7667a6f 100644 |
| --- a/internal/release/release.go |
| +++ b/internal/release/release.go |
| @@ -769,6 +769,17 @@ func (sg Signer) promote(ctx context.Context, s Store, acct, app, channel, versi |
| if expectVersion != "" && old.Version != expectVersion { |
| return fmt.Errorf("promote --retry renews a promotion: %s now names %s, not %s", channel, old.Version, expectVersion) |
| } |
| + // A retry's whole effect is a new PromotedAt, so freshness is checked |
| + // HERE, against the pointer whose etag guards the write, not against an |
| + // earlier read: a concurrent retry that renewed the same version in |
| + // between would otherwise be re-stamped with its own second-truncated |
| + // time, and a box that refused that promotion would stay blocked behind |
| + // a "successful" retry (final review F7). |
| + if expectVersion != "" { |
| + if err := waitPastPromotedAt(ctx, channel, old.PromotedAt); err != nil { |
| + return err |
| + } |
| + } |
| if authorize != nil { |
| // Between the pointer read and the conditional write, so the write |
| // lands only if the pointer is still what was authorized against |
| @@ -802,6 +813,29 @@ func (sg Signer) promote(ctx context.Context, s Store, acct, app, channel, versi |
| return err |
| } |
| +// waitPastPromotedAt returns once the stamp signPointer would produce (now, |
| +// truncated to the second) is strictly after prev, waiting into the next |
| +// second if it must. The wait is bounded at 2 s: a prev further ahead than |
| +// that is clock skew, and it refuses rather than write a stamp that is not |
| +// newer. |
| +func waitPastPromotedAt(ctx context.Context, channel string, prev time.Time) error { |
| + if time.Now().UTC().Truncate(time.Second).After(prev) { |
| + return nil |
| + } |
| + t := time.NewTimer(min(time.Until(prev.Add(time.Second)), 2*time.Second)) |
| + select { |
| + case <-ctx.Done(): |
| + t.Stop() |
| + return ctx.Err() |
| + case <-t.C: |
| + } |
| + if !time.Now().UTC().Truncate(time.Second).After(prev) { |
| + return fmt.Errorf("promote --retry: %s was promoted at %s, ahead of this clock; nothing was written", |
| + channel, prev.Format(time.RFC3339)) |
| + } |
| + return nil |
| +} |
| + |
| // PromoteOptions carries what the ladder needs to know about a promotion |
| // beyond "which version, which channel". |
| type PromoteOptions struct { |
| @@ -947,19 +981,8 @@ func (sg Signer) PromoteOnLadder(ctx context.Context, s Store, acct, app, channe |
| if opt.Retry { |
| res.From = res.To |
| rec := Promotion{From: res.To, To: res.To, Actor: opt.Actor, Retry: true} |
| - if !time.Now().UTC().Truncate(time.Second).After(p.PromotedAt) { |
| - // Wait into the next second, bounded: a PromotedAt further |
| - // ahead than that is clock skew, and hanging on it would |
| - // help nobody. The write below is guarded either way. |
| - wait := min(time.Until(p.PromotedAt.Add(time.Second)), 2*time.Second) |
| - t := time.NewTimer(wait) |
| - select { |
| - case <-ctx.Done(): |
| - t.Stop() |
| - return res, ctx.Err() |
| - case <-t.C: |
| - } |
| - } |
| + // The version guard and the fresh-stamp wait both live in |
| + // sg.promote, on the pointer its write is conditional on. |
| if err := sg.promote(ctx, s, acct, app, channel, version, opt.Authorize, version, rec); err != nil { |
| return res, err |
| } |
diff --git a/internal/release/release_test.go b/internal/release/release_test.go| index c868996d2..3a208eada 100644 |
| --- a/internal/release/release_test.go |
| +++ b/internal/release/release_test.go |
| @@ -882,6 +882,82 @@ func TestPromoteRetryWriteRefusesWhenTheChannelMoved(t *testing.T) { |
| } |
| } |
| +// Final review F7: the guarded write's own pointer was stamped this very |
| +// second. The retry must still produce a strictly later PromotedAt. |
| +func TestPromoteRetryStampsAfterTheGuardedPointer(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + shipRelease(t, s, "bab", "console", "v1", adoptPlatform, []byte("a")) |
| + sg := RootSigner(priv) |
| + if err := sg.Promote(ctx, s, "bab", "console", "canary/x", "v1"); err != nil { |
| + t.Fatal(err) |
| + } |
| + before, _ := Channel(ctx, s, pub, "bab", "console", "canary/x") |
| + if err := sg.promote(ctx, s, "bab", "console", "canary/x", "v1", nil, "v1", Promotion{Retry: true}); err != nil { |
| + t.Fatalf("retry: %v", err) |
| + } |
| + after, _ := Channel(ctx, s, pub, "bab", "console", "canary/x") |
| + if !after.PromotedAt.After(before.PromotedAt) { |
| + t.Fatalf("retry stamped %v over %v, want strictly later", after.PromotedAt, before.PromotedAt) |
| + } |
| +} |
| + |
| +// getHook runs onGet before every Get, so a test can land a competing write |
| +// between a caller's first pointer read and its guarded one. |
| +type getHook struct { |
| + Store |
| + onGet func(key string) |
| +} |
| + |
| +func (h *getHook) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + h.onGet(key) |
| + return h.Store.Get(ctx, key) |
| +} |
| + |
| +// Final review F7, the race itself: PromoteOnLadder's first read sees an old |
| +// pointer (no wait needed), then a concurrent retry renews it before the |
| +// guarded read. The stamp is judged against the pointer the write is |
| +// conditional on, so it is still strictly later than the competitor's. |
| +func TestConcurrentRetriesNeverShareAStamp(t *testing.T) { |
| + inner := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + priv, pub := testKeys(t) |
| + shipRelease(t, inner, "bab", "console", "v1", adoptPlatform, []byte("a")) |
| + sg := RootSigner(priv) |
| + if err := sg.Promote(ctx, inner, "bab", "console", "canary/x", "v1"); err != nil { |
| + t.Fatal(err) |
| + } |
| + time.Sleep(1100 * time.Millisecond) // the first read's pointer is a second old |
| + key := ChannelKey("bab", "console", "canary/x") |
| + var competitor time.Time |
| + gets := 0 |
| + s := &getHook{Store: inner} |
| + s.onGet = func(k string) { |
| + if k != key { |
| + return |
| + } |
| + gets++ |
| + if gets == 2 { // sg.promote's guarded read |
| + if err := sg.promote(ctx, inner, "bab", "console", "canary/x", "v1", nil, "v1", Promotion{Retry: true}); err != nil { |
| + t.Errorf("competing retry: %v", err) |
| + } |
| + cp, _ := Channel(ctx, inner, pub, "bab", "console", "canary/x") |
| + competitor = cp.PromotedAt |
| + } |
| + } |
| + if _, err := sg.PromoteOnLadder(ctx, s, "bab", "console", "canary/x", "v1", PromoteOptions{Retry: true}); err != nil { |
| + t.Fatalf("retry: %v", err) |
| + } |
| + if gets < 2 || competitor.IsZero() { |
| + t.Fatalf("the competing retry never ran (gets=%d)", gets) |
| + } |
| + after, _ := Channel(ctx, inner, pub, "bab", "console", "canary/x") |
| + if !after.PromotedAt.After(competitor) { |
| + t.Fatalf("retry stamped %v, the competitor %v: want strictly later", after.PromotedAt, competitor) |
| + } |
| +} |
| + |
| func TestPromoteRetryNeverChangesTheVersion(t *testing.T) { |
| s := NewDirStore(t.TempDir()) |
| ctx := context.Background() |