feat: a namespace walk can be stopped, so a caller's clock reaches inside it - #159
Conversation
…side it A directory is an unbounded amount of work behind one call. `\GLOBAL??` on an ordinary Windows guest holds a couple of hundred entries and each costs several reads of target memory, so over a kernel debugging wire the enumeration alone can outlast the patience of whoever asked for it. Until now the only bound on it was a timeout on *waiting*, which abandons the waiter and not the walk -- so the work carried on holding the session it ran on, which is the one thing a deadline is supposed to prevent. `Namespace::halting` takes a predicate, polled per directory entry and per link of a bucket's chain, which is where the reads are. What it stops is reported rather than raised wherever there is a partial answer to give: `Listing::halted` on an enumeration, so a caller reports a short list as short instead of losing the work, and `ObjectError::Halted` on a lookup, which has none. That error is deliberately not `NotFound`: the component may be in the part of the directory the walk never reached, and a caller told the name is absent stops looking for something that is there. **The two polls hid each other, and the first version of the test did not notice.** Removing either one left it green because the other still stopped the walk. Each is now checked on a construction the other cannot reach: the chain poll against a bucket of entries that name nothing, which `entries_of` follows to its bound and which `named_in` therefore never sees -- placed in the root, since a sub-path would resolve its directory first and halt there instead, proving nothing -- and the entry poll against a predicate held off until the chain has finished, where an empty listing can only be the poll inside `named_in`. All three mutations fail now; two of them passed before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97535237d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.stopped() { | ||
| return Ok((out, true)); |
There was a problem hiding this comment.
When a directory is empty, or has a long run of empty buckets, this check is never reached because the while entry != 0 body does not execute. Consequently, even .halting(&|| true) performs one target-memory read per bucket and can return halted: false; with a remote kernel and a layout permitting up to 1024 buckets, the supposedly cancelled operation can continue holding the debug session for many slow reads. Poll before each bucket read as well as while following nonempty chains.
Useful? React with 👍 / 👎.
| /// one left this green while the other still stopped the walk, so each is checked on a | ||
| /// construction the other cannot reach. | ||
| #[test] | ||
| fn a_walk_stops_where_its_caller_asks_and_keeps_what_it_read() { |
There was a problem hiding this comment.
test_
Rename this test so its function name starts with test_, as required by the repository's test naming convention.
AGENTS.md reference: AGENTS.md:L28-L30
Useful? React with 👍 / 👎.
| /// True when the walk was stopped before the directory ran out. | ||
| /// | ||
| /// **The list is then a prefix of the directory rather than the directory**, and the counts | ||
| /// beside it describe only what was reached. Separate from them because it is not an entry | ||
| /// this could not read -- it is entries it never looked at, and there is no saying how many. | ||
| pub halted: bool, |
There was a problem hiding this comment.
When callers enable halting and the walk stops before reading any entry, this new state produces halted == true while Listing::skipped() still returns zero. That contradicts the helper's documented purpose of telling callers that a list is short and can cause consumers checking only skipped() to treat a partial namespace as complete; provide a completeness helper that includes halted or otherwise update this API contract so the partial result cannot be mistaken for a complete one.
Useful? React with 👍 / 👎.
Round five, and the finding is right: `objects_in` was one opaque call with no deadline in it, and the poll came afterwards, in the loop over a listing that was already complete. `\GLOBAL??` is a couple of hundred entries and several target reads apiece, so over a kernel debugging wire the enumeration alone can outlast the whole call -- and a supervisor timeout abandons the *waiter* rather than the job, so an unstoppable walk goes on holding the session after nobody is waiting for it. That is the failure a deadline exists to prevent, and this was the one walk in the server without one. The bound had to go into dbgscope, since the enumeration is entirely inside it: glslang/dbgscope#159 adds `Namespace::halting`, polled per entry and per chain link. This hands it the same predicate the rest of the loop already used. **A stopped enumeration is a prefix, so it is recorded as a halt rather than left to the counts.** `links_examined + links_unnamed` is what the directory holds only when the walk reached the end of it; stopped, the entries nobody looked at are in neither figure. The two counts alone would say the search checked everything it saw, which is true and not what a reader needs. So the halt sets `stopped`, which makes the verdict `partial`, and the live test takes `stopped` being unset as the precondition for the identity rather than asserting it away. Pin moves to that branch; repoint at `main` once it merges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
…ete one Two findings on #159, both correct. **The halt was polled inside a chain and not before a bucket.** An empty bucket never enters that loop, so a directory that is empty -- or merely has a long run of empty buckets -- was unstoppable: `halting(&|| true)` still paid one target read per bucket, up to the thousand a layout may declare, and then answered `halted: false` having done all of it. It is polled before the read now. The test gains a third construction for it, because the two it had could not reach this one: an empty directory enters no chain and names no entry, so the bucket poll is the only one that can fire there. **And `skipped()` was being read as a completeness test it never was.** It counts entries the walk *reached* and could not present, so a walk stopped before reading any of them returns zero while missing an unknown number -- which a caller branching on `skipped() == 0` reads as the whole directory, the one reading these counts exist to prevent. `is_complete()` is the question to ask, and `skipped()` now says in its own docs that it is not that question. Both mutation-verified. Declined: renaming the new test to `test_*` per `AGENTS.md`. The convention is real and this module does not follow it -- `src/object.rs` has twenty-four descriptively named tests and no `test_`-prefixed one, merged that way across #157 and #158. Renaming one would make it the only exception in the file, which is worse than either consistency; renaming all twenty-four is a change of its own and not this PR's subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 017f9b5473
ℹ️ 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".
| let after_the_chain = || { | ||
| polls.set(polls.get() + 1); | ||
| polls.get() > 1 | ||
| }; |
There was a problem hiding this comment.
With the current root fixture, entries_of polls once before bucket 0 and again before following its first chain entry, so polls.get() > 1 becomes true during the chain walk rather than in named_in. The assertion therefore still passes if the per-entry stop check at lines 674–677 is removed, leaving cancellation during the several name/security/type reads untested. Keep the predicate false through all bucket and chain polls, or instrument which phase invoked it, before asserting that the entry poll halted the listing.
Useful? React with 👍 / 👎.
Round two, and the finding is against the fix from round one -- which claimed this exact thing was fixed. Verified before acting: delete the per-entry `stopped()` in `named_in` and the test stayed green, so the rule it says it pins was untested through two rounds of saying otherwise. The reason it is easy to get wrong is worth writing down, because it defeated two attempts. Every *other* halt leaves an **empty** listing, because `entries_of` returns before a single entry is named -- and an empty listing is exactly what deleting the entry poll produces as well. So `objects.len() == 0` cannot tell the two apart, and a threshold of `polls.get() > 1` was in fact being crossed by the chain poll, one link into bucket zero. What only a stop *between two namings* can produce is a **proper prefix**. So the directory gets a second entry and the assertion is one object of two, which no bucket or chain halt can reach. The threshold is counted off the fixture -- one poll per bucket the layout declares, then one per chain link, then one per entry -- rather than off a calibrating run, which would have moved with the mutation and passed against it a third time. Two assertions now, and each fails on its own with the poll removed: the poll count of a complete walk, which states the three phases outright, and the prefix. Re-verified the other two polls against the new shape while here; removing the bucket poll and removing the chain poll each still fail their own construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08431e47c0
ℹ️ 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".
| let (bodies, mut halted) = self.entries_of(directory)?; | ||
| for body in bodies { |
There was a problem hiding this comment.
When the predicate is a one-shot interrupt check, entries_of can return (bodies, true) after consuming the signal, but this loop ignores the existing halted value and polls again; the next poll is then false and every gathered body is still named. This applies directly to DebugEngine::interrupted, whose measured GetInterrupt behavior clears the request on its first poll (src/dbgeng.rs:11135-11161). On a remote session this can perform thousands of reads after Ctrl+C, and object_at can even return a found object as though the interrupted lookup completed. Break before entering this loop whenever entries_of already reported a halt.
Useful? React with 👍 / 👎.
… consumes it A real one, and the sharpest finding this walk has had. `named_in` took the `halted` that `entries_of` handed back and then asked the predicate again before naming the first entry. With a predicate that *stays* true that is harmless, which is why four constructions and three rounds of mutation testing all missed it. It is not harmless with the predicate this crate actually supplies. `DebugEngine::interrupted` is `GetInterrupt`, which **clears** the pending request on its first poll -- not documented as doing so, which is why this crate measures it: `test_get_interrupt_drain_semantics` asserts the vector `[true, false, false, false, false]`. So a Ctrl+C caught while the buckets were being walked came back here as `halted`, the second ask answered false because the first had taken it, and every entry the walk had gathered was named anyway. On a remote kernel that is thousands of reads after the stop was asked for. `object_at` inherited it in a worse form: it consults `halted` only when the name is *not* in the listing, so a lookup that was interrupted mid-walk returned the object as though it had finished. Fixed at the one site, which repairs both. The test needs a **one-shot** predicate, and needs the halt to land after at least one body is gathered -- a stop at bucket zero gathers nothing and hides the whole thing. So it fires on the third poll: bucket zero, its chain (where the body is pushed), then bucket one. Mutation-verified: with the break removed the listing comes back holding the entry it was told to stop before naming. One thing that bit while writing it, recorded because it is invisible: a one-shot closure cannot be reused for a second walk. Its counter is spent, so the second walk ran to the end and asserted nothing until it got its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc4b06f4dc
ℹ️ 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".
| .ok_or_else(|| match skipped { | ||
| // A walk that stopped says so first: the component may be in the part of the | ||
| // directory it never reached, so neither absence below is a thing it knows. | ||
| _ if halted => ObjectError::Halted { |
There was a problem hiding this comment.
halted before accepting a found component
When a one-shot predicate fires between naming two entries and the requested component is already in the completed prefix, find returns Some and this ok_or_else guard never examines halted. For a multi-component path, the consumed interrupt is then false while the walk descends into the child directory, so a lookup can continue doing remote reads and ultimately succeed after Ctrl+C. Fresh evidence beyond the earlier finding is the new two-entry proper-prefix fixture, which demonstrates that named_in can return both one object and halted: true but only tests enumeration; reject the lookup before accepting or descending through that object.
Useful? React with 👍 / 👎.
| /// this: `entries_of` returned `halted`, `named_in` asked again, the answer was false because | ||
| /// the first ask had taken it, and the gathered entries were named anyway. | ||
| #[test] | ||
| fn a_halt_the_walk_already_reported_is_not_polled_for_a_second_time() { |
There was a problem hiding this comment.
test_
This newly added test does not use the repository-required test_* naming convention; rename it with that prefix so the test suite remains consistent with the documented rule.
AGENTS.md reference: AGENTS.md:L28-L30
Useful? React with 👍 / 👎.
…d after it The hole the previous commit left, and it is the same bug one level up. That one stopped the *walk* from naming entries after a halt; `object_at` still took a component out of whatever prefix the walk had managed, because it consulted `halted` only down the not-found path. `find` returning `Some` skipped the guard entirely. A one-shot predicate firing *between* two namings is what reaches it: the listing then holds the first name **and** says it was halted. With a single-component path that merely answers early. With two, the lookup descends into the named directory with the interrupt already spent -- so the second component enumerates in full and the lookup **succeeds**, having done the rest of its remote reads after the Ctrl+C that was meant to end them. The new two-entry fixture from the previous commit is exactly the shape that produces it, which is why this was worth finding there rather than later. So the check moves ahead of the lookup and the dead `_ if halted` arm goes with it. A name is not less found for the walk having been stopped; the point is that the caller asked for no more work, and a prefix is precisely where that reads as success. Mutation-verified: with the guard removed the lookup walks on into `Device` and answers. Declined in the same round: renaming this module's tests to the `test_*` prefix AGENTS.md specifies. The rule is real and the repo does follow it -- 211 tests to 41 -- but `src/object.rs` is the single biggest exception at 25 of those 41, and `src/dbgeng.rs` holds 16 more, so renaming the two tests this branch adds would leave the module inconsistent without making the repo consistent. It wants one mechanical commit of its own rather than a corner of this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfc7f9c7f3
ℹ️ 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".
| /// interrupt already spent. A multi-component path is what shows it: with one component the | ||
| /// answer is merely returned early, with two the walk carries on reading. | ||
| #[test] | ||
| fn a_lookup_stopped_after_naming_a_component_does_not_go_on_through_it() { |
There was a problem hiding this comment.
test_
Rename this newly added test so its function name starts with test_, as required by the repository’s test naming convention.
AGENTS.md reference: AGENTS.md:L28-L30
Useful? React with 👍 / 👎.
…ete one Two findings on #159, both correct. **The halt was polled inside a chain and not before a bucket.** An empty bucket never enters that loop, so a directory that is empty -- or merely has a long run of empty buckets -- was unstoppable: `halting(&|| true)` still paid one target read per bucket, up to the thousand a layout may declare, and then answered `halted: false` having done all of it. It is polled before the read now. The test gains a third construction for it, because the two it had could not reach this one: an empty directory enters no chain and names no entry, so the bucket poll is the only one that can fire there. **And `skipped()` was being read as a completeness test it never was.** It counts entries the walk *reached* and could not present, so a walk stopped before reading any of them returns zero while missing an unknown number -- which a caller branching on `skipped() == 0` reads as the whole directory, the one reading these counts exist to prevent. `is_complete()` is the question to ask, and `skipped()` now says in its own docs that it is not that question. Both mutation-verified. Declined: renaming the new test to `test_*` per `AGENTS.md`. The convention is real and this module does not follow it -- `src/object.rs` has twenty-four descriptively named tests and no `test_`-prefixed one, merged that way across #157 and #158. Renaming one would make it the only exception in the file, which is worse than either consistency; renaming all twenty-four is a change of its own and not this PR's subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
1 participant
Raised on the consumer's PR (windbg-mcp#311): a directory is an unbounded amount of work behind one call, and nothing in it polls a deadline.
\GLOBAL??on an ordinary Windows guest holds a couple of hundred entries and each costs several reads of target memory, so over a kernel debugging wire the enumeration alone can outlast the patience of whoever asked. Until now the only bound was a timeout on waiting, which abandons the waiter and not the walk — so the work carried on holding the session it ran on, which is the one thing a deadline is supposed to prevent.Namespace::haltingtakes a predicate, polled per directory entry and per link of a bucket's chain, which is where the reads are. Existing callers are unaffected:Namespace::newleaves it unset andDebugEngine's wrappers pass nothing.What it stops is reported rather than raised wherever there is a partial answer to give.
Listing::haltedon an enumeration, so a caller reports a short list as short instead of losing the work;ObjectError::Haltedon a lookup, which has none. That error is deliberately notNotFound— the component may be in the part of the directory the walk never reached, and a caller told the name is absent stops looking for something that is there.The two polls hid each other
The first version of the test was green with either poll removed, because the other still stopped the walk. Each is now checked on a construction the other cannot reach:
entries_offollows to its bound and whichnamed_intherefore never sees. Placed in the root, since a sub-path would resolve its directory first and halt there, proving nothing about the enumeration.named_in.All three mutations fail now; two of them passed before.
Verification
cargo fmt --all --checkclean, no new clippy warnings inobject.rs.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf