diff --git a/workspace-server/internal/handlers/workspace.go b/workspace-server/internal/handlers/workspace.go index 2c033561..bfccb092 100644 --- a/workspace-server/internal/handlers/workspace.go +++ b/workspace-server/internal/handlers/workspace.go @@ -8,6 +8,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "log" "net/http" @@ -285,17 +286,51 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "delivery_mode must be 'push' or 'poll'"}) return } - // Insert workspace with runtime + delivery_mode persisted in DB (inside transaction) - _, err := tx.ExecContext(ctx, ` + // Insert workspace with runtime + delivery_mode persisted in DB (inside transaction). + // + // Auto-suffix on (parent_id, name) collision via insertWorkspaceWithNameRetry: + // the partial-unique index `workspaces_parent_name_uniq` (migration + // 20260506000000) protects /org/import from TOCTOU duplicates, but the + // pre-fix Canvas Create path bubbled the raw pq violation as a 500 on + // double-click. Helper retries with " (2)", " (3)", … up to maxNameSuffix, + // returns the actually-persisted name (which we MUST thread back into + // payload + broadcast so the canvas displays what the DB has). + const insertWorkspaceSQL = ` INSERT INTO workspaces (id, name, role, tier, runtime, awareness_namespace, status, parent_id, workspace_dir, workspace_access, budget_limit, max_concurrent_tasks, delivery_mode) VALUES ($1, $2, $3, $4, $5, $6, 'provisioning', $7, $8, $9, $10, $11, $12) - `, id, payload.Name, role, payload.Tier, payload.Runtime, awarenessNamespace, payload.ParentID, workspaceDir, workspaceAccess, payload.BudgetLimit, maxConcurrent, deliveryMode) + ` + insertArgs := []any{id, payload.Name, role, payload.Tier, payload.Runtime, awarenessNamespace, payload.ParentID, workspaceDir, workspaceAccess, payload.BudgetLimit, maxConcurrent, deliveryMode} + persistedName, currentTx, err := insertWorkspaceWithNameRetry( + ctx, + tx, + // Closure captures ctx so the retry tx uses the same request context; + // nil opts mirrors the original BeginTx call above. + func(ctx context.Context) (*sql.Tx, error) { return db.DB.BeginTx(ctx, nil) }, + payload.Name, + 1, // args[1] is name + insertWorkspaceSQL, + insertArgs, + ) if err != nil { - tx.Rollback() //nolint:errcheck + if currentTx != nil { + currentTx.Rollback() //nolint:errcheck + } + if errors.Is(err, errWorkspaceNameExhausted) { + log.Printf("Create workspace: name suffix exhausted for base %q under parent %v", payload.Name, payload.ParentID) + c.JSON(http.StatusConflict, gin.H{"error": "workspace name already in use; please pick a different name"}) + return + } log.Printf("Create workspace error: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to create workspace"}) return } + // Helper may have rolled back the original tx and returned a fresh one; + // rebind so the remaining secrets-INSERT + Commit run on the live tx. + tx = currentTx + if persistedName != payload.Name { + log.Printf("Create workspace %s: name collision auto-suffix %q -> %q", id, payload.Name, persistedName) + payload.Name = persistedName + } // Persist initial secrets from the create payload (inside same transaction). // nil/empty map is a no-op. Any failure rolls back the workspace insert diff --git a/workspace-server/internal/handlers/workspace_create_name.go b/workspace-server/internal/handlers/workspace_create_name.go new file mode 100644 index 00000000..7638724c --- /dev/null +++ b/workspace-server/internal/handlers/workspace_create_name.go @@ -0,0 +1,183 @@ +package handlers + +// workspace_create_name.go — disambiguate workspace names on the +// Canvas POST /workspaces path so a double-clicked template card +// does not surface raw Postgres errors. +// +// Background (#2872 + post-2026-05-06 follow-up): +// - Migration 20260506000000_workspaces_unique_parent_name added a +// partial UNIQUE index on (COALESCE(parent_id, sentinel), name) +// WHERE status != 'removed'. It exists to close the TOCTOU race in +// /org/import that previously let two concurrent POSTs both INSERT +// the same (parent_id, name) row. +// - /org/import handles the constraint via `ON CONFLICT DO NOTHING` +// + idempotent re-select (handlers/org_import.go). +// - The Canvas Create handler (handlers/workspace.go) did NOT — a +// duplicate POST returned an opaque HTTP 500 with the raw pq error +// in the server log. Repro path: user clicks a template card twice +// in canvas before the first response paints. +// +// Resolution: auto-suffix the user-typed name on collision. The +// uniqueness constraint required for #2872 stays in place; only the +// Canvas Create path's reaction to it changes. Names become a +// free-form display label that the platform disambiguates; row +// identity is carried by the workspace id (UUID). +// +// Suffix shape: " (2)", " (3)", … up to N=maxNameSuffix. Chosen over +// numeric "-2" / "_2" because the parenthesised form is the standard +// disambiguation pattern users already expect from Finder / Explorer +// / Google Docs / file managers. Stays under the 255-char name cap +// (#688 — validated by validateWorkspaceFields) for any reasonable +// base name; parens are not in yamlSpecialChars so the existing YAML- +// safety guard is unaffected. + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" + + "github.com/lib/pq" +) + +// maxNameSuffix bounds the suffix-retry loop. 20 is well above any +// plausible accidental-double-click rate (typical: 2-3 races) and +// keeps the worst-case handler latency to ~20 round-trips. If a +// caller actually wants 21+ workspaces with the same base name, they +// can pre-disambiguate client-side; the platform refuses to spin +// indefinitely. +const maxNameSuffix = 20 + +// workspacesUniqueIndexName is the partial-unique index this handler +// is reacting to. Pinned to the migration's index name so we +// distinguish "the base name collision we know how to handle" from +// every other unique violation (which we surface as 409 without +// retry — silently auto-suffixing a name on the wrong constraint +// would mask real bugs). +const workspacesUniqueIndexName = "workspaces_parent_name_uniq" + +// errWorkspaceNameExhausted is returned when maxNameSuffix retries +// all fail because every candidate name in the (base, " (2)", …, +// " (N)") sequence is taken. The caller maps this to HTTP 409 +// Conflict — the user must rename and re-try. +var errWorkspaceNameExhausted = errors.New("workspace name exhausted: too many duplicates of base name under same parent") + +// dbExec is the minimum surface our retry helper needs from +// *sql.Tx (or *sql.DB). Declared as an interface so tests can +// substitute a fake without standing up a real DB connection. +type dbExec interface { + ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error) +} + +// insertWorkspaceWithNameRetry runs the workspace INSERT and, if it +// hits the parent-name unique-violation, retries with a suffixed +// name. Returns the name actually persisted (which the caller MUST +// use in the response and in broadcast payloads — without it the +// canvas would show the user-typed name while the DB has the +// suffixed one, and the next poll would surprise the user with the +// "real" name). +// +// The query string is intentionally a parameter (not hardcoded) so +// the helper composes with future schema additions without growing +// a new arity each time. Only the FIRST arg of args must be the +// name placeholder ($1) — the helper rewrites args[0] on retry; all +// other args pass through verbatim. (This matches the workspace.go +// INSERT below where $1 is the id and $2 is name, so the caller +// passes nameArgIndex=1.) +// +// On the unique-violation, the original tx is rolled back and a +// fresh one is begun before retry — Postgres marks the tx aborted +// on any error, so re-using it would silently no-op every +// subsequent statement. +// +// `beginTx` is a closure (not a *sql.DB) so the caller controls the +// transaction-options + the context. Returning the fresh tx each +// retry means the caller can commit it once the helper succeeds. +// +// `query` MUST be parameterized — the name placeholder is rewritten +// via args[nameArgIndex], not via string substitution. Passing a +// fmt.Sprintf'd query string would silently disable the safety. +func insertWorkspaceWithNameRetry( + ctx context.Context, + tx *sql.Tx, + beginTx func(ctx context.Context) (*sql.Tx, error), + baseName string, + nameArgIndex int, + query string, + args []any, +) (finalName string, finalTx *sql.Tx, err error) { + if nameArgIndex < 0 || nameArgIndex >= len(args) { + return "", tx, fmt.Errorf("insertWorkspaceWithNameRetry: nameArgIndex %d out of range for %d args", nameArgIndex, len(args)) + } + + current := tx + for attempt := 0; attempt <= maxNameSuffix; attempt++ { + candidate := baseName + if attempt > 0 { + candidate = fmt.Sprintf("%s (%d)", baseName, attempt+1) + } + args[nameArgIndex] = candidate + _, execErr := current.ExecContext(ctx, query, args...) + if execErr == nil { + return candidate, current, nil + } + if !isParentNameUniqueViolation(execErr) { + // Any other error (encoding, connection, FK violation, + // other unique index) — return as-is. Caller decides + // status code. + return "", current, execErr + } + // Hit the partial-unique index. Postgres has aborted this + // tx — roll it back and start fresh before retrying with a + // new candidate name. + _ = current.Rollback() + if attempt == maxNameSuffix { + break + } + next, txErr := beginTx(ctx) + if txErr != nil { + return "", nil, fmt.Errorf("begin retry tx after name collision: %w", txErr) + } + current = next + } + // Exhausted: the helper rolled back the last tx already. Return + // nil tx so the caller does not try to commit/rollback again. + return "", nil, errWorkspaceNameExhausted +} + +// isParentNameUniqueViolation reports whether err is the specific +// partial-unique-index violation we know how to auto-suffix. We pin +// on BOTH the SQLSTATE 23505 (unique_violation) AND the constraint +// name so we don't silently rename around an unrelated unique index +// (e.g. a future workspaces.slug unique). +// +// errors.As is used (not a `.(*pq.Error)` type assertion) because +// lib/pq wraps the error through fmt.Errorf in some paths. +// +// Defensive fallback: if Constraint is empty (older pq builds, or +// the error came through a wrapper that dropped the field), match +// on the error message as well. The message form is brittle +// (postgres locale-dependent) but every English-locale Postgres +// emits the index name verbatim. +func isParentNameUniqueViolation(err error) bool { + if err == nil { + return false + } + var pqErr *pq.Error + if errors.As(err, &pqErr) { + if pqErr.Code != "23505" { + return false + } + if pqErr.Constraint == workspacesUniqueIndexName { + return true + } + // Fallback for builds that drop Constraint metadata. + return strings.Contains(pqErr.Message, workspacesUniqueIndexName) + } + // Last-resort string match — the pq.Error type was lost + // through wrapping. Same English-locale caveat as above; keeps + // the helper robust in test seams that synthesize errors via + // fmt.Errorf("pq: …"). + return strings.Contains(err.Error(), workspacesUniqueIndexName) +} diff --git a/workspace-server/internal/handlers/workspace_create_name_integration_test.go b/workspace-server/internal/handlers/workspace_create_name_integration_test.go new file mode 100644 index 00000000..7866a359 --- /dev/null +++ b/workspace-server/internal/handlers/workspace_create_name_integration_test.go @@ -0,0 +1,251 @@ +//go:build integration +// +build integration + +// workspace_create_name_integration_test.go — REAL Postgres +// integration test for the duplicate-name auto-suffix retry +// helper. +// +// Run with: +// +// INTEGRATION_DB_URL="postgres://postgres:test@localhost:55432/molecule?sslmode=disable" \ +// go test -tags=integration ./internal/handlers/ -run Integration_WorkspaceCreate_NameRetry -v +// +// CI: piggybacks on .github/workflows/handlers-postgres-integration.yml +// (path-filter includes workspace-server/internal/handlers/**, which +// covers this file). +// +// Why this is NOT a sqlmock test +// ------------------------------ +// sqlmock CANNOT verify the actual partial-unique-index +// behaviour. The unit tests in workspace_create_name_test.go pin +// the helper's retry contract under a fake driver error, but only +// a real Postgres can confirm: +// +// - The migration 20260506000000 actually created the index. +// - lib/pq emits SQLSTATE 23505 with Constraint = +// "workspaces_parent_name_uniq" (not a synonym, not the message +// fallback). +// - The COALESCE(parent_id, sentinel) target collapses NULL +// parent_ids so two root-level workspaces with the same name +// collide as the migration intends. +// - The WHERE status != 'removed' partial filter exempts +// tombstoned rows from blocking re-use. +// +// Per feedback_mandatory_local_e2e_before_ship: ship-mode requires +// the helper to be exercised against a real Postgres before the PR +// merges. + +package handlers + +import ( + "context" + "database/sql" + "fmt" + "os" + "testing" + + "github.com/google/uuid" + _ "github.com/lib/pq" +) + +// integrationDB_WorkspaceCreateName opens $INTEGRATION_DB_URL, +// applies the parent-name partial unique index if missing +// (idempotent), wipes the test row range, and returns the +// connection. +// +// We intentionally do NOT wipe every row in `workspaces` because +// the integration DB may be shared with other tests in this +// package; we tag inserts with a per-test UUID prefix and clean up +// only those. +func integrationDB_WorkspaceCreateName(t *testing.T) *sql.DB { + t.Helper() + url := os.Getenv("INTEGRATION_DB_URL") + if url == "" { + t.Skip("INTEGRATION_DB_URL not set; skipping (see file header)") + } + conn, err := sql.Open("postgres", url) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := conn.Ping(); err != nil { + t.Fatalf("ping: %v", err) + } + t.Cleanup(func() { conn.Close() }) + + // Ensure the constraint we're testing exists. If the migration + // already ran (the dev/CI default), this is a fast no-op via + // IF NOT EXISTS. If the test DB was created from a snapshot + // taken before 2026-05-06, we apply it here. + if _, err := conn.ExecContext(context.Background(), ` + CREATE UNIQUE INDEX IF NOT EXISTS workspaces_parent_name_uniq + ON workspaces ( + COALESCE(parent_id, '00000000-0000-0000-0000-000000000000'::uuid), + name + ) + WHERE status != 'removed' + `); err != nil { + t.Fatalf("ensure constraint: %v", err) + } + return conn +} + +// cleanupTestRows removes any rows inserted under the given name +// prefix. Called via t.Cleanup so a failing test still leaves the +// DB usable for the next run. +func cleanupTestRows(t *testing.T, conn *sql.DB, namePrefix string) { + t.Helper() + if _, err := conn.ExecContext(context.Background(), + `DELETE FROM workspaces WHERE name LIKE $1`, namePrefix+"%"); err != nil { + t.Logf("cleanup (non-fatal): %v", err) + } +} + +// TestIntegration_WorkspaceCreate_NameRetry_AutoSuffixesOnCollision +// exercises the helper end-to-end against a real Postgres: +// +// 1. INSERT a row with name "-Repro" — succeeds. +// 2. Run insertWorkspaceWithNameRetry with the same name — +// partial-unique violation fires, helper retries with +// " (2)", that succeeds. +// 3. SELECT the row by id, confirm name = "-Repro (2)". +// 4. Run helper AGAIN — second collision, helper retries with +// " (3)". +// +// This is the live-test that proves the partial-index behaviour +// matches the migration's intent — sqlmock cannot reach this depth. +func TestIntegration_WorkspaceCreate_NameRetry_AutoSuffixesOnCollision(t *testing.T) { + conn := integrationDB_WorkspaceCreateName(t) + ctx := context.Background() + + // Per-test prefix so concurrent test runs don't collide on the + // shared integration DB; also tags rows for cleanupTestRows. + prefix := fmt.Sprintf("itest-namesuffix-%s", uuid.New().String()[:8]) + t.Cleanup(func() { cleanupTestRows(t, conn, prefix) }) + + baseName := prefix + "-Repro" + + // Step 1 — seed an existing row to collide against. Uses a + // minimal column set (the production INSERT has many more + // columns; we only need the ones the partial-unique index + // targets + the NOT NULL columns required by the schema). + firstID := uuid.New().String() + if _, err := conn.ExecContext(ctx, ` + INSERT INTO workspaces (id, name, tier, runtime, awareness_namespace, status) + VALUES ($1, $2, 2, 'claude-code', $3, 'provisioning') + `, firstID, baseName, "workspace:"+firstID); err != nil { + t.Fatalf("seed first row: %v", err) + } + + // Step 2 — same name, helper must auto-suffix to " (2)". + beginTx := func(ctx context.Context) (*sql.Tx, error) { return conn.BeginTx(ctx, nil) } + + tx, err := beginTx(ctx) + if err != nil { + t.Fatalf("begin tx: %v", err) + } + secondID := uuid.New().String() + query := ` + INSERT INTO workspaces (id, name, tier, runtime, awareness_namespace, status) + VALUES ($1, $2, 2, 'claude-code', $3, 'provisioning') + ` + args := []any{secondID, baseName, "workspace:" + secondID} + persistedName, finalTx, err := insertWorkspaceWithNameRetry( + ctx, tx, beginTx, baseName, 1, query, args, + ) + if err != nil { + t.Fatalf("retry helper on second insert: %v", err) + } + if persistedName != baseName+" (2)" { + t.Fatalf("persistedName = %q, want exactly %q", persistedName, baseName+" (2)") + } + if err := finalTx.Commit(); err != nil { + t.Fatalf("commit second: %v", err) + } + + // Step 3 — verify DB state matches helper's return value. + var actualName string + if err := conn.QueryRowContext(ctx, + `SELECT name FROM workspaces WHERE id = $1`, secondID).Scan(&actualName); err != nil { + t.Fatalf("re-select second: %v", err) + } + if actualName != baseName+" (2)" { + t.Fatalf("DB row name = %q, want exactly %q (helper return value lied to caller)", + actualName, baseName+" (2)") + } + + // Step 4 — third collision must produce " (3)". + tx3, err := beginTx(ctx) + if err != nil { + t.Fatalf("begin tx3: %v", err) + } + thirdID := uuid.New().String() + args3 := []any{thirdID, baseName, "workspace:" + thirdID} + persistedName3, finalTx3, err := insertWorkspaceWithNameRetry( + ctx, tx3, beginTx, baseName, 1, query, args3, + ) + if err != nil { + t.Fatalf("retry helper on third insert: %v", err) + } + if persistedName3 != baseName+" (3)" { + t.Fatalf("third persistedName = %q, want exactly %q", + persistedName3, baseName+" (3)") + } + if err := finalTx3.Commit(); err != nil { + t.Fatalf("commit third: %v", err) + } +} + +// TestIntegration_WorkspaceCreate_NameRetry_TombstonedRowDoesNotCollide +// confirms the partial-index `WHERE status != 'removed'` predicate +// matches the helper's assumptions: a deleted (status='removed') +// workspace MUST NOT block re-creation under the same name. +// +// This is the post-2026-05-06 contract /org/import already relies +// on; the helper inherits it for the Canvas Create path. A +// regression in the migration's predicate would silently break +// both surfaces. +func TestIntegration_WorkspaceCreate_NameRetry_TombstonedRowDoesNotCollide(t *testing.T) { + conn := integrationDB_WorkspaceCreateName(t) + ctx := context.Background() + + prefix := fmt.Sprintf("itest-tombstone-%s", uuid.New().String()[:8]) + t.Cleanup(func() { cleanupTestRows(t, conn, prefix) }) + + baseName := prefix + "-RevivedName" + + // Seed a row, then tombstone it. + firstID := uuid.New().String() + if _, err := conn.ExecContext(ctx, ` + INSERT INTO workspaces (id, name, tier, runtime, awareness_namespace, status) + VALUES ($1, $2, 2, 'claude-code', $3, 'removed') + `, firstID, baseName, "workspace:"+firstID); err != nil { + t.Fatalf("seed tombstoned row: %v", err) + } + + // New INSERT with the same name MUST succeed without any + // suffix — the partial index excludes the tombstoned row. + beginTx := func(ctx context.Context) (*sql.Tx, error) { return conn.BeginTx(ctx, nil) } + tx, err := beginTx(ctx) + if err != nil { + t.Fatalf("begin tx: %v", err) + } + secondID := uuid.New().String() + query := ` + INSERT INTO workspaces (id, name, tier, runtime, awareness_namespace, status) + VALUES ($1, $2, 2, 'claude-code', $3, 'provisioning') + ` + args := []any{secondID, baseName, "workspace:" + secondID} + persistedName, finalTx, err := insertWorkspaceWithNameRetry( + ctx, tx, beginTx, baseName, 1, query, args, + ) + if err != nil { + t.Fatalf("retry helper after tombstone: %v", err) + } + if persistedName != baseName { + t.Fatalf("persistedName = %q, want %q (tombstoned row should NOT force a suffix)", + persistedName, baseName) + } + if err := finalTx.Commit(); err != nil { + t.Fatalf("commit: %v", err) + } +} diff --git a/workspace-server/internal/handlers/workspace_create_name_test.go b/workspace-server/internal/handlers/workspace_create_name_test.go new file mode 100644 index 00000000..6fc711df --- /dev/null +++ b/workspace-server/internal/handlers/workspace_create_name_test.go @@ -0,0 +1,302 @@ +package handlers + +// workspace_create_name_test.go — unit + table tests for the +// duplicate-name auto-suffix retry helper. +// +// Phase 3 of the dev-SOP: write the test first, watch it fail in +// the way you predicted, then watch the fix make it pass. The fix +// landed in workspace_create_name.go; these tests pin its contract +// so a refactor that drops the retry (or auto-suffixes on the +// WRONG constraint) blows up loud. +// +// sqlmock CANNOT verify the real partial-index behaviour — that +// lives in the companion integration test +// workspace_create_name_integration_test.go (real Postgres). + +import ( + "context" + "database/sql" + "errors" + "fmt" + "strings" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/db" + "github.com/lib/pq" +) + +// fakePqUniqueViolation reproduces the SQLSTATE/Constraint shape +// the real lib/pq driver emits when an INSERT hits +// workspaces_parent_name_uniq. Used by the unit test to drive the +// retry path without standing up a real Postgres. +func fakePqUniqueViolation(constraint string) error { + return &pq.Error{ + Code: "23505", + Constraint: constraint, + Message: fmt.Sprintf("duplicate key value violates unique constraint %q", constraint), + } +} + +// TestIsParentNameUniqueViolation_PinsTheConstraint exhaustively +// pins which error shapes the helper considers "auto-suffix +// eligible." A regression that broadens this predicate (e.g. +// matching ANY 23505) would mask real bugs; a regression that +// narrows it (e.g. dropping the message fallback) would let the +// 500-on-double-click bug recur on driver builds that strip +// Constraint metadata. +func TestIsParentNameUniqueViolation_PinsTheConstraint(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil error", nil, false}, + {"plain string error", errors.New("network down"), false}, + { + name: "23505 on parent_name_uniq via pq.Error", + err: fakePqUniqueViolation("workspaces_parent_name_uniq"), + want: true, + }, + { + name: "23505 on a DIFFERENT unique index — must NOT be auto-suffixed", + err: fakePqUniqueViolation("workspaces_slug_uniq"), + want: false, + }, + { + name: "23505 with empty Constraint — fall back to message match", + err: &pq.Error{ + Code: "23505", + Message: `duplicate key value violates unique constraint "workspaces_parent_name_uniq"`, + }, + want: true, + }, + { + name: "non-23505 (e.g. FK violation) on the same index name in message — must NOT match", + err: &pq.Error{ + Code: "23503", + Message: `foreign key references workspaces_parent_name_uniq region`, + }, + want: false, + }, + { + name: "wrapped via fmt.Errorf (errors.As must unwrap)", + err: fmt.Errorf("create workspace: %w", fakePqUniqueViolation("workspaces_parent_name_uniq")), + want: true, + }, + { + name: "raw string from a non-pq error mentioning the index — last-resort fallback", + err: errors.New(`pq: duplicate key value violates unique constraint "workspaces_parent_name_uniq"`), + want: true, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + got := isParentNameUniqueViolation(tc.err) + if got != tc.want { + t.Fatalf("isParentNameUniqueViolation(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// TestInsertWorkspaceWithNameRetry_FirstAttemptSucceeds confirms +// the helper does NOT modify the name when the first INSERT +// succeeds — a naive implementation that always wraps in a retry +// loop could accidentally add a " (1)" suffix even on the happy +// path. +func TestInsertWorkspaceWithNameRetry_FirstAttemptSucceeds(t *testing.T) { + mock := setupTestDB(t) + + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs("id-1", "MyWorkspace"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + tx, err := getDBHandle(t).BeginTx(context.Background(), nil) + if err != nil { + t.Fatalf("begin: %v", err) + } + + name, finalTx, err := insertWorkspaceWithNameRetry( + context.Background(), + tx, + func(ctx context.Context) (*sql.Tx, error) { + return getDBHandle(t).BeginTx(ctx, nil) + }, + "MyWorkspace", + 1, + "INSERT INTO workspaces (id, name) VALUES ($1, $2)", + []any{"id-1", "MyWorkspace"}, + ) + if err != nil { + t.Fatalf("retry helper: %v", err) + } + if name != "MyWorkspace" { + t.Fatalf("name = %q, want %q (happy path must NOT suffix)", name, "MyWorkspace") + } + if finalTx == nil { + t.Fatalf("finalTx == nil; caller needs a live tx to commit") + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + +// TestInsertWorkspaceWithNameRetry_SecondAttemptSuffixed confirms +// that on a single collision the helper retries with " (2)" and +// returns that as the persisted name. The dispatched-name suffix +// shape is part of the user-visible contract — if a future +// refactor switches to "-2" / "_2" / "MyWorkspace2", the canvas +// renders the wrong label until the next poll. +func TestInsertWorkspaceWithNameRetry_SecondAttemptSuffixed(t *testing.T) { + mock := setupTestDB(t) + + // First begin (caller-owned), then first INSERT fails with the + // partial-unique violation, helper rolls back the tx, opens a + // fresh tx, and the second INSERT (with " (2)") succeeds. + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs("id-1", "MyWorkspace"). + WillReturnError(fakePqUniqueViolation("workspaces_parent_name_uniq")) + mock.ExpectRollback() + mock.ExpectBegin() + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs("id-1", "MyWorkspace (2)"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + tx, err := getDBHandle(t).BeginTx(context.Background(), nil) + if err != nil { + t.Fatalf("begin: %v", err) + } + + name, finalTx, err := insertWorkspaceWithNameRetry( + context.Background(), + tx, + func(ctx context.Context) (*sql.Tx, error) { + return getDBHandle(t).BeginTx(ctx, nil) + }, + "MyWorkspace", + 1, + "INSERT INTO workspaces (id, name) VALUES ($1, $2)", + []any{"id-1", "MyWorkspace"}, + ) + if err != nil { + t.Fatalf("retry helper: %v", err) + } + // Exact-equality assertion (per feedback_assert_exact_not_substring): + // substring-match on "MyWorkspace" would also pass for the bug case + // where the helper accidentally returns "MyWorkspace (1)" or + // "MyWorkspace2". + if name != "MyWorkspace (2)" { + t.Fatalf("name = %q, want exactly %q", name, "MyWorkspace (2)") + } + if finalTx == nil { + t.Fatalf("finalTx == nil after successful retry") + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + +// TestInsertWorkspaceWithNameRetry_NonRetryableErrorPassesThrough +// pins that we do NOT retry on errors we don't recognize. A +// connection drop, an FK violation, a check-constraint failure +// must propagate verbatim — the helper is NOT a generic +// SQL-retry wrapper. +func TestInsertWorkspaceWithNameRetry_NonRetryableErrorPassesThrough(t *testing.T) { + mock := setupTestDB(t) + + mock.ExpectBegin() + connErr := errors.New("connection reset by peer") + mock.ExpectExec("INSERT INTO workspaces"). + WithArgs("id-1", "MyWorkspace"). + WillReturnError(connErr) + + tx, err := getDBHandle(t).BeginTx(context.Background(), nil) + if err != nil { + t.Fatalf("begin: %v", err) + } + + name, _, err := insertWorkspaceWithNameRetry( + context.Background(), + tx, + func(ctx context.Context) (*sql.Tx, error) { + return getDBHandle(t).BeginTx(ctx, nil) + }, + "MyWorkspace", + 1, + "INSERT INTO workspaces (id, name) VALUES ($1, $2)", + []any{"id-1", "MyWorkspace"}, + ) + if err == nil { + t.Fatalf("expected error, got nil (name=%q)", name) + } + if !errors.Is(err, connErr) && !strings.Contains(err.Error(), "connection reset") { + t.Fatalf("expected connection-reset to propagate, got %v", err) + } + if name != "" { + t.Fatalf("name = %q, want empty on failure", name) + } +} + +// TestInsertWorkspaceWithNameRetry_ExhaustsAfterMaxSuffix pins the +// upper bound: after maxNameSuffix retries the helper returns +// errWorkspaceNameExhausted so the caller maps it to 409 Conflict +// rather than spinning indefinitely. +func TestInsertWorkspaceWithNameRetry_ExhaustsAfterMaxSuffix(t *testing.T) { + mock := setupTestDB(t) + + // Every attempt collides. Expect maxNameSuffix+1 INSERTs (the + // initial + maxNameSuffix retries), each followed by a Rollback, + // and a Begin between rollbacks except the final terminal one. + mock.ExpectBegin() + for i := 0; i <= maxNameSuffix; i++ { + mock.ExpectExec("INSERT INTO workspaces"). + WillReturnError(fakePqUniqueViolation("workspaces_parent_name_uniq")) + mock.ExpectRollback() + if i < maxNameSuffix { + mock.ExpectBegin() + } + } + + tx, err := getDBHandle(t).BeginTx(context.Background(), nil) + if err != nil { + t.Fatalf("begin: %v", err) + } + + _, finalTx, err := insertWorkspaceWithNameRetry( + context.Background(), + tx, + func(ctx context.Context) (*sql.Tx, error) { + return getDBHandle(t).BeginTx(ctx, nil) + }, + "MyWorkspace", + 1, + "INSERT INTO workspaces (id, name) VALUES ($1, $2)", + []any{"id-1", "MyWorkspace"}, + ) + if !errors.Is(err, errWorkspaceNameExhausted) { + t.Fatalf("err = %v, want errWorkspaceNameExhausted", err) + } + if finalTx != nil { + t.Fatalf("finalTx must be nil on exhaustion (helper already rolled back); got %v", finalTx) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + +// getDBHandle exposes the package-level db.DB the test infrastructure +// stashes after setupTestDB. Kept as a helper so the test reads as +// the production code does ("BeginTx on the platform's DB") without +// the cross-package import noise. +func getDBHandle(t *testing.T) *sql.DB { + t.Helper() + // db.DB is the package-level handle; setupTestDB assigns it to + // the sqlmock-backed *sql.DB. Use this helper everywhere instead + // of dereferencing db.DB directly so a future move to a per-test + // container fixture has one rename surface. + return db.DB +}