From f781223e3125548306c3cbdb2c249209e4e3e922 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:44:05 -0700 Subject: [PATCH] fix(terminal): correct the fence justification and key the cache by window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The markWritten docblock claimed "zero bytes cannot mutate the buffer". That is false, and I verified it: `_core.writeSync('')` drains xterm's pending queue and applies it. The exemption is still correct, but for a different reason — a fence cannot introduce an *unattributed* mutation, because any bytes it drains belong to a queued async write whose own completion callback bumps first. The two write regimes are exhaustive: with writeSync present nothing can queue, without it every write is async and self-bumps. A comment asserting a false invariant is worse than no comment, since the next change may rely on it, so it now states the real one. Key the cache by scrollback window instead of a single slot. Consumers ask for different windows against the same emulator — attach passes the full window while agent/text reads pass 0 — so one slot thrashed to a 0% hit rate whenever they alternated, silently removing the benefit on runtime-side emulators. Two entries cover every caller pair in the tree. Drop the epoch counter: markMutated already nulls the retained entry and an entry is only ever stored under the current epoch, so the comparison could never fail. Invalidation is simply "clear the cache". --- .../headless-emulator-snapshot-cache.test.ts | 18 +++++++++- src/main/daemon/headless-emulator.ts | 16 +++++---- src/main/daemon/headless-snapshot-cache.ts | 35 +++++++++++-------- 3 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/main/daemon/headless-emulator-snapshot-cache.test.ts b/src/main/daemon/headless-emulator-snapshot-cache.test.ts index 7c41f0b79ce..75a08a5e623 100644 --- a/src/main/daemon/headless-emulator-snapshot-cache.test.ts +++ b/src/main/daemon/headless-emulator-snapshot-cache.test.ts @@ -34,7 +34,7 @@ describe('HeadlessEmulator snapshot cache', () => { expect(second.scrollbackAnsi).toBe(first.scrollbackAnsi) }) - it('re-serializes for a different scrollbackRows window', async () => { + it('re-serializes the first time a different scrollbackRows window is asked for', async () => { emulator = new HeadlessEmulator({ cols: 80, rows: 24 }) await emulator.write('hello world') emulator.getSnapshot({ scrollbackRows: 100 }) @@ -45,6 +45,22 @@ describe('HeadlessEmulator snapshot cache', () => { expect(serialize.calls()).toBeGreaterThan(0) }) + it('keeps two alternating scrollback windows warm', async () => { + // Why: attach asks for the full window while agent/text reads ask for 0, + // and a single-slot cache thrashes to a 0% hit rate when they alternate. + emulator = new HeadlessEmulator({ cols: 80, rows: 24 }) + await emulator.write('alternating windows') + emulator.getSnapshot({ scrollbackRows: 0 }) + emulator.getSnapshot() + const serialize = spyOnSerialize(emulator) + + emulator.getSnapshot({ scrollbackRows: 0 }) + emulator.getSnapshot() + emulator.getSnapshot({ scrollbackRows: 0 }) + + expect(serialize.calls()).toBe(0) + }) + it('reflects an async write that lands after a cached snapshot', async () => { emulator = new HeadlessEmulator({ cols: 80, rows: 24 }) await emulator.write('first line') diff --git a/src/main/daemon/headless-emulator.ts b/src/main/daemon/headless-emulator.ts index 264b7b1bb21..d75ab2a51db 100644 --- a/src/main/daemon/headless-emulator.ts +++ b/src/main/daemon/headless-emulator.ts @@ -159,12 +159,16 @@ export class HeadlessEmulator { } /** - * Bumps only for real bytes. Why: flushParsedWrites() is a zero-byte write - * used purely as a parse fence (session-output-plane.ts), and every - * getSettledSnapshot runs one. Zero bytes cannot mutate the buffer — the OSC - * and mouse-mode scans are no-ops and the escape tail is idempotent — so - * bumping would evict the cache on every checkpoint read. Any write a fence - * orders behind has already bumped on its own completion. + * Bumps only for real bytes. Why this is safe even though a zero-byte write + * is NOT inert — `_core.writeSync('')` drains xterm's pending queue and + * applies it (verified) — is that a fence can never introduce an + * unattributed mutation. Any bytes it drains belong to a queued async write, + * and xterm runs that write's completion callback first, which bumps. The + * two write regimes are exhaustive: with writeSync present every write takes + * the sync path and nothing can queue; without it every write is async and + * self-bumps. Fences are exempt because flushParsedWrites() is one, and + * every getSettledSnapshot runs it — bumping would evict the cache on each + * checkpoint read. */ private markWritten(data: string): void { if (data.length > 0) { diff --git a/src/main/daemon/headless-snapshot-cache.ts b/src/main/daemon/headless-snapshot-cache.ts index 0216f5bd6c2..9e111e15e2b 100644 --- a/src/main/daemon/headless-snapshot-cache.ts +++ b/src/main/daemon/headless-snapshot-cache.ts @@ -38,6 +38,9 @@ type CachedParts = { // (MAX_COLD_RESTORE_CACHE_BYTES) so the two budgets read in one unit. const MAX_CACHED_SNAPSHOT_BYTES = 4 * 1024 * 1024 +/** Distinct scrollback windows retained per emulator. */ +const MAX_CACHED_SNAPSHOT_WINDOWS = 2 + // Why code units: bounds V8 string storage without rescanning or flattening // multi-MB ropes — same sizing rule as getColdRestorePayloadBytes. function retainedSnapshotBytes(parts: CachedParts): number { @@ -55,31 +58,35 @@ export type HeadlessSnapshotSource = { } export class HeadlessSnapshotCache { - private epoch = 0 - private retained: (CachedParts & { epoch: number; scrollbackRows: number | undefined }) | null = - null + // Why keyed and not a single slot: consumers ask for different scrollback + // windows against the same emulator — attach passes the full window while + // agent/text reads pass 0 — and one slot thrashes to a 0% hit rate when they + // alternate. Two covers every caller pair in the tree; a third evicts the + // oldest rather than growing per emulator. + private readonly entries = new Map() /** Invalidates the cache. Called for every mutation of a memoized part; * fields build() re-reads per call (cwd, lastTitle, escape tail) do not. */ markMutated(): void { - this.epoch += 1 - this.retained = null + this.entries.clear() } /** Builds a caller-owned snapshot, reusing the memoized serialize on a hit. */ build(source: HeadlessSnapshotSource, scrollbackRows: number | undefined): TerminalSnapshot { - const retained = this.retained - let parts: CachedParts - if (retained && retained.epoch === this.epoch && retained.scrollbackRows === scrollbackRows) { - parts = retained - } else { + let parts = this.entries.get(scrollbackRows) + if (!parts) { parts = computeCachedParts(source, scrollbackRows) // Why size-gated: see MAX_CACHED_SNAPSHOT_BYTES. Declining to retain costs // the pre-existing serialize, never correctness. - this.retained = - retainedSnapshotBytes(parts) <= MAX_CACHED_SNAPSHOT_BYTES - ? { ...parts, epoch: this.epoch, scrollbackRows } - : null + if (retainedSnapshotBytes(parts) <= MAX_CACHED_SNAPSHOT_BYTES) { + if (this.entries.size >= MAX_CACHED_SNAPSHOT_WINDOWS) { + const oldest = this.entries.keys().next() + if (!oldest.done) { + this.entries.delete(oldest.value) + } + } + this.entries.set(scrollbackRows, parts) + } } // Why cloned: a hit hands back the retained entry, so a caller mutating // its snapshot would otherwise corrupt every later one.