diff --git a/src/hosting/runTags.spec.ts b/src/hosting/runTags.spec.ts index 621aa061c43..f4c2f140ff1 100644 --- a/src/hosting/runTags.spec.ts +++ b/src/hosting/runTags.spec.ts @@ -265,6 +265,53 @@ describe("runTags", () => { expect(runTags.ensureLatestRevisionTagged); expect(runTags.gcTagsForServices).to.have.been.called; }); + + it("deduplicates multiple rewrites to the same service", async () => { + const rewrites: hostingNS.Rewrite[] = [ + { + glob: "/a", + run: { + serviceId: SERVICE, + region: REGION, + tag: runTagsNS.TODO_TAG_NAME, + }, + }, + { + glob: "/b", + run: { + serviceId: SERVICE, + region: REGION, + tag: runTagsNS.TODO_TAG_NAME, + }, + }, + ]; + + run.getService.withArgs(svcName).resolves(svc); + runTags.ensureLatestRevisionTagged.resetBehavior(); + runTags.ensureLatestRevisionTagged.resolves({ [REGION]: { [SERVICE]: "fh-version" } }); + + await runTags.setRewriteTags(rewrites, PROJECT, "version"); + + expect(run.getService).to.have.been.calledOnce; + expect(rewrites).to.deep.equal([ + { + glob: "/a", + run: { + serviceId: SERVICE, + region: REGION, + tag: "fh-version", + }, + }, + { + glob: "/b", + run: { + serviceId: SERVICE, + region: REGION, + tag: "fh-version", + }, + }, + ]); + }); }); describe("ensureLatestRevisionTagged", () => { diff --git a/src/hosting/runTags.ts b/src/hosting/runTags.ts index 4ada8359e91..2b96ae84c6a 100644 --- a/src/hosting/runTags.ts +++ b/src/hosting/runTags.ts @@ -87,25 +87,22 @@ export async function setRewriteTags( project: string, version: string, ): Promise { - // Note: this is sub-optimal in the case where there are multiple rewrites - // to the same service. Should we deduplicate this? - const services: run.Service[] = await Promise.all( - rewrites - .map((rewrite) => { - if (!("run" in rewrite)) { - return null; - } - if (rewrite.run.tag !== TODO_TAG_NAME) { - return null; - } + // Note: Deduplication is necessary to avoid race conditions when there are multiple + // rewrites pointing to the same service. + const uniqueServices: Record> = {}; + for (const rewrite of rewrites) { + if (!("run" in rewrite) || rewrite.run.tag !== TODO_TAG_NAME) { + continue; + } + const key = `${rewrite.run.region}/${rewrite.run.serviceId}`; + if (!(key in uniqueServices)) { + uniqueServices[key] = run.getService( + `projects/${project}/locations/${rewrite.run.region}/services/${rewrite.run.serviceId}`, + ); + } + } - return run.getService( - `projects/${project}/locations/${rewrite.run.region}/services/${rewrite.run.serviceId}`, - ); - }) - // filter does not drop the null annotation - .filter((s) => s !== null) as Array>, - ); + const services: run.Service[] = await Promise.all(Object.values(uniqueServices)); // Unnecessary due to functional programming, but creates an observable side effect for tests if (!services.length) { return;