Skip to content

Commit c651633

Browse files
committed
fix(functions): avoid recreating a deleted task queue on functions:delete
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.
1 parent c056411 commit c651633

5 files changed

Lines changed: 64 additions & 11 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Fixed `functions:delete` recreating an already-deleted Cloud Tasks queue for task queue functions. (#9305)

src/deploy/functions/release/fabricator.spec.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ describe("Fabricator", () => {
9090
tasks.upsertQueue.rejects(new Error("unexpected tasks.upsertQueue"));
9191
tasks.createQueue.rejects(new Error("unexpected tasks.createQueue"));
9292
tasks.updateQueue.rejects(new Error("unexpected tasks.updateQueue"));
93+
tasks.disableQueue.rejects(new Error("unexpected tasks.disableQueue"));
9394
tasks.deleteQueue.rejects(new Error("unexpected tasks.deleteQueue"));
9495
tasks.setEnqueuer.rejects(new Error("unexpected tasks.setEnqueuer"));
9596
tasks.setIamPolicy.rejects(new Error("unexpected tasks.setIamPolicy"));
@@ -1211,19 +1212,16 @@ describe("Fabricator", () => {
12111212
const ep = endpoint({
12121213
taskQueueTrigger: {},
12131214
}) as backend.Endpoint & backend.TaskQueueTriggered;
1214-
tasks.updateQueue.resolves();
1215+
tasks.disableQueue.resolves();
12151216
await fab.disableTaskQueue(ep);
1216-
expect(tasks.updateQueue).to.have.been.calledWith({
1217-
name: tasks.queueNameForEndpoint(ep),
1218-
state: "DISABLED",
1219-
});
1217+
expect(tasks.disableQueue).to.have.been.calledWith(tasks.queueNameForEndpoint(ep));
12201218
});
12211219

12221220
it("wraps errors", async () => {
12231221
const ep = endpoint({
12241222
taskQueueTrigger: {},
12251223
}) as backend.Endpoint & backend.TaskQueueTriggered;
1226-
tasks.updateQueue.rejects(new Error("Not today"));
1224+
tasks.disableQueue.rejects(new Error("Not today"));
12271225
await expect(fab.disableTaskQueue(ep)).to.eventually.be.rejectedWith(
12281226
reporter.DeploymentError,
12291227
"disable task queue",

src/deploy/functions/release/fabricator.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -918,12 +918,8 @@ export class Fabricator {
918918
}
919919

920920
async disableTaskQueue(endpoint: backend.Endpoint & backend.TaskQueueTriggered): Promise<void> {
921-
const update = {
922-
name: cloudtasks.queueNameForEndpoint(endpoint),
923-
state: "DISABLED" as cloudtasks.State,
924-
};
925921
await this.executor
926-
.run(() => cloudtasks.updateQueue(update))
922+
.run(() => cloudtasks.disableQueue(cloudtasks.queueNameForEndpoint(endpoint)))
927923
.catch(rethrowAs(endpoint, "disable task queue"));
928924
}
929925

src/gcp/cloudtasks.spec.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ describe("CloudTasks", () => {
2525
ct.triggerFromQueue.restore();
2626
ct.setEnqueuer.restore();
2727
ct.upsertQueue.restore();
28+
ct.disableQueue.restore();
2829
});
2930

3031
afterEach(() => {
@@ -179,6 +180,38 @@ describe("CloudTasks", () => {
179180
});
180181
});
181182

183+
describe("disableQueue", () => {
184+
const NAME = "projects/p/locations/r/queues/f";
185+
186+
it("issues the update only when the queue exists", async () => {
187+
ct.getQueue.resolves({ name: NAME, ...cloudtasks.DEFAULT_SETTINGS });
188+
ct.updateQueue.resolves({ name: NAME });
189+
190+
await cloudtasks.disableQueue(NAME);
191+
192+
// queues.patch cannot actually change `state` (it is output only); we only
193+
// assert that the long-standing PATCH is still issued for an existing queue.
194+
expect(ct.getQueue).to.have.been.calledWith(NAME);
195+
expect(ct.updateQueue).to.have.been.calledWith({ name: NAME, state: "DISABLED" });
196+
});
197+
198+
it("is a no-op when the queue no longer exists", async () => {
199+
ct.getQueue.rejects({ context: { response: { statusCode: 404 } } });
200+
201+
await cloudtasks.disableQueue(NAME);
202+
203+
expect(ct.getQueue).to.have.been.calledWith(NAME);
204+
expect(ct.updateQueue).to.not.have.been.called;
205+
});
206+
207+
it("rethrows non-404 errors without patching", async () => {
208+
ct.getQueue.rejects({ context: { response: { statusCode: 500 } } });
209+
210+
await expect(cloudtasks.disableQueue(NAME)).to.be.rejected;
211+
expect(ct.updateQueue).to.not.have.been.called;
212+
});
213+
});
214+
182215
describe("setEnqueuer", () => {
183216
const NAME = "projects/p/locations/r/queues/f";
184217
const ADMIN_BINDING: iam.Binding = {

src/gcp/cloudtasks.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,31 @@ export async function upsertQueue(queue: Queue): Promise<boolean> {
125125
}
126126
}
127127

128+
/**
129+
* Best-effort update issued when a task queue triggered function is deleted.
130+
* Skips the call when the queue no longer exists: updateQueue issues a PATCH,
131+
* which creates the queue if it is absent, so patching an already-deleted queue
132+
* would recreate it -- or fail with "existed too recently" when it was just
133+
* deleted out of band (issue #9305).
134+
*
135+
* Note: queues.patch cannot change `state` (it is output only in the Cloud Tasks
136+
* API; state changes go through pause/resume or queue.yaml), so this preserves
137+
* the long-standing PATCH behavior while no longer resurrecting a queue the user
138+
* already removed.
139+
*/
140+
export async function disableQueue(name: string): Promise<void> {
141+
try {
142+
// Here and throughout we use module.exports to ensure late binding & enable stubs in unit tests.
143+
await (module.exports.getQueue as typeof getQueue)(name);
144+
} catch (err: any) {
145+
if (err?.context?.response?.statusCode === 404) {
146+
return;
147+
}
148+
throw err;
149+
}
150+
await (module.exports.updateQueue as typeof updateQueue)({ name, state: "DISABLED" });
151+
}
152+
128153
/** Purges all messages in a queue with a given name. */
129154
export async function purgeQueue(name: string): Promise<void> {
130155
await client.post(`${name}:purge`);

0 commit comments

Comments
 (0)