diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..2b3927b5dbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- Fixed `functions:delete` recreating an already-deleted Cloud Tasks queue for task queue functions. (#9305) diff --git a/src/deploy/functions/release/fabricator.spec.ts b/src/deploy/functions/release/fabricator.spec.ts index 20fda610483..f5167b3da69 100644 --- a/src/deploy/functions/release/fabricator.spec.ts +++ b/src/deploy/functions/release/fabricator.spec.ts @@ -90,6 +90,7 @@ describe("Fabricator", () => { tasks.upsertQueue.rejects(new Error("unexpected tasks.upsertQueue")); tasks.createQueue.rejects(new Error("unexpected tasks.createQueue")); tasks.updateQueue.rejects(new Error("unexpected tasks.updateQueue")); + tasks.disableQueue.rejects(new Error("unexpected tasks.disableQueue")); tasks.deleteQueue.rejects(new Error("unexpected tasks.deleteQueue")); tasks.setEnqueuer.rejects(new Error("unexpected tasks.setEnqueuer")); tasks.setIamPolicy.rejects(new Error("unexpected tasks.setIamPolicy")); @@ -1211,19 +1212,16 @@ describe("Fabricator", () => { const ep = endpoint({ taskQueueTrigger: {}, }) as backend.Endpoint & backend.TaskQueueTriggered; - tasks.updateQueue.resolves(); + tasks.disableQueue.resolves(); await fab.disableTaskQueue(ep); - expect(tasks.updateQueue).to.have.been.calledWith({ - name: tasks.queueNameForEndpoint(ep), - state: "DISABLED", - }); + expect(tasks.disableQueue).to.have.been.calledWith(tasks.queueNameForEndpoint(ep)); }); it("wraps errors", async () => { const ep = endpoint({ taskQueueTrigger: {}, }) as backend.Endpoint & backend.TaskQueueTriggered; - tasks.updateQueue.rejects(new Error("Not today")); + tasks.disableQueue.rejects(new Error("Not today")); await expect(fab.disableTaskQueue(ep)).to.eventually.be.rejectedWith( reporter.DeploymentError, "disable task queue", diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts index be42e9a0e3a..f4dd2d2e5ab 100644 --- a/src/deploy/functions/release/fabricator.ts +++ b/src/deploy/functions/release/fabricator.ts @@ -918,12 +918,8 @@ export class Fabricator { } async disableTaskQueue(endpoint: backend.Endpoint & backend.TaskQueueTriggered): Promise { - const update = { - name: cloudtasks.queueNameForEndpoint(endpoint), - state: "DISABLED" as cloudtasks.State, - }; await this.executor - .run(() => cloudtasks.updateQueue(update)) + .run(() => cloudtasks.disableQueue(cloudtasks.queueNameForEndpoint(endpoint))) .catch(rethrowAs(endpoint, "disable task queue")); } diff --git a/src/gcp/cloudtasks.spec.ts b/src/gcp/cloudtasks.spec.ts index 70c817d7d0c..f091b33c251 100644 --- a/src/gcp/cloudtasks.spec.ts +++ b/src/gcp/cloudtasks.spec.ts @@ -25,6 +25,7 @@ describe("CloudTasks", () => { ct.triggerFromQueue.restore(); ct.setEnqueuer.restore(); ct.upsertQueue.restore(); + ct.disableQueue.restore(); }); afterEach(() => { @@ -179,6 +180,38 @@ describe("CloudTasks", () => { }); }); + describe("disableQueue", () => { + const NAME = "projects/p/locations/r/queues/f"; + + it("issues the update only when the queue exists", async () => { + ct.getQueue.resolves({ name: NAME, ...cloudtasks.DEFAULT_SETTINGS }); + ct.updateQueue.resolves({ name: NAME }); + + await cloudtasks.disableQueue(NAME); + + // queues.patch cannot actually change `state` (it is output only); we only + // assert that the long-standing PATCH is still issued for an existing queue. + expect(ct.getQueue).to.have.been.calledWith(NAME); + expect(ct.updateQueue).to.have.been.calledWith({ name: NAME, state: "DISABLED" }); + }); + + it("is a no-op when the queue no longer exists", async () => { + ct.getQueue.rejects({ context: { response: { statusCode: 404 } } }); + + await cloudtasks.disableQueue(NAME); + + expect(ct.getQueue).to.have.been.calledWith(NAME); + expect(ct.updateQueue).to.not.have.been.called; + }); + + it("rethrows non-404 errors without patching", async () => { + ct.getQueue.rejects({ context: { response: { statusCode: 500 } } }); + + await expect(cloudtasks.disableQueue(NAME)).to.be.rejected; + expect(ct.updateQueue).to.not.have.been.called; + }); + }); + describe("setEnqueuer", () => { const NAME = "projects/p/locations/r/queues/f"; const ADMIN_BINDING: iam.Binding = { diff --git a/src/gcp/cloudtasks.ts b/src/gcp/cloudtasks.ts index f3c10decea6..81c20d3f71c 100644 --- a/src/gcp/cloudtasks.ts +++ b/src/gcp/cloudtasks.ts @@ -125,6 +125,31 @@ export async function upsertQueue(queue: Queue): Promise { } } +/** + * Best-effort update issued when a task queue triggered function is deleted. + * Skips the call when the queue no longer exists: updateQueue issues a PATCH, + * which creates the queue if it is absent, so patching an already-deleted queue + * would recreate it -- or fail with "existed too recently" when it was just + * deleted out of band (issue #9305). + * + * Note: queues.patch cannot change `state` (it is output only in the Cloud Tasks + * API; state changes go through pause/resume or queue.yaml), so this preserves + * the long-standing PATCH behavior while no longer resurrecting a queue the user + * already removed. + */ +export async function disableQueue(name: string): Promise { + try { + // Here and throughout we use module.exports to ensure late binding & enable stubs in unit tests. + await (module.exports.getQueue as typeof getQueue)(name); + } catch (err: any) { + if (err?.context?.response?.statusCode === 404) { + return; + } + throw err; + } + await (module.exports.updateQueue as typeof updateQueue)({ name, state: "DISABLED" }); +} + /** Purges all messages in a queue with a given name. */ export async function purgeQueue(name: string): Promise { await client.post(`${name}:purge`);