From fa62564b9898566cccb3e1e8640937366b889625 Mon Sep 17 00:00:00 2001 From: Jack Decker <24392469+jackowfish@users.noreply.github.com> Date: Wed, 15 Jul 2026 13:43:26 +0200 Subject: [PATCH] Exec runs unbounded by default and is never retried --- src/_baseClient.ts | 28 ++++++++++++----- src/_errors.ts | 10 ++++++ src/resources/sandboxes.ts | 4 ++- tests/timeout.test.ts | 63 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 tests/timeout.test.ts diff --git a/src/_baseClient.ts b/src/_baseClient.ts index 85076be..2740a61 100644 --- a/src/_baseClient.ts +++ b/src/_baseClient.ts @@ -1,4 +1,4 @@ -import { errorForStatus, SandboxError } from './_errors.js'; +import { errorForStatus, SandboxError, SandboxTimeoutError } from './_errors.js'; import { resolveConfig, type Config, type ConfigInput } from './_config.js'; import { DEFAULT_MAX_RETRIES, shouldRetry, sleepForAttempt } from './_retries.js'; @@ -13,6 +13,13 @@ export interface RequestOptions { path: string; query?: Record | undefined; body?: unknown; + // Per-request timeout. `null` disables the timeout entirely, used for + // long-running calls like exec, where the API works for the full duration + // of the request. Omitted means the client-wide default. + timeoutMs?: number | null | undefined; + // Set false for calls that must not be re-sent (exec): a failed attempt may + // have executed server-side, so retrying could run the command again. + retry?: boolean | undefined; } // HTTP transport shared by the generated resource classes. Owns auth/header @@ -36,16 +43,21 @@ export class BaseClient { async request(options: RequestOptions): Promise { const url = buildUrl(this.config.baseUrl, options.path, options.query); const init = buildInit(this.config, options); + const timeoutMs = options.timeoutMs === undefined ? this.config.timeoutMs : options.timeoutMs; + const maxRetries = options.retry === false ? 0 : this.maxRetries; let lastError: unknown = null; - for (let attempt = 0; attempt <= this.maxRetries; attempt++) { + for (let attempt = 0; attempt <= maxRetries; attempt++) { let response: Response; try { - response = await withTimeout(fetch(url, init), this.config.timeoutMs); + response = await withTimeout(fetch(url, init), timeoutMs); } catch (err) { + // A timed-out request may have executed server-side, so retrying + // could run it again. Surface the timeout instead. + if (err instanceof SandboxTimeoutError) throw err; lastError = err; - if (attempt < this.maxRetries) { + if (attempt < maxRetries) { await sleepForAttempt(attempt); continue; } @@ -56,7 +68,7 @@ export class BaseClient { return (await decodeBody(response)) as T; } - if (shouldRetry(response.status) && attempt < this.maxRetries) { + if (shouldRetry(response.status) && attempt < maxRetries) { await sleepForAttempt(attempt); continue; } @@ -100,13 +112,15 @@ const buildInit = (config: Config, options: RequestOptions): RequestInit => { return { method: options.method, headers, body }; }; -const withTimeout = (promise: Promise, timeoutMs: number): Promise => { +const withTimeout = (promise: Promise, timeoutMs: number | null): Promise => { + // `null` disables the timeout: the caller waits as long as the request runs. + if (timeoutMs === null) return promise; // AbortController would be cleaner, but fetch in older Node 18 had spotty // signal support. setTimeout+race is universally supported and keeps the // pending fetch from leaking once we've already given up. let timer: NodeJS.Timeout | undefined; const timeout = new Promise((_, reject) => { - timer = setTimeout(() => reject(new Error(`request timed out after ${timeoutMs}ms`)), timeoutMs); + timer = setTimeout(() => reject(new SandboxTimeoutError(`request timed out after ${timeoutMs}ms`)), timeoutMs); }); return Promise.race([promise, timeout]).finally(() => { if (timer) clearTimeout(timer); diff --git a/src/_errors.ts b/src/_errors.ts index 16d64a6..26607a3 100644 --- a/src/_errors.ts +++ b/src/_errors.ts @@ -46,6 +46,16 @@ export class ServerError extends SandboxError { } } +// Raised when a request exceeds its timeout. Never retried by the client: the +// server may still be executing the request (e.g. a long-running exec), so a +// retry could run it again. +export class SandboxTimeoutError extends SandboxError { + constructor(message: string) { + super(message); + this.name = 'SandboxTimeoutError'; + } +} + // Map an HTTP status code to the appropriate SandboxError subclass. export const errorForStatus = ( statusCode: number, diff --git a/src/resources/sandboxes.ts b/src/resources/sandboxes.ts index 1da9177..ca675f0 100644 --- a/src/resources/sandboxes.ts +++ b/src/resources/sandboxes.ts @@ -106,11 +106,13 @@ export class Sandboxes { * * Run a command in a running sandbox and return its output and exit code */ - async exec(id: string, body: ExecRequest): Promise { + async exec(id: string, body: ExecRequest, requestOptions?: { timeoutMs?: number }): Promise { return this.client.request({ method: 'POST', path: `/v1/sandbox/${id}/exec`, body, + timeoutMs: requestOptions?.timeoutMs ?? null, + retry: false, }); } } diff --git a/tests/timeout.test.ts b/tests/timeout.test.ts new file mode 100644 index 0000000..6a67323 --- /dev/null +++ b/tests/timeout.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { BaseClient } from '../src/_baseClient.js'; +import { SandboxTimeoutError } from '../src/_errors.js'; + +const jsonResponse = (): Response => + new Response('{}', { status: 200, headers: { 'content-type': 'application/json' } }); + +const slowFetch = (delayMs: number): typeof fetch => + vi.fn( + () => + new Promise((resolve) => { + setTimeout(() => resolve(jsonResponse()), delayMs); + }), + ); + +describe('request timeouts', () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('timeoutMs: null disables the client-wide timeout', async () => { + vi.stubGlobal('fetch', slowFetch(50)); + const client = new BaseClient({ baseUrl: 'http://sandbox.test', timeoutMs: 10 }); + + await expect( + client.request({ method: 'POST', path: '/v1/sandbox/abc/exec', timeoutMs: null }), + ).resolves.toEqual({}); + }); + + it('a per-request timeoutMs bounds the call and throws without retrying', async () => { + const fetchMock = slowFetch(50); + vi.stubGlobal('fetch', fetchMock); + const client = new BaseClient({ baseUrl: 'http://sandbox.test' }); + + await expect( + client.request({ method: 'POST', path: '/v1/sandbox/abc/exec', timeoutMs: 10 }), + ).rejects.toBeInstanceOf(SandboxTimeoutError); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('retry: false stops network errors from being re-sent', async () => { + const fetchMock = vi.fn().mockRejectedValue(new TypeError('fetch failed')); + vi.stubGlobal('fetch', fetchMock); + const client = new BaseClient({ baseUrl: 'http://sandbox.test' }); + + await expect( + client.request({ method: 'POST', path: '/v1/sandbox/abc/exec', retry: false }), + ).rejects.toThrow('Network error'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('other requests still retry network errors', async () => { + const fetchMock = vi + .fn() + .mockRejectedValueOnce(new TypeError('fetch failed')) + .mockResolvedValueOnce(jsonResponse()); + vi.stubGlobal('fetch', fetchMock); + const client = new BaseClient({ baseUrl: 'http://sandbox.test' }); + + await expect(client.request({ method: 'GET', path: '/v1/sandbox' })).resolves.toEqual({}); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +});