fix: preserve ENOTEMPTY error code in extension rename (fixes #323155)#323162
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
fix: preserve ENOTEMPTY error code in extension rename (fixes #323155)#323162vs-code-engineering[bot] wants to merge 1 commit into
vs-code-engineering[bot] wants to merge 1 commit into
Conversation
The ENOTEMPTY benign-race handler in extractUserExtension was defeated because the private rename() helper wrapped every failure via toExtensionManagementError(..., Rename), clobbering error.code from 'ENOTEMPTY' to 'Rename'. Preserve the raw ENOTEMPTY error so the caller can detect the benign concurrent-install race; genuine failures are still wrapped for telemetry classification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
extractUserExtensioninstalls an extension by extracting the VSIX into a temporary directory and then renaming that temp directory to the final extension location. When two sources install the same extension concurrently (for example a built-in language pack being auto-updated while it is also installed by another path), the final directory already exists and the rename fails withENOTEMPTY. The call site explicitly treats this as a benign race and swallows it — but the checkerror.code === 'ENOTEMPTY'never matched, so the benign race was re-thrown as an unhandled error into telemetry:ENOTEMPTY: directory not empty, rename '.../extensions/.6de5c025-...' -> '.../extensions/ms-ceintl.vscode-language-pack-ja-1.126....'The root cause is that the private
rename()helper wraps every failure withtoExtensionManagementError(error, ExtensionManagementErrorCode.Rename), which constructs a newExtensionManagementErrorwhose.codeis set to the enum value'Rename'. This clobbers the original'ENOTEMPTY'code before the caller ever sees it, so the benign-race branch is effectively dead code.Fixes #323155
Recommended reviewer:
@sandy081Culprit Commit
The error-code-clobbering bug is latent / pre-existing: both the benign-race handler in
extractUserExtensionand the wrappingrename()helper already exist unchanged in7f503b81and earlier, so neither was introduced inside the observed telemetry window.What turned this latent bug into a high-volume bucket is
143b631d— "Enable auto-update for built-in extensions" (Sandeep Somavarapu /@sandy081, 2026-03-31), a confirmed ancestor of the shipped commit7e7950df(1.126.0). Enabling auto-update for built-in extensions routes bundled language packs such asms-ceintl.vscode-language-pack-jathrough the same extract→rename path, dramatically increasing the concurrent-install race that produces the benignENOTEMPTY. The bucket first appears in1.119.0(the first stable release carrying this feature) and persists through1.126.0.Later commits in the same feature line (
7f503b81,88e21a0"handle edge cases while updating built in extensions") touched neighbouring update logic but did not repair the brokenENOTEMPTYdetection.Code Flow
flowchart TD A[extractUserExtension extracts VSIX to temp dir] --> B[calls rename temp to final] B --> C[pfs.Promises.rename] C -- final dir already exists --> D[throws raw error with code ENOTEMPTY] D --> E{rename helper catch block} E -- before fix --> F[wraps via toExtensionManagementError with code Rename] F --> G{caller catch checks for ENOTEMPTY} G -- no the code is now Rename --> H[else branch re-throws] H --> I[unhandled error reaches telemetry] E -- after fix --> J[re-throws raw ENOTEMPTY] J --> K{caller catch checks for ENOTEMPTY} K -- yes --> L[logs info deletes temp swallows benign race]Affected Files
src/vs/platform/extensionManagement/node/extensionManagementService.ts— theExtensionsScanner.rename()helper (producer of the mis-coded error) and theextractUserExtension()benign-race handler that consumeserror.code. Modified.Supporting context (not modified):
src/vs/platform/extensionManagement/common/abstractExtensionManagementService.ts—toExtensionManagementError()constructs the wrapped error that dropped the original.code.src/vs/platform/extensionManagement/common/extensionManagement.ts—ExtensionManagementErrorsets.code/.nameto the enum value.src/vs/base/node/pfs.ts—rename/renameWithRetryretries onlyEACCES/EPERM/EBUSY, soENOTEMPTYsurfaces immediately with its raw code.Repro Steps
ms-ceintl.vscode-language-pack-ja) eligible for auto-update.rename(temp → final); the final directory already exists, sopfs.Promises.renamethrowsENOTEMPTY.Rename, the caller'serror.code === 'ENOTEMPTY'benign-race check fails, and the error is re-thrown into telemetry instead of being swallowed.How the Fix Works
Chosen approach —
src/vs/platform/extensionManagement/node/extensionManagementService.ts,ExtensionsScanner.rename():The mis-coded error object is produced inside
rename()at thetoExtensionManagementError(error, ...Rename)call — that is where a rawENOTEMPTYis turned into anExtensionManagementErrorwhose.codebecomes'Rename'. The fix adds a guard at that producer, before the wrapping throw: if the caught error's code isENOTEMPTY, re-throw it unchanged; otherwise keep the existingRenamewrapping. This restores the original author's intent — the caller's benign-race branch (error.code === 'ENOTEMPTY') becomes reachable again, so the concurrent-install race is logged and swallowed, while every genuine rename failure is still wrapped asRenameand its telemetry classification is preserved.This follows the fix-at-the-data-producer principle rather than patching the crash site: the guard lives where the bad error object is created, not at the caller's
catchwhere the symptom surfaces. Notry/catchwas added (the existing one is reused), nologServicecall was removed, and no genuine error is silenced — only the already-intended benign race is allowed to reach its existing handler.Alternatives considered:
catch— rejected: a consumer-side string-match hack at the crash site that does not address where.codeis lost.toExtensionManagementError()itself preserveENOTEMPTY— rejected: it is a shared helper used by many unrelated callers, so special-casing one filesystem code there is too broad.Recommended Owner
@sandy081(Sandeep Somavarapu) — owns the extension management area, authored the built-in-extension auto-update feature that made this race hot, and is the author of essentially every recent change toextensionManagementService.ts.