EventSubscriptionRunner: fire each subscription at most once, not once per firing path (main-red flake) by rbuergi · Pull Request #659 · Systemorph/MeshWeaver

rbuergi · 2026-07-26T21:07:20Z

Why

main is red on EventSubscriptionRunnerTest.PreExistingUserAndSubscription_FireOnRunnerStartup_WithoutTheChangeFeed — CI shard 4 on both 89332e347 and 4fb03f017, and 3 of 10 full MeshWeaver.Graph.Test runs on a clean main worktree locally. Not a new regression: it predates today's merges.

The test body passes. What fails is the class dispose gate:

EventSubscriptionRunnerTest left Observe subscriptions pending past the Quiescing budget.
This is a leaked callback — the test posted a request and never received (or never awaited) its reply.
  1 pending callback(s) after 0.50s: …=CreateOrUpdateNodeRequest@mesh/…(640ms)

That callback is real, and the budget is not the problem.

Root cause

The runner fires one subscription three times at startup.

All four firing paths — change feed, cold-start authoritative reconcile, pending-set reconcile, trigger-node watch — draw their candidates from the same pending snapshot, and a subscription only leaves that snapshot once its terminal Fired write has round-tripped through the Admin partition. At startup every path evaluates within a few milliseconds of the others, i.e. while the subscription is still Pending in all of them, so each launches its own continuation: three concurrent upserts of the same membership/grant node plus three SetStatus writes, every one fire-and-forget (.Subscribe(...) handle discarded).

The assertions are satisfied by whichever execution lands first; #2 and #3 are still in flight, serialising behind each other on the target node's hub. Measured in an isolated run: assertions satisfied at 148 ms, the three fires logged at 160/178/194 ms, one CreateOrUpdateNodeRequest still pending at test-body end. Under full-suite load that tail stretches past the 0.5 s quiescing budget — which is exactly why it only flakes there and passes in isolation.

The class doc asserted that idempotence made the overlapping paths safe. Idempotent means they cannot double-apply, not that they do not double-run — and a duplicate run still costs an unobserved request that outlives whatever triggered it. Doc corrected in this PR.

The fix

A per-subscription in-flight reservation in EventSubscriptionRunner — the same instance-scoped ConcurrentDictionary.TryAdd pattern the class already uses for timerSubs / statusSubs / nodeChangeSubs / migratedLegacyIds:

reserved before the continuation chain is built, so whichever path arrives first owns it;
released in the error handler, so a later pending-set emission still retries a transient failure (a permanent one writes Failed and leaves the pending set anyway);
pruned alongside the existing registries when a subscription leaves the pending set — terminal subscriptions can never fire again, so the registry stays bounded by the pending set rather than by runner uptime;
cleared on Dispose.

No timeout widened, no retry loop, no sleep, no Clear()-for-test-isolation, nothing async introduced.

Evidence

New regression test StartupFiresEachSubscriptionExactlyOnce_NotOncePerFiringPath counts continuations via a capturing ILogger with all firing paths armed (live change feed, not the silent one):

	result
Regression test before the fix	fails 3/3 — "the subscription's continuation ran 3 times"
Regression test after	passes 3/3 — 1 continuation
Pending callbacks at test-body end, after	zero (was 1 `CreateOrUpdateNodeRequest` + 2 `PatchDataRequest`)
Full `MeshWeaver.Graph.Test`, baseline clean `main`	3 of 10 runs failed, always this test, 625/626
Full suite after the fix	11/11 green, 627/627 — plus 3 independent confirmation runs, 627/627 each

Builds clean at -c Release -p:CIRun=true -warnaserror, 0 warnings.

No What's New entry: internal reliability fix with no user-visible behaviour change (the duplicate writes were idempotent — they cost work and a leaked callback, not wrong data).

Found but not fixed (follow-ups, deliberately out of scope)

