diff --git a/internal/store/export.go b/internal/store/export.go index 63b68210..5d018ca9 100644 --- a/internal/store/export.go +++ b/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 diff --git a/internal/store/import_workspace_husk_test.go b/internal/store/import_workspace_husk_test.go new file mode 100644 index 00000000..dbf8c735 --- /dev/null +++ b/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") + } +} diff --git a/internal/store/trait_dedupe_test.go b/internal/store/trait_dedupe_test.go index 5eac6ebd..158f32ad 100644 --- a/internal/store/trait_dedupe_test.go +++ b/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") diff --git a/internal/store/workspaces.go b/internal/store/workspaces.go index 6965dd1f..a62ebb44 100644 --- a/internal/store/workspaces.go +++ b/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 }