Fix concurrent map writes in Manager.LookupSdk #610
Conversation
Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>
Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>
…ocking Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>
Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>
Co-authored-by: aooohan <40265686+aooohan@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
==========================================
+ Coverage 21.94% 22.35% +0.40%
==========================================
Files 80 80
Lines 5376 5387 +11
==========================================
+ Hits 1180 1204 +24
+ Misses 4065 4047 -18
- Partials 131 136 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical concurrent map access bug in Manager.LookupSdk that was causing fatal panics when multiple goroutines called the method simultaneously (as occurs in activate.go). The fix adds thread-safe synchronization using sync.RWMutex and corrects a cache key inconsistency.
Changes:
- Added
sync.RWMutexto theManagerstruct to synchronize access to theopenSdksmap - Fixed cache key normalization inconsistency where reads used the original name but writes used
strings.ToLower(name) - Added comprehensive concurrency tests covering multiple scenarios including concurrent lookups, cache hits, and the actual activate.go use case
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/manager.go | Added RWMutex field and synchronized all map access operations in LookupSdk, LoadAllSdk, and Close methods |
| internal/manager_test.go | Added 5 new test functions to verify thread-safe concurrent access to the SDK manager |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m.mu.Lock() | ||
| m.openSdks[normalizedName] = s | ||
| m.mu.Unlock() |
There was a problem hiding this comment.
There's a potential race condition where two goroutines could both pass the cache check (lines 83-85), then both load the SDK (line 102), and both attempt to cache it. While this won't cause a panic with the mutex, it creates unnecessary duplicate work and could lead to resource leaks if sdk.NewSdk allocates resources. Consider implementing a double-check lock pattern: after acquiring the write lock, check if another goroutine has already cached the SDK, and if so, close the newly created SDK and return the cached one instead.
| if err == nil { | ||
| t.Errorf("Expected error for non-existent SDK, got nil") | ||
| } |
There was a problem hiding this comment.
The test uses t.Errorf inside a goroutine, which is unsafe and can cause races on the testing.T object. Use t.Error or better yet, collect errors in a channel and check them after wg.Wait(), or use a mutex-protected slice to collect error messages.
| // We expect errors since these SDKs don't exist | ||
| // But we should not panic or race | ||
| if err == nil { | ||
| t.Errorf("Expected error for non-existent SDK %s, got nil", sdkName) |
There was a problem hiding this comment.
The test uses t.Errorf inside a goroutine, which is unsafe and can cause races on the testing.T object. Use t.Error or better yet, collect errors in a channel and check them after wg.Wait(), or use a mutex-protected slice to collect error messages.
The
Manager.openSdksmap was accessed concurrently without synchronization, causing fatal panics whenactivate.gospawns multiple goroutines callingLookupSdk()in parallel.Changes
Added
sync.RWMutextoManagerstruct to protect map accessLookupSdkcache lookups) useRLockfor concurrent readsLookupSdk,LoadAllSdk) useLockfor exclusive accessClose()usesLockto prevent access during cleanupFixed cache key inconsistency where reads used original name but writes used
strings.ToLower(name), causing cache missesExample
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
version-fox.github.io/tmp/go-build1703830412/b355/http.test /tmp/go-build1703830412/b355/http.test -test.testlogfile=/tmp/go-build1703830412/b355/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build286749925/b156/vet.cfg x64/src/runtime/go1.24.11 4160351/b189/vet-symabis ache/go/1.24.11//tmp/go-build286749925/b252/symabis 0Y9pJDtq_ --64 -o 2.o -o x64/src/runtime/cgo -trimpath x64/pkg/tool/linux_amd64/compile--64 -p main -lang=go1.24 x64/pkg/tool/lin-buildtags(dns block)/tmp/go-build2841787517/b371/http.test /tmp/go-build2841787517/b371/http.test -test.testlogfile=/tmp/go-build2841787517/b371/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.short=true 749925/b192/_pkg-p -buildtags ux_amd64/compile-lang=go1.24 -errorsas g/x/crypto/inter-o -nilfunc ux_amd64/compile-trimpath -uns�� x64/src/os/user /tmp/go-build141github.com/version-fox/vfox/internal/shared/printer ux_amd64/vet jqT2/5HkHinSjDRibash me.go(dns block)/tmp/go-build1072179428/b355/http.test /tmp/go-build1072179428/b355/http.test -test.testlogfile=/tmp/go-build1072179428/b355/testlog.txt -test.paniconexit0 -test.timeout=10m0s o_.o�� x64/src/net -trimpath ux_amd64/vet -p github.com/shirols-files -lang=go1.23 ux_amd64/vet ter-�� ache/go/1.24.11/x64/src/os/user cfg x64/pkg/tool/linux_amd64/vet SR_6/YDHLU3HiOzV/bin/sh ions =0 x64/pkg/tool/linux_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.