Auto-downgrade implicit concurrency (#5196)

* Auto-downgrade implicit concurrency

* Alternate approach which also upgrades concurrency

* Remove outdated comment

src/deploy/functions/prepare.ts

@@ -213,7 +213,7 @@ export async function prepare(
   for (const [codebase, { wantBackend, haveBackend }] of Object.entries(payload.functions)) {
     inferDetailsFromExisting(wantBackend, haveBackend, codebaseUsesEnvs.includes(codebase));
     await ensureTriggerRegions(wantBackend);
-    resolveCpu(wantBackend);
+    resolveCpuAndConcurrency(wantBackend);
     validate.endpointsAreValid(wantBackend);
     inferBlockingDetails(wantBackend);
   }
@@ -321,17 +321,15 @@ export function inferDetailsFromExisting(
       wantE.availableMemoryMb = haveE.availableMemoryMb;
     }
 
-    // N.B. This code doesn't handle automatic downgrading of concurrency if
-    // the customer sets CPU <1. We'll instead error that you can't have both.
-    // We may want to handle this case, though it might also be surprising to
-    // customers if they _don't_ get an error and we silently drop concurrency.
-    if (typeof wantE.concurrency === "undefined" && haveE.concurrency) {
-      wantE.concurrency = haveE.concurrency;
-    }
     if (typeof wantE.cpu === "undefined" && haveE.cpu) {
       wantE.cpu = haveE.cpu;
     }
 
+    // N.B. concurrency has different defaults based on CPU. If the customer
+    // only specifies CPU and they change that specification to < 1, we should
+    // turn off concurrency.
+    // We'll hanndle this in setCpuAndConcurrency
+
     wantE.securityLevel = haveE.securityLevel ? haveE.securityLevel : "SECURE_ALWAYS";
 
     maybeCopyTriggerRegion(wantE, haveE);
@@ -408,7 +406,7 @@ export function inferBlockingDetails(want: backend.Backend): void {
  * provided and sets concurrency based on the CPU level if not provided.
  * After this function, CPU will be a real number and not "gcf_gen1".
  */
-export function resolveCpu(want: backend.Backend): void {
+export function resolveCpuAndConcurrency(want: backend.Backend): void {
   for (const e of backend.allEndpoints(want)) {
     if (e.platform === "gcfv1") {
       continue;
@@ -418,5 +416,9 @@ export function resolveCpu(want: backend.Backend): void {
     } else if (!e.cpu) {
       e.cpu = backend.memoryToGen2Cpu(e.availableMemoryMb || backend.DEFAULT_MEMORY);
     }
+
+    if (!e.concurrency) {
+      e.concurrency = e.cpu >= 1 ? backend.DEFAULT_CONCURRENCY : 1;
+    }
   }
 }

src/test/deploy/functions/prepare.spec.ts

@@ -122,6 +122,59 @@ describe("prepare", () => {
       prepare.inferDetailsFromExisting(backend.of(want), backend.of(have), /* usedDotEnv= */ false);
       expect(want.availableMemoryMb).to.equal(512);
     });
+
+    it("downgrades concurrency if necessary (explicit)", () => {
+      const have: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+        concurrency: 80,
+        cpu: 1,
+      };
+      const want: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+        cpu: 0.5,
+      };
+
+      prepare.inferDetailsFromExisting(backend.of(want), backend.of(have), /* useDotEnv= */ false);
+      prepare.resolveCpuAndConcurrency(backend.of(want));
+      expect(want.concurrency).to.equal(1);
+    });
+
+    it("downgrades concurrency if necessary (implicit)", () => {
+      const have: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+        concurrency: 80,
+        cpu: 1,
+      };
+      const want: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+        cpu: "gcf_gen1",
+      };
+
+      prepare.inferDetailsFromExisting(backend.of(want), backend.of(have), /* useDotEnv= */ false);
+      prepare.resolveCpuAndConcurrency(backend.of(want));
+      expect(want.concurrency).to.equal(1);
+    });
+
+    it("upgrades default concurrency with CPU upgrades", () => {
+      const have: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+        availableMemoryMb: 256,
+        cpu: "gcf_gen1",
+      };
+      const want: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        httpsTrigger: {},
+      };
+
+      prepare.inferDetailsFromExisting(backend.of(want), backend.of(have), /* useDotEnv= */ false);
+      prepare.resolveCpuAndConcurrency(backend.of(want));
+      expect(want.concurrency).to.equal(1);
+    });
   });
 
   describe("inferBlockingDetails", () => {

src/test/deploy/functions/validate.spec.ts

@@ -8,7 +8,7 @@ import * as projectPath from "../../../projectPath";
 import * as secretManager from "../../../gcp/secretManager";
 import * as backend from "../../../deploy/functions/backend";
 import { BEFORE_CREATE_EVENT, BEFORE_SIGN_IN_EVENT } from "../../../functions/events/v1";
-import { resolveCpu } from "../../../deploy/functions/prepare";
+import { resolveCpuAndConcurrency } from "../../../deploy/functions/prepare";
 
 describe("validate", () => {
   describe("functionsDirectoryExists", () => {
@@ -331,7 +331,7 @@ describe("validate", () => {
           availableMemoryMb: mem,
           cpu: "gcf_gen1",
         };
-        resolveCpu(backend.of(ep));
+        resolveCpuAndConcurrency(backend.of(ep));
         expect(() => validate.endpointsAreValid(backend.of(ep))).to.not.throw;
       }
     });
@@ -344,7 +344,7 @@ describe("validate", () => {
           cpu: "gcf_gen1",
           concurrency: 42,
         };
-        resolveCpu(backend.of(ep));
+        resolveCpuAndConcurrency(backend.of(ep));
         expect(() => validate.endpointsAreValid(backend.of(ep))).to.not.throw;
       }
     });
@@ -356,7 +356,7 @@ describe("validate", () => {
         concurrency: 2,
         cpu: "gcf_gen1",
       };
-      resolveCpu(backend.of(ep));
+      resolveCpuAndConcurrency(backend.of(ep));
       expect(() => validate.endpointsAreValid(backend.of(ep))).to.throw(
         /concurrent execution and less than one full CPU/
       );