fix(lsp): mark column-unlocatable edges unresolvable to stop zombie re-walks#147
Merged
Merged
Conversation
…e-walks Edges whose target_name can't be located as a word on its recorded line (a whole-expression or multi-line target) can't be turned into an LSP definition query, so drain_language silently dropped them. They stayed resolution_state=0 and, because reopen_heuristic_exhausted un-seals state 4->0 before every LSP-enabled reindex, were re-fetched and re-attempted on every pass with zero progress (~10.9k permanent state-0 zombies on the family-check DB). Capture the dropped edge_ids in a new LangOutcomes.pending_unlocatable bucket and mark them unresolvable (state 2) via the existing mark_edge_unresolvable. Gated on !server_died only (no resolved>0 gate): it's a deterministic cartog-side fact, independent of server health, like the external bucket. Sticky-but-self-correcting via --force (reset_all_unresolvable); the name-keyed reopen can't match a compound target name, so --force is the only reopen path. Resolution-side, all-language fix; no extractor change. The Rust-extractor root cause (emit bare identifiers) is deferred to a separate PR. Also add a #[cfg(unix)] regression test that a write to a dead server fails fast via BrokenPipe (Ok(Err(_)) ack arm), not after the full write_timeout, locking the server_died close_file seam against a future edit reintroducing a stall. Widen mark_edge_unresolvable's precondition docstring to sanction the cartog-side unlocatable determination alongside the server Ok(None) case.
📝 WalkthroughWalkthroughIntroduces a ChangesUnlocatable Edge Detection and Marking
CI Cache Update Not applicable — this cohort is instead: Dead LSP Server Write Regression Test
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 88.35% 88.38% +0.03%
==========================================
Files 96 96
Lines 32474 32574 +100
==========================================
+ Hits 28692 28792 +100
Misses 3782 3782 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cartog-lsp/src/client.rs (1)
865-903: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the assertion to positively confirm BrokenPipe, not just "not a write-timeout".
The test only asserts
!is_write_timeout(&err)and that it returned quickly. That proves the failure isn't a write-timeout wedge, but it doesn't prove it is the intended broken-pipe failure — a future regression that returns some other fast, non-write-timeout error would still pass, silently defeating the regression guard the doc comment (lines 868-872) claims to provide.♻️ Suggested stronger assertion
assert!( !is_write_timeout(&err), "dead-server write is a broken pipe, not a write-ack timeout: {err}" ); + assert!( + err.chain() + .filter_map(|c| c.downcast_ref::<std::io::Error>()) + .any(|io_err| io_err.kind() == std::io::ErrorKind::BrokenPipe), + "expected a BrokenPipe io error in the chain, got: {err:#}" + ); assert!( elapsed < Duration::from_secs(5), "a broken pipe must return promptly, not wait out write_timeout; took {elapsed:?}" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cartog-lsp/src/client.rs` around lines 865 - 903, The test in write_to_a_dead_server_fails_fast_via_broken_pipe_not_write_timeout only checks that the error is not a write timeout and that it returns quickly, but it should also positively assert the failure is BrokenPipe. Update the assertion logic in this test to inspect the error returned by client.send_notification for the expected broken-pipe kind/variant, using the existing helper is_write_timeout and the error value from expect_err so the regression guard in this dead_fake_server path actually proves the intended failure mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/cartog-lsp/src/client.rs`:
- Around line 865-903: The test in
write_to_a_dead_server_fails_fast_via_broken_pipe_not_write_timeout only checks
that the error is not a write timeout and that it returns quickly, but it should
also positively assert the failure is BrokenPipe. Update the assertion logic in
this test to inspect the error returned by client.send_notification for the
expected broken-pipe kind/variant, using the existing helper is_write_timeout
and the error value from expect_err so the regression guard in this
dead_fake_server path actually proves the intended failure mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b2d825f-aca1-4ef3-8848-ba910dd72cff
📒 Files selected for processing (5)
crates/cartog-db/src/store/lsp.rscrates/cartog-lsp/README.mdcrates/cartog-lsp/src/client.rscrates/cartog-lsp/src/resolve.rsdocs/troubleshooting.md
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
1 participant
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.
Problem
During LSP edge resolution,
drain_languagelocates each edge's target column on its source line viafind_column_in_line. Edges whosetarget_namecan't be located as a word on its recorded line — a whole-expression or multi-line target (e.g. the Rust extractor emitsPool::new, method-chain receivers) — were silently dropped by the query-batchfilter_map. A dropped edge is never queried, never marked, and staysresolution_state = 0forever.Because
reopen_heuristic_exhaustedun-seals state 4→0 before every LSP-enabled reindex, these edges are re-fetched and re-attempted on every pass with zero progress. On the family-check DB this was ~10,928 permanent state-0 zombies (only ~72 edges were locatable).Fix (Part 1)
An edge cartog can't even form a query for is definitively unresolvable-by-LSP. Capture the dropped edge_ids in a new
LangOutcomes.pending_unlocatablebucket and mark themunresolvable(state 2) via the existingmark_edge_unresolvable, so they stop re-walking.!server_diedonly (noresolved > 0gate, unlikepending_unresolvable) — unlocatable is a deterministic cartog-side fact independent of server health, same trust model as the external bucket.--force(reset_all_unresolvable). The name-keyed reopen (reset_unresolvable_for_names) can't match a compound target name, so--forceis the only reopen path (documented).The Rust-extractor root cause (emit bare identifiers instead of
Pool::new) is a separate, larger PR (breaks extractor tests +webapp_rs.jsonground-truth, needs bench re-verify) — explicitly deferred.Part 2 — dead-server
close_fileinvariant (test-only)Investigated and found already safe: on the
server_diedpath the child is reaped, so its stdin read-end is closed andclose_file's write returnsBrokenPipepromptly via theOk(Err(_))ack arm — nowrite_timeoutblock. Added a#[cfg(unix)]regression test so a future edit can't reintroduce a 10s stall on this seam. No production change.Also
Widened
mark_edge_unresolvable's precondition docstring to sanction the cartog-side unlocatable determination alongside the serverOk(None)case.Tests
unlocatable_edges_are_marked_unresolvable— folds intomarked_unresolvablealongside a real success.unlocatable_marked_even_with_zero_resolved— proves the health-gate difference vspending_unresolvable;server_died=truesuppresses.unlocatable_edges_are_bucketed_and_not_queried(#[cfg(unix)], scripted fake server) — the unfindable edge lands inpending_unlocatableand is never queried; only the findable edge reaches the server.write_to_a_dead_server_fails_fast_via_broken_pipe_not_write_timeout(#[cfg(unix)]) — dead-server write returnsErrwell under a 30swrite_timeout, not classified as a wedge.Gates
cargo fmt --check,cargo clippy --all-targets -- -D warnings(+--no-default-features),cargo test -p cartog-lsp -p cartog-db(83 + 199),cargo test --workspace(+--no-default-features),cargo doc --no-deps(lsp + db, warning-clean) — all green.Docs
Internal resilience change (no new command/flag/config/MCP-tool/language/count), so site + reference docs untouched per doc-sync rule. Updated the cartog-lsp
README.mdoutcome list (adds the unlocatable branch) and onedocs/troubleshooting.mdentry.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation