perf(tests): build the workspace schema once per session, not per test (#979)

[frankbria]

Closes #979.

~910 s → 377.9 s (6m17s) on a real filesystem, identical results. This is the issue's option 2 (build the schema once, copy it per test), chosen over the tmpfs workaround because it helps CI too, and over PRAGMA synchronous=OFF because that would mean touching production code to serve tests (AC4).

I re-measured first — the issue's numbers don't reproduce here

The issue reports ~4 h locally vs 4m38s on CI (~47×). On this machine the gap is far milder, so the fix had to be justified by these numbers rather than the reported ones:

Scope	disk	tmpfs (/dev/shm)	ratio
tests/core/test_workspace.py (23)	6.14 s	0.91 s	6.7×
Full non-e2e suite	~910 s (median of 8 runs today: 883–967 s)	427.6 s	2.1×

Same results either way. So AC1's "under ~15 min" was already met here, at 15m10s — barely. That doesn't make the issue wrong; it means the pathology is environment-scaled, and the fix has to help everywhere rather than only on tmpfs.

Where the time went

operation	disk	tmpfs
_init_database	1272.9 ms	2.7 ms
create_or_load_workspace	1322.6 ms	3.1 ms
shutil copy of a prebuilt template	0.1 ms	0.1 ms

910 − 428 = 482 s of the suite is fsync, and at 1.32 s per workspace that is ~366 builds. That is the whole gap, and copying a template is ~13,000× cheaper than rebuilding.

Result

	before	after
Full suite, disk	~910 s	377.9 s
Full suite, tmpfs	427.6 s	(no longer needed)
Tests	6139 passed / 49 skipped / 2 deselected	6151 / 49 / 2

6151 − 6139 = exactly this PR's 12 new tests. Nothing skipped or weakened to hit the number (AC2). On disk it now beats what tmpfs previously achieved, so the workaround is redundant here — and CI gets the same win.

Why the template is safe — verified, not assumed

• _init_database output is byte-identical across builds (md5), with no -wal/-shm sidecars, so a single-file copy is exactly equivalent rather than merely similar.
• Drift is structurally impossible: the template is produced by calling the real _init_database once per session, so a schema change or SCHEMA_VERSION bump (three happened in this issue batch alone) is picked up automatically. Nothing is hand-maintained.
• A self-check test asserts the copy stays byte-identical to a real build, so the optimisation can never silently diverge.

Two traps, both caught and fixed

1. A file copy carries the source's permission bits. state.db landed 0644 under a 007 umask where sqlite gives 0640 — caught by the existing test_state_db_permissions_match_a_plain_sqlite_create[7]. The copy now lets sqlite create the file and only overwrites its contents, so the mode is faithful by construction rather than by recomputing a umask formula (the formula is what the original #954 test was written to catch).

2. An existing database is a migration, not a build — codex review [P2], and the more serious of the two because it masked coverage instead of failing. _init_database is CREATE TABLE IF NOT EXISTS plus ALTER TABLE steps, so copying over an existing file discarded the caller's data and skipped every migration. test_blocker_origin.py::test_alter_table_migration_adds_created_by_column creates a pre-created_by blockers table, calls _init_database, and asserts the column appeared — under the templated version it appeared because the table had been replaced. The test went green while proving nothing.

The template now serves only a path that doesn't yet exist; anything else delegates to the real function. The hot path is unaffected (create_or_load_workspace builds at a fresh temp path), which the final 377.9 s confirms. Two tests added, because the existing migration test cannot detect this by construction: one asserts an existing DB keeps its rows, the other that the migration runs and the pre-existing row survives.

Acceptance criteria

• AC1 under ~15 min: 6m17s on disk ✅
• AC2 identical pass/fail, nothing skipped or weakened: 6139 → 6151, delta is exactly the new tests ✅
• AC3 discoverable: CLAUDE.md → "Suite speed ([P2.18] Full test suite takes ~4h locally vs 4m38s on CI — per-test SQLite fsync #979)" documents the fix and the tmpfs opt-in, with the caveats the issue lists (RAM-backed, Linux-only, opt-in because a real filesystem is what CI runs) ✅
• AC4 production durability unchanged: only tests/conftest.py and test files change — no codeframe/ source touched ✅
• AC5 re-check [P1.34] Parallel-execution barrier timeout is load-sensitive — flakes under full-suite runs #976: already CLOSED; its barrier tests pass in the full run, and the I/O-pressure reduction removes the cause the issue identified ✅

Known limitations

• The 12 new tests are worth ~2 s; the reported number includes them.
• Timings are single-machine. The reported 47× box should see a larger absolute win, but I can't verify that from here — the tmpfs escape hatch stays documented for exactly that case.
• The template lives for the pytest session, so a test that mutates the schema file in place would affect only its own copy, not the template. Nothing does; the byte-identity self-check would catch it if something started to.

[github-actions]

Claude finished @frankbria's task in 7m 19s —— View job

---

Precision bug-hunting review — complete

Reviewed PR #1101 for concrete defects (logic errors, data loss/corruption, races, test-isolation breakage, contract changes). Style, architecture, naming, and test-coverage are out of scope (covered by the general review).

• Gather PR diff + context
• Read changed files + callees (_init_database, create_or_load_workspace, _ensure_schema_upgrades)
• Verify the byte-identity / no-WAL-sidecar claim
• Check the "template only when path absent" guard + permission-bit handling
• Check test isolation (cross-test leakage, monkeypatch interaction)
• Post findings + summary

✅ GLM review: no defects found.

What I verified against the checked-out code (not just the diff):

• Hot path is correct. create_or_load_workspace (workspace.py:1023–1057) builds the schema at a fresh temp path .{state.db}.{uuid}.tmp, then os.replaces it into place with a BaseException cleanup. Because the temp path never pre-exists, the patched copy is what runs, and the copied bytes (a checkpointed WAL-mode db) behave identically to a real build under the subsequent _open_db → INSERT → PRAGMA wal_checkpoint(TRUNCATE) → rename sequence.
• The existing-DB guard is sound. _copy_template delegates to _REAL_INIT_DATABASE whenever the target exists (tests/conftest.py:355). This correctly preserves test_blocker_origin.py::test_alter_table_migration_adds_created_by_column (pre-built blockers table at workspace.py-test line 96 → migration runs, not replacement) and test_atomic_writes_954.py::test_init_is_retryable_after_a_failure (retry hits an existing tmp_db → real init).
• No patch bypass. No production caller imports _init_database by name (the only codeframe/ reference is a comment in react_agent.py:420), and no test asserts the function's identity or call-count — so the module-attribute patch reaches every call site, and the session-finally restore is safe.
• Byte-identity premise holds. _init_database is pure DDL + PRAGMA user_version, no timestamps/entropy, clean close → checkpoint removes -wal/-shm; pinned by test_two_builds_are_byte_identical / test_no_wal_sidecars_are_left_behind.
• Permission handling is faithful. sqlite3.connect(db_path).close() creates the file with sqlite's mode, then open(..., "wb") overwrites contents without touching mode bits (truncating an existing file doesn't reapply O_CREAT mode); guarded by the parametrized mode test across umasks 022/002/007.
• Test isolation preserved. Each test gets its own copy at its own tmp_path; the template is only ever opened "rb" and built once from the real function, so no cross-test or schema-drift contamination is possible.

