Fix dispose leaks: register CursorBlinkStateManager + _pausedResizeTask#5817
Open
srid wants to merge 1 commit intoxtermjs:masterfrom
Open
Fix dispose leaks: register CursorBlinkStateManager + _pausedResizeTask#5817srid wants to merge 1 commit intoxtermjs:masterfrom
srid wants to merge 1 commit intoxtermjs:masterfrom
Conversation
Three related disposable-registration gaps that leak xterm Terminal instances when the host application unmounts a Terminal. Evidence + full writeup at juspay/kolu#606. 1. addon-webgl/WebglRenderer._cursorBlinkStateManager was declared `= new MutableDisposable()` without the `this._register(...)` wrapper that every sibling MutableDisposable in the class uses. CursorBlinkStateManager runs a setInterval for the cursor blink and its `dispose()` correctly clears it — but dispose() was never called because the disposable wasn't registered. The live timer kept the renderer (and thus the Terminal) alive past host unmount. 2. common/TaskQueue.DebouncedIdleTask had no `dispose()` method. Added one that clears the internal IdleTaskQueue (cancelling any pending idle callback). 3. browser/services/RenderService._pausedResizeTask (a DebouncedIdleTask) was not registered. Wrapped with `this._register(...)` so RenderService.dispose propagates. Verified in a downstream app (kolu) with a repro that toggles Focus vs Canvas layout modes: - Pre-fix: 24/28 disposed xterm Terminals retained past host unmount + forced GC. - Post-fix: 0/28 retained. Retention chain (before fix): DOMTimer → ScheduledAction → V8Function → CursorBlinkStateManager._renderCallback → WebglRenderer → Terminal
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 17, 2026
Replaces the minified-mjs pnpm patches from the previous commit with a git-URL override pointing at the juspay/xterm.js fork. The fork's `fix/dispose-leaks-built` branch commits both: 1. The source-level .ts fix (upstream-ready; PR at xtermjs/xterm.js#5817, tracking issue xtermjs/xterm.js#5818). 2. Pre-built .mjs bundles so pnpm can consume the fork directly without running xterm's build toolchain at install time. Changes: - package.json: drop the two xterm patches from `patchedDependencies`; add `overrides` for `@xterm/xterm` and `@xterm/addon-webgl` pointing at `github:juspay/xterm.js#fix/dispose-leaks-built` (the addon-webgl entry uses `&path:/addons/addon-webgl` to select the monorepo subdirectory). - patches/: delete both @xterm patch files. - default.nix: bump the `fetchPnpmDeps` hash. - pnpm-lock.yaml: regenerated — pins the fork via its tarball URL + commit SHA, so the build is reproducible across machines. Verified: `nix build .#default` succeeds end-to-end; dev-server heap reproduction still shows `aliveDetached: 0, gced: 7/8` after 6 mode toggles. Previous commit (the .mjs-patch approach) is preserved in git history for record of the interim fix. Once xtermjs/xterm.js#5817 merges + an xterm release ships, this override can be dropped in favor of a plain version bump.
This was referenced Apr 17, 2026
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 17, 2026
Replaces the minified-mjs pnpm patches from the previous commit with a git-URL override pointing at the juspay/xterm.js fork. The fork's `fix/dispose-leaks-built` branch commits both: 1. The source-level .ts fix (upstream-ready; PR at xtermjs/xterm.js#5817, tracking issue xtermjs/xterm.js#5818). 2. Pre-built .mjs bundles so pnpm can consume the fork directly without running xterm's build toolchain at install time. Changes: - package.json: drop the two xterm patches from `patchedDependencies`; add `overrides` for `@xterm/xterm` and `@xterm/addon-webgl` pointing at `github:juspay/xterm.js#fix/dispose-leaks-built` (the addon-webgl entry uses `&path:/addons/addon-webgl` to select the monorepo subdirectory). - patches/: delete both @xterm patch files. - default.nix: bump the `fetchPnpmDeps` hash. - pnpm-lock.yaml: regenerated — pins the fork via its tarball URL + commit SHA, so the build is reproducible across machines. Verified: `nix build .#default` succeeds end-to-end; dev-server heap reproduction still shows `aliveDetached: 0, gced: 7/8` after 6 mode toggles. Previous commit (the .mjs-patch approach) is preserved in git history for record of the interim fix. Once xtermjs/xterm.js#5817 merges + an xterm release ships, this override can be dropped in favor of a plain version bump.
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 17, 2026
…ermjs/xterm.js#5817) (#609) Closes #606. Part of #610 (master tracking). Follow-up to #607. After the Kolu-side addon-null cleanup reduced orphaned xterm Terminals from 24 → 6, heap-snapshot BFS-from-root traced all 6 survivors to one xterm-internal retainer chain: ``` DOMTimer → ScheduledAction → V8Function → CursorBlinkStateManager._renderCallback → WebglRenderer → Terminal ``` Three disposables in xterm.js are created but never registered: 1. **`addon-webgl/WebglRenderer._cursorBlinkStateManager`** — declared `= new MutableDisposable()` without `this._register(...)`. Its internal `setInterval` for the cursor blink survives `WebglRenderer.dispose()`, pinning the renderer. 2. **`xterm/TaskQueue.DebouncedIdleTask`** — no `dispose()` method at all. 3. **`xterm/RenderService._pausedResizeTask`** — not registered. Full writeup: [`docs/perf-investigations/mode-toggle-leak.md`](https://github.com/juspay/kolu/blob/master/docs/perf-investigations/mode-toggle-leak.md). ## Upstream - **Issue**: xtermjs/xterm.js#5818 — problem description + heap evidence - **PR**: xtermjs/xterm.js#5817 — 6-line source fix, ready for review ## Approach — fork via pnpm overrides This PR consumes the fix via the **juspay/xterm.js** fork until upstream merges + releases. Two commits on this branch tell the story: 1. **`98df21e` — interim fix via `pnpm patch`** (historical record; reverted in the next commit). Large minified-`.mjs` diff files, worked but not maintainable. 2. **`dfa2b5b` — switch to fork via git URL** (final approach): - `package.json` adds `pnpm.overrides` for `@xterm/xterm` and `@xterm/addon-webgl` pointing at `github:juspay/xterm.js#fix/dispose-leaks-built` (the `&path:/addons/addon-webgl` selector pulls the addon from the monorepo subdirectory). - Fork branch `fix/dispose-leaks-built` commits both the `.ts` source fix and pre-built `.mjs` bundles so pnpm consumes them directly — no build step during `pnpm install`, works in Nix's restricted-network sandbox. - `patches/@xterm__*.patch` deleted. - `default.nix` `fetchPnpmDeps` hash bumped. Once `xtermjs/xterm.js#5817` merges and a new xterm release ships, the overrides collapse to a one-line version bump in `package.json`. ## Verification | Stage | Rn | yn disposed-retained | | -------------------------------------- | --- | -------------------- | | Pre-any-fix (after forced GC) | 29 | 24 | | After #607 only | 11 | 6 | | **After this PR (full fix via fork)** | 5 | **0** | Residual 1 `Rn` above live-count is a transitional instance freshly created during restore. - `nix build .#default` ✓ end-to-end build with fork via `fetchPnpmDeps` tarball URL. - `just check`, `just fmt` ✓ - Production verified on pre-fork-patch builds: `aliveDetached: 0, gced: 54/55` after extensive mode toggling. - Dev-server repro with fork: 6 mode toggles → `aliveDetached: 0`, `gced: 7/8`. ## Test plan - [x] typecheck + fmt - [x] heap-snapshot reproduction against fork confirms zero disposed-retained xterm Terminals - [x] full `nix build .#default` succeeds - [x] upstream issue + PR filed - [ ] CI green across all systems 🤖 Generated with [Claude Code](https://claude.com/claude-code)
srid
added a commit
to juspay/xterm.js
that referenced
this pull request
Apr 17, 2026
Downstream kolu (juspay/kolu) consumes this fork via pnpm.overrides github URL. The main fix/kolu-xterm-fixes branch is source-only; this branch adds the pre-built .mjs bundles so no build step runs at pnpm install time. Built output contains both fixes: - xtermjs#5817: register CursorBlinkStateManager + _pausedResizeTask - xtermjs#5821: WeakRef this in IntersectionObserver callback (xtermjs#5820)
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 17, 2026
…fork Bumps the pnpm override pointer from `fix/dispose-leaks-built` to `fix/kolu-xterm-fixes-built`, which stacks a second fix on top of the existing dispose-leaks patches: - xtermjs/xterm.js#5817 (`fix/dispose-leaks`): register CursorBlinkStateManager + _pausedResizeTask disposables. Already in production. - xtermjs/xterm.js#5821 (new, `fix/intersection-observer-weakref`): wrap `this` in a `WeakRef` inside `RenderService`'s IntersectionObserver callback. `observer.disconnect()` on our side wasn't releasing the callback in practice — heap snapshot showed 175,594 retained Uint32Arrays (~220 MB / 30 mode-toggles with 7 terminals) traced through the global IntersectionObserver registry → callback closure → RenderService → BufferService → BufferLines. WeakRef breaks that chain regardless of why the native registry held the callback. Local measurement on kolu@zest (fresh tab, 30 canvas↔focus toggles, 7 terminals restored): Before fix → After fix Task Manager Memory Footprint +367 MB → -3 MB BufferLine (Z1) instances Δ +175,594 → ~0 Upstream tracking: xtermjs/xterm.js#5820 (issue), xtermjs/xterm.js#5821 (PR). Fork consumption will collapse to a plain version bump once upstream merges + releases.
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 17, 2026
…fork Bumps the pnpm override pointer from `fix/dispose-leaks-built` to `fix/kolu-xterm-fixes-built`, which stacks a second fix on top of the existing dispose-leaks patches: - xtermjs/xterm.js#5817 (`fix/dispose-leaks`): register CursorBlinkStateManager + _pausedResizeTask disposables. Already in production. - xtermjs/xterm.js#5821 (new, `fix/intersection-observer-weakref`): wrap `this` in a `WeakRef` inside `RenderService`'s IntersectionObserver callback. `observer.disconnect()` on our side wasn't releasing the callback in practice — heap snapshot showed 175,594 retained Uint32Arrays (~220 MB / 30 mode-toggles with 7 terminals) traced through the global IntersectionObserver registry → callback closure → RenderService → BufferService → BufferLines. WeakRef breaks that chain regardless of why the native registry held the callback. Local measurement on kolu@zest (fresh tab, 30 canvas↔focus toggles, 7 terminals restored): Before fix → After fix Task Manager Memory Footprint +367 MB → -3 MB BufferLine (Z1) instances Δ +175,594 → ~0 Upstream tracking: xtermjs/xterm.js#5820 (issue), xtermjs/xterm.js#5821 (PR). Fork consumption will collapse to a plain version bump once upstream merges + releases.
srid
added a commit
to juspay/kolu
that referenced
this pull request
Apr 18, 2026
Draft blog post for review. Push revisions as additional commits for iteration. Preview on GitHub by clicking the file in the diff, or view raw for a cleaner markdown render: - [docs/perf-investigations/the-leak-that-wasnt-in-any-context.md](https://github.com/juspay/kolu/blob/docs/leak-blog-post/docs/perf-investigations/the-leak-that-wasnt-in-any-context.md) Shape of the post: - Intro — what Kolu is (cockpit for coding agents), canvas mode (linked to [@sridca tweet](https://x.com/sridca/status/2044953014100726221)), symptom - Wrong turn — compressed, not storied: six commits chasing the `Context` count proxy on #614, 89% reduction, zero Task Manager movement - What I was actually measuring — brief explainer on Task Manager vs `performance.memory` for readers who aren't web-perf fluent - The one-line fix — byte-delta diff, retainer chain, WeakRef diff - The xterm.js side — highlights both contributions (xtermjs/xterm.js#5817 + xtermjs/xterm.js#5821) as "laughably small, hours to find" - What I'd tell past-me — three takeaways aimed at backend/systems readers - Links to master-branch docs/skill/scripts for anyone who wants the full investigation record Attribution: I drove; [Claude Code](https://claude.com/claude-code) did the agent-side work. Not for merge as-is — draft is to iterate on wording before deciding where it's actually published.
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
1 participant
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 #5818.
Three small edits that add missing
this._register(...)wrappers to disposables that were created but never registered. Full problem description with heap-snapshot evidence in the linked issue.Diff
Downstream verification
Reproduced in kolu (SolidJS + xterm.js terminal workspace) with a Focus ↔ Canvas layout toggle that synchronously remounts all
<Terminal>components:Downstream PR exercising the fork via git-URL override (and passing CI across both the host typecheck and full Nix build): juspay/kolu#609.
Risk
Zero behavior change for hosts that already dispose the renderer correctly — this PR just ensures disposal actually propagates where it was already intended to. The three edits follow the same pattern as surrounding sibling fields in each file.
Happy to address any feedback. Thanks for xterm.js.