diff --git a/internal/server/handlers_workspaces.go b/internal/server/handlers_workspaces.go index 8f2eec947..45084be6a 100644 --- a/internal/server/handlers_workspaces.go +++ b/internal/server/handlers_workspaces.go @@ -420,10 +420,11 @@ func (s *Server) handleCreateWorkspace(w http.ResponseWriter, r *http.Request) { input.OwnerID = mint.OwnerID } - // Charged to the AUTHENTICATED owner, never to input.OwnerID: for a - // caller with no resolved user that field is whatever the body said, - // and a body must not be able to spend someone else's plan. The - // pre-check in beginWorkspaceMint charges the same identity. + // The limit is enabled only for an AUTHENTICATED owner (mint.OwnerID), + // which input.OwnerID equals by this point. For a caller with no + // resolved user input.OwnerID is whatever the body said, and a body must + // not be able to spend someone else's plan. The pre-check in + // beginWorkspaceMint makes the same decision. ws, err := s.store.CreateWorkspace(input, s.planLimitMintOpts(mint.OwnerID)...) if err != nil { if writeStorePlanLimitError(w, err, "") { diff --git a/internal/store/api_tokens.go b/internal/store/api_tokens.go index 3a330f089..8bcb20af8 100644 --- a/internal/store/api_tokens.go +++ b/internal/store/api_tokens.go @@ -20,7 +20,8 @@ import ( // CreateAPIToken mints a user-owned token. With WithPlanLimit() the insert runs // in a transaction that first takes the owner lock and counts the owner's // tokens (enforceUserLimitTx, BUG-2808), refusing with a *PlanLimitError at the -// limit. Every door that mints through here therefore enforces the same cap. +// limit. Only callers that pass the option are limited; both server token +// doors do, in cloud mode. func (s *Store) CreateAPIToken(userID string, input models.APITokenCreate, defaultExpiryDays, maxLifetimeDays int, opts ...MintOption) (*models.APITokenWithSecret, error) { // Generate 32 random bytes → 64 hex chars raw := make([]byte, 32) diff --git a/internal/store/export.go b/internal/store/export.go index 79ae6e3e6..e95d41f0e 100644 --- a/internal/store/export.go +++ b/internal/store/export.go @@ -1126,7 +1126,9 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow // at the top: the owner lock it takes is held until the commit, and an // import of a large bundle would otherwise hold it for the whole import. // The count therefore includes this transaction's own workspace row, - // which is what pendingOwn=1 accounts for. + // which is what pendingOwn=1 accounts for. CreateWorkspace takes the same + // two locks in the same order (slug row, then owner), which is what keeps + // a concurrent create and import of one name from deadlocking. if resolveMintOptions(opts).planLimit { if err := s.enforceUserLimitTx(tx, ownerID, "workspaces", 1); err != nil { return nil, err diff --git a/internal/store/limits.go b/internal/store/limits.go index 53138c300..78bb21b81 100644 --- a/internal/store/limits.go +++ b/internal/store/limits.go @@ -248,8 +248,9 @@ func resolveMintOptions(opts []MintOption) mintOptions { // committed before the lock was granted. // // pendingOwn is the number of rows THIS transaction has already inserted that -// the count will include (1 when the check runs after the insert, as -// ImportWorkspace does to keep the lock short; 0 when it runs before). +// the count will include: 1 for the workspace mints, which insert first and +// then lock (a single lock order for both, and a short hold for the import); +// 0 for CreateAPIToken, which locks before it inserts. // // A missing owner is an error, not a pass: the caller asked for a limit and // there is no plan to read it from. diff --git a/internal/store/limits_mint_race_test.go b/internal/store/limits_mint_race_test.go index b42aff2a9..7bcf475ee 100644 --- a/internal/store/limits_mint_race_test.go +++ b/internal/store/limits_mint_race_test.go @@ -229,3 +229,64 @@ func TestLimitedMint_WithoutOption_NotEnforced(t *testing.T) { t.Errorf("CreateAPIToken with the option at cap: err = %v, want *PlanLimitError", err) } } + +// Both workspace mints take their two locks in ONE order: the workspace row's +// UNIQUE slug entry, then the owner's users row (review round 2 on PR A). This +// builds the interleaving the opposite order deadlocks on. An import-shaped +// transaction has inserted slug "same-name" and not committed. A limited +// create of the same name is then observed waiting on that row. The import +// then asks for the owner lock under a short lock_timeout. If the create held +// the owner lock while it waited (the old lock-then-insert order), that request +// would wait and time out. With one order the create holds nothing, the lock +// is granted at once, and the import commits. +func TestLimitedMint_CreateAndImportOfOneName_NoLockCycle(t *testing.T) { + t.Parallel() + s := testStore(t) + if s.dialect.Driver() != DriverPostgres { + t.Skip("PAD_TEST_POSTGRES_URL not set — the cycle needs row locks, which SQLite does not have") + } + u, err := s.CreateUser(models.UserCreate{Email: "bug2808-cycle@example.com", Name: "C", Password: "pw-bug2808-12345"}) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + if err := s.SetUserPlan(u.ID, "free", ""); err != nil { + t.Fatalf("SetUserPlan: %v", err) + } + if err := s.SetUserPlanOverrides(u.ID, `{"workspaces":5}`); err != nil { + t.Fatalf("SetUserPlanOverrides: %v", err) + } + + imp, err := s.db.Begin() + if err != nil { + t.Fatalf("begin import-shaped tx: %v", err) + } + defer imp.Rollback() + if _, err := s.createWorkspaceQ(imp, models.WorkspaceCreate{Name: "same-name", OwnerID: u.ID}); err != nil { + t.Fatalf("import-shaped insert: %v", err) + } + + done := make(chan error, 1) + go func() { + _, err := s.CreateWorkspace(models.WorkspaceCreate{Name: "same-name", OwnerID: u.ID}, WithPlanLimit()) + done <- err + }() + waitForLockWait(t, s, "INSERT INTO workspaces", done) + + if _, err := imp.Exec(`SET LOCAL lock_timeout = '500ms'`); err != nil { + t.Fatalf("set lock_timeout: %v", err) + } + if _, err := imp.Exec(s.q(`SELECT id FROM users WHERE id = ? FOR NO KEY UPDATE`), u.ID); err != nil { + t.Fatalf("the import could not take the owner lock while the create waited on its slug: %v "+ + "(the create is holding the owner lock, which is the order that deadlocks)", err) + } + if err := imp.Commit(); err != nil { + t.Fatalf("commit import-shaped tx: %v", err) + } + // The create loses the slug race exactly as it did before BUG-2808 (a + // unique violation). What matters here is that it finishes. + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("the create never finished after the import committed") + } +} diff --git a/internal/store/workspaces.go b/internal/store/workspaces.go index 4524930a7..890191da3 100644 --- a/internal/store/workspaces.go +++ b/internal/store/workspaces.go @@ -89,6 +89,14 @@ type execQueryer interface { // With WithPlanLimit() the mint runs in its own transaction and is refused with // a *PlanLimitError when the owner is already at their workspaces limit, // counted under the owner lock (enforceUserLimitTx, BUG-2808). +// +// INSERT FIRST, THEN LOCK AND COUNT, the same order ImportWorkspace uses. Both +// workspace mints must take their two locks in one order: the workspace row's +// UNIQUE slug entry, then the owner's users row. Review round 2 on PR A found +// the cycle the opposite order makes on Postgres: a create holding the owner +// lock while it waits on an import's uncommitted row for the same slug, and +// the import asking for the owner lock at its end. With one order, the create +// waits on the slug holding nothing, and the import commits. func (s *Store) CreateWorkspace(input models.WorkspaceCreate, opts ...MintOption) (*models.Workspace, error) { if !resolveMintOptions(opts).planLimit { return s.createWorkspaceQ(s.db, input) @@ -98,13 +106,13 @@ func (s *Store) CreateWorkspace(input models.WorkspaceCreate, opts ...MintOption return nil, fmt.Errorf("create workspace: begin: %w", err) } defer tx.Rollback() - if err := s.enforceUserLimitTx(tx, input.OwnerID, "workspaces", 0); err != nil { - return nil, err - } ws, err := s.createWorkspaceQ(tx, input) if err != nil { return nil, err } + if err := s.enforceUserLimitTx(tx, input.OwnerID, "workspaces", 1); err != nil { + return nil, err + } if err := tx.Commit(); err != nil { return nil, fmt.Errorf("create workspace: commit: %w", err) }