fix(code): serialize AUTHENTICATION_ADDITIONAL_URLS writes to fix data race [IDE-2169]#1364
Open
basti-snyk wants to merge 2 commits into
Open
fix(code): serialize AUTHENTICATION_ADDITIONAL_URLS writes to fix data race [IDE-2169]#1364basti-snyk wants to merge 2 commits into
basti-snyk wants to merge 2 commits into
Conversation
…a race [IDE-2169] CodeConfig.SnykCodeApi and updateCodeApiLocalEngine both performed an unsynchronized read-modify-write (GetStringSlice -> append -> Set) on the shared engine configuration key AUTHENTICATION_ADDITIONAL_URLS. Each workspace-folder scan creates its own *CodeConfig but they share one workflow.Engine configuration, so concurrent scans raced on the slice and the -race detector aborted Test_SmokeIssueCaching. Remove the useless per-instance mutex (it could never guard cross-instance access) and introduce a single package-level authURLsMu sync.Mutex that is the one exclusion domain for every writer of the key in this package. Guard both write paths with it and add a slices.Contains idempotency check so repeated scans of the same URL no longer append duplicates. Tests: three concurrent -race tests reproduce the failure (same-instance, cross-instance, and SnykCodeApi-vs-updateCodeApiLocalEngine). They fire the data race without the fix (RED) and pass with it (GREEN).
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
PR Reviewer Guide 🔍
|
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
1 participant
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.
What & why IDE-2169
Fixes a data race on
AUTHENTICATION_ADDITIONAL_URLSin thecodepackage, surfaced as a flaky test.Root cause: every
CodeConfiginstance andupdateCodeApiLocalEngine()share oneengine.GetConfiguration()object and each performed an unsynchronized read-modify-write onAUTHENTICATION_ADDITIONAL_URLS. A per-instance mutex was insufficient — distinct*CodeConfiginstances each hold a different lock, leaving concurrent cross-instance and local-engine writes unprotected.Fix: introduce a package-level
var authURLsMu sync.Mutexincodeconfig.gothat all writers ofAUTHENTICATION_ADDITIONAL_URLS(CodeConfig.SnykCodeApi()andupdateCodeApiLocalEngine()) acquire before their read-modify-write. The misleading per-instancemufield is removed. Theslices.Containsidempotency guard is retained and also applied toupdateCodeApiLocalEngineso repeated registrations don't accumulate duplicate URLs.This is a real synchronization fix, not a test skip/retry.
Tests
Replaced the single-instance test with three race-detector tests:
Test_SnykCodeApi_ConcurrentAccess_SameInstance(50 goroutines)Test_SnykCodeApi_ConcurrentAccess_CrossInstance(two*CodeConfigsharing one engine — reproduces the production race)Test_SnykCodeApi_ConcurrentWithLocalEngine(SnykCodeApiracingupdateCodeApiLocalEngine)Evidence: RED (per-instance mutex / no guard) →
WARNING: DATA RACE+ count assertion fail; GREEN after fix at-race -count=50; fullmake testsuite green at commit2ee57b0; lint 0, gofmt clean.🤖 Opened by the automated flake-fix loop.