Repository navigation
Report a spawned process's exit unasked: bash in one round trip - #86
Conversation
There was a problem hiding this comment.
Ultimator's review of cbd5af9
This PR adds SPAWN_REPORT_EXIT: the spawning session is sent an EXIT event carrying the exit record (tag 19 advertises the flag), and yas-client's Process::wait uses that event instead of sending WAIT. The wire, limits and client parts look right. The server's handling for an attachment that goes away before the exit has three problems, each reproduced with a probe in client_host: the fallback re-binds the process to the session, and in two races no EXIT is ever sent, so Process::wait() hangs where a WAIT used to answer.
3 issues: 3 bugs
No issue is open since the review of 31d4331.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
Review of yas-run#86: the server's fallback wait, for a REPORT_EXIT process whose attachment went before its exit, bound the process again (a detached one's slot was taken back and ATTACH answered CONFLICT), hung Process::wait when that wait failed, and an attachment aborted mid-send lost the EXIT. The fallback is gone: the attachment reports the exit, from a task of its own so removing the attachment meanwhile cannot cut the EXIT short. When the attachment goes first (a Transfer RESET on any of its streams, stdin included, sent or received, or a DETACH) no EXIT comes, and yas-client knows: its router marks the process's report lost, and Process::wait and wait_timeout then WAIT, after taking an EXIT that already arrived.
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of 31d4331
31d4331 removes the server's fallback wait. Only the attachment that sees the exit reports it, and the send runs in a task of its own. The client now falls back to WAIT once that attachment goes first: a Transfer RESET is sent or received on any of its streams, or detach() is called. I re-ran my round-1 probes at 31d4331. Nothing re-binds the process any more: the new a_process_whose_attachment_went_is_waited_for_and_held_by_nothing test passes. A stream dropped between the exit being queued and routed makes wait answer CONFLICT at once, as it does without the flag. A stream dropped while the EXIT is being sent no longer loses it. No new issues.
No issues found · 3 resolved
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of 958f763
958f763 widens tag 19's hard maximum to 65535 (any u16 of SPAWN flags). Limits::from_extensions now masks tag 19 to the flags this side knows, so a later server's new flag won't fail this client's HELLO. The tag-11 consistency check still applies to the masked value, all the generated schema copies (Rust, TS, JSON, wire.md) agree, and the yas-wire report_exit tests pass at 958f763. No new issues.
No issues found.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
A SPAWN with the new opt-in flag REPORT_EXIT (16) gets its process's exit as an EXIT Event (Process 0x0002, sensitive) once it is final: the record a WAIT would return then. A command's SPAWN Result, output and exit thus all come from one request, where a client had to send WAIT and wait a round trip more. - Servers advertise it in a new optional family limit, LAUNCHER_FLAGS_EXTENDED (tag 19), since clients from before accept at most 12 in tag 11; a client sets REPORT_EXIT only when tag 19 offers it, so either side may be older. - The report takes none of the session's pending WAITs, is sent once (a SPAWN retried under its operation ID shares it), and outlives the streams: when the spawning session's attachment goes before the exit (its streams dropped, their route failed), the server waits for the exit itself and still sends it. - The streams go on at the pace of their credit, so the EXIT may arrive before their last bytes and CLOSE. - yas-client sets the flag whenever offered; Process::wait/wait_timeout then take no request and keep the reported exit. Attached processes, and servers without the flag, still WAIT. Tests (client_host): a_spawned_process_reports_its_exit_without_a_wait (with the only WAIT held, a non-zero exit with stderr, output beyond the stream buffer read while the exit is awaited, a signal, and another session's attachment that still WAITs), and a_server_from_before_report_exit_is_waited_for (ignored; YAS_OLD_SERVER=<yas without the flag>).
Review of yas-run#86: the server's fallback wait, for a REPORT_EXIT process whose attachment went before its exit, bound the process again (a detached one's slot was taken back and ATTACH answered CONFLICT), hung Process::wait when that wait failed, and an attachment aborted mid-send lost the EXIT. The fallback is gone: the attachment reports the exit, from a task of its own so removing the attachment meanwhile cannot cut the EXIT short. When the attachment goes first (a Transfer RESET on any of its streams, stdin included, sent or received, or a DETACH) no EXIT comes, and yas-client knows: its router marks the process's report lost, and Process::wait and wait_timeout then WAIT, after taking an EXIT that already arrived.
LAUNCHER_FLAGS_EXTENDED's hard maximum was 28, the flags it carries today: a later server adding one would have failed the HELLO of every client from this PR, as tag 11's maximum of 12 would have failed older clients for REPORT_EXIT. Its hard maximum is now 65535 (any u16 of SPAWN flags) and Limits::from_extensions keeps only the flags it knows, so the next flag needs no new tag.
958f763 to
29cd072
Compare
|
Reviewed |
There was a problem hiding this comment.
Ultimator's review of 29cd072
29cd072 rebases the PR's three commits onto main c812b11, now that #83 and #85 are merged. git range-diff shows each patch unchanged (cbd5af9 → 622109a, 31d4331 → f9a3673, 958f763 → 29cd072). What main added in between touches only the FS and surface code, nothing in the process code. At 29cd072, cargo xtask protocol --check says the generated artifacts are current, and the yas-wire process:: tests pass. No new issues.
No issues found.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
Stacked on #85 (itself on #83): merge those first. This PR's own changes are the last three commits: cbd5af9, the review fixes in 31d4331, and 958f763 (tag 19 as a flag set).
With this change, a command's exit costs no round trip of its own. A SPAWN that sets the new opt-in flag
REPORT_EXIT(16) is sent its process's exit as anEXITEvent (Process0x0002, sensitive) once the exit is final. It carries the record aWAITwould return at that moment. So the SPAWN Result, the output and the exit all come from one request. Today a client has to sendWAITand wait one more round trip: over 100 ms that is 200 ms fortrue.Protocol
LAUNCHER_FLAGS_EXTENDED(tag 19). Clients from before accept at most 12 in tag 11 (LEAVE_RESIDUE,STDIN_NULL), so tag 11 is unchanged. A client setsREPORT_EXITonly when tag 19 offers it, so either side may be older.REPORT_EXIT.WAITs. It is sent once: a SPAWN retried under its operation ID shares it.RESETon any of its streams, stdin included, from either side, or aDETACH), noEXITis sent and the clientWAITs, as it would without the flag. The server never waits on the client's behalf, so it never binds a process again (review, 31d4331). The pump sends theEXITfrom a task of its own, so removing the attachment at that moment can't cut it short.EXITmay arrive before their last bytes andCLOSE.WAIT.yas-client
Client::spawnsets the flag whenever the server offers it.Process::waitandwait_timeoutthen send no request and keep the reported exit.RESETsent or received on any of them, ordetach(), marks the report as lost. Waiting then takes anEXITthat already arrived, else falls back toWAITwith the time left.WAITas before.Tests (
crates/cli/tests/client_host.rs)a_spawned_process_reports_its_exit_without_a_waitruns with the server's onlyWAITheld, so every exit here must come unasked. It covers:a_process_whose_attachment_went_is_waited_for_and_held_by_nothing(review). It covers:max_processes_per_sessiondetached sleeps, then one more spawn still fits;WAIT.Negative control: without the send-side
RESEThook, it hangs at the dropped-streams exit (30 s timeout).a_server_from_before_report_exit_is_waited_foris ignored by default. Run it withYAS_OLD_SERVER=<yas without the flag, with #85>: the same commands then go throughWAIT.Checks
Run locally at 31d4331, then again at 958f763 for the tag 19 change (
cargo xtask protocol --check, yas-wire 171 passed, client_host 36/36, clippy--workspace --all-targets):cargo fmt --checkpasses;--workspace --all-targetsand yas-server with--no-default-featuresgives no warnings (--all-featureswas clean at cbd5af9; this commit only removes server code);--include-ignoredagainst Pace process output by its owner; drop a lagging watcher alone #85's server;Fork CI (indent-com#40) results will be posted here.