2026-05-11 05:39:19 +00:00
4 changed files with 775 additions and 4 deletions
--- 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
--- a/workspace-server/internal/handlers/workspace_create_name.go
+++ 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)
+}
--- a/workspace-server/internal/handlers/workspace_create_name_integration_test.go
+++ 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 "<prefix>-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 = "<prefix>-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)
+	}
+}
--- a/workspace-server/internal/handlers/workspace_create_name_test.go
+++ 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
+}