fix: make ParserManager and parse caches thread-safe - #48
Merged
Conversation
WorkspaceWatcher builds its ParserManager on the main thread but parses from threading.Timer callbacks — a new thread per debounce cycle. That means a single tree_sitter.Parser, which holds mutable state across a parse, is already shared between threads today, and the cache backends were doing unsynchronised read-then-act bookkeeping alongside it. - ParserManager: parsers are now thread-local, created on first use per thread. Language and compiled Query objects stay shared, which is safe because extractors allocate a fresh QueryCursor per call. The thread that constructs the manager keeps the parsers built in _init_languages, so single-threaded behaviour is unchanged. - ParseCache / BoundedParseCache / SQLiteParseCache: guard state with a lock. check_same_thread=False only silenced sqlite3's ownership assertion; it never serialised access. - LazyTree._ensure: documented as running the loader "exactly once" but was racy — two threads could both parse. Now double-checked locked, so the fast path stays lock-free. - BoundedParseCache._hashes grew without bound: the LRU evicted trees but never the corresponding hashes, so a bounded cache leaked on a long watch session. Hashes are now pruned to match the live entries. Adds tests covering parser isolation, counter coherence under contention, the load-once guarantee, and the hash leak. Also adds benchmarks/parallel_parsing_benchmark.py, which measures whether parallel parsing (#12) is worth doing. It is not, on this binding: py-tree-sitter does not release the GIL around Parser.parse, so 8 threads give 0.99x on pure parsing and 0.96x end to end, while a process pool is ~0.22x once pickling extracted nodes and edges is paid for. Recorded so the numbers can be re-checked rather than re-argued.
r0h1tb
force-pushed
the
feat/parallel-file-parsing
branch
from
August 2, 2026 12:03
41dc4c4 to
cce8593
Compare
Collaborator
Author
|
Rebased onto current Verified locally against
The +9 are this PR's own |
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
I picked up #12 (parallelise file parsing), built it, benchmarked it — and it doesn't pay off on this binding. So this PR ships the thread-safety work that came out of it, which turned out to be justified for an unrelated and more immediate reason, and drops the parallel driver.
The thread-safety bugs are live today
WorkspaceWatcherbuilds itsParserManageron the main thread but parses fromthreading.Timercallbacks — a new thread per debounce cycle (ast_rag/services/watcher_service.py). So a singletree_sitter.Parser, which holds mutable state across a parse, is already shared between threads during any watch session. Three concrete defects:Shared parser across threads. Parsers are now thread-local, created on first use per thread.
Languageand compiledQueryobjects stay shared — safe, because the extractors allocate a freshQueryCursorper call and never mutate theQuery. The constructing thread keeps the parsers built in_init_languages, so single-threaded behaviour is byte-identical.LazyTree._ensurewas racy. Its docstring promises the loader runs "exactly once", but two threads could both see_tree is Noneand both parse. Now double-checked locked, so the already-resolved fast path stays lock-free.BoundedParseCache._hashesgrew without bound. The LRU evicted trees but never the corresponding hashes, so a bounded cache leaked over a long watch session. Hashes are now pruned to the live entries onput().Cache backends (
ParseCache,BoundedParseCache,SQLiteParseCache) are guarded by locks. Worth notingcheck_same_thread=Falseon the SQLite connection only silenced sqlite3's ownership assertion — it never serialised access.On #12: parallel parsing isn't the lever
Measured on 10 cores, CPython 3.12, tree-sitter 0.24 (
benchmarks/parallel_parsing_benchmark.py, included so this can be re-checked rather than re-argued):ProcessPoolExecutorpy-tree-sitter does not release the GIL around
Parser.parse, so the C parsing that should have been the whole win is fully serialised. Extraction is Python (walking query matches, buildingASTNode/ASTEdge) and is GIL-bound by construction. What remains is lock and scheduling overhead, which is why end-to-end lands slightly below 1.0.Processes are worse: per-file work is only a couple of milliseconds,
Treeobjects aren't picklable so workers can't share the parse cache and must re-parse, and shipping extracted nodes/edges back over a pipe dominates.I had a working
ThreadPoolExecutordriver and a--jobsflag, and removed both rather than add complexity and a config surface for a measurable slowdown. The direction in #12's own note — moving parsing to Rust, or otherwise getting the work out from under the GIL — is the one that would actually pay.Tests
9 new tests in
tests/test_parser_thread_safety.pycovering parser isolation per thread, extraction equivalence between sequential and concurrent runs, cache counter coherence under contention, the SQLite single-connection path, the load-once guarantee, and the hash leak.Full suite: 182 passed. The 4 failures on this branch (
test_summarizer,test_unsupported_language,test_update_project_dry_runx2) also fail on a cleanmain— missing optional deps such asmcp, unrelated to this change.Closes nothing outright; leaves #12 open with data attached.