refactor(server): extract IdleReaper; one owner for the close deadline

Quality audit of my own feature. Four structural problems, all self-inflicted.

1. The idle sweeper was inlined into SessionManager: +204 lines onto an
   already-1472-line file, plus 4 constructor options (idleCloseMs, idleSweepMs,
   setTimer, clearTimer), 6 private fields and its own re-entrancy flag. It is a
   periodic policy collaborator, which this codebase already has a shape for —
   MetricsCollector and the ports service both own their window, clock and
   injected timer and are wired in server.ts. Now `idle_reaper.ts` does the same,
   reaching the session world through a two-function seam. manager.ts drops back
   to +59 over base, ManagerOpts loses 4 options, and serve.ts returns to
   untouched (the env parsing lives with the module that owns the policy).

   The tests got the bigger win: the reaper reads a handful of session fields, so
   they now use plain object doubles instead of standing up a SessionManager,
   SQLite store and temp dir per case. Each case states exactly the session shape
   it is about and nothing else can affect the outcome.

2. Two nested close deadlines with one caller between them. The adapter bounded
   its own `session/close` AND the manager bounded `adapter.close()` — which is
   how the previous commit shipped them inverted, and why it needed an invariant
   test to hold them in order. Defence in depth against nobody: `closeSession` is
   the only caller. Deleted the adapter-level deadline, the constant, the option,
   the invariant test, and `bestEffort()` (a wrapper with one call site once the
   redundancy was gone). Adapters now issue the plain request and MAY reject or
   hang; the manager bounds and swallows it in one place, for every back end, and
   reaps either way. The contract is stated on `AgentAdapter.close` instead of
   half-implemented in three adapters.

3. `deadline.ts` was created as the canonical home for bounded agent waits and
   then acp.ts's three pre-existing ad-hoc timeouts were left in place — four ways
   to do one thing. Converged onto `withDeadline`, deleting the local `withTimeout`
   closure and two raw `Promise.race` blocks; acp.ts shrinks despite gaining the
   close path.

4. A triple-negative status check (`!== "running" && !== "awaiting-input" &&
   !== "awaiting-approval"`) was the only place asking "is this agent mid-flight?"
   — a missing model, not a formatting problem. Now `isBusy(status)` next to the
   SessionStatus type it interrogates, and `Session.cold` replaces two
   `instanceof DetachedAdapter` checks.

Also caught by the type checker mid-refactor: I had added a second `allSessions()`
returning Iterable when a canonical one returning Session[] already existed, which
broke its existing callers. Removed; the reaper reuses the canonical accessor.

No behaviour change. 1297 server tests green, and the stub e2e still proves
close -> not active -> in the Closed list -> a message transparently reopens.

Author: leduckhc

server/src/adapters/acp.test.ts

@@ -1312,10 +1312,12 @@ test("close() is a no-op when the agent does not advertise session/close", async
 });
 
 /**
- * A close that rejects (agent already wedged) must never block teardown — the
- * caller still has to reap the process, or we are back to leaking RSS.
+ * The adapter makes the plain request and lets a failure surface;
+ * `SessionManager.closeSession` owns bounding and swallowing it (and reaps
+ * regardless). Asserted so the contract cannot quietly drift back to each
+ * adapter half-handling teardown policy on its own.
  */
