From 53b51c8b7be370893b39b5887d6de45d0e216bd7 Mon Sep 17 00:00:00 2001 From: Dima Date: Mon, 21 Sep 2026 09:20:43 +0100 Subject: [PATCH 1/5] fix: keep request timeout active while reading response body Keep the request timeout active while the response body is read so a response that stalls after headers cannot hang indefinitely. Preserve existing non-2xx retry handling, ParseError behaviour and the original Response object. Successful-body timeouts are reported but are not automatically retried. Only the SDK read that owns the body disarms the deadline; a competing toJSON() or raw consumer cannot release it. On the successful-body path only the deadline's own expiry is reported as a timeout, and the WorkOS client translates only that 408 so other body-read errors propagate unchanged. Fixes #1679 --- README.md | 9 + setup-jest.ts | 9 +- src/common/net/fetch-client.spec.ts | 710 ++++++++++++++++++++++++++++ src/common/net/fetch-client.ts | 246 ++++++++-- src/workos.spec.ts | 120 +++++ src/workos.ts | 30 +- 6 files changed, 1079 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 34b08b45d..acda00f4b 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,15 @@ await workos.get('/organizations', { maxRetries: 0 }); Set `maxRetries: 0` to disable automatic retries entirely. +The request timeout (`timeout`, 60 seconds by default) covers each attempt from +the request being sent to the response body being read, so a server that +returns headers promptly but stalls the body still times out. A timeout while +reading a successful response body is reported as a 408 but is not +automatically retried, because the request has already completed and its +response has been handed back. A response consumed through `getRawResponse()` +stays subject to the same deadline; it does not grant an unlimited streaming +lifetime. + ### Access token issuer validation Session helpers (`authenticateWithSessionCookie`, `loadSealedSession(...).authenticate()`) diff --git a/setup-jest.ts b/setup-jest.ts index 019680cb6..95a06c3d2 100644 --- a/setup-jest.ts +++ b/setup-jest.ts @@ -1,6 +1,13 @@ -import { enableFetchMocks } from 'jest-fetch-mock'; import { webcrypto } from 'crypto'; +// jest-fetch-mock replaces the global fetch the moment it is loaded. Keep the +// runtime's own implementation reachable for tests that need real HTTP +// streaming and abort behaviour (see fetch-client.spec.ts). +const NATIVE_FETCH_KEY = '__workosNativeFetch'; +(globalThis as Record)[NATIVE_FETCH_KEY] = globalThis.fetch; + +const { enableFetchMocks } = require('jest-fetch-mock'); + enableFetchMocks(); // Make Node's crypto.webcrypto available as global.crypto for tests diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index d318f26a2..a2041f2c0 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -3,6 +3,8 @@ import { fetchOnce, fetchURL } from '../../common/utils/test-utils'; import { FetchHttpClient } from './fetch-client'; import { HttpClientError } from './http-client'; import { ParseError } from '../exceptions/parse-error'; +import http from 'node:http'; +import { AddressInfo } from 'node:net'; const fetchClient = new FetchHttpClient('https://test.workos.com', { headers: { @@ -658,3 +660,711 @@ describe('FetchHttpClient with timeout', () => { expect(result).toBeDefined(); }); }); + +describe('request timeout covers the response body (GH-1679)', () => { + // `fetch` in this file is the jest-fetch-mock import. + type FetchFn = typeof globalThis.fetch; + + type FakeAttempt = { + signal: AbortSignal; + response: any; + releaseBody: (body: string) => void; + failBody: (error: Error) => void; + readonly textCalls: number; + }; + + type FakeAttemptPlan = { + status?: number; + headers?: Record; + /** `null` for no body, `'pending'` to stall until released, else the body. */ + body?: null | 'pending' | string; + /** Delay before the headers resolve. */ + headersDelayMs?: number; + }; + + const abortError = () => { + const error = new Error('The operation was aborted'); + error.name = 'AbortError'; + return error; + }; + + /** + * A fetch double that honours the abort signal the way real + * implementations do: an abort rejects the pending headers or the pending + * body read with an `AbortError`. + */ + function createFakeFetch(plan: FakeAttemptPlan[]) { + const attempts: FakeAttempt[] = []; + + const fetchFn = jest.fn((_url: string, init: RequestInit) => { + const step = plan[Math.min(attempts.length, plan.length - 1)]; + const signal = init.signal as AbortSignal; + const status = step.status ?? 200; + const headers = new Headers({ + 'content-type': 'application/json', + 'x-request-id': 'req_1679', + ...step.headers, + }); + + let settleBody!: { + resolve: (body: string) => void; + reject: (error: Error) => void; + }; + const bodyPromise = new Promise((resolve, reject) => { + settleBody = { resolve, reject }; + }); + bodyPromise.catch(() => undefined); + signal.addEventListener('abort', () => settleBody.reject(abortError())); + + let textCalls = 0; + const body = step.body === null ? null : { locked: false }; + const response = { + ok: status < 400, + status, + statusText: status < 400 ? 'OK' : 'Error', + headers, + body, + get bodyUsed() { + return textCalls > 0; + }, + text: () => { + textCalls++; + return textCalls === 1 && !body?.locked + ? bodyPromise + : Promise.reject(new TypeError('body used already')); + }, + }; + + if (typeof step.body === 'string' && step.body !== 'pending') { + settleBody.resolve(step.body); + } + + attempts.push({ + signal, + response, + releaseBody: settleBody.resolve, + failBody: settleBody.reject, + get textCalls() { + return textCalls; + }, + }); + + return new Promise((resolve, reject) => { + signal.addEventListener('abort', () => reject(abortError())); + if (step.headersDelayMs) { + setTimeout(() => resolve(response), step.headersDelayMs); + } else { + resolve(response); + } + }); + }); + + return { fetchFn: fetchFn as unknown as FetchFn, attempts }; + } + + function createClient( + plan: FakeAttemptPlan[], + options: { maxRetries?: number; timeout?: number } = {}, + ) { + const fake = createFakeFetch(plan); + const client = new FetchHttpClient( + 'https://api.example.com', + { timeout: 100, maxRetries: 0, ...options }, + fake.fetchFn, + ); + return { client, ...fake }; + } + + function settledFlag(promise: Promise) { + const state = { settled: false }; + promise.then( + () => (state.settled = true), + () => (state.settled = true), + ); + return state; + } + + const timeout408 = { + message: 'Request timeout after 100ms', + response: { status: 408, data: { error: 'Request timeout' } }, + }; + + describe('with deterministic timers', () => { + beforeEach(() => jest.useFakeTimers()); + afterEach(() => jest.useRealTimers()); + + it('keeps the deadline armed after the headers arrive and fails a stalled JSON body with a 408', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); + const state = settledFlag(read); + + await jest.advanceTimersByTimeAsync(99); + expect(state.settled).toBe(false); + expect(attempts[0].signal.aborted).toBe(false); + + await jest.advanceTimersByTimeAsync(1); + await expect(read).rejects.toThrow(HttpClientError); + await expect(read).rejects.toMatchObject(timeout408); + const error = await read.catch((e) => e); + expect(error.response.headers.get('x-request-id')).toBe('req_1679'); + expect(attempts[0].signal.aborted).toBe(true); + expect(attempts[0].textCalls).toBe(1); + }); + + it('charges the wait for headers against the same deadline instead of restarting it', async () => { + const { client } = createClient([ + { body: 'pending', headersDelayMs: 80 }, + ]); + + const request = client.get('/users', {}); + await jest.advanceTimersByTimeAsync(80); + const res = await request; + + const read = res.toJSON(); + read.catch(() => undefined); + const state = settledFlag(read); + + await jest.advanceTimersByTimeAsync(19); + expect(state.settled).toBe(false); + + await jest.advanceTimersByTimeAsync(1); + await expect(read).rejects.toMatchObject(timeout408); + }); + + it('does not start another attempt when a successful body times out, even with retries enabled', async () => { + const { client, fetchFn, attempts } = createClient( + [{ body: 'pending' }], + { + maxRetries: 2, + }, + ); + + const res = await client.post('/users', { name: 'x' }, {}); + const read = res.toJSON(); + read.catch(() => undefined); + + await jest.advanceTimersByTimeAsync(100); + await expect(read).rejects.toMatchObject(timeout408); + + await jest.advanceTimersByTimeAsync(20_000); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(attempts).toHaveLength(1); + expect( + (fetchFn as unknown as jest.Mock).mock.calls[0][1].headers[ + 'Idempotency-Key' + ], + ).toMatch(/^retry-/); + }); + + it('reads a stalled error body inside the attempt so the retry policy applies, with a fresh deadline per attempt', async () => { + const { client, fetchFn, attempts } = createClient( + [ + { + status: 503, + headers: { 'x-request-id': 'req_first', 'retry-after': '1' }, + body: 'pending', + }, + { status: 200, body: '{"ok":true}' }, + ], + { maxRetries: 1 }, + ); + + const request = client.post('/users', { name: 'x' }, {}); + request.catch(() => undefined); + + await jest.advanceTimersByTimeAsync(100); + expect(attempts[0].signal.aborted).toBe(true); + expect(attempts[0].textCalls).toBe(1); + expect(fetchFn).toHaveBeenCalledTimes(1); + + // Retry-After from the aborted attempt's headers is honoured. + await jest.advanceTimersByTimeAsync(999); + expect(fetchFn).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(1); + expect(fetchFn).toHaveBeenCalledTimes(2); + + const res = await request; + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + + const calls = (fetchFn as unknown as jest.Mock).mock.calls; + expect(calls[0][1].headers['Idempotency-Key']).toBe( + calls[1][1].headers['Idempotency-Key'], + ); + + // The first attempt's deadline cannot touch the second attempt, and + // the second attempt's deadline is gone once its body is consumed. + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[1].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); + }); + + it('surfaces a stalled error body as a 408 carrying the response headers once retries are exhausted', async () => { + const { client, attempts } = createClient([ + { + status: 422, + headers: { 'x-request-id': 'req_422' }, + body: 'pending', + }, + ]); + + const request = client.get('/users', {}); + request.catch(() => undefined); + await jest.advanceTimersByTimeAsync(100); + + await expect(request).rejects.toMatchObject(timeout408); + const error = await request.catch((e) => e); + expect(error.response.headers.get('x-request-id')).toBe('req_422'); + expect(attempts[0].signal.aborted).toBe(true); + }); + + it('keeps a timeout before the headers on its existing path, including retries', async () => { + const { client, fetchFn, attempts } = createClient( + [{ body: 'pending', headersDelayMs: 500 }], + { maxRetries: 1 }, + ); + + const request = client.get('/users', {}); + request.catch(() => undefined); + + await jest.advanceTimersByTimeAsync(100); + expect(attempts[0].signal.aborted).toBe(true); + expect(fetchFn).toHaveBeenCalledTimes(1); + + // Backoff (max 1687.5ms at attempt 2) then the second attempt's deadline. + await jest.advanceTimersByTimeAsync(2000); + expect(fetchFn).toHaveBeenCalledTimes(2); + expect(attempts[1].signal.aborted).toBe(true); + + await expect(request).rejects.toMatchObject(timeout408); + const error = await request.catch((e) => e); + expect(error.response.headers.get('x-request-id')).toBeNull(); + }); + + it('still reports a complete but malformed JSON body as a ParseError and releases the deadline', async () => { + const { client, attempts } = createClient([{ body: '{ invalid' }]); + + const res = await client.get('/users', {}); + const error = await res.toJSON().catch((e) => e); + + expect(error).toBeInstanceOf(ParseError); + expect(error.rawBody).toBe('{ invalid'); + expect(error.rawStatus).toBe(200); + expect(error.requestID).toBe('req_1679'); + + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[0].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); + }); + + it('propagates a body failure that is not a timeout unchanged', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); + attempts[0].failBody(new TypeError('terminated')); + + await expect(read).rejects.toThrow(TypeError); + await expect(read).rejects.toThrow('terminated'); + await expect(read).rejects.not.toBeInstanceOf(HttpClientError); + + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[0].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); + }); + + it('preserves an AbortError raised by the body before the deadline and leaves the attempt unaborted', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + const independentAbort = abortError(); + + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); + attempts[0].failBody(independentAbort); + + await expect(read).rejects.toBe(independentAbort); + expect(attempts[0].signal.aborted).toBe(false); + + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[0].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); + }); + + it('still reports an AbortError from an error-body read as a timeout, as it did before the headers', async () => { + const { client, attempts } = createClient([ + { status: 500, body: 'pending' }, + ]); + + const request = client.get('/users', {}); + request.catch(() => undefined); + await jest.advanceTimersByTimeAsync(0); + attempts[0].failBody(abortError()); + + await expect(request).rejects.toMatchObject(timeout408); + expect(jest.getTimerCount()).toBe(0); + }); + + it('rejects a second toJSON() while the first is pending without disarming the first read', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + const first = res.toJSON(); + first.catch(() => undefined); + + await expect(res.toJSON()).rejects.toThrow('body used already'); + expect(jest.getTimerCount()).toBe(1); + expect(attempts[0].signal.aborted).toBe(false); + + await jest.advanceTimersByTimeAsync(100); + await expect(first).rejects.toMatchObject(timeout408); + expect(attempts[0].signal.aborted).toBe(true); + }); + + it('rejects toJSON() once a raw read has started and leaves that read under the deadline', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + const rawRead = (res.getRawResponse() as Response).text(); + rawRead.catch(() => undefined); + + await expect(res.toJSON()).rejects.toThrow('body used already'); + expect(jest.getTimerCount()).toBe(1); + + await jest.advanceTimersByTimeAsync(100); + const error = await rawRead.catch((e) => e); + expect(error.name).toBe('AbortError'); + expect(attempts[0].signal.aborted).toBe(true); + }); + + it('rejects toJSON() while the raw body is locked and leaves the lock holder under the deadline', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + attempts[0].response.body.locked = true; + + await expect(res.toJSON()).rejects.toThrow('body used already'); + expect(attempts[0].textCalls).toBe(1); + expect(jest.getTimerCount()).toBe(1); + + await jest.advanceTimersByTimeAsync(100); + expect(attempts[0].signal.aborted).toBe(true); + }); + + it('releases the deadline once the body is consumed and keeps single-consumption semantics', async () => { + const { client, attempts } = createClient([{ body: '{"ok":true}' }]); + + const res = await client.get('/users', {}); + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + expect(jest.getTimerCount()).toBe(0); + + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[0].signal.aborted).toBe(false); + + await expect(res.toJSON()).rejects.toThrow('body used already'); + }); + + it('fails toJSON() immediately once the deadline has already passed', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + await jest.advanceTimersByTimeAsync(100); + expect(attempts[0].signal.aborted).toBe(true); + + await expect(res.toJSON()).rejects.toMatchObject(timeout408); + expect(attempts[0].textCalls).toBe(0); + }); + + it('returns the exact original object from getRawResponse() and leaves raw reads under the same deadline', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + const res = await client.get('/users', {}); + const raw = res.getRawResponse() as Response; + expect(raw).toBe(attempts[0].response); + + const rawRead = raw.text(); + rawRead.catch(() => undefined); + await jest.advanceTimersByTimeAsync(100); + + const error = await rawRead.catch((e) => e); + expect(error.name).toBe('AbortError'); + expect(error).not.toBeInstanceOf(HttpClientError); + }); + + it('leaves the deadline armed for an unread non-JSON body and releases it for an absent one', async () => { + const withBody = createClient([ + { headers: { 'content-type': 'text/plain' }, body: 'pending' }, + ]); + const withoutBody = createClient([ + { headers: { 'content-type': 'text/plain' }, body: null }, + ]); + + const first = await withBody.client.get('/users', {}); + const second = await withoutBody.client.get('/users', {}); + await expect(first.toJSON()).resolves.toBeNull(); + await expect(second.toJSON()).resolves.toBeNull(); + expect(withBody.attempts[0].textCalls).toBe(0); + expect(jest.getTimerCount()).toBe(1); + + await jest.advanceTimersByTimeAsync(100); + expect(withBody.attempts[0].signal.aborted).toBe(true); + expect(withoutBody.attempts[0].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); + }); + + it('bounds a low-level delete response that nobody reads', async () => { + const { client, attempts } = createClient([{ body: 'pending' }]); + + await client.delete('/users/123', {}); + expect(jest.getTimerCount()).toBe(1); + + await jest.advanceTimersByTimeAsync(100); + expect(attempts[0].signal.aborted).toBe(true); + expect(jest.getTimerCount()).toBe(0); + }); + }); + + // Captured in setup-jest.ts before jest-fetch-mock replaces the global. + const nativeFetch = (globalThis as Record) + .__workosNativeFetch as FetchFn; + // jest-fetch-mock registers a mock for the 'node-fetch' module id. + const nodeFetch = jest.requireActual('node-fetch') as FetchFn; + + describe.each([ + ['native fetch', nativeFetch], + ['node-fetch 2.7.0', nodeFetch], + ])('over real HTTP with %s', (_name, fetchImpl) => { + let server: http.Server; + let baseURL: string; + let handler: (req: http.IncomingMessage, res: http.ServerResponse) => void; + let requestCount: number; + const openResponses: http.ServerResponse[] = []; + + const sleep = (ms: number) => + new Promise((resolve) => setTimeout(resolve, ms)); + + function stallBody(status: number, head = '{"ok":') { + handler = (_req, res) => { + res.writeHead(status, { + 'content-type': 'application/json', + 'x-request-id': 'req_real', + 'retry-after': '0', + }); + res.write(head); + // Hold the body open until the test tears the connection down. + }; + } + + function respond(status: number, body: string) { + handler = (_req, res) => { + res.writeHead(status, { + 'content-type': 'application/json', + 'x-request-id': 'req_real', + 'retry-after': '0', + }); + res.end(body); + }; + } + + function createClient(maxRetries = 0) { + const signals: AbortSignal[] = []; + const rawResponses: unknown[] = []; + const fetchFn: FetchFn = async (url, init) => { + signals.push(init!.signal as AbortSignal); + const response = await fetchImpl(url, init); + rawResponses.push(response); + return response; + }; + const client = new FetchHttpClient( + baseURL, + { timeout: 100, maxRetries }, + fetchFn, + ); + return { client, signals, rawResponses }; + } + + beforeAll(async () => { + server = http.createServer((req, res) => { + requestCount++; + openResponses.push(res); + handler(req, res); + }); + await new Promise((resolve) => + server.listen(0, '127.0.0.1', resolve), + ); + const { port } = server.address() as AddressInfo; + baseURL = `http://127.0.0.1:${port}`; + }); + + afterAll(async () => { + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + }); + + beforeEach(() => { + requestCount = 0; + }); + + afterEach(() => { + for (const res of openResponses) { + res.destroy(); + } + openResponses.length = 0; + }); + + it('times out a successful JSON body that stalls after the headers', async () => { + stallBody(200); + const { client, signals } = createClient(); + + const res = await client.get('/users', {}); + expect(res.getStatusCode()).toBe(200); + + const error = await res.toJSON().catch((e) => e); + expect(error).toBeInstanceOf(HttpClientError); + expect(error).toMatchObject(timeout408); + expect(error.response.headers.get('x-request-id')).toBe('req_real'); + expect(signals[0].aborted).toBe(true); + expect(requestCount).toBe(1); + }); + + it('times out a stalled error body inside the attempt and retries it with a fresh deadline', async () => { + // First attempt stalls its 503 body; the retry gets a clean 200. + handler = (_req, res) => { + if (requestCount === 1) { + res.writeHead(503, { + 'content-type': 'application/json', + 'retry-after': '0', + }); + res.write('{"error":'); + return; + } + res.writeHead(200, { 'content-type': 'application/json' }); + res.end('{"ok":true}'); + }; + const { client, signals } = createClient(1); + + const res = await client.get('/users', {}); + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + expect(requestCount).toBe(2); + expect(signals[0].aborted).toBe(true); + expect(signals[1].aborted).toBe(false); + }); + + it('surfaces a stalled error body as a 408 carrying the response headers when not retried', async () => { + stallBody(422); + const { client, signals } = createClient(); + + const error = await client.get('/users', {}).catch((e) => e); + expect(error).toBeInstanceOf(HttpClientError); + expect(error).toMatchObject(timeout408); + expect(error.response.headers.get('x-request-id')).toBe('req_real'); + expect(signals[0].aborted).toBe(true); + }); + + it('delivers a body that completes in time and disarms the deadline', async () => { + respond(200, '{"ok":true}'); + const { client, signals } = createClient(); + + const res = await client.get('/users', {}); + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + + await sleep(150); + expect(signals[0].aborted).toBe(false); + }); + + it('reports a connection dropped mid-body before the deadline as its own error', async () => { + handler = (_req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.write('{"ok":'); + setTimeout(() => res.destroy(), 10); + }; + const { client } = createClient(); + + const res = await client.get('/users', {}); + const error = await res.toJSON().catch((e) => e); + // The implementation's own error (undici raises it in Jest's outer + // realm, so no instanceof Error here), not a timeout or parse error. + expect(typeof error.message).toBe('string'); + expect(error).not.toBeInstanceOf(HttpClientError); + expect(error).not.toBeInstanceOf(ParseError); + expect(error.name).not.toBe('AbortError'); + }); + + it("hands back the implementation's own response for raw access, still under the deadline", async () => { + stallBody(200); + const { client, rawResponses, signals } = createClient(); + + const res = await client.get('/users', {}); + const raw = res.getRawResponse() as Response; + expect(raw).toBe(rawResponses[0]); + + const error = await raw.text().catch((e) => e); + expect(error.name).toBe('AbortError'); + expect(error).not.toBeInstanceOf(HttpClientError); + expect(signals[0].aborted).toBe(true); + }); + + it('rejects a competing toJSON() and still aborts the first read at the deadline', async () => { + stallBody(200); + const { client, signals } = createClient(); + + const res = await client.get('/users', {}); + const first = res.toJSON(); + first.catch(() => undefined); + + await expect(res.toJSON()).rejects.toMatchObject({ name: 'TypeError' }); + expect(signals[0].aborted).toBe(false); + + await expect(first).rejects.toMatchObject(timeout408); + expect(signals[0].aborted).toBe(true); + }); + + it('rejects toJSON() after a raw read has started and still aborts that read at the deadline', async () => { + stallBody(200); + const { client, signals } = createClient(); + + const res = await client.get('/users', {}); + const rawRead = (res.getRawResponse() as Response).text(); + rawRead.catch(() => undefined); + + await expect(res.toJSON()).rejects.toMatchObject({ name: 'TypeError' }); + expect(signals[0].aborted).toBe(false); + + const error = await rawRead.catch((e) => e); + expect(error.name).toBe('AbortError'); + expect(signals[0].aborted).toBe(true); + }); + + if (fetchImpl === nativeFetch) { + // Only the Web stream body can be locked by a reader. + it('rejects toJSON() while the raw body is locked and still aborts the reader at the deadline', async () => { + stallBody(200); + const { client, signals } = createClient(); + + const res = await client.get('/users', {}); + const reader = (res.getRawResponse() as Response).body!.getReader(); + await reader.read(); + + await expect(res.toJSON()).rejects.toMatchObject({ + name: 'TypeError', + }); + expect(signals[0].aborted).toBe(false); + + const error = await reader.read().catch((e) => e); + expect(error.name).toBe('AbortError'); + expect(signals[0].aborted).toBe(true); + }); + } + + it('lets an unread response reach its deadline', async () => { + stallBody(200); + const { client, signals } = createClient(); + + await client.delete('/users/123', {}); + await sleep(200); + expect(signals[0].aborted).toBe(true); + }); + }); +}); diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index 42586b382..b3d1fb709 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -17,6 +17,140 @@ import { ParseError } from '../exceptions/parse-error'; type FetchHttpClientOptions = HttpClientOptions; const DEFAULT_FETCH_TIMEOUT = 60_000; // 60 seconds + +/** + * Deadline for one request attempt, from the initial `fetch()` through to + * the last byte of the response body. + */ +interface RequestTimeout { + /** + * Run one step of the attempt (the `fetch()` itself or a body read) under + * the deadline. A step interrupted by the deadline, or started after it has + * passed, rejects with the SDK's 408 `HttpClientError`; any other failure + * propagates unchanged, including an `AbortError` raised for some other + * reason while the deadline is still running. Pass the response `headers` + * once they are known so the request ID and `Retry-After` survive the + * translation. `abortIsTimeout` keeps the pre-existing request-path + * behaviour of treating any `AbortError` as the timeout. + */ + guard(operation: () => Promise, options?: GuardOptions): Promise; + /** Disarm the deadline. Idempotent. */ + release(): void; + /** + * Stop the armed deadline from keeping the process alive on its own while + * no SDK read is awaiting it. `guard()` references it again. + */ + unref(): void; +} + +/** + * The deadline armed by `fetchRequest()` alongside the attempt's + * `AbortController`. + * + * It is disarmed when the body has been consumed or the attempt has failed, + * not when the headers arrive, so a server that responds promptly and then + * stalls the body still trips the configured timeout (GH-1679). Ownership + * follows the body: `fetchRequest()` keeps it while reading error bodies and + * hands it to `FetchHttpClientResponse` for successful responses. A body + * nobody reads through the SDK (raw access, a discarded delete response, a + * non-JSON response) keeps the original deadline as a bounded fallback: when + * it fires the attempt's controller is aborted and the timer is gone. + */ +class AttemptTimeout implements RequestTimeout { + private handle: ReturnType | null; + private expired = false; + + constructor( + controller: AbortController, + private readonly timeoutMs: number, + ) { + this.handle = setTimeout(() => { + this.handle = null; + this.expired = true; + controller.abort(); + }, timeoutMs); + } + + async guard( + operation: () => Promise, + { headers, abortIsTimeout = false }: GuardOptions = {}, + ): Promise { + if (this.expired) { + throw this.timeoutError(headers); + } + + this.setRef(true); + + try { + return await operation(); + } catch (error) { + // The deadline's own expiry is the timeout signal. A caller-supplied + // fetch can abort for reasons of its own; on the successful-body path + // that failure is theirs and passes through unchanged. + if ( + this.expired || + (abortIsTimeout && AttemptTimeout.isAbortError(error)) + ) { + throw this.timeoutError(headers); + } + throw error; + } + } + + release(): void { + if (this.handle !== null) { + clearTimeout(this.handle); + this.handle = null; + } + } + + unref(): void { + this.setRef(false); + } + + private setRef(referenced: boolean): void { + // Node timers can be unreferenced; browser and worker runtimes hand back + // a number, which has nothing to toggle. + const handle = this.handle as { + ref?: () => void; + unref?: () => void; + } | null; + + if (referenced) { + handle?.ref?.(); + } else { + handle?.unref?.(); + } + } + + private timeoutError(headers?: Headers): HttpClientError<{ error: string }> { + return new HttpClientError({ + message: `Request timeout after ${this.timeoutMs}ms`, + response: { + status: 408, + headers: headers ?? new Headers(), + data: { error: 'Request timeout' }, + }, + }); + } + + private static isAbortError(error: unknown): boolean { + return error instanceof Error && error.name === 'AbortError'; + } +} + +type GuardOptions = { + headers?: Headers; + abortIsTimeout?: boolean; +}; + +/** For responses constructed outside a request attempt. */ +const NO_TIMEOUT: RequestTimeout = { + guard: (operation) => operation(), + release: () => undefined, + unref: () => undefined, +}; + export class FetchHttpClient extends HttpClient implements HttpClientInterface { private readonly _fetchFn; @@ -190,32 +324,45 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { // Access timeout from the options with default of 60 seconds const timeout = this.options?.timeout ?? DEFAULT_FETCH_TIMEOUT; // Default 60 seconds const abortController = new AbortController(); - const timeoutId = setTimeout(() => { - abortController?.abort(); - }, timeout); + // Armed for the whole attempt, body included; see AttemptTimeout. + const requestTimeout = new AttemptTimeout(abortController, timeout); try { - const res = await this._fetchFn(url, { - method, - headers: { - Accept: 'application/json, text/plain, */*', - 'Content-Type': 'application/json', - ...this.options?.headers, - ...headers, - 'User-Agent': (userAgent || 'workos-node').toString(), - }, - body: requestBody, - signal: abortController?.signal, - }); - - // Clear timeout if request completed successfully - if (timeoutId) { - clearTimeout(timeoutId); - } + // As before this deadline covered the body, an AbortError from the + // request itself or from an error-body read is reported as a timeout. + const res = await requestTimeout.guard( + () => + this._fetchFn(url, { + method, + headers: { + Accept: 'application/json, text/plain, */*', + 'Content-Type': 'application/json', + ...this.options?.headers, + ...headers, + 'User-Agent': (userAgent || 'workos-node').toString(), + }, + body: requestBody, + signal: abortController.signal, + }), + { abortIsTimeout: true }, + ); if (!res.ok) { const requestID = res.headers.get('X-Request-ID') ?? ''; - const rawBody = await res.text(); + + // Read the error body under the same deadline: a stalled error + // response surfaces as a 408 here, inside the attempt, where the + // retry policy already handles it. + let rawBody: string; + + try { + rawBody = await requestTimeout.guard(() => res.text(), { + headers: res.headers, + abortIsTimeout: true, + }); + } finally { + requestTimeout.release(); + } let responseJson: any; @@ -243,24 +390,12 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { }, }); } - return new FetchHttpClientResponse(res); + // The body is still on the wire: the deadline goes with the response. + return new FetchHttpClientResponse(res, requestTimeout); } catch (error) { - // Clear timeout if request failed - if (timeoutId) { - clearTimeout(timeoutId); - } - - // Handle timeout errors - if (error instanceof Error && error.name === 'AbortError') { - throw new HttpClientError({ - message: `Request timeout after ${timeout}ms`, - response: { - status: 408, - headers: new Headers(), - data: { error: 'Request timeout' }, - }, - }); - } + // Nothing took ownership of the deadline, so disarm it. Timeouts have + // already been translated to a 408 HttpClientError by guard(). + requestTimeout.release(); throw error; } @@ -414,13 +549,26 @@ export class FetchHttpClientResponse implements HttpClientResponseInterface { _res: Response; + private readonly _requestTimeout: RequestTimeout; - constructor(res: Response) { + constructor(res: Response, requestTimeout: RequestTimeout = NO_TIMEOUT) { super( res.status, FetchHttpClientResponse._transformHeadersToObject(res.headers), ); this._res = res; + this._requestTimeout = requestTimeout; + + if (res.body === null) { + // Nothing left to wait for. + requestTimeout.release(); + } else { + // Until toJSON() reads the body, or for good if nobody does (raw + // access, a discarded delete response, a non-JSON response), the + // deadline stays armed as a bounded fallback but must not keep the + // process alive by itself. + requestTimeout.unref(); + } } getRawResponse(): Response { @@ -435,7 +583,25 @@ export class FetchHttpClientResponse return null; } - const rawBody = await this._res.text(); + let rawBody: string; + + if (this._res.bodyUsed || this._res.body?.locked) { + // Someone else already holds the body: an earlier toJSON() or a raw + // consumer. Their read keeps the deadline; this call only surfaces + // the response's own rejection for a body that is already in use. + rawBody = await this._res.text(); + } else { + // This call owns the read. Only the read runs under the deadline: an + // interrupted body is a timeout, a complete but malformed one is still + // a ParseError below. + try { + rawBody = await this._requestTimeout.guard(() => this._res.text(), { + headers: this._res.headers, + }); + } finally { + this._requestTimeout.release(); + } + } try { return JSON.parse(rawBody); diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 708864185..06dedc28a 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -12,6 +12,7 @@ import { WorkOS } from './index'; import { WorkOS as WorkOSWorker } from './index.worker'; import { RateLimitExceededException } from './common/exceptions/rate-limit-exceeded.exception'; import { FetchHttpClient } from './common/net/fetch-client'; +import { HttpClientError } from './common/net/http-client'; import { SubtleCryptoProvider } from './common/crypto/subtle-crypto-provider'; jest.mock('./common/utils/runtime-info', () => ({ @@ -665,6 +666,125 @@ describe('WorkOS', () => { }); }); + describe('when the request times out while the response body is streaming (GH-1679)', () => { + const abortError = () => { + const error = new Error('Aborted'); + error.name = 'AbortError'; + return error; + }; + + /** + * Resolves headers immediately; the body read stalls until the abort + * signal fires, as real Fetch implementations behave. + */ + function stalledBodyFetch({ text }: { text?: () => Promise } = {}) { + const signals: AbortSignal[] = []; + const textMock = jest.fn(text); + const fetchFn = jest.fn(async (_url: any, init: any) => { + const signal = init.signal as AbortSignal; + signals.push(signal); + return { + ok: true, + status: 200, + headers: new Headers({ + 'X-Request-ID': 'req_body', + 'content-type': 'application/json', + }), + body: {}, + text: + text !== undefined + ? textMock + : () => + new Promise((_resolve, reject) => { + signal.addEventListener('abort', () => + reject(abortError()), + ); + }), + }; + }); + return { fetchFn, signals, textMock }; + } + + const sleep = (ms: number) => + new Promise((resolve) => setTimeout(resolve, ms)); + + it('surfaces the same 408 OauthException as a timeout before the headers', async () => { + const { fetchFn, signals } = stalledBodyFetch(); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + await expect(workos.get('/path')).rejects.toMatchObject({ + name: 'OauthException', + status: 408, + requestID: 'req_body', + message: 'Error: Request timeout', + }); + expect(signals[0].aborted).toBe(true); + }); + + it('propagates a body read failure that is not a timeout unchanged', async () => { + const { fetchFn } = stalledBodyFetch({ + text: () => Promise.reject(new TypeError('terminated')), + }); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + const error = await workos.post('/path', {}).catch((e) => e); + expect(error).toBeInstanceOf(TypeError); + expect(error.message).toBe('terminated'); + }); + + it('propagates a non-timeout HttpClientError from the body read as the same object', async () => { + const transportError = new HttpClientError({ + message: 'stream failed', + response: { + status: 401, + headers: new Headers(), + data: { error: 'stream failed' }, + }, + }); + const { fetchFn } = stalledBodyFetch({ + text: () => Promise.reject(transportError), + }); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + await expect(workos.get('/path')).rejects.toBe(transportError); + }); + + it.each([ + ['delete', (workos: WorkOS) => workos.delete('/path')], + [ + 'deleteWithBody', + (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), + ], + ])( + '%s resolves without reading the body and leaves the deadline as a bounded fallback', + async (_name, call) => { + const { fetchFn, signals, textMock } = stalledBodyFetch({ + text: () => new Promise(() => undefined), + }); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + await expect(call(workos)).resolves.toBeUndefined(); + expect(textMock).not.toHaveBeenCalled(); + expect(signals[0].aborted).toBe(false); + + await sleep(60); + expect(signals[0].aborted).toBe(true); + }, + ); + }); + describe('when in a worker environment', () => { it('uses the worker client', () => { const workos = new WorkOSWorker('sk_test_key'); diff --git a/src/workos.ts b/src/workos.ts index 7f0d568df..07b95f83b 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -257,7 +257,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async get( @@ -291,7 +291,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async put( @@ -323,7 +323,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async patch( @@ -355,7 +355,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async delete( @@ -390,6 +390,28 @@ export class WorkOS { } } + /** + * Consume a successful response body. The request deadline covers the body, + * so a stall while it is still streaming surfaces from the transport as a + * 408 `HttpClientError` and is translated like a timeout before the headers. + * Everything else (a `ParseError` for malformed JSON, a network failure + * mid-body, any other error a custom transport raises) propagates unchanged. + */ + private async readResponseJSON( + path: string, + response: HttpClientResponseInterface, + ): Promise { + try { + return await response.toJSON(); + } catch (error) { + if (error instanceof HttpClientError && error.response.status === 408) { + this.handleHttpError({ path, error }); + } + + throw error; + } + } + emitWarning(warning: string) { // tslint:disable-next-line:no-console console.warn(`WorkOS: ${warning}`); From 3c139ceb0565d8cdd1894e61d2270b498a7f527c Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 21 Sep 2026 12:26:15 -0400 Subject: [PATCH 2/5] refactor(fetch-client): read the response body inside the request attempt The body-timeout fix kept the body lazy so getRawResponse() could hand back an unconsumed Response, and paid for it with a deadline that had to be handed from the attempt to the response object: a RequestTimeout interface plus null object, ref/unref bookkeeping, competing-reader detection, two AbortError policies and a 408 translation layer in WorkOS. Nothing reads raw bodies, and the API is JSON-only. Reading res.text() before clearing the existing timer covers the same stall with the timer we already had. The 408 then leaves fetchRequest() like any other timeout, so the retry policy, Idempotency-Key handling and handleHttpError() apply unchanged, and bodies nobody reads (delete, non-JSON) are drained instead of left on the connection. The timeout is now detected on the attempt's own signal rather than the error's name: the AbortError undici raises for an aborted body can come from another realm, where instanceof Error is false. --- README.md | 10 +- src/common/net/fetch-client.spec.ts | 339 ++++++---------------------- src/common/net/fetch-client.ts | 257 ++++----------------- src/workos.spec.ts | 107 +++------ src/workos.ts | 30 +-- 5 files changed, 160 insertions(+), 583 deletions(-) diff --git a/README.md b/README.md index acda00f4b..2e34c02e2 100644 --- a/README.md +++ b/README.md @@ -61,13 +61,9 @@ await workos.get('/organizations', { maxRetries: 0 }); Set `maxRetries: 0` to disable automatic retries entirely. The request timeout (`timeout`, 60 seconds by default) covers each attempt from -the request being sent to the response body being read, so a server that -returns headers promptly but stalls the body still times out. A timeout while -reading a successful response body is reported as a 408 but is not -automatically retried, because the request has already completed and its -response has been handed back. A response consumed through `getRawResponse()` -stays subject to the same deadline; it does not grant an unlimited streaming -lifetime. +the request being sent to the response body being fully read, so a server that +returns headers promptly but stalls the body still times out. A body timeout is +reported as a 408 and retried like any other timeout. ### Access token issuer validation diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index a2041f2c0..f2f3846ca 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -668,7 +668,6 @@ describe('request timeout covers the response body (GH-1679)', () => { type FakeAttempt = { signal: AbortSignal; response: any; - releaseBody: (body: string) => void; failBody: (error: Error) => void; readonly textCalls: number; }; @@ -676,8 +675,8 @@ describe('request timeout covers the response body (GH-1679)', () => { type FakeAttemptPlan = { status?: number; headers?: Record; - /** `null` for no body, `'pending'` to stall until released, else the body. */ - body?: null | 'pending' | string; + /** `'pending'` to stall until the signal aborts, else the body. */ + body?: 'pending' | string; /** Delay before the headers resolve. */ headersDelayMs?: number; }; @@ -717,32 +716,24 @@ describe('request timeout covers the response body (GH-1679)', () => { signal.addEventListener('abort', () => settleBody.reject(abortError())); let textCalls = 0; - const body = step.body === null ? null : { locked: false }; const response = { ok: status < 400, status, statusText: status < 400 ? 'OK' : 'Error', headers, - body, - get bodyUsed() { - return textCalls > 0; - }, text: () => { textCalls++; - return textCalls === 1 && !body?.locked - ? bodyPromise - : Promise.reject(new TypeError('body used already')); + return bodyPromise; }, }; - if (typeof step.body === 'string' && step.body !== 'pending') { - settleBody.resolve(step.body); + if (step.body !== 'pending') { + settleBody.resolve(step.body ?? ''); } attempts.push({ signal, response, - releaseBody: settleBody.resolve, failBody: settleBody.reject, get textCalls() { return textCalls; @@ -793,25 +784,25 @@ describe('request timeout covers the response body (GH-1679)', () => { beforeEach(() => jest.useFakeTimers()); afterEach(() => jest.useRealTimers()); - it('keeps the deadline armed after the headers arrive and fails a stalled JSON body with a 408', async () => { + it('keeps the deadline armed after the headers arrive and fails a stalled body with a 408 carrying the response headers', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); - const res = await client.get('/users', {}); - const read = res.toJSON(); - read.catch(() => undefined); - const state = settledFlag(read); + const request = client.get('/users', {}); + request.catch(() => undefined); + const state = settledFlag(request); await jest.advanceTimersByTimeAsync(99); expect(state.settled).toBe(false); expect(attempts[0].signal.aborted).toBe(false); await jest.advanceTimersByTimeAsync(1); - await expect(read).rejects.toThrow(HttpClientError); - await expect(read).rejects.toMatchObject(timeout408); - const error = await read.catch((e) => e); + await expect(request).rejects.toThrow(HttpClientError); + await expect(request).rejects.toMatchObject(timeout408); + const error = await request.catch((e) => e); expect(error.response.headers.get('x-request-id')).toBe('req_1679'); expect(attempts[0].signal.aborted).toBe(true); expect(attempts[0].textCalls).toBe(1); + expect(jest.getTimerCount()).toBe(0); }); it('charges the wait for headers against the same deadline instead of restarting it', async () => { @@ -820,46 +811,48 @@ describe('request timeout covers the response body (GH-1679)', () => { ]); const request = client.get('/users', {}); - await jest.advanceTimersByTimeAsync(80); - const res = await request; - - const read = res.toJSON(); - read.catch(() => undefined); - const state = settledFlag(read); + request.catch(() => undefined); + const state = settledFlag(request); - await jest.advanceTimersByTimeAsync(19); + await jest.advanceTimersByTimeAsync(99); expect(state.settled).toBe(false); await jest.advanceTimersByTimeAsync(1); - await expect(read).rejects.toMatchObject(timeout408); + await expect(request).rejects.toMatchObject(timeout408); }); - it('does not start another attempt when a successful body times out, even with retries enabled', async () => { + it('retries a stalled successful body like any other timeout, with a fresh deadline and the same idempotency key', async () => { const { client, fetchFn, attempts } = createClient( - [{ body: 'pending' }], - { - maxRetries: 2, - }, + [{ body: 'pending' }, { body: '{"ok":true}' }], + { maxRetries: 1 }, ); - const res = await client.post('/users', { name: 'x' }, {}); - const read = res.toJSON(); - read.catch(() => undefined); + const request = client.post('/users', { name: 'x' }, {}); + request.catch(() => undefined); await jest.advanceTimersByTimeAsync(100); - await expect(read).rejects.toMatchObject(timeout408); - - await jest.advanceTimersByTimeAsync(20_000); + expect(attempts[0].signal.aborted).toBe(true); expect(fetchFn).toHaveBeenCalledTimes(1); - expect(attempts).toHaveLength(1); - expect( - (fetchFn as unknown as jest.Mock).mock.calls[0][1].headers[ - 'Idempotency-Key' - ], - ).toMatch(/^retry-/); + + // Backoff (max 1687.5ms at attempt 2), then the second attempt. + await jest.advanceTimersByTimeAsync(2000); + expect(fetchFn).toHaveBeenCalledTimes(2); + + const res = await request; + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + + const calls = (fetchFn as unknown as jest.Mock).mock.calls; + expect(calls[0][1].headers['Idempotency-Key']).toMatch(/^retry-/); + expect(calls[0][1].headers['Idempotency-Key']).toBe( + calls[1][1].headers['Idempotency-Key'], + ); + + await jest.advanceTimersByTimeAsync(10_000); + expect(attempts[1].signal.aborted).toBe(false); + expect(jest.getTimerCount()).toBe(0); }); - it('reads a stalled error body inside the attempt so the retry policy applies, with a fresh deadline per attempt', async () => { + it('reads a stalled error body inside the attempt so Retry-After applies, with a fresh deadline per attempt', async () => { const { client, fetchFn, attempts } = createClient( [ { @@ -889,13 +882,6 @@ describe('request timeout covers the response body (GH-1679)', () => { const res = await request; await expect(res.toJSON()).resolves.toEqual({ ok: true }); - const calls = (fetchFn as unknown as jest.Mock).mock.calls; - expect(calls[0][1].headers['Idempotency-Key']).toBe( - calls[1][1].headers['Idempotency-Key'], - ); - - // The first attempt's deadline cannot touch the second attempt, and - // the second attempt's deadline is gone once its body is consumed. await jest.advanceTimersByTimeAsync(10_000); expect(attempts[1].signal.aborted).toBe(false); expect(jest.getTimerCount()).toBe(0); @@ -947,6 +933,7 @@ describe('request timeout covers the response body (GH-1679)', () => { const { client, attempts } = createClient([{ body: '{ invalid' }]); const res = await client.get('/users', {}); + expect(jest.getTimerCount()).toBe(0); const error = await res.toJSON().catch((e) => e); expect(error).toBeInstanceOf(ParseError); @@ -956,171 +943,56 @@ describe('request timeout covers the response body (GH-1679)', () => { await jest.advanceTimersByTimeAsync(10_000); expect(attempts[0].signal.aborted).toBe(false); - expect(jest.getTimerCount()).toBe(0); }); it('propagates a body failure that is not a timeout unchanged', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); - const res = await client.get('/users', {}); - const read = res.toJSON(); - read.catch(() => undefined); - attempts[0].failBody(new TypeError('terminated')); - - await expect(read).rejects.toThrow(TypeError); - await expect(read).rejects.toThrow('terminated'); - await expect(read).rejects.not.toBeInstanceOf(HttpClientError); - - await jest.advanceTimersByTimeAsync(10_000); - expect(attempts[0].signal.aborted).toBe(false); - expect(jest.getTimerCount()).toBe(0); - }); - - it('preserves an AbortError raised by the body before the deadline and leaves the attempt unaborted', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - const independentAbort = abortError(); - - const res = await client.get('/users', {}); - const read = res.toJSON(); - read.catch(() => undefined); - attempts[0].failBody(independentAbort); - - await expect(read).rejects.toBe(independentAbort); - expect(attempts[0].signal.aborted).toBe(false); - - await jest.advanceTimersByTimeAsync(10_000); - expect(attempts[0].signal.aborted).toBe(false); - expect(jest.getTimerCount()).toBe(0); - }); - - it('still reports an AbortError from an error-body read as a timeout, as it did before the headers', async () => { - const { client, attempts } = createClient([ - { status: 500, body: 'pending' }, - ]); - const request = client.get('/users', {}); request.catch(() => undefined); await jest.advanceTimersByTimeAsync(0); - attempts[0].failBody(abortError()); + attempts[0].failBody(new TypeError('terminated')); - await expect(request).rejects.toMatchObject(timeout408); + await expect(request).rejects.toThrow(TypeError); + await expect(request).rejects.toThrow('terminated'); + await expect(request).rejects.not.toBeInstanceOf(HttpClientError); expect(jest.getTimerCount()).toBe(0); - }); - - it('rejects a second toJSON() while the first is pending without disarming the first read', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - const res = await client.get('/users', {}); - const first = res.toJSON(); - first.catch(() => undefined); - - await expect(res.toJSON()).rejects.toThrow('body used already'); - expect(jest.getTimerCount()).toBe(1); + await jest.advanceTimersByTimeAsync(10_000); expect(attempts[0].signal.aborted).toBe(false); - - await jest.advanceTimersByTimeAsync(100); - await expect(first).rejects.toMatchObject(timeout408); - expect(attempts[0].signal.aborted).toBe(true); - }); - - it('rejects toJSON() once a raw read has started and leaves that read under the deadline', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - - const res = await client.get('/users', {}); - const rawRead = (res.getRawResponse() as Response).text(); - rawRead.catch(() => undefined); - - await expect(res.toJSON()).rejects.toThrow('body used already'); - expect(jest.getTimerCount()).toBe(1); - - await jest.advanceTimersByTimeAsync(100); - const error = await rawRead.catch((e) => e); - expect(error.name).toBe('AbortError'); - expect(attempts[0].signal.aborted).toBe(true); }); - it('rejects toJSON() while the raw body is locked and leaves the lock holder under the deadline', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - - const res = await client.get('/users', {}); - attempts[0].response.body.locked = true; - - await expect(res.toJSON()).rejects.toThrow('body used already'); - expect(attempts[0].textCalls).toBe(1); - expect(jest.getTimerCount()).toBe(1); - - await jest.advanceTimersByTimeAsync(100); - expect(attempts[0].signal.aborted).toBe(true); - }); - - it('releases the deadline once the body is consumed and keeps single-consumption semantics', async () => { + it('delivers a body that completes in time, releases the deadline and keeps the original response reachable', async () => { const { client, attempts } = createClient([{ body: '{"ok":true}' }]); const res = await client.get('/users', {}); - await expect(res.toJSON()).resolves.toEqual({ ok: true }); expect(jest.getTimerCount()).toBe(0); + await expect(res.toJSON()).resolves.toEqual({ ok: true }); + expect(res.getRawResponse()).toBe(attempts[0].response); await jest.advanceTimersByTimeAsync(10_000); expect(attempts[0].signal.aborted).toBe(false); - - await expect(res.toJSON()).rejects.toThrow('body used already'); - }); - - it('fails toJSON() immediately once the deadline has already passed', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - - const res = await client.get('/users', {}); - await jest.advanceTimersByTimeAsync(100); - expect(attempts[0].signal.aborted).toBe(true); - - await expect(res.toJSON()).rejects.toMatchObject(timeout408); - expect(attempts[0].textCalls).toBe(0); }); - it('returns the exact original object from getRawResponse() and leaves raw reads under the same deadline', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); - - const res = await client.get('/users', {}); - const raw = res.getRawResponse() as Response; - expect(raw).toBe(attempts[0].response); - - const rawRead = raw.text(); - rawRead.catch(() => undefined); - await jest.advanceTimersByTimeAsync(100); - - const error = await rawRead.catch((e) => e); - expect(error.name).toBe('AbortError'); - expect(error).not.toBeInstanceOf(HttpClientError); - }); - - it('leaves the deadline armed for an unread non-JSON body and releases it for an absent one', async () => { - const withBody = createClient([ - { headers: { 'content-type': 'text/plain' }, body: 'pending' }, - ]); - const withoutBody = createClient([ - { headers: { 'content-type': 'text/plain' }, body: null }, + it('drains a non-JSON body inside the deadline and still reports it as null', async () => { + const { client, attempts } = createClient([ + { headers: { 'content-type': 'text/plain' }, body: 'hello' }, ]); - const first = await withBody.client.get('/users', {}); - const second = await withoutBody.client.get('/users', {}); - await expect(first.toJSON()).resolves.toBeNull(); - await expect(second.toJSON()).resolves.toBeNull(); - expect(withBody.attempts[0].textCalls).toBe(0); - expect(jest.getTimerCount()).toBe(1); - - await jest.advanceTimersByTimeAsync(100); - expect(withBody.attempts[0].signal.aborted).toBe(true); - expect(withoutBody.attempts[0].signal.aborted).toBe(false); + const res = await client.get('/users', {}); + expect(attempts[0].textCalls).toBe(1); expect(jest.getTimerCount()).toBe(0); + await expect(res.toJSON()).resolves.toBeNull(); }); - it('bounds a low-level delete response that nobody reads', async () => { + it('bounds a delete response whose body stalls instead of resolving on the headers', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); - await client.delete('/users/123', {}); - expect(jest.getTimerCount()).toBe(1); - + const request = client.delete('/users/123', {}); + request.catch(() => undefined); await jest.advanceTimersByTimeAsync(100); + + await expect(request).rejects.toMatchObject(timeout408); expect(attempts[0].signal.aborted).toBe(true); expect(jest.getTimerCount()).toBe(0); }); @@ -1134,7 +1006,7 @@ describe('request timeout covers the response body (GH-1679)', () => { describe.each([ ['native fetch', nativeFetch], - ['node-fetch 2.7.0', nodeFetch], + ['node-fetch', nodeFetch], ])('over real HTTP with %s', (_name, fetchImpl) => { let server: http.Server; let baseURL: string; @@ -1170,7 +1042,7 @@ describe('request timeout covers the response body (GH-1679)', () => { function createClient(maxRetries = 0) { const signals: AbortSignal[] = []; - const rawResponses: unknown[] = []; + const rawResponses: Response[] = []; const fetchFn: FetchFn = async (url, init) => { signals.push(init!.signal as AbortSignal); const response = await fetchImpl(url, init); @@ -1214,14 +1086,11 @@ describe('request timeout covers the response body (GH-1679)', () => { openResponses.length = 0; }); - it('times out a successful JSON body that stalls after the headers', async () => { + it('times out a successful body that stalls after the headers', async () => { stallBody(200); const { client, signals } = createClient(); - const res = await client.get('/users', {}); - expect(res.getStatusCode()).toBe(200); - - const error = await res.toJSON().catch((e) => e); + const error = await client.get('/users', {}).catch((e) => e); expect(error).toBeInstanceOf(HttpClientError); expect(error).toMatchObject(timeout408); expect(error.response.headers.get('x-request-id')).toBe('req_real'); @@ -1282,8 +1151,7 @@ describe('request timeout covers the response body (GH-1679)', () => { }; const { client } = createClient(); - const res = await client.get('/users', {}); - const error = await res.toJSON().catch((e) => e); + const error = await client.get('/users', {}).catch((e) => e); // The implementation's own error (undici raises it in Jest's outer // realm, so no instanceof Error here), not a timeout or parse error. expect(typeof error.message).toBe('string'); @@ -1292,79 +1160,16 @@ describe('request timeout covers the response body (GH-1679)', () => { expect(error.name).not.toBe('AbortError'); }); - it("hands back the implementation's own response for raw access, still under the deadline", async () => { - stallBody(200); + it("reads the body inside the attempt, so the implementation's own response comes back already consumed", async () => { + respond(200, '{}'); const { client, rawResponses, signals } = createClient(); - const res = await client.get('/users', {}); - const raw = res.getRawResponse() as Response; - expect(raw).toBe(rawResponses[0]); - - const error = await raw.text().catch((e) => e); - expect(error.name).toBe('AbortError'); - expect(error).not.toBeInstanceOf(HttpClientError); - expect(signals[0].aborted).toBe(true); - }); - - it('rejects a competing toJSON() and still aborts the first read at the deadline', async () => { - stallBody(200); - const { client, signals } = createClient(); - - const res = await client.get('/users', {}); - const first = res.toJSON(); - first.catch(() => undefined); - - await expect(res.toJSON()).rejects.toMatchObject({ name: 'TypeError' }); - expect(signals[0].aborted).toBe(false); - - await expect(first).rejects.toMatchObject(timeout408); - expect(signals[0].aborted).toBe(true); - }); - - it('rejects toJSON() after a raw read has started and still aborts that read at the deadline', async () => { - stallBody(200); - const { client, signals } = createClient(); - - const res = await client.get('/users', {}); - const rawRead = (res.getRawResponse() as Response).text(); - rawRead.catch(() => undefined); + const res = await client.delete('/users/123', {}); + expect(res.getRawResponse()).toBe(rawResponses[0]); + expect(rawResponses[0].bodyUsed).toBe(true); - await expect(res.toJSON()).rejects.toMatchObject({ name: 'TypeError' }); + await sleep(150); expect(signals[0].aborted).toBe(false); - - const error = await rawRead.catch((e) => e); - expect(error.name).toBe('AbortError'); - expect(signals[0].aborted).toBe(true); - }); - - if (fetchImpl === nativeFetch) { - // Only the Web stream body can be locked by a reader. - it('rejects toJSON() while the raw body is locked and still aborts the reader at the deadline', async () => { - stallBody(200); - const { client, signals } = createClient(); - - const res = await client.get('/users', {}); - const reader = (res.getRawResponse() as Response).body!.getReader(); - await reader.read(); - - await expect(res.toJSON()).rejects.toMatchObject({ - name: 'TypeError', - }); - expect(signals[0].aborted).toBe(false); - - const error = await reader.read().catch((e) => e); - expect(error.name).toBe('AbortError'); - expect(signals[0].aborted).toBe(true); - }); - } - - it('lets an unread response reach its deadline', async () => { - stallBody(200); - const { client, signals } = createClient(); - - await client.delete('/users/123', {}); - await sleep(200); - expect(signals[0].aborted).toBe(true); }); }); }); diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index b3d1fb709..9f6b5d890 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -17,140 +17,6 @@ import { ParseError } from '../exceptions/parse-error'; type FetchHttpClientOptions = HttpClientOptions; const DEFAULT_FETCH_TIMEOUT = 60_000; // 60 seconds - -/** - * Deadline for one request attempt, from the initial `fetch()` through to - * the last byte of the response body. - */ -interface RequestTimeout { - /** - * Run one step of the attempt (the `fetch()` itself or a body read) under - * the deadline. A step interrupted by the deadline, or started after it has - * passed, rejects with the SDK's 408 `HttpClientError`; any other failure - * propagates unchanged, including an `AbortError` raised for some other - * reason while the deadline is still running. Pass the response `headers` - * once they are known so the request ID and `Retry-After` survive the - * translation. `abortIsTimeout` keeps the pre-existing request-path - * behaviour of treating any `AbortError` as the timeout. - */ - guard(operation: () => Promise, options?: GuardOptions): Promise; - /** Disarm the deadline. Idempotent. */ - release(): void; - /** - * Stop the armed deadline from keeping the process alive on its own while - * no SDK read is awaiting it. `guard()` references it again. - */ - unref(): void; -} - -/** - * The deadline armed by `fetchRequest()` alongside the attempt's - * `AbortController`. - * - * It is disarmed when the body has been consumed or the attempt has failed, - * not when the headers arrive, so a server that responds promptly and then - * stalls the body still trips the configured timeout (GH-1679). Ownership - * follows the body: `fetchRequest()` keeps it while reading error bodies and - * hands it to `FetchHttpClientResponse` for successful responses. A body - * nobody reads through the SDK (raw access, a discarded delete response, a - * non-JSON response) keeps the original deadline as a bounded fallback: when - * it fires the attempt's controller is aborted and the timer is gone. - */ -class AttemptTimeout implements RequestTimeout { - private handle: ReturnType | null; - private expired = false; - - constructor( - controller: AbortController, - private readonly timeoutMs: number, - ) { - this.handle = setTimeout(() => { - this.handle = null; - this.expired = true; - controller.abort(); - }, timeoutMs); - } - - async guard( - operation: () => Promise, - { headers, abortIsTimeout = false }: GuardOptions = {}, - ): Promise { - if (this.expired) { - throw this.timeoutError(headers); - } - - this.setRef(true); - - try { - return await operation(); - } catch (error) { - // The deadline's own expiry is the timeout signal. A caller-supplied - // fetch can abort for reasons of its own; on the successful-body path - // that failure is theirs and passes through unchanged. - if ( - this.expired || - (abortIsTimeout && AttemptTimeout.isAbortError(error)) - ) { - throw this.timeoutError(headers); - } - throw error; - } - } - - release(): void { - if (this.handle !== null) { - clearTimeout(this.handle); - this.handle = null; - } - } - - unref(): void { - this.setRef(false); - } - - private setRef(referenced: boolean): void { - // Node timers can be unreferenced; browser and worker runtimes hand back - // a number, which has nothing to toggle. - const handle = this.handle as { - ref?: () => void; - unref?: () => void; - } | null; - - if (referenced) { - handle?.ref?.(); - } else { - handle?.unref?.(); - } - } - - private timeoutError(headers?: Headers): HttpClientError<{ error: string }> { - return new HttpClientError({ - message: `Request timeout after ${this.timeoutMs}ms`, - response: { - status: 408, - headers: headers ?? new Headers(), - data: { error: 'Request timeout' }, - }, - }); - } - - private static isAbortError(error: unknown): boolean { - return error instanceof Error && error.name === 'AbortError'; - } -} - -type GuardOptions = { - headers?: Headers; - abortIsTimeout?: boolean; -}; - -/** For responses constructed outside a request attempt. */ -const NO_TIMEOUT: RequestTimeout = { - guard: (operation) => operation(), - release: () => undefined, - unref: () => undefined, -}; - export class FetchHttpClient extends HttpClient implements HttpClientInterface { private readonly _fetchFn; @@ -324,46 +190,38 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { // Access timeout from the options with default of 60 seconds const timeout = this.options?.timeout ?? DEFAULT_FETCH_TIMEOUT; // Default 60 seconds const abortController = new AbortController(); - // Armed for the whole attempt, body included; see AttemptTimeout. - const requestTimeout = new AttemptTimeout(abortController, timeout); + const timeoutId = setTimeout(() => { + abortController.abort(); + }, timeout); + // Set once the headers arrive so a timeout while reading the body still + // carries the request ID and Retry-After. + let res: Response | undefined; try { - // As before this deadline covered the body, an AbortError from the - // request itself or from an error-body read is reported as a timeout. - const res = await requestTimeout.guard( - () => - this._fetchFn(url, { - method, - headers: { - Accept: 'application/json, text/plain, */*', - 'Content-Type': 'application/json', - ...this.options?.headers, - ...headers, - 'User-Agent': (userAgent || 'workos-node').toString(), - }, - body: requestBody, - signal: abortController.signal, - }), - { abortIsTimeout: true }, - ); + res = await this._fetchFn(url, { + method, + headers: { + Accept: 'application/json, text/plain, */*', + 'Content-Type': 'application/json', + ...this.options?.headers, + ...headers, + 'User-Agent': (userAgent || 'workos-node').toString(), + }, + body: requestBody, + signal: abortController.signal, + }); + + // The deadline covers the body as well as the headers (GH-1679): a + // server that responds promptly and then stalls the body still times + // out, and a timeout here is retried like any other. + const rawBody = await res.text(); + + // Clear timeout once the whole response has arrived + clearTimeout(timeoutId); if (!res.ok) { const requestID = res.headers.get('X-Request-ID') ?? ''; - // Read the error body under the same deadline: a stalled error - // response surfaces as a 408 here, inside the attempt, where the - // retry policy already handles it. - let rawBody: string; - - try { - rawBody = await requestTimeout.guard(() => res.text(), { - headers: res.headers, - abortIsTimeout: true, - }); - } finally { - requestTimeout.release(); - } - let responseJson: any; try { @@ -390,12 +248,24 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { }, }); } - // The body is still on the wire: the deadline goes with the response. - return new FetchHttpClientResponse(res, requestTimeout); + return new FetchHttpClientResponse(res, rawBody); } catch (error) { - // Nothing took ownership of the deadline, so disarm it. Timeouts have - // already been translated to a 408 HttpClientError by guard(). - requestTimeout.release(); + // Clear timeout if request failed + clearTimeout(timeoutId); + + // Handle timeout errors. Checked on our own signal rather than the + // error's type: the AbortError a fetch implementation raises for an + // aborted body may come from another realm. + if (abortController.signal.aborted) { + throw new HttpClientError({ + message: `Request timeout after ${timeout}ms`, + response: { + status: 408, + headers: res?.headers ?? new Headers(), + data: { error: 'Request timeout' }, + }, + }); + } throw error; } @@ -549,26 +419,15 @@ export class FetchHttpClientResponse implements HttpClientResponseInterface { _res: Response; - private readonly _requestTimeout: RequestTimeout; + private readonly _rawBody: string; - constructor(res: Response, requestTimeout: RequestTimeout = NO_TIMEOUT) { + constructor(res: Response, rawBody: string) { super( res.status, FetchHttpClientResponse._transformHeadersToObject(res.headers), ); this._res = res; - this._requestTimeout = requestTimeout; - - if (res.body === null) { - // Nothing left to wait for. - requestTimeout.release(); - } else { - // Until toJSON() reads the body, or for good if nobody does (raw - // access, a discarded delete response, a non-JSON response), the - // deadline stays armed as a bounded fallback but must not keep the - // process alive by itself. - requestTimeout.unref(); - } + this._rawBody = rawBody; } getRawResponse(): Response { @@ -583,33 +442,13 @@ export class FetchHttpClientResponse return null; } - let rawBody: string; - - if (this._res.bodyUsed || this._res.body?.locked) { - // Someone else already holds the body: an earlier toJSON() or a raw - // consumer. Their read keeps the deadline; this call only surfaces - // the response's own rejection for a body that is already in use. - rawBody = await this._res.text(); - } else { - // This call owns the read. Only the read runs under the deadline: an - // interrupted body is a timeout, a complete but malformed one is still - // a ParseError below. - try { - rawBody = await this._requestTimeout.guard(() => this._res.text(), { - headers: this._res.headers, - }); - } finally { - this._requestTimeout.release(); - } - } - try { - return JSON.parse(rawBody); + return JSON.parse(this._rawBody); } catch (error) { if (error instanceof SyntaxError) { throw new ParseError({ message: error.message, - rawBody, + rawBody: this._rawBody, rawStatus: this._res.status, requestID: this._res.headers.get('X-Request-ID') ?? '', }); diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 06dedc28a..8503d592f 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -12,7 +12,6 @@ import { WorkOS } from './index'; import { WorkOS as WorkOSWorker } from './index.worker'; import { RateLimitExceededException } from './common/exceptions/rate-limit-exceeded.exception'; import { FetchHttpClient } from './common/net/fetch-client'; -import { HttpClientError } from './common/net/http-client'; import { SubtleCryptoProvider } from './common/crypto/subtle-crypto-provider'; jest.mock('./common/utils/runtime-info', () => ({ @@ -679,7 +678,6 @@ describe('WorkOS', () => { */ function stalledBodyFetch({ text }: { text?: () => Promise } = {}) { const signals: AbortSignal[] = []; - const textMock = jest.fn(text); const fetchFn = jest.fn(async (_url: any, init: any) => { const signal = init.signal as AbortSignal; signals.push(signal); @@ -690,99 +688,60 @@ describe('WorkOS', () => { 'X-Request-ID': 'req_body', 'content-type': 'application/json', }), - body: {}, text: - text !== undefined - ? textMock - : () => - new Promise((_resolve, reject) => { - signal.addEventListener('abort', () => - reject(abortError()), - ); - }), + text ?? + (() => + new Promise((_resolve, reject) => { + signal.addEventListener('abort', () => reject(abortError())); + })), }; }); - return { fetchFn, signals, textMock }; + return { fetchFn, signals }; } - const sleep = (ms: number) => - new Promise((resolve) => setTimeout(resolve, ms)); - - it('surfaces the same 408 OauthException as a timeout before the headers', async () => { - const { fetchFn, signals } = stalledBodyFetch(); - const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { - timeout: 20, - fetchFn: fetchFn as any, - }); - - await expect(workos.get('/path')).rejects.toMatchObject({ - name: 'OauthException', - status: 408, - requestID: 'req_body', - message: 'Error: Request timeout', - }); - expect(signals[0].aborted).toBe(true); - }); - - it('propagates a body read failure that is not a timeout unchanged', async () => { - const { fetchFn } = stalledBodyFetch({ - text: () => Promise.reject(new TypeError('terminated')), - }); - const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { - timeout: 20, - fetchFn: fetchFn as any, - }); - - const error = await workos.post('/path', {}).catch((e) => e); - expect(error).toBeInstanceOf(TypeError); - expect(error.message).toBe('terminated'); - }); - - it('propagates a non-timeout HttpClientError from the body read as the same object', async () => { - const transportError = new HttpClientError({ - message: 'stream failed', - response: { - status: 401, - headers: new Headers(), - data: { error: 'stream failed' }, - }, - }); - const { fetchFn } = stalledBodyFetch({ - text: () => Promise.reject(transportError), - }); - const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { - timeout: 20, - fetchFn: fetchFn as any, - }); - - await expect(workos.get('/path')).rejects.toBe(transportError); - }); - it.each([ + ['get', (workos: WorkOS) => workos.get('/path')], + ['post', (workos: WorkOS) => workos.post('/path', {})], ['delete', (workos: WorkOS) => workos.delete('/path')], [ 'deleteWithBody', (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), ], ])( - '%s resolves without reading the body and leaves the deadline as a bounded fallback', + '%s surfaces the same 408 OauthException as a timeout before the headers', async (_name, call) => { - const { fetchFn, signals, textMock } = stalledBodyFetch({ - text: () => new Promise(() => undefined), - }); + const { fetchFn, signals } = stalledBodyFetch(); const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { timeout: 20, + maxRetries: 0, fetchFn: fetchFn as any, }); - await expect(call(workos)).resolves.toBeUndefined(); - expect(textMock).not.toHaveBeenCalled(); - expect(signals[0].aborted).toBe(false); - - await sleep(60); + await expect(call(workos)).rejects.toMatchObject({ + name: 'OauthException', + status: 408, + requestID: 'req_body', + message: 'Error: Request timeout', + }); expect(signals[0].aborted).toBe(true); }, ); + + it('handles a body read failure that is not a timeout like a network failure before the headers', async () => { + const transportError = new TypeError('terminated'); + const { fetchFn } = stalledBodyFetch({ + text: () => Promise.reject(transportError), + }); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + maxRetries: 0, + fetchFn: fetchFn as any, + }); + + const error = await workos.post('/path', {}).catch((e) => e); + expect(error.message).toBe('Unexpected error: TypeError: terminated'); + expect(error.cause).toBe(transportError); + }); }); describe('when in a worker environment', () => { diff --git a/src/workos.ts b/src/workos.ts index 07b95f83b..7f0d568df 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -257,7 +257,7 @@ export class WorkOS { throw error; } - return { data: await this.readResponseJSON(path, res) }; + return { data: await res.toJSON() }; } async get( @@ -291,7 +291,7 @@ export class WorkOS { throw error; } - return { data: await this.readResponseJSON(path, res) }; + return { data: await res.toJSON() }; } async put( @@ -323,7 +323,7 @@ export class WorkOS { throw error; } - return { data: await this.readResponseJSON(path, res) }; + return { data: await res.toJSON() }; } async patch( @@ -355,7 +355,7 @@ export class WorkOS { throw error; } - return { data: await this.readResponseJSON(path, res) }; + return { data: await res.toJSON() }; } async delete( @@ -390,28 +390,6 @@ export class WorkOS { } } - /** - * Consume a successful response body. The request deadline covers the body, - * so a stall while it is still streaming surfaces from the transport as a - * 408 `HttpClientError` and is translated like a timeout before the headers. - * Everything else (a `ParseError` for malformed JSON, a network failure - * mid-body, any other error a custom transport raises) propagates unchanged. - */ - private async readResponseJSON( - path: string, - response: HttpClientResponseInterface, - ): Promise { - try { - return await response.toJSON(); - } catch (error) { - if (error instanceof HttpClientError && error.response.status === 408) { - this.handleHttpError({ path, error }); - } - - throw error; - } - } - emitWarning(warning: string) { // tslint:disable-next-line:no-console console.warn(`WorkOS: ${warning}`); From f6869e6ef5ab3b78a6601010798cb1a2d8c10be2 Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 21 Sep 2026 12:42:55 -0400 Subject: [PATCH 3/5] fix(fetch-client): do not retry a request whose successful body times out Reading the whole body inside the retryable attempt meant a 2xx response whose body stalled or failed re-entered the retry policy. The server had already applied that request, and PUT, PATCH and DELETE carry no idempotency key, so the retry could apply a mutation twice or turn a completed DELETE into a 404. Keep the error-body read inside the attempt, where the existing retry policy and Retry-After handling belong, but start the successful-body read as a promise handed to the response: it still runs under the attempt's deadline and a stall still surfaces as a 408 from toJSON(), translated by WorkOS like a timeout before the headers, but the retry loop never sees it. A body nobody awaits is still drained. --- README.md | 5 +- src/common/net/fetch-client.spec.ts | 129 ++++++++++++++++------------ src/common/net/fetch-client.ts | 72 ++++++++++------ src/workos.spec.ts | 42 ++++++--- src/workos.ts | 30 ++++++- 5 files changed, 181 insertions(+), 97 deletions(-) diff --git a/README.md b/README.md index 2e34c02e2..0224c0714 100644 --- a/README.md +++ b/README.md @@ -62,8 +62,9 @@ Set `maxRetries: 0` to disable automatic retries entirely. The request timeout (`timeout`, 60 seconds by default) covers each attempt from the request being sent to the response body being fully read, so a server that -returns headers promptly but stalls the body still times out. A body timeout is -reported as a 408 and retried like any other timeout. +returns headers promptly but stalls the body still times out. A timeout while +reading a successful response body is reported as a 408 but is not +automatically retried, because the server has already applied the request. ### Access token issuer validation diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index f2f3846ca..f84d53ffe 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -784,21 +784,22 @@ describe('request timeout covers the response body (GH-1679)', () => { beforeEach(() => jest.useFakeTimers()); afterEach(() => jest.useRealTimers()); - it('keeps the deadline armed after the headers arrive and fails a stalled body with a 408 carrying the response headers', async () => { + it('keeps the deadline armed after the headers arrive and fails a stalled JSON body with a 408', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); - const request = client.get('/users', {}); - request.catch(() => undefined); - const state = settledFlag(request); + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); + const state = settledFlag(read); await jest.advanceTimersByTimeAsync(99); expect(state.settled).toBe(false); expect(attempts[0].signal.aborted).toBe(false); await jest.advanceTimersByTimeAsync(1); - await expect(request).rejects.toThrow(HttpClientError); - await expect(request).rejects.toMatchObject(timeout408); - const error = await request.catch((e) => e); + await expect(read).rejects.toThrow(HttpClientError); + await expect(read).rejects.toMatchObject(timeout408); + const error = await read.catch((e) => e); expect(error.response.headers.get('x-request-id')).toBe('req_1679'); expect(attempts[0].signal.aborted).toBe(true); expect(attempts[0].textCalls).toBe(1); @@ -811,45 +812,36 @@ describe('request timeout covers the response body (GH-1679)', () => { ]); const request = client.get('/users', {}); - request.catch(() => undefined); - const state = settledFlag(request); + await jest.advanceTimersByTimeAsync(80); + const res = await request; - await jest.advanceTimersByTimeAsync(99); + const read = res.toJSON(); + read.catch(() => undefined); + const state = settledFlag(read); + + await jest.advanceTimersByTimeAsync(19); expect(state.settled).toBe(false); await jest.advanceTimersByTimeAsync(1); - await expect(request).rejects.toMatchObject(timeout408); + await expect(read).rejects.toMatchObject(timeout408); }); - it('retries a stalled successful body like any other timeout, with a fresh deadline and the same idempotency key', async () => { + it('does not retry a request whose successful body times out: the server has already applied it', async () => { const { client, fetchFn, attempts } = createClient( - [{ body: 'pending' }, { body: '{"ok":true}' }], - { maxRetries: 1 }, + [{ body: 'pending' }], + { maxRetries: 2 }, ); - const request = client.post('/users', { name: 'x' }, {}); - request.catch(() => undefined); + const res = await client.patch('/users/123', { name: 'x' }, {}); + const read = res.toJSON(); + read.catch(() => undefined); await jest.advanceTimersByTimeAsync(100); - expect(attempts[0].signal.aborted).toBe(true); - expect(fetchFn).toHaveBeenCalledTimes(1); - - // Backoff (max 1687.5ms at attempt 2), then the second attempt. - await jest.advanceTimersByTimeAsync(2000); - expect(fetchFn).toHaveBeenCalledTimes(2); + await expect(read).rejects.toMatchObject(timeout408); - const res = await request; - await expect(res.toJSON()).resolves.toEqual({ ok: true }); - - const calls = (fetchFn as unknown as jest.Mock).mock.calls; - expect(calls[0][1].headers['Idempotency-Key']).toMatch(/^retry-/); - expect(calls[0][1].headers['Idempotency-Key']).toBe( - calls[1][1].headers['Idempotency-Key'], - ); - - await jest.advanceTimersByTimeAsync(10_000); - expect(attempts[1].signal.aborted).toBe(false); - expect(jest.getTimerCount()).toBe(0); + await jest.advanceTimersByTimeAsync(20_000); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(attempts).toHaveLength(1); }); it('reads a stalled error body inside the attempt so Retry-After applies, with a fresh deadline per attempt', async () => { @@ -882,6 +874,11 @@ describe('request timeout covers the response body (GH-1679)', () => { const res = await request; await expect(res.toJSON()).resolves.toEqual({ ok: true }); + const calls = (fetchFn as unknown as jest.Mock).mock.calls; + expect(calls[0][1].headers['Idempotency-Key']).toBe( + calls[1][1].headers['Idempotency-Key'], + ); + await jest.advanceTimersByTimeAsync(10_000); expect(attempts[1].signal.aborted).toBe(false); expect(jest.getTimerCount()).toBe(0); @@ -933,32 +930,38 @@ describe('request timeout covers the response body (GH-1679)', () => { const { client, attempts } = createClient([{ body: '{ invalid' }]); const res = await client.get('/users', {}); - expect(jest.getTimerCount()).toBe(0); const error = await res.toJSON().catch((e) => e); expect(error).toBeInstanceOf(ParseError); expect(error.rawBody).toBe('{ invalid'); expect(error.rawStatus).toBe(200); expect(error.requestID).toBe('req_1679'); + expect(jest.getTimerCount()).toBe(0); await jest.advanceTimersByTimeAsync(10_000); expect(attempts[0].signal.aborted).toBe(false); }); - it('propagates a body failure that is not a timeout unchanged', async () => { - const { client, attempts } = createClient([{ body: 'pending' }]); + it('propagates a body failure that is not a timeout unchanged and does not retry it', async () => { + const { client, fetchFn, attempts } = createClient( + [{ body: 'pending' }], + { + maxRetries: 2, + }, + ); - const request = client.get('/users', {}); - request.catch(() => undefined); - await jest.advanceTimersByTimeAsync(0); + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); attempts[0].failBody(new TypeError('terminated')); - await expect(request).rejects.toThrow(TypeError); - await expect(request).rejects.toThrow('terminated'); - await expect(request).rejects.not.toBeInstanceOf(HttpClientError); + await expect(read).rejects.toThrow(TypeError); + await expect(read).rejects.toThrow('terminated'); + await expect(read).rejects.not.toBeInstanceOf(HttpClientError); expect(jest.getTimerCount()).toBe(0); await jest.advanceTimersByTimeAsync(10_000); + expect(fetchFn).toHaveBeenCalledTimes(1); expect(attempts[0].signal.aborted).toBe(false); }); @@ -966,8 +969,8 @@ describe('request timeout covers the response body (GH-1679)', () => { const { client, attempts } = createClient([{ body: '{"ok":true}' }]); const res = await client.get('/users', {}); - expect(jest.getTimerCount()).toBe(0); await expect(res.toJSON()).resolves.toEqual({ ok: true }); + expect(jest.getTimerCount()).toBe(0); expect(res.getRawResponse()).toBe(attempts[0].response); await jest.advanceTimersByTimeAsync(10_000); @@ -980,19 +983,21 @@ describe('request timeout covers the response body (GH-1679)', () => { ]); const res = await client.get('/users', {}); + await expect(res.toJSON()).resolves.toBeNull(); expect(attempts[0].textCalls).toBe(1); + + await jest.advanceTimersByTimeAsync(0); expect(jest.getTimerCount()).toBe(0); - await expect(res.toJSON()).resolves.toBeNull(); }); - it('bounds a delete response whose body stalls instead of resolving on the headers', async () => { + it('bounds a stalled body that nobody reads without failing the call', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); - const request = client.delete('/users/123', {}); - request.catch(() => undefined); - await jest.advanceTimersByTimeAsync(100); + await client.delete('/users/123', {}); + expect(attempts[0].textCalls).toBe(1); + expect(jest.getTimerCount()).toBe(1); - await expect(request).rejects.toMatchObject(timeout408); + await jest.advanceTimersByTimeAsync(100); expect(attempts[0].signal.aborted).toBe(true); expect(jest.getTimerCount()).toBe(0); }); @@ -1086,11 +1091,14 @@ describe('request timeout covers the response body (GH-1679)', () => { openResponses.length = 0; }); - it('times out a successful body that stalls after the headers', async () => { + it('times out a successful JSON body that stalls after the headers', async () => { stallBody(200); const { client, signals } = createClient(); - const error = await client.get('/users', {}).catch((e) => e); + const res = await client.get('/users', {}); + expect(res.getStatusCode()).toBe(200); + + const error = await res.toJSON().catch((e) => e); expect(error).toBeInstanceOf(HttpClientError); expect(error).toMatchObject(timeout408); expect(error.response.headers.get('x-request-id')).toBe('req_real'); @@ -1098,6 +1106,18 @@ describe('request timeout covers the response body (GH-1679)', () => { expect(requestCount).toBe(1); }); + it('does not retry a successful body that times out, even when retries are enabled', async () => { + stallBody(200); + const { client } = createClient(2); + + const res = await client.patch('/users/123', { name: 'x' }, {}); + await expect(res.toJSON()).rejects.toMatchObject(timeout408); + + // Retry-After is 0, so a retry would already have been sent. + await sleep(50); + expect(requestCount).toBe(1); + }); + it('times out a stalled error body inside the attempt and retries it with a fresh deadline', async () => { // First attempt stalls its 503 body; the retry gets a clean 200. handler = (_req, res) => { @@ -1151,7 +1171,8 @@ describe('request timeout covers the response body (GH-1679)', () => { }; const { client } = createClient(); - const error = await client.get('/users', {}).catch((e) => e); + const res = await client.get('/users', {}); + const error = await res.toJSON().catch((e) => e); // The implementation's own error (undici raises it in Jest's outer // realm, so no instanceof Error here), not a timeout or parse error. expect(typeof error.message).toBe('string'); @@ -1160,7 +1181,7 @@ describe('request timeout covers the response body (GH-1679)', () => { expect(error.name).not.toBe('AbortError'); }); - it("reads the body inside the attempt, so the implementation's own response comes back already consumed", async () => { + it("hands back the implementation's own response, with the body read by the SDK", async () => { respond(200, '{}'); const { client, rawResponses, signals } = createClient(); diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index 9f6b5d890..b5f4292b0 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -193,8 +193,18 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { const timeoutId = setTimeout(() => { abortController.abort(); }, timeout); - // Set once the headers arrive so a timeout while reading the body still - // carries the request ID and Retry-After. + // Pass the response headers once they are known so a timeout while + // reading the body still carries the request ID and Retry-After. + const timeoutError = (responseHeaders?: Headers) => + new HttpClientError({ + message: `Request timeout after ${timeout}ms`, + response: { + status: 408, + headers: responseHeaders ?? new Headers(), + data: { error: 'Request timeout' }, + }, + }); + // Set once the headers arrive. let res: Response | undefined; try { @@ -211,16 +221,12 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { signal: abortController.signal, }); - // The deadline covers the body as well as the headers (GH-1679): a - // server that responds promptly and then stalls the body still times - // out, and a timeout here is retried like any other. - const rawBody = await res.text(); - - // Clear timeout once the whole response has arrived - clearTimeout(timeoutId); - if (!res.ok) { const requestID = res.headers.get('X-Request-ID') ?? ''; + // Read the error body under the same deadline, inside the attempt, + // so a stalled error response is retried like any other timeout. + const rawBody = await res.text(); + clearTimeout(timeoutId); let responseJson: any; @@ -248,23 +254,37 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { }, }); } + + // The deadline also covers a successful body (GH-1679), but the server + // has already applied this request, so the read happens outside the + // retry boundary: a stall surfaces from toJSON() as a 408 and is not + // retried. The body is read regardless of whether anyone awaits it, so + // an ignored response is drained and its deadline cleared. + const response = res; + const rawBody = (async () => { + try { + return await response.text(); + } catch (error) { + // Checked on our own signal rather than the error's type: the + // AbortError a fetch implementation raises for an aborted body may + // come from another realm. + throw abortController.signal.aborted + ? timeoutError(response.headers) + : error; + } finally { + clearTimeout(timeoutId); + } + })(); + rawBody.catch(() => undefined); + return new FetchHttpClientResponse(res, rawBody); } catch (error) { // Clear timeout if request failed clearTimeout(timeoutId); - // Handle timeout errors. Checked on our own signal rather than the - // error's type: the AbortError a fetch implementation raises for an - // aborted body may come from another realm. + // Handle timeout errors if (abortController.signal.aborted) { - throw new HttpClientError({ - message: `Request timeout after ${timeout}ms`, - response: { - status: 408, - headers: res?.headers ?? new Headers(), - data: { error: 'Request timeout' }, - }, - }); + throw timeoutError(res?.headers); } throw error; @@ -419,9 +439,9 @@ export class FetchHttpClientResponse implements HttpClientResponseInterface { _res: Response; - private readonly _rawBody: string; + private readonly _rawBody: Promise; - constructor(res: Response, rawBody: string) { + constructor(res: Response, rawBody: Promise) { super( res.status, FetchHttpClientResponse._transformHeadersToObject(res.headers), @@ -442,13 +462,15 @@ export class FetchHttpClientResponse return null; } + const rawBody = await this._rawBody; + try { - return JSON.parse(this._rawBody); + return JSON.parse(rawBody); } catch (error) { if (error instanceof SyntaxError) { throw new ParseError({ message: error.message, - rawBody: this._rawBody, + rawBody, rawStatus: this._res.status, requestID: this._res.headers.get('X-Request-ID') ?? '', }); diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 8503d592f..5501cd054 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -702,18 +702,14 @@ describe('WorkOS', () => { it.each([ ['get', (workos: WorkOS) => workos.get('/path')], ['post', (workos: WorkOS) => workos.post('/path', {})], - ['delete', (workos: WorkOS) => workos.delete('/path')], - [ - 'deleteWithBody', - (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), - ], + ['put', (workos: WorkOS) => workos.put('/path', {})], + ['patch', (workos: WorkOS) => workos.patch('/path', {})], ])( - '%s surfaces the same 408 OauthException as a timeout before the headers', + '%s surfaces the same 408 OauthException as a timeout before the headers, without retrying', async (_name, call) => { const { fetchFn, signals } = stalledBodyFetch(); const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { timeout: 20, - maxRetries: 0, fetchFn: fetchFn as any, }); @@ -724,24 +720,46 @@ describe('WorkOS', () => { message: 'Error: Request timeout', }); expect(signals[0].aborted).toBe(true); + expect(fetchFn).toHaveBeenCalledTimes(1); }, ); - it('handles a body read failure that is not a timeout like a network failure before the headers', async () => { + it('propagates a body read failure that is not a timeout unchanged', async () => { const transportError = new TypeError('terminated'); const { fetchFn } = stalledBodyFetch({ text: () => Promise.reject(transportError), }); const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { timeout: 20, - maxRetries: 0, fetchFn: fetchFn as any, }); - const error = await workos.post('/path', {}).catch((e) => e); - expect(error.message).toBe('Unexpected error: TypeError: terminated'); - expect(error.cause).toBe(transportError); + await expect(workos.post('/path', {})).rejects.toBe(transportError); + expect(fetchFn).toHaveBeenCalledTimes(1); }); + + it.each([ + ['delete', (workos: WorkOS) => workos.delete('/path')], + [ + 'deleteWithBody', + (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), + ], + ])( + '%s resolves on the headers and leaves the stalled body bounded by the deadline', + async (_name, call) => { + const { fetchFn, signals } = stalledBodyFetch(); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + await expect(call(workos)).resolves.toBeUndefined(); + expect(signals[0].aborted).toBe(false); + + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(signals[0].aborted).toBe(true); + }, + ); }); describe('when in a worker environment', () => { diff --git a/src/workos.ts b/src/workos.ts index 7f0d568df..72154aada 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -257,7 +257,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async get( @@ -291,7 +291,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async put( @@ -323,7 +323,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async patch( @@ -355,7 +355,7 @@ export class WorkOS { throw error; } - return { data: await res.toJSON() }; + return { data: await this.readResponseJSON(path, res) }; } async delete( @@ -390,6 +390,28 @@ export class WorkOS { } } + /** + * Consume a successful response body. The request deadline covers the body, + * so a stall while it is still streaming surfaces from the transport as a + * 408 `HttpClientError` and is translated like a timeout before the headers. + * Everything else (a `ParseError` for malformed JSON, a network failure + * mid-body) propagates unchanged. + */ + private async readResponseJSON( + path: string, + response: HttpClientResponseInterface, + ): Promise { + try { + return await response.toJSON(); + } catch (error) { + if (error instanceof HttpClientError) { + this.handleHttpError({ path, error }); + } + + throw error; + } + } + emitWarning(warning: string) { // tslint:disable-next-line:no-console console.warn(`WorkOS: ${warning}`); From 635a52ff14c3ddf610ad5c0117aeeedea4bcbfd8 Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 21 Sep 2026 12:55:47 -0400 Subject: [PATCH 4/5] fix(fetch-client): wait for the body before treating a non-JSON response as read toJSON() returned null for a non-JSON content type without observing the body read, so a stalled non-JSON body resolved the call early and its 408 was swallowed. Await the body first. delete() and deleteWithBody() now consume the response the same way, so every WorkOS request waits for the full body the timeout is documented to cover. --- src/common/net/fetch-client.spec.ts | 20 ++++++++++++++++++-- src/common/net/fetch-client.ts | 6 ++++-- src/workos.spec.ts | 28 +++++----------------------- src/workos.ts | 12 ++++++++++-- 4 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index f84d53ffe..3e94962d4 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -985,11 +985,27 @@ describe('request timeout covers the response body (GH-1679)', () => { const res = await client.get('/users', {}); await expect(res.toJSON()).resolves.toBeNull(); expect(attempts[0].textCalls).toBe(1); - - await jest.advanceTimersByTimeAsync(0); expect(jest.getTimerCount()).toBe(0); }); + it('fails toJSON() with a 408 for a stalled non-JSON body instead of resolving to null early', async () => { + const { client, attempts } = createClient([ + { headers: { 'content-type': 'text/plain' }, body: 'pending' }, + ]); + + const res = await client.get('/users', {}); + const read = res.toJSON(); + read.catch(() => undefined); + const state = settledFlag(read); + + await jest.advanceTimersByTimeAsync(99); + expect(state.settled).toBe(false); + + await jest.advanceTimersByTimeAsync(1); + await expect(read).rejects.toMatchObject(timeout408); + expect(attempts[0].signal.aborted).toBe(true); + }); + it('bounds a stalled body that nobody reads without failing the call', async () => { const { client, attempts } = createClient([{ body: 'pending' }]); diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index b5f4292b0..1b76c4f41 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -455,6 +455,10 @@ export class FetchHttpClientResponse } async toJSON(): Promise { + // Awaited before the content-type check so a stalled non-JSON body still + // surfaces its timeout instead of resolving to null early. + const rawBody = await this._rawBody; + const contentType = this._res.headers.get('content-type'); const isJsonResponse = contentType?.includes('application/json'); @@ -462,8 +466,6 @@ export class FetchHttpClientResponse return null; } - const rawBody = await this._rawBody; - try { return JSON.parse(rawBody); } catch (error) { diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 5501cd054..5b8d29bb2 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -704,6 +704,11 @@ describe('WorkOS', () => { ['post', (workos: WorkOS) => workos.post('/path', {})], ['put', (workos: WorkOS) => workos.put('/path', {})], ['patch', (workos: WorkOS) => workos.patch('/path', {})], + ['delete', (workos: WorkOS) => workos.delete('/path')], + [ + 'deleteWithBody', + (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), + ], ])( '%s surfaces the same 408 OauthException as a timeout before the headers, without retrying', async (_name, call) => { @@ -737,29 +742,6 @@ describe('WorkOS', () => { await expect(workos.post('/path', {})).rejects.toBe(transportError); expect(fetchFn).toHaveBeenCalledTimes(1); }); - - it.each([ - ['delete', (workos: WorkOS) => workos.delete('/path')], - [ - 'deleteWithBody', - (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), - ], - ])( - '%s resolves on the headers and leaves the stalled body bounded by the deadline', - async (_name, call) => { - const { fetchFn, signals } = stalledBodyFetch(); - const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { - timeout: 20, - fetchFn: fetchFn as any, - }); - - await expect(call(workos)).resolves.toBeUndefined(); - expect(signals[0].aborted).toBe(false); - - await new Promise((resolve) => setTimeout(resolve, 60)); - expect(signals[0].aborted).toBe(true); - }, - ); }); describe('when in a worker environment', () => { diff --git a/src/workos.ts b/src/workos.ts index 72154aada..53f524bba 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -364,8 +364,10 @@ export class WorkOS { ): Promise { this.requireApiKey(path); + let res: HttpClientResponseInterface; + try { - await this.client.delete(path, { + res = await this.client.delete(path, { params: query, }); } catch (error) { @@ -373,6 +375,8 @@ export class WorkOS { throw error; } + + await this.readResponseJSON(path, res); } async deleteWithBody( @@ -381,13 +385,17 @@ export class WorkOS { ): Promise { this.requireApiKey(path); + let res: HttpClientResponseInterface; + try { - await this.client.deleteWithBody(path, entity, {}); + res = await this.client.deleteWithBody(path, entity, {}); } catch (error) { this.handleHttpError({ path, error }); throw error; } + + await this.readResponseJSON(path, res); } /** From 5f9c1893e746de7c7f7d5cd4500cd9c4d5a9a825 Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 21 Sep 2026 13:06:40 -0400 Subject: [PATCH 5/5] fix(workos): let delete() complete on the response again Routing delete() and deleteWithBody() through toJSON() made a successful deletion depend on its body parsing as JSON, so an empty or malformed body labelled application/json would have surfaced as a ParseError. Neither method uses the body; they resolve once the response arrives, as before, while the body is still drained in the background and bounded by the attempt's deadline. --- src/workos.spec.ts | 28 +++++++++++++++++++++++----- src/workos.ts | 12 ++---------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 5b8d29bb2..2e61a1107 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -704,11 +704,6 @@ describe('WorkOS', () => { ['post', (workos: WorkOS) => workos.post('/path', {})], ['put', (workos: WorkOS) => workos.put('/path', {})], ['patch', (workos: WorkOS) => workos.patch('/path', {})], - ['delete', (workos: WorkOS) => workos.delete('/path')], - [ - 'deleteWithBody', - (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), - ], ])( '%s surfaces the same 408 OauthException as a timeout before the headers, without retrying', async (_name, call) => { @@ -742,6 +737,29 @@ describe('WorkOS', () => { await expect(workos.post('/path', {})).rejects.toBe(transportError); expect(fetchFn).toHaveBeenCalledTimes(1); }); + + it.each([ + ['delete', (workos: WorkOS) => workos.delete('/path')], + [ + 'deleteWithBody', + (workos: WorkOS) => workos.deleteWithBody('/path', { id: 'x' }), + ], + ])( + '%s resolves once the response arrives, without requiring a body, and leaves a stalled body bounded by the deadline', + async (_name, call) => { + const { fetchFn, signals } = stalledBodyFetch(); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + timeout: 20, + fetchFn: fetchFn as any, + }); + + await expect(call(workos)).resolves.toBeUndefined(); + expect(signals[0].aborted).toBe(false); + + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(signals[0].aborted).toBe(true); + }, + ); }); describe('when in a worker environment', () => { diff --git a/src/workos.ts b/src/workos.ts index 53f524bba..72154aada 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -364,10 +364,8 @@ export class WorkOS { ): Promise { this.requireApiKey(path); - let res: HttpClientResponseInterface; - try { - res = await this.client.delete(path, { + await this.client.delete(path, { params: query, }); } catch (error) { @@ -375,8 +373,6 @@ export class WorkOS { throw error; } - - await this.readResponseJSON(path, res); } async deleteWithBody( @@ -385,17 +381,13 @@ export class WorkOS { ): Promise { this.requireApiKey(path); - let res: HttpClientResponseInterface; - try { - res = await this.client.deleteWithBody(path, entity, {}); + await this.client.deleteWithBody(path, entity, {}); } catch (error) { this.handleHttpError({ path, error }); throw error; } - - await this.readResponseJSON(path, res); } /**