Skip to content

fix(http): clear per-attempt timeout timers#137

Merged
zeshi-du merged 2 commits into
TestSprite:mainfrom
JerryNee:fix/http-timeout-cleanup
Jul 6, 2026
Merged

fix(http): clear per-attempt timeout timers#137
zeshi-du merged 2 commits into
TestSprite:mainfrom
JerryNee:fix/http-timeout-cleanup

Conversation

@JerryNee

@JerryNee JerryNee commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace per-attempt AbortSignal.timeout(...) with an explicit AbortController + clearable timer
  • clear the attempt timeout after response bodies settle and before retry backoff sleeps
  • add regression coverage proving settled attempt signals do not abort later

Fixes #82.

Tests

  • npm test -- src/lib/http.test.ts
  • npm run typecheck
  • npm run lint
  • npm test
  • npm run build

Summary by CodeRabbit

  • Bug Fixes
    • Fixed per-request timeout behavior so timeouts are cleared after successful responses and don’t interfere across retry/backoff periods.
    • Improved combined caller-abort and timeout handling to ensure the correct error type is surfaced when both could trigger.
    • Refined retry behavior for non-OK responses by honoring a clamped Retry-After value during error classification and retry decisions.
  • Tests
    • Added coverage for per-attempt timeout clearing, retry signal handling, and timeout-vs-caller-abort error precedence.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e6e35216-217c-4d8b-9634-66f9cc4f5240

📥 Commits

Reviewing files that changed from the base of the PR and between ae5aa7c and c4967f9.

📒 Files selected for processing (2)
  • src/lib/http.test.ts
  • src/lib/http.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/http.test.ts
  • src/lib/http.ts

Walkthrough

HttpClient now uses an AbortController-backed per-attempt timeout that is cleared after request settle and before retries. Abort classification and retry-after handling were updated, and tests cover success, retry, and timeout-versus-caller-abort precedence.

Changes

Request Timeout Lifecycle Management

Layer / File(s) Summary
Timeout handle utility
src/lib/http.ts
Adds RequestTimeoutHandle, createRequestTimeout, makeTimeoutReason, and unrefTimer for an AbortController-backed timeout with Node-compatible timeout reasons and timer unref support.
Fetch and retry flow using composed timeout signal
src/lib/http.ts
rethrowIfAbort now uses the effective signal when classifying timeout-versus-caller aborts, and requestWithMeta composes timeout and caller signals, clears timeout resources around retries and settle, and passes clamped retry-after values into ApiError.fromEnvelope(...).
Tests verifying timeout cleanup
src/lib/http.test.ts
Adds success, retry, and precedence tests that capture per-attempt signals and verify timeout cleanup and classification behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Related issues: #82

Possibly related PRs

  • TestSprite/testsprite-cli#166: Both changes touch HttpClient.requestWithMeta’s response-body/error handling path and its interaction with abort classification during parsing.

Suggested labels: bug, performance

Suggested reviewers: ruili-testsprite, zeshi-du

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: clearing per-attempt HTTP timeout timers.
Linked Issues check ✅ Passed The implementation replaces AbortSignal.timeout with a clearable timer and clears it on settle, matching issue #82's proposed fix.
Out of Scope Changes check ✅ Passed No clearly unrelated code changes are evident; the additions stay within HTTP timeout, abort, and retry behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

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 `@src/lib/http.ts`:
- Line 454: The timeout/caller-abort race in the request error path is causing
`rethrowIfAbort` to classify based on `options.signal.aborted`, which can
rethrow a raw `TimeoutError` instead of preserving `RequestTimeoutError` and
exit code 7. Update the error handling around `this.rethrowIfAbort(...)` in
`src/lib/http.ts` to use the composed signal’s winning reason (from the
timeout/caller combined signal) when deciding whether to wrap/throw, so the
timeout path stays mapped to `RequestTimeoutError` even if the caller signal
aborts afterward.
🪄 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: CHILL

Plan: Pro Plus

Run ID: 8ab00822-4e63-49c7-9f50-09f45d402335

📥 Commits

Reviewing files that changed from the base of the PR and between e53257d and 2af4939.

📒 Files selected for processing (2)
  • src/lib/http.test.ts
  • src/lib/http.ts

Comment thread src/lib/http.ts Outdated
@zeshi-du

zeshi-du commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

This is approved in substance — replacing AbortSignal.timeout() with a cleared, unref'd timer is right (no timer pile-up across retries, no exit-latency tail), and the effectiveSignal tie-break fix for "caller aborts after the timeout wins" is a genuinely subtle catch. The tests for both are exactly what we'd want.

It just went CONFLICTING because #166 landed in the same response.json() catch block (non-JSON 200 → typed INTERNAL error). The resolution when you rebase:

} catch (err) {
  this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId, effectiveSignal); // yours
  throw malformedResponseError(response, requestId, err); // #166's, instead of `throw err`
}

i.e. keep your five-arg rethrowIfAbort, but the fall-through now throws #166's malformedResponseError rather than the raw parse error. Rebase and it merges.

@JerryNee JerryNee force-pushed the fix/http-timeout-cleanup branch from ae5aa7c to c4967f9 Compare July 6, 2026 18:25

@zeshi-du zeshi-du left a comment

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.

Reviewed: scoped, tests included, CI green across the matrix.

@zeshi-du zeshi-du merged commit c6d37b7 into TestSprite:main Jul 6, 2026
9 checks passed
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.

[Hackathon] Per-Attempt AbortSignal.timeout Timers Are Never Cleared (Timer/Memory Growth in Large Batches)

2 participants