From e939cd04feae51ed4670f3068c8a32e0038ef8e2 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 15 Nov 2026 03:34:52 +0800 Subject: [PATCH] fix(hopper): name the unconfirmed shutdown step in retained cleanup (#10594) * fix(hopper): name the unconfirmed shutdown step in retained cleanup When close_binary retains a Hopper document, the cleanup error lists the retained resources but not why shutdown went unconfirmed. On Linux CI, hopperClient.handshakeRecovery "recovers an application exit after the first valid handshake occurs during cleanup" fails intermittently. The vitest blob reports show only hopper-document and the runtime root retained with authenticated_application "readable". From those facts we cannot tell whether the shutdown request was not sent, failed, or was sent while the application was still observed as present. Record the shutdown request outcome (method, status, error tag and provider state, or why it was not sent) and the final exit observation of the authenticated application in the retained-document diagnostics. Exit observation now reports a positively observed Linux exit in progress (PF_EXITING before state Z) as "exiting", separate from "present". Neither confirms exit. The flaky test asserts the projected error, so a later failure prints these diagnostics in the CI log. Refs #10556 * fix(hopper): retain fallback shutdown diagnostics * test(hopper): retry cleanup after confirmed application exit --- src/hopper/HopperCleanup.test.ts | 75 ++++++++++++++++++- src/hopper/HopperCleanup.ts | 84 +++++++++++++++++++--- src/process/ProcessOwnershipObservation.ts | 11 ++- src/process/ProcessStartIdentity.test.ts | 20 ++++++ 4 files changed, 175 insertions(+), 15 deletions(-) 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"),