amadan
release: fix EnsureTag race condition and error handling
Three critical fixes: 1. Thread etag through setTargetUnchecked to close check-then-write race: - setTargetUnchecked now takes ifMatch parameter instead of doing its own Get - SetTarget and EnsureTag pass the etag from their own GetTarget reads - CAS window now spans full check-then-write, making advisory loss truly safe - Operator raises between check and write now properly protected 2. Propagate non-ENOENT errors in EnsureTag changelog loop: - Only skip os.ErrNotExist (genuine gaps in iteration history) - Any other error on releaseVersion aborts before writing (prevents Commit="" tags) - Any other error on other iterations also aborts (prevents partial changelogs) - Ensures immutable tags are always complete and correct 3. Add test coverage for blocked-by-higher-tag and mid-range-gap scenarios: - TestEnsureTagBlockedByHigherTag: verify tag cut succeeds but advance blocked - TestEnsureTagMidRangeGap: verify changelog skips missing iteration (ENOENT)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 files changed,
+85
−21
internal/release/tag.go+18 −3internal/release/tag_test.go+48 −0internal/release/target.go+19 −18
diff --git a/internal/release/tag.go b/internal/release/tag.go| index a647fb73c..1616e99f5 100644 |
| --- a/internal/release/tag.go |
| +++ b/internal/release/tag.go |
| @@ -127,7 +127,16 @@ func EnsureTag(ctx context.Context, s Store, acct, app, releaseVersion, actor st |
| ver := IterationVersion(target, i) |
| m, err := GetManifest(ctx, s, acct, app, ver) |
| if err != nil { |
| - continue // an unshipped or unreadable iteration is a gap, not a failure |
| + // Only skip genuine gaps (not-found); propagate other errors. |
| + if errors.Is(err, os.ErrNotExist) { |
| + continue |
| + } |
| + // For the promoted release itself, any error prevents tag creation. |
| + if ver == releaseVersion { |
| + return nil, false, err |
| + } |
| + // For other iterations, a transient error also aborts. |
| + return nil, false, err |
| } |
| if ver == releaseVersion { |
| tag.Commit = m.Commit |
| @@ -153,7 +162,7 @@ func EnsureTag(ctx context.Context, s Store, acct, app, releaseVersion, actor st |
| // stays unset only if the next patch would collide with a HIGHER |
| // existing tag (a hotfix line under a fleet already past it). |
| next := target.NextPatch() |
| - cur, _, err := GetTarget(ctx, s, acct, app) |
| + cur, etag, err := GetTarget(ctx, s, acct, app) |
| if err == nil && (cur == nil || func() bool { c, ok := ParseSemVer(cur.Target); return ok && c.Compare(target) <= 0 }()) { |
| if higher, lerr := ListTags(ctx, s, acct, app); lerr == nil { |
| blocked := false |
| @@ -164,7 +173,13 @@ func EnsureTag(ctx context.Context, s Store, acct, app, releaseVersion, actor st |
| } |
| } |
| if !blocked { |
| - _ = setTargetUnchecked(ctx, s, acct, app, next, actor, now) // advisory; the tag is already cut |
| + // Advance with the etag from our check. The CAS window now spans |
| + // the full check-then-write, so a lost race is truly advisory |
| + // (operator's raise wins, concurrent advance is idempotent). |
| + if cur == nil { |
| + etag = "" // first write: create-only |
| + } |
| + _ = setTargetUnchecked(ctx, s, acct, app, next, actor, now, etag) |
| } |
| } |
| } |
diff --git a/internal/release/tag_test.go b/internal/release/tag_test.go| index a20df48b5..af9aa02ef 100644 |
| --- a/internal/release/tag_test.go |
| +++ b/internal/release/tag_test.go |
| @@ -152,3 +152,51 @@ func TestEnsureTagDoesNotLowerRaisedTarget(t *testing.T) { |
| t.Fatalf("target was moved: %+v; want 0.6.0 kept", vt) |
| } |
| } |
| + |
| +func TestEnsureTagBlockedByHigherTag(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + now := time.Unix(2000, 0).UTC() |
| + shipIteration(t, s, "0.5.0-1", "first", "- first iteration") |
| + // Operator or a fleet already past has a 0.6.0 tag; next patch would collide. |
| + higherTag := &Tag{Schema: 1, Version: "0.6.0", Release: "0.6.0-1", TaggedAt: now, Actor: "admin"} |
| + if err := writeTag(ctx, s, "acct1", "km", higherTag); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Cutting 0.5.0 from -1 must succeed, but advance is blocked by the collision. |
| + tag, created, err := EnsureTag(ctx, s, "acct1", "km", "0.5.0-1", "paul@example.com", now) |
| + if err != nil || !created || tag == nil { |
| + t.Fatalf("EnsureTag should succeed: %+v, %v, %v", tag, created, err) |
| + } |
| + if tag.Version != "0.5.0" { |
| + t.Fatalf("tag version: %s; want 0.5.0", tag.Version) |
| + } |
| + // Target must not have been set (advance blocked by 0.6.0). |
| + vt, _, _ := GetTarget(ctx, s, "acct1", "km") |
| + if vt != nil { |
| + t.Fatalf("target was set: %+v; want nil (blocked by higher tag 0.6.0)", vt) |
| + } |
| +} |
| + |
| +func TestEnsureTagMidRangeGap(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + ctx := context.Background() |
| + now := time.Unix(2000, 0).UTC() |
| + shipIteration(t, s, "0.5.0-1", "first", "- first") |
| + // Skip 0.5.0-2 intentionally. |
| + shipIteration(t, s, "0.5.0-3", "third", "- third") |
| + tag, created, err := EnsureTag(ctx, s, "acct1", "km", "0.5.0-3", "paul@example.com", now) |
| + if err != nil || !created || tag == nil { |
| + t.Fatalf("EnsureTag: %+v, %v, %v", tag, created, err) |
| + } |
| + // Changelog should have sections for -1 and -3 only (gap at -2 skipped). |
| + want := "## 0.5.0-1 — first\n\n- first\n\n## 0.5.0-3 — third\n\n- third\n" |
| + if tag.Changelog != want { |
| + t.Fatalf("changelog with gap:\n%q\nwant:\n%q", tag.Changelog, want) |
| + } |
| + // Target should auto-advance to 0.5.1 (no collision). |
| + vt, _, _ := GetTarget(ctx, s, "acct1", "km") |
| + if vt == nil || vt.Target != "0.5.1" { |
| + t.Fatalf("target: %+v; want 0.5.1", vt) |
| + } |
| +} |
diff --git a/internal/release/target.go b/internal/release/target.go| index e474dde33..0140c10d4 100644 |
| --- a/internal/release/target.go |
| +++ b/internal/release/target.go |
| @@ -69,18 +69,26 @@ func SetTarget(ctx context.Context, s Store, acct, app, target, actor string, no |
| return fmt.Errorf("version target %s: already released as tag %s — the target must be greater", target, tag.Version) |
| } |
| } |
| - return setTargetUnchecked(ctx, s, acct, app, t, actor, now) |
| + cur, etag, err := GetTarget(ctx, s, acct, app) |
| + if err != nil { |
| + return err |
| + } |
| + if cur == nil { |
| + etag = "" // first write: create-only |
| + } |
| + return setTargetUnchecked(ctx, s, acct, app, t, actor, now, etag) |
| } |
| // setTargetUnchecked writes the target with no tag comparison — the |
| // auto-advance after a tag cut has already proved its ordering, and |
| -// re-checking would race the tag it just wrote. The write is still a real |
| -// CAS: a lost race returns ErrCASConflict like any other Put here, it does |
| -// not silently overwrite. The auto-advance caller treats that conflict as |
| -// ADVISORY and does not retry — whoever won the race was either an operator |
| -// raising the target (which must not be clobbered by a stale advance) or |
| -// another advance landing the same value, and both outcomes are fine to |
| -// leave as they are. |
| +// re-checking would race the tag it just wrote. The caller provides the |
| +// ifMatch etag from their own GetTarget read, so the CAS window spans |
| +// the entire check-then-write: a lost race returns ErrCASConflict and the |
| +// auto-advance caller treats that conflict as ADVISORY and does not retry. |
| +// Both outcomes are correct: an operator's raise between the caller's check |
| +// and this write wins (operator's ordering was higher), and a concurrent |
| +// advance landing the same value is idempotent. Neither should be |
| +// retried by a stale check. |
| // |
| // Put mechanism: this codebase's Store.Put treats ifMatch "" as create-only |
| // (If-None-Match: *) and non-empty ifMatch as replace-only against that |
| @@ -91,21 +99,14 @@ func SetTarget(ctx context.Context, s Store, acct, app, target, actor string, no |
| // Get the key first, treat os.ErrNotExist as "" (first write, create-only), |
| // otherwise use the etag just read, then Put once with that ifMatch and let |
| // a lost race surface as ErrCASConflict to the caller — re-read and retry, |
| -// never overwrite blind. The target follows that same convention rather |
| -// than the brief's literal "*", which store.go does not special-case. |
| -func setTargetUnchecked(ctx context.Context, s Store, acct, app string, t SemVer, actor string, now time.Time) error { |
| +// never overwrite blind. The target follows that same convention. |
| +func setTargetUnchecked(ctx context.Context, s Store, acct, app string, t SemVer, actor string, now time.Time, ifMatch string) error { |
| raw, err := json.Marshal(VersionTarget{Schema: 1, Target: t.String(), SetBy: actor, SetAt: now}) |
| if err != nil { |
| return err |
| } |
| key := TargetKey(acct, app) |
| - _, etag, err := s.Get(ctx, key) |
| - if errors.Is(err, os.ErrNotExist) { |
| - etag = "" // first write: create-only |
| - } else if err != nil { |
| - return fmt.Errorf("version target: read %s: %w", key, err) |
| - } |
| - if _, err := s.Put(ctx, key, raw, etag); err != nil { |
| + if _, err := s.Put(ctx, key, raw, ifMatch); err != nil { |
| return fmt.Errorf("version target: write: %w", err) |
| } |
| return nil |