From f6752bbe2c4f32ffc8ac737f86cca5fe93e88f32 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 15 Nov 2026 18:05:34 +0800 Subject: [PATCH] refactor(process): let debugger session cleanup errors read a confirmed stop (#10805) * refactor(errors): read cleanup completion from dependencies instead of pushing it to dependents AnalysisError.cleanupIncomplete becomes a getter over the error's own unresolved state and its bound dependencies. The WeakRef dependents graph and resolveCleanup's push propagation are removed, and subclasses mark their initial state through markCleanupIncomplete instead of overriding a public mutable field. * fix(errors): settle bound dependencies before binding more A composite whose dependencies had released but whose status was never read stayed incomplete when a later dependency was bound. * refactor(errors): let errors read a lease group's release instead of resolving them (#10759) AnalysisCleanupLeaseGroup no longer calls resolveCleanup() on its error. The error holds the group as a release claim and reads cleanupReleased when asked, so completion follows the lease owner without a push. * chore(verify): count resolveCleanup and bindCleanupDependencies calls so they only shrink (#10763) * refactor(errors): let errors read a lease group's release instead of resolving them AnalysisCleanupLeaseGroup no longer calls resolveCleanup() on its error. The error holds the group as a release claim and reads cleanupReleased when asked, so completion follows the lease owner without a push. * chore(verify): count resolveCleanup and bindCleanupDependencies calls so they only shrink * refactor(process): let worker cleanup errors read their root epoch's release OwnedWorkerRuntime's root epoch is now an AnalysisCleanupReleaseClaim. Errors bound to it read its release when asked instead of the epoch pushing resolveCleanup() through weak references, and a runtime without a retained root binds an already-released owner. Removes the OwnedWorkerCleanupFailure structural type and four resolveCleanup() pushes; the counted baseline shrinks accordingly. * refactor(pcap): let pcap cleanup errors read command and snapshot release (#10792) Retained pcap commands, retained snapshot failures and snapshot completion descriptors now own their release and errors read it: commands and snapshot failures implement AnalysisCleanupReleaseClaim, and a failed completion-descriptor close is retried through an AnalysisCleanupLease bound to its error. Removes four resolveCleanup() pushes; the counted baseline shrinks accordingly. * refactor(process): let Darwin helper cleanup errors read their owner's release Darwin helper cache and run-token reader root cleanup now record an AnalysisCleanupRelease that every failure reports; a successful retry confirms it. Replaces the first-failure anchor that later failures depended on and the resolveCleanup() push. Removes two resolveCleanup() and two bindCleanupDependencies() calls; both counted baselines shrink. * refactor(ghidra): let Ghidra cleanup errors read the client's and checkpoint's release GhidraClient and GhidraCheckpoint bind each cleanup failure to an AnalysisCleanupRelease and confirm it when a later close or retry releases everything, instead of keeping a set of failures and pushing resolveCleanup() to each. Removes two resolveCleanup() calls; the counted baseline shrinks. * refactor(process): let debugger session cleanup errors read a confirmed stop OwnedDebuggerSessionRegistry binds each retained gdb or rizin cleanup failure to the session's AnalysisCleanupRelease and confirms it when a stop is confirmed, keeping the latest failure for retry reporting, instead of pushing resolveCleanup() to a set of failures. Removes one resolveCleanup() call; the counted baseline shrinks. * refactor(process): drop the debugger registry's unused closeOperation option Returning the latest retained failure removed the last reader of closeOperation; remove the option and the values gdb and rizin supplied. * refactor(server): let environment shutdown errors read a successful close (#10807) serveEnvironment binds each shutdown failure to an AnalysisCleanupRelease that a later successful close confirms, instead of pushing resolveCleanup() to a set of failures. The state-file retry test now also requires the earlier report to read the release. Removes one resolveCleanup() call; the counted baseline shrinks. --- .../verify/architecture-guard-baselines.json | 2 - src/gdb/GdbSessionManager.ts | 1 - src/process/OwnedDebuggerSessionRegistry.ts | 43 +++++++++---------- src/rizin/RizinDebugSessionManager.ts | 1 - src/server/environment/serveEnvironment.ts | 15 ++++--- .../environment/serveEnvironmentState.test.ts | 5 ++- 6 files changed, 33 insertions(+), 34 deletions(-) diff --git a/scripts/verify/architecture-guard-baselines.json b/scripts/verify/architecture-guard-baselines.json index 6e5d9c2955..85a9ae47a3 100644 --- a/scripts/verify/architecture-guard-baselines.json +++ b/scripts/verify/architecture-guard-baselines.json @@ -330,9 +330,7 @@ "src/kuna/KunaProjectCache.ts": 1, "src/native/CommandRunner.ts": 1, "src/native/LldbCallTracer.ts": 1, - "src/process/OwnedDebuggerSessionRegistry.ts": 1, "src/providers/AnalysisClientFailure.ts": 1, - "src/server/environment/serveEnvironment.ts": 1, "src/sqlite/SqliteDatabaseProvider.ts": 2 }, "cleanup-dependency-binding": { diff --git a/src/gdb/GdbSessionManager.ts b/src/gdb/GdbSessionManager.ts index ddf9d49882..c54e9e78c7 100644 --- a/src/gdb/GdbSessionManager.ts +++ b/src/gdb/GdbSessionManager.ts @@ -455,7 +455,6 @@ export class GdbSessionManager { this.#sessions = new OwnedDebuggerSessionRegistry({ provider: "gdb", displayName: "GDB", - closeOperation: "close_gdb_session", sealSession: (session) => { session.cleanupStarted = true; }, diff --git a/src/process/OwnedDebuggerSessionRegistry.ts b/src/process/OwnedDebuggerSessionRegistry.ts index ba207f3219..a6df2c1844 100644 --- a/src/process/OwnedDebuggerSessionRegistry.ts +++ b/src/process/OwnedDebuggerSessionRegistry.ts @@ -3,6 +3,7 @@ import type { AnalysisErrorOptions, } from "../domain/analysisErrorBase.js"; import { err, ok, type Result } from "../domain/result.js"; +import { AnalysisCleanupRelease } from "../domain/analysisCleanupRelease.js"; import { closeOwnedDebuggerSessions, retainedDebuggerCleanupFailure, @@ -23,8 +24,6 @@ export interface OwnedDebuggerSessionRegistryOptions { readonly provider: string; /** Debugger name shown in cleanup failures, such as "GDB". */ readonly displayName: string; - /** Operation a retained failure names when no caller operation applies. */ - readonly closeOperation: string; /** Seal the session against new commands; runs on every stop request. */ readonly sealSession: (session: Session) => void; /** Stop the owned debugger process; protocol hooks stay with the adapter. */ @@ -45,7 +44,14 @@ export class OwnedDebuggerSessionRegistry< > { readonly #options: OwnedDebuggerSessionRegistryOptions; readonly #sessions = new Map(); - readonly #cleanupFailures = new WeakMap>(); + /** + * Each session's unconfirmed stop: the release its retained failures read, + * and the latest of them. + */ + readonly #cleanupFailures = new WeakMap< + Session, + { readonly release: AnalysisCleanupRelease; latest: AnalysisError } + >(); readonly #stopAttempts = new WeakMap< Session, Promise @@ -116,9 +122,8 @@ export class OwnedDebuggerSessionRegistry< })) .then((stopped) => { if (stopped.status !== "incomplete") { - const retained = this.#cleanupFailures.get(session); - for (const failure of retained ?? []) failure.resolveCleanup(); - retained?.clear(); + this.#cleanupFailures.get(session)?.release.confirm(); + this.#cleanupFailures.delete(session); } return stopped; }) @@ -132,13 +137,13 @@ export class OwnedDebuggerSessionRegistry< /** Keep a failure whose cleanup is incomplete until a stop is confirmed. */ retain(session: Session, failure: AnalysisError): AnalysisError { - let retained = this.#cleanupFailures.get(session); - if (retained === undefined) { - retained = new Set(); - this.#cleanupFailures.set(session, retained); - } - retained.add(failure); - return failure; + const retained = this.#cleanupFailures.get(session) ?? { + release: new AnalysisCleanupRelease(), + latest: failure, + }; + retained.latest = failure; + this.#cleanupFailures.set(session, retained); + return retained.release.bind(failure); } /** Retain the failure of an operation that could not confirm the stop. */ @@ -174,17 +179,9 @@ export class OwnedDebuggerSessionRegistry< ): Promise> { for (const session of this.#sessions.values()) { const retained = this.#cleanupFailures.get(session); - if (retained === undefined || retained.size === 0) continue; + if (retained === undefined) continue; const stopped = await this.stop(session); - if (stopped.status === "incomplete") - return err( - [...retained].at(-1) ?? - this.retainCleanupFailure( - session, - this.#options.closeOperation, - stopped.reason, - ), - ); + if (stopped.status === "incomplete") return err(retained.latest); if (!keepAddressable(session)) this.remove(session); } return ok(null); diff --git a/src/rizin/RizinDebugSessionManager.ts b/src/rizin/RizinDebugSessionManager.ts index 2621354205..dd630f1755 100644 --- a/src/rizin/RizinDebugSessionManager.ts +++ b/src/rizin/RizinDebugSessionManager.ts @@ -312,7 +312,6 @@ export class RizinDebugSessionManager { this.#sessions = new OwnedDebuggerSessionRegistry({ provider: "rizin", displayName: "Rizin", - closeOperation: "close_rizin_debug_session", sealSession: (session) => { session.closing = true; }, diff --git a/src/server/environment/serveEnvironment.ts b/src/server/environment/serveEnvironment.ts index 1332ddab81..dcaf960948 100644 --- a/src/server/environment/serveEnvironment.ts +++ b/src/server/environment/serveEnvironment.ts @@ -29,6 +29,7 @@ import { type EnvironmentToolDefinition, } from "../../application/environment/environmentTools.js"; import { projectAnalysisError } from "../../domain/analysisErrorProjection.js"; +import { AnalysisCleanupRelease } from "../../domain/analysisCleanupRelease.js"; import { safeParseJson } from "../../domain/safeJson.js"; import { AnalysisOutputError, @@ -519,7 +520,8 @@ export const serveEnvironment = async ( let responseClosePending = false; let settledSuccessful = false; let closing: Promise | undefined; - const shutdownFailures = new Set(); + // Every shutdown failure reports this release until a close succeeds. + let shutdownRelease: AnalysisCleanupRelease | undefined; const owner: EnvironmentShutdownOwner = { get cleanupResources() { const stateResources = stateFiles.cleanupResources; @@ -598,12 +600,13 @@ export const serveEnvironment = async ( "Keep this REA process available for cleanup. Confirm the reported resources are released before starting a replacement environment server.", }, ).bindCleanupResourceOwner(owner); - shutdownFailures.add(failure); - throw failure; + throw (shutdownRelease ??= new AnalysisCleanupRelease()).bind( + failure, + ); } retainedEnvironmentShutdowns.delete(owner); - for (const failure of shutdownFailures) failure.resolveCleanup(); - shutdownFailures.clear(); + shutdownRelease?.confirm(); + shutdownRelease = undefined; settledSuccessful = true; })(); closing = attempt; @@ -625,7 +628,7 @@ export const serveEnvironment = async ( closing = undefined; } } else if ( - shutdownFailures.size === 0 && + shutdownRelease === undefined && !httpClosePending && !stateClosePending ) { diff --git a/src/server/environment/serveEnvironmentState.test.ts b/src/server/environment/serveEnvironmentState.test.ts index 5a2fd433bd..8559a1efac 100644 --- a/src/server/environment/serveEnvironmentState.test.ts +++ b/src/server/environment/serveEnvironmentState.test.ts @@ -1229,7 +1229,8 @@ describe("serveEnvironment state file cleanup", () => { await expect( add({ kind: "data", data: renamedDocument() }), ).resolves.toMatchObject({ result: {} }); - await expect(environment.close()).rejects.toMatchObject({ + const reported = await environment.close().catch((cause: unknown) => cause); + expect(reported).toMatchObject({ _tag: "AnalysisOutputError", diagnostics: { cleanup_stages: ["state-files"] }, cause: expect.any(ArtifactResourceRetryFailure), @@ -1239,6 +1240,8 @@ describe("serveEnvironment state file cleanup", () => { failing = false; await expect(environment.close()).resolves.toBeUndefined(); expect(owners[0]?.handle.fd).toBe(-1); + // The earlier report reads the confirmed release instead of staying open. + expect(reported).toMatchObject({ cleanupIncomplete: false }); }); });