perf(lsp): parse-pipeline hardening — immutable docs, partial-result cache, cancellation, workspace pruning - #9
Merged
Conversation
The store guards its map with a mutex but handed out shared *Document pointers that DidChange and parseDocument then mutated in place (content, version, parse result). The mutex protects the map slots, not the structs they point to, so a reader holding an earlier snapshot could observe a torn update. It is latent today only because jsonrpc2.AsyncHandler serializes handler bodies (audit I1). - DidChange now builds a new *Document instead of mutating the stored one, and clears the cached result since the content changed. - Add Store.SetResult: attaches a parse result via copy-on-write with a version guard, so a stale parse is never cached and snapshots stay immutable. parseDocument uses it instead of mutating doc.Result, and passes the result explicitly to importDiagnostics. - Test pins the invariant: a snapshot taken before DidChange keeps its content/version.
While a document is mid-edit and fails to parse, parseDocument cleared the cached result. Navigation then re-parsed the same invalid buffer on every request (parsePathForNavigation re-parses when the cache is empty) and produced the identical best-effort AST — so clearing bought nothing but repeated work (audit I3). Cache the partial result (Root is populated even on parse errors) so definition/hover/completion reuse it instead of re-parsing per request. Diagnostics are unaffected — they still come from result.Errors. Repurposed the existing valid-to-invalid test to pin the retained-partial behavior.
Handlers ignored ctx entirely, so a client $/cancelRequest cancelled nothing and a long import-chasing parse always ran to completion (audit I6). - Parser.Parse takes a context and checks it before each (recursive) parse, so a cancelled or superseded request stops walking the import graph promptly. - The diagnostics path (parseDocument, importDiagnostics) threads the request ctx through. - parsePathForNavigation deliberately keeps using context.Background(): it is a fast single-file parse, and threading ctx through the resolution-callback layer would be high-churn for negligible benefit. The workspace-walk cancellation (the other heavy path) is handled in the I7 commit.
find-references, workspace-symbols, and missing-import quick-fixes scan the workspace root synchronously on the request. The scan descended into every directory and swallowed walk errors (audit I7), so on a monorepo it walked .git/node_modules/vendor for nothing and a transient FS error silently truncated results while looking complete. - Prune .git, node_modules, vendor, and hidden directories (they never hold project schemas). - Log unreadable entries and an incomplete walk via the server logger instead of discarding the error. Deferred (needs broader plumbing / file-watching, tracked with I2): threading request cancellation into the walk and a cached, watch-invalidated path index.
Every caller ignores the bool, so it was dead surface. Removing it also drops the lone what-comment from the godoc, leaving only the why.
After I6 threaded a cancellable ctx into the parser, a cancelled or superseded request made Parse return ctx.Err(), which parseDocument turned into a bogus line-1 "context canceled" diagnostic and cached a nil result — clobbering the document's real diagnostics mid-edit. Found in self-review. Treat a context error as stop, not failure: parseDocument returns no diagnostics and leaves the cache intact, and parseAndPublishDiagnostics skips publishing when the request is already cancelled.
The version-mismatch drop path is the reason SetResult takes a version, but no test exercised it. Add unit tests for the stale-version drop and the gone-document no-op.
Codex adversarial review found two gaps in the initial cancellation fix: - Import recursion (buildPartialSchema) swallows ctx.Canceled as a skipped import, so Parse can return err==nil with an incomplete result after a cancel. parseDocument only checked ctx in the err\!=nil branch, so it cached that incomplete result. Move the ctx guard above the result handling so a cancelled request never caches or publishes. - importDiagnostics threaded ctx but still called parsePathForNavigation, which uses context.Background(), leaking cancellation on the transitive-reimport path. Extract a ctx-aware parsePath (parsePathForNavigation now delegates to it with Background) and use it from importDiagnostics.
Codex follow-up on I2: importDiagnostics still reached the background parser via uniqueImportCandidatePath, which scans and parses every workspace candidate. Thread ctx into uniqueImportCandidatePath — it now bails when the request is cancelled and parses candidates with the request ctx. The diagnostics caller passes the real ctx; interactive code-action callers pass context.Background(), consistent with parsePathForNavigation.
klaidliadon
force-pushed
the
ridl-lsp-hardening/parse-pipeline
branch
from
June 20, 2026 07:05
5c318c4 to
855168c
Compare
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.
The parse pipeline had a latent data race (the store handed out shared mutable
*Documentpointers), re-parsed the same buffer on every keystroke during edits, ignored cancellation entirely, and walked the whole workspace —.git/node_modulesincluded — on every find-references. This hardens all four without changing behavior users rely on.Second of the prod-readiness stack. Depends on #8 — merge that first.
Changes (audit findings)
store.go,server.go):DidChangebuilds a new*Documentinstead of mutating the shared one;Store.SetResultattaches parse results via COW with a version guard, so a slow parse of superseded content can't clobber a newer result. Latent race today (handlers are serialized byAsyncHandler), but the store no longer relies on that for safety.diagnostics.go): the parser produces a usable AST even on error; caching it lets navigation reuse the partial result instead of re-parsing the same invalid buffer per request.parser.go,diagnostics.go):Parser.Parsetakes actxand bails before each recursive import parse; the diagnostics path threads it through. A cancelled request no longer surfacesctx.Err()as a diagnostic, clears diagnostics, or caches incomplete work.references.go): prune.git/node_modules/vendor/hidden dirs and surface walk errors via the logger instead of swallowing them.Scope boundary (deliberate)
parsePathForNavigation(interactive nav) and interactive code-actions intentionally usecontext.Background()— single-file parses where threading ctx through the resolution-callback layer is high-churn for negligible benefit. The cancellable callers (diagnostics) use the ctx-awareparsePath.Deferred (NOT in this PR — flagged for review)
WalkDiritself and a cached, watch-invalidated path index (needs file-watching plumbing).Test plan
New tests: COW invariant (
document_cow_test.go), partial-result retention (repurposeddiagnostics_test.go),SetResultversion guard + missing-doc no-op (documents/store_test.go), parser + parsePath cancellation (parser_test.go,cancellation_test.go), workspace dir pruning (workspace_walk_test.go).Known coverage gap: the
WalkDirper-entry error-logging branch (references.go) is not unit-tested — injecting an unreadable entry hermetically is permission-dependent and flaky in CI. The pruning happy-path is covered; the error branch is by-inspection.Review
Self-review caught a Critical (cancellation surfaced a bogus "context canceled" diagnostic) — fixed. A Codex adversarial pass (via agent-comms, 3 rounds to convergence) caught two more Important issues: cache poisoning when cancellation hit mid-import-recursion, and the diagnostics candidate-scan still escaping cancellation through the background parser. Both fixed; Codex approved the final revision. Security-review: no findings (
WalkDirdoesn't follow symlinks, so the scan is narrowed, not a new traversal surface).Stack:
ridl-lsp-hardeningThis stack is managed with sdf.