Fix RefCountCache.Ref panic race between concurrent snapshot builds - #64184
Open
NAVEENKUMARKR777 wants to merge 1 commit into
Open
Fix RefCountCache.Ref panic race between concurrent snapshot builds#64184NAVEENKUMARKR777 wants to merge 1 commit into
NAVEENKUMARKR777 wants to merge 1 commit into
Conversation
Fixes microsoft#63844. Two independent code paths can build a new project Snapshot from the same base: normal edits (serialized under Session.snapshotUpdateMu) and the speculative auto-import clone used by completions needing auto-imports (CloneSnapshotWithAutoImports / warmAutoImportCache), which intentionally bypasses that mutex so completions don't stall on edits. Both read and mutate the same host-level, ref-counted parseCache/contentMappedParseCache. When a project's Program is unchanged across several edits, it (and its files) stay shared across many snapshot generations. Project. CreateProgram's clone path re-refs those reused files via RefCountCache.Ref, assuming the entry must still exist because some other owner is presumed to still hold it. That assumption can lose a benign race: a concurrent, independent snapshot build can drop the last other claim on the same entry between this call's lookup and its lock acquisition, and Ref panics. Two prior attempts to fix this by making Ref tolerate the race (microsoft/typescript-go#4400, microsoft#4455) were closed as "wrong fix" on the belief that the race shouldn't be reachable. It is: this change adds a stress test (TestSnapshotConcurrentAutoImportCloneDoesNotPanic) that drives both paths concurrently across several projects sharing file content and reproduces the exact reported panic in well under a second. Two isolation variants (edits alone, auto-import clones alone against an idle session) do not reproduce it - it takes both. The fix replaces the panicking Ref with two methods that make the existing recovery behavior (already used for a narrower window in the old code) total instead of partial: - RefOrAcquire(identity, value): re-refs an existing entry, or recreates it from a value the caller already possesses. Used at the two call sites in CreateProgram that hold the *ast.SourceFile they're re-claiming, so recreating the entry can never hand back a value the caller didn't already have. - RefIfPresent(identity): refs an existing entry or safely no-ops. Used for duplicate source file bookkeeping refs, which have no value to recreate the entry with; a later no-op is safe because the matching Deref for a duplicate already tolerates a missing entry. Ref itself is now unused and removed, so a future call site can't reintroduce this panic by picking the wrong method. Validated with `go build ./...`, `go vet`, `gofmt`, and the full internal/project, internal/lsp, and internal/api suites under -race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new stress test should retain a ref to the base snapshot during cloning, and RefIfPresent’s current “return false on deleted-while-locking” behavior can lead to ref/deref accounting hazards if the key is recreated before disposal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes an LSP crash (panic: cache entry not found) by making refcounted parse-cache re-referencing resilient to concurrent snapshot builds, and adds stress/unit tests to reproduce and guard the race.
Changes:
- Replace panicking
Refwith non-panickingRefOrAcquireandRefIfPresentinRefCountCache. - Update
Project.CreateProgramclone/ref paths to use the new APIs for both regular and duplicate source-file bookkeeping. - Add a concurrent snapshot stress test plus cache-primitive unit tests to cover missing-entry race scenarios.
File summaries
| File | Description |
|---|---|
| tsc/internal/project/snapshot_stress_test.go | New concurrency stress test that drives edit updates and auto-import snapshot clones in parallel. |
| tsc/internal/project/refcountcache.go | Introduces non-panicking ref APIs intended to tolerate cache-entry deletion races. |
| tsc/internal/project/refcountcache_test.go | Adds unit tests for RefOrAcquire/RefIfPresent behavior around missing entries. |
| tsc/internal/project/project.go | Switches program-clone ref accounting to use RefOrAcquire/RefIfPresent instead of Ref. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
120
to
+124
| entry.mu.Lock() | ||
| defer entry.mu.Unlock() | ||
| if entry.refCount <= 0 && !c.Options.DisableDeletion { | ||
| // Entry was deleted while we were acquiring the lock | ||
| newEntry, _ := c.loadOrStoreNewLockedEntry(identity) | ||
| defer newEntry.mu.Unlock() | ||
| newEntry.value = entry.value | ||
| return | ||
| // Entry was deleted while we were acquiring the lock. | ||
| return false |
Comment on lines
+122
to
+125
| baseSnapshot := session.Snapshot() | ||
| preparedSnapshot := session.SnapshotHost.CloneSnapshotWithAutoImports(ctx, baseSnapshot, uri, nil) | ||
| session.TryAdoptSnapshotInBackground(baseSnapshot, preparedSnapshot) | ||
| preparedSnapshot.Deref() |
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.
AI assistance disclosure
Per CONTRIBUTING.md: this patch was authored with Claude Code. I've read and understood the diagnosis and the change, verified the reproduction and fix locally (see below), and I'll be the one responding to review feedback.
Summary
Fixes #63844 (
panic: cache entry not found), a language-server crash reported by several monorepo users and reproduced by Ryan Cavanaugh (@RyanCavanaugh), but without a reliable repro to work from at the time.Root cause: two independent paths can build a new project
Snapshotfrom the same base and both read/mutate the same host-level, ref-countedparseCache/contentMappedParseCache:Session.snapshotUpdateMu.CloneSnapshotWithAutoImports/warmAutoImportCache), which intentionally does not take that mutex, so completions don't stall behind edits.When a project's
Programis unchanged across several edits, it (and its files) stay shared across many snapshot generations.Project.CreateProgram's clone path re-refs those reused files viaRefCountCache.Ref, assuming the entry must still exist because it presumes some other owner still holds it. That assumption can lose a benign race: an independent, concurrent snapshot build can drop the last other claim on the same entry between this call's lookup and its lock acquisition, andRefpanics.I want to flag directly: two earlier attempts at fixing this (microsoft/typescript-go#4400 by a Copilot agent, and microsoft/typescript-go#4455 by Ryan Cavanaugh (@RyanCavanaugh) himself) took essentially the same approach and were both closed as "wrong fix," on the reasoning that this race "shouldn't" be reachable given the intended invariant (a new snapshot refs its files before its parent's disposal derefs them). I believe it is reachable — see the test below, which reproduces the exact panic reliably and quickly. If there's context on why this approach was rejected that isn't captured in those PR threads, I'd genuinely like to hear it before this goes further.
Reproduction
Added
TestSnapshotConcurrentAutoImportCloneDoesNotPanic, which drives both paths concurrently across 6 projects with shared file content. On the pre-fix code it reproduces the reported panic in well under a second:I also checked two isolation variants (concurrent edits alone; concurrent auto-import clones alone against an otherwise idle session) — neither reproduces it on its own. It takes both paths running at once, which is likely why it evaded repro attempts based on either edits or completions in isolation.
Fix
RefCountCache.Refalready had a partial recovery path for a narrower window of the same race (entry found, but its refcount had already dropped to zero before the lock was acquired). This change makes that recovery total instead of partial, replacingRefwith two non-panicking methods:RefOrAcquire(identity, value): re-refs an existing entry, or recreates it from a value the caller already possesses. Used at the twoCreateProgramcall sites that hold the*ast.SourceFile(or content-mapper bundle) they're re-claiming — since the caller already has a valid value in hand, recreating the entry can never hand back something it didn't already have.RefIfPresent(identity): refs an existing entry, or safely no-ops. Used for duplicate-source-file bookkeeping refs, which have no value to recreate an entry with; a no-op here is safe because the matchingDerefissued later for the same duplicate already tolerates a missing entry.Refitself is now dead code (no remaining callers) and is removed, so a future call site can't reintroduce this panic by reaching for the wrong method.Testing
go build ./...go vet ./internal/project/...gofmt -l(clean)go test ./internal/project/... ./internal/lsp/... ./internal/api/... -race(all pass, including the new stress test at 15/15 runs)RefOrAcquireandRefIfPresentcovering the "entry deleted before ref" scenario directly on the cache primitiveI did not run the full
npx hereby test:all/ lint pipeline (it needs the Node/npm toolchain and a custom golangci-lint build I couldn't easily set up in this environment); happy to address anything CI surfaces.🤖 Generated with Claude Code