Repository navigation
fix: a detached upsert reply can no longer go silent on an empty completion (#3674) - #3915
Conversation
…letion (#3674) `CreateOrUpdateNodeRequest` returns `Processed()` at once and owes its response from chains that outlive the turn. Three of those chains subscribed with two of Rx's three terminations, so a source that COMPLETED EMPTY ran neither arm: no reply posted, no fault logged, and the caller left to wait out its whole budget on a delivery trail that reads `HANDLER_ENTER … → HANDLER_EXIT state=Processed`. That is #3674's surviving shape. Its filed premise does not survive — the `@` in a request-fate entry names the hub that RECORDED the stage, not the delivery's target, so two of the three "forwarded, not mine" verdicts were correct forwards, and the 8055-vs-8071 regression framing is falsified because both commits carry the same code in the respect that matters. What is left, and what is real, is a sanctioned lifecycle request whose caller receives no terminal at all. The reachable instance is the NO-OP PROBE, i.e. a RE-INSTALL: identical content takes the no-op branch, which asks `GetEffectivePermissions(path, requestedBy)` whether the caller could have written anyway. That probe terminating without emitting is a documented outcome — half of what `HubPermissionExtensions` classifies as `Undetermined`, "because it FAULTED, or because it terminated without ever emitting (#2742)". Against a two-arm Subscribe it posted nothing. - `DetachedReplyOutcome.Of` composes value / fault / empty into EXACTLY ONE emission, so a detached-reply leg cannot be silent whatever its source does — totality by construction rather than by remembering the third arm at each site (the discipline that failed: `MeshExtensions` states the rule twice in its own comments and still left three legs on two arms). - No-op probe: silence falls through to the ordinary write path, exactly as the FAULT arm already did — a non-verdict is neither a denial nor a grant, and answering "skipped" on unproven authority would be a success WITHOUT a write. - NodeType gate: silence refuses and SAYS the probe produced no answer, never "not registered". - Update leg (`WriteThroughStream`): silence refuses and says the write is not confirmed and the outcome unknown — a missing reply never proves rollback. No bound moves, nothing is retried, nothing is swallowed: a termination that previously reached no handler now reaches one. Control, both directions, Release: - fix reverted, test kept → RED in 36 s: "Expected the observable to emit a value within 36s … but it did not. The observable emitted nothing at all." - fix re-applied → GREEN in 0.7 s. The lever is the framework's own `WithPermissionEvaluator` seam narrowed to one user id, not a mock: the create, the access-control pipeline and the cache's own write gate (which probes for the CALLER's identity) all run unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
The control run with the fix backed out emits the SAME `[STALE-CALLBACK] … CreateOrUpdateNodeRequest@portal/nodeops-…` line the issue quotes, and the same interleaved `ROUTED onTarget=False state=Forwarded` entries that were read there as "forwarded by every hub, handled by none". The trail also carries what settles that reading: `ROUTED onTarget=True`, `HANDLER_ENTER`, the leg's own stage, `HANDLER_EXIT state=Processed`, nothing. A trail stopping at a leg's stage is a leg that answered nobody; it is distinguishable from a pump that never turned only by whether `HANDLER_ENTER` is present — which is why each leg records a stage at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes silent CreateOrUpdateNodeRequest completions by converting empty observables into explicit outcomes.
Changes:
- Adds
DetachedReplyOutcomehandling for upsert reply paths. - Adds a regression test for silent permission probes.
- Updates architecture documentation and What’s New.
File summaries
| File | Description |
|---|---|
| test/MeshWeaver.Graph.Test/UpsertAnswersWhenThePermissionProbeIsSilentTest.cs | Updated as part of this pull request. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Updated as part of this pull request. |
| src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs | Updated as part of this pull request. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-a-re-import-of-unchanged-content-can-no-longer-go-quiet.md | Updated as part of this pull request. |
| src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md | Updated as part of this pull request. |
Review details
Suppressed comments (4)
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-10-a-re-import-of-unchanged-content-can-no-longer-go-quiet.md:45
- The same overstatement appears in this user-facing entry: the read and create legs were not converted to
DetachedReplyOutcome.Of; they still depend on explicitonCompletedarms. Saying the whole class is closed “by construction” misdescribes the guarantee and can lead future changes to remove the remaining handlers. Distinguish the three wrapped legs from the two already-manual ones.
Every other place the same request can hand its reply to background work was given the same
treatment, so this class of silence is now closed by construction rather than by remembering to
handle it: each one produces an outcome on every path, and an outcome always gets answered. Where the
src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs:79
Ofis the totality boundary for three independent detached-reply legs, but the added test only drives the permission-probe leg. There is no regression test that makesNodeTypeResolution.Resolvesor the mesh-nodeUpdatecomplete empty and checks their distinct refusal responses; removing either wrapper would leave those paths silent while this test stayed green. Add focused coverage for both remaining legs (or a direct helper test plus branch assertions).
=> source
.Take(1)
.Select(value => new DetachedReplyOutcome<T>(true, value, null))
.Catch((Exception ex) => Observable.Return(new DetachedReplyOutcome<T>(false, default, ex)))
.DefaultIfEmpty(new DetachedReplyOutcome<T>(false, default, null));
src/MeshWeaver.Mesh.Contract/MeshExtensions.cs:4845
- The local
Catchconverts every permission-probe fault intofalsebeforeDetachedReplyOutcome.Ofsees it, so theoutcome.Errorbranch below is unreachable for the source this wrapper is meant to make total. Probe failures therefore remain silently downgraded to the write path, while the surrounding comments claim the exception is carried for logging. Remove this local catch (the outcome error branch already logs and takes the same write path), or otherwise preserve the exception.
.Catch((Exception _) => Observable.Return(false)))
test/MeshWeaver.Graph.Test/UpsertAnswersWhenThePermissionProbeIsSilentTest.cs:79
- This test waits on
TestTimeouts.Convergence, but the assembly-wide xUnitmethodTimeoutis 30 seconds whileConvergenceis CI-scaled beyond that (andTestTimeoutsexplicitly providesTestMillisecondsfor the outer bound). Without an explicit larger timeout, the pre-fix silent completion can be killed by xUnit before the assertion reports the intended empty-emission failure, producing an anonymous timeout instead of exercising the regression.
[Fact]
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| different route. The legs are now composed through `DetachedReplyOutcome.Of`, which converts value / | ||
| fault / empty into exactly one emission, so a leg cannot be silent whatever its source does: |
There was a problem hiding this comment.
Correct, and fixed. The sentence claimed the construction for all five legs when only three use the helper — the read leg and DispatchInnerCreate carry their own explicit third arm. Reworded to what is true: "Every leg now handles all three of Rx's terminations — the read leg and the inner create by their own explicit third arm, the other three by composing through DetachedReplyOutcome.Of". The per-leg table was already accurate; the introduction was not.
| /// Turns any source a detached reply hangs off into a source that emits EXACTLY ONE | ||
| /// <see cref="DetachedReplyOutcome{T}"/> and completes. See that type for why. |
There was a problem hiding this comment.
Correct, and fixed — this is the more important of the two. Totality and liveness are different properties and the summary conflated them. Of is now documented strictly: if the source terminates, exactly one outcome is emitted. A source that never emits AND never completes reaches no arm at all, and no composition over a source can invent a termination it never produces. The remarks now say so explicitly, name the #2742 starvation shape, and state that a leg whose source can starve still owes its own Timeout — pointing at the two that already carry one (InnerCreateVerdictBound on the inner create, NodeOpForwardTimeout inside the no-op probe), and saying that composing through this type neither adds nor excuses that bound. The doc page carries the same caveat, pointing at its own "The FOURTH outcome — never emits, never completes" section.
| NodeTypeResolution.ProbeFailedMessage(node.Path, node.NodeType!, | ||
| new InvalidOperationException( | ||
| "the existence probe completed without producing an answer")), | ||
| NodeUpsertRejectionReason.Unknown); |
There was a problem hiding this comment.
Real, pre-existing, and filed rather than half-done: #3917.
Measured: PostFail in this handler has no key parameter at all, and all 16 of its call sites pass an English literal — "Node path and Id must not be empty", "Inner UpdateNode faulted: …", RejectionMessage/ProbeFailedMessage, the create-timeout refusal, the mis-scoped-grant refusal. PostOk is keyed; PostFail is not.
🚨 Keying only the three messages this PR adds would deepen the inconsistency rather than reduce it: the same dialog would then show German for "the probe produced no answer" and English for "the probe faulted" — worse than uniformly English. The real fix is PostFail gaining a key + arguments, 16 sites keyed, 16 keys in both catalogs, an explicit decision about whether the wire field CreateOrUpdateNodeResponse.Error is itself localized (it is consumed by non-GUI callers), the React mirror sync handed over, and the same treatment for CreateNodeResponse.Fail so the create half does not stay English. That is #3917's scope, written out there.
| $"The update of '{node.Path}' completed without producing a result " | ||
| + "or a fault, so the write is NOT confirmed. The outcome is " | ||
| + "unknown: the owner may have committed it without acknowledging. " | ||
| + "Check the node's state before retrying.", | ||
| NodeUpsertRejectionReason.Unknown); |
There was a problem hiding this comment.
Same finding as the thread above, and the same disposition — tracked as #3917. Your point about CreateOrUpdateNodeResponse.Error being relayed verbatim by IMeshService.CreateOrUpdateNode is the part that makes it more than a transcript question, and it is called out explicitly in that issue as a decision that has to be made rather than assumed: the Error field is a wire value read by non-GUI callers, so "localize the LogMessage, keep Error English as the fallback" and "localize both" are genuinely different designs, and the same choice faces every *Response.Error in the mesh.
Test Results (shard 3)389 tests 389 ✅ 54s ⏱️ Results for commit f9e7a5d. |
Test Results (shard 1)496 tests 496 ✅ 42s ⏱️ Results for commit f9e7a5d. |
Test Results (shard 0)254 tests 254 ✅ 2m 9s ⏱️ Results for commit f9e7a5d. |
Test Results (shard 4)1 889 tests 1 889 ✅ 3m 21s ⏱️ Results for commit f9e7a5d. |
Test Results (shard 2)1 220 tests 1 218 ✅ 3m 42s ⏱️ Results for commit f9e7a5d. |
Test Results (shard 5) 6 files 6 suites 4m 47s ⏱️ Results for commit f9e7a5d. |
Test Results 17 files 17 suites 15m 36s ⏱️ Results for commit f9e7a5d. |
Fixes #3674.
The filed premise does not survive — the shape does
#3674 reads its trail as "forwarded by the node hub, by the mesh hub and by the nodeops hub — each says 'not mine' — and no handler ever runs". Its own thread falsified that: the
@in a request-fate entry names the hub that RECORDED the stage, not the delivery's target, so two of those three verdicts were correct forwards. The 8055-vs-8071 regression framing is falsified too — both commits carry the same code in the respect that matters, and a green control on racy code localises nothing.Re-measured today (2026-09-10): the falsifying observation the thread was waiting for is still unavailable. MeshWeaver.Reinsurance's pin moved past the control (Reinsurance#184, merged 09-09 15:48Z) and its gate now resolves
3.0.0-ci.8261/ core47d94201e— but everyReinsurance Plugins CIrun today dies before the render step, atupstream 'plugins' has no SEALED publication … for framework identity s6c3644c091f6675035c9cfaf59453d21(run 34467892111).Ifrs17/AocConfigurationhas not been re-rendered on any platform ≥ 8071. Nothing here changes that; it is an upstream-publication blocker, not this defect.What survives every re-reading is the shape, and it is the thing this PR fixes: the caller of a sanctioned lifecycle request received no terminal at all — the write was neither applied nor refused.
Root cause
HandleCreateOrUpdateNodeRequestreturnsProcessed()immediately and owes itsCreateOrUpdateNodeResponsefrom chains that outlive the turn. Three of those chains subscribed with two of Rx's three terminations, so a source that completes empty ran neither arm — no reply, no fault, no log line, and a delivery trail readingHANDLER_ENTER … → HANDLER_EXIT state=Processedwhile the caller waits out its entire budget.The reachable instance is the no-op probe, i.e. a re-install: identical content takes the no-op branch, which asks
hub.GetEffectivePermissions(path, requestedBy)whether the caller could have written anyway. That probe terminating without ever emitting is a documented outcome, not an Rx corner — it is half of whatHubPermissionExtensionsclassifies asUndetermined, "because it FAULTED, or because it terminated without ever emitting (issue #2742)". CD 7950's[FAIL] Chess … idempotence: re-install failedis an idempotent re-install; that it took this branch is a fit, not a measurement, and the PR says so.The file states the rule twice in its own comments (#2454, "THREE terminal states, not two") and still left three legs on two arms — which is why the fix is composition, not another remembered arm.
The fix
DetachedReplyOutcome.Ofmaps value / fault / empty into exactly one emission, so a detached-reply leg cannot be silent whatever its source does — totality by construction.SkipNoOpIfAuthorized)ApplyUpdateViaStream)WriteThroughStream)🚨 No band-aid: no bound moves, nothing is retried, nothing is swallowed, no watchdog is added.
Catchhere does not resume a sequence — it converts a fault into a terminal the caller is told about, and carries the exception through for the site to log. The only change is that a termination which previously reached no handler now reaches one.Control, both directions (Release,
-warnaserror, full rebuild each way)src/fix reverted, test kept → RED in 36 s:🚨 The reverted run reproduces #3674's own artefact, not merely its symptom — same
[STALE-CALLBACK] … CreateOrUpdateNodeRequest@portal/nodeops-…line, same interleavedROUTED onTarget=False state=Forwardedentries that were read there as "forwarded by every hub, handled by none":The forwards are correct (both hubs declined a delivery targeted at
portal/nodeops). What names the defect is what follows on the target:onTarget=True→HANDLER_ENTER→ the leg's stage →HANDLER_EXIT state=Processed→ nothing. A trail stopping at a leg's stage is a leg that answered nobody; it is distinguishable from a pump that never turned only by whetherHANDLER_ENTERis present.UpsertAnswersWhenThePermissionProbeIsSilentTestuses the framework's ownWithPermissionEvaluatorseam narrowed to one user id — not a mock: the create, the access-control pipeline and the cache's own write gate (which probes for the CALLER's identity, not the probe id) all run unchanged.Also green locally:
MeshWeaver.Graph.Testupsert/create/dangling/release-wave filter (15/15),MeshWeaver.Documentation.Testin full (368/368, includingDocumentationLinkIntegrityTestandSubscribeErrorArmRatchetGuard), andMesh.Contract/Hosting/Graph/Mesh.Operations/Documentationeach0 Error(s)in Release.Maintainer decision left
#3674 should not be closed on this PR alone. This closes one measured, reproducible instance of its shape. Two things it deliberately does not do, both still open:
portal/nodeopsenqueued the delivery and never took the turn (🚨 3.0.0-ci.8071: a CreateOrUpdateNodeRequest for an adopted NodeType's own path is Forwarded by every hub and handled by none — Reinsurance's Ifrs17/AocConfiguration render gate times out (ok on 8055) #3674's own comment of 09-09, and Bake wedge: portal/nodeops stops draining mid-bake — one shared node-CRUD hub seen from eight places (queue latency bimodal: <=3.2s vs 33-49s) #2543 / Hub disposal wedges before Quiescing: the action block never dequeues the buffered ShutdownRequest(Quiescing) #3593). Nothing about assembly identity or handler selection is consulted before a turn starts, so a pump that does not turn is a separate defect — and the disposal-stall verdict from fix(#3593): a failed drain schedule must not latch the pump, and the verdict must measure its mechanism #3798 only prints for a hub that is stalled while disposing, never one merely stalled while alive. That gap is named in the thread and is not closed here.upstream 'plugins'publication is sealed for the current framework identity. Per the thread's own standing caution, a single green gate afterwards would confirm nothing; only a recurrence (falsifying) or a long clean run moves it.Work product conserved:
Doc/Architecture/WriteVerdictTotalitygains the corrected per-leg table (the old one judged "covered" by "does it survive the owner being disposed under it?" — a leg can answer yes to that and still answer nobody on an empty completion), the #3674 section, and the honest fit-not-measurement caveat. What's New entry added,Category: Fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7