Repository navigation
test(orleans): wait for the pod-hub CLAIM, not for reachability (#3298) - #3313
Merged
Merged
Conversation
CrossSiloNack_ReachesASenderWhoseStreamSubscriptionIsGone erases the sender's stream subscription and then asserts that a NACK arrives over the DIRECTED pod-hub transport. That assertion has a precondition the test never checked: the claim which makes the directed transport available at all. RegisterStream establishes two things on different clocks. The local route is synchronous and never fails. The cluster-wide pod-hub claim is asynchronous, retried on a capped backoff (100ms doubling to 2s) with NO give-up on a silo. Until it lands, [PreferLocalPlacement] puts a directed IPodHubGrain call on the CALLER's silo, which has no local route for the address, and it answers PodHubNotHere. PostFailure then falls back to the stream — which this test has just emptied — and the NACK is discarded in silence. That is the reported failure exactly: a 30s timeout on the final wait with both controls green. The probe that was supposed to cover this proved only REACHABILITY, and at that moment reachability is satisfied by the STREAM — the transport the test is about to erase. So it was green whether or not the claim had landed. Worse, it is adversarial to the claim: it posts from the other silo in a loop, and each failed directed call mints a throw-away activation there that the owner's next Attach must bounce off, restarting the backoff. That is why this reproduced by luck rather than by tree. MEASURED, not inferred. With one bounce forced and the claim backoff pinned long, the old probe goes green in under a second while PodHubClaimSettled has demonstrably not completed — 6 runs, 6 times. I did not commit that experiment: its setup depends on an activation-lifetime ordering I cannot ENFORCE, so as a permanent test it would risk becoming the next flake. It is recorded in the PR and on the issue instead. The fix waits on PodHubClaimSettled — the positive signal for "the claim stopped attempting", which OrleansCrossSiloReplyTest already waits on before its own directed cross-silo delivery. Its remarks say why a settle-by-silence poll is not an acceptable substitute, so this uses the signal rather than a poll. SiloHostedHub_ReceivesDelivery_EvenWithItsStreamSubscriptionGone has the same latent gap — it waits for the stream SUBSCRIPTION, a different condition that merely happens to take similar time — so it gets the same wait. No production code changes: the platform behaviour here (fall back, then say so loudly) is what it is designed to do during the claim window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RpJi9EZHtGYQCahUwWa7jH
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change aligns the tests with the existing PodHubClaimSettled seam and adds documentation; only a small comment/backoff-value mismatch needs a minor tweak.
Pull request overview
This PR stabilizes Orleans pod-hub transport tests by waiting for the pod-hub claim to settle (the prerequisite for directed cross-silo delivery) instead of inferring readiness from reachability, and documents the pattern to prevent similar flakes.
Changes:
- Updated two
PodHubTransportTestcases to awaitPodHubClaimSettledbefore isolating transports. - Added a
ClaimSettledhelper to centralize the “claim is ready” wait logic. - Documented the rule (“Reachability is not a claim”) in the Orleans testing routing pattern guide.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Orleans.Test/PodHubTransportTest.cs | Adds an explicit wait for claim settlement before removing stream subscriptions and asserting directed delivery. |
| src/MeshWeaver.Documentation/Data/Architecture/OrleansTestRoutingPattern.md | Documents the testing rule and provides a canonical code pattern for waiting on claim settlement. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // now subscriber-less stream, and the NACK was silently discarded — a 30 s timeout on the | ||
| // final wait with both controls having passed, which is the failure exactly as reported. | ||
| // | ||
| // The claim retries on a capped backoff (250 ms doubling to 4 s) with no give-up on a silo, |
Contributor
Test Results (shard 0)251 tests 251 ✅ 2m 16s ⏱️ Results for commit a69176a. |
Contributor
Test Results (shard 1)475 tests 475 ✅ 42s ⏱️ Results for commit a69176a. |
Contributor
Test Results (shard 3)368 tests 366 ✅ 55s ⏱️ Results for commit a69176a. |
Contributor
Test Results (shard 5) 4 files 4 suites 1m 44s ⏱️ Results for commit a69176a. |
Contributor
Test Results (shard 4)2 291 tests 2 291 ✅ 2m 41s ⏱️ Results for commit a69176a. |
Contributor
Test Results (shard 2)588 tests 396 ✅ 2m 57s ⏱️ Results for commit a69176a. |
Contributor
Test Results 15 files 15 suites 11m 17s ⏱️ Results for commit a69176a. |
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.
Closes #3298.
The test asserts a NACK arriving over the directed pod-hub transport after erasing the sender's
stream subscription. It never checked the precondition that makes that transport available.
The mechanism
RegisterStreamestablishes two things, on different clocks:PostFailure's NACKUntil the claim lands,
[PreferLocalPlacement]puts a directedIPodHubGraincall on the caller'ssilo — which has no local route for the address — and it answers
PodHubNotHere.PostFailurethenfalls back to the stream, which this test has just emptied, and the NACK is discarded in silence.
That is the reported failure exactly: a 30 s timeout on the final wait, with both controls green.
Why the existing probe could not catch it
It proved reachability, and at that moment reachability is satisfied by the stream — the very
transport the test erases three lines later. So it was green whether or not the claim had landed.
It is also adversarial to the claim it appeared to prove: it posts from the other silo in a loop,
and each failed directed call mints a throw-away activation there that the owner's next
Attachmustbounce off, restarting the backoff. That is why this reproduced by luck rather than by tree — the
issue's own control arm (two heads differing only in a string-assertion test in another assembly).
Measured, not inferred
With one bounce forced and the claim backoff pinned long, the old probe goes green in under a
second while
PodHubClaimSettledhas demonstrably not completed — 6 runs, 6 times.I deliberately did not commit that experiment. Its setup depends on an activation-lifetime
ordering I cannot enforce (whether the throw-away activation is still alive when
Attacharrives),so as a permanent test it would risk becoming the next flake — the exact thing this PR is fixing.
It belongs in the record, not in the suite.
The fix
Wait on
PodHubClaimSettled— the positive signal for "the claim stopped attempting".OrleansCrossSiloReplyTestalready waits on it before its own directed cross-silo delivery, with thecomment "Claimed ⇒ a cross-silo directed delivery lands"; this brings the two into line. A
settle-by-silence poll is explicitly not an acceptable substitute (see the seam's remarks: the retry
hops the thread-pool scheduler, so a poll reads mid-hop).
SiloHostedHub_ReceivesDelivery_EvenWithItsStreamSubscriptionGonecarries the same latent gap —it waits for the stream subscription, a different condition that merely happens to take similar
time — so it gets the same wait. Two tests, one cause.
No production code changes. Falling back and then saying so loudly is what the platform is
designed to do during the claim window; the defect was in what the test asserted without checking.
What this rules out
Per the issue's own history, and re-checked here rather than assumed:
PostFailurenever touchesHierarchicalRouting),different precondition (nothing is shutting down here), different waiter.
684cc6ac1does not fix it.ShuttingDownverdict — that needs a shutting-down pod hub, which this testdoes not create.
Verification
-c Release -warnaserror,0 Error(s)onMeshWeaver.Hosting.Orleans.TestandMeshWeaver.Documentation.Test. Tests: 20 passed acrossPodHubTransportTest,PodHubClaim*andOrleansCrossSiloReplyTest;DocumentationLinkIntegrityTestpassed.The rule is written up in
Doc/Architecture/OrleansTestRoutingPattern→ "Reachability is not aclaim", so the next test that isolates one transport to assert another finds it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RpJi9EZHtGYQCahUwWa7jH