Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions packages/coding-agent/src/beta/omo-local-update-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
// serializes against concurrent updates through the existing pid lock.

import { spawn } from "node:child_process";
import { closeSync, mkdirSync, openSync } from "node:fs";
import { mkdirSync } from "node:fs";
import { open } from "node:fs/promises";
import { dirname, join, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import { detectInstallMethod } from "../config.ts";
Expand All @@ -23,7 +24,9 @@ export type OmoLocalWorkerSpawnOutcome =
| { ok: true; pid: number | undefined; logPath: string }
| { ok: false; message: string };

export type OmoLocalSpawnWorker = (request: OmoLocalWorkerSpawnRequest) => OmoLocalWorkerSpawnOutcome;
export type OmoLocalSpawnWorker = (
request: OmoLocalWorkerSpawnRequest,
) => OmoLocalWorkerSpawnOutcome | Promise<OmoLocalWorkerSpawnOutcome>;

export function omoLocalUpdateWorkerLogPath(agentDir: string): string {
return join(agentDir, "omo-local-update", "worker.log");
Expand All @@ -40,21 +43,21 @@ function workerCommandArgs(force: boolean): string[] {
return [...process.execArgv, cliMainPath, ...updateArgs];
}

export const defaultSpawnWorker: OmoLocalSpawnWorker = (request) => {
export const defaultSpawnWorker: OmoLocalSpawnWorker = async (request) => {
const logPath = omoLocalUpdateWorkerLogPath(request.agentDir);
try {
mkdirSync(dirname(logPath), { recursive: true });
const logFd = openSync(logPath, "w");
const logFile = await open(logPath, "w");
try {
const child = spawn(process.execPath, workerCommandArgs(request.force), {
detached: true,
env: process.env,
stdio: ["ignore", logFd, logFd],
stdio: ["ignore", logFile.fd, logFile.fd],
});
child.unref();
return { ok: true, pid: child.pid, logPath };
} finally {
closeSync(logFd);
await logFile.close();
}
} catch (error) {
return { ok: false, message: error instanceof Error ? error.message : String(error) };
Expand Down
2 changes: 1 addition & 1 deletion packages/coding-agent/src/beta/omo-local-update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ export async function runOmoLocalUpdateBeta(options: RunOmoLocalUpdateBetaOption
if (dispatchRequested) {
releaseOmoLocalLock(lock);
const spawnWorker = options.spawnWorker ?? defaultSpawnWorker;
const spawned = spawnWorker({ agentDir: options.agentDir, force: options.force ?? false });
const spawned = await spawnWorker({ agentDir: options.agentDir, force: options.force ?? false });
if (spawned.ok) {
log(
chalk.dim(
Expand Down
21 changes: 21 additions & 0 deletions packages/coding-agent/src/changes.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,24 @@
## Clean up Windows update-worker and test temp state (2026-08-06)

### What changed

- The detached OMO local-plugin update worker now keeps its log file handle alive until asynchronous close completes, and the dispatcher awaits worker startup.
- Vitest worker quarantine directories are grouped under a per-run `mkdtempSync` root that global teardown removes after the worker pool stops; standalone setup imports retain a process-exit cleanup fallback.
- Regression coverage pins worker log-handle ordering and verifies both global teardown and naturally exiting standalone-worker cleanup.

### Why

- On Windows, synchronously closing the numeric log descriptor immediately after `spawn()` and `unref()` could race detached-child handle setup and trigger `UV_HANDLE_CLOSING` during process teardown.
- `test/setup.ts` created a unique `senpi-vitest-*` directory for each test worker but never removed it, so normal repeated test runs accumulated stale temp directories.

### Why this cannot be expressed externally

- Descriptor ownership belongs to the internal OMO update dispatcher, and the quarantine directory is owned by the Vitest process bootstrap.

### Expected merge conflict zones

- LOW: `src/beta/omo-local-update-worker.ts`, the dispatch branch in `src/beta/omo-local-update.ts`, `vitest.config.ts`, and the test setup files.

## Joined user aborts override system provenance (2026-08-05)

### What changed
Expand Down
19 changes: 19 additions & 0 deletions packages/coding-agent/test/global-setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { mkdtempSync, rmSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";

const QUARANTINE_ROOT_ENV = "SENPI_VITEST_QUARANTINE_ROOT";

export default function setup(): (() => void) | undefined {
if (process.env.SENPI_CODING_AGENT_DIR || process.env[QUARANTINE_ROOT_ENV]) return;

const quarantineRoot = mkdtempSync(join(tmpdir(), "senpi-vitest-"));
process.env[QUARANTINE_ROOT_ENV] = quarantineRoot;

return () => {
if (process.env[QUARANTINE_ROOT_ENV] === quarantineRoot) {
delete process.env[QUARANTINE_ROOT_ENV];
}
rmSync(quarantineRoot, { recursive: true, force: true });
};
}
89 changes: 89 additions & 0 deletions packages/coding-agent/test/omo-local-update-worker.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import type { ChildProcess } from "node:child_process";
import { mkdtempSync, rmSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { afterEach, describe, expect, it, vi } from "vitest";

const mocks = vi.hoisted(() => ({
open: vi.fn(),
spawn: vi.fn(),
}));

vi.mock("node:fs/promises", async (importOriginal) => ({
...(await importOriginal<typeof import("node:fs/promises")>()),
open: mocks.open,
}));

vi.mock("node:child_process", async (importOriginal) => ({
...(await importOriginal<typeof import("node:child_process")>()),
spawn: mocks.spawn,
}));

import { defaultSpawnWorker, omoLocalUpdateWorkerLogPath } from "../src/beta/omo-local-update-worker.ts";

describe("defaultSpawnWorker", () => {
let tempRoot: string | undefined;

afterEach(() => {
if (tempRoot) rmSync(tempRoot, { recursive: true, force: true });
tempRoot = undefined;
vi.clearAllMocks();
});

it("waits for the detached worker log handle to close asynchronously", async () => {
tempRoot = mkdtempSync(join(tmpdir(), "omo-local-update-worker-test-"));
const logPath = omoLocalUpdateWorkerLogPath(tempRoot);
const events: string[] = [];
let finishClose: (() => void) | undefined;
const close = vi.fn(
() =>
new Promise<void>((resolve) => {
events.push("close-start");
finishClose = () => {
events.push("close-end");
resolve();
};
}),
);
const child = {
pid: 4242,
unref: vi.fn(() => {
events.push("unref");
}),
} as unknown as ChildProcess;

mocks.open.mockResolvedValue({ fd: 41, close });
mocks.spawn.mockImplementation(() => {
events.push("spawn");
return child;
});

const outcome = Promise.resolve(defaultSpawnWorker({ agentDir: tempRoot, force: true }));
await vi.waitFor(() => expect(close).toHaveBeenCalledOnce());
let settled = false;
void outcome.then(() => {
settled = true;
});
await Promise.resolve();
expect(settled).toBe(false);

finishClose?.();
await expect(outcome).resolves.toEqual({
ok: true,
pid: 4242,
logPath,
});

expect(mocks.open).toHaveBeenCalledWith(logPath, "w");
expect(mocks.spawn).toHaveBeenCalledWith(
process.execPath,
expect.arrayContaining(["update", "--omo-local-update-worker", "--force"]),
expect.objectContaining({
detached: true,
stdio: ["ignore", 41, 41],
}),
);
expect(child.unref).toHaveBeenCalledOnce();
expect(events).toEqual(["spawn", "unref", "close-start", "close-end"]);
});
});
59 changes: 59 additions & 0 deletions packages/coding-agent/test/setup-cleanup.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { spawnSync } from "node:child_process";
import { existsSync } from "node:fs";
import { dirname } from "node:path";
import { describe, expect, it } from "vitest";
import globalSetup from "./global-setup.ts";

describe("Vitest agent-directory quarantine", () => {
it("removes its temporary root when the worker exits", () => {
const setupUrl = new URL("./setup.ts", import.meta.url).href;
const script = `await import(${JSON.stringify(setupUrl)}); console.log(process.env.SENPI_CODING_AGENT_DIR);`;
const env = { ...process.env };
delete env.SENPI_CODING_AGENT_DIR;
delete env.SENPI_VITEST_QUARANTINE_ROOT;

const child = spawnSync(
process.execPath,
["--experimental-strip-types", "--input-type=module", "--eval", script],
{
encoding: "utf8",
env,
},
);

expect(child.status, child.stderr).toBe(0);
const agentDir = child.stdout.trim();
expect(agentDir).toContain("senpi-vitest-");
expect(existsSync(dirname(agentDir))).toBe(false);
});
it("removes every worker quarantine through global teardown", () => {
const originalAgentDir = process.env.SENPI_CODING_AGENT_DIR;
const originalRoot = process.env.SENPI_VITEST_QUARANTINE_ROOT;
let teardown: (() => void) | undefined;
try {
delete process.env.SENPI_CODING_AGENT_DIR;
delete process.env.SENPI_VITEST_QUARANTINE_ROOT;
teardown = globalSetup();
const quarantineRoot = process.env.SENPI_VITEST_QUARANTINE_ROOT;
expect(quarantineRoot).toBeTypeOf("string");
expect(existsSync(quarantineRoot!)).toBe(true);

teardown?.();
teardown = undefined;
expect(existsSync(quarantineRoot!)).toBe(false);
expect(process.env.SENPI_VITEST_QUARANTINE_ROOT).toBeUndefined();
} finally {
teardown?.();
if (originalAgentDir === undefined) {
delete process.env.SENPI_CODING_AGENT_DIR;
} else {
process.env.SENPI_CODING_AGENT_DIR = originalAgentDir;
}
if (originalRoot === undefined) {
delete process.env.SENPI_VITEST_QUARANTINE_ROOT;
} else {
process.env.SENPI_VITEST_QUARANTINE_ROOT = originalRoot;
}
}
});
});
18 changes: 12 additions & 6 deletions packages/coding-agent/test/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* tools (e.g. tokscale) then mis-count them as real usage.
*/

import { mkdirSync } from "node:fs";
import { mkdirSync, mkdtempSync, rmSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";

Expand All @@ -19,11 +19,17 @@ for (const key of ["PI_RULES_DISABLED", "PI_RULES_MAX_RULE_CHARS", "PI_RULES_MAX

// Guarded so an explicit `SENPI_CODING_AGENT_DIR=...` env (CI / opt-in) wins.
if (!process.env.SENPI_CODING_AGENT_DIR) {
const quarantineDir = join(
tmpdir(),
`senpi-vitest-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}`,
"agent",
);
const sharedQuarantineRoot = process.env.SENPI_VITEST_QUARANTINE_ROOT;
const quarantineRoot = sharedQuarantineRoot ?? mkdtempSync(join(tmpdir(), "senpi-vitest-"));
const workerRoot = sharedQuarantineRoot
? mkdtempSync(join(sharedQuarantineRoot, `worker-${process.pid}-`))
: quarantineRoot;
const quarantineDir = join(workerRoot, "agent");
mkdirSync(quarantineDir, { recursive: true });
process.env.SENPI_CODING_AGENT_DIR = quarantineDir;
if (!sharedQuarantineRoot) {
process.once("exit", () => {
rmSync(quarantineRoot, { recursive: true, force: true });
});
}
}
1 change: 1 addition & 0 deletions packages/coding-agent/vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export default mergeConfig(
globals: true,
environment: "node",
testTimeout: 30000,
globalSetup: ["./test/global-setup.ts"],
setupFiles: ["./test/setup.ts"],
// Tests run offline by default; opt in with allowNetwork() from test/test-network-env.ts.
env: { PI_OFFLINE: "1" },
Expand Down