Repository navigation
fix(hosting): close all site servers during shutdown - #11236
jabrailkhalil wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Firebase Hosting serve module to support stopping multiple sites concurrently by tracking server destroyers in a Set, and adds comprehensive unit tests to verify shutdown behaviors. The feedback identifies a critical issue in the stop function: if one server destroyer rejects, Promise.all will reject immediately, potentially leaving other servers running. A code suggestion is provided to ensure all destroyers are allowed to settle before throwing any encountered errors.
|
Merged current main in e975bf7 and resolved the changelog conflict without restoring already-released entries. The Hosting implementation and regression tests are unchanged from 6b42eba, including waiting for every sibling before rethrowing a shutdown error. Fresh validation: frozen install/production build, test compilation and 100 lifecycle tests pass, including all 12 Hosting cases and the real two-listener regression. Lint passes on an exact LF Git snapshot. The actual full Windows test run gives 5,674 passed / 19 pending / 100 failed; clean current main gives 5,669 / 19 / 100 in the same environment, with the same failing test identities. I have kept the old green Linux run explicitly historical, not presented it as new-head CI, and changed no unrelated assertions, dependencies or lint rules. The merge conflict is gone. Could a maintainer review this integration head and approve the upstream test workflows? I am not requesting a CI bypass. |
|
I merged current Fresh Linux Node24 head/base comparison, checking out the exact source commits:
That includes the normal publish build, lint/format checks, test compilation and complete unit suite. No checks or thresholds were relaxed. The PR still contains only the Hosting source, tests and changelog. The fork's separate push-only Hosting integration jobs are not green without the upstream service-account/project secrets: Linux stops at authentication with an empty credential JSON, and Windows stops at the empty secret setup before tests. I am not claiming credential-backed cloud integration/E2E passed. The conflict is cleared. Upstream CI is awaiting maintainer authorization; could a maintainer authorize that run and complete the review? |
|
I merged current |
|
I merged current main and resolved the changelog conflict, preserving both release notes. |
|
I merged the five newer main commits into |
|
Updated with main |
Description
When serving multiple Hosting sites, each server overwrites the previous shutdown callback, so stopping Hosting closes only the last site's listener. Earlier sites remain running.
I retain every server's destroyer and wait for all of them during shutdown, including when one fails. Concurrent stops share the in-progress cleanup, and servers started during an earlier shutdown remain available for the next stop.
Scenarios Tested
Regression coverage includes two real HTTP servers that both serve a page before shutdown, repeated and overlapping stops, a new start during shutdown, and one server failing to close while its sibling is still pending.
Current head:
4f8cac388013e2d8216c6d208088eb55e6d1f705, integrated with main926b3fe846dd861f5743e50d45f3475bac6fa209. The latest main merge is conflict-free; the Hosting implementation, regression tests and changelog are byte-identical to the previous PR head08dc3ff1. The net PR still changes only those three files.Fresh isolated Linux validation on this exact source (Node v24.22.1, npm 9.5.1, 4 GiB V8 heap):
npm ci, including the production build, passes.npm test -- -- --forbid-onlypasses lint, test compilation and Mocha: 5,876 passing / 11 pending. This includes the Hosting regressions.The isolated local checks are separate from upstream CI authorization and maintainer review. Cloud deployment E2E was not run.
Sample Commands
For a project with multiple Hosting configurations: