Eliminate spiky terminal lag under load (#1500)
* Eliminate spiky terminal lag under load
Terminal output stuttered during heavy output and when the daemon was
busy. Two compounding causes, both confirmed by measurement:
- Every 256KB of output, the daemon dropped pending frames and re-sent a
full cell-grid snapshot (cloned across IPC, stringified to JSON) — a
10MB build did this ~40 times. The snapshot fallback is now gated on
real client backpressure (ws bufferedAmount), so a client that keeps
draining streams continuously and never pays the snapshot tax. This
also removes the periodic GC hitch the churn caused.
- Per-chunk overhead on the shared event loop: one IPC message per pty
chunk, a duplicate input-mode regex scan on the daemon main loop, a
double JSON.stringify per outbound message, and 16KB string realloc
per chunk. Output now coalesces in the worker before IPC (leading-edge
so keystrokes still echo immediately), the duplicate scan is gone, and
the client feeds xterm back-to-back instead of one render tick a frame.
Adds eventLoopDelay percentiles to ws_runtime_metrics for main-loop
stall visibility, plus a reproducible Node benchmark (no port 6767).
Measured: echo p50 7.7ms to 2.3ms; a 2MB burst now streams fully with
zero snapshots; loop-stall spikes 100-173ms to 2ms.
* Guard trace-level check against partial logger stubs
The isLevelEnabled gate added for the emit() perf fix crashed every
session test that injects a hand-rolled logger stub (10 of them omit
isLevelEnabled). Real pino loggers always provide it; optional-chaining
keeps the perf gate intact in production and no-ops on stubs.
* Make recent-output exit-summary test deterministic
The added test self-exited the child immediately after writing 3000
lines, then asserted the newest line was in the exit summary — but the
summary reads the headless xterm buffer, which parses writes
asynchronously, so a loaded CI runner saw a stale buffer (line-2707 not
line-2999). Keep the process alive and poll the parsed buffer until the
final line lands before killing, removing the race.
* Skip ConPTY-fragile worker terminal tests on Windows
The coalescing and input-mode-preamble tests assert byte-contiguous PTY
output and an exact kitty-escape round-trip. Windows ConPTY injects
repaint sequences between writes and normalizes the escape, so both time
out there while passing on Linux/macOS. Gate them with skipIf(win32),
matching the existing terminal-test convention for PTY-sensitive cases.
* Apply terminal barriers immediately when no writes are pending
A barrier op (snapshot/restore/clear) only needs the sentinel-write gate
to wait out plain writes still parsing in xterm's buffer. When none are
ungated — at mount, or right after another barrier — the sentinel cost a
wasted parse cycle, adding latency to the first snapshot/restore on the
hot first-paint path. Track ungated writes with a flag and skip the
sentinel when there's nothing to gate.
* Fix mobile startup saved-host e2e
* Unskip Windows worker terminal coverage
* Tighten worker coalescing assertion
* Reset terminal ungated writes on unmount
* Fix terminal regressions found in adversarial review
Three issues this PR's terminal changes introduced:
- Worker snapshot could duplicate coalesced output. getTerminalState
snapshotted without flushing the worker output coalescer, so a batch
spanning the snapshot point carried a revision past it and the
controller's dedup couldn't drop it — the client saw the bytes twice.
Flush before snapshotting.
- Relay clients lost backpressure protection. The snapshot fallback was
gated on bufferedAmount, which the multiplexed relay socket reports as
absent; that read as 0 ("keeping up") so a slow relay client never
caught up via a snapshot. Distinguish "no signal" (null) from 0 and
keep the unconditional byte-threshold fallback for signal-less
transports, preserving the pre-change relay behavior.
- Recent-output buffer was no longer a hard cap. A single chunk larger
than the limit was retained whole; slice its tail.
Documents the live-restore preamble gap (unreachable: no client sends
restore mode "live").
* Skip input-mode preamble test on Windows ConPTY
CI confirmed Windows ConPTY normalizes away the kitty keyboard escape
the child writes, so it never reaches the worker's input-mode tracker
and the preamble stays empty — the test times out. ConPTY can't exercise
this contract; the coalescing test (file-gated, line-oriented) stays on
all platforms, only the preamble assertion is gated to Linux/macOS.
This commit is contained in:
@@ -22,6 +22,7 @@ import {
|
||||
} from "@getpaseo/protocol/binary-frames/index";
|
||||
import { TerminalOutputCoalescer } from "./terminal-output-coalescer.js";
|
||||
import {
|
||||
MAX_CLIENT_BUFFERED_BYTES,
|
||||
MAX_TERMINAL_OUTPUT_FRAME_BYTES,
|
||||
encodeLegacyTerminalSnapshotFrame,
|
||||
encodeTerminalRestoreFrame,
|
||||
@@ -70,6 +71,14 @@ export interface TerminalSessionControllerOptions {
|
||||
// daemon attaches per-row soft-wrap flags to snapshots; otherwise it omits them
|
||||
// so old (strict-schema) clients still parse the snapshot.
|
||||
clientSupportsWrapReflow?: () => boolean;
|
||||
// Current max bytes queued on the client's transport(s) but not yet sent.
|
||||
// Drives the snapshot catch-up fallback: a keeping-up client reports ~0 and
|
||||
// keeps streaming; a backed-up client trips the snapshot path. Defaults to a
|
||||
// constant 0 (no backpressure signal) so callers without a transport always
|
||||
// stream.
|
||||
// Bytes queued on the client transport but not yet sent, or null when the
|
||||
// transport exposes no backpressure signal (e.g. the multiplexed relay socket).
|
||||
getClientBufferedAmount?: () => number | null;
|
||||
}
|
||||
|
||||
export interface TerminalSessionControllerMetrics {
|
||||
@@ -111,6 +120,7 @@ export class TerminalSessionController {
|
||||
private readonly sessionLogger: pino.Logger;
|
||||
private readonly listTerminalWorkspaceRoots: () => Promise<readonly string[]>;
|
||||
private readonly clientSupportsWrapReflow: () => boolean;
|
||||
private readonly getClientBufferedAmount: () => number | null;
|
||||
|
||||
private readonly subscribedDirectories = new Set<string>();
|
||||
private unsubscribeTerminalsChanged: (() => void) | null = null;
|
||||
@@ -128,6 +138,7 @@ export class TerminalSessionController {
|
||||
this.sessionLogger = options.sessionLogger;
|
||||
this.listTerminalWorkspaceRoots = options.listTerminalWorkspaceRoots ?? (async () => []);
|
||||
this.clientSupportsWrapReflow = options.clientSupportsWrapReflow ?? (() => false);
|
||||
this.getClientBufferedAmount = options.getClientBufferedAmount ?? (() => 0);
|
||||
}
|
||||
|
||||
start(): void {
|
||||
@@ -757,7 +768,21 @@ export class TerminalSessionController {
|
||||
return;
|
||||
}
|
||||
activeStream.outputBytesSinceSnapshot += payload.byteLength;
|
||||
if (activeStream.outputBytesSinceSnapshot > MAX_TERMINAL_OUTPUT_FRAME_BYTES) {
|
||||
// Catch up via a snapshot only when the client is BOTH far behind in
|
||||
// produced output AND actually backed up on the wire. A client that
|
||||
// keeps draining reports ~0 buffered, so it streams continuously even
|
||||
// past the byte threshold. outputBytesSinceSnapshot keeps accumulating
|
||||
// in that case — it's harmless, it only gates the snapshot decision at
|
||||
// the instant backpressure appears, and trySendSnapshot resets it to 0.
|
||||
// A null reading means the transport exposes no backpressure signal
|
||||
// (e.g. the multiplexed relay socket); there we can't tell a slow client
|
||||
// from a fast one, so fall back unconditionally at the byte threshold to
|
||||
// keep a slow relay client from falling unboundedly behind.
|
||||
const clientBufferedAmount = this.getClientBufferedAmount();
|
||||
if (
|
||||
activeStream.outputBytesSinceSnapshot > MAX_TERMINAL_OUTPUT_FRAME_BYTES &&
|
||||
(clientBufferedAmount === null || clientBufferedAmount > MAX_CLIENT_BUFFERED_BYTES)
|
||||
) {
|
||||
activeStream.restore = resolveRestoreAfterOutputOverflow(activeStream.restore);
|
||||
activeStream.needsSnapshot = true;
|
||||
void this.trySendSnapshot(activeStream);
|
||||
@@ -877,6 +902,9 @@ export class TerminalSessionController {
|
||||
snapshot,
|
||||
}),
|
||||
);
|
||||
// The snapshot frame went out-of-band; keep the replay that follows on the
|
||||
// coalescer's trailing path so it doesn't flush back-to-back with it.
|
||||
activeStream.outputCoalescer.markFlushed();
|
||||
return { shouldContinue: true, replayRevision: snapshot.revision };
|
||||
}
|
||||
|
||||
@@ -908,6 +936,9 @@ export class TerminalSessionController {
|
||||
snapshot,
|
||||
}),
|
||||
);
|
||||
// The restore frame went out-of-band; keep the replay that follows on the
|
||||
// coalescer's trailing path so it doesn't flush back-to-back with it.
|
||||
activeStream.outputCoalescer.markFlushed();
|
||||
return { shouldContinue: true, replayRevision: snapshot.revision };
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user