Conversation
The connection-failures suite hardcoded redis://localhost:12345 as a 'guaranteed bad' address. On a CI runner where something else happened to be listening on 12345, the RedisClient connected, sent HELLO, and received non-RESP bytes back, so the tests failed with 'Failed to read data (stack path)' instead of the expected connection error (seen in build 74111 on :debian: 13 x64, 4/4 retries). Allocate a refused port with the bind(0)->close pattern already used by test/js/sql/wire-frames.ts closedPort().
|
Updated 5:07 PM PT - Jul 16th, 2026
❌ @robobun, your commit ec62f5d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34412That installs a local version of the PR into your bun-34412 --bun |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only flake fix replacing a hardcoded port with the established bind(0)→close pattern.
What was reviewed:
- Confirmed the inline logic is byte-identical to
closedPort()intest/js/sql/wire-frames.ts. netwas already imported (used by the auto-reconnect suite below);beforeAlladded to the bun:test import.- All
BAD_CONNECTION_URLreads are inside test bodies in the same describe, so they run afterbeforeAllassigns it. localhost→127.0.0.1matches the bind host, so no v4/v6 mismatch.
Extended reasoning...
Overview
Single-file, test-only change to test/js/valkey/reliability/connection-failures.test.ts. Replaces the hardcoded redis://localhost:12345 "refused" URL with a port allocated via net.createServer().listen(0) → read port → close() in a beforeAll, then builds redis://127.0.0.1:${port}. This is the same pattern already exported as closedPort() from test/js/sql/wire-frames.ts and used in sql-connect-error-reporting.test.ts and sql-onconnect-onclose-throw.test.ts.
Security risks
None. Test code only; binds to 127.0.0.1 on an ephemeral port and immediately closes. No runtime, auth, crypto, or network-facing production code is touched.
Level of scrutiny
Low. This is a CI flake fix with a well-documented root cause (something occupied TCP 12345 on one Debian shard, so RedisClient connected and failed at RESP parse instead of connect). The fix is mechanical and copies an established in-tree idiom line-for-line. The assertions in the affected tests are unchanged, so the test contract is preserved.
Other factors
netwas already imported for theAuto-Reconnect In-Flight Commandsblock, so no new dependency.BAD_CONNECTION_URLis declared with a definite-assignment assertion and only read inside test callbacks within the samedescribe, all of which execute afterbeforeAll— no TDZ/undefined risk.- Switching
localhost→127.0.0.1aligns the connect target with the bind host, avoiding any IPv6-first resolution edge case. - The bug hunting system found no issues.
|
Self-review: no concerns survived. CI on build 74142:
Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
test/js/valkey/reliability/connection-failures.test.tswent red in build 74111 on:debian: 13 x64(4/4 retries on the shard, the other 19 shards for the same lane passed):Cause
The suite hardcoded
redis://localhost:12345as a "guaranteed bad" address and asserted connection-refused behaviour. On that runner something was listening on TCP 12345, soRedisClientconnected, wroteHELLO, read non-RESP bytes back, and the reply parser failed withFailed to read data (stack path)(theon_datastack-path error insrc/runtime/valkey_jsc/valkey.rs) instead of the expected connection error.Reproduced locally by binding a junk-writing TCP server to 12345 before driving the same code path:
Fix
Replace the fixed port with a
bind(0)→ read the port →close()sequence inbeforeAll, the same patterntest/js/sql/wire-frames.tsexports asclosedPort(). The connection-failure tests then target a freshly released ephemeral port on127.0.0.1, independent of what else is bound on the machine.The code paths exercised are unchanged: every affected test still drives
RedisClientintoon_connect_error/on_closeand asserts the same/connection closed|socket closed|failed to connect/imessages and"Connection closed"snapshot.Verification
With a garbage server deliberately bound to 12345:
bun bd test test/js/valkey/reliability/connection-failures.test.tspasses (connection-failure cases are gated on Docker and skip in this container; the non-gatedAuto-Reconnect In-Flight Commandscase passes).