Skip to content

[cherry-pick] Fix main process OOM from unconsumed buffered pty host service events - #336198

Merged
Dmitriy Vasyura (dmitrivMS) merged 1 commit into
release/1.138from
cherry-pick/323980
Sep 15, 2026
Merged

Dmitriy Vasyura (dmitrivMS) merged 1 commit into
release/1.138from
cherry-pick/323980

Conversation

@vs-code-engineering

Copy link
Copy Markdown
Contributor

Cherry-pick of #323980 from main.

Fixes #234393. The crash dumps, heap snapshots, and retainer chains referenced below are in #307156, which was closed as a duplicate of it.

Problem

The main process accumulates all terminal PTY output in the V8 heap until it hits the ~4 GB heap ceiling and OOM-aborts (Reached heap limit). Heavy ANSI producers in the integrated terminal (tmux status/pane redraws, TUI apps) crash the main process within hours; median ~4.2h across the 99 crash dumps in #307156.

Root cause

ProxyChannel.fromService eagerly wraps every service event in Event.buffer(..., flushAfterTimeout=true), which subscribes to the source immediately and queues each event into an array that is only flushed (and released) when a first listener attaches to the channel event.

The main process registers ILocalPtyService this way (app.ts) as the TerminalIpcChannels.LocalPty channel. On desktop, the renderer consumes terminal data over the dedicated pty host connection (PtyHostWindow MessagePort, see localTerminalBackend.ts); the LocalPty channel itself is only used for two method calls (getLatency, getProfiles) plus a handful of low-frequency pty-host lifecycle events (onPtyHostStart/onPtyHostExit/onPtyHostUnresponsive/onPtyHostResponsive/onPtyHostRequestResolveVariables, consumed by BaseTerminalBackend) — it never gets a listener for the high-volume onProcessData event. So the eager buffer for onProcessData (one entry per PTY data chunk) grows for the lifetime of the process. Retainer chains in heap snapshots (see #307156) show ~99% of the heap held by exactly this array: Emitter._listeners → closure → Array[N] → { event: }.

Fix

Adds a disableEventBuffering option to ProxyChannel.ICreateServiceChannelOptions (naming follows the existing disableMarshalling) that exposes service events on the channel without the eager Event.buffer wrapper, and applies it to the LocalPty channel registration. Events fired before a listener attaches are dropped instead of retained. This is a whole-channel opt-out, so it also stops buffering the pty-host lifecycle events BaseTerminalBackend listens to here; in practice that's low-risk (those fire only after a window's listener has attached, and the one early-fireable case — the initial onPtyHostStart — is a no-op per BaseTerminalBackend's own hasStarted guard, see review discussion below). Per Dmitriy Vasyura (@dmitrivMS)'s plan, this PR ships as the accepted stop-gap; a follow-up telemetry pass will confirm whether any channel actually needs buffering restored before removing it from fromService generally.

Testing

  • New unit tests in ipc.test.ts cover the default (eager subscription, buffered) and the new option (no subscription until listen, events delivered after).
  • Full node unit suite passes (11,908 passing).
  • A/B with the repro from the issue (ANSI-flood script in the integrated terminal, fresh profile, --disable-extensions, macOS arm64), measuring main-process RSS every 15 s on the same dev build with only this change toggled:
Configuration Main-process RSS under flood
Without the fix 191 MB → 1,750 MB in 8.5 min (~180 MB/min, monotonic); at that rate the ~4 GB ceiling lands in under 30 min
With the fix flat at ~250–330 MB over 9+ min

Copilot AI balanced review requested due to automatic review settings September 14, 2026 20:27
@vs-code-engineering vs-code-engineering Bot added the cherry-pick-artifact Auto-generated cherry-pick PR label Sep 14, 2026
@vs-code-engineering

Copy link
Copy Markdown
Contributor Author

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Robo (@deepak1556)

Matched files:

  • src/vs/code/electron-main/app.ts

Anthony Kim (@anthonykim1)

Matched files:

  • src/vs/platform/terminal/common/localPtyChannel.ts
  • src/vs/workbench/contrib/terminal/test/electron-browser/localPty.test.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The PR description materially misstates the implemented option and lifecycle-event behavior.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents unconsumed PTY process events from accumulating in the main process.

Changes:

  • Selectively disables buffering for PTY process events while preserving lifecycle buffering.
  • Adds unit, replay, restart, and smoke-test coverage.
File summaries
File Description
src/vs/platform/terminal/common/localPtyChannel.ts Configures selective event buffering.
src/vs/code/electron-main/app.ts Uses the specialized PTY channel.
src/vs/base/parts/ipc/test/common/ipc.test.ts Tests buffered and unbuffered events.
src/vs/platform/terminal/test/common/localPtyChannel.test.ts Tests channel subscription behavior.
src/vs/platform/terminal/test/node/ptyHostService.test.ts Tests lifecycle events across restarts.
src/vs/workbench/contrib/terminal/test/electron-browser/localPty.test.ts Tests direct-channel replay behavior.
test/smoke/src/areas/terminal/terminal-persistence.test.ts Tests ordered replay after reconnection.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • 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 thread src/vs/platform/terminal/common/localPtyChannel.ts
@anthonykim1

Copy link
Copy Markdown
Contributor

We should get manager approval

@dmitrivMS
Dmitriy Vasyura (dmitrivMS) merged commit 37a7dda into release/1.138 Sep 15, 2026
34 checks passed
@dmitrivMS
Dmitriy Vasyura (dmitrivMS) deleted the cherry-pick/323980 branch September 15, 2026 00:33
@vs-code-engineering vs-code-engineering Bot added this to the 1.138.0 milestone Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-artifact Auto-generated cherry-pick PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants