fix(store): mint the imported workspace inside the import transaction (BUG-2892) (#1259)

ImportWorkspace called CreateWorkspace — its own committed write — before
opening the transaction that carries every other row. Eight error returns sit
between that INSERT and the commit (begin, de-duplicate declarations, import
collection, item slug-after-truncation, import item, remap item, import
comment, and the commit itself), and each returned an error while leaving the
workspace row behind: named, slugged, owned by the caller, holding no
collections and no items.

The husk was not only clutter. uniqueWorkspaceSlug probes
`WHERE slug = ? AND deleted_at IS NULL`, and a husk is not soft-deleted, so it
kept the slug: an operator who fixed the bundle and retried landed on `name-2`.
Measured on the unfixed build — the retry leg returns `retry-slug-2`. The
attempt that stored nothing took the name from the one that worked.

CreateWorkspace is now a thin wrapper over createWorkspaceQ, which takes the
caller's executor, and ImportWorkspace opens its transaction first and mints on
it. Both of createWorkspaceQ's reads — the slug probe and the read-back — take
that executor too, which is load-bearing rather than tidy: a read routed
through the pool while the caller's transaction holds its connection can wait
for a free one, and under MaxOpenConns(1) there is none (BUG-2778 / BUG-2409).
workspaces.slug has been globally UNIQUE since 001_initial, so the constraint
still covers the race the in-transaction probe cannot.

The filing said this needed a look rather than a one-liner because
CreateWorkspace "does more than one INSERT (owner membership, seeding hooks)".
That premise was wrong, and reading the function is what retired it: it does
one INSERT. Owner membership and template seeding are both handler-level, and
neither is on the import path inside the store — handleImportWorkspace calls
AddWorkspaceMember after ImportWorkspace returns, and import never seeds
because the bundle carries the collections.

Tests: two legs, both red on the unfixed build for the reasons named above and
green here. The negative fixture is a bundle whose two collections share a slug
and differ in everything else, so it fails for exactly one reason, asserted on
by message.

CONVE-23 sweep: two comments said the workspace "survives as a husk" when the
de-duplication rolls an import back (export.go's trait-dedupe note and
TestImportDeduplicatesAConflictingArchive). Both were true when written and are
now false in that clause only; corrected without disturbing the argument they
were making, which is unchanged.

CONVE-18 sweep: the class is a committed pool write followed by a transaction
whose failure orphans it. One instance across the 64 non-test files of
internal/store — this one. The one other hit is a false positive
(CreateItemLink's early `return s.SetParentLink(...)`, a mutually exclusive
path). Search boundary: internal/store non-test files, by syntax; the
instrument recognises `s.db.Exec(` and s.<write-verb> calls, does not model
control flow, and does not follow writes into helpers named otherwise. It was
run against the pre-fix export.go as a positive control and fired on exactly
the defect.

Claude-Session: https://claude.ai/code/session_01HeChkgZVYb3NTgTcckF5KR

internal/store/export.go

@@ -366,7 +366,33 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow
 		wsSlug = newName
 	}
 
