Skip to content

Commit 4ef9eb3

Browse files
nkaradzhovclaude
andauthored
feat(sentinel): emit connect/ready/reconnecting/end lifecycle events (#3430)
* feat(sentinel): emit connect/ready/reconnecting/end lifecycle events 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 #3012. Supersedes #3276 (abandoned). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sentinel): emit reconnecting only on real failover; harden lifecycle 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> * fix(sentinel): honest readiness on failed reconfigure; correct error 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> * fix(sentinel): clear #destroy on teardown so reconnect after close works 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> * test(sentinel): pin reopen-after-close lifecycle event sequence 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> * docs(sentinel): note failed connect() emits connect then end 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> * fix(sentinel): release the reserved client lease on teardown 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> * fix(sentinel): let destroy() await the in-flight connect when a connect 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> * docs(sentinel): qualify the standalone failed-connect comparison 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> * fix(sentinel): release the reserved lease even when an end listener throws 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> * fix(sentinel): keep a reentrant connect() tracked past the failed attempt'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> * fix(sentinel): surface throwing ready/reconnecting listeners on the error 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> * fix(sentinel): serialize overlapping close()/destroy() calls 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> * fix(sentinel): start a new teardown generation before emitting end #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> * fix(sentinel): defer lifecycle-listener errors and let destroy() preempt a blocked close() A throwing ready/reconnecting listener was routed to `error` synchronously from inside #connect()'s topology-retry loop; with no `error` listener the re-emit re-threw and the retry mistook it for a discovery failure, swallowing it (plus a spurious rediscovery). Defer the routed emit to a microtask so it escapes the retry boundary and surfaces as documented. Now that teardown is serialized on a shared #teardownPromise, a destroy() overlapping an in-flight graceful close() merely joined it — so a close() blocked draining a command queue made destroy() hang too. Share a #teardown(kind) coalescer; an overlapping destroy() preempts the close by force-destroying the draining clients (RedisClient.close() never settles once destroyed, so #doClose() races the drain against an escalation signal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sentinel): force-terminate draining clients when destroy() preempts close() The destroy()-preempts-close() escalation force-destroyed clients behind an `if (client.isOpen)` guard, but a graceful close() has already flipped each client to isOpen === false while leaving its socket draining — so the guard skipped exactly the clients that had to be terminated, and RedisSocket.destroy() would have thrown ClientClosedError on them anyway. The pending blocking command was never rejected and the socket lingered. The previous test stubbed close(), so its clients stayed open and never exercised this path. Make RedisSocket.destroy() force-kill a closing socket (throw only when nothing remains: not open and no socket), drop the isOpen guard in the sentinel's force-destroy (swallowing ClientClosedError for a client whose drain already finished), and replace the stubbed test with a real blocking command that asserts the command is actually rejected on preemption. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(client): settle close() on forced socket teardown; track reentrant sentinel teardown Two follow-ups to the destroy()-preempts-close() work: - RedisClient.close() waited only for a `data` event to confirm the queue had drained, then tore the socket down itself. When a concurrent destroy() (e.g. sentinel's escalation) force-kills the draining socket first, no further `data` arrives, so the close() promise hung forever — leaking the promise and its listeners. close() now also settles on the socket's `end`, which destroySocket() emits, without double-tearing-down. - RedisSentinelInternal#teardown() assigned #teardownPromise only after invoking #doDestroy()/#doClose(). On an established sentinel #doDestroy() runs to completion — emitting `end` — before that assignment, so a destroy() started by an `end` listener registered a new teardown generation that the outer call then overwrote, leaving it untracked. Register the outer teardown only if a reentrant one has not already claimed the slot (same identity guard used elsewhere). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3c6a7e8 commit 4ef9eb3

7 files changed

Lines changed: 569 additions & 32 deletions

File tree

docs/sentinel.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,32 @@ const sentinel = await createSentinel({
8585
| passthroughClientErrorEvents | `false` | When `true`, error events from client instances inside the sentinel will be propagated to the sentinel instance. This allows handling all client errors through a single error handler on the sentinel instance. |
8686
| reserveClient | `false` | When `true`, one client will be reserved for the sentinel object. When `false`, the sentinel object will wait for the first available client from the pool. |
8787

88+
## Events
89+
90+
The sentinel object is an `EventEmitter` and emits the following lifecycle events, tracking its own `isOpen`/`isReady` state (see [`client.isReady`](../README.md#events) for the standalone client equivalents):
91+
92+
| Name | When | Listener arguments |
93+
| -------------- | --------------------------------------------------------------------------------------------------- | ------------------ |
94+
| `connect` | The sentinel starts opening a connection (`isOpen` becomes `true`) | _No arguments_ |
95+
| `ready` | The sentinel has discovered the topology and is ready to accept commands (`isReady` becomes `true`) | _No arguments_ |
96+
| `reconnecting` | The monitored master actually changed, so the sentinel is reconfiguring and momentarily not ready (`isReady` becomes `false`). Not emitted for routine topology scans that change nothing. | _No arguments_ |
97+
| `end` | The connection has been closed via `close()` or `destroy()` (`isOpen` becomes `false`) | _No arguments_ |
98+
| `error` | An error on the sentinel itself (e.g. topology discovery failed, or a `ready`/`reconnecting` listener threw). Errors from the underlying master/replica/sentinel clients are included here **only when** [`passthroughClientErrorEvents: true`](#createsentinel-configuration); otherwise they are not surfaced on the sentinel object. May be a formatted string rather than an `Error`. | `(error: Error \| string)` |
99+
| `topology-change` | The monitored topology changed (master/replica added, removed, or promoted) | `(event: RedisSentinelEvent)` |
100+
101+
```javascript
102+
sentinel
103+
.on('connect', () => console.log('sentinel connecting'))
104+
.on('ready', () => console.log('sentinel ready'))
105+
.on('reconnecting', () => console.log('sentinel reconfiguring after a topology change'))
106+
.on('end', () => console.log('sentinel closed'))
107+
.on('error', err => console.error('Redis Sentinel Error', err));
108+
```
109+
110+
> :warning: You **MUST** listen to `error` events. Without at least one `error` listener, an emitted error is thrown and crashes the process.
111+
112+
**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 `connect` only once the socket is established: a standalone `connect()` that never reaches the server emits nothing, though it still emits `connect` before rejecting when the socket connects but initialization (e.g. authentication) fails afterwards.
113+
88114
## Reconnecting after an outage
89115

90116
As the client learns the sentinel topology it discovers additional sentinel nodes (reported by the sentinels as IP addresses). The nodes you pass in `sentinelRootNodes` are kept as **seeds**: they are always retained as reconnection candidates and are tried first, alongside the discovered nodes. This matters after an outage where the whole sentinel set restarts.

packages/client/lib/client/index.spec.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1698,6 +1698,20 @@ describe('Client', () => {
16981698

16991699
server.close();
17001700
});
1701+
1702+
// A blocking command keeps the queue from draining, so close() waits; a concurrent
1703+
// destroy() force-kills the socket, and close() must settle instead of hanging forever
1704+
// on a `data` event a destroyed socket never delivers.
1705+
testUtils.testWithClient('settles when a concurrent destroy() force-kills the draining socket', async client => {
1706+
const blocked = client.blPop('close-destroy-block-key', 0).then(() => 'resolved', () => 'rejected');
1707+
await new Promise<void>(resolve => { globalThis.setTimeout(resolve, 50); }); // let BLPOP reach the wire
1708+
1709+
const closing = client.close();
1710+
client.destroy();
1711+
1712+
await closing; // hangs (test times out) if close() never settles
1713+
assert.equal(await blocked, 'rejected');
1714+
}, GLOBAL.SERVERS.OPEN);
17011715
});
17021716
});
17031717

packages/client/lib/client/index.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2206,15 +2206,26 @@ export default class RedisClient<
22062206
return resolve();
22072207
}
22082208

2209+
// A concurrent destroy() can force-kill the draining socket; it tears the socket down
2210+
// itself and emits `end`, so settle then too — otherwise we would wait forever for a
2211+
// `data` event that a destroyed socket never delivers, leaking this promise and its
2212+
// listeners.
2213+
const onEnd = () => {
2214+
this._self.#socket.off('data', maybeClose);
2215+
this._self.#unregisterFromMetrics();
2216+
resolve();
2217+
};
22092218
const maybeClose = () => {
22102219
if (!this._self.#queue.isEmpty()) return;
22112220

22122221
this._self.#socket.off('data', maybeClose);
2222+
this._self.#socket.off('end', onEnd);
22132223
this._self.#unregisterFromMetrics();
22142224
this._self.#socket.destroySocket();
22152225
resolve();
22162226
};
22172227
this._self.#socket.on('data', maybeClose);
2228+
this._self.#socket.once('end', onEnd);
22182229
});
22192230
}
22202231

