fix(functions): avoid recreating a deleted task queue on functions:delete

CHANGELOG.md

@@ -0,0 +1 @@
+- Fixed `functions:delete` recreating an already-deleted Cloud Tasks queue for task queue functions. (#9305)

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",

src/deploy/functions/release/fabricator.ts

@@ -918,12 +918,8 @@ export class Fabricator {
   }
 
   async disableTaskQueue(endpoint: backend.Endpoint & backend.TaskQueueTriggered): Promise<void> {
-    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"));
   }
 

src/gcp/cloudtasks.spec.ts

@@ -3,6 +3,7 @@ import * as sinon from "sinon";
 
 import * as iam from "./iam";
 import * as backend from "../deploy/functions/backend";
+import { FirebaseError } from "../error";
 import * as cloudtasks from "./cloudtasks";
 import * as proto from "./proto";
 
@@ -25,6 +26,7 @@ describe("CloudTasks", () => {
     ct.triggerFromQueue.restore();
     ct.setEnqueuer.restore();
     ct.upsertQueue.restore();
+    ct.disableQueue.restore();
   });
 
   afterEach(() => {
@@ -179,6 +181,39 @@ 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 () => {
+      const err = new FirebaseError("boom", { context: { response: { statusCode: 500 } } });
+      ct.getQueue.rejects(err);
+
+      await expect(cloudtasks.disableQueue(NAME)).to.be.rejectedWith(err);
+      expect(ct.updateQueue).to.not.have.been.called;
+    });
+  });
+
   describe("setEnqueuer", () => {
     const NAME = "projects/p/locations/r/queues/f";
     const ADMIN_BINDING: iam.Binding = {

src/gcp/cloudtasks.ts

@@ -125,6 +125,32 @@ export async function upsertQueue(queue: Queue): Promise<boolean> {
   }
 }
 
+/**
+ * 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<void> {
+  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) {
+    const responseError = err as { context?: { response?: { statusCode?: number } } };
+    if (responseError.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<void> {
   await client.post(`${name}:purge`);