[AI Improvement] [Task] Add process-tree teardown and shutdown timeout to integration CLIProcess

[joehan]

Summary

Addresses Buganizer issue b/563411339.

This PR enhances CLIProcess in scripts/integration-helpers/cli.ts to ensure clean process teardown on Unix platforms:

• Spawns the CLI process with detached: process.platform !== "win32" in CLIProcess.start() so that a dedicated process group is created on Unix.
• In CLIProcess.stop(), sends SIGINT to the process group (process.kill(-pid, "SIGINT")) to gracefully stop the top-level CLI and all child emulator processes (e.g. Java Firestore/PubSub emulators and worker runtimes).
• Introduces a 3000ms fallback timeout (SHUTDOWN_TIMEOUT_MS) to force-kill the process group with SIGKILL (process.kill(-pid, "SIGKILL")) if processes fail to exit after SIGINT, preventing integration tests from hanging indefinitely until outer CI timeouts.
• Cleans up fallback timers upon process exit and resets this.process = undefined.
• Preserves existing Windows process tree teardown (taskkill /pid ${p.pid} /T /F) intact.

Verification

• npm run build: Successful (exit code 0).
• npm run test:compile: Successful (exit code 0).
• eslint scripts/integration-helpers/cli.ts: 0 errors.
• mocha scripts/emulator-tests/unzipEmulators.spec.ts: Passed (2 passing in 6s).
• Port checks (lsof -i :4400, lsof -i :8080): Verified clean with no lingering orphan processes.

[gemini-code-assist]

Code Review

This pull request improves process termination handling in 'scripts/integration-helpers/cli.ts' by introducing explicit shutdown timeouts, spawning processes as detached on non-Windows platforms, and implementing a robust termination sequence using SIGINT with a fallback SIGKILL timeout. A review comment correctly identifies a potential race condition where stopping a process could clear 'this.process' even if a new process has already been started, and suggests a fix to ensure the reference is only cleared if it matches the stopped process.

[ajperel]

My AI had feedback after asking it a bunch of questions, but I admit my process magic is not at this level to have much direct feedback.

[ajperel]

🔴 [Process Lifecycle & Timeout Mismatch] SHUTDOWN_TIMEOUT_MS (3000ms) is shorter than firebase's internal emulator shutdown timeout (4000ms) and process.kill(-pid) does not reach detached: true Java emulators

Rationale:

1. In src/emulator/downloadableEmulators.ts:451, firebase-tools spawns every downloadable emulator (firestore, database, pubsub, storage, ui, dataconnect) with detached: true. On Unix, detached: true invokes setsid(), placing each Java emulator into its own process group (PGID = java_pid), separate from CLIProcess's process group (-pid).
2. Consequently, process.kill(-pid, "SIGINT") and process.kill(-pid, "SIGKILL") only signal firebase and non-detached children (e.g. Functions workers)—they never signal the Java emulator processes.
3. Worse, firebase's own downloadableEmulators.stop() waits up to EMULATOR_INSTANCE_KILL_TIMEOUT = 4000 ms (downloadableEmulators.ts:27) after controller.onExit() (--export-on-exit). Because SHUTDOWN_TIMEOUT_MS is only 3000 ms, if --export-on-exit + JVM teardown takes >3s, process.kill(-pid, "SIGKILL") kills the parent firebase CLI process mid-shutdown, preventing it from stopping the detached Java emulators and leaving them orphaned under PID 1 holding ports 8080/4400.

Suggested Fix:

1. Increase SHUTDOWN_TIMEOUT_MS (e.g., to 10000 ms) so firebase has sufficient time to complete exportOnExit and EMULATOR_INSTANCE_KILL_TIMEOUT (4000 ms).
2. Before SIGKILL-ing -pid, collect descendant PIDs via PPID (while p is still alive, before p exits and reparents them to PID 1) so detached child emulators are also terminated on timeout.

[joehan]

Addressed in commit 7058d19. Updated SHUTDOWN_TIMEOUT_MS to 10,000ms (10s) to give firebase ample time to complete --export-on-exit and respect the 4000ms JVM kill timeout (EMULATOR_INSTANCE_KILL_TIMEOUT). Added explanatory comments documenting the rationale behind both SHUTDOWN_TIMEOUT_MS and WINDOWS_KILL_TIMEOUT_MS.

[ajperel]

🟡 [Simplification] Clear this.process = undefined once at the start of stop() instead of checking if (this.process === p) 4 times

Rationale:

1. Lines 114 and 121 execute synchronously after const p = this.process; (line 79) with no await in between, so this.process === p is unconditionally true there.
2. Once stop() captures const p = this.process, stop() only needs the local variable p. Setting this.process = undefined immediately at line 83 (right after if (!p) return Promise.resolve();) removes all 4 if (this.process === p) blocks, guarantees stop()'s .then() callbacks never clobber a subsequent start(), and ensures a duplicate stop() call becomes a no-op instead of sending a second SIGINT (which in commandUtils.ts:297 triggers the double-Ctrl-C branch that skips cleanShutdown()).

[joehan]

Addressed in commit 7058d19. Set this.process = undefined immediately upon entering stop(), removing the redundant if (this.process === p) checks across all exit branches, protecting against race conditions with subsequent start() invocations, and making duplicate stop() calls a no-op so double-Ctrl-C bypasses are avoided.

[ajperel]

🔴 [Signal Routing & Orphan Cleanup] Send SIGINT to p first and ensure surviving process tree members are reaped when p exits

Rationale:

1. Sending process.kill(-pid, "SIGINT") immediately signals non-detached child worker processes at the exact same instant firebase starts controller.onExit() (--export-on-exit), which can tear down workers while firebase is still exporting data. Sending p.kill("SIGINT") allows firebase (commandUtils.ts:295-296) to orchestrate onExit() followed by cleanShutdown().
2. In both the early-exit path (lines 120–125) and exitPromise.then() (lines 155–160), once the leader process p emits "exit", clearTimeout(timeoutId) cancels the fallback SIGKILL. If p crashed or exited before a child process in -pid (or a detached child with PPID = pid), those surviving processes are never killed.

Suggested Fix:

    const killProcessTree = (sig: NodeJS.Signals): void => {
      try {
        const children = execSync(`ps -o pid= --ppid ${pid}`, { stdio: ["pipe", "pipe", "ignore"] })
          .toString()
          .trim()
          .split(/\s+/)
          .filter(Boolean)
          .map(Number);
        for (const childPid of children) {
          try {
            process.kill(-childPid, sig);
          } catch {
            try {
              process.kill(childPid, sig);
            } catch {
              // Child already exited.
            }
          }
        }
      } catch {
        // No child processes found.
      }
      try {
        process.kill(-pid, sig);
      } catch {
        try {
          p.kill(sig);
        } catch {
          // Process already exited.
        }
      }
    };

[joehan]

Addressed in commit 7058d19. We now send p.kill("SIGINT") directly to the leader process first to allow firebase to run onExit() and cleanShutdown(). If the shutdown timeout expires, killProcessTree("SIGKILL") queries child PIDs via ps -o pid= --ppid ${pid} and kills both child emulators and the root process group. In exitPromise.then(), any surviving processes in -pid are also killed with SIGKILL.

[ajperel]

If there's a good reason for these timeout #s might be nice to explain them in a comment.