Fix race condition whith multiple rewrites to the same service

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", () => {

src/hosting/runTags.ts

@@ -87,25 +87,22 @@
   project: string,
   version: string,
 ): Promise<void> {
-  // 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<string, Promise<run.Service>> = {};
+  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<Promise<run.Service>>,
-  );
+  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;
@@ -117,11 +114,11 @@
     })
     .some((length) => length >= garbageCollectionThreshold);
   if (needsGC) {
     await exports.gcTagsForServices(project, services);
   }
 
   // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
   const tags: Record<string, Record<string, string>> = await exports.ensureLatestRevisionTagged(
     services,
     `fh-${version}`,
   );
@@ -163,7 +160,7 @@
     if (alreadyTagged) {
       // Null assertion is safe because the predicate that found alreadyTagged
       // checked for tag.
       tags[region][serviceId] = alreadyTagged.tag!;
       continue;
     }
     tags[region][serviceId] = defaultTag;