docs(proxy): stop claiming a 503 is unreachable after the queue caps went - #3425
Conversation
…went #3420 removed the two cold-load queue caps and left a comment saying a saturated proxy shows up as latency and "finally an honest 504, never a phantom 503". That overstates it: PermitSemaphore still carries its own DEFAULT_PERMIT_SEMAPHORE_MAX_QUEUE_SIZE backstop, and exhausting it raises ReleaseAssetOverloadedError and returns 503. Says so, and says why the distinction still holds: that threshold is two orders of magnitude above one page's module graph, so reaching it means a real flood rather than a project being shed for the size of the document this proxy just served it. Comment only; no behaviour change. Raised by Copilot on #3420, which merged before the correction landed.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe asset handler documentation distinguishes 504 responses for queued requests from 503 responses when the semaphore reaches its maximum queue threshold. ChangesConcurrency documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 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
Updates the proxy asset handler’s inline documentation to accurately describe when a 503 can still occur after the cold-load queue cap removals, clarifying the remaining PermitSemaphore backstop behavior.
Changes:
- Rewords the comment to stop claiming a 503 is impossible and to explain the remaining semaphore queue backstop.
- Keeps the original point intact: typical saturation should manifest as latency and eventually 504, not spurious 503s.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses the Copilot review comment on #3425: drop the stray "it" at the end of the 503 paragraph, and format DEFAULT_PERMIT_SEMAPHORE_MAX_QUEUE_SIZE as code to match the neighbouring `timeoutMs` / `MAX_UPSTREAM_TIMEOUT_MS` references.
Description
Follow-up to a Copilot review comment on #3420 that merged before the correction landed, so the inaccurate comment is on
mainnow.#3420 removed the two cold-load queue caps and left this in
asset-handler.ts:That overstates it.
PermitSemaphorestill carries its ownDEFAULT_PERMIT_SEMAPHORE_MAX_QUEUE_SIZEbackstop, and exhausting it raisesReleaseAssetOverloadedErrorand returns 503. TheserviceUnavailable()path was deliberately kept for exactly that case.The comment now says so, and says why the distinction the PR was making still holds: that threshold is two orders of magnitude above one page's module graph, so reaching it means a genuine flood rather than a project being shed for the size of the document this proxy just served it.
Type of Change
Comment only, no behaviour change.
asset-handler.test.ts29 steps pass;fmt:check,lintanddocs:api-reference:checkall exit 0.Summary by CodeRabbit