fix: return 503 for service_unavailable build errors#1716
Conversation
Builder availability errors (not configured, call failed, error response, missing upload URL) are transient server-side failures, not client errors. Returning 503 allows the CLI retry logic to automatically retry these requests instead of treating them as terminal 400 errors.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Adjusts build request/upload error handling so transient “builder unavailable” conditions return HTTP 503 (instead of 400), enabling the CLI’s existing retry behavior for these cases.
Changes:
- Switched multiple
service_unavailableerrors in the build request flow fromsimpleError(...)(400) toquickError(503, ...). - Switched the TUS upload proxy “builder not configured” error to return 503.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| supabase/functions/_backend/public/build/request.ts | Return 503 for builder configuration/call failures during build job creation. |
| supabase/functions/_backend/public/build/upload.ts | Return 503 when builder config is missing for the TUS proxy upload path. |
Comments suppressed due to low confidence (3)
supabase/functions/_backend/public/build/request.ts:205
- The
catchblock will also catch theHTTPExceptionthrown byquickErrorin the non-OK response path (line 189). As a result, the more specific "(builder error)" exception is immediately replaced with the generic "(builder call failed)" error and logs an incorrect "fetch failed" entry. Consider rethrowingHTTPExceptioninstances (or otherwise bypassing the catch for already-formed errors) so only actual network/serialization failures hit this catch path.
throw quickError(503, 'service_unavailable', 'Build service unavailable (builder error)')
}
}
catch (error) {
cloudlogErr({
requestId: c.get('requestId'),
message: 'Builder API fetch failed',
builder_url: builderUrl,
error: (error as Error)?.message,
error_stack: (error as Error)?.stack,
error_name: (error as Error)?.name,
org_id,
app_id,
platform,
})
throw quickError(503, 'service_unavailable', 'Build service unavailable (builder call failed)')
}
supabase/functions/_backend/public/build/request.ts:133
- This change alters the HTTP status for
service_unavailablebuild-request failures to 503 (to enable retries), but there doesn't appear to be any test coverage asserting the new 503 behavior for the /build/request flow. Please add/extend tests to assert these service-unavailable scenarios return 503 (and that other validation errors remain 400).
if (!builderUrl || !builderApiKey) {
cloudlogErr({
requestId: c.get('requestId'),
message: 'Builder API not configured',
builder_url_configured: !!builderUrl,
builder_api_key_configured: !!builderApiKey,
})
throw quickError(503, 'service_unavailable', 'Build service unavailable (builder not configured)')
}
supabase/functions/_backend/public/build/upload.ts:37
- The
tusProxyendpoint now returns 503 when the builder is not configured. There doesn't appear to be a test asserting this specific status code for the /build/upload/:jobId TUS proxy path when BUILDER_URL/BUILDER_API_KEY are missing. Adding a test for this would prevent regressions and validate the CLI retry-trigger behavior.
if (!builderUrl || !builderApiKey) {
cloudlogErr({
requestId: c.get('requestId'),
message: 'Builder not configured for TUS proxy',
job_id: jobId,
})
throw quickError(503, 'service_unavailable', 'Builder service not configured')
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/public/build/request.ts (1)
189-204:⚠️ Potential issue | 🟡 MinorHTTPException thrown on line 189 will be re-caught and re-wrapped on line 204.
The
quickErroron line 189 throws anHTTPException, which the catch block on line 192 will intercept. The error is then re-thrown with the message"builder call failed"instead of the intended"builder error". This loses the distinction between "builder returned an error response" vs "network/fetch failure".This is a pre-existing structural issue, but since this PR focuses on error handling, consider restructuring:
🔧 Proposed fix: Re-throw HTTPException to preserve error context
catch (error) { + // Re-throw if already an HTTPException (from line 189) + if (error instanceof HTTPException) { + throw error + } cloudlogErr({ requestId: c.get('requestId'), message: 'Builder API fetch failed',You'll also need to import
HTTPExceptionfromhono/http-exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/build/request.ts` around lines 189 - 204, The catch block is currently intercepting the HTTPException thrown by quickError in the build response handling and re-wrapping it as a generic "builder call failed" error, losing original context; update the catch in the function handling the builder call to detect and re-throw instances of HTTPException (importing HTTPException from 'hono/http-exception') instead of logging and converting them, so only non-HTTP errors are logged via cloudlogErr (which uses c.get('requestId'), builderUrl, org_id, app_id, platform) and then wrapped with quickError('service_unavailable', 'Build service unavailable (builder call failed)') — preserve the original HTTPException for builder error responses and only handle other exceptions in the existing logging/throwing path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/public/build/request.ts`:
- Around line 189-204: The catch block is currently intercepting the
HTTPException thrown by quickError in the build response handling and
re-wrapping it as a generic "builder call failed" error, losing original
context; update the catch in the function handling the builder call to detect
and re-throw instances of HTTPException (importing HTTPException from
'hono/http-exception') instead of logging and converting them, so only non-HTTP
errors are logged via cloudlogErr (which uses c.get('requestId'), builderUrl,
org_id, app_id, platform) and then wrapped with
quickError('service_unavailable', 'Build service unavailable (builder call
failed)') — preserve the original HTTPException for builder error responses and
only handle other exceptions in the existing logging/throwing path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supabase/functions/_backend/public/build/request.tssupabase/functions/_backend/public/build/upload.ts
|
Summary
simpleError('service_unavailable', ...)(HTTP 400) withquickError(503, ...)at 5 call sites in the build request/upload flowTest plan
Summary by CodeRabbit