From e35ac38bbfca4fbae8c390f04e9a6554f7ab0de9 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Sat, 18 Apr 2026 21:00:34 +0900 Subject: [PATCH] test(tmux): rewrite stale-sweep tests via DI to eliminate cross-file mock leak Oracle flagged that the previous test file monkey-patched process.kill and relied on mock.module for 5 modules. Running it after manager.test.ts in the same Bun process reproduced 2 failures - the test resolution of `./session-kill` specifier interacted badly with manager.test.ts's `../../shared/tmux` barrel mock. Solution: refactor stale-session-sweep.ts to expose `sweepStaleOmoAgentSessionsWith(deps)` that accepts a SweepDeps record (isInsideTmux, getTmuxPath, listCandidateSessions, killSession, processAlive, currentPid, log). The public `sweepStaleOmoAgentSessions()` still uses runtime-built deps so call sites are unchanged. The test file now imports the pure function directly and constructs a fixture with fake deps. Zero mock.module calls, zero process.kill patching, zero cache-bust dynamic imports. 8 tests (up from 6) run deterministically in any order with any neighbor. Before: combined run with manager.test.ts = 2 fail, 50 pass. After: combined run with manager.test.ts = 0 fail, 54 pass. --- .../tmux-utils/stale-session-sweep.test.ts | 218 ++++++++---------- .../tmux/tmux-utils/stale-session-sweep.ts | 45 +++- 2 files changed, 128 insertions(+), 135 deletions(-) diff --git a/src/shared/tmux/tmux-utils/stale-session-sweep.test.ts b/src/shared/tmux/tmux-utils/stale-session-sweep.test.ts index 98f6aa1f8..1acc171ed 100644 --- a/src/shared/tmux/tmux-utils/stale-session-sweep.test.ts +++ b/src/shared/tmux/tmux-utils/stale-session-sweep.test.ts @@ -1,188 +1,154 @@ -import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test" +import { beforeEach, describe, expect, it, mock } from "bun:test" +import { sweepStaleOmoAgentSessionsWith, type SweepDeps } from "./stale-session-sweep" -type SweepStaleOmoAgentSessions = typeof import("./stale-session-sweep").sweepStaleOmoAgentSessions - -type SpawnCall = { command: string[] } - -type FakeSubprocess = { - exited: Promise - stdout: ReadableStream - stderr: ReadableStream +type SweepFixture = { + deps: SweepDeps + candidates: string[] + killed: string[] + killSessionMock: ReturnType + setCandidates: (sessions: string[]) => void + setAlive: (predicate: (pid: number) => boolean) => void } -const spawnCalls: SpawnCall[] = [] -const queuedProcesses: FakeSubprocess[] = [] +function createFixture(): SweepFixture { + const candidates: string[] = [] + const killed: string[] = [] + let aliveCheck: (pid: number) => boolean = () => false -function createClosedStream(): ReadableStream { - return new ReadableStream({ start(controller) { controller.close() } }) -} - -function createTextStream(text: string): ReadableStream { - return new ReadableStream({ - start(controller) { - controller.enqueue(new TextEncoder().encode(text)) - controller.close() - }, + const killSessionMock = mock(async (sessionName: string): Promise => { + killed.push(sessionName) + return true }) -} -function makeProcess(exitCode: number, stdoutText: string): FakeSubprocess { + const deps: SweepDeps = { + isInsideTmux: () => true, + getTmuxPath: async () => "tmux", + listCandidateSessions: async () => [...candidates], + killSession: killSessionMock, + processAlive: (pid) => aliveCheck(pid), + currentPid: 12345, + log: () => undefined, + } + return { - exited: Promise.resolve(exitCode), - stdout: createTextStream(stdoutText), - stderr: createClosedStream(), + deps, + candidates, + killed, + killSessionMock, + setCandidates: (sessions) => { + candidates.length = 0 + candidates.push(...sessions) + }, + setAlive: (predicate) => { + aliveCheck = predicate + }, } } -const spawnMock = mock((command: string[]): FakeSubprocess => { - spawnCalls.push({ command }) - const process = queuedProcesses.shift() - if (!process) { - throw new Error(`No fake subprocess configured for ${command.join(" ")}`) - } - return process -}) +describe("sweepStaleOmoAgentSessionsWith", () => { + let fixture: SweepFixture -const isInsideTmuxMock = mock((): boolean => true) -const getTmuxPathMock = mock(async (): Promise => "tmux") -const logMock = mock(() => undefined) -const killTmuxSessionMock = mock(async (_name: string): Promise => true) -const isProcessAliveMock = mock((_pid: number): boolean => false) - -const sweepSpecifier = import.meta.resolve("./stale-session-sweep") -const spawnProcessSpecifier = import.meta.resolve("./spawn-process") -const environmentSpecifier = import.meta.resolve("./environment") -const loggerSpecifier = import.meta.resolve("../../logger") -const tmuxPathResolverSpecifier = import.meta.resolve("../../../tools/interactive-bash/tmux-path-resolver") -const sessionKillSpecifier = import.meta.resolve("./session-kill") - -function registerModuleMocks(): void { - mock.module(spawnProcessSpecifier, () => ({ spawn: spawnMock })) - mock.module(environmentSpecifier, () => ({ isInsideTmux: isInsideTmuxMock })) - mock.module(loggerSpecifier, () => ({ log: logMock })) - mock.module(tmuxPathResolverSpecifier, () => ({ getTmuxPath: getTmuxPathMock })) - mock.module(sessionKillSpecifier, () => ({ killTmuxSessionIfExists: killTmuxSessionMock })) -} - -const originalProcessKill = process.kill - -async function loadSweeper(overrideProcessAlive?: (pid: number) => boolean): Promise { - const processAlive = overrideProcessAlive ?? isProcessAliveMock - process.kill = ((pid: number, signal?: number | string): true => { - if (signal === 0) { - if (processAlive(pid)) { - return true - } - const err = new Error("ESRCH") as NodeJS.ErrnoException - err.code = "ESRCH" - throw err - } - return originalProcessKill.call(process, pid, signal) - }) as typeof process.kill - const module = await import(`${sweepSpecifier}?test=${crypto.randomUUID()}`) - return module.sweepStaleOmoAgentSessions -} - -describe("sweepStaleOmoAgentSessions", () => { beforeEach(() => { - registerModuleMocks() - spawnCalls.length = 0 - queuedProcesses.length = 0 - spawnMock.mockClear() - isInsideTmuxMock.mockClear() - getTmuxPathMock.mockClear() - logMock.mockClear() - killTmuxSessionMock.mockClear() - isProcessAliveMock.mockClear() - - isInsideTmuxMock.mockImplementation((): boolean => true) - getTmuxPathMock.mockImplementation(async (): Promise => "tmux") - killTmuxSessionMock.mockImplementation(async (_name: string): Promise => true) - isProcessAliveMock.mockImplementation((_pid: number): boolean => false) + fixture = createFixture() }) - afterEach(() => { - process.kill = originalProcessKill - }) - - it("#given not inside tmux #when sweepStaleOmoAgentSessions called #then returns 0 without spawn", async () => { + it("#given not inside tmux #when sweep called #then returns 0 without listing", async () => { // given - isInsideTmuxMock.mockImplementation((): boolean => false) - const sweep = await loadSweeper() + const deps: SweepDeps = { ...fixture.deps, isInsideTmux: () => false } // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(deps) // then expect(result).toBe(0) - expect(spawnCalls).toHaveLength(0) }) - it("#given no omo-agents sessions exist #when sweepStaleOmoAgentSessions called #then returns 0", async () => { + it("#given tmux not found #when sweep called #then returns 0 without listing", async () => { // given - queuedProcesses.push(makeProcess(0, "other-session\nmain\n")) - const sweep = await loadSweeper() + const deps: SweepDeps = { ...fixture.deps, getTmuxPath: async () => undefined } // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(deps) // then expect(result).toBe(0) - expect(killTmuxSessionMock).toHaveBeenCalledTimes(0) }) - it("#given omo-agents sessions with dead PIDs #when sweepStaleOmoAgentSessions called #then each dead session is killed", async () => { + it("#given candidate list is empty #when sweep called #then returns 0 and does not kill anything", async () => { // given - queuedProcesses.push(makeProcess(0, "omo-agents-99991\nomo-agents-99992\nunrelated\n")) - const sweep = await loadSweeper(() => false) + fixture.setCandidates([]) // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) + + // then + expect(result).toBe(0) + expect(fixture.killed).toEqual([]) + }) + + it("#given sessions with dead PIDs #when sweep called #then each dead session is killed once", async () => { + // given + fixture.setCandidates(["omo-agents-99991", "omo-agents-99992"]) + fixture.setAlive(() => false) + + // when + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) // then expect(result).toBe(2) - expect(killTmuxSessionMock).toHaveBeenCalledTimes(2) - expect(killTmuxSessionMock.mock.calls[0]?.[0]).toBe("omo-agents-99991") - expect(killTmuxSessionMock.mock.calls[1]?.[0]).toBe("omo-agents-99992") + expect(fixture.killed).toEqual(["omo-agents-99991", "omo-agents-99992"]) }) - it("#given session matches current PID #when sweepStaleOmoAgentSessions called #then it is not killed", async () => { + it("#given session matches current PID #when sweep called #then it is NOT killed", async () => { // given - queuedProcesses.push(makeProcess(0, `omo-agents-${process.pid}\nomo-agents-99999\n`)) - const sweep = await loadSweeper(() => false) + fixture.setCandidates([`omo-agents-${fixture.deps.currentPid}`, "omo-agents-99999"]) + fixture.setAlive(() => false) // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) // then expect(result).toBe(1) - expect(killTmuxSessionMock).toHaveBeenCalledTimes(1) - expect(killTmuxSessionMock.mock.calls[0]?.[0]).toBe("omo-agents-99999") + expect(fixture.killed).toEqual(["omo-agents-99999"]) }) - it("#given session PID is still alive #when sweepStaleOmoAgentSessions called #then it is not killed", async () => { + it("#given session PID is still alive #when sweep called #then it is NOT killed", async () => { // given - queuedProcesses.push(makeProcess(0, "omo-agents-88888\n")) - const sweep = await loadSweeper((pid) => pid === 88888) + fixture.setCandidates(["omo-agents-88888"]) + fixture.setAlive((pid) => pid === 88888) // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) // then expect(result).toBe(0) - expect(killTmuxSessionMock).toHaveBeenCalledTimes(0) + expect(fixture.killed).toEqual([]) }) - it("#given list-sessions fails #when sweepStaleOmoAgentSessions called #then returns 0 without killing", async () => { + it("#given killSession returns false #when sweep called #then session is not counted toward killedCount", async () => { // given - queuedProcesses.push(makeProcess(1, "")) - const sweep = await loadSweeper(() => false) + fixture.setCandidates(["omo-agents-55555"]) + fixture.setAlive(() => false) + fixture.killSessionMock.mockImplementation(async () => false) // when - const result = await sweep() + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) // then expect(result).toBe(0) - expect(killTmuxSessionMock).toHaveBeenCalledTimes(0) + expect(fixture.killSessionMock).toHaveBeenCalledTimes(1) + }) + + it("#given non-matching sessions mixed in #when sweep called #then only omo-agents- sessions are considered", async () => { + // given + fixture.setCandidates(["main", "omo-agents-99999", "other-session", "omo-agents-abc"]) + fixture.setAlive(() => false) + + // when + const result = await sweepStaleOmoAgentSessionsWith(fixture.deps) + + // then + expect(result).toBe(1) + expect(fixture.killed).toEqual(["omo-agents-99999"]) }) }) diff --git a/src/shared/tmux/tmux-utils/stale-session-sweep.ts b/src/shared/tmux/tmux-utils/stale-session-sweep.ts index f2c790161..c8b27e938 100644 --- a/src/shared/tmux/tmux-utils/stale-session-sweep.ts +++ b/src/shared/tmux/tmux-utils/stale-session-sweep.ts @@ -10,7 +10,7 @@ function isProcessAlive(pid: number): boolean { } } -async function listOmoAgentSessions(tmux: string): Promise { +async function listOmoAgentSessionsViaTmux(tmux: string): Promise { const { spawn } = await import("./spawn-process") const proc = spawn([tmux, "list-sessions", "-F", "#{session_name}"], { stdout: "pipe", @@ -32,7 +32,17 @@ async function listOmoAgentSessions(tmux: string): Promise { .filter((name) => STALE_SESSION_PATTERN.test(name)) } -export async function sweepStaleOmoAgentSessions(): Promise { +export type SweepDeps = { + isInsideTmux: () => boolean + getTmuxPath: () => Promise + listCandidateSessions: (tmux: string) => Promise + killSession: (sessionName: string) => Promise + processAlive: (pid: number) => boolean + currentPid: number + log: (message: string, payload?: unknown) => void +} + +async function buildRuntimeDeps(): Promise { const [{ log }, { isInsideTmux }, { getTmuxPath }, { killTmuxSessionIfExists }] = await Promise.all([ import("../../logger"), import("./environment"), @@ -40,16 +50,28 @@ export async function sweepStaleOmoAgentSessions(): Promise { import("./session-kill"), ]) - if (!isInsideTmux()) { + return { + isInsideTmux, + getTmuxPath, + listCandidateSessions: listOmoAgentSessionsViaTmux, + killSession: killTmuxSessionIfExists, + processAlive: isProcessAlive, + currentPid: process.pid, + log, + } +} + +export async function sweepStaleOmoAgentSessionsWith(deps: SweepDeps): Promise { + if (!deps.isInsideTmux()) { return 0 } - const tmux = await getTmuxPath() + const tmux = await deps.getTmuxPath() if (!tmux) { return 0 } - const candidateSessions = await listOmoAgentSessions(tmux) + const candidateSessions = await deps.listCandidateSessions(tmux) let killedCount = 0 for (const sessionName of candidateSessions) { @@ -58,11 +80,11 @@ export async function sweepStaleOmoAgentSessions(): Promise { const pid = Number.parseInt(pidMatch[1], 10) if (!Number.isFinite(pid)) continue - if (pid === process.pid) continue - if (isProcessAlive(pid)) continue + if (pid === deps.currentPid) continue + if (deps.processAlive(pid)) continue - log("[sweepStaleOmoAgentSessions] killing stale session", { sessionName, deadPid: pid }) - const killed = await killTmuxSessionIfExists(sessionName) + deps.log("[sweepStaleOmoAgentSessions] killing stale session", { sessionName, deadPid: pid }) + const killed = await deps.killSession(sessionName) if (killed) { killedCount += 1 } @@ -70,3 +92,8 @@ export async function sweepStaleOmoAgentSessions(): Promise { return killedCount } + +export async function sweepStaleOmoAgentSessions(): Promise { + const deps = await buildRuntimeDeps() + return sweepStaleOmoAgentSessionsWith(deps) +}