amadan

carlos / platform Public

registry: Upsert never changes hold_op on conflict; fingerprints see it (Task 12 fix round 1)

A held row keeps its status whatever the incoming row carries; only SetHold,
ReleaseHold and DeleteIfHold change a hold. A fresh INSERT may materialise a
held row. Fingerprint and SnapshotFingerprint fold hold_op in only when
non-empty, so unheld registries hash byte-for-byte as before (pinned by
goldens taken from the parent commit).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude a81caec40865857468c4d864d64c571aa316fd0f parent 95e3d45
2 files changed, +170 −13
  • internal/registry/hold_test.go +139 −0
  • internal/registry/registry.go +31 −13
diff --git a/internal/registry/hold_test.go b/internal/registry/hold_test.go
index 670844901..e5b4569d9 100644
--- a/internal/registry/hold_test.go
+++ b/internal/registry/hold_test.go
@@ -227,3 +227,142 @@ func TestHoldOpMigrationIdempotentOnLegacyDB(t *testing.T) {
r.Close()
}
}
+
+// Fix round 1: on conflict Upsert never changes hold_op, and a held row keeps
+// its status whatever the incoming row carries. Only SetHold, ReleaseHold and
+// DeleteIfHold change a hold.
+func TestUpsertStaleOpCannotTakeAHoldBack(t *testing.T) {
+ r := holdFixture(t, "h")
+ if err := r.SetHold("h", "moving", holdOpA); err != nil {
+ t.Fatal(err)
+ }
+ stale, _ := r.Get("h") // a caller reads op A's row ...
+ if ok, err := r.ReleaseHold("h", holdOpA); err != nil || !ok {
+ t.Fatal(ok, err)
+ }
+ if err := r.SetHold("h", "landing", holdOpB); err != nil { // ... B takes the row ...
+ t.Fatal(err)
+ }
+ if err := r.Upsert(*stale); err != nil { // ... and the stale write arrives.
+ t.Fatal(err)
+ }
+ rt, _ := r.Get("h")
+ if rt.HoldOp != holdOpB || rt.Status != "landing" {
+ t.Fatalf("stale Upsert changed B's hold: %+v", rt)
+ }
+ if ok, err := r.ReleaseHold("h", holdOpA); err != nil || ok {
+ t.Fatalf("ReleaseHold under the stale op: %v %v", ok, err)
+ }
+ if rt, _ = r.Get("h"); rt.HoldOp != holdOpB {
+ t.Fatalf("hold lost: %+v", rt)
+ }
+}
+
+func TestUpsertSameOpWithReadyStatusKeepsHeldStatus(t *testing.T) {
+ r := holdFixture(t, "h")
+ if err := r.SetHold("h", "restoring", holdOpA); err != nil {
+ t.Fatal(err)
+ }
+ for _, status := range []string{"ready", "failed", "", "landing"} {
+ if err := r.Upsert(Route{Host: "h", Kind: "instance", Socket: "/s", Status: status, HoldOp: holdOpA}); err != nil {
+ t.Fatal(err)
+ }
+ rt, _ := r.Get("h")
+ if rt.Status != "restoring" || rt.HoldOp != holdOpA {
+ t.Fatalf("incoming status %q: %+v", status, rt)
+ }
+ }
+}
+
+func TestUpsertCannotHoldAnUnheldRowOnConflict(t *testing.T) {
+ r := holdFixture(t, "h")
+ if err := r.Upsert(Route{Host: "h", Kind: "instance", Socket: "/s", Status: "moving", HoldOp: holdOpA}); err != nil {
+ t.Fatal(err)
+ }
+ rt, _ := r.Get("h")
+ if rt.HoldOp != "" {
+ t.Fatalf("Upsert took a hold on conflict: %+v", rt)
+ }
+ if rows, _ := r.HoldRows(); len(rows) != 0 {
+ t.Fatalf("HoldRows = %+v", rows)
+ }
+ if err := r.SetHold("h", "moving", holdOpA); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// A held row can be materialised: a fresh INSERT writes the incoming hold.
+func TestUpsertInsertMaterialisesAHeldRow(t *testing.T) {
+ r := openTestRegistry(t)
+ if err := r.Upsert(Route{Host: "new", Kind: "instance", Socket: "/s", Status: "landing", HoldOp: holdOpA}); err != nil {
+ t.Fatal(err)
+ }
+ rt, _ := r.Get("new")
+ if rt.Status != "landing" || rt.HoldOp != holdOpA {
+ t.Fatalf("%+v", rt)
+ }
+}
+
+// The edge reloads only when Fingerprint moves: a hold handed from one op to
+// another with the status unchanged must move it, and an unheld registry must
+// hash exactly as it did before the column existed.
+func TestFingerprintSeesHoldOpChangeAndIgnoresUnheld(t *testing.T) {
+ r := holdFixture(t, "h")
+ fp := func() (string, string) {
+ a, err := r.Fingerprint()
+ if err != nil {
+ t.Fatal(err)
+ }
+ b, err := r.SnapshotFingerprint()
+ if err != nil {
+ t.Fatal(err)
+ }
+ return a, b
+ }
+ if err := r.SetHold("h", "moving", holdOpA); err != nil {
+ t.Fatal(err)
+ }
+ f1, s1 := fp()
+ if ok, _ := r.ReleaseHold("h", holdOpA); !ok {
+ t.Fatal("release")
+ }
+ if err := r.SetHold("h", "moving", holdOpB); err != nil { // same status, other op
+ t.Fatal(err)
+ }
+ f2, s2 := fp()
+ if f1 == f2 {
+ t.Fatal("Fingerprint did not move when only hold_op changed")
+ }
+ if s1 == s2 {
+ t.Fatal("SnapshotFingerprint did not move when only hold_op changed")
+ }
+ // Direct column change with the status untouched.
+ if _, err := r.sql.Exec(`UPDATE routes SET hold_op = ? WHERE host = 'h'`, holdOpA); err != nil {
+ t.Fatal(err)
+ }
+ if f3, s3 := fp(); f3 == f2 || s3 == s2 {
+ t.Fatal("a bare hold_op change did not move both fingerprints")
+ }
+
+}
+
+// Switch-off contract: an unheld registry hashes byte-for-byte as it did
+// before hold_op existed. The goldens were produced by the same two rows on
+// the parent commit's registry.go (8437004c^).
+func TestFingerprintsUnchangedForUnheldRows(t *testing.T) {
+ u := openTestRegistry(t)
+ if err := u.Upsert(Route{Host: "u", Kind: "instance", Socket: "/s", App: "a", CreatedAt: 1700000000}); err != nil {
+ t.Fatal(err)
+ }
+ if err := u.Upsert(Route{Host: "v", Kind: "static", Status: "starting", Hibernate: true, CreatedAt: 1700000001}); err != nil {
+ t.Fatal(err)
+ }
+ const wantF = "2:f1013971d496e9401cd6a9928a1f4e18669deda40089d2c2a8b84263b32f34c6"
+ const wantS = "2:149e8f668771c7493bdb7d4245fdabe03e957b0909c4daa1d037484b11064b80"
+ if got, _ := u.Fingerprint(); got != wantF {
+ t.Fatalf("Fingerprint = %s, want %s", got, wantF)
+ }
+ if got, _ := u.SnapshotFingerprint(); got != wantS {
+ t.Fatalf("SnapshotFingerprint = %s, want %s", got, wantS)
+ }
+}
diff --git a/internal/registry/registry.go b/internal/registry/registry.go
index b7b330950..d14547170 100644
--- a/internal/registry/registry.go
+++ b/internal/registry/registry.go
@@ -59,9 +59,10 @@ type Route struct {
// HoldOp is the operation holding Status (moves spec §5.2); "" when not
// held. A held row (Status in HoldStatuses) belongs to that operation: the
// edge never serves it, ticks refuse it and no ordinary wake reaches it.
- // Only SetHold and ReleaseHold change a hold; Upsert keeps an existing one
- // when the incoming row carries none. Deliberately NOT in Fingerprint():
- // every hold also moves Status, which is.
+ // Only SetHold, ReleaseHold and DeleteIfHold change a hold; Upsert's
+ // conflict path never touches it. IN Fingerprint() and SnapshotFingerprint,
+ // folded in only when non-empty so an unheld registry hashes exactly as it
+ // did before the column existed.
HoldOp string
LastError string
@@ -842,10 +843,12 @@ func (r *Registry) Delete(host string) error {
return err
}
-// Upsert inserts or updates a route by host. A held row (a non-empty hold_op) is
-// not released by it: when the incoming Route carries no HoldOp, the existing
-// row keeps its hold_op AND its status (only SetHold and ReleaseHold change a
-// hold; moves spec §5.2, defence in depth beside the reconciler's own skip).
+// Upsert inserts or updates a route by host. On conflict it NEVER changes
+// hold_op, and a row that is held keeps its status too, whatever the incoming
+// Route carries: only SetHold, ReleaseHold and DeleteIfHold change a hold
+// (moves spec §5.2; a stale Upsert carrying a finished operation's HoldOp must
+// not hand the row back to it). A fresh INSERT does write the incoming HoldOp,
+// so a held row can be materialised.
// Twelve columns are deliberately
// not plain overwrites — `environment`, `caps`, `tunnel`, `provisioned`,
// `seed`, `unit`, `fleet_box`, `replication`, `gate`, `args`, `idle_seconds`
@@ -1003,8 +1006,7 @@ VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,
ON CONFLICT(host) DO UPDATE SET
slug=excluded.slug, kind=excluded.kind, socket=excluded.socket,
addr=excluded.addr,
- status=CASE WHEN excluded.hold_op = '' AND routes.hold_op != '' THEN routes.status ELSE excluded.status END,
- hold_op=CASE WHEN excluded.hold_op = '' THEN routes.hold_op ELSE excluded.hold_op END,
+ status=CASE WHEN routes.hold_op != '' THEN routes.status ELSE excluded.status END,
last_error=excluded.last_error,
db_path=excluded.db_path, version=excluded.version,
prev_version=excluded.prev_version, app=excluded.app, pool=excluded.pool,
@@ -2318,7 +2320,7 @@ func (r *Registry) Fingerprint() (string, error) {
}
rows, err := db.Query(`
SELECT rowid, status, socket, addr, version, hibernate, channel, backing, tunnel,
- db_path, region, fleet_box, addrs, health, gate
+ db_path, region, fleet_box, addrs, health, gate, hold_op
FROM routes ORDER BY rowid`)
if err != nil {
return "", err
@@ -2328,14 +2330,22 @@ FROM routes ORDER BY rowid`)
n := 0
for rows.Next() {
var rowid, hibernate int64
- var status, socket, addr, version, channel, backing, tunnel, dbPath, region, fleetBox, addrs, health, gate string
+ var status, socket, addr, version, channel, backing, tunnel, dbPath, region, fleetBox, addrs, health, gate, holdOp string
if err := rows.Scan(&rowid, &status, &socket, &addr, &version, &hibernate,
- &channel, &backing, &tunnel, &dbPath, &region, &fleetBox, &addrs, &health, &gate); err != nil {
+ &channel, &backing, &tunnel, &dbPath, &region, &fleetBox, &addrs, &health, &gate, &holdOp); err != nil {
return "", err
}
- fmt.Fprintf(h, "%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1e",
+ fmt.Fprintf(h, "%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%d\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s\x1f%s",
rowid, status, socket, addr, version, hibernate, channel, backing, tunnel,
dbPath, region, fleetBox, addrs, health, gate)
+ // Only a held row carries the extra field: an unheld registry's digest
+ // is byte-identical to the pre-hold_op encoding (switch-off contract),
+ // while a hold handed to another op with the status unchanged still
+ // moves it.
+ if holdOp != "" {
+ fmt.Fprintf(h, "\x1fhold_op=%s", holdOp)
+ }
+ fmt.Fprint(h, "\x1e")
n++
}
if err := rows.Err(); err != nil {
@@ -2537,6 +2547,14 @@ func (r *Registry) SnapshotFingerprint() (string, error) {
if snapshotChurnCols[cols[i]] {
continue
}
+ // hold_op is hashed only when set, tagged, so a registry with no
+ // held rows digests exactly as it did before the column existed.
+ if cols[i] == "hold_op" {
+ if v.Valid && v.String != "" {
+ fmt.Fprintf(h, "hold_op=%s\x1f", v.String)
+ }
+ continue
+ }
// NULL and "" must not hash alike: a column that has never been
// written is a different restore than one explicitly cleared.
if v.Valid {