fix(core): retry busy write admission - #235
Conversation
|
Warning Review limit reached
Next review available in: 25 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughDirect Relayfile HTTP writes now apply workspace-busy-specific ChangesDirect write admission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant writeJsonFile
participant RelayFileSDK
participant RelayfileAPI
Caller->>writeJsonFile: request direct write
writeJsonFile->>RelayFileSDK: submit write admission
RelayFileSDK->>RelayfileAPI: send HTTP request
RelayfileAPI-->>RelayFileSDK: workspace_busy 429 with Retry-After
RelayFileSDK->>RelayfileAPI: retry within configured deadline
RelayfileAPI-->>writeJsonFile: queued response or admission failure
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the direct HTTP write admission layer to honor Retry-After headers up to 30 seconds for workspace_busy / write_admission_limit responses, while preserving the existing two-second cap for other retryable responses and aborting cleanly at the writeback deadline. It also introduces a new RelayfileWritebackAdmissionTimeoutError and adds extensive test coverage. The review feedback highlights three key improvement opportunities: refining retryAfterDelayMs to prevent date strings starting with digits from being incorrectly parsed as relative seconds, optimizing responseWithRetryAfter by passing response.body directly to avoid buffering the response in memory, and ensuring isDirectWriteAdmissionRequest handles relative URLs correctly by providing a dummy base URL to the URL constructor.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function retryAfterDelayMs(value: string): number | undefined { | ||
| const seconds = Number.parseInt(value, 10); | ||
| if (Number.isFinite(seconds) && seconds >= 0) return seconds * 1_000; | ||
| const timestamp = Date.parse(value); | ||
| return Number.isNaN(timestamp) ? undefined : Math.max(0, timestamp - Date.now()); | ||
| } |
There was a problem hiding this comment.
The current implementation of retryAfterDelayMs uses Number.parseInt(value, 10) to check if the Retry-After header is a relative number of seconds. However, parseInt parses characters from left to right until it encounters a non-digit.
If the Retry-After header contains an ISO 8601 date string (e.g., "2026-06-19T12:01:04Z") or a date string that starts with a digit (e.g., "29 Oct 2026 07:28:00 GMT"), parseInt will successfully parse the leading digits (e.g., 2026 or 29) and treat them as a relative delay in seconds instead of parsing the string as a timestamp.
To prevent this, we should use a regular expression to ensure the entire string consists only of digits before parsing it as relative seconds.
function retryAfterDelayMs(value: string): number | undefined {
const trimmed = value.trim();
if (/^\d+$/.test(trimmed)) {
return Number.parseInt(trimmed, 10) * 1_000;
}
const timestamp = Date.parse(trimmed);
return Number.isNaN(timestamp) ? undefined : Math.max(0, timestamp - Date.now());
}There was a problem hiding this comment.
Fixed in 3c09d3d. Relative-seconds parsing now requires the entire trimmed header to be digits. Exact workspace-admission responses are normalized to a numeric delay before the SDK sees them, which also avoids the SDK misreading digit-leading date strings. Added a red/green regression using a digit-leading 1970 date (old behavior slept 1s per retry; fixed behavior retries immediately).
| async function responseWithRetryAfter( | ||
| response: Response, | ||
| retryAfter: string | ||
| ): Promise<Response> { | ||
| const headers = new Headers(response.headers); | ||
| headers.set("Retry-After", retryAfter); | ||
| return new Response(await response.arrayBuffer(), { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers | ||
| }); | ||
| } |
There was a problem hiding this comment.
Using await response.arrayBuffer() buffers the entire response body in memory. For large 429 or 5xx responses (such as custom HTML error pages from proxies or CDN gateways), this can lead to unnecessary memory overhead and latency.
We can optimize this by passing response.body (which is a ReadableStream) directly to the Response constructor. This avoids buffering the body in memory and allows the function to be synchronous.
| async function responseWithRetryAfter( | |
| response: Response, | |
| retryAfter: string | |
| ): Promise<Response> { | |
| const headers = new Headers(response.headers); | |
| headers.set("Retry-After", retryAfter); | |
| return new Response(await response.arrayBuffer(), { | |
| status: response.status, | |
| statusText: response.statusText, | |
| headers | |
| }); | |
| } | |
| function responseWithRetryAfter( | |
| response: Response, | |
| retryAfter: string | |
| ): Response { | |
| const headers = new Headers(response.headers); | |
| headers.set("Retry-After", retryAfter); | |
| return new Response(response.body, { | |
| status: response.status, | |
| statusText: response.statusText, | |
| headers | |
| }); | |
| } |
There was a problem hiding this comment.
Fixed in 3c09d3d. The response reconstruction helper is now synchronous and forwards the response stream rather than buffering an arrayBuffer.
| function isDirectWriteAdmissionRequest(input: RequestInfo | URL, init?: RequestInit): boolean { | ||
| const method = (init?.method ?? (input instanceof Request ? input.method : "GET")).toUpperCase(); | ||
| const url = input instanceof Request ? input.url : String(input); | ||
| try { | ||
| return method === "PUT" && new URL(url).pathname.endsWith("/fs/file"); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
If input is a relative URL string (e.g., "/v1/workspaces/rw_7ccfea89/fs/file"), new URL(url) will throw a TypeError: Invalid URL because it lacks a base URL. This will cause the catch block to execute and return false, failing to recognize valid direct write admission requests.
To make this robust against relative URLs, we should provide a dummy base URL to the URL constructor. If the input URL is absolute, the base URL is ignored; if it is relative, it resolves correctly without throwing.
| function isDirectWriteAdmissionRequest(input: RequestInfo | URL, init?: RequestInit): boolean { | |
| const method = (init?.method ?? (input instanceof Request ? input.method : "GET")).toUpperCase(); | |
| const url = input instanceof Request ? input.url : String(input); | |
| try { | |
| return method === "PUT" && new URL(url).pathname.endsWith("/fs/file"); | |
| } catch { | |
| return false; | |
| } | |
| } | |
| function isDirectWriteAdmissionRequest(input: RequestInfo | URL, init?: RequestInit): boolean { | |
| const method = (init?.method ?? (input instanceof Request ? input.method : "GET")).toUpperCase(); | |
| const url = input instanceof Request ? input.url : String(input); | |
| try { | |
| return method === "PUT" && new URL(url, "http://dummy.com").pathname.endsWith("/fs/file"); | |
| } catch { | |
| return false; | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 3c09d3d. Relative request URLs now resolve against a non-routable parsing-only base; absolute URLs remain unchanged.
![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: 22d47b1d61
ℹ️ 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".
| const timeoutMs = client.writebackTimeoutMs ?? DEFAULT_WRITEBACK_TIMEOUT_MS; | ||
| const controller = timeoutMs > 0 ? new AbortController() : undefined; | ||
| const deadlineTimer = controller | ||
| ? setTimeout(() => controller.abort(), timeoutMs) | ||
| : undefined; |
There was a problem hiding this comment.
When direct callers leave writebackTimeoutMs unset, timeoutMs is the 3s receipt default, but this same signal is now passed into the SDK retry sleep. For a workspace_busy admission response with Retry-After: 5, the controller aborts at 3s and throws RelayfileWritebackAdmissionTimeoutError before the SDK can make the second attempt, so default clients still fail instead of honoring the advertised retry. Use a separate admission deadline/default that can cover the intended 30s admission cap, or do not apply the receipt timeout to admission backoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4cad79. Implicit defaults are now split: receipt polling remains 3 seconds, while direct admission is bounded at 90 seconds so the SDK can honor three advertised delays up to 30 seconds. An explicit writebackTimeoutMs still governs both and zero remains unbounded/no receipt wait. Red-first regressions prove an omitted timeout survives a 5-second delay to attempt 2 and that repeated 30-second delays abort at 90 seconds with no orphan fourth attempt.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/tests/vfs-client/vfs-client.test.ts (1)
385-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond assertion doesn't actually verify retry-timer cancellation.
After the rejection, the test waits only 40ms of real time (line 422) before re-checking
attempts === 1, but the advertisedRetry-Afterdelay is 30s (retryAfterSeconds: 30, line 394). Since 40ms is nowhere near 30,000ms, this assertion would pass identically whether or not the SDK actually canceled its scheduled retry timer — it only proves the retry hasn't fired yet, not that it was canceled. This is exactly the guarantee the test's own comment claims to verify ("the SDK retry timer must be canceled"), so it currently gives false confidence for a resilience-critical behavior (avoiding orphan/duplicate write attempts after the caller gives up).Consider reusing the
withImmediateTimeoutspattern already in this file to capture/cancel the scheduled retry timer deterministically (e.g., trackclearTimeoutcalls on the id returned for the retry'ssetTimeout), rather than relying on a short real-time wait against a 30s schedule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/vfs-client/vfs-client.test.ts` around lines 385 - 424, Strengthen the retry-cancellation test around writeJsonFile by reusing the file’s withImmediateTimeouts pattern to control the scheduled retry timer deterministically. Capture the timer returned for the advertised retry and assert that the client deadline invokes clearTimeout for that timer, replacing the ineffective 40ms real-time wait while preserving the single-attempt assertion.packages/core/src/vfs-client/index.ts (1)
538-584: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdmission wait and receipt wait are independently bounded by the same
writebackTimeoutMs, so combined worst-case latency can reach ~2x the configured timeout.The new
AbortControllerdeadline here (lines 539-542) bounds only the admission phase.waitForOperationReceipt(unchanged, lines 475-528) computes its own separateDate.now() + timeoutMsdeadline using the sameclient.writebackTimeoutMs, and doesn't receivecontroller.signal. A caller settingwritebackTimeoutMs: 5000expecting a 5s overall bound could see up to ~10s total (5s admission wait + 5s receipt wait) in the worst case. Consider sharing a single deadline (e.g., pass the already-computed absolute deadline orcontroller.signal/remaining budget intowaitForOperationReceipt) so the total bound matches caller expectations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/vfs-client/index.ts` around lines 538 - 584, Use one overall writeback deadline across both admission and receipt waiting: update the flow around the admission AbortController and waitForOperationReceipt so the receipt wait receives the same absolute deadline or remaining timeout budget instead of starting a fresh writebackTimeoutMs window. Preserve the existing timeout/error behavior while ensuring combined admission plus receipt latency cannot exceed the configured timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/vfs-client/index.ts`:
- Around line 241-317: Update directRetryFetch to also clamp retryable 429/5xx
responses when Retry-After is absent, preserving the workspace-busy admission
exception. Ensure responses without the header are rewritten with the SDK legacy
2-second retry limit so getOp polling and unrelated write failures retain the
intended cap.
---
Nitpick comments:
In `@packages/core/src/vfs-client/index.ts`:
- Around line 538-584: Use one overall writeback deadline across both admission
and receipt waiting: update the flow around the admission AbortController and
waitForOperationReceipt so the receipt wait receives the same absolute deadline
or remaining timeout budget instead of starting a fresh writebackTimeoutMs
window. Preserve the existing timeout/error behavior while ensuring combined
admission plus receipt latency cannot exceed the configured timeout.
In `@packages/core/tests/vfs-client/vfs-client.test.ts`:
- Around line 385-424: Strengthen the retry-cancellation test around
writeJsonFile by reusing the file’s withImmediateTimeouts pattern to control the
scheduled retry timer deterministically. Capture the timer returned for the
advertised retry and assert that the client deadline invokes clearTimeout for
that timer, replacing the ineffective 40ms real-time wait while preserving the
single-attempt assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b9fce26-761d-4689-8c01-17171c864134
📒 Files selected for processing (3)
CHANGELOG.mdpackages/core/src/vfs-client/index.tspackages/core/tests/vfs-client/vfs-client.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/vfs-client/index.ts">
<violation number="1" location="packages/core/src/vfs-client/index.ts:232">
P2: Three 30s `Retry-After` delays cannot reach the promised fourth request: the 90s deadline is armed before attempt one and fires when the third retry becomes due. Allow time for the final attempt (or explicitly clamp/rework the final retry) so a valid max-delay sequence is not aborted after only three attempts.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const DEFAULT_WRITEBACK_TIMEOUT_MS = 3_000; | ||
| const SDK_LEGACY_RETRY_MAX_DELAY_MS = 2_000; | ||
| const WORKSPACE_BUSY_RETRY_MAX_DELAY_MS = 30_000; | ||
| const DEFAULT_WRITEBACK_ADMISSION_TIMEOUT_MS = WORKSPACE_BUSY_RETRY_MAX_DELAY_MS * 3; |
There was a problem hiding this comment.
P2: Three 30s Retry-After delays cannot reach the promised fourth request: the 90s deadline is armed before attempt one and fires when the third retry becomes due. Allow time for the final attempt (or explicitly clamp/rework the final retry) so a valid max-delay sequence is not aborted after only three attempts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/vfs-client/index.ts, line 232:
<comment>Three 30s `Retry-After` delays cannot reach the promised fourth request: the 90s deadline is armed before attempt one and fires when the third retry becomes due. Allow time for the final attempt (or explicitly clamp/rework the final retry) so a valid max-delay sequence is not aborted after only three attempts.</comment>
<file context>
@@ -228,6 +229,7 @@ export interface WritebackResult {
const DEFAULT_WRITEBACK_TIMEOUT_MS = 3_000;
const SDK_LEGACY_RETRY_MAX_DELAY_MS = 2_000;
const WORKSPACE_BUSY_RETRY_MAX_DELAY_MS = 30_000;
+const DEFAULT_WRITEBACK_ADMISSION_TIMEOUT_MS = WORKSPACE_BUSY_RETRY_MAX_DELAY_MS * 3;
function isRecord(value: unknown): value is Record<string, unknown> {
</file context>
There was a problem hiding this comment.
Reviewed against 495b329. This boundary is intentional: the 90s admission deadline is installed before attempt 1 and must win at t+90 when all three advertised delays equal the 30s safety maximum. The red regression pins attempts=3 plus cancellation of the pending fourth attempt, so no retry survives the caller deadline. Four total attempts remain proven for advertised delays that fit inside the deadline (t+0/5/10/15). Allowing the t+90 request would exceed the client bound and conflict with the daily-ship 90s deadline. The PR timeline documents this pathological boundary explicitly.
There was a problem hiding this comment.
The parent comment was wrong here: the 90s admission deadline is intentional and documented, and it’s meant to win at t+90. The PR also proves the four-attempt path for delays that stay within the deadline, so the missing t+90 attempt is expected behavior.
|
Bot triage on final hardening: (1) CodeRabbit cancellation nitpick was valid and is fixed by deterministic retry-handle capture plus clearTimeout proof; the 40ms real-time wait is gone. (2) Independent admission and receipt bounds are intentional compatibility semantics, now explicit in the option docs and CHANGELOG; an explicit value bounds each phase, while omitted defaults are 90s admission / 3s receipt. (3) Bare retryables remain on the SDK 100/200/400ms schedule, pinned with jitter disabled, so the raised 30s ceiling cannot change their timing. (4) Cubic t+90 concern is the intentionally tested boundary: the client deadline wins after three maximal intervals and cancels the pending fourth attempt; four attempts are still proven when advertised delays fit inside the deadline. Current head 554f2f3 also states that boundary directly in the public option docs. |
1 participant
Summary
Retry-Afterup to 30 seconds only for pre-opPUT .../fs/fileresponses with HTTP 429,code=workspace_busy, andreason=write_admission_limitwritebackTimeoutMsand surface a retryableRelayfileWritebackAdmissionTimeoutErrorwithout leaving an orphan retry timerAttempt timeline
The SDK keeps its default
maxRetries=3, so there are exactly four total attempts. A five-second server delay produces attempts at t+0s, t+5s, t+10s, and t+15s.At the 30-second safety bound, the theoretical fourth attempt is t+90s. Daily-ship's 90-second client deadline is installed before the first request, so it governs that pathological boundary: it aborts the SDK wait at t+90s, returns the terminal admission-timeout error, and cancels the pending retry rather than allowing an orphaned attempt. No independent adapter retry loop is stacked on the SDK.
URL, request body, and idempotency payload remain byte-identical across attempts.
Scope audit
Tests
Validation
npm ci(real install)npx turbo build typecheck test: 147/147 tasks greengit diff --check: clean