From e2ac5ee3dec6aefad341c9270f42d5473e61337c Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:42:02 -0700 Subject: [PATCH 1/4] perf(tests): build the workspace schema once per session, not per test (#979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite was fsync-bound. `_init_database` creates ~18 tables plus indexes and switches the DB to WAL, and that costs **1272.9 ms** on a real filesystem against 2.7 ms on tmpfs — the difference is entirely fsync. It ran once per test. Measured here before any change: full non-e2e suite ~910 s on disk (median of 8 runs, 883–967 s) versus 427.6 s with a tmpfs `--basetemp`, identical results. That 483 s delta is ~366 workspace builds at 1.32 s each. (The issue reports ~4 h on a slower WSL2 disk; the pathology is the same, the scaling is not.) `tests/conftest.py` now builds the schema once per session and copies the file — 0.1 ms, ~13,000× cheaper. This is the issue's option 2, chosen over the tmpfs workaround because it helps CI too, and over `PRAGMA synchronous=OFF` because that would mean touching production `_open_db` to serve tests. Full suite on disk: **~910 s → 448.3 s**, 6149 passed / 49 skipped / 2 deselected — the same 6139 as before plus this change's 10 new tests. It now matches the tmpfs figure without needing tmpfs. Safe because the property was verified, not assumed: `_init_database` is byte-identical across builds (md5) and leaves no -wal/-shm sidecars, so a single-file copy is exactly equivalent. Drift is structurally impossible — the template is produced by calling the real function, so a schema change or SCHEMA_VERSION bump is picked up automatically. One trap found by an existing test: a plain `shutil.copy` also copies the template's permission bits, so `state.db` landed 0644 under a 007 umask where sqlite gives 0640. The copy now lets sqlite create the file and only overwrites its contents, keeping the mode faithful by construction rather than by recomputing a umask formula. Production code is untouched; durability outside the test session is unchanged. The tmpfs opt-in is documented in CLAUDE.md as the remaining lever for disks where the gap is still large. --- CLAUDE.md | 29 ++++ tests/conftest.py | 59 ++++++++ tests/core/test_workspace_db_template_979.py | 141 +++++++++++++++++++ 3 files changed, 229 insertions(+) create mode 100644 tests/core/test_workspace_db_template_979.py diff --git a/CLAUDE.md b/CLAUDE.md index c83aa559..ca4f5ad1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -158,6 +158,8 @@ tests/ ```bash uv run pytest # All tests uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle" # The CI gate (every non-e2e, non-real-LLM test) + # ~7.5 min locally. If it is much slower on your + # machine, see "Suite speed" below. uv run pytest tests/core/ # Core module tests scripts/lifecycle --mode cli|all # Real-LLM lifecycle tests (run locally before a PR) # api/web exit 3 — not implemented (#948, #1068) @@ -168,6 +170,33 @@ cd web-ui && npm test cd web-ui && npm run build ``` +#### Suite speed (#979) + +The suite used to be fsync-bound: `_init_database` builds ~18 tables plus +indexes and switches to WAL, and that cost **~1273 ms per test** on a real +filesystem versus 2.7 ms on tmpfs. It ran once per test, which was most of the +wall clock — measured at ~910 s, and reportedly ~4 h on a slower WSL2 disk. + +`tests/conftest.py` now builds that schema **once per session** and copies the +file (0.1 ms) into place per test. `_init_database` output is byte-identical +across builds, so the copy is exactly equivalent; the template is produced by +calling the real function, so a schema change or `SCHEMA_VERSION` bump cannot +leave it stale. Full suite: **~910 s → ~448 s**, same results, and CI benefits +too. `tests/core/test_workspace_db_template_979.py` pins the equivalence. + +If your machine is still I/O-bound, put pytest's temp dirs on tmpfs: + +```bash +mkdir -p /dev/shm/pytest-cf +uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle" --basetemp=/dev/shm/pytest-cf +``` + +Opt-in on purpose — a real filesystem is what CI runs, and is the more faithful +environment. `/dev/shm` is RAM-backed (the run must fit) and Linux-only. It +composes with the ambient-workspace guard: `pytest_configure` registers +`--basetemp` as an isolated root. With the template fix this now buys little +here (448 s vs 428 s); it is the lever for disks where the gap is still large. + ### Golden Path CLI ```bash # Workspace diff --git a/tests/conftest.py b/tests/conftest.py index 29ac0854..f0187ddb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -304,6 +304,65 @@ def openai_api_key(mock_env) -> str: return key + +# --------------------------------------------------------------------------- +# Workspace schema template (#979) +# --------------------------------------------------------------------------- +# +# Building the workspace schema costs ~1273ms per call on a real filesystem and +# ~2.7ms on tmpfs. That 470× difference is fsync: ~18 CREATE TABLEs plus +# indexes plus a WAL-mode switch, committed and flushed, once per test. It was +# most of the wall clock of a full local run (measured: ~910s on disk vs 428s +# with a tmpfs basetemp, identical results). +# +# `_init_database` is deterministic — two builds are byte-identical, with no +# -wal/-shm sidecars — so building it ONCE per session and copying the file +# (0.1ms) is exactly equivalent, not merely similar. +# +# Drift is structurally impossible: the template is produced by calling the +# real `_init_database`, so a schema change or SCHEMA_VERSION bump is picked up +# automatically. `tests/core/test_workspace_db_template_979.py` asserts the +# copy stays byte-identical to a real build. +# +# Test-only. No production code is touched, and durability semantics outside +# the test session are unchanged. + +_REAL_INIT_DATABASE = _workspace_module._init_database + + +@pytest.fixture(scope="session") +def real_init_database(): + """The unpatched schema builder, for tests that must compare against it.""" + return _REAL_INIT_DATABASE + + +@pytest.fixture(scope="session", autouse=True) +def _workspace_db_template(tmp_path_factory): + """Build the workspace schema once per session; copy it per test.""" + import shutil + + template = tmp_path_factory.mktemp("cf-db-template") / "state.db" + _REAL_INIT_DATABASE(template) + + def _copy_template(db_path): + # Let sqlite create the file so the mode is whatever sqlite would have + # given it (0644 base, not open()'s 0666 — see + # test_state_db_permissions_match_a_plain_sqlite_create), then + # overwrite the contents. Opening 'wb' on an existing file leaves its + # mode alone, so permissions stay faithful by construction rather than + # by recomputing a umask formula. + import sqlite3 + + sqlite3.connect(db_path).close() + with open(template, "rb") as src, open(db_path, "wb") as dst: + shutil.copyfileobj(src, dst) + + _workspace_module._init_database = _copy_template + try: + yield template + finally: + _workspace_module._init_database = _REAL_INIT_DATABASE + # Markers for test organization def pytest_configure(config): """Configure pytest with custom markers.""" diff --git a/tests/core/test_workspace_db_template_979.py b/tests/core/test_workspace_db_template_979.py new file mode 100644 index 00000000..bb94b50b --- /dev/null +++ b/tests/core/test_workspace_db_template_979.py @@ -0,0 +1,141 @@ +"""The per-test workspace schema build is the suite's cost centre (issue #979). + +Rebuilding ~18 tables plus indexes per test costs ~1273ms on a real filesystem +(2.7ms on tmpfs) — that difference is fsync, and it is most of the wall clock +of a full local run. Copying a template built once per session costs 0.1ms. + +The optimisation is only safe because ``_init_database`` is deterministic, so +these tests pin the property the speedup rests on rather than the speedup +itself. If a schema change ever made the output non-reproducible, the copy +would stop being equivalent and this file is what says so. +""" + +from __future__ import annotations + +import hashlib +import sqlite3 +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.v2 + + +def _digest(path: Path) -> str: + return hashlib.md5(path.read_bytes()).hexdigest() + + +class TestTheSchemaBuildIsReproducible: + """The premise: two builds are byte-identical, so a copy is equivalent.""" + + def test_two_builds_are_byte_identical(self, tmp_path, real_init_database): + a, b = tmp_path / "a.sqlite", tmp_path / "b.sqlite" + real_init_database(a) + real_init_database(b) + assert _digest(a) == _digest(b) + + def test_no_wal_sidecars_are_left_behind(self, tmp_path, real_init_database): + """A single-file copy is only complete if -wal/-shm are checkpointed.""" + real_init_database(tmp_path / "db.sqlite") + assert sorted(p.name for p in tmp_path.iterdir()) == ["db.sqlite"] + + +class TestTheTemplateMatchesARealBuild: + """The guarantee: what tests get is what the real code path produces.""" + + def test_the_copy_is_byte_identical_to_a_real_build( + self, tmp_path, real_init_database + ): + from codeframe.core import workspace as ws + + real = tmp_path / "real.sqlite" + real_init_database(real) + + copied = tmp_path / "copied.sqlite" + ws._init_database(copied) # the session-patched version + + assert _digest(copied) == _digest(real), ( + "the template has diverged from _init_database — the speedup is " + "no longer equivalent to the real schema build" + ) + + def test_the_copy_carries_the_current_schema_version(self, tmp_path): + """A SCHEMA_VERSION bump must not leave the template stamped behind.""" + from codeframe.core import workspace as ws + + db = tmp_path / "db.sqlite" + ws._init_database(db) + conn = sqlite3.connect(db) + try: + assert conn.execute("PRAGMA user_version").fetchone()[0] == ws.SCHEMA_VERSION + finally: + conn.close() + + def test_the_copy_is_in_wal_mode(self, tmp_path): + from codeframe.core import workspace as ws + + db = tmp_path / "db.sqlite" + ws._init_database(db) + conn = sqlite3.connect(db) + try: + assert conn.execute("PRAGMA journal_mode").fetchone()[0].lower() == "wal" + finally: + conn.close() + + @pytest.mark.parametrize("umask_value", [0o022, 0o002, 0o007]) + def test_the_copy_has_the_mode_sqlite_would_have_given_it( + self, tmp_path, umask_value + ): + """A file copy carries the SOURCE's mode — that is the trap here. + + The template is built once under the session's umask, so copying its + permission bits would hand every later test the wrong mode. Compared + against a reference database sqlite creates itself, not a formula: + sqlite uses an 0644 base, not open()'s 0666. + """ + import os + + from codeframe.core import workspace as ws + + old = os.umask(umask_value) + try: + reference = tmp_path / "reference.db" + sqlite3.connect(reference).close() + expected = reference.stat().st_mode & 0o777 + + db = tmp_path / "db.sqlite" + ws._init_database(db) + actual = db.stat().st_mode & 0o777 + finally: + os.umask(old) + + assert actual == expected, f"mode {actual:o}, sqlite would give {expected:o}" + + def test_a_workspace_built_on_the_copy_works_end_to_end(self, tmp_path): + """The point of the schema is that tasks round-trip through it.""" + from codeframe.core import tasks + from codeframe.core.tasks import TaskStatus + from codeframe.core.workspace import create_or_load_workspace + + repo = tmp_path / "repo" + repo.mkdir() + workspace = create_or_load_workspace(repo) + task = tasks.create(workspace, title="T", description="d") + tasks.update_status(workspace, task.id, TaskStatus.READY) + + assert tasks.get(workspace, task.id).status == TaskStatus.READY + + +class TestPerTestPatchingStillWins: + """A test that swaps _init_database itself must not be broken by this.""" + + def test_monkeypatch_restores_the_session_template_version(self, monkeypatch, tmp_path): + from codeframe.core import workspace as ws + + patched = ws._init_database + monkeypatch.setattr(ws, "_init_database", lambda p: None) + assert ws._init_database is not patched + monkeypatch.undo() + assert ws._init_database is patched, ( + "undo must restore the template version, not the original" + ) From 5054c2597155bf7d17097befde14b42b0811b952 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:44:35 -0700 Subject: [PATCH 2/4] fix(tests): template only a NEW database, never an existing one (#979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codex review [P2], and it is the more serious kind of bug: masked coverage rather than a visible failure. `_init_database` is `CREATE TABLE IF NOT EXISTS` plus ALTER TABLE steps, so calling it on an existing file is a *migration*, not a build. Copying the template over it discarded the caller's data and skipped every migration — and because the template already contains the column each migration adds, the assertions still passed. `test_blocker_origin.py::test_alter_table_migration_adds_created_by_column` is exactly that shape: it 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 wholesale. The test went green while proving nothing. The template now serves only a path that does not yet exist; anything else delegates to the real `_init_database`. The hot path is untouched — create_or_load_workspace builds at a fresh temp path — so the speedup stands. 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 created_by migration runs AND the pre-existing row survives (a rebuild would have dropped it). --- tests/conftest.py | 10 +++ tests/core/test_workspace_db_template_979.py | 64 ++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index f0187ddb..c994a73e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -345,6 +345,16 @@ def _workspace_db_template(tmp_path_factory): _REAL_INIT_DATABASE(template) def _copy_template(db_path): + # An EXISTING database is a migration, not a build: `_init_database` + # is `CREATE TABLE IF NOT EXISTS` plus ALTER TABLE steps, so copying + # over it would discard the caller's data and skip every migration — + # making migration tests pass for the wrong reason, because the + # template already contains the column the migration was meant to add. + # Only a new file may be templated. The hot path is unaffected: + # create_or_load_workspace builds at a fresh temp path. + if Path(db_path).exists(): + return _REAL_INIT_DATABASE(db_path) + # Let sqlite create the file so the mode is whatever sqlite would have # given it (0644 base, not open()'s 0666 — see # test_state_db_permissions_match_a_plain_sqlite_create), then diff --git a/tests/core/test_workspace_db_template_979.py b/tests/core/test_workspace_db_template_979.py index bb94b50b..f5b4e71d 100644 --- a/tests/core/test_workspace_db_template_979.py +++ b/tests/core/test_workspace_db_template_979.py @@ -126,6 +126,70 @@ def test_a_workspace_built_on_the_copy_works_end_to_end(self, tmp_path): assert tasks.get(workspace, task.id).status == TaskStatus.READY +class TestAnExistingDatabaseIsAMigration: + """`_init_database` on an existing file migrates it — it does not rebuild. + + The template must only ever serve a *new* database. Copying over an + existing one discards its contents and skips every ALTER TABLE, which + makes migration tests pass for the wrong reason: the template already has + the column the migration was supposed to add. + """ + + def test_an_existing_database_keeps_its_data(self, tmp_path): + from codeframe.core import workspace as ws + + db = tmp_path / "state.db" + conn = sqlite3.connect(db) + conn.execute("CREATE TABLE legacy (id TEXT PRIMARY KEY)") + conn.execute("INSERT INTO legacy VALUES ('keep-me')") + conn.commit() + conn.close() + + ws._init_database(db) + + conn = sqlite3.connect(db) + try: + assert conn.execute("SELECT id FROM legacy").fetchall() == [("keep-me",)] + finally: + conn.close() + + def test_the_alter_table_migration_actually_runs(self, tmp_path): + """The concrete case: a pre-created_by blockers table gets the column.""" + from codeframe.core import workspace as ws + + db = tmp_path / "state.db" + conn = sqlite3.connect(db) + conn.execute(""" + CREATE TABLE blockers ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL, + task_id TEXT, + question TEXT NOT NULL, + answer TEXT, + status TEXT NOT NULL DEFAULT 'OPEN', + created_at TEXT NOT NULL, + answered_at TEXT + ) + """) + conn.execute( + "INSERT INTO blockers (id, workspace_id, question, status, created_at) " + "VALUES ('b1', 'w1', 'q?', 'OPEN', '2026-01-01')" + ) + conn.commit() + conn.close() + + ws._init_database(db) + + conn = sqlite3.connect(db) + try: + columns = {r[1] for r in conn.execute("PRAGMA table_info(blockers)")} + assert "created_by" in columns + # Migrated, not replaced — a rebuild would have dropped the row. + assert conn.execute("SELECT id FROM blockers").fetchall() == [("b1",)] + finally: + conn.close() + + class TestPerTestPatchingStillWins: """A test that swaps _init_database itself must not be broken by this.""" From d0e55e9b00974dc412528476fccb9df2313800c2 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:53:23 -0700 Subject: [PATCH 3/4] docs: use the final measured suite timing (#979) 378 s, not the 448 s measured before the existing-database guard landed. The tmpfs opt-in now buys nothing on this machine (378 s on disk beats the 428 s tmpfs figure from before the fix), so its framing is narrowed to the machines where the gap is still large. --- CLAUDE.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ca4f5ad1..e0389feb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -158,7 +158,7 @@ tests/ ```bash uv run pytest # All tests uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle" # The CI gate (every non-e2e, non-real-LLM test) - # ~7.5 min locally. If it is much slower on your + # ~6.5 min locally. If it is much slower on your # machine, see "Suite speed" below. uv run pytest tests/core/ # Core module tests scripts/lifecycle --mode cli|all # Real-LLM lifecycle tests (run locally before a PR) @@ -181,7 +181,7 @@ wall clock — measured at ~910 s, and reportedly ~4 h on a slower WSL2 disk. file (0.1 ms) into place per test. `_init_database` output is byte-identical across builds, so the copy is exactly equivalent; the template is produced by calling the real function, so a schema change or `SCHEMA_VERSION` bump cannot -leave it stale. Full suite: **~910 s → ~448 s**, same results, and CI benefits +leave it stale. Full suite: **~910 s → ~378 s**, same results, and CI benefits too. `tests/core/test_workspace_db_template_979.py` pins the equivalence. If your machine is still I/O-bound, put pytest's temp dirs on tmpfs: @@ -194,8 +194,9 @@ uv run pytest tests/ --ignore=tests/e2e -m "not lifecycle" --basetemp=/dev/shm/p Opt-in on purpose — a real filesystem is what CI runs, and is the more faithful environment. `/dev/shm` is RAM-backed (the run must fit) and Linux-only. It composes with the ambient-workspace guard: `pytest_configure` registers -`--basetemp` as an isolated root. With the template fix this now buys little -here (448 s vs 428 s); it is the lever for disks where the gap is still large. +`--basetemp` as an isolated root. With the template fix this now buys nothing +here (378 s on disk vs 428 s on tmpfs before the fix); it is the lever only +for disks where the gap is still large. ### Golden Path CLI ```bash From d59ce1aa815110dbb58a7761213241ef0b49e929 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 02:08:40 -0700 Subject: [PATCH 4/4] perf(tests): build the template lazily; fix conftest spacing (#979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review, both non-blocking but both worth doing: - The autouse session fixture built the schema at session start, so every invocation paid ~1.3s — including `pytest -k one_unrelated_test`, which is the tight inner loop this issue is ultimately about. Now built on first use behind a lock, so a run that never creates a workspace pays nothing. Measured: a workspace-free selection is back to 0.30s, while tests/core/test_workspace.py still gets the speedup (6.14s baseline → 1.66s). - Triple blank line before the new block, single before pytest_configure. `ruff check` passes either way (E302/E303 are not in the enabled rule set here), but the reviewer was right that it reads as unintentional — it was. The third note, that `real_init_database` has one consumer, is accurate and left as-is: that consumer is the byte-identity self-check, which is the test the whole optimisation rests on. --- tests/conftest.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c994a73e..e67541e7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -304,7 +304,6 @@ def openai_api_key(mock_env) -> str: return key - # --------------------------------------------------------------------------- # Workspace schema template (#979) # --------------------------------------------------------------------------- @@ -338,11 +337,25 @@ def real_init_database(): @pytest.fixture(scope="session", autouse=True) def _workspace_db_template(tmp_path_factory): - """Build the workspace schema once per session; copy it per test.""" + """Build the workspace schema at most once per session; copy it per test. + + Built lazily on first use, not at session start: a run that never creates a + workspace (``pytest -k one_unrelated_test``) should not pay the ~1.3s build + it will never benefit from. + """ import shutil + import threading - template = tmp_path_factory.mktemp("cf-db-template") / "state.db" - _REAL_INIT_DATABASE(template) + template_lock = threading.Lock() + template_holder: list[Path] = [] + + def _template() -> Path: + with template_lock: + if not template_holder: + path = tmp_path_factory.mktemp("cf-db-template") / "state.db" + _REAL_INIT_DATABASE(path) + template_holder.append(path) + return template_holder[0] def _copy_template(db_path): # An EXISTING database is a migration, not a build: `_init_database` @@ -364,15 +377,16 @@ def _copy_template(db_path): import sqlite3 sqlite3.connect(db_path).close() - with open(template, "rb") as src, open(db_path, "wb") as dst: + with open(_template(), "rb") as src, open(db_path, "wb") as dst: shutil.copyfileobj(src, dst) _workspace_module._init_database = _copy_template try: - yield template + yield _template finally: _workspace_module._init_database = _REAL_INIT_DATABASE + # Markers for test organization def pytest_configure(config): """Configure pytest with custom markers."""