packages/client/lib/client/socket.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -485,13 +485,16 @@ export default class RedisSocket extends EventEmitter {
485485
}
486486

487487
destroy() {
488-
// Idempotent: return instead of throwing when already closed. A terminal
489-
// connect failure (reconnectStrategy gave up) leaves #isOpen === false, and
490-
// the owning client still needs to dispose itself (unregister metrics,
491-
// dispose credentials) — throwing here would abort that cleanup. Returning
492-
// also means a repeated destroy() won't re-run destroySocket() and
493-
// republish CONNECTION_CLOSED / re-emit 'end'.
494-
if (!this.#isOpen) return;
488+
// Idempotent: return instead of throwing when there is nothing left to tear down.
489+
// A terminal connect failure (reconnectStrategy gave up) leaves #isOpen === false, and
490+
// the owning client still needs to dispose itself (unregister metrics, dispose
491+
// credentials) — throwing here would abort that cleanup. Returning also means a
492+
// repeated destroy() won't re-run destroySocket() and republish CONNECTION_CLOSED /
493+
// re-emit 'end'. But a graceful close() also leaves #isOpen === false while keeping the
494+
// socket alive to drain, so key the guard on the socket too: while one remains, destroy()
495+
// must still force-terminate it (e.g. when a caller escalates close() to destroy()).
496+
if (!this.#isOpen && !this.#socket) return;
497+
495498
this.#isOpen = false;
496499
this.destroySocket();
497500
}

packages/client/lib/sentinel/index.spec.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -966,6 +966,33 @@ describe('legacy tests', () => {
966966
assert.notEqual(masterNode!.port, newMaster.port);
967967
});
968968

969+
// a failover drops readiness while reconfiguring (emits `reconnecting`) and
970+
// restores it once the new topology is connected (re-emits `ready`)
971+
it('emits reconnecting then ready across a failover', async function () {
972+
this.timeout(60000);
973+
974+
sentinel = frame.getSentinelClient();
975+
sentinel.setTracer(tracer);
976+
sentinel.on('error', () => { });
977+
await sentinel.connect();
978+
979+
let sawReconnecting = false;
980+
let readyAfterReconnectResolve: () => void;
981+
const readyAfterReconnect = new Promise<void>(res => { readyAfterReconnectResolve = res; });
982+
sentinel
983+
.on('reconnecting', () => { sawReconnecting = true; })
984+
.on('ready', () => { if (sawReconnecting) readyAfterReconnectResolve(); });
985+
986+
const masterNode = sentinel.getMasterNode();
987+
tracer.push(`stopping master node`);
988+
await frame.stopNode(masterNode!.port.toString());
989+
990+
// resolves only after a `reconnecting` followed by a `ready`; otherwise the
991+
// test times out, which is the failure signal we want
992+
await readyAfterReconnect;
993+
assert.ok(sawReconnecting);
994+
});
995+
969996
// if master changes, client should make sure user knows watches are invalid
970997
it('watch across master change', async function () {
971998
this.timeout(60000);

0 commit comments

Comments
 (0)