Skip to content

test(client): fix three flaky tests around socket/connect lifecycle - #3299

Merged
nkaradzhov merged 4 commits into
redis:masterfrom
nkaradzhov:fix-flaky-socket-timeout-test
Jun 2, 2026
Merged

nkaradzhov merged 4 commits into
redis:masterfrom
nkaradzhov:fix-flaky-socket-timeout-test

Conversation

@nkaradzhov

@nkaradzhov nkaradzhov commented May 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Three flaky tests in packages/client consolidated into one branch, one commit each:

  • Socket > socketTimeout > should timeout with positive socketTimeout values — listener attachment race let the socket-timeout error surface as uncaught on slow CI.
  • Client > ConnectionTimeoutError — connectTimeout: 1 raced a 1ms event-loop block; on fast hardware the connect event landed before the timeout callback and the connection succeeded.
  • Client > PubSub > should resubscribe — once(subscriber, 'error') removed itself after the first emit, leaving the second (transient) error during the forced reconnect uncaught.

All three are wired up via the same root-cause shape: relying on a one-shot once listener for an event source that can fire more than once. The fix in each case is a permanent listener plus a one-shot promise gate to keep the test deterministic.

The ConnectionTimeoutError case is purely a timing margin bump.

Test plan

  • should timeout with positive socketTimeout values × 10 runs locally — all pass
  • ConnectionTimeoutError × 5 runs locally — all pass
  • should resubscribe × 10 runs locally — all pass
  • Full Socket spec — 11/11 pass
  • Full PubSub spec — 5/5 pass
  • CI on Node 18, 20, 22, 24

🤖 Generated with Claude Code


Note

Low Risk
Only test harness changes in spec files; no production client or socket behavior is modified.

Overview
Stabilizes three flaky client integration tests that were racing socket/connect lifecycle timing and mishandling multiple error emissions.

For Pub/Sub resubscribe and socketTimeout, tests no longer rely on once(..., 'error') alone: they add a permanent error listener plus a one-shot promise to await the first error while swallowing follow-up errors (e.g. after CLIENT KILL or with reconnectStrategy: false).

The socketTimeout case also connects explicitly (disableClientSetup), bumps the timeout to 200ms, sets reconnectStrategy: false, and asserts isOpen synchronously instead of deferring via nextTick.

The ConnectionTimeoutError test blocks the event loop for ~50ms (was ~1ms) so a 1ms connectTimeout reliably fires before a successful connect on fast hosts.

Reviewed by Cursor Bugbot for commit e5fdc19. Bugbot is set up for automated code reviews on this repo. Configure here.

Attach a permanent error listener before connecting so the timeout
event cannot surface as an uncaught error during the handshake or in
the idle gap before the test body attached its listener. Drive the
test off the first emitted error via a one-shot promise gate instead
of a fixed-duration `setTimeout`, and use `reconnectStrategy: false`
+ `disableClientSetup` to make the run deterministic.

Bump the timeout from 50ms to 200ms to give the localhost handshake
slack on slow CI.
The test pairs `connectTimeout: 1` with a 1ms event-loop block to force
the socket's idle timer to fire. On a fast host the kernel finishes the
TCP handshake almost instantly and the two 1ms durations race: the block
can exit just as (or before) the timer expires, so the `connect` event
is processed before the timeout callback and the connection succeeds.
The test then fails with `Missing expected rejection`.

Block for 50ms instead so the timer is guaranteed to have expired —
and its callback to be queued ahead of the `connect` event — by the
time the event loop runs again.
The test forced the subscriber connection down with `CLIENT KILL` and
used `once(subscriber, 'error')` to wait for the resulting error. A
single forced reconnect can emit `error` more than once — the initial
SocketClosedUnexpectedlyError, plus any transient connect error while
the server tears the old connection down — and `once` removes its
listener after the first emit, so the next one surfaced as
"Uncaught Error: Socket closed unexpectedly".

Swap in a permanent error listener and use a one-shot promise gate to
await the first error deterministically. Subsequent errors during the
reconnect cycle are caught (and ignored) by the permanent listener.
@nkaradzhov
nkaradzhov requested a review from PavelPashov May 29, 2026 11:57
// handshake nor in the idle window afterwards. `reconnectStrategy:
// false` makes the run deterministic and causes the client to emit
// `error` twice, so a permanent listener (not `once`) is required.
const errors: Error[] = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this array of errors?

Comment on lines +253 to +260
let resolveFirstError!: (err: Error) => void;
const firstError = new Promise<Error>(resolve => {
resolveFirstError = resolve;
});
client.on('error', err => {
errors.push(err);
resolveFirstError(err);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit simpler

client.on('error', () => {
  // keep a permanent listener for any later error emission
});
const firstError = once(client, 'error') as Promise<[Error]>;
// ...
const [err] = await firstError;

Address review feedback on redis#3299: drop the unused `errors[]` array and
the hand-rolled deferred promise. Use `once(client, 'error')` for the
first-error gate, with a no-op `client.on('error', ...)` listener to
absorb the second emission from `reconnectStrategy: false`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit e5fdc19. Configure here.

const firstError = new Promise<void>(resolve => {
resolveFirstError = resolve;
});
subscriber.on('error', () => resolveFirstError());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessarily complex promise pattern versus simpler once idiom

Low Severity

The should resubscribe test uses a manual resolveFirstError promise-gate pattern (with let resolveFirstError!: () => void and a custom new Promise) while the socketTimeout test in the same PR already demonstrates the simpler and equivalent approach: a permanent no-op on('error') listener paired with once(client, 'error'). The simpler pattern is fewer lines, more idiomatic, captures the actual error object, and maintains consistency across both tests in this PR.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e5fdc19. Configure here.

@nkaradzhov
nkaradzhov merged commit 2f3a86e into redis:master Jun 2, 2026
14 checks passed
@nkaradzhov
nkaradzhov deleted the fix-flaky-socket-timeout-test branch June 2, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants