From df888e35b6e6cd638e8611f6f020ca674b025257 Mon Sep 17 00:00:00 2001 From: Izaak Gough Date: Wed, 12 Aug 2026 13:16:57 +0100 Subject: [PATCH 1/2] fix(extensions): skip extensions deploy when there is nothing to deploy A functions deploy for a codebase that declares no extensions still ran the extensions deploy stage, whose first action is a Cloud Billing API check. The functions SDK always emits an extensions record in the manifest, empty when there are none, so `build.extensions` was truthy and functions/prepare handed the extensions stages a payload even though prepareDynamicExtensions had returned early without building a plan. Only pass the extensions context and payload on when a plan was actually made, and guard extensions deploy the way release already does. Billing is now only checked when an instance is created, updated or configured, since deleting one does not require the Blaze plan. Fixes #7584 --- CHANGELOG.md | 1 + src/deploy/extensions/deploy.spec.ts | 68 +++++++++++++++++++++++++++ src/deploy/extensions/deploy.ts | 38 +++++++++++---- src/deploy/extensions/prepare.spec.ts | 16 +++++++ src/deploy/functions/prepare.ts | 9 +++- 5 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 src/deploy/extensions/deploy.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index bf459b51c40..2d18e2eb3f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,3 +6,4 @@ - Added web app support for Crashlytics MCP tools and prompts. - Added support for forwarding custom HTTP headers (`Mcp-Param-*`) to remote MCP tools when defined in tool parameter input schemas (`x-mcp-header`), per [SEP-2243](https://modelcontextprotocol.io/seps/2243-http-standardization). - Improved function parameter prompting clarity for multi-codebase deploys (#10897) +- Fixed a bug where deploying functions from a codebase with no extensions still required the Cloud Billing API to be enabled. (#7584) diff --git a/src/deploy/extensions/deploy.spec.ts b/src/deploy/extensions/deploy.spec.ts new file mode 100644 index 00000000000..ea5acb0039b --- /dev/null +++ b/src/deploy/extensions/deploy.spec.ts @@ -0,0 +1,68 @@ +import { expect } from "chai"; +import * as sinon from "sinon"; + +import { deploy } from "./deploy"; +import { Context, Payload } from "./args"; +import * as cloudbilling from "../../gcp/cloudbilling"; +import * as provisioningHelper from "../../extensions/provisioningHelper"; + +describe("Extensions deploy", () => { + let checkBillingEnabledStub: sinon.SinonStub; + let bulkCheckProductsProvisionedStub: sinon.SinonStub; + + const options: any = { nonInteractive: true, project: "test-project" }; + + beforeEach(() => { + checkBillingEnabledStub = sinon.stub(cloudbilling, "checkBillingEnabled").resolves(true); + bulkCheckProductsProvisionedStub = sinon + .stub(provisioningHelper, "bulkCheckProductsProvisioned") + .resolves(); + }); + + afterEach(() => { + sinon.restore(); + }); + + it("should not check billing when there is nothing to deploy", async () => { + // A functions deploy for a codebase that declares no extensions reaches this + // stage with an empty payload, and must not require the Cloud Billing API. + await deploy({} as Context, options, {} as Payload); + + expect(checkBillingEnabledStub.called).to.be.false; + expect(bulkCheckProductsProvisionedStub.called).to.be.false; + }); + + it("should not check billing when the payload only has empty instance lists", async () => { + const payload: Payload = { + instancesToCreate: [], + instancesToUpdate: [], + instancesToConfigure: [], + instancesToDelete: [], + }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.false; + }); + + it("should not check billing for a delete-only deploy", async () => { + // Deleting an instance does not require the Blaze plan. + const payload: Payload = { + instancesToDelete: [{ instanceId: "doomed", params: {}, systemParams: {} } as any], + }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.false; + }); + + it("should check billing when there is an instance to create", async () => { + const payload: Payload = { + instancesToCreate: [{ instanceId: "new-instance", params: {}, systemParams: {} } as any], + }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.calledWith("test-project")).to.be.true; + }); +}); diff --git a/src/deploy/extensions/deploy.ts b/src/deploy/extensions/deploy.ts index 1cfdee1dbfc..f5aed598a3f 100644 --- a/src/deploy/extensions/deploy.ts +++ b/src/deploy/extensions/deploy.ts @@ -16,15 +16,37 @@ import { checkBilling } from "./validate"; * @param payload The deploy payload */ export async function deploy(context: Context, options: Options, payload: Payload): Promise { + const instancesToCreate = payload.instancesToCreate ?? []; + const instancesToUpdate = payload.instancesToUpdate ?? []; + const instancesToConfigure = payload.instancesToConfigure ?? []; + const instancesToDelete = payload.instancesToDelete ?? []; + + // Nothing to do. `release` already guards this way; without the same guard here + // a functions deploy that declares no extensions still reaches the billing check + // below, because the SDK always emits an (empty) extensions record. + if ( + !instancesToCreate.length && + !instancesToUpdate.length && + !instancesToConfigure.length && + !instancesToDelete.length + ) { + return; + } + const projectId = needProjectId(options); - // First, check that billing is enabled - await checkBilling(projectId, options.nonInteractive); + + // First, check that billing is enabled. Creating, updating or configuring an + // instance requires the Blaze plan; deleting one does not, so a delete-only + // deploy doesn't need the Cloud Billing API. + if (instancesToCreate.length || instancesToUpdate.length || instancesToConfigure.length) { + await checkBilling(projectId, options.nonInteractive); + } // Then, check that required products are provisioned. await bulkCheckProductsProvisioned(projectId, [ - ...(payload.instancesToCreate ?? []), - ...(payload.instancesToUpdate ?? []), - ...(payload.instancesToConfigure ?? []), + ...instancesToCreate, + ...instancesToUpdate, + ...instancesToConfigure, ]); if (context.have) { @@ -43,17 +65,17 @@ export async function deploy(context: Context, options: Options, payload: Payloa // Validate all creates, updates and configures. // Skip validating local extensions, since doing so requires us to create a new source. // No need to validate deletes. - for (const create of payload.instancesToCreate?.filter((i) => !!i.ref) ?? []) { + for (const create of instancesToCreate.filter((i) => !!i.ref)) { const task = tasks.createExtensionInstanceTask(projectId, create, /* validateOnly=*/ true); void validationQueue.run(task); } - for (const update of payload.instancesToUpdate?.filter((i) => !!i.ref) ?? []) { + for (const update of instancesToUpdate.filter((i) => !!i.ref)) { const task = tasks.updateExtensionInstanceTask(projectId, update, /* validateOnly=*/ true); void validationQueue.run(task); } - for (const configure of payload.instancesToConfigure?.filter((i) => !!i.ref) ?? []) { + for (const configure of instancesToConfigure.filter((i) => !!i.ref)) { const task = tasks.configureExtensionInstanceTask( projectId, configure, diff --git a/src/deploy/extensions/prepare.spec.ts b/src/deploy/extensions/prepare.spec.ts index f8f6e8dc2a3..2354e1369fe 100644 --- a/src/deploy/extensions/prepare.spec.ts +++ b/src/deploy/extensions/prepare.spec.ts @@ -52,6 +52,22 @@ describe("Extensions prepare", () => { await expect(prepareDynamicExtensions(context, options, payload, builds)).to.not.be.rejected; }); + it("should leave the payload untouched when nothing is defined and nothing exists", async () => { + // functions/prepare relies on this to tell "no extensions" apart from a real + // plan, so that the deploy and release stages are skipped entirely. + const context: Context = {}; + const payload: Payload = {}; + const options: any = { + config: { + src: { functions: { source: "functions" } }, + }, + }; + + await prepareDynamicExtensions(context, options, payload, {}); + + expect(payload).to.deep.equal({}); + }); + it("should proceed normally if extensions API is healthy", async () => { haveDynamicStub.resolves([ { diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index 7e13b1094e2..85e644a996a 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -279,8 +279,13 @@ export async function prepare( const extContext: ExtContext = {}; const extPayload: ExtPayload = {}; await prepareDynamicExtensions(extContext, options, extPayload, wantBuilds); - context.extensions = extContext; - payload.extensions = extPayload; + // prepareDynamicExtensions returns without touching the payload when there is + // nothing to deploy and nothing to delete. Only hand the extensions stages a + // plan when it actually made one, otherwise they run against an empty payload. + if (Object.keys(extPayload).length) { + context.extensions = extContext; + payload.extensions = extPayload; + } } // == Phase 2. Resolve build to backend. From 2e8d2e2182940f8be2a295e498db40c1fb3c7a1b Mon Sep 17 00:00:00 2001 From: Izaak Gough Date: Wed, 12 Aug 2026 14:20:56 +0100 Subject: [PATCH 2/2] fix(extensions): address review feedback Make the "was a plan prepared" signal explicit rather than inferring it from whether prepareDynamicExtensions happened to populate the payload, so the two modules no longer share an implicit invariant. Drop the delete-only billing exemption. It was not needed to fix the reported problem and was the only behaviour change affecting a real extensions deploy. Correct the comment in extensions/deploy: release guards on the payload fields being absent, not on them being empty, so the two guards are not equivalent. Strengthen the tests. Deploys that should proceed now assert that they reached bulkCheckProductsProvisioned, so widening the empty-payload guard cannot turn a real deploy into a silent no-op, and prepareDynamicExtensions is covered on both the no-plan and the delete-only-plan paths. --- src/deploy/extensions/deploy.spec.ts | 36 +++++++++------ src/deploy/extensions/deploy.ts | 16 +++---- src/deploy/extensions/prepare.spec.ts | 63 ++++++++++++++++++++++++--- src/deploy/extensions/prepare.ts | 18 +++----- src/deploy/functions/prepare.ts | 9 ++-- 5 files changed, 98 insertions(+), 44 deletions(-) diff --git a/src/deploy/extensions/deploy.spec.ts b/src/deploy/extensions/deploy.spec.ts index ea5acb0039b..d291451d60e 100644 --- a/src/deploy/extensions/deploy.spec.ts +++ b/src/deploy/extensions/deploy.spec.ts @@ -11,6 +11,7 @@ describe("Extensions deploy", () => { let bulkCheckProductsProvisionedStub: sinon.SinonStub; const options: any = { nonInteractive: true, project: "test-project" }; + const instance = (instanceId: string) => ({ instanceId, params: {}, systemParams: {} }) as any; beforeEach(() => { checkBillingEnabledStub = sinon.stub(cloudbilling, "checkBillingEnabled").resolves(true); @@ -23,7 +24,7 @@ describe("Extensions deploy", () => { sinon.restore(); }); - it("should not check billing when there is nothing to deploy", async () => { + it("should do nothing when there is nothing to deploy", async () => { // A functions deploy for a codebase that declares no extensions reaches this // stage with an empty payload, and must not require the Cloud Billing API. await deploy({} as Context, options, {} as Payload); @@ -32,7 +33,7 @@ describe("Extensions deploy", () => { expect(bulkCheckProductsProvisionedStub.called).to.be.false; }); - it("should not check billing when the payload only has empty instance lists", async () => { + it("should do nothing when the payload only has empty instance lists", async () => { const payload: Payload = { instancesToCreate: [], instancesToUpdate: [], @@ -43,26 +44,35 @@ describe("Extensions deploy", () => { await deploy({} as Context, options, payload); expect(checkBillingEnabledStub.called).to.be.false; + expect(bulkCheckProductsProvisionedStub.called).to.be.false; }); - it("should not check billing for a delete-only deploy", async () => { - // Deleting an instance does not require the Blaze plan. - const payload: Payload = { - instancesToDelete: [{ instanceId: "doomed", params: {}, systemParams: {} } as any], - }; + it("should deploy normally when there is an instance to create", async () => { + const payload: Payload = { instancesToCreate: [instance("new-instance")] }; await deploy({} as Context, options, payload); - expect(checkBillingEnabledStub.called).to.be.false; + expect(checkBillingEnabledStub.calledWith("test-project")).to.be.true; + // Asserted so that widening the empty-payload guard above cannot silently turn + // a real deploy into a no-op. + expect(bulkCheckProductsProvisionedStub.called).to.be.true; }); - it("should check billing when there is an instance to create", async () => { - const payload: Payload = { - instancesToCreate: [{ instanceId: "new-instance", params: {}, systemParams: {} } as any], - }; + it("should not skip a delete-only deploy", async () => { + const payload: Payload = { instancesToDelete: [instance("doomed")] }; await deploy({} as Context, options, payload); - expect(checkBillingEnabledStub.calledWith("test-project")).to.be.true; + expect(checkBillingEnabledStub.called).to.be.true; + expect(bulkCheckProductsProvisionedStub.called).to.be.true; + }); + + it("should deploy normally when only configuring an instance", async () => { + const payload: Payload = { instancesToConfigure: [instance("existing")] }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.true; + expect(bulkCheckProductsProvisionedStub.called).to.be.true; }); }); diff --git a/src/deploy/extensions/deploy.ts b/src/deploy/extensions/deploy.ts index f5aed598a3f..88d1ddaea42 100644 --- a/src/deploy/extensions/deploy.ts +++ b/src/deploy/extensions/deploy.ts @@ -21,9 +21,10 @@ export async function deploy(context: Context, options: Options, payload: Payloa const instancesToConfigure = payload.instancesToConfigure ?? []; const instancesToDelete = payload.instancesToDelete ?? []; - // Nothing to do. `release` already guards this way; without the same guard here - // a functions deploy that declares no extensions still reaches the billing check - // below, because the SDK always emits an (empty) extensions record. + // Nothing to do. A functions deploy reaches this stage even when the codebase + // declares no extensions, since every codebase reports an extensions record and + // it is empty in that case. Without this guard such a deploy would go on to + // require the Cloud Billing API below. if ( !instancesToCreate.length && !instancesToUpdate.length && @@ -34,13 +35,8 @@ export async function deploy(context: Context, options: Options, payload: Payloa } const projectId = needProjectId(options); - - // First, check that billing is enabled. Creating, updating or configuring an - // instance requires the Blaze plan; deleting one does not, so a delete-only - // deploy doesn't need the Cloud Billing API. - if (instancesToCreate.length || instancesToUpdate.length || instancesToConfigure.length) { - await checkBilling(projectId, options.nonInteractive); - } + // First, check that billing is enabled + await checkBilling(projectId, options.nonInteractive); // Then, check that required products are provisioned. await bulkCheckProductsProvisioned(projectId, [ diff --git a/src/deploy/extensions/prepare.spec.ts b/src/deploy/extensions/prepare.spec.ts index 2354e1369fe..c89940c67b6 100644 --- a/src/deploy/extensions/prepare.spec.ts +++ b/src/deploy/extensions/prepare.spec.ts @@ -52,10 +52,23 @@ describe("Extensions prepare", () => { await expect(prepareDynamicExtensions(context, options, payload, builds)).to.not.be.rejected; }); - it("should leave the payload untouched when nothing is defined and nothing exists", async () => { - // functions/prepare relies on this to tell "no extensions" apart from a real - // plan, so that the deploy and release stages are skipped entirely. - const context: Context = {}; + it("should report no plan if the extensions API is down", async () => { + haveDynamicStub.rejects(new Error("Extensions API is having an outage")); + + const options: any = { + config: { + src: { functions: { source: "functions" } }, + }, + }; + + const prepared = await prepareDynamicExtensions({}, options, {}, {}); + + expect(prepared).to.be.false; + }); + + it("should report no plan when nothing is defined and nothing exists", async () => { + // functions/prepare uses this to tell "no extensions" apart from a real plan, + // so that the deploy and release stages are skipped entirely. const payload: Payload = {}; const options: any = { config: { @@ -63,8 +76,9 @@ describe("Extensions prepare", () => { }, }; - await prepareDynamicExtensions(context, options, payload, {}); + const prepared = await prepareDynamicExtensions({}, options, payload, {}); + expect(prepared).to.be.false; expect(payload).to.deep.equal({}); }); @@ -106,5 +120,44 @@ describe("Extensions prepare", () => { v2apistub.restore(); tosStub.restore(); }); + + it("should report a plan when an existing instance needs deleting", async () => { + haveDynamicStub.resolves([ + { + instanceId: "test-extension", + ref: { publisherId: "test", extensionId: "test", version: "0.1.0" }, + params: {}, + systemParams: {}, + labels: { codebase: "default" }, + }, + ]); + + const payload: Payload = {}; + const options: any = { + config: { + get: () => [], + src: { functions: { source: "functions" } }, + }, + rc: { getEtags: () => [] }, + dryRun: true, + }; + + const wantDynamicStub: sinon.SinonStub = sinon.stub(planner, "wantDynamic").resolves([]); + const v2apistub: sinon.SinonStub = sinon + .stub(v2FunctionHelper, "ensureNecessaryV2ApisAndRoles") + .resolves(); + const tosStub: sinon.SinonStub = sinon + .stub(tos, "getAppDeveloperTOSStatus") + .resolves({ lastAcceptedVersion: "1.0.0" } as any); + + const prepared = await prepareDynamicExtensions({}, options, payload, {}); + + expect(prepared).to.be.true; + expect(payload.instancesToDelete).to.have.length(1); + + wantDynamicStub.restore(); + v2apistub.restore(); + tosStub.restore(); + }); }); }); diff --git a/src/deploy/extensions/prepare.ts b/src/deploy/extensions/prepare.ts index 72a42cf1976..c83d9abbcc5 100644 --- a/src/deploy/extensions/prepare.ts +++ b/src/deploy/extensions/prepare.ts @@ -161,13 +161,15 @@ async function prepareHelper( * @param options The prepare options * @param payload The prepare payload * @param builds firebase functions builds + * @return Whether a deployment plan was prepared. False means the context and + * payload were left untouched, so there is nothing for the caller to deploy. */ export async function prepareDynamicExtensions( context: Context, options: DeployOptions, payload: Payload, builds: Record, -): Promise { +): Promise { const functionsConfig = normalizeAndValidate(options.config.src.functions); const filters = getEndpointFilters(options, functionsConfig); const extensions = extractExtensionsFromBuilds(builds, filters); @@ -189,12 +191,12 @@ export async function prepareDynamicExtensions( "Failed to fetch the list of extensions. Assuming for now that there are no existing extensions. " + "If you are trying to install an extension through Firebase Functions this may fail later.", ); - return; + return false; } if (Object.keys(extensions).length === 0 && haveExtensions.length === 0) { // Nothing defined, and nothing to delete - return; + return false; } const dynamicWant = await planner.wantDynamic({ @@ -203,14 +205,8 @@ export async function prepareDynamicExtensions( extensions, }); - return prepareHelper( - context, - options, - payload, - dynamicWant, - haveExtensions, - true /* isDynamic */, - ); + await prepareHelper(context, options, payload, dynamicWant, haveExtensions, true /* isDynamic */); + return true; } /** diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index 85e644a996a..14a4f93fe34 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -278,11 +278,10 @@ export async function prepare( if (Object.values(wantBuilds).some((b) => b.extensions)) { const extContext: ExtContext = {}; const extPayload: ExtPayload = {}; - await prepareDynamicExtensions(extContext, options, extPayload, wantBuilds); - // prepareDynamicExtensions returns without touching the payload when there is - // nothing to deploy and nothing to delete. Only hand the extensions stages a - // plan when it actually made one, otherwise they run against an empty payload. - if (Object.keys(extPayload).length) { + // Every codebase reports an extensions record, empty when it declares none, so + // reaching here does not mean there is anything to deploy. Only hand the + // extensions stages a plan when one was actually prepared. + if (await prepareDynamicExtensions(extContext, options, extPayload, wantBuilds)) { context.extensions = extContext; payload.extensions = extPayload; }