diff --git a/packages/host/tests/unit/virtual-network-retry-test.ts b/packages/host/tests/unit/virtual-network-retry-test.ts index 4377399441..3bbf864825 100644 --- a/packages/host/tests/unit/virtual-network-retry-test.ts +++ b/packages/host/tests/unit/virtual-network-retry-test.ts @@ -1,6 +1,11 @@ import { module, test } from 'qunit'; -import { shouldRetryFetch } from '@cardstack/runtime-common/virtual-network'; +import { VirtualNetwork } from '@cardstack/runtime-common'; +import { + buildRequest, + shouldRetryFetch, + shouldTimeoutRetryableFetch, +} from '@cardstack/runtime-common/virtual-network'; // `shouldRetryFetch` is the pure predicate that gates the in-app fetch-retry // safety net (see virtual-network.ts). The net papers over a transient CI @@ -101,3 +106,219 @@ module('Unit | virtual-network shouldRetryFetch', function () { }); }); }); + +// `shouldTimeoutRetryableFetch` gates the header-arrival timeout that turns a +// stall — response headers that never arrive, so the fetch hangs instead of +// throwing — into the same retryable failure the throw path already recovers +// from. It bounds only fetches that are already retryable, and only in the +// browser test suite: a slow response in node / worker / env-mode (e.g. a +// heavy `_search`) must never be aborted and retried. +module('Unit | virtual-network shouldTimeoutRetryableFetch', function () { + test('bounds a retryable base-realm fetch in the test environment', function (assert) { + withEnvironment('test', () => { + assert.true( + shouldTimeoutRetryableFetch( + new URL('https://realm-server.ci.localhost/base/_info'), + ), + 'the base _info fetch that can stall before headers is bounded', + ); + assert.true( + shouldTimeoutRetryableFetch( + new URL('https://cardstack.com/base/card-api'), + ), + 'a virtual base-realm artifact is bounded', + ); + }); + }); + + test('does not bound a non-retryable fetch even in the test environment', function (assert) { + withEnvironment('test', () => { + assert.false( + shouldTimeoutRetryableFetch( + new URL( + 'https://realm-server.ci.localhost/testuser/personal/Author/1', + ), + ), + 'a user realm on the same host is not bounded', + ); + assert.false( + shouldTimeoutRetryableFetch( + new URL('http://test-realm/test/SystemCard/default'), + ), + 'the in-browser test realm host is not bounded', + ); + }); + }); + + test('does not bound anything outside the test environment', function (assert) { + withEnvironment(undefined, () => { + assert.false( + shouldTimeoutRetryableFetch( + new URL('https://cardstack.com/base/card-api'), + ), + 'production / env-mode fetch flow keeps no header-timeout', + ); + }); + }); +}); + +// A native fetch whose first attempt stalls at the header stage (never +// resolves; rejects only when its request signal aborts, exactly as a real +// fetch does), then succeeds. Mirrors a base-realm fetch whose response headers +// never arrive, so the unbounded fetch hangs until QUnit's global timeout. +function stallingThenOkFetch(): { + fetch: typeof globalThis.fetch; + attempts: () => number; + signalSeen: () => boolean; +} { + let attempt = 0; + let sawAbort = false; + let fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + attempt++; + let signal = + input instanceof Request ? input.signal : (init?.signal ?? undefined); + if (attempt === 1) { + return new Promise((_resolve, reject) => { + if (!signal) { + // No signal reached the fetch, so nothing can abort this attempt: it + // stays pending and the test fails via QUnit's timeout. + return; + } + let onAbort = () => { + sawAbort = true; + reject(signal.reason); + }; + if (signal.aborted) { + onAbort(); + return; + } + signal.addEventListener('abort', onAbort, { once: true }); + }); + } + let ok = new Response('{"ok":true}', { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + // A real fetch response carries its resolved URL; mirror that (configurable + // so the virtual network's own url bookkeeping can still overwrite it) + // rather than returning the empty-url default a bare Response would have. + let url = input instanceof Request ? input.url : String(input); + Object.defineProperty(ok, 'url', { value: url, configurable: true }); + return Promise.resolve(ok); + }) as typeof globalThis.fetch; + return { fetch, attempts: () => attempt, signalSeen: () => sawAbort }; +} + +module('Unit | virtual-network header-stall recovery', function () { + test('aborts a stalled retryable base fetch and recovers on the retry', async function (assert) { + let g = globalThis as { __environment?: string }; + let had = '__environment' in g; + let prev = g.__environment; + g.__environment = 'test'; + try { + let { fetch, attempts, signalSeen } = stallingThenOkFetch(); + // Tiny header timeout so the stalled first attempt is aborted promptly; + // the second attempt then succeeds. + let vn = new VirtualNetwork(fetch, { fetchHeaderTimeoutMs: 20 }); + let response = await vn.fetch('https://cardstack.com/base/card-api'); + assert.strictEqual( + response.status, + 200, + 'recovered with a 200 after the stalled attempt was aborted', + ); + assert.true( + attempts() >= 2, + 'the stalled first attempt was retried on a fresh fetch', + ); + assert.true( + signalSeen(), + 'the aborted attempt fired the per-attempt timeout signal', + ); + } finally { + if (had) { + g.__environment = prev; + } else { + delete g.__environment; + } + } + }); + + // The host reaches the base realm through a virtual-to-real URL mapping, and + // the per-attempt timeout signal is attached before that mapping runs. The + // remapped request must carry the signal through, or the native fetch for a + // base artifact addressed at the virtual host never sees the abort. + test('buildRequest carries the abort signal onto the remapped request', async function (assert) { + let controller = new AbortController(); + let original = new Request('https://cardstack.com/base/_info', { + method: 'QUERY', + body: JSON.stringify({ realms: ['https://cardstack.com/base/'] }), + signal: controller.signal, + }); + let remapped = await buildRequest( + 'https://realm-server.ci.localhost/base/_info', + original, + ); + assert.notStrictEqual( + remapped.url, + original.url, + 'the request was rebuilt at the mapped real URL', + ); + assert.false(remapped.signal.aborted, 'not aborted before the abort fires'); + controller.abort(new Error('stall')); + assert.true( + remapped.signal.aborted, + 'aborting the original aborts the remapped request', + ); + }); + + test('a caller-initiated abort is surfaced at once, not retried', async function (assert) { + let g = globalThis as { __environment?: string }; + let had = '__environment' in g; + let prev = g.__environment; + g.__environment = 'test'; + try { + let attempts = 0; + let fetch = ((input: RequestInfo | URL) => { + attempts++; + let signal = input instanceof Request ? input.signal : undefined; + // Reject with the signal's reason when aborted, as a real fetch does. + return new Promise((_resolve, reject) => { + if (!signal) { + return; + } + let onAbort = () => reject(signal.reason); + if (signal.aborted) { + onAbort(); + } else { + signal.addEventListener('abort', onAbort, { once: true }); + } + }); + }) as typeof globalThis.fetch; + // A large header timeout so only the caller's abort ends the fetch. The + // URL is retryable, so without the guard this would retry the + // already-aborted request rather than surfacing the abort. + let vn = new VirtualNetwork(fetch, { fetchHeaderTimeoutMs: 60_000 }); + let caller = new AbortController(); + let pending = vn.fetch( + new Request('https://cardstack.com/base/card-api', { + signal: caller.signal, + }), + ); + caller.abort(); + let caught: { name?: string } | undefined; + try { + await pending; + } catch (e) { + caught = e as { name?: string }; + } + assert.strictEqual(caught?.name, 'AbortError', 'the abort was surfaced'); + assert.strictEqual(attempts, 1, 'the aborted request was not retried'); + } finally { + if (had) { + g.__environment = prev; + } else { + delete g.__environment; + } + } + }); +}); diff --git a/packages/runtime-common/virtual-network.ts b/packages/runtime-common/virtual-network.ts index ef4d8130f9..1e2e8a9290 100644 --- a/packages/runtime-common/virtual-network.ts +++ b/packages/runtime-common/virtual-network.ts @@ -42,8 +42,13 @@ export class VirtualNetwork { // relationship is only stable between changes. private mappingChangeListeners = new Set<() => void>(); - constructor(nativeFetch = createEnvironmentAwareFetch()) { + constructor( + nativeFetch = createEnvironmentAwareFetch(), + opts?: { fetchHeaderTimeoutMs?: number }, + ) { this.nativeFetch = nativeFetch; + this.fetchHeaderTimeoutMs = + opts?.fetchHeaderTimeoutMs ?? defaultFetchHeaderTimeoutMs; this.mount(this.packageShimHandler.handle); } @@ -385,6 +390,7 @@ export class VirtualNetwork { } private nativeFetch: typeof globalThis.fetch; + private fetchHeaderTimeoutMs: number; private resolveURLMapping( url: string, @@ -488,8 +494,22 @@ export class VirtualNetwork { return next(await this.mapRequest(request, 'virtual-to-real')); }); - return withRetries(new URL(request.url), () => - fetcher(this.nativeFetch, handlers, this)(request, init), + return withRetries( + new URL(request.url), + this.fetchHeaderTimeoutMs, + (attemptSignal?: AbortSignal) => { + // Each attempt gets its own abort signal (see withRetries) so that a + // fetch aborted for stalling on one attempt doesn't poison the next. + // Rebuild the Request from a clone rather than mutating the original: + // the original is reused across attempts and constructing a Request + // consumes its body. + let attemptRequest = attemptSignal + ? new Request(request.clone(), { + signal: mergeAbortSignals(request.signal, attemptSignal), + }) + : request; + return fetcher(this.nativeFetch, handlers, this)(attemptRequest, init); + }, ); } @@ -577,6 +597,62 @@ const maxAttempts = 10; const backOffMs = 100; const retryableLocalHosts = new Set(['localhost', '127.0.0.1']); +// Longest a retryable base-realm fetch may wait for response headers in the +// browser test suite before it is aborted and retried (see withRetries). A +// stall where the response headers never arrive is the failure mode the +// thrown-error retry net above does not cover: the fetch neither resolves nor +// rejects. Comfortably above normal base-artifact header latency (sub-second) +// yet far below a per-test timeout, so it only ever fires on a genuine stall, +// and the retry then gets its headers on a fresh connection. +const defaultFetchHeaderTimeoutMs = 10_000; + +// Whether a retryable fetch should also be bounded by a header-arrival timeout. +// Browser test suite only: `document` is absent in node / worker / env-mode +// processes, where a legitimately slow response (e.g. a heavy `_search`) must +// never be aborted and retried. `__environment === 'test'` is the same gate +// shouldRetryFetch uses to decide base-realm retryability in the host. +export function shouldTimeoutRetryableFetch(url: URL): boolean { + let g = globalThis as { document?: unknown; __environment?: string }; + return ( + typeof g.document !== 'undefined' && + g.__environment === 'test' && + shouldRetryFetch(url) + ); +} + +// Combine an optional caller-supplied signal with the per-attempt timeout +// signal so a fetch is aborted when either fires. +function mergeAbortSignals( + a: AbortSignal | null | undefined, + b: AbortSignal | null | undefined, +): AbortSignal | undefined { + let signals = [a, b].filter((s): s is AbortSignal => Boolean(s)); + if (signals.length <= 1) { + return signals[0]; + } + let combine = ( + AbortSignal as unknown as { + any?: (signals: AbortSignal[]) => AbortSignal; + } + ).any; + if (typeof combine === 'function') { + return combine(signals); + } + // Fallback for a runtime without AbortSignal.any: mirror both signals onto a + // single controller so either firing aborts the fetch, carrying its reason. + let controller = new AbortController(); + for (let signal of signals) { + if (signal.aborted) { + controller.abort(signal.reason); + break; + } + signal.addEventListener('abort', () => controller.abort(signal.reason), { + once: true, + }); + } + return controller.signal; +} + export function shouldRetryFetch(url: URL): boolean { // Env-mode services live at `..localhost` and are // reached through a local Traefik. The realm-server worker fetches @@ -631,13 +707,41 @@ export function shouldRetryFetch(url: URL): boolean { async function withRetries( url: URL, - fetchFn: () => ReturnType, + timeoutMs: number, + fetchFn: (attemptSignal?: AbortSignal) => ReturnType, ) { let attempt = 0; for (;;) { + // For a retryable fetch in the browser test suite, bound how long this + // attempt may wait for response headers. A stall where the headers never + // arrive would otherwise leave the fetch neither resolved nor rejected, + // hanging the fetcher test-waiter until QUnit's global timeout; the abort + // turns that into the same retryable failure the catch below recovers + // from. The timer is cleared the moment the fetch settles, so a resolved + // response's body stream is never aborted (withRetries only awaits the + // response's headers, not its body). + let controller: AbortController | undefined; + let timeoutId: ReturnType | undefined; + if (shouldTimeoutRetryableFetch(url)) { + controller = new AbortController(); + timeoutId = setTimeout(() => { + let timeoutError = new Error( + `fetch for ${url.href} exceeded ${timeoutMs}ms without response headers`, + ); + timeoutError.name = 'FetchHeaderTimeout'; + controller!.abort(timeoutError); + }, timeoutMs); + } try { - return await fetchFn(); + return await fetchFn(controller?.signal); } catch (err: any) { + // A caller-initiated abort is not a transient failure — surface it at + // once rather than retrying the already-aborted request. The per-attempt + // header-stall abort names its error `FetchHeaderTimeout`, so it falls + // through to the retry path below. + if (err?.name === 'AbortError') { + throw err; + } if (!shouldRetryFetch(url) || ++attempt > maxAttempts) { if (shouldRetryFetch(url) && attempt > maxAttempts) { // Final-exhaustion log: distinct from the per-attempt warning so @@ -650,17 +754,24 @@ async function withRetries( } throw err; } + // Include the error so a failure surfaces which shape it took — a thrown + // `TypeError: Failed to fetch` or an aborted header stall + // (`FetchHeaderTimeout`). console.error( - `Encountered fetch failed for ${ - url.href - } retry attempt #${attempt} in ${attempt * backOffMs}ms`, + `Encountered fetch failed for ${url.href} (${err?.name ?? 'Error'}: ${ + err?.message ?? String(err) + }) retry attempt #${attempt} in ${attempt * backOffMs}ms`, ); await new Promise((r) => setTimeout(r, attempt * backOffMs)); + } finally { + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + } } } } -async function buildRequest(url: string, originalRequest: Request) { +export async function buildRequest(url: string, originalRequest: Request) { if (url === originalRequest.url) { return originalRequest; } @@ -693,5 +804,11 @@ async function buildRequest(url: string, originalRequest: Request) { cache: originalRequest.cache, redirect: originalRequest.redirect, integrity: originalRequest.integrity, + // Carry the abort signal across the remap so a caller's abort — and the + // per-attempt header-stall timeout in withRetries — still cancels the + // native fetch. The host maps virtual base-realm URLs + // (https://cardstack.com/base/...) to the resolved realm URL here, so + // without this the base fetch that reaches the network has no signal. + signal: originalRequest.signal, }); } diff --git a/packages/software-factory/src/factory-skill-loader.ts b/packages/software-factory/src/factory-skill-loader.ts index bb5fc2a758..48c750f9f4 100644 --- a/packages/software-factory/src/factory-skill-loader.ts +++ b/packages/software-factory/src/factory-skill-loader.ts @@ -165,7 +165,7 @@ export const ALWAYS_LOAD_REFERENCES: readonly string[] = [ * and the validation test uses this set strictly: once a name ships in the * built skill, the test fails until it is removed from here. */ -export const PENDING_BOXEL_REFERENCES: readonly string[] = ['qunit-testing.md']; +export const PENDING_BOXEL_REFERENCES: readonly string[] = []; // --------------------------------------------------------------------------- // Internal types for tracking reference metadata diff --git a/packages/software-factory/tests/factory-skill-loader.test.ts b/packages/software-factory/tests/factory-skill-loader.test.ts index 6502f6aed7..6447dba969 100644 --- a/packages/software-factory/tests/factory-skill-loader.test.ts +++ b/packages/software-factory/tests/factory-skill-loader.test.ts @@ -1006,7 +1006,9 @@ module('factory-skill-loader > curated boxel references', function () { test('pending references are still pending', function (assert) { // Strict on purpose: once a pending name ships in the built skill, // remove it from PENDING_BOXEL_REFERENCES (and any transitional notes - // that reference it). + // that reference it). expect() accepts the empty case, where the loop + // runs no assertions. + assert.expect(PENDING_BOXEL_REFERENCES.length); for (let name of PENDING_BOXEL_REFERENCES) { assert.false( existsSync(join(builtRefsDir, name)),