-test("close() swallows a failing session/close so teardown can proceed", async () => {
+test("close() propagates a failing session/close for the manager to absorb", async () => {
   const { transport } = pair((conn) => {
     const a = new ScriptedAgent(conn, async () => {});
     (a as unknown as { initialize: () => Promise<unknown> }).initialize = async () => ({
@@ -1331,7 +1333,7 @@ test("close() swallows a failing session/close so teardown can proceed", async (
   const adapter = new AcpAdapter({ spec: { agent: "codex", command: "x" }, connect: () => transport });
   await adapter.start({ cwd: "/tmp" });
 
-  await adapter.close(); // must not throw
+  await assert.rejects(() => adapter.close());
 });
 
 test("start({resumeAgentSessionId}) loads and drops the replayed history (silent mode, SPEC-29)", async () => {
@@ -1573,33 +1575,3 @@ test("start() leaves the agent's model alone when it already matches, or is unkn
   await a2.kill();
 });
 
-/**
- * A wedged agent is the single most important case for close-then-reap: it is
- * exactly the agent that needs killing. If `session/close` never answers,
- * `close()` must still settle, or `manager.closeSession` never reaches `kill()`
- * — the RSS is never reclaimed and the idle sweeper stays wedged forever.
- */
-test("close() gives up on a session/close that never answers", async () => {
-  const { transport } = pair((conn) => {
-    const a = new ScriptedAgent(conn, async () => {});
-    (a as unknown as { initialize: () => Promise<unknown> }).initialize = async () => ({
-      protocolVersion: 1 as const,
-      agentCapabilities: { sessionCapabilities: { close: {} } },
-    });
-    // Never resolves — a hung agent, not a rejecting one.
-    (a as unknown as { closeSession: () => Promise<unknown> }).closeSession = () =>
-      new Promise(() => {});
-    return a;
-  });
-
-  const adapter = new AcpAdapter({
-    spec: { agent: "codex", command: "x" },
-    connect: () => transport,
-    closeTimeoutMs: 30,
-  });
-  await adapter.start({ cwd: "/tmp" });
-
-  const started = Date.now();
-  await adapter.close(); // must settle, not hang
-  assert.ok(Date.now() - started < 2000, "close() must not block on a hung agent");
-});

server/src/adapters/acp.ts

@@ -37,7 +37,6 @@ import { sharedMediaStore, type MediaStore } from "../media/store.js";
 import { LocalMediaResolver, rewriteMarkdownImages } from "../media/local.js";
 import { prepareTurnOrFail } from "../media/attach.js";
 import { spawnLineProcess } from "./child_transport.js";
-import { bestEffort } from "./deadline.js";
 import { onPath } from "./catalog.js";
 import { mapElicitation, type ElicitationParams } from "./interaction.js";
 import { isRecord } from "./wire.js";
@@ -51,16 +50,6 @@ import { log } from "../log.js";
  */
 const ACP_HANDSHAKE_TIMEOUT = 15_000;
 
-/**
- * Deadline for the graceful `session/close` (SPEC-29). Deliberately its own
- * constant rather than reusing {@link ACP_HANDSHAKE_TIMEOUT}: closing is a
- * teardown courtesy, not a launch step, and it MUST stay strictly below the
- * manager's `DEFAULT_CLOSE_GRACE_MS` backstop. If it were the looser of the two
- * it could never fire on the normal `closeSession` path — the manager would
- * abandon the call first and `kill()` would dispose the transport out from under
- * a still-pending timer. Asserted by a test in `manager.test.ts`.
- */
-export const ACP_CLOSE_TIMEOUT = 8_000;
 
 export interface AcpSpawnSpec {
   /** makit agent label surfaced in the session DTO ("pi", "codex", …). */
@@ -91,12 +80,6 @@ export interface AcpAdapterOpts {
    * spawning a subprocess. Production leaves this unset → real subprocess.
    */
   connect?: (cwd: string, env: Record<string, string>) => AcpTransport;
-  /**
-   * Deadline for the graceful `session/close` (default
-   * {@link ACP_CLOSE_TIMEOUT}). A hung agent must not be able to stall teardown
-   * — see {@link AcpAdapter.close}. Tests shorten it.
-   */
-  closeTimeoutMs?: number;
   /**
    * Blob store for assistant display media (SPEC-22). Defaults to the shared
    * `~/.makit/media` store the `/media` route serves from; tests inject a
@@ -109,8 +92,6 @@ export class AcpAdapter extends SubprocessAdapter {
   readonly agent: string;
 
   private readonly spec: AcpSpawnSpec;
-  /** Deadline for the graceful `session/close` (see {@link close}). */
-  private readonly closeTimeoutMs: number;
   private readonly connectFn: (cwd: string, env: Record<string, string>) => AcpTransport;
   /**
    * True when this adapter spawns {@link AcpSpawnSpec.command} for real, so
@@ -157,7 +138,6 @@ export class AcpAdapter extends SubprocessAdapter {
     this.agent = opts.spec.agent;
     this.connectFn = opts.connect ?? defaultConnect(opts.spec);
     this.spawnsRealBinary = opts.connect === undefined;
-    this.closeTimeoutMs = opts.closeTimeoutMs ?? ACP_CLOSE_TIMEOUT;
     const media = (this.media = opts.media ?? sharedMediaStore());
     this.mapper = new AcpEventMapper({
       emit: (e) => this.emit("event", e),
@@ -520,25 +500,16 @@ export class AcpAdapter extends SubprocessAdapter {
   }
 
   /**
-   * Graceful ACP `session/close`: cancels any in-flight turn agent-side and
-   * frees the session's resources, leaving it listable/resumable. Best-effort
-   * by contract — neither a rejection NOR a hang here may stop {@link kill} from
-   * reclaiming the process, which is what actually returns the memory.
-   *
-   * The deadline is the load-bearing part: a wedged agent is exactly the one
-   * that needs killing, and `conn.closeSession()` against it never settles. An
-   * unbounded await would strand teardown before `kill()` and leave the caller's
-   * sweep permanently in-flight. Mirrors codex's bounded `thread/unsubscribe`.
+   * ACP `session/close`: cancels any in-flight turn agent-side and frees the
+   * session's resources, leaving it listable/resumable. Plain request — a hung
+   * or rejecting agent is `SessionManager.closeSession`'s problem, and it bounds
+   * this call for every back end rather than each adapter re-implementing (and
+   * having to keep in step with) the same deadline.
    */
   async close(): Promise<void> {
     if (!this.conn || !this.acpSessionId) return;
     if (!this.capabilities.close || !this.conn.closeSession) return;
-    await bestEffort(
-      () => this.conn!.closeSession({ sessionId: this.acpSessionId! }).then(() => undefined),
-      this.closeTimeoutMs,
-      "ACP session/close",
-      (why) => log.warn(`[makit] ACP session/close gave up: ${why}`),
-    );
+    await this.conn.closeSession({ sessionId: this.acpSessionId });
   }
 
   async kill(): Promise<void> {

server/src/adapters/adapter.ts

@@ -171,14 +171,16 @@ export interface AgentAdapter extends EventEmitter {
   sendAction?(action: string, args?: Record<string, unknown>): Promise<void>;
   cancel(): Promise<void>;
   /**
-   * Gracefully release the agent-side session before the process is reaped
-   * (ACP `session/close`, codex `thread/unsubscribe`). Per ACP this implies a
-   * cancel of any in-flight turn, then frees the session's resources while
-   * leaving it listable and resumable.
+   * Ask the agent to release this session before its process is reaped (ACP
+   * `session/close`, codex `thread/unsubscribe`). Per ACP this implies a cancel
+   * of any in-flight turn, then frees the session's resources while leaving it
+   * listable and resumable. A back end without the capability no-ops.
    *
-   * MUST NOT throw: it is the courtesy half of a teardown whose second half
-   * ({@link kill}) is what actually reclaims memory, so a wedged agent must not
-   * be able to block the reap. A back end without the capability no-ops.
+   * MAY reject and MAY hang: this is the courtesy half of a teardown whose
+   * second half ({@link kill}) is what actually reclaims memory.
+   * `SessionManager.closeSession` is the single owner of that policy — it bounds
+   * this call and swallows the outcome, then reaps regardless — so adapters
+   * implement the plain request and nothing more.
    */
   close(): Promise<void>;
   kill(signal?: NodeJS.Signals): Promise<void>;

server/src/adapters/codex.test.ts

@@ -869,13 +869,13 @@ test("close() before start is a no-op (no thread to unsubscribe)", async () => {
 });
 
 /**
- * A wedged codex must not be able to block the reap — otherwise the RSS this
- * whole path exists to reclaim stays held.
+ * As with ACP: the adapter just issues `thread/unsubscribe`. Bounding and
+ * swallowing belong to `SessionManager.closeSession`, which reaps either way.
  */
-test("close() swallows a failing thread/unsubscribe", async () => {
+test("close() propagates a failing thread/unsubscribe for the manager to absorb", async () => {
   const fake = fakeAppServer({ unsubscribe: () => ({ error: { code: -32603, message: "boom" } }) });
   const adapter = new CodexAppServerAdapter({ connect: () => fake.transport });
   await adapter.start({ cwd: "/tmp" });
 
-  await adapter.close(); // must not throw
+  await assert.rejects(() => adapter.close());
 });

server/src/adapters/codex.ts

@@ -365,15 +365,9 @@ export class CodexAppServerAdapter extends SubprocessAdapter {
    */
   async close(): Promise<void> {
     if (!this.transport || !this.threadId) return;
-    try {
-      await withDeadline(
-        this.request("thread/unsubscribe", { threadId: this.threadId }),
-        CODEX_HANDSHAKE_TIMEOUT,
-        "codex thread/unsubscribe",
-      );
-    } catch (e) {
-      log.warn(`[makit] codex thread/unsubscribe failed: ${(e as Error).message}`);
-    }
+    // Plain request: `request()` already carries its own per-call timeout, and
+    // `SessionManager.closeSession` owns the bound-and-swallow policy.
+    await this.request("thread/unsubscribe", { threadId: this.threadId });
   }
 
   async kill(): Promise<void> {

server/src/adapters/deadline.ts

@@ -1,11 +1,11 @@
 /**
- * Deadline helpers shared by the adapters and the manager.
+ * Bounded waits on an agent, shared by the adapters and the manager.
  *
- * Extracted from `codex.ts` when `AgentAdapter.close()` landed: a graceful
- * agent-side release is only ever best-effort, so every wait on an agent must be
- * bounded. An unbounded one turns "release then reap" into "hang forever and
- * never reap" — precisely the RSS leak the close path exists to fix, in the one
- * case that matters most (a wedged agent is the one that needs killing).
+ * Extracted from `codex.ts` when `AgentAdapter.close()` landed: every wait on an
+ * agent must be bounded. An unbounded one turns "release then reap" into "hang
+ * forever and never reap" — precisely the RSS leak the close path exists to fix,
+ * in the one case that matters most, since a wedged agent is the one that needs
+ * killing.
  */
 
 /** Thrown when a bounded wait on an agent does not settle in time. */
@@ -24,23 +24,3 @@ export function withDeadline<T>(p: Promise<T>, ms: number, label: string): Promi
   });
   return Promise.race([p, deadline]).finally(() => clearTimeout(timer));
 }
-
-/**
- * Await [p], but never for longer than [ms] and never throwing: on timeout or
- * rejection the reason is handed to [onGaveUp] and the call resolves anyway.
- *
- * For teardown steps that are courtesies rather than requirements — the caller
- * has a hard fallback (killing the process) that MUST still run.
- */
-export async function bestEffort(
-  p: () => Promise<void>,
-  ms: number,
-  label: string,
-  onGaveUp: (why: string) => void,
-): Promise<void> {
-  try {
-    await withDeadline(p(), ms, label);
-  } catch (e) {
-    onGaveUp(e instanceof Error ? e.message : String(e));
-  }
-}