From cf324741dc1f54b06c8cc2d07214b7ad15bb1bb7 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 28 Sep 2026 20:55:28 -0400 Subject: [PATCH] fix(store): delete issued grants before tabs in account deletion (BUG-3288) (#1654) * fix(store): delete issued grants before tabs in account deletion (BUG-3288) A revoke of a grant that removes the grantee's last access to a workspace takes the grant row, then the grantee's tab row there. DeleteAccountAtomic deleted the tabs of its owned workspaces first and the grants it issued later, so a revoke of one of those grants racing the granter's account deletion deadlocked on Postgres (40P01). The deletion now deletes the issued grants first, so both transactions take the grant row first. delete_account_writer_lockorder_test.go probes eight writers against an account deletion: a free race, plus two constructed interleavings that park the deletion mid-transaction. Both grant-revoke legs were red before this change in all three modes. The deleting user as a share-link viewer fails on a foreign key instead (23503), which is BUG-3289 and is not in this probe. Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9 * test(store): the probe's writers must succeed or lose one declared way (BUG-3288) Codex round 1: the parked harness accepted any non-deadlock writer error, so a writer that failed before reaching the contested rows passed. Each writer now declares the one error it may lose with (measured on Postgres), and the writer check no longer forgives sql.ErrNoRows. Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9 * test(store): match the probe's writer losses on sentinels, not wrapper text (BUG-3288) Codex round 2: the grantee-side revoke's allowance matched "lock user", which a deadlock taking that lock also carries. Losses are now predicates: sql.ErrNoRows for a row the deletion removed, and for the assignment leg the membership refusal or an FK violation. The free race also accepts sql.ErrNoRows, since the deletion can commit before the writer reads its row (always on SQLite, which the previous head failed on in make test); the parked legs stay strict. Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9 --- .../delete_account_writer_lockorder_test.go | 309 ++++++++++++++++++ internal/store/users.go | 19 +- internal/store/workspace_tabs.go | 5 +- 3 files changed, 330 insertions(+), 3 deletions(-) create mode 100644 internal/store/delete_account_writer_lockorder_test.go diff --git a/internal/store/delete_account_writer_lockorder_test.go b/internal/store/delete_account_writer_lockorder_test.go new file mode 100644 index 000000000..d0793d29e --- /dev/null +++ b/internal/store/delete_account_writer_lockorder_test.go @@ -0,0 +1,309 @@ +package store + +import ( + "database/sql" + "errors" + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// An account deletion racing an ordinary writer that touches rows the +// deletion also writes (BUG-3288). The advisory lock in DeleteAccountAtomic +// serialises deletions against each other only; every writer here is outside +// it. One subtest per writer, so a red names the writer. Only Postgres +// discriminates (40P01); SQLite serialises every writer. +// +// The deleting user as a share-link VIEWER is not here: that race fails the +// deletion on a foreign key rather than deadlocking, and is BUG-3289. +// +// The deleting user is a. u is another user, a guest or member of a's +// workspace wa, with a tab on it; wu is a workspace u owns, where a is a +// member. +func TestDeleteAccountAtomic_ConcurrentWriters(t *testing.T) { + t.Parallel() + + type world struct { + a, u *models.User + wa, wu string + ca, cu string + ia, iu string // ia is authored by a + gCollA string // collection grant on wa issued by a to u + gItemA string // item grant on wa issued by a to u + gCollU string // collection grant on wu issued by u to a + linkA string // share link on ia created by a + linkU string // share link on iu created by u + } + + writers := []struct { + name string + // access is how u reaches wa. A revoke prunes u's tab on wa only + // when it removes u's LAST access there, so a grant writer's u holds + // that one grant and nothing else. + access string + // loses reports the one error the writer may return in a parked + // leg, because the deletion removed something it needed first + // (measured on Postgres). nil means it must succeed. Any other + // error fails the leg, so a writer that failed before reaching the + // contested rows cannot pass. + loses func(err error) bool + run func(s *Store, w world) error + }{ + {"collection grant revoke (issued by the deleting user)", accessCollGrant, nil, func(s *Store, w world) error { + return s.DeleteCollectionGrant(w.gCollA, w.wa) + }}, + {"item grant revoke (issued by the deleting user)", accessItemGrant, nil, func(s *Store, w world) error { + return s.DeleteItemGrant(w.gItemA, w.wa) + }}, + {"collection grant revoke (the deleting user is the grantee)", accessMember, userGone, func(s *Store, w world) error { + return s.DeleteCollectionGrant(w.gCollU, w.wu) + }}, + {"member removal from the deleting user's workspace", accessMember, nil, func(s *Store, w world) error { + return s.RemoveWorkspaceMember(w.wa, w.u.ID) + }}, + {"share link view of the deleting user's link", accessMember, nil, func(s *Store, w world) error { + _, err := s.RecordShareLinkView(w.linkA, "fp-u", w.u.ID, nil) + return err + }}, + {"assign the deleting user's item to another user", accessMember, nil, func(s *Store, w world) error { + uid := w.u.ID + _, err := s.UpdateItem(w.ia, models.ItemUpdate{AssignedUserID: &uid}) + return err + }}, + {"assign an item to the deleting user", accessMember, assigneeGone, func(s *Store, w world) error { + aid := w.a.ID + _, err := s.UpdateItem(w.iu, models.ItemUpdate{AssignedUserID: &aid}) + return err + }}, + } + + seed := func(t *testing.T, s *Store, tag string, access string) world { + t.Helper() + a := tabsUser(t, s, tag+"a") + u := tabsUser(t, s, tag+"u") + wa := tabsWorkspace(t, s, a, tag+"WA") + wu := tabsWorkspace(t, s, u, tag+"WU") + if access == accessMember { + if err := s.AddWorkspaceMember(wa.ID, u.ID, "editor"); err != nil { + t.Fatal(err) + } + } + if err := s.AddWorkspaceMember(wu.ID, a.ID, "editor"); err != nil { + t.Fatal(err) + } + ca := createTestCollection(t, s, wa.ID, tag+"CA") + cu := createTestCollection(t, s, wu.ID, tag+"CU") + ia := createTestItem(t, s, wa.ID, ca.ID, tag+" item a", "") + iu := createTestItem(t, s, wu.ID, cu.ID, tag+" item u", "") + if _, err := s.db.Exec(s.q(`UPDATE items SET created_by_user_id = ? WHERE id = ?`), a.ID, ia.ID); err != nil { + t.Fatal(err) + } + w := world{a: a, u: u, wa: wa.ID, wu: wu.ID, ca: ca.ID, cu: cu.ID, ia: ia.ID, iu: iu.ID} + if access != accessItemGrant { + gc, err := s.CreateCollectionGrant(wa.ID, ca.ID, u.ID, "view", a.ID) + if err != nil { + t.Fatal(err) + } + w.gCollA = gc.ID + } + if access != accessCollGrant { + gi, err := s.CreateItemGrant(wa.ID, ia.ID, u.ID, "view", a.ID) + if err != nil { + t.Fatal(err) + } + w.gItemA = gi.ID + } + gcu, err := s.CreateCollectionGrant(wu.ID, cu.ID, a.ID, "view", u.ID) + if err != nil { + t.Fatal(err) + } + w.gCollU = gcu.ID + la, err := s.CreateShareLink(wa.ID, "item", ia.ID, "view", a.ID, nil) + if err != nil { + t.Fatal(err) + } + w.linkA = la.ID + lu, err := s.CreateShareLink(wu.ID, "item", iu.ID, "view", u.ID, nil) + if err != nil { + t.Fatal(err) + } + w.linkU = lu.ID + for _, tab := range [][2]string{{u.ID, wa.ID}, {u.ID, wu.ID}, {a.ID, wa.ID}, {a.ID, wu.ID}} { + if _, err := s.OpenWorkspaceTab(tab[0], tab[1], false); err != nil { + t.Fatal(err) + } + } + if _, err := s.CreateSession(a.ID, "", "", "", time.Hour); err != nil { + t.Fatal(err) + } + return w + } + + check := func(t *testing.T, s *Store, what string, loses func(error) bool, w world, delErr, wrErr error) { + t.Helper() + failOnDeadlockOrError(t, what+": account deletion", delErr) + // Not failOnDeadlockOrError: it forgives sql.ErrNoRows, which is + // what a writer that never reached the contested rows returns. + if wrErr != nil && (loses == nil || !loses(wrErr)) { + t.Fatalf("%s: writer: %v", what, wrErr) + } + if got, err := s.GetUser(w.a.ID); err == nil && got != nil { + t.Fatalf("%s: user survived its account deletion", what) + } + } + + // A free race: both start together. It rarely lands the writer inside + // the deletion's transaction, so on its own it is weak evidence; the + // parked modes below construct those interleavings instead. + t.Run("race", func(t *testing.T) { + t.Parallel() + for i, wr := range writers { + i, wr := i, wr + t.Run(wr.name, func(t *testing.T) { + t.Parallel() + s := testStore(t) + for round := 0; round < 16; round++ { + w := seed(t, s, fmt.Sprintf("Wr%dr%d", i, round), wr.access) + var wg sync.WaitGroup + start := make(chan struct{}) + var delErr, wrErr error + wg.Add(2) + go func() { + defer wg.Done() + <-start + delErr = s.DeleteAccountAtomic(w.a.ID) + }() + go func() { + defer wg.Done() + <-start + wrErr = wr.run(s, w) + }() + close(start) + wg.Wait() + // In a free race the whole deletion can also commit + // before the writer reads its row (always, on SQLite), + // which is an honest loss too. + loses := func(err error) bool { + return userGone(err) || (wr.loses != nil && wr.loses(err)) + } + check(t, s, fmt.Sprintf("%s round %d", wr.name, round), loses, w, delErr, wrErr) + } + }) + } + }) + + // Parked: a third transaction holds a row the deletion needs next, so + // the deletion stops mid-transaction holding everything it has written + // so far. The writer then runs, and the holder lets go. Postgres only: + // SQLite's BEGIN IMMEDIATE would block the deletion before its first + // statement. + parks := []struct { + name string + needle string // the deletion's statement that waits on the holder + hold string // the holder's lock, bound to one id from the world + id func(w world) string + }{ + // Waits in step 2, after step 1 deleted the tabs of a's workspaces. + {"parked after tabs", "SET created_by_user_id = NULL", `SELECT id FROM items WHERE id = $1 FOR UPDATE`, func(w world) string { return w.ia }}, + // Waits in step 3, after step 2 de-identified a's rows. + {"parked after de-identify", "DELETE FROM sessions WHERE user_id", `SELECT id FROM sessions WHERE user_id = $1 FOR UPDATE`, func(w world) string { return w.a.ID }}, + } + for pi, park := range parks { + pi, park := pi, park + t.Run(park.name, func(t *testing.T) { + t.Parallel() + for i, wr := range writers { + i, wr := i, wr + t.Run(wr.name, func(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("Postgres only: the park needs row locks") + } + w := seed(t, s, fmt.Sprintf("Pk%dw%d", pi, i), wr.access) + holder, err := s.db.Begin() + if err != nil { + t.Fatal(err) + } + defer holder.Rollback() + if _, err := holder.Exec(park.hold, park.id(w)); err != nil { + t.Fatal(err) + } + delDone := make(chan error, 1) + go func() { delDone <- s.DeleteAccountAtomic(w.a.ID) }() + waitForLockWait(t, s, park.needle, delDone) + + wrDone := make(chan error, 1) + go func() { wrDone <- wr.run(s, w) }() + // The writer either finishes against the parked deletion, + // or queues behind something the deletion (or the holder) + // holds. The count names the writer without its pid: this + // database is the test's own, the holder is idle in its + // transaction, and the deletion is the one waiter already. + var wrErr error + wrFinished := false + deadline := time.Now().Add(10 * time.Second) + for !wrFinished && lockWaiters(t, s) < 2 { + select { + case wrErr = <-wrDone: + wrFinished = true + case <-time.After(20 * time.Millisecond): + } + if time.Now().After(deadline) { + t.Fatal("the writer neither finished nor waited on a lock") + } + } + if err := holder.Rollback(); err != nil { + t.Fatal(err) + } + delErr := <-delDone + if !wrFinished { + wrErr = <-wrDone + } + check(t, s, fmt.Sprintf("%s, %s", park.name, wr.name), wr.loses, w, delErr, wrErr) + }) + } + }) + } +} + +// How the other user reaches the deleting user's workspace in +// TestDeleteAccountAtomic_ConcurrentWriters. +const ( + accessMember = "member" // plus one grant of each kind + accessCollGrant = "collection grant" + accessItemGrant = "item grant" +) + +// userGone is a writer's loss when a row it reads first (its user, its +// grant) was deleted by the account deletion. Matched on the sentinel, not on +// message text: a deadlock taking the same lock carries the same wrapper. +func userGone(err error) bool { return errors.Is(err, sql.ErrNoRows) } + +// assigneeGone is an assignment naming a user the deletion removed: the +// store's membership check refuses it when the deletion committed first, and +// the foreign key refuses it when the deletion commits in between. +func assigneeGone(err error) bool { + msg := err.Error() + return strings.Contains(msg, "assigned user is not a member of this workspace") || + strings.Contains(msg, "SQLSTATE 23503") || strings.Contains(msg, "FOREIGN KEY constraint failed") +} + +// lockWaiters counts this test database's backends waiting on a lock. +func lockWaiters(t *testing.T, s *Store) int { + t.Helper() + var n int + if err := s.db.QueryRow(` + SELECT COUNT(*) FROM pg_stat_activity + WHERE pid <> pg_backend_pid() + AND datname = current_database() + AND state = 'active' + AND wait_event_type = 'Lock'`).Scan(&n); err != nil { + t.Fatalf("poll pg_stat_activity: %v", err) + } + return n +} diff --git a/internal/store/users.go b/internal/store/users.go index 3363ecdeb..5cd5739fe 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -1019,6 +1019,23 @@ func (s *Store) DeleteAccountAtomic(userID string) error { return fmt.Errorf("delete account: lock user: %w", err) } + // 0b. Delete the grants the user issued BEFORE the tabs below (BUG-3288). + // A revoke of such a grant that removes the grantee's last access to one + // of these workspaces takes the grant row and then the grantee's tab row + // there, which is the row step 1 deletes. Deleting the grants after the + // tabs took the two rows in the opposite order, and the two transactions + // deadlocked (40P01, delete_account_writer_lockorder_test.go). Now both + // take the grant row first, so whichever gets it second waits holding + // nothing the other needs. + for _, stmt := range []struct{ what, query string }{ + {"delete issued collection grants", "DELETE FROM collection_grants WHERE granted_by = ?"}, + {"delete issued item grants", "DELETE FROM item_grants WHERE granted_by = ?"}, + } { + if _, err := tx.Exec(s.q(stmt.query), userID); err != nil { + return fmt.Errorf("delete account: %s: %w", stmt.what, 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. @@ -1098,8 +1115,6 @@ func (s *Store) DeleteAccountAtomic(userID string) error { {"delete sent invitations", "DELETE FROM workspace_invitations WHERE invited_by = ?"}, {"delete password reset tokens", "DELETE FROM password_reset_tokens WHERE user_id = ?"}, {"delete email verification tokens", "DELETE FROM email_verification_tokens WHERE user_id = ?"}, - {"delete issued collection grants", "DELETE FROM collection_grants WHERE granted_by = ?"}, - {"delete issued item grants", "DELETE FROM item_grants WHERE granted_by = ?"}, {"delete created share links", "DELETE FROM share_links WHERE created_by = ?"}, {"delete mcp audit log", "DELETE FROM mcp_audit_log WHERE user_id = ?"}, {"delete oauth connections", "DELETE FROM oauth_connections WHERE user_id = ?"}, diff --git a/internal/store/workspace_tabs.go b/internal/store/workspace_tabs.go index 9efab00fb..c59cf495d 100644 --- a/internal/store/workspace_tabs.go +++ b/internal/store/workspace_tabs.go @@ -448,7 +448,10 @@ func (s *Store) pruneWorkspaceTabIfNoAccessTx(ex execer, userID, workspaceID str // NO KEY UPDATE on the workspaces row does not block. // - A prune caller (member removal, grant revoke): users(U) before its // first write. -// - Account deletion: users(D), then D's owned workspaces rows. A soft +// - Account deletion: users(D), then the grant rows D issued, then D's +// owned workspaces rows and the tabs on them. A grant revoke prunes a +// tab after deleting its grant row, so the grants go first here too +// (BUG-3288: taken after the tabs, the two deadlocked). A soft // delete that took the workspaces row first and D's users row second // would cross it (measured: 40P01, // TestWorkspaceTabs_SoftDeleteVsOwnerAccountDeletion, codex round 3),