diff --git a/bridge/hopper_bridge.py b/bridge/hopper_bridge.py index 8a19e463a..1bc75217a 100644 --- a/bridge/hopper_bridge.py +++ b/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)}} diff --git a/src/hopper/HopperClient.ts b/src/hopper/HopperClient.ts index 0421dde74..8de3ad704 100644 --- a/src/hopper/HopperClient.ts +++ b/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 { + 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, diff --git a/tests/bridgeTruthfulness.test.ts b/tests/bridgeTruthfulness.test.ts index e42f3ad2d..23d537ba7 100644 --- a/tests/bridgeTruthfulness.test.ts +++ b/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(