Repository navigation
fix(storage): wait out a busy graph DB instead of discarding it - #27
Merged
Merged
Conversation
A session that started while another process had graph.db open (any sibling session or daemon mid-load or mid-persist) treated the lock error as corruption: it set the healthy DB aside as graph.db.corrupt and redirected every project to a fresh, empty generation. On Linux the stale-LOCK cleanup could also delete a live holder's LOCK file, because its flock probe cannot see RocksDB's fcntl lock, letting two processes open the same DB. - RocksDBBackend::open reports lock contention as GraphError::Locked. - open_waiting_for_lock replaces open_with_stale_lock_recovery: it retries with backoff for a bounded time and never removes LOCK. The OS drops a lock when its holder exits, so a leftover LOCK file never blocked an open; removing one only ever risked a second writer. fs2 is no longer needed. - Every open of the shared DB goes through memory::open_shared_graph_db (10s wait), including vector claim/save/load and cross-project impact. - open_persistent_graph redirects only on a real load failure, never on Locked. - Saved file hashes are ignored when the project's graph loads empty. After any redirect they made the indexer skip every unchanged file, leaving the project with no symbols until a file changed. Verified end to end with the release binary (live holder for 4s and 20s, SIGKILLed holder, damaged CURRENT) and by new crate and integration tests on macOS, SLES 15-SP4 and Windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017rVbt7rENTwXkdHt3Bpgb5
…oss-project handle
🔍 CodeGraph PR Review12 files changed (+448/−212, 32 functions) · Risk: 🟡 medium Blast radius44 direct callers affected (18 breaking) across
|
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.
Intent
The developer wanted to commit the graph-database lock fix on the fix/graph-db-live-lock branch and push it through the no-mistakes gate. The fix addresses two pre-existing bugs: a busy graph.db held by another live process was treated as corruption, which moved the healthy database aside and wiped every project's persisted graph, and on Linux the stale-lock probe could delete a live database's LOCK file. The developer had earlier approved adding libc as a direct dependency for a Linux lock probe. The final fix made that unnecessary: opening the database now waits up to 10s for a lock holder, never deletes LOCK, and only quarantines the database on real damage. Saved file hashes are also ignored when a project's graph loads empty, so files get reindexed, and fs2 is dropped. Standing constraints apply: push only through no-mistakes rather than directly to origin, use conventional commit messages with the Claude co-author line, never use em dashes, and keep the project sole-maintainer with no external PRs merged.
What Changed
graph.dbwhile another process holds it now returns a newGraphError::Lockedinstead of being treated as corruption. A newRocksDBBackend::open_waiting_for_lock(_with)retries with backoff for up to 10s, and every shared-DB open path in the server (MCP load and persist, LSP backend, vector-set claims, cross-project impact) uses it throughmemory::open_shared_graph_db. On the MCP server, a lock that outlasts the wait is reported as contention, so the database is no longer redirected or moved aside. Cross-project impact now reuses a single handle for the registry scan and every per-project load.open_with_stale_lock_recoveryand its probe that deleted theLOCKfile, and dropped thefs2dependency. ALOCKfile left behind by a crashed process no longer blocks the next open, so the file is never deleted. Thegraph.loading.<pid>sentinel is removed while the open waits for another holder, so a session killed during the wait leaves no false crash evidence.graph_db_recovery_test.rsintegration test.🤖 Generated with Claude Code
Risk Assessment
✅ Low: The fix round does what both prior findings asked for. The sentinel is now armed for every DB open attempt and through the load. It is removed after each refused attempt, so it never exists during a backoff sleep, including on the final Locked error. Every non-lock failure and every success ends in a disarm. The crate tests check the hook pairing by calling the API, not by reading source. The only remaining note is a latency trade-off the user chose.
Testing
I built the server and ran the new end-to-end recovery test, which spawns the real MCP binary for both the contention case and the corrupt-DB redirect case. I also ran the focused RocksDB backend unit tests. Then I drove three live scenarios against the real binary, with a separate OS process holding graph.db: a 3s hold, a 15s hold (longer than the 10s wait), and a SIGKILL during the lock wait. Every scenario passed. No redirect happened under contention, no sentinel was ever present during a wait or after the kill, LOCK and the holder's data survived, and every session's symbol search found the indexed symbol. This change has no UI, so the evidence is CLI transcripts.
cargo test -p codegraph-server --test graph_db_recovery_testa_session_after_a_redirect_reindexes_files_the_old_db_heldEvidence: Live lock-contention scenario transcript (real codegraph-server, separate holder process)
Evidence: End-to-end graph_db_recovery_test run
Evidence: RocksDB backend lock-wait unit tests
Evidence: Key live excerpt
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
crates/codegraph-server/src/mcp/server.rs:348-open_persistent_graphwrites the per-PIDgraph.loading.<pid>sentinel before callingload_persistent_graph_inner. That call now waits up to 10s inopen_shared_graph_dbwhile a sibling holds the DB. The sentinel is only removed when the load returns, and the SIGINT/SIGTERM/panic paths in main.rs end inprocess::exitwithout removing it. Concrete sequence: session A is persisting. Session B starts, arms its sentinel and waits on the lock. The user closes the editor window, or the MCP client kills B, inside that wait. The next startup'sclassify_load_sentinelssees a sentinel whose PID is dead, treats it as a native crash on a poisoned DB, and callsbump_graph_generation. That abandons every project's persisted graph, the same data loss this change sets out to stop. Before this change the exposure was only the open/scan itself, measured in milliseconds. Now it includes the whole lock wait. Two ways to fix it: (1) acquire the lock before arming the sentinel, for example a waiting open followed by writing the sentinel and then the scan; the trade-off is that the sentinel would no longer cover an AV inside DB::open recovery. (2) Remove this process's sentinel from the exit and signal hook, so a clean kill never leaves poison evidence.crates/codegraph-server/src/domain/impact.rs:402-find_cross_project_consumersused to skip a project immediately when the shared DB was busy. Now it reopens the DB once per registered project, and each open can wait up to 10s. If a sibling holds the DB for a long persist, one impact query can stall for up to 10s times N projects. Contention normally clears within one wait, so this is a latency note, not a correctness defect. A single open for the registry scan and all per-project loads would avoid the repeated waits.🔧 Fix applied.
1 info still open:
crates/codegraph-server/src/domain/impact.rs:342-find_cross_project_consumersnow keeps one shared handle open from the registry scan through every per-project load. This is what the user asked for, and it does cap the query at one lock wait. The trade-off is that the RocksDB lock is now held for the whole loop instead of being released between projects. If loading every registered project takes longer than SHARED_GRAPH_DB_LOCK_WAIT (10s), a sibling session that starts, or tries to persist, during the loop gets Locked. A starting session then runs without its persisted graph. This only happens with many large projects, and the fix-round instructions explicitly chose this design. No action is needed, but if it shows up in practice, holding the handle only across the scan plus the per-project load would cap the hold time.✅ **Test** - passed
✅ No issues found.
cargo test -p codegraph-server --test graph_db_recovery_testa_session_after_a_redirect_reindexes_files_the_old_db_heldcargo build -p codegraph-server --bin codegraph-servercargo test -p codegraph-server --test graph_db_recovery_test(spawns the real MCP binary against a temporary HOME and workspace)cargo test -p codegraph --lib rocksdb_backend(Locked classification, waiting open, per-attempt hook bracketing)Manual live driver (python, real codegraph-server --mcp, temporary HOME): seed session, then a separate holder process holding graph.db for 3s, then 15s (longer than the wait), then a session SIGKILLed 3s into a lock wait, checking symbol_search results, graph.generation, graph.loading.* sentinels, LOCK and holder data after each✅ **Document** - passed
✅ No issues found.
crates/codegraph-server/src/backend.rs:1168- Runningcargo fmt --all --checkshows formatting drift in about 60 files in crates this change does not touch (language parsers, codegraph-harness, codegraph-memory examples). Runningcargo clippyshows warnings in codegraph-server (backend.rs:1168/1253 unusedtotal_indexed, mcp/server.rs:1484/4108/4155/4254), none of them on lines this change touched. All of it predates this change, and CI enforces neither tool. The changed files themselves are fmt-clean and add no new clippy warnings. This is left for a separatecargo fmt --allplus clippy cleanup PR so this fix stays focused.✅ **Push** - passed
✅ No issues found.