Skip to content

Commit c4967f9

Browse files
committed
fix(http): preserve timeout race classification
1 parent e6f5abd commit c4967f9

2 files changed

Lines changed: 45 additions & 12 deletions

File tree

src/lib/http.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -521,6 +521,33 @@ describe('HttpClient per-request timeout', () => {
521521
expect((err as Error).name).toBe('AbortError');
522522
});
523523

524+
it('preserves RequestTimeoutError when the caller aborts after the request timeout wins', async () => {
525+
const controller = new AbortController();
526+
const fetchImpl = vi.fn(async (_input: unknown, init?: { signal?: AbortSignal }) => {
527+
return new Promise<Response>((_resolve, reject) => {
528+
init?.signal?.addEventListener('abort', () => {
529+
const reason = init.signal?.reason;
530+
controller.abort(new Error('caller aborted after timeout'));
531+
const err = new Error(reason?.message ?? 'timed out');
532+
err.name = reason?.name ?? 'TimeoutError';
533+
reject(err);
534+
});
535+
});
536+
});
537+
const client = new HttpClient({
538+
baseUrl: 'https://api.example.com/api/cli/v1',
539+
apiKey: 'sk-test',
540+
fetchImpl: fetchImpl as unknown as typeof fetch,
541+
sleep: () => Promise.resolve(),
542+
random: () => 0,
543+
requestTimeoutMs: 1,
544+
});
545+
const err = await client.get('/me', { signal: controller.signal }).catch(e => e);
546+
expect(err).toBeInstanceOf(RequestTimeoutError);
547+
expect((err as RequestTimeoutError).exitCode).toBe(7);
548+
expect(fetchImpl).toHaveBeenCalledTimes(1);
549+
});
550+
524551
it('defaults to REQUEST_TIMEOUT_DEFAULT_MS when no requestTimeoutMs is supplied', () => {
525552
// Verify the default is wired without actually waiting 120s.
526553
// We test via the exported constant rather than exercising the stall.

src/lib/http.ts

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -395,9 +395,15 @@ export class HttpClient {
395395
timeoutSignal: AbortSignal,
396396
callerSignal: AbortSignal | undefined,
397397
requestId: string,
398+
effectiveSignal: AbortSignal = timeoutSignal,
398399
): void {
399400
if (isAbortError(err) || isTimeoutError(err)) {
400-
if (timeoutSignal.aborted && (callerSignal == null || !callerSignal.aborted)) {
401+
const timeoutWon =
402+
timeoutSignal.aborted &&
403+
(callerSignal == null ||
404+
!callerSignal.aborted ||
405+
effectiveSignal.reason === timeoutSignal.reason);
406+
if (timeoutWon) {
401407
throw new RequestTimeoutError(this.requestTimeoutMs, requestId);
402408
}
403409
throw err;
@@ -451,7 +457,7 @@ export class HttpClient {
451457
// the caller hadn't already aborted, surface a clear RequestTimeoutError.
452458
// A timeout/abort during the fetch itself: classify it (RequestTimeoutError
453459
// when our deadline fired; otherwise rethrow the caller's abort unmodified).
454-
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId);
460+
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId, effectiveSignal);
455461
// If a RequestTimeoutError already propagated from somewhere (e.g. from a
456462
// nested call or from a test-injected fetchImpl), pass it through unchanged
457463
// rather than re-wrapping it as a TransportError.
@@ -500,8 +506,14 @@ export class HttpClient {
500506
return { body: (await response.json()) as T, requestId, status: response.status };
501507
} catch (err) {
502508
// A timeout/abort can fire mid-body-read (headers received, stream stalls).
503-
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId);
504-
throw err;
509+
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId, effectiveSignal);
510+
// Otherwise the successful response body was not valid JSON — a
511+
// misconfigured endpoint, a proxy / captive-portal / login page that
512+
// returns HTML with a 200 status, or an empty body. Surface a typed
513+
// error carrying the requestId instead of letting the raw SyntaxError
514+
// escape to index.ts, where it would print a bare `{"error":"..."}`
515+
// and break the --output json envelope contract.
516+
throw malformedResponseError(response, requestId, err);
505517
}
506518
}
507519

@@ -511,14 +523,8 @@ export class HttpClient {
511523
} catch (err) {
512524
// safeReadJson rethrows aborts/timeouts (it swallows only non-abort parse
513525
// errors), so a timeout fired mid-body-read on a non-OK response lands here.
514-
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId);
515-
// Otherwise the successful response body was not valid JSON — a
516-
// misconfigured endpoint, a proxy / captive-portal / login page that
517-
// returns HTML with a 200 status, or an empty body. Surface a typed
518-
// error carrying the requestId instead of letting the raw SyntaxError
519-
// escape to index.ts, where it would print a bare `{"error":"..."}`
520-
// and break the --output json envelope contract.
521-
throw malformedResponseError(response, requestId, err);
526+
this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId, effectiveSignal);
527+
throw err;
522528
}
523529

524530
// Edge proxies / load balancers return 408/502/504 without our error

0 commit comments

Comments
 (0)