diff --git a/src/hopper/HopperCleanup.test.ts b/src/hopper/HopperCleanup.test.ts index 70737c7449..dd920d082a 100644 --- a/src/hopper/HopperCleanup.test.ts +++ b/src/hopper/HopperCleanup.test.ts @@ -9,7 +9,10 @@ import { Socket } from "node:net"; import pino from "pino"; import { describe, expect, it, vi } from "vitest"; -import { ok } from "../domain/result.js"; +import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; +import { HopperProcessError } from "../domain/hopperErrors.js"; +import { err, ok } from "../domain/result.js"; +import { observeProcessStartIdentity } from "../process/ProcessOwnershipObservation.js"; import type { BridgeLaunch } from "./BridgeLauncher.js"; import { cleanupHopperSession, @@ -142,7 +145,7 @@ describe("external Hopper process exit confirmation", () => { expect(released).toEqual([undefined]); }); - it.each(["present", "unverified"] as const)( + it.each(["exiting", "present", "unverified"] as const)( "does not confirm an external application while its exact identity is %s", async (observation) => { const resources = externalApplicationResources(); @@ -157,6 +160,58 @@ describe("external Hopper process exit confirmation", () => { expect(releaseTransport).not.toHaveBeenCalled(); }, ); + + it.skipIf(process.platform === "win32")( + "names the failed shutdown request and the live application it retains", + async () => { + // This test process is a live, exactly identified application. + const identity = await observeProcessStartIdentity(process.pid); + if (identity?.state !== "readable") + throw new Error("Expected a readable identity for the test process"); + const resources = externalApplicationResources(); + resources.externalApplication = { pid: process.pid, identity }; + const result = await cleanupHopperSession({ + socket: new Socket(), + resources, + activeRequest: null, + retainDocument: false, + progress: undefined, + logger: loggerHarness().logger, + onDiagnostic: undefined, + request: async (method) => + err( + new HopperProcessError(null, { + operation: method, + providerState: "unreachable", + }), + ), + releaseTransport: () => undefined, + }); + if (result.ok) + throw new Error("Expected the live application to be kept"); + expect(projectAnalysisError(result.error)).toMatchObject({ + details: { + diagnostics: { + issues: [ + { + resource: "hopper-document", + reason: + "authenticated shutdown acknowledgement was not observed", + }, + ], + authenticated_application: "readable", + shutdown_request: { + method: "shutdown_document", + status: "failed", + error_tag: "HopperProcessError", + provider_state: "unreachable", + }, + application_exit: "present", + }, + }, + }); + }, + ); }); describe("Hopper shutdown rejection diagnostics", () => { @@ -230,6 +285,22 @@ describe("Hopper shutdown rejection diagnostics", () => { ok: false, error: { cleanupResources: ["hopper-document"] }, }); + if (result.ok) throw new Error("Expected the document to be retained"); + expect(projectAnalysisError(result.error)).toMatchObject({ + details: { + diagnostics: { + shutdown_request: { + method: "shutdown", + status: "unconfirmed", + fallback: { + method: "shutdown_document", + status: "failed", + error_tag: "HopperProcessError", + }, + }, + }, + }, + }); expect(logs).toContainEqual( expect.objectContaining({ msg: "Hopper document shutdown fallback was not confirmed", diff --git a/src/hopper/HopperCleanup.ts b/src/hopper/HopperCleanup.ts index e13bbae5fd..d48a9cb9e8 100644 --- a/src/hopper/HopperCleanup.ts +++ b/src/hopper/HopperCleanup.ts @@ -33,11 +33,20 @@ interface CleanupIssue { readonly reason: string; } +/** Exit evidence for an authenticated external application, when it applies. */ +type ExternalApplicationExitObservation = + | Awaited> + | undefined; + interface CleanupState { readonly issues: CleanupIssue[]; readonly resources: Set; cleanupResult: ProcessCleanupResult | undefined; processStopped: boolean; + /** Why document shutdown was or was not confirmed by the bridge request. */ + shutdownRequest: JsonValue | undefined; + /** The final external-application exit observation before retention. */ + applicationExit: ExternalApplicationExitObservation; } /** Owned capabilities survive failed cleanup until their release is verified. */ @@ -77,6 +86,8 @@ export const cleanupHopperSession = async ( resources: new Set(), cleanupResult: undefined, processStopped: input.resources.processSupervisor === undefined, + shutdownRequest: undefined, + applicationExit: undefined, }; await confirmExternalApplicationExit( input.resources, @@ -100,7 +111,7 @@ export const cleanupHopperSession = async ( input.releaseTransport(input.socket); await stopProcess(input, state); // A late authenticated application may exit while shutdown is in flight. - await confirmExternalApplicationExit( + state.applicationExit = await confirmExternalApplicationExit( input.resources, input.socket, input.releaseTransport, @@ -145,21 +156,23 @@ export const confirmExternalApplicationExit = async ( socket: Socket | undefined, releaseTransport: (socket: Socket | undefined) => void, observeExit: typeof observeProcessStartIdentityExit = observeProcessStartIdentityExit, -): Promise => { +): Promise => { const application = resources.externalApplication; if ( resources.shutdownConfirmed || resources.launch?.providerLifetime !== "external-application" || application?.identity.state !== "readable" ) - return; - if ( - (await observeExit(application.pid, application.identity.identity)) === - "exited" - ) { + return undefined; + const observed = await observeExit( + application.pid, + application.identity.identity, + ); + if (observed === "exited") { resources.shutdownConfirmed = true; releaseTransport(socket); } + return observed; }; const requestShutdown = async ( @@ -172,6 +185,16 @@ const requestShutdown = async ( input.socket.destroyed || input.activeRequest !== null ) { + state.shutdownRequest = { + status: "not_sent", + reason: input.resources.shutdownConfirmed + ? "shutdown_already_confirmed" + : input.socket === undefined + ? "no_bridge_transport" + : input.socket.destroyed + ? "bridge_transport_closed" + : "bridge_operation_active", + }; if (input.activeRequest !== null) input.logger.warn( { @@ -191,6 +214,7 @@ const requestShutdown = async ( const shutdown = await input .request(method) .catch((cause: unknown) => err(shutdownRequestFailure(method, cause))); + let fallback: Result | undefined; if ( shutdown.ok && isHopperCleanupRequired(shutdown.value) && @@ -200,13 +224,37 @@ const requestShutdown = async ( .cleanup() .catch(providerCleanupFailure); if (!state.cleanupResult.cleaned) { - input.resources.shutdownConfirmed = await fallbackDocumentShutdown(input); + fallback = await fallbackDocumentShutdown(input); + input.resources.shutdownConfirmed = + fallback.ok && isHopperShutdownAcknowledgement(fallback.value); } } else if (shutdown.ok) { input.resources.shutdownConfirmed = isHopperShutdownAcknowledgement( shutdown.value, ); } + state.shutdownRequest = { + method, + status: !shutdown.ok + ? "failed" + : input.resources.shutdownConfirmed + ? "confirmed" + : "unconfirmed", + ...(shutdown.ok ? {} : shutdownErrorDetails(shutdown.error)), + ...(fallback === undefined + ? {} + : { + fallback: { + method: "shutdown_document", + status: !fallback.ok + ? "failed" + : input.resources.shutdownConfirmed + ? "confirmed" + : "unconfirmed", + ...(fallback.ok ? {} : shutdownErrorDetails(fallback.error)), + }, + }), + }; if ( !shutdown.ok || (!input.resources.shutdownConfirmed && state.cleanupResult === undefined) @@ -222,7 +270,7 @@ const requestShutdown = async ( const fallbackDocumentShutdown = async ( input: HopperCleanupInput, -): Promise => { +): Promise> => { const fallback = await input .request("shutdown_document") .catch((cause: unknown) => @@ -238,7 +286,7 @@ const fallbackDocumentShutdown = async ( }, "Hopper document shutdown fallback was not confirmed", ); - return confirmed; + return fallback; }; const shutdownRequestFailure = ( @@ -248,6 +296,16 @@ const shutdownRequestFailure = ( return new HopperProcessError(null, { operation, cause }); }; +/** Name the shutdown failure in retained-cleanup details without its cause text. */ +const shutdownErrorDetails = ( + error: HopperError, +): Record => ({ + error_tag: error._tag, + ...(error instanceof HopperProcessError + ? { provider_state: error.providerState } + : {}), +}); + const shutdownFailureDetails = ( error: HopperError, ): Record => ({ @@ -402,6 +460,12 @@ const cleanupOutcome = ( input.resources.externalApplication === undefined ? "not_observed" : input.resources.externalApplication.identity.state, + ...(state.shutdownRequest === undefined + ? {} + : { shutdown_request: state.shutdownRequest }), + ...(state.applicationExit === undefined + ? {} + : { application_exit: state.applicationExit }), } : {}), }, diff --git a/src/process/ProcessOwnershipObservation.ts b/src/process/ProcessOwnershipObservation.ts index 2b5f09c276..45e0cf68a6 100644 --- a/src/process/ProcessOwnershipObservation.ts +++ b/src/process/ProcessOwnershipObservation.ts @@ -1741,17 +1741,22 @@ export const observeProcessStartIdentity = async ( }; }; -/** Confirm whether the process that held an authenticated bridge has exited. */ +/** + * Confirm whether the process that held an authenticated bridge has exited. + * `exiting` is a positively observed native exit in progress (Linux + * PF_EXITING before state Z); like `present`, it does not confirm exit. + */ export const observeProcessStartIdentityExit = async ( pid: number, expectedIdentity: string, host: ProcessOwnershipHost = systemProcessOwnershipHost, -): Promise<"exited" | "present" | "unverified"> => { +): Promise<"exited" | "exiting" | "present" | "unverified"> => { try { const observed = await observeProcessStartIdentity(pid, host); if (observed === undefined) return "exited"; if (observed.state !== "readable") return "unverified"; - return observed.identity === expectedIdentity ? "present" : "exited"; + if (observed.identity !== expectedIdentity) return "exited"; + return processIdentityIsExiting(observed) ? "exiting" : "present"; } catch (cause: unknown) { rethrowInspectionFailure(cause); return "unverified"; diff --git a/src/process/ProcessStartIdentity.test.ts b/src/process/ProcessStartIdentity.test.ts index 000e921a16..2735c7059d 100644 --- a/src/process/ProcessStartIdentity.test.ts +++ b/src/process/ProcessStartIdentity.test.ts @@ -94,6 +94,26 @@ describe("selected native process identity", () => { }, ); + it("reports an observed exit in progress without confirming exit", async () => { + const exiting: ProcessOwnershipHost = { + ...host("start-1"), + processIdentities: async () => + new Map([ + [ + 42, + { + state: "readable", + identity: "start-1", + lifecycle: { state: "observed", lifecycle: "exiting" }, + }, + ], + ]), + }; + await expect( + observeProcessStartIdentityExit(42, "start-1", exiting), + ).resolves.toBe("exiting"); + }); + it("binds an omitted native PID to its directly observed run token", async () => { const omitted: ProcessOwnershipHost = { ...host("start-1"),