Skip to content

fix(db): never raise from the connect handler, and scale the budget test right - #124

Merged
abrichr merged 1 commit into
mainfrom
codex/sqlite-corrupt-db-handle
Aug 28, 2026
Merged

fix(db): never raise from the connect handler, and scale the budget test right#124
abrichr merged 1 commit into
mainfrom
codex/sqlite-corrupt-db-handle

Conversation

@abrichr

@abrichr abrichr commented Aug 28, 2026

Copy link
Copy Markdown
Member

Two failures from the post-merge run of 12842d28 (#123). One is a real defect I introduced in #122; the other is my test's scaling.

test-windows — a real handle leak

tests/test_highlevel.py::TestCaptureEdgeCases::test_capture_load_corrupt_db passed and then failed its own temporary-directory cleanup:

PermissionError: [WinError 32] The process cannot access the file because it is
being used by another process: '...\tmpw2fwt3c6\capture\recording.db'

get_engine asks each new connection for its journal mode so it can match synchronous to it. A corrupt file cannot answer, and that 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 happily delete an open file, so only Windows reported it.

Never raise from that handler. It is a tuning step, not a correctness one. A file that cannot answer it must fail on the caller's own statement instead, where the engine still owns the connection and disposes it.

test_a_corrupt_capture_database_leaves_no_open_handle pins this, and fails against the unguarded handler.

test-macos — my test's scaling, not the helper

The budget test measured 2.05s, then 2.22s, against declared worst cases of 2.0s and 2.2s.

An attempt costs its busy timeout plus a fixed overhead that does not shrink with it. A 0.2s ceiling against a 0.05s busy timeout is mostly that overhead, so one slow attempt overruns. Production deliberately keeps the ceiling several times the timeout (2.0s vs 0.5s); the test did not.

Scaled to match: 0.25s timeout, 1.0s ceiling, 5.0s budget.

old scaling new scaling
measured 2.05–2.22s 4.65–4.74s (8 runs)
worst case 2.2s 6.0s
margin 0.02s ~1.3s

No change to the retry helper in either case.

Still discriminating

Verified again with only the old retry policy restored: test_the_total_wait_never_runs_past_the_declared_budget and test_concurrent_writers_all_survive_a_busy_write_lock both fail, the other 14 pass.

Full suite: 722 passed, 33 skipped. ruff clean.

…est right

Two failures from the post-merge run of 12842d2, 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 <noreply@anthropic.com>
@abrichr
abrichr merged commit e46fe55 into main Aug 28, 2026
12 checks passed
@abrichr
abrichr deleted the codex/sqlite-corrupt-db-handle branch August 28, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant