From fe3a06e57f3ff847130e4d8b147572cf65ab329f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 16:58:02 +0000 Subject: [PATCH 1/2] Bound and remember the remote-realm visibility probe Placing a module that lives in a realm the serving realm-server does not host means asking that realm over the network. The probe was unbounded and its outcome was not remembered, so an origin that accepts no connections cost the platform's full connect timeout - 10s under Node's fetch - once per lookup, and again on every search that followed. The host suite produces that shape constantly. Its virtual realm at `http://test-realm/` is answered in-page, but a card chooser's search fans out to the live base realm, which then tries to reach `test-realm` for real. One CI shard's realm-server log carries 132 such probes, 55 of them paying the whole 10s, arriving in bursts of 10-15 for a single module - with `Integration | operator-mode | card chooser: cancel button closes the field picker` exhausting QUnit's 60s budget on top of them. Three bounds now hold that cost flat: every probe is bounded by a deadline the caller enforces, concurrent probes of one module by one caller share a request, and a transport failure is remembered against the origin that produced it for the same window an errored definition is cached for. An HTTP answer is never remembered, so a 404 still costs a probe next time. The deadline is raced on the caller rather than left to the request's abort signal. The signal is still passed, so the socket is torn down wherever the fetch stack honors it, but it cannot be relied on for the bound: the request is rebuilt by the virtual network's URL remapping, the retry wrapper and the undici dispatcher in turn, and measured against a realm-server serving a black-holed origin the probe ran to undici's 10s connect timeout with a 5s signal attached. Racing on the caller makes the bound hold regardless of which layer the signal survives. Reproduced against a local stack with `test-realm` resolving to an address that drops SYNs, which is what the CI runner's resolver effectively did. The card-chooser test spent 63.3s over 24 probes, each hitting undici's 10s connect timeout; with the origin remembered and the deadline enforced it spends 24.4s over 2 probes of 5.0s each. Also record per-test fetches that answered slowly and print them in the host timeout diagnostic. The in-flight snapshot names only what was still outstanding and the failed-fetch buffer only what rejected, so a run of requests that each answered in seconds - the signature here - left no trace in either. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TJp8v1CnpxAUrbtqcjKQYv --- packages/host/tests/helpers/setup.ts | 55 ++++- .../tests/definition-lookup-test.ts | 224 ++++++++++++++++++ packages/runtime-common/definition-lookup.ts | 167 ++++++++++++- 3 files changed, 436 insertions(+), 10 deletions(-) diff --git a/packages/host/tests/helpers/setup.ts b/packages/host/tests/helpers/setup.ts index c1eace22d6e..261c68e7196 100644 --- a/packages/host/tests/helpers/setup.ts +++ b/packages/host/tests/helpers/setup.ts @@ -59,6 +59,21 @@ let nextFetchId = 0; // cleared the buffer would repopulate it and misattribute URLs to N+1. const recentFailedFetches: string[] = []; const RECENT_FAILED_FETCHES_LIMIT = 20; + +// Fetches that took a long time but SUCCEEDED, per test. The in-flight +// snapshot names only what was still outstanding when the diagnostic ran, and +// `recentFailedFetches` only what rejected, so a request that answered +// eventually leaves no trace in either — yet a handful of them in sequence is +// how a test that does nothing wrong exhausts QUnit's 60s budget. A round trip +// whose far end has to reach a third service (a realm searching a type it has +// to place in another realm, say) is the shape that shows up here: correct, +// answered, and expensive enough that a few of them are the whole budget. +// Recorded with the duration so the timeout dump separates a suite that was +// stuck from one that was merely slow, and names the endpoint that cost the +// time. +const slowFetches: { desc: string; durationMs: number }[] = []; +const SLOW_FETCH_THRESHOLD_MS = 1_000; +const SLOW_FETCHES_LIMIT = 20; let currentTestEpoch = 0; // Module/name of the test currently running, captured from QUnit.testStart so a @@ -254,6 +269,7 @@ function setupFetchDebugging(hooks: NestedHooks) { hooks.beforeEach(function () { inFlightFetches.clear(); recentFailedFetches.length = 0; + slowFetches.length = 0; currentTestEpoch++; installTimeoutDiagnosticsOnce(); if (!globalThis.fetch) { @@ -265,9 +281,10 @@ function setupFetchDebugging(hooks: NestedHooks) { let { method, url } = describeFetchRequest(input, init); let id = nextFetchId++; let epoch = currentTestEpoch; + let startedAt = Date.now(); inFlightFetches.set(id, { desc: `${method} ${url}`, - startedAt: Date.now(), + startedAt, }); try { // Requests to a registered test realm are answered in-page rather @@ -297,6 +314,7 @@ function setupFetchDebugging(hooks: NestedHooks) { throw error; } finally { inFlightFetches.delete(id); + rememberSlowFetch(epoch, method, url, Date.now() - startedAt); } }; globalThis.fetch = wrappedFetch; @@ -335,6 +353,40 @@ function rememberFailedFetch( } } +function rememberSlowFetch( + epoch: number, + method: string, + url: string, + durationMs: number, +) { + // Same per-test gate as the failed-fetch buffer: a slow request that + // finishes after the next test has started belongs to neither test's + // diagnostic, so it is dropped rather than misattributed. + if (epoch !== currentTestEpoch || durationMs < SLOW_FETCH_THRESHOLD_MS) { + return; + } + slowFetches.push({ desc: `${method} ${url}`, durationMs }); + if (slowFetches.length > SLOW_FETCHES_LIMIT) { + // Keep the most expensive, not the most recent: the budget is spent by + // the slowest requests, and a test can make hundreds. + slowFetches.sort((a, b) => b.durationMs - a.durationMs); + slowFetches.length = SLOW_FETCHES_LIMIT; + } +} + +function summarizeSlowFetches(): string { + if (!slowFetches.length) { + return `completed fetches over ${SLOW_FETCH_THRESHOLD_MS}ms this test: `; + } + let ranked = [...slowFetches].sort((a, b) => b.durationMs - a.durationMs); + let total = ranked.reduce((sum, f) => sum + f.durationMs, 0); + return ( + `completed fetches over ${SLOW_FETCH_THRESHOLD_MS}ms this test ` + + `(${ranked.length}, ${total}ms in total):\n ` + + ranked.map((f) => `${f.desc} (took ${f.durationMs}ms)`).join('\n ') + ); +} + // Resolve the qunit runtime object (the one the qunit package installs on the // global) rather than the ES module namespace. Importing `import * as QUnit // from 'qunit'` produces a frozen module record where `onUncaughtException` @@ -453,6 +505,7 @@ function logRejectionDiagnostics(prefix: string, formattedReason: string) { recent.length ? `recent failed fetches this test (${recent.length}):\n ${recent.join('\n ')}` : 'recent failed fetches this test: ', + summarizeSlowFetches(), summarizeSettledState(), summarizePendingRunloopTimers(), summarizeRealmAuth(), diff --git a/packages/realm-server/tests/definition-lookup-test.ts b/packages/realm-server/tests/definition-lookup-test.ts index 5fc653a5ea6..5fa760c8285 100644 --- a/packages/realm-server/tests/definition-lookup-test.ts +++ b/packages/realm-server/tests/definition-lookup-test.ts @@ -7,6 +7,7 @@ import { logger, mintRealmLoaderEpoch, trimExecutableExtension, + type DefinitionLookup, type ErrorEntry, type JobInfo, type ModuleDefinitionResult, @@ -2779,6 +2780,229 @@ module(basename(import.meta.filename), function () { // setupPermissionedRealmsCached fixture — pre-warm runs in the worker, // which has no realm-server / prerender / Chromium, so we only need a pg // adapter, a fake registered realm for the reader, and a mock prerenderer. + module('remote realm visibility probe', function (hooks) { + let adapter: PgAdapter; + let localRealmURL = 'http://127.0.0.1:4453/'; + let testUserId = '@user1:localhost'; + + // A module in a realm this lookup does not host is placed by probing the + // realm that holds it, and every case below is one the probe cannot place: + // buildLookupContext returns null and the lookup throws before it reads or + // writes the definition cache. What is under test is therefore what the + // probe costs, not what gets cached. + let localRealm = { + url: localRealmURL, + async getRealmOwnerUserId() { + return testUserId; + }, + async visibility(): Promise<'private'> { + return 'private'; + }, + }; + + let unusedPrerenderer: Prerenderer = { + async prerenderModule(): Promise { + throw new Error( + 'a probe that cannot place its module must never reach the prerenderer', + ); + }, + async prerenderVisit() { + throw new Error('Not implemented in mock'); + }, + async runCommand() { + throw new Error('Not implemented in mock'); + }, + }; + + let virtualNetwork: VirtualNetwork; + let lookup: DefinitionLookup; + + hooks.before(async function () { + prepareTestDB(); + adapter = await createTestPgAdapter(); + }); + + hooks.after(async function () { + await adapter.close(); + }); + + hooks.beforeEach(function () { + virtualNetwork = createVirtualNetwork(); + lookup = new CachingDefinitionLookup( + adapter, + unusedPrerenderer, + virtualNetwork, + testCreatePrerenderAuth, + ).forRealm(localRealm); + }); + + test('a probe whose remote never answers is abandoned at its own deadline', async function (assert) { + let remoteModuleURL = 'http://silent-remote-realm/person.gts'; + let probed = false; + + // A remote that accepts the request and then goes quiet, and that + // ignores the abort signal entirely — which is the shape that matters, + // because the signal is rebuilt by every layer between here and the + // socket (URL remapping, the retry wrapper, the undici dispatcher) and + // a probe must be bounded even where none of them honors it. A handler + // that resolved on abort instead would pass on the strength of the + // signal alone and say nothing about the caller's own bound. + let handler = async (request: Request) => { + if (request.method !== 'HEAD' || request.url !== remoteModuleURL) { + return null; + } + probed = true; + return await new Promise(() => {}); + }; + virtualNetwork.mount(handler); + + try { + let startedAt = Date.now(); + await assert.rejects( + lookup.lookupDefinition({ + module: rri(remoteModuleURL), + name: 'Person', + }), + 'a realm that never answers the probe reads as one that cannot place the module', + ); + let elapsed = Date.now() - startedAt; + + assert.true(probed, 'the remote was probed'); + assert.ok( + elapsed < 9_000, + `the probe is abandoned on the caller's own deadline rather than on the network's (took ${elapsed}ms)`, + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + + test('concurrent probes of one module by one caller share a single request', async function (assert) { + let remoteModuleURL = 'http://slow-remote-realm/person.gts'; + let probes = 0; + let releaseProbe: (() => void) | undefined; + let probeGate = new Promise((resolve) => { + releaseProbe = resolve; + }); + + // A 404 answer: enough to establish that the probe was made, while + // leaving no unreachable-origin record behind — so the count below + // measures concurrent probes sharing one request and nothing else. + let handler = async (request: Request) => { + if (request.method !== 'HEAD' || request.url !== remoteModuleURL) { + return null; + } + probes++; + await probeGate; + return new Response(null, { status: 404 }); + }; + virtualNetwork.mount(handler); + + try { + let lookups = [1, 2, 3].map(() => + assert.rejects( + lookup.lookupDefinition({ + module: rri(remoteModuleURL), + name: 'Person', + }), + 'no module to place under a path the realm does not serve', + ), + ); + // Let all three lookups reach the probe before any probe answers, so + // a first probe that happened to settle early cannot be mistaken for + // three probes having been shared. + await new Promise((resolve) => setTimeout(resolve, 50)); + releaseProbe!(); + await Promise.all(lookups); + + assert.strictEqual( + probes, + 1, + 'three concurrent lookups of one module probe the remote realm once', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + + test('an origin whose probe failed at the transport is not probed again while the record stands', async function (assert) { + let remoteRealmURL = 'http://unreachable-remote-realm/'; + let probes = 0; + + // A transport failure — the connection never opens — as opposed to a + // server that answers with a status. It is a property of the origin, so + // the one record covers every module under it. + let handler = async (request: Request) => { + if ( + request.method !== 'HEAD' || + !request.url.startsWith(remoteRealmURL) + ) { + return null; + } + probes++; + throw new TypeError('fetch failed'); + }; + virtualNetwork.mount(handler); + + try { + for (let moduleName of ['person.gts', 'pet.gts', 'person.gts']) { + await assert.rejects( + lookup.lookupDefinition({ + module: rri(`${remoteRealmURL}${moduleName}`), + name: 'Person', + }), + `${moduleName} cannot be placed while the realm holding it is unreachable`, + ); + } + + assert.strictEqual( + probes, + 1, + 'the origin is probed once; later lookups under it read the record instead of the network', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + + test('a probe answered with a status is repeated rather than recorded', async function (assert) { + let remoteModuleURL = 'http://answering-remote-realm/person.gts'; + let probes = 0; + + // A 404 says the path is absent, not that the origin is unreachable, so + // it must not suppress the next probe: the module may appear a moment + // later, and a realm that answers is already cheap to ask. + let handler = async (request: Request) => { + if (request.method !== 'HEAD' || request.url !== remoteModuleURL) { + return null; + } + probes++; + return new Response(null, { status: 404 }); + }; + virtualNetwork.mount(handler); + + try { + for (let attempt of [1, 2]) { + await assert.rejects( + lookup.lookupDefinition({ + module: rri(remoteModuleURL), + name: 'Person', + }), + `attempt ${attempt} finds no module to place`, + ); + } + + assert.strictEqual( + probes, + 2, + 'an origin that answers is probed again on the next lookup', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + }); + module('module pre-warm (worker bare lookup)', function (hooks) { let adapter: PgAdapter; let realmURL = 'http://127.0.0.1:4452/'; diff --git a/packages/runtime-common/definition-lookup.ts b/packages/runtime-common/definition-lookup.ts index 229b2466424..a823248ba05 100644 --- a/packages/runtime-common/definition-lookup.ts +++ b/packages/runtime-common/definition-lookup.ts @@ -103,6 +103,30 @@ const COALESCE_MAX_ITERATIONS = 4; // converges as soon as no invalidation lands mid-populate; the cap keeps an // invalidation storm from spinning on prerenders instead of surfacing. const POPULATE_RACE_MAX_ATTEMPTS = 3; +// A module in a realm this server does not host can only be placed by asking +// the realm that holds it, so `buildLookupContext` reaches the network — and +// the cost of that request is set by the far end, not by anything local. An +// origin that accepts no connections spends the platform's connect timeout +// (10s under Node's fetch) before rejecting, and none of that is per-module: +// a search whose filter names types from an unreachable realm pays it once +// per type, and the next search pays the whole bill again. +// +// Three bounds keep it flat. Each probe carries its own deadline, so an +// unreachable origin costs the deadline rather than the platform's timeout — +// generous enough that a realm answering slowly under load is still seen. +// Concurrent probes for the same module and requesting user share one +// request. And a transport failure is remembered against the origin that +// produced it, short-circuiting later probes there until the window lapses: +// keyed by origin because a connection that never opens says nothing about +// the path. An HTTP answer — 404 included — is never remembered, since a +// server that answers at all is already fast. +// +// The record's window matches ERROR_CACHE_TTL_MS above, and for the same +// reason: it is long enough that a failure is not re-paid on every request +// that follows it, and short enough that a realm coming back is seen without +// anything having to be restarted or cleared. +const REALM_PROBE_TIMEOUT_MS = 5_000; +const REALM_PROBE_UNREACHABLE_TTL_MS = ERROR_CACHE_TTL_MS; const modulesTableCoerceTypes: TypeCoercion = Object.freeze({ definitions: 'JSON', deps: 'JSON', @@ -377,6 +401,61 @@ interface LookupContext { priority?: number; } +interface RemoteRealmProbe { + isPublic: boolean; + resolvedRealmURL?: string; +} + +// The origin a probe's transport failure is recorded against. A module URL +// that cannot be parsed has no origin to blame, so such a probe is bounded by +// its deadline alone and nothing is remembered. +function originOf(moduleURL: string): string | undefined { + try { + return new URL(moduleURL).origin; + } catch { + return undefined; + } +} + +// Settles with `work` if it finishes inside `ms`, and rejects otherwise. The +// abandoned promise's rejection is absorbed so it cannot surface as an +// unhandled rejection after the deadline has already been reported. +async function withDeadline( + work: Promise, + ms: number, + description: string, +): Promise { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + work, + new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error(`${description} exceeded ${ms}ms`)), + ms, + ); + }), + ]); + } finally { + if (timer !== undefined) { + clearTimeout(timer); + } + work.catch(() => {}); + } +} + +// Identifies the caller a probe is answered for, so probes are shared only +// among callers whose visibility answer must agree. +function probeAuthKey(headers?: HeadersInit): string { + if (!headers) { + return ''; + } + return [...new Headers(headers).entries()] + .map(([name, value]) => `${name}:${value}`) + .sort() + .join(','); +} + export class CachingDefinitionLookup implements DefinitionLookup { #dbAdapter: DBAdapter; #prerenderer: Prerenderer; @@ -410,6 +489,12 @@ export class CachingDefinitionLookup implements DefinitionLookup { #moduleGenerations = new Map(); #realmGenerations = new Map(); #globalGeneration = 0; + // Remote-realm visibility probes. `#probeInFlight` shares one request among + // concurrent probes of the same module and requesting user; + // `#unreachableOrigins` maps an origin whose transport failed to the + // timestamp its record stops applying. See REALM_PROBE_TIMEOUT_MS. + #probeInFlight = new Map>(); + #unreachableOrigins = new Map(); // CS-10953 cross-process prerender coalescer. Optional — when undefined, // loadDefinitionCacheEntryUncached runs the original uncoordinated path. // Constructed only by the realm-server main when @@ -1340,16 +1425,67 @@ export class CachingDefinitionLookup implements DefinitionLookup { private async probeRemoteRealm( moduleURL: string, headers?: HeadersInit, - ): Promise<{ - isPublic: boolean; - resolvedRealmURL?: string; - } | null> { + ): Promise { + let origin = originOf(moduleURL); + if (origin) { + let unreachableUntil = this.#unreachableOrigins.get(origin); + if (unreachableUntil !== undefined) { + if (unreachableUntil > Date.now()) { + log.debug( + `Skipping remote realm visibility probe for ${moduleURL}: ${origin} is recorded unreachable for another ${unreachableUntil - Date.now()}ms`, + ); + return null; + } + this.#unreachableOrigins.delete(origin); + } + } + + // The requesting user is part of the key: visibility is answered per + // caller, so two users probing one module must not share a result. + let key = `${moduleURL}|${probeAuthKey(headers)}`; + let inFlight = this.#probeInFlight.get(key); + if (inFlight) { + return await inFlight; + } + let probe = this.probeRemoteRealmUncoalesced(moduleURL, origin, headers); + this.#probeInFlight.set(key, probe); try { - let response = await this.#fetch(moduleURL, { - method: 'HEAD', - headers, - }); + return await probe; + } finally { + if (this.#probeInFlight.get(key) === probe) { + this.#probeInFlight.delete(key); + } + } + } + + private async probeRemoteRealmUncoalesced( + moduleURL: string, + origin: string | undefined, + headers?: HeadersInit, + ): Promise { + let startedAt = Date.now(); + try { + // The deadline is enforced on the caller, not only on the request. The + // signal is passed as well so the socket is torn down wherever the + // fetch stack honors it, but a probe must be bounded even where it does + // not: the request travels through the virtual network's remapping, + // retry and dispatcher layers, each of which rebuilds it, and a + // platform connect timeout an order of magnitude longer than this + // deadline sits underneath them all. Racing here makes the bound hold + // regardless of which layer the signal survives. + let response = await withDeadline( + this.#fetch(moduleURL, { + method: 'HEAD', + headers, + signal: AbortSignal.timeout(REALM_PROBE_TIMEOUT_MS), + }), + REALM_PROBE_TIMEOUT_MS, + `remote realm visibility probe for ${moduleURL}`, + ); if (!response.ok) { + log.debug( + `Remote realm visibility probe for ${moduleURL} answered ${response.status} in ${Date.now() - startedAt}ms`, + ); return null; } let publicReadable = response.headers.get( @@ -1365,7 +1501,20 @@ export class CachingDefinitionLookup implements DefinitionLookup { resolvedRealmURL, }; } catch (err) { - log.warn(`Failed to probe remote realm visibility for ${moduleURL}`, err); + if (origin) { + this.#unreachableOrigins.set( + origin, + Date.now() + REALM_PROBE_UNREACHABLE_TTL_MS, + ); + } + log.warn( + `Failed to probe remote realm visibility for ${moduleURL} after ${Date.now() - startedAt}ms${ + origin + ? `; treating ${origin} as unreachable for the next ${REALM_PROBE_UNREACHABLE_TTL_MS}ms` + : '' + }`, + err, + ); return null; } } From e3f09af7ab4c96599e3709f0f9eaf9c196c692d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 20:38:13 +0000 Subject: [PATCH 2/2] Address review of the remote-realm probe bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from the automated reviewers, all in the probe added here. `probeAuthKey` read the request headers with `Headers.entries()`, which is only typed under a `dom.iterable` lib. runtime-common is type-checked by packages that enable no such lib, so billing, ai-bot and bot-runner failed `lint:types` on a file none of them changed. Reads the headers with `forEach` instead, which every lib version carries. A probe that is answered now clears its origin's unreachable record. The record is written on transport failure and read before any probe is made, so the only probe that can reach an already-recorded origin is one that was already in flight when the record was written — and on that path an origin demonstrably answering was being reported as unable to place its types until the window lapsed. The unreachable record is swept and capped on insertion. A filter carries caller-supplied module URLs, and a record was only retired when its own origin was probed again, which for a one-off origin never happens; the map could grow for the life of the process. The deadline aborts with a reason named `AbortError` rather than the `TimeoutError` an `AbortSignal.timeout` raises. The retry wrapper classifies by name and treats only a caller's abort as final, so a `TimeoutError` was retried — leaving attempts running behind a deadline that had already been reported. The host timeout diagnostic counts only fetches that answered. Recording from `finally` also counted rejected ones, which `recentFailedFetches` already names, so failures were double-reported and inflated a total the dump presents as the cost of work that succeeded. Three tests, one per behavioral finding: a deadline on a retryable origin is not retried, an origin answering one probe clears a record another probe left, and two owners probing one module each get their own request carrying their own assumed user. The retryable-origin test rejects its request with the abort reason the way a real fetch does — a handler that ignores the signal leaves the request pending, the retry loop only ever sees rejections, and the test then passes whatever the reason is named. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TJp8v1CnpxAUrbtqcjKQYv --- packages/host/tests/helpers/setup.ts | 10 +- .../tests/definition-lookup-test.ts | 213 +++++++++++++++++- packages/runtime-common/definition-lookup.ts | 158 +++++++++---- 3 files changed, 328 insertions(+), 53 deletions(-) diff --git a/packages/host/tests/helpers/setup.ts b/packages/host/tests/helpers/setup.ts index 261c68e7196..a5d6d3ebd9f 100644 --- a/packages/host/tests/helpers/setup.ts +++ b/packages/host/tests/helpers/setup.ts @@ -282,6 +282,7 @@ function setupFetchDebugging(hooks: NestedHooks) { let id = nextFetchId++; let epoch = currentTestEpoch; let startedAt = Date.now(); + let rejected = false; inFlightFetches.set(id, { desc: `${method} ${url}`, startedAt, @@ -308,13 +309,20 @@ function setupFetchDebugging(hooks: NestedHooks) { } return await boundFetch(input, init); } catch (error) { + rejected = true; let reason = formatErrorForLog(error); console.error(`[test-fetch] ${method} ${url} failed: ${reason}`); rememberFailedFetch(epoch, method, url, reason); throw error; } finally { inFlightFetches.delete(id); - rememberSlowFetch(epoch, method, url, Date.now() - startedAt); + // Only requests that answered: one that rejected is already named in + // `recentFailedFetches`, and counting it here as well would both + // double-report it and inflate a total the dump presents as the cost + // of work that succeeded. + if (!rejected) { + rememberSlowFetch(epoch, method, url, Date.now() - startedAt); + } } }; globalThis.fetch = wrappedFetch; diff --git a/packages/realm-server/tests/definition-lookup-test.ts b/packages/realm-server/tests/definition-lookup-test.ts index 5fa760c8285..f263b920f69 100644 --- a/packages/realm-server/tests/definition-lookup-test.ts +++ b/packages/realm-server/tests/definition-lookup-test.ts @@ -2815,8 +2815,23 @@ module(basename(import.meta.filename), function () { }; let virtualNetwork: VirtualNetwork; + let sharedLookup: CachingDefinitionLookup; let lookup: DefinitionLookup; + // A realm like the one above but owned by someone else, so a lookup + // scoped to it probes as a different user. + function realmOwnedBy(url: string, userId: string) { + return { + url, + async getRealmOwnerUserId() { + return userId; + }, + async visibility(): Promise<'private'> { + return 'private'; + }, + }; + } + hooks.before(async function () { prepareTestDB(); adapter = await createTestPgAdapter(); @@ -2828,12 +2843,14 @@ module(basename(import.meta.filename), function () { hooks.beforeEach(function () { virtualNetwork = createVirtualNetwork(); - lookup = new CachingDefinitionLookup( + // One instance per test, so the probe maps a test observes are its own. + sharedLookup = new CachingDefinitionLookup( adapter, unusedPrerenderer, virtualNetwork, testCreatePrerenderAuth, - ).forRealm(localRealm); + ); + lookup = sharedLookup.forRealm(localRealm); }); test('a probe whose remote never answers is abandoned at its own deadline', async function (assert) { @@ -2965,6 +2982,198 @@ module(basename(import.meta.filename), function () { } }); + test('a deadline is not mistaken for a retryable failure on a retryable origin', async function (assert) { + // `localhost` is a retryable host in the test suite, so this probe goes + // through the retry loop rather than past it. The retry layer classifies + // by error name and treats only a caller's abort as final; a deadline + // that raised anything else would be retried, leaving attempts running + // in the background after the deadline had been reported. + let remoteModuleURL = 'http://localhost:9/person.gts'; + let attempts = 0; + + // Rejects with the abort reason the way a real fetch does, which is what + // puts the reason's name in front of the retry layer's classifier. A + // handler that ignored the signal would leave the request pending + // instead, and the retry loop — which only ever sees rejections — would + // never be reached at all, so the test would pass whatever the name. + let handler = async (request: Request) => { + if (request.method !== 'HEAD' || request.url !== remoteModuleURL) { + return null; + } + attempts++; + return await new Promise((_resolve, reject) => { + request.signal.addEventListener( + 'abort', + () => reject(request.signal.reason), + { once: true }, + ); + }); + }; + virtualNetwork.mount(handler); + + try { + let startedAt = Date.now(); + await assert.rejects( + lookup.lookupDefinition({ + module: rri(remoteModuleURL), + name: 'Person', + }), + 'a retryable origin that never answers still cannot place the module', + ); + let elapsed = Date.now() - startedAt; + + assert.strictEqual(attempts, 1, 'the probe was attempted once'); + assert.ok( + elapsed < 9_000, + `the deadline ended the probe rather than the retry ladder (took ${elapsed}ms)`, + ); + + // Longer than the first retry's backoff, so a retried attempt would + // have landed by now. + await new Promise((resolve) => setTimeout(resolve, 400)); + assert.strictEqual( + attempts, + 1, + 'no further attempt is issued after the deadline is reported', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + + test('an origin answering one probe clears a record another probe left', async function (assert) { + // The record is written by a probe that fails at the transport, and read + // before any probe is made — so the only way a probe reaches an origin + // that is already recorded is by having been in flight when the record + // was written. That overlap is what this covers: leaving the record + // standing would hide a realm that is demonstrably answering. + let remoteRealmURL = 'http://recovering-remote-realm/'; + let attempted: string[] = []; + let releaseFailing: (() => void) | undefined; + let releaseAnswering: (() => void) | undefined; + let failingGate = new Promise((resolve) => { + releaseFailing = resolve; + }); + let answeringGate = new Promise((resolve) => { + releaseAnswering = resolve; + }); + + let handler = async (request: Request) => { + if ( + request.method !== 'HEAD' || + !request.url.startsWith(remoteRealmURL) + ) { + return null; + } + attempted.push(request.url); + if (request.url.endsWith('/fails.gts')) { + await failingGate; + throw new TypeError('fetch failed'); + } + await answeringGate; + return new Response(null, { status: 404 }); + }; + virtualNetwork.mount(handler); + + try { + let failing = assert.rejects( + lookup.lookupDefinition({ + module: rri(`${remoteRealmURL}fails.gts`), + name: 'Person', + }), + 'the entry whose probe fails at the transport cannot be placed', + ); + let answering = assert.rejects( + lookup.lookupDefinition({ + module: rri(`${remoteRealmURL}answers.gts`), + name: 'Person', + }), + 'the entry whose probe is answered 404 has no module to place', + ); + + // Both probes are past the record check before either settles; the + // failing one then records the origin, and the answered one has to + // clear it. + await new Promise((resolve) => setTimeout(resolve, 50)); + releaseFailing!(); + await failing; + releaseAnswering!(); + await answering; + + attempted.length = 0; + await assert.rejects( + lookup.lookupDefinition({ + module: rri(`${remoteRealmURL}later.gts`), + name: 'Person', + }), + 'a later entry under the same origin still cannot be placed', + ); + assert.deepEqual( + attempted, + [`${remoteRealmURL}later.gts`], + 'the origin is probed again rather than read as unreachable', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + + test('two owners probing one module do not share a visibility result', async function (assert) { + // The probe's answer decides `cacheScope` and `cacheUserId`, so sharing + // one across callers would hand one user's view of a private realm to + // another. Each caller must get its own request, carrying its own + // assumed user. + let remoteRealmURL = 'http://per-user-remote-realm/'; + let remoteModuleURL = `${remoteRealmURL}person.gts`; + let firstOwner = '@owner-one:localhost'; + let secondOwner = '@owner-two:localhost'; + let assumedUsers: string[] = []; + let release: (() => void) | undefined; + let gate = new Promise((resolve) => { + release = resolve; + }); + + let handler = async (request: Request) => { + if (request.method !== 'HEAD' || request.url !== remoteModuleURL) { + return null; + } + assumedUsers.push(request.headers.get('X-Boxel-Assume-User') ?? ''); + await gate; + return new Response(null, { status: 404 }); + }; + virtualNetwork.mount(handler); + + try { + let asFirst = sharedLookup.forRealm( + realmOwnedBy('http://127.0.0.1:4454/', firstOwner), + ); + let asSecond = sharedLookup.forRealm( + realmOwnedBy('http://127.0.0.1:4455/', secondOwner), + ); + + let lookups = [asFirst, asSecond].map((scoped) => + assert.rejects( + scoped.lookupDefinition({ + module: rri(remoteModuleURL), + name: 'Person', + }), + 'neither caller finds a module to place', + ), + ); + await new Promise((resolve) => setTimeout(resolve, 50)); + release!(); + await Promise.all(lookups); + + assert.deepEqual( + assumedUsers.slice().sort(), + [firstOwner, secondOwner].slice().sort(), + 'each owner gets its own probe, carrying its own assumed user', + ); + } finally { + virtualNetwork.unmount(handler); + } + }); + test('a probe answered with a status is repeated rather than recorded', async function (assert) { let remoteModuleURL = 'http://answering-remote-realm/person.gts'; let probes = 0; diff --git a/packages/runtime-common/definition-lookup.ts b/packages/runtime-common/definition-lookup.ts index a823248ba05..ae5ef24776b 100644 --- a/packages/runtime-common/definition-lookup.ts +++ b/packages/runtime-common/definition-lookup.ts @@ -127,6 +127,10 @@ const POPULATE_RACE_MAX_ATTEMPTS = 3; // anything having to be restarted or cleared. const REALM_PROBE_TIMEOUT_MS = 5_000; const REALM_PROBE_UNREACHABLE_TTL_MS = ERROR_CACHE_TTL_MS; +// How many unreachable origins are remembered at once. Sized well above the +// number of realms any one deployment federates with, so the cap is a guard +// against caller-supplied origins accumulating rather than a working limit. +const REALM_PROBE_UNREACHABLE_MAX = 100; const modulesTableCoerceTypes: TypeCoercion = Object.freeze({ definitions: 'JSON', deps: 'JSON', @@ -417,43 +421,57 @@ function originOf(moduleURL: string): string | undefined { } } -// Settles with `work` if it finishes inside `ms`, and rejects otherwise. The -// abandoned promise's rejection is absorbed so it cannot surface as an -// unhandled rejection after the deadline has already been reported. -async function withDeadline( - work: Promise, - ms: number, - description: string, -): Promise { - let timer: ReturnType | undefined; - try { - return await Promise.race([ - work, - new Promise((_resolve, reject) => { - timer = setTimeout( - () => reject(new Error(`${description} exceeded ${ms}ms`)), - ms, - ); - }), - ]); - } finally { - if (timer !== undefined) { +// One timer serving both halves of a probe's bound. +// +// `signal` goes on the request, and the reason it aborts with is named +// `AbortError` rather than the `TimeoutError` an `AbortSignal.timeout` would +// raise. That name is load-bearing: the virtual network's retry wrapper +// treats an `AbortError` as caller cancellation and rethrows it, while any +// other rejection enters its retry loop — so a `TimeoutError` here would +// leave attempts running in the background after the deadline had already +// been reported, which is the opposite of a bound. +// +// `expired` settles the caller's own race, so the bound holds even where the +// signal is dropped between here and the socket. +function probeDeadline(ms: number, description: string) { + let controller = new AbortController(); + let rejectExpired: (reason: unknown) => void; + let expired = new Promise((_resolve, reject) => { + rejectExpired = reject; + }); + // Never surfaces as an unhandled rejection: `settle` attaches a sink, and + // when the request wins the race the timer is cleared before it can fire. + expired.catch(() => {}); + let timer = setTimeout(() => { + let reason = new DOMException( + `${description} exceeded ${ms}ms`, + 'AbortError', + ); + controller.abort(reason); + rejectExpired(reason); + }, ms); + return { + signal: controller.signal, + expired, + settle() { clearTimeout(timer); - } - work.catch(() => {}); - } + }, + }; } // Identifies the caller a probe is answered for, so probes are shared only -// among callers whose visibility answer must agree. +// among callers whose visibility answer must agree. `forEach` rather than +// `entries()`: `Headers` is iterable only under a `dom.iterable` lib, and +// this module is type-checked by packages that do not enable one. function probeAuthKey(headers?: HeadersInit): string { if (!headers) { return ''; } - return [...new Headers(headers).entries()] - .map(([name, value]) => `${name}:${value}`) - .sort() - .join(','); + let parts: string[] = []; + new Headers(headers).forEach((value, name) => { + parts.push(`${name}:${value}`); + }); + return parts.sort().join(','); } export class CachingDefinitionLookup implements DefinitionLookup { @@ -1464,24 +1482,37 @@ export class CachingDefinitionLookup implements DefinitionLookup { headers?: HeadersInit, ): Promise { let startedAt = Date.now(); + // The deadline is enforced on the caller, not only on the request. The + // signal is passed as well so the socket is torn down wherever the fetch + // stack honors it, but a probe must be bounded even where it does not: + // the request travels through the virtual network's remapping, retry and + // dispatcher layers, each of which rebuilds it, and a platform connect + // timeout an order of magnitude longer than this deadline sits underneath + // them all. Racing here makes the bound hold regardless of which layer + // the signal survives. + let deadline = probeDeadline( + REALM_PROBE_TIMEOUT_MS, + `remote realm visibility probe for ${moduleURL}`, + ); + let request = this.#fetch(moduleURL, { + method: 'HEAD', + headers, + signal: deadline.signal, + }); + // The abandoned request settles after the race is already lost, so its + // rejection needs a sink of its own. + request.catch(() => {}); try { - // The deadline is enforced on the caller, not only on the request. The - // signal is passed as well so the socket is torn down wherever the - // fetch stack honors it, but a probe must be bounded even where it does - // not: the request travels through the virtual network's remapping, - // retry and dispatcher layers, each of which rebuilds it, and a - // platform connect timeout an order of magnitude longer than this - // deadline sits underneath them all. Racing here makes the bound hold - // regardless of which layer the signal survives. - let response = await withDeadline( - this.#fetch(moduleURL, { - method: 'HEAD', - headers, - signal: AbortSignal.timeout(REALM_PROBE_TIMEOUT_MS), - }), - REALM_PROBE_TIMEOUT_MS, - `remote realm visibility probe for ${moduleURL}`, - ); + let response = await Promise.race([request, deadline.expired]); + // Any HTTP answer proves the origin is reachable, whatever its status. + // Clearing here matters when probes for the same origin overlap: one + // may fail at the transport and record the origin while another is + // being answered, and leaving that record standing would hide a + // demonstrably reachable realm — reporting its types as nonexistent — + // until the window lapsed. + if (origin) { + this.#unreachableOrigins.delete(origin); + } if (!response.ok) { log.debug( `Remote realm visibility probe for ${moduleURL} answered ${response.status} in ${Date.now() - startedAt}ms`, @@ -1502,10 +1533,7 @@ export class CachingDefinitionLookup implements DefinitionLookup { }; } catch (err) { if (origin) { - this.#unreachableOrigins.set( - origin, - Date.now() + REALM_PROBE_UNREACHABLE_TTL_MS, - ); + this.recordUnreachableOrigin(origin); } log.warn( `Failed to probe remote realm visibility for ${moduleURL} after ${Date.now() - startedAt}ms${ @@ -1516,6 +1544,36 @@ export class CachingDefinitionLookup implements DefinitionLookup { err, ); return null; + } finally { + deadline.settle(); + } + } + + // A filter carries arbitrary module URLs, so the origins reaching this are + // caller-supplied and unbounded in variety. Expiry alone would not contain + // the map — a record is only read, and so only retired, when that same + // origin is probed again, which for a one-off origin never happens — so + // sweep what has lapsed on the way in and cap what remains. Evicting the + // oldest insertion is the right sacrifice: it costs one probe the next time + // that origin is named, and the origins worth remembering are the ones + // being named repeatedly. + private recordUnreachableOrigin(origin: string): void { + let now = Date.now(); + for (let [recorded, expiresAt] of this.#unreachableOrigins) { + if (expiresAt <= now) { + this.#unreachableOrigins.delete(recorded); + } + } + // Re-inserting moves the origin to the end of the Map's insertion order, + // which is what makes the eviction below oldest-first. + this.#unreachableOrigins.delete(origin); + this.#unreachableOrigins.set(origin, now + REALM_PROBE_UNREACHABLE_TTL_MS); + while (this.#unreachableOrigins.size > REALM_PROBE_UNREACHABLE_MAX) { + let oldest = this.#unreachableOrigins.keys().next(); + if (oldest.done) { + break; + } + this.#unreachableOrigins.delete(oldest.value); } }