fix: test(client-typescript) disconnect timeout guard should reject instead of resolve#1002
Conversation
The previous implementation used resolve() inside setTimeout(), which silently passed the test if client.disconnect() took longer than the timeout. Switched it to reject() with an error message so tests properly fail on timeout.
aryaMehta26
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
📝 WalkthroughWalkthroughThis PR modifies timeout guards in the RocketRideClient integration test suite to enforce stricter teardown behavior. Three Promise.race timeout branches are changed from resolving to rejecting, causing test failures when client disconnection or pipeline cleanup operations exceed their time limits instead of silently timing out. ChangesTeardown Timeout Enforcement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-typescript/tests/RocketRideClient.test.ts`:
- Around line 74-75: The timeout guard Promises used inside Promise.race (e.g.,
new Promise((_, reject) => setTimeout(() => reject(new Error('disconnect timeout
exceeded')), 10000))) create orphaned timers; update the teardown code in
RocketRideClient.test.ts to capture the setTimeout handle, expose a small helper
(e.g., makeTimeoutRejector or withTimeoutGuard) that returns {promise, clear} or
returns a promise plus the timer id, and then in the finally block call
clearTimeout on the captured handle so timers are cleared when
client.disconnect() or pipeline cleanup finishes early; also include the timeout
duration and context (e.g., "disconnect timeout 10000ms") in the rejection Error
messages to aid debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cf63480-dc4d-4a74-95ea-6b2fd03d7ed4
📒 Files selected for processing (1)
packages/client-typescript/tests/RocketRideClient.test.ts
| await Promise.race([client.disconnect(), new Promise<void>((_, reject) => setTimeout(() => reject(new Error('disconnect timeout exceeded')), 10000))]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify timeout races that reject via setTimeout but do not clear timer handles.
rg -n -C3 "Promise\.race\(\[.*setTimeout\(\(\)\s*=>\s*reject" packages/client-typescript/tests/RocketRideClient.test.ts
rg -n -C2 "clearTimeout\(" packages/client-typescript/tests/RocketRideClient.test.tsRepository: rocketride-org/rocketride-server
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Find all timeout guards related to disconnect/cleanup in this test file
rg -n "disconnect timeout exceeded|pipeline cleanup timeout exceeded" packages/client-typescript/tests/RocketRideClient.test.ts
# Find other Promise.race + setTimeout reject patterns
rg -n -C3 "Promise\.race\(\[.*setTimeout\(" packages/client-typescript/tests/RocketRideClient.test.ts
# Check for any clearTimeout usage in this file
rg -n "clearTimeout\(" packages/client-typescript/tests/RocketRideClient.test.tsRepository: rocketride-org/rocketride-server
Length of output: 1149
🏁 Script executed:
#!/bin/bash
# Find all timeout guards related to disconnect/cleanup in this test file
rg -n "disconnect timeout exceeded|pipeline cleanup timeout exceeded" packages/client-typescript/tests/RocketRideClient.test.ts
# Find other Promise.race + setTimeout reject patterns
rg -n -C3 "Promise\.race\(\[.*setTimeout\(" packages/client-typescript/tests/RocketRideClient.test.ts
# Check for any clearTimeout usage in this file
rg -n "clearTimeout\(" packages/client-typescript/tests/RocketRideClient.test.tsRepository: rocketride-org/rocketride-server
Length of output: 1149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/client-typescript/tests/RocketRideClient.test.ts"
# Show full blocks around each reported line
sed -n '55,90p' "$FILE"
echo "----"
sed -n '1650,1695p' "$FILE"
echo "----"
sed -n '2025,2065p' "$FILE"
# Specifically confirm whether this file uses clearTimeout anywhere
echo "----"
rg -n "clearTimeout\(" "$FILE" || trueRepository: rocketride-org/rocketride-server
Length of output: 3738
Clear orphaned setTimeout timers used as timeout guards in teardown Promise.races (74, ~1672, 2045).
The timeout Promises created inside each Promise.race use setTimeout(...reject...) but never clear/capture the timer handle when disconnect() / pipeline cleanup finishes early; there’s also no clearTimeout( usage in this file. This can leave pending timers and cause late rejections. Capture the timeout handle and clearTimeout it in finally (via a small helper), and include the timeout duration/context in the error messages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-typescript/tests/RocketRideClient.test.ts` around lines 74 -
75, The timeout guard Promises used inside Promise.race (e.g., new Promise((_,
reject) => setTimeout(() => reject(new Error('disconnect timeout exceeded')),
10000))) create orphaned timers; update the teardown code in
RocketRideClient.test.ts to capture the setTimeout handle, expose a small helper
(e.g., makeTimeoutRejector or withTimeoutGuard) that returns {promise, clear} or
returns a promise plus the timer id, and then in the finally block call
clearTimeout on the captured handle so timers are cleared when
client.disconnect() or pipeline cleanup finishes early; also include the timeout
duration and context (e.g., "disconnect timeout 10000ms") in the rejection Error
messages to aid debugging.
1 participant
Fixes #968
The previous implementation of the disconnect timeout guard in the TypeScript client tests used
resolve()instead ofreject()when the timeout was reached. This meant that ifclient.disconnect()hung, the test would silently pass instead of failing as intended.This commit updates the timeout promises to use
reject()with an appropriate error message, ensuring that test failures on disconnect are properly caught and reported.Summary by CodeRabbit