From 197262bec1cca54bf0a34bd0e070f677076de09f Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 28 Aug 2026 15:23:11 -0400 Subject: [PATCH] fix(db): never raise from the connect handler, and scale the budget test right Two failures from the post-merge run of 12842d28, one real and one mine. test-windows: tests/test_highlevel.py's corrupt-database test passed and then failed its own temporary-directory cleanup with WinError 32, because recording.db was still open. get_engine asks each new connection for its journal mode so it can match the synchronous setting to it. A corrupt file cannot answer, and the exception was raised inside the connect handler, which leaves the new connection outside the pool that would have closed it: engine.dispose() cannot reach it and the file stays open. macOS and Linux delete an open file, so only Windows reported it. Never raise from that handler. It is a tuning step, not a correctness one, and a file that cannot answer it has to fail on the caller's own statement instead, where the engine still owns the connection. A new test asserts the corrupt-file path leaves no open handle, and fails against the unguarded handler. test-macos: the budget test measured 2.05s, then 2.22s, against declared worst cases of 2.0s and 2.2s. The helper is right; the test's scaling was wrong. An attempt costs its busy timeout plus a fixed overhead that does not shrink with it, so a ceiling of 0.2s against a 0.05s timeout is mostly spent on overhead and one slow attempt overruns. Production keeps the ceiling several times the timeout, so scale the test the same way: 0.25s timeout, 1.0s ceiling, 5.0s budget. Eight local runs measure 4.65-4.74s against a 6.0s worst case, where the old scaling had 0.02s of margin. Verified again with only the old retry policy restored: both regression tests fail, the other 14 pass. Co-Authored-By: Claude Opus 5 --- openadapt_capture/db/__init__.py | 15 +++++++++- tests/test_db_lock_retry.py | 48 ++++++++++++++++++++++++++++---- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/openadapt_capture/db/__init__.py b/openadapt_capture/db/__init__.py index 0a53950..5b55931 100644 --- a/openadapt_capture/db/__init__.py +++ b/openadapt_capture/db/__init__.py @@ -108,13 +108,26 @@ def _match_synchronous_to_the_journal_mode(dbapi_connection, _record) -> None: power loss can cost the last commits, and a capture interrupted by a power loss is incomplete anyway. A rollback-journal database keeps the default FULL, so an existing capture's durability is unchanged. + + Never raise from here. This is a tuning step, not a correctness one, and + a file that cannot answer it -- a corrupt capture, most of all -- has to + fail on the caller's own statement instead. An exception raised inside a + connect handler leaves the new connection outside the pool that would + have closed it, so ``engine.dispose()`` cannot reach it and the file + stays open. On Windows that open handle makes the capture directory + undeletable, which is how it shows up. """ - cursor = dbapi_connection.cursor() + try: + cursor = dbapi_connection.cursor() + except sqlite3.Error: + return try: cursor.execute("PRAGMA journal_mode") row = cursor.fetchone() if row and str(row[0]).lower() == "wal": cursor.execute("PRAGMA synchronous=NORMAL") + except sqlite3.Error: + return finally: cursor.close() diff --git a/tests/test_db_lock_retry.py b/tests/test_db_lock_retry.py index f426da0..fcb461a 100644 --- a/tests/test_db_lock_retry.py +++ b/tests/test_db_lock_retry.py @@ -444,11 +444,17 @@ def test_the_total_wait_never_runs_past_the_declared_budget(tmp_path, monkeypatc """ # Scale the busy timeout, the attempt ceiling and the budget together, so # the loop behaves exactly as it does in production and the test still - # finishes in about two seconds. The production attempt cost is measured by + # finishes in about four seconds. The production attempt cost is measured by # test_one_locked_attempt_costs_less_than_the_declared_ceiling. - monkeypatch.setattr(db, "SQLITE_BUSY_TIMEOUT_SECONDS", 0.05) - monkeypatch.setattr(crud, "SQLITE_LOCK_ATTEMPT_CEILING_SECONDS", 0.2) - monkeypatch.setattr(crud, "SQLITE_WRITE_LOCK_BUDGET_SECONDS", 2.0) + # + # Keep the ceiling several times the busy timeout, as production does. An + # attempt costs its busy timeout plus a fixed overhead that does not shrink + # with it, so a ceiling too close to the timeout is spent on that overhead: + # at 0.05/0.2/2.0 this measured 2.22s against its 2.2s worst case on hosted + # macOS, which says the scaling was wrong, not the helper. + monkeypatch.setattr(db, "SQLITE_BUSY_TIMEOUT_SECONDS", 0.25) + monkeypatch.setattr(crud, "SQLITE_LOCK_ATTEMPT_CEILING_SECONDS", 1.0) + monkeypatch.setattr(crud, "SQLITE_WRITE_LOCK_BUDGET_SECONDS", 5.0) engine, db_path, recording = _capture_database_with_a_recording(tmp_path, "budget") session = db.get_session_for_path(str(db_path)) competitor = _hold_the_write_lock(db_path, recording.id) @@ -465,8 +471,8 @@ def test_the_total_wait_never_runs_past_the_declared_budget(tmp_path, monkeypatc # The helper starts its last attempt strictly before BUDGET - CEILING, so # the total is over the budget only by however long that one attempt ran. - # A loaded runner does make it run long: this measured 2.05s against a 2.0s - # budget on hosted macOS. Bound it by what always holds. + # A loaded runner does make it run long, so bound the total by what always + # holds rather than by the budget alone. worst_case = ( crud.SQLITE_WRITE_LOCK_BUDGET_SECONDS + crud.SQLITE_LOCK_ATTEMPT_CEILING_SECONDS @@ -657,3 +663,33 @@ def _run(): assert not writer.is_alive(), "a writer hung under contention" for name, event in started_events.items(): assert event.is_set(), f"{name} never announced readiness" + + +def test_a_corrupt_capture_database_leaves_no_open_handle(tmp_path): + """Failing to open a capture must not keep its file open. + + ``get_engine`` asks each new connection for its journal mode so it can + match the synchronous setting to it. A file that cannot answer -- a corrupt + capture -- must still fail on the caller's own statement, because an + exception raised inside a connect handler leaves the new connection outside + the pool that would have closed it. ``engine.dispose()`` then cannot reach + it and the file stays open. + + On Windows an open handle makes the capture directory undeletable, which is + where this surfaced: tests/test_highlevel.py's corrupt-database test passed + and then failed its own temporary-directory cleanup with WinError 32. + """ + psutil = pytest.importorskip("psutil") + db_path = tmp_path / "recording.db" + db_path.write_text("this is not a sqlite database") + + process = psutil.Process() + before = {handle.path for handle in process.open_files()} + with pytest.raises(sa.exc.DatabaseError): + db.get_session_for_path(str(db_path)) + still_open = [ + path + for path in {handle.path for handle in process.open_files()} - before + if "recording.db" in path + ] + assert not still_open, f"the corrupt capture stayed open: {still_open}"