Repository navigation
fix(mq): keep one owner per shard under load, handover and stop - #696
Merged
Merged
Conversation
A shard owner's pull consumer ran the handler on its delivery goroutine and pulled again only once the handler returned. At the held-rows cap the handler blocked, the server saw no pull for longer than pinned_ttl and dropped the pin, and Unowned and ResetOrphaned then read a live owner as gone. Each unit now has a puller that never runs the handler and a deliverer that does. The puller fetches only what the unit's cap leaves room for; at the cap, and once halted, it sends pulls with max_bytes 1 that renew the pin and deliver nothing. Unowned and ResetOrphaned also require no delivery or ack within the durable's pinned_ttl. The held-rows cap is per unit: an even share of MaxHeld over the units the process owns, never below two batches, so one stuck table cannot stall the process's other shards. A clean stop halts every unit first, routes what they still deliver, flushes, and only then releases; a handover releases once its rows have settled (bounded by ack_wait). A row the worker drops while stopping is NAKed before the release. A durable deleted while no pull waits answers the next pull with no responders; the unit then looks the durable up and ends delivery with failed. A bind whose durable or stream is gone, or whose durable no longer fits, is fatal; a bind failure that lasts logs an error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
…ivery Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
The order test's first process now writes slower than a batch window, so a handover that released before its rows were written, or a stop that left rows unsettled, fails it. A delivery a consumer that cannot halt makes after the worker stopped reading is NAKed before and after the consumer's stop, not stranded until ack_wait. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
The deployment, pipeline and architecture pages said a stuck live owner keeps its pins, and that a process caps its held rows at 10,000 across its shards; neither held. They now describe the per-shard share, the renewing pulls, the halt before a clean stop's flush, the activity test before a takeover reset, and which bind failures end the worker. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
A renewal pull with max_bytes 1 holds any row back, and the server puts a redelivery it held back at the tail of its redelivery queue, so rows NAKed as 0, 1, 2, 3 came back as 2, 3, 0, 1 after two renewals. At the cap the renewal is now a one-row fetch, and they come back in order; a halted unit still renews without taking a row. The docs now describe that, the 1-second fetch against the 5 seconds between renewals, and the one-minute handover bound, and say rows redelivered across a handover may come back out of order among themselves. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Renewing with a one-row fetch at the cap took a row every 5 seconds however long the handler stayed blocked, and rows queued for a blocked handler never expire, so a stuck shard's held rows grew without end. A unit now takes a row to renew only while nothing waits for its handler and it is less than one ack_wait of renewals past its cap; otherwise it renews with a pull that delivers nothing. The docs say the cap is exceeded by at most that dozen rows, and that every pull waits a second at most. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Refusing a renewal row whenever rows waited for the handler sent a backed-up shard straight to the renewal that reorders due redeliveries, while the count bound alone already stops the growth: rows queued for the handler stay counted. A shard now takes renewal rows until it is one ack_wait of renewals past its cap, whatever the handler does, and a test with a shortened ack_wait holds it there. The docs state the condition and the 12 at the generated one-minute ack_wait. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
A halted unit renewed its pin for ack_wait from the halt, while a handover waits ack_wait from when the halt had handed its rows on, so a slow drain could end the renewals before the release. The renewals now run until ack_wait and one renewal after the drain. A renewal row at the cap also counts against the prefetch share of rows waiting for the handler. AGENTS.md names the new mq surface. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Also says, wherever the halted renewal bound is stated, that it runs ack_wait and one renewal from when the halt handed its rows on. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EricAndrechek
added a commit
that referenced
this pull request
Sep 30, 2026
Conflicts: - internal/mq/external.go: NewNATS's ExternalNATS literal keeps apiPrefix and activeWithin (#696) and the permission watch (#695). - CHANGELOG.md: the topology entry keeps #696's pinned TTL wording and #695's new manifests flags; both Fixed entries kept. Adds TestExternalNATS_PinRenewalUnderAJSDomain: a halted shard's raw pin renewals, sent under $JS.<domain>.API., keep the pin as the restricted user on a server in the domain and through a leafnode outside it, with no refusal recorded. The deployment docs say the domain prefix covers those pulls. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
7 tasks done
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.
Part of #613. Stacked on #624 (
feat/mq-nats-topology); merge into it, not intomain.This fixes four problems with shard ownership under
mq.backend: nats. Each one broke the promise that a table has one writer and keeps its order under load, across handovers and across rolling deploys.What changes
1. A busy owner keeps its pin
The pull consumer ran the worker's handler on its delivery goroutine and only pulled again once the handler returned. The server renews a pin only when it gets a pull. So a process that sat at its held-rows cap for longer than the pinned TTL (10s) lost its pins.
wavehouse_ingest_shards_unownedthen counted its shards, and a peer that took the process for dead reset them.internal/mq/external_consumer.go, moved out ofexternal.go):ack_waitof such rows past the cap (12 at the generated 1-minuteack_wait).max_bytes: 1. The server holds the row back, so that pull delivers nothing.max_bytes-1 renewals at the cap, rows NAKed as 0, 1, 2, 3 came back as 2, 3, 0, 1 (measured).ResetOrphanedandUnownednow also require that the durable delivered nothing and had no ack for its pinned TTL. A missing pin alone no longer means the owner is dead.2. Each shard gets its own budget
The process-wide cap of 10,000 held rows let one stuck table take every slot, which stalled every shard the process owned.
ConsumerConfig.MaxHeldand is read before every fetch.wavehouse_ingest_rows_held_waits_totalis removed, because nothing waits at the cap any more.3. A clean stop and a handover keep order
Before:
ack_wait, behind newer rows.Now:
mq.Halter) and keeps the pins. The worker writes what it holds plus whatever the shards still deliver. Only then does the stop release the shards.4. A deleted durable ends the worker
failed.mq.ErrConsumerMismatch), now ends the worker.The
releasecheck-then-unpin race is documented, not changed:Measurements
These were run against an in-process single-node nats-server v2.14.6.
ResetOrphanedansweredErrUnitHeld.failedfired 5s after the delete, with the handler busy.Tests
Each new test was checked by removing its fix and confirming the test then fails:
TestShardClaims_BlockedOwnerKeepsItsShardTestShardClaims_StuckShardDoesNotStallTheOthersTestShardClaims_TableOrderAcrossHandoverAndStopTestExternalNATS_DurableDeletedWhileBusyEndsDeliveryTestExternalNATS_RedeliveryOrderKeptAtTheCapTestExternalNATS_BlockedHandlerTakesABoundedOvershootTestExternalNATS_BlockedHandlerOvershootBoundedByTheShareTestExternalNATS_HaltKeepsThePinPastASlowDrainTestIngestWorker_StopHaltsBeforeTheFlushAndStopsAfterTestIngestWorker_StopNaksWhatArrivesAfterItStopsReadingTestClaims_UnitShareOfMaxHeldTestClaims_MissingDurableEndsDeliverymake cipasses locally.Left for later
ack_wait. It is documented.mqCLI are in a separate PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq