fix(functions): don't orphan the Python discovery server

CHANGELOG.md

@@ -1,3 +1,4 @@
 - Added `appcheck:providers:list`, `appcheck:providers:get` and `appcheck:providers:set` to configure App Check attestation providers for an app.
 - Added `appcheck:apps:list` to show every app with its configured App Check providers.
-- Added web app support for Crashlytics MCP tools and prompts.>>>>>>> main
+- Added web app support for Crashlytics MCP tools and prompts.
+- Fixed `firebase deploy` leaving the Python discovery admin server (`serving.py`) running after a killed or wedged deploy, which caused later deploys to hang indefinitely on `connect ETIMEDOUT` (#10847).

src/deploy/functions/runtimes/python/index.spec.ts

@@ -1,7 +1,12 @@
+import { ChildProcess } from "child_process";
+import { EventEmitter } from "events";
+
 import { expect } from "chai";
 import * as sinon from "sinon";
 
 import * as python from ".";
+import * as pythonUtils from "../../../../functions/python";
+import { IS_WINDOWS } from "../../../../utils";
 
 const PROJECT_ID = "test-project";
 const SOURCE_DIR = "/some/path/fns";
@@ -34,4 +39,167 @@ describe("PythonDelegate", () => {
       expect(delegate.getPythonBinary()).to.equal("python.exe");
     });
   });
+
+  describe("serveAdmin", () => {
+    const ADMIN_PORT = 8081;
+    // Mirrors the constants in ./index.ts.
+    const FORCE_KILL_DELAY_MS = 10_000;
+    const SHUTDOWN_TIMEOUT_MS = 15_000;
+
+    let sandbox: sinon.SinonSandbox;
+    let clock: sinon.SinonFakeTimers;
+    let child: ChildProcess;
+    let runWithVirtualEnvStub: sinon.SinonStub;
+    let killProcessTreeStub: sinon.SinonStub;
+    let fetchStub: sinon.SinonStub;
+    let destroyStdoutStub: sinon.SinonStub;
+    let destroyStderrStub: sinon.SinonStub;
+    let unrefStub: sinon.SinonStub;
+    let delegate: python.Delegate;
+
+    beforeEach(() => {
+      sandbox = sinon.createSandbox();
+      child = new EventEmitter() as ChildProcess;
+      destroyStdoutStub = sandbox.stub();
+      destroyStderrStub = sandbox.stub();
+      unrefStub = sandbox.stub();
+      Object.assign(child, {
+        pid: 4242,
+        exitCode: null,
+        signalCode: null,
+        stdout: Object.assign(new EventEmitter(), { destroy: destroyStdoutStub }),
+        stderr: Object.assign(new EventEmitter(), { destroy: destroyStderrStub }),
+        unref: unrefStub,
+      });
+      runWithVirtualEnvStub = sandbox.stub(pythonUtils, "runWithVirtualEnv").returns(child);
+      killProcessTreeStub = sandbox.stub(pythonUtils, "killProcessTree");
+      // Tracking installs real process-level signal handlers; not under test here.
+      sandbox.stub(pythonUtils, "trackVirtualEnvChild");
+      sandbox.stub(pythonUtils, "untrackVirtualEnvChild");
+      fetchStub = sandbox.stub(global, "fetch" as never);
+      delegate = new python.Delegate(PROJECT_ID, SOURCE_DIR, "python312");
+      sandbox.stub(delegate, "modulesDir").resolves("/some/site-packages/firebase_functions");
+      clock = sandbox.useFakeTimers();
+    });
+
+    afterEach(() => {
+      sandbox.restore();
+    });
+
+    it("spawns detached so the kill can reach Python under the venv shell wrapper", async () => {
+      await delegate.serveAdmin(ADMIN_PORT, {});
+
+      const spawnOpts = runWithVirtualEnvStub.firstCall.args[3] as { detached: boolean };
+      expect(spawnOpts.detached).to.equal(!IS_WINDOWS);
+    });
+
+    it("asks the server to quit and resolves once it exits", async () => {
+      fetchStub.resolves(new Response("", { status: 200 }));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      const shutdown = killProcess();
+      child.emit("exit", 0);
+      await shutdown;
+
+      expect(fetchStub.firstCall.args[0]).to.equal(
+        `http://127.0.0.1:${ADMIN_PORT}/__/quitquitquit`,
+      );
+      expect(killProcessTreeStub).to.not.have.been.called;
+    });
+
+    it("force-kills the process group when the server never answers quitquitquit", async () => {
+      // A wedged server: bound to the port but not accepting connections.
+      fetchStub.rejects(Object.assign(new Error("connect ETIMEDOUT"), { code: "ETIMEDOUT" }));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      const shutdown = killProcess();
+      await clock.tickAsync(FORCE_KILL_DELAY_MS);
+
+      expect(killProcessTreeStub).to.have.been.calledOnceWithExactly(4242);
+
+      child.emit("exit", null, "SIGKILL");
+      await shutdown;
+    });
+
+    it("gives up instead of hanging the deploy when the process refuses to die", async () => {
+      fetchStub.rejects(Object.assign(new Error("connect ETIMEDOUT"), { code: "ETIMEDOUT" }));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      // The child never emits "exit". Before the fix this awaited forever, which
+      // is what left CI deploys hanging until the job timeout.
+      const shutdown = killProcess();
+      let settled = false;
+      void shutdown.then(() => (settled = true));
+
+      await clock.tickAsync(SHUTDOWN_TIMEOUT_MS);
+      await shutdown;
+
+      expect(settled).to.be.true;
+    });
+
+    it("releases the surviving child's handles so it cannot hold the CLI open", async () => {
+      fetchStub.rejects(Object.assign(new Error("connect ETIMEDOUT"), { code: "ETIMEDOUT" }));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      const shutdown = killProcess();
+      await clock.tickAsync(SHUTDOWN_TIMEOUT_MS);
+      await shutdown;
+
+      // A detached child and its pipes each keep the event loop alive, which
+      // would move the hang from the deploy to process exit rather than fix it.
+      expect(destroyStdoutStub).to.have.been.called;
+      expect(destroyStderrStub).to.have.been.called;
+      expect(unrefStub).to.have.been.called;
+    });
+
+    it("does not reject when the child errors, so the real discovery error survives", async () => {
+      fetchStub.rejects(new Error("connect ECONNREFUSED"));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      const shutdown = killProcess();
+      child.emit("error", new Error("spawn failed"));
+
+      await shutdown;
+    });
+
+    it("returns immediately when the server died before shutdown was called", async () => {
+      fetchStub.rejects(new Error("connect ECONNREFUSED"));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      // A server that failed to start, e.g. a venv that could not be activated.
+      // "exit" does not replay, so a listener attached at shutdown time would
+      // never fire and the deploy would stall for SHUTDOWN_TIMEOUT_MS.
+      child.emit("exit", 1);
+
+      const shutdown = killProcess();
+      let settled = false;
+      void shutdown.then(() => (settled = true));
+
+      await clock.tickAsync(0);
+      expect(settled).to.be.true;
+      await shutdown;
+
+      // Nothing left alive, so we must not signal a pid that has been reaped
+      // and possibly recycled.
+      await clock.tickAsync(FORCE_KILL_DELAY_MS);
+      expect(killProcessTreeStub).to.not.have.been.called;
+    });
+
+    it("returns immediately when the child failed to spawn before shutdown was called", async () => {
+      fetchStub.rejects(new Error("connect ECONNREFUSED"));
+      const killProcess = await delegate.serveAdmin(ADMIN_PORT, {});
+
+      // A spawn failure leaves exitCode and signalCode null, so checking those
+      // is not enough on its own to notice the process is gone.
+      child.emit("error", new Error("spawn ENOENT"));
+
+      const shutdown = killProcess();
+      let settled = false;
+      void shutdown.then(() => (settled = true));
+
+      await clock.tickAsync(0);
+      expect(settled).to.be.true;
+      await shutdown;
+    });
+  });
 });

