fix(hosting): close all site servers during shutdown

CHANGELOG.md

@@ -1,2 +1,3 @@
 - Updated chokidar dependency to ^4.0.3 to remove vulnerable braces dependency (#11308).
+- Fixed Hosting shutdown leaving earlier sites running when serving multiple sites.
 - Fixed an issue where the Firebase MCP server referred to `update_firebase_environment` instead of `firebase_update_environment`.

src/serve/hosting.spec.ts

@@ -8,6 +8,15 @@ import * as hosting from "./hosting";
 import * as requireHostingSite from "../requireHostingSite";
 import * as utils from "../utils";
 import { Writable } from "stream";
+import { once } from "events";
+import { Server } from "http";
+import { AddressInfo } from "net";
+import * as fs from "fs/promises";
+import * as os from "os";
+import * as path from "path";
+
+const realSuperstatic = superstatic.server;
+const realCreateDestroyer = utils.createDestroyer;
 
 describe("hosting", () => {
   const sandbox = sinon.createSandbox();
@@ -45,12 +54,16 @@ describe("hosting", () => {
       }),
     };
     superstaticStub = sandbox.stub(superstatic, "server").returns(superstaticServer as any);
-    createDestroyerStub = sandbox.stub(utils, "createDestroyer");
+    createDestroyerStub = sandbox.stub(utils, "createDestroyer").returns(sandbox.stub().resolves());
     sandbox.stub(Writable.prototype, "_write").resolves();
   });
 
