Repository navigation
docs: state what #143 measured about #141, not what the issue first assumed - #201
Conversation
… first assumed The source still carried the two premises #141 was opened on and #143 measured wrong, and in places described the fix as more than it is. - `Arrival`: dropping a launch guard no longer removes its entry, and a launch that cannot start fails at `launch_process_begin` (0x80070002, no guard), not inside the wait, so there is no entry to retire. What is left is identification: a claim makes arrivals distinct, not attributed. - `Arrivals::forget`: keeping the entry is right because of accounting (one entry absorbs one arrival), not because of order. Which process it absorbs is a coin flip, so the next launch is still handed the abandoned one about half the time. - `Arrivals` and `Arrivals::deliver`, and the two tests' headings: two pending launches are given different processes, not each its own. - `deferred_arrival` arm C: it calls the combined `launch_process`, so it could never show where the failure lands; `abandoned_launch` arm B does. - `abandoned_launch`: `session_processes` is public since #188; the example reads `|` because it needs image names. - CLAUDE.md: the same "told apart" overclaim. Comments only; no code or test names change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H7Gnmok17PyEYbcc3aHUm5
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7cbd430409
ℹ️ 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".
| /// hands back no pid to match. It is documented rather than built; the issue records why, and what | ||
| /// would change that. | ||
| /// | ||
| /// **A launch abandoned before it is delivered anything keeps its entry** ([`Arrivals::forget`]). |
There was a problem hiding this comment.
For a launch guard dropped before delivery, Arrivals::forget marks the entry abandoned and returns without removing it, so this newly documented entry outlives its guard until session cleanup. That directly contradicts the same comment's assertion at lines 9579–9580 that an entry cannot outlive its guard and leaves no stale state to clear. Please update that lifecycle explanation (and the equivalent summary in CLAUDE.md) so future cleanup changes account for abandoned entries.
Useful? React with 👍 / 👎.
Codex found that the `Arrival` doc still says an entry cannot outlive the guard that made it, beside a new paragraph saying `Arrivals::forget` keeps an abandoned launch's entry until the session goes. The fact holds, and the claim was in eight places rather than two: `Arrivals`, `forget_the_previous_session`, the clear in `end_session`, `Registered`, `Arrival`, two test comments and CLAUDE.md twice. All now state the exception rather than the rule alone. Two of them were more than wording. The clear in `end_session` was called "tidiness rather than a hazard", but for an abandoned entry it is the hazard's only fix: the create dies with the session, so the entry would wait on into the next one and take its first launch's stop. And this PR's own new text overclaimed in the same direction. It said no abandoned entry can be stranded because a launch that cannot start never constructs one. That is measured only for a missing image, which fails at begin. A process that is created and never stops would leave its entry waiting, to take the next launch's stop. Nothing has measured that case, and the text now says so instead of ruling it out. Comments only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H7Gnmok17PyEYbcc3aHUm5
Comments and example docs only. No code changes and no test renames. Refs #141.
Why
Re-checking #141 against
main(aea1f90) found that the source still states the two premises #141 was opened on, which #143 measured to be wrong. In places it also describes #143's fix as more than it is:Arrivaldoc still says dropping a launch guard removes its entry. It also says keeping the entry isn't the fix because "an image that does not exist fails inside the wait". Both contradictArrivals::forgetanddiscard_registrationin the same file. A missing image fails atlaunch_process_begin(0x80070002, no guard), so there is no entry to retire.Arrivals::forgetjustifies keeping the entry by registration order ("the engine realises queued creates in it"). That contradicts the coin-flip measurement in A launch abandoned before delivery leaves its process to the next launch #141. What makes keeping it right is accounting, not order.Arrivals,Arrivals::deliver,test_two_launches_pending_at_once_are_told_apart's heading andCLAUDE.mdsay two pending launches are "told apart". They are given different processes, not each its own.test_an_abandoned_launch_does_not_hand_its_process_to_the_next_one's heading claims the property its own body says it does not assert and that is not true.examples/deferred_arrival.rsarm C says the failure lands inside the wait. It calls the fusedlaunch_process, so it could never show that.abandoned_launcharm B is the measurement that splits the two halves.examples/abandoned_launch.rssayssession_processesis private. It has been public since Expose what the engine holds: debuggee type and dump files #188, and the example reads|because it needs image names.What changes
Each passage now states what was measured, and says which half #141 still holds open: identification, not accounting. Every figure is the one already recorded in the issue or the tests (0 → 2 processes on one pump, about half of runs,
0x80070002in 1.5 ms). None is new.Test function names are unchanged even where they say more than the test asserts, because #141's comments cite them by name.
CHANGELOG.md's 0.2.0 entry, which uses the same "told apart" wording, is left as written: it is a release record.Commands run
On macOS against the Windows target:
cargo fmt --all -- --check: clean.cargo check --target x86_64-pc-windows-msvc --examples --locked: clean.cargo doc --target x86_64-pc-windows-msvc --no-deps --document-private-items --locked: 55 warnings, the same count as unmodifiedmain. The new intra-doc links (Arrivals::deliver,Arrivals::discard,DebugEngine::launch_process_begin) resolve.No Windows test run, since no code changed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H7Gnmok17PyEYbcc3aHUm5