Repository navigation
fix(#3986): the refusal names WHICH of the three ends the stream met - #4384
Merged
Merged
Conversation
`DataExtensions.RefuseUserAction` wrote one sentence for three causes — "disposed circuit, released read stream, or never-created sync hub" — so the incident fingerprint could not tell a DESIGNED refusal from a live defect. #3986 was filed on the client-side release ordering, fixed (1594bb3), deployed, and then reopened on 2026-09-14 by an occurrence the fix cannot reach: an owner-side per-node hub (rbuergi/Requests/provision-pearl-20260914) whose sync/{id} had never been registered on the activation that received the click. Same sentence, same fingerprint, different defect. The owner now records, per stream id and per HUB ACTIVATION, whether it ever registered a sync/{id} and whether an UnsubscribeRequest reached it (SyncStreamActivationLedger — an instance registered beside IWorkspace, never static, so its lifetime IS the activation it answers about). The refusal reads that and names the cause: · released by the subscriber — an UnsubscribeRequest for it reached this hub · reaped by the owner — served on this activation, never told to stop · never registered here — this activation never served that stream id Each is its own literal log TEMPLATE, not a parameter on a shared one, so the split holds however a reader derives a fingerprint. A fourth line covers the bounded ledger honestly: once it has pruned anything, an absent id is reported as "no record", with its own numbers, rather than as "never registered" — a full ledger must never masquerade as the third cause. Attribution only. Nothing retries, nothing waits, no bound moves, the Error level is unchanged and a lost action is still lost — the page says why that is deliberate. RefusalNamesWhichEndTheStreamMetTest produces each cause by its own real route and reads the refusals out of the host's own log; each asserts its sentence AND the absence of the other two. Reverting the attribution: Failed: 3, Passed: 1 — the positive control (an accepted action still RUNS and produces no refusal at all) stays green, which is the mutant a more descriptive blanket refusal would otherwise satisfy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Test Results (shard 1)519 tests 519 ✅ 40s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 0)263 tests 263 ✅ 2m 37s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 3)1 333 tests 1 333 ✅ 2m 55s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in classification, generation handling, coverage, and test teardown.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds activation-scoped diagnostics that identify why a synchronization stream ended, while preserving existing refusal behavior.
Changes:
- Tracks stream registration and unsubscribe history.
- Emits distinct refusal messages, including bounded-ledger diagnostics.
- Adds integration tests and documentation.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Layout.Test/RefusalNamesWhichEndTheStreamMetTest.cs |
Tests refusal attribution and successful actions. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-15-a-click-that-could-not-run-says-why-it-could-not-run.md |
Documents the user-facing diagnostic improvement. |
src/MeshWeaver.Documentation/Data/Architecture/RefusingALostUserAction.md |
Documents refusal causes and measurements. |
src/MeshWeaver.Data/SyncStreamActivationLedger.cs |
Tracks per-activation stream dispositions. |
src/MeshWeaver.Data/Serialization/SynchronizationStream.cs |
Records synchronization-hub registrations. |
src/MeshWeaver.Data/DataExtensions.cs |
Classifies and logs refused actions. |
Review details
Suppressed comments (2)
src/MeshWeaver.Data/DataExtensions.cs:638
- When
NoRecordcame from an ancestor inwalked,RefuseUserActionreceives only the enum and re-resolves the ledger for the originalhub. The{Aged}/{Held}values can therefore describe a different ledger (or0/0), while the message claims to report the ledger that established the uncertainty. Carry the source ledger or its counters throughClassifyLostStreamso the fourth diagnostic line has the promised denominator.
var ledger = SyncStreamActivationLedger.For(hub);
logger?.LogError(
"REFUSING {MessageType} on area {Area} for stream {StreamId} on hub {Address}: "
+ "this hub holds NO RECORD of the stream — its activation ledger has already "
+ "aged {Aged} disposition(s) out and holds {Held}, so 'never served here' and "
+ "'served and long since ended' cannot be told apart. The action did NOT run "
+ "and never will. Sender: {Sender}.{Age}",
messageType, userAction.ActionArea, streamMessage.StreamId, hub.Address,
ledger?.Pruned ?? 0, ledger?.Held ?? 0, request.Sender, age);
src/MeshWeaver.Data/DataExtensions.cs:306
- The unsubscribe is recorded only on the hub where
RouteStreamMessagehappened to run, but classification later walkswalkedand may find the stream's ledger on an ancestor. In that routed shape, a real subscriber release leaves the owner ledger unmarked and the later refusal is misclassified asReapedByOwner. Record the unsubscribe against the same walked hub/activation that can supply the classification (or otherwise carry the owning ledger through the route).
if (streamMessage is UnsubscribeRequest)
SyncStreamActivationLedger.For(hub)?.RecordUnsubscribeReceived(streamMessage.StreamId);
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+510
to
+529
| var verdict = LostStreamCause.NeverRegistered; | ||
| foreach (var candidate in walked) | ||
| { | ||
| switch (SyncStreamActivationLedger.For(candidate)?.Classify(streamId)) | ||
| { | ||
| case LostStreamCause.ReleasedBySubscriber: | ||
| return LostStreamCause.ReleasedBySubscriber; | ||
| case LostStreamCause.ReapedByOwner: | ||
| verdict = LostStreamCause.ReapedByOwner; | ||
| break; | ||
| case LostStreamCause.NoRecord when verdict == LostStreamCause.NeverRegistered: | ||
| verdict = LostStreamCause.NoRecord; | ||
| break; | ||
| // NeverRegistered from one ledger says nothing about the others; a hub with no | ||
| // ledger at all (no data plugin, or a closed DI scope) equally says nothing. | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| return verdict; |
Comment on lines
+114
to
+117
| dispositions.AddOrUpdate( | ||
| streamId, | ||
| _ => new Disposition(seq), | ||
| (_, existing) => existing); |
Comment on lines
+151
to
+154
| // Absent. Definitive only while nothing has aged out — see the class remarks. | ||
| return Interlocked.Read(ref pruned) == 0 | ||
| ? LostStreamCause.NeverRegistered | ||
| : LostStreamCause.NoRecord; |
Comment on lines
+284
to
+295
| public override async ValueTask DisposeAsync() | ||
| { | ||
| clicked.Dispose(); | ||
| logRecords.Dispose(); | ||
| await base.DisposeAsync(); | ||
| } | ||
|
|
||
| /// <summary>Publishes every record the host produced into the owning test's instance subject.</summary> | ||
| private sealed class SubjectLoggerProvider(IObserver<(LogLevel, string)> sink) : ILoggerProvider | ||
| { | ||
| public ILogger CreateLogger(string categoryName) => new SubjectLogger(sink); | ||
| public void Dispose() { } |
Contributor
Test Results (shard 4)1 641 tests 1 641 ✅ 4m 45s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5) 4 files 4 suites 4m 57s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 2)1 636 tests 1 636 ✅ 9m 0s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
Contributor
Test Results 14 files 14 suites 24m 57s ⏱️ Results for commit e46d389. ♻️ This comment has been updated with latest results. |
rbuergi
added this pull request to the merge queue
Sep 15, 2026
…tream id is judged on its second life
Six findings from the automatic review, all acted on. Four were real defects in
the attribution itself — each one a way the new sentence could be confidently
WRONG, which is the only failure mode that matters for a diagnostic.
1. A registration KEPT the previous disposition for the same stream id
(AddOrUpdate … existing). A stream id is deliberately reused across a
re-subscribe, so the second life's owner-side reap would have been reported
as the first life's subscriber release — the same conflation, one level
down, and invisible because both sentences are plausible. It now REPLACES,
which also stops repeated registrations ageing the live entry out early.
Pinned by AStreamIdReusedAfterAReleaseIsJudgedOnItsSecondLife.
2. An UNRESOLVABLE ledger read as "never registered". `For()` answers null for
a closed DI scope — routine on the teardown path a refusal fires from — and
the fold left the verdict at NeverRegistered, i.e. printed the fingerprint
this change exists to make trustworthy on no evidence at all. A searched hub
with no ledger is now skipped, and when the REFUSING hub's own ledger cannot
be resolved the answer is its own fifth sentence, with no numbers in it
because there are none.
3. The NoRecord line re-resolved the refusing hub's ledger for its {Aged}/{Held}
numbers, which on a routed shape need not be the ledger that established the
uncertainty. ClassifyLostStream now returns the GROUNDING ledger with the
verdict, so the numbers belong to the answer.
4. The unsubscribe was marked only on the hub RouteStreamMessage ran on, while
classification walks the ancestor chain — so a release could be reported as
an owner reap. It is now marked across the same self-and-ancestors chain,
with the same self-parent guard. Marking creates nothing, so a hop that never
served the stream costs one dictionary miss.
5. The bounded-ledger branch was not exercised. ActivationLedgerCapacity joins
SyncStreamOptions (default 512) so a test can reach that answer with three
streams instead of six hundred: AnAgedOutStreamSaysSoRatherThanClaimingIt-
WasNeverServed asserts the fourth sentence, its numbers, and the ABSENCE of
the third cause's.
6. The log sink was disposed BEFORE the base teardown, which logs while it
drains the mesh — ReplaySubject.OnNext then throws inside a logger call.
Disposal moved into a finally after base.DisposeAsync().
Negative control re-run after rebuilding the test project: Failed: 5, Passed: 1
— every cause test red on its own assertion, the positive control still green.
Layout.Test 504/504, Data.Test 519/519, Documentation.Test 469/469.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi
removed this pull request from the merge queue due to a manual request
Sep 15, 2026
Contributor
|
ℹ️ Merge-queue steward: no action — removed from the queue with reason |
rbuergi
enabled auto-merge
September 15, 2026 08:29
Contributor
Author
All six review findings acted on —
|
| # | finding | what it would have printed | fix |
|---|---|---|---|
| 1 | a registration KEPT the previous disposition for the same stream id | a reused id's second life ending by an owner reap, reported as the first life's subscriber release — both sentences plausible, so invisible | registration REPLACES; also stops repeated registrations ageing the live entry out early. Pinned by AStreamIdReusedAfterAReleaseIsJudgedOnItsSecondLife |
| 2 | an unresolvable ledger read as "never registered" | NO sync hub … EVER registered on no evidence at all — a closed DI scope is routine on the teardown path a refusal fires from | a searched hub with no ledger is skipped; when the REFUSING hub's own ledger cannot be resolved the answer is its own fifth sentence, with no numbers in it because there are none |
| 3 | the NoRecord line re-resolved hub's ledger for {Aged}/{Held} |
numbers from a ledger other than the one that established the uncertainty | ClassifyLostStream returns the grounding ledger with the verdict |
| 4 | the unsubscribe was marked only on the routing hub, while classification walks the ancestor chain | a real subscriber release reported as an owner-side reap | marked across the same self-and-ancestors chain, same self-parent guard. Marking creates nothing, so a hop that never served the stream costs one dictionary miss |
| 5 | the bounded-ledger branch was not exercised | a safeguard nothing tests | SyncStreamOptions.ActivationLedgerCapacity (default 512) makes it reachable with three streams instead of six hundred — AnAgedOutStreamSaysSoRatherThanClaimingItWasNeverServed asserts the fourth sentence, its numbers, and the ABSENCE of the third cause's |
| 6 | the log sink was disposed before base teardown, which logs while it drains the mesh | ReplaySubject.OnNext throwing inside a logger call during teardown |
disposal moved into a finally after base.DisposeAsync() |
Re-measured (test project rebuilt between runs, per the stale-assembly trap):
- negative control — attribution reverted:
Failed: 5, Passed: 1, every cause test red on its
own assertion, the positive control still green MeshWeaver.Layout.Test504/504 ·MeshWeaver.Data.Test519/519 ·
MeshWeaver.Documentation.Test469/469dotnet build … -c Release -warnaserror→0 Error(s)onMeshWeaver.Data,
MeshWeaver.Layout.Test,MeshWeaver.Hosting,MeshWeaver.Documentation.Test
The PR was dequeued to push this (a queued branch cannot be updated) and auto-merge re-armed.
Doc/Architecture/RefusingALostUserAction and the What's New entry carry the fifth line and the two
new controls.
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 #3986
The residue, and why the issue kept reopening
DataExtensions.RefuseUserActionwrote one sentence for three causes —"the target stream is gone (disposed circuit, released read stream, or never-created sync hub)" —
so the incident fingerprint conflated a designed refusal with a live defect.
The 2026-09-15 triage on the issue established the shape precisely: the filed defect IS fixed
(core
1594bb31e4+ Plugins #1652) and the running image provably carries it(
memex.meshweaver.cloud/api/version→c84c6c0550,git merge-base --is-ancestor 1594bb31e4 c84c6c0550→ true). What reopened the ticket on 2026-09-14 was a different route — anowner-side per-node request hub (
rbuergi/Requests/provision-client-b-20260914, areaOverview/Actions/1) whosesync/{id}had never been registered on the activation that receivedthe click. The client-side ordering fix cannot reach that, and the sentence could not distinguish
it, so the bot correctly reopened a fixed issue on evidence that named nothing — and two triages
were spent re-deriving which cause each occurrence was.
The three causes, as now distinguished
The owner records, per stream id and per hub activation, two booleans — whether it ever
registered a
sync/{id}(fromSynchronizationStream's constructor, the one place one is created)and whether an
UnsubscribeRequestfor it reached this hub (fromRouteStreamMessage, before thesub-hub walk).
RefuseUserActionreads that and names which it was:EvictClientSubscriptions. The person is still on the page and the platform dropped the subscriptionEach is its own literal log template, not a parameter on a shared one, so the split holds
however a reader derives a fingerprint (from the template, or from the rendered line).
🚨 A fourth line, which is not a cause. The ledger is bounded (one entry per distinct stream id;
a long-lived owner on a written path mints fresh ones through the change-feed eviction cycle). The
instant it has pruned anything, an ABSENT id is no longer evidence of "never registered" — it is
equally "registered, and long since aged out". So it says exactly that, with its own numbers
(
aged N out and holds M). A diagnostic that cannot fail to give an answer is not a diagnostic, andthis is what stops a FULL ledger masquerading as the third cause.
What this is not
Attribution only. No timer, no retry, no widened grace, no watchdog — the three things #3566 and
#3986 rule out stay ruled out. The
Errorlevel is unchanged, the localized sentence the personreads (
error.userActionNotRun) is byte-identical, no i18n key moved, and a lost action is stilllost —
Doc/Architecture/RefusingALostUserActionsays why that is deliberate.SyncStreamActivationLedgeris an INSTANCE registered besideIWorkspacein the data plugin'sservices — never static — so its lifetime IS the activation it answers about. That is not an
implementation detail, it is the whole answer: a recycled owner starts from empty, and a subscriber
still holding a stream id from the previous activation is then correctly reported as addressing
something that no longer exists.
The measurement
RefusalNamesWhichEndTheStreamMetTest(new) produces each cause by its own real route againstreal hubs, and reads the refusals out of the host's own log. Each cause test asserts its sentence
and the absence of the other two — pairwise, because "distinguishable" is exactly the property a
shared sentence lacked.
AStreamThisActivationNeverServed_IsNamedAsSuchAStreamTheSubscriberReleased_IsNamedAsSuchUnsubscribeRequestreaches the owner and kills the sub-hubAStreamTheOwnerReaped_IsNamedAsSuchSubscribeRequest, then the owner's ownsync/{id}disposed — the routeEvictClientSubscriptionstakesAnAcceptedActionIsStillNotDiscardedSubmitUserActionon a live stream — the action RUNS and no refusal is written at allThe reaped case deliberately has no client-side stream behind it: one would re-subscribe on the
owner's
StreamEndedEventand re-create async/{id}under the same stream id, so the reap would beundone by a race. With nothing to re-ask, the window is closed by construction rather than by a wait.
Falsified — the attribution reverted (single shared sentence restored), test project rebuilt,
rerun:
Failed: 3, Passed: 1, all three cause tests red on their own assertion and thepositive control stays green. That is why the positive control exists: a "fix" that refused
everything with a more descriptive sentence would satisfy the other three on its own.
Verification
dotnet build src/MeshWeaver.Data -c Release -warnaserror→0 Error(s)dotnet build test/MeshWeaver.Layout.Test -c Release -warnaserror→0 Error(s)dotnet build src/MeshWeaver.Hosting -c Release -warnaserror→0 Error(s)(dependent)dotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserror→0 Error(s)MeshWeaver.Layout.Test502/502,MeshWeaver.Data.Test519/519,MeshWeaver.Documentation.Test469/469 (incl.DocumentationLinkIntegrityTest)scripts/check-record-signatures.py→✓ 4 changed C# file(s): no binary-breaking …cross-repo pair, interface-addition and i18n-mirror gates do not apply.
Docs
Doc/Architecture/RefusingALostUserAction— new section "One sentence for three causes — theresidue, and what it cost": the three occurrences and which images they ran on, what the ledger
records, the three sentences, the fourth honest line, and the falsification tally.
Category: Fix): A click that could not run says why it could not run.🤖 Generated with Claude Code