From f6c007a6685dfd92aeac10f41a5c561adf90b600 Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 26 Sep 2026 09:47:46 -0400 Subject: [PATCH] fix(store): account deletion reads the owned workspaces inside its transaction under the owner lock, so a workspace minted or unjoined in the window no longer survives its deleted owner (BUG-3099) (#1567) --- .../bug3099_account_delete_owned_test.go | 118 +++++++++++++++++ internal/server/handlers_account.go | 20 +-- .../store/account_deletion_comments_test.go | 2 +- internal/store/account_deletion_test.go | 9 +- .../store/bug3099_delete_account_lock_test.go | 120 ++++++++++++++++++ internal/store/event_outbox_scrub_test.go | 2 +- .../invitation_deleted_workspace_test.go | 2 +- internal/store/users.go | 44 +++++-- 8 files changed, 286 insertions(+), 31 deletions(-) create mode 100644 internal/server/bug3099_account_delete_owned_test.go create mode 100644 internal/store/bug3099_delete_account_lock_test.go diff --git a/internal/server/bug3099_account_delete_owned_test.go b/internal/server/bug3099_account_delete_owned_test.go new file mode 100644 index 000000000..ccfbeefdc --- /dev/null +++ b/internal/server/bug3099_account_delete_owned_test.go @@ -0,0 +1,118 @@ +package server + +import ( + "net/http" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" + "github.com/PerpetualSoftware/pad/internal/store" + "github.com/PerpetualSoftware/pad/internal/store/storetest" +) + +// BUG-3099. The delete-account handler read the list of workspaces to delete +// BEFORE the deletion's transaction, and DeleteAccountAtomic soft-deleted +// only the slugs it was handed. A workspace the same account minted in +// between survived as a LIVE workspace owned by a deleted user. + +func accountDeleteServer(t *testing.T, driver store.DriverType) *Server { + t.Helper() + var s *store.Store + if driver == store.DriverPostgres { + s = storetest.NewPostgres(t) // skips when PAD_TEST_POSTGRES_URL is unset + } else { + s = storetest.NewSQLite(t) + } + if got := s.D().Driver(); got != driver { + t.Fatalf("wanted a %s store, got %s", driver, got) + } + srv := New(s) + t.Cleanup(func() { srv.Stop() }) + return srv +} + +func liveWorkspace(t *testing.T, srv *Server, id string) bool { + t.Helper() + var n int + if err := srv.store.DB().QueryRow(srv.store.D().Rebind( + `SELECT COUNT(*) FROM workspaces WHERE id = ? AND deleted_at IS NULL`), id).Scan(&n); err != nil { + t.Fatalf("read workspace %s: %v", id, err) + } + return n == 1 +} + +// TestDeleteAccount_WorkspaceMintedInTheWindowIsDeleted mints the workspace +// from inside the billing-cancel call, which the handler makes AFTER it used +// to read the owned list and BEFORE the deletion transaction: the exact +// window the filing names, reached deterministically on the real door. The +// control workspace, owned before the request, was deleted before the fix +// too; if it survives, the harness is wrong, not the fix. +func TestDeleteAccount_WorkspaceMintedInTheWindowIsDeleted(t *testing.T) { + for _, driver := range []store.DriverType{store.DriverSQLite, store.DriverPostgres} { + t.Run(string(driver), func(t *testing.T) { + srv := accountDeleteServer(t, driver) + userID, token := bootstrapAccountDeleteUser(t, srv, "cus_window") + before, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Before", Slug: "before-3099", OwnerID: userID}) + if err != nil { + t.Fatal(err) + } + if err := srv.store.AddWorkspaceMember(before.ID, userID, "owner"); err != nil { + t.Fatal(err) + } + + var minted *models.Workspace + fake := &fakeSidecar{} + fake.hook = func(string) { + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "In window", Slug: "window-3099", OwnerID: userID}) + if err != nil { + t.Errorf("mint in window: %v", err) + return + } + if err := srv.store.AddWorkspaceMember(ws.ID, userID, "owner"); err != nil { + t.Errorf("join in window: %v", err) + } + minted = ws + } + srv.SetCloudSidecar(fake) + + rr := deleteAccountReq(srv, map[string]interface{}{"password": "correct-horse-battery-staple"}, token) + if rr.Code != http.StatusOK { + t.Fatalf("delete-account: got %d: %s", rr.Code, rr.Body.String()) + } + if minted == nil { + t.Fatal("the window hook never minted; the test measured nothing") + } + if liveWorkspace(t, srv, before.ID) { + t.Fatal("control: a workspace owned before the request survived the deletion") + } + if liveWorkspace(t, srv, minted.ID) { + t.Fatal("a workspace minted between the owned-list read and the deletion survived as a live workspace of a deleted user") + } + }) + } +} + +// TestDeleteAccount_OwnedWorkspaceWithoutMembershipIsDeleted covers the +// second window the premise check found: the handler's list came from a +// MEMBERSHIP read, and CreateWorkspace commits the workspace before the +// owner's membership row is added, outside that transaction. A workspace +// that is owned but not joined was therefore invisible to the list with no +// race on the read at all. +func TestDeleteAccount_OwnedWorkspaceWithoutMembershipIsDeleted(t *testing.T) { + for _, driver := range []store.DriverType{store.DriverSQLite, store.DriverPostgres} { + t.Run(string(driver), func(t *testing.T) { + srv := accountDeleteServer(t, driver) + userID, token := bootstrapAccountDeleteUser(t, srv, "") + unjoined, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Unjoined", Slug: "unjoined-3099", OwnerID: userID}) + if err != nil { + t.Fatal(err) + } + rr := deleteAccountReq(srv, map[string]interface{}{"password": "correct-horse-battery-staple"}, token) + if rr.Code != http.StatusOK { + t.Fatalf("delete-account: got %d: %s", rr.Code, rr.Body.String()) + } + if liveWorkspace(t, srv, unjoined.ID) { + t.Fatal("an owned workspace the owner had not joined survived the deletion") + } + }) + } +} diff --git a/internal/server/handlers_account.go b/internal/server/handlers_account.go index 287b2e29a..974fb563a 100644 --- a/internal/server/handlers_account.go +++ b/internal/server/handlers_account.go @@ -80,20 +80,10 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { } } - // Delete all owned workspaces, sessions, and the user atomically. - // If any workspace deletion fails, the entire operation is aborted. - workspaces, err := s.store.GetUserWorkspaces(user.ID) - if err != nil { - writeInternalError(w, err) - return - } - - var ownedSlugs []string - for _, ws := range workspaces { - if ws.OwnerID == user.ID { - ownedSlugs = append(ownedSlugs, ws.Slug) - } - } + // Owned workspaces, sessions and the user are deleted atomically below. + // The owned set is read INSIDE that transaction, under the user-row lock + // (BUG-3099): read here, before the Stripe cancel, a workspace minted in + // between survived as a live workspace of a deleted user. // Cascade Stripe cancel BEFORE the local delete. If this ordering is // reversed, a mid-flight failure would wipe the user's StripeCustomerID @@ -128,7 +118,7 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { stripeWasCancelled = true } - if err := s.store.DeleteAccountAtomic(user.ID, ownedSlugs); err != nil { + if err := s.store.DeleteAccountAtomic(user.ID); err != nil { // Cross-system danger zone: if Stripe was already cancelled, the // user's billing is gone but their account data is still present. // Stripe cancel is NOT reversible programmatically. Operator must diff --git a/internal/store/account_deletion_comments_test.go b/internal/store/account_deletion_comments_test.go index 58ecad19e..70e7dceee 100644 --- a/internal/store/account_deletion_comments_test.go +++ b/internal/store/account_deletion_comments_test.go @@ -44,7 +44,7 @@ func TestDeleteAccountAtomic_DetachesAuthoredComments(t *testing.T) { } // Must not fail on the comments.user_id FK. - if err := s.DeleteAccountAtomic(user.ID, []string{ws.Slug}); err != nil { + if err := s.DeleteAccountAtomic(user.ID); err != nil { t.Fatalf("DeleteAccountAtomic: %v", err) } diff --git a/internal/store/account_deletion_test.go b/internal/store/account_deletion_test.go index d2eb32a18..dd737734a 100644 --- a/internal/store/account_deletion_test.go +++ b/internal/store/account_deletion_test.go @@ -36,7 +36,12 @@ func TestDeleteAccountAtomic_FullyPopulatedUser(t *testing.T) { t.Fatalf("create other user: %v", err) } - ws := createTestWorkspace(t, s, "Owned") + // OWNED means owner_id: the deletion reads its set by owner_id inside + // its transaction (BUG-3099), as the handler's list filtered on it. + ws, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "Owned", OwnerID: u.ID}) + if err != nil { + t.Fatalf("create owned workspace: %v", err) + } coll := createTestCollection(t, s, ws.ID, "Tasks") item1 := createTestItem(t, s, ws.ID, coll.ID, "Item One", "") item2 := createTestItem(t, s, ws.ID, coll.ID, "Item Two", "") @@ -151,7 +156,7 @@ func TestDeleteAccountAtomic_FullyPopulatedUser(t *testing.T) { activitiesBefore := count(`SELECT COUNT(*) FROM activities`) // The whole point: this must NOT fail on any FK. - if err := s.DeleteAccountAtomic(u.ID, []string{ws.Slug}); err != nil { + if err := s.DeleteAccountAtomic(u.ID); err != nil { t.Fatalf("DeleteAccountAtomic on fully-populated user: %v", err) } diff --git a/internal/store/bug3099_delete_account_lock_test.go b/internal/store/bug3099_delete_account_lock_test.go new file mode 100644 index 000000000..58aaf1e84 --- /dev/null +++ b/internal/store/bug3099_delete_account_lock_test.go @@ -0,0 +1,120 @@ +package store + +import ( + "os" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3099, the lock order on Postgres. DeleteAccountAtomic takes the users- +// row lock a limited workspace mint takes (enforceUserLimitTx) as its FIRST +// statement and reads the owned set under it. These tests hold a real mint +// open mid-transaction at each of its two points and drive the deletion +// against it. SQLite is not driven: BEGIN IMMEDIATE serialises every writer, +// so the interleavings below cannot arise there. + +func pgStoreFor3099(t *testing.T) *Store { + t.Helper() + url := os.Getenv("PAD_TEST_POSTGRES_URL") + if url == "" { + t.Skip("PAD_TEST_POSTGRES_URL not set; Postgres lock-order test skipped") + } + return testStorePostgres(t, url) +} + +func deleteAsync(s *Store, userID string) <-chan error { + done := make(chan error, 1) + go func() { done <- s.DeleteAccountAtomic(userID) }() + return done +} + +func liveOwned(t *testing.T, s *Store, wsID string) bool { + t.Helper() + var n int + if err := s.DB().QueryRow(s.q(`SELECT COUNT(*) FROM workspaces WHERE id = ? AND deleted_at IS NULL`), wsID).Scan(&n); err != nil { + t.Fatal(err) + } + return n == 1 +} + +// The mint holds the owner lock. The deletion must WAIT for it, and then see +// the committed workspace and delete it. Without the lock the deletion reads +// its set before the mint commits, and the workspace survives live. +func TestDeleteAccountAtomic_WaitsForAMintHoldingTheOwnerLock(t *testing.T) { + s := pgStoreFor3099(t) + u, err := s.CreateUser(models.UserCreate{Email: "m1@example.com", Name: "M", Password: "correct-horse-battery-staple"}) + if err != nil { + t.Fatal(err) + } + tx, err := s.db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + ws, err := s.createWorkspaceQ(tx, models.WorkspaceCreate{Name: "Minting", OwnerID: u.ID}) + if err != nil { + t.Fatal(err) + } + if err := s.enforceUserLimitTx(tx, u.ID, "workspaces", 1); err != nil { + t.Fatal(err) + } + + done := deleteAsync(s, u.ID) + select { + case err := <-done: + t.Fatalf("deletion finished while the mint held the owner lock (err=%v): it read its set without waiting", err) + case <-time.After(500 * time.Millisecond): + } + if err := tx.Commit(); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("deletion: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("deletion never finished after the mint committed") + } + if liveOwned(t, s, ws.ID) { + t.Fatal("the workspace the mint committed survived the deletion") + } +} + +// The mint has inserted its row but not yet taken the lock. The deletion must +// NOT wait on it (the mint holds nothing on users: no deadlock), and the mint +// must then fail, finding no owner, so no workspace outlives its owner. +func TestDeleteAccountAtomic_DoesNotWaitOnAMintBeforeItsLock(t *testing.T) { + s := pgStoreFor3099(t) + u, err := s.CreateUser(models.UserCreate{Email: "m2@example.com", Name: "M", Password: "correct-horse-battery-staple"}) + if err != nil { + t.Fatal(err) + } + tx, err := s.db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + ws, err := s.createWorkspaceQ(tx, models.WorkspaceCreate{Name: "Minting", OwnerID: u.ID}) + if err != nil { + t.Fatal(err) + } + + select { + case err := <-deleteAsync(s, u.ID): + if err != nil { + t.Fatalf("deletion: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("deletion blocked on a mint that has not taken the owner lock") + } + if err := s.enforceUserLimitTx(tx, u.ID, "workspaces", 1); err == nil { + t.Fatal("the mint took the owner lock of a deleted user") + } + _ = tx.Rollback() + if liveOwned(t, s, ws.ID) { + t.Fatal("a workspace outlived its owner") + } +} diff --git a/internal/store/event_outbox_scrub_test.go b/internal/store/event_outbox_scrub_test.go index cd97aac0f..c95d07e48 100644 --- a/internal/store/event_outbox_scrub_test.go +++ b/internal/store/event_outbox_scrub_test.go @@ -526,7 +526,7 @@ func TestDeleteAccountAtomic_ScrubsOutbox(t *testing.T) { return s.emitMemberEventTx(tx, kernelevents.MemberJoined, ws.ID, bystander.ID, "viewer", now()) }) - if err := s.DeleteAccountAtomic(deleted.ID, nil); err != nil { + if err := s.DeleteAccountAtomic(deleted.ID); err != nil { t.Fatalf("DeleteAccountAtomic: %v", err) } diff --git a/internal/store/invitation_deleted_workspace_test.go b/internal/store/invitation_deleted_workspace_test.go index 75adcab28..98b5321de 100644 --- a/internal/store/invitation_deleted_workspace_test.go +++ b/internal/store/invitation_deleted_workspace_test.go @@ -70,7 +70,7 @@ func TestGetInvitationByCode_SoftDeletedWorkspace(t *testing.T) { t.Fatalf("CONTROL: a live workspace's invitation must be found; got %+v", before) } - if err := s.DeleteAccountAtomic(owner.ID, []string{ws.Slug}); err != nil { + if err := s.DeleteAccountAtomic(owner.ID); err != nil { t.Fatalf("DeleteAccountAtomic: %v", err) } if got, err := s.GetWorkspaceByID(ws.ID); err != nil || got != nil { diff --git a/internal/store/users.go b/internal/store/users.go index a45228dc6..e83c90021 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -967,7 +967,7 @@ func (s *Store) DeleteUser(id string) error { // (ON DELETE CASCADE); items.assigned_user_id and activities.user_id // (ON DELETE SET NULL; activities gained its FK action in migrations // 072/050). -func (s *Store) DeleteAccountAtomic(userID string, ownedWorkspaceSlugs []string) error { +func (s *Store) DeleteAccountAtomic(userID string) error { tx, err := s.db.Begin() if err != nil { return fmt.Errorf("delete account: begin tx: %w", err) @@ -976,16 +976,38 @@ func (s *Store) DeleteAccountAtomic(userID string, ownedWorkspaceSlugs []string) ts := now() - // 1. Soft-delete all owned workspaces. They keep their rows (and every - // item/activity/comment within) so the data stays recoverable; only the - // user identity is hard-removed below. - for _, slug := range ownedWorkspaceSlugs { - if _, err := tx.Exec(s.q(` - UPDATE workspaces SET deleted_at = ?, updated_at = ? - WHERE slug = ? AND deleted_at IS NULL - `), ts, ts, slug); err != nil { - return fmt.Errorf("delete account: delete workspace %s: %w", slug, err) - } + // 0. Lock the user's row FIRST, with the lock every limited mint of a + // user-scoped row takes (enforceUserLimitTx, BUG-2808). The owned set + // below is read under it, so it is exact (BUG-3099): a mint that + // committed before the lock is in the set, and one still open waits + // here, then finds no user row when this commits and rolls back. The + // set used to be read by the HANDLER before this transaction began, and + // from memberships, so a workspace minted in between, or owned but not + // yet joined, survived as a live workspace of a deleted user. On SQLite + // the transaction is already BEGIN IMMEDIATE, which serialises every + // writer, and the row-locking clause would be a syntax error there. + // + // Deadlock-free against the mint because the mint's only write before + // its own lock is the workspaces row, which takes no lock on users (no + // foreign key from workspaces.owner_id on Postgres): it holds nothing + // this transaction waits for. + lock := `SELECT id FROM users WHERE id = ?` + if s.dialect.Driver() == DriverPostgres { + lock += ` FOR NO KEY UPDATE` + } + var lockedID string + if err := tx.QueryRow(s.q(lock), userID).Scan(&lockedID); err != nil { + return fmt.Errorf("delete account: lock user: %w", err) + } + + // 1. Soft-delete every workspace the user OWNS. They keep their rows (and + // every item/activity/comment within) so the data stays recoverable; only + // the user identity is hard-removed below. + if _, err := tx.Exec(s.q(` + UPDATE workspaces SET deleted_at = ?, updated_at = ? + WHERE owner_id = ? AND deleted_at IS NULL + `), ts, ts, userID); err != nil { + return fmt.Errorf("delete account: delete owned workspaces: %w", err) } // exec runs one cleanup statement keyed on userID, wrapping the error with