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}"