fix(rizin): retain failed TTY cleanup ownership (#9608)

fix(rizin): retain TTY admission cleanup ownership

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<string, RizinDebugSession>();
+  readonly #ttyResources = new ArtifactResourceScope();
   #activeStarts = 0;
   #sessionReservations = 0;
   readonly #environment: Readonly<NodeJS.ProcessEnv>;
@@ -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<Result<null, AnalysisError>> {
+    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 {

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<typeof import("node:tty")>();
+  return { ...actual, isatty: vi.fn(actual.isatty) };
+});
+
+vi.mock("node:fs/promises", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("node:fs/promises")>();
+  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<typeof import("node:fs/promises")>(
+        "node:fs/promises",
+      );
+    const closeFailure = new Error("TTY descriptor close refused");
+    let closeCalls = 0;
+    let closeFixture: (() => Promise<void>) | 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<typeof import("node:fs/promises")>(
+        "node:fs/promises",
+      );
+    vi.mocked(isatty).mockReturnValue(true);
+    const controller = new AbortController();
+    const reason = { cancelled: "during TTY close" };
+    let releaseFixture: (() => Promise<void>) | 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<typeof import("node:fs/promises")>(
+        "node:fs/promises",
+      );
+    vi.mocked(isatty).mockReturnValue(true);
+    let opens = 0;
+    let closeCalls = 0;
+    let allowClose = false;
+    let releaseFixture: (() => Promise<void>) | 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);
+  },
+);