-	ws, err := s.CreateWorkspace(models.WorkspaceCreate{
+	// Run all data inserts in a single transaction for atomicity — INCLUDING
+	// the workspace row itself (BUG-2892).
+	//
+	// The workspace used to be minted first, by a CreateWorkspace call that
+	// committed on its own. Eight error returns sit between that write and the
+	// commit below (begin, de-duplicate declarations, import collection, item
+	// slug-after-truncation, import item, remap item, import comment, and the
+	// commit itself), and every one of them returned an error while leaving the
+	// workspace row behind: named, slugged, owned by the caller, holding no
+	// collections and no items.
+	//
+	// The husk was not only clutter. uniqueWorkspaceSlug probes
+	// `WHERE slug = ? AND deleted_at IS NULL`, and a husk is not soft-deleted,
+	// so it kept the slug: an operator who fixed the bundle and retried landed
+	// on `name-2`, and that slug is in every URL for the workspace afterwards.
+	// The attempt that stored nothing took the name from the one that worked.
+	tx, err := s.db.Begin()
+	if err != nil {
+		return nil, fmt.Errorf("begin transaction: %w", err)
+	}
+	defer tx.Rollback()
+
+	// createWorkspaceQ rather than CreateWorkspace: the slug probe and the
+	// read-back must run on THIS transaction, both so they see its own
+	// uncommitted row and so neither reaches for the pool while the
+	// transaction holds its connection (BUG-2778's deadlock class).
+	ws, err := s.createWorkspaceQ(tx, models.WorkspaceCreate{
 		Name:        wsName,
 		Slug:        wsSlug,
 		Description: data.Workspace.Description,
@@ -377,13 +403,6 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow
 		return nil, fmt.Errorf("create workspace: %w", err)
 	}
 
-	// Run all data inserts in a single transaction for atomicity
-	tx, err := s.db.Begin()
-	if err != nil {
-		return nil, fmt.Errorf("begin transaction: %w", err)
-	}
-	defer tx.Rollback()
-
 	// ID mapping: old ID -> new ID
 	collMap := make(map[string]string)
 	itemMap := make(map[string]string)
@@ -404,11 +423,17 @@ func (s *Store) ImportWorkspace(data *models.WorkspaceExport, newName string, ow
 	//
 	// Import used to warn and insert both, which was right while nothing
 	// forbade the pair. With the partial unique indexes the second INSERT is
-	// refused, the whole transaction rolls back, and the workspace minted
-	// before it survives as a husk ([[BUG-2892]]) — so an archive carrying a
+	// refused and the whole transaction rolls back — so an archive carrying a
 	// duplicate would be unimportable, and archives carrying duplicates are
 	// exactly the ones this release repairs.
 	//
+	// That rollback now takes the workspace row with it (BUG-2892); when this
+	// paragraph was written the workspace was minted before the transaction
+	// and survived the failure as a husk, which made the same archive both
+	// unimportable AND a source of empty workspaces. The de-duplication is
+	// what keeps it importable; the transaction boundary only decides how
+	// cleanly the other failures fail.
+	//
 	// THE CHECK HAPPENS IN THE INSERT LOOP, ON THE FINAL BYTES, and that
 	// placement is the fix for codex round 3's P1 rather than a style choice.
 	// An earlier version pre-computed the strips from the traits the BUNDLE

internal/store/import_workspace_husk_test.go

@@ -0,0 +1,122 @@
+package store
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/PerpetualSoftware/pad/internal/models"
+)
+
+// BUG-2892. ImportWorkspace minted the workspace through CreateWorkspace —
+// its own committed write — BEFORE opening the transaction that carries every
+// other row. Eight error returns sit between that INSERT and the commit, and
+// each one left the workspace row behind: named, slugged, owned by the caller,
+// carrying no collections and no items.
+//
+// The second-order consequence is the user-visible one. uniqueWorkspaceSlug
+// probes `WHERE slug = ? AND deleted_at IS NULL`, and a husk is not
+// soft-deleted, so it HOLDS the slug: the retry that finally succeeds lands on
+// `name-2`, and that slug is in every URL for the workspace from then on.
+
+// failingImportBundle builds a bundle that imports successfully up to its
+// second collection and then fails there, at
+// `INSERT INTO collections ... UNIQUE(workspace_id, slug)`.
+//
+// The two collections share a SLUG and differ in every other respect, so the
+// bundle has exactly one thing wrong with it. That matters: a fixture
+// rejectable for two reasons cannot tell you which one the run hit, and this
+// one is asserted on by message below.
+func failingImportBundle(name, slug string) *models.WorkspaceExport {
+	return &models.WorkspaceExport{
+		Version:    1,
+		ExportedAt: "2026-09-06T00:00:00Z",
+		Workspace: models.WorkspaceExportMeta{
+			Name:     name,
+			Slug:     slug,
+			Settings: "{}",
+		},
+		Collections: []models.CollectionExport{
+			{
+				ID:        "coll-first",
+				Name:      "First",
+				Slug:      "duplicated",
+				Prefix:    "FIR",
+				Schema:    `{"fields":[]}`,
+				Settings:  "{}",
+				CreatedAt: "2026-09-06T00:00:00Z",
+				UpdatedAt: "2026-09-06T00:00:00Z",
+			},
+			{
+				ID:        "coll-second",
+				Name:      "Second",
+				Slug:      "duplicated",
+				Prefix:    "SEC",
+				Schema:    `{"fields":[]}`,
+				Settings:  "{}",
+				CreatedAt: "2026-09-06T00:00:00Z",
+				UpdatedAt: "2026-09-06T00:00:00Z",
+			},
+		},
+	}
+}
+
+// validImportBundle is failingImportBundle with the slug collision removed and
+// nothing else changed, so a retry differs from the failed attempt in exactly
+// the way the operator's fix would.
+func validImportBundle(name, slug string) *models.WorkspaceExport {
+	b := failingImportBundle(name, slug)
+	b.Collections[1].Slug = "distinct"
+	return b
+}
+
+// TestImportWorkspaceFailureLeavesNoWorkspace is the first leg: a failed
+// import leaves no workspace row at all.
+//
+// The counterfactual matters here — `GetWorkspaceBySlug` returning nil would
+// also be satisfied by an import that never got as far as creating anything,
+// so the test first pins that the failure is the one it engineered (the
+// collection INSERT, not a decode or a version check) and that the import did
+// fail rather than quietly succeeding.
+func TestImportWorkspaceFailureLeavesNoWorkspace(t *testing.T) {
+	s := testStore(t)
+
+	ws, err := s.ImportWorkspace(failingImportBundle("Husk Probe", "husk-probe"), "", "")
+	if err == nil {
+		t.Fatalf("ImportWorkspace succeeded on a bundle with two collections sharing a slug; got workspace %+v", ws)
+	}
+	if !strings.Contains(err.Error(), "import collection") {
+		t.Fatalf("bundle failed for the wrong reason — want an `import collection` failure, got: %v", err)
+	}
+
+	got, gerr := s.GetWorkspaceBySlug("husk-probe")
+	if gerr != nil {
+		t.Fatalf("GetWorkspaceBySlug: %v", gerr)
+	}
+	if got != nil {
+		t.Fatalf("failed import left a workspace behind: slug=%q id=%s", got.Slug, got.ID)
+	}
+}
+
+// TestImportWorkspaceRetryKeepsOriginalSlug is the second leg, and the
+// user-visible one: after a failed import, fixing the bundle and retrying
+// lands on the ORIGINAL slug.
+//
+// On the unfixed build the husk from the first attempt still holds
+// `retry-slug`, so uniqueWorkspaceSlug hands the successful import
+// `retry-slug-2` — a degraded slug in every URL, caused by an attempt that
+// stored nothing.
+func TestImportWorkspaceRetryKeepsOriginalSlug(t *testing.T) {
+	s := testStore(t)
+
+	if _, err := s.ImportWorkspace(failingImportBundle("Retry Probe", "retry-slug"), "", ""); err == nil {
+		t.Fatal("ImportWorkspace succeeded on the deliberately-broken bundle; the retry leg proves nothing")
+	}
+
+	ws, err := s.ImportWorkspace(validImportBundle("Retry Probe", "retry-slug"), "", "")
+	if err != nil {
+		t.Fatalf("retry with the corrected bundle failed: %v", err)
+	}
+	if ws.Slug != "retry-slug" {
+		t.Fatalf("retry landed on a degraded slug: got %q, want %q — the failed attempt is still holding the original", ws.Slug, "retry-slug")
+	}
+}

internal/store/trait_dedupe_test.go

@@ -363,10 +363,13 @@ func TestMalformedTraitsDoNotBreakTheInvariant(t *testing.T) {
 
 // TestImportDeduplicatesAConflictingArchive covers TASK-2710 item 4. Before
 // the indexes, import warned about a duplicate declaration and inserted both.
-// With them the second INSERT is refused, the whole transaction rolls back and
-// the workspace minted beforehand survives as a husk — so an archive carrying
-// a duplicate would become unimportable, and those archives are exactly the
-// ones this release exists to repair.
+// With them the second INSERT is refused and the whole transaction rolls back
+// — so an archive carrying a duplicate would become unimportable, and those
+// archives are exactly the ones this release exists to repair.
+//
+// The rollback used to leave the workspace behind as well, because it was
+// minted before the transaction opened; BUG-2892 moved it inside. That changed
+// what a failure COSTS, not whether this de-duplication is needed.
 func TestImportDeduplicatesAConflictingArchive(t *testing.T) {
 	s := testStore(t)
 	owner := createTestUser(t, s, "importdedupe@test.com", "Owner", "password123")

internal/store/workspaces.go

@@ -65,7 +65,35 @@ func effectiveWorkspaceUpdatedAt(workspaceUpdatedAt string, lastItemActivity sql
 	return wsTS
 }
 
+// execQueryer is the subset of *sql.DB / *sql.Tx that createWorkspaceQ needs.
+// Minting a workspace is one Exec between two reads — the slug probe before
+// it and the read-back after — so it needs both halves, unlike the read-only
+// Queryer and the write-only sqlExecer that already exist here.
+type execQueryer interface {
+	sqlExecer
+	rowQueryer
+}
+
+// CreateWorkspace mints a workspace against the pool. Callers already inside a
+// transaction want createWorkspaceQ instead.
 func (s *Store) CreateWorkspace(input models.WorkspaceCreate) (*models.Workspace, error) {
+	return s.createWorkspaceQ(s.db, input)
+}
+
+// createWorkspaceQ is CreateWorkspace against a caller-supplied executor.
+//
+// It exists for ImportWorkspace (BUG-2892), which has to mint the workspace on
+// the SAME transaction that carries the collections and items: minting it
+// first as a committed write left the row behind on every one of the eight
+// error paths between there and the commit, and the husk went on holding the
+// slug, so the retry that finally worked landed on `name-2`.
+//
+// Both reads take the caller's executor rather than reaching for s.db, and
+// that is load-bearing rather than tidy. A read routed through the POOL while
+// the caller's transaction holds its own connection can wait for a free one,
+// and under MaxOpenConns(1) there is none to wait for — the deadlock class
+// BUG-2778 and BUG-2409 are both instances of.
+func (s *Store) createWorkspaceQ(q execQueryer, input models.WorkspaceCreate) (*models.Workspace, error) {
 	id := newID()
 	ts := now()
 
@@ -77,7 +105,7 @@ func (s *Store) CreateWorkspace(input models.WorkspaceCreate) (*models.Workspace
 	// Workspace slugs are globally unique (not scoped to a workspace
 	// like collection/item slugs), so we use a workspace-specific
 	// uniqueness check rather than the generic uniqueSlug helper.
-	finalSlug, err := s.uniqueWorkspaceSlug(slug)
+	finalSlug, err := s.uniqueWorkspaceSlug(q, slug)
 	if err != nil {
 		return nil, err
 	}
@@ -97,7 +125,7 @@ func (s *Store) CreateWorkspace(input models.WorkspaceCreate) (*models.Workspace
 		}
 	}
 
-	_, err = s.db.Exec(s.q(`
+	_, err = q.Exec(s.q(`
 		INSERT INTO workspaces (id, name, slug, owner_id, description, settings, source, created_at, updated_at)
 		VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
 	`), id, input.Name, finalSlug, input.OwnerID, input.Description, settings, input.Source, ts, ts)
@@ -105,14 +133,17 @@ func (s *Store) CreateWorkspace(input models.WorkspaceCreate) (*models.Workspace
 		return nil, fmt.Errorf("insert workspace: %w", err)
 	}
 
-	return s.GetWorkspaceBySlug(finalSlug)
+	return s.getWorkspaceBySlugQ(q, finalSlug)
 }
 
-func (s *Store) uniqueWorkspaceSlug(baseSlug string) (string, error) {
+// uniqueWorkspaceSlug probes on the caller's executor for the same reason
+// createWorkspaceQ does: inside an import it must see that transaction's own
+// uncommitted rows, and it must not reach for the pool from inside one.
+func (s *Store) uniqueWorkspaceSlug(q rowQueryer, baseSlug string) (string, error) {
 	slug := baseSlug
 	for i := 2; ; i++ {
 		var count int
-		err := s.db.QueryRow(s.q("SELECT COUNT(*) FROM workspaces WHERE slug = ? AND deleted_at IS NULL"), slug).Scan(&count)
+		err := q.QueryRow(s.q("SELECT COUNT(*) FROM workspaces WHERE slug = ? AND deleted_at IS NULL"), slug).Scan(&count)
 		if err != nil {
 			return "", err
 		}