fix(proxy): stop shedding release-asset cold loads onto the client - #3420
Conversation
A page load requests every asset in its module graph at once. On a cold pod the asset handler admitted only 4 concurrent loads plus 16 queued, so the 21st distinct hash was rejected outright with a 503. Browser `import()` never retries, so one shed asset killed hydration for the whole page. Two of the three bounds were guarding nothing. The queue cap turned "busy" into "this module does not exist", and the waiter cap rejected followers of a load that had already succeeded -- callers whose bytes were sitting in memory, identical for all of them. Both are removed. PermitSemaphore keeps its own 10,000-entry backstop, and callers were already bounded by their own deadline and the producer ceiling, so a saturated proxy now surfaces as latency and finally an honest 504. MAX_CONCURRENT_COLD_LOADS stays as-is: it bounds the one real resource, since each in-flight load buffers a whole asset in memory. Raise MAX_CACHED_ASSETS 100 -> 2000. Measured against a real page, assets average ~2.9KB, so 100 entries could only ever hold ~285KB of the 32MB byte budget -- the entry cap did all the evicting and the byte budget was unreachable. At 100 entries a single page nearly filled the per-pod cache, which made cold-load storms steady state rather than a deploy-time event. Both updated tests had pinned the defect as intended behaviour: 50 concurrent distinct assets asserted 30 x 503, and 80 same-hash followers asserted 16 x 503.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesAsset loading
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Request
participant AssetHandler
participant Semaphore
participant Upstream
Request->>AssetHandler: Request cold asset
AssetHandler->>Semaphore: Acquire load slot or queue
Semaphore->>Upstream: Start upstream load
Upstream-->>AssetHandler: Return asset
AssetHandler-->>Request: Return shared response or request failure
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the release-asset proxy handler to avoid failing page hydration during cold-cache fan-out by removing hard 503-shedding limits and relying on queueing plus per-caller/per-producer timeouts instead.
Changes:
- Increased the in-memory LRU entry cap for cached immutable assets (while keeping the existing 32MB byte budget).
- Removed the explicit cold-load queue and waiter caps so requests queue behind
MAX_CONCURRENT_COLD_LOADSrather than being rejected with 503. - Updated proxy tests to assert queueing behavior (no 503 shedding) and unlimited same-hash followers (still single upstream fetch).
Verification
- Not run as part of this automated review; PR description reports proxy suite + full suite passing, plus
deno check/lint/fmt --checkclean.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/proxy/asset-handler.ts | Removes cold-load shedding limits, keeps concurrency bound, and raises the cached-asset entry cap to reduce cold-load storms. |
| src/proxy/asset-handler.test.ts | Updates concurrency/follower tests to reflect queueing semantics and the removal of 503 shedding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Excess demand queues rather than failing. A page's module graph is a fan-out | ||
| * this proxy itself produced by serving the HTML, so shedding it would reject | ||
| * the predictable consequence of our own response — and browser `import()` | ||
| * never retries, which turns one shed asset into a dead page. Callers are | ||
| * already bounded by their own deadline (`timeoutMs`) and the producer ceiling | ||
| * (`MAX_UPSTREAM_TIMEOUT_MS`); a saturated proxy therefore shows up as latency | ||
| * and finally an honest 504, never a phantom 503. |
…ration Project pages are served with `script-src 'self' 'nonce-...' https://esm.sh`. That has no 'unsafe-eval', so the `new Function("specifier", ...)` in dynamicImport throws EvalError in the browser and hydration dies before first paint, leaving the page on its skeleton loaders. The module is reachable from three client entry points, all converging on platform/compat/process/command.ts: chat/index.ts -> chat/stream-watchdog.ts -> agent/streaming/lifecycle/ watchdog-compat-adapter.ts -> platform/compat/process.ts mdx/index.ts -> react/components/MDXProvider.tsx -> types/index.ts -> types/server.ts -> config/loader.ts -> ... workflow/react/ -> use-workflow.ts -> workflow/types.ts -> agent/types.ts -> tool/index.ts -> tool/context7.ts -> ... The `new Function` was there to keep the import opaque to bundlers and `deno compile`. It was not needed: the specifier is a runtime parameter, so neither can resolve it to a concrete module. Verified with `deno compile` on a bare parameterized `import()` — it bundles only the local files, does not trace into the specifier, and the compiled binary still resolves it at runtime. src/platform/adapters/fs/veryfront/default-invalidation-callbacks.ts already uses the same bare form. This is the second half of the blank-page failure. Even with the release-asset 503 shedding fixed, loads that served every asset cleanly still failed to hydrate on this EvalError.
|
Pushed Live 503 reproductionReproduced on One page load requests 143 distinct asset hashes against
The 143-vs-100 measurement supports the Worth noting the failure needs the browser's concurrent HTTP/2 burst. Every "503" asset returns 200 when fetched on its own — 50/50 sequential and 30/30 parallel curls all succeeded. Anyone checking with curl will conclude the assets are fine. Second bug: CSP blocks the hydration runtime's own
|
…lows hydration" This reverts commit 0c24ce8.
|
Correction to my previous comment — I reverted the CSP commit ( My fix was wrong and it broke I checked that removing Every client import has to be rewritable to a content-addressed The two findings in my previous comment still stand. The 503 reproduction is unaffected (143 distinct assets vs The CSP bug is also still real and still blanks the page — loads 4 and 5 shed zero assets and died on Cutting those (lazy import at the call site, or splitting the type-only imports) removes the eval from the client graph without touching the helper's build behaviour. That is a bigger change than belongs on this PR, so I am leaving it off. Happy to open a separate issue or PR for it — say the word. Nothing here blocks this PR; it is back to what you had. |
Description
A page load requests every asset in its module graph at once. On a cold pod the release-asset handler admitted only 4 concurrent loads plus 16 queued, so the 21st distinct hash was rejected outright with a
503. Browserimport()never retries, so one shed asset killed hydration for the entire page.Observed in production on
j-agent.production.veryfront.com: one load 503'd 31 assets, and the next reload 503'd 11 different, deeper hashes —dca28b9f…was a 503 victim in the first log and a successful referrer in the second. That is a per-pod cache warming one module-graph level per reload and hitting the same wall on the next fan-out level.What changed
Two of the three bounds were guarding nothing:
MAX_QUEUED_COLD_LOADS(16) turned "busy" into "this module does not exist". A queue cap here is a hard page failure by construction, becauseimport()has no retry andRetry-After: 1is decoration.MAX_COLD_LOAD_WAITERS(64) rejected followers of a load that had already succeeded — callers whose bytes were sitting in memory, byte-identical for all of them. Nothing was being protected.Both are removed.
PermitSemaphorekeeps its own 10,000-entry backstop, and callers were already bounded by their own deadline (timeoutMs) and the producer ceiling (MAX_UPSTREAM_TIMEOUT_MS). A saturated proxy now surfaces as latency and finally an honest504, never a phantom503.MAX_CONCURRENT_COLD_LOADS = 4is unchanged. It bounds the one real resource — each in-flight load buffers a whole asset in memory, so peak bytes is this count ×RELEASE_ASSET_MAX_SIZE_BYTES. Retuning it is a performance question, not this bug.MAX_CACHED_ASSETS100 → 2000. Measured against a real page (65 assets, 185KB total): mean asset is ~2.9KB, so 100 entries could only ever hold ~285KB of the 32MB byte budget. The entry cap did 100% of the evicting and the byte budget was ~115× out of reach. At 100 entries a single page nearly filled the per-pod cache — shared across every project on that pod — which made cold-load storms steady state rather than a deploy-time event.Related Issue(s)
Type of Change
Checklist
Both updated tests had pinned the defect as intended behaviour:
19 × 200,30 × 50349 × 200,0 × 503(1 × 499 = deliberately disconnected caller)64 × 200,16 × 50380 × 200,0 × 503, stillcalls === 1maxActiveis still asserted at 4, confirming queueing did not widen the bound that guards real memory. The existing byte-weight eviction test still passes, so the 32MB budget still binds.Verification: proxy suite 49 passed / 482 steps / 0 failed; full pre-push suite 3798 passed / 27834 steps / 0 failed;
deno check,deno lint,deno fmt --checkclean.Notes for review
MAX_CONCURRENT_COLD_LOADS = 4is still a guess at what/release-assets/{hash}will absorb. If cold loads feel slow after this, that is the number to revisit — and it is now the only one left to tune.Summary by CodeRabbit