From e0d6028cdd9cc4dad2cdbc1ee38966c3a8bed4df Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Fri, 11 Jul 2025 07:03:57 -0700 Subject: [PATCH 01/18] feat: add stdio-based function discovery mode Adds an alternative discovery mechanism that outputs function manifests via stderr instead of starting an HTTP server. This improves reliability by avoiding issues where module loading blocks the HTTP endpoint from becoming available. When FUNCTIONS_DISCOVERY_MODE=stdio is set: - Outputs base64-encoded manifest to stderr with __FIREBASE_FUNCTIONS_MANIFEST__: prefix - Outputs errors to stderr with __FIREBASE_FUNCTIONS_MANIFEST_ERROR__: prefix - Exits immediately without starting HTTP server - Maintains backward compatibility (HTTP remains default) Includes comprehensive tests that verify both HTTP and stdio discovery work correctly for all test cases (commonjs, esm, various configurations). --- scripts/bin-test/test.ts | 112 +++++++++++++++++++++++++++++++++- src/bin/firebase-functions.ts | 71 +++++++++++++-------- 2 files changed, 154 insertions(+), 29 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 85b2eb249..e7457c46a 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -199,11 +199,46 @@ async function startBin( }; } +async function runStdioDiscovery( + modulePath: string +): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { + return new Promise((resolve, reject) => { + const proc = subprocess.spawn("npx", ["firebase-functions"], { + cwd: path.resolve(modulePath), + env: { + PATH: process.env.PATH, + GCLOUD_PROJECT: "test-project", + FUNCTIONS_CONTROL_API: "true", + FUNCTIONS_DISCOVERY_MODE: "stdio", + }, + }); + + let stdout = ""; + let stderr = ""; + + proc.stdout?.on("data", (chunk: Buffer) => { + stdout += chunk.toString("utf8"); + }); + + proc.stderr?.on("data", (chunk: Buffer) => { + stderr += chunk.toString("utf8"); + }); + + proc.on("close", (code) => { + resolve({ stdout, stderr, exitCode: code }); + }); + + proc.on("error", (err) => { + reject(err); + }); + }); +} + describe("functions.yaml", function () { // eslint-disable-next-line @typescript-eslint/no-invalid-this this.timeout(TIMEOUT_XL); - function runTests(tc: Testcase) { + function runHttpDiscoveryTests(tc: Testcase) { let port: number; let cleanup: () => Promise; @@ -233,6 +268,31 @@ describe("functions.yaml", function () { }); } + function runStdioDiscoveryTests(tc: Testcase) { + it("discovers functions via stdio", async function () { + // eslint-disable-next-line @typescript-eslint/no-invalid-this + this.timeout(TIMEOUT_M); + + const result = await runStdioDiscovery(tc.modulePath); + + // Should exit successfully + expect(result.exitCode).to.equal(0); + + // Should not start HTTP server + expect(result.stdout).to.not.contain("Serving at port"); + + // Should output manifest to stderr + const manifestMatch = result.stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:(.+)/); + expect(manifestMatch).to.not.be.null; + + // Decode and verify manifest + const base64 = manifestMatch![1]; + const manifestJson = Buffer.from(base64, "base64").toString("utf8"); + const manifest = JSON.parse(manifestJson); + expect(manifest).to.deep.equal(tc.expected); + }); + } + describe("commonjs", function () { // eslint-disable-next-line @typescript-eslint/no-invalid-this this.timeout(TIMEOUT_L); @@ -320,7 +380,13 @@ describe("functions.yaml", function () { for (const tc of testcases) { describe(tc.name, () => { - runTests(tc); + describe("http discovery", () => { + runHttpDiscoveryTests(tc); + }); + + describe("stdio discovery", () => { + runStdioDiscoveryTests(tc); + }); }); } }); @@ -350,8 +416,48 @@ describe("functions.yaml", function () { for (const tc of testcases) { describe(tc.name, () => { - runTests(tc); + describe("http discovery", () => { + runHttpDiscoveryTests(tc); + }); + + describe("stdio discovery", () => { + runStdioDiscoveryTests(tc); + }); }); } }); + + describe("stdio discovery error handling", function () { + it("outputs error for broken module", async function () { + // Create a temporary broken module + const fs = require("fs"); + const brokenModulePath = path.join(__dirname, "temp-broken-module"); + + try { + // Create directory and files + fs.mkdirSync(brokenModulePath, { recursive: true }); + fs.writeFileSync( + path.join(brokenModulePath, "package.json"), + JSON.stringify({ name: "broken-module", main: "index.js" }) + ); + fs.writeFileSync( + path.join(brokenModulePath, "index.js"), + "const functions = require('firebase-functions');\nsyntax error here" + ); + + const result = await runStdioDiscovery(brokenModulePath); + + // Should exit with error + expect(result.exitCode).to.equal(1); + + // Should output error to stderr + const errorMatch = result.stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:(.+)/); + expect(errorMatch).to.not.be.null; + expect(errorMatch![1]).to.contain("Unexpected identifier"); + } finally { + // Cleanup + fs.rmSync(brokenModulePath, { recursive: true, force: true }); + } + }); + }); }); diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index d04d09fb8..35a26545d 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -49,34 +49,53 @@ if (args.length > 1) { functionsDir = args[0]; } -let server: http.Server = undefined; -const app = express(); - -function handleQuitquitquit(req: express.Request, res: express.Response) { - res.send("ok"); - server.close(); +async function runStdioDiscovery() { + try { + const stack = await loadStack(functionsDir); + const wireFormat = stackToWire(stack); + const manifestJson = JSON.stringify(wireFormat); + const base64 = Buffer.from(manifestJson).toString("base64"); + process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST__:${base64}\n`); + process.exit(0); + } catch (e) { + console.error("Failed to generate manifest from function source:", e); + process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:${e.message}\n`); + process.exit(1); + } } -app.get("/__/quitquitquit", handleQuitquitquit); -app.post("/__/quitquitquit", handleQuitquitquit); +if (process.env.FUNCTIONS_CONTROL_API === "true" && process.env.FUNCTIONS_DISCOVERY_MODE === "stdio") { + runStdioDiscovery(); +} else { + let server: http.Server = undefined; + const app = express(); -if (process.env.FUNCTIONS_CONTROL_API === "true") { - app.get("/__/functions.yaml", async (req, res) => { - try { - const stack = await loadStack(functionsDir); - res.setHeader("content-type", "text/yaml"); - res.send(JSON.stringify(stackToWire(stack))); - } catch (e) { - console.error(e); - res.status(400).send(`Failed to generate manifest from function source: ${e}`); - } - }); -} + function handleQuitquitquit(req: express.Request, res: express.Response) { + res.send("ok"); + server.close(); + } -let port = 8080; -if (process.env.PORT) { - port = Number.parseInt(process.env.PORT); -} + app.get("/__/quitquitquit", handleQuitquitquit); + app.post("/__/quitquitquit", handleQuitquitquit); -console.log("Serving at port", port); -server = app.listen(port); + if (process.env.FUNCTIONS_CONTROL_API === "true") { + app.get("/__/functions.yaml", async (req, res) => { + try { + const stack = await loadStack(functionsDir); + res.setHeader("content-type", "text/yaml"); + res.send(JSON.stringify(stackToWire(stack))); + } catch (e) { + console.error(e); + res.status(400).send(`Failed to generate manifest from function source: ${e}`); + } + }); + } + + let port = 8080; + if (process.env.PORT) { + port = Number.parseInt(process.env.PORT); + } + + console.log("Serving at port", port); + server = app.listen(port); +} From 62e37ba4226d239fffa859b5c5c73c8c57ce0ba1 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Fri, 11 Jul 2025 07:48:57 -0700 Subject: [PATCH 02/18] test: add comprehensive tests for stdio discovery - Add test fixture for broken syntax error handling - Refactor tests to use unified DiscoveryResult interface - Parameterize tests with nested loops to test all cases with both discovery methods - Add error handling tests for both HTTP and stdio discovery - All stdio discovery tests passing (16/16) --- .../bin-test/sources/broken-syntax/index.js | 6 + .../sources/broken-syntax/package-lock.json | 81 ++++++ .../sources/broken-syntax/package.json | 7 + scripts/bin-test/test.ts | 260 +++++++----------- 4 files changed, 199 insertions(+), 155 deletions(-) create mode 100644 scripts/bin-test/sources/broken-syntax/index.js create mode 100644 scripts/bin-test/sources/broken-syntax/package-lock.json create mode 100644 scripts/bin-test/sources/broken-syntax/package.json diff --git a/scripts/bin-test/sources/broken-syntax/index.js b/scripts/bin-test/sources/broken-syntax/index.js new file mode 100644 index 000000000..05cbeaa10 --- /dev/null +++ b/scripts/bin-test/sources/broken-syntax/index.js @@ -0,0 +1,6 @@ +const functions = require("firebase-functions"); + +// This will cause a syntax error +exports.broken = functions.https.onRequest((request, response) => { + response.send("Hello from Firebase!" +}); // Missing closing parenthesis \ No newline at end of file diff --git a/scripts/bin-test/sources/broken-syntax/package-lock.json b/scripts/bin-test/sources/broken-syntax/package-lock.json new file mode 100644 index 000000000..cfb88c358 --- /dev/null +++ b/scripts/bin-test/sources/broken-syntax/package-lock.json @@ -0,0 +1,81 @@ +{ + "name": "broken-syntax", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "name": "broken-syntax", + "dependencies": { + "firebase-functions": "file:../../../.." + } + }, + "../../../..": { + "version": "6.3.1", + "license": "MIT", + "dependencies": { + "@types/cors": "^2.8.5", + "@types/express": "^4.17.21", + "cors": "^2.8.5", + "express": "^4.21.0", + "protobufjs": "^7.2.2" + }, + "bin": { + "firebase-functions": "lib/bin/firebase-functions.js" + }, + "devDependencies": { + "@firebase/api-documenter": "^0.2.0", + "@microsoft/api-documenter": "^7.13.45", + "@microsoft/api-extractor": "^7.18.7", + "@types/chai": "^4.1.7", + "@types/chai-as-promised": "^7.1.0", + "@types/jsonwebtoken": "^9.0.0", + "@types/mocha": "^5.2.7", + "@types/mock-require": "^2.0.0", + "@types/nock": "^10.0.3", + "@types/node": "^14.18.24", + "@types/node-fetch": "^3.0.3", + "@types/sinon": "^9.0.11", + "@typescript-eslint/eslint-plugin": "^5.33.1", + "@typescript-eslint/parser": "^5.33.1", + "api-extractor-model-me": "^0.1.1", + "chai": "^4.2.0", + "chai-as-promised": "^7.1.1", + "child-process-promise": "^2.2.1", + "eslint": "^8.6.0", + "eslint-config-google": "^0.14.0", + "eslint-config-prettier": "^8.3.0", + "eslint-plugin-jsdoc": "^39.2.9", + "eslint-plugin-prettier": "^4.0.0", + "firebase-admin": "^13.0.0", + "genkit": "^1.0.0-rc.4", + "js-yaml": "^3.13.1", + "jsdom": "^16.2.1", + "jsonwebtoken": "^9.0.0", + "jwk-to-pem": "^2.0.5", + "mocha": "^10.2.0", + "mock-require": "^3.0.3", + "mz": "^2.7.0", + "nock": "^13.2.9", + "node-fetch": "^2.6.7", + "portfinder": "^1.0.28", + "prettier": "^2.7.1", + "protobufjs-cli": "^1.1.1", + "semver": "^7.3.5", + "sinon": "^9.2.4", + "ts-node": "^10.4.0", + "typescript": "^4.3.5", + "yargs": "^15.3.1" + }, + "engines": { + "node": ">=14.10.0" + }, + "peerDependencies": { + "firebase-admin": "^11.10.0 || ^12.0.0 || ^13.0.0" + } + }, + "node_modules/firebase-functions": { + "resolved": "../../../..", + "link": true + } + } +} diff --git a/scripts/bin-test/sources/broken-syntax/package.json b/scripts/bin-test/sources/broken-syntax/package.json new file mode 100644 index 000000000..f3fbb2f71 --- /dev/null +++ b/scripts/bin-test/sources/broken-syntax/package.json @@ -0,0 +1,7 @@ +{ + "name": "broken-syntax", + "main": "index.js", + "dependencies": { + "firebase-functions": "file:../../../.." + } +} \ No newline at end of file diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index e7457c46a..c7550f441 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -105,7 +105,13 @@ const BASE_STACK = { interface Testcase { name: string; modulePath: string; - expected: Record; + expected: Record; +} + +interface DiscoveryResult { + success: boolean; + manifest?: Record; + error?: string; } async function retryUntil( @@ -134,74 +140,48 @@ async function retryUntil( await Promise.race([retry, timedOut]); } -async function startBin( - tc: Testcase, - debug?: boolean -): Promise<{ port: number; cleanup: () => Promise }> { + +async function runHttpDiscovery(modulePath: string): Promise { const getPort = promisify(portfinder.getPort) as () => Promise; const port = await getPort(); const proc = subprocess.spawn("npx", ["firebase-functions"], { - cwd: path.resolve(tc.modulePath), + cwd: path.resolve(modulePath), env: { PATH: process.env.PATH, - GLCOUD_PROJECT: "test-project", + GCLOUD_PROJECT: "test-project", PORT: port.toString(), FUNCTIONS_CONTROL_API: "true", }, }); - if (!proc) { - throw new Error("Failed to start firebase functions"); - } - proc.stdout?.on("data", (chunk: Buffer) => { - console.log(chunk.toString("utf8")); - }); - proc.stderr?.on("data", (chunk: Buffer) => { - console.log(chunk.toString("utf8")); - }); - await retryUntil(async () => { - try { - await fetch(`http://localhost:${port}/__/functions.yaml`); - } catch (e) { - if (e?.code === "ECONNREFUSED") { - return false; + try { + // Wait for server to be ready + await retryUntil(async () => { + try { + await fetch(`http://localhost:${port}/__/functions.yaml`); + return true; + } catch (e: unknown) { + const error = e as { code?: string }; + return error.code !== "ECONNREFUSED"; } - throw e; + }, TIMEOUT_L); + + const res = await fetch(`http://localhost:${port}/__/functions.yaml`); + const body = await res.text(); + + if (res.status === 200) { + const manifest = yaml.load(body) as Record; + return { success: true, manifest }; + } else { + return { success: false, error: body }; } - return true; - }, TIMEOUT_L); - - if (debug) { - proc.stdout?.on("data", (data: unknown) => { - console.log(`[${tc.name} stdout] ${data}`); - }); - - proc.stderr?.on("data", (data: unknown) => { - console.log(`[${tc.name} stderr] ${data}`); - }); + } finally { + proc.kill(9); } - - return { - port, - cleanup: async () => { - process.kill(proc.pid, 9); - await retryUntil(async () => { - try { - process.kill(proc.pid, 0); - } catch { - // process.kill w/ signal 0 will throw an error if the pid no longer exists. - return Promise.resolve(true); - } - return Promise.resolve(false); - }, TIMEOUT_L); - }, - }; } -async function runStdioDiscovery( - modulePath: string -): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { +async function runStdioDiscovery(modulePath: string): Promise { return new Promise((resolve, reject) => { const proc = subprocess.spawn("npx", ["firebase-functions"], { cwd: path.resolve(modulePath), @@ -213,19 +193,31 @@ async function runStdioDiscovery( }, }); - let stdout = ""; let stderr = ""; - proc.stdout?.on("data", (chunk: Buffer) => { - stdout += chunk.toString("utf8"); - }); - proc.stderr?.on("data", (chunk: Buffer) => { stderr += chunk.toString("utf8"); }); - proc.on("close", (code) => { - resolve({ stdout, stderr, exitCode: code }); + proc.on("close", () => { + // Try to parse manifest + const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:(.+)/); + if (manifestMatch) { + const base64 = manifestMatch[1]; + const manifestJson = Buffer.from(base64, "base64").toString("utf8"); + const manifest = JSON.parse(manifestJson) as Record; + resolve({ success: true, manifest }); + return; + } + + // Try to parse error + const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:(.+)/); + if (errorMatch) { + resolve({ success: false, error: errorMatch[1] }); + return; + } + + resolve({ success: false, error: "No manifest or error found" }); }); proc.on("error", (err) => { @@ -238,58 +230,17 @@ describe("functions.yaml", function () { // eslint-disable-next-line @typescript-eslint/no-invalid-this this.timeout(TIMEOUT_XL); - function runHttpDiscoveryTests(tc: Testcase) { - let port: number; - let cleanup: () => Promise; - - before(async () => { - const r = await startBin(tc); - port = r.port; - cleanup = r.cleanup; - }); - - after(async () => { - await cleanup?.(); - }); - - it("functions.yaml returns expected Manifest", async function () { - // eslint-disable-next-line @typescript-eslint/no-invalid-this - this.timeout(TIMEOUT_M); - - const res = await fetch(`http://localhost:${port}/__/functions.yaml`); - const text = await res.text(); - let parsed: any; - try { - parsed = yaml.load(text); - } catch (err) { - throw new Error(`Failed to parse functions.yaml: ${err}`); - } - expect(parsed).to.be.deep.equal(tc.expected); - }); - } - - function runStdioDiscoveryTests(tc: Testcase) { - it("discovers functions via stdio", async function () { + function runDiscoveryTests( + tc: Testcase, + discoveryFn: (path: string) => Promise + ) { + it("returns expected manifest", async function () { // eslint-disable-next-line @typescript-eslint/no-invalid-this this.timeout(TIMEOUT_M); - const result = await runStdioDiscovery(tc.modulePath); - - // Should exit successfully - expect(result.exitCode).to.equal(0); - - // Should not start HTTP server - expect(result.stdout).to.not.contain("Serving at port"); - - // Should output manifest to stderr - const manifestMatch = result.stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:(.+)/); - expect(manifestMatch).to.not.be.null; - - // Decode and verify manifest - const base64 = manifestMatch![1]; - const manifestJson = Buffer.from(base64, "base64").toString("utf8"); - const manifest = JSON.parse(manifestJson); - expect(manifest).to.deep.equal(tc.expected); + const result = await discoveryFn(tc.modulePath); + expect(result.success).to.be.true; + expect(result.manifest).to.deep.equal(tc.expected); }); } @@ -378,15 +329,18 @@ describe("functions.yaml", function () { }, ]; + const discoveryMethods = [ + { name: "http", fn: runHttpDiscovery }, + { name: "stdio", fn: runStdioDiscovery }, + ]; + for (const tc of testcases) { describe(tc.name, () => { - describe("http discovery", () => { - runHttpDiscoveryTests(tc); - }); - - describe("stdio discovery", () => { - runStdioDiscoveryTests(tc); - }); + for (const discovery of discoveryMethods) { + describe(`${discovery.name} discovery`, () => { + runDiscoveryTests(tc, discovery.fn); + }); + } }); } }); @@ -414,50 +368,46 @@ describe("functions.yaml", function () { }, ]; + const discoveryMethods = [ + { name: "http", fn: runHttpDiscovery }, + { name: "stdio", fn: runStdioDiscovery }, + ]; + for (const tc of testcases) { describe(tc.name, () => { - describe("http discovery", () => { - runHttpDiscoveryTests(tc); - }); - - describe("stdio discovery", () => { - runStdioDiscoveryTests(tc); - }); + for (const discovery of discoveryMethods) { + describe(`${discovery.name} discovery`, () => { + runDiscoveryTests(tc, discovery.fn); + }); + } }); } }); - describe("stdio discovery error handling", function () { - it("outputs error for broken module", async function () { - // Create a temporary broken module - const fs = require("fs"); - const brokenModulePath = path.join(__dirname, "temp-broken-module"); - - try { - // Create directory and files - fs.mkdirSync(brokenModulePath, { recursive: true }); - fs.writeFileSync( - path.join(brokenModulePath, "package.json"), - JSON.stringify({ name: "broken-module", main: "index.js" }) - ); - fs.writeFileSync( - path.join(brokenModulePath, "index.js"), - "const functions = require('firebase-functions');\nsyntax error here" - ); - - const result = await runStdioDiscovery(brokenModulePath); - - // Should exit with error - expect(result.exitCode).to.equal(1); - - // Should output error to stderr - const errorMatch = result.stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:(.+)/); - expect(errorMatch).to.not.be.null; - expect(errorMatch![1]).to.contain("Unexpected identifier"); - } finally { - // Cleanup - fs.rmSync(brokenModulePath, { recursive: true, force: true }); - } - }); + describe("error handling", function () { + const errorTestcases = [ + { + name: "broken syntax", + modulePath: "./scripts/bin-test/sources/broken-syntax", + expectedError: "missing ) after argument list", + }, + ]; + + const discoveryMethods = [ + { name: "http", fn: runHttpDiscovery }, + { name: "stdio", fn: runStdioDiscovery }, + ]; + + for (const tc of errorTestcases) { + describe(tc.name, () => { + for (const discovery of discoveryMethods) { + it(`${discovery.name} discovery handles error correctly`, async function () { + const result = await discovery.fn(tc.modulePath); + expect(result.success).to.be.false; + expect(result.error).to.include(tc.expectedError); + }); + } + }); + } }); }); From 0c2da1f97dd5aa7a485f224d6cca3b312118bd6d Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Fri, 11 Jul 2025 08:20:57 -0700 Subject: [PATCH 03/18] fix: resolve linting errors in firebase-functions.ts - Mark runStdioDiscovery() promise with void operator - Move handleQuitquitquit function to program root - Fix TypeScript warnings for error handling - Add eslint-disable comment for Express async handler --- src/bin/firebase-functions.ts | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index 35a26545d..6ea3dfe6f 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -59,26 +59,31 @@ async function runStdioDiscovery() { process.exit(0); } catch (e) { console.error("Failed to generate manifest from function source:", e); - process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:${e.message}\n`); + const errorMessage = e instanceof Error ? e.message : String(e); + process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:${errorMessage}\n`); process.exit(1); } } -if (process.env.FUNCTIONS_CONTROL_API === "true" && process.env.FUNCTIONS_DISCOVERY_MODE === "stdio") { - runStdioDiscovery(); +function handleQuitquitquit(req: express.Request, res: express.Response, server: http.Server) { + res.send("ok"); + server.close(); +} + +if ( + process.env.FUNCTIONS_CONTROL_API === "true" && + process.env.FUNCTIONS_DISCOVERY_MODE === "stdio" +) { + void runStdioDiscovery(); } else { let server: http.Server = undefined; const app = express(); - function handleQuitquitquit(req: express.Request, res: express.Response) { - res.send("ok"); - server.close(); - } - - app.get("/__/quitquitquit", handleQuitquitquit); - app.post("/__/quitquitquit", handleQuitquitquit); + app.get("/__/quitquitquit", (req, res) => handleQuitquitquit(req, res, server)); + app.post("/__/quitquitquit", (req, res) => handleQuitquitquit(req, res, server)); if (process.env.FUNCTIONS_CONTROL_API === "true") { + // eslint-disable-next-line @typescript-eslint/no-misused-promises app.get("/__/functions.yaml", async (req, res) => { try { const stack = await loadStack(functionsDir); @@ -86,7 +91,8 @@ if (process.env.FUNCTIONS_CONTROL_API === "true" && process.env.FUNCTIONS_DISCOV res.send(JSON.stringify(stackToWire(stack))); } catch (e) { console.error(e); - res.status(400).send(`Failed to generate manifest from function source: ${e}`); + const errorMessage = e instanceof Error ? e.message : String(e); + res.status(400).send(`Failed to generate manifest from function source: ${errorMessage}`); } }); } From a5f0b05cc0af71afb2a9ee152f2be4d242710596 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 11:40:24 -0700 Subject: [PATCH 04/18] fix: address PR review comments for stdio discovery - Add proper error handling for non-Error objects - Define MANIFEST_PREFIX and MANIFEST_ERROR_PREFIX constants - Fix HTTP discovery retry logic regression in tests - Make regex patterns multiline-safe for manifest and error parsing - Add timeout handling for stdio discovery in tests --- scripts/bin-test/test.ts | 18 +++++++++++++++--- src/bin/firebase-functions.ts | 9 ++++++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index c7550f441..1a1fc09bb 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -163,7 +163,12 @@ async function runHttpDiscovery(modulePath: string): Promise { return true; } catch (e: unknown) { const error = e as { code?: string }; - return error.code !== "ECONNREFUSED"; + if (error.code === "ECONNREFUSED") { + // This is an expected error during server startup, so we should retry. + return false; + } + // Any other error is unexpected and should fail the test immediately. + throw e; } }, TIMEOUT_L); @@ -199,9 +204,15 @@ async function runStdioDiscovery(modulePath: string): Promise { stderr += chunk.toString("utf8"); }); + const timeoutId = setTimeout(() => { + proc.kill(9); + reject(new Error("Stdio discovery timed out after " + TIMEOUT_M + "ms")); + }, TIMEOUT_M); + proc.on("close", () => { + clearTimeout(timeoutId); // Try to parse manifest - const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:(.+)/); + const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:([\s\S]+)/); if (manifestMatch) { const base64 = manifestMatch[1]; const manifestJson = Buffer.from(base64, "base64").toString("utf8"); @@ -211,7 +222,7 @@ async function runStdioDiscovery(modulePath: string): Promise { } // Try to parse error - const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:(.+)/); + const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:([\s\S]+)/); if (errorMatch) { resolve({ success: false, error: errorMatch[1] }); return; @@ -221,6 +232,7 @@ async function runStdioDiscovery(modulePath: string): Promise { }); proc.on("error", (err) => { + clearTimeout(timeoutId); reject(err); }); }); diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index 6ea3dfe6f..1c0ba6aa5 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -49,18 +49,21 @@ if (args.length > 1) { functionsDir = args[0]; } +const MANIFEST_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST__:"; +const MANIFEST_ERROR_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:"; + async function runStdioDiscovery() { try { const stack = await loadStack(functionsDir); const wireFormat = stackToWire(stack); const manifestJson = JSON.stringify(wireFormat); const base64 = Buffer.from(manifestJson).toString("base64"); - process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST__:${base64}\n`); + process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); process.exit(0); } catch (e) { console.error("Failed to generate manifest from function source:", e); - const errorMessage = e instanceof Error ? e.message : String(e); - process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:${errorMessage}\n`); + const message = e instanceof Error ? e.message : String(e); + process.stderr.write(`${MANIFEST_ERROR_PREFIX}${message}\n`); process.exit(1); } } From 08aca82b1d09a42c03351806012b513de5968985 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 11:42:41 -0700 Subject: [PATCH 05/18] fix: resolve linting errors --- scripts/bin-test/test.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 1a1fc09bb..62c0c8aa4 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -140,7 +140,6 @@ async function retryUntil( await Promise.race([retry, timedOut]); } - async function runHttpDiscovery(modulePath: string): Promise { const getPort = promisify(portfinder.getPort) as () => Promise; const port = await getPort(); @@ -174,7 +173,7 @@ async function runHttpDiscovery(modulePath: string): Promise { const res = await fetch(`http://localhost:${port}/__/functions.yaml`); const body = await res.text(); - + if (res.status === 200) { const manifest = yaml.load(body) as Record; return { success: true, manifest }; @@ -206,7 +205,7 @@ async function runStdioDiscovery(modulePath: string): Promise { const timeoutId = setTimeout(() => { proc.kill(9); - reject(new Error("Stdio discovery timed out after " + TIMEOUT_M + "ms")); + reject(new Error(`Stdio discovery timed out after ${TIMEOUT_M}ms`)); }, TIMEOUT_M); proc.on("close", () => { @@ -220,14 +219,14 @@ async function runStdioDiscovery(modulePath: string): Promise { resolve({ success: true, manifest }); return; } - + // Try to parse error const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:([\s\S]+)/); if (errorMatch) { resolve({ success: false, error: errorMatch[1] }); return; } - + resolve({ success: false, error: "No manifest or error found" }); }); @@ -243,7 +242,7 @@ describe("functions.yaml", function () { this.timeout(TIMEOUT_XL); function runDiscoveryTests( - tc: Testcase, + tc: Testcase, discoveryFn: (path: string) => Promise ) { it("returns expected manifest", async function () { @@ -395,8 +394,8 @@ describe("functions.yaml", function () { }); } }); - - describe("error handling", function () { + + describe("error handling", () => { const errorTestcases = [ { name: "broken syntax", @@ -413,7 +412,7 @@ describe("functions.yaml", function () { for (const tc of errorTestcases) { describe(tc.name, () => { for (const discovery of discoveryMethods) { - it(`${discovery.name} discovery handles error correctly`, async function () { + it(`${discovery.name} discovery handles error correctly`, async () => { const result = await discovery.fn(tc.modulePath); expect(result.success).to.be.false; expect(result.error).to.include(tc.expectedError); From 0222d2f4198b26b1a2af107169d348b8c05c826e Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 11:47:21 -0700 Subject: [PATCH 06/18] fix: clean up broken-syntax fixture to match other fixtures - Remove unnecessary dependencies from package.json - Remove package-lock.json - The npm link is handled by run.sh script --- .../sources/broken-syntax/package-lock.json | 81 ------------------- .../sources/broken-syntax/package.json | 6 +- 2 files changed, 1 insertion(+), 86 deletions(-) delete mode 100644 scripts/bin-test/sources/broken-syntax/package-lock.json diff --git a/scripts/bin-test/sources/broken-syntax/package-lock.json b/scripts/bin-test/sources/broken-syntax/package-lock.json deleted file mode 100644 index cfb88c358..000000000 --- a/scripts/bin-test/sources/broken-syntax/package-lock.json +++ /dev/null @@ -1,81 +0,0 @@ -{ - "name": "broken-syntax", - "lockfileVersion": 3, - "requires": true, - "packages": { - "": { - "name": "broken-syntax", - "dependencies": { - "firebase-functions": "file:../../../.." - } - }, - "../../../..": { - "version": "6.3.1", - "license": "MIT", - "dependencies": { - "@types/cors": "^2.8.5", - "@types/express": "^4.17.21", - "cors": "^2.8.5", - "express": "^4.21.0", - "protobufjs": "^7.2.2" - }, - "bin": { - "firebase-functions": "lib/bin/firebase-functions.js" - }, - "devDependencies": { - "@firebase/api-documenter": "^0.2.0", - "@microsoft/api-documenter": "^7.13.45", - "@microsoft/api-extractor": "^7.18.7", - "@types/chai": "^4.1.7", - "@types/chai-as-promised": "^7.1.0", - "@types/jsonwebtoken": "^9.0.0", - "@types/mocha": "^5.2.7", - "@types/mock-require": "^2.0.0", - "@types/nock": "^10.0.3", - "@types/node": "^14.18.24", - "@types/node-fetch": "^3.0.3", - "@types/sinon": "^9.0.11", - "@typescript-eslint/eslint-plugin": "^5.33.1", - "@typescript-eslint/parser": "^5.33.1", - "api-extractor-model-me": "^0.1.1", - "chai": "^4.2.0", - "chai-as-promised": "^7.1.1", - "child-process-promise": "^2.2.1", - "eslint": "^8.6.0", - "eslint-config-google": "^0.14.0", - "eslint-config-prettier": "^8.3.0", - "eslint-plugin-jsdoc": "^39.2.9", - "eslint-plugin-prettier": "^4.0.0", - "firebase-admin": "^13.0.0", - "genkit": "^1.0.0-rc.4", - "js-yaml": "^3.13.1", - "jsdom": "^16.2.1", - "jsonwebtoken": "^9.0.0", - "jwk-to-pem": "^2.0.5", - "mocha": "^10.2.0", - "mock-require": "^3.0.3", - "mz": "^2.7.0", - "nock": "^13.2.9", - "node-fetch": "^2.6.7", - "portfinder": "^1.0.28", - "prettier": "^2.7.1", - "protobufjs-cli": "^1.1.1", - "semver": "^7.3.5", - "sinon": "^9.2.4", - "ts-node": "^10.4.0", - "typescript": "^4.3.5", - "yargs": "^15.3.1" - }, - "engines": { - "node": ">=14.10.0" - }, - "peerDependencies": { - "firebase-admin": "^11.10.0 || ^12.0.0 || ^13.0.0" - } - }, - "node_modules/firebase-functions": { - "resolved": "../../../..", - "link": true - } - } -} diff --git a/scripts/bin-test/sources/broken-syntax/package.json b/scripts/bin-test/sources/broken-syntax/package.json index f3fbb2f71..bfada79a1 100644 --- a/scripts/bin-test/sources/broken-syntax/package.json +++ b/scripts/bin-test/sources/broken-syntax/package.json @@ -1,7 +1,3 @@ { - "name": "broken-syntax", - "main": "index.js", - "dependencies": { - "firebase-functions": "file:../../../.." - } + "name": "broken-syntax" } \ No newline at end of file From 900d31660b3c5848e356aa6c29a03da6e36fd8fd Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 12:18:16 -0700 Subject: [PATCH 07/18] fix: address additional PR review comments - Use process.exitCode instead of process.exit() to prevent race conditions with stderr buffer flushing - Refactor test to define discoveryMethods array once to reduce code duplication --- scripts/bin-test/test.ts | 20 +++++--------------- src/bin/firebase-functions.ts | 4 ++-- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 62c0c8aa4..c3ab7d9bf 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -241,6 +241,11 @@ describe("functions.yaml", function () { // eslint-disable-next-line @typescript-eslint/no-invalid-this this.timeout(TIMEOUT_XL); + const discoveryMethods = [ + { name: "http", fn: runHttpDiscovery }, + { name: "stdio", fn: runStdioDiscovery }, + ]; + function runDiscoveryTests( tc: Testcase, discoveryFn: (path: string) => Promise @@ -340,11 +345,6 @@ describe("functions.yaml", function () { }, ]; - const discoveryMethods = [ - { name: "http", fn: runHttpDiscovery }, - { name: "stdio", fn: runStdioDiscovery }, - ]; - for (const tc of testcases) { describe(tc.name, () => { for (const discovery of discoveryMethods) { @@ -379,11 +379,6 @@ describe("functions.yaml", function () { }, ]; - const discoveryMethods = [ - { name: "http", fn: runHttpDiscovery }, - { name: "stdio", fn: runStdioDiscovery }, - ]; - for (const tc of testcases) { describe(tc.name, () => { for (const discovery of discoveryMethods) { @@ -404,11 +399,6 @@ describe("functions.yaml", function () { }, ]; - const discoveryMethods = [ - { name: "http", fn: runHttpDiscovery }, - { name: "stdio", fn: runStdioDiscovery }, - ]; - for (const tc of errorTestcases) { describe(tc.name, () => { for (const discovery of discoveryMethods) { diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index 1c0ba6aa5..e422d8c2d 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -59,12 +59,12 @@ async function runStdioDiscovery() { const manifestJson = JSON.stringify(wireFormat); const base64 = Buffer.from(manifestJson).toString("base64"); process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); - process.exit(0); + process.exitCode = 0; } catch (e) { console.error("Failed to generate manifest from function source:", e); const message = e instanceof Error ? e.message : String(e); process.stderr.write(`${MANIFEST_ERROR_PREFIX}${message}\n`); - process.exit(1); + process.exitCode = 1; } } From d48b3a23fa7901f66642614b68621aa86c5eb413 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 12:20:41 -0700 Subject: [PATCH 08/18] remove extraneous comments --- scripts/bin-test/test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index c3ab7d9bf..722b9e979 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -130,7 +130,7 @@ async function retryUntil( }, timeoutMs); }); const retry = (async () => { - for (;;) { + for (; ;) { if (await fn()) { break; } @@ -210,7 +210,6 @@ async function runStdioDiscovery(modulePath: string): Promise { proc.on("close", () => { clearTimeout(timeoutId); - // Try to parse manifest const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:([\s\S]+)/); if (manifestMatch) { const base64 = manifestMatch[1]; @@ -220,7 +219,6 @@ async function runStdioDiscovery(modulePath: string): Promise { return; } - // Try to parse error const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:([\s\S]+)/); if (errorMatch) { resolve({ success: false, error: errorMatch[1] }); From 088534ae68b4a98a5df249aa3372eb0c4d1d419e Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 14:09:02 -0700 Subject: [PATCH 09/18] fix: resolve prettier formatting issue --- scripts/bin-test/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 722b9e979..14e0bb430 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -130,7 +130,7 @@ async function retryUntil( }, timeoutMs); }); const retry = (async () => { - for (; ;) { + for (;;) { if (await fn()) { break; } From d384aaf4381b059e855c3a76b529cad02709c2e7 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 14:46:42 -0700 Subject: [PATCH 10/18] refactor: use XML-style tags for stdio discovery output - Replace magic string prefixes with self-documenting XML tags - Add explicit start/end markers to handle OS buffering edge cases - Make regex parsing more robust and less greedy - Addresses review comment about regex being too greedy --- scripts/bin-test/test.ts | 4 ++-- src/bin/firebase-functions.ts | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 14e0bb430..b0deae3f0 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -210,7 +210,7 @@ async function runStdioDiscovery(modulePath: string): Promise { proc.on("close", () => { clearTimeout(timeoutId); - const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:([\s\S]+)/); + const manifestMatch = stderr.match(/\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST>/); if (manifestMatch) { const base64 = manifestMatch[1]; const manifestJson = Buffer.from(base64, "base64").toString("utf8"); @@ -219,7 +219,7 @@ async function runStdioDiscovery(modulePath: string): Promise { return; } - const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:([\s\S]+)/); + const errorMatch = stderr.match(/\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST_ERROR>/); if (errorMatch) { resolve({ success: false, error: errorMatch[1] }); return; diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index e422d8c2d..7f0edb6ac 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -49,8 +49,10 @@ if (args.length > 1) { functionsDir = args[0]; } -const MANIFEST_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST__:"; -const MANIFEST_ERROR_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:"; +const MANIFEST_START_TAG = ""; +const MANIFEST_END_TAG = ""; +const MANIFEST_ERROR_START_TAG = ""; +const MANIFEST_ERROR_END_TAG = ""; async function runStdioDiscovery() { try { @@ -58,12 +60,12 @@ async function runStdioDiscovery() { const wireFormat = stackToWire(stack); const manifestJson = JSON.stringify(wireFormat); const base64 = Buffer.from(manifestJson).toString("base64"); - process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`); + process.stderr.write(`${MANIFEST_START_TAG}\n${base64}\n${MANIFEST_END_TAG}\n`); process.exitCode = 0; } catch (e) { console.error("Failed to generate manifest from function source:", e); const message = e instanceof Error ? e.message : String(e); - process.stderr.write(`${MANIFEST_ERROR_PREFIX}${message}\n`); + process.stderr.write(`${MANIFEST_ERROR_START_TAG}\n${message}\n${MANIFEST_ERROR_END_TAG}\n`); process.exitCode = 1; } } From ebebfa0b6a65106992520a49ff67797b64f0d72d Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 14:52:28 -0700 Subject: [PATCH 11/18] fix: resolve prettier formatting for long regex lines --- scripts/bin-test/test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index b0deae3f0..5ec4d696f 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -210,7 +210,9 @@ async function runStdioDiscovery(modulePath: string): Promise { proc.on("close", () => { clearTimeout(timeoutId); - const manifestMatch = stderr.match(/\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST>/); + const manifestMatch = stderr.match( + /\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST>/ + ); if (manifestMatch) { const base64 = manifestMatch[1]; const manifestJson = Buffer.from(base64, "base64").toString("utf8"); @@ -219,7 +221,9 @@ async function runStdioDiscovery(modulePath: string): Promise { return; } - const errorMatch = stderr.match(/\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST_ERROR>/); + const errorMatch = stderr.match( + /\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST_ERROR>/ + ); if (errorMatch) { resolve({ success: false, error: errorMatch[1] }); return; From 74e3275cd842cf589d08c47bcae25a05683c0652 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Fri, 11 Jul 2025 15:04:10 -0700 Subject: [PATCH 12/18] Update src/bin/firebase-functions.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- src/bin/firebase-functions.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index 7f0edb6ac..f3d77ad80 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -63,10 +63,8 @@ async function runStdioDiscovery() { process.stderr.write(`${MANIFEST_START_TAG}\n${base64}\n${MANIFEST_END_TAG}\n`); process.exitCode = 0; } catch (e) { - console.error("Failed to generate manifest from function source:", e); - const message = e instanceof Error ? e.message : String(e); + const message = `Failed to generate manifest from function source: ${e instanceof Error ? e.message : String(e)}`; process.stderr.write(`${MANIFEST_ERROR_START_TAG}\n${message}\n${MANIFEST_ERROR_END_TAG}\n`); - process.exitCode = 1; } } From 478b8c2dd9931fda05d9fdff13eda3d1051d90c0 Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Fri, 11 Jul 2025 16:41:31 -0700 Subject: [PATCH 13/18] linter. --- src/bin/firebase-functions.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index f3d77ad80..a68535701 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -63,7 +63,9 @@ async function runStdioDiscovery() { process.stderr.write(`${MANIFEST_START_TAG}\n${base64}\n${MANIFEST_END_TAG}\n`); process.exitCode = 0; } catch (e) { - const message = `Failed to generate manifest from function source: ${e instanceof Error ? e.message : String(e)}`; + const message = `Failed to generate manifest from function source: ${ + e instanceof Error ? e.message : String(e) + }`; process.stderr.write(`${MANIFEST_ERROR_START_TAG}\n${message}\n${MANIFEST_ERROR_END_TAG}\n`); } } From b5b75e1f1890fcbf4707d47ba5396eb2e359a1e7 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Sun, 13 Jul 2025 09:13:37 -0700 Subject: [PATCH 14/18] refactor: replace stdio discovery with file-based manifest output - Remove XML-style stdio discovery mode - Add FUNCTIONS_MANIFEST_OUTPUT_PATH environment variable support - Write manifest directly to specified file path when env var is set - Update tests to use file-based discovery instead of stdio parsing - Simplify error handling with direct stderr output --- scripts/bin-test/test.ts | 48 ++++++++++++++++------------------ src/bin/firebase-functions.ts | 49 ++++++++++++++++------------------- 2 files changed, 46 insertions(+), 51 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 5ec4d696f..dc1ab1d0d 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -1,6 +1,8 @@ import * as subprocess from "child_process"; import * as path from "path"; import { promisify } from "util"; +import * as fs from "fs/promises"; +import * as os from "os"; import { expect } from "chai"; import * as yaml from "js-yaml"; @@ -185,15 +187,16 @@ async function runHttpDiscovery(modulePath: string): Promise { } } -async function runStdioDiscovery(modulePath: string): Promise { +async function runFileDiscovery(modulePath: string): Promise { + const outputPath = path.join(os.tmpdir(), `firebase-functions-test-${Date.now()}.json`); + return new Promise((resolve, reject) => { const proc = subprocess.spawn("npx", ["firebase-functions"], { cwd: path.resolve(modulePath), env: { PATH: process.env.PATH, GCLOUD_PROJECT: "test-project", - FUNCTIONS_CONTROL_API: "true", - FUNCTIONS_DISCOVERY_MODE: "stdio", + FUNCTIONS_MANIFEST_OUTPUT_PATH: outputPath, }, }); @@ -205,31 +208,26 @@ async function runStdioDiscovery(modulePath: string): Promise { const timeoutId = setTimeout(() => { proc.kill(9); - reject(new Error(`Stdio discovery timed out after ${TIMEOUT_M}ms`)); + resolve({ success: false, error: `File discovery timed out after ${TIMEOUT_M}ms` }); }, TIMEOUT_M); - proc.on("close", () => { + proc.on("close", async (code) => { clearTimeout(timeoutId); - const manifestMatch = stderr.match( - /\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST>/ - ); - if (manifestMatch) { - const base64 = manifestMatch[1]; - const manifestJson = Buffer.from(base64, "base64").toString("utf8"); - const manifest = JSON.parse(manifestJson) as Record; - resolve({ success: true, manifest }); - return; - } - - const errorMatch = stderr.match( - /\n([\s\S]+?)\n<\/FIREBASE_FUNCTIONS_MANIFEST_ERROR>/ - ); - if (errorMatch) { - resolve({ success: false, error: errorMatch[1] }); - return; + + if (code === 0) { + try { + const manifestJson = await fs.readFile(outputPath, "utf8"); + const manifest = JSON.parse(manifestJson) as Record; + await fs.unlink(outputPath).catch(() => {}); + resolve({ success: true, manifest }); + } catch (e) { + resolve({ success: false, error: `Failed to read manifest file: ${e}` }); + } + } else { + const errorLines = stderr.split('\n').filter(line => line.trim()); + const errorMessage = errorLines.join(' ') || "No error message found"; + resolve({ success: false, error: errorMessage }); } - - resolve({ success: false, error: "No manifest or error found" }); }); proc.on("error", (err) => { @@ -245,7 +243,7 @@ describe("functions.yaml", function () { const discoveryMethods = [ { name: "http", fn: runHttpDiscovery }, - { name: "stdio", fn: runStdioDiscovery }, + { name: "file", fn: runFileDiscovery }, ]; function runDiscoveryTests( diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index a68535701..9c35e2f75 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -24,6 +24,7 @@ import * as http from "http"; import * as express from "express"; +import * as fs from "fs/promises"; import { loadStack } from "../runtime/loader"; import { stackToWire } from "../runtime/manifest"; @@ -49,37 +50,33 @@ if (args.length > 1) { functionsDir = args[0]; } -const MANIFEST_START_TAG = ""; -const MANIFEST_END_TAG = ""; -const MANIFEST_ERROR_START_TAG = ""; -const MANIFEST_ERROR_END_TAG = ""; - -async function runStdioDiscovery() { - try { - const stack = await loadStack(functionsDir); - const wireFormat = stackToWire(stack); - const manifestJson = JSON.stringify(wireFormat); - const base64 = Buffer.from(manifestJson).toString("base64"); - process.stderr.write(`${MANIFEST_START_TAG}\n${base64}\n${MANIFEST_END_TAG}\n`); - process.exitCode = 0; - } catch (e) { - const message = `Failed to generate manifest from function source: ${ - e instanceof Error ? e.message : String(e) - }`; - process.stderr.write(`${MANIFEST_ERROR_START_TAG}\n${message}\n${MANIFEST_ERROR_END_TAG}\n`); - } -} - function handleQuitquitquit(req: express.Request, res: express.Response, server: http.Server) { res.send("ok"); server.close(); } -if ( - process.env.FUNCTIONS_CONTROL_API === "true" && - process.env.FUNCTIONS_DISCOVERY_MODE === "stdio" -) { - void runStdioDiscovery(); +if (process.env.FUNCTIONS_MANIFEST_OUTPUT_PATH) { + void (async () => { + try { + const stack = await loadStack(functionsDir); + const wireFormat = stackToWire(stack); + await fs.writeFile( + process.env.FUNCTIONS_MANIFEST_OUTPUT_PATH, + JSON.stringify(wireFormat, null, 2) + ); + process.exit(0); + } catch (e) { + console.error( + `Failed to generate manifest from function source: ${ + e instanceof Error ? e.message : String(e) + }` + ); + if (e instanceof Error && e.stack) { + console.error(e.stack); + } + process.exit(1); + } + })(); } else { let server: http.Server = undefined; const app = express(); From 9d78c6d44eaa3f20c433d6e1e21d08c7d37c85ef Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Sun, 13 Jul 2025 10:21:36 -0700 Subject: [PATCH 15/18] fix: resolve lint and formatting issues --- scripts/bin-test/test.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index dc1ab1d0d..4fb27e907 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -189,7 +189,7 @@ async function runHttpDiscovery(modulePath: string): Promise { async function runFileDiscovery(modulePath: string): Promise { const outputPath = path.join(os.tmpdir(), `firebase-functions-test-${Date.now()}.json`); - + return new Promise((resolve, reject) => { const proc = subprocess.spawn("npx", ["firebase-functions"], { cwd: path.resolve(modulePath), @@ -213,19 +213,21 @@ async function runFileDiscovery(modulePath: string): Promise { proc.on("close", async (code) => { clearTimeout(timeoutId); - + if (code === 0) { try { const manifestJson = await fs.readFile(outputPath, "utf8"); const manifest = JSON.parse(manifestJson) as Record; - await fs.unlink(outputPath).catch(() => {}); + await fs.unlink(outputPath).catch(() => { + // Ignore errors + }); resolve({ success: true, manifest }); } catch (e) { resolve({ success: false, error: `Failed to read manifest file: ${e}` }); } } else { - const errorLines = stderr.split('\n').filter(line => line.trim()); - const errorMessage = errorLines.join(' ') || "No error message found"; + const errorLines = stderr.split("\n").filter((line) => line.trim()); + const errorMessage = errorLines.join(" ") || "No error message found"; resolve({ success: false, error: errorMessage }); } }); From c0dba6cff500b7210a3a44989c00619379352095 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Sun, 13 Jul 2025 12:54:17 -0700 Subject: [PATCH 16/18] feat: add robust path validation and error handling for file output - Validate output directory exists and is writable before attempting manifest generation - Provide specific error messages for common failure cases (ENOENT, EACCES) - Check directory permissions early to fail fast with helpful guidance - Improve error messages to help users resolve issues quickly --- src/bin/firebase-functions.ts | 41 ++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/bin/firebase-functions.ts b/src/bin/firebase-functions.ts index 9c35e2f75..19cee056e 100644 --- a/src/bin/firebase-functions.ts +++ b/src/bin/firebase-functions.ts @@ -25,6 +25,7 @@ import * as http from "http"; import * as express from "express"; import * as fs from "fs/promises"; +import * as path from "path"; import { loadStack } from "../runtime/loader"; import { stackToWire } from "../runtime/manifest"; @@ -57,20 +58,40 @@ function handleQuitquitquit(req: express.Request, res: express.Response, server: if (process.env.FUNCTIONS_MANIFEST_OUTPUT_PATH) { void (async () => { + const outputPath = process.env.FUNCTIONS_MANIFEST_OUTPUT_PATH; try { + // Validate the output path + const dir = path.dirname(outputPath); + try { + await fs.access(dir, fs.constants.W_OK); + } catch (e) { + console.error( + `Error: Cannot write to directory '${dir}': ${e instanceof Error ? e.message : String(e)}` + ); + console.error("Please ensure the directory exists and you have write permissions."); + process.exit(1); + } + const stack = await loadStack(functionsDir); const wireFormat = stackToWire(stack); - await fs.writeFile( - process.env.FUNCTIONS_MANIFEST_OUTPUT_PATH, - JSON.stringify(wireFormat, null, 2) - ); + await fs.writeFile(outputPath, JSON.stringify(wireFormat, null, 2)); process.exit(0); - } catch (e) { - console.error( - `Failed to generate manifest from function source: ${ - e instanceof Error ? e.message : String(e) - }` - ); + } catch (e: any) { + if (e.code === "ENOENT") { + console.error(`Error: Directory '${path.dirname(outputPath)}' does not exist.`); + console.error("Please create the directory or specify a valid path."); + } else if (e.code === "EACCES") { + console.error(`Error: Permission denied writing to '${outputPath}'.`); + console.error("Please check file permissions or choose a different location."); + } else if (e.message?.includes("Failed to generate manifest")) { + console.error(e.message); + } else { + console.error( + `Failed to generate manifest from function source: ${ + e instanceof Error ? e.message : String(e) + }` + ); + } if (e instanceof Error && e.stack) { console.error(e.stack); } From aef0d6d452c1eb53d5987cc160b44dd1ed4426d1 Mon Sep 17 00:00:00 2001 From: Daniel Lee Date: Sun, 13 Jul 2025 12:58:37 -0700 Subject: [PATCH 17/18] fix: address PR review comments for test reliability - Wait for process termination after kill(9) to prevent orphaned processes - Use fs.mkdtemp() instead of Date.now() for unique temp directories - Clean up temp directories after tests complete - Add proper error cleanup for temp directories --- scripts/bin-test/test.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index 4fb27e907..c3230b264 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -183,12 +183,16 @@ async function runHttpDiscovery(modulePath: string): Promise { return { success: false, error: body }; } } finally { - proc.kill(9); + if (proc.pid) { + proc.kill(9); + await new Promise((resolve) => proc.on("exit", resolve)); + } } } async function runFileDiscovery(modulePath: string): Promise { - const outputPath = path.join(os.tmpdir(), `firebase-functions-test-${Date.now()}.json`); + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "firebase-functions-test-")); + const outputPath = path.join(tempDir, "manifest.json"); return new Promise((resolve, reject) => { const proc = subprocess.spawn("npx", ["firebase-functions"], { @@ -218,7 +222,7 @@ async function runFileDiscovery(modulePath: string): Promise { try { const manifestJson = await fs.readFile(outputPath, "utf8"); const manifest = JSON.parse(manifestJson) as Record; - await fs.unlink(outputPath).catch(() => { + await fs.rm(tempDir, { recursive: true }).catch(() => { // Ignore errors }); resolve({ success: true, manifest }); @@ -234,6 +238,10 @@ async function runFileDiscovery(modulePath: string): Promise { proc.on("error", (err) => { clearTimeout(timeoutId); + // Clean up temp directory on error + fs.rm(tempDir, { recursive: true }).catch(() => { + // Ignore errors + }); reject(err); }); }); From e20a52887d216426edf39780a9b0d87bc52107dc Mon Sep 17 00:00:00 2001 From: Daniel Young Lee Date: Mon, 14 Jul 2025 10:47:29 -0700 Subject: [PATCH 18/18] fix: Gracefully kill child processes in tests Ensures that child processes spawned during tests are properly killed and awaited, preventing orphaned processes in the CI environment. --- scripts/bin-test/test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/bin-test/test.ts b/scripts/bin-test/test.ts index c3230b264..616b50fb5 100644 --- a/scripts/bin-test/test.ts +++ b/scripts/bin-test/test.ts @@ -210,8 +210,11 @@ async function runFileDiscovery(modulePath: string): Promise { stderr += chunk.toString("utf8"); }); - const timeoutId = setTimeout(() => { - proc.kill(9); + const timeoutId = setTimeout(async () => { + if (proc.pid) { + proc.kill(9); + await new Promise((resolve) => proc.on("exit", resolve)); + } resolve({ success: false, error: `File discovery timed out after ${TIMEOUT_M}ms` }); }, TIMEOUT_M);