diff --git a/src/rizin/RizinDebugSessionManager.ts b/src/rizin/RizinDebugSessionManager.ts index dd0b3749dc..87b2c452d9 100644 --- a/src/rizin/RizinDebugSessionManager.ts +++ b/src/rizin/RizinDebugSessionManager.ts @@ -5,6 +5,16 @@ import { isUtf8 } from "node:buffer"; import { open, readlink, realpath, stat } from "node:fs/promises"; import { constants as fsConstants, type Stats } from "node:fs"; import { isatty } from "node:tty"; +import { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; +import { bindArtifactFileCleanup } from "../artifacts/ArtifactFileOperation.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; +import { + RegularFileCleanupFailure, + regularFileCancellation, + withOwnedFileHandle, +} from "../filesystem/OwnedRegularFile.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { AnalysisCancelledError, @@ -246,6 +256,7 @@ class RizinFrameTimeoutError extends Error { /** Owns persistent Rizin debugger processes independently from BinarySession. */ export class RizinDebugSessionManager { readonly #sessions = new Map(); + readonly #ttyResources = new ArtifactResourceScope(); #activeStarts = 0; #sessionReservations = 0; readonly #environment: Readonly; @@ -374,38 +385,99 @@ export class RizinDebugSessionManager { let ttyPath: string; let ttyDevice: number; let ttyInode: number; + let ttyLease: AnalysisCleanupLease | undefined; + try { + await this.#ttyResources.retryCleanup(); + } catch (cause: unknown) { + const cleanup = new ProviderCleanupError( + "rizin", + this.#ttyResources.cleanupResources, + {}, + { operation: "start_rizin_debug_session", cause }, + ); + bindArtifactFileCleanup(cause, cleanup); + return err(cleanup); + } try { ttyPath = await realpath(input.inferior_tty); const ttyHandle = await open( ttyPath, fsConstants.O_RDWR | fsConstants.O_NOCTTY | fsConstants.O_NONBLOCK, ); - try { - const ttyStat = await ttyHandle.stat(); - if ( - !isatty(ttyHandle.fd) || - (ttyStat.mode & fsConstants.S_IFMT) !== fsConstants.S_IFCHR - ) - throw new Error("The selected inferior_tty is not a character TTY."); - ttyDevice = ttyStat.dev; - ttyInode = ttyStat.ino; - } finally { - await ttyHandle.close(); - } - } catch (cause) { - return err( - new AnalysisInputError( - "start_rizin_debug_session", - { cause, diagnostics: { inferior_tty: input.inferior_tty } }, - [ - { - path: ["inferior_tty"], - reason: "invalid_value", - message: `Cannot use selected terminal ${input.inferior_tty}: ${cause instanceof Error ? cause.message : String(cause)}`, - }, - ], - ), + const tty = await withOwnedFileHandle( + ttyPath, + new OwnedFileHandle(ttyHandle), + { + ...(signal === undefined ? {} : { signal }), + retainFailedClose: (owner) => { + ttyLease = new AnalysisCleanupLease(() => owner.close(), [ttyPath]); + this.#ttyResources.retain({ + kind: "lease", + resource: ttyPath, + lease: ttyLease, + }); + }, + }, + async (handle) => { + const ttyStat = await handle.stat(); + if ( + !isatty(handle.fd) || + (ttyStat.mode & fsConstants.S_IFMT) !== fsConstants.S_IFCHR + ) + throw new Error( + "The selected inferior_tty is not a character TTY.", + ); + return { device: ttyStat.dev, inode: ttyStat.ino }; + }, ); + ttyDevice = tty.device; + ttyInode = tty.inode; + } catch (cause) { + const primaryCause = + cause instanceof RegularFileCleanupFailure && + cause.outcome.kind === "failed" + ? cause.outcome.cause + : cause; + const cancellation = regularFileCancellation(cause); + const primary = + cancellation === undefined + ? new AnalysisInputError( + "start_rizin_debug_session", + { + cause: primaryCause, + diagnostics: { inferior_tty: input.inferior_tty }, + }, + [ + { + path: ["inferior_tty"], + reason: "invalid_value", + message: `Cannot use selected terminal ${input.inferior_tty}: ${primaryCause instanceof Error ? primaryCause.message : String(primaryCause)}`, + }, + ], + ) + : rizinCancellation( + "start_rizin_debug_session", + undefined, + cancellation.reason, + ); + if (!(cause instanceof RegularFileCleanupFailure)) return err(primary); + const cleanup = new ProviderCleanupError( + "rizin", + [ttyPath!], + {}, + { operation: "start_rizin_debug_session", cause: cause.cleanupCause }, + ); + const error = + cause.outcome.kind === "failed" || cancellation !== undefined + ? analysisErrorWithCleanupFailure( + primary, + cleanup, + "start_rizin_debug_session", + ) + : cleanup; + ttyLease?.bindCleanupError(cleanup); + if (error !== cleanup) ttyLease?.bindCleanupError(error); + return err(error); } // TTY identity validation is asynchronous. Cancellation may arrive while // realpath/open/stat are pending, and must prevent the debugger spawn. @@ -999,6 +1071,21 @@ export class RizinDebugSessionManager { } async retryCleanupFailures(): Promise> { + try { + await this.#ttyResources.retryCleanup(); + } catch (cause: unknown) { + const error = new ProviderCleanupError( + "rizin", + this.#ttyResources.cleanupResources, + {}, + { + operation: "start_rizin_debug_session", + cause, + }, + ); + bindArtifactFileCleanup(cause, error); + return err(error); + } for (const session of this.#sessions.values()) { if (session.cleanupFailures.size === 0) continue; const stopped = await this.#stopDebuggerOnly(session); @@ -1023,11 +1110,20 @@ export class RizinDebugSessionManager { this.#startDrainWaiters.push(resolve), ); for (const session of this.#sessions.values()) session.closing = true; - return closeOwnedDebuggerSessions( - this.#sessions, - (session) => this.#stopDebuggerOnly(session), - "Rizin", + const outcomes = await Promise.allSettled([ + closeOwnedDebuggerSessions( + this.#sessions, + (session) => this.#stopDebuggerOnly(session), + "Rizin", + ), + this.#ttyResources.close(), + ]); + const failures = outcomes.flatMap((outcome) => + outcome.status === "rejected" ? [outcome.reason] : [], ); + if (failures.length === 1) throw failures[0]; + if (failures.length > 1) + throw new AggregateError(failures, "Rizin cleanup remains incomplete"); } #receive(session: RizinDebugSession, chunk: Buffer): void { diff --git a/tests/boundary/providers/RizinTtyCleanup.test.ts b/tests/boundary/providers/RizinTtyCleanup.test.ts new file mode 100644 index 0000000000..04abee7475 --- /dev/null +++ b/tests/boundary/providers/RizinTtyCleanup.test.ts @@ -0,0 +1,197 @@ +import { isatty } from "node:tty"; +import { open } from "node:fs/promises"; +import { expect, it, onTestFinished, vi } from "vitest"; +import { createInjectedProcess } from "./RizinDebugSessionCoverage.fixtures.js"; +import { RizinDebugSessionManager } from "../../../src/rizin/RizinDebugSessionManager.js"; + +vi.mock("node:tty", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, isatty: vi.fn(actual.isatty) }; +}); + +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, open: vi.fn(actual.open) }; +}); + +it.skipIf(process.platform === "win32")( + "retains the TTY descriptor when its close fails", + async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + const closeFailure = new Error("TTY descriptor close refused"); + let closeCalls = 0; + let closeFixture: (() => Promise) | undefined; + let allowClose = false; + vi.mocked(open).mockImplementation(async (...args) => { + const handle = await actual.open(...args); + const close = handle.close.bind(handle); + closeFixture = close; + vi.spyOn(handle, "close").mockImplementation(async () => { + closeCalls++; + if (!allowClose) throw closeFailure; + await close(); + }); + return handle; + }); + onTestFinished(async () => { + await closeFixture?.().catch(() => undefined); + vi.restoreAllMocks(); + }); + const manager = new RizinDebugSessionManager( + {}, + { + platform: "linux", + spawnProcess: () => { + throw new Error("unexpected spawn"); + }, + }, + ); + const result = await manager.start({ + path: "/fixture", + inferior_tty: "/dev/null", + }); + if (result.ok) throw new Error("expected TTY admission failure"); + expect(result.error.cleanupIncomplete).toBe(true); + expect(result.error.cause).toBeInstanceOf(AggregateError); + const cleanupChild = (result.error.cause as AggregateError).errors[1]; + expect(cleanupChild.cleanupIncomplete).toBe(true); + expect(result.error.cleanupResources).toContain("/dev/null"); + expect(result.error.cleanupPrimary).toMatchObject({ + issues: [ + expect.objectContaining({ + message: expect.stringContaining("not a character TTY"), + }), + ], + }); + expect(closeCalls).toBe(1); + const retryFailure = await manager.retryCleanupFailures(); + if (retryFailure.ok) throw new Error("expected retry failure"); + expect(retryFailure.error.cleanupResources).toContain("/dev/null"); + expect(closeCalls).toBe(2); + allowClose = true; + expect((await manager.retryCleanupFailures()).ok).toBe(true); + expect(closeCalls).toBe(3); + expect(result.error.cleanupIncomplete).toBe(false); + expect(cleanupChild.cleanupIncomplete).toBe(false); + expect(retryFailure.error.cleanupIncomplete).toBe(false); + await manager.closeAll(); + }, +); + +it.skipIf(process.platform === "win32")( + "preserves cancellation during final TTY close with failed cleanup", + async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + vi.mocked(isatty).mockReturnValue(true); + const controller = new AbortController(); + const reason = { cancelled: "during TTY close" }; + let releaseFixture: (() => Promise) | undefined; + let closeCalls = 0; + vi.mocked(open).mockImplementation(async (...args) => { + const handle = await actual.open(...args); + const close = handle.close.bind(handle); + releaseFixture = close; + vi.spyOn(handle, "close").mockImplementation(async () => { + if (++closeCalls === 1) { + controller.abort(reason); + throw new Error("close denied"); + } + await close(); + }); + return handle; + }); + onTestFinished(async () => { + await releaseFixture?.().catch(() => undefined); + vi.restoreAllMocks(); + }); + const manager = new RizinDebugSessionManager( + {}, + { + platform: "linux", + spawnProcess: () => { + throw new Error("unexpected spawn"); + }, + }, + ); + const result = await manager.start( + { path: "/fixture", inferior_tty: "/dev/null" }, + controller.signal, + ); + if (result.ok) throw new Error("expected cancellation"); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + expect(result.error.cleanupIncomplete).toBe(true); + await manager.closeAll(); + expect(result.error.cleanupIncomplete).toBe(false); + }, +); + +it.skipIf(process.platform === "win32")( + "retries a failed TTY owner without stopping a released session again", + async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + vi.mocked(isatty).mockReturnValue(true); + let opens = 0; + let closeCalls = 0; + let allowClose = false; + let releaseFixture: (() => Promise) | undefined; + vi.mocked(open).mockImplementation(async (...args) => { + const handle = await actual.open(...args); + if (++opens === 2) { + const close = handle.close.bind(handle); + releaseFixture = close; + vi.spyOn(handle, "close").mockImplementation(async () => { + closeCalls++; + if (!allowClose) throw new Error("TTY close denied"); + await close(); + }); + } + return handle; + }); + const injected = createInjectedProcess(); + let stops = 0; + const manager = new RizinDebugSessionManager( + {}, + { + platform: "linux", + spawnProcess: injected.spawnProcess, + stopProcess: async () => { + stops++; + injected.control.exit(); + return { status: "stopped" }; + }, + }, + ); + onTestFinished(async () => { + injected.control.exit(); + await releaseFixture?.().catch(() => undefined); + vi.restoreAllMocks(); + }); + const first = await manager.start({ + path: "/fixture", + inferior_tty: "/dev/null", + }); + if (!first.ok) throw first.error; + const second = await manager.start({ + path: "/fixture", + inferior_tty: "/dev/null", + }); + if (second.ok) throw new Error("expected failed TTY close"); + await expect(manager.closeAll()).rejects.toThrow(); + expect(stops).toBe(1); + expect(closeCalls).toBe(2); + allowClose = true; + await manager.closeAll(); + expect(stops).toBe(1); + expect(closeCalls).toBe(3); + expect(second.error.cleanupIncomplete).toBe(false); + }, +);