[shettyvarun268]

Description

When deploying Cloud Functions with declarative security, newly provisioned managed service accounts (firebase-fn-[0-9]+@<project>.iam.gserviceaccount.com) take time to propagate across Google Cloud IAM and downstream services (Cloud Functions, Eventarc, Cloud Run). While QueueExecutor retries these transient 404/400/403 errors, functionsOpLogReject in src/gcp/cloudfunctionsv2.ts and src/gcp/cloudfunctions.ts previously logged immediate failure warnings (⚠ functions: Request to ... had HTTP Error: 404... and ⚠ functions: failed to create function ...) before each retry attempt—alarming users even when a subsequent retry succeeded. Similarly, 2nd Gen 429 Quota Exceeded errors logged failed to <type> function immediately after Waiting to retry....

This change:

• Logs service account propagation errors (isServiceAccountPropagationError) at debug level in functionsOpLogReject instead of printing warning banners during retries.
• Suppresses the immediate failed to <type> function warning during 429 Quota Exceeded retries in cloudfunctionsv2.ts (matching cloudfunctions.ts).
• Preserves the underlying GCP error message in the thrown FirebaseError (Failed to ${type} function ${funcName}: ${errMessage}) so the root cause is still surfaced if all retries are exhausted.

Scenarios Tested

• Live GCP deployment of 2nd Gen functions with requiresRole newly provisioned firebase-fn-* service accounts (verified 404 and 403 propagation retries log only at --debug level with zero premature warning banners and succeed on retry).
• Unit tests in src/gcp/cloudfunctionsv2.spec.ts and src/gcp/cloudfunctions.spec.ts verifying:
  • 404, 400 (Eventarc), and 403 (Cloud Run actAs) service account propagation errors log at debug level without emitting warning banners across createFunction and updateFunction.
  • Non-managed service account 404 errors still log warning banners immediately.
  • 429 Quota Exceeded errors in cloudfunctionsv2 only log the Waiting to retry... warning.

Sample Commands

firebase deploy --only functions

[gemini-code-assist]

Code Review

This pull request addresses an issue where Cloud Functions deployments logged premature failure warnings when retrying transient service account propagation or quota errors. It updates the error logging logic in both v1 and v2 Cloud Functions modules to log service account propagation errors at the debug level, and adds corresponding unit tests. The review feedback suggests a more robust extraction of error messages when the error is a string, and recommends refactoring the unit tests to clean up stubs and intercepts in afterEach hooks instead of inline try..finally blocks, adhering to the repository style guide.

[wandamora]

I left comments to fix the current direction of this PR, but Andy's review tool had an interesting proposal:

🟡 [Architecture & Layering] Decouple src/gcp/ REST clients from src/deploy/functions/release/executor

Rationale: src/gcp/cloudfunctions.ts (line 14) and src/gcp/cloudfunctionsv2.ts (line 21) are single-attempt REST clients, whereas retry orchestration is decided per call site in Fabricator + QueueExecutor (src/deploy/functions/release/executor.ts:108). Importing isServiceAccountPropagationError upward from ../deploy/functions/release/executor leaves other QueueExecutor-retried errors (409 Conflict and 503 Service Unavailable in isTransientError, and code === 8 in isCloudRunResourceExhausted) still emitting premature failure warnings before succeeding on retry, while non-queued callers (src/functions/secrets.ts:363,376 in firebase functions:secrets:set, and deleteV1Function in fabricator.ts:908) log "Waiting to retry..." / "Waiting for service account propagation..." when no retry will occur.

Consider keeping src/gcp/cloudfunctions*.ts side-effect-free (matching src/gcp/runv2.ts:229-307) and logging retry notices (Waiting to retry... on isQuotaExhaustion, logger.debug on other matched retryPredicates) directly inside QueueExecutor's handler (executor.ts:108-110). If keeping warning logic in functionsOpLogReject for this PR, consider also suppressing isTransientError (409/503) warnings and moving shared error predicates to a lower-level module.

---

This comment is interesting in offering a different architectural approach to solve this retry logging problem and centralizing this in QueueExecutor. This approach may be "cleaner" so that the HTTP cloudfunctions clients do not need to guess if a retry occurred because the QueueExecutor has that information. After talking a bit with Jetski, this direction would be more complex for a few reasons:

• QueueExecutor doesn't know what it is running: Executor.run only receives an anonymous () => Promise<T>—it has no funcName or "create" | "update" | "delete" verb. Worse, Fabricator uses QueueExecutor at ~35 call sites (Pub/Sub topics, Eventarc channels, Cloud Scheduler, Cloud Tasks, Cloud Run IAM, plus pollOperation inside the same lambda). Teaching QueueExecutor to print "Waiting to retry..." with the function name requires threading operation metadata through RunOptions across Fabricator (or sniffing err.context?.function) while avoiding new log noise on the other ~30 tasks.

• Higher UX blast radius: Removing logWarning / logLabeledWarning from functionsOpLogReject requires reworking how multi-line remediation messages (maxScale may not exceed, Runtime validation errors, first-time Eventarc setup) render across v1, v2, and direct callers like secrets.ts.

