Repository navigation
C11-26: route blocking v2 socket methods off main (closes 4×/day deadlock) - #112
Merged
Merged
Conversation
Introduce SocketCommandExecutionPolicy + V2SocketRequest and a nonisolated dispatcher (parseV2SocketRequest, socketWorkerV2Methods, executionPolicy(forV2Method:), socketWorkerV2ResponseIfNeeded, socketWorkerV2Response, processCommandUsingSocketExecutionPolicy) so the per-client socket loop can route specific v2 methods directly on the worker thread instead of always hopping to @mainactor first. Wire handleClient through processCommandUsingSocketExecutionPolicy and add an invalid_dispatch guard inside processV2Command so any worker- policy method that mistakenly reaches the main-actor handler errors loudly instead of silently re-entering the deadlock path. The allowlist (socketWorkerV2Methods) is empty in this commit; every method still flows through the legacy v2MainSync { processCommand(...) } fallback. Subsequent commits add the surface.* family that actually needs the off-main routing. Mark withSocketCommandPolicy and the v2 encoding helpers (v2OrNull, v2Ok, v2Error, v2Result, v2Encode) nonisolated so they can be called from the new dispatcher. Refs: upstream cmux 2597d88b (hand-port; c11 lacks the auth/feedback/ feed/vm prerequisites and uses a slimmer per-client read loop). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
surface.send_text now runs on the socket worker thread via the
SocketCommandExecutionPolicy.socketWorker policy. The old shape wrapped
the entire body in `v2MainSync { ... }` and called
`waitForTerminalSurface` (→ v2AwaitCallback) from inside that block;
when the surface was not yet attached, v2AwaitCallback took its
main-thread branch and entered a nested CFRunLoopRun, which could
never observe its own `DispatchQueue.main.asyncAfter` timeout because
the main dispatch queue was held by the outer `v2MainSync.sync` block.
That is the C11-26 hang (sample-2026-05-03-19-09).
New shape:
Phase A (Task @mainactor + DispatchSemaphore): resolve tabManager,
workspace, surfaceId, terminalPanel; capture the surface if already
attached. v2Ref / v2OrNull are called here so the response envelope
is built while we are on the main actor. No notification waits in
this phase; it cannot deadlock.
Phase B (worker thread): if the surface was not attached, call the
new `waitForTerminalSurfaceOffMain` helper. It registers the same
NotificationCenter observers as the legacy `waitForTerminalSurface`
(queue: .main), but blocks the worker on a DispatchSemaphore instead
of nesting a CFRunLoopRun on main — observers fire on the
still-free main queue and the semaphore times out cleanly. Once the
surface is ready (or the wait elapses), re-hop to @mainactor for
`sendSocketText` + `forceRefresh`. On timeout, fall through to the
existing `terminalPanel.sendText` queue path.
The user-visible response envelope is unchanged (workspace_id /
workspace_ref / surface_id / surface_ref / window_id / window_ref;
queued flag is preserved via the same code path; dlog shape and field
set unchanged). A DEBUG `dlog("v2.surface.send_text isMain=…")` line
is added at the top of the handler so off-main routing is grep-able
from logs if a future hang regresses.
Drops the `case "surface.send_text"` arm in processV2Command (it is
unreachable now: the executionPolicy guard above the dispatch already
returns `invalid_dispatch` for any worker-policy method that mistakenly
reaches the main-actor handler) and replaces it with a comment.
Refs: upstream cmux 2597d88b for the policy/dispatcher infra; the
allowlist contents and the Phase A/B handler shape are c11-specific
(upstream did not migrate any surface.* method).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same shape as the surface.send_text migration: Phase A on @mainactor to resolve refs and pre-build the response envelope, Phase B on the worker thread to optionally wait for surface attach (via the non-nesting waitForTerminalSurfaceOffMain helper), then a final @mainactor task for the actual sendNamedKey + forceRefresh. surface.send_key wrapped its body in v2MainSync and called waitForTerminalSurface from inside it — the same deadlock pattern that fired for surface.send_text in the captured 2026-05-03 hang. Migrating it now keeps the surface.* family uniform and removes the second copy of the deadlock vector before it gets exercised. Adds the allowlist entry, the dispatcher case in socketWorkerV2Response, and a comment in processV2Command's switch explaining where the case went. Marks v2String nonisolated (it does no MainActor state access; just trims a string) so the param validation can happen on the worker thread before Phase A. A DEBUG dlog at the top of the handler tags off-main routing for forensics, matching the pattern added for surface.send_text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Neither handler has the deadlock vector — neither calls
waitForTerminalSurface, neither waits on a notification — but moving
them to the same socketWorker policy keeps the surface.* family
uniform and makes future audits ("which v2 handlers run on main?")
trivial.
Single-phase migration: one Task @mainactor + DispatchSemaphore wraps
the existing body verbatim, with no Phase B because there is nothing
to wait on. The handler bodies are unchanged from a behavior
standpoint — same param resolution, same response envelope, same
error codes.
Marks v2Bool and v2Int nonisolated so the read_text param validation
can run on the worker thread before the @mainactor block (matches the
pattern already used for v2String).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds tests_v2/test_v2_surface_send_text_no_main_hang.py covering:
1. surface.send_text completes within a 3.0 s wall-clock budget
while a background thread keeps the main queue under pressure
(50 system.tree calls in a tight loop). Pre-fix this could hang
indefinitely if the surface was momentarily detached during a
layout reshuffle; post-fix surface.send_text runs on the worker
pool and the wait is bounded.
2. 20 parallel surface.send_text calls (each on its own socket
connection) all return within a 5.0 s wall-clock budget,
exercising the off-main routing under concurrent load.
The test follows c11 CLAUDE.md "Test quality policy": end-to-end via
the real socket, no source-grep assertions, no plist/pbxproj reads.
Per c11 CLAUDE.md "Testing policy", this test runs against a tagged
debug build only — never an untagged `c11 DEV.app`. Header docstring
documents the C11_SOCKET environment variable for the tagged build
socket the delegator produces (e.g. /tmp/c11-debug-c11-26-fix.sock).
Per the plan's note on the not-yet-attached precondition: a clean
repro of the original detached-surface deadlock requires a
workspace.apply primitive (CMUX-37 territory) that c11 doesn't have
yet. The single-call scenario uses parallel main-actor pressure to
stress the same dispatch path; the burst scenario broadens coverage.
A future test extension can target the not-yet-attached case
specifically once workspace.apply lands.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Trident-flagged blocker B1: worker-policy methods bypass
processV2Command's `v2MainSync { v2RefreshKnownRefs() }` (line 2132),
so a fresh `surface:N` / `workspace:N` ref handle is unresolved on
the first worker call and `v2UUID(...)` silently falls back to the
focused panel — meaning text/keys can be injected into the wrong
terminal.
Add v2RefreshKnownRefs() as the first call inside the @mainactor
block of resolveSurfaceSendTargets, v2SurfaceClearHistory, and
v2SurfaceReadText so the handle map is current before any
v2UUID/v2ResolveWorkspace/v2ResolveTabManager call.
Sources: trident synthesis-action.md B1 (Critical Codex).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Trident-flagged blocker B2: Phase A snapshots terminalPanel.surface.surface on @mainactor, then the worker schedules a separate Task @mainactor for Phase B's sendSocketText / sendNamedKey. Between the two main-actor turns, TerminalSurface.teardownSurface() (also @mainactor — nils-then-frees the ghostty_surface_t) can run. Phase B then calls sendSocketText / sendNamedKey on a freed pointer — undefined behavior, plausible segfault under workspace close + concurrent send_text/send_key. Pre-fix code re-resolved the surface inside one v2MainSync block, so the window did not exist; the C11-26 split into two main-actor hops opens it. Re-read resolved.terminalPanel.surface.surface inside Phase B's @mainactor block in v2SurfaceSendText and v2SurfaceSendKey. If the live pointer is non-nil, use it. If nil, fall through to the pending queue (send_text) or return the existing "Surface not ready" error (send_key). Sources: trident synthesis-action.md B2 (Critical Claude, Critical Codex, Evolutionary Codex). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Trident-flagged importants: - I1 (comment update): The handler comment on v2SurfaceSendText and the test docstring still described the migrated path as using v2AwaitCallback's worker-thread semaphore branch. The implementation uses a parallel waitForTerminalSurfaceOffMain helper (the ticket non-goals protect v2AwaitCallback's @mainactor callers from repointing). Updated both comments to name the helper actually in use. - I2 (dead param): resolveSurfaceSendTargets accepted an errMessageOnInternalError String parameter that the body never read (every error path returned its own message). Removed the parameter and both call sites. Sources: trident synthesis-action.md I1 (Standard Codex), I2 (Critical Claude + Critical Codex two-reviewer consensus). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Trident-flagged straightforward mediums:
- M1 (dlog promotion): every migrated handler opened with the same
`#if DEBUG dlog("v2.<method> isMain=\(...)") #endif` block. Lift the
diagnostic to socketWorkerV2Response so future migrated methods get it
for free instead of "we forgot." Behaviorally identical.
- M2 (DEBUG assertionFailure on invalid_dispatch): if a worker-policy
method reaches processV2Command (routing bug), we previously returned
an invalid_dispatch error string and continued. In DEBUG, also dlog
the violation and call assertionFailure so CI catches the regression
before it ships. Release-mode behavior unchanged.
Sources: trident synthesis-action.md M1 (Evolutionary Claude),
M2 (Evolutionary Claude).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
S3: replace 200ms hardcoded sleep in _seed_workspace_and_surface with a 50ms-step poll up to 2s. Cuts CI variance noise without changing the contract the surrounding tests rely on. S5: dlog the slow path in waitForTerminalSurfaceOffMain when requestBackgroundSurfaceStartIfNeeded is invoked. Pure observability — makes slow send_text traces in dev/CI obvious about what they're waiting on. S6: comment the off-main read of TerminalSurface.surface — the recheck post-observer-registration closes the race window, the off-main load is acceptable on Darwin given pointer-sized atomicity, and a future @mainactor migration of TerminalSurface.surface must revisit this helper. Five Trident reviewers flagged this as document-don't-block. S8: docstring runner reference updated from gh workflow run test-e2e.yml to scripts/run-tests-v2.sh, matching the actual runner.
This was referenced May 4, 2026
Merged
BenevolentFutures
added a commit
that referenced
this pull request
May 4, 2026
…xes EXC_BREAKPOINT crash, 4× today) (#121) * C11-26 followup: route bare main.sync handlers through v2MainSync (closes EXC_BREAKPOINT crash) After C11-26 (#112), the new socket dispatcher routes default-policy commands through `DispatchQueue.main.sync { MainActor.assumeIsolated { processCommand(...) } }` from the worker thread. That moves every v1 handler (and every default-policy v2 handler) onto the main thread — a behavior change vs. pre-C11-26 where `handleClient` called `processCommand(trimmed)` directly on the worker. Roughly 100 handlers in `TerminalController` and 12 in `ThemeSocketMethods` were written against the old assumption: each does its own `DispatchQueue.main.sync { … }` to hop onto main. Post-C11-26 that hop is reentrant — libdispatch's self-deadlock guard (`__DISPATCH_WAIT_FOR_QUEUE__`) traps with EXC_BREAKPOINT. The operator hit this 4× today on builds 95/96/97 — once each from DEV-main at 14:41 and 15:01 EDT, then on 0.45.0 at 15:56, then on 0.45.1 at 18:49 — every dump bottoms out at `setProgress(_:) + 924` → `closure #1 in processCommand(_:) + 3856` → `__DISPATCH_WAIT_FOR_QUEUE__ + 484`. The earlier 14:26 IPS hang on 0.44.1 (build 95) is the same class of bug pre-detection: same dispatcher path, same self-wait, but on an older libdispatch that hung instead of trapping. Fix: replace the bare `DispatchQueue.main.sync { … }` calls with the existing `v2MainSync` helper (TerminalController) and a parallel `Self.mainSync` helper (ThemeSocketMethods). Both short-circuit when already on main and only hop when called from a worker, so the same handler is correct under either dispatcher path. The `ThemeSocketMethods` enum is not `@MainActor`, so its helper takes a `@MainActor () -> T` body and uses `MainActor.assumeIsolated`, mirroring `GhosttyTerminalView.performOnMain`. The dispatcher comment in `TerminalController` (lines 1639–) is updated to call out the behavior change explicitly so future handler authors know not to use bare `DispatchQueue.main.sync`. The two intentional bare-`DispatchQueue.main.sync` sites are kept: - line 1734 (the dispatcher itself, which must hop from worker) - line 3366 (inside `v2MainSync`'s own implementation) Other call sites in the tree (`TabManager.swift:586`, `Workspace.swift:5656`, `GhosttyTerminalView.swift:1891`) already gate on `Thread.isMainThread` and are unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * C11-26 followup: regression test — v1 handlers don't self-deadlock on main Adds tests_v2/test_v1_handler_main_self_deadlock.py covering two scenarios: 1. 30 successive `set_progress` calls on a fresh workspace, each on a fresh socket connection. Pre-fix this trapped EXC_BREAKPOINT inside libdispatch's __DISPATCH_WAIT_FOR_QUEUE__ on the first call; subsequent connects then failed with ECONNREFUSED because the listener was gone. Post-fix all 30 return "OK". 2. A spread of v1 commands that share the bare-`DispatchQueue.main.sync` shape (`set_status`, `clear_status`, `report_pwd`, `clear_progress`, `report_git_branch`, `clear_git_branch`). They all route through `processCommand` → switch case → bare main.sync (pre-fix). The dispatcher path is the load-bearing piece; covering several anchors means a regression on the dispatcher cannot regress only one sibling. The test follows c11 CLAUDE.md "Test quality policy": runs through the real socket, no source-grep assertions, no plist/pbxproj reads. Per "Testing policy", runs against a tagged debug build only — header docstring documents `C11_SOCKET=/tmp/c11-debug-<slug>.sock`. A regression that produces only a hang (older libdispatch) is also caught by the per-call wall-clock deadline; one that produces a crash is caught by the next-connection ECONNREFUSED check. Both shapes route through the same assertion ("the call returned OK and the next connect succeeds"), which is what the user actually cares about. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the recurring main-thread deadlock in c11 0.44.1 (4 hangs in a single day) where
surface.send_textwould parkCFRunLoopRun()on the main thread insidev2AwaitCallback, beach-balling the whole app with no recovery. Hand-port of upstream cmux PR #3340 (commit2597d88bby Lawrence Chen, merged 2026-04-30) plus c11-specific scope expansion to actually cover the surface.* methods (upstream's allowlist did not include any of them).Lattice ticket: C11-26 — Route blocking v2 socket methods off main (port upstream cmux #3340). Continues C11-7 scope items 5+6.
What changed
SocketCommandExecutionPolicyenum (mainActorvssocketWorker) +socketWorkerV2Methodsallowlist +parseV2SocketRequest/processCommandUsingSocketExecutionPolicydispatcher. Worker-policy methods run nonisolated on the socket worker thread; only bounded slices hop to@MainActorviaTask { @MainActor in ... } + DispatchSemaphore.socketWorkerpolicy:surface.send_text,surface.send_key,surface.read_text,surface.clear_history. The four handlers that wrapv2MainSync { ... waitFor* ... }and could deadlock.waitForTerminalSurfaceOffMain— observer-then-recheck-then-DispatchSemaphorepattern that is correct on a worker thread (the main queue stays free, observers fire, semaphore signals). LegacywaitForTerminalSurfaceandv2AwaitCallbackare untouched (per ticket non-goals); existing main-actor callers keep working.dlog(\"v2.<method> isMain=… tid=…\")at the dispatcher seam (forensics for off-main verification) +assertionFailureoninvalid_dispatchso a routing-bug regression fails loud at dev/CI time.v2MainSyncWithDeadlineandkTier1MainThreadDeadlineSecondsare unchanged. C11-26 routes thev2MainSync-wrapping family off main entirely so the deadline bridge no longer has to absorb their hangs.Commits (10)
5 implementation commits (
401264a5..0a06e208), 4 review-pass commits applying Trident findings (ae417b30,85d9209a,8b3a2236,474083df), 1 absorb-pass commit for trivial Trident escalations (6d4f1c8d).Validation evidence
c11-26-fix(PID 66809, socket/tmp/c11-debug-c11-26-fix.sock) green via./scripts/reload.sh --tag c11-26-fix.tests_v2/test_v2_surface_send_text_no_main_hang.py: PASS both scenarios (single-call 85ms / 3000ms budget; 20-way concurrent burst 30ms wall / 5000ms budget).surface.send_textload (notes/c11-26-validate-sample-fix-20260503-2237.txt, 5s @ 1ms): 0 v2AwaitCallback / 0 v2MainSync / 0 waitForTerminalSurface* anywhere. Pre-fix had 7120/7120 ticks under that exact tree (notes/hangs/sample-2026-05-03-19-09-c11-0.44.1.txt). New architecture observable on 13 worker threads parked in_dispatch_semaphore_wait_slow.WindowTerminalHostView.hitTest,TabItemView,TerminalSurface.forceRefresh) untouched; diff scoped toSources/TerminalController.swiftregions (processV2Command / socketWorker* / v2Surface*) plus the newtests_v2test file.Full Validate report:
notes/c11-26-validate-report.md(includes ~2-min operator smoke-test instructions).Trident review pack:
notes/trident-review-C11-26-pack-20260503-2146/(9 reviews + 4 syntheses; verdictfix-then-merge, all Apply-by-default items applied).Out-of-scope, deferred to follow-up tickets
Per delegator routing of Trident escalations:
socketCommandFocusAllowanceStackthread-locality (pre-existing global static; no migrated handler reads it today).waitForTerminalSurfaceOffMainslow branch (gated on CMUX-37workspace.applyor a DEBUG-only socket detach helper; 9/9 reviewer consensus).phaseASema.wait()is unbounded (could extend C11-7v2MainSyncWithDeadlinesubstrate to migrated handlers).runOnMainAndWait<T>helper to deduplicate the Phase A/B choreography (3/3 evolutionary reviewers; explicitly held back by operator routing).awaitNotification(name:object:timeout:).Test plan
./scripts/reload.sh --tag c11-26-fixgreennotes/c11-26-validate-report.md(~2 min)References