fix: races during extension activation#704
Conversation
when multiple windows were opened and activated at the same time, global state was retrieved simultaneously, causing unintended duplicate execution of business logic.
✅ 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. |
Summary of ChangesHello @bastiandoetsch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses and resolves critical race conditions that could arise during the Snyk extension's activation, particularly when multiple VS Code windows were initialized simultaneously. The changes prevent unintended duplicate executions of core business logic, such as analytics event logging and modal displays, by introducing a controlled and safe mechanism for accessing and updating global state. This significantly enhances the extension's stability and ensures a consistent user experience across different VS Code window scenarios. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix a race condition during extension activation by checking for the focused window and adding a delay to state checks. While this is a step in the right direction, the use of a random delay is not a robust solution for preventing race conditions and can still lead to the issues it's trying to solve. I've identified a couple of areas for improvement to make the logic more resilient against race conditions, particularly around state updates for analytics events. My feedback includes specific suggestions to improve the atomicity of these operations.
bastiandoetsch
force-pushed
the
fix/race-condition-in-snyk-studio-enablement
branch
from
e487815 to
2a9a90e
Compare
Replace the suboptimal random delay approach with a proper file-based locking mechanism to prevent race conditions when multiple VS Code windows activate simultaneously. Changes: - Add FileLockService using atomic file operations (O_CREAT | O_EXCL) - Add globalStoragePath getter to ExtensionContext - Replace checkGlobalStateSafely() random delay with withLock() - Remove window.state.focused check (file lock handles synchronization) - Add 8 unit tests for FileLockService The file lock ensures only one window can execute the plugin-installed event logic at a time, preventing duplicate modal dialogs.
The no-await-in-loop warnings are intentional: - FileLockService retry loop requires sequential await for lock acquisition - Test teardown cleanup requires sequential file deletion
When lock file has empty or invalid JSON content (e.g., still being written), fall back to checking file modification time (mtime) instead of treating it as stale. This prevents a race condition where: 1. Process A creates lock file and starts writing JSON 2. Process B reads empty/partial content, treats as stale 3. Process B deletes lock and creates its own 4. Both processes now think they have the lock Now if JSON is invalid but mtime is recent, the lock is considered valid. Only if mtime is older than threshold is it considered stale.
Instead of immediately falling back to mtime check when JSON is invalid, retry reading the file content multiple times (5 attempts, 50ms apart). This gives the writing process time to complete. Only consider the lock stale if: 1. Valid JSON with old timestamp, OR 2. Invalid/empty content AND mtime is older than threshold This better handles the race condition where we read while another process is still writing the lock file content.
Move the globalState update outside of the analytics callback to ensure it happens synchronously while holding the lock, preventing race conditions where the callback runs after lock release.
rrama
left a comment
rrama
left a comment
There was a problem hiding this comment.
The solution is not the best file lock I have ever seen and has subtle bugs, but they are very unlikely to come up with our usage, those have all been protected against. So I will approve.
bastiandoetsch
deleted the
fix/race-condition-in-snyk-studio-enablement
branch
2 participants
Description
Problem:
When multiple VS Code windows are opened and activated simultaneously, a race condition occurs in
sendPluginInstalledEvent(). Multiple windows check global state concurrently, all see it as unset, and all proceed to show the "Secure at Inception" modal dialog, resulting in duplicate modals.Solution:
Replaced the suboptimal random delay approach with a proper file-based locking mechanism using atomic file operations.
Changes:
FileLockServiceusing Node.jsfs.open()withO_CREAT | O_EXCLflags for atomic lock acquisitionglobalStoragePathgetter toExtensionContextcheckGlobalStateSafely()(random delay) withwithLock()patternwindow.state.focusedcheck (file lock handles synchronization)FileLockServiceHow it works:
Why file locking over random delay:
fsmodule)Checklist
Screenshots / GIFs
N/A - No UI changes, internal synchronization fix.