fix(hopper): complete owned shutdown cleanup

bridge/hopper_bridge.py

@@ -628,16 +628,23 @@ def _dispatch(method, params):
     global _selected_document
     if method == "health":
         return {"name": "REA Hopper bridge", "version": "1.0.0", "run_id": REA_RUN_ID}
-    if method == "shutdown":
+    if method in ("shutdown", "shutdown_document"):
         document = _session_document()
         if document is None:
             return {"shutdown": True, "analysis_stopped": True, "document_closed": True}
         if document.backgroundProcessActive():
             document.requestBackgroundProcessStop()
+        if method == "shutdown" and REA_OWNS_PROCESS_LIFETIME:
+            return {
+                "shutdown": True,
+                "analysis_stopped": not document.backgroundProcessActive(),
+                "document_closed": False,
+                "cleanup_required": True,
+            }
         if document.backgroundProcessActive():
             document.waitForBackgroundProcessToEnd()
-        analysis_stopped = not document.backgroundProcessActive()
         document.closeDocument()
+        analysis_stopped = not document.backgroundProcessActive()
         document_closed = _session_document() is None
         return {
             "shutdown": True,
@@ -806,7 +813,9 @@ def _serve_connection(connection):
             if not isinstance(request["token"], str) or not hmac.compare_digest(request["token"], REA_TOKEN):
                 raise PermissionError("Invalid bridge capability")
             result = _dispatch(request["method"], request["params"])
-            should_stop = request["method"] == "shutdown"
+            should_stop = request["method"] == "shutdown_document" or (
+                request["method"] == "shutdown" and not result.get("cleanup_required", False)
+            )
             response = {"id": request_id, "result": _json_safe(result)}
         except Exception as error:
             response = {"id": request_id if isinstance(request_id, int) else 0, "error": {"code": -32000, "message": str(error)[:512], "type": _diagnostic_type(error)}}

src/hopper/BridgeLauncher.ts

@@ -27,6 +27,8 @@ export interface BridgeSession {
 export interface BridgeLaunch {
   readonly process: ChildProcess;
   readonly ownsProcessLifetime: boolean;
+  /** True when verified process cleanup completes bridge shutdown. */
+  readonly shutdownByCleanup?: boolean;
   readonly cleanup?: () => Promise<ProcessCleanupResult>;
 }
 
@@ -69,11 +71,13 @@ export class HopperApplicationLauncher implements BridgeLauncher {
     options: { readonly signal?: AbortSignal } = {},
   ): Promise<Result<BridgeLaunch, HopperStartError | HopperCancelledError>> {
     const bootstrapPath = `${session.directory}/bootstrap.py`;
+    const ownsProcessLifetime = usesLinuxDemo(this.options);
     const source = [
       `REA_SOCKET = ${JSON.stringify(session.socketPath)}`,
       `REA_TOKEN = ${JSON.stringify(session.token)}`,
       `REA_RUN_ID = ${JSON.stringify(session.runId)}`,
       `REA_TARGET_PATH = ${JSON.stringify(this.options.targetPath)}`,
+      `REA_OWNS_PROCESS_LIFETIME = ${ownsProcessLifetime ? "True" : "False"}`,
       `exec(compile(open(${JSON.stringify(this.options.bridgeScriptPath)}, 'rb').read(), ${JSON.stringify(this.options.bridgeScriptPath)}, 'exec'))`,
       "",
     ].join("\n");
@@ -141,6 +145,7 @@ export class HopperApplicationLauncher implements BridgeLauncher {
       return ok({
         process: child,
         ownsProcessLifetime: true,
+        shutdownByCleanup: ownsProcessLifetime,
         cleanup: () =>
           cleanupOwnedProcessGroup(
             ownedProcessGroup(session, pid, ownershipCommand),
@@ -163,11 +168,7 @@ const linuxDemoLaunch = (
       readonly ownershipCommand: string;
     }
   | undefined => {
-  if (
-    process.platform !== "linux" ||
-    basename(options.launcherPath) !== "Hopper" ||
-    options.demoHelperPath === undefined
-  )
+  if (!usesLinuxDemo(options) || options.demoHelperPath === undefined)
     return undefined;
   return {
     command: "/usr/bin/python3",
@@ -185,6 +186,11 @@ const linuxDemoLaunch = (
   };
 };
 
+const usesLinuxDemo = (options: HopperApplicationLauncherOptions): boolean =>
+  process.platform === "linux" &&
+  basename(options.launcherPath) === "Hopper" &&
+  options.demoHelperPath !== undefined;
+
 const spawnLauncher = async (
   command: string,
   args: readonly string[],

src/hopper/HopperClient.ts

@@ -167,13 +167,31 @@ export class HopperClient {
     this.#closing = true;
     try {
       const socket = this.#socket;
+      let cleanupAttempted = false;
       if (socket !== undefined && !socket.destroyed) {
         const shutdown = await this.#request(
           "shutdown",
           {},
           { timeoutMs: SHUTDOWN_TIMEOUT_MS },
         ).catch(() => err(new HopperProcessError(null)));
-        if (!shutdown.ok || !isShutdownAcknowledgement(shutdown.value))
+        if (
+          shutdown.ok &&
+          isCleanupRequired(shutdown.value) &&
+          this.#launch?.shutdownByCleanup === true &&
+          this.#launch.cleanup !== undefined
+        ) {
+          cleanupAttempted = true;
+          const cleanup = await this.#launch.cleanup();
+          if (!cleanup.cleaned) {
+            await this.#fallbackDocumentShutdown();
+            const retried = await this.#launch.cleanup();
+            if (!retried.cleaned)
+              this.#logger.warn(
+                { reason: retried.reason },
+                "Owned launcher cleanup failed closed",
+              );
+          }
+        } else if (!shutdown.ok || !isShutdownAcknowledgement(shutdown.value))
           this.#logger.warn(
             { status: shutdown.ok ? "invalid-acknowledgement" : "failed" },
             "Hopper document shutdown was not confirmed",
@@ -183,7 +201,7 @@ export class HopperClient {
       socket?.destroy();
       this.#socket = undefined;
       if (this.#launch?.ownsProcessLifetime === true) {
-        if (this.#launch.cleanup !== undefined) {
+        if (this.#launch.cleanup !== undefined && !cleanupAttempted) {
           const cleanup = await this.#launch.cleanup();
           if (!cleanup.cleaned)
             this.#logger.warn(
@@ -205,6 +223,19 @@ export class HopperClient {
     }
   }
 
+  async #fallbackDocumentShutdown(): Promise<void> {
+    const fallback = await this.#request(
+      "shutdown_document",
+      {},
+      { timeoutMs: SHUTDOWN_TIMEOUT_MS },
+    ).catch(() => err(new HopperProcessError(null)));
+    if (!fallback.ok || !isShutdownAcknowledgement(fallback.value))
+      this.#logger.warn(
+        { status: fallback.ok ? "invalid-acknowledgement" : "failed" },
+        "Hopper document shutdown fallback was not confirmed",
+      );
+  }
+
   async #connect(
     socketPath: string,
     signal?: AbortSignal,
@@ -412,13 +443,21 @@ const connectOnce = async (
     });
   });
 
-const isShutdownAcknowledgement = (value: JsonValue): boolean =>
+const isShutdownAcknowledgement = (value: JsonValue): boolean => {
+  if (typeof value !== "object" || value === null || Array.isArray(value))
+    return false;
+  if (value.shutdown !== true) return false;
+  if (value.analysis_stopped === true && value.document_closed === true)
+    return true;
+  return false;
+};
+
+const isCleanupRequired = (value: JsonValue): boolean =>
   typeof value === "object" &&
   value !== null &&
   !Array.isArray(value) &&
   value.shutdown === true &&
-  value.analysis_stopped === true &&
-  value.document_closed === true;
+  value.cleanup_required === true;
 
 const parseServerInfo = (
   value: JsonValue,

tests/bridgeTruthfulness.test.ts

@@ -104,6 +104,13 @@ describe("Hopper bridge truthfulness", () => {
     expect(bridgeSource).toContain("document.getDatabaseFilePath()");
     expect(bridgeSource).toContain("document.backgroundProcessActive()");
     expect(bridgeSource).toContain("document.requestBackgroundProcessStop()");
+    expect(bridgeSource).toContain(
+      'if method == "shutdown" and REA_OWNS_PROCESS_LIFETIME:',
+    );
+    expect(bridgeSource).toContain('"cleanup_required": True');
+    expect(bridgeSource).toContain(
+      'method in ("shutdown", "shutdown_document")',
+    );
     expect(bridgeSource).toContain("document.waitForBackgroundProcessToEnd()");
     expect(bridgeSource).toContain("document.closeDocument()");
     expect(bridgeSource).toContain(