C11-217: Move browser waits off main socket path

[BenevolentFutures]

Summary

• Move browser.eval, browser.wait, and browser.download.wait onto the socket worker policy.
• Resolve routing, panel, document, selector, frame, and response snapshots on main; hop each WebKit call to main with DispatchQueue.main.async while the worker owns the wait and timeout.
• Route the legacy send, send_key, send_surface, and send_key_surface waits through the existing off-main terminal-surface wait, then revalidate and inject on main.
• Preserve the C11-222 private-queue download watcher and add the browser worker-policy regression seam.

Validation

• Guarded local build: ./scripts/with-build-lock.sh xcodebuild -project GhosttyTabs.xcodeproj -scheme c11-logic -configuration Debug -destination 'platform=macOS' build CODE_SIGNING_ALLOWED=NO -> ** BUILD SUCCEEDED **.
• Tagged QA build/launch: C11_QA_LAUNCH=fresh ./scripts/reload.sh --tag c11-217-browser-offmain.
• Loaded-page browser.eval '1 + 1' returned 2 in 0.15s.
• C11-209 no-document oracle: eval against a fresh browser surface returned no_document in 0.14s.
• Positive control: historical C11-209 main-held stall was 10.0s; during a current 20s unmet browser wait, tagged tree --no-layout returned in 0.14s while the wait ran to 20.15s.
• Two concurrent waits on different loaded browser surfaces both resolved after DOM mutation (10.28s and 6.30s).
• Download path wait resolved early after a 1-byte fixture appeared (4.24s against a 10s timeout).
• Local XCTest actions were not run, per the brief; CI is the test gate.

Deliberate boundaries

• No merge, deployment, or untagged app launch.
• Other browser.* handlers remain on the main-actor path.

[github-actions]

Deep review — C11-217: Move browser waits off main socket path

Thanks for this — it's a careful, well-documented piece of concurrency work, and the write-up (with the positive-control timings: tree returning in 0.14s while a 20s browser wait ran to 20.15s) is exactly the kind of evidence that makes a threading change reviewable. I traced every migrated path against the surrounding source and the C11-209 / C11-165 / C11-222 history it builds on.

Summary

The PR moves browser.eval, browser.wait, and browser.download.wait off the main-actor socket path onto the socket-worker policy, and routes the legacy send / send_key / send_surface / send_key_surface family through the existing off-main terminal-surface wait. Main-actor state (routing, selector/frame maps, no-document guard, response envelope) is snapshotted on main before the worker starts waiting; each WebKit call hops back to main via DispatchQueue.main.async while the worker owns the timeout. This is the correct shape and it directly extends the established pane.confirm / feedback.submit / surface.send_text worker pattern.

Alignment

Clearly in scope — this is socket/CLI correctness and a latency fix on a shared hot path (a browser.eval no longer freezes the control plane and UI for its whole timeout), squarely under "bug fixes (esp. socket/CLI correctness)" and the Sources/** bugfix category. No tenant-config writes, no display links, no focus theft (all migrated commands stay read-only w.r.t. app focus). It correctly landed in deep review rather than the autonomous lane: 707+/262− across 5 files is well over the 500-line cap, and a concurrency refactor is not a mechanical bug fix.

No prompt-injection or anything untoward in the diff.

Skills contract: satisfied without action. Command names, params, and response envelopes are byte-for-byte identical (browser.eval still returns workspace_id/workspace_ref/surface_id/surface_ref/value; browser.wait adds waited: true; download.wait unchanged), so no skills/c11/** update is required — this is a pure threading move, not a surface-model change.

Findings

Nothing blocking. The core concurrency design holds up under scrutiny:

• Gate + await lifetime is safe. V2BrowserAwaitGate plus the escaping-capture semantics of v2AwaitCallback's off-main branch mean a late WebKit completion after timeout can neither double-finish nor write through a dead frame (result/lock/semaphore are boxed by the escaping closure and outlive the return). gate.cancel() after the await, gate.begin() before launching JS, and gate.complete() before signaling all compose correctly.
• Download-wait ordering is actually tightened. Deferring the observer's signal via Task { @MainActor in finish(true) } guarantees the controller's synchronous append observer has run before the worker pops the queue, closing the pop-ahead-of-append window the old synchronous finish(true) had. The observer is still removed on every outcome.
• Isolation is correct. Selector resolution (v2BrowserResolveSelector) and the frame-selector snapshot (v2BrowserCurrentFrameSelector) now happen inside the @MainActor resolve rather than off-main, and the frame selector is passed by value so the worker never touches v2BrowserFrameSelectorBySurface. The nonisolated conversions (v2NormalizeJSValue, v2JSONLiteral, v2BrowserSelector, the decode/envelope helpers) only touch params and static state.
• Legacy send family preserves behavior. send, send_key, and send_key_surface match their prior error strings and control-scalar / unescape handling exactly; the post-wait resolved.terminalPanel.surface.surface == surface revalidation is a sensible add.

1. [nit] send_surface error message changed for the "resolves but never attaches" case — Sources/SocketHandlers/SocketDispatch.swift (send_surface case) → performLegacySurfaceSendOffMain(..., missingTargetError: "ERROR: Failed to send input", ...). The original sendInputToSurface funneled both "panel not found" and "surface not ready within 2.0s" to "ERROR: Failed to send input" (via resolveSurface returning nil). The new path returns "ERROR: Failed to send input" only for a missing panel; a found-but-not-yet-attached surface now returns "ERROR: Surface not ready". This is arguably an improvement (more specific, and consistent with send_key_surface), but it is a wording change on a diagnostic string an agent might match on. If exact parity matters, pass the not-ready message through; otherwise consider it intentional and ignore.

2. [nit / pre-existing] Off-main hasIssuedLoad read. v2RunJavaScriptOffMain now calls v2BrowserWebViewHasIssuedLoad from the worker in production for the first time, which reads CmuxWebView.hasIssuedLoad (a private(set) var Bool mutated on main) off-main. The value is monotonic false→true and word-atomic, so the worst case is a benign stale read (a spurious no_document, self-correcting on retry) — not a crash. No change needed, but if TerminalSurface-style isolation ever tightens on CmuxWebView, this read should round-trip through main like the comment on waitForTerminalSurfaceOffMain already warns.

Recommendation

Merge after fixes — where "fixes" is really just a decision on finding #1 (keep the new message and it's merge-as-is). The concurrency core is sound and well-guarded. Two asks before landing, both outside this reviewer's reach: (a) confirm CI is green on the host-required c11Tests target, since BrowserAwaitPolicyTests is where the new policy-seam test lives and build-for-testing alone won't exercise it; and (b) since this is a large refactor on a code path shared with upstream cmux's browser substrate, a quick check on whether the worker-policy split is worth surfacing upstream (per the bidirectional-relationship note) — not a blocker for c11.

Really nice work. This retires a genuinely nasty class of stall.

---

🌉 Drawbridge deep review (lane 3) — an automated pass; a maintainer has been notified and makes the final call.