-  afterEach(() => {
-    sandbox.restore();
+  afterEach(async () => {
+    try {
+      await hosting.stop();
+    } finally {
+      sandbox.restore();
+    }
   });
 
   describe("start", () => {
@@ -112,6 +125,159 @@ describe("hosting", () => {
   });
 
   describe("stop", () => {
+    it("should stop every hosting site", async () => {
+      const first = sandbox.stub().resolves();
+      const second = sandbox.stub().resolves();
+      hostingConfigStub.returns([
+        { site: "site-one", public: "public" },
+        { site: "site-two", public: "public" },
+      ]);
+      createDestroyerStub.onFirstCall().returns(first);
+      createDestroyerStub.onSecondCall().returns(second);
+
+      await hosting.start({ port: 8080, host: "localhost" });
+      await hosting.stop();
+
+      expect(first).to.have.been.calledOnce;
+      expect(second).to.have.been.calledOnce;
+    });
+
+    it("should wait for an in-progress shutdown when stopped again", async () => {
+      let finish!: () => void;
+      const closing = new Promise<void>((resolve) => {
+        finish = resolve;
+      });
+      createDestroyerStub.returns(() => closing);
+      await hosting.start({ port: 8080, host: "localhost" });
+
+      const firstStop = hosting.stop();
+      let secondFinished = false;
+      const secondStop = hosting.stop().then(() => {
+        secondFinished = true;
+      });
+      try {
+        await new Promise<void>((resolve) => setImmediate(resolve));
+        expect(secondFinished).to.equal(false);
+      } finally {
+        finish();
+        await Promise.all([firstStop, secondStop]);
+      }
+      expect(secondFinished).to.equal(true);
+    });
+
+    it("should retain a server started while an earlier server is stopping", async () => {
+      let finish!: () => void;
+      const closing = new Promise<void>((resolve) => {
+        finish = resolve;
+      });
+      const first = sandbox.stub().returns(closing);
+      const second = sandbox.stub().resolves();
+      createDestroyerStub.onFirstCall().returns(first);
+      createDestroyerStub.onSecondCall().returns(second);
+      await hosting.start({ port: 8080, host: "localhost" });
+      const firstStop = hosting.stop();
+      try {
+        await hosting.start({ port: 9090, host: "localhost" });
+      } finally {
+        finish();
+        await firstStop;
+      }
+
+      expect(second).to.not.have.been.called;
+      await hosting.stop();
+      expect(first).to.have.been.calledOnce;
+      expect(second).to.have.been.calledOnce;
+    });
+
+    it("should wait for every server before reporting a shutdown failure", async () => {
+      const error = new Error("shutdown failed");
+      let finish!: () => void;
+      const closing = new Promise<void>((resolve) => {
+        finish = resolve;
+      });
+      let secondFinished = false;
+      const first = sandbox.stub().rejects(error);
+      const second = sandbox.stub().callsFake(async () => {
+        await closing;
+        secondFinished = true;
+      });
+      hostingConfigStub.returns([
+        { site: "site-one", public: "public" },
+        { site: "site-two", public: "public" },
+      ]);
+      createDestroyerStub.onFirstCall().returns(first);
+      createDestroyerStub.onSecondCall().returns(second);
+      await hosting.start({ port: 8080, host: "localhost" });
+
+      let stopFinished = false;
+      let stopError: unknown;
+      const stopping = hosting.stop().then(
+        () => {
+          stopFinished = true;
+        },
+        (err: unknown) => {
+          stopFinished = true;
+          stopError = err;
+        },
+      );
+      try {
+        await new Promise<void>((resolve) => setImmediate(resolve));
+        expect(stopFinished).to.equal(false);
+      } finally {
+        finish();
+        await stopping;
+      }
+
+      expect(stopError).to.equal(error);
+      expect(secondFinished).to.equal(true);
+      expect(first).to.have.been.calledOnce;
+      expect(second).to.have.been.calledOnce;
+    });
+
+    it("should close both real HTTP listeners after serving multiple sites", async () => {
+      const directory = await fs.mkdtemp(path.join(os.tmpdir(), "firebase-hosting-stop-"));
+      const servers: Server[] = [];
+      const destroyers: (() => Promise<void>)[] = [];
+      try {
+        await fs.writeFile(path.join(directory, "index.html"), "multi-site hosting");
+        await fs.writeFile(path.join(directory, "firebase.json"), "{}");
+        hostingConfigStub.returns([
+          { site: "site-one", public: "." },
+          { site: "site-two", public: "." },
+        ]);
+        // Let the OS choose a free port for each real server.
+        superstaticStub.callsFake((options: Parameters<typeof realSuperstatic>[0]) =>
+          realSuperstatic({ ...options, port: 0 }),
+        );
+        createDestroyerStub.callsFake((server: Server) => {
+          servers.push(server);
+          const destroyer = realCreateDestroyer(server);
+          destroyers.push(destroyer);
+          return destroyer;
+        });
+        await hosting.start({ port: 8080, host: "127.0.0.1", cwd: directory });
+        await Promise.all(
+          servers.map((server) =>
+            server.listening ? Promise.resolve() : once(server, "listening"),
+          ),
+        );
+        expect(servers).to.have.length(2);
+        for (const server of servers) {
+          const response = await fetch(
+            `http://127.0.0.1:${(server.address() as AddressInfo).port}/`,
+          );
+          expect(response.status).to.equal(200);
+          expect(await response.text()).to.equal("multi-site hosting");
+        }
+
+        await hosting.stop();
+        expect(servers.map((server) => server.listening)).to.deep.equal([false, false]);
+      } finally {
+        await Promise.all(destroyers.map((destroy) => destroy()));
+        await fs.rm(directory, { recursive: true, force: true });
+      }
+    });
+
     it("should call the destroyer if the server was started", async () => {
       const destroyer = sandbox.stub().resolves();
       createDestroyerStub.returns(destroyer);

src/serve/hosting.ts

@@ -19,7 +19,7 @@ import { getProjectId } from "../projectUtils";
 import { checkListenable } from "../emulator/portUtils";
 import { IncomingMessage, ServerResponse } from "http";
 
-let destroyServer: undefined | (() => Promise<void>) = undefined;
+const serverDestroyers = new Set<() => Promise<void>>();
 
 const logger = EmulatorLogger.forEmulator(Emulators.HOSTING);
 
@@ -83,7 +83,7 @@ function startServer(options: any, config: any, port: number, init: TemplateServ
     );
   });
 
-  destroyServer = createDestroyer(server);
+  serverDestroyers.add(createDestroyer(server));
 
   server.on("error", (err: Error) => {
     logger.log("DEBUG", `Error from superstatic server: ${err.stack || ""}`);
@@ -94,10 +94,24 @@ function startServer(options: any, config: any, port: number, init: TemplateServ
 }
 
 /**
- * Stop the Hosting server.
+ * Stop the Hosting servers.
  */
-export function stop(): Promise<void> {
-  return destroyServer ? destroyServer() : Promise.resolve();
+export async function stop(): Promise<void> {
+  const errors: unknown[] = [];
+  await Promise.all(
+    Array.from(serverDestroyers, async (destroyServer) => {
+      try {
+        await destroyServer();
+      } catch (err) {
+        errors.push(err);
+      } finally {
+        serverDestroyers.delete(destroyServer);
+      }
+    }),
+  );
+  if (errors.length > 0) {
+    throw errors[0];
+  }
 }
 
 /**