fix: an abandoned launch keeps its place in the queue (#141, partly) - #143
Conversation
dbgscope#141, partly. Dropping a launch guard before anything pumps does not un-queue its `CreateProcessWide`, and `forget` removed the entry with nothing left behind -- so that process still arrived, was new to the *next* launch's snapshot because it did not exist when that snapshot was taken, and was claimed by it. Measured before the fix: a launch of `ping.exe` behind an abandoned `cmd.exe` was delivered the `cmd.exe`. The entry now stays, marked `abandoned`. There is nothing to leave as an exclusion instead -- which process the queued create will produce is exactly what nobody knows yet -- and being first in registration order is what makes it work, since `deliver` offers in that order. Only a launch, and only one given nothing. An abandoned *attach* is already covered by the engine's `attached_processes`, which is what `Pending::wants` reads it for, and keeping its entry would leave `presence` answering `Listed` for an id nobody holds. An entry with a claim is removed and its claim inherited, as before. `Launched(None)` is removed too: it can never claim anything, so keeping it would leave an entry that does nothing. **What this does not fix, and cannot.** The next launch still does not reliably get its *own* process. `deliver` offers in registration order and the abandoned entry is first, so it takes whichever process arrives first -- and one `WaitForEvent` realises *both* queued creates (measured: session 0 -> 2 on a single pump), so which one the event names is a coin flip. Identifying a launch by arrival order is the residual ambiguity `Arrival` has always documented for two launches pending at once; #139 closed "both guards get the same arrival", not "each guard gets its own". What this does deliver is that the abandoned create is *accounted for*: one entry absorbs one arrival, so the next launch's wait() returns only once a second process has stopped, where before it could return with its own process not yet created at all. #141 stays open, narrowed to the identification. Two of this change's own tests passed for the wrong reason before landing, both worth knowing. Counting processes when wait() returns says nothing, because one pump realises both creates. Asking whether the second program is *listed* says nothing either -- membership is the weaker claim this module is built on not confusing with having stopped -- and it reported "0 short in 10" while the defect was live. The test reads the register instead, and asserts the property that holds: 8/8 with the fix, 4/4 failing without it. `examples/abandoned_launch.rs` is the measurement, kept as the record of what the public surface can and cannot see. Refs #141, #136, #133 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54cc34f125
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.pending[index].claim == Claim::Waiting | ||
| && matches!(self.pending[index].what, Arrival::Launched(Some(_))) | ||
| { | ||
| self.pending[index].abandoned = true; | ||
| return; |
There was a problem hiding this comment.
When CreateProcessWide accepts a request but WaitForEvent later reports that the spawn failed—a behavior this module still explicitly documents in wait_for_live_target—PendingTarget::wait returns Err and then drops its Registered. This branch nevertheless marks that still-Waiting entry as abandoned, even though no process will arrive to satisfy it, so the next successful launch's stop is consumed by the stale entry and its own guard can time out. Distinguish a never-waited drop from a wait that ended with a definitive launch failure, and remove the registration on the latter path.
Useful? React with 👍 / 👎.
Review on #143, and a regression this branch introduced. `forget` keeps an abandoned launch's entry so its queued create is accounted for -- but `PendingTarget::wait` drops its `Registered` on the *failure* path too, so a launch that pumped its whole bound and saw nothing was kept as abandoned and would then take the next launch's stop. That is the exact failure keeping an entry exists to prevent, reached from the other side. `Arrivals::discard` forgets outright whatever the state, and `wait_for_live_target` calls it beside `retire_deferred_attachment` on the one ending that says nothing is coming: pumped the whole bound, still absent. Not on the interrupted branch, which says nothing about whether the process is still on its way, and not on a pump that failed with a live session -- the same narrowing `retire_deferred_attachment` already carries and for the same reason. **The call site is not pinned and the comment says so.** The rule is (`..._keeps_its_place_and_an_attach_does_not` gained a block that fails if `discard` keeps the entry), but the wiring needs a launch whose process never arrives inside `LIVE_WAIT_MS`, and a create that reaches the queue produces a process -- every way a launch fails with nothing created lands on `launch_process_begin`'s own `?`, measured at 1.5ms for a missing image. Backing the call out leaves all 189 green; that was checked rather than supposed, which is why it is written down instead of implied. The attach side has a test for the equivalent wiring only because a pid that never joins is constructible where a launch is not. Refs #141, #143 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
|
Correct, and it is this branch's own regression — fixed in
One correction to the finding, which does not change the fixIt cites "a behavior this module still explicitly documents in What is reachable is the ending you describe arriving by a different route — the bound expiring — What is not pinned, said rather than impliedThe rule is: Verification
|
1 participant
Partial, deliberately, and the part it does not fix is structural. #141 stays open, narrowed.
The defect, reproduced
Dropping a launch guard before anything pumps does not un-queue its
CreateProcessWide, andforgetremoved the entry leaving nothing behind. That process still arrives, is new to the nextlaunch's snapshot because it did not exist when that snapshot was taken, and is claimed by it.
Measured before the fix — a launch of
ping.exebehind an abandonedcmd.exe:The fix
The entry stays, marked
abandoned. There is nothing to leave as an exclusion instead — whichprocess the queued create will produce is exactly what nobody knows yet — and being first in
registration order is what makes it work, since
deliveroffers in that order.Only a launch, and only one given nothing. An abandoned attach is already covered by the
engine's
attached_processes, which is whatPending::wantsreads it for; keeping its entry wouldleave
presenceansweringListedfor an id nobody holds. An entry with a claim is removed and itsclaim inherited, as before.
Launched(None)is removed too — it can never claim anything.What it does not fix, and cannot
The next launch still does not reliably get its own process.
deliveroffers in registrationorder and the abandoned entry is first, so it takes whichever process arrives first — and one
WaitForEventrealises both queued creates (measured: session 0 → 2 on a single pump), so whichone the event names is a coin flip. Identifying a launch by arrival order is the residual ambiguity
Arrivalhas always documented for two launches pending at once; #139 closed "both guards get thesame arrival", not "each guard gets its own". Closing the second needs something to match on
rather than an order.
What this does deliver: the abandoned create is accounted for. One entry absorbs one arrival, so
the next launch's
wait()returns only once a second process has stopped — where before it couldreturn with its own process not yet created at all.
Two of my own tests passed for the wrong reason
Both instances of the same trap, and worth reading before adding a third:
wait()returns says nothing, because one pump realises both creates.claim this module is built on not confusing with having stopped. It reported 0 short in 10 while
the defect was live.
A third draft asserted the image name and failed ~50% of runs with the fix in place, which is
how the ordering limitation above was found rather than shipped. The test now reads the register and
asserts the property that actually holds: 8/8 with the fix, 4/4 failing without it, deterministic
both ways.
examples/abandoned_launch.rsis the measurement, kept as the record of what the public surface canand cannot see. It also settles a contradiction between two recorded facts: a bad image fails at
launch_process_begin(0x80070002, 1.5 ms), not inside the wait — sodeferred_arrival's armC description is wrong,
worker.rswas right, and the "ghost that nothing retires" objection thisissue was deferred on does not exist.
Verification
cargo nextest run: 189 passed.cargo test --doc: 4. fmt and clippy clean.deferred_arrival: arm A 0 short in 40 under 24 spinners, both orderings — unmoved.session_fuzz: clean over seeds 1, 2, 7, 13.Refs #141, #136, #133
🤖 Generated with Claude Code
https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY