Bound and remember the remote-realm visibility probe - #6082
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJp8v1CnpxAUrbtqcjKQYv
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe3a06e57f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate findings remain involving timeout retries, cache growth, diagnostic accuracy, and cross-user coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR bounds and coalesces remote-realm visibility probes, caches transport failures, and improves timeout diagnostics.
Changes:
- Adds probe deadlines, per-user coalescing, and origin failure caching.
- Adds hermetic remote-probe regression tests.
- Records slow fetches in host timeout diagnostics.
File summaries
| File | Summary |
|---|---|
packages/runtime-common/definition-lookup.ts |
Implements probe deadlines, coalescing, and failure caching; review findings concern retry handling, cache growth, and cross-user coverage. |
packages/realm-server/tests/definition-lookup-test.ts |
Adds remote-probe timeout, coalescing, caching, and status-response tests. |
packages/host/tests/helpers/setup.ts |
Adds slow-fetch timeout diagnostics; failed requests may be mislabeled as completed fetches. |
Review details
Suppressed comments (2)
packages/runtime-common/definition-lookup.ts:1480
- [Claude Code 🤖]
AbortSignal.timeout()rejects fetches with aTimeoutError, butVirtualNetwork.withRetriestreats onlyAbortErroras caller cancellation (packages/runtime-common/virtual-network.ts:1043-1049). For retryable probe URLs, this timeout therefore enters the retry loop afterwithDeadlinehas returned, issuing background attempts and defeating the transport bound. Make the deadline use anAbortError-compatible controller or teach the retry layer to recognize this caller signal, and add a retryable-URL regression test.
signal: AbortSignal.timeout(REALM_PROBE_TIMEOUT_MS),
packages/runtime-common/definition-lookup.ts:1445
- [Claude Code 🤖] The probe result determines
cacheScopeandcacheUserId, so the user dimension in this key is security-sensitive. The added tests cover coalescing for one caller only; add a concurrent lookup case with two different owners and assert separate HEAD requests and headers, otherwise a regression here could share one user's private/public visibility result with another user.
// 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)}`;
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TJp8v1CnpxAUrbtqcjKQYv
|
[Claude Code 🤖] The Copilot review listed two further findings as suppressed comments, so they have no threads to answer on. Both are real and both are fixed in
The regression test you asked for is The user dimension of the coalescing key is security-sensitive and only covered for one caller. Fair. Added Verification. 44/44 in Generated by Claude Code |
Flaky test fix
Fixes the intermittent failure of
Integration | operator-mode | card chooser: cancel button closes the field picker, which fails asTest took longer than 60000ms; test timed out.The test is not at fault and its code is untouched here — it times out because
a search it makes waits on a realm-server request that blocks 10 seconds at a
time. The fix is server-side, in the definition lookup; nothing is skipped,
retried, or given a longer timeout.
Not a test-only concern, either: any search filtered on a type from a realm
that has gone offline stalls the same way in production, once per clause, on
every request.
Why it flakes
Placing a module that lives in a realm the serving realm-server does not host
means asking that realm over the network —
probeRemoteRealm, reached frombuildLookupContextwhenever a lookup's module falls outside every localrealm. That probe's cost was set entirely by the far end: an origin that
accepts no connections spends the platform's full connect timeout (10s under
Node's fetch) before rejecting, none of it is per-module, and nothing was
remembered. A search whose filter names types from an unreachable realm paid
that once per type, and the next search paid the whole bill again.
The host test suite produces exactly that shape. Its virtual realm at
http://test-realm/is answered in-page by the harness, but a card chooser'ssearch fans out to every available realm — including the live base realm,
which then tries to reach
test-realmfor real and cannot. One CI shard'srealm-server log carries 132 such probes, 55 of them paying the full 10s,
arriving in bursts of 10–15 for a single module. The field-picker test opens
the picker, types into it and reopens it, so it absorbs several of those
bursts and runs out of QUnit's 60s budget.
Whether a probe costs 15ms or 10s comes down to what the runner's resolver
does with
test-realm— fail fast, or hand back an address that then neverconnects. That is the coin flip behind the flake.
The fix
Three bounds now hold the cost flat:
keyed by the requesting user since visibility is answered per caller.
for the same window an errored definition is cached for
(
ERROR_CACHE_TTL_MS). Keyed by origin because a connection that neveropens says nothing about the path, and time-boxed so a realm that comes
back is seen without anything being restarted or cleared. An HTTP answer —
404 included — is never remembered, since a server that answers at all is
already cheap to ask.
Why the deadline is enforced on the caller
The probe request carries an
AbortSignalas well, so the socket is torndown wherever the fetch stack honors it, but the bound cannot rest on that
signal alone. The request is rebuilt on its way out by the virtual network's
URL remapping, by the retry wrapper's per-attempt signal merge, and by the
undici dispatcher wrapper, and a platform connect timeout an order of
magnitude longer than the deadline sits underneath all of them. Measured
against a realm-server serving an origin that drops SYNs, a probe carrying a
5s signal still ran to undici's 10s connect timeout. Racing the deadline on
the caller makes the bound hold regardless of which layer the signal
survives.
Diagnostics, so a future timeout is readable
Host tests now record fetches that answered slowly and print them in the
timeout diagnostic. The in-flight snapshot names only what was still
outstanding when the dump ran and the failed-fetch buffer only what rejected,
so a run of requests that each answered in seconds — the signature of this
failure — left no trace in either. This is deliberately kept even though the
fix above looks sufficient: if the flake recurs for a different reason, the
next failure names the endpoint and the cost in the shard log.
Not in scope
No caller of
lookupDefinitionchanges, and the probe's contract isunchanged: an unreachable or non-answering realm still resolves to "cannot
place this module", which surfaces as
FilterRefersToNonexistentTypeErrorexactly as before. Only the latency and the number of network round trips
change.
Test plan
packages/realm-server/tests/definition-lookup-test.ts— a new hermeticremote realm visibility probemodule, four tests, one per property:abandoned on the caller's deadline. The fake remote deliberately ignores
the signal, because a handler that resolved on abort would pass on the
strength of the signal alone and say nothing about the caller's bound —
with the caller-side deadline removed and the signal still attached, this
test fails with
Test took longer than 60000ms; test timed out., the samesignature as the flake itself
record stands, across different modules under it
All 41 tests in
definition-lookup-test.tspass.Verified end to end against a local stack with
test-realmresolving to anaddress that drops SYNs, reproducing the CI runner's
ConnectTimeoutError … timeout: 10000ms. Running the flaky test against thatstack:
🤖 Generated with Claude Code
https://claude.ai/code/session_01TJp8v1CnpxAUrbtqcjKQYv