Repository navigation
fix(terminal): serialize agent-prompt submits per PTY so overlapping sends stop gluing - #14980
brennanb2025 wants to merge 3 commits into
Conversation
…sends stop gluing A submit writes its body, waits a frame for the TUI to render it, then writes Enter. That wait was not atomic, so two overlapping sends to one PTY interleaved across it and their bytes landed on ONE agent input line — Orca manufacturing the merged transcript row the pending-send matchers then have to recognize. Serialize submits per ptyId, reusing the `enqueueLayout` shape without its coalescing: every prompt must be delivered, none may be dropped. A send that arrives while another is in flight is queued behind it, so the RPC contract is unchanged for every client. Covers both gapped write paths: `writeTerminalAgentPrompt` (desktop agent prompts) and `writeTerminalAction` when text is submitted with Enter, which is the path mobile native chat takes. Single-write actions — a bare interrupt, a bare Enter, keystrokes with no submit — stay unqueued so Ctrl+C and live typing are never held behind a queued send. Also: `restoreRejectedDraft` merges the rejected text with anything typed since instead of dropping it. The composer is cleared at send time and a rejected send never reaches `acceptSend`, so skipping left no surviving copy of the message.
📝 WalkthroughWalkthroughThe mobile session now merges rejected draft text with newer composer edits and avoids duplicate restoration. Tests cover empty drafts, whitespace, duplicate text, and tab isolation. The terminal runtime now serializes submit actions per PTY, routes agent prompts through the same queue, preserves direct non-submit actions, and uses a shared 500 millisecond delay constant. Tests cover overlapping submissions, interrupts, and queue recovery after failed writes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
mobile/src/session/mobile-native-chat-rejected-draft-merge.ts (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the implementation comment.
Keep only the non-obvious ordering rule. The current comment is too long for this small helper.
As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line.”
Proposed change
-/** - * Restoring the text of a send the host definitely rejected. - * - * Merge, never skip. The composer is cleared at send time, and a rejected send - * never reaches `acceptSend`, so the rejected text has no other surviving copy — - * declining to restore it because the user has since typed again loses the - * message outright. Over relay/SSH that window is seconds, not milliseconds. - * - * Ordering is by authorship: the rejected text was composed before whatever was - * typed after it, so it goes first and the newer text follows on its own line. - */ +/** Restore rejected text before later composer edits. */Source: Coding guidelines
src/main/runtime/orca-runtime.test.ts (3)
17125-17128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the block comment.
The comment explains the test intent in four narrative lines. Reduce it to one line that states the non-obvious fact: fake timers are required to interleave sends inside the submit gap.
As per coding guidelines: "Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code."
♻️ Proposed comment trim
- // Why: these four cover the glue window at the seam that creates it. The flow - // is ~500ms wide, roughly 10x finer than a device automation step, so it is - // only reachable deterministically here — fake timers park a send inside its - // own submit gap and start the next one by hand. + // Fake timers are the only deterministic way to park a send inside its submit gap.Source: Coding guidelines
17217-17218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the submit marker over the joined writes.
writesis astring[], soexpect(writes).not.toContain(AGENT_PROMPT_SUBMIT)checks array membership. The assertion only holds if the runtime writes the submit marker as its own element. If the runtime ever appends the marker to the body write, the test passes even when the submit already happened. Assert on the joined string to make the check independent of write chunking.♻️ Proposed assertion change
- expect(writes).toContain('\x03') - expect(writes).not.toContain(AGENT_PROMPT_SUBMIT) + expect(writes.join('')).toContain('\x03') + expect(writes.join('')).not.toContain(AGENT_PROMPT_SUBMIT)
17136-17147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated fake PTY setup.
The four tests repeat the same
setPtyControllerblock and terminal creation. Extract one local factory, for examplecreateRecordingPtyRuntime(store), that returns the runtime, the handle, and the recordedwrites. Keep the writable flag as a parameter so the failure test can still reject writes.Also applies to: 17170-17181, 17199-17210, 17230-17245
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f52b9201-ce2c-4a50-b761-03c9a9fd1e3d
📒 Files selected for processing (7)
mobile/src/session/mobile-native-chat-rejected-draft-merge.test.tsmobile/src/session/mobile-native-chat-rejected-draft-merge.tsmobile/src/session/use-mobile-native-chat-drafts.test.tsmobile/src/session/use-mobile-native-chat-drafts.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/shared/terminal-input.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 1 remains after this review.
| setDrafts((previous) => | ||
| (previous[origin.draftKey] ?? '') === '' ? { ...previous, [origin.draftKey]: text } : previous | ||
| ) | ||
| setDrafts((previous) => mergeRejectedDraftInto(previous, origin.draftKey, text)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the restoreRejectedDraft contract comment.
Line 66 says restoration is skipped when newer edits exist. This callback now merges the rejected text before those edits. Update the comment to state the new behavior.
Proposed change
- /** Put the text back after a definite rejection, unless newer edits exist. */
+ /** Restore rejected text before newer composer edits after a definite rejection. */| private writeTerminalAction( | ||
| ptyId: string, | ||
| action: { text?: string; enter?: boolean; interrupt?: boolean }, | ||
| payload: string, | ||
| options: { | ||
| beforeWrite?: (ptyId: string) => void | Promise<void> | ||
| reserveWrite?: (ptyId: string) => void | ||
| afterWrite?: (ptyId: string) => void | Promise<void> | ||
| suffixFailureError?: string | ||
| } = {} | ||
| ): Promise<void> { | ||
| const isSubmit = | ||
| typeof action.text === 'string' && action.text.length > 0 && Boolean(action.enter) | ||
| // Why: only a body→delay→suffix sequence owns a gap to interleave across. | ||
| // Single-write actions — a bare interrupt, a bare Enter, keystrokes with no | ||
| // submit — stay unqueued so Ctrl+C and live typing are never held behind a | ||
| // queued send. | ||
| if (!isSubmit) { | ||
| return this.runTerminalAction(ptyId, action, payload, options) | ||
| } | ||
| return this.enqueueTerminalSubmit(ptyId, () => | ||
| this.runTerminalAction(ptyId, action, payload, options) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant runtime implementation ---'
sed -n '18330,18445p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- action type and terminal send callers ---'
rg -n -C 8 'RuntimeTerminalSend|sendTerminal|writeTerminalAction|writeTerminalAgentPrompt|enqueueTerminalSubmit' src/main/runtime/orca-runtime.ts src -g '*.ts' -g '*.tsx'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- runtime implementation ---'
sed -n '18350,18440p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- focused declarations and call sites ---'
rg -n -C 5 --glob '*.ts' --glob '*.tsx' \
'type RuntimeTerminalSend|interface RuntimeTerminalSend|RuntimeTerminalSend|sendTerminal\s*\(|writeTerminalAction\s*\(' \
src/main/runtime src/shared src/renderer | head -n 500Repository: stablyai/orca
Length of output: 43000
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- terminal.send input schema and flag handling ---'
sed -n '280,365p' src/main/runtime/rpc/methods/terminal.ts
sed -n '1340,1405p' src/main/runtime/rpc/methods/terminal.ts
printf '%s\n' '--- action construction and payload semantics ---'
rg -n -C 8 'buildSendPayload|terminal\.send|interrupt.*enter|enter.*interrupt' src/main/runtime src/shared docs README.md -g '*.ts' -g '*.tsx' -g '*.md' 2>/dev/null | head -n 400Repository: stablyai/orca
Length of output: 35670
Serialize text-plus-interrupt terminal actions.
When action.text is non-empty and action.interrupt is true without action.enter, runTerminalAction still waits between the body and suffix, but isSubmit bypasses the per-PTY queue. Include action.interrupt in the queue condition.
|
This older combined patch can be closed while the remaining overlapping-send problem is carried into the current terminal writer. Failed-message draft restoration already landed in #25149. |
1 participant
Headline: the seam that was named is not the seam mobile uses
The prior-art review that commissioned this work named
writeTerminalAgentPromptas the write seam.Mobile native chat never reaches it. In
src/main/runtime/rpc/methods/terminal.tsthe settled agent-promptpath is gated on
params.client?.type === 'desktop', and mobile sendsterminal.sendwithenter: trueandno
agentPrompt— so mobile routes tosendTerminal->writeTerminalAction, which carries its ownidentical body-to-Enter gap.
Fixing only the named seam would have left the mobile glue window fully open, under a title claiming the
race was fixed, and closed the ticket. That is worse than not fixing it.
This matters past this PR: STA-4492 is the mobile half of the same family, and mobile QA for it was already
reported blocked because the ~500ms window is roughly 10x finer than the emulator can drive. So the mobile
seam had no device-level coverage either — it was neither fixed nor testable. Both are addressed here.
The specific question I want reviewers to attack: is one per-
ptyIdqueue across both entry pointscorrect, or do the two paths have different ordering or lifetime requirements that a shared queue silently
conflates?
Problem
A terminal submit is not atomic. It writes the message body, waits a frame for the TUI to render it, then writes Enter:
awaitthe render gate — or, with no gate,awaitthe fixed submit delay (500ms; 1500ms on Windows),\r.Nothing serialized step 2 per PTY. Two overlapping sends to the same pane interleaved freely across it: send A's body landed, A parked, send B's body landed on the same agent input line, then A's
\rsubmitted A+B as one line.So Orca manufactured the glued transcript row that #14935 and #14936 spend their time reconciling. The host produced one row from two sends, and the pending-send matchers downstream had to recognize it after the fact.
Reproduced deterministically before the fix — two
terminal.sendcalls, 499ms apart:Fix — serialize submits per PTY
enqueueTerminalSubmit(ptyId, run)chains each submit behind the previous one for the sameptyId. This reuses the per-PTY async-queue idiom already in this class (enqueueLayout/runLayoutSlot), minus its coalescing — layout targets may supersede each other, prompts may not. Every prompt is delivered, none is dropped. The stored tail absorbs a rejection so one failed send cannot jam the queue, while the caller still sees its own error.Both gapped write paths, not just one
The write seam exists twice, and the second one is the one that matters for the mobile cluster:
writeTerminalAgentPromptagentPrompt: true)AGENT_PROMPT_SUBMIT_DELAY_MSwriteTerminalActionterminal.sendwith text + EnterTERMINAL_SEND_SUBMIT_DELAY_MSMobile native chat sends
terminal.sendwithenter: trueand noagentPrompt, and the desktop settled-prompt path is gated onparams.client?.type === 'desktop'. So mobile never reacheswriteTerminalAgentPromptat all — fixing only that path would have left the mobile glue window fully open. Both now share one per-ptyIdqueue, so a desktop prompt and a mobile send racing on the same pane are also serialized against each other.Disposition for a concurrent send: queued, deliberately
A send arriving while another is in flight on the same PTY is queued behind it (FIFO), not rejected and not given an interrupt/steer disposition. Reasons, in priority order:
accepted: truewith the samebytesWritten. No new field, no new error code, no new opcode, no capability to negotiate. See the wire section below.refusedReasonthat shipped clients would render as an unknown failure, and on mobile the composer is cleared at send time — a rejection the client can't classify is a lost message.beforeWriteguard awaited inside the slot is capped at 1050ms wall-clock, but a single agent-status read inside that guard has no timeout of its own and can block on SSH/relay. See "Serialization also introduces one new failure mode".What deliberately does not queue
Only a body→delay→suffix sequence owns a gap to interleave across. Single-write actions stay unqueued:
This also means the downstream matchers stay necessary: the user can still type into the TUI by hand while a send is in flight, and shipped clients will keep producing glued rows against updated hosts. This PR removes the source Orca controls.
Test that closes a blocked QA gap
Mobile QA for the glued-send flow was reported blocked: emulator actions cost ~1.2–2.2s each against a ~500ms window, roughly 10× too coarse to land a second send inside the gap. That reasoning is right, and the answer is not a faster emulator.
src/main/runtime/orca-runtime.test.tsgains a deterministic fake-PTY interleaving test on each path. Fake timers park send A inside its own submit gap; send B is started by hand; the bytes the fake PTY controller received are asserted to split into two submitted lines. The race stops being a timing hope and becomes a scheduling decision the test makes — no device, no emulator, no real agent, permanently covered in CI.Every existing test in this cluster tests the matcher: a glued row is synthesized by hand and fed to the reconciler. Nothing tested that the glue happens. These do, at the seam that creates it.
Four tests total:
does not glue two overlapping agent prompt sends into one submitted linedoes not glue two overlapping terminal.send submits into one submitted linelets a bare interrupt through while a submit holds the per-PTY queue— pins the exemption abovereleases the per-PTY submit queue when a send fails mid-sequence— pins that a failure cannot jam the queueSecond fix —
restoreRejectedDraftmerges instead of droppingrestoreRejectedDraftrestored a rejected send's text only if the composer was still empty. The intent — never clobber text typed while the rejection was in flight — is right, but the implementation lost data: the composer is cleared at send time and a rejected send never reachesacceptSend, so the rejected text had no other surviving copy. It was not in the composer, there was no pending bubble, and the only trace was a "Message not sent" toast. Over relay/SSH that window is seconds, not milliseconds.It now merges. Ordering is by authorship: the rejected text was composed before whatever the user typed after it, so it goes first and the newer text follows on its own line (
ping+newer edit→ping\nnewer edit). An identical composer is left untouched rather than duplicated. Whitespace is preserved verbatim on both sides — trailing whitespace is load-bearing here, since the host writes it onto the agent's input line.The rule is extracted to
mobile-native-chat-rejected-draft-merge.tswith its own unit tests;use-mobile-native-chat-drafts.tswas exactly at the 300-line cap, and nomax-linesdisable was added.Known hole this does NOT close — measured, not hypothesised
A submit that fails after its body is written but before Enter (an authority check throwing, a chunk write
returning false) leaves the body text sitting on the agent's input line. The next queued send then pastes on
top of it. Probed against the fixed code — fail the first submit's Enter, then send again on the same PTY:
So, stated precisely: this lock closes the concurrent-send glue path and does not close the failed-send glue
path. That is pre-existing behaviour — the same scenario glues on
main, where the sends also raced on topof it — but serialization promotes it to the only remaining Orca-manufactured glue path.
It is not fixed here because the obvious fix is not obviously safe: clearing the input line (Ctrl+U) on
failure would also erase text the user may have typed into the TUI, and when the failure was
terminal_not_writablethe clear write fails too. This is the open question I most expect to be blocking, andit is question 1 of 5 in the plan.
Serialization also introduces one new failure mode
beforeWriteis awaited inside the queue slot. Its guard is bounded by a 1050ms wall-clock deadline, but asingle
getTerminalAgentStatuscall inside that loop has no timeout of its own, and on SSH/relay it canblock on a real foreground-process read. Before this change a stalled guard blocked only its own send; now
it blocks every subsequent send to that PTY. Bounded queue depth and a per-slot watchdog are open questions
2 and 1 in the plan.
Relationship to #14935 and #14936
All three are complementary. This PR sits upstream of both and blocks neither.
use-mobile-native-chat-drafts.tsis shared with fix(mobile-native-chat): reland glued pending retirement without the two revert causes (STA-4482, STA-4492) #14936, but their hunks are at lines 1–16, 138–141 and 291–322; this PR touches line 22 (one import) and 158–163 (restoreRejectedDraft), which fix(mobile-native-chat): reland glued pending retirement without the two revert causes (STA-4482, STA-4492) #14936 does not modify.git merge-treeagainst both PR heads is conflict-free.Remote wire compatibility
docs/reference/remote-wire-compatibility.mdreviewed. No wire change. The fix is entirely host-side insideOrcaRuntimeService:The one observable difference for an old client is latency: a second concurrent send's ack arrives after its predecessor drains, up to one submit delay later (~500ms, 1.5s on Windows, or the render gate's hard bound). Mobile's send deadline is 15s, so this is immaterial at the concurrency this fixes; a pathologically deep queue is bounded by that budget, and an ack-lost send already has the
holdUnconfirmedSendpath rather than a false "not sent". An older host paired with an updated client is simply unfixed — it keeps gluing, and the downstream matchers handle it as they do today.SSH, relay and folder workspaces: the queue is keyed on
ptyIdinside the runtime that owns the PTY, which is the host in every topology — local, SSH, and relay. Serializing there covers all clients, including ones that never update. Nothing here is worktree-shaped, so folder workspaces behave identically.Verification
orca-runtime.test.ts(full file)typecheck(node + cli + web)tsc --noEmitoxlint --deny-warningson changed fileslint:react-doctorcheck:max-lines-ratchetaudit:code-quality:nativeoxfmtNon-vacuity — production reverted, tests kept: 3 RED.
does not glue two overlapping agent prompt sends into one submitted linedoes not glue two overlapping terminal.send submits into one submitted linemerges a rejected send with newer edits instead of dropping either0 failed merely on a missing export.
TERMINAL_SEND_SUBMIT_DELAY_MSand themobile-native-chat-rejected-draft-mergemodule were deliberately retained during the revert so nothing failed on imports.Two runtime tests stay green under the revert by design — they are guards for the deliberate exemptions (bare interrupt passes through; a failed send releases the queue), not regression tests for the lock. The 6 merge-module unit tests likewise stay green because the module is intact; the hook test is what proves the wiring.
Two unrelated tests (
orchestration-creator-authority-performance,mobile-image-base64-accumulator) failed only under CPU contention from parallel suite runs and pass in isolation.Reviewer findings
Populated by the review loop; mirrored in
PLAN-pty-send-lock.mdsection 13. Records what was accepted,what was rejected, and why.
Round 1 (in progress).