terminal: do not buffer local pty events that have no IPC client - #334664
Open
Daniel Garza (DanielGarzaB) wants to merge 3 commits into
Open
terminal: do not buffer local pty events that have no IPC client#334664Daniel Garza (DanielGarzaB) wants to merge 3 commits into
Daniel Garza (DanielGarzaB) wants to merge 3 commits into
Conversation
`ProxyChannel.fromService` eagerly creates an `Event.buffer` for every event of the service, and such a buffer only drains once a client calls `listen`. The window consumes every `IPtyService` event over a direct message port to the pty host (`TerminalIpcChannels.PtyHostWindow`), never over the localPty channel, so these buffers never drained: the main process archived every chunk of terminal output for the lifetime of the session. Reported at 1,150,631 events / 602 MB, ending in a V8 OOM that closes every window at once, with no warning and no reload prompt. Mark them unbuffered. The five `IPtyHostController` events stay buffered, those do have a client on this channel. Fixes microsoft#328885 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The local pty channel opted the `IPtyService` events out of buffering by name, and nothing checked that list against the service: an event added to `IPtyService` later would have been buffered again in the main process for a client that never comes, which is exactly how microsoft#328885 came to be. Move the list next to the interface as `ptyServiceEvents`, derived from an object that `satisfies Record<..., true>` so that a missing or an extra name does not compile, and add a test that reflects over a `PtyHostService` instance the way `ProxyChannel.fromService` does, so an `on*` property the class gains outside the interface has to be classified too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The guardian of the previous commit checks that every event of `PtyHostService` is classified, but nothing exercised the channel: dropping `unbufferedEvents` from the registration in app.ts kept every test green while the leak came back. Move the registration into `createLocalPtyChannel`, next to the service, so that a test can build the channel production builds and assert that none of the `IPtyService` events has a listener before a client asks for one, and that a client who does ask gets live events without a backlog. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot started reviewing on behalf of
Daniel Garza (DanielGarzaB)
September 4, 2026 23:27
View session
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Robo (@deepak1556)Matched files:
Anthony Kim (@anthonykim1)Matched files:
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The regression is covered by focused tests; only non-blocking documentation nits remain.
Pull request overview
Prevents unbounded buffering of unused local PTY events in the main process.
Changes:
- Marks
IPtyServiceevents as unbuffered. - Adds exhaustive event coverage and regression tests.
- Centralizes local PTY channel creation while retaining buffered controller events.
File summaries
| File | Description |
|---|---|
src/vs/platform/terminal/test/node/ptyHostService.test.ts |
Tests event coverage and live delivery without backlog. |
src/vs/platform/terminal/node/ptyHostService.ts |
Creates the local channel with unbuffered PTY service events. |
src/vs/platform/terminal/common/terminal.ts |
Defines the exhaustive PTY service event list. |
src/vs/code/electron-main/app.ts |
Uses the local PTY channel factory. |
Review details
Suppressed comments (1)
src/vs/platform/terminal/test/node/ptyHostService.test.ts:103
- This method-body explanation exceeds the repository's one-line inline-comment limit. A short statement of what the eager-listener check proves is sufficient.
// a buffered event has a listener on the service from the moment the channel is created, before any
// client asked for it: that listener is what retained every chunk of terminal output in the main
// process (#328885), so the events a window never asks for over this channel must not have one
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+399
to
+407
| /** | ||
| * The events of {@link IPtyService} by name, kept exhaustive by the type: adding an event to the interface | ||
| * without adding it here does not compile. | ||
| * | ||
| * The local pty channel of the main process needs this list at runtime. A window consumes these events over | ||
| * a direct message port to the pty host, never over that channel, so the channel must not buffer them for a | ||
| * client that never comes: `onProcessData` carries raw terminal output and grows without bound otherwise. | ||
| * See https://github.com/microsoft/vscode/issues/328885 | ||
| */ |
Comment on lines
+434
to
+443
| /** | ||
| * The channel a window reaches the {@link PtyHostService} of the main process over, | ||
| * {@link TerminalIpcChannels.LocalPty}. | ||
| * | ||
| * The window consumes every `IPtyService` event over a direct message port to the pty host | ||
| * ({@link TerminalIpcChannels.PtyHostWindow}), never over this channel, so buffering them here for a client | ||
| * that never comes only retains: `onProcessData` carries raw terminal output and grows by hundreds of MBs an | ||
| * hour. The `IPtyHostController` events are left buffered, those do have a client on this channel. | ||
| * See https://github.com/microsoft/vscode/issues/328885 | ||
| */ |
Comment on lines
+75
to
+79
| // `ProxyChannel.fromService` buffers every `on*` property of the service it is handed until a client | ||
| // listens, and `createLocalPtyChannel` lists the `IPtyService` events as unbuffered because a window | ||
| // never listens to them there (#328885). `ptyServiceEvents` is exhaustive for the interface; this | ||
| // checks the class, which is what the channel reflects over: an event it gains that is in neither | ||
| // list would be buffered in the main process for a client that never comes. |
Author
|
@microsoft-github-policy-service agree company="REBAN" |
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.
Fixes #328885
Problem
The main process grows with terminal output until V8's ~4 GB heap limit and crashes, closing every window at once.
ProxyChannel.fromServicewraps everyon*property of a service in an eagerEvent.buffer(src/vs/base/parts/ipc/common/ipc.ts) that keeps each emission until the first client listens. TheTerminalIpcChannels.LocalPtychannel registered inapp.tsexposesPtyHostService, and a window never listens to the sevenIPtyServiceevents over it: it consumes them over the direct message port to the pty host (TerminalIpcChannels.PtyHostWindow,localTerminalBackend.ts). So the main process archived everyonProcessDatachunk for the lifetime of the session.The pty host and the remote server do not leak by this path: the main process subscribes to the pty host's events as soon as it connects, and
RemoteTerminalChannel.listenhands events out directly.Fix
unbufferedEventsfor theIPtyServiceevents, the mechanism ipc: Avoid buffering unused native host blur events #328535 introduced foronDidBlurMainWindow. The fiveIPtyHostControllerevents stay buffered, those do have a client on this channel.ptyServiceEventslives next toIPtyServiceand is exhaustive by type (satisfies Record<..., true>): adding an event to the interface without listing it does not compile.createLocalPtyChannel, so a test can build the very channel production builds.Tests
src/vs/platform/terminal/test/node/ptyHostService.test.ts:on*property ofPtyHostServiceis either anIPtyServiceor anIPtyHostControllerevent, reflected the wayfromServicedoes;IPtyServiceevent before a client asks for it, and a client who asks gets live events without a backlog.Each guard was checked against its mutation: removing a name from the constant fails to compile, adding an
on*to the class fails the first test, droppingunbufferedEventsfrom the factory fails the second.How to test manually
Open a few integrated terminals producing heavy output (for example
yes | head -c 500M) and watch the private bytes of the main process. Before this change they grow with the output and never come back; after it they stay flat.A general cap for
Event.buffer, as defence in depth for anyfromServicechannel with an unconsumed event, is a possible follow-up. This PR is deliberately limited to the channel that leaks.