You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix: an operation closing on a request nobody read drains the engine's break
Review found a P1 in the new bookkeeping, and it is right. An operation accepts
requests for slightly longer than it reads them: everything from
`took_break_request` to the guard dropping -- which includes
`record_where_it_stopped`'s two COM calls -- is time in which `interrupt()`
still names this operation and still answers `Raised`. The record is then
discarded on drop, so nobody reports it, while the `SetInterrupt` behind it is
still pending and free to break into whatever runs next with nothing to explain
it.
**The window cannot be closed, and the suggested remedy would break a working
path.** Closing an operation to requests at the take is wrong:
`wait_for_live_target` pumps repeatedly inside one operation, and a request
arriving between two of its pumps is one the next pump legitimately reads.
Closing it later has no later to close at -- whether the engine thread has a
read left is not knowable to the calling thread at the moment it asks. So the
fix is the half that *is* reachable, plus honesty about the half that is not.
`Operation::drop` now drains the engine's own pending request when, and only
when, it is discarding a record nobody read. That is not a new policy: it is
what `execute_and_wait`, `settle` and the bounded command path already do
wherever a break belongs to no operation -- "consume anything the engine did
not, so the next operation starts clean" -- generalised to the one window that
had no site to put it at. So the second shape of the residue can no longer
become the first.
And `BreakRequest::Raised` stops promising what it cannot keep. It said the
operation "will report `Interruption::OnRequest` when it ends"; it now says the
request was filed against the operation running at that instant, and says why
that is not the same claim.
`test_a_discarded_request_drains_the_engines_pending_break` covers it, paired
against a control in the same test because `GetInterrupt` is a consuming read
and "reads false" is unfalsifiable without a case that reads true. Verified by
backing the drain out: it fails on the assertion it is written for.
`#[ignore]`d for the reason the other `GetInterrupt` tests are -- it needs a
live debuggee.
Re-verified after the change: 170 tests, 4 doctests, the full Miri suite (138),
the four `#[ignore]`d interrupt tests, `session_fuzz` seeds 1 and 7,
`deferred_arrival` unmoved (arm A 0 short in 40, arm F 4.2us, arm H 5.2us).
Refs #136, #135
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HyRDk1yX5UqRkdpGgodrMY
-**Commands & events**: `execute_command`, `wait_for_event`, `execute_and_wait`, `settle`; output is captured through `OutputCallbacks` (an `IDebugOutputCallbacks` impl).
57
57
- **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. Stages 3 and 4 of #136 — delivering a stop instead of broadcasting into `stopped_on`, and dropping `unsafe impl Sync for DebugEngine` — are not done.
58
-
- **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 in `stopped_on` as an initial break, 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**: a break aimed at operation N that lands on N+1 stops N+1, which sees no request of its own. Bookkeeping cannot reach that; `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.
58
+
- **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 in `stopped_on` as an initial break, 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.
59
59
- **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.
60
60
-**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 `<no event>`, 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.
0 commit comments