From c8a3e7c046884f8cb4bac8d96d73204baaa6328a Mon Sep 17 00:00:00 2001
From: Christophe Dervieux <christophe.dervieux@gmail.com>
Date: Fri, 25 Oct 2024 15:50:47 +0200
Subject: [PATCH] cri - add browser process in  cleanupHandler for processes
 for quarto  exitWithCleanup logic

As of 2024-10 and chrome v130, there is a problem where Chrome client `close()` function won't return correctly, skipping the normal code part of closing a chromium process started. So processes were not killed. Now those browser processes are added to the same cleanup handler used by `execProcess()` logic so that in case of something going wrong, they are killed and closed at last resort when Quarto exists.
---
 news/changelog-1.6.md |  1 +
 src/core/cri/cri.ts   | 15 ++++++++++++++-
 src/core/process.ts   | 17 +++++++++++++----
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/news/changelog-1.6.md b/news/changelog-1.6.md
index 5cf5d434a2..da91c282f1 100644
--- a/news/changelog-1.6.md
+++ b/news/changelog-1.6.md
@@ -111,6 +111,7 @@ All changes included in 1.6:
 
 - ([#11135](https://github.com/quarto-dev/quarto-cli/issues/11135)): Use `--headless=old` mode for Chromium to avoid recent issues with the new `--headless` mode. Setting `--headless=new` can be configured with `QUARTO_CHROMIUM_HEADLESS_MODE=new` environment variable, however it is not recommended new headless mode seems to be unstable. Only use to be unblocked of a situation (like `QUARTO_CHROMIUM_HEADLESS_MODE="none"` if you use an old chrome version somehow that don't support `--headless=old`).
 - ([#10170](https://github.com/quarto-dev/quarto-cli/issues/10170)): Quarto should find chrome executable automatically on most OS. If this is does not find it, or a specific version is needed, set `QUARTO_CHROMIUM` environment variable to the executable path.
+- Quarto now makes sure that all started chromium instances are closed when the process ends, no matter how it ends (success, error, or interruption).
 
 ## Other Fixes and Improvements
 
diff --git a/src/core/cri/cri.ts b/src/core/cri/cri.ts
index 478fe9224f..08dcd4498d 100644
--- a/src/core/cri/cri.ts
+++ b/src/core/cri/cri.ts
@@ -17,6 +17,10 @@ import { InternalError } from "../lib/error.ts";
 import { getenv } from "../env.ts";
 import { kRenderFileLifetime } from "../../config/constants.ts";
 import { debug } from "../../deno_ral/log.ts";
+import {
+  registerForExitCleanup,
+  unregisterForExitCleanup,
+} from "../process.ts";
 
 async function waitForServer(port: number, timeout = 3000) {
   const interval = 50;
@@ -101,6 +105,9 @@ export async function criClient(appPath?: string, port?: number) {
   ];
   const browser = Deno.run({ cmd, stdout: "piped", stderr: "piped" });
 
+  // Register for cleanup inside exitWithCleanup() in case something goes wrong
+  const thisProcessId = registerForExitCleanup(browser);
+
   if (!(await waitForServer(port as number))) {
     let msg = "Couldn't find open server.";
     // Printing more error information if chrome process errored
@@ -121,7 +128,13 @@ export async function criClient(appPath?: string, port?: number) {
   const result = {
     close: async () => {
       await client.close();
-      browser.close();
+      // FIXME: 2024-10
+      // We have a bug where `client.close()` doesn't return properly and we don't go below
+      // meaning the `browser` process is not killed here, and it will be handled in exitWithCleanup().
+
+      browser.kill(); // Chromium headless won't terminate on its own, so we need to send kill signal
+      browser.close(); // Closing the browser Deno process
+      unregisterForExitCleanup(thisProcessId); // All went well so not need to cleanup on quarto exit
     },
 
     rawClient: () => client,
diff --git a/src/core/process.ts b/src/core/process.ts
index 0787993bf5..6912af70e1 100644
--- a/src/core/process.ts
+++ b/src/core/process.ts
@@ -14,6 +14,16 @@ const processList = new Map<number, Deno.Process>();
 let processCount = 0;
 let cleanupRegistered = false;
 
+export function registerForExitCleanup(process: Deno.Process) {
+  const thisProcessId = ++processCount; // don't risk repeated PIDs
+  processList.set(thisProcessId, process);
+  return thisProcessId;
+}
+
+export function unregisterForExitCleanup(processId: number) {
+  processList.delete(processId);
+}
+
 function ensureCleanup() {
   if (!cleanupRegistered) {
     cleanupRegistered = true;
@@ -61,12 +71,11 @@ export async function execProcess(
       stdout: typeof (options.stdout) === "number" ? options.stdout : "piped",
       stderr: typeof (options.stderr) === "number" ? options.stderr : "piped",
     });
-    const thisProcessId = ++processCount; // don't risk repeated PIDs
-    processList.set(thisProcessId, process);
+    const thisProcessId = registerForExitCleanup(process);
 
     if (stdin !== undefined) {
       if (!process.stdin) {
-        processList.delete(processCount);
+        unregisterForExitCleanup(thisProcessId);
         throw new Error("Process stdin not available");
       }
       // write in 4k chunks (deno observed to overflow at > 64k)
@@ -170,7 +179,7 @@ export async function execProcess(
     // close the process
     process.close();
 
-    processList.delete(thisProcessId);
+    unregisterForExitCleanup(thisProcessId);
 
     debug(`[execProcess] Success: ${status.success}, code: ${status.code}`);