Repository navigation
revert(daemon): dispatch broker requests on the connection task again - #4078
Conversation
This reverts merge ab27bb6 (#4065). It polled each broker request on a blocking-pool thread through native_runtime::off_native_thread. That thread runs outside the connection task's task-local admission scopes, so park_admission could not find the lease and a request parked on a project open, the writer gate, or a generation decode kept its admission slot. The hop also moved whole handler futures onto pool threads with smaller stacks, lost disconnect cancellation, and measured no latency gain. The long-poll failure it worked around is gone: long native polls are now counted, not failed.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ) | ||
| } | ||
| }, | ||
| Ok(request) => execute(request).await, |
There was a problem hiding this comment.
🔴 Long dispatch drops broker responses
When execute(request) uses over 250 ms of CPU in one poll, LongPollGuard still panics in debug builds. The connection closes without answering the request.
Learn more
Broker connections run as native tasks. LongPollGuard still asserts when a native task poll exceeds LONG_POLL_CPU (250 ms). Awaiting dispatch directly makes its synchronous CPU work part of the connection task's poll again. The removed long-dispatch test exercised exactly this case; the assertion prevents the response from being written.
Example: A broker request spends 300 ms of thread CPU inside dispatch before its next await. The native task panics at 250 ms and the waiting client receives a closed socket instead of its response.
Recommended fix: Preserve connection-task admission scope without subjecting synchronous dispatch to the fatal native-poll guard. Change the guard to nonfatal counting as planned, or isolate CPU-heavy work while retaining the admission lease, and retain a regression test for long dispatch.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
| } | ||
| }, | ||
| Ok(request) => execute(request).await, |
There was a problem hiding this comment.
🔴 Writer waits stall broker workers
When execute(request) reaches a synchronous writer wait, it parks a native worker until the writer replies. recv_writer_reply cannot yield that worker, so concurrent waits delay unrelated socket responses and deadlines.
Learn more
The foreground broker runs each connection on a native worker. Synchronous storage ports reach recv_writer_reply, which calls recv_blocking directly outside a Tokio multithread runtime. A writer queue wait therefore occupies the native worker rather than yielding it. Moving the entire dispatch off native workers previously prevented this; running it inline reintroduces native worker starvation under concurrent writer waits.
Example: If all native workers dispatch requests whose synchronous ports wait on a busy writer, none can poll another accepted socket or timer until a writer reply arrives.
Recommended fix: Keep park_admission on the connection task, but move synchronous storage operations to blocking threads or introduce asynchronous writer waits at their ports. Verify native writer-wait counts under concurrent broker requests.
Was this helpful? React with 👍 or 👎 to provide feedback.
Why
A request parked on a project open, the writer gate, or a generation decode gives its admission slot back again, so parked requests cannot starve new calls into
bulk_capacity_reached.#4065 moved every broker request's dispatch onto a blocking-pool thread with
native_runtime::off_native_thread.park_admissionfinds the connection's lease through the Tokiotask_local!CONNECTION_ADMISSIONscopes thatwith_connection_admissioninstalls on the connection task (core_admission.rs,tracedecay-code-index-runtime/src/ports.rs). A future polled on another thread sees no scope,try_withfails, and the park keeps its slot. The move also put whole handler futures on 2 MiB pool stacks, dropped disconnect cancellation, and showed no latency gain. The long-poll failure it worked around goes away in #4076, which counts long native polls instead of failing them.Scope
ab27bb6d5f(fix(daemon): dispatch broker requests off native workers #4065): theoff_native_thread(execute(request))wrap indrive_retained_invocation_responses, the'staticdispatch closures added for the move, andlong_dispatch_tests.broker_dispatch_admission_tests::a_dispatch_parked_on_a_barrier_releases_its_admission_slot. It drives a real broker connection underwith_connection_admissionwith one general slot, parks the dispatch inpark_admission, and expects the slot back while it is parked.off_native_threadsites (invocation/work.rsoff_native_worker,observability_producer.rswork_fan_out_binding) stay. Neither moved future reachespark_admissionor reads a task-local: Work and Workflow dispatch call synchronous storage and session evidence ports, none of the code-index or project-open park sites. That move removed 594 native writer waits. Its one loss is span parentage for child spans ofdaemon.service.work.execute, which no behavior reads.Blast Radius
Broker dispatch runs on the connection task again, as it did before #4065. Long dispatch polls are logged and counted by #4076 instead of failing the connection. Merged without #4076, a debug build's long dispatch poll can still trip the old guard, as it did before #4065.
Verification
the parked dispatch kept its connection's admission slot(left: 0, right: 1) and passes after the revert.//crates/tracedecay:unit_test(897 tests),//crates/tracedecay:daemon_suite(77), and//crates/tracedecay:mcp_suite(666) pass.--config=clippyand rustfmt are clean fortracedecay.