amadan

carlos / platform Public

release: fix stale CAS comment, validate GetTarget hits, add CAS/etag tests

Review found two issues in the version-target CAS resolution:

- setTargetUnchecked's doc comment still said "last write wins," which
  contradicted the actual behavior (a lost race returns ErrCASConflict).
  Replaced with the real contract, including the auto-advance caller's
  decision: treat a lost race as advisory and don't retry, since the
  winner was either an operator raising the target or another advance
  landing the same value.
- GetTarget validated nothing it read, unlike every sibling reader
  (validateProductionFlag gates schema, GetTag checks payload-vs-key).
  Now refuses schema != 1 and a target that fails ParseSemVer.

Added TestGetTargetValidates (garbage JSON, wrong schema, bad target),
TestSetTargetCASConflict (a racingStore double proves a competing write
between SetTarget's read and its own Put surfaces ErrCASConflict), and
etag assertions in TestTargetSetGet (empty when unset, non-empty on a
hit).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 67fc6e3bfc4d4ff641abb3a05c5122dafc702b8a parent a2d058a
2 files changed, +104 −8
  • internal/release/target.go +20 −5
  • internal/release/target_test.go +84 −3
diff --git a/internal/release/target.go b/internal/release/target.go
index 45f2c01a6..e474dde33 100644
--- a/internal/release/target.go
+++ b/internal/release/target.go
@@ -26,9 +26,13 @@ type VersionTarget struct {
func TargetKey(acct, app string) string { return appPrefix(acct, app) + "version-target.json" }
// GetTarget reads the target; (nil, "", nil) when none is set — unset is a
-// state, not an error. The etag is returned for CAS on update.
+// state, not an error. The etag is returned for CAS on update. Like
+// validateProductionFlag and GetTag, a hit is validated before it is
+// trusted: an unsupported schema or a target that no longer parses as a
+// bare semver is refused rather than handed to a caller that assumes both.
func GetTarget(ctx context.Context, s Store, acct, app string) (*VersionTarget, string, error) {
- raw, etag, err := s.Get(ctx, TargetKey(acct, app))
+ key := TargetKey(acct, app)
+ raw, etag, err := s.Get(ctx, key)
if errors.Is(err, os.ErrNotExist) {
return nil, "", nil
}
@@ -37,7 +41,13 @@ func GetTarget(ctx context.Context, s Store, acct, app string) (*VersionTarget,
}
var vt VersionTarget
if err := json.Unmarshal(raw, &vt); err != nil {
- return nil, "", fmt.Errorf("version target at %s: %w", TargetKey(acct, app), err)
+ return nil, "", fmt.Errorf("version target at %s: %w", key, err)
+ }
+ if vt.Schema != 1 {
+ return nil, "", fmt.Errorf("version target at %s: unsupported schema %d", key, vt.Schema)
+ }
+ if _, ok := ParseSemVer(vt.Target); !ok {
+ return nil, "", fmt.Errorf("version target at %s: target %q is not a bare semver", key, vt.Target)
}
return &vt, etag, nil
}
@@ -64,8 +74,13 @@ func SetTarget(ctx context.Context, s Store, acct, app, target, actor string, no
// 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. Last write wins: the
-// target is an intention, not a ledger.
+// 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.
//
// Put mechanism: this codebase's Store.Put treats ifMatch "" as create-only
// (If-None-Match: *) and non-empty ifMatch as replace-only against that
diff --git a/internal/release/target_test.go b/internal/release/target_test.go
index 3a54adce3..fbfea3546 100644
--- a/internal/release/target_test.go
+++ b/internal/release/target_test.go
@@ -2,6 +2,8 @@ package release
import (
"context"
+ "encoding/json"
+ "errors"
"testing"
"time"
)
@@ -9,17 +11,20 @@ import (
func TestTargetSetGet(t *testing.T) {
s := NewDirStore(t.TempDir())
ctx := context.Background()
- if vt, _, err := GetTarget(ctx, s, "acct1", "km"); err != nil || vt != nil {
- t.Fatalf("unset target: got %v, %v; want nil, nil", vt, err)
+ if vt, etag, err := GetTarget(ctx, s, "acct1", "km"); err != nil || vt != nil || etag != "" {
+ t.Fatalf("unset target: got %v, %q, %v; want nil, \"\", nil", vt, etag, err)
}
now := time.Unix(1000, 0).UTC()
if err := SetTarget(ctx, s, "acct1", "km", "0.5.0", "paul@example.com", now); err != nil {
t.Fatal(err)
}
- vt, _, err := GetTarget(ctx, s, "acct1", "km")
+ vt, etag, err := GetTarget(ctx, s, "acct1", "km")
if err != nil || vt == nil || vt.Target != "0.5.0" || vt.SetBy != "paul@example.com" || !vt.SetAt.Equal(now) {
t.Fatalf("round trip: %+v, %v", vt, err)
}
+ if etag == "" {
+ t.Fatal("etag on a hit must not be empty — CAS on update depends on it")
+ }
// Raising is fine; garbage and iterations are not.
if err := SetTarget(ctx, s, "acct1", "km", "0.6.0", "paul@example.com", now); err != nil {
t.Fatal(err)
@@ -70,3 +75,79 @@ func TestNextIteration(t *testing.T) {
t.Fatalf("after -1 and -3 exist: %q, %v; want 0.5.0-4", v, err)
}
}
+
+// TestGetTargetValidates matches GetTag/validateProductionFlag's discipline:
+// a hit is checked before it is trusted, not handed back as-is.
+func TestGetTargetValidates(t *testing.T) {
+ ctx := context.Background()
+ cases := []struct {
+ name string
+ raw []byte
+ }{
+ {"garbage json", []byte("not json")},
+ {"unsupported schema", mustMarshal(t, VersionTarget{Schema: 2, Target: "0.5.0"})},
+ {"target not a bare semver", mustMarshal(t, VersionTarget{Schema: 1, Target: "not-semver"})},
+ }
+ for _, c := range cases {
+ t.Run(c.name, func(t *testing.T) {
+ s := NewDirStore(t.TempDir())
+ if _, err := s.Put(ctx, TargetKey("acct1", "km"), c.raw, ""); err != nil {
+ t.Fatal(err)
+ }
+ if vt, etag, err := GetTarget(ctx, s, "acct1", "km"); err == nil {
+ t.Fatalf("want error, got vt=%+v etag=%q", vt, etag)
+ }
+ })
+ }
+}
+
+func mustMarshal(t *testing.T, v any) []byte {
+ t.Helper()
+ b, err := json.Marshal(v)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return b
+}
+
+// racingStore wraps a Store and, the first time its key of interest is Get,
+// sneaks in a competing write directly against the underlying store before
+// returning — simulating a second operator's write landing between the
+// caller's Get and its own Put. Everything else passes through unchanged.
+type racingStore struct {
+ Store
+ key string
+ raced bool
+ race func()
+}
+
+func (r *racingStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ raw, etag, err := r.Store.Get(ctx, key)
+ if key == r.key && !r.raced {
+ r.raced = true
+ r.race()
+ }
+ return raw, etag, err
+}
+
+// TestSetTargetCASConflict is the insurance for setTargetUnchecked's CAS
+// rewrite: a competing write landing between SetTarget's read and its own
+// Put must surface as ErrCASConflict, not a silent overwrite.
+func TestSetTargetCASConflict(t *testing.T) {
+ base := NewDirStore(t.TempDir())
+ ctx := context.Background()
+ now := time.Unix(1000, 0).UTC()
+ key := TargetKey("acct1", "km")
+ rs := &racingStore{Store: base, key: key}
+ rs.race = func() {
+ // A second writer creates the target first, directly against the
+ // underlying store — bypassing rs so this doesn't re-trigger itself.
+ if err := SetTarget(ctx, base, "acct1", "km", "0.4.0", "other", now); err != nil {
+ t.Fatalf("competing write: %v", err)
+ }
+ }
+ err := SetTarget(ctx, rs, "acct1", "km", "0.5.0", "actor", now)
+ if !errors.Is(err, ErrCASConflict) {
+ t.Fatalf("want ErrCASConflict, got %v", err)
+ }
+}