From d59b8d90ce80cf38a9268606aa5a4b99fbf4df55 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Wed, 25 Mar 2026 14:39:35 -0700 Subject: [PATCH 1/3] Fix race condition whith multiple rewrites to the same service --- src/hosting/runTags.spec.ts | 47 +++++++++++++++++++++++++++++++++++++ src/hosting/runTags.ts | 32 +++++++++++++------------ 2 files changed, 64 insertions(+), 15 deletions(-) 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..5ca3bc9121a 100644 --- a/src/hosting/runTags.ts +++ b/src/hosting/runTags.ts @@ -89,22 +89,24 @@ export async function setRewriteTags( ): 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; - } - - return run.getService( + const uniqueServicePromises = new Map>(); + 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 (!uniqueServicePromises.has(key)) { + uniqueServicePromises.set( + key, + 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( + Array.from(uniqueServicePromises.values()), ); // Unnecessary due to functional programming, but creates an observable side effect for tests if (!services.length) { From 8f7e72d9bf9931e9d8822c7734c5cb2406a5ca09 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Wed, 25 Mar 2026 14:39:35 -0700 Subject: [PATCH 2/3] Fix race condition whith multiple rewrites to the same service --- src/hosting/runTags.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/hosting/runTags.ts b/src/hosting/runTags.ts index 5ca3bc9121a..2e5b415f267 100644 --- a/src/hosting/runTags.ts +++ b/src/hosting/runTags.ts @@ -87,27 +87,21 @@ 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 uniqueServicePromises = new Map>(); + // Note: Deduplication is necessary to avoid race conditions when there are mutiple + 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 (!uniqueServicePromises.has(key)) { - uniqueServicePromises.set( - key, - run.getService( - `projects/${project}/locations/${rewrite.run.region}/services/${rewrite.run.serviceId}`, - ), + if (!(key in uniqueServices)) { + uniqueServices[key] = run.getService( + `projects/${project}/locations/${rewrite.run.region}/services/${rewrite.run.serviceId}`, ); } } - const services: run.Service[] = await Promise.all( - Array.from(uniqueServicePromises.values()), - ); + 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; From 7e2e0a17cd6ae38e9501fa3f6e4869478c8e5939 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Wed, 8 Apr 2026 10:39:42 -0700 Subject: [PATCH 3/3] Finish thoughts --- src/hosting/runTags.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hosting/runTags.ts b/src/hosting/runTags.ts index 2e5b415f267..2b96ae84c6a 100644 --- a/src/hosting/runTags.ts +++ b/src/hosting/runTags.ts @@ -87,7 +87,8 @@ export async function setRewriteTags( project: string, version: string, ): Promise { - // Note: Deduplication is necessary to avoid race conditions when there are mutiple + // 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) {