Repository navigation
Owner-side patch ack watcher has no completion arm: a stream that ends without emitting leaves a cross-hub write unacknowledged #3033
Description
What is failing
The owner-side ack watcher for a cross-hub PatchDataRequest subscribes with onNext and onError only. If the stream it watches completes without emitting, no arm runs and no acknowledgement is ever posted — the writer then waits out its full 31 s confirmation window and reports OwnerUnreachable, for a patch the owner may already have committed.
src/MeshWeaver.Data/DataExtensions.cs, in ApplyMeshNodePatchInTurn:
var postSub = stream
.Where(c => ChangeContainsStampedWrite(
c.Value, Volatile.Read(ref stampedId), Interlocked.Read(ref stampedVersion),
idKey, versionKey, jsonOpts))
.Take(1)
.Timeout(TimeSpan.FromSeconds(20))
.Subscribe(
committed =>
{
var flush = hub.ServiceProvider.GetService<IPostCommitFlush>();
if (flush is null) { AckOnce(true); return; }
var flushSub = flush.Flush(committed.Value!)
.Take(1)
.Timeout(TimeSpan.FromSeconds(10))
.Subscribe(_ => AckOnce(true),
ex => AckOnce(false, ClassifyPatchException(ex, hubPath)));
hub.RegisterForDisposal(flushSub);
},
ex => AckOnce(false, ClassifyPatchException(ex, hubPath))); // <- no onCompletedOn completion-without-emission: Take(1) ends empty → Timeout(20s) is cancelled, so it never fires → neither onNext nor onError runs → AckOnce is never called. The request is accepted and silently unanswered.
Why it is reachable
SynchronizationStream.Dispose completes its store (Store.OnCompleted()) rather than disposing it, and mirror eviction (ReclaimIfUnheld) can dispose an evicted mirror while the hub itself is still alive. RegisterOwnerDisposingNack — registered immediately after postSub, deliberately so the composite disposes the watcher first — covers the case where the hub is disposing. It does not cover a stream that completes while the hub lives.
Why this is the owner-side twin of #3001 / #3020
#3020 fixed exactly this defect on the writer side: UpdateRemote's base read was subscribed without a completion arm, so a read that completed empty settled nothing and posted nothing. The fix was RequireBaseState — DefaultIfEmpty(null) + an explicit throw, so "the observable ended without a value" becomes a verdict instead of silence.
The same reasoning applies here, one hub over. An observable that can complete without emitting must produce a verdict on that path, or the request it was serving is lost.
🚨 The naive fix is wrong
A bare onCompleted => AckOnce(false, …) would NACK writes that are about to succeed: on the successful path Take(1) emits and then completes immediately, while the inner flush.Flush(...) subscription is still in flight. AckOnce latches (Interlocked.Exchange(ref ackPosted, 1) != 0 → no-op), so the completion arm would win the race against the flush's own AckOnce(true).
The correct shape is a completion arm guarded on "the commit echo was never observed" — e.g. a local flag set in onNext, read in onCompleted, and only then AckOnce(false, …) with an error that names the condition (the stream ended before the write's commit echo arrived) rather than a generic timeout.
The sibling generic path at DataExtensions.cs (stream.Take(1).Subscribe(change => { … }), in ApplyJsonMergePatchAndUpdate) has neither an error nor a completion arm and should be assessed in the same change.
How it presents
Indistinguishable from a slow owner, from the caller's side: MeshNodeStreamException with MeshNodeErrorCode.OwnerUnreachable after ~31 s. It is separable from the "owner answered with its own timeout" shape by the error code — see Doc/Architecture/ReadingAWriteVerdict.
Provenance
Found while re-measuring #2896 against #3009 / #3020 (see #2896 (comment)). It is not what #2896's recorded occurrence was — that one carried Code = Unknown with the owner's own TimeoutException, i.e. the owner did answer. This is a distinct, code-provable route to the same user-visible outcome, filed on its own rather than patched blind inside an unrelated change set.
No test exists for it yet; a fix should carry one that drives a reduced stream to completion without a stamped-write emission while the owner hub stays alive.
Activity
rbuergi commentedon Sep 2, 2026
The gap is in TWO subscriptions, not one — and it is the owner-side twin of #3001
Analysis and a building fix are on branch
fix/3033-ack-completion-arm(72e31a2a8), deliberately not opened as a PR: it has no test yet, and the house standard is a test that would have failed before. Recording the work so it is not re-derived.ApplyMeshNodePatchInTurnsubscribes withonNextandonErroronly — two of Rx's three terminations — in two places:postSub.Skip(1).Take(1)needs two emissions; a stream that ends before the commit is observed completes emptyflushSubThe issue named the first. The second has the identical defect and would have been left behind by a one-line fix.
This is a known class, already written up on the other side of the wire
(#3001, the writer side) states it exactly:
#3001 fixed the writer half. This is the owner half of the same mistake.
The verdict chosen, and why
MeshNodeErrorCode.OwnerDisposingrather than a new code: a stream ending under a live patch is the owner's stream going away, which is what that code means, and it is documented TRANSIENT and retry-worthy — so the resubscribe latch rides it out (#2986) instead of tearing down. "Unknown, retry-worthy" is the honest verdict here: the merge may or may not have committed, and the caller must be free to retry rather than left waiting on silence.Safe by construction:
AckOnceis anInterlockedgate, so a real ack that already won makes the new arm a no-op — which is what makes adding a third arm safe rather than a race.What is needed to land it
Follow #3001's own remedy: extract the arming into a pure function (as
MeshNodeStreamHandle.RebaseSourcewas), so an empty completion is testable withObservable.Empty<T>()the wayWriteBaseStateTotalityTestdoes. Then pin: an empty completion yields a terminal; the normal emit path is unchanged; the error path still faults; and exactly one terminal is posted per path — that last one is the real invariant.#2896 closes with this. Its own re-measurement already falsified its title — "This was NOT a
VERDICT_TIMEOUT. The owner DID produce a terminal" — the ack was simply never sent.rbuergi commentedon Sep 2, 2026
Correction to the comment above — one sentence lost a word to my own shell. The line reading
should read
I wrote that comment with an unescaped backtick inside a double-quoted
gh issue comment --body, so the shell ran the test's name as a command (command not found: WriteBaseStateTotalityTest) and substituted the empty result. Everything else in the comment is intact; only that identifier vanished.Worth naming because it is a repeatable trap and I have hit it before: markdown for
ghgoes through--body-file, never--body "…". Backticks in a double-quoted argument are command substitution, and the failure is silent in the direction that matters — the comment posts successfully, reads almost right, and quietly drops exactly the identifier a reader needs to find the file.The file is
test/MeshWeaver.Graph.Test/WriteBaseStateTotalityTest.cs, and it is worth reading before fixing this issue: it documents this defect class on the writer side of the wire, and its remedy (extracting a pure function so an empty completion is testable withObservable.Empty<T>()) is the pattern this fix should follow on the owner side.rbuergi commentedon Sep 2, 2026
Ownership moved, and there is an ordering hazard for whoever writes the tests.
The WIP branch
fix/3033-ack-completion-arm(72e31a2a8) adds the completion arm to bothpostSubandflushSubinsrc/MeshWeaver.Data/DataExtensions.cs. I pushed it and deliberately opened no PR: it has no test, and a completion-arm fix without one is exactly the shape that regresses silently. Another session's agent is now cherry-picking it and adding the tests. I am not touching that branch again.🚨 The hazard: this issue overlaps #3058, and #3059 changes the ordering the tests would encode.
#3059 moves
SubscribeAckoffHandleSubscribeRequest— where it was posted the instantSubscribeToClientreturned, i.e. before the snapshot answering the subscribe existed — and onto the update turn'sappliedcallback, so the ack follows the frame it acknowledges. The ack's meaning changes with it: it now asserts "your snapshot has been sent", which is what the subscriber's give-up already read it as.So a test written against today's main would pin the pre-#3059 ordering — the behaviour being removed — and would then fail or, worse, quietly assert the defect. Write the tests against the post-#3059 ordering, or land them after #3059 is on main. #3059 is green on its required check and its review is resolved.
Related but distinct: this issue is the completion arm (a stream that ends without a terminal leaves the waiter hanging); #3058 is the ordering of the acknowledgement. Both produce a waiter that never resolves, which is why they are easy to conflate — the fixes are independent and both are needed.
Merge pull request #3070 from Systemorph/fix/3033-patch-ack-completio…
rbuergi commentedon Sep 2, 2026
Fixed by #3070 (merged today). Both owner-side ack subscriptions (postSub and flushSub) now carry a completion arm guarded on 'the commit echo was never observed'; an empty completion is a prompt OwnerDisposing NACK the writer retries, never a 31 s OwnerUnreachable. Likely also the owner-side silence behind #2896 (left open until re-measured on a deployed build).
🤖 Generated with Claude Code
rbuergi commentedon Sep 2, 2026
🚨 #3070 (this issue's fix) is the first bad commit for a trunk-wide red in MeshWeaver.Plugins.
Bisected today with the plugins tree held fixed at its
main(3993c83f) and only$(MeshWeaverRoot)moving — two samples at each adjacent commit:67b0f1947(#3068, the parent)9b3b00201(#3070)Failing, on
MeshWeaver.Hosting.Monolith.Test(in MeshWeaver.Plugins):Both script the owner-disposal interleaving the completion arm changed. The first now gets its NACK
and its re-enqueue, and then the retry is refused by the same dying activation
(
Hub TestData/late-nack-node is shutting down (RunLevel=DisposeHostedHubs, activation #030DAE56))rather than landing on the fresh one — the claim in the test's own name. The second wedges the SYNC
hub:
Green runs finish in 17 s, red ones in 55 s, so the two states are not adjacent.
Why core CI could not see it. #3070's own suite,
PatchAckTotalityTest, is a pure compositiontest with no mesh — correctly so, and it is green. The behaviour change is only observable to a
suite that BOOTS a mesh, and since #2276 every one of those lives in MeshWeaver.Plugins, which
core's gates do not run. So this merged green here and turned
Systemorph/MeshWeaver.Plugins#1184 red on thirteen unrelated PRs and on the required
Build + test the portal hostscontext.Full bisect table, both failure logs and the exact repro command are on
Systemorph/MeshWeaver.Plugins#1184.