fix(hopper): complete owned shutdown cleanup

bridge/hopper_bridge.py

@@ -628,13 +628,13 @@ 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 REA_OWNS_PROCESS_LIFETIME:
+        if method == "shutdown" and REA_OWNS_PROCESS_LIFETIME:
             return {
                 "shutdown": True,
                 "analysis_stopped": not document.backgroundProcessActive(),
@@ -813,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/HopperClient.ts

@@ -167,6 +167,7 @@ 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",
@@ -174,12 +175,23 @@ export class HopperClient {
           { timeoutMs: SHUTDOWN_TIMEOUT_MS },
         ).catch(() => err(new HopperProcessError(null)));
         if (
-          !shutdown.ok ||
-          !isShutdownAcknowledgement(
-            shutdown.value,
-            this.#launch?.shutdownByCleanup === true,
-          )
-        )
+          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",
@@ -189,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(
@@ -211,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,
@@ -418,18 +443,22 @@ const connectOnce = async (
     });
   });
 
-const isShutdownAcknowledgement = (
-  value: JsonValue,
-  ownsProcessLifetime: boolean,
-): 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 ownsProcessLifetime && value.cleanup_required === true;
+  return false;
 };
 
+const isCleanupRequired = (value: JsonValue): boolean =>
+  typeof value === "object" &&
+  value !== null &&
+  !Array.isArray(value) &&
+  value.shutdown === true &&
+  value.cleanup_required === true;
+
 const parseServerInfo = (
   value: JsonValue,
   expectedRunId: string,

tests/bridgeTruthfulness.test.ts

@@ -104,8 +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 REA_OWNS_PROCESS_LIFETIME:");
+    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(