diff --git a/CHANGELOG.md b/CHANGELOG.md index e171e217116..20df854e649 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,4 @@ +- Fixed an issue where unhandled exceptions during Cloud Functions deployment could be silently ignored and exit with code 0 (#6989). - Added `crashlytics:heapdumps:{enable,disable,status}` commands to manage Crashlytics Android heap dump collection. (#11225) - Removed unused dependencies `proxy-agent`, `universal-analytics`, and `@apphosting/common`. - Fixed an issue where throttled task queues stopped processing remaining tasks when a task exhausted its retries or timed out (#6989, #11040). diff --git a/src/deploy/functions/release/fabricator.spec.ts b/src/deploy/functions/release/fabricator.spec.ts index b51b92d6072..2e807a74afc 100644 --- a/src/deploy/functions/release/fabricator.spec.ts +++ b/src/deploy/functions/release/fabricator.spec.ts @@ -1976,6 +1976,123 @@ describe("Fabricator", () => { expect(scrapers).to.have.lengthOf(2); expect(scrapers[0]).to.not.equal(scrapers[1]); }); + + it("throws FirebaseError and skips deletes when applyUpserts rejects unexpectedly", async () => { + const ep1 = endpoint({ httpsTrigger: {} }, { id: "createFn", region: "us-central1" }); + const ep2 = endpoint({ httpsTrigger: {} }, { id: "deleteFn", region: "us-west1" }); + const plan: planner.DeploymentPlan = { + default: { + plannedBackend: backend.of(ep1), + regionalChangesets: { + "us-central1": { + endpointsToCreate: [ep1], + endpointsToUpdate: [], + endpointsToDelete: [], + endpointsToSkip: [], + }, + "us-west1": { + endpointsToCreate: [], + endpointsToUpdate: [], + endpointsToDelete: [ep2], + endpointsToSkip: [], + }, + }, + }, + }; + + const upsertCause = new Error("Unexpected upsert crash"); + const applyUpsertsStub = sinon.stub(fab, "applyUpserts"); + applyUpsertsStub.onFirstCall().rejects(upsertCause); + applyUpsertsStub.onSecondCall().resolves([]); + const applyDeletesSpy = sinon.spy(fab, "applyDeletes"); + + const err = (await expect(fab.applyPlan(plan)).to.be.rejectedWith( + FirebaseError, + "Fabricator.applyUpserts encountered an unhandled exception: Unexpected upsert crash", + )) as FirebaseError; + expect(err.original).to.equal(upsertCause); + expect(applyDeletesSpy).to.not.have.been.called; + }); + + it("throws FirebaseError with children and skips role removals when applyDeletes rejects across regions", async () => { + const ep1 = endpoint({ httpsTrigger: {} }, { id: "deleteFnCentral", region: "us-central1" }); + const ep2 = endpoint({ httpsTrigger: {} }, { id: "deleteFnWest", region: "us-west1" }); + const plan: planner.DeploymentPlan = { + default: { + plannedBackend: backend.empty(), + regionalChangesets: { + "us-central1": { + endpointsToCreate: [], + endpointsToUpdate: [], + endpointsToDelete: [ep1], + endpointsToSkip: [], + }, + "us-west1": { + endpointsToCreate: [], + endpointsToUpdate: [], + endpointsToDelete: [ep2], + endpointsToSkip: [], + }, + }, + }, + }; + + const applyDeletesStub = sinon.stub(fab, "applyDeletes"); + applyDeletesStub.onFirstCall().rejects(new Error("Unexpected delete crash central")); + applyDeletesStub.onSecondCall().rejects(new Error("Unexpected delete crash west")); + const removeOldRolesSpy = sinon.spy(fab, "removeOldRoles"); + + const err = (await expect(fab.applyPlan(plan)).to.be.rejectedWith( + FirebaseError, + "Fabricator.applyDeletes encountered unhandled exceptions", + )) as FirebaseError; + expect(err.children).to.have.lengthOf(2); + expect((err.children[0] as Error).message).to.equal("Unexpected delete crash central"); + expect((err.children[1] as Error).message).to.equal("Unexpected delete crash west"); + expect(removeOldRolesSpy).to.not.have.been.called; + }); + + it("isolates logOpStart errors to the failing endpoint and handles unknown runtimes", async () => { + const epUnknownRuntime = endpoint( + { httpsTrigger: {} }, + { + id: "unknownRuntimeFn", + region: "us-central1", + runtime: "custom-future-runtime" as backend.Endpoint["runtime"], + }, + ); + const epFailingLog = endpoint( + { httpsTrigger: {} }, + { id: "failingLogFn", region: "us-central1" }, + ); + const changes: planner.Changeset = { + endpointsToCreate: [epUnknownRuntime, epFailingLog], + endpointsToUpdate: [], + endpointsToDelete: [], + endpointsToSkip: [], + }; + + sinon.stub(fab, "createEndpoint").resolves(); + const origLogOpStart = fab.logOpStart.bind(fab); + sinon.stub(fab, "logOpStart").callsFake((op: string, ep: backend.Endpoint) => { + if (ep.id === "failingLogFn") { + throw new Error("logOpStart failure"); + } + origLogOpStart(op, ep); + }); + + const results = await fab.applyUpserts( + changes, + new scraper.SourceTokenScraper(), + new scraper.SourceTokenScraper(), + ); + + expect(results).to.have.lengthOf(2); + expect(results[0].endpoint.id).to.equal("unknownRuntimeFn"); + expect(results[0].error).to.be.undefined; + expect(results[1].endpoint.id).to.equal("failingLogFn"); + expect(results[1].error?.message).to.equal("logOpStart failure"); + }); }); describe("createRunFunction", () => { diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts index 20f6669f5e3..2fd6f29ba23 100644 --- a/src/deploy/functions/release/fabricator.ts +++ b/src/deploy/functions/release/fabricator.ts @@ -8,7 +8,7 @@ import { parseErrorCode, } from "./executor"; import * as ensure from "../ensure"; -import { FirebaseError } from "../../../error"; +import { FirebaseError, getError, getErrStack } from "../../../error"; import { SourceTokenScraper } from "./sourceTokenScraper"; import { Timer } from "./timer"; @@ -275,20 +275,37 @@ export class Fabricator { const createAndUpdateResultsArray = await Promise.allSettled(createAndUpdatePromises); // Process results of Phase 1 + const upsertErrors: Error[] = []; summary.results = createAndUpdateResultsArray.reduce((acc, r) => { if (r.status === "fulfilled") { return [...acc, ...r.value]; } // Handle rejection + upsertErrors.push(getError(r.reason)); logger.debug( "Fabricator.applyUpserts returned an unhandled exception.", - JSON.stringify(r.reason, null, 2), + getErrStack(r.reason), ); return acc; }, []); await this.cleanupUnusedServiceAccounts(plan, summary.results); + // If applyUpserts rejected outside wrapOperation, fail immediately instead of + // proceeding to function deletions with an incomplete result set and exiting 0. + if (upsertErrors.length) { + const opts = + upsertErrors.length === 1 ? { original: upsertErrors[0] } : { children: upsertErrors }; + const msg = + upsertErrors.length === 1 + ? `Fabricator.applyUpserts encountered an unhandled exception: ${upsertErrors[0].message}` + : "Fabricator.applyUpserts encountered unhandled exceptions"; + throw new FirebaseError(msg, { + ...opts, + exit: 2, + }); + } + const hasFailures = summary.results.some((r) => r.error); if (hasFailures) { @@ -312,19 +329,36 @@ export class Fabricator { allChangesets.map((changes) => this.applyDeletes(changes)), ); + const deleteErrors: Error[] = []; const deleteResults = deleteResultsArray.reduce((acc, r) => { if (r.status === "fulfilled") { return [...acc, ...r.value]; } + deleteErrors.push(getError(r.reason)); logger.debug( "Fabricator.applyDeletes returned an unhandled exception. This should never happen", - JSON.stringify(r.reason, null, 2), + getErrStack(r.reason), ); return acc; }, []); summary.results.push(...deleteResults); + // If applyDeletes rejected outside wrapOperation, fail instead of returning + // an incomplete summary that allows the deploy to succeed with exit code 0. + if (deleteErrors.length) { + const opts = + deleteErrors.length === 1 ? { original: deleteErrors[0] } : { children: deleteErrors }; + const msg = + deleteErrors.length === 1 + ? `Fabricator.applyDeletes encountered an unhandled exception: ${deleteErrors[0].message}` + : "Fabricator.applyDeletes encountered unhandled exceptions"; + throw new FirebaseError(msg, { + ...opts, + exit: 2, + }); + } + // Similarly to grantNewRoles, we execute role removals sequentially to avoid concurrent // IAM policy update conflicts (409 Conflict / etag mismatch) on GCP. for (const [codebase, codebasePlan] of Object.entries(plan)) { @@ -370,11 +404,11 @@ export class Fabricator { const ops: Array> = []; for (const endpoint of changes.endpointsToCreate) { - this.logOpStart("creating", endpoint); ops.push( - this.wrapOperation("create", endpoint, () => - this.createEndpoint(endpoint, scraperV1, scraperV2), - ), + this.wrapOperation("create", endpoint, () => { + this.logOpStart("creating", endpoint); + return this.createEndpoint(endpoint, scraperV1, scraperV2); + }), ); } @@ -383,11 +417,11 @@ export class Fabricator { } for (const update of changes.endpointsToUpdate) { - this.logOpStart("updating", update.endpoint); ops.push( - this.wrapOperation("update", update.endpoint, () => - this.updateEndpoint(update, scraperV1, scraperV2), - ), + this.wrapOperation("update", update.endpoint, () => { + this.logOpStart("updating", update.endpoint); + return this.updateEndpoint(update, scraperV1, scraperV2); + }), ); } @@ -398,8 +432,12 @@ export class Fabricator { const ops: Array> = []; for (const endpoint of changes.endpointsToDelete) { - this.logOpStart("deleting", endpoint); - ops.push(this.wrapOperation("delete", endpoint, () => this.deleteEndpoint(endpoint))); + ops.push( + this.wrapOperation("delete", endpoint, () => { + this.logOpStart("deleting", endpoint); + return this.deleteEndpoint(endpoint); + }), + ); } return Promise.all(ops); @@ -1177,7 +1215,9 @@ export class Fabricator { } logOpStart(op: string, endpoint: backend.Endpoint): void { - const runtime = endpoint.runtime ? RUNTIMES[endpoint.runtime].friendly : "unknown"; + const runtime = endpoint.runtime + ? RUNTIMES[endpoint.runtime]?.friendly ?? endpoint.runtime + : "unknown"; const platform = getHumanFriendlyPlatformName(endpoint.platform); const label = helper.getFunctionLabel(endpoint); utils.logLabeledBullet(