Commit c7ab305
authored
fix(xterm): patch dispose leaks via juspay/xterm.js fork (upstream xtermjs/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)1 parent 461e167 commit c7ab305
8 files changed
Lines changed: 526 additions & 431 deletions
File tree
- .claude/skills/perf-diagnose
- agents/.apm/skills/perf-diagnose
- docs
- perf-investigations
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
49 | | - | |
| 49 | + | |
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
| |||
Large diffs are not rendered by default.
This file was deleted.
0 commit comments