Repository navigation
fix: a finished activity was a KEEP-ALIVE, not something nobody had collected yet (#1324) - #1435
Merged
Merged
Conversation
…ollected yet Closes the last residual of #1324 — the per-compile `_Activity/compile-<ts>` node hubs. The previous pass left the question open as "is this a permanent leak or a ~25-minute bounded retention", because both existing reclaimers are time-based and invisible to a seconds-long test. The framing was wrong in the consumer's favour. A cross-hub write opens a shared IMeshNodeStreamCache mirror on the path, and that mirror posts a HeartBeatEvent to the owning hub every 45 s FOR THE EXPRESS PURPOSE of keeping it alive — which re-arms every idle clock the platform has: the cache's own ten-minute window, an Orleans grain's collection age, KernelContainer.DisposeOnTimeout. So a finished compile was not waiting to be reclaimed; it was preventing its own reclamation, and holding its node hub and both sides' sync sub-hubs up with it. The cure is the missing EVENT, not a shorter timer. An ActivityLog that reaches a status where IsTerminal() holds will never be written again — by definition — so ActivityLogAppender.Append releases the path's warm entry as part of that same write. Nothing about the idle heuristic changes; what changed is that a caller who KNOWS the answer no longer waits for the heuristic to guess it. IMeshNodeStreamCache.ReleaseIfUnwatched(path) is the idle sweep's own detach → claim → tear-down protocol with TimeSpan.Zero in place of the window; both callers now share one implementation. The atomic zero-subscriber guard is unchanged, so a reader still watching the finished activity wins the race and keeps its mirror — the release simply declines. Released on onCompleted, not onNext: tearing the upstream down while the write is still in flight would strand its own terminal. Measured on NodeTypeRecompileAlcLeakTest, matched runs from an identical 38-hub baseline with four retained activities: hubs per compile activity 6.5 -> 5.0 of which mirror-side 3.0 -> 0 hubs per recompile (total) 8.7 -> 6.0 and the direct property — no terminal activity still holds a warm mirror — now holds; it fails before the change, with all four still warm after a 30 s wait. The ratchet tightens 9 -> 8, above the measured 6.0/6.7 and below the 8.7 the un-released mirror produced, so a regression of THIS fix reds the test. What is left is the activity's own node hub plus the four sync sub-hubs its data source builds at startup. Nothing subscribes to it any more, so on Orleans it is finally eligible for the idle deactivation the heartbeat used to keep pushing away; the monolith has no idle collection for node hubs at all, which is what the test still counts. That is a host-lifecycle question, and not one to "fix" by shortening a timer. Verified locally, -c Release -warnaserror: - MeshWeaver.Hosting.Monolith.Test (full) - MeshWeaver.Documentation.Test 20/20 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses the remaining retention from #1324 by treating “warm” activity mirrors as an explicit keep-alive and releasing the shared mesh-node cache entry when an ActivityLog reaches a terminal status, allowing the per-activity hub(s) to go idle and be reclaimed.
Changes:
- Add an event-driven cache release API (
IMeshNodeStreamCache.ReleaseIfUnwatched) and implement it inMeshNodeStreamCacheby reusing the existing detach → claim → teardown protocol. - Release a terminal activity’s warm mirror from
ActivityLogAppender.Appendon completion of the terminal-status write. - Tighten and extend
NodeTypeRecompileAlcLeakTestto assert the “no terminal activity remains warm” mechanism, plus update docs/What’s New to reflect the keep-alive semantics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeRecompileAlcLeakTest.cs | Tightens hub-growth bound and adds an explicit assertion that terminal activities no longer keep read streams warm. |
| test/MeshWeaver.Graph.Test/NodeTypeEnrichmentDoubleCallTest.cs | Updates test stub to implement the new cache interface member. |
| test/MeshWeaver.Graph.Test/IndeterminateProbeReResolutionTest.cs | Updates test stub to implement the new cache interface member. |
| src/MeshWeaver.Mesh.Contract/Services/IMeshNodeStreamCache.cs | Adds ReleaseIfUnwatched(path) to the cache contract with detailed lifecycle documentation. |
| src/MeshWeaver.Mesh.Contract/ActivityLogAppender.cs | Releases the shared mirror after a terminal activity write completes. |
| src/MeshWeaver.Hosting/MeshNodeStreamCache.cs | Implements ReleaseIfUnwatched by factoring out shared release logic used by both idle sweep and event-driven release. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-finished-activity-lets-go-of-its-hub.md | Adds a What’s New entry describing the user-visible impact of reclaiming finished activity hubs. |
| src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md | Documents why “final” must be event-driven (warm mirror = keep-alive) and references the new API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Test Results (shard 3) 11 files ± 0 11 suites ±0 5m 22s ⏱️ + 2m 0s Results for commit dc3f478. ± Comparison against base commit 00ff4b9. This pull request removes 9 and adds 75 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results (shard 4)1 920 tests - 57 1 910 ✅ - 59 7m 59s ⏱️ -5s Results for commit dc3f478. ± Comparison against base commit 00ff4b9. This pull request removes 66 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Contributor
Contributor
…entity capture CI shard 5 caught this and it is the good kind of catch. Wrapping Append's pipeline in Observable.Defer (so the settled-node capture would be per subscription) also deferred the `stream.Update(...)` CALL — and UpdateRemote captures the caller's AccessContext at that call, not at subscribe (`LastModifiedBy = capturedContextAtEntry`). Every activity producer invokes Append inside a scope that has deliberately re-established the activity's OWNER (ActivityRunner + AccessContextScope.FromNode), precisely because the real work runs on pooled hops carrying someone else's ambient identity. Deferring construction moved the capture out of that scope and onto the subscribing thread — the identity-confusion defect ActivityOwnerCredentialTest exists to pin. It failed exactly as designed: `Access denied: user 'activity-intruder' lacks Read permission`. So the Update call goes back to being eager, and `settled` is per Append CALL — which is what this method's contract already implies (an Append is one write, subscribed once), and harmless even under a second subscriber since the release reads only Status. The onCompleted placement, which is about not tearing the upstream down mid-write, is unchanged. MeshWeaver.GitSync.Test 163/163, and the #1324 measurement is unmoved: 5.0 hubs per compile activity, 0 mirror-side, 6.0 per recompile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 2, 2026
This was referenced Sep 13, 2026
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 the last residual of #1324 — the per-compile
_Activity/compile-<ts>node hubs. #1345, #1415 and #1425 took a recompile from 20 MB / 63 hubs to 3–5 MB / ~6 hubs; what was left was the activity nodes themselves.The previous pass asked the right question and guessed the wrong answer
It ended with: "whether the residual is a permanent leak or a ~25-minute bounded retention turns entirely on whether the activity's mirror is actually released once the compile ends" — because both existing reclaimers are time-based and therefore invisible to a seconds-long test (the cache's ten-minute idle sweep; an idle node hub's own collection age).
The answer is that the framing was wrong in the consumer's favour. A warm mirror is not something waiting to be tidied away — it is a keep-alive. A cross-hub write resolves a shared
IMeshNodeStreamCacheentry for the path, and that entry's mirror posts aHeartBeatEventto the owning hub every 45 s, expressly to keep it alive (SyncStreamOptions.HeartbeatInterval). That message re-arms every idle clock the platform has: the cache's own window, an Orleans grain's collection age,KernelContainer.DisposeOnTimeout.So a finished compile was not waiting to be reclaimed. It was preventing its own reclamation, and holding its
_Activity/…node hub and thesync/sub-hubs on both sides up with it. On a morning of merges — each firing a GitSync re-import and another round of recompiles — that is one of these per compile, never given back.The change: the missing EVENT, not a shorter timer
An
ActivityLogthat reaches a status whereIsTerminal()holds will never be written again, by definition.ActivityLogAppender.Append— the one way an activity log is written — now releases the path's warm entry as part of that same write.IMeshNodeStreamCache.ReleaseIfUnwatched(path)is the idle sweep's own detach → claim pair-exact → tear down protocol withTimeSpan.Zeroin place of the window; both callers now go through one implementation, so they cannot drift. Three things are deliberately unchanged:false. Identical check, identical race semantics.onCompleted, notonNext— tearing the upstream down while the write is still in flight would strand that write's own terminal and leave the per-path queue to advance on its 5 s backstop.Generic, not compile-specific: every activity in the mesh (import, export, mirror, compile) takes the same path.
Measured
NodeTypeRecompileAlcLeakTest, matched runs from an identical 38-hub baseline with four retained activities,-c Release:cache/…)The last row is the direct property, asserted at the end of the test, and it fails before the change — all four still warm after a 30 s wait. The per-activity figure is new: the whole-mesh delta carries the settling of every unrelated framework hub with it (5.3 / 7.0 / 6.3 measured for the same code historically), which is exactly the noise that would have hidden this.
The ratchet tightens 9 → 8 — above the 6.0/6.7 measured here, below the 8.7 the un-released mirror produced, so a regression of this fix reds the test.
What is left, and why it stops here
The whole residual is now the activity's own node hub plus the four
sync/sub-hubs its data source builds at startup. Nothing subscribes to it any more, so on Orleans it is finally eligible for the idle deactivation the heartbeat used to keep pushing away — which is the production shape. The monolith has no idle collection for node hubs at all, which is what the test still counts.That is a host-lifecycle question rather than a stream one, and explicitly not one to close by shortening a timer.
Verification
Local,
-c Release -warnaserror:MeshWeaver.Hosting.Monolith.TestMeshWeaver.Documentation.Test🤖 Generated with Claude Code