Ask for the active node numbers once per node, not once per database (GH-3846) - #3847
Merged
jeremydmiller merged 1 commit intoAug 5, 2026
Merged
Conversation
DurabilityAgent's recovery timer needs the active node numbers to spot messages orphaned by a departed node. The only call that yields them, LoadAllNodesAsync, also selects the entire assignment table so it can populate ActiveAgents — which this caller never reads — and there is one durability agent per message database. On a sharded deployment that turns a per-node fact into a per-database query. At 512 databases on the default five-second ScheduledJobPollingTime and ~10,000 agents, measured on the main store: 76 calls and 772,000 rows a second, with Client:ClientWrite at 164 of 170 average active sessions while Lock:transactionid sat at 0.04 — the server's time went into writing those result sets. Each call also held a pooled connection, so the heartbeat writes timed out, the leader declared healthy nodes stale, and reassigning their agents churned the very table being read. ActiveNodeNumberCache fetches it at most once per polling interval for the whole node. The caller already tolerates data one interval old, since that is how often it looks, so behaviour is unchanged and the query count drops by the database count. Same shape as PersistenceMetricsSweeper for the metrics polling in JasperFxGH-3375. Failures are deliberately not cached: the durability agent keeps its own try/catch and decides what a failed lookup means.
There was a problem hiding this comment.
Pull request overview
This PR reduces persistence load in clustered/sharded deployments by introducing a node-wide cache for active Wolverine node numbers, so DurabilityAgent no longer hits LoadAllNodesAsync() once per database per polling tick.
Changes:
- Added
ActiveNodeNumberCache(per-runtime, polling-interval TTL) to collapse active-node lookups across all databases on the same node. - Updated
DurabilityAgentrecovery polling to use the cache instead of querying node persistence directly. - Added CoreTests coverage for correctness, concurrency collapsing, refetch behavior, failure behavior, and per-runtime scoping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Wolverine/Persistence/ActiveNodeNumberCache.cs | Introduces the per-runtime, node-wide cache for active node numbers. |
| src/Testing/CoreTests/Persistence/active_node_number_cache_tests.cs | Adds tests validating cache behavior and concurrency collapsing. |
| src/Persistence/Wolverine.RDBMS/DurabilityAgent.cs | Switches recovery polling to use ActiveNodeNumberCache instead of querying persistence directly. |
Suppressed comments (2)
src/Wolverine/Persistence/ActiveNodeNumberCache.cs:75
- Update the cache via a single snapshot assignment (and publish it with Volatile.Write) so readers never see mismatched numbers/expiry values.
var nodes = await _runtime.Storage.Nodes.LoadAllNodesAsync(token).ConfigureAwait(false);
var numbers = nodes.Select(x => x.AssignedNodeNumber).ToList();
_numbers = numbers;
_staleAt = DateTimeOffset.UtcNow.Add(_runtime.DurabilitySettings.ScheduledJobPollingTime);
return numbers;
src/Wolverine/Persistence/ActiveNodeNumberCache.cs:93
- Use Volatile.Read when checking cache state on the fast path to guarantee safe publication of the snapshot across threads.
private bool fresh(out IReadOnlyList<int> numbers)
{
var current = _numbers;
if (current != null && DateTimeOffset.UtcNow < _staleAt)
{
numbers = current;
return true;
}
numbers = Array.Empty<int>();
return false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+46
| private readonly IWolverineRuntime _runtime; | ||
| private readonly SemaphoreSlim _gate = new(1, 1); | ||
| private IReadOnlyList<int>? _numbers; | ||
| private DateTimeOffset _staleAt = DateTimeOffset.MinValue; | ||
|
|
Comment on lines
+13
to
+15
| /// orphaned by a departed node, but the only persistence call that yields them — | ||
| /// <see cref="Runtime.Agents.INodeAgentPersistence.LoadAllNodesAsync" /> — also selects the entire | ||
| /// assignment table so it can populate <c>WolverineNode.ActiveAgents</c>, which this caller never |
This was referenced Aug 5, 2026
Closed
portlogicsvn
pushed a commit
to portlogicsvn/wolverine
that referenced
this pull request
Aug 6, 2026
…-3850) Follow-up to JasperFxGH-3846/JasperFx#3847, which cached the active node numbers per node instead of fetching them per database. That is a good trade -- the failure it broke was self-amplifying -- but it widens a window that is not symmetric. ReleaseOrphanedMessagesForAncillaryOperation resets owner_id to 0 for every owner missing from the list. A stale list that still names a DEAD node merely delays recovery one interval, which is benign. A stale list MISSING a LIVE node resets that node's in-flight rows and lets another node claim work it is already doing -- duplicate handling on the inbox, duplicate send on the outbox. The list cannot describe a node that registered after it was taken, and MessageRoute stamps OwnerId on outbox persist, so a newcomer owns rows within milliseconds of registering. Node numbers are database-generated and monotonic (SERIAL on Postgres, AutoNumber on SQL Server), so anything above the list's horizon registered after it and must not be judged against it. The bound is deliberately NOT max(active), which looks equivalent and is not: when the highest-numbered node dies its number leaves the active list, the max drops below it, and its orphaned messages become permanently unreclaimable. ActiveNodeNumberCache keeps the mark monotonic across fetches instead -- raised, never lowered -- so a node that was ever seen stays reclaimable after it departs while one never seen stays protected. The remaining gap is a node that registers and departs entirely between two fetches: its rows stay owned until the mark rises, which parks messages rather than double-processing them, and the next registration clears it. Also documents the gate's cost, which JasperFx#3847 left implicit: one SemaphoreSlim means a hung LoadAllNodesAsync parks every database's callback rather than each failing on its own timeout. That is the intended trade, but a reader diagnosing a stall should not have to derive it. Tests: four on the cache pinning the mark's monotonicity and the staleness window it exists for, four on the generated SQL. Verified non-vacuous -- disabling the ceiling turns 3 of the 4 SQL tests red (the fourth asserts the guard's absence when no mark is supplied). CoreTests 2264/0; SqlServerTests orphan suites 7/7; dotnet build wolverine.slnx -c Release -f net9.0 clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHAuhdWS3XeAk16swV9G8m
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 #3846.
DurabilityAgent's recovery timer wants the active node numbers. The only call that yields them,LoadAllNodesAsync, also selects the entire assignment table to populateActiveAgents— which this caller never reads — and there is one durability agent per message database. So a per-node fact was being fetched per database.Measured on a 512-database, five-node, ~10,000-agent cluster (details in the issue): 76 calls and 772,000 rows a second against the main store, with
Client:ClientWriteat 164 of 170 average active sessions andLock:transactionidat 0.04. Each call also held a pooled connection, so the heartbeat writes timed out, the leader declared healthy nodes stale, and reassigning their agents churned the very table being read.The change
ActiveNodeNumberCachefetches the numbers at most once perScheduledJobPollingTimefor the whole node, andDurabilityAgentasks it instead of the store. Same shape asPersistenceMetricsSweeperfor the metrics polling in GH-3375 — aConditionalWeakTablekeyed on the runtime, so one instance per node and nothing to dispose.Behaviour is unchanged: the caller already tolerates data one interval old, since that is how often it looks. Failures are deliberately not cached and not swallowed —
DurabilityAgentkeeps its owntry/catchand decides what a failed lookup means, and a failure leaves the previous value in place rather than remembering an empty one as the truth.The double-check inside the gate is what does the collapsing: the databases whose timers queue up behind the first caller are served by its fetch.
Tests
active_node_number_cache_tests— five tests, green:Not in scope
LoadAllNodesAsyncitself still returns assignments for every caller, including the ones that only want nodes. Narrowing that means an interface change across four persistence implementations, so it seemed worth keeping separate from the fix for the hot path.https://claude.ai/code/session_01LZkaXgbeEgiT9BrZR4aXsb