Repository navigation
fix(functions): surface unhandled exceptions during functions deployment - #11257
Conversation
![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 improves error handling during Cloud Functions deployment by ensuring that unhandled exceptions in both upsert and delete operations are captured and re-thrown as FirebaseErrors, preventing silent failures. It also includes comprehensive unit tests to verify this behavior and adds a small fix to runtime label resolution. The review feedback correctly identified that the promise rejection reason casting is unsafe and that the exit code used in the new error handling should be consistent with the repository's standard (exit: 1).
| upsertErrors.length === 1 ? { original: upsertErrors[0] } : { children: upsertErrors }; | ||
| throw new FirebaseError("Fabricator.applyUpserts encountered an unhandled exception", { | ||
| ...opts, | ||
| exit: 2, |
There was a problem hiding this comment.
Using exit: 2 is inconsistent with other errors in this file and the rest of the codebase, which standardly use exit: 1 for general deployment failures.
To maintain consistency and adhere to the standard exit codes, consider changing this to exit: 1 or omitting it entirely to let it default to 1.
| exit: 2, | |
| exit: 1, |
References
- Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition, specify a non-zero exit code ({ exit: 1 }). (link)
There was a problem hiding this comment.
I chatted with AI a whole lot about this and it was convinced we should do 2 since it's an unknown internal error and not a user caused error. I defer to @joehan
| deleteErrors.length === 1 ? { original: deleteErrors[0] } : { children: deleteErrors }; | ||
| throw new FirebaseError("Fabricator.applyDeletes encountered an unhandled exception", { | ||
| ...opts, | ||
| exit: 2, |
There was a problem hiding this comment.
Using exit: 2 is inconsistent with other errors in this file and the rest of the codebase, which standardly use exit: 1 for general deployment failures.
To maintain consistency and adhere to the standard exit codes, consider changing this to exit: 1 or omitting it entirely to let it default to 1.
| exit: 2, | |
| exit: 1, |
References
- Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition, specify a non-zero exit code ({ exit: 1 }). (link)
There was a problem hiding this comment.
I chatted with AI a whole lot about this and it was convinced we should do 2 since it's an unknown internal error and not a user caused error. I defer to @joehan
There was a problem hiding this comment.
I buy your logic here. We've not been super consistent about error code handling accross the codebase, but thats no reason to not try to be better here.
f31cfbb to
3917c4c
Compare
3917c4c to
97aaf11
Compare
wandamora
left a comment
wandamora
left a comment
There was a problem hiding this comment.
LGTM % nit and Joe's feedback on the exit code.
59ab205 to
fbd7dad
Compare
### Description Prevent `Fabricator.applyPlan` from silently ignoring rejected `applyUpserts` or `applyDeletes` promises at `logger.debug`, which could allow a failed Cloud Functions deployment to proceed to function deletions and exit with code 0. Also move `logOpStart` inside `wrapOperation` and guard the `RUNTIMES` display-name lookup so per-endpoint logging errors are isolated to the affected function. Part of #6989 ### Scenarios Tested - Manually tested with `firebase deploy --only functions`. - Unit tests verifying `applyPlan` throws a `FirebaseError` and skips subsequent phases when `applyUpserts` or `applyDeletes` rejects unexpectedly. - Unit tests verifying `logOpStart` handles unrecognized runtime strings and isolates errors inside `wrapOperation`. ### Sample Commands ```bash firebase deploy --only functions ```
fbd7dad to
97a2869
Compare
Description
Prevent
Fabricator.applyPlanfrom silently ignoring rejectedapplyUpsertsorapplyDeletespromises atlogger.debug, which could allow a failed Cloud Functions deployment to proceed to function deletions and exit with code 0. Also movelogOpStartinsidewrapOperationand guard theRUNTIMESdisplay-name lookup so per-endpoint logging errors are isolated to the affected function.Part of #6989
Scenarios Tested
firebase deploy --only functions.applyPlanthrows aFirebaseErrorand skips subsequent phases whenapplyUpsertsorapplyDeletesrejects unexpectedly.logOpStarthandles unrecognized runtime strings and isolates errors insidewrapOperation.Sample Commands