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)

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")
+			}
+		})
+	}
+}

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

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)
 	}
 

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)
 	}
 

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")
+	}
+}

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)
 	}
 

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 {

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