fix(sidecar): keep multi-VM hosts fair — flush queued events, round-robin the stdio loop, rotate the emit round, report control EOF honestly - #1928
Open
WyvernMonarch wants to merge 4 commits into
Conversation
…hutdown The only `shutdown_tx` lives in the response/control reader task, so the shutdown channel closes both on a real host shutdown and when that reader dies on an abnormal EOF or decode error. The `biased` select polls the shutdown arm first, so whichever the cause, `None` meant `break 'protocol` and a 0 exit — and whether the sidecar instead surfaced the reader's `write_error_rx` report (exit 1) came down to whether the protocol task got polled in the window between the reader's `try_send` and its drop. The reader always enqueues its failure before exiting, so a queued transport error is authoritative: consult it before treating the closed shutdown channel as a clean stop. `closing_either_required_ingress_stream_is_terminal` was passing on that race, not on the invariant it asserts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK2HMiUAJryC8KcZCHgUJp
odw-43s, event-pump half. The `'protocol` loop's `tokio::select!` is `biased`, so it polls arms in declaration order and takes the first ready one. `stdin_rx` sat above both event-pump arms, so a host that pipelines requests keeps that arm permanently ready and NO VM's queued output is ever flushed: one tenant's request stream starves every other tenant's events indefinitely. Move `event_ready_rx` and `process_event_notify` above `stdin_rx`, below the control lane (cancels and permission replies must still outrank routine event flushing). Both arms are bounded and cannot starve stdin in return: the drain only empties what is already queued and never produces, and the pump is capped by `runtime.fairness.vm_quantum_operations` plus the pending-process-event capacity. The other half — a long dispatch is still awaited INLINE holding `&mut sidecar`, so an ACP prompt blocks every other VM — needs shared-state access that `dispatch_wire`, `poll_event_wire` and `pump_process_events` cannot give while they are all `&mut self`. DESIGN-43s.md specifies that restructure (per-VM `Arc<Mutex<VmState>>`, `&self` dispatch, actor per VM in the transport) and the regression test it unlocks.
…them 27a1199 moved `event_ready_rx` and `process_event_notify` above `stdin_rx` in the `biased` select. That inverts the starvation rather than removing it, and the inverted form is the worse one: neither event arm is self-limiting under a guest emitting sustained stdout. `queue_pending_execution_event` notifies for every queued event (execution/process.rs:243, :374) and `pump_process_events` re-arms the notify whenever a VM burns its `vm_quantum_operations` quantum (execution/process_events.rs:531), so a permit is stored at essentially all times; and the drain is no pure drain either — a zero-timeout `poll_event` still pulls `process_event_receiver` into `pending_process_events` (service.rs:1694-1717), so the guest refills it as fast as the loop empties it. Above stdin, those arms alternate forever and `stdin_rx.recv()` is never polled. `stdin_rx` is also the cancel lane, contrary to the comment that shipped: `route_decoded_combined_frame` sends every `RequestFrame` — including `RequestPayload::CancelExecution` — to `stdin_tx`, while the control lane admits only `SidecarResponseFrame` and shutdown. So one untrusted guest could stall every other tenant's requests on a shared sidecar and make its own flood uncancellable. Restore the arm order and make the two lanes alternate instead: `service_process_events` does one bounded round (pump every active session, then emit at most `runtime.fairness.vm_quantum_operations` frames, re-arming `event_ready_tx` for whatever the cap leaves), called once per `'protocol` turn at the top of the loop. Every frame-handling path — the `pending_frame` park, the stdin arm, the control lane — returns through it, so a pipelining host can no longer keep queued VM output unflushed, and the cap keeps the converse true. The wake arms now only consume the edge that woke the loop. DESIGN-43s.md §"What shipped" is corrected on both counts. A new unit test pins the routing fact the ordering rests on: cancels are admitted on the stdin lane, not the control lane. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK2HMiUAJryC8KcZCHgUJp
…cap cannot pin one `service_process_events` caps a round at `runtime.fairness.vm_quantum_operations` frames (default 64, crates/runtime/src/lib.rs:83) and the cap is global across sessions, not per session. The emit pass walks the sessions one frame each, so they share a round fairly, but a round that hits the cap breaks partway down the list — and the list was rebuilt in `BTreeSet` order every round, so the break always fell in the same place. With more than 64 sessions emitting, every round served the same leading 64 and the tail was never reached: the fairness fix introduced its own starvation, just one lane over from the one it removed. `rotated_sessions` advances the round's starting session by one per call, so the cut moves instead of pinning. Keeping the cap global (rather than per session) is deliberate: a per-session cap would make one round cost `sessions * quantum` frames and put the cancel lane back behind a wait that grows with tenant count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VK2HMiUAJryC8KcZCHgUJp
Member
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
2 participants
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.
Context
Driving many VMs from one host process through a single sidecar, we hit three fairness/correctness problems in the
'protocolstdio loop and one shutdown misreport. Each is a small, independent change; they are grouped because they all concern the same loop.What each commit does
report a control-stream EOF instead of racing it as a shutdown— a closed control stream was raced against the shutdown path, so a normal EOF could be reported as an abnormal end. It is now reported for what it is.flush queued VM events before admitting new stdin requests— theselect!isbiased, andstdin_rxsat above both event-pump arms. A host that pipelines requests keeps that arm permanently ready, so queued VM output is never flushed: one tenant's request stream starves every other tenant's events indefinitely.round-robin stdin and the process-event pump, not rank them— the follow-up to (2): strict reordering would just move the starvation. The arms now alternate, and both moved arms are bounded — the drain arm only empties what is already queued (it callspoll_event_wire, neverpump_process_events), and the pump arm is capped byruntime.fairness.vm_quantum_operationsper VM plus the pending-event capacity — so they cannot starve stdin in return.rotate the event round's starting session— with a per-round frame cap, always starting the emit round at the same session lets one busy session pin the cap and monopolize output. The round's starting point now rotates.What this does not claim to fix
A long dispatch is still awaited inline holding
&mut sidecar(dispatch_wire,poll_event_wire,pump_process_eventsare all&mut self), so a blocking ACP prompt on one VM still blocks other VMs' requests. That needs a per-VM ownership restructure (actor per VM,&selfdispatch), which is deliberately out of scope here; this PR fixes the event-starvation half only. Happy to share the design notes if that restructure is wanted upstream.Why it is safe
cargo build -p agentos-native-sidecarclean;cargo test -p agentos-native-sidecar --libshows 187 passed / 6 failed, and those same 6 fail on pristinemainat this commit (environment-dependent:/varvs/private/var, pnpm-store paths, an npm shim home expansion), so this branch adds 2 passing tests and no failures.