Repository navigation
The upsert's inner create registered its callback after posting, so the reply could be dropped - #1083
Merged
Merged
Conversation
…he reply could be dropped
`CreateOrUpdateNodeRequest`'s `DispatchInnerCreate` posted a self-targeted
`CreateNodeRequest` and only THEN registered the response subject via the
`Observe(delivery)` overload. `MessageHub.HandleCallbacks` drops any response whose
correlation id has no registered subject ("No subject found for response … treating as
processed"), so a reply that lands in that window is consumed and gone — the upsert then
never answers and the caller's callback stays pending until the teardown quiescing budget
reports it as a leaked `CreateNodeRequest@mesh/<self>`, which is exactly the #981
signature.
The window is not theoretical here. `DispatchInnerCreate` runs OFF the hub action block —
it is the continuation of `persistence.Read(...)` — so `KickDrain` finds `draining ==
false` and schedules the create's turn onto `turnScheduler`, on another thread, before
`Post` even returns (`PostImplGeneric` calls `ScheduleNotify` synchronously). Preempt the
posting thread and the turn can answer first.
This is the same defect the codebase has already fixed at four other sites, with the rule
written down in `MeshNodeStreamExtensions` and proven deterministically by
`GetMeshNode_WarmOwner_DropsResponse_WhenSubjectRegisteredAfterPost`: always the
pre-registering `hub.Observe(request, options)`, never `hub.Post` then
`hub.Observe(delivery)`.
It also destroyed the evidence. `RequestFateLedger` starts a trail at REGISTRATION, so the
post-hoc overload lost POSTED / RECEIVED / ENQUEUED / ROUTED and left the CI capture
reading "AWAITING → REGISTERED_AFTER_POST" and nothing else — the trail the ledger itself
annotates with "re-run with the caller switched to Observe(request, options), which
registers first".
Adds `UpsertInnerCreateObservationTest`: the drop is pinned deterministically on the exact
#981 message shape (a self-targeted `CreateNodeRequest` on the mesh hub) with both
orderings side by side, fenced on node readability plus two round-trips rather than a
sleep, and the upsert seam is guarded end to end.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GetMeshNode answers null and COMPLETES for a node that is not there yet, so awaiting it raced the create instead of fencing behind it. Re-query on an interval until the node is readable, then take the two round-trips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AsynchronousCalls.md presented `Observe(delivery)` and `Observe(request, options)` as interchangeable, and carried a worked example titled "When you already have a delivery" that is the defect verbatim. That is why this bug has now been written five times. Rule 3a states the asymmetry: the pre-registering overload always; `Observe(delivery)` only when the response provably cannot have arrived, which "the delivery is already posted" is not. The Rules Summary row and the RegisterCallback migration row now say the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race in CreateOrUpdateNodeRequest where the inner self-targeted CreateNodeRequest could post its response before the response subject was registered, causing the response to be dropped and leaving a callback pending until teardown (as reported in #981 / run 31363873251).
Changes:
- Switched the upsert’s inner create dispatch to the pre-registering
hub.Observe(request, options)pattern so responses can’t be dropped betweenPostandObserve. - Added a deterministic regression test that demonstrates the drop with post-then-observe ordering and verifies the safe pre-registered ordering.
- Updated async/reactive guidance docs to explicitly call out the unsafe pattern, and added a What’s New entry for the user-visible impact (“save could succeed but never confirm”).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/UpsertInnerCreateObservationTest.cs | Adds regression coverage pinning the dropped-response mechanism and verifying upsert always answers. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Fixes CreateOrUpdateNodeRequest inner create to pre-register the callback before posting. |
| src/MeshWeaver.Documentation/Data/Architecture/AsynchronousCalls.md | Documents that Observe(request, options) is required to avoid response drops; updates examples and rules summary. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-10-upsert-always-answers.md | Changelog entry describing the resolved “save completed but never reported back” symptom. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Test Results (shard 1)1 198 tests +3 1 198 ✅ +3 6m 7s ⏱️ -8s Results for commit f748e6d. ± Comparison against base commit df1ed76. This pull request removes 18 and adds 21 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Test Results (shard 2)1 967 tests - 1 1 965 ✅ ±0 5m 54s ⏱️ -40s Results for commit f748e6d. ± Comparison against base commit df1ed76. This pull request removes 19 and adds 18 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Contributor
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.
Fixes the
CreateNodeRequest@mesh/<self>leaked callback that reddened main inrun 31363873251 (
df1ed76e1,shard 2,
PandasExplorerLayoutAreaTest). Reopens and addresses #981.What the capture actually said
The test BODY passed;
DisposeAsyncthrew:The leaked request is identified. It is
CreateOrUpdateNodeRequest's inner create —MeshExtensions.DispatchInnerCreate— forDoc/…/PandasExplorer/_Activity/compile-state. The samecapture logs
[CreateOrUpdate] inner CreateNode faulted for <that path>at teardown, which is theOnErrorarm of that subscription firing as the hub tore down. And aCreateNodeRequestposted bythe mesh hub to itself was uniquely that call site: every other awaited
CreateNodeRequestinsrc/already used the pre-registering overload.The defect
HandleCallbacksdrops a response whose correlation id has no registered subject ("No subjectfound for response … treating as processed"). So a reply landing in that window is consumed and
gone — the upsert never answers and the callback is pending forever.
The window is not theoretical here:
DispatchInnerCreateruns off the hub action block — it is the continuation ofpersistence.Read(...)— soKickDrainfindsdraining == falseand schedules the create's turnonto
turnScheduler, on another thread, beforePostreturns (PostImplGenericcallsScheduleNotifysynchronously).This is the same defect already fixed at four other sites, with the rule written down in
MeshNodeStreamExtensionsand proven deterministically byGetMeshNode_WarmOwner_DropsResponse_WhenSubjectRegisteredAfterPost.It also erased the evidence:
RequestFateLedgerstarts a trail at REGISTRATION, so the post-hocoverload lost
POSTED/RECEIVED/ENQUEUED/ROUTEDand produced exactly the trail the ledgeritself annotates with "re-run with the caller switched to
Observe(request, options), whichregisters first." This PR is that re-run.
Fix
One call instead of two —
hub.Observe(inner, options), which registers theAsyncSubjectbeforeposting. Identity is unaffected:
inner.CreatedBypins it as a request FIELD, the post stampsinboundCtx,ApplyUpdateViaStreamopens its ownSwitchAccessContext(inboundCtx), andPostOk/PostFailareResponseForposts.Pin — deterministic, no sleep
UpsertInnerCreateObservationTestpins the drop on the exact #981 message shape (a self-targetedCreateNodeRequeston the mesh hub), with both orderings side by side so the arms are each other'scontrol:
NotEmit),The fence is node readability plus two complete round-trips, never a fixed delay — so it cannot
race. A second test guards the production seam end to end (an upsert of a missing node must answer).
And the reason it keeps being written
AsynchronousCalls.mdpresented the two overloads as interchangeable and carried a worked exampleheaded "When you already have a delivery" that is the defect verbatim. That is fixed too (rule
3a + the Rules Summary row + the
RegisterCallbackmigration row), which is the actual root cause ofthe recurrence.
Honest limit
The pin proves the mechanism and identifies this as the only call site that could produce a
CreateNodeRequest@mesh/<self>leak. It does not prove this particular CI capture went throughthat window — and it cannot, because the post-hoc registration is what destroyed the trail. If the
leak recurs after this lands, the trail will carry every stage and name the mechanism outright.
Known remaining post-then-observe site, deliberately out of scope:
MeshNodeStreamExtensions'UpdateRemote(post at ~1007, observe at ~1134). It is bounded byUpdateResponseWaitBoundwith anoptimistic-snapshot fallback, so a lost reply degrades rather than hangs — but it should be migrated.
Verification
dotnet build test/MeshWeaver.Hosting.Monolith.Test -c Release -warnaserror— 0 warnings, 0 errors.MeshWeaver.Hosting.Monolith.Testfull project: 515 passed, 0 failed, 4 skipped — includingPandasExplorerLayoutAreaTestandCreateOrUpdateNodeRequestTest.DocumentationLinkIntegrityTestgreen.main.Category: Fix).Not a fix for the shard-0
Hosting.Orleans.Testfailure in the same run — that is #1081, owned by#1073, and unrelated to this change.
🤖 Generated with Claude Code