Repository navigation
fix(storage): close the sqlite connection without racing the consumer thread - #480
Merged
Merged
Conversation
… thread Session.close() joins the consumer with a bounded timeout and closes storage unconditionally afterwards, so a slow consumer (WAL fsync, write-retry backoffs) can still be inside write_atomic when the caller thread closes the shared connection. That is a use-after-close: native access violation on the Windows CI runner (reproduced as SIGSEGV on Linux). Storage now serialises consumer-facing DML and close() behind a threading.Lock + _closed flag: close() waits out the in-flight write, then any residual batch the consumer drains afterwards is dropped instead of executing on the closed connection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the native access violation in
test_storage_does_not_deadlock_under_threaded_contentionseen on the Windows CI runner (PR #479, re-run passed but the crash is a real race, not a one-off).Root cause.
Session.close()joins the consumer thread with a bounded timeout (_CLOSE_JOIN_TIMEOUT+ grace) then closes storage unconditionally — the comment at the close site explicitly says so ("storage is closed below regardless of whether the consumer finished"). When the consumer is slow (WAL fsync,write_atomicretry backoffs of 0.1/0.5/2.0s on a loaded runner), it is still executingwrite_atomicon the sharedsqlite3.Connectionwhen the caller thread runsself._conn.close(). Closing a connection under an in-flight statement is a use-after-close: native access violation on Windows (the #479 trace — consumer thread faulting insidewrite_atomic), SIGSEGV on Linux (reproduced:EXIT=139).The old design contract — "temporal partitioning, no mutex" (module docstring) — assumed
closejoins before touching the connection again, which is exactly what the bounded-timeout close stopped guaranteeing.Fix.
Storagenow serialises consumer-facing DML andclose()behind athreading.Lock+_closedflag:write_atomic/archive_skill_blocksacquire the lock; if_closedis set (close won the race), the residual batch is dropped — the documented "loss ceiling = kill -9" path inSession.close, made crash-free.close()acquires the lock, so it waits out the in-flight write before closing; idempotent, safe to call twice (existing contract preserved).load, before the consumer starts) — unchanged contract; the lock is uncontended in the single-writer steady state and only contends at the close boundary.promote_session/ archive helpers use their own connections (verified) — no otherself._connuser can raceclose.Tests / CI
New regression test
test_close_without_waiting_for_consumer_drops_residual_without_crashing: zeroes the join timeouts + enqueues 1000 writes soSession.close()runs while the consumer is deterministically mid-drain. Without the fix this segfaults the whole pytest process (verifiedEXIT=139); with the fix it passes (consumer drops residuals, exits cleanly). Also covers the exact production timeout path that crashed on Windows.Local gates:
uv run ruff check✓,uv run ty check✓,uv run pytest tests/test_session.py✓ (49 pass). Full suite ✓ except the pre-existing environmentaltest_startup_latency.pyflake (fails on cleanmaintoo — machine-load, unrelated).