[frankbria]

Third-party review — codex review --base main

Pass 1 — [P2] Preserve migration tests when templating databases (tests/conftest.py)

When a test passes an existing DB into workspace._init_database (for example, the blocker-origin migration test creates a legacy blockers table and then calls _init_database), this replacement connects to that file and then truncates it to the current template, so none of the ALTER TABLE migration code runs and any existing schema/data is discarded.

Confirmed, and this is the finding that mattered most on this PR — because it did not fail, it went green while proving nothing. _init_database is CREATE TABLE IF NOT EXISTS plus ALTER TABLE steps, so an existing file is a migration path. test_alter_table_migration_adds_created_by_column builds a pre-created_by blockers table and asserts the column appears afterwards; with the template it appeared because the whole table had been replaced. Exactly the masked-coverage shape that #979 exists to complain about — a suite nobody can trust — so shipping it inside the fix for that issue would have been particularly bad.

Fixed: the template serves only a path that does not yet exist, anything else delegates to the real _init_database. The hot path is unaffected (create_or_load_workspace builds at a fresh temp path), confirmed by the final 377.9 s. Two tests added, because the existing migration test cannot detect this by construction — one asserts an existing DB keeps its rows, the other that the migration runs and the pre-existing row survives (a rebuild would have dropped it). Both were RED before the guard.

