fix(functions): avoid recreating a deleted task queue on functions:delete - #10685
IzaakGough merged 11 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request resolves issue #9305 by preventing functions:delete from recreating an already-deleted Cloud Tasks queue. It introduces a new disableQueue helper in src/gcp/cloudtasks.ts that safely checks for the queue's existence before attempting to disable it, and updates the Fabricator and associated tests accordingly. Feedback on these changes includes a recommendation to avoid using any for the caught error in disableQueue to adhere to the repository's TypeScript style guide, as well as a suggestion to use idiomatic chai-as-promised assertions in the unit tests instead of manual try-catch blocks.
…lete functions:delete failed for onTaskDispatched functions whose Cloud Tasks queue had already been deleted out of band. disableTaskQueue calls updateQueue, which issues a queues.patch (create-or-update), so patching the missing queue tried to recreate it and failed with "The queue cannot be created because a queue with this name existed too recently". Guard the update with a getQueue existence check, mirroring upsertQueue: skip the patch when the queue is already gone (404) and rethrow otherwise.
88e36e8 to
c651633
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new disableQueue helper in cloudtasks.ts to check if a Cloud Tasks queue exists before attempting to disable it, preventing functions:delete from accidentally recreating an already-deleted queue (fixing issue #9305). The reviewer feedback suggests avoiding the use of any when catching errors in disableQueue to comply with the repository's style guide, recommending a more specific type cast instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10685 +/- ##
=======================================
Coverage ? 61.11%
=======================================
Files ? 653
Lines ? 44006
Branches ? 9056
=======================================
Hits ? 26895
Misses ? 14856
Partials ? 2255 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Heyo! Just popping in - is there a reason this hasn't been merged in yet? @IzaakGough @kyungseopk1m |
…rethrown error Address review: drop the `any` escape hatch in the getQueue catch block in favor of an inline type for the response error, and make the non-404 test assert that disableQueue rethrows the exact upstream error instead of accepting any rejection.
…task-queue-no-op # Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
Fixes #9305
firebase functions:deletefails for a Gen 2onTaskDispatchedfunction when its Cloud Tasks queue has already been deleted out of band:Root cause
On delete,
Fabricator.disableTaskQueuecallscloudtasks.updateQueue, which issues aqueues.patch. Per the Cloud Tasks API,queues.patchis create-or-update, so patching a queue that no longer exists tries to (re)create it — failing with the 400 above when it was deleted too recently, or otherwise silently resurrecting a queue the user intentionally removed.upsertQueuealready guards this exact case with agetQueue404 check; the disable path did not.Fix
Add a
getQueueexistence check before the patch (mirroringupsertQueue): if the queue is already gone (404) the update is skipped; any other error is rethrown.Note (out of scope)
queues.patchcannot actually changestate(it is output only — state changes go throughpause/resumeorqueue.yaml), so the pre-existing{ state: "DISABLED" }patch is best-effort and is preserved here unchanged. Making delete truly stop dispatch (viaqueues.pause) would also require teachingupsertQueueto resume/purge paused queues on redeploy, which looks like a separate change — happy to follow up if you'd prefer that direction.Notes
cloudtasks.queues.getpermission.