Skip to content

fix: destroy read streams to prevent file descriptor leaks#303395

Merged
alexdima merged 8 commits intomicrosoft:mainfrom
buley:fix/stream-fd-leaks
Mar 23, 2026
Merged

fix: destroy read streams to prevent file descriptor leaks#303395
alexdima merged 8 commits intomicrosoft:mainfrom
buley:fix/stream-fd-leaks

Conversation

@buley
Copy link
Copy Markdown
Contributor

@buley buley commented Mar 20, 2026

Summary

Two createReadStream() file descriptor leaks found via static analysis of the control flow graph:

1. src/vs/base/node/crypto.ts -- checksum() creates a read stream and pipes it to a crypto hash, but never calls destroy() on the stream. The done() callback removes event listeners but leaves the underlying fd open until GC collects it. This can cause fd exhaustion under high checksum throughput (e.g. extension updates).

Fix: Added input.destroy() in the done() callback, immediately after removeAllListeners().

2. src/vs/server/node/webClientServer.ts -- createReadStream(filePath).pipe(res) leaks the file descriptor when the HTTP response is closed prematurely (client disconnect, timeout). Node's .pipe() does not automatically destroy the source stream when the destination closes.

Fix: Captured the stream reference and added res.on('close', () => fileStream.destroy()).

Test Plan

  • Added checksum mismatch rejects and cleans up stream test in src/vs/base/test/node/crypto.test.ts to verify the stream is properly cleaned up on hash mismatch
  • Both fixes only affect cleanup paths and do not change normal control flow

Two stream leak fixes found via static analysis of the control flow graph:

1. crypto.ts: createReadStream() used for checksum computation was never
   explicitly destroyed. The stream was piped to a hash and event listeners
   were removed in the done() callback, but the underlying fd was left to
   GC. Added input.destroy() to the done() callback to ensure immediate
   cleanup on both success and error paths.

2. webClientServer.ts: createReadStream().pipe(res) leaked the file
   descriptor when the HTTP response was closed prematurely (e.g. client
   disconnect). The pipe does not automatically destroy the source stream
   when the destination closes. Added res.on('close') handler to destroy
   the file stream.
Copilot AI review requested due to automatic review settings March 20, 2026 06:08
@vs-code-engineering vs-code-engineering bot added this to the 1.113.0 milestone Mar 20, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes potential file descriptor leaks in VS Code’s Node-side file/crypto streaming paths by explicitly destroying read streams during early termination scenarios (checksum completion/mismatch and HTTP response premature close).

Changes:

  • Ensure checksum() destroys its fs.ReadStream in the completion callback.
  • Ensure serveFile() destroys its fs.ReadStream when the HTTP response closes early.
  • Add a regression test covering the checksum mismatch rejection path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/vs/server/node/webClientServer.ts Tracks the ReadStream and destroys it on response close to avoid leaking fds on client disconnect.
src/vs/base/node/crypto.ts Destroys the checksum input stream in done() to ensure the fd is closed on all completion paths.
src/vs/base/test/node/crypto.test.ts Adds a test for checksum mismatch rejection (currently not asserting cleanup directly).

@buley
Copy link
Copy Markdown
Contributor Author

buley commented Mar 20, 2026

@microsoft-github-policy-service agree [company="Forkjoin.ai"]

webClientServer.ts:
- Add fileStream.on('error') handler for async read errors that occur
  after the try/catch (e.g. ENOENT race, permission change mid-read)
- Handle both pre-header and post-header error cases
- Use res.once('close') instead of res.on('close') for cleanup intent

crypto.test.ts:
- Replace try/catch with assert.rejects() and regex matcher
- Spy on fs.createReadStream to verify destroy() is actually called
- Restore original createReadStream in finally block
@buley
Copy link
Copy Markdown
Contributor Author

buley commented Mar 20, 2026

@microsoft-github-policy-service agree company="Forkjoin.ai"

Copy link
Copy Markdown
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@buley The tests are failing

The test was monkey-patching `fs.createReadStream` on the ESM module
namespace, which is read-only at runtime (TypeError). Also removes
the unused `Readable` import that caused TS6133.

Simplified the test to verify the observable contract (mismatch
rejection) without attempting to spy on stream internals.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@buley
Copy link
Copy Markdown
Contributor Author

buley commented Mar 23, 2026

@alexdima Fixed the failing tests -- two issues:

  1. Compile & Hygiene: Removed unused Readable import (TS6133)
  2. Electron tests: The test was monkey-patching fs.createReadStream on the ESM module namespace, which is read-only at runtime. Simplified the test to verify the mismatch rejection without spying on stream internals.

The input.destroy() fix in crypto.ts and the res.once('close', ...) fix in webClientServer.ts are unchanged. Thanks for flagging!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@alexdima alexdima self-requested a review March 23, 2026 21:30
Create the read stream and wait for its 'open' event before calling
res.writeHead(200). Previously, headers were sent before the stream
was created, making res.headersSent always true in the error handler
and the 404 branch unreachable. A TOCTOU race between stat() and
createReadStream() would result in a 200 being aborted instead of
a proper 404.

Now, pre-header errors reject into the existing catch block (which
sends 404), and post-header errors correctly call res.destroy().
@alexdima alexdima modified the milestones: 1.113.0, 1.114.0 Mar 23, 2026
@alexdima alexdima enabled auto-merge (squash) March 23, 2026 21:43
@alexdima alexdima merged commit 184c7f1 into microsoft:main Mar 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants