Repository navigation
Kernel: the idle-disconnect never disposes a hub whose submission is still running (#4422) - #4423
Conversation
…still running (#4422) KernelContainer.DisposeOnTimeout re-armed its one-shot 15-minute timer only on messages DELIVERED to the kernel-hosting hub. A running script delivers none: it executes on the hosted kernelExec child, its mesh reads answer elsewhere, and its log lines leave as outgoing writes. So a script still working 15 min after the last inbound message had the hub, and its executor, disposed mid-run with no SubmitCodeResponse and no terminal status. Measured on memex.systemorph.com 2026-09-15: an approved OperationRequest's follow loop went silent at 10:45:06Z and the request's 20-minute silence watchdog failed it at 11:05:06Z. - IdleState (weak hub ref, window, in-flight count, weak executor ref): the forwarder Begins on every SubmitCodeRequest and Ends in Finally, so a response, an error or a torn-down subscription all release it. - Elapsed re-arms while a submission is in flight on a LIVE executor and disposes otherwise, so a dead executor cannot pin the hub. - End restarts the window, so a finished hub is reclaimed one full window after its run ends, which keeps the #1324/#1435 reclamation. - KernelIdleDisconnectWhileRunningTest pins both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical setup and concurrency races remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes kernel hub disposal during long-running submissions by tracking active work and rearming idle timeouts.
Changes:
- Tracks in-flight submissions and executor liveness.
- Documents updated idle-disconnect behavior.
- Adds regression tests for idle-state handling.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Compiler.Pipeline.Test/KernelIdleDisconnectWhileRunningTest.cs |
Adds idle-state regression tests. |
src/MeshWeaver.Kernel/KernelHubOptions.cs |
Documents updated idle semantics. |
src/MeshWeaver.Kernel.Hub/KernelContainer.cs |
Tracks submissions and controls idle-timeout disposal. |
Review details
Suppressed comments (1)
test/MeshWeaver.Compiler.Pipeline.Test/KernelIdleDisconnectWhileRunningTest.cs:83
- The only assertion is after a subsequent
Begin(), so this test would pass ifEnd()were a no-op; it never verifies the claimed zero clamp after the two unbalanced calls. AssertInFlight == 0immediately after theEnd()calls, then separately assert the laterBegin()reaches one.
public void UnbalancedEnd_NeverCountsBelowZero()
{
var (state, _, _) = Arrange();
state.End();
state.End();
state.Begin();
Assert.Equal(1, state.InFlight);
- Files reviewed: 3/3 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public void End() | ||
| { | ||
| if (Interlocked.Decrement(ref inFlight) < 0) | ||
| Interlocked.Exchange(ref inFlight, 0); | ||
| Rearm(); | ||
| } |
There was a problem hiding this comment.
Fixed in 5846d9c by restructuring, so there is no CAS loop and no counter left to clamp.
IdleState now holds one immutable Snapshot(Closed, ImmutableHashSet<Claim> InFlight). Every transition is a single ImmutableInterlocked.Update on that one reference:
- claim, in
Track; - release, in
Release; - close, in
ElapsedandDispose.
A submission's slot is a Claim object, and only that claim removes it. So the count cannot go negative. There is also no gap between a decrement and a clamp for a concurrent claim to fall into. The old shape had one: Decrement→-1, then a concurrent Begin→0, then Exchange(0) erased the Begin. Elapsed decides on the snapshot it swaps, so it cannot act on a torn value.
Pinned by:
ARunningSubmission_OutlivesTheWindow_AndTheWindowRestartsWhenItEnds(×3), which assertsInFlight == 0after each termination;ASetupThatThrows_ReleasesItsClaim.
Forcing that interleaving deterministically would need a seam inside the swap, so this property holds by construction rather than by a test.
| var executor = GetOrCreateExecutor(hub); | ||
|
|
||
| // In flight until the executor answers — the idle-disconnect must not dispose this hub | ||
| // under a running script (MeshWeaver#4422). Finally runs on the response, on an error, | ||
| // and on a disposal that tears the subscription down, so the count cannot leak upward. |
There was a problem hiding this comment.
Fixed in 5846d9c.
- Claim before setup. The claim is taken when
IdleState.Trackis subscribed, BEFOREdispatchruns.GetOrCreateExecutornow runs insidedispatch. - A claim being set up counts as working. A claim with no executor bound yet counts as working, so a tick during executor setup sees a submission being set up and keeps the hub.
- Claim and close are one ordered decision. Both are a compare-and-swap on the same snapshot, so a claim lands either before the close or after it. A claim after the close is refused and nothing is set up on the closing hub. The forwarder answers the requester with
SubmitCodeResponse { Success = false }.
Pinned by:
TheWindowElapsingDuringExecutorSetup_KeepsTheHub: the clock is advanced past the window from INSIDEdispatch;AClaimAfterTheHubClosed_IsRefused_AndNothingIsSetUp.
Negative controls, each rebuilt and run:
- moving the claim after the setup turns both tests red;
- letting a claim ignore
Closedturns the second one red.
| hub.Observe<SubmitCodeResponse>(request.Message, o => o.WithTarget(executor.Address)) | ||
| .Take(1) | ||
| .Finally(() => state?.End()) |
There was a problem hiding this comment.
Fixed in 5846d9c. ForwardSubmitCodeRequest now hands the whole setup and post to IdleState.Track as Dispatch: GetOrCreateExecutor, bind the claim, then hub.Observe. Track runs it as Observable.Defer(() => dispatch(claim)).Finally(() => Release(claim)).
A synchronous throw from Observe's post, or from the executor setup, therefore becomes OnError of the deferred observable. Finally releases the claim, and the forwarder's onError answers the requester.
Pinned by ASetupThatThrows_ReleasesItsClaim, which makes dispatch throw the way Observe does on a refused post.
Negative control: calling dispatch(claim) outside the Defer turns it red, because InFlight stays at 1 and the hub is never reclaimed.
| var hub = Substitute.For<IMessageHub>(); | ||
| var executor = Substitute.For<IMessageHub>(); | ||
| executor.IsDisposing.Returns(false); | ||
| var state = new KernelContainer.IdleState(new WeakReference<IMessageHub>(hub), TimeSpan.FromMinutes(15)); |
There was a problem hiding this comment.
Fixed in 5846d9c. IdleState now creates its timer from an injected TimeProvider: KernelHubOptions.TimeProvider, which is TimeProvider.System in every host. The tests drive a manual clock through the production 15-minute window and assert WHEN the one-shot timer fires.
ARunningSubmission_OutlivesTheWindow_AndTheWindowRestartsWhenItEnds is a Theory over Response, Error and Teardown (the subscription disposed). Each case:
- keeps the hub at W and re-arms;
- ends the run at 1.5 W;
- requires the hub still alive at 2 W, which is the due time the keep had armed;
- requires it disposed at 2.5 W.
So each termination path must reset the due time. ADeadExecutor_CannotKeepTheHubAliveForever additionally needs the keep's re-arm, because without it the one-shot timer never fires again.
Negative controls, each rebuilt and run:
- dropping the re-arm on release turns all three Theory cases red;
- dropping the re-arm on a keep turns the dead-executor test red.
| /// <para>🚨 <b>Idle means no inbound message AND no submission in flight</b> (#4422). The | ||
| /// window is re-armed by delivered messages, and a running script delivers none, so without the | ||
| /// in-flight check a script still working 15 min after the last inbound message had its hub | ||
| /// disposed mid-run, silently. <c>KernelContainer.IdleState</c> keeps the hub while a forwarded | ||
| /// submission has not answered and restarts the window when it does.</para> |
There was a problem hiding this comment.
Added in 5846d9c: src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-15-a-long-running-script-is-no-longer-stopped-after-15-quiet-minutes.md. It has Category: Fix, Order: -20260915, and the same front matter as the other entries.
I also added the in-flight rule to the idle-disconnect section of Doc/Architecture/MeshNodeStreamCache, where the reaper is documented.
| var hub = Substitute.For<IMessageHub>(); | ||
| var executor = Substitute.For<IMessageHub>(); | ||
| executor.IsDisposing.Returns(false); |
There was a problem hiding this comment.
Fixed in 5846d9c. NSubstitute is gone from this test, and the class now derives from HubTestBase:
- the kernel host and its executor are real hosted hubs;
- "disposed" is the hub's own
IsDisposing; - the dead executor is a real
Dispose()of the executor hub.
The only injected seam is the clock, a TimeProvider, not a hub interface. That is what lets the tests step through the production window instead of shrinking it.
…setup, released on every path (#4422) Answers the six #4423 review threads. - One immutable Snapshot(Closed, ImmutableHashSet<Claim>) swapped with ImmutableInterlocked.Update replaces the counter plus clamp. A claim, a release and the close are each one CAS on the same reference, so a claim is ordered against the close. Nothing can go negative, and a concurrent claim can no longer be erased by a clamp. - IdleState.Track takes the claim BEFORE the executor is set up. A claim with no executor bound yet counts as working, so a tick during setup keeps the hub. A claim after the close is refused and nothing is set up. - The setup and the post run inside Observable.Defer(...).Finally(Release). A synchronous throw from Observe's post or from GetOrCreateExecutor releases the claim instead of pinning the hub. - The timer comes from KernelHubOptions.TimeProvider (TimeProvider.System in every host). KernelIdleDisconnectWhileRunningTest now uses real HubTestBase hubs, not NSubstitute, and steps a manual clock through the production window. It asserts the due time moves on response, error and teardown, and that a dead executor cannot pin the hub. - What's New entry (Category: Fix), and the in-flight rule in Doc/Architecture/MeshNodeStreamCache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dle (#4422) Closed is set by the idle tick AND by the hub's own disposal (a recycle, a host teardown), so the refusal names both instead of asserting an idle reclamation it cannot know. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
⛔ Merge-queue steward: left dequeued — the group build failed on an assertion the flake catalogue does not know.
Not a catalogued flake. Fix the failure, or — with run URLs, an issue and an assertion-message pattern — add it to |
|
Re-arming after the The group build failed on Measured since (details and numbers on #4448): on a fully saturated 18-core host the pooled subscription's terminal arrives in 0 ms, and the test did not fail in 8 isolated runs or in 4 class runs under that load. So the 5 s bound is not the problem, and widening it would be a band-aid for something that is not slow. The mechanism of the single CI occurrence is unexplained and stays open on #4448 — deliberately not catalogued as a flake, because one unexplained occurrence is not evidence. Re-armed once. If the same assertion fails again, that is the second data point and it goes back to #4448 as a real lost-terminal hunt rather than a re-queue. 🤖 Generated with Claude Code |
|
🔀 Merge-queue steward: re-queued alone (bisecting) — bisecting: your group of 2 pull requests failed on an uncatalogued assertion while this PR's own run was green; re-queued ALONE — if the solo build fails too, the PR stays out (group: #4454).
enqueued |
Fixes #4422.
What was wrong.
KernelContainer.DisposeOnTimeoutre-armed its one-shotIdleDisconnectTimeouttimer, 15 min, only when a message was delivered to the kernel-hosting hub. A running script delivers none. It executes on the hostedkernelExec/{path}child, its mesh reads answer elsewhere, and its log lines leave as outgoing writes. So a script still working 15 min after the last inbound message had its host hub, and the executor with it, disposed mid-run. That ended the run silently, with noSubmitCodeResponseand no terminal activity status.Measured on memex.systemorph.com, 2026-09-15:
rbuergi/Requests/provision-client-b-20260914stopped reporting after10:45:06Z.11:05:06Z.Change.
KernelContainer.IdleStateholds the hub weakly, the window, and the one-shot timer. It creates the timer fromKernelHubOptions.TimeProvider, which isTimeProvider.Systemin every host. It also holds ONE immutableSnapshot(Closed, ImmutableHashSet<Claim> InFlight), and every transition is a singleImmutableInterlocked.Updateon it: claim, release and close. There is no counter to go negative, and a claim and a close are always ordered.SubmitCodeRequestthroughIdleState.Track, which takes the claim BEFORE the executor is set up.GetOrCreateExecutor, binding the claim to the executor, andhub.Observeall run insideObservable.Defer(…).Finally(release). So a response, an error, a teardown, or a synchronous throw from the setup or the post each release the claim.Elapsedkeeps the hub and re-arms while any claim is still being set up or sits on a live executor. Otherwise it closes and disposes, in the same compare-and-swap. A claim after the close is refused, nothing is set up, and the requester getsSubmitCodeResponse { Success = false }.Tests.
KernelIdleDisconnectWhileRunningTestuses realHubTestBasehubs, not NSubstitute: the kernel host and its executor are real hosted hubs, and a dead executor is really disposed. A manualTimeProvidersteps through the production 15-min window, so every assertion is about WHEN the one-shot timer fires. 7/7 green locally.Each row below is red when that production change alone is reverted: rebuilt, then run. On
mainthe class does not compile, becauseIdleStatedoes not exist there. M1 is the pre-#4422 behaviour inside the new structure.ARunningSubmission_OutlivesTheWindow_AndTheWindowRestartsWhenItEnds(Response / Error / Teardown)Elapsedignores in-flight (pre-fix); M4 release does not re-armADeadExecutor_CannotKeepTheHubAliveForeverTheWindowElapsingDuringExecutorSetup_KeepsTheHubASetupThatThrows_ReleasesItsClaimDefer/FinallyAClaimAfterTheHubClosed_IsRefused_AndNothingIsSetUpClosedAfter the controls, the fix was restored byte-identical, rebuilt, and run green again.
Builds. Each of these builds with
-c Release -warnaserrorand reports 0 warnings and 0 errors:MeshWeaver.KernelMeshWeaver.Kernel.HubMeshWeaver.Compiler.Pipeline.TestMeshWeaver.DocumentationMeshWeaver.Documentation.TestGuard / Ratchet / WhatsNew / FrontMatter tests: 321/321 green.Docs.
Category: Fix.Doc/Architecture/MeshNodeStreamCache.Surface.
KernelHubOptions.TimeProvideris a new init property on a public record. It is not a primary-constructor parameter, and no public type, member or interface is removed or added.Pairs-with: none — nothing is removed from the public surface; the only addition is an init property with a default.
Companion: Systemorph/MeshWeaver.Plugins#1924 makes the request's watchdog stamp say what went quiet and when.
Kept as a draft for the maintainer's review.
🤖 Generated with Claude Code