Pass 2 (final diff) — clean:

The changes are limited to test infrastructure, tests, and documentation. The session-scoped workspace DB template preserves existing migration behavior for existing databases and representative affected tests pass with the patch applied.

No Critical or Major findings outstanding.

---

The other trap on this PR was caught by an existing test rather than a reviewer, and is worth naming for the same reason: shutil.copy carries the source's permission bits, so state.db landed 0644 under a 007 umask where sqlite gives 0640. test_state_db_permissions_match_a_plain_sqlite_create[7] failed immediately. Its docstring says it was itself rewritten (after a GLM review on #954) to compare against a real sqlite-created reference instead of recomputing 0o666 & ~umask — and that rewrite is the only reason it caught this. A formula-based version would have agreed with my wrong answer.

[claude]

Review

Solid change, and the diff shows real self-correction: the codex review's [P2] finding (template masking migrations) and the pre-existing permission test both caught genuine traps, and both are now handled correctly rather than patched over. I traced the fix independently rather than just taking the PR description's word for it — notes below.

Verified correct:

• Hot path actually benefits. create_or_load_workspace (codeframe/core/workspace.py:1023-1025) always calls _init_database on a fresh .{STATE_DB_NAME}.{uuid4}.tmp path that cannot pre-exist, so production code's real call site always takes the fast copy branch — this isn't just a speedup for test scaffolding that calls _init_database directly.
• Migration guard is real, not just asserted. Traced both existing tests that call _init_database on a pre-existing path (tests/core/test_blocker_origin.py:122, tests/core/test_atomic_writes_954.py:300-318) — both pick up ws._init_database via attribute lookup at call time (module-global, not a captured reference), so they get the session-patched _copy_template, which correctly delegates to the real builder whenever db_path.exists(). No test in the suite reaches the bug the codex review flagged.
• Permission fix is structurally sound, not coincidentally correct. _copy_template's sqlite3.connect(db_path).close() mirrors exactly what production _open_db (workspace.py:96) does to create a fresh file, so the mode comes from sqlite's own creation under the process umask in both the templated and non-templated paths — not a recomputed formula. test_state_db_permissions_match_a_plain_sqlite_create goes through the real create_or_load_workspace hot path, so it's actually exercising this, not a mocked shortcut.
• No stale-reference risk elsewhere. Grepped codeframe/ — _init_database is only defined and called within workspace.py itself, so the monkeypatch-by-module-attribute approach (same pattern as the existing _get_state_dir ambient-workspace guard just above it in conftest.py) covers every call site; nothing imports a bound reference that would bypass it.
• Scope is test-only as claimed (AC4) — no codeframe/ source file appears in the diff outside test infra.

Minor, non-blocking:

• tests/conftest.py has a triple blank line before the new # Workspace schema template (#979) comment block (~line 305-308) and only a single blank line before def pytest_configure immediately after the new fixture (~line 376). uv run ruff check . is the documented quality gate in CLAUDE.md — worth confirming this passes clean, since E302/E303 would normally flag both.
• The new real_init_database session fixture isn't consumed by anything outside the new test file. Fine as a documented escape hatch for future tests, just flagging it's currently single-purpose.
• The autouse session fixture adds a fixed ~1.3s to every pytest invocation, even ones selecting a single unrelated test. Negligible against the full-suite win and not worth guarding, just noting it's a flat tax now paid by pytest -k ... one-liners too.

Test coverage: the new file is thorough — reproducibility of the build, byte-identity of the copy vs. a real build, schema version/WAL-mode sanity, permission bits parametrized over three umasks, an end-to-end workspace round-trip, and — the important pair — an existing DB keeping its rows and the concrete created_by ALTER TABLE migration actually running against the templated _init_database. Both of the latter two were red before the guard per the PR description, which is the right way to prove the fix.

