feat(sentinel): emit connect/ready/reconnecting/end lifecycle events - #3430
feat(sentinel): emit connect/ready/reconnecting/end lifecycle events#3430nkaradzhov wants to merge 14 commits into
Conversation
RedisSentinel exposed isOpen/isReady but never emitted the lifecycle events the standalone client provides, so applications could not observe sentinel startup, failover, or shutdown via events (only `error` and `topology-change` were emitted). Wire the events to the internal state transitions via #setOpen/#setReady setters (single source of truth) rather than scattering emits through connect/close/destroy: `connect`/`end` track isOpen, `ready` tracks isReady, and a readiness drop during a reconfigure emits `reconnecting`. The setters make `end` fire at most once across repeated close()/destroy() and let `reconnecting`/re-`ready` fall out of the failover path (#reset). The public facade forwards the new events from the internal emitter. Closes redis#3012. Supersedes redis#3276 (abandoned). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b235285fce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…cle events Addresses automated review of the lifecycle-events change: - reconnecting: drive the readiness drop from transform() only when the master actually changes (analyze() leaves masterToOpen undefined otherwise), instead of from every #reset(). A healthy periodic scan (scanInterval) no longer emits a spurious reconnecting/ready cycle. - coalescing: revert #reset() to leave #isReady untouched and check the in-flight #connectPromise BEFORE the readiness gate, so a control event arriving during a reconfigure still registers via #anotherReset (no dropped topology update). - spurious ready: #setReady(true) no-ops while #destroy is set, so an in-flight connect aborted by close()/destroy() cannot emit ready after end. - re-entrancy: assign #connectPromise before emitting connect, so a listener that calls close()/destroy() from the event awaits the in-flight attempt. - failed reconfigure: restore #isReady silently so later control events can retry. - docs: distinguish sentinel-level error (may be a string) from client-error, and note reconnecting fires only on a real master change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3db3a4a83e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…docs Second round of automated review: - Gate #reset() on #isOpen instead of #isReady and drop the silent readiness restore. A failed reconfigure now honestly stays not-ready (no false isReady, no dangling reconnecting) while later control events can still retry and re-emit ready, because the gate no longer depends on readiness. - docs: client-error is emitted only on the internal and is not forwarded to the public sentinel, so document that underlying-client errors reach `error` only when passthroughClientErrorEvents is true (removed the misleading client-error row). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f1315a0b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Third round of automated review: - close() never cleared #destroy (only destroy() did), so a later connect() hit #connect()'s teardown guard and returned immediately — emitting `connect` but never `ready`, leaving isOpen=true with no topology. close() now clears #destroy at the end, mirroring destroy(). - Clear #destroy BEFORE emitting `end` (via #setOpen) in both close() and destroy(), so a reentrant connect() from an `end` listener sees teardown finalized and reopens cleanly instead of landing in a half-open state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Took this one apart on a live sentinel deployment rather than by reading, since the whole value of the PR is the event ordering and that is exactly what reading can't confirm. Returning the favour for the review on #3428. Short version: the design holds up under every reentrancy and ordering case I could construct, including a real failover. One test-coverage suggestion and one doc nit, both non-blocking. The structural claim checks out
So there is no path that can emit Executed evidenceEnvironment: real master
Plus: A few of these are worth calling out specifically, because they're the ones the bots were worried about: #8 is non-vacuous. "No events during healthy scans" passes trivially if the scan timer never fires, so I attached the built-in tracer and counted actual reconfigure cycles: 7 #7 is the one I most wanted to see fail and didn't. Exactly one #6 and #9 confirm the reentrancy reasoning. Assigning #3 confirms the One suggestion: the reopen path has no testCase #3 above is the one behaviour change in this PR with no test covering it, and it's also the subtlest: It's cheap to pin, in the same style as the existing cases in testUtils.testWithClientSentinel('reopens after close(): connect+ready emitted again', async sentinel => {
const events: Array<string> = [];
sentinel
.on('connect', () => events.push('connect'))
.on('ready', () => events.push('ready'))
.on('end', () => events.push('end'))
.on('error', () => { });
await sentinel.connect();
await sentinel.close();
await sentinel.connect();
assert.deepEqual(events, ['connect', 'ready', 'end', 'connect', 'ready']);
await sentinel.destroy();
}, OPEN);That's the exact sequence I observed on this head, so it should go green as-is. One doc nitThe Checked, nothing to doThe four new events aren't typed on the public interface — but Test harness usage is also correct: Nice, tightly-scoped change — the decision to route every transition through two setters is what makes it auditable, and the deferrals on |
close() must clear #destroy before #setOpen(false); nothing else in the suite or type system holds that ordering in place. Covers connect() -> close() -> connect() re-emitting the full connect/ready pair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unlike the standalone client (connect = established socket, emits nothing on a failed attempt), the sentinel emits connect at the start of the attempt, so a failed connect() emits connect followed by end before rejecting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abde08a4da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Clear #destroy before emitting `end` (via #setOpen) and before returning, so a | ||
| // later connect() — or a reentrant connect() from an `end` listener — is not left | ||
| // half-open by #connect()'s teardown guard. Mirrors destroy(). | ||
| this.#destroy = false; |
There was a problem hiding this comment.
When reserveClient: true, the first connection removes a lease from the master queue and stores it in #reservedClientInfo, which is not cleared by close(). Since clearing #destroy here now makes reopening supported, a subsequent connect() unconditionally requests another lease; with the default one-client master pool, none is available and the second connect() hangs after the internal client has already emitted ready. Reuse the existing reservation or release and clear it during teardown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 67b0305: teardown now releases the reserved lease, so reopening with reserveClient no longer hangs. Test added.
| this.#destroy = false; | ||
| this.#setOpen(false); |
There was a problem hiding this comment.
When two close() calls overlap, the first can be awaiting its node-client close promises after clearing the client arrays, while the second sees empty arrays, reaches this assignment immediately, and emits end. A caller awaiting the second close can then reconnect, but when the first close eventually resumes it destroys the shared PubSub proxy and calls #setOpen(false), leaving the reopened facade closed while its newly created master clients remain live. Track and await a single teardown operation, or otherwise keep the teardown guard active until every overlapping close has completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — real but needs overlapping close() calls plus an instant reconnect; serializing teardown changes destroy-during-close semantics, so deferring to a follow-up.
There was a problem hiding this comment.
Revisited after a second independent confirmation — fixed in db5749a: close()/destroy() join a single in-flight teardown promise. Test covers close+destroy racing with a reconnect from the end listener.
|
|
||
| > :warning: You **MUST** listen to `error` events. Without at least one `error` listener, an emitted error is thrown and crashes the process. | ||
|
|
||
| **Divergence from the standalone client:** these events track the sentinel facade's own state, not the sockets of the individual master/replica node clients. `reconnecting` is emitted only when the monitored master actually changes (a failover) — routine periodic topology scans that find no change are silent — and `ready` is re-emitted once the new master is connected; the per-socket reconnect churn of the underlying node clients is not surfaced. Because the sentinel abstracts failover away, a `connect`/`ready` pair is emitted once on the initial `connect()`, and `end` fires at most once even if `close()` and `destroy()` are both called. Also note that `connect` marks the start of the connection attempt (not an established socket), so a `connect()` that fails emits `connect` followed by `end` before rejecting — whereas the standalone client emits nothing in that case. |
There was a problem hiding this comment.
The standalone client does not always emit nothing when connect() fails: if the TCP/TLS socket connects but initialization subsequently fails, such as an invalid AUTH or SELECT, RedisSocket.#connect() emits connect before awaiting the initiator and its public connect() can still reject afterward. This statement therefore gives users an incorrect event contract; qualify it to failures that happen before the socket is established or describe the initialization-failure case separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b5bfc9b: standalone emits nothing only when the socket never connects; connect still precedes an init-failure rejection.
| // close()/destroy() from within the event awaits the in-flight attempt instead | ||
| // of tearing down before it starts. Readiness (and `ready`) is set by #connect(). | ||
| this.#connectPromise = this.#connect(); | ||
| this.#setOpen(true); |
There was a problem hiding this comment.
When a public connect listener throws, this synchronous call throws after #connectPromise has already started observe(). The catch path clears the field before calling destroy(), so teardown cannot await that still-running discovery and resets #destroy to false; the abandoned #connect() can then resume, create persistent sentinel/master clients, and emit ready after end even though the public connect() rejected. Keep the in-flight promise reachable and cancel or await it before completing teardown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 60a1d4b: destroy() now awaits the in-flight attempt, so a throwing connect listener settles closed — no ready after end. Test added.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit abde08a. Configure here.
The master client queue is only filled at construction. With reserveClient: true and the default one-client pool, connect() took the only lease and close()/destroy() never returned it, so a reopening connect() waited forever in getClientLease(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ct listener throws connect()'s catch cleared #connectPromise before calling destroy(), so when a `connect` listener threw, teardown could not await the still-running discovery. That attempt then resurrected clients and emitted `ready` after `end` with the facade reporting isOpen=false. The finally already clears #connectPromise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The standalone client emits `connect` on an established socket, so it does emit `connect` before rejecting when initialization (e.g. AUTH) fails after the socket is up; only a connect() that never reaches the server emits nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5bfc9b79f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async close() { | ||
| return this._self.#internal.close(); | ||
| await this._self.#internal.close(); | ||
| this._self.#releaseReservedLease(); |
There was a problem hiding this comment.
When reserveClient: true, an end listener that throws makes #internal.close() reject after the internal teardown has completed, so execution never reaches #releaseReservedLease(); destroy() has the same sequencing. The Sentinel can then be reopened, but connect() waits forever for the only default pool slot because the old reservation was never returned. Run the lease release in a finally block so listener exceptions cannot strand it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f61e80c: the lease release runs in a finally, so a throwing end listener cannot strand it. Test added.
| this.#isReady = value; | ||
| if (value) { | ||
| this.emit('ready'); | ||
| } else if (this.#isOpen && !this.#destroy) { | ||
| this.emit('reconnecting'); |
There was a problem hiding this comment.
When a newly added ready or reconnecting listener throws, #setReady has already changed the state and its emit() is still inside #connect()'s topology-retry try block. The exception is therefore treated as a discovery failure; after the default one-second delay, the next attempt skips the same event because the state already changed and succeeds, silently swallowing the listener exception while performing an unnecessary rediscovery. Emit these lifecycle events outside the topology retry boundary, or otherwise distinguish listener failures so they reach the initiating caller/background error path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1819e9d: listener exceptions from these emits route to error instead of masquerading as discovery failures inside the retry loop. Test added.
…hrows A throwing `end` listener makes the internal close()/destroy() reject after teardown has completed, skipping the lease release and reviving the reopen hang with reserveClient. Run the release in a finally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…empt's finally connect()'s and #reset()'s finally blocks cleared #connectPromise unconditionally. An `end` listener that reconnects during the failed attempt's teardown re-assigns the field, and the clobber left the new discovery untracked — close()/destroy() no longer awaited it, so it could leak clients and emit `ready` after `end`. Clear only when the promise is still our own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rror event These emits fire inside #connect()'s topology-retry loop, so a throwing listener was treated as a discovery failure: a pointless one-second rediscovery, and the exception silently swallowed because the state had already flipped. Route listener exceptions to `error` instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A second teardown pass over already-emptied client arrays finished early, emitted `end` while the first pass was still awaiting its client-close promises, and the first pass's tail then destroyed the pub/sub proxy and flipped isOpen on a sentinel that an `end` listener had since reopened. Both methods now join a single in-flight teardown promise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db5749a853
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.#teardownPromise ??= this.#doDestroy().finally(() => { | ||
| this.#teardownPromise = undefined; | ||
| }); |
There was a problem hiding this comment.
When an end listener starts connect() and a later end listener—or the nested connect handler—calls destroy(), #teardownPromise still refers to the teardown currently emitting end. This ??= therefore returns that nearly completed promise without running #doDestroy() against the new connection; after the old teardown finishes, the reentrant attempt can create clients and emit ready, so destroy() has resolved while the sentinel remains open. Distinguish teardown of the reopened generation from overlapping calls targeting the old one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3aec96: #teardownPromise is cleared before end is emitted (new generation) and the wrapper's finally is identity-guarded, so a destroy() from inside the emit tears down the reentrant reopen instead of joining the finished pass. Test added.
#teardownPromise was cleared only after `end` listeners ran, so a destroy() called from inside the emit — after an `end` listener had reentrantly reconnected — joined the nearly-finished old teardown and resolved without touching the reopened sentinel, which went on to emit `ready`. Clear the promise before the emit so such calls start a fresh teardown against the new generation, and identity-guard the wrapper's finally so it cannot wipe that newer generation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3aec96671
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async destroy() { | ||
| if (this.#teardownPromise === undefined) { |
There was a problem hiding this comment.
When close() is already waiting for pending commands, a subsequent destroy() reaches this method with #teardownPromise set and merely returns the graceful-close promise instead of invoking #doDestroy(). The underlying RedisClient.close() explicitly waits for its command queue to empty, so a blocking command with commandOptions.timeout: undefined can keep both calls pending indefinitely even though destroy() is expected to reject commands and terminate immediately. Preserve teardown serialization while allowing a destructive request to upgrade or cancel an in-progress close.
Useful? React with 👍 / 👎.
| } catch (err) { | ||
| this.emit('error', err); |
There was a problem hiding this comment.
Fresh evidence in the current code is that the replacement error emission is still inside #connect()'s topology-retry boundary: when a ready or reconnecting listener throws and the public sentinel has no error listener, this line throws through the forwarding handler, the outer retry loop catches it as a discovery failure, and the next iteration skips the lifecycle event because #isReady was already changed. The initiating operation can therefore succeed after a one-second delay while silently swallowing the exception, contrary to the documented unhandled-error behavior; dispatch or propagate this error outside the topology retry path.
Useful? React with 👍 / 👎.
2 participants
This pull request resolves #3012 by making
RedisSentinelemit theconnect,ready,reconnecting, andendlifecycle events, matching the events the standalone client already provides.Background
RedisSentinelexposedisOpen/isReadybut never emitted lifecycle events — onlyerrorandtopology-change. Applications therefore had no event-driven way to observe sentinel startup, failover, or shutdown, which #3012 asked for. This also supersedes #3276 (abandoned), and follows the review direction given there.What changed
#setOpen/#setReadysetters (single source of truth), instead of scatteringemit(...)calls acrossconnect()/close()/destroy():#setOpen(true|false)→connect/end#setReady(true)→ready#setReady(false)while still open and not tearing down →reconnecting#reset()(the reconfigure/failover path) now drops readiness (reconnecting) and restores it once the new topology is connected (ready).RedisSentinelfacade forwards the new events from the internal emitter (it previously forwarded onlyerror/topology-change).docs/sentinel.md, including the divergences from the standalone client.Behavior notes
endfires at most once, even if bothclose()anddestroy()are called (the setter guards on the actual transition).reconnectingonce while it reconfigures andreadyagain afterward; it does not surface the per-socket reconnect churn of the underlying master/replica node clients (the facade tracks its own state, not their sockets).isReadyis brieflyfalse. This flag is not read on the command-dispatch path, so command routing is unchanged; it only affects the#resetre-entry guard and the publicisReadygetter (which now correctly readsfalsemid-reconfigure).ready, so background resets can keep retrying (matches prior behavior whereisReadystayed effectively usable across a failed background reset).Tests
packages/client/lib/sentinel/lifecycle-events.spec.ts— deterministicconnect→ready→end,destroy()path, andend-fires-exactly-once across repeatedclose()/destroy().packages/client/lib/sentinel/index.spec.ts— a framework-backed failover test assertingreconnectingthen re-readyafter the master is stopped.Closes #3012.
🤖 Generated with Claude Code
Note
Medium Risk
Touches connect/close/destroy and failover reconfigure paths with reentrant listener edge cases; behavior change is intentional but apps relying on old silent
isReadyduring scans may now observe events.Overview
RedisSentinelnow emitsconnect,ready,reconnecting, andend(plus existingerror/topology-change), aligned with standalone client ergonomics for #3012.Lifecycle is driven by
#setOpen/#setReadyon the internal layer: the public sentinel forwards those events;reconnectingfires when readiness drops during a real master change (not idle scans), thenreadyagain after reconfigure.endis deduped across repeatedclose()/destroy().Connect/teardown behavior is tightened for event listeners and
reserveClient: coalesced#teardownPromise, guarded#connectPromiseclearing for reentrantconnect()fromend,#destroycleared beforeendso reopen works,#releaseReservedLease()inclose()/destroy()finally, and#reset()gated on#isOpenso failed failovers can retry.Docs add an Events section in
docs/sentinel.md(including differences from the standalone client). Tests addlifecycle-events.spec.tsand a Docker failover case forreconnecting→ready.Reviewed by Cursor Bugbot for commit e3aec96. Bugbot is set up for automated code reviews on this repo. Configure here.