Repository navigation
fix(orchestration): fence worker release on mobile keystrokes - #19337
Conversation
A settled worker's terminal stayed ownership_state='owned' unless a takeover was recorded, and the only recorder was orchestration.workerTerminalUserInput, which only the desktop/web xterm input signal and the native-chat composer call. Mobile input arrives as terminal.send / stream input frames instead of a report, so a phone user typing in a settled worker's pane never fenced anything: worker-list kept recommending release and worker-release closed the PTY under them. Give the host one definition of "a human typed into this terminal" and route every lane through it. The mobile input floor claim is that definition and already exists on both byte lanes: it is taken only for deliberate phone input, never for the emulator's own query replies, and never for an agent's `orca terminal send`, which names itself a desktop client and so is indistinguishable from a keystroke at this layer. Settling that claim after an accepted write now records the takeover through the same code the RPC reporter uses, throttled to one write per pane per 30s so a keystroke does not pay for an immediate transaction. The record lands on the runtime that owns both the terminal and the orchestration database, so SSH-hosted and remote workers behave exactly like local ones. No mobile change: mobile already sends client.type (mobile/src/terminal/terminal-send-request.ts:24).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change centralizes worker terminal takeover recording in shared helpers. Terminal input now classifies human input separately from agent sends and query replies. Accepted human input records takeover before committing the input floor claim. Worker release handling uses the shared recorder. Live terminal handle resolution validates the PTY first. Tests cover input sources, repeated takeovers, retry behavior, dispatch scoping, panes without worker terminals, renderer reload, and resume-fence removal. Merge Risk: ⚪ Minimal · up to Mobile human input now retains worker terminals through release, while agent sends and terminal query replies do not incorrectly claim user takeover. The covered input, reload, and release paths have no remaining actionable merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed summaries, rationale, implementation scope, and test evidence, but it does not follow the required template. It omits the required ELI5, What Changed, Why, Linked Issue with a Resolution Reformat the description to include all required template sections. Add a valid linked issue using ✨ Finishing Touches 💡 1📝 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
A single host-side definition of "a human typed into this terminal" now feeds both the desktop/web report lane and the mobile byte lane, so mobile keystrokes fence workerRelease the same way the desktop reporter already did.
- Shared takeover recorder —
recordWorkerTerminalUserTakeoverinworker-terminal-user-takeover.tsextracts themarkWorkerTerminalUserOwned+sweepSettledWorkerResumeFencesrecording previously inlined inworker-release.ts; the RPC reporter path is behavior-identical. - Mobile byte-lane recording —
settleMobileInputWriterecords the takeover (viarecordWorkerTerminalUserTakeoverFromInput) when an accepted mobile write settles, gated on the mobile floor claim so agent and emulator traffic never fences; throttled to one record per pane per 30s and wrapped so a takeover record can never fail a keystroke. - Tests —
worker-release-input-lane-takeover.test.tscovers the desktop report, mobile keystrokes, mobile stream frames, SSH-hosted parity, coordinator and query-reply negatives, per-window throttling, and non-worker panes; the four positive cases assert the exactretained/user_takeoveroutcome.
ℹ️ Cross-runtime takeover parity is asserted, not exercised
The recorder's doc comment claims the mobile lane lands on "the runtime that owns the terminal … which is also the runtime holding the orchestration database" — true for local, SSH-hosted, and federated workers only if mobile input is delivered to the same host that holds that worker's worker_terminal_resource. The "SSH-hosted" test doesn't exercise that: it only tags hostScope: { kind: 'ssh' } on the same in-memory runtime and DB. That assumption is consistent with the existing runtimeEnvironmentId routing the desktop reporter already relies on, but a federated end-to-end test (mobile input on host → release on home) is the only thing that would pin the real invariant. No change required for this fix; worth covering before the #16904 fix-forward set lands.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
… is fenced The keystroke throttle armed on the attempt rather than on the outcome, so a zero-row or thrown record poisoned the pane for 30s. A phone keystroke during the worker-start readiness wait lands before prepareStartingWorkerAuthority creates the owned resource; a real keystroke seconds later was then suppressed, the worker settled, and workerRelease closed the terminal under the phone user. A SQLITE_BUSY on the first write did the same, with no retry. The cache was the defect, not its arming condition. Its precondition is the set of owned resources on the pane, which changes underneath it, and any cache keyed on ownership identity would have to read the database to learn that identity -- which is the whole question. So the input lane now asks: a read using the same predicate the write uses answers "is anything still fenceable here?" without taking BEGIN IMMEDIATE, and only then is the write attempted. Ordinary typing costs a lookup instead of a write lock, a failed write is retried by the next keystroke, and a takeover writes once per ownership epoch rather than once per window, because the flip to user_owned removes the pane from the candidate set. Sharing the predicate keeps the probe from drifting from the writer. Adds the two escape cases as permanent regressions, drives the mocked send through the real RuntimeTerminalWriter, and asserts a mobile takeover lifts the settled-worker resume fence, which no test covered.
…he read probe The probe was meant to keep keystrokes off BEGIN IMMEDIATE, so it had to earn that with a number. Measured against a real WAL database it costs more than the write it avoids: at 25 live workers the probe is 0.19ms and the no-op write is 0.10ms, because the probe runs the same candidate selection with each statement taking its own read snapshot instead of sharing the transaction's. It is a compensating mechanism with negative value, so it goes, along with the database method and the predicate extraction it needed. owned -> user_owned is one-way and scoped to a resource, so the database is already the dedupe: every deliberate human write attempts the transition, the second attempt matches no row, and the fence sweep runs only on changed > 0. Nothing is remembered between keystrokes, so no state can outlive the ownership it described -- a keystroke before the worker's authority attaches, a write the database refuses, and a re-dispatch onto the same pane all resolve against the rows as they are at that instant. An attempt costs about 0.1ms at typical fleet size and 0.34ms at 100 live workers, on mobile writes only. Replaces the write-count test, which asserted the old mechanism, with the invariant: many keystrokes settle into one takeover and one fence sweep. Adds the re-dispatch case, where a pane's next worker is fenced on its own merits.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The new commit replaces the host-side in-memory 30-second throttle with a database-backed probe, so a remembered "nothing to fence" can no longer outlive its precondition.
- Replaced the in-memory throttle in
recordWorkerTerminalUserTakeoverFromInputwith a database probe — the byte lane now askshasWorkerTerminalUserTakeoverCandidate(the same predicate the write uses) before taking the write lock, which cannot go stale when a worker authority attaches mid-typing. - Extracted
selectTakeoverCandidatesand a sharedCANDIDATE_PREDICATEinto the DB layer, somarkWorkerTerminalUserOwnedand the probe cannot drift; the write still re-reads candidates inside itsBEGIN IMMEDIATEtransaction, keeping the unlocked probe a hint only. - Drove the test's accepted-write stub through the real
RuntimeTerminalWriter(reservation + after-write) and added coverage for resume-fence lifting plus three new cases: write-lock-once, a follow-on keystroke typed before the worker owned the pane, and aSQLITE_BUSYretry.
ℹ️ The byte-lane probe reads the database on every keystroke
hasWorkerTerminalUserTakeoverCandidate runs at least one query per mobile keystroke and falls through to a full scan of worker_terminal_resources for a pane that owns no worker (pane_key is not indexed). This is a deliberate, documented trade-off that holds up while the table stays bounded by live workers; just worth revisiting if worker populations grow, where the existing paneKeyMatchSuffix idiom in pane-key-match.ts offers an indexable pre-filter for the reminted-leaf fallback.
ℹ️ PR description is now stale on the throttle
The ## Summary still claims the lane is "throttled to one write per pane per 30 s"; that throttle was removed in this commit in favor of the database probe. Worth updating the prose before the #16904 fix-forward set lands in review.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The third commit walks back the database read probe from the prior review and lets the owned → user_owned transition itself do the deduping.
- Dropped the
hasWorkerTerminalUserTakeoverCandidateprobe — the DB layer reverts to a singlemarkWorkerTerminalUserOwned(net-zero againstmain), andrecordWorkerTerminalUserTakeoverFromInputnow just calls the shared recorder on every keystroke instead of probing first. - Database dedupes the takeover — because the transition is one-way and per resource, later keystrokes match no
ownedrow and the resume-fence sweep runs once per takeover rather than once per keystroke. - Tests reworked and added one — the write-lock-once case became "settles into a single takeover however many keystrokes arrive" (asserting the sweep runs once), and a new case pins that a takeover fences the dispatch that owns the pane now, not the one that just released it — solid coverage for the reminted-leaf/fresh-dispatch timing.
ℹ️ The steady-state keystroke still pays the fallback scan under the write lock
The "0.1 ms … the second attempt matches no row" comment understates the per-keystroke cost. markWorkerTerminalUserOwned re-runs its candidate SELECT inside the BEGIN IMMEDIATE transaction on every keystroke, and once a pane is already user_owned (or never owned a worker) the indexed exact-pane query returns empty, so each keystroke falls through to the reminted-leaf fallback — a scan over all owned, non-stopping worker resources filtered in JS with isEquivalentPaneKey — while holding the write lock (busy_timeout = 5000, WAL, better-sqlite3 so the block is synchronous). The dedupe is real on the UPDATE side, but the SELECT and write-lock acquisition are not deduped. This is a deliberate, documented correctness-over-cost trade-off and I don't consider it blocking; just noting that the comment's cost figure reflects only the unconcontended happy path, not the steady state of a phone actively using a worker terminal.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
… off The fence rode the mobile input floor claim, with only a comment tying the two together. The floor is arbitration -- who may write next -- while the fence needs provenance -- who produced the bytes. They agree today, so anyone reweighing the floor would have moved the fence without noticing. isDeliberateHumanInput states the provenance rule on its own terms, and both byte lanes decide with it when they open a write: the claim carries the verdict beside the handle, and settlement records the takeover only when a human produced the bytes. No behavior change -- afterWrite is wired only where the predicate already answers true -- and the rule is now pinned by its own cases, so a future arbitration change has to answer this question again rather than inherit it.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This commit names the provenance rule the takeover fence hangs off, decoupling "who produced these bytes" from "who may write next", and pins it with a unit test.
- Extracted
isDeliberateHumanInput— the fence now hangs off explicit byte provenance (client.type === 'mobile', or a pre-refactor clientless mobile lane, and never aquery-reply) instead of reading the mobile input floor. - Introduced
newMobileInputWrite— aMobileInputFloorClaimHoldernow carries the floor handle and a precomputedhumanInputverdict together, decided before the bytes move. - Gated
settleMobileInputWriteonclaim.humanInput— the takeover record fires only for deliberate human input, with the floor commit unchanged. - Added a table-driven test — five cases pin the provenance rule directly.
I traced both input lanes for behavioral equivalence against the prior floor-claim gate. In the stream lane isMobile is derived from client.type === 'mobile', so the after-write settles exactly when humanInput is true. In the unary terminal.send lane, resolveMobileFloorClientId is non-null in exactly the cases isDeliberateHumanInput resolves true (both read the same driver). No divergence.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/rpc/methods/terminal/terminal-input-delivery.ts (1)
154-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord takeover after legacy mobile stream input.
When
args.isMobileis true andargs.clientis absent,newMobileInputWritemarks the write as human input. This branch callsruntime.sendTerminalwithoutafterWrite, sosettleMobileInputWritenever records the takeover. A later worker release can then close the terminal after an accepted legacy-mobile keystroke.Invoke
settleMobileInputWritethroughafterWritewhenfloorClaim.humanInputis true. Add a regression test for a clientless mobile stream write.Proposed fix
- const result = await runtime.sendTerminal(args.terminal, action) + const result = await runtime.sendTerminal(args.terminal, action, { + ...(floorClaim.humanInput + ? { afterWrite: () => settleMobileInputWrite(runtime, floorClaim) } + : {}) + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 20a2740b-52a6-4df3-8277-501ce357fcde
📒 Files selected for processing (4)
src/main/runtime/rpc/methods/orchestration/worker/worker-release-input-lane-takeover.test.tssrc/main/runtime/rpc/methods/terminal/terminal-input-delivery.tssrc/main/runtime/rpc/methods/terminal/terminal-send-method.tssrc/main/runtime/rpc/methods/worker-terminal-user-takeover.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
…phone A phone build older than client.type is recognised only by its pane's mobile driver, which the unary lane passes as the provenance evidence. Nothing proved it did: replacing that argument with false left all 17 tests green while a shipped phone silently stopped fencing worker release. The new case drives a clientless send on a mobile-driven pane and fails under that mutation. The stream lane now passes false outright. Its isMobile is read off the same client object it carries, so the metadata-less phone cannot reach it, and passing the flag suggested a legacy path that does not exist there. Also states what the per-keystroke cost scales with. A pane owning no resource misses the pane_key index and falls through to a scan of owned resources, so the figure is tens of microseconds at realistic worker counts rather than a flat 0.1ms, and it grows with rows that are never released.
…epted write A phone older than client.type sends no client metadata, and both stream initializers derive isMobile from that metadata alone, so such a subscription reported false and took the stream lane's uninstrumented branch: provenance was computed and then never consumed. Bytes from a real person landed through both frame adapters and the resource stayed owned, so workerRelease closed the PTY under them. The unary lane already fenced that population off the pane's mobile driver, which is the host's standing reading of clientless input, so the two byte lanes disagreed at the destructive boundary. The predicate was still subordinate to floor plumbing: it could only be consulted where a floor client id existed. Now the accepted-write callback attaches on both lanes regardless of whether a floor was reserved, and humanInput alone decides recording; a write holding no claim commits nothing. Arbitration keeps its own condition around reserveWrite, where it belongs, and the unary lane's duplicate outer provenance filter is gone. The stream lane reads clientless provenance from the pane's driver, the same policy the unary lane uses. The claim holder is now TerminalInputWrite, carrying the verdict beside an optional floorClaim, so the structure says what the doc said: a write may fence without holding the floor. Regressions drive both real frame adapters, clientless direct delivery, and the paired-web desktop negative. Metadata-only provenance fails 3 on the stream lane and 1 on the unary lane; gating the callback on a reservation fails the same 3.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: bee0c902-612c-4602-aed2-b3563e56225e
📒 Files selected for processing (4)
src/main/runtime/rpc/methods/orchestration/worker/worker-release-input-lane-takeover.test.tssrc/main/runtime/rpc/methods/terminal/terminal-input-delivery.tssrc/main/runtime/rpc/methods/terminal/terminal-send-method.tssrc/main/runtime/rpc/methods/worker-terminal-user-takeover.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/runtime/rpc/methods/worker-terminal-user-takeover.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The two new commits close the round-2 stream-lane gap: a phone that predates client.type now fences the release on every byte lane, and provenance alone decides the record.
- Derived clientless stream provenance from the live pane's mobile driver —
isMobileDrivenPanenow feedsnewTerminalInputWritefor clientless stream frames instead of theisMobileflag a pre-client.typephone never sets, so a legacy phone'sInputframes fence the release through both real frame adapters (registerLegacyBinaryControlFrames,installMultiplexSlotFrames). - Made the provenance verdict alone decide the takeover —
MobileInputFloorClaimHolderbecameTerminalInputWrite(verdict beside an optionalfloorClaim), and both byte lanes now attachsettleTerminalInputWriteto every accepted write rather than gating the callback on a floor reservation, so a write holding no claim still records while arbitration keeps its own floor condition. - Dropped the unary lane's outer duplicate gate —
terminal.sendalways routes itsafterWritethrough the shared settle path; the takeover record and floor-claim commit now share one settlement. - Added regression tests — a clientless unary send on a mobile-driven pane, a two-lane loop pushing real input frames through both frame adapters, direct clientless stream input, and a paired-desktop-web negative that stays on the report lane.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
A renderer reload clears transient handles while retaining runtime-owned PTY identities. Legacy mobile provenance saw no leaf, then sendTerminal restored the same handle and delivered an unfenced key. Normalize through getLivePtyForHandle at the shared live-leaf resolver entry so classification and writes agree, preserving existing leaf generation/incarnation checks. Caller audit: - terminal-send-method: driver, query-reply authority, lock and floor checks now resolve the retained PTY before sending. - terminal-input-delivery: legacy mobile classification and exact-PTY binding now see the same target as the writer; equality checks remain. - terminal-multiplex-subscribe-resolution: retained PTYs resolve directly without a spurious missing-terminal wait. - terminal-lifecycle-methods resize and terminal-viewport-methods display mode, restore-fit and updateViewport retain their original PTY target. - inspectTerminalProcess: avoids false terminal_gone after reload while preserving provider inspection and incarnation fences. - getLivePaneKeyForTerminalHandle and getOrchestrationDispatchAuthority: unaffected because both already call getLivePtyForHandle first. No wire/schema changes, host fallback, process-death inference, or Git workspace assumptions; SSH providers keep ownership of execution evidence. Validation: - Unmodified round-3 reviewer probe: reproduced 2/2 failures, then 2/2 pass. - Unmodified round-2 reviewer probes: 13/13 pass. - Checked-in takeover suites: 24/24 pass. Removing only the resolver call fails both new reload cases; source restored afterward. - RPC orchestration + terminal, aggregate runtime handle registry, handle incarnation, mobile tab mount, stale geometry, and reload probe: 2027 passed, 1 skipped (89 files). - tc:node and check:code-quality:changed pass; background launch enabled.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor readability suggestion inline.
Reviewed changes
The new commit closes the round-3 reload gap: markRendererReloading clears the transient handles record while the ptyId-to-handle mapping survives, so legacy mobile provenance read a missing leaf, failed the mobile-driver check, and let an accepted keystroke land unfenced.
- Resolved retained handles inside the shared live-leaf resolver —
resolveLiveLeafForHandlenow normalizes throughgetLivePtyForHandlefirst, so classification (isMobileDrivenPane,terminal.senddriver resolution) and the writer both recover the runtime-owned PTY a renderer reload clears and agree on the target, while the leaf-generation/incarnation stale check stays intact. - Added a renderer-reload regression test —
worker-release-input-lane-reload-takeover.test.tsdrives a clientless legacy phone through both the stream and unary lanes aftermarkRendererReloading, asserting the resource flips touser_ownedand the release returnsretained/user_takeoverwith nocloseTerminal.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Update exact sendTerminal expectations for the round-2 accepted-write contract. Preserve beforeWrite expectations, absence of reserveWrite, byte payloads and call-count checks; require afterWrite to be a function. Reproduced the requested two-file run: 5 failed, 31 passed. The full RPC suite exposed the same stale shape in ACK budget/overflow, desktop resize (including its later retry), and agent-prompt fallback assertions. Update those too, for 11 assertions across six test files. No production changes. Validation: ORCA_BACKGROUND_LAUNCH=1 full src/main/runtime/rpc suite: 264 files passed; 2292 tests passed, 1 skipped. Changed-code quality and staged oxlint/React Doctor/oxfmt checks passed. Ran lint-staged --no-stash manually to honor checkout safety rather than its default backup hook.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The new commit is test-only: it tightens the existing terminal RPC assertions so every sendTerminal call must carry the unconditional afterWrite callback, pinning the round-3 invariant that both byte lanes settle each accepted write regardless of a floor reservation.
- Required
afterWriteacross six existing suites —terminal-send,terminal-agent-prompt-send, and the fourterminal-multiplex-*suites now assert{ afterWrite: expect.any(Function) }on theirsendTerminalcalls, so a regression that drops the accepted-write callback (and reopens the clientless-legacy-phone takeover gap) fails instead of silently passing. The assertions are exact enough to fail (expect.any(Function)is required, not optional), and the settle path already no-ops for the non-human cases these suites cover (desktop web, agent prompt, query-reply), so nothing is over-constrained.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
New phones announce accepted real user input through the existing worker report RPC, addressed by terminal handle. Share a per-client/per-handle 30-second gate with one bounded retry; report through the same RPC client as the input. Cover live commits and dictation via their shared sender, accessory keys, gestures, buffered submit, paste and accepted native chat. Query replies, attachment heals, triage and diff-review sends do not report. Phones predating this build do not fence release. Remove byte provenance and takeover callbacks from host delivery. Restore both lanes' pre-PR floor-claim plumbing and the original options assertions. Keep the host recorder uncached with its conditional resume-fence sweep. No DB schema or stream change; terminal is an optional report address. Retain the shared resolver recovery independently of takeover: the new SSH inspection test fails without it during renderer reload. Other callers still benefit for subscription, resize, viewport and exact-PTY binding; unary driver/lock checks see the retained PTY. Pane routing and dispatch authority already recover through getLivePtyForHandle and are unaffected. Existing leaf generation checks and first-PTY adoption remain unchanged. No other input-plumbing hunk is retained relative to the PR base. Replace byte-takeover tests with handle-addressed local/SSH report and unknown-handle tests, plus real unary/stream writes asserting zero SQL prepare/exec calls. Mobile send-site integration covers reports, exclusions, rejected writes and gate counts. Desktop report tests are unchanged. Register replacement coverage in the settled-worker release manifest. Validation (all background): host/RPC/runtime 3541 passed, 2 skipped; mobile session/terminal 2045 passed; node and mobile typechecks, changed quality, mobile oxlint, reliability manifest and max-lines ratchet passed. All five requested mutations fail assertions; resolver revert also fails independent inspection. Staged checks run manually with --no-stash. Final src diff against PR base: 5 files, +165/-13 (previously +839/-85).
The mobile RPC allow-list gates every phone request before dispatch and the reporter swallows a refusal, so without this entry every phone shipped unfenced. Pin it beside the report tests, and pin the once-per-takeover fence sweep the replaced byte-lane suite used to assert.
…rts too A key during worker startup reports before the resource is owned; caching that zero-change reply for 30 s suppressed the report that would have fenced the worker once it attached. Native-chat Stop is deliberate input and now reports on an accepted Escape.
There was a problem hiding this comment.
ℹ️ No critical issues — one informational observation below.
Reviewed changes
This round replaces the byte-inference design with an explicit mobile reporter and deletes the host-side byte-lane takeover plumbing.
- Replaced host byte-lane inference with an out-of-band mobile report — a phone now sends
orchestration.workerTerminalUserInput(handle-addressed) from a throttledreportWorkerTerminalUserInput, and the byte-lane provenance/floor-claim plumbing (isDeliberateHumanInput,TerminalInputWrite,settleTerminalInputWrite) was deleted with the unary/stream lanes restored to their pre-PR form, so real byte deliveries now do zero orchestration SQL. - Extended the report RPC to accept a
terminalhandle —worker-release.tsresolvespaneKey ?? sessionId ?? getTerminalPaneKey(terminal)through a sharedrecordWorkerTerminalUserTakeover; the desktoppaneKeypath is behavior-identical. - Instrumented the mobile send sites — live field/submit/accessory, raw accessory keys, buffered composer submit, gesture arrows, paste, dictation, and accepted native-chat send/stop report on accepted sends; query replies, image heals/attachments, PR-triage, diff-review, and programmatic sends are excluded, each pinned by the send-sites integration test.
- Added
orchestration.workerTerminalUserInputto the mobile RPC allowlist — the reporter swallows a refusal, so a missing entry would silently revert every phone to the unfenced behavior. - Retained the reload-aware handle resolver — the round-4
getLivePtyForHandlenormalization is now justified (and pinned by a regression) by process inspection reaching a surviving SSH PTY during renderer reload, not by the deleted byte lane.
ℹ️ The 30-second throttle does not gate steady-state keystrokes
markWorkerTerminalUserOwned returns 0 both when no worker owns the pane and when the pane is already user_owned (the UPDATE … WHERE ownership_state = 'owned' matches no row either way), and the reporter releases its gate on changed === 0. So the throttle holds for the 30 s after a real takeover, but a phone typing for longer — or typing into a plain, non-worker terminal, the common case — re-arms and re-reports on every keystroke. The practical rate is one orchestration.workerTerminalUserInput RPC plus a full BEGIN IMMEDIATE / fallback owned-scan per keystroke, not "once per 30 s" as the summary claims. The release is necessary to avoid the round-1 worker-startup race (a keystroke landing before ownership cannot be cached as "nothing to fence"), so this is the same deliberate correctness-over-cost trade-off the earlier rounds documented — but the summary's "once per 30 s" framing should be accurate about it.
Technical details
# The 30-second throttle only gates the one-time `owned → user_owned` transition
## Affected sites
- mobile/src/terminal/worker-terminal-takeover-report.ts:30-40 — `changed === 0` (and `.catch`) delete the gate entry, so a completed no-op report re-opens the throttle.
- src/main/runtime/orchestration/db/worker-terminal/worker-terminal-user-takeover.ts:5-63 — returns `0` for both "no worker" and "already `user_owned`", so the client cannot distinguish them without another query.
## Required outcome
- Make the PR summary's throttling claim accurate for steady state, or have the host return enough signal for the client to keep the gate armed when the pane is already `user_owned`.
## Open questions for the human
- Is the per-keystroke report volume in the already-`user_owned` / plain-terminal steady state acceptable, given it reproduces the per-keystroke host cost this redesign aimed to remove?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The new commit closes the armed-on-attempt gap in the reporter and brings the Stop (Escape) send into the reporting set.
- Released the throttle gate on a no-op report —
sendTakeoverReportnow returns thechangedcount, andreportWorkerTerminalUserInputdeletes the gate entry whenchanged === 0, so a keystroke landing before the worker owns the pane no longer suppresses the later report that fences the freshly attached worker (the.catchfailure release is unchanged). A regression pins that a no-op report does not arm the gate, and the existing no-op/failure probes keep the transition-only gating. - Made the Stop send report a takeover — an accepted Escape from
useMobileNativeChatStopnow callsreportWorkerTerminalUserInput(client, handle)using the same client as the send, with regression tests for the accepted and host-rejected cases.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Reopening the gate on a zero-change reply made every accepted key on an ordinary terminal an RPC plus a host write transaction (round 6: 100 for 100). The startup window it closed is unreachable: the agent has no prompt to accept input until after its resource row exists. Plain terminals now pay one report per 30 s window; the native-chat Stop report stays. Send-site fixture answers the report RPC with a changed count; the draft test filters to terminal.send calls.
# Conflicts: # mobile/src/session/mobile-session-route-parity.test.ts
There was a problem hiding this comment.
Important
The gate now arms on a no-op report, which re-opens the round-1 unfenceable-worker window — and the justification that the startup window is "unreachable" doesn't hold.
Reviewed changes
The three commits since the last review change the mobile takeover reporter's throttle semantics, document the retained handle resolver, and pull origin/main in.
- Made the takeover gate ignore the host answer —
reportWorkerTerminalUserInputnow arms the 30-second gate on the send attempt and releases it only on failure, so achanged: 0no-op report no longer re-opens the throttle (a plain terminal now pays one report per 30 s window, closing the round-5 steady-state ℹ️) — at the cost of the startup-window suppression detailed inline. - Documented the reload-aware resolver re-link —
resolveLiveLeafForHandlegained the// Whynote on its discardedgetLivePtyForHandlecall, closing the round-4 nit. - Adjusted send-site fixtures and the native-chat draft test — the send-sites fixture answers the report RPC with
{ changed: 1 }, and the native-chat send test filters toterminal.sendcalls to exclude the unawaited takeover report. - Merged
origin/main— the merge only updates the parity test's pinned hook hashes.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
# Conflicts: # src/main/runtime/rpc/methods/orchestration/worker/worker-release.ts
There was a problem hiding this comment.
No new issues in this incremental round. The takeover report path is correctly realigned with #19544, which removed the settled-worker automatic-resume/hibernation fence machinery from main. Approval withheld — the round-1 takeover-gate concern flagged in the prior review remains open and is untouched by these commits.
Reviewed changes
- Removed the resume-fence sweep from the takeover path —
worker-terminal-user-takeover.tsis deleted andworker-release.tsnow callsmarkWorkerTerminalUserOwneddirectly, sincesweepSettledWorkerResumeFencesandprepareLegacyWorkerTerminalRecoveryno longer exist onmainand there is no fence left to lift on anowned → user_ownedtransition. - Dropped the fence-sweep assertions from
worker-release-mobile-report.test.tsto match the no-fence policy; thechanged: 1 → 0and retained-release assertions still pin the durable takeover. - Shed changes now owned by
main— thedb-stopping-worker-task-guard.test.tsRun fixture and the i18n catalog regeneration came in via theorigin/mainmerge rather than the PR branch.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…s possible A handle-addressed report during the worker's tui-idle wait now finds the custody row written at terminal creation, so it flips the pane to user_owned and worker-release retains it instead of closing it under the user.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This round closes the one open concern from the prior review — the round-1 unfenceable-worker window — by pulling in a main-side custody fix and pinning it with a regression test.
- Merged
origin/main— brings in #19608, which writes the worker-terminal custody row at terminal creation rather than after thetui-idleboot wait, so a phone keystroke landing mid-boot now finds anownedrow to flip instead of reportingchanged: 0. - Pinned the mid-boot takeover —
a phone report during the boot wait takes the pane and fences the later releaseholdswaitForTerminalopen, reportsorchestration.workerTerminalUserInput { terminal }mid-boot, asserts{ changed: 1 }anduser_owned, then requires the laterworkerReleaseto returnretained / user_takeoverwithcloseTerminalnever called. Moving the custody write back below the boot wait fails that assertion, so the regression is fenced.
The round-1 P1 is now genuinely closed: the changed: 0 cases that remain (an already user_owned pane, a plain non-worker terminal, an unknown handle) cannot suppress a later fence. The author's "not to be merged without explicit sign-off" gate for the #16904 fix-forward set still stands; this approval reflects only the outcome of this review round.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
1 participant
Summary
A phone user taking over a settled worker terminal must fence
orchestration.workerRelease. New phones now announce real user input, using the same report-based design as desktop. The host no longer infers takeover from terminal bytes.Compatibility decision: phones predating this build do not fence release. Old mobile clients are out of scope; phone and desktop builds will be OTA-synced going forward.
Fix
orchestration.workerTerminalUserInputunawaited, once per terminal handle per 30 seconds on each owning RPC client. It retries once after failure and releases the gate if both attempts fail. Reports use the exact client used by the send, preserving relay/direct ownership.terminalhandle alongside existingpaneKeyandsessionIdaddresses. The owning runtime resolves handles withgetTerminalPaneKey; clients do not learn pane keys.owned → user_ownedtransition, and sweeps resume fences only when a row changes. Release-table reads and desktop reporting are unchanged.Tests
ORCA_BACKGROUND_LAUNCH=1.srcdiff against the PR base: 5 files, +165/-13, reduced from +839/-85. Mobile changes are additional to that host-only comparison.Round-1 review follow-ups
Codex and Opus both found the same P1: the write was gated by a 30-second pane-keyed throttle armed on the attempt rather than the outcome, so a zero-row or thrown record suppressed every later takeover on that pane for the window. A keystroke during the worker-start readiness wait, or a busy database, left the worker unfenceable.
Fixed by deleting the throttle rather than repairing its arming condition. A cache of "nothing to fence here" cannot be sound: its precondition is the pane's ownership, which changes underneath it, and any cache keyed on ownership identity must read the database to learn that identity, which is the question it was avoiding. I measured the read-first probe I tried next and dropped that too, because against a real WAL database it costs more than the write it avoids.
Round-2 review follow-ups
Codex found the fix incomplete: a phone predating
client.typereportsisMobilefalse on both stream initializers, so its Input frames took an uninstrumented branch and provenance was computed but never consumed. Both frame adapters left the resourceownedand the release closed the terminal. The named predicate was still reachable only through floor-claim plumbing.Closed by three changes. Every accepted write now settles: both lanes attach the accepted-write callback whether or not a floor was reserved, so provenance alone decides recording and a write holding no claim commits nothing. The stream lane derives clientless provenance from the live pane's mobile driver, the policy the unary lane already used, instead of an
isMobileflag read from metadata a legacy phone never sends. And the unary lane's duplicate outer gate around that callback is gone, leaving arbitration only around the floor reservation. The claim holder becameTerminalInputWrite, carrying the verdict beside an optional floor claim, so the structure says what the doc says. Opus separately found that nothing proved the unary lane passed the driver as evidence; that case is now covered. The per-keystroke cost comment now states that it scales with the owned-resource count rather than quoting a flat figure.Known and not addressed here: a phone that subscribes without metadata still cannot hold the input floor, which is pre-existing arbitration behavior this PR does not widen; and the desktop PTY-backed chat composer still reports no takeover, which is a gap in the other direction.
Part of the fix-forward set for #16904 alongside the fence, federated-mailbox, and lifecycle PRs. Not to be merged without explicit sign-off.
Round-3 review follow-ups
Reproduced the reviewer’s P1 in both byte lanes: renderer reload clears the transient handle record while retaining the runtime-owned PTY handle. Legacy mobile provenance read the missing record and decided false before the writer restored the same handle and accepted the key. Release then closed that worker terminal.
Fixed at the identity owner:
resolveLiveLeafForHandlecalls the existinggetLivePtyForHandlebefore reading the transient map. Classification and writes now resolve the same retained PTY. Existing leaf generation/incarnation checks and first-PTY adoption remain intact. No retry, cache, post-write provenance recomputation, wire change, or host fallback was added.Audited every caller: unary and stream provenance, exact-PTY binding, multiplex subscription, resize, viewport operations, and process inspection now recognize retained PTYs through reload. Pane-key routing and orchestration authority already perform owner recovery first and are unaffected. Provider inspection fences remain in place; SSH execution stays with its provider, and resolution introduces no Git-worktree or platform assumptions.
Validation at local commit
ce297b19a84eb51c441d78a9e5a740ef90b64ebf(not pushed):user_takeoverand zero closes.tc:nodeand changed-code quality passed. All tests ran withORCA_BACKGROUND_LAUNCH=1; reviewer-only probe files were removed after acceptance.Round-5 review follow-ups
The user removed old-phone compatibility from scope, superseding the byte-inference design described in earlier rounds. Mobile now explicitly reports accepted real input through one throttled reporter; host byte lanes perform no takeover database work. The prior byte-lane suites were replaced with report-address and no-SQL tests plus mobile send-site integration. Existing floor and options assertions return to the pre-PR contract. The resolver fix remains only because the independent process-inspection regression proves it is needed.
Implemented and validated at local commit
6c665f8b04fcca286148f2383400e5e38df949db; not pushed by the worker. Detailed evidence:/Users/jinwoo/orca/review-archive/followups/mobile-takeover-r5-report.md.