No new findings beyond what's already in the thread (the codex [P2] fix and the permission-bit trap are both correctly resolved, confirmed above by tracing rather than re-flagging). Nothing here blocks merge.

[frankbria]

All three notes addressed in the latest push.

The ~1.3s flat tax was the one worth fixing — you're right that it lands on pytest -k ... one-liners, and the tight inner loop is ultimately what this issue is about, so making the fast full suite cost something on every single-test run would have traded the wrong way. The template is now built on first use behind a lock rather than at session start, so a run that never creates a workspace pays nothing:

tests/core/test_state_machine.py   (no workspace)  0.30s   # was ~1.6s with the eager build
tests/core/test_workspace.py       (workspace-heavy) 1.66s  # baseline 6.14s

Blank lines: correct, and unintentional — collapsed. On the ruff check question: it passes both before and after, because E302/E303 aren't in this repo's enabled rule set, so the documented gate would not have caught it. Worth knowing rather than assuming the linter has it covered.

real_init_database having one consumer: accurate. Leaving it — that consumer is test_the_copy_is_byte_identical_to_a_real_build, which is the assertion the entire optimisation rests on, so it needs an unpatched handle by construction rather than as a convenience.

---

Worth adding as evidence the fix helps CI, not just local runs — the issue predicted this and it shows up in this PR's own checks:

PR	Backend Unit Tests
#1099, #1100 (before)	6m38s, 6m39s, 6m43s, 6m55s
#1101 (this)	5m54s

~12% off CI as well, on the same hardware and the same command.

Re-running the full local gate on the final commit.

[github-actions]

Claude finished @frankbria's task in 4m 18s —— View job

---

Precision bug-hunting review — complete

Reviewed the latest push (d59ce1a, the synchronize that made the template build lazy + collapsed the blank lines) for concrete defects only — logic errors, races, data loss/corruption, contract breaks, unhandled error paths. Style/architecture/coverage are out of scope.

• Gather full diff (gh pr diff) + view latest commit changes
• Read changed files + callees (_init_database, create_or_load_workspace, _ensure_schema_upgrades, _open_db)
• Verify the migration / permission guards still hold after the lazy-build refactor
• Check the new lazy-init lock for races / deadlock / exception-safety
• Post inline findings (if any) + summary

✅ GLM review: no defects found.

The delta since the prior (pre-synchronize) reviews is the lazy-build lock, so that's what I traced hardest. What I verified against the checked-out code (not the diff alone):

• The lazy lock is race-free and exception-safe. _template() (conftest.py:352-358) holds template_lock across both the emptiness check and the append, with _REAL_INIT_DATABASE(path) called inside the critical section — so a second caller can only ever observe a fully-built, appended template. If the build raises, template_holder is left empty (the append is strictly after the build call), so the next caller rebuilds from a fresh mktemp dir rather than caching a partial file. _REAL_INIT_DATABASE never re-enters _template/_copy_template, so there's no re-entrant deadlock. The lock is never contended in practice (pytest is single-threaded per process; xdist workers are separate processes with independent locks/templates), but even if it were, the worst case is ~1.3 s of serialized waiting, not corruption.
• The hot path still takes the copy branch. create_or_load_workspace (workspace.py:1023) builds at state_dir / f".{STATE_DB_NAME}.{uuid4().hex}.tmp", which cannot pre-exist, so Path(db_path).exists() in _copy_template is always False there — production's real call site always gets the fast copy, exactly as before the lazy refactor.
• The retry test survives the lazy change. tests/core/test_atomic_writes_954.py::test_init_is_retryable_after_a_failure captures real_init = ws._init_database, which is now _copy_template (not the raw builder). On the simulated crash, create_or_load_workspace's except BaseException (workspace.py:1049-1057) unlinks the templated tmp_db and its -wal/-shm siblings, so state.db never lands and the retry rebuilds — the test's contract holds. The companion test_a_crash_...leaves_nothing asserts the same no-state.db outcome and still passes for the same reason.
• _ensure_schema_upgrades is not routed through the patch. It's a distinct function (workspace.py:534), not the swapped attribute, and it short-circuits on PRAGMA user_version. Since the template is stamped with SCHEMA_VERSION (workspace.py:488), a copied template passes that gate immediately — the upgrade path is a no-op on fresh copies, which is correct (it's already current), and the existing-DB migration guard (_copy_template → _REAL_INIT_DATABASE when db_path.exists()) still routes real migrations through the unpatched builder.
• Signature/contract intact. _init_database(db_path: Path) -> None is single-arg; _copy_template(db_path) matches; the only production call site (workspace.py:1025) and every test call site pass one positional path. _REAL_INIT_DATABASE is captured at module-import (conftest.py:329) before any test can patch the attribute, so the session-finally restore is faithful. Path (used in list[Path] and Path(db_path).exists()) is imported at conftest.py:7.

