amadan
accounts: a diverged destination pointer converges — the instruction now has an exit
The verify refusal told the operator to "re-run the migration once the writer has stopped", but the destination pointer existed by then and the copy is create-only, so the re-run died at "already exists with different content". The state was safe and un-re-runnable, and the message forbade the only way out. The split that fixes it is the one the bucket already makes: release manifests and artifacts are content-addressed and immutable, so different bytes at an existing path stays a hard refusal — an immutable path must never change meaning. A channel pointer is the one MUTABLE object; it moves on every promote, so a destination pointer that disagrees is this migration's own leftover and the verb's whole job is to make the destination serve what the source serves. Pointers now converge via CAS-with-etag (one retry on a lost race), in the copy AND in verify, and verify re-reads once after converging. Only a second consecutive mismatch — something actively writing — is fatal, and it now names the exact key to delete rather than a vague "try again". Also: the bake clock key was not injective. Channels may contain "/", so ops app "kass" on channel "canary/x" produced the same key as account "kass", app "canary", channel "x" — one account's observation satisfying another's window, the same bug from a different direction. Non-ops keys now join on "|", which cannot occur in a sqid, an app name or a channel segment; ops keeps the pre-accounts shape (injective on its own, since app names have no "/") so every existing box's observations still resolve. And a one-sentence hedge on the account-mismatch banner: the console row is usually what is behind, but if the ROUTES are the ones in the wrong account, move those back — whichever is right, the two have to agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5 files changed,
+255
−14
accounts.go+92 −12e2e_test.go+118 −0internal/console/templates/app.html+3 −0internal/release/bake.go+17 −2internal/release/bake_test.go+25 −0
diff --git a/accounts.go b/accounts.go| index d5e2b02a8..1f9b26ead 100644 |
| --- a/accounts.go |
| +++ b/accounts.go |
| @@ -15,6 +15,7 @@ import ( |
| "crypto/ed25519" |
| "crypto/rand" |
| "encoding/binary" |
| + "errors" |
| "flag" |
| "fmt" |
| "log" |
| @@ -219,16 +220,37 @@ func runAccountsMigrate(args []string) { |
| if err != nil { |
| log.Fatalf("accounts migrate: read %s: %v", c.from, err) |
| } |
| - // Create-only, then verify: re-running the migration over identical |
| - // bytes is a no-op, and DIFFERENT bytes at a destination that already |
| - // exists is refused loudly rather than overwritten. |
| + // Channel pointers CONVERGE; everything else is immutable and is |
| + // claimed create-only. The split matters: a release manifest or |
| + // artifact at an existing path must never change meaning, so |
| + // different bytes there is a hard refusal. A channel pointer is the |
| + // one mutable object in the bucket — it moves on every promote — so a |
| + // destination pointer that disagrees is this migration's own |
| + // leftover, and the verb's whole job is to make the destination serve |
| + // what the source serves. Refusing there would strand the operator: |
| + // the pointer exists, so a re-run would hit exactly this branch |
| + // again, with no way forward that did not involve hand-deleting |
| + // objects mid-migration. |
| + if c.pointer { |
| + switch changed, err := putOverwrite(ctx, store, c.to, body); { |
| + case err != nil: |
| + log.Fatalf("accounts migrate: write %s: %v", c.to, err) |
| + case changed: |
| + fmt.Printf("converged %s -> %s (the destination pointer disagreed)\n", c.from, c.to) |
| + default: |
| + fmt.Printf("already copied %s -> %s\n", c.from, c.to) |
| + } |
| + continue |
| + } |
| if _, err := store.Put(ctx, c.to, body, ""); err != nil { |
| cur, _, gerr := store.Get(ctx, c.to) |
| if gerr != nil { |
| log.Fatalf("accounts migrate: write %s: %v", c.to, err) |
| } |
| if string(cur) != string(body) { |
| - log.Fatalf("accounts migrate: %s already exists with different content — refusing to overwrite", c.to) |
| + log.Fatalf("accounts migrate: %s already exists with different content — refusing to\n"+ |
| + "overwrite an immutable release object. Nothing has been deleted; work out which\n"+ |
| + "of the two is the real %s before going further.", c.to, *app) |
| } |
| fmt.Printf("already copied %s -> %s\n", c.from, c.to) |
| continue |
| @@ -275,8 +297,46 @@ func runAccountsMigrate(args []string) { |
| fmt.Printf("\nThe console keeps its own app→account row: run this on the console's box to\ncomplete the move:\n\n %s\n", followUp) |
| } |
| -// copy is one object the migration moves. |
| -type copyOp struct{ from, to string } |
| +// copyOp is one object the migration moves. pointer marks a channel pointer — |
| +// the only mutable object shape in the bucket, and so the only one the |
| +// migration is allowed to overwrite. |
| +type copyOp struct { |
| + from, to string |
| + pointer bool |
| +} |
| + |
| +// isChannelPointer reports whether a key under an app prefix is a channel |
| +// pointer rather than an immutable release object. |
| +func isChannelPointer(rel string) bool { return strings.HasPrefix(rel, "channels/") } |
| + |
| +// putOverwrite writes body at key whatever is there now: read the etag, put |
| +// with it, one retry on a lost race (the CAS-guarded read-then-write the |
| +// fleet publisher and the console's manifest writer both use). Reports |
| +// whether the bytes actually changed, so the caller can say so. |
| +func putOverwrite(ctx context.Context, s release.Store, key string, body []byte) (changed bool, err error) { |
| + attempt := func() (bool, error) { |
| + etag := "" |
| + cur, cetag, gerr := s.Get(ctx, key) |
| + switch { |
| + case gerr == nil: |
| + if string(cur) == string(body) { |
| + return false, nil // already right; do not churn the etag |
| + } |
| + etag = cetag |
| + case !errors.Is(gerr, os.ErrNotExist): |
| + return false, gerr |
| + } |
| + if _, err := s.Put(ctx, key, body, etag); err != nil { |
| + return false, err |
| + } |
| + return true, nil |
| + } |
| + changed, err = attempt() |
| + if errors.Is(err, release.ErrCASConflict) { |
| + return attempt() |
| + } |
| + return changed, err |
| +} |
| // migrationPlan works out which objects to copy, and from where. This is the |
| // most dangerous decision the verb makes. |
| @@ -312,7 +372,8 @@ func migrationPlan(ctx context.Context, store release.Store, app, from, to, dst |
| } |
| var out []copyOp |
| for _, k := range keys { |
| - out = append(out, copyOp{k, dst + strings.TrimPrefix(k, src)}) |
| + rel := strings.TrimPrefix(k, src) |
| + out = append(out, copyOp{k, dst + rel, isChannelPointer(rel)}) |
| } |
| return out, from |
| } |
| @@ -348,14 +409,14 @@ func migrationPlan(ctx context.Context, store release.Store, app, from, to, dst |
| for _, k := range opsKeys { |
| rel := strings.TrimPrefix(k, opsPrefix) |
| seen[rel] = true |
| - out = append(out, copyOp{k, dst + rel}) |
| + out = append(out, copyOp{k, dst + rel, isChannelPointer(rel)}) |
| } |
| for _, k := range legacyKeys { |
| rel := strings.TrimPrefix(k, legacyPrefix) |
| if seen[rel] { |
| continue // the qualified copy is the live one |
| } |
| - out = append(out, copyOp{k, dst + rel}) |
| + out = append(out, copyOp{k, dst + rel, isChannelPointer(rel)}) |
| } |
| return out, release.OpsAccount |
| } |
| @@ -395,9 +456,28 @@ func verifyMigratedPointers(ctx context.Context, store release.Store, app, srcAc |
| log.Fatalf("accounts migrate: verify: re-read %s/%s at the source: %v", app, channel, err) |
| } |
| if string(got) != string(want) { |
| - log.Fatalf("accounts migrate: VERIFICATION FAILED for channel %s: what %s now serves is not\n"+ |
| - "what %s serves. Something wrote to this app during the migration. Do NOT delete\n"+ |
| - "anything; re-run the migration once the writer has stopped.", channel, dstAcct, srcAcct) |
| + // A write landed between the copy and this read — a promote |
| + // racing the migration. CONVERGE rather than refuse: the pointer |
| + // is this migration's own object, and telling an operator to |
| + // "re-run" while leaving a destination pointer that the create-only |
| + // copy would then reject is an instruction with no exit. |
| + log.Printf("accounts migrate: %s/%s moved during the migration — re-copying", app, channel) |
| + if _, err := putOverwrite(ctx, store, key, want); err != nil { |
| + log.Fatalf("accounts migrate: converge %s: %v", key, err) |
| + } |
| + got, _, err = store.Get(ctx, key) |
| + if err != nil { |
| + log.Fatalf("accounts migrate: verify %s: %v", key, err) |
| + } |
| + if string(got) != string(want) { |
| + // Twice in a row means something is actively writing. Name the |
| + // exact recovery rather than a vague "try again". |
| + log.Fatalf("accounts migrate: VERIFICATION FAILED for channel %s: %s still does not serve\n"+ |
| + "what %s serves after a re-copy, so something is writing to this app right now.\n"+ |
| + "Nothing has been deleted. Stop the writer, then either re-run this command or\n"+ |
| + "delete this one key and re-run:\n\n %s\n", |
| + channel, dstAcct, srcAcct, key) |
| + } |
| } |
| if pub != nil { |
| p, err := release.Channel(ctx, store, pub, dstAcct, app, channel) |
diff --git a/e2e_test.go b/e2e_test.go| index 5179c4cd7..527d91c15 100644 |
| --- a/e2e_test.go |
| +++ b/e2e_test.go |
| @@ -937,6 +937,124 @@ func TestAccountsMigrateTakesTheLiveObjects(t *testing.T) { |
| } |
| } |
| +// A destination channel pointer that disagrees with the source must CONVERGE, |
| +// not strand the operator. Channel pointers are the one mutable thing in the |
| +// bucket — they move on every promote — so a stale or half-written one at the |
| +// destination is this migration's own leftover, not foreign data, and the |
| +// verb's job is to make the destination serve what the source serves. |
| +// |
| +// The alternative shape (refuse, and tell the operator to re-run) was a dead |
| +// end: the re-run would hit the create-only copy and die at "already exists |
| +// with different content", so the instruction forbade the only exit. |
| +func TestAccountsMigrateConvergesADivergedPointer(t *testing.T) { |
| + goBin, err := exec.LookPath("go") |
| + if err != nil { |
| + t.Skip("go toolchain not found") |
| + } |
| + tmp := t.TempDir() |
| + carlosBin := filepath.Join(tmp, "carlos") |
| + build := exec.Command(goBin, "build", "-o", carlosBin, ".") |
| + build.Env = os.Environ() |
| + if out, err := build.CombinedOutput(); err != nil { |
| + t.Fatalf("build carlos: %v\n%s", err, out) |
| + } |
| + storeDir := filepath.Join(tmp, "store") |
| + regPath := filepath.Join(tmp, "registry.db") |
| + store := release.NewDirStore(storeDir) |
| + ctx := context.Background() |
| + |
| + seed, pubB64, err := release.GenerateKey() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + priv, err := release.ParseSigningKey(seed, "e2e") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + pub, err := release.ParsePublicKey(pubB64, "e2e") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + env := append(os.Environ(), |
| + "CARLOS_DEPLOYMENT_DIR="+storeDir, |
| + "CARLOS_RELEASE_PUBKEY="+pubB64) |
| + run := func(args ...string) (string, error) { |
| + t.Helper() |
| + cmd := exec.Command(carlosBin, args...) |
| + cmd.Env = env |
| + out, err := cmd.CombinedOutput() |
| + return string(out), err |
| + } |
| + |
| + for _, ver := range []string{"v1", "v2"} { |
| + m := release.Manifest{Schema: 1, App: "demo", Version: ver, Kind: "binary", |
| + Artifacts: map[string]release.Artifact{"linux-amd64": {File: "demo"}}} |
| + if err := release.Ship(ctx, store, release.OpsAccount, m, map[string][]byte{"linux-amd64": []byte(ver + " bytes")}); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if err := release.Promote(ctx, store, priv, release.OpsAccount, "demo", "stable", "v2"); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg, err := registry.Open(regPath) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + reg.Close() |
| + |
| + out, err := run("accounts", "create", "-registry", regPath, "-name", "Kass") |
| + if err != nil { |
| + t.Fatalf("accounts create: %v\n%s", err, out) |
| + } |
| + sqid := regexp.MustCompile(`(?m)^\s*sqid:\s*(\S+)`).FindStringSubmatch(out)[1] |
| + |
| + // A half-finished earlier attempt: the destination already holds this |
| + // app's releases AND a pointer at the OLD version. (Copied the way the |
| + // verb copies — same bytes, so only the POINTER diverges.) |
| + srcKeys, err := store.List(ctx, release.AppPrefix(release.OpsAccount, "demo")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, k := range srcKeys { |
| + if strings.Contains(k, "/channels/") { |
| + continue |
| + } |
| + body, _, err := store.Get(ctx, k) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + dstKey := release.AppPrefix(sqid, "demo") + strings.TrimPrefix(k, release.AppPrefix(release.OpsAccount, "demo")) |
| + if _, err := store.Put(ctx, dstKey, body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if err := release.Promote(ctx, store, priv, sqid, "demo", "stable", "v1"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if p, _ := release.Channel(ctx, store, pub, sqid, "demo", "stable"); p == nil || p.Version != "v1" { |
| + t.Fatalf("fixture: destination pointer is %+v, want v1", p) |
| + } |
| + |
| + out, err = run("accounts", "migrate", "-registry", regPath, "-app", "demo", "-to", sqid) |
| + if err != nil { |
| + t.Fatalf("migrate over a diverged destination pointer must converge, not strand:\n%s", out) |
| + } |
| + got, err := release.Channel(ctx, store, pub, sqid, "demo", "stable") |
| + if err != nil { |
| + t.Fatalf("resolve the migrated channel: %v", err) |
| + } |
| + if got.Version != "v2" { |
| + t.Fatalf("destination serves %s, want v2 — the migration did not converge the pointer", got.Version) |
| + } |
| + if !strings.Contains(out, "stable") { |
| + t.Errorf("the verb overwrote a pointer without saying so:\n%s", out) |
| + } |
| + // And it is re-runnable: that was the whole point. |
| + if out, err := run("accounts", "migrate", "-registry", regPath, "-app", "demo", "-to", sqid); err != nil { |
| + t.Fatalf("a second run must be a no-op:\n%s", out) |
| + } |
| +} |
| + |
| // TestAddUnitBackingRequiresDBForHibernate pins the I1 fix's operator-facing |
| // half: `carlos add -backing unit -hibernate` with no -db must refuse, |
| // because ops.FleetDBs (the box-wide litestream unit) has no way to |
diff --git a/internal/console/templates/app.html b/internal/console/templates/app.html| index ab359aeae..68c50820a 100644 |
| --- a/internal/console/templates/app.html |
| +++ b/internal/console/templates/app.html |
| @@ -10,6 +10,9 @@ |
| nothing. Its bucket objects were moved without the console's own record |
| following. On the console's box, run: |
| <br><code>console migrate-app -app {{.Detail.App}} -to {{.Detail.RouteAccount}}</code> |
| + <br><span class="small">If it is the routes that are in the wrong account, |
| + move those back instead — whichever of the two is right, they have to |
| + agree.</span> |
| </p> |
| {{end}} |
diff --git a/internal/release/bake.go b/internal/release/bake.go| index c604df0cf..9def2aae6 100644 |
| --- a/internal/release/bake.go |
| +++ b/internal/release/bake.go |
| @@ -89,14 +89,29 @@ type FirstSeen struct { |
| // The ops account keeps the pre-accounts key shape ("<app>/<channel>"), so |
| // every existing box's observations carry across an upgrade. Restarting them |
| // would freeze the whole fleet for 72h on the day this ships, which is a |
| -// worse failure than the one it would be protecting against. |
| +// worse failure than the one it would be protecting against. That shape is |
| +// injective on its own: an app name can never contain "/", so the first |
| +// segment is always the app and the rest is always the channel. |
| +// |
| +// Everyone else is joined with clockSep, NOT "/" — channels may contain a |
| +// "/" (canary/<branch>), so "/" between segments would make ops app "kass" on |
| +// channel "canary/x" collide with account "kass", app "canary", channel "x", |
| +// and one account's observation would satisfy another's window. clockSep |
| +// cannot appear in a sqid, an app name or a channel segment, so the split is |
| +// unambiguous — and because it can never appear in an ops key either, the two |
| +// shapes cannot collide with each other. |
| func channelClockKey(acct, app, channel string) string { |
| if acct == "" || acct == OpsAccount { |
| return app + "/" + channel |
| } |
| - return acct + "/" + app + "/" + channel |
| + return acct + clockSep + app + clockSep + channel |
| } |
| +// clockSep separates the bake clock's key segments. "|" is outside every |
| +// character class the platform allows in a sqid (lowercase letters), an app |
| +// name (appRe) or a channel segment (channelSegRe). |
| +const clockSep = "|" |
| + |
| // LoadFirstSeen reads the bake clock from path. A missing or corrupt file is |
| // an empty clock — every head then bakes from "now", which fails closed (a |
| // wiped clock delays updates; it never lets one through early). |
diff --git a/internal/release/bake_test.go b/internal/release/bake_test.go| index 883521407..1217e5734 100644 |
| --- a/internal/release/bake_test.go |
| +++ b/internal/release/bake_test.go |
| @@ -190,6 +190,31 @@ func TestBakeClockIsPerAccount(t *testing.T) { |
| } |
| } |
| +// The clock key must be injective. Channels may contain "/", so joining |
| +// segments with "/" would make ops app "kass" + channel "canary/x" collide |
| +// with account "kass" + app "canary" + channel "x" — one account's |
| +// observation silently satisfying another's window, which is the same bug |
| +// TestBakeClockIsPerAccount closes, reached by a different route. |
| +func TestBakeClockKeyIsInjective(t *testing.T) { |
| + st := LoadFirstSeen(filepath.Join(t.TempDir(), "clock.json")) |
| + long := time.Now().Add(-100 * time.Hour) |
| + |
| + // ops app "kass" on channel "canary/x". |
| + st.Observe(OpsAccount, "kass", "canary/x", "v1", long) |
| + // Account "kass", app "canary", channel "x" — three different segments |
| + // that concatenate the same way, and must NOT resolve to that record. |
| + if _, ok := st.EligibleAt("kass", "canary", "x", "v1", DefaultHolds); ok { |
| + t.Fatal("the clock key collides across (account, app, channel) boundaries") |
| + } |
| + if hold := Hold(st, "kass", "canary", "x", "v1", DefaultHolds, false, time.Now()); hold == "" { |
| + t.Fatal("a colliding key let another account skip its bake window") |
| + } |
| + // The record it was actually written under still resolves. |
| + if hold := Hold(st, OpsAccount, "kass", "canary/x", "v1", DefaultHolds, false, time.Now()); hold != "" { |
| + t.Fatalf("the real observation stopped resolving: %s", hold) |
| + } |
| +} |
| + |
| // A clock written before accounts existed has bare "<app>/<channel>" keys. |
| // Reading those as the ops account keeps every existing box's observations — |
| // otherwise the whole fleet's windows restart on upgrade, and nothing |