• No circular dependency today: executor.ts only imports src/throttler/*, so importing predicates from executor.ts into src/gcp/—while slightly awkward layering—is acyclic and safe at runtime.

Would love to hear @ajperel's thoughts about what to do. Varun's change does work at minimally fixing this, but this appraoch may be better for the "long-term architecture".

[wandamora]

🔴 [CLI UX & Error Surfacing] Duplicate raw HTTP error output on non-retryable 2nd Gen failures

Rationale: On any non-SA, non-429, non-maxScale failure (e.g., 400 invalid config, 403 permission denied, or 404 custom service account typo), line 260 logs utils.logLabeledWarning(\"functions\", errMessage) and line 271 logs utils.logLabeledWarning(\"functions\", \"failed to ...\"). Because line 274 now appends : ${errMessage} unconditionally to the thrown FirebaseError, Fabricator (src/deploy/functions/release/fabricator.ts:82,699,860) immediately catches the unretried error and calls logger.error((err as Error).message) (which prints at default CLI info level per src/logger.ts:310-319), dumping both Failed to <type> function ... and the entire Request to https://... had HTTP Error: ... payload a second time back-to-back. Additionally, maxScale may not exceed (lines 235-249), which previously suppressed the raw HTTP error in favor of a friendly quota remediation message, now also dumps the raw HTTP error via Fabricator.

Suggested Fix: Only include : ${errMessage} on the thrown FirebaseError for the branches where the raw error warning was suppressed (isServiceAccountPropagationError and 429, so RetriesExhaustedError still surfaces the root cause if retries exhaust), or drop the redundant utils.logLabeledWarning(\"functions\", errMessage) at line 260 while keeping maxScale without the : ${errMessage} suffix:

  const suppressed = isServiceAccountPropagationError(err) || err?.context?.response?.statusCode === 429;
  throw new FirebaseError(
    `Failed to ${type} function ${func.name}${suppressed && errMessage ? `: ${errMessage}` : \"\"}`,
    {
      original: err,
      status: err?.context?.response?.statusCode,
      context: { function: func.name },
    },
  );

(Note: if gating : ${errMessage} on suppressed, update the non-managed SA 404 test in cloudfunctionsv2.spec.ts:1030 to assert rejectedWith(\"Failed to create function ...\") while verifying warnSpy.firstCall received the raw 404 message.)

[shettyvarun268]

Good catch! Updated this so we only append errMessage to the thrown error when the warning was suppressed (for SA propagation and 429 retries), and updated the test accordingly.

[wandamora]

🔴 [Test Isolation & Mock Verification] Clone shared CLOUD_FUNCTION fixture and assert scope.isDone() per test

Rationale:

1. cloudfunctions.createFunction mutates its argument in-place by assigning cloudFunction.buildEnvironmentVariables = { ..., GOOGLE_NODE_RUN_SCRIPTS: \"\" } (src/gcp/cloudfunctions.ts:242), and cloudfunctionsv2.createFunction / updateFunction likewise mutate buildConfig.environmentVariables and serviceConfig.environmentVariables in-place (src/gcp/cloudfunctionsv2.ts:309,316,392,398). Passing the suite-level CLOUD_FUNCTION (here and line 802) and CLOUD_FUNCTION_V2 (src/gcp/cloudfunctionsv2.spec.ts:966,995,1000,1028,1048,1099) constants directly mutates shared fixtures, causing functionFromEndpoint .to.deep.equal tests (cloudfunctions.spec.ts:67-71, cloudfunctionsv2.spec.ts:82) to fail if test order changes or suites are filtered (note how cloudfunctionsv2.spec.ts:916-927 clones CLOUD_FUNCTION_V2 for this reason).
2. Because afterEach calls nock.cleanAll() (and src/test/helpers/mocha-bootstrap.js also clears nock after each test), the legacy suite-level after(() => expect(nock.isDone()).to.be.true) at line 49 cannot detect unconsumed mocks (GEMINI.md). Capture the nock scope (or check expect(nock.isDone()).to.be.true) inside each test in both spec files.

Suggested Fix:

      const scope = nock(functionsOrigin())
        .post(\"/v1/projects/project/locations/region/functions\")
        .reply(404, {
          error: { 
            message:
              \"Service account projects/-/serviceAccounts/firebase-fn-201@project.iam.gserviceaccount.com does not exist.\",
          },
        });

      await expect(
        cloudfunctions.createFunction({ ...CLOUD_FUNCTION }),
      ).to.eventually.be.rejectedWith(
        \"Service account projects/-/serviceAccounts/firebase-fn-201@project.iam.gserviceaccount.com does not exist.\",
      );
      expect(warnSpy.called).to.be.false;
      expect(debugSpy.calledWith(sinon.match(\"Waiting for service account propagation\"))).to.be
        .true;
      expect(scope.isDone()).to.be.true;

[shettyvarun268]

Done! cloned the fixtures and added expect(scope.isDone()).to.be.true across the tests in both spec files.

[wandamora]

🟡 [Architecture & Layering] Decouple src/gcp/ REST clients from src/deploy/functions/release/executor

Rationale: src/gcp/cloudfunctions.ts (line 14) and src/gcp/cloudfunctionsv2.ts (line 21) are single-attempt REST clients, whereas retry orchestration is decided per call site in Fabricator + QueueExecutor (src/deploy/functions/release/executor.ts:108). Importing isServiceAccountPropagationError upward from ../deploy/functions/release/executor leaves other QueueExecutor-retried errors (409 Conflict and 503 Service Unavailable in isTransientError, and code === 8 in isCloudRunResourceExhausted) still emitting premature failure warnings before succeeding on retry, while non-queued callers (src/functions/secrets.ts:363,376 in firebase functions:secrets:set, and deleteV1Function in fabricator.ts:908) log \"Waiting to retry...\" / \"Waiting for service account propagation...\" when no retry will occur.

Consider keeping src/gcp/cloudfunctions*.ts side-effect-free (matching src/gcp/runv2.ts:229-307) and logging retry notices (Waiting to retry... on isQuotaExhaustion, logger.debug on other matched retryPredicates) directly inside QueueExecutor's handler (executor.ts:108-110). If keeping warning logic in functionsOpLogReject for this PR, consider also suppressing isTransientError (409/503) warnings and moving shared error predicates to a lower-level module.