amadan

carlos / platform Public

promote --retry: guard the version at the write, bound the wait

The retry decided on a version, slept, then wrote without re-checking, so a promote landing in the window was overwritten. sg.promote takes an expectVersion and refuses when the pointer it replaces names another version. The wait is now context-aware and capped at 2s (a PromotedAt further ahead is clock skew).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude b6a26ab549eb89120b2dc3f6d715b1292e109227 parent a13bd63
2 files changed, +51 −5
  • internal/release/release.go +24 −5
  • internal/release/release_test.go +27 −0
diff --git a/internal/release/release.go b/internal/release/release.go
index ab18f828b..c5b300f26 100644
--- a/internal/release/release.go
+++ b/internal/release/release.go
@@ -696,13 +696,19 @@ func Promote(ctx context.Context, s Store, priv ed25519.PrivateKey, acct, app, c
// migration verb, the tests, anything moving a pointer mechanically — have no
// promotion to record; a promotion a person made does.
func (sg Signer) Promote(ctx context.Context, s Store, acct, app, channel, version string, rec ...Promotion) error {
- return sg.promote(ctx, s, acct, app, channel, version, nil, rec...)
+ return sg.promote(ctx, s, acct, app, channel, version, nil, "", rec...)
}
// promote is Promote with an optional authorize hook, called on a pipeline
// read after the pointer read and before the conditional write. nil means no
// hook and no extra reads.
-func (sg Signer) promote(ctx context.Context, s Store, acct, app, channel, version string, authorize func(*Pipeline) error, rec ...Promotion) error {
+//
+// expectVersion, when set, is the version the pointer being replaced must
+// still name: the retry path's guarantee that it renews a promotion and never
+// changes the version. It is checked on the very pointer whose etag the write
+// is conditional on, so a promote that lands in between fails the write
+// rather than being overwritten. "" means no check.
+func (sg Signer) promote(ctx context.Context, s Store, acct, app, channel, version string, authorize func(*Pipeline) error, expectVersion string, rec ...Promotion) error {
if !ValidAccount(acct) {
return fmt.Errorf("promote: invalid account %q", acct)
}
@@ -753,6 +759,9 @@ func (sg Signer) promote(ctx context.Context, s Store, acct, app, channel, versi
default:
return err
}
+ if expectVersion != "" && old.Version != expectVersion {
+ return fmt.Errorf("promote --retry renews a promotion: %s now names %s, not %s", channel, old.Version, expectVersion)
+ }
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
@@ -932,9 +941,19 @@ func (sg Signer) PromoteOnLadder(ctx context.Context, s Store, acct, app, channe
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) {
- time.Sleep(time.Until(p.PromotedAt.Add(time.Second)))
+ // 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:
+ }
}
- if err := sg.promote(ctx, s, acct, app, channel, version, opt.Authorize, rec); err != nil {
+ if err := sg.promote(ctx, s, acct, app, channel, version, opt.Authorize, version, rec); err != nil {
return res, err
}
return res, nil
@@ -1026,7 +1045,7 @@ func (sg Signer) PromoteOnLadder(ctx context.Context, s Store, acct, app, channe
}
}
rec := Promotion{From: res.From, To: res.To, Actor: opt.Actor, Hotfix: opt.Hotfix}
- if err := sg.promote(ctx, s, acct, app, channel, version, opt.Authorize, rec); err != nil {
+ if err := sg.promote(ctx, s, acct, app, channel, version, opt.Authorize, "", rec); err != nil {
return res, err
}
stableTag(ctx, s, acct, app, version, opt.Actor, &res)
diff --git a/internal/release/release_test.go b/internal/release/release_test.go
index 25b9726c3..c868996d2 100644
--- a/internal/release/release_test.go
+++ b/internal/release/release_test.go
@@ -855,6 +855,33 @@ func TestPromoteRetryRenewsPromotedAt(t *testing.T) {
}
}
+// A retry that decided on v1, then finds the channel moved to v2 before its
+// write, must refuse and leave v2 in place. The guarded write is called
+// directly with the version the retry saw, so the window is deterministic.
+func TestPromoteRetryWriteRefusesWhenTheChannelMoved(t *testing.T) {
+ s := NewDirStore(t.TempDir())
+ ctx := context.Background()
+ priv, pub := testKeys(t)
+ shipRelease(t, s, "bab", "console", "v1", adoptPlatform, []byte("a"))
+ shipRelease(t, s, "bab", "console", "v2", adoptPlatform, []byte("b"))
+ sg := RootSigner(priv)
+ if err := sg.Promote(ctx, s, "bab", "console", "canary/x", "v2"); err != nil {
+ t.Fatal(err)
+ }
+ err := sg.promote(ctx, s, "bab", "console", "canary/x", "v1", nil, "v1", Promotion{Retry: true})
+ if err == nil || !strings.Contains(err.Error(), "v2") || !strings.Contains(err.Error(), "v1") {
+ t.Fatalf("guarded retry write = %v, want a refusal naming both versions", err)
+ }
+ cp, cerr := Channel(ctx, s, pub, "bab", "console", "canary/x")
+ if cerr != nil || cp.Version != "v2" {
+ t.Fatalf("channel after refused retry = %+v, %v; want v2 untouched", cp, cerr)
+ }
+ // And a retry whose expectation holds still writes.
+ if err := sg.promote(ctx, s, "bab", "console", "canary/x", "v2", nil, "v2", Promotion{Retry: true}); err != nil {
+ t.Fatalf("guarded retry write with a matching version: %v", err)
+ }
+}
+
func TestPromoteRetryNeverChangesTheVersion(t *testing.T) {
s := NewDirStore(t.TempDir())
ctx := context.Background()