fix: kill Java emulators on unexpected CLI process exits#10224
fix: kill Java emulators on unexpected CLI process exits#10224kevmoo wants to merge 3 commits intofirebase:mainfrom
Conversation
Previously, if the `firebase` CLI crashed abruptly
(e.g., due to an unhandled exception or explicit fatal error bypass)
during an active Emulator session, the detached Java background processes
created for emulators like Database, Firestore, and PubSub were left orphaned.
This caused unintended resource leakage and frustrating port collisions on subsequent emulator launches.
This commit resolves the issue by introducing a native `process.on("exit")`
cleanup hook to `downloadableEmulators.ts` that explicitly issues a
`SIGKILL` to all recorded Java sub-processes strictly right upon CLI
termination. The graceful shutdown `stop()` handler has also been fortified
with a `SIGKILL` timeout fallback mechanism specifically designed to terminate
definitively hanging JVM emulator instances.
An integration test suite was also explicitly authored in
`scripts/emulator-tests/downloadableEmulators.spec.ts` which correctly
wraps CLI execution, spins up an isolated orchestrator locally, internally
aborts, and validates the OS system call returns `ESRCH`, guaranteeing the
associated Java PID dissolved.
- Tested standard UI / Graceful `SIGINT` timeout logic.
- Simulated Hard CLI process crashes, immediately reaping lingering JVM processes.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to ensure that detached Java emulator processes are forcefully terminated when the parent CLI process exits abruptly. It adds a process.on('exit') hook to trigger a SIGKILL on the emulator instance and updates the stop function to send a SIGKILL if the initial termination attempt times out. A new integration test has been added to verify this behavior. The review feedback focuses on adhering to the repository's TypeScript style guide by removing the use of any in catch blocks in favor of more type-safe alternatives.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a safety mechanism to ensure detached Java emulators are terminated when the CLI process exits abruptly. Key changes include adding a process exit hook to kill emulator instances and a SIGKILL fallback if an emulator fails to stop within the timeout period. An integration test was also added to verify these process constraints. Review feedback recommends replacing the use of 'any' with a specific error type in the test and restoring the async/await pattern for fatal error handling to ensure consistency and avoid floating promises.
| ); | ||
| } catch (e) { | ||
| // ESRCH directly maps to 'No such process'. This proves our SIGKILL 'on exit' safety net works! | ||
| expect((e as any).code).to.equal("ESRCH"); |
There was a problem hiding this comment.
Avoid using any as an escape hatch, as per the repository style guide. You can use a more specific type like NodeJS.ErrnoException to access the code property on the error object thrown by process.kill.
| expect((e as any).code).to.equal("ESRCH"); | |
| expect((e as NodeJS.ErrnoException).code).to.equal("ESRCH"); |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| emulator.instance.once("exit", (code, signal) => { | ||
| process.removeListener("exit", exitCleanup); | ||
| if (signal) { | ||
| utils.logWarning(`${description} has exited upon receiving signal: ${signal}`); | ||
| } else if (code && code !== 0 && code !== /* SIGINT */ 130) { | ||
| await _fatal(emulator.name, `${description} has exited with code: ${code}`); | ||
| void _fatal(emulator.name, `${description} has exited with code: ${code}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The previous implementation used async/await for the _fatal call. Restoring this pattern ensures consistency with other _fatal calls in this file (e.g., lines 407, 413, 420) and avoids floating promises, even if _fatal eventually terminates the process.
| emulator.instance.once("exit", (code, signal) => { | |
| process.removeListener("exit", exitCleanup); | |
| if (signal) { | |
| utils.logWarning(`${description} has exited upon receiving signal: ${signal}`); | |
| } else if (code && code !== 0 && code !== /* SIGINT */ 130) { | |
| await _fatal(emulator.name, `${description} has exited with code: ${code}`); | |
| void _fatal(emulator.name, `${description} has exited with code: ${code}`); | |
| } | |
| }); | |
| emulator.instance.once("exit", async (code, signal) => { | |
| process.removeListener("exit", exitCleanup); | |
| if (signal) { | |
| utils.logWarning(`${description} has exited upon receiving signal: ${signal}`); | |
| } else if (code && code !== 0 && code !== /* SIGINT */ 130) { | |
| await _fatal(emulator.name, `${description} has exited with code: ${code}`); | |
| } | |
| }); |
Previously, if the
firebaseCLI crashed abruptly(e.g., due to an unhandled exception or explicit fatal error bypass)
during an active Emulator session, the detached Java background processes
created for emulators like Database, Firestore, and PubSub were left orphaned.
This caused unintended resource leakage and frustrating port collisions on subsequent emulator launches.
This commit resolves the issue by introducing a native
process.on("exit")cleanup hook to
downloadableEmulators.tsthat explicitly issues aSIGKILLto all recorded Java sub-processes strictly right upon CLItermination. The graceful shutdown
stop()handler has also been fortifiedwith a
SIGKILLtimeout fallback mechanism specifically designed to terminatedefinitively hanging JVM emulator instances.
An integration test suite was also explicitly authored in
scripts/emulator-tests/downloadableEmulators.spec.tswhich correctlywraps CLI execution, spins up an isolated orchestrator locally, internally
aborts, and validates the OS system call returns
ESRCH, guaranteeing theassociated Java PID dissolved.
SIGINTtimeout logic.