Skip to content

refactor: an arrival is delivered to the open waiting for it (#136 stage 3) - #139

Merged
glslang merged 10 commits into
mainfrom
refactor/deliver-an-arrival-instead-of-broadcasting-it
Sep 4, 2026
Merged

refactor: an arrival is delivered to the open waiting for it (#136 stage 3)#139
glslang merged 10 commits into
mainfrom
refactor/deliver-an-arrival-instead-of-broadcasting-it

Conversation

@glslang

@glslang glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Stage 3 of #136: deliver instead of broadcast. Only stage 4 is left after this.

What was wrong

stopped_on was an engine-wide set of every (engine id, system pid) the engine had ever stopped
on. Every wait wrote into it and every guard polled it — and because it outlived the opens that read
it, it needed a lifecycle of its own:

  • pruned at both openers, because engine ids are reused immediately and a .detach plus a reattach
    of the same pid brings a whole pair back;
  • cleared where a session is replaced;
  • cleared again where one is ended.

Each of those three arrived as a review finding on #133 rather than as a design. That is the
lifecycle cluster #136 names.

What this does

An opener registers what it is waiting for (Registered), the pump routes a stop to the
first open that wants it and has nothing yet (Arrivals), and the entry dies with its guard.

None of the three lifecycle rules is needed any more: nothing outlives its reader, so nothing can go
stale. prune_processes_that_left is down to the attachment record, which is about the teardown
decision rather than about an open.

Two launches pending at once are told apart

Arrival documented this as an accepted ambiguity — a launch is identified by elimination, so the
first arrival was new to both snapshots and ended both waits. The fix was weighed and rejected at
the time because it needed "new engine-wide state, cleared everywhere a session is replaced and
pruned for pid reuse"
. That was the cost of the record it would have joined, and it is the cost
this shape does not have. An arrival is now claimed by the open it is delivered to, so the
second launch is still waiting when the next one comes.

The state is per client, not per wrapper

A new ClientState, held by Arc and keyed by client pointer in a Weak map. Two DebugEngines
can be live around one IDebugClient6, and a wait_for_event through one used to complete an open
held by the other in its own copy of the record alone — the other then read Listed, waited
again, and spent its whole bound on an event that had already happened. That is
examples/deferred_arrival.rs arm F (29.36s against 8.6 µs) undone by a wrapper boundary rather
than by a missing record, and it was written down as a known gap for two releases.

Stage 2's break scope had the mirror of the same gap and moves into the same Arc, because #136
says it is the same field and doing the halves apart would be two reviews of one seam.

A Weak map needs no equivalent of reissue_identity: a dead entry identifies itself, and the
asymmetry is the point — a stale identity costs a re-read, where a stale arrival would answer
Arrived for a target that never stopped.

One smaller thing

The pending-attach exclusion is registry-local now, as well as still reading attached_processes.
An attach's process is new to a launch's snapshot and so looks like the launch's own; the register
knows about pending attaches where that per-wrapper set does not. Both are kept — the set also
covers an attach whose guard was dropped, whose process joins with no registration left to name it.

A claim that has gone is not a claim that was never made

Arrivals::forget_departed stops an open reporting a process that has left the session as arrived.
Written as clearing the field it also made the entry pending again — and an Arrival::Attached
open wants any stop bearing its pid, so the finished open, registered first, took the next attach of
that pid and the new guard waited out its bound on a target in front of it. So what an open has been
given is a ClaimWaiting, Arrived(entry) or Departed — and Departed is terminal:
deliver skips it, presence answers Absent, and forget hands it to nobody.

Terminal rather than convenient: a .detach and a reattach are a new debug attachment, so the
reattach's initial break does not satisfy the postcondition the first guard's wait() promised.

And a claim is read against what the session holds rather than believed on its own. That half
the old shape had for free — presence_of located the process in held first and asked
stopped_on second, so Arrived could not outlive the target — and replacing the set with a
register dropped the ordering without noticing it had a second job. Since forget_departed runs
from the openers, a session that opens nothing more never prunes, so the claim outlived its process
by an unbounded amount and a guard's wait() answered Ok(()) for a target the caller could not
read. The two rules are independent: one is what presence reports, the other what deliver
offers, and backing either out alone fails its own test and only its own.

Tests

Eight new, each mutation-verified against the mutation it is for:

mutation caught by
delivery ignores whether an open already has one ..._delivered_to_one_open_and_claimed, ..._two_launches_...
a claimed process is offered again ..._delivered_to_one_open_and_claimed
a launch ignores other pending opens ..._pending_attach_keeps_its_process_...
forget keeps the entry ..._delivered_to_one_open_and_claimed
a missing registration is not Absent ..._delivered_to_one_open_and_claimed
state is per wrapper again ..._pump_through_one_wrapper_completes_an_open_held_by_another
a departed claim clears instead of tombstoning ..._departed_claim_does_not_reopen_the_open_that_held_it
presence falls through on a departed claim ..._departed_claim_does_not_reopen_the_open_that_held_it
forget hands its claim along a chain ..._claim_is_inherited_by_every_open_still_waiting
an arrived claim is believed without reading held ..._arrived_claim_is_read_against_what_the_session_holds
any attach registration excludes, whatever its claim ..._finished_attach_does_not_keep_a_launch_from_a_reused_pid

The last three are what review rounds six and seven asked about, and they are worth reading
together: two are defects this branch introduced and the chain one is not, but none was
distinguishable from the code that is right
by any test here before — each new test fails under
its own mutation and nothing else in the suite does. That is the whole reason they got through five
rounds of review, and it is a property of the constructions rather than of the reviewers:

  • the tombstone needs two attaches — the test beside it stages the same sequence with a launch,
    which an unrelated rule (a pending attach keeps its process from a pending launch) protects either
    way;
  • the inheritance needs three launches, because with two "every remaining open" and "the next
    one" are the same set;
  • the held read needs a second process in the session, because presence_of refuses an engine
    holding nothing at all before the register is asked;
  • the attach exclusion needs a departed attach whose guard is still held, since dropping the
    guard takes the entry out of the register and the question with it.

The two pure-bookkeeping ones need no engine, so they run under Miri. Worth noting one thing I got
wrong on the way: my first mutation check aimed at "a claimed process is offered again" was caught
by neither end-to-end test — the two-launch test is pinned by a different rule
(arrived.is_none()), and the claim guard is about a repeat stop on an already-delivered process.
That is why the rules are asserted where they live rather than through an engine.

Two tests are gone rather than passing, with the construction that makes each unreachable named
where the test was:

  • test_ending_a_session_forgets_which_processes_it_stopped_on — there is no record to forget.
  • test_a_process_that_left_takes_its_stop_with_it became
    test_reclaiming_an_engine_id_does_not_reclaim_its_arrival, which asserts the property end to end
    rather than the guard that used to hold it.

The four "records no stop" tests now ask a registered open whether anything was delivered to it,
which is the same claim put to the thing that now does the work, and put through the real predicate
rather than around it.

Verification

  • cargo nextest run: 186 passed. cargo test --doc: 4. fmt and clippy clean.
  • Miri, full suite: 146 passed. Run by hand — CI's Miri does not run on PRs, and this adds a
    Weak map and an Arc shared across wrappers.
  • examples/deferred_arrival.rs, re-run per stage as Refactor: a wait outcome should be a value, not state three parties re-derive #136 asks: arm A 0 short in 40 under 24
    spinners, both orderings; arm F 1.1 µs, arm H 2.1 µs, arm G 0/40 — unmoved.
  • examples/session_fuzz.rs: clean over seeds 1, 2, 7, 13 (40 rounds, 14 steps).
  • The #[ignore]d interrupt and retirement tests: 14 passed, including the 30 s one.
  • Downstream: windbg-mcp against a local path dependency, 644 unit + 99 smoke green with the
    dump tier on. No public API changes, so its repoint is the pin alone.

What is left

One case of the ambiguity, and it is named rather than quietly left. A launch abandoned before
it is delivered anything takes its Waiting entry out of the register with no exclusion behind it,
so the process its deferred CreateProcessWide still produces is new to the next launch's snapshot
and is claimed by it. That is pre-existing — Arrival's doc on main records the same ambiguity as
accepted — and the obvious remedy is a trade rather than a fix, since nothing retires an entry whose
guard is gone and a launch that never starts would then take the next launch's process. #141 has
the analysis; the doc comment here says which case is left and why.

Stage 4 — dropping unsafe impl Sync for DebugEngine, or replacing it with a safety comment. It is
semver-visible and independent of this.

Refs #136, #133, #141

🤖 Generated with Claude Code

https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY

…adcast

`stopped_on` was an engine-wide set of every `(engine id, system pid)` the
engine had ever stopped on: every wait wrote into it and every guard polled it.
Because it outlived the opens that read it, it needed a lifecycle of its own --
pruned at both openers for pid reuse, cleared where a session is replaced, and
cleared again where one is ended. Each of those three arrived as a review
finding on #133 rather than as a design, which is what #136 calls the lifecycle
cluster.

An opener now registers what it is waiting for, the pump routes a stop to the
first open that wants it and has nothing yet, and the entry dies with its guard.
None of the three is needed: nothing outlives its reader, so nothing can go
stale. `prune_processes_that_left` is down to the attachment record, which is
about the teardown decision rather than about an open.

**Two launches pending at once are told apart.** A launch is identified by
elimination, so the first arrival was new to both snapshots and ended both
waits; `Arrival` documented that as accepted because the fix needed "new
engine-wide state, cleared everywhere a session is replaced and pruned for pid
reuse" -- which was the cost of the record it would have joined, and is the cost
this shape does not have. An arrival is claimed by the open it goes to, so the
second launch is still waiting when the next one comes.

**The state is per client rather than per wrapper** (`ClientState`, held by
`Arc`, keyed by client pointer in a `Weak` map). Two `DebugEngine`s can be live
around one `IDebugClient6`, and a `wait_for_event` through one completed an open
held by the other in its own copy of the record alone -- the other then read
`Listed`, waited again, and spent its whole bound on an event that had already
happened, which is `deferred_arrival` arm F undone by a wrapper boundary. That
was written down as a known gap for two releases. Stage 2's break scope had the
mirror of it and moves into the same `Arc`, because #136 says it is the same
field and doing the halves apart would be two reviews of one seam. A `Weak` map
needs no `reissue_identity` equivalent: a dead entry identifies itself, where a
stale identity costs a re-read and a stale arrival would answer `Arrived` for a
target that never stopped.

The pending-attach exclusion is registry-local now as well as reading
`attached_processes`: an attach's process is new to a launch's snapshot and
looks like the launch's own, and the register knows about pending attaches where
that per-wrapper set does not.

**Two tests are gone rather than passing**, with the construction that makes
each unreachable named where it was:
`test_ending_a_session_forgets_which_processes_it_stopped_on` has no record to
forget, and `test_a_process_that_left_takes_its_stop_with_it` became
`test_reclaiming_an_engine_id_does_not_reclaim_its_arrival`, which asserts the
property end to end instead of the guard that used to hold it. The four
"records no stop" tests now ask a registered open whether anything was delivered
to it, which is the same claim put to the thing that does the work.

Four new tests, each mutation-verified against the mutation it is for, and the
two pure-bookkeeping ones run under Miri. No public API changes.

Verified: 173 tests, 4 doctests, the full Miri suite (140), the three
`#[ignore]`d interrupt tests, `deferred_arrival` unmoved (arm A 0 short in 40
under 24 spinners, arm F 3.2us, arm H 3.6us, arm G 0/40), `session_fuzz` clean
over seeds 1/2/7/13, and windbg-mcp against a local path dependency (644 unit +
99 smoke).

Refs #136, #133

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: b6681f71-30bb-45f6-973c-b40b51ac746c


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b0e75ac77

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +498 to +499
fn forget(&mut self, id: ArrivalId) {
self.pending.retain(|pending| pending.id != id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep claimed processes excluded after their guard drops

When two launch guards overlap, consuming or dropping the first guard removes the only record of the process it claimed. If that first target produces another genuine stop before the second launch reaches its initial break, deliver no longer recognizes the process as claimed, and because it is absent from the second launch's snapshot, the stop is delivered to the second guard. Its wait() can then return Ok even though its own process has not stopped; transfer completed claims into the remaining registrations before forgetting this entry.

Useful? React with 👍 / 👎.

Comment thread src/dbgeng.rs Outdated
Comment on lines +3426 to +3428
let entry = held.into_iter().find(|(held, _)| *held == id)?;
self.stopped_on
let attached = self.attached_pids();
self.state

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve attach provenance across wrapper pumps

When wrapper A starts an attach, drops that attach guard, and then holds a pending launch, an outside pump through wrapper B reads B's empty per-wrapper attached_processes set here. Since dropping the attach guard also removed its registry entry, the attach stop can be delivered to A's launch as a newly launched process, causing the launch guard to report success before its own target stops. The attach exclusion used during delivery must survive the guard and be visible regardless of which wrapper performs the pump.

Useful? React with 👍 / 👎.

…ession's

Two P2s from review, both real, and the second is a regression this stage
introduced.

**A claim died with the guard that made it**, so the ambiguity stage 3 claims to
close was closed only while both launch guards were held. Consume the first, and
its target's *next* genuine stop is claimed by nobody -- and is absent from the
second launch's snapshot, because it did not exist when that snapshot was taken
-- so it is delivered to the second launch, whose `wait()` returns `Ok` for a
process that never stopped. A claim is now inherited by the opens still waiting
when its own entry goes. That is not the lifecycle creeping back: it is held by
the opens that exist at that moment and goes when they do, where the record this
replaced lived for the whole session, and the test asserts that half too.

**A session's attachments were still per wrapper**, so delivery through a second
wrapper read an empty set: wrapper A attaches, drops that guard, holds a pending
launch, and an outside pump through wrapper B delivers the attached process to
A's launch as a newly launched one. `attached_processes` moves into
`ClientState` with the rest.

That fixes a second thing nobody raised, and it is the sharper one. An
`end_session` through a wrapper that did not perform the attach saw no
attachment to detach, so its passive end **killed** somebody else's process --
the exact failure that record exists to prevent, reached through the wrapper
boundary. `test_a_sessions_attachments_are_visible_through_every_wrapper`
asserts both halves, and the mutation that stands in for the old placement fails
it on the process being taken.

The sentence that used to keep the record per wrapper argued that sharing would
put "the crate's most consequential decision behind an eviction policy". That is
true of `client_identities`, which is a cache with a cap, and not of
`ClientState`, whose entry is `Weak` and dies with the last wrapper holding it.
Rewritten where it stood rather than deleted, because the objection is right
about the identity cache.

Two new tests, both mutation-verified. Re-verified after: 175 tests, 4
doctests, the full Miri suite (141), `deferred_arrival` unmoved (arm A 0 short
in 40, arm F 3.3us, arm H 3us, arm G 0/40), `session_fuzz` seeds 1 and 7.

Refs #136

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Both right, and the second is a regression this stage introduced. 4101365.

Claims dying with their guard. Correct, and it means the ambiguity this stage claims to close was closed only while both launch guards were held — exactly the case my test covered. Consume the first, its target's next genuine stop is claimed by nobody, and it is absent from the second launch's snapshot because it did not exist when that snapshot was taken. A claim is now inherited by the opens still waiting when its own entry goes, which is your "transfer completed claims into the remaining registrations".

Worth saying why that is not the lifecycle creeping back: the claim is held by the opens that exist at that moment and goes when they do, where the record it replaced lived for the whole session and needed a prune and two clears. test_a_claim_outlives_the_open_that_made_it asserts that half too — an open registered after everything else finished must not inherit anything.

Attach provenance across wrapper pumps. Correct, and it was a regression: the register went per client while attached_processes stayed per wrapper, so delivery through a second wrapper read an empty set. It moves into ClientState with the rest.

That turned out to fix a sharper thing nobody raised. An end_session through a wrapper that did not perform the attach also saw no attachment to detach — so its passive end killed somebody else's process, which is the exact failure that record exists to prevent, reached through the wrapper boundary. test_a_sessions_attachments_are_visible_through_every_wrapper asserts both halves; the mutation standing in for the old placement fails it on the process being taken, not on the delivery.

The sentence that kept the record per wrapper argued sharing would put "the crate's most consequential decision behind an eviction policy". That is true of client_identities — a cache with a cap — and not of ClientState, whose entry is Weak and dies with the last wrapper holding it. Rewritten where it stood rather than deleted, since the objection is right about the thing it was written about.

Re-verified after: 175 tests, 4 doctests, full Miri (141), deferred_arrival unmoved (arm A 0 short in 40, arm F 3.3 µs, arm H 3 µs, arm G 0/40), session_fuzz seeds 1 and 7.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4101365d86

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +619 to +622
if held
.iter()
.any(|entry| pending.wants(*entry, &others, attached))
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude arrivals already claimed by another open

When two launch guards overlap and the first process has been delivered while its guard remains alive, this predicate still treats that process as evidence that the second launch is Listed: wants excludes inherited claims and pending attaches, but not others[*].arrived. If the second wait is interrupted before its own deferred process joins, wait_for_live_target consequently takes the non-Absent terminal branch and returns Ok(()) instead of LiveTargetInterrupted. Exclude entries currently claimed by another pending open when computing presence, just as deliver does.

Useful? React with 👍 / 👎.

Comment thread src/dbgeng.rs Outdated
Comment on lines 5277 to 5281
self.state
.attached_processes
.lock()
.unwrap_or_else(|e| e.into_inner())
.retain(|pid| held.iter().any(|(_, held)| held == pid));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve deferred attachments while pruning

When attach_process_begin(pid) is dropped before its first wait and launch_process_begin is called next, the launch invokes this prune while session_processes() still omits the deferred attachment, so retain removes the PID that claim_attached just recorded. The fresh evidence that the existing review thread remains applicable is that moving attached_processes into shared state does not bypass this prune: after the attach registration is dropped, the next pump can still deliver that process to the launch, and teardown can then treat the externally owned process as launched and kill it. Pending attachment provenance must survive until the attachment joins the session or is canceled.

Useful? React with 👍 / 👎.

…eferred attach

Two more P2s from review. The first is this stage's own claim surviving in the
predicate's other caller; the second is pre-existing and is the more expensive
of the two.

**`presence` did not exclude another open's claim.** `wants` excluded inherited
claims and pending attaches, and `deliver` excluded a claimed process with an
early return of its own -- so the *listing* half never applied it. A second
launch was therefore told `Listed` on the strength of a process the first had
been given, and `Listed` is not `Absent`, so a wait interrupted before its own
process joined answered `Ok(())` instead of `LiveTargetInterrupted`. The rule
moves into `wants`, where both callers get it, and `deliver`'s early return goes
because it now says the same thing twice.

**A prune took an attachment that had not joined yet.** `AttachProcess` joins
its process at the next `WaitForEvent` -- measured, and printed by
`deferred_arrival` arm E -- so between `attach_process_begin` and that wait the
pid is recorded and the session does not list it. An opener pruning in that
window dropped the record, on a rule whose whole subject is a pid that no longer
names anything. What that costs is not a missed exclusion: the teardown then
treats somebody else's process as one this engine launched, and takes it.

An attachment now carries whether it has been seen (`Attachment::Deferred` /
`Joined`). A pid the session holds is promoted; a pid it does not hold has
either left, and is dropped, or not yet joined, and is kept -- it cannot have
left a session it was never in.

**Promotion has to happen at the pump as well as at the prune**, which the
existing `test_a_launch_after_a_lost_attach_is_still_a_launch` caught when it
did not: an attach completes at its wait, and nothing lists the session again
until the next opener prunes, by which time a `.detach` may have taken the
process -- leaving a pid that joined and left still marked `Deferred` and kept
for ever. Every stop lists the session, so `record_where_it_stopped` promotes
there too.

Two new tests, and three mutations checked: the prune's old rule, promotion
removed, and the round-3 finding. The second is caught by the pre-existing test
above rather than by anything new, which is the whole reason it was worth
running.

Re-verified: 176 tests, 4 doctests, the full Miri suite (141), the three
`#[ignore]`d interrupt tests, `deferred_arrival` unmoved (arm A 0 short in 40,
arm F 3.7us, arm H 3.9us, arm G 0/40), `session_fuzz` seeds 1 and 7, and
windbg-mcp against a local path dependency (644 unit + 99 smoke).

Refs #136

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Both right again. 984049a.

presence not excluding another open's claim. This is my own stage-3 claim surviving in the predicate's other caller: wants excluded inherited claims and pending attaches, and deliver excluded a claimed process with an early return of its own — so the listing half never applied it. The rule moves into wants, where both callers get it, and deliver's early return goes because it now says the same thing twice.

The prune taking a deferred attachment. Correct, pre-existing, and the more expensive of the two. AttachProcess joins at the next WaitForEvent — measured, and printed by deferred_arrival arm E — so between attach_process_begin and that wait the pid is recorded and the session does not list it, and an opener pruning in that window drops the record on a rule whose whole subject is a pid that no longer names anything. As you say, moving the record into shared state does not bypass it; and the cost is worse than a missed exclusion, because the teardown then treats somebody else's process as one this engine launched and takes it.

An attachment now carries whether it has been seen (Attachment::Deferred / Joined): a pid the session holds is promoted, and a pid it does not hold has either left (dropped) or not yet joined (kept — it cannot have left a session it was never in).

One thing that fell out and is worth flagging, because it is the reason the mutation pass was worth running: promotion has to happen at the pump as well as at the prune. An attach completes at its wait, and nothing lists the session again until the next opener prunes — by which time a .detach may have taken the process, leaving a pid that joined and left still marked Deferred and kept for ever. The existing test_a_launch_after_a_lost_attach_is_still_a_launch caught that when my first version did not. Every stop lists the session, so record_where_it_stopped promotes there too.

Two new tests; three mutations checked (the prune's old rule, promotion removed, and the presence finding), each caught by the assertion written for it.

Re-verified: 176 tests, 4 doctests, full Miri (141), the three #[ignore]d interrupt tests, deferred_arrival unmoved (arm A 0 short in 40, arm F 3.7 µs, arm H 3.9 µs, arm G 0/40), session_fuzz seeds 1 and 7, and windbg-mcp against a local path dependency (644 unit + 99 smoke).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 984049a957

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +5314 to +5315
.retain(|pid, state| {
*state == Attachment::Deferred || held.iter().any(|(_, held)| held == pid)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire deferred attachment records after failed joins

When AttachProcess is accepted but the target exits before the first WaitForEvent (so the deferred attach never joins and the wait fails), this predicate retains its PID forever because only a successful session listing can promote it to Joined. If Windows later reuses that PID for a process launched through this same engine, Pending::wants excludes the launch's stop as attached and end_session detaches the launched process instead of terminating it. Track when the deferred request has been processed unsuccessfully and remove its attachment record then, while continuing to preserve requests that genuinely have not been pumped yet.

Useful? React with 👍 / 👎.

…r ever

Third round on this PR, and it is about the state the second round added.
`Attachment::Deferred` is kept precisely because a deferred attach has not
arrived and so cannot have left -- but only a successful listing promotes one,
so an `AttachProcess` the engine accepted for a process that then exited before
the first `WaitForEvent` left a pid recorded for the life of the session. If
Windows handed that number to a process this engine went on to launch, the
launch's own stop was excluded from delivery as somebody else's and its teardown
*detached* from it instead of taking it: a launched process outliving its
session.

A widening of an existing residue rather than a new one -- before an attachment
could be deferred, the next opener's prune dropped any pid the session did not
hold -- and the bound is restored by the only party that can say the attach
cannot join: a live open that pumped its whole `LIVE_WAIT_MS` and never saw its
process retires the record. Deliberately not on the *interrupted* branch beside
it, which says nothing about whether the attach is still coming, and only for
`Deferred`, because an attachment that joined and has merely left is the prune's
business.

The residue left is an attach that never joins whose guard is abandoned without
waiting: nothing then waits the bound, so nothing reaches the retirement. That
is the documented hand-off, and the pid is held against a reuse this crate
already lives with -- `prune_processes_that_left` names a retained handle as the
real answer and declines it.

Two tests. The rule is asserted directly, because reaching it through a real
open costs thirty seconds; the end-to-end half is `#[ignore]`d and was run by
hand (`30.55s`, passing), which is what says the timeout branch is actually
wired to it. Both mutations caught -- the branch not retiring, and retirement
taking a `Joined` record.

Its first draft failed for a reason worth keeping: with no debuggee, the wait
fails `E_UNEXPECTED` immediately rather than pumping, so the open never reached
its bound. It launches a target first now.

Re-verified: 177 tests, 4 doctests, the full Miri suite (141),
`deferred_arrival` unmoved (arm A 0 short in 40, arm F 3.3us, arm H 5.2us, arm
G 0/40), `session_fuzz` seeds 1 and 7.

Refs #136

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Right, and it is about the state the last round added. 9fcf9a3.

Attachment::Deferred is kept precisely because a deferred attach has not arrived and so cannot have left — but only a successful listing promotes one, so an AttachProcess the engine accepted for a process that then exited before the first WaitForEvent left a pid recorded for the life of the session. A widening of an existing residue rather than a new one: before an attachment could be deferred, the next opener's prune dropped any pid the session did not hold.

The bound is restored by the only party that can say the attach cannot join: a live open that pumped its whole LIVE_WAIT_MS and never saw its process retires the record. Deliberately not on the interrupted branch beside it — an open the host cut short says nothing about whether the attach is still coming — and only for Deferred, since an attachment that joined and has merely left is the prune's business.

What is left is an attach that never joins whose guard is abandoned without waiting: nothing then waits the bound, so nothing reaches the retirement. That is the documented hand-off (PendingTarget says a dropped guard's target materialises at the next wait from any source), and the pid is held against a reuse this crate already lives with — prune_processes_that_left names a retained handle as the real answer and declines it. Written down at the function rather than left implied.

Two tests: the rule directly, because reaching it through a real open costs thirty seconds, and an #[ignore]d end-to-end half that I ran by hand — 30.55s, passing — which is what says the timeout branch is actually wired to it. Both mutations caught.

Its first draft failed for a reason worth keeping: with no debuggee the wait fails E_UNEXPECTED immediately rather than pumping, so the open never reached its bound. It launches a target first now.

Re-verified: 177 tests, 4 doctests, full Miri (141), deferred_arrival unmoved (arm A 0 short in 40, arm F 3.3 µs, arm H 5.2 µs, arm G 0/40), session_fuzz seeds 1 and 7.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9fcf9a306f

ℹ️ 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".

Comment thread src/dbgeng.rs
#[derive(Debug, Default)]
struct ClientState {
/// The opens waiting for a target to join this session and stop.
arrivals: Mutex<Arrivals>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear shared arrivals when the owner drops

When an owning wrapper is dropped while another wrapper around the same client still holds a pending guard, DebugEngine::drop ends the session but never calls forget_all, so this newly shared register survives through the other wrapper. If that wrapper reuses the client for a new launch, the canceled old registration remains first and can claim the new process's stop, leaving the new guard to time out even though its target stopped. Clear the shared arrivals after EndSession succeeds in the owning drop path, as end_session already does.

Useful? React with 👍 / 👎.

Comment thread src/dbgeng.rs
Comment on lines +527 to +529
if self.inherited.contains(&entry)
|| others.iter().any(|other| other.arrived == Some(entry))
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire claims when their process leaves

When an outside pump completes a launch while its guard remains held, then that process is removed with raw .detach and the same PID is attached again, DbgEng can reuse the same engine ID, producing the identical pair here. The old guard's arrived value then makes the new attach reject its own stop as another open's claim, so its wait() eventually times out despite the reattached target having stopped. The previous stopped_on pruning explicitly handled this ordinary detach/reattach reuse; retained claims likewise need to stop excluding a later process instance once the claimed process leaves.

Useful? React with 👍 / 👎.

…process

Fourth round, and both halves are the register being *shared* where it used to
be a field on each wrapper -- the change this PR makes, meeting two places that
had never had to care.

**`Drop` never forgot the pending opens.** It tears the session down inline
rather than calling `end_session`, so it did not inherit that line. With a
per-wrapper register that cost nothing; with a shared one, a guard held by a
second wrapper outlives the owner's drop, and an entry registered against a
session that has ended is first in line for the next launch's stop through that
wrapper -- leaving the new guard to time out on a target that stopped. Both
teardowns now call one `forget_pending_opens`, gated on `EndSession`'s own
outcome as `end_session` already was.

**A claim outlived the process it named.** Engine ids are handed back
immediately, so a `.detach` and a reattach of the same pid reproduce a
`(engine id, pid)` pair exactly -- the reuse the record this replaced was pruned
for, arriving from the opposite side. A stale entry there made a new open read
`Arrived` for a target that had not stopped; a stale claim makes it read the
opposite, refusing the reattach its own stop until its bound runs out.
`Arrivals::forget_departed`, called from the same opener prune that handles the
attachment record, clears both the claim and any inherited exclusion naming a
process the session no longer holds. Clearing `arrived` and not merely the
exclusion is what the old prune did, and is the honest answer: an open whose
target has left has not arrived anywhere its caller can use.

Three tests. Two pin the rules; the third is the **wiring**, and it earns its
place -- with only the rule tests, removing the `forget_departed` call from the
prune leaves everything green, which I measured before writing it. It asserts
the pair actually comes back, or a run where DbgEng handed out a different
engine id would pass without meeting the collision. All four mutations caught.

Re-verified: 180 tests, 4 doctests, the full Miri suite (142), the `#[ignore]`d
30s retirement test (31.18s, passing), `deferred_arrival` unmoved (arm A 0 short
in 40, arm F 3.5us, arm H 5.8us, arm G 0/40), `session_fuzz` seeds 1 and 7.

Refs #136

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Both right, and both are this PR's own doing: the register is shared where it used to be a field on each wrapper, and these are two places that never had to care. c9486dc.

Drop never forgot the pending opens. It tears the session down inline rather than calling end_session, so it did not inherit that line — free while each wrapper had its own register, a stale entry now. Both teardowns call one forget_pending_opens, gated on EndSession's own outcome as end_session already was.

A claim outlived the process it named. Arrivals::forget_departed, called from the same opener prune that handles the attachment record, clears the claim and any inherited exclusion naming a process the session no longer holds. Worth noting the symmetry you're pointing at: this is the reuse the old record was pruned for, arriving from the opposite side — a stale entry there made a new open read Arrived for a target that had not stopped, where a stale claim makes it read Absent for one that had. Clearing arrived and not merely the exclusion is what the old prune did, and is the honest answer: an open whose target has left has not arrived anywhere its caller can use.

Three tests, and the third earns its place. Two pin the rules. With only those, removing the forget_departed call from the prune leaves everything green — I measured that before writing the third, which drives the whole path (launch delivered by an outside pump with its guard held, .detach, reattach) and asserts the pair actually comes back, or a run where DbgEng handed out a different engine id would pass without meeting the collision at all.

All four mutations caught. Re-verified: 180 tests, 4 doctests, full Miri (142), the #[ignore]d 30s retirement test (31.18s, passing), deferred_arrival unmoved (arm A 0 short in 40, arm F 3.5 µs, arm H 5.8 µs, arm G 0/40), session_fuzz seeds 1 and 7.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c9486dc8da

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +3027 to +3029
if waited {
self.retire_deferred_attachment(registered);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire deferred attachments when their wait fails

When AttachProcess is accepted but the target exits before its first WaitForEvent, pump returns an error through the ? at the bottom of this loop, so this new cleanup is never reached and the PID remains Deferred indefinitely. The fresh evidence that the existing review issue remains is that cleanup was added only to the timeout branch; a later launch reusing that PID is therefore still excluded from arrival delivery and detached rather than terminated during teardown. Retire the deferred record when a failed pump confirms that the attachment did not join, while preserving genuinely pending or interrupted attaches.

Useful? React with 👍 / 👎.

… until delivered

Two things: a fifth review round, and a real failure of my own test on the
coverage runner.

**The retirement covered the bound and not the ending the scenario takes.**
When the target exits before its first `WaitForEvent` the session holds nothing,
so the pump *fails* rather than expiring
(`test_a_wait_with_no_debuggee_fails_rather_than_expiring`) and the open returns
through its `?` without ever reaching the bound -- leaving the pid `Deferred`
for the life of the session, which is what the last round was supposed to close.
The error ending retires too.

**On a narrower condition, and the difference is evidence rather than taste.**
At the bound an open has pumped for `LIVE_WAIT_MS`, so a pid still not listed is
one that is not coming. On a failed pump it may have pumped nothing at all, and
the same reading would retire an attach the engine had not yet had a chance to
process -- after which the teardown takes somebody else's process, which is the
direction that costs. So the error ending asks `holds_nothing_at_all`, which is
the one reading that *proves* the attach produced no target.

That predicate is named and pinned both ways, because the branch reading it can
only be built one way: a pump that fails with a *live* session is not something
this bench can arrange, and the doc comment says so rather than implying the
half is covered.

**And the wiring test I added last round failed on the coverage runner**, for
the reason this whole seam exists: one `WaitForEvent` is one *event*. That
session holds an attached process whose own events compete, so a single pump
delivered the launch on this bench and did not on the instrumented runner --
dbgscope#128 in miniature, met while testing the fix for it. It pumps until
delivered now, bounded, and says why.

182 tests, 4 doctests, the full Miri suite (142), `deferred_arrival` unmoved
(arm A 0 short in 40), `session_fuzz` seeds 1 and 7.

Refs #136, #128

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Right — the retirement covered the bound and not the ending the scenario actually takes. 74fa416.

When the target exits before its first WaitForEvent the session holds nothing, so the pump fails rather than expiring (test_a_wait_with_no_debuggee_fails_rather_than_expiring) and the open returns through its ? without reaching the bound. The error ending retires too now.

On a narrower condition, and the difference is evidence rather than taste, which is your "while preserving genuinely pending or interrupted attaches": at the bound an open has pumped for LIVE_WAIT_MS, so a pid still not listed is not coming; on a failed pump it may have pumped nothing at all, and the same reading would retire an attach the engine had not yet had a chance to process — after which the teardown takes somebody else's process, which is the direction that costs. So the error ending asks holds_nothing_at_all, the one reading that proves the attach produced no target.

That predicate is named and pinned both ways, because the branch reading it can only be built one way: a pump that fails with a live session is not something this bench can arrange, and the doc comment says so rather than implying the half is covered. If you can see a construction for it, I'll take it.

Separately, and worth reporting because it is not from review: the wiring test I added last round failed on the coverage runner, for the reason this whole seam exists. One WaitForEvent is one event, that session holds an attached process whose own events compete, and a single pump delivered the launch on this bench and did not on the instrumented one — dbgscope#128 in miniature, met while testing the fix for it. It pumps until delivered now, bounded, and says why.

182 tests, 4 doctests, full Miri (142), deferred_arrival unmoved (arm A 0 short in 40), session_fuzz seeds 1 and 7.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74fa416b7e

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +591 to +594
if let Some(entry) = claimed {
for pending in &mut self.pending {
pending.inherited.push(entry);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Forward inherited claims when forgetting an open

With three overlapping launches, if the first receives process A and is dropped, the second and third inherit A; if the second then receives process B and is dropped, this loop forwards only B, while removing the sole copy of its inherited A claim. A subsequent stop from process A can therefore be delivered to the third launch, making its wait() succeed before its own process stops. Fresh evidence beyond the existing two-launch review thread is this non-transitive handoff: propagate both the completed entry's direct claim and its inherited claims.

Useful? React with 👍 / 👎.

Comment thread src/dbgeng.rs Outdated
Comment on lines +616 to +617
if pending.arrived.is_some_and(|entry| !still_here(&entry)) {
pending.arrived = None;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retire completed registrations when their process departs

When an outside pump completes an attach while its guard remains held, and that process is detached before the same PID is attached again, this clears the old guard's arrived value but leaves its older Arrival::Attached(pid) registration eligible for delivery. The reattached process's stop is consequently assigned to the old guard first, and the new attach guard times out despite its target having stopped. The fresh regression after retiring the stale claim is that the completed registration itself is reactivated; tombstone or remove it instead of resetting it to a pending open.

Useful? React with 👍 / 👎.

…eaving

`Arrivals::forget_departed` stops an open reporting a process that has left
the session as arrived. Written as clearing the field it also made the entry
*pending again* -- and nothing distinguishes a registration that has been
given nothing from one whose process has gone.

An `Arrival::Attached` open wants any stop bearing its pid, and the finished
one is registered before the reattach's, so `deliver` -- which offers in
registration order -- handed it the reattached process and the new guard
waited out its whole bound on a target sitting in front of it.

So what an open has been given is a `Claim` rather than an `Option`:
`Waiting`, `Arrived(entry)`, or `Departed`. `Departed` is terminal --
`deliver` skips it, `presence` answers `Absent`, and `forget` hands it to
nobody, since `forget_departed` has already taken that process out of every
inheritance and putting it back is the stale claim it exists to remove.

Terminal is right rather than convenient: a `.detach` and a reattach are a
*new* debug attachment, so the reattach's initial break does not satisfy the
postcondition the first guard's `wait()` promised. There is nothing left for
that open to be waiting for.

`test_a_departed_claim_does_not_reopen_the_open_that_held_it` is in two
phases because the two rules fail differently. Alone, with the pair coming
back through the raw hatch, is what pins `presence`: with a second open in
the register the pair is spoken for and the answer is `Absent` either way,
so `Listed` -- keep waiting, your target is here -- can only be asked where
there is nobody else. Two attaches is what pins `deliver`, and is why this
went unnoticed: `test_a_claim_stops_excluding_once_its_process_leaves`
stages the same sequence with a *launch* held open, which an unrelated rule
(a pending attach keeps its process from a pending launch) protects either
way.

Also pins the claim handoff, which review read as a chain and which is a
broadcast: `forget` pushes onto every open still pending, so a third launch
already holds its own copy and there is no sole copy to lose.
`test_a_claim_is_inherited_by_every_open_still_waiting` puts it to three,
because with two "every remaining open" and "the next one" are the same set
and a chained handoff passes everything else here. Its last two steps assert
the exclusion is targeted -- a genuinely new process still reaches the open
-- or a register that refused everything would pass.

Mutation-verified, one at a time: the tombstone back to a clear, `presence`
falling through on a departed claim, and `forget` handing along a chain.
Each fails exactly one test and nothing else.

And rewraps 24 assertion messages whose `\` line-continuation had been eaten
when they were written, leaving runs of interior spaces in the value. Text
only, and text a human reads at the moment they can least guess what was
meant. Four predate this branch.

Raised in review on #139. Refs #136, #133

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Round six. One of the two is real and is fixed; the other names a failure the code does not have, but it named a property nothing was pinning, so it is pinned now.

Retire completed registrations when their process departs — correct, fixed

Right about the fact and right about the remedy, which is the less common pairing. forget_departed
cleared the field back to nothing, and nothing is what a registration starts life as, so an open
that had finished became a pending open again. An Arrival::Attached open wants any stop bearing
its pid and is registered before the reattach's, and deliver offers in registration order — so the
finished one took the reattached process and the new guard waited out its bound on a target sitting
in front of it.

The state is a value now rather than an Option:

enum Claim {
    Waiting,
    Arrived((u32, u32)),
    Departed,
}

Departed is terminal: deliver skips it, presence answers Absent for it, and forget hands
it to nobody — forget_departed has already taken that process out of every inheritance, and
putting it back is the stale claim that method exists to remove.

Terminal is the right answer rather than a convenient one: a .detach and a reattach are a new
debug attachment, so the reattach's initial break does not satisfy the postcondition the first
guard's wait() promised. There is nothing left for that open to be waiting for.

test_a_departed_claim_does_not_reopen_the_open_that_held_it, in two phases, because the two rules
fail differently and each needs a construction the other cannot reach:

  • Alone, with the pair coming back through the raw hatch and no second open in the register.
    That is what pins presence's answer: with a second open present the pair is spoken for and the
    reply is Absent whichever way the arm goes, so Listedkeep waiting, your target is here
    can only be asked where there is nobody else.
  • Two attaches, which is what pins deliver. The existing
    test_a_claim_stops_excluding_once_its_process_leaves stages the same sequence and passes either
    way, because its held open is a launch and a launch is kept off a reattached process by an
    unrelated rule (a pending attach keeps its process from a pending launch). Two attaches have no
    such rule between them. That is why this went unnoticed.

Mutation-verified both ways — Departed back to Waiting, and presence falling through instead
of answering Absent. Each fails this test and nothing else.

Forward inherited claims when forgetting an open — not reachable, and now pinned

The scenario needs the handoff to be a chain. It is a broadcast: forget pushes the departing
open's claim onto every open still pending, not onto the next one. So in the three-launch
sequence, L3 is given its own copy of A when L1 is forgotten, and still holds it after L2 is
forgotten — there is no sole copy to remove.

register L1, L2, L3
deliver A            -> L1.claim = Arrived(A)
forget L1            -> L2.inherited = [A]   L3.inherited = [A]      <- both, not just the next
deliver B            -> L2.claim = Arrived(B)
forget L2            -> L3.inherited = [A, B]

And an open registered after that point needs no inheritance at all: A is in the session by then,
so it is in that open's own snapshot and Launched(Some(before)) excludes it on !before.contains.

What the finding was right about is that nothing said so. With two opens "every remaining open" and
"the next one" are the same set, so every test here passed on either reading, and a chained handoff
— the proposed fix — would have been indistinguishable. test_a_claim_is_inherited_by_every_open_still_waiting
puts it to three, and I mutation-checked it by writing the chain: it fails that test alone, with the
rest of the suite green. Its last two steps assert the exclusion is targeted — a genuinely new
process still reaches the third launch — or a register that refused everything would pass.

Also in this commit

24 assertion messages had runs of interior spaces where a \ line-continuation had been eaten
writing them. Value-only, in test failure text — but it is text a human reads at the moment they are
least able to guess what was meant. Rewrapped to the file's existing style; four of them predate
this branch and are fixed here too rather than left for a PR that would exist for nothing else.

Verification at this commit

  • cargo nextest run 186, cargo test --doc 4, fmt and clippy clean.
  • Miri, full suite: 144 passed (both new tests are pure bookkeeping, so they run under it).
  • deferred_arrival: arm A 0 short in 40 under 24 spinners, both orderings; F 1.2 µs, H 2.4 µs,
    G 0/40 — unmoved. session_fuzz: clean over seeds 1, 2, 7 and 13.
  • The #[ignore]d interrupt and retirement tests: 14 passed, including the 30 s one.
  • Downstream re-run rather than assumed: windbg-mcp against a local path dependency, 644 unit +
    99 smoke
    green with the dump tier on. No public API change — Claim and Pending are private —
    so its repoint is still the pin alone.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9073385c62

ℹ️ 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".

Comment thread src/dbgeng.rs Outdated
Comment on lines +715 to +716
match pending.claim {
Claim::Arrived(_) => return Presence::Arrived,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate arrived claims against the live process list

When an outside pump delivers a target while its guard remains held, and that process then exits or is detached while another process keeps the session alive, this branch returns Arrived without checking whether the claimed pair is still in held. Calling the guard's wait() afterward therefore returns Ok(()) for a target that is no longer present; the previous implementation first located the target in the current session. Mark a missing claim as departed or otherwise return Absent during this presence check.

Useful? React with 👍 / 👎.

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown

(retracted - posted in error, please ignore)

…its own

`presence` answered `Arrived` from the claim alone, so an open delivered a
process that has since left reported it as arrived until something pruned.
`Arrivals::forget_departed` is called from the openers, so a session that
opens nothing more never calls it: the claim outlives the process by an
unbounded amount, and the guard's `wait()` returns `Ok(())` for a target the
caller cannot read.

This branch's own regression rather than a pre-existing gap, and the old
shape could not have it: `presence_of` located the process in `held` first
and asked `stopped_on` second, so `Arrived` could not outlive the target.
Replacing the set with a register dropped the ordering without noticing it
had a second job.

`Claim::Departed` is not made redundant by this and stays. The two rules
answer different questions -- this one is what `presence` *reports*, the
tombstone is what `deliver` *offers* -- and an open cleared back to waiting
takes the next arrival bearing its pid however carefully `presence` reads.
Backing either out alone was tried: each fails its own test and only its
own.

The answer is `Absent` rather than a fourth `Presence`, which costs the open
the rest of its bound because the loop reads `Absent` as "keep pumping".
That is what every release has done. "Arrived and then left" is a genuinely
different state, but nothing above here has a use for the distinction, and
inventing one to carry it would be wider than a regression fix.

`test_an_arrived_claim_is_read_against_what_the_session_holds` keeps a
second process in `held` deliberately: `presence_of` refuses an engine
holding nothing at all before the register is asked, so a session kept alive
by somebody else is the only way the question reaches here.
Mutation-verified -- believing the claim again fails that test and nothing
else in the suite, which is why this got through five rounds.

Raised in review on #139. Refs #136, #133

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Round seven. Correct, and it is this branch's regression rather than a pre-existing gap — fixed.

Validate arrived claims against the live process list

The fact checks out against the old code, which is the part worth stating rather than taking on
trust. presence_of on main locates the process in the session first and asks whether it had
stopped second:

let ours = match arrival { Arrival::Attached(pid) => held.iter().find(|(_, held)| held == pid),};
let Some(entry) = ours else { return Presence::Absent };
if self.stopped_on.lock().contains(entry) { Presence::Arrived } else { Presence::Listed }

So Arrived could not outlive the target there. Answering from the claim alone let it, and I did
not notice the ordering had a second job when I replaced the set with a register.

What makes it more than theoretical is when the prune runs: forget_departed is called from the
openers, so a session that opens nothing more never calls it. The claim then outlives the process by
an unbounded amount, and a guard's wait() returns Ok(()) for a process the caller cannot read.

The claim is read against held now:

Claim::Arrived(entry) if held.contains(&entry) => return Presence::Arrived,
// Delivered, and gone since — whether or not a prune has caught up with it.
Claim::Arrived(_) | Claim::Departed => return Presence::Absent,

Two notes on the shape, since the alternative was tempting.

Claim::Departed stays, and is not made redundant by this: the two rules answer different
questions. This one is about what presence reports; the tombstone is about what deliver
offers, and without it an open cleared back to waiting takes the next arrival bearing its pid
however carefully presence reads. Backing either out alone leaves a hole the other does not cover
— checked by doing it.

The answer is Absent rather than a fourth Presence. It costs the open the rest of its bound,
because the loop reads Absent as "keep pumping", and that is what every release has done. "It
arrived and then left" is a genuinely different state, but nothing above here has a use for the
distinction yet, and inventing one to carry it would be a wider change than a regression fix.

test_an_arrived_claim_is_read_against_what_the_session_holds pins it, with the second process in
held for a reason: presence_of refuses an engine holding nothing at all before the register is
asked, so a session kept alive by somebody else is the only way the question reaches here.
Mutation-verified — believing the claim again fails that test and nothing else in the suite,
which is why this got through five rounds.

A correction to my last comment

I wrote cargo nextest run 186 there. It was 184 at 9073385 — I carried a number instead
of re-deriving it, which is the one thing this repo's own notes say to distrust. 185 at this commit
with the test above. The rest of that comment's figures were read off their runs and stand.

Verification at this commit

  • cargo nextest run 185, cargo test --doc 4, fmt and clippy clean.
  • Miri, full suite: 145 passed.
  • deferred_arrival: arm A 0 short in 40 under 24 spinners, both orderings; F 1.2 us, H 2.1 us, G 0/40 -- unmoved. session_fuzz: clean over seeds 1, 2, 7 and 13.
  • No public API change; Claim and presence are private.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c80d60f1c2

ℹ️ 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".

Comment thread src/dbgeng.rs
.iter()
.find(|pending| pending.id == id)
.and_then(|pending| pending.claim.held());
self.pending.retain(|pending| pending.id != id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve abandoned launch registrations until delivery

When a launch_process_begin guard is dropped before any pump, CreateProcessWide remains deferred as documented, but this removes its still-Waiting entry without leaving any exclusion. If another launch begins before the next WaitForEvent, its snapshot cannot contain the first process; the next pump can therefore materialize the first queued launch and deliver its stop to the second guard, making that guard return Ok before its own process reaches its initial break. Keep an abandoned launch registered until its deferred arrival is consumed or the session ends.

Useful? React with 👍 / 👎.

Comment thread src/dbgeng.rs Outdated
Comment on lines +571 to +573
&& !others
.iter()
.any(|other| matches!(other.what, Arrival::Attached(pid) if pid == entry.1))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Ignore departed attach registrations when filtering launches

When an attach guard remains alive after its delivered process exits or is detached, pruning changes its claim to Departed but leaves its Arrival::Attached(pid) registration here. If Windows then reuses that PID for a process launched by a later open, this predicate rejects the launch's stop solely because of the departed attach; no open claims the stop and the launch guard eventually times out despite its target being stopped. Restrict this exclusion to attach registrations that are still waiting or whose process is still present.

Useful? React with 👍 / 👎.

…m it

The exclusion that keeps a pending attach's process from a pending launch
read the *registration* -- which pid the open named -- and not whether it
could still have it. So an attach whose process had left, sitting at
`Claim::Departed` with its guard still held, went on excluding that pid; and
Windows reuses pids, so a later launch that inherited the number was refused
its own stop, nobody else claimed it, and that guard timed out with its
target stopped in front of it.

Restricted to attaches still `Waiting`, which is exactly the span over which
the reason holds. One already delivered is covered by the `claim.held()`
check above -- exact where this is by pid -- and one that has departed will
never claim anything again. The clause is new in this branch, so the defect
is too.

Also narrows what `Arrival`'s doc claims. It said stage 3 "removes"
the two-pending-launches ambiguity; it removes the two cases that have a
signal to close them with -- both guards live, and delivered-then-dropped,
where `forget` hands the claim to the opens still waiting. A launch
abandoned *before* it is delivered anything has neither, and its deferred
`CreateProcessWide` still produces a process that the next launch's
elimination cannot tell from its own.

That one is #141 rather than this commit. Keeping the entry is the obvious
fix and is a trade: nothing retires an entry whose guard is gone, so an
abandoned launch that never starts -- `deferred_arrival` arm C measures a
missing image failing inside the wait -- leaves a registration that takes
the next launch's process and times that guard out instead. A wrong `Ok`
for a wrong timeout, in the shape stage 3 exists to remove. It is also
pre-existing: `Arrival`'s doc on `main` records the same ambiguity as
knowingly accepted.

Raised in review on #139. Refs #136, #133, #141

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang

glslang commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Round eight. One is this branch's own bug and is fixed; the other is real, pre-existing, and its
proposed remedy is a trade rather than a fix — so it is #141,
and what changes here is the doc comment that overclaimed.

Ignore departed attach registrations when filtering launches — correct, fixed

The exclusion reads other.what — which pid the open named — and never asked whether it could
still have it. An attach at Claim::Departed with its guard still held went on excluding that pid,
so a later launch inheriting the number was refused its own stop, nobody else claimed it, and that
guard timed out with its target stopped in front of it.

matches!(other.claim, Claim::Waiting) beside it now, which is exactly the span over which the
reason for the exclusion holds — a launch is identified by elimination, so an attach's process looks
like the launch's own only while the attach could still claim it. One already delivered is covered by
the claim.held() check above, and that check is exact where this one is by pid; one that has
departed will never claim anything again.

The clause is new in this branch, so the defect is. Mutation-verified: reinstating the broad
exclusion fails test_a_finished_attach_does_not_keep_a_launch_from_a_reused_pid and nothing else.

Preserve abandoned launch registrations until delivery — real, and not this PR

The mechanism is right. Dropping a launch guard before any pump takes a Waiting entry out with no
exclusion left, the deferred CreateProcessWide still produces a process, and the next launch's
snapshot cannot contain it. Its wait() then returns Ok(()) for a process that is not its own.

Two things about it, both checked rather than assumed.

It is pre-existing. Arrival's doc comment on main records this ambiguity as knowingly
accepted, and says what closing it would need:

The elimination is exact for opens made one after another and ambiguous for two launches
pending at once
… Telling the launches apart needs the engine to record which arrivals earlier
waits claimed — new engine-wide state, cleared everywhere a session is replaced and pruned for pid
reuse …

That record is what this PR built, and it closed the two cases that have a signal to close them
with: both guards live, and delivered-then-dropped, where forget hands the claim to the opens
still waiting. This is the third, and it has neither.

Keeping the entry is a trade, not a fix. A guard is what ends an entry's life and this one has
none, so an abandoned launch that never starts leaves a registration competing for arrivals for
the rest of the session — and that is measured rather than imagined: deferred_arrival arm C has a
launch whose image does not exist failing inside the wait (Err(0x80070002) in 13 ms, no debuggee
behind it). The ghost then takes the next launch's process and times that guard out. A wrong Ok
swapped for a wrong timeout, in exactly the shape #136 stage 3 exists to remove: an entry outliving
its reader with no signal saying when it is done.

So #141 has the analysis and three options that have not been weighed against each other, and none
is urgent — every caller in this crate and in windbg-mcp waits on the guard it is given.

What does change here is the doc. Arrival said stage 3 "removes" the ambiguity. It removes two
of the three cases, and a comment that overclaims is worse than the gap it is hiding — so it now
names the case that is left, why it is left, and where it went.

Verification at this commit

  • cargo nextest run 186, cargo test --doc 4, fmt and clippy clean.
  • Miri, full suite: 146 passed.
  • deferred_arrival: arm A 0 short in 40 under 24 spinners, both orderings; F 1.1 µs, H 2.1 µs,
    G 0/40 — unmoved. session_fuzz: clean over seeds 1, 2, 7, 13.
  • The #[ignore]d interrupt and retirement tests: 14 passed.
  • Downstream: windbg-mcp against a local path dependency, 644 unit + 99 smoke green with the
    dump tier on.

On the red tick

claude-review has failed, passed, and failed across this branch's last three heads and produced no
review in any of them — is_error: true with "No buffered inline comments", at 3m08s, 16m02s and
1m38s. It is the action aborting, not a finding. Every other check is green.

@glslang
glslang merged commit f86a57c into main Sep 4, 2026
6 checks passed
glslang added a commit that referenced this pull request Sep 4, 2026
…eaving

`Arrivals::forget_departed` stops an open reporting a process that has left
the session as arrived. Written as clearing the field it also made the entry
*pending again* -- and nothing distinguishes a registration that has been
given nothing from one whose process has gone.

An `Arrival::Attached` open wants any stop bearing its pid, and the finished
one is registered before the reattach's, so `deliver` -- which offers in
registration order -- handed it the reattached process and the new guard
waited out its whole bound on a target sitting in front of it.

So what an open has been given is a `Claim` rather than an `Option`:
`Waiting`, `Arrived(entry)`, or `Departed`. `Departed` is terminal --
`deliver` skips it, `presence` answers `Absent`, and `forget` hands it to
nobody, since `forget_departed` has already taken that process out of every
inheritance and putting it back is the stale claim it exists to remove.

Terminal is right rather than convenient: a `.detach` and a reattach are a
*new* debug attachment, so the reattach's initial break does not satisfy the
postcondition the first guard's `wait()` promised. There is nothing left for
that open to be waiting for.

`test_a_departed_claim_does_not_reopen_the_open_that_held_it` is in two
phases because the two rules fail differently. Alone, with the pair coming
back through the raw hatch, is what pins `presence`: with a second open in
the register the pair is spoken for and the answer is `Absent` either way,
so `Listed` -- keep waiting, your target is here -- can only be asked where
there is nobody else. Two attaches is what pins `deliver`, and is why this
went unnoticed: `test_a_claim_stops_excluding_once_its_process_leaves`
stages the same sequence with a *launch* held open, which an unrelated rule
(a pending attach keeps its process from a pending launch) protects either
way.

Also pins the claim handoff, which review read as a chain and which is a
broadcast: `forget` pushes onto every open still pending, so a third launch
already holds its own copy and there is no sole copy to lose.
`test_a_claim_is_inherited_by_every_open_still_waiting` puts it to three,
because with two "every remaining open" and "the next one" are the same set
and a chained handoff passes everything else here. Its last two steps assert
the exclusion is targeted -- a genuinely new process still reaches the open
-- or a register that refused everything would pass.

Mutation-verified, one at a time: the tombstone back to a clear, `presence`
falling through on a departed claim, and `forget` handing along a chain.
Each fails exactly one test and nothing else.

And rewraps 24 assertion messages whose `\` line-continuation had been eaten
when they were written, leaving runs of interior spaces in the value. Text
only, and text a human reads at the moment they can least guess what was
meant. Four predate this branch.

Raised in review on #139. Refs #136, #133

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
glslang added a commit that referenced this pull request Sep 4, 2026
…its own

`presence` answered `Arrived` from the claim alone, so an open delivered a
process that has since left reported it as arrived until something pruned.
`Arrivals::forget_departed` is called from the openers, so a session that
opens nothing more never calls it: the claim outlives the process by an
unbounded amount, and the guard's `wait()` returns `Ok(())` for a target the
caller cannot read.

This branch's own regression rather than a pre-existing gap, and the old
shape could not have it: `presence_of` located the process in `held` first
and asked `stopped_on` second, so `Arrived` could not outlive the target.
Replacing the set with a register dropped the ordering without noticing it
had a second job.

`Claim::Departed` is not made redundant by this and stays. The two rules
answer different questions -- this one is what `presence` *reports*, the
tombstone is what `deliver` *offers* -- and an open cleared back to waiting
takes the next arrival bearing its pid however carefully `presence` reads.
Backing either out alone was tried: each fails its own test and only its
own.

The answer is `Absent` rather than a fourth `Presence`, which costs the open
the rest of its bound because the loop reads `Absent` as "keep pumping".
That is what every release has done. "Arrived and then left" is a genuinely
different state, but nothing above here has a use for the distinction, and
inventing one to carry it would be wider than a regression fix.

`test_an_arrived_claim_is_read_against_what_the_session_holds` keeps a
second process in `held` deliberately: `presence_of` refuses an engine
holding nothing at all before the register is asked, so a session kept alive
by somebody else is the only way the question reaches here.
Mutation-verified -- believing the claim again fails that test and nothing
else in the suite, which is why this got through five rounds.

Raised in review on #139. Refs #136, #133

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
@glslang
glslang deleted the refactor/deliver-an-arrival-instead-of-broadcasting-it branch September 4, 2026 10:58

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 304fe116d7

ℹ️ 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".

Comment thread src/dbgeng.rs
Comment on lines +558 to +560
if self.inherited.contains(&entry)
|| others.iter().any(|other| other.claim.held() == Some(entry))
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow later attach stops for an already-claimed process

When a completed attach guard remains alive and DbgEng accepts another attach_process_begin for the same PID, both stops identify the same (engine id, PID). The first guard's claim makes this blanket check reject that pair for the second guard even when a subsequent stop was generated by the second attach, so the second wait() eventually reports LiveTargetTimeout despite its process having stopped. Apply the existing-claim exclusion to launches, while allowing a separate stop on the named PID to satisfy the next waiting attach.

Useful? React with 👍 / 👎.

glslang added a commit that referenced this pull request Sep 4, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant