diff --git a/CLAUDE.md b/CLAUDE.md index 2adadbd..bf26ee6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,7 @@ Key capabilities: - **Post-mortem**: `open_dump(path)`, `open_trace(path)`. - **Commands & events**: `execute_command`, `wait_for_event`, `execute_and_wait`, `settle`; output is captured through `OutputCallbacks` (an `IDebugOutputCallbacks` impl). - **One `WaitForEvent` is one `pump`, and it attributes its own outcome** (`WaitOutcome`, `Bound`; dbgscope#136 stage 1). Every wait in the crate goes through `DebugEngine::pump(bound)` and comes back as `Stopped { process }`, `Expired`, `Deadline` or `OnRequest` — produced once, by the call that waited, before anything can overwrite the engine's last-event slot. That is sound only because waits here are single-threaded; it is not a general property of DbgEng. What it replaced was two waits answering `Result<(), _>` (plus a bare `bool`), with the four endings reconstructed afterwards by three parties out of shared mutable state — and #136's evidence is that **15 of the 22 findings** on the #133 review were one of those reads moving. Four things follow. **Only `Stopped` records**, so the expiry and forced-break gates are unreachable rather than guarded. **A break outranks the wait's own error**, either origin's, which is `execute_and_wait`'s long-standing rule now applied to `run_to_address` and `wait_for_event` as well. **The watchdog is read before the shared flag**, because it raises that flag too. And **the flag is taken, not read** — the request belongs to the wait it ended. Two bounded operations are *not* pumps (`execute_command_bounded`'s `Execute`, `new_breakpoint`'s symbolic resolve): they have no wait to attribute anything, so they use `cut_short_by`, which is the same rule read from the only two things they have. #136 is now complete: stage 4 is the bullet below. -- **An arrival is delivered to the open waiting for it, not broadcast** (`ClientState`, `Arrivals`, `Registered`; dbgscope#136 stage 3). What it replaced, `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 again where one is ended -- and each of those three arrived as a review finding rather than as a design. Now an opener 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. Four things follow. **There is no lifecycle**: `prune_processes_that_left` is down to the attachment record, which is about the teardown decision rather than about an open, and nothing can go stale because nothing outlives its reader. **Two launches pending at once are told apart**, which `Arrival` documented as an accepted ambiguity: an arrival is *claimed*, so the second launch waits for the next one -- and the reason that fix was rejected before (it needed engine-wide state with a prune and two clears) was the cost of the record it would have joined. **The state is per client, not per wrapper**: `ClientState` is held by `Arc` and keyed by client pointer in a `Weak` map, so a pump through one `DebugEngine` completes an open held by another around the same `IDebugClient6` -- the gap the old field's doc comment described and could not close, along with the interrupt scope, which moved into the same `Arc` because it is the same field. A `Weak` map needs no `reissue_identity` equivalent: a dead entry identifies itself, where a stale identity costs only a re-read. **`attached_processes` moved with them, and that half was not planned**: review found delivery reading a second wrapper's empty set, and the same placement meant an `end_session` through a wrapper that did not perform the attach saw nothing to detach and its passive end *killed* somebody else's process. Which processes a session attached to is the session's fact; the sentence that kept it per wrapper argued sharing would put the decision "behind an eviction policy", which is true of the identity cache and not of a `Weak` map. **And a claim outlives the open that made it**, inherited by the opens still waiting -- without that the two-launch fix holds only while both guards are held, since the first target's *next* stop is claimed by nobody and absent from the second launch's snapshot. It goes when those opens go, so it is not the lifecycle coming back. And **the pending-attach exclusion is registry-local now** as well as reading `attached_processes` -- a launch is identified by elimination, so an attach's process looks like the launch's own, and the register knows about pending attaches where that per-wrapper set does not. +- **An arrival is delivered to the open waiting for it, not broadcast** (`ClientState`, `Arrivals`, `Registered`; dbgscope#136 stage 3). What it replaced, `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 again where one is ended -- and each of those three arrived as a review finding rather than as a design. Now an opener 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 -- all but an abandoned launch's, which is kept until the session goes so that it absorbs the process its still-queued create produces (dbgscope#141). Four things follow. **The only lifecycle left is the session's**: `prune_processes_that_left` is down to the attachment record, which is about the teardown decision rather than about an open, and the one entry that outlives its reader is retired by the clears at a session's end and replacement. **Two launches pending at once are no longer given the same arrival**, which `Arrival` documented as an accepted ambiguity: an arrival is *claimed*, so the second launch waits for the next one -- and the reason that fix was rejected before (it needed engine-wide state with a prune and two clears) was the cost of the record it would have joined. That makes them distinct, not attributed: one pump realises every queued create, so which launch gets which process is still a coin flip, and dbgscope#141 is where that is parked. **The state is per client, not per wrapper**: `ClientState` is held by `Arc` and keyed by client pointer in a `Weak` map, so a pump through one `DebugEngine` completes an open held by another around the same `IDebugClient6` -- the gap the old field's doc comment described and could not close, along with the interrupt scope, which moved into the same `Arc` because it is the same field. A `Weak` map needs no `reissue_identity` equivalent: a dead entry identifies itself, where a stale identity costs only a re-read. **`attached_processes` moved with them, and that half was not planned**: review found delivery reading a second wrapper's empty set, and the same placement meant an `end_session` through a wrapper that did not perform the attach saw nothing to detach and its passive end *killed* somebody else's process. Which processes a session attached to is the session's fact; the sentence that kept it per wrapper argued sharing would put the decision "behind an eviction policy", which is true of the identity cache and not of a `Weak` map. **And a claim outlives the open that made it**, inherited by the opens still waiting -- without that the two-launch fix holds only while both guards are held, since the first target's *next* stop is claimed by nobody and absent from the second launch's snapshot. It goes when those opens go, so it is not the lifecycle coming back. And **the pending-attach exclusion is registry-local now** as well as reading `attached_processes` -- a launch is identified by elimination, so an attach's process looks like the launch's own, and the register knows about pending attaches where that per-wrapper set does not. - **A break request names the operation it is for** (`BreakScope`, `Operation`, `OperationId`, `BreakRequest`; dbgscope#136 stage 2, closing #135). `interrupt_raised` was an engine-wide `AtomicBool` answering *has an interrupt been requested*, where every reader wanted *was **this** operation asked to stop*. Six operations cleared it as they opened, so a request lodged between an operation's clear and its wait was **erased while its break was still on the way** — and the synthetic Ctrl+Break that then arrived was reported as the target's own stop, up to and including being recorded as an initial break in the arrival record of the day, which is the exact misattribution the gate above exists to prevent, reached around it. Now: `DebugEngine::begin_operation` opens an operation and hands back a guard, `InterruptHandle::interrupt` files a request against whatever is running **under the same lock it delivers `SetInterrupt` on**, and the guard discards an unread request on drop. Five things to know. **The lock is the fix, not the id** — a ticket or generation counter does not close it, because the window is between two writes rather than between two values, and #136 says so in as many words. **Delivery stays engine-wide and only attribution is scoped**: `SetInterrupt` cannot be aimed, so the break is issued unconditionally (that is what lets a host abort a long unbounded `execute_command`), and `BreakRequest::NothingRunning` is the honest answer when no bounded operation is running. **Operations nest** — `wait_for_kernel_break_in` holds one across `absorb_initial_break_artifact`, which runs a whole `execute_and_wait` — so `running` is a stack and `asked` a set; a single slot answers most tests and fails the nested one, and a `bool` cannot express it at all. **The watchdog files nothing**, going through `break_in_only`, which is what deletes the `by_watchdog | flag` OR from five sites. And **the residue is named rather than closed**, in two shapes of one fact: a break aimed at operation N can land on N+1, and a request can be filed against N after N's last read of one, because an operation accepts requests for slightly longer than it reads them (it cannot stop accepting at the read -- `wait_for_live_target` pumps repeatedly inside one operation and the next pump legitimately reads). Neither is reportable, and `BreakRequest::Raised` deliberately does not promise otherwise. The second gets a **drain**: `Operation::drop` consumes the engine's own pending break when it discards a record nobody read, so it cannot become the first shape. Bookkeeping cannot reach the rest; `examples/interrupt_provenance.rs` is the measurement #136 asked for first, and it says a post-wait `GetInterrupt` is a *forward* signal — a request that ended a wait is consumed before the wait returns (`[false; 5]`), one that did not is still readable (`[true, false, …]`), and two back to back are one flag rather than two. That is stage 3's to use. - **Nothing runs on an engine with no debuggee, and that is a safety property rather than a policy.** Driving DbgEng with no target faults *inside* it — a `STATUS_ACCESS_VIOLATION`, which is a structured exception `catch_unwind` cannot trap, so it takes the host process down instead of failing the call. Measured twice on dbgeng 10.0.26100.1: on an engine whose debuggee had just exited, and on a **fresh** one that never had a target, which is what says the trigger is the missing debuggee rather than the departure. So `refuse_without_a_debuggee` guards every road in — `execute_command`, `execute_command_bounded`, `execute_and_wait`, `run_to_address` — and it cannot be narrowed to text that looks like execution control, because an alias, a `.if` branch and `dx …ExecuteCommand("g")` all reach execution without saying so. The one exception is `execute_fixed_command`, for this crate's own literals: `sxe ibp` arms the initial break *before* a target exists, so guarding it would refuse every `launch_process` and `attach_process` on the machine. Nothing a caller supplied may go through it. - **A target that ends is not a failure.** A debuggee running to completion during a wait leaves `WaitForEvent` answering `E_UNEXPECTED` ("Catastrophic failure", which names nothing) and `GetExecutionStatus` reading `DEBUG_STATUS_NO_DEBUGGEE` — with `GetNumberProcesses`, `GetCurrentProcessSystemId` and `GetExitCode` all failing beside it and `.lastevent` answering ``, so the status is the only one of them that says anything. `execute_and_wait` and `settle` report it as `CommandRun::target_gone` and `run_to_address` as `RunToOutcome::TargetGone`, each **keeping the output the run captured**: the pump is where the module loads, the breakpoint banner and an embedded script's prints arrive, and on the run that ends the target there is no successor to print them again. @@ -70,7 +70,7 @@ Key capabilities: - **Scope**: `scope()` / `set_scope()` and the `ScopeGuard<'a>` RAII type, over `IDebugSymbols3::GetScope`/`SetScope` — for running a command that moves the debugger's scope (measured: `!analyze -v` leaves it at the target's default, discarding a frame or `.ecxr` context the caller had selected) without the session ending up somewhere else. `GetScope` does **not** report the size of the context blob it wants: it rejects a buffer smaller than the target's `CONTEXT` and accepts any larger one, so the ask walks `SCOPE_CONTEXT_SIZES` upward. A scope carries the target identity it was read from and is refused rather than applied to a later target. `examples/scope_restore.rs` re-validates all of it against a dump. - **Ending a session is three different things, and the choice is per *process*, not per session.** `end_session` (and `Drop`, which mirrors it) ends passively — which for a process this engine *launched* means the kernel takes it, since a passive end destroys the debug port and `DebugSetProcessKillOnExit` defaults to true. Two exceptions. A **live kernel** is resumed and actively detached, or it stays frozen at its last break with one CPU halted and the rest spinning. And every user-mode process this engine **attached** to is detached individually (`DetachCurrentProcess`) *before* the session ends, or the same passive end kills it — somebody else's service, taken down by a debugger that was only looking. **Per process because DbgEng holds several user-mode targets at once** (`|` lists them, and says `attach` or `create` against each): `EndSession` takes one flag for the whole session, so no choice of flag can both keep an attached process and take a launched one. Provenance is a set of pids the openers record (`claim_attached`), because the API cannot be asked — `GetDebuggeeType` answers `DEBUG_CLASS_USER_WINDOWS` / `DEBUG_USER_WINDOWS_PROCESS` for a launch and an attach alike, and `|` knows but that is text. The walk is by engine id from `GetProcessIdsByIndex`, so a recorded pid the session no longer holds simply does not match and needs no separate staleness check; `session_processes` answers empty with no debuggee, because `GetNumberProcesses` fails `E_UNEXPECTED` there and would turn "the program had already finished" into a failed teardown. Failures are per process and **reported without stopping the teardown**, because this is what a client's disconnect runs: a session that will not close is worse than a debuggee that was killed, and a caller told "released" would never go and look. `bc *` first, or an `int3` this engine patched in stays patched in a process that goes on running. **What the teardown lets go of is gated on `EndSession`'s own outcome, not on the value `end_session` returns**, which differ exactly when a detach fails: the caller is told about that failure, but a process this engine could not detach from is one left attached and running — it does not keep the session alive, and holding the deferred buffers and the session's pending opens back on a session that has definitely gone is a leak in the first case and a guard left waiting on nothing in the second. - **Opening a target is two steps, not one**: each of the four openers above is a thin `x_begin()? .wait()`. `x_begin` performs only the side effect that creates or claims the target and hands back a `PendingTarget` guard; `PendingTarget::wait` completes the initial break. The split exists so a caller can tell "nothing happened, retry is clean" from "the target exists and only the wait failed" — opposite recoveries, since re-running the second spawns a second process, attaches twice, or re-dials a live KD link. **Deferred input buffers belong to the engine, not the guard** (`DebugEngine::deferred_inputs`). `CreateProcessWide` defers the spawn and reads the command line at the *next* `WaitForEvent` — whichever call makes it — and a kernel dial behaves the same way, so a buffer owned by `PendingTarget` would be a use-after-free the moment the guard was dropped without waiting, which `#[must_use]` cannot prevent. Parking them on the engine instead makes dropping a guard safe and **non-blocking**: it forfeits the initial-break wait but cancels nothing, and the target still materializes at the next wait. Driving the wait from `Drop` instead was tried and rejected — on a kernel attach whose link is still coming up it can block with *no* bound, since `SetInterrupt` cannot cancel that wait. The buffers are released only once `end_session` confirms teardown. `examples/split_open.rs` re-validates the user-mode paths, including a guard dropped before the engine is pumped. -- **A live `wait()` is a pump, not a wait** (`Arrival`, `DebugEngine::wait_for_live_target`), because **one `WaitForEvent` is one *event*, and not necessarily this target's**. The spawn a `CreateProcessWide` defers is realised inside that wait — but so is everything else the session is holding, so an engine that already has a target can return from it on *that* target's event and leave the open reporting success with its own process absent. Measured (dbgscope#128): an `AttachProcess` break-in whose injected thread is slow to be scheduled arrives a whole wait late, and the `launch_process` after it spends its only wait on that break. Six things to know before editing it. **The terminal condition is that the target has *stopped*, and it is read from a record rather than from the engine's last event** — `cpr` is an ignored filter, so a process is registered when its create event is processed and its initial breakpoint arrives later, which leaves membership too weak; but `GetLastEventInformation` is a single session-wide slot that every later event overwrites, so any rule built on reading it in the moment answers the same way for a target still coming and for one that stopped before its guard was waited on. So a stop is **delivered as a wait observes it** — in `record_where_it_stopped`, reached from the `WaitOutcome::Stopped` arm of the one `pump`, since `PendingTarget` names `execute_and_wait` and `run_to_address` as pumps that complete a held guard's target — to the open that registered for it, and `presence_of` asks that open. **A wait that came back having stopped on nothing must record nothing**, and there are three of those: an expiry, which `WaitForEvent` reports as `S_FALSE` and the generated wrapper flattens into the same `Ok` a stop gets (read the raw `HRESULT` through the vtable, as `interrupted` already does for `GetInterrupt`); a **watchdog-forced Ctrl+Break**; and a **host-requested** one through an `InterruptHandle`. The last two are the same stop — it stops whatever was running, so in a mixed session it can stop a deferred target before its initial breakpoint and hand that guard an arrival that never happened. They were three gates in front of one recorder and cost nine of #133's twenty-two findings, added one writer at a time; since #136 stage 1 they are three *arms* of `WaitOutcome` and only `Stopped` reaches the recorder, so there is no gate left for a fourth pump to forget. (Only the forced break was ever actually reaching the record: an expired wait leaves `GetLastEventInformation` reporting `DEBUG_ANY_ID` rather than the event before it — measured, round 7 of #133 — so the join found no process and the safety was incidental. `test_an_expired_wait_records_no_stop` asserts the outcome now, not that sentinel.) **Both interrupt origins and both bounds**, which took three rounds one door at a time (watchdog/bounded, host/bounded, host/finite -- the last being the bound a live open actually pumps with). A host's `InterruptHandle` and the watchdog both reach the engine through `SetInterrupt`, and until #136 stage 2 they also raised the same flag, so every classify site had to OR the two and then trust the watchdog's private one to say which it was. The watchdog now goes through `break_in_only` and files no request, so `pump` reads two independent signals and the order matters only for the genuine coincidence of both in one window. A host's request is **taken** rather than read, because it belongs to the operation it named. The precondition used to be that every operation *cleared* the flag going in -- six sites, and `run_to_address` and `wait_for_kernel_break_in` did not, which cost every later wait once the record began reading the shared one -- and that clear is gone rather than fixed: a request names an operation, so a later one cannot inherit it and there is nothing to clear. And **a departed process used to take its stop with it, and now has none to take**: the old record was keyed by `(engine id, pid)` and engine ids are reused immediately (measured -- detaching id 0 and attaching another process hands the freed 0 straight back), so a `.detach` and a reattach of the same pid brought the whole pair back and read as an arrival, which `prune_processes_that_left` guarded against at the two openers. Since #136 stage 3 an arrival is delivered to a registered open and an entry dies with its guard, so there is nothing to prune; that function is down to the attachment record, which is about the teardown decision rather than about an open. Three review rounds moved this defect around before it landed here, and each intermediate rule was locally right; the measurement that separates them is the example's arm H (29.4s and `E_UNEXPECTED` reading the slot, single-digit µs reading the record). The id in that slot is the **engine** id and not the pid — measured against `session_processes`, which answers both, and pinned by `test_the_last_event_names_its_process_by_engine_id`, because comparing the wrong one fails silently in the worst direction: nothing ever matches, and every live open pumps to its bound and reports a timeout on a target sitting in front of it. At the bound the asymmetry returns: a process visibly in the session ends the wait `Ok` and only a missing one is `LiveTargetTimeout`, because "not observed to stop" is not "never arrived" ([Unknown, not absent](docs/unknown-not-absent.md)) — which is why `Presence` is three states rather than a boolean. **A launch is identified by elimination and an attach by name** — `CreateProcessWide` hands back no pid, so a launch waits for a process the session did not hold when the launch was issued *and* that this engine did not attach to; exact for opens made one after another, and two launches pending *at once* cannot be told apart by it. **`Presence::Unknown` is the *ask* failing and nothing else** — an unreadable status or process list, or a launch with no snapshot to diff against; a session holding nothing at all is `Absent`, because `has_target` answering `Ok(false)` is knowledge rather than a question that could not be put. Round 6 read those as the same state and concluded a live open could end `Ok` with no debuggee: half right, since an expired finite wait really is `S_FALSE` flattened to `Ok` (measured, 311ms against a 300ms bound), and wrong as a composite — a wait with no debuggee *fails* rather than expiring (`E_UNEXPECTED`, 200µs on a fresh engine, 14µs after `end_session`) and the loop propagates it, so no open passes through "returned `Ok`, holds nothing". The mapping is honest anyway, and the two facts are pinned together (`test_a_wait_with_no_debuggee_fails_rather_than_expiring`, `test_a_session_holding_nothing_is_absence_rather_than_a_question`) so that an engine that starts expiring instead fails a test rather than a caller. What still needs `waited` is a launch with no snapshot in a *mixed* session, where the other target answers `has_target` on its behalf and returning on the first ask would leave the deferred spawn unrealised. **`LIVE_WAIT_MS` bounds the open, not each wait**, and **a host interrupt ends it early** (`LiveTargetInterrupted`, distinct from the timeout because a timeout says the target is not coming and this says nothing about the target): pumping through a break the host asked for cost the whole bound *and* the target, which ran to completion under the pump -- measured, 29.5s and `CommandFailed(0x8000FFFF)` with the check backed out. And **asking before the first wait is measured rather than assumed** — neither opener lists its process before the wait that completes it, so the ordinary open still waits exactly once, and what the ask buys is a guard waited *after* something else pumped its target in (29.36s and `E_UNEXPECTED` before, 8.6µs after). `examples/deferred_arrival.rs` is the measurement, including the CPU load that reproduces the race: 3 short in 40 rounds before, 0 in 40 after, 0 in 40 either way on a quiet machine. Its arm G asks the narrower question — whether the wait ended on the launch's *own* event — and answers 0 in 40, which is **not** evidence the window is unreachable: this fixture holds one competing break (the attach's injected break-in), and it is spent on the wider window before the narrower one opens. The condition is tightened on the mechanism, not on that count. +- **A live `wait()` is a pump, not a wait** (`Arrival`, `DebugEngine::wait_for_live_target`), because **one `WaitForEvent` is one *event*, and not necessarily this target's**. The spawn a `CreateProcessWide` defers is realised inside that wait — but so is everything else the session is holding, so an engine that already has a target can return from it on *that* target's event and leave the open reporting success with its own process absent. Measured (dbgscope#128): an `AttachProcess` break-in whose injected thread is slow to be scheduled arrives a whole wait late, and the `launch_process` after it spends its only wait on that break. Six things to know before editing it. **The terminal condition is that the target has *stopped*, and it is read from a record rather than from the engine's last event** — `cpr` is an ignored filter, so a process is registered when its create event is processed and its initial breakpoint arrives later, which leaves membership too weak; but `GetLastEventInformation` is a single session-wide slot that every later event overwrites, so any rule built on reading it in the moment answers the same way for a target still coming and for one that stopped before its guard was waited on. So a stop is **delivered as a wait observes it** — in `record_where_it_stopped`, reached from the `WaitOutcome::Stopped` arm of the one `pump`, since `PendingTarget` names `execute_and_wait` and `run_to_address` as pumps that complete a held guard's target — to the open that registered for it, and `presence_of` asks that open. **A wait that came back having stopped on nothing must record nothing**, and there are three of those: an expiry, which `WaitForEvent` reports as `S_FALSE` and the generated wrapper flattens into the same `Ok` a stop gets (read the raw `HRESULT` through the vtable, as `interrupted` already does for `GetInterrupt`); a **watchdog-forced Ctrl+Break**; and a **host-requested** one through an `InterruptHandle`. The last two are the same stop — it stops whatever was running, so in a mixed session it can stop a deferred target before its initial breakpoint and hand that guard an arrival that never happened. They were three gates in front of one recorder and cost nine of #133's twenty-two findings, added one writer at a time; since #136 stage 1 they are three *arms* of `WaitOutcome` and only `Stopped` reaches the recorder, so there is no gate left for a fourth pump to forget. (Only the forced break was ever actually reaching the record: an expired wait leaves `GetLastEventInformation` reporting `DEBUG_ANY_ID` rather than the event before it — measured, round 7 of #133 — so the join found no process and the safety was incidental. `test_an_expired_wait_records_no_stop` asserts the outcome now, not that sentinel.) **Both interrupt origins and both bounds**, which took three rounds one door at a time (watchdog/bounded, host/bounded, host/finite -- the last being the bound a live open actually pumps with). A host's `InterruptHandle` and the watchdog both reach the engine through `SetInterrupt`, and until #136 stage 2 they also raised the same flag, so every classify site had to OR the two and then trust the watchdog's private one to say which it was. The watchdog now goes through `break_in_only` and files no request, so `pump` reads two independent signals and the order matters only for the genuine coincidence of both in one window. A host's request is **taken** rather than read, because it belongs to the operation it named. The precondition used to be that every operation *cleared* the flag going in -- six sites, and `run_to_address` and `wait_for_kernel_break_in` did not, which cost every later wait once the record began reading the shared one -- and that clear is gone rather than fixed: a request names an operation, so a later one cannot inherit it and there is nothing to clear. And **a departed process used to take its stop with it, and now has none to take**: the old record was keyed by `(engine id, pid)` and engine ids are reused immediately (measured -- detaching id 0 and attaching another process hands the freed 0 straight back), so a `.detach` and a reattach of the same pid brought the whole pair back and read as an arrival, which `prune_processes_that_left` guarded against at the two openers. Since #136 stage 3 an arrival is delivered to a registered open and an entry dies with its guard (an abandoned launch's with its session), so there is nothing to prune; that function is down to the attachment record, which is about the teardown decision rather than about an open. Three review rounds moved this defect around before it landed here, and each intermediate rule was locally right; the measurement that separates them is the example's arm H (29.4s and `E_UNEXPECTED` reading the slot, single-digit µs reading the record). The id in that slot is the **engine** id and not the pid — measured against `session_processes`, which answers both, and pinned by `test_the_last_event_names_its_process_by_engine_id`, because comparing the wrong one fails silently in the worst direction: nothing ever matches, and every live open pumps to its bound and reports a timeout on a target sitting in front of it. At the bound the asymmetry returns: a process visibly in the session ends the wait `Ok` and only a missing one is `LiveTargetTimeout`, because "not observed to stop" is not "never arrived" ([Unknown, not absent](docs/unknown-not-absent.md)) — which is why `Presence` is three states rather than a boolean. **A launch is identified by elimination and an attach by name** — `CreateProcessWide` hands back no pid, so a launch waits for a process the session did not hold when the launch was issued *and* that this engine did not attach to; exact for opens made one after another, and two launches pending *at once* cannot be told apart by it. **`Presence::Unknown` is the *ask* failing and nothing else** — an unreadable status or process list, or a launch with no snapshot to diff against; a session holding nothing at all is `Absent`, because `has_target` answering `Ok(false)` is knowledge rather than a question that could not be put. Round 6 read those as the same state and concluded a live open could end `Ok` with no debuggee: half right, since an expired finite wait really is `S_FALSE` flattened to `Ok` (measured, 311ms against a 300ms bound), and wrong as a composite — a wait with no debuggee *fails* rather than expiring (`E_UNEXPECTED`, 200µs on a fresh engine, 14µs after `end_session`) and the loop propagates it, so no open passes through "returned `Ok`, holds nothing". The mapping is honest anyway, and the two facts are pinned together (`test_a_wait_with_no_debuggee_fails_rather_than_expiring`, `test_a_session_holding_nothing_is_absence_rather_than_a_question`) so that an engine that starts expiring instead fails a test rather than a caller. What still needs `waited` is a launch with no snapshot in a *mixed* session, where the other target answers `has_target` on its behalf and returning on the first ask would leave the deferred spawn unrealised. **`LIVE_WAIT_MS` bounds the open, not each wait**, and **a host interrupt ends it early** (`LiveTargetInterrupted`, distinct from the timeout because a timeout says the target is not coming and this says nothing about the target): pumping through a break the host asked for cost the whole bound *and* the target, which ran to completion under the pump -- measured, 29.5s and `CommandFailed(0x8000FFFF)` with the check backed out. And **asking before the first wait is measured rather than assumed** — neither opener lists its process before the wait that completes it, so the ordinary open still waits exactly once, and what the ask buys is a guard waited *after* something else pumped its target in (29.36s and `E_UNEXPECTED` before, 8.6µs after). `examples/deferred_arrival.rs` is the measurement, including the CPU load that reproduces the race: 3 short in 40 rounds before, 0 in 40 after, 0 in 40 either way on a quiet machine. Its arm G asks the narrower question — whether the wait ended on the launch's *own* event — and answers 0 in 40, which is **not** evidence the window is unreachable: this fixture holds one competing break (the attach's injected break-in), and it is spent on the wider window before the narrower one opens. The condition is tightened on the mechanism, not on that count. - **An engine does not cross threads and an `InterruptHandle` does, and since #136 stage 4 that is the types rather than a sentence.** `DebugEngine` is neither `Send` nor `Sync`: the two `unsafe impl`s that said otherwise are deleted, because they asserted the opposite of what `InterruptHandle`'s own doc has always said - `SetInterrupt` is the one DbgEng call documented as safe from any thread *because the rest of the engine is single-thread-affine* - and neither carried a safety comment, because neither could have been given a true one. **Nothing needed them**, which is what made it a deletion rather than a trade: measured by removing each and building, this crate `--all-targets` and windbg-mcp both compile unchanged, and both consumers already create the engine on the thread that uses it (windbg-mcp's `worker::build_engine` on its engine thread; the pool extension around the client DbgEng hands its command). Three things follow. **`deferred_inputs` is a `RefCell`**, not a `Mutex` - every reader reaches it through `&self` and there is now one thread by construction, so the lock was guarding against a caller the type cannot produce; `Drop` uses `get_mut`, so that path cannot panic on a borrow at all. **`InterruptHandle`'s SAFETY comment states its own assumption** rather than delegating to `DebugEngine`'s impls, which is what it used to do and which no longer exists to delegate to. And **it is asserted, not left to the absence of an `unsafe impl`** (`test_the_engine_does_not_cross_threads_and_the_handle_does`): re-adding one is a single line that compiles and reads as a fix for whatever error it silences. Rust has no `T: !Send`, so the test uses the inherent-method trick - an inherent `impl` beats a trait one in resolution, so the `true` constant is reachable only when the bound holds. Mutation-verified by putting both impls back. diff --git a/examples/abandoned_launch.rs b/examples/abandoned_launch.rs index 0c9066c..e10f38d 100644 --- a/examples/abandoned_launch.rs +++ b/examples/abandoned_launch.rs @@ -1,6 +1,7 @@ //! Scratch measurement for dbgscope#141 (not part of the public API): a launch guard dropped -//! **before** anything pumps leaves its `Waiting` entry removed and no exclusion behind it, while -//! `CreateProcessWide` is still queued. Whose process does the *next* launch get? +//! **before** anything pumps, while its `CreateProcessWide` is still queued. Whose process does the +//! *next* launch get? (When this was written the dropped guard's `Waiting` entry was removed with +//! no exclusion left behind; #143 keeps it.) //! //! The issue was written from reasoning. Both of its load-bearing claims needed measuring, and the //! first draft of this file measured the wrong thing: @@ -11,10 +12,11 @@ //! example did, and why it reported "waited for its own process" for a case it had not tested. //! //! - **B.** Does a launch whose image does not exist fail at `launch_process_begin`, or later -//! inside the wait? This decides whether the fix is hard. `deferred_arrival`'s arm C says "fails +//! inside the wait? This decides whether the fix is hard. `deferred_arrival`'s arm C said "fails //! *inside* the wait", but that arm calls the combined `launch_process`, which cannot tell the -//! two apart — the error comes back from one call either way. windbg-mcp's `worker.rs` says the -//! opposite and claims a live check. +//! two apart — the error comes back from one call either way. windbg-mcp's `worker.rs` said the +//! opposite and claimed a live check, and was right: **it fails at begin**, `0x80070002` in +//! 1.5ms with no guard returned. //! //! - **C.** The contract itself, observed rather than inferred: when the second guard's `wait()` //! returns, is that guard's **own program** in the session? Distinguishable images (`cmd.exe` @@ -30,7 +32,8 @@ use dbgscope::dbgeng::DebugEngine; use std::time::Instant; /// The session's process listing, through the public surface only — the same `|` reading -/// `deferred_arrival` uses, since `session_processes` is private to the crate. +/// `deferred_arrival` uses. Not `session_processes`, though that is public now: arm C tells the two +/// programs apart by image name, and that answers only `(engine id, pid)` pairs. fn listing(e: &DebugEngine) -> String { e.execute_command("|").unwrap_or_default() } diff --git a/examples/deferred_arrival.rs b/examples/deferred_arrival.rs index e0295a0..0f8120c 100644 --- a/examples/deferred_arrival.rs +++ b/examples/deferred_arrival.rs @@ -22,9 +22,12 @@ //! names it: `510.29b4: Break instruction exception - code 80000003 (first chance)`, on the //! *attached* pid. `AttachProcess` injects a thread to raise it, and under load that thread is //! scheduled late enough to land a whole wait after its own. -//! - **A pump must not swallow a real failure** (arm C). A launch whose image does not exist fails -//! *inside* the wait — `Err(0x80070002)` in 13ms, no debuggee behind it, a further wait -//! answering `E_UNEXPECTED` in 37µs — so the loop propagates rather than pumping on. +//! - **A launch that cannot start never reaches a pump** (arm C). A launch whose image does not +//! exist answers `Err(0x80070002)` in 13ms with no debuggee behind it, and a further wait answers +//! `E_UNEXPECTED` in 37µs. This arm was read as failing *inside* the wait, which it cannot show: +//! it calls the combined `launch_process`. `abandoned_launch` arm B splits the two and the error +//! is `launch_process_begin`'s, before any guard exists — so there is no failure here for a pump +//! to swallow, and the further wait failing rather than expiring is arm I's fact. //! - **A guard may ask before it waits** (arms E, F and H). Neither opener lists its process //! before the wait that completes it, so the ask cannot fire on an ordinary open; a guard whose //! target arrived meanwhile took 29.36s and `E_UNEXPECTED` before the fix against single-digit @@ -403,9 +406,10 @@ fn what_a_wait_can_conclude() { wait_once("a stopped target, nothing left to report", &e); } -/// Arm C: the failure a pump must not turn into a hang — a launch whose image does not exist. -/// The spawn is deferred, so this fails *inside* the wait, and a fix that pumps until a process -/// appears has to see that failure rather than wait out its whole deadline. +/// Arm C: a launch whose image does not exist. Written as the failure a pump must not turn into a +/// hang, on the reading that the deferred spawn fails *inside* the wait — it does not: the error is +/// `launch_process_begin`'s, before any guard exists (`abandoned_launch` arm B), which this arm +/// cannot see because `launch_process` fuses the two. What it still shows is that nothing hangs. fn a_launch_that_cannot_start() { println!("\n=== C. launch_process on an image that does not exist ==="); let e = DebugEngine::new(); diff --git a/src/dbgeng.rs b/src/dbgeng.rs index 5a7c84c..e9264b8 100644 --- a/src/dbgeng.rs +++ b/src/dbgeng.rs @@ -699,9 +699,12 @@ struct ArrivalId(u64); /// 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 for pid reuse, cleared where a session /// was replaced and again where one was ended, each of which arrived as a review finding -/// (dbgscope#133 rounds 7 to 9). A register of *pending* opens needs none of that: an entry lives -/// exactly as long as the guard that made it, so there is no stale record to prune, clear or -/// match by accident. +/// (dbgscope#133 rounds 7 to 9). A register of *pending* opens needs none of that while an entry +/// lives as long as the guard that made it, so there is no stale record to prune, clear or match +/// by accident. The one entry that outlives its guard is an abandoned launch's, kept until the +/// session goes so that it absorbs the process its still-queued create produces +/// ([`Arrivals::forget`], dbgscope#141) — and retiring that one is what the clears at a session's +/// end and replacement are still for. /// /// It also gives each open an identity, which is what closes the ambiguity [`Arrival`] used to /// document as accepted: two launches pending at once could not be told apart, because the first @@ -709,6 +712,9 @@ struct ArrivalId(u64); /// here, so the second launch waits for the next one. The reason that fix was weighed and rejected /// before is that it needed "new engine-wide state, cleared everywhere a session is replaced and /// pruned for pid reuse" — which is the very cost this shape does not have. +/// +/// What a claim settles is that two opens are not given the *same* process. Which launch is given +/// which is a different question and is still open — see [`Arrival`] and dbgscope#141. #[derive(Debug, Default)] struct Arrivals { /// Ids handed out so far. Never reused. @@ -899,13 +905,17 @@ impl Arrivals { // (dbgscope#141). Dropping the guard does not un-queue the `CreateProcessWide` behind it, // so that process still arrives — and it is new to the *next* launch's snapshot, because // it did not exist when that snapshot was taken, so without this the next launch claims it - // and its `wait()` returns for a target it never asked for. Measured before it was fixed: - // a launch of `ping.exe` behind an abandoned `cmd.exe` was delivered the `cmd.exe`. + // and its `wait()` returns on that stop, before its own process has stopped at all. + // Measured before it was fixed: a launch of `ping.exe` behind an abandoned `cmd.exe` was + // delivered the `cmd.exe`. // // The entry is kept rather than an exclusion left behind, because there is nothing to - // exclude yet — which process the create will produce is exactly what nobody knows. Being - // first in registration order is what makes it right: `deliver` offers in that order, and - // the engine realises queued creates in it. + // exclude yet — which process the create will produce is exactly what nobody knows. **What + // makes keeping it right is accounting, not order**: one entry absorbs one arrival, so the + // next launch's `wait()` returns only once a second process has stopped. Being first does + // not decide *which* one it absorbs — one `WaitForEvent` realises both queued creates and + // which of them the event names is a coin flip — so the next launch is still handed the + // `cmd.exe` about half the time. That is identification, the half dbgscope#141 keeps open. // // **Only a launch, and only one that has been given nothing.** An abandoned *attach* is // already covered by the engine's `attached_processes`, which is what `Pending::wants` @@ -965,9 +975,12 @@ impl Arrivals { /// Routes a stop to the open that wants it, if any. /// - /// **First registered wins**, which is the whole of what makes two pending launches - /// distinguishable: the arrival is offered in registration order and claimed by one open, so - /// the second launch is still waiting when the next one comes. + /// **First registered wins**, which is the whole of what keeps two pending launches from being + /// given the same process: the arrival is offered in registration order and claimed by one + /// open, so the second launch is still waiting when the next one comes. It does not give each + /// launch its *own*: one `WaitForEvent` realises every queued create and which of them the + /// event names is a coin flip, so the first launch registered takes whichever stops first + /// (dbgscope#141). /// /// A process already claimed is offered to nobody, which [`Pending::wants`] enforces for every /// caller rather than this one alone: a target stops more than once in a session -- every later @@ -9070,10 +9083,11 @@ impl DebugEngine { /// hurt: the operating system reusing it for a process this engine went on to launch, which /// would then be detached from and survive a session that is supposed to take it. /// - /// The pending opens go for a narrower reason than the record they replace did. An entry - /// cannot outlive its guard, so nothing here is about staleness across time; what it is about - /// is a guard held *across* a session replacement, whose `(engine id, pid)` predicate would - /// otherwise be evaluated against a session it never asked about. Forgetting the entry leaves + /// The pending opens go for a narrower reason than the record they replace did. Two kinds of + /// entry reach here: a guard held *across* a session replacement, whose `(engine id, pid)` + /// predicate would otherwise be evaluated against a session it never asked about; and an + /// abandoned launch's, the one entry that outlives its guard ([`Arrivals::forget`]), which + /// would otherwise go on waiting into the next session. Forgetting the entry leaves /// [`Arrivals::presence`] answering [`Presence::Absent`] for it, which is the truth. /// /// **And it reissues the target identity, which is why that lives here rather than in the @@ -9181,9 +9195,11 @@ impl DebugEngine { // sharper reason -- engine process ids are handed out from zero again for the next // session, so a pair that outlived this one was matched by a later process that inherited // both numbers, which for two `attach_process` calls to the same pid on one engine is the - // ordinary case rather than a coincidence. Nothing outlives its guard any more, so what is - // left here is tidiness rather than a hazard. (`detach_attached_processes` already takes - // the attach record; this is the other half of what a session owns.) + // ordinary case rather than a coincidence. Only an abandoned launch's entry outlives its + // guard now, and it is why this is more than tidiness: its create dies with the session, + // so left here it would wait on into the next one and take that session's first launch's + // stop. (`detach_attached_processes` already takes the attach record; this is the other + // half of what a session owns.) // // **The condition is `EndSession`'s own outcome and not the value this returns**, which // are two different facts wherever a detach fails: the `.and` above reports that failure to @@ -9512,9 +9528,11 @@ enum WaitKind<'a> { /// to complete a guard that is still held: [`PendingTarget`] documents driving the engine yourself /// as a thing to do, and until the entry exists there is nobody for a stop to be delivered to. /// -/// **Forgotten on drop, and that is the whole of the lifecycle** the record it replaces needed -/// pruning, clearing and a review round each for. An entry cannot outlive the open that made it, -/// so there is no stale arrival for a reused engine id or a reused pid to match. +/// **Forgotten on drop, and that is nearly the whole of the lifecycle** the record it replaces +/// needed pruning, clearing and a review round each for. An entry does not outlive the open that +/// made it, so there is no stale arrival for a reused engine id or a reused pid to match. The +/// exception is a launch dropped before it was delivered anything: [`Arrivals::forget`] keeps its +/// entry for the process its queued create still produces, and the session's end retires it. struct Registered<'a> { engine: &'a DebugEngine, id: ArrivalId, @@ -9566,20 +9584,29 @@ impl Drop for Registered<'_> { /// /// dbgscope#136 stage 3 removes most of it, and removes that cost with it. An arrival is delivered /// to a registered open ([`Arrivals`]) and **claimed** by it, so the second launch is still waiting -/// when the next one comes; and because an entry cannot outlive the guard that made it, there is no -/// stale claim to prune or clear. The record whose lifecycle made the fix expensive was the thing -/// the fix would have joined. A guard that finishes and is dropped hands its claim to the opens -/// still waiting ([`Arrivals::forget`]), so the two live guards and the delivered-then-dropped case -/// are both exact. +/// when the next one comes; and because an entry does not outlive the guard that made it, bar an +/// abandoned launch's (below), there is no stale claim to prune or clear. The record whose +/// lifecycle made the fix expensive was the thing the fix would have joined. A guard that finishes +/// and is dropped hands its claim to the opens still waiting ([`Arrivals::forget`]), so no process +/// is given to two opens, whether both guards are live or one was delivered and dropped. /// -/// **What is left is a launch abandoned before it is delivered anything**, and it is dbgscope#141 -/// rather than this change: dropping such a guard takes a `Waiting` entry out of the register and -/// leaves no exclusion, so the process the deferred `CreateProcessWide` still produces is new to -/// the *next* launch's snapshot and is claimed by it. Keeping the entry is not simply the fix — -/// nothing would ever retire one whose launch failed to start (`deferred_arrival` arm C: an image -/// that does not exist fails inside the wait), and it would then take the next launch's process -/// and time that guard out instead. A wrong `Ok` for a wrong timeout is a trade rather than a fix, -/// and it wants deciding on its own. +/// **What is left is identification, and it is dbgscope#141.** A claim makes arrivals *distinct*, +/// not *attributed*: [`Arrivals::deliver`] offers in registration order, but one `WaitForEvent` +/// realises every queued create (measured: session 0 → 2 on a single pump) and which of them the +/// event names is a coin flip, so the first launch registered takes whichever process stops first. +/// Telling them apart needs something to match on rather than an order, and `CreateProcessWide` +/// 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 until the session goes** +/// ([`Arrivals::forget`]). Its `CreateProcessWide` is still queued, and the process it produces is +/// new to the next launch's snapshot — so without the entry the next launch claimed it and its +/// `wait()` returned before its own process had stopped. A missing image leaves no entry behind: +/// it fails at [`DebugEngine::launch_process_begin`], before any guard exists (`0x80070002` in +/// 1.5 ms, `examples/abandoned_launch.rs` arm B). What would strand one is a process that is +/// created and never stops, which nothing has measured: its entry would stay waiting and take the +/// next launch's stop. An open that pumps its whole bound and sees nothing is discarded rather +/// than kept ([`Arrivals::discard`]). #[derive(Debug)] enum Arrival { /// A process this engine launched: one the session did not hold when the launch was issued and @@ -10414,9 +10441,9 @@ mod tests { "the second launch was not given the arrival nobody had claimed" ); - // An entry dies with its guard, and an id that names nothing is `Absent` -- which is what - // a guard held across a session replacement reads, and the whole of the lifecycle the - // record this replaces needed a prune and two clears for. + // A delivered entry dies with its guard, and an id that names nothing is `Absent` -- which + // is what a guard held across a session replacement reads, and the whole of the lifecycle + // the record this replaces needed a prune and two clears for. arrivals.forget(first); assert_eq!( arrivals.presence(first, &[(0, 100)], &none), @@ -13674,8 +13701,9 @@ mod tests { let _ = theirs.wait(); } - /// **Two launches pending at once are told apart**, which [`Arrival`] used to document as an - /// accepted ambiguity. + /// **Two launches pending at once are given different processes**, where [`Arrival`] used to + /// document both waits ending on the first arrival as an accepted ambiguity. Not each its own + /// process: that is dbgscope#141, and this does not assert it. /// /// A launch is identified by elimination -- `CreateProcessWide` hands back no pid -- so with /// two of them pending the first arrival is new to both snapshots and satisfied both waits. @@ -13737,11 +13765,10 @@ mod tests { e.end_session().expect("end_session failed"); } - /// **A launch abandoned before anything pumps does not hand its process to the next launch** - /// (dbgscope#141). + /// **A launch abandoned before anything pumps absorbs an arrival of its own** (dbgscope#141). /// - /// Dropping a guard takes a `Waiting` entry out of the register and leaves no exclusion behind - /// — there is nothing to leave, since nothing was ever delivered to it. The queued + /// Dropping a guard used to take its `Waiting` entry out of the register and leave no exclusion + /// behind — there was nothing to leave, since nothing was ever delivered to it. The queued /// `CreateProcessWide` still produces a process, and it is new to the *next* launch's snapshot /// because it did not exist when that snapshot was taken. /// @@ -16331,9 +16358,10 @@ mod tests { /// it was review that put it there. /// /// **The construction that makes it unreachable** is that an arrival is delivered to a - /// registered open rather than broadcast into a set: an entry cannot outlive the guard that - /// made it, so the reattach's open starts with nothing delivered to it whatever numbers it - /// inherits. There is no record left to prune, and the prune's arrival half is gone with it. + /// registered open rather than broadcast into a set: an attach's entry cannot outlive the guard + /// that made it (only an abandoned *launch's* is kept), so the reattach's open starts with + /// nothing delivered to it whatever numbers it inherits. There is no record left to prune, and + /// the prune's arrival half is gone with it. /// /// Kept as an end-to-end statement of that rather than deleted, since it costs one attach: the /// second process is what makes this a *detach* rather than a teardown, so nothing else could