From 46f4dada4eb8e15a9dd76d4256e774fff68ba70b Mon Sep 17 00:00:00 2001 From: Christopher Plieger <917744+cplieger@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:02:21 +0100 Subject: [PATCH] fix: parse a new owner cache entry without holding its lock LoadAndAcquire still returns a live, unexpired entry under its lock. Otherwise it parses with no lock held, then stores the value and adds the owner under the entry's lock, so two loads that re-enter the cache for each other no longer wait on each other's lock. Concurrent loads of one key that find no live, unexpired entry may each parse it; the first value stored wins and a later parse is dropped. Tested with go vet and go test -race on internal/project and internal/api, npx hereby lint, npx hereby check:format and npx hereby test, all passing. TestOwnerCacheConcurrentlyLoadKeysThatLoadEachOther fails on main with its deadlock message and passes here. TestOwnerCacheConcurrentLoadsKeepFirstStoredValue fails on main after 10 seconds because the second load waits on the first parse's lock, fails on this change with every parse stored because the slower parse replaces the first value, and passes here. --- tsc/internal/project/ownercache.go | 13 +++- tsc/internal/project/ownercache_test.go | 98 +++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tsc/internal/project/ownercache_test.go diff --git a/tsc/internal/project/ownercache.go b/tsc/internal/project/ownercache.go index 801233d748b03..294e73d79da07 100644 --- a/tsc/internal/project/ownercache.go +++ b/tsc/internal/project/ownercache.go @@ -36,10 +36,21 @@ func NewOwnerCache[K comparable, V any, LoadArgs any]( } func (c *OwnerCache[K, V, LoadArgs]) LoadAndAcquire(identity K, owner uint64, loadArgs LoadArgs) V { + if entry, ok := c.entries.Load(identity); ok { + entry.mu.Lock() + if len(entry.owners) > 0 && (c.isExpired == nil || !c.isExpired(identity, entry.value, loadArgs)) { + entry.owners[owner] = struct{}{} + value := entry.value + entry.mu.Unlock() + return value + } + entry.mu.Unlock() + } + value := c.parse(identity, loadArgs) entry, loaded := c.loadOrStoreLockedEntry(identity) defer entry.mu.Unlock() if !loaded || c.isExpired != nil && c.isExpired(identity, entry.value, loadArgs) { - entry.value = c.parse(identity, loadArgs) + entry.value = value } entry.owners[owner] = struct{}{} return entry.value diff --git a/tsc/internal/project/ownercache_test.go b/tsc/internal/project/ownercache_test.go new file mode 100644 index 0000000000000..78ae23eb54476 --- /dev/null +++ b/tsc/internal/project/ownercache_test.go @@ -0,0 +1,98 @@ +package project + +import ( + "slices" + "sync" + "testing" + "time" + + "gotest.tools/v3/assert" +) + +type ownerCacheTestArgs struct { + owner uint64 + stack []string +} + +func TestOwnerCacheConcurrentlyLoadKeysThatLoadEachOther(t *testing.T) { + t.Parallel() + + extends := map[string]string{"a": "b", "b": "a"} + var started sync.WaitGroup + started.Add(2) + var cache *OwnerCache[string, string, ownerCacheTestArgs] + cache = NewOwnerCache(func(key string, args ownerCacheTestArgs) string { + if len(args.stack) == 0 { + started.Done() + started.Wait() + } + stack := append(slices.Clone(args.stack), key) + if !slices.Contains(stack, extends[key]) { + cache.LoadAndAcquire(extends[key], args.owner, ownerCacheTestArgs{owner: args.owner, stack: stack}) + } + return key + }, nil) + + values := make([]string, 2) + var wg sync.WaitGroup + for i, key := range []string{"a", "b"} { + wg.Go(func() { + values[i] = cache.LoadAndAcquire(key, uint64(i+1), ownerCacheTestArgs{owner: uint64(i + 1)}) + }) + } + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + select { + case <-done: + assert.DeepEqual(t, values, []string{"a", "b"}) + assert.Assert(t, cache.Has("a")) + assert.Assert(t, cache.Has("b")) + case <-time.After(10 * time.Second): + t.Fatal("loads of keys that load each other deadlocked") + } +} + +func TestOwnerCacheConcurrentLoadsKeepFirstStoredValue(t *testing.T) { + t.Parallel() + + parsing := make(chan struct{}) + release := make(chan struct{}) + cache := NewOwnerCache(func(key string, value string) string { + if value == "slow" { + close(parsing) + <-release + } + return value + }, nil) + + slow := make(chan string) + go func() { + slow <- cache.LoadAndAcquire("a", 1, "slow") + }() + fast := make(chan string) + select { + case <-parsing: + go func() { + fast <- cache.LoadAndAcquire("a", 2, "fast") + }() + case <-time.After(10 * time.Second): + t.Fatal("the first load never started its parse") + } + select { + case value := <-fast: + assert.Equal(t, value, "fast") + case <-time.After(10 * time.Second): + t.Fatal("a load waited for another load's parse") + } + close(release) + select { + case value := <-slow: + assert.Equal(t, value, "fast") + case <-time.After(10 * time.Second): + t.Fatal("the first load never returned after its parse finished") + } + assert.Equal(t, cache.LoadAndAcquire("a", 3, "other"), "fast") +}