amadan
console: stop the ops app backfill undoing every migration — it recreated a migrated app in ops on restart
Found in PRODUCTION, running the multi-account migration on the flagship — not in review. After `carlos accounts migrate` + `console migrate-app` moved jelly into account bac, a console restart put a jelly row back in ops from the still-present legacy grant. The same app then existed in two accounts at once, and kass and keymail were queued for the same on the next restart: every migration undone, indefinitely, by restarting the console. It was worked around by hand (duplicate row deleted, superseded grants revoked); this is the code fix. ON CONFLICT could never have caught it — the apps primary key is (account_id, app), so an ops row and a bac row for one app are two different apps by the schema's own rules. The claim is now conditioned on the name being unclaimed in ANY account, which is what the backfill always meant: take the names nothing has placed yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+92
−1
internal/console/account_test.go+69 −0internal/console/store.go+23 −1
diff --git a/internal/console/account_test.go b/internal/console/account_test.go| index 06ca4b103..d509c0939 100644 |
| --- a/internal/console/account_test.go |
| +++ b/internal/console/account_test.go |
| @@ -367,6 +367,75 @@ func TestBackfillPromotesNobodyWhenThereIsNoOperator(t *testing.T) { |
| } |
| } |
| +// FOUND IN PRODUCTION, not in review — and it was undoing the migration it |
| +// ran alongside. |
| +// |
| +// `carlos accounts migrate` + `console migrate-app` moved jelly out of ops and |
| +// into account bac. The legacy `owners` grant for jelly stays readable on |
| +// purpose (the backfill's comment says so: the grants stay while the migration |
| +// is verified). The very next console RESTART re-ran this backfill, saw a grant |
| +// for "jelly", saw no jelly row in OPS, and created one — so the same app |
| +// existed in two accounts at once, and kass and keymail were queued to get the |
| +// same treatment on the restart after that. Every migration would have been |
| +// undone, indefinitely, by the ordinary act of restarting the console. |
| +// |
| +// The rule the SQL was missing: the backfill claims a name that nothing claims |
| +// yet. An app with a row in ANY account has already been placed — by the |
| +// migration, by the console, by a previous backfill — and placing it again is |
| +// not a claim, it is a duplicate. |
| +func TestBackfillNeverRecreatesAnAppThatLivesInAnotherAccount(t *testing.T) { |
| + st := openTestStore(t) |
| + now := time.Unix(1700000000, 0) |
| + const migrated = int64(3) |
| + if err := st.CreateAccount(migrated, "bac", "Jelly", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := st.CreateAppIn(migrated, "jelly", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Exactly the live shape: the app is migrated, and its pre-accounts grant |
| + // is still sitting in the owners table waiting to be revoked. |
| + if err := st.GrantAt("dev@keymail.dev", "jelly", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + // A grant-only app that was never migrated still has to be claimed in ops — |
| + // that is the job this function exists for, and the fix must not cost it. |
| + if err := st.GrantAt("dev@keymail.dev", "toy", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if _, err := st.BackfillOpsMemberships(now.Add(time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if ok, err := st.HasAppIn(OpsAccountID, "jelly"); err != nil || ok { |
| + t.Error("the backfill recreated jelly in ops — the migration is undone on every restart") |
| + } |
| + holders, err := st.AppAccounts("jelly") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(holders) != 1 || holders[0] != migrated { |
| + t.Errorf("jelly is claimed by accounts %v, want only %d", holders, migrated) |
| + } |
| + // The surviving row is the migrated one, unchanged: same owner, same |
| + // creation time. A backfill that "fixed" it by rewriting the row would be |
| + // the same bug wearing a different hat. |
| + var createdBy string |
| + var createdAt int64 |
| + if err := st.db.QueryRow(`SELECT created_by, created_at FROM apps WHERE app = 'jelly'`). |
| + Scan(&createdBy, &createdAt); err != nil { |
| + t.Fatal(err) |
| + } |
| + if createdBy != "paul@keymail.dev" || createdAt != now.Unix() { |
| + t.Errorf("migrated jelly row = (%s, %d), want it untouched (paul@keymail.dev, %d)", |
| + createdBy, createdAt, now.Unix()) |
| + } |
| + if ok, err := st.HasAppIn(OpsAccountID, "toy"); err != nil || !ok { |
| + t.Error("a grant-only app that was never migrated lost its ops claim") |
| + } |
| +} |
| + |
| // The advice is the whole point of the report: an operator reading the log |
| // must be able to copy three commands per person, not go and design a |
| // migration. It names the apps, so nobody is left behind. |
diff --git a/internal/console/store.go b/internal/console/store.go| index 7fb7cb82f..47ba938e2 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -365,8 +365,30 @@ SELECT ?, address, ?, ? FROM users WHERE operator != 0`, |
| // app physically lives, and claiming them keeps two accounts from minting |
| // the same name before the migration moves them. `console migrate-app` is |
| // what re-homes them afterwards. |
| + // |
| + // The NOT IN is not a tidy-up — it is the fix for a bug that was live on the |
| + // flagship, and it was found by running the migration in production, not by |
| + // reading this code. `carlos accounts migrate` + `console migrate-app` moved |
| + // jelly out of ops into account bac. The migration deliberately leaves the |
| + // legacy grant readable (see the note above about not deleting the owners |
| + // table). The next console RESTART ran this statement, found a grant for |
| + // "jelly", found no jelly row in OPS — because it had just been moved out — |
| + // and inserted one. The same app then existed in two accounts at once, and |
| + // kass and keymail were lined up for the same on the following restart. The |
| + // migration was being undone, indefinitely, by restarting the console; it |
| + // took a hand-deleted row and hand-revoked grants to stop it. |
| + // |
| + // ON CONFLICT could never have caught this: the primary key is |
| + // (account_id, app), so an ops row and a bac row for the same app do not |
| + // conflict — they are, by the schema's own rules, two different apps. The |
| + // claim has to be conditioned on the app name being unclaimed EVERYWHERE, |
| + // which is what the backfill actually means: take the names nothing has |
| + // placed yet. An app that already has a row somewhere has been placed, and |
| + // the row that placed it is the authority — never this sweep. |
| if _, err := tx.Exec(`INSERT OR IGNORE INTO apps (app, account_id, created_by, created_at) |
| -SELECT app, ?, MIN(address), ? FROM owners GROUP BY app`, OpsAccountID, now.Unix()); err != nil { |
| +SELECT o.app, ?, MIN(o.address), ? FROM owners o |
| +WHERE o.app NOT IN (SELECT app FROM apps) |
| +GROUP BY o.app`, OpsAccountID, now.Unix()); err != nil { |
| return nil, err |
| } |
| // Who is left over: grant holders who are not operators and hold no ops |