Repository navigation
test(#5931): order the parent-snapshot disposal test by the condition, not a reading that precedes it - #5950
Merged
Conversation
…a reading that precedes it The test waited for the child's drainsAwaitingScheduler=1 and then asserted the parent's RunLevel. A frozen child can hold a drain before its parent is disposed, so the wait returned while the parent was still Quiescing (core #5943's shard: red then green on identical code; rbuergi/Feedback/disposal-stall-snapshot-test-races-runlevel). The test now produces that state on purpose: the child holds a drain before the parent's Dispose(), and a responder on its own paused scheduler holds the parent in Quiescing on an unanswered request. It asserts the trap is armed, releases the responder, then waits through RunLevelChanged for DisposeHostedHubs and for the child's Disposal=Pending before asserting. Negative control: the counter-only wait fails 3/3 here, reading RunLevel=Quiescing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Contributor
![systemorph[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Test-only change to `DisposalStallNamesThePumpTest.AParentsSnapshot_NamesAChildDrainTheSchedulerHasNotStarted`: the old flow waited on the `drainsAwaitingScheduler=1` counter and then asserted the parent's live run level, which could return while the parent was still `Quiescing` (a frozen child can hold a drain before its parent is ever disposed). The new flow produces that state deliberately — the paused child holds a drain before `parent.Dispose()`, and a responder hub on a second paused scheduler holds the parent in `Quiescing` on an unanswered request — asserts the trap is armed (the counter is already visible in the parent's snapshot at `Quiescing`), then waits through `RunLevelChanged` for `DisposeHostedHubs` and for the child's `Disposal=Pending` before asserting the snapshot names the child's held drain. No product code changed. Checked: the precondition ordering (responder still paused while the `Quiescing` snapshot is taken; child still paused while the final snapshot is asserted, so the parent cannot advance past `DisposeHostedHubs` and the snapshot's claims are pinned); the `=1)` literals guard against multi-digit counts; the `finally` resumes both schedulers and disposes the held subscription on every path, including failures before each pause; the final teardown wait is unchanged apart from the once-captured `ct`. Not verifiable from the diff: `RunLevelChanged`'s replay semantics for late subscribers (both waits subscribe after the transition may have fired), whether the harness disposes the never-disposed mesh-scope responder hub, and the author-reported evidence (negative control 3/3, 547/547 green, 0 warnings) was not re-run here.
Findings: 0 blocking · 0 should-fix · 2 question · 0 nit
Internal review of 51081f1d8488b53228774bde6e42a4db7a451d61 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| .Subscribe(_ => { }, _ => { }); | ||
|
|
||
| parent.Dispose(); | ||
| await parent.RunLevelChanged |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
Both `RunLevelChanged` waits subscribe after the transition they wait for may already have happened: the `Quiescing` wait here subscribes after `parent.Dispose()` has returned (line 212), and the `DisposeHostedHubs` wait (line 231) subscribes after `responderScheduler.Resume()` (line 227). A hub that is held at a level does not re-emit a change event, so if `RunLevelChanged` publishes change events only, without replaying the current level to new subscribers, a subscription that loses this race waits for an event that never comes again and fails only on `Within(TestTimeouts.Convergence)` — a latent miss of the same family this change removes from the counter wait. If `RunLevelChanged` replays the current level on subscribe, both waits are sound. The stream's replay semantics are not visible in this diff.
Contributor
Author
There was a problem hiding this comment.
It replays the current level, so both waits are sound. RunLevelChanged is runLevelChanged.AsObservable(), and runLevelChanged is Subject.Synchronize(new BehaviorSubject<MessageHubRunLevel>(MessageHubRunLevel.Starting)) (MessageHub.cs, around line 343). A late subscriber receives the level the hub is currently at. Neither level can be skipped before its wait subscribes. Quiescing is held by the parent's unanswered request until the test releases the responder, and DisposeHostedHubs is held by the child's frozen drain until the finally.
| h.Post(new Pong(), o => o.ResponseFor(d)); | ||
| return d.Processed(); | ||
| }), HostedHubCreation.Always)!; | ||
| var responder = (MessageHub)Mesh.GetHostedHub(new Address("responder", "holds-parent-quiescing"), c => c |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`responder` is created at mesh scope but never disposed by the test: `parent` is disposed explicitly and the child is torn down with it, while the responder — a live hub with a registered `Ping` handler — is left to whatever teardown the test harness performs. Whether the harness disposes root-level hosted hubs after each test (per-test mesh) or whether they outlive the test is not visible in this diff; in the latter case the hub at `responder/holds-parent-quiescing` and its handler leak into subsequent tests of the suite.
Contributor
Author
There was a problem hiding this comment.
Fixed in c87cf45. The test now disposes the responder itself after the parent tree is Dead, waits on its DisposalCompleted, and asserts RunLevel == Dead. The responder no longer depends on the fixture's teardown. DisposalStallNamesThePumpTest 3/3 pass, and the Release -warnaserror build is clean.
Contributor
Test Results (shard 0) 1 files ± 0 1 suites ±0 7m 2s ⏱️ + 4m 26s Results for commit c87cf45. ± Comparison against base commit 68b9705. This pull request removes 348 and adds 547 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Test Results 1 files - 17 1 suites - 17 7m 2s ⏱️ - 31m 2s Results for commit c87cf45. ± Comparison against base commit 68b9705. This pull request removes 9611 and adds 9 tests. Note that renamed tests count towards both. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #5820 · Refs #5931 · fixes the race filed as
rbuergi/Feedback/disposal-stall-snapshot-test-races-runlevelWhat was wrong
DisposalStallNamesThePumpTest.AParentsSnapshot_NamesAChildDrainTheSchedulerHasNotStarted(added in #5931) waited for the child line to readdrainsAwaitingScheduler=1and then asserted that the parent was atDisposeHostedHubs. A frozen child can hold a drain before its parent is ever disposed: any delivery that reaches it after the pause does that. When it does, the wait returned while the parent was stillQuiescing. #5943's shard went red on one run and green on the next with identical code.The fix (test only)
The test now produces the racing state on purpose and orders its waits by the condition:
parent.Dispose(). This is asserted as a precondition.Quiescingon an unanswered request. The test asserts the trap is armed: atQuiescingthe parent's snapshot already shows the child's counter.RunLevelChanged(.Should().Within(...).Match(...)) forDisposeHostedHubs. It then waits for the child line'sDisposal=Pending, because that phase sets the level before it disposes the children in the same turn. Only then does it assert.No sleep, no widened timeout, no product change.
Evidence
Hub parent/hosts-starved-child RunLevel=Quiescing …. With the fix restored it passes.MeshWeaver.Messaging.Hub.Test: 547/547 passed, 0 skipped.dotnet build -c Release -warnaserroron the test project gave 0 warnings and 0 errors.Pairs-with: none — test-only change
Mirror-sync: none — no localization catalog change
🤖 Generated with Claude Code