fix(cluster): retry a member that refused a live registration - #33
Merged
Merged
Conversation
…p specs Four specs each carried their own copy of the same mock: a subscribe() that refuses until a flag flips and then records the handler at completion, as RedisQueue.subscribe does. A fifth spec that needs it is about to land, so the copies become one `refusing()` helper next to the fake host it complements. It takes the target, which is a single host or RedisQueue.prototype for every host, and hands back the gate and the mock. No behaviour under test changes: 418 specs before and after.
The retry added for #31 was wired to the join only. subscribe() on a cluster that already has members fans syncHost() out to them with nothing behind it, so a member that refused its first subscribe ended exactly where a joining host used to: RedisQueue.subscribe() recorded no handler, the connection layer reconnected and restored a socket subscribed to no channel, and nothing asked again. A statically configured cluster takes no other path, because its hosts are added without a catch-up, so for it the defect in #31 was still fully open. The application could not repair it either. A rejected subscribe() is documented as not retryable: the registration stays remembered, and a second call registers a duplicate on every host that did accept it. Against a real broker behind a port that refuses first and then answers, measured 9 s after the address came up: scenario 3.5.2 after #32 now subscribe, then the host joins silent repaired repaired static member down at subscribe time silent silent repaired host joins, subscribe while it refuses silent silent repaired "silent" is a connected subscription socket, zero subscribers reported by redis, and a handler that never fires. The live path now hands a refusing member to the same scheduleSync() the join uses. Nothing else is new. The per-host timer guard already stops a second schedule while a retry is pending, and that retry installs the whole missing suffix, so a registration arriving meanwhile is covered by it. A join's announcement cannot be displaced: its catch-up is enqueued on the host's chain in the same synchronous step that makes the host a member, so its retry is always scheduled before a live registration's can be. scheduleSync()'s startup and announce arguments become optional, since a member has nothing to announce. subscribe() still rejects, so the caller learns a host was unreachable when it subscribed; it no longer means that host stays unsubscribed. The log line and the doc-blocks say so. One unit spec pins the wiring, and nothing in it touches scheduleSync. The suffix spec loses the manual scheduleSync() call it needed to get the same effect, which is what showed the wiring was missing. One integration spec drives a real broker through a statically configured member that refuses and then answers, and asks redis itself for the subscriber, since the live path announces nothing. Removing the wiring fails both unit specs and times the integration spec out.
restoreSubscription() clears the connection's `message` listeners before it re-attaches the remembered handlers. That is what stops a subscribe() racing a reconnect from leaving its handler attached twice and every message delivered twice. Removing that one line left every unit spec green. The only spec that saw it was the integration one, and CI has no broker, so there it is skipped and the line was unguarded. The race does not need a broker to be pinned, only its end state: a connection that already carries the handler when the restore runs. The spec subscribes, runs the restore again on the same connection, and asserts one listener and one delivery. With the line removed it fails on a listener count of 2. Against a real broker the race is not rare: without the reconcile, 18 of 20 runs of the join repair spec delivered the payload twice.
Contributor
|
All contributors have signed the @imqueue Contribution Terms. ✅ |
Member
Author
|
I have read the CLA Document and I hereby sign the CLA |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Follow-up to #32, closing the half of #31 it left open.
#32 retries a joining host whose subscription catch-up failed. The same failure reached through a live registration —
subscribe()on a cluster that already has members — still ended in a host that was connected,readyand subscribed to nothing for the life of the process. A statically configured cluster takes no other path: its hosts are added without a catch-up, so every registration reaches them live.The application cannot repair this itself. A rejected
subscribe()is documented as not retryable, because the registration stays remembered and a second call registers a duplicate on every host that did accept it.Against a real broker behind a port that refuses first and then answers, measured 9 s after the address came up:
"silent" is a connected subscription socket, zero subscribers reported by redis, and a handler that never fires.
The change
The live path hands a refusing member to the same
scheduleSync()the join uses, and nothing else is new. The per-host timer guard already stops a second schedule while a retry is pending, and that retry installs the whole missing suffix, so a registration arriving meanwhile is covered by it. A join's announcement cannot be displaced: its catch-up is enqueued on the host's chain in the same synchronous step that makes the host a member, so its retry is always scheduled before a live registration's can be.scheduleSync()'s startup and announce arguments become optional, since a member has nothing to announce.subscribe()still rejects, so the caller learns a host was unreachable when it subscribed. It no longer means that host stays unsubscribed. The log line, the doc-blocks andCHANGELOG.mdsay so.Tests
Three commits, each green on its own.
test(cluster): the four copies of the refusingsubscribemock become onerefusing()helper, ahead of the fifth spec that needs it. 418 specs before and after.fix(cluster): one unit spec pins the live wiring, and nothing in it touchesscheduleSync. The suffix spec loses the manualscheduleSync()call it needed to get the same effect, which is what showed the wiring was missing. One integration spec drives a real broker through a statically configured member that refuses and then answers, and asks redis itself for the subscriber, since the live path announces nothing.test(queue): pins the listener reconcile from fix(cluster): retry a joining host whose subscription catch-up failed #32. RemovingremoveAllListeners('message')left every unit spec green; only the integration spec saw it, and CI has no broker. The new spec subscribes, runs the restore again on the same connection, and asserts one listener and one delivery. Against a real broker the race is not rare: without the reconcile, 18 of 20 runs of the join repair spec delivered the payload twice.How it was verified
npm test: 420 pass, three runs.npm run test-integrationagainst a real broker: 22 pass, none skipped, and both repair specs 10 of 10.npm run lintandnpm run format:checkclean.Each new spec was checked by mutation. Removing the live wiring fails both unit specs and times the integration spec out. Removing the reconcile line fails the new queue spec on a listener count of 2.