Bound the cached node-number release by a high-water mark (GH-3850) - #3851
Merged
Conversation
Follow-up to GH-3846/#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 #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 #3850. Follow-up to #3847 (merged), which cached the active node numbers per node instead of fetching them per database.
That trade is right — the failure #3847 broke was self-amplifying (pool exhaustion → heartbeat timeouts → the leader declaring healthy nodes stale → reassignment churning the table being read). This PR handles the window it widens.
The staleness is not symmetric
ReleaseOrphanedMessagesForAncillaryOperationresetsowner_idto 0 for every owner missing from the cached list:owner_id = 0A cached list cannot describe a node that registered after it was taken, and
MessageRoute.cs:197stampsOwnerIdon outbox persist — so a newcomer owns rows within milliseconds of registering. Registration does strictly precede ownership (PersistAsyncreturns the number beforeOptions.Durability.AssignedNodeNumberis set), which is what keeps this narrow, but it does not close it.Worth noting the Main store was never exposed:
ReleaseOrphanedMessagesOperationevaluatesnot in (select node_number from wolverine_nodes)in-transaction. Only ancillary databases take the cached list.Why not
max(active)Node numbers are database-generated and monotonic —
SERIALon Postgres (PostgresqlMessageStore.cs:695),AutoNumber()on SQL Server (SqlServerMessageStore.cs:570) — so the obvious guard isand owner_id <= max(cached list).That is wrong, and the reason is worth recording. If the highest-numbered node dies, its number leaves the active list, the max drops below it, and its orphaned messages become permanently unreclaimable. Trading a rare duplicate for a permanent leak is a bad trade.
ActiveNodeNumberCachetherefore keeps a high-water mark — the highest number it has ever observed, raised across fetches and never lowered:Remaining gap, stated in the code: a node that both registers and departs between two fetches is never observed, so its rows stay owned until the mark rises. That parks messages rather than double-processing them — the safe direction — and the next registration clears it.
Also
Documents the gate's cost, which #3847 left implicit: one
SemaphoreSlimmeans a hungLoadAllNodesAsyncparks every database's timer callback rather than each failing on its own timeout. That is the intended trade (one slow query beats one per database), but someone diagnosing a stall should not have to derive it from the code.Tests
Four on the cache — the mark is the highest seen, it does not drop when the highest node departs, it rises for a joiner, and a node joining mid-interval sits above the mark. Four on the generated SQL, including that both the inbox and outbox statements carry the bound.
Verified non-vacuous: disabling the ceiling turns 3 of the 4 SQL tests red. The fourth stays green by design — it asserts the guard is absent when no mark is supplied, so
0restores the un-bounded behaviour rather than releasing nothing.CoreTests— 2264/0SqlServerTestsorphan-release suites — 7/7 (includes the pre-existing Solo tests that call the changedbuildOperationBatchsignature)dotnet build wolverine.slnx -c Release -f net9.0— clean🤖 Generated with Claude Code
https://claude.ai/code/session_01WHAuhdWS3XeAk16swV9G8m