From 5ee2093715b89e19e8d09c3c5b32b5671ca810aa Mon Sep 17 00:00:00 2001 From: NAVEENKUMARKR777 Date: Sun, 6 Sep 2026 23:39:52 +0000 Subject: [PATCH] Fix RefCountCache.Ref panic race between concurrent snapshot builds Fixes #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, #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 --- tsc/internal/project/project.go | 23 +++- tsc/internal/project/refcountcache.go | 46 +++++-- tsc/internal/project/refcountcache_test.go | 64 +++++++++ tsc/internal/project/snapshot_stress_test.go | 137 +++++++++++++++++++ 4 files changed, 256 insertions(+), 14 deletions(-) create mode 100644 tsc/internal/project/snapshot_stress_test.go diff --git a/tsc/internal/project/project.go b/tsc/internal/project/project.go index 80cacf0b7b834..9fe6a5e16135f 100644 --- a/tsc/internal/project/project.go +++ b/tsc/internal/project/project.go @@ -415,20 +415,33 @@ func (p *Project) CreateProgram() CreateProgramResult { // Use pointer identity: dirtyFile is the exact instance UpdateProgram acquired, // and it is the only file whose refcount is already accounted for. if file != dirtyFile && !file.IsContentMapperFailureStub() && !file.IsContentMapperSupplemental() { - // UpdateProgram acquired the changed file only, so we need to ref everything else + // UpdateProgram acquired the changed file only, so we need to ref everything else. + // We already hold file itself, so RefOrAcquire (rather than Ref) tolerates losing + // a benign race against a concurrent, independent snapshot build that drops the + // last other claim on this cache entry between our lookup and our lock (e.g. a + // normal edit racing a speculative auto-import clone sharing this file's old + // Program, see GetLanguageServiceWithAutoImports): recreating the entry from a + // value we already possess is always correct. if file.ContentMapper() != "" { - p.host.builder.contentMappedParseCache.Ref(contentMappedParseCacheKeyForFile(file)) + p.host.builder.contentMappedParseCache.RefOrAcquire( + contentMappedParseCacheKeyForFile(file), + contentmapper.SourceFiles{Canonical: file, Supplemental: file.SupplementalSourceFiles()}, + ) } else { - p.host.builder.parseCache.Ref(parseCacheKeyForFile(file)) + p.host.builder.parseCache.RefOrAcquire(parseCacheKeyForFile(file), file) } } } for _, file := range newProgram.DuplicateSourceFiles() { if !file.IsContentMapperFailureStub { + // Duplicates are pure bookkeeping refs on an entry acquired elsewhere: we have no + // value to recreate it with, so RefIfPresent no-ops if it loses the same race + // described above. That's safe because the matching Deref issued when this + // bookkeeping owner is later released already tolerates a missing entry. if file.ContentMapper != "" { - p.host.builder.contentMappedParseCache.Ref(contentMappedParseCacheKeyForDuplicate(file)) + p.host.builder.contentMappedParseCache.RefIfPresent(contentMappedParseCacheKeyForDuplicate(file)) } else { - p.host.builder.parseCache.Ref(parseCacheKeyForDuplicate(file)) + p.host.builder.parseCache.RefIfPresent(parseCacheKeyForDuplicate(file)) } } } diff --git a/tsc/internal/project/refcountcache.go b/tsc/internal/project/refcountcache.go index 7a3c01d0f2ed5..a4072e876499e 100644 --- a/tsc/internal/project/refcountcache.go +++ b/tsc/internal/project/refcountcache.go @@ -80,23 +80,51 @@ func (c *RefCountCache[K, V, AcquireArgs]) AcquireOrError(identity K, produce fu return value, nil } -// Ref increments the reference count for an existing entry. -// Panics if the entry does not exist. -func (c *RefCountCache[K, V, AcquireArgs]) Ref(identity K) { +// RefOrAcquire increments the reference count for an existing entry, or +// installs value as a fresh entry with refCount 1 if none exists. +// +// It never panics on a missing entry. It exists for callers that already +// hold value from elsewhere (e.g. a *ast.SourceFile reused from +// an old Program while cloning a new one) and are re-establishing their own +// claim on it. Such callers can legitimately race with a concurrent Deref of +// the last other claim on the same identity: two independent snapshot builds +// (for example a normal edit and a speculative auto-import clone, see +// GetLanguageServiceWithAutoImports) can each be cloning from the same +// shared Program concurrently, and the moment the file's last other owner +// releases it can fall between this call's initial lookup and its lock +// acquisition. Since the caller already possesses a valid value for +// identity, recreating the entry is always safe: it never returns a value +// the caller didn't already have. +func (c *RefCountCache[K, V, AcquireArgs]) RefOrAcquire(identity K, value V) { + entry, loaded := c.loadOrStoreNewLockedEntry(identity) + if !loaded { + entry.value = value + } + entry.mu.Unlock() +} + +// RefIfPresent increments the reference count for an existing entry and +// reports true, or does nothing and reports false if no entry exists. +// +// It exists for callers that are recording an additional owner of an entry +// they do not themselves have a value for (e.g. a duplicate source file, +// which is only ever a bookkeeping reference to a canonical entry acquired +// elsewhere). Skipping the ref when the entry is already gone is safe: the +// corresponding Deref for this same identity, issued later when the +// bookkeeping owner is released, is itself a no-op against a missing entry. +func (c *RefCountCache[K, V, AcquireArgs]) RefIfPresent(identity K) bool { entry, ok := c.entries.Load(identity) if !ok { - panic("cache entry not found") + return false } 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 } entry.refCount++ + return true } // Deref decrements the reference count for an entry. diff --git a/tsc/internal/project/refcountcache_test.go b/tsc/internal/project/refcountcache_test.go index 9af1a6eab689d..f6552d1cb7dd6 100644 --- a/tsc/internal/project/refcountcache_test.go +++ b/tsc/internal/project/refcountcache_test.go @@ -89,6 +89,70 @@ func TestParseCacheBindsBeforePublishing(t *testing.T) { assert.Assert(t, file.CommonJSModuleIndicator != nil) } +func TestRefOrAcquireRecreatesConcurrentlyDeletedEntry(t *testing.T) { + t.Parallel() + + cache := NewParseCache(RefCountCacheOptions{}) + key := NewParseCacheKey(ast.SourceFileParseOptions{FileName: "/a.ts", Path: "/a.ts"}, xxh3.Hash128([]byte("a")), core.ScriptKindTS) + file := &ast.SourceFile{} + + // A caller holding file (e.g. reused, by pointer, from an old Program while + // cloning a new one) can lose a benign race: some other, independent owner + // derefs the entry to zero and it's deleted from the map entirely before + // this caller gets a chance to record its own claim. Plain Ref would panic + // in that situation (see refcountcache.go); RefOrAcquire must instead + // recreate the entry from the value the caller already has. + assert.Assert(t, !cache.Has(key)) + cache.RefOrAcquire(key, file) + assert.Assert(t, cache.Has(key)) + entry, ok := cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 1) + assert.Assert(t, entry.value == file) + + // A second RefOrAcquire for a live entry behaves like Ref: it bumps the + // existing entry rather than replacing its value. + other := &ast.SourceFile{} + cache.RefOrAcquire(key, other) + entry, ok = cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 2) + assert.Assert(t, entry.value == file) + + cache.Deref(key) + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) +} + +func TestRefIfPresentSkipsMissingEntry(t *testing.T) { + t.Parallel() + + cache := NewParseCache(RefCountCacheOptions{}) + key := NewParseCacheKey(ast.SourceFileParseOptions{FileName: "/a.ts", Path: "/a.ts"}, xxh3.Hash128([]byte("a")), core.ScriptKindTS) + + // Duplicates are bookkeeping-only refs on an entry owned elsewhere: there's + // no value on hand to recreate it with, so a missing entry must be a no-op + // (never a panic) rather than fabricating a zero-value entry. + assert.Equal(t, cache.RefIfPresent(key), false) + assert.Assert(t, !cache.Has(key)) + + file := &ast.SourceFile{} + cache.RefOrAcquire(key, file) + assert.Equal(t, cache.RefIfPresent(key), true) + entry, ok := cache.entries.Load(key) + assert.Assert(t, ok) + assert.Equal(t, entry.refCount, 2) + + cache.Deref(key) + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) + + // The corresponding Deref for a duplicate whose RefIfPresent no-op'd must + // also no-op rather than panicking or corrupting an unrelated entry. + cache.Deref(key) + assert.Assert(t, !cache.Has(key)) +} + func TestRefCountingCaches(t *testing.T) { t.Parallel() diff --git a/tsc/internal/project/snapshot_stress_test.go b/tsc/internal/project/snapshot_stress_test.go new file mode 100644 index 0000000000000..1ad52f5d36680 --- /dev/null +++ b/tsc/internal/project/snapshot_stress_test.go @@ -0,0 +1,137 @@ +package project + +import ( + "context" + "fmt" + "sync" + "sync/atomic" + "testing" + + "github.com/microsoft/TypeScript/tsc/internal/bundled" + "github.com/microsoft/TypeScript/tsc/internal/lsp/lsproto" + "github.com/microsoft/TypeScript/tsc/internal/vfs/vfstest" + "gotest.tools/v3/assert" +) + +// TestSnapshotConcurrentAutoImportCloneDoesNotPanic reproduces +// https://github.com/microsoft/TypeScript/issues/63844: a "cache entry not +// found" panic in RefCountCache.Ref hit by real monorepo users of the +// language server. +// +// Two entry points can build a new Snapshot from a shared base: the normal, +// serialized edit path (getSnapshot/updateSnapshot, under snapshotUpdateMu) +// and the speculative auto-import clone used by completions needing +// auto-imports (CloneSnapshotWithAutoImports, used by +// GetLanguageServiceWithAutoImports and warmAutoImportCache), which does NOT +// go through snapshotUpdateMu. 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; a concurrent auto-import clone that reuses one of +// those files via Project.CreateProgram's clone path can lose a benign race +// against a concurrent edit's disposal of an older generation, such that the +// file's cache entry is gone by the time the clone tries to Ref it. +// +// Neither concurrent edits alone nor concurrent auto-import clones alone +// (against an otherwise idle session) are sufficient to reproduce this; it +// takes both running at once, which is what this test drives. +func TestSnapshotConcurrentAutoImportCloneDoesNotPanic(t *testing.T) { + if !bundled.Embedded { + t.Skip("bundled files are not embedded") + } + + const numProjects = 6 + + files := map[string]any{} + for i := range numProjects { + files[fmt.Sprintf("/home/projects/TS/p%d/tsconfig.json", i)] = "{}" + files[fmt.Sprintf("/home/projects/TS/p%d/index.ts", i)] = "import { foo } from './foo'; export const value = foo;" + files[fmt.Sprintf("/home/projects/TS/p%d/foo.ts", i)] = "export const foo = 1;" + } + + fs := bundled.WrapFS(vfstest.FromMap(files, false /*useCaseSensitiveFileNames*/)) + session := NewSession(&SessionInit{ + BackgroundCtx: context.Background(), + Options: &SessionOptions{ + CurrentDirectory: "/", + DefaultLibraryPath: bundled.LibPath(), + TypingsLocation: "/home/src/Library/Caches/typescript", + PositionEncoding: lsproto.PositionEncodingKindUTF8, + WatchEnabled: false, + LoggingEnabled: false, + }, + FS: fs, + }) + defer session.Close() + + ctx := context.Background() + uris := make([]lsproto.DocumentUri, numProjects) + for i := range numProjects { + uri := lsproto.DocumentUri(fmt.Sprintf("file:///home/projects/TS/p%d/index.ts", i)) + uris[i] = uri + session.DidOpenFile(ctx, uri, 1, files[fmt.Sprintf("/home/projects/TS/p%d/index.ts", i)].(string), lsproto.LanguageKindTypeScript) + _, err := session.GetLanguageService(ctx, uri) + assert.NilError(t, err) + } + + var version int32 = 1 + var wg sync.WaitGroup + stop := make(chan struct{}) + + // Goroutines that keep editing files, forcing a steady stream of new + // snapshot generations (and disposal of old ones) via the normal, + // snapshotUpdateMu-serialized path. + for i := range numProjects { + wg.Add(1) + go func(i int) { + defer wg.Done() + uri := uris[i] + for { + select { + case <-stop: + return + default: + } + v := atomic.AddInt32(&version, 1) + session.DidChangeFile(ctx, uri, v, []lsproto.TextDocumentContentChangePartialOrWholeDocument{ + { + WholeDocument: &lsproto.TextDocumentContentChangeWholeDocument{ + Text: fmt.Sprintf("import { foo } from './foo'; export const value = foo; export const v = %d;", v), + }, + }, + }) + _, _ = session.GetLanguageService(ctx, uri) + } + }(i) + } + + // Goroutines that repeatedly take a speculative auto-import clone off of + // whatever the current snapshot happens to be, mimicking the + // ErrNeedsAutoImports path used by completions (ctrl+space), which does + // NOT go through snapshotUpdateMu. + for i := range numProjects { + wg.Add(1) + go func(i int) { + defer wg.Done() + uri := uris[i] + for { + select { + case <-stop: + return + default: + } + baseSnapshot := session.Snapshot() + preparedSnapshot := session.SnapshotHost.CloneSnapshotWithAutoImports(ctx, baseSnapshot, uri, nil) + session.TryAdoptSnapshotInBackground(baseSnapshot, preparedSnapshot) + preparedSnapshot.Deref() + } + }(i) + } + + // Let the race run for a bounded number of edit cycles rather than wall time. + for n := 0; n < 400; n++ { + _, _ = session.GetLanguageService(ctx, uris[n%numProjects]) + } + close(stop) + wg.Wait() + session.WaitForBackgroundTasks() +}