fix(runtime): keep interactive PTY control responsive under output floods - #5406
Conversation
466f0ed to
7171635
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Independent agent review. Reviewed at 7171635ee99759b38bb380d4541983f800790e9d. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.
No P0–P2 findings. One sequencing collision worth deciding deliberately, below.
The epoch is already taken by another open PR
RUNTIME_HOST_COMPATIBILITY_EPOCH is 161 here, and main is at 160 — but #5366 also sets 161. Both are open, and both carry incompatible protocol changes.
I checked whether that can silently produce two mutually-incompatible builds that both advertise 161, which would defeat the handshake the epoch exists for. It cannot: scripts/protocol-epoch-check.mjs rejects headEpoch === baseEpoch when incompatible protocol files changed, and separately rejects any backward move. So whichever merges second fails that check and is forced to 162.
That makes this a sequencing question rather than a defect — but it is one worth deciding rather than discovering in CI, because the loser rebases a 23-file or 129-file branch. Raising it here only because both are open at once and I reviewed the other one an hour ago.
The client/model split is justified by what the reply carries
writeStdin now branches on input.caller, and the reasoning holds where it matters: a client control reply carries no output, so the ordered mutation is all it needs, while model keeps mutateAndSnapshotAtCut because its tool result feeds the transcript. The code says why, and it names the exception rather than leaving it implicit — a client resize still persists eagerly, because it changes the screen without producing output that would otherwise refresh the record.
The stop path still takes the cut (mutateAtCut(() => this.beginStopTermination(...))), so ordering for termination is unchanged by the split.
The relaxation you flagged is bounded by its consumers
You call out that a queued cut is no longer a strict admission fence — bytes admitted after mutateAtCut was queued can merge into the pre-cut tail and be parsed before the mutation runs. The two consumers are the control mutation, which reads current size/mode, and stop termination, which signals the process; neither depends on the screen being exactly at the cut, and everything admitted before the cut is still parsed before it. Flagging it yourself rather than letting a reviewer find it is what made it cheap to check.
Evidence
Built core, storage, runtime, runtime-host, ui and CLI from a cleaned dist: zero TypeScript errors. pty-screen-collector + shell-run*: 77 pass, 0 fail. runtime-resource-coordinator: 27/27. CLI suite: 1062 pass, 0 fail, 3 skipped. Desktop main: 2697/2697.
A note for re-verification, since I hit it again on this PR: @maka/desktop's typecheck consumes packages/ui/dist, so a dist left from another branch surfaces as errors in files this PR never touches — here, VirtualizerHandle and attachCommitScheduler from #5366, producing 38 failures that vanish once @maka/ui is rebuilt at this head. Build order matters more than usual while several large branches are in flight.
Not covered by me
Every performance number in this PR. write() p95 5.6s → 0–8ms during a seq 1 300000 flood is a real-Electron, real-PTY measurement; I cannot launch Electron, so that figure and the mutateAndSnapshotAtCut 5.9s baseline are your measurements, not something this review confirms. The same applies to the renderer-side coalescing and the replay-buffer trim — I read them, I did not observe them under load.
The three adversarial reviews and their findings are likewise reported rather than reproduced here; what I verified is that the fixes they prompted are present in the diff, not that the original defects were as described.
This PR is currently a draft.
Code review, CI status and merge readiness are separate. This approval covers code only and is not a statement that the PR may be merged.
…oods A node-pty output flood queued one paced parser task per event (~15B each), so a keystroke's control cut waited behind tens of thousands of queued parses — measured 5.8s during `seq 1 300000`. Client writes also paid a full screen snapshot, a shell-run row rewrite, and a ~44KB update fanout per key, none of which any caller consumed. PtyScreenCollector.accept now merges unstarted admissions into the pending tail, bounding queue depth to one in-flight write plus one merged tail instead of one task per event; byte budget, eviction, generation, and cut ordering are unchanged. Client-originated writeStdin mutates at the cut without the synchronous snapshot + persist — its reply carries no output — while model-originated writes keep the snapshot contract their tool result feeds. A client resize still schedules persistence since it changes the screen without producing output. Runtime Resource replies are slimmed to match their consumers: control and stop replies drop the unused resource snapshot, start replies accept compact state, and Desktop-owned terminal updates project compact state — identified by the desktop-terminal launch prefix, now in @maka/core — since no transcript tool call consumes their output. Compatibility epoch bumps to 159; mismatched peers are rejected at handshake. The renderer's SessionTerminalRenderQueue coalesces adjacent pending writes the same way, and the PTY replay buffer trims amortized instead of copying 16KB per event. Generated-by: Devin
…ce PTY per publish Adversarial review of the previous commit found three defects. The runtime.resource.start reply lost its wire bound: a one-shot command that reaches terminal status inside runBackgroundBash carries up to ~50KB per stream, which overflowed RUNTIME_RESOURCE_RESULT_MAX_BYTES, failed the operation, and drained the Host. The reply is bounded again through the same shrinkStateToFit path the query pages use. The durable-status read in acquire dropped the repair inspectResource performed for an active record with no live handle, so a stale running PTY record could never be marked orphaned by an attach and kept blocking retirement. Acquire now checks the live handle first and, only when there is none, reads through the manager so the record is repaired; a dying PTY (driverExit/finalizeOnce) is no longer acquirable. Pre-existing: rawSequence advanced once per 4K chunk while each published event carried only the last value, so any publish spanning more than one chunk looked like a gap to the renderer and forced a snapshot resync on every output flood. The sequence now advances once per published event, and getLivePtySnapshot flushes pending bytes first so a snapshot always sits on a publish boundary. Test fixtures still returning the removed control/stop resource shapes were updated to the current wire contract. Generated-by: Devin
7171635 to
50b2f3a
Compare
getLivePtySnapshot calls collector.currentSize() on any live PTY whose exit has not landed yet. A collector that already failed (integrity failure, startup cleanup disposal) throws there, and acquire maps the throw to a Host-wide drain — one dying PTY plus a terminal attach would terminate every session's runs. Report the resource as gone instead, so acquire falls through to the stale-record repair path and answers the existing operation_conflict. Generated-by: Devin
2 participants
Summary
Opening or typing in a Maka terminal stalled for seconds whenever the PTY was producing output. Measured in real Electron with a real PTY:
write()p95 = 5.6s during aseq 1 300000flood.Root cause:
PtyScreenCollector.acceptchained one paced parser task per node-pty event (~15B each), so a keystroke's control cut queued behind tens of thousands of pending parses — 5.9s for a singlemutateAndSnapshotAtCut. The queue was bounded by bytes (1MB) but not by task count. On top of that, every client keystroke paid a full screen snapshot, a synchronous shell-run row rewrite, and a ~44KBshell-runs:updatefanout, none of which any client-side caller consumed.Changes, all inside the existing authorities:
PtyScreenCollector.acceptmerges unstarted admissions into the pending tail. Queue depth goes from O(events) to one in-flight write plus one merged tail; byte budget, eviction, generation tracking, and cut ordering are unchanged. Flood benchmark:writeStdinduringseq 1 300000drops from ~5.8s to 0–8ms.writeStdinsplits by caller.client(Desktop keystrokes) runs the ordered mutation only — no synchronous snapshot or persist, since the reply carries no output — whilemodelkeepsmutateAndSnapshotAtCutbecause its tool result feeds the transcript. A client resize still schedules persistence because it changes the screen without producing output.controlandstopreplies drop the unusedresourcesnapshot,startreplies accept compact state, and updates for Desktop-owned terminals project compact state. Desktop terminals are identified by thedesktop-terminal-launch prefix, moved to@maka/corewith a sharedisDesktopTerminalShellRunpredicate.runtime.resource.controller.acquirechecks the live PTY handle first instead of forcing a live snapshot persist on every terminal attach; when there is no live handle it still reads through the manager so a stale active record is repaired toorphaned, and a PTY that has already exited is no longer acquirable.sequencenow advances once per published event, andgetLivePtySnapshotflushes pending bytes first so a snapshot always sits on a publish boundary. Previously the sequence advanced per 4K chunk while each publish carried only the last value, so every multi-chunk publish (any output flood) looked like a gap to the renderer and forced a snapshot resync. Pre-existing since feat(desktop): add Codex-style side conversations #2428; fixed here because it sits on the same seam and undermines the flood behaviour this PR targets.shell-runs:startsynthesizes theShellRunUpdatefrom the start reply instead of a secondgetRuntimeResourceround trip. Preload'sruntime-host:identitiespull becomes pull-on-miss since the push channel already maintains the maps; previously it fired once per keystroke.SessionTerminalRenderQueuecoalesces adjacent pending writes (same defect on the renderer side), and the PTY raw replay buffer trims amortized instead of copying 16KB per event.coordinator.inspectResourceis removed from the interface (no remaining caller); the manager-level method stays for session recovery.Breaking change
Runtime Resource protocol shape changed;
RUNTIME_HOST_COMPATIBILITY_EPOCHbumps 160 → 161 so mismatched Client/Host pairs are rejected at handshake. CLIMakaUserCommand.resultwidens toShellRunStateResult; after a successfulruntime.resource.stopthe driver retires the command locally and publishes the terminal state through the existing resource-query path.Review focus
Three independent adversarial reviews (SWE-2) ran against the first commit, split by authority: Runtime collector/manager, Host protocol/coordinator plus CLI consumers, and Desktop main/preload/renderer. The second commit fixes what they found: the unbounded
startreply (a one-shot command finishing inside the launch with ~50KB per stream overflowed the wire limit and drained the Host), the lost orphan repair inacquire, and the PTY sequence contract above. Two accepted relaxations remain and are worth a look:mutateAtCutwas queued can merge into the pre-cut tail and be parsed before the mutation runs. Everything admitted before the cut is still parsed before it, and no consumer in the tree depends on the stronger property (the mutation reads current size/mode, and snapshot consumers only need "at least through the cut").runtimeHostSessionReftrusts a cache hit. During the gap between a Host generation flip and delivery ofruntime-host-profiles:changed, a keystroke can be routed to the deadtargetEpochand rejected by the scoped handler; the old per-call identities pull narrowed but did not close the same window, and the maps converge on push delivery.Verification
@maka/runtimebuild plusshell-run-managerandpty-screen-collectortests: 67 pass, 4 skipped, 0 fail, including a new coalescing test, a per-publish sequence test, and a reworked deterministic exit-before-cut test.@maka/runtime-hostbuild plus resource coordinator, process, protocol, two-client UDS, and session continuity tests: 82 pass, including new tests for the bounded start reply and the acquire repair path.maka-agent(CLI) build plus session-driver and transcript tests: 74 + 113 pass.@maka/desktopbuild plus terminal hydration, observer, domains IPC, and workbar tests: 71 pass.npm run formatandnpm run lint: clean.writeStdinunder aseq 1 300000flood goes from ~5.8s to 0–8ms; the pending parser queue stays at 2 entries across ~114k events.AI use
Tool(s) and scope: Devin — diagnosis, benchmarks, implementation, tests.
Checklist
Does this PR entail a change in behavior?