src/deploy/functions/runtimes/python/index.ts

@@ -1,6 +1,7 @@
 import * as fs from "fs";
 import * as path from "path";
 import { promisify } from "util";
+import { ChildProcess } from "child_process";
 
 import * as portfinder from "portfinder";
 
@@ -9,10 +10,29 @@ import * as backend from "../../backend";
 import * as discovery from "../discovery";
 import * as supported from "../supported";
 import { logger } from "../../../../logger";
-import { DEFAULT_VENV_DIR, runWithVirtualEnv, virtualEnvCmd } from "../../../../functions/python";
+import {
+  DEFAULT_VENV_DIR,
+  killProcessTree,
+  runWithVirtualEnv,
+  trackVirtualEnvChild,
+  untrackVirtualEnvChild,
+  virtualEnvCmd,
+} from "../../../../functions/python";
 import { FirebaseError } from "../../../../error";
 import { Build } from "../../build";
 import { assertExhaustive } from "../../../../functional";
+import { IS_WINDOWS } from "../../../../utils";
+
+// How long to wait for the admin server to shut down in response to
+// /__/quitquitquit before force-killing it.
+const FORCE_KILL_DELAY_MS = 10_000;
+// Cap on how long to keep waiting for the child once the shutdown request has
+// settled. A wedged server that survives even SIGKILL of its process group must
+// not be able to hang the deploy.
+const SHUTDOWN_TIMEOUT_MS = 15_000;
+// A server that is bound but not accepting connections will never answer, so the
+// shutdown request needs its own timeout rather than relying on the socket layer.
+const QUITQUITQUIT_TIMEOUT_MS = 5_000;
 
 /**
  * Create a runtime delegate for the Python runtime, if applicable.
@@ -149,7 +169,7 @@ export class Delegate implements runtimes.RuntimeDelegate {
     return Promise.resolve();
   }
 
-  async serveAdmin(port: number, envs: backend.EnvironmentVariables) {
+  async serveAdmin(port: number, envs: backend.EnvironmentVariables): Promise<() => Promise<void>> {
     const modulesDir = await this.modulesDir();
     const envWithAdminPort = {
       ...envs,
@@ -161,30 +181,85 @@ export class Delegate implements runtimes.RuntimeDelegate {
         envWithAdminPort,
       )} in ${this.sourceDir}`,
     );
-    const childProcess = runWithVirtualEnv(args, this.sourceDir, envWithAdminPort);
+    // detached so the shell runWithVirtualEnv spawns becomes the leader of its
+    // own process group: that lets killProcessTree() force-kill the shell *and*
+    // the Python process underneath it, instead of just the shell.
+    const childProcess = runWithVirtualEnv(args, this.sourceDir, envWithAdminPort, {
+      detached: !IS_WINDOWS,
+    });
     childProcess.stdout?.on("data", (chunk: Buffer) => {
       logger.info(chunk.toString("utf8"));
     });
     childProcess.stderr?.on("data", (chunk: Buffer) => {
       logger.error(chunk.toString("utf8"));
     });
-    return Promise.resolve(async () => {
-      try {
-        await fetch(`http://127.0.0.1:${port}/__/quitquitquit`);
-      } catch (e) {
-        logger.debug("Failed to call quitquitquit. This often means the server failed to start", e);
-      }
-      const quitTimeout = setTimeout(() => {
-        if (!childProcess.killed) {
-          childProcess.kill("SIGKILL");
-        }
-      }, 10_000);
-      clearTimeout(quitTimeout);
-      return new Promise<void>((resolve, reject) => {
-        childProcess.once("exit", resolve);
-        childProcess.once("error", reject);
+    // Attached here rather than in shutdownAdmin() because 'exit' and 'error' do
+    // not replay: a server that dies before shutdown is called (a venv that fails
+    // to activate, a missing interpreter) would otherwise leave a listener that
+    // never fires and stall the whole shutdown until SHUTDOWN_TIMEOUT_MS.
+    const exited = new Promise<void>((resolve) => {
+      childProcess.once("exit", () => resolve());
+      childProcess.once("error", () => resolve());
+    });
+    trackVirtualEnvChild(childProcess);
+    return Promise.resolve(() => this.shutdownAdmin(childProcess, port, exited));
+  }
+
+  /**
+   * Shut down a discovery admin server, escalating from an HTTP request to a
+   * force-kill of its process group. Bounded by QUITQUITQUIT_TIMEOUT_MS followed
+   * by SHUTDOWN_TIMEOUT_MS, since the timers only start once the request settles.
+   *
+   * `exited` must have been attached at spawn time; see serveAdmin().
+   */
+  private async shutdownAdmin(
+    childProcess: ChildProcess,
+    port: number,
+    exited: Promise<void>,
+  ): Promise<void> {
+    try {
+      await fetch(`http://127.0.0.1:${port}/__/quitquitquit`, {
+        signal: AbortSignal.timeout(QUITQUITQUIT_TIMEOUT_MS),
       });
+    } catch (e) {
+      logger.debug("Failed to call quitquitquit. This often means the server failed to start", e);
+    }
+    const forceKill = setTimeout(() => {
+      // No childProcess.killed check: that flag only reflects calls to .kill()
+      // on this object, and killProcessTree() is already a no-op for a process
+      // group that has gone away.
+      if (childProcess.pid) {
+        logger.debug(
+          `Discovery admin server on port ${port} did not shut down when asked. Force-killing it.`,
+        );
+        killProcessTree(childProcess.pid);
+      }
+    }, FORCE_KILL_DELAY_MS);
+    let giveUp: NodeJS.Timeout | undefined;
+    const timedOut = new Promise<boolean>((resolve) => {
+      giveUp = setTimeout(() => resolve(false), SHUTDOWN_TIMEOUT_MS);
     });
+    try {
+      const exitedCleanly = await Promise.race([exited.then(() => true), timedOut]);
+      if (exitedCleanly) {
+        untrackVirtualEnvChild(childProcess);
+      } else {
+        // A detached child and its pipes both hold the CLI's event loop open, so
+        // leaving it alive would hang the deploy at process exit instead of here.
+        // Releasing them is also what lets the 'exit' handler run at all, and the
+        // child stays tracked so that handler gets one last attempt at it.
+        childProcess.stdout?.destroy();
+        childProcess.stderr?.destroy();
+        childProcess.unref();
+        logger.debug(
+          `Discovery admin server on port ${port} survived being force-killed. ` +
+            `Continuing without it; it may need to be cleaned up manually.`,
+        );
+      }
+    } finally {
+      clearTimeout(forceKill);
+      clearTimeout(giveUp);
+    }
   }
 
   async discoverBuild(