WatchTriggerNodeType is a live query over every node of the trigger type and re-reconciles all pending Created subscriptions on each emission, so any unrelated User write re-enters Execute for every pending subscription. This PR removes the concurrent duplicates, but the repeated evaluation (one meshService.Query per pending subscription per emission) is still O(pending × emissions) at boot — a plausible contributor to the known boot-storm write amplification.
The no-op upsert path in MeshExtensions.HandleCreateOrUpdateNodeRequest can hold a request open for up to NodeOpForwardTimeout (15 s) waiting on GetEffectivePermissions before falling through to the write path. Correct as written, but it means any duplicate upsert parks a callback far longer than the 0.5 s dispose budget — it amplified this flake and would amplify the next one into the same misleading teardown failure.

🤖 Generated with Claude Code

…e per firing path main was red on EventSubscriptionRunnerTest.PreExistingUserAndSubscription_ FireOnRunnerStartup_WithoutTheChangeFeed — 3 of 10 full Graph.Test runs on clean main, and CI shard 4 on 89332e3 and 4fb03f0. The test body passes; what fails is the class dispose gate, on an unanswered CreateOrUpdateNodeRequest still in flight past the 0.5s quiescing budget. That callback is real: the runner fires ONE subscription THREE times at startup. All four firing paths (change feed, cold-start authoritative reconcile, pending-set reconcile, trigger-node watch) draw candidates from the same "pending" snapshot, and a subscription only leaves that snapshot once its terminal Fired write has round-tripped through the Admin partition. At startup every path evaluates within a few ms — while the subscription is still Pending in every snapshot — so each launches its own continuation: three concurrent upserts of the same membership/grant node plus three SetStatus writes, every one of them fire-and-forget. The assertions are satisfied by whichever lands first; the rest serialise behind it on the target hub and outlive the test. The class doc claimed idempotence made this safe. Idempotent means they cannot double-APPLY — not that they do not double-RUN. Fix: a per-subscription in-flight reservation (the "executing" registry), the same instance-scoped TryAdd pattern the timer / status / trigger-node registries already use. Reserved before the chain starts, released on failure so a later pending-set emission retries a transient error, pruned when the subscription leaves the pending set (so it stays bounded by that set, not by uptime), cleared on Dispose. No timeout widened, no retry, no sleep, nothing async added. Regression test StartupFiresEachSubscriptionExactlyOnce_NotOncePerFiringPath counts continuations with a capturing logger and all firing paths armed: fails 3/3 before the fix (3 continuations), passes after. Full Graph.Test suite: 3/10 runs failing before, 11/11 green after, plus 3 independent confirmation runs at 627/627 each and a clean Release/-warnaserror build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR addresses a CI flake in EventSubscriptionRunnerTest.PreExistingUserAndSubscription_FireOnRunnerStartup_WithoutTheChangeFeed by ensuring a given pending EventSubscription continuation is executed at most once, even when multiple runner firing paths observe the same still-Pending snapshot during startup.

Changes:

Add an instance-scoped in-flight reservation (executing) in EventSubscriptionRunner to prevent duplicate concurrent continuation launches across firing paths.
Update runner documentation to clarify that idempotent continuations can still “double-run” without an explicit at-most-once guard.
Add a regression test that arms all firing paths and asserts exactly one continuation run.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/MeshWeaver.Graph/EventSubscriptionRunner.cs`	Adds an in-flight execution registry to enforce at-most-once continuation execution per subscription across concurrent startup firing paths.
`test/MeshWeaver.Graph.Test/EventSubscriptionRunnerTest.cs`	Adds a regression test that detects duplicate continuation launches and validates the at-most-once behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+                    // Release the reservation so a later pending-set emission can retry a TRANSIENT
+                    // failure — the same rule MigrateLegacyScheduledActions applies to its own id guard.
+                    // (A permanent failure writes Failed below and leaves the pending set anyway.)
+                    executing.TryRemove(subscription.Id, out _);


+        await Mesh.GetWorkspace().GetMeshNodeStream(membershipPath)
+            .Where(n => n?.Content is GroupMembership gm && gm.Member == InviteeId)
+            .FirstAsync().Timeout(10.Seconds());


+        // Observation window for a MUST-NOT-HAPPEN assertion: the duplicate fires this pins arrive within
+        // ~50ms of the first, so the window can only ever hide the defect, never invent it. (Before the
+        // fix this reports 3 fires; after it, 1.)
+        await Task.Delay(3.Seconds());


github-actions · 2026-07-26T21:24:36Z