Repository navigation
fix: Recycle must land its recompile trigger BEFORE it disposes the hub - #2763
Merged
Merged
Conversation
RecycleCore stamped the release request with a fire-and-forget .Subscribe()
and then posted DisposeRequest to the same hub synchronously. The two were
ordered only by both being issued inside the caller's hub turn — an invariant
nothing enforced, and one the permission gate's own Take(1) comment already
records as load-bearing ("must stay ordered against the caller's turn, or the
recompile that follows re-runs its source query against a half-invalidated
state and matches zero Code nodes").
When the dispose won, the reactivated hub recompiled the PRE-FIX source,
matched zero Code nodes, re-settled at Error with the stale diagnostics and
stayed parked — the user-visible symptom being "I fixed it and recycled, and
it still reports the old errors".
Sequence the changefeed publish + DisposeRequest onto the stamp's completion
so the ordering is explicit and turn-independent. A failed stamp still
proceeds with the bounce (that is the caller's actual ask) and logs, rather
than silently degrading to the old racy path.
Also: ReactiveWait dropped the IDisposable returned by source.Subscribe(...),
so every assertion leaked its subscription until the source terminated. Real
bug, separate from the above — fixed here since it was found in the same hunt.
Evidence: NodeTypeCompileParkTest.ParkedNodeType_RecycleRetry_AfterFix_
SettlesOkAndUnparks failed ~20-40% locally and reddened PR #2743's shard 1 in
CI. With the sequencing fix: 15/15 locally (p(0/15 | 20%) = 0.035), and the
captured failure output confirms the mechanism — the failing compile reports
the BROKEN configuration's diagnostics (CS1040 on the '#' of "C#", CS0246 on
'valid') after the node provably carries the fix.
The test's own "wait for the fix to be visible" step is REMOVED, deliberately:
it was a band-aid over this defect and would mask a regression of it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oses-its-subscription
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race in Recycle so a fixed NodeType reliably recompiles the updated source by explicitly sequencing the release-request stamp before the hub-dispose bounce; also addresses a test-assertion subscription leak uncovered during investigation.
Changes:
- Sequence
RecycleCore’s release-request stamp completion ahead of postingDisposeRequestto eliminate turn-order dependence. - Dispose
ReactiveWaitsubscriptions on settle to prevent assertion subscribers leaking across the test process. - Add a regression guard comment in the test and a What’s New entry documenting the user-visible fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeCompileParkTest.cs | Adds a regression-guard comment to prevent masking the recycle ordering bug in the test. |
| src/MeshWeaver.Reactive.Assertions/ReactiveWait.cs | Disposes assertion wait subscriptions on settle to prevent subscriber leaks/starvation. |
| src/MeshWeaver.Mesh.Operations/MeshOperations.cs | Sequences the release-request stamp before dispose during recycle to avoid recompiling stale/empty sources. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-30-recycling-a-fixed-node-type-now-compiles-the-fix.md | Documents the fix and its user-visible behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var completion = new TaskCompletionSource<T?>(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| source.Subscribe( | ||
| // 🚨 DISPOSE THE SUBSCRIPTION WHEN THE WAIT SETTLES. Rx's own bridge did; dropping the |
Comment on lines
+3671
to
+3679
| hub.Post(new DisposeRequest(), o => o.WithTarget(new Address(resolvedPath))); | ||
| return JsonSerializer.Serialize( | ||
| new | ||
| { | ||
| status = "Recycled", | ||
| path = resolvedPath, | ||
| message = "DisposeRequest posted + cache invalidation broadcast via MeshChangeFeed. Wait ~100ms before the next access." | ||
| }, | ||
| hub.JsonSerializerOptions); |
Contributor
Test Results (shard 3)721 tests 529 ✅ 4m 9s ⏱️ Results for commit 52455c6. |
Contributor
Test Results (shard 0)917 tests 917 ✅ 5m 27s ⏱️ Results for commit 52455c6. |
Contributor
Test Results (shard 5)1 547 tests 1 547 ✅ 6m 37s ⏱️ Results for commit 52455c6. |
Contributor
Test Results (shard 4) 10 files 10 suites 5m 59s ⏱️ Results for commit 52455c6. |
Contributor
Test Results (shard 2)973 tests 973 ✅ 5m 47s ⏱️ Results for commit 52455c6. |
Contributor
Test Results (shard 1)793 tests 789 ✅ 7m 1s ⏱️ Results for commit 52455c6. |
Contributor
Test Results 41 files 41 suites 35m 3s ⏱️ Results for commit 52455c6. |
This was referenced Aug 30, 2026
Merged
rbuergi
added a commit
that referenced
this pull request
Sep 1, 2026
…vious fix loses writes #2776 asked one question: what ended the activity node's two sync streams while the activity was still running. This answers it, records the measurement that shows the obvious cure is worse than the disease, and makes the diagnostic that caused the mis-triage state its own age. It changes NO behaviour. ── the answer ── Its own previous progress write did, at +0.12 s. IMeshChangeFeed is published after EVERY create/update/delete, and Workspace.EvictForPath drops every cached remote stream whose owner matches the changed path — including the mirror the writer itself is using. A write leases that mirror only for its Observable.Create subscription, so the lease is released moments after its own change event lands; the evicted stream then has no declared holder and ReclaimIfUnheld DISPOSES it — UnsubscribeRequest to the owner, both sync/{id} hubs gone — and the owner then announces StreamEndedEvent to a subscriber that no longer exists. Measured: six progress writes to one activity node with one live reader mint 7 client-side and 11 owner-side sync/ hubs, with 5 mirrors disposed mid-run. ── the two arithmetic traps that made it read as a hub teardown ── 1. The drop line is written FIVE SECONDS after the event it describes: RouteStreamMessage holds a StreamMessage whose sync/{id} hub is missing for SyncStreamOptions.SyncHubRegistrationGrace (5 s) and logs only on that Timeout. In #2776 the streams ended at +0.12 s — confirmed independently by the failure's own arithmetic (FAILED seq=3 elapsedMs=31015.7 at 13:28:58.952 ⇒ EnteredAt 13:28:27.936). The ADVANCE_WITHOUT_HANDOFF beside it is a DIFFERENT 5 s bound (QueueAdvanceBound) counting from the same instant. Two unrelated five-second bounds produced a coincidence that looked like the hosted-hub drain cap. 2. The SENDER rules out a recycle. The recycle announcement is posted by a carrier (a dying hub must never speak for itself); the healthy-owner announcement is posted by the owner and refuses to fire once RunLevel > Started. Both #2776 lines name the activity node — so the owner was Started and NOT disposing. #2763/#2582 are eliminated by the trace, not by argument. ── why the obvious fix is NOT taken ── Giving EvictForPath the liveness gate its two sibling listeners already have (MeshNodeStreamCache.ResetFailureState: "A healthy live entry is left untouched"; JsonSynchronizationStream's version-gated Resubscribe: "resubscribing on it is pure churn") was implemented and measured, and it LOSES WRITES. The eviction is incidentally what keeps a cross-hub writer's BASE current with respect to writes the OWNER makes for itself: _pendingSelfWrites carries only this cache's writes forward, while an owner-side write (an activity's messageCount, a sealed segment) reaches the mirror only through the async fan-out. Controlled, same box, DOTNET_PROCESSOR_COUNT=4, on StaticRepoImportActivityWriteCountTest.AppendCost_DoesNotGrowWithTheLengthOfTheActivity: gate ON FAILED — 2000 messages appended, 1975 recorded (a 25-message batch lost) gate OFF PASSED and on CI the gated build turned 80 append calls into 99 writes with 44 OWNER_NACK_REENQUEUE / "MergeGuard: refused stale/reordered cross-hub write to 'messageCount'". The O(1) append pin (6a48f9f, d1a239a) was right and is not touched. EvictForPath now carries a comment saying the unconditional eviction is deliberate, and why. ── what DOES change ── LogStreamMessageDrop states how long the message was HELD, so its timestamp can never again be read as the time the stream ended. That is the whole behavioural diff; the rest is a doc page and comments. ── the real fix, for whoever takes it ── Make the writer's base version-aware (MeshChangeEvent already carries Version) so a write waits for the mirror to reach the announced version instead of throwing the mirror away to force a fresh snapshot. That is a change to the write path, not to EvictForPath. ── not claimed ── #2776's visible failure was a write with no verdict at 31 s. That silence has a separate, already-fixed cause (#2882 / #2890, 4d23117 — the write registered its response subject after posting; the trail could only say REGISTERED_AFTER_POST, which is exactly what that trace says). Verified: Release -warnaserror 0/0 for Graph.Test and Documentation.Test (which transitively build MeshWeaver.Data, Hosting, Mesh.Contract, Graph, Layout, Messaging.Hub, Documentation). Graph.Test 1086/1086 and Documentation.Test 166/166 at DOTNET_PROCESSOR_COUNT=4; earlier full runs on this tree also had Data.Test 419/419 and Hosting.Test 274/274. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa
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.
The user-visible bug
Fix a NodeType that failed to compile, hit Recycle, and it can come back reporting the same errors you just fixed — still parked. Recycling again sometimes helps, which makes it read as a slow cache rather than a defect.
Root cause
RecycleCoredid two things back to back:stream.Update(curr => WithReleaseRequest(curr, now))— fire-and-forget.Subscribe(...)hub.Post(new DisposeRequest(), target: that same node)— synchronously, immediatelyNothing sequenced them. The ordering held only because both were issued inside the caller's hub turn — and the permission gate's own comment right above already records that invariant as load-bearing:
When the dispose won, the reactivated hub recompiled the pre-fix source, matched zero Code nodes, re-settled at
Errorwith the stale diagnostics, and stayed parked.The evidence, not the inference
The captured failure output shows the post-recycle compile reporting the broken configuration's own diagnostics —
CS1040on the#of"C#",CS0246on'valid', from the literal"config => this is not valid C# at all (("— after the node provably carried the fix, plusMatched Code nodes (0). That is the mechanism, verbatim.The fix
The changefeed publish +
DisposeRequestare sequenced onto the stamp's completion, so the ordering is explicit and turn-independent. A failed stamp still proceeds with the bounce (that is the caller's actual ask) and logs it, rather than silently degrading to the old racy path.What I deliberately did NOT do
I first "fixed" this in the test, by waiting for the node to carry the fix before recycling. That passed 8/10 — and it was a band-aid over a real framework race that also fires in production. That wait is removed, and a comment stands in its place telling the next person not to re-add it, because it would mask a regression of this fix.
Two earlier hypotheses of mine were refuted by measurement before this one: the impersonation swap in
MonolithMeshTestBase(reverting it would have re-opened the identity leak its ratchet exists to prevent) andReactiveWait's dropped subscription.Also in here
ReactiveWaitdropped theIDisposablefromsource.Subscribe(...), leaking every assertion's subscription until its source terminated. A genuine bug, not the cause of this flake (measured 7/10) — fixed here because the hunt surfaced it.Blast radius
Recycleis the MCPrecycletool and the UI's recycle action. Behaviour is unchanged on the success path except that the response now returns after the stamp lands rather than before it.🤖 Generated with Claude Code