-
Notifications
You must be signed in to change notification settings - Fork 63
Description
We have been seeing some race condition with stale diagnostics.
This might be relevant in server/src/incrementalCompilation.ts
The Vulnerability: await Creates a Race Window
The critical issue is on line 555:
if (
!error?.killed &&
triggerToken != null &&
verifyTriggerToken(entry.file.sourceFilePath, triggerToken)
) {
getLogger().log("Resetting compilation status.");
// Reset compilation status as this compilation finished
entry.compilation = null;
const { result, codeActions } = await utils.parseCompilerLogOutput(
`${stderr}\n#Done()`,
);
The Race Condition Scenario:
-
t=0ms: User edits file (Compilation A starts)
triggerToken = 100- Process starts compiling
-
t=100ms: Compilation A finishes
- Line 549-551: Verification passes ✅ (token 100 matches)
- Line 554: Sets
entry.compilation = null - Line 555: Enters
awaitfor parseCompilerLogOutput⚠️
-
t=101ms: User edits file again (Compilation B starts)
triggerToken = 200- Line 357: Tries to kill old compilation (but A already finished)
- Line 365-373: Sets
entry.compilation = { timeout: ..., triggerToken: 200 }
-
t=110ms: Compilation B finishes quickly
- Verification passes ✅ (token 200 matches)
- Sends diagnostics for new content to client ✅
-
t=150ms: Compilation A's
awaitfinally completes- Continues from line 557 without re-verifying token
- Line 577:
entry.codeActions = actions(overwrites B's actions!) - Line 660-667: Sends diagnostics for old content to client ❌
Result: The user sees stale diagnostics even though a newer compilation has completed!
Additional Issues:
-
Code Actions Clobbering (Line 577):
entry.codeActions = actions;Compilation A can overwrite code actions from Compilation B after the await.
-
Shared State Mutation:
Multiple compilations can modify the sameentryobject concurrently, leading to unpredictable state.
Why Current Approach Isn't Bulletproof:
The token verification happens once (before the await), but the processing and side effects happen after the await. JavaScript's single-threaded nature doesn't protect you here because await yields control back to the event loop.
Recommended Fix:
Add token re-verification after the await:
if (
!error?.killed &&
triggerToken != null &&
verifyTriggerToken(entry.file.sourceFilePath, triggerToken)
) {
getLogger().log("Resetting compilation status.");
entry.compilation = null;
const { result, codeActions } = await utils.parseCompilerLogOutput(
`${stderr}\n#Done()`,
);
// RE-VERIFY: Token may have changed during the await above
if (!verifyTriggerToken(entry.file.sourceFilePath, triggerToken)) {
getLogger().log(
`Discarding stale compilation results for ${entry.file.sourceFileName} (token mismatch after parsing)`
);
return;
}
const actions = Object.values(codeActions)[0] ?? [];
// ... rest of the processing
This ensures that even if a newer compilation started and finished during the parseCompilerLogOutput await, the stale results won't be sent to the client.
@zth does this seem reasonable?
As you can guess, I may have ask a friend to look into this 🙈.