Preserve quota statuses so function deploy retries work #9497
Conversation
kdawgwilk
commented
Nov 20, 2025
kdawgwilk
commented
- Fix Cloud Functions v1/v2 error wrapping to retain HTTP status so 429/RESOURCE_EXHAUSTED surfaces correctly instead of being lost on rethrow.
- Broaden retry code parsing in the deploy executor to check nested original.status, letting quota errors re-queue instead of silently succeeding.
- This ensures quota-hit deploys actually retry and fail properly instead of yielding false-positive success.
- Keep Cloud Functions v1/v2 errors carrying HTTP status so 429/RESOURCE_EXHAUSTED isn't dropped on rethrow. - Teach deploy executor to read nested original.status when detecting retryable quota errors. - Deploys that hit quota now requeue/fail correctly instead of showing false success.
Summary of ChangesHello @kdawgwilk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where Cloud Functions deployments encountering quota exhaustion errors would not correctly surface these errors, leading to silent failures or misleading success reports. The changes involve preserving HTTP status codes during error wrapping and enhancing the retry mechanism to accurately recognize these statuses. As a result, the system will now reliably detect and handle quota-related deployment failures, ensuring proper retries and preventing incorrect deployment outcomes. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 correctly addresses an issue with error handling for function deployments, ensuring that quota-related errors are properly retried. The changes to propagate the HTTP status code are well-implemented. I have a couple of suggestions to make the status code extraction more robust against unexpected error types by ensuring the extracted status is always a number.
| * @param err The error returned from the operation. | ||
| */ | ||
| function functionsOpLogReject(funcName: string, type: string, err: any): void { | ||
| const status = err?.context?.response?.statusCode || err?.status; |
There was a problem hiding this comment.
While err.status is expected to be a number from FirebaseError, the type of err is any, which doesn't guarantee this. If err.status were a non-numeric string (e.g., an error status string like 'RESOURCE_EXHAUSTED'), it would be assigned to status. This could lead to incorrect behavior, as status === 429 would fail and a non-numeric value would be passed to the FirebaseError constructor, which expects a number. To make this more robust, consider ensuring the extracted value is a number.
| const status = err?.context?.response?.statusCode || err?.status; | |
| const code = err?.context?.response?.statusCode || err?.status; | |
| const status = typeof code === "number" ? code : undefined; |
There was a problem hiding this comment.
The error paths feeding these handlers already give us numeric status codes: responseToError sets statusCode as a number, gax errors use numeric code, and FirebaseError stores a numeric status (default 500). If a strange upstream ever handed us a string like "RESOURCE_EXHAUSTED", the 429 check would simply skip the quota branch and the FirebaseError would carry that string, but it wouldn’t change retry behavior because the executor’s parseErrorCode still looks for numeric codes.
| * @param err The error returned from the operation. | ||
| */ | ||
| function functionsOpLogReject(func: InputCloudFunction, type: string, err: any): void { | ||
| const status = err?.context?.response?.statusCode || err?.status; |
There was a problem hiding this comment.
Similar to the v1 file, err.status is not guaranteed to be a number since err is of type any. If it were a string, it could cause issues with status code checks and when constructing the FirebaseError. To improve type safety and prevent potential runtime issues, it's safer to ensure that status is a number.
| const status = err?.context?.response?.statusCode || err?.status; | |
| const code = err?.context?.response?.statusCode || err?.status; | |
| const status = typeof code === "number" ? code : undefined; |
There was a problem hiding this comment.
The error paths feeding these handlers already give us numeric status codes: responseToError sets statusCode as a number, gax errors use numeric code, and FirebaseError stores a numeric status (default 500). If a strange upstream ever handed us a string like "RESOURCE_EXHAUSTED", the 429 check would simply skip the quota branch and the FirebaseError would carry that string, but it wouldn’t change retry behavior because the executor’s parseErrorCode still looks for numeric codes.
1 participant
