Repository navigation
Conversation
A WAIT on a process its session holds no route to looks at it with a temporary WATCH. That look is refused as CONFLICT while the process's exit is on its way to its watchers (the record is final only once they all have it), or while the session's own previous look, such as the one a KILL's CONTROL just made, is still leaving. So a WAIT right after a KILL of a detached process could answer CONFLICT instead of the exit: client_host's a_process_whose_attachment_went_is_waited_for_and_held_by_nothing failed so on a loaded machine. Such a WAIT now waits for the process catalogue's next change, with which either settles, and looks again, within its timeout. a_wait_right_after_a_kill_of_a_detached_process_gets_its_exit (64 rounds of spawn detachable, detach, KILL, WAIT) failed 1 run in 10 without the change, and none in 20 with it. a_watcher_whose_queue_fills_after_the_exit_fails_alone took only output in its frame loop and panicked on the stdin's progress event, which a slow machine delivers among the frames; it lets it pass now, and waits for the child's reaping (Server::wait_reaped, tests only) rather than 500 ms.
There was a problem hiding this comment.
Ultimator's review of 3ec30d5
When a WAIT's temporary look gets CONFLICT, it now waits for the catalogue's next change and looks again within its timeout. This fixes KILL → WAIT on a detached process (the terminal_queued case settles through finish_detached/release_record, and the new test passes here). It also hardens two tests. The other case the new comment names, this session's own look still bound, isn't settled by a catalogue change (#1).
2 issues: 1 bug, 1 nit
Outside the diff (1)
- ⚪ Nit ·
crates/server/src/yas_process.rs:995-997· Exit-arm comment still says the WAIT answers CONFLICT (#2): With this change, a WAIT in this session no longer answers CONFLICT while the exit is in flight. It waits for the exit to land, then answers with the final record (for a detachable process) or NOT_FOUND. The PR description points to this comment as describing the path.
No issue is open since the review of d721756.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
A WAIT's temporary look can be refused as CONFLICT because this session's own look at the process is still bound: a concurrent CONTROL's, or a route that an eviction failed and has yet to detach (dispatch_outbound fails the route, then sends the Detach). Such a WAIT waited for the catalogue's next change, which a Detach doesn't make, so it slept until the process ended and then found nothing of an ordinary process: NOT_FOUND. It now waits with Manager::wait_native_look, which also wakes when the record changes and returns once nothing refuses a look: no exit in flight and no binding of this endpoint. a_wait_that_finds_its_own_look_still_bound_answers_once_it_goes fails a watcher's route, WAITs, and detaches the look 200 ms later. Without the change the WAIT answered NOT_FOUND once the child's second had passed; with it, the exit. The Exit arm's comment still said such a WAIT answers CONFLICT; it says what it does now.
|
Reviewed |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Ultimator's review of d721756
d721756 swaps the plain catalogue wait for Manager::wait_native_look. It arms catalog_changed and the record's changed before checking. It returns once the revision moves, the handle leaves live, or neither terminal_queued nor this endpoint's own binding still refuses the look. A Detach (which notifies record.changed) now lets the WAIT attach again and get the exit. The new eviction-order test and the KILL → WAIT test pass here (3/3 runs). The Exit-arm comment is updated. Nothing new.
No issues found · 2 resolved
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
1 participant
A WAIT right after a KILL of a detached process could answer CONFLICT instead of the exit. This PR fixes that server race, and one flaky process test.
The race
A WAIT on a process its session holds no route to looks at it with a temporary WATCH (
wait_route→watch_process→watch_native). The native side refuses that WATCH with CONFLICT in two cases:terminal_queued: the record becomes final only once they all have it);dispatch_outboundfails the route, then sends the Detach).The fix: in
wait_route, a CONFLICT from that look now waits for it to settle, then looks again, all within the WAIT's timeout. NewManager::wait_native_lookreturns once either holds:finish_detached/release_record, which move it;record.changed, which it also waits on.Reading the revision needs no snapshot: new
Server::native_catalogue_revision().This surfaced as a flake of client_host's
a_process_whose_attachment_went_is_waited_for_and_held_by_nothingon a loaded machine (the loop that KILLs and WAITs on each detached process gotProcess WAIT: Conflict).New test
a_wait_right_after_a_kill_of_a_detached_process_gets_its_exit, 64 rounds of spawn detachable → detach → KILL → WAIT:with the exit: Conflict);New test
a_wait_that_finds_its_own_look_still_bound_answers_once_it_goes(r2, from the review of 3ec30d5). A watcher's route is failed, it WAITs, and its look is detached 200 ms later:Test hardening
a_watcher_whose_queue_fills_after_the_exit_fails_alone:StdinProgressevent, which a slow machine delivers among the frames. The event now passes, as the other tests already allow.Server::wait_reaped,cfg(all(test, unix))) instead of sleeping 500 ms.Known, left for a follow-up PR
an_owner_that_drops_its_stream_after_the_exit_still_waits_for_it(unchanged here). It has two problems.finals, so a WAIT that looks after the release answers NOT_FOUND. The comment indispatch_outbound's Exit arm describes this path.finals. That is a design change in Pace process output by its owner; drop a lagging watcher alone #85/Report a spawned process's exit unasked: bash in one round trip #86's area, and its PR will say so.a_watcher_whose_queue_fills_after_the_exit_fails_alonestill assumes 80 lines make 80 frames. A loaded reader can read several lines at once, and then the Exit comes before the 80th frame. The follow-up reworks it so it doesn't depend on read sizes.Checks
cargo fmt --check: passes.cargo clippy -p yas-server --all-targets -D warnings: clean.a_watcher_whose_queue_fills_after_the_exit_fails_alone: 10/10 (r1).an_owner_that_drops_its_stream_after_the_exit_still_waits_for_it, which failed 1 lib run with NOT_FOUND.All on a machine at load 50–110. Fork CI: see the linked indent-com draft.