No new findings. The migration-masking and permission-bit traps were already raised and resolved on earlier commits (codex [P2] and the existing test_state_db_permissions_match_a_plain_sqlite_create), and the lazy-build refactor does not reopen either — confirmed above rather than re-flagged.

[claude]

Review

Re-reviewed the final commit (d59ce1aa, lazy-build + spacing fix) independently of the three prior passes on this thread (GLM automated review, codex [P2] migration-masking finding, and the earlier Claude pass). I re-traced the logic rather than re-stating prior conclusions; summary below is only what's new or worth confirming explicitly.

Confirmed correct, independently traced:

• The lazy-build fix (this PR's final commit) is sound: the autouse session fixture assigns _copy_template cheaply at first-test setup, but the actual ~1.3s _REAL_INIT_DATABASE(path) call only happens inside _template(), which _copy_template only reaches on a new db_path. A pytest -k selection that never touches a workspace genuinely pays nothing, matching the claimed 0.30s figure.
• Blank-line spacing (tests/conftest.py around the new block and before pytest_configure) is now two lines each side — PEP8-clean, matching the stated fix.
• I specifically hunted for a case the migration-guard rework could have missed: whether any test drives _init_database/_open_db with a Path-incompatible object. workspace.py:92-95's _open_db docstring warns against str()-wrapping because of "a test's MagicMock," which made me want to check whether _copy_template's new Path(db_path).exists() call (which _open_db itself doesn't do) could blow up on such an object. Grepped every call site of _init_database across tests/ and codeframe/ — nothing ever passes it a mock; the only callers are create_or_load_workspace (a real Path) and the test file's own Path fixtures. Not a live bug, but worth confirming since it's exactly the kind of interaction a coverage-preserving wrapper like this one can quietly break.
• Re-verified the two things this PR most depends on being true rather than trusting the description: _init_database's signature (-> None, workspace.py:136) matches what _copy_template implicitly returns on the new-file branch (falls off the end, i.e. None), so callers that use the return value see no behavior change; and the hot path (create_or_load_workspace, workspace.py:1025) always calls _init_database on a freshly-uuid'd temp path, so production behavior can't regress through the existing-file branch.
• The 12-new-tests arithmetic checks out: 10 def test_ functions in the new file, one parametrized ×3, 10 - 1 + 3 = 12, matching the PR body's 6151 − 6139 = 12 claim.
• --basetemp composing with the ambient-workspace guard (CLAUDE.md's claim) is real, not aspirational — pytest_configure (tests/conftest.py:391-395) already appends config.option.basetemp to _EXTRA_ISOLATED_ROOTS, pre-existing and unaffected by this change.

No new findings. Everything substantive was already caught and fixed in this thread — the migration-masking bug (codex [P2]) and the permission-bit trap are both handled correctly, and I couldn't find a gap in either fix on this pass. Test coverage for a change this deep into shared test infra is appropriately paranoid (reproducibility, byte-identity, WAL mode, schema version, permissions across 3 umasks, an end-to-end round-trip, and the two red→green migration-preservation tests). Scope stays test-only as claimed (AC4) — no codeframe/ source file in the diff.

Nothing here blocks merge.