Skip to content

Commit cc95778

Browse files
committed
fix(redis): leave the readiness wait exactly once per waiter
A waiter that timed out was still subscribed to the shared readiness signal, so when that signal later settled for another waiter its cleanup ran a second time. The waiter count drifted negative, the last-waiter teardown could never match again, and the settled signal stayed memoized — a later subscribe during a reconnect would have observed a readiness that had already passed. Each waiter now leaves exactly once, and a settling signal clears itself from the memo so the next waiter observes the connection afresh regardless of the count.
1 parent 28b482d commit cc95778

2 files changed

Lines changed: 61 additions & 5 deletions

File tree

apps/sim/lib/execution/execution-signal.test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,50 @@ describe('ExecutionSignalHub', () => {
427427
}
428428
})
429429

430+
it('keeps its waiter accounting exact when one waiter times out before the signal settles', async () => {
431+
vi.useFakeTimers()
432+
try {
433+
connection.status = 'connect'
434+
const hub = getExecutionSignalHub()
435+
// Waiter A will time out; waiter B, started later, is still waiting when it does.
436+
const early = hub.subscribe('execution-early', vi.fn())
437+
const earlySettled = vi.fn()
438+
void early.then(earlySettled, earlySettled)
439+
await vi.advanceTimersByTimeAsync(readyBudgetMs() - 1000)
440+
const late = hub.subscribe('execution-late', vi.fn())
441+
await vi.advanceTimersByTimeAsync(1000)
442+
await expect(early).rejects.toThrow('Timed out waiting for Redis subscriber readiness')
443+
444+
// The signal settles for B — and must not run A's cleanup a second time.
445+
connection.status = 'ready'
446+
connection.client?.emit('ready')
447+
await late
448+
expect(connection.client?.listenerCount('ready')).toBe(1)
449+
expect(connection.client?.listenerCount('end')).toBe(0)
450+
451+
// Connection drops again: a new subscribe must wait for a fresh ready,
452+
// not reuse a readiness that has already passed.
453+
connection.status = 'connect'
454+
connection.client?.emit('close')
455+
mockSubscribe.mockClear()
456+
const again = hub.subscribe('execution-again', vi.fn())
457+
const againSettled = vi.fn()
458+
void again.then(againSettled, againSettled)
459+
await vi.advanceTimersByTimeAsync(readyBudgetMs() - 1)
460+
expect(againSettled).not.toHaveBeenCalled()
461+
expect(mockSubscribe).not.toHaveBeenCalled()
462+
463+
connection.status = 'ready'
464+
connection.client?.emit('ready')
465+
await again
466+
expect(mockSubscribe).toHaveBeenCalledOnce()
467+
expect(connection.client?.listenerCount('ready')).toBe(1)
468+
expect(vi.getTimerCount()).toBe(0)
469+
} finally {
470+
vi.useRealTimers()
471+
}
472+
})
473+
430474
it('warms to true once the subscriber becomes ready', async () => {
431475
connection.status = 'connecting'
432476
const warm = warmExecutionSignalHub()

apps/sim/lib/execution/execution-signal.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,12 @@ class RedisExecutionSignalHub implements ExecutionSignalHub {
197197
const signal = (this.readySignal ??= this.createReadySignal())
198198
this.readyWaiters++
199199
return new Promise<void>((resolve, reject) => {
200+
// A waiter that timed out is still subscribed to the signal, so it must
201+
// leave exactly once whichever of its deadline or the signal fires first.
202+
let left = false
200203
const leave = () => {
204+
if (left) return
205+
left = true
201206
clearTimeout(timeout)
202207
if (--this.readyWaiters === 0 && this.readySignal === signal) {
203208
signal.detach()
@@ -223,13 +228,20 @@ class RedisExecutionSignalHub implements ExecutionSignalHub {
223228

224229
private createReadySignal(): ReadySignal {
225230
let detach = () => {}
226-
const promise = new Promise<void>((resolve, reject) => {
231+
const signal: ReadySignal = { promise: Promise.resolve(), detach: () => detach() }
232+
// A settled signal describes a moment that has passed; the next waiter must
233+
// observe the connection afresh rather than a readiness that may be gone.
234+
const settle = () => {
235+
detach()
236+
if (this.readySignal === signal) this.readySignal = undefined
237+
}
238+
signal.promise = new Promise<void>((resolve, reject) => {
227239
const onReady = () => {
228-
detach()
240+
settle()
229241
resolve()
230242
}
231243
const onEnd = () => {
232-
detach()
244+
settle()
233245
reject(new Error('Redis subscriber connection ended'))
234246
}
235247
detach = () => {
@@ -241,8 +253,8 @@ class RedisExecutionSignalHub implements ExecutionSignalHub {
241253
})
242254
// Detached before settling, this promise is simply dropped; an `end` that
243255
// arrives after every waiter has left must not surface as unhandled.
244-
promise.catch(() => undefined)
245-
return { promise, detach }
256+
signal.promise.catch(() => undefined)
257+
return signal
246258
}
247259

248260
private async handleReady(): Promise<void> {

0 commit comments

Comments
 (0)