From 4701122342929c8aa6a59b7e5f9bdc1dc1721d90 Mon Sep 17 00:00:00 2001 From: Madhav Kumar <77185670+miniMaddy@users.noreply.github.com> Date: Wed, 29 Apr 2026 01:24:55 +0530 Subject: [PATCH] fix: reduce opencode session list churn (#1478) * fix: reduce opencode session list churn * fix: make bun-tmp-janitor cross-platform and move to process-level boot - Extend platform support from Linux-only to Linux + macOS (win32 skipped since opencode ships no Windows binary and the kernel disallows unlinking mapped files there) - Use os.tmpdir() instead of hardcoded /tmp to handle macOS $TMPDIR paths - Extend file pattern from \.so to \.(so|dylib) to cover macOS dylib leaks - Move startBunTmpJanitor() from ensureLifecycleWorker() (per-project) to the process-level boot in registerStart() immediately after register(), where the single-instance contract is already in force - Drop the project-observer-bound onSweep closure that incorrectly attributed janitor health to whichever project happened to start first; replaced with a simple stderr warn on errors (no project context needed for a process-wide sweep of /tmp) - Move stopBunTmpJanitor() into the SIGINT/SIGTERM shutdown handler in registerStart() alongside stopAllLifecycleWorkers() - Remove startBunTmpJanitor/stopBunTmpJanitor from lifecycle-service.ts entirely; lifecycle workers have no business knowing about a process-wide OS resource * fix(opencode): address PR #1478 review (TMPDIR isolation, shared cache, janitor cleanup) Implements all seven findings from the PR #1478 review: Core / agent-opencode: - New @aoagents/ao-core/opencode-shared module owns the single TTL cache + in-flight dedup for 'opencode session list' (was duplicated across core and the plugin, doubling spawns per poll cycle). - TTL dropped from 3s to 500ms so the send-confirmation loop's updatedAt > baselineUpdatedAt delivery signal can actually fire. - New invalidateOpenCodeSessionListCache() called by deleteOpenCodeSession so reuse / remap / restore code paths cannot observe a deleted id. - New getOpenCodeChildEnv() / getOpenCodeTmpDir(): every opencode child spawned by core, the plugin, or the agent runtime points TMPDIR/TMP/TEMP at ~/.agent-orchestrator/.bun-tmp. Bounds the janitor's blast radius to AO-owned files even on shared hosts. CLI janitor: - Sweeps only the AO-owned tmp dir (not the system /tmp). - Filters synchronously before spawning per-entry stat/unlink work. - stopBunTmpJanitor() is now async and awaits any in-flight sweep so SIGTERM cannot exit while unlink() is mid-flight; start.ts shutdown handler awaits it. - onSweep callback in start.ts now logs successful reclaims, not just errors, so operators can confirm the janitor is doing useful work. Tests: - packages/core/__tests__/opencode-shared.test.ts (TTL contract, TMPDIR location, env merge semantics). - packages/cli/__tests__/lib/bun-tmp-janitor.test.ts (sweep behavior, stop-awaits-in-flight, pattern matching, missing-dir tolerance). * chore: remove review postmortem artifact * fix(cli): remove start command non-null assertions --- .../opencode-tmpdir-and-shared-cache.md | 37 +++ .../cli/__tests__/lib/bun-tmp-janitor.test.ts | 122 +++++++++ packages/cli/src/commands/start.ts | 53 +++- packages/cli/src/lib/bun-tmp-janitor.ts | 145 +++++++++++ .../src/__tests__/opencode-shared.test.ts | 68 +++++ packages/core/src/__tests__/test-utils.ts | 2 + packages/core/src/index.ts | 10 + packages/core/src/opencode-shared.ts | 235 ++++++++++++++++++ packages/core/src/session-manager.ts | 49 ++-- .../plugins/agent-opencode/src/index.test.ts | 3 +- packages/plugins/agent-opencode/src/index.ts | 46 ++-- 11 files changed, 700 insertions(+), 70 deletions(-) create mode 100644 .changeset/opencode-tmpdir-and-shared-cache.md create mode 100644 packages/cli/__tests__/lib/bun-tmp-janitor.test.ts create mode 100644 packages/cli/src/lib/bun-tmp-janitor.ts create mode 100644 packages/core/src/__tests__/opencode-shared.test.ts create mode 100644 packages/core/src/opencode-shared.ts diff --git a/.changeset/opencode-tmpdir-and-shared-cache.md b/.changeset/opencode-tmpdir-and-shared-cache.md new file mode 100644 index 000000000..fe5b4f38e --- /dev/null +++ b/.changeset/opencode-tmpdir-and-shared-cache.md @@ -0,0 +1,37 @@ +--- +"@aoagents/ao-core": patch +"@aoagents/ao-cli": patch +"@aoagents/ao-plugin-agent-opencode": patch +--- + +opencode: bound /tmp blast radius and consolidate session-list cache + +Addresses review feedback on PR #1478: + +- **TMPDIR isolation.** Every `opencode` child we spawn now points at + `~/.agent-orchestrator/.bun-tmp/` via `TMPDIR`/`TMP`/`TEMP`. Bun's + embedded shared-library extraction lands there instead of the system + `/tmp`, so the cli janitor only ever sweeps AO-owned files. Other + users' or other applications' Bun artifacts on a shared host can no + longer be touched by the regex. +- **Single shared session-list cache.** Core and the agent-opencode + plugin previously kept independent caches; per poll cycle the system + spawned at least two `opencode session list` processes instead of + one. Both consumers now use the shared cache exported from + `@aoagents/ao-core` (`getCachedOpenCodeSessionList`). +- **TTL no longer covers the send-confirmation loop.** The cache TTL + dropped from 3s to 500ms so the + `updatedAt > baselineUpdatedAt` delivery signal in + `sendWithConfirmation` actually fires. Concurrent callers still + share the in-flight promise. +- **Delete invalidates the cache.** `deleteOpenCodeSession` now calls + `invalidateOpenCodeSessionListCache()` on success so reuse, remap, + and restore code paths cannot observe a deleted session id within + the TTL window. +- **Janitor reliability.** `sweepOnce` now filters synchronously + before allocating per-file promises (matters on hosts with thousands + of `/tmp` entries), and `stopBunTmpJanitor()` is now async and awaits + any in-flight sweep so SIGTERM cannot exit while `unlink` is mid-flight. +- **Janitor observability.** The sweep callback in `ao start` now logs + successful reclaims, not just errors, so operators can confirm the + janitor is doing useful work. diff --git a/packages/cli/__tests__/lib/bun-tmp-janitor.test.ts b/packages/cli/__tests__/lib/bun-tmp-janitor.test.ts new file mode 100644 index 000000000..66742f5db --- /dev/null +++ b/packages/cli/__tests__/lib/bun-tmp-janitor.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; +import { mkdtempSync, writeFileSync, existsSync, statSync, utimesSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import type * as AoCore from "@aoagents/ao-core"; + +// We point the shared module at a fresh temp dir per test by mocking the +// AO base path resolver. The janitor pulls the dir from +// @aoagents/ao-core's getOpenCodeTmpDir. +let mockedDir = ""; +vi.mock("@aoagents/ao-core", async () => { + const actual = await vi.importActual("@aoagents/ao-core"); + return { + ...actual, + getOpenCodeTmpDir: () => mockedDir, + }; +}); + +import { + isBunTmpJanitorRunning, + startBunTmpJanitor, + stopBunTmpJanitor, +} from "../../src/lib/bun-tmp-janitor.js"; + +const PATTERN_NAME = ".fcb8efb7fbaad77d-00000000.so"; + +function setMtime(path: string, ageMs: number): void { + const t = (Date.now() - ageMs) / 1000; + utimesSync(path, t, t); +} + +describe("bun-tmp-janitor", () => { + beforeEach(() => { + mockedDir = mkdtempSync(join(tmpdir(), "ao-bun-janitor-test-")); + }); + + afterEach(async () => { + await stopBunTmpJanitor(); + }); + + it("sweeps matching files older than ageMs in the AO-owned dir only", async () => { + const oldFile = join(mockedDir, PATTERN_NAME); + const youngFile = join(mockedDir, ".aaaaaaaa-bbbbbbbb.so"); + const unrelated = join(mockedDir, "regular-file.txt"); + writeFileSync(oldFile, "x".repeat(1024)); + writeFileSync(youngFile, "x"); + writeFileSync(unrelated, "x"); + setMtime(oldFile, 120_000); // 2 minutes old + + const sweeps: { removed: number; freedBytes: number; errors: number }[] = []; + startBunTmpJanitor({ + intervalMs: 60_000, + ageMs: 60_000, + onSweep: (r) => sweeps.push(r), + }); + + // Wait for the immediate sweep to complete. + await stopBunTmpJanitor(); + + expect(sweeps.length).toBe(1); + expect(sweeps[0]?.removed).toBe(1); + expect(sweeps[0]?.freedBytes).toBe(1024); + expect(existsSync(oldFile)).toBe(false); + expect(existsSync(youngFile)).toBe(true); + expect(existsSync(unrelated)).toBe(true); + }); + + it("treats a missing AO tmp dir as empty (no error)", async () => { + mockedDir = join(tmpdir(), `ao-bun-janitor-missing-${Date.now()}`); + expect(existsSync(mockedDir)).toBe(false); + + const sweeps: { removed: number; freedBytes: number; errors: number }[] = []; + startBunTmpJanitor({ onSweep: (r) => sweeps.push(r) }); + await stopBunTmpJanitor(); + + // No callback fires when removed=0 and errors=0. + expect(sweeps).toEqual([]); + }); + + it("stopBunTmpJanitor awaits the in-flight sweep", async () => { + // Drop a matching file so the immediate sweep does real work. + const path = join(mockedDir, PATTERN_NAME); + writeFileSync(path, "x"); + setMtime(path, 120_000); + + startBunTmpJanitor({ ageMs: 60_000 }); + expect(isBunTmpJanitorRunning()).toBe(true); + + await stopBunTmpJanitor(); + + // After awaiting stop, the sweep must have run to completion: the + // file must be gone, and the timer cleared. + expect(isBunTmpJanitorRunning()).toBe(false); + expect(existsSync(path)).toBe(false); + }); + + it("does not double-start", async () => { + const a = startBunTmpJanitor(); + const b = startBunTmpJanitor(); + expect(a).toBe(true); + expect(b).toBe(false); + await stopBunTmpJanitor(); + }); + + it("ignores files that do not match the Bun tmp library pattern", async () => { + const matching = join(mockedDir, ".1234567890abcdef-12345678.dylib"); + const nonMatching = join(mockedDir, "libopentui.so"); + writeFileSync(matching, "x"); + writeFileSync(nonMatching, "x"); + setMtime(matching, 120_000); + setMtime(nonMatching, 120_000); + + startBunTmpJanitor({ ageMs: 60_000 }); + await stopBunTmpJanitor(); + + expect(existsSync(matching)).toBe(false); + expect(existsSync(nonMatching)).toBe(true); + // Sanity: stat works (file exists), proves we did not unlink it. + expect(statSync(nonMatching).size).toBeGreaterThan(0); + }); +}); diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 1c5088ca6..293cf1a7b 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -48,6 +48,7 @@ import { parse as yamlParse, stringify as yamlStringify } from "yaml"; import { exec, execSilent, git } from "../lib/shell.js"; import { getSessionManager } from "../lib/create-session-manager.js"; import { ensureLifecycleWorker, stopAllLifecycleWorkers } from "../lib/lifecycle-service.js"; +import { startBunTmpJanitor, stopBunTmpJanitor } from "../lib/bun-tmp-janitor.js"; import { findWebDir, buildDashboardEnv, @@ -1636,6 +1637,10 @@ export function registerStart(program: Command): void { // Non-TTY callers (scripts/agents) keep the old "AO is already // running" message and do NOT mutate config behind the user's back. if (running && projectArgIsUrlOrPath && isHumanCaller()) { + const requestedProjectArg = projectArg; + if (!requestedProjectArg) { + throw new Error("Expected project path or URL argument"); + } // Always register against the GLOBAL config — never the cwd's // local config. Cross-project visibility lives in the global // registry, and addProjectToConfig only routes to global when @@ -1646,9 +1651,9 @@ export function registerStart(program: Command): void { : loadConfig(); let existingId: string | null = null; - if (isRepoUrl(projectArg!)) { + if (isRepoUrl(requestedProjectArg)) { try { - const parsed = parseRepoUrl(projectArg!); + const parsed = parseRepoUrl(requestedProjectArg); for (const [id, p] of Object.entries(globalCfg.projects)) { if (p.repo === parsed.ownerRepo) { existingId = id; @@ -1660,7 +1665,7 @@ export function registerStart(program: Command): void { } } else { const resolvedPath = resolve( - projectArg!.replace(/^~/, process.env["HOME"] || ""), + requestedProjectArg.replace(/^~/, process.env["HOME"] || ""), ); let canonicalTarget: string; try { @@ -1697,14 +1702,14 @@ export function registerStart(program: Command): void { let resolvedId: string; if (existingId) { resolvedId = existingId; - } else if (isRepoUrl(projectArg!)) { + } else if (isRepoUrl(requestedProjectArg)) { // Clone + register inline. We DO NOT call handleUrlStart — // that helper writes a legacy wrapped (`projects:`) local // config that the new resolver rejects, requiring a repair // pass after the fact. Instead, we write a flat local config // here so the global registry + repo can be loaded cleanly // on the very first read. - const parsed = parseRepoUrl(projectArg!); + const parsed = parseRepoUrl(requestedProjectArg); console.log( chalk.bold.cyan(`\n Cloning ${parsed.ownerRepo} (${parsed.host})\n`), ); @@ -1736,7 +1741,7 @@ export function registerStart(program: Command): void { throw new Error( `Repository "${parsed.ownerRepo}" appears to be empty (no commits or refs).\n` + ` AO needs at least one commit on the default branch to spawn an orchestrator.\n` + - ` Push an initial commit, then re-run \`ao start ${projectArg}\`.`, + ` Push an initial commit, then re-run \`ao start ${requestedProjectArg}\`.`, ); } @@ -1775,7 +1780,7 @@ export function registerStart(program: Command): void { } } else { const resolvedPath = resolve( - projectArg!.replace(/^~/, process.env["HOME"] || ""), + requestedProjectArg.replace(/^~/, process.env["HOME"] || ""), ); resolvedId = await addProjectToConfig(globalCfg, resolvedPath); } @@ -2221,6 +2226,25 @@ export function registerStart(program: Command): void { }); unlockStartup(); + // Start the Bun-extracted /tmp/.*.{so,dylib} janitor once per AO + // process. Single-instance is enforced by running.json + the + // startup lock above, so this call site is reached at most once + // per process. The janitor uses an unref'd interval timer, so it + // does not keep the event loop alive on its own and dies with the + // process on SIGTERM/SIGINT. + startBunTmpJanitor({ + onSweep: ({ removed, freedBytes, errors }) => { + if (removed > 0) { + console.info( + `[bun-tmp-janitor] reclaimed ${removed} file(s) / ${freedBytes} bytes`, + ); + } + if (errors > 0) { + console.warn(`[bun-tmp-janitor] sweep had ${errors} error(s)`); + } + }, + }); + // Install shutdown handlers so Ctrl+C and `ao stop` (which sends // SIGTERM) perform a full graceful shutdown: kill sessions, record // last-stop state for restore, unregister, then exit. @@ -2290,11 +2314,22 @@ export function registerStart(program: Command): void { } catch { // Best-effort — always exit even if cleanup fails } + try { + // Await any in-flight sweep so shutdown does not exit while + // unlink() calls are still mid-flight against the filesystem. + await stopBunTmpJanitor(); + } catch { + // Best-effort cleanup. + } process.exit(exitCode); })(); }; - process.once("SIGINT", shutdown); - process.once("SIGTERM", shutdown); + process.once("SIGINT", (sig) => { + void shutdown(sig); + }); + process.once("SIGTERM", (sig) => { + void shutdown(sig); + }); } catch (err) { if (err instanceof Error) { console.error(chalk.red("\nError:"), err.message); diff --git a/packages/cli/src/lib/bun-tmp-janitor.ts b/packages/cli/src/lib/bun-tmp-janitor.ts new file mode 100644 index 000000000..1e7ba8d24 --- /dev/null +++ b/packages/cli/src/lib/bun-tmp-janitor.ts @@ -0,0 +1,145 @@ +import { readdir, stat, unlink } from "node:fs/promises"; +import { join } from "node:path"; + +import { getOpenCodeTmpDir } from "@aoagents/ao-core"; + +// Bun-bundled binaries (opencode, etc.) extract embedded shared libraries to +// `TMPDIR` on startup and never unlink them on exit — this is a known upstream +// Bun bug that leaks ~4.3 MB per process invocation. Files look like +// `.{16hex}-{8hex}.{so|dylib}` (e.g. `.fcb8efb7fbaad77d-00000000.so`). +// +// We point every `opencode` child we spawn at an AO-owned subdirectory via +// `TMPDIR` (see `getOpenCodeChildEnv` in `@aoagents/ao-core`). The janitor +// then sweeps **only that directory**, which keeps the blast radius bounded: +// no other user's or other application's Bun artifacts can ever be touched +// even if AO runs as root on a shared host. +// +// Deleting these files is safe even while a live process has them mmap'd: on +// POSIX systems, `unlink` removes the directory entry but the kernel keeps +// the inode alive until the last mapping is torn down, at which point the +// space is reclaimed. For already-exited processes the unlink frees disk +// immediately. Windows does not allow unlinking mapped files, and opencode +// does not ship a Windows binary, so the janitor is a no-op there. +// +// This janitor runs once per `ao start` process and sweeps matching files +// older than `ageMs` at every interval. + +const BUN_TMP_LIB_PATTERN = /^\.[0-9a-f]{8,}-[0-9a-f]{6,}\.(so|dylib)$/i; +const DEFAULT_INTERVAL_MS = 60_000; +const DEFAULT_AGE_MS = 60_000; + +export interface BunTmpJanitorOptions { + intervalMs?: number; + ageMs?: number; + onSweep?: (result: { removed: number; freedBytes: number; errors: number }) => void; +} + +let timer: NodeJS.Timeout | null = null; +let inFlightTick: Promise | null = null; + +async function sweepOnce( + dir: string, + ageMs: number, +): Promise<{ removed: number; freedBytes: number; errors: number }> { + let removed = 0; + let freedBytes = 0; + let errors = 0; + + let entries: string[]; + try { + entries = await readdir(dir); + } catch { + // Directory may not exist yet (no opencode child has run). That is not + // an error condition — there is nothing to sweep. + return { removed, freedBytes, errors: 0 }; + } + + // Filter synchronously *before* spawning per-entry stat/unlink work. On a + // host with thousands of /tmp entries this avoids allocating one promise + // per file we are about to discard. (Belt-and-suspenders: TMPDIR isolation + // already bounds the directory contents to AO's own children.) + const matches = entries.filter((name) => BUN_TMP_LIB_PATTERN.test(name)); + if (matches.length === 0) { + return { removed, freedBytes, errors: 0 }; + } + + const cutoff = Date.now() - ageMs; + + await Promise.all( + matches.map(async (name) => { + const path = join(dir, name); + try { + const st = await stat(path); + if (!st.isFile() || st.mtimeMs > cutoff) return; + await unlink(path); + removed += 1; + freedBytes += st.size; + } catch { + // File may have been deleted by another sweeper, or stat raced + // with an unlink, or we lack permission. Best-effort — don't throw. + errors += 1; + } + }), + ); + + return { removed, freedBytes, errors }; +} + +export function startBunTmpJanitor(options: BunTmpJanitorOptions = {}): boolean { + // Windows: opencode ships no win32 binary and unlinking mapped files is + // disallowed by the kernel, so the janitor would be both unnecessary and + // potentially error-prone. Skip. + if (process.platform === "win32") return false; + if (timer) return false; + + const intervalMs = options.intervalMs ?? DEFAULT_INTERVAL_MS; + const ageMs = options.ageMs ?? DEFAULT_AGE_MS; + const { onSweep } = options; + const dir = getOpenCodeTmpDir(); + + const tick = async (): Promise => { + // Single-flight: if a previous tick is still running, skip this one. + if (inFlightTick) return; + const promise = (async () => { + try { + const result = await sweepOnce(dir, ageMs); + if (onSweep && (result.removed > 0 || result.errors > 0)) { + onSweep(result); + } + } finally { + inFlightTick = null; + } + })(); + inFlightTick = promise; + await promise; + }; + + // Run an immediate sweep to clear any backlog, then on an interval. + void tick(); + timer = setInterval(() => void tick(), intervalMs); + timer.unref(); + return true; +} + +/** + * Stop the janitor and await any sweep currently in flight. The shutdown + * handler in `start.ts` awaits this so the process never exits while + * `unlink` calls are still mid-flight against the filesystem. + */ +export async function stopBunTmpJanitor(): Promise { + if (timer) { + clearInterval(timer); + timer = null; + } + if (inFlightTick) { + try { + await inFlightTick; + } catch { + // Best-effort — don't block shutdown on an in-flight sweep error. + } + } +} + +export function isBunTmpJanitorRunning(): boolean { + return timer !== null; +} diff --git a/packages/core/src/__tests__/opencode-shared.test.ts b/packages/core/src/__tests__/opencode-shared.test.ts new file mode 100644 index 000000000..85d3d8ab6 --- /dev/null +++ b/packages/core/src/__tests__/opencode-shared.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it, beforeEach } from "vitest"; + +import { + ensureOpenCodeTmpDir, + getOpenCodeChildEnv, + getOpenCodeTmpDir, + invalidateOpenCodeSessionListCache, + resetOpenCodeSessionListCache, + OPENCODE_SESSION_LIST_CACHE_TTL_MS, +} from "../opencode-shared.js"; + +describe("opencode-shared", () => { + beforeEach(() => { + resetOpenCodeSessionListCache(); + }); + + describe("OPENCODE_SESSION_LIST_CACHE_TTL_MS", () => { + it("does not cover the 500ms send-confirmation poll interval", () => { + // The send-confirmation loop in session-manager polls 6× at 500ms + // intervals (~3s total). The TTL must be ≤500ms so each loop + // iteration sees fresh data and the + // `updatedAt > baselineUpdatedAt` delivery signal can fire. + expect(OPENCODE_SESSION_LIST_CACHE_TTL_MS).toBeLessThanOrEqual(500); + expect(OPENCODE_SESSION_LIST_CACHE_TTL_MS).toBeGreaterThan(0); + }); + }); + + describe("getOpenCodeTmpDir / ensureOpenCodeTmpDir", () => { + it("lives under the AO base dir, not the system /tmp", () => { + const dir = getOpenCodeTmpDir(); + expect(dir).toMatch(/\.agent-orchestrator[\\/]\.bun-tmp$/); + expect(dir.startsWith("/tmp")).toBe(false); + }); + + it("creates the directory and tolerates pre-existing", () => { + const a = ensureOpenCodeTmpDir(); + const b = ensureOpenCodeTmpDir(); + expect(a).toBe(b); + }); + }); + + describe("getOpenCodeChildEnv", () => { + it("sets TMPDIR/TMP/TEMP to the AO-owned dir", () => { + const env = getOpenCodeChildEnv(); + const dir = getOpenCodeTmpDir(); + expect(env["TMPDIR"]).toBe(dir); + expect(env["TMP"]).toBe(dir); + expect(env["TEMP"]).toBe(dir); + }); + + it("merges extra env vars on top, but keeps TMPDIR pointed at AO dir", () => { + const env = getOpenCodeChildEnv({ FOO: "bar" }); + expect(env["FOO"]).toBe("bar"); + expect(env["TMPDIR"]).toBe(getOpenCodeTmpDir()); + }); + + it("allows extra env to override TMPDIR for explicit opt-out", () => { + const env = getOpenCodeChildEnv({ TMPDIR: "/elsewhere" }); + expect(env["TMPDIR"]).toBe("/elsewhere"); + }); + }); + + describe("invalidateOpenCodeSessionListCache", () => { + it("is a no-throw on an empty cache", () => { + expect(() => invalidateOpenCodeSessionListCache()).not.toThrow(); + }); + }); +}); diff --git a/packages/core/src/__tests__/test-utils.ts b/packages/core/src/__tests__/test-utils.ts index 1fd9a2a90..70e6fb5b6 100644 --- a/packages/core/src/__tests__/test-utils.ts +++ b/packages/core/src/__tests__/test-utils.ts @@ -4,6 +4,7 @@ import { join } from "node:path"; import { tmpdir } from "node:os"; import { randomUUID } from "node:crypto"; import { getProjectSessionsDir, getProjectDir } from "../paths.js"; +import { resetOpenCodeSessionListCache } from "../session-manager.js"; import { createInitialCanonicalLifecycle, deriveLegacyStatus } from "../lifecycle-state.js"; import { createActivitySignal } from "../activity-signal.js"; import type { @@ -364,6 +365,7 @@ export interface TestContext { } export function setupTestContext(): TestContext { + resetOpenCodeSessionListCache(); const originalPath = process.env.PATH; const originalHome = process.env["HOME"]; const tmpDir = join(tmpdir(), `ao-test-session-mgr-${randomUUID()}`); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9c662d2e0..0d684b9a8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -151,6 +151,16 @@ export { parseWebhookBranchRef, } from "./scm-webhook-utils.js"; export { asValidOpenCodeSessionId } from "./opencode-session-id.js"; +export { + OPENCODE_SESSION_LIST_CACHE_TTL_MS, + getOpenCodeTmpDir, + ensureOpenCodeTmpDir, + getOpenCodeChildEnv, + getCachedOpenCodeSessionList, + invalidateOpenCodeSessionListCache, + resetOpenCodeSessionListCache, +} from "./opencode-shared.js"; +export type { OpenCodeSessionListEntry } from "./opencode-shared.js"; export { getWorkspaceAgentsMdPath, writeWorkspaceOpenCodeAgentsMd, diff --git a/packages/core/src/opencode-shared.ts b/packages/core/src/opencode-shared.ts new file mode 100644 index 000000000..ba0ebc88d --- /dev/null +++ b/packages/core/src/opencode-shared.ts @@ -0,0 +1,235 @@ +/** + * Shared opencode-child plumbing. + * + * This module exists for two reasons: + * + * 1. **Bounded /tmp blast radius** (issue #1046, PR #1478 review). + * Bun-bundled binaries leak `.so`/`.dylib` files into the system temp + * directory and never unlink them. Rather than sweeping all of `/tmp` + * with a regex (which would touch other users' or other apps' Bun + * artifacts), we point every `opencode` child at an AO-owned temp dir + * via `TMPDIR`. The cli-side janitor then sweeps only that directory. + * + * 2. **Single shared cache for `opencode session list`.** + * Both `@aoagents/ao-core` and the `agent-opencode` plugin previously + * kept independent module-level caches. Per poll cycle the system + * therefore spawned at least two `opencode session list` processes + * instead of one. A single cache exported from core collapses them. + */ + +import { execFile } from "node:child_process"; +import { mkdirSync } from "node:fs"; +import { join } from "node:path"; +import { promisify } from "node:util"; + +import { getAoBaseDir } from "./paths.js"; +import { safeJsonParse } from "./utils/validation.js"; +import { asValidOpenCodeSessionId } from "./opencode-session-id.js"; + +const execFileAsync = promisify(execFile); + +// ----------------------------------------------------------------------------- +// AO-owned temp dir for opencode children +// ----------------------------------------------------------------------------- + +let cachedOpenCodeTmpDir: string | null = null; + +/** + * Path that opencode children should treat as `TMPDIR`. Lives under the AO + * base dir so a stray sweep cannot touch unrelated files. + */ +export function getOpenCodeTmpDir(): string { + if (cachedOpenCodeTmpDir) return cachedOpenCodeTmpDir; + cachedOpenCodeTmpDir = join(getAoBaseDir(), ".bun-tmp"); + return cachedOpenCodeTmpDir; +} + +/** + * Best-effort: create the AO-owned temp dir. Spawn paths call this before + * launching opencode so the child sees a real directory at `TMPDIR`. + * + * Synchronous because the spawn helpers are themselves synchronous in their + * env construction. Failures are swallowed — opencode will fall back to the + * OS default temp dir, which is the pre-PR behavior. + */ +export function ensureOpenCodeTmpDir(): string { + const dir = getOpenCodeTmpDir(); + try { + mkdirSync(dir, { recursive: true }); + } catch { + // Best-effort. If creation fails opencode still works; we just leak to + // the system temp dir as before. + } + return dir; +} + +/** + * Build the env passed to every spawned `opencode` child. + * + * Setting both `TMPDIR` and `TMP`/`TEMP` covers POSIX (TMPDIR) and Windows + * fallbacks. Bun honors `TMPDIR` for its embedded shared-library extraction. + */ +export function getOpenCodeChildEnv( + extra?: NodeJS.ProcessEnv, +): NodeJS.ProcessEnv { + const dir = ensureOpenCodeTmpDir(); + return { + ...process.env, + TMPDIR: dir, + TMP: dir, + TEMP: dir, + ...extra, + }; +} + +// ----------------------------------------------------------------------------- +// Shared `opencode session list` cache +// ----------------------------------------------------------------------------- + +export interface OpenCodeSessionListEntry { + id: string; + title: string; + /** Raw `updated` field as emitted by opencode (string or number). */ + updated?: string | number; + /** Normalized epoch ms, parsed from `updated`. */ + updatedAt?: number; +} + +/** + * TTL for the `opencode session list` cache. + * + * **Why 500ms.** The send-confirmation loop in `session-manager.sendMessage` + * polls at 500ms intervals up to 6 times (~3s total). The original PR sized + * the TTL to *cover* that window (3s), which made every loop iteration return + * the same cached snapshot — the `updatedAt > baselineUpdatedAt` delivery + * signal could not fire by construction. Sizing the TTL at 500ms means each + * poll iteration sees fresh data while still collapsing tight bursts (e.g. + * lifecycle poll + UI enrichment in the same tick) onto a single child. + * Concurrent callers always share the in-flight promise regardless of TTL. + */ +export const OPENCODE_SESSION_LIST_CACHE_TTL_MS = 500; + +const OPENCODE_SESSION_LIST_DEFAULT_TIMEOUT_MS = 30_000; + +interface OpenCodeSessionListCache { + entries: OpenCodeSessionListEntry[]; + timestamp: number; + promise?: Promise; +} + +let sessionListCache: OpenCodeSessionListCache | null = null; + +function parseUpdatedToEpochMs(value: unknown): number | undefined { + if (typeof value === "number" && Number.isFinite(value)) return value; + if (typeof value !== "string") return undefined; + const trimmed = value.trim(); + if (trimmed.length === 0) return undefined; + if (/^\d+$/.test(trimmed)) { + const n = Number(trimmed); + return Number.isFinite(n) ? n : undefined; + } + const parsed = Date.parse(trimmed); + return Number.isNaN(parsed) ? undefined : parsed; +} + +function parseSessionListStdout(stdout: string): OpenCodeSessionListEntry[] { + const parsed = safeJsonParse(stdout); + if (!Array.isArray(parsed)) return []; + + return parsed.flatMap((entry) => { + if (!entry || typeof entry !== "object") return []; + const record = entry as Record; + const id = asValidOpenCodeSessionId(record["id"]); + if (!id) return []; + const title = typeof record["title"] === "string" ? record["title"] : ""; + const rawUpdated = record["updated"]; + const updated = + typeof rawUpdated === "string" || typeof rawUpdated === "number" + ? rawUpdated + : undefined; + const updatedAt = parseUpdatedToEpochMs(rawUpdated); + return [ + { + id, + title, + ...(updated !== undefined ? { updated } : {}), + ...(updatedAt !== undefined ? { updatedAt } : {}), + }, + ]; + }); +} + +/** + * Fetch the opencode session list, sharing both the cached snapshot and any + * in-flight request across all callers in core and plugins. + * + * Pass `forceRefresh: true` to bypass the TTL when a write is known to have + * just landed. Concurrent callers still collapse onto the in-flight promise. + */ +export async function getCachedOpenCodeSessionList(options?: { + timeoutMs?: number; + forceRefresh?: boolean; +}): Promise { + const timeoutMs = options?.timeoutMs ?? OPENCODE_SESSION_LIST_DEFAULT_TIMEOUT_MS; + const forceRefresh = options?.forceRefresh ?? false; + const now = Date.now(); + + if (sessionListCache) { + if (sessionListCache.promise) { + // A fetch is already in flight — every caller waits on it, even if + // they wanted a refresh. + return sessionListCache.promise; + } + if ( + !forceRefresh && + now - sessionListCache.timestamp < OPENCODE_SESSION_LIST_CACHE_TTL_MS + ) { + return sessionListCache.entries; + } + } + + const promise: Promise = execFileAsync( + "opencode", + ["session", "list", "--format", "json"], + { timeout: timeoutMs, env: getOpenCodeChildEnv() }, + ) + .then(({ stdout }) => { + const entries = parseSessionListStdout(stdout); + if (sessionListCache?.promise === promise) { + sessionListCache = { entries, timestamp: Date.now() }; + } + return entries; + }) + .catch(() => { + if (sessionListCache?.promise === promise) { + sessionListCache = null; + } + return [] as OpenCodeSessionListEntry[]; + }); + + sessionListCache = { entries: [], timestamp: now, promise }; + return promise; +} + +/** + * Drop any cached snapshot. Call this immediately after any code path that + * mutates opencode session state (delete, create) so that the next reader + * does not observe a stale entry. + */ +export function invalidateOpenCodeSessionListCache(): void { + // If a fetch is currently in flight we leave it alone — its result is + // about to land and a fresh fetch on top of it would be wasted work. + // Subsequent callers will see a stale snapshot for at most one tick; + // this is acceptable because the in-flight result already reflects state + // captured after the mutation began. + if (sessionListCache?.promise) { + sessionListCache = { ...sessionListCache, timestamp: 0 }; + return; + } + sessionListCache = null; +} + +/** Test-only: clear the cache including any in-flight promise. */ +export function resetOpenCodeSessionListCache(): void { + sessionListCache = null; +} diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index 29f6bc336..8dc946c92 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -73,6 +73,13 @@ import { generateSessionName, } from "./paths.js"; import { asValidOpenCodeSessionId } from "./opencode-session-id.js"; +import { + getCachedOpenCodeSessionList, + getOpenCodeChildEnv, + invalidateOpenCodeSessionListCache, + resetOpenCodeSessionListCache as resetSharedOpenCodeSessionListCache, + type OpenCodeSessionListEntry, +} from "./opencode-shared.js"; import { writeWorkspaceOpenCodeAgentsMd, } from "./opencode-agents-md.js"; @@ -114,10 +121,15 @@ async function deleteOpenCodeSession(sessionId: string): Promise { try { await execFileAsync("opencode", ["session", "delete", validatedSessionId], { timeout: 30_000, + env: getOpenCodeChildEnv(), }); + // Drop cached list immediately so reuse / remap / restore call sites + // do not observe the deleted id for the remainder of the TTL window. + invalidateOpenCodeSessionListCache(); return; } catch (err) { if (errorIncludesSessionNotFound(err)) { + invalidateOpenCodeSessionListCache(); return; } lastError = err; @@ -126,42 +138,15 @@ async function deleteOpenCodeSession(sessionId: string): Promise { throw lastError instanceof Error ? lastError : new Error(String(lastError)); } -interface OpenCodeSessionListEntry { - id: string; - title: string; - updatedAt?: number; +/** Re-export so existing core test-utils + session-manager call sites keep working. */ +export function resetOpenCodeSessionListCache(): void { + resetSharedOpenCodeSessionListCache(); } async function fetchOpenCodeSessionList( - timeoutMs = OPENCODE_DISCOVERY_TIMEOUT_MS, + timeoutMs: number = OPENCODE_DISCOVERY_TIMEOUT_MS, ): Promise { - try { - const { stdout } = await execFileAsync("opencode", ["session", "list", "--format", "json"], { - timeout: timeoutMs, - }); - const parsed = safeJsonParse(stdout); - if (!Array.isArray(parsed)) return []; - - return parsed.flatMap((entry) => { - if (!entry || typeof entry !== "object") return []; - const title = typeof entry["title"] === "string" ? entry["title"] : ""; - const id = asValidOpenCodeSessionId(entry["id"]); - if (!id) return []; - const rawUpdated = entry["updated"]; - let updatedAt: number | undefined; - if (typeof rawUpdated === "number" && Number.isFinite(rawUpdated)) { - updatedAt = rawUpdated; - } else if (typeof rawUpdated === "string") { - const parsedUpdated = Date.parse(rawUpdated); - if (!Number.isNaN(parsedUpdated)) { - updatedAt = parsedUpdated; - } - } - return [{ id, title, ...(updatedAt !== undefined ? { updatedAt } : {}) }]; - }); - } catch { - return []; - } + return getCachedOpenCodeSessionList({ timeoutMs }); } async function discoverOpenCodeSessionIdsByTitle( diff --git a/packages/plugins/agent-opencode/src/index.test.ts b/packages/plugins/agent-opencode/src/index.test.ts index cecfecbc3..97f277dfe 100644 --- a/packages/plugins/agent-opencode/src/index.test.ts +++ b/packages/plugins/agent-opencode/src/index.test.ts @@ -34,7 +34,7 @@ vi.mock("node:child_process", () => ({ }, })); -import { create, manifest, default as defaultExport } from "./index.js"; +import { create, manifest, default as defaultExport, resetOpenCodeSessionListCache } from "./index.js"; function makeSession(overrides: Partial = {}): Session { return { @@ -95,6 +95,7 @@ function mockTmuxWithProcess(processName: string, found = true) { beforeEach(() => { vi.clearAllMocks(); + resetOpenCodeSessionListCache(); }); describe("plugin manifest & exports", () => { diff --git a/packages/plugins/agent-opencode/src/index.ts b/packages/plugins/agent-opencode/src/index.ts index 3f0fcdbaf..6878a348a 100644 --- a/packages/plugins/agent-opencode/src/index.ts +++ b/packages/plugins/agent-opencode/src/index.ts @@ -7,6 +7,10 @@ import { getActivityFallbackState, recordTerminalActivity, asValidOpenCodeSessionId, + getCachedOpenCodeSessionList, + getOpenCodeChildEnv, + ensureOpenCodeTmpDir, + resetOpenCodeSessionListCache, type Agent, type AgentSessionInfo, type AgentLaunchConfig, @@ -18,18 +22,13 @@ import { type Session, type WorkspaceHooksConfig, type OpenCodeAgentConfig, + type OpenCodeSessionListEntry, } from "@aoagents/ao-core"; import { execFile, execFileSync } from "node:child_process"; import { promisify } from "node:util"; const execFileAsync = promisify(execFile); -interface OpenCodeSessionListEntry { - id: string; - title?: string; - updated?: string | number; -} - function parseUpdatedTimestamp(updated: string | number | undefined): Date | null { if (typeof updated === "number") { if (!Number.isFinite(updated)) return null; @@ -54,20 +53,8 @@ function parseUpdatedTimestamp(updated: string | number | undefined): Date | nul return new Date(parsedMs); } -function parseSessionList(raw: string): OpenCodeSessionListEntry[] { - let parsed: unknown; - try { - parsed = JSON.parse(raw); - } catch { - return []; - } - if (!Array.isArray(parsed)) return []; - return parsed.filter((item): item is OpenCodeSessionListEntry => { - if (!item || typeof item !== "object") return false; - const record = item as Record; - return asValidOpenCodeSessionId(record["id"]) !== undefined; - }); -} +// Re-export for backward compat — see @aoagents/ao-core/opencode-shared. +export { resetOpenCodeSessionListCache }; /** * Parse JSON stream lines from `opencode run --format json` output. @@ -158,13 +145,7 @@ async function findOpenCodeSession( session: Session, ): Promise { try { - const { stdout } = await execFileAsync( - "opencode", - ["session", "list", "--format", "json"], - { timeout: 30_000 }, - ); - - const sessions = parseSessionList(stdout); + const sessions = await getCachedOpenCodeSessionList(); // Prefer exact ID match from metadata if (session.metadata?.opencodeSessionId) { @@ -273,6 +254,15 @@ function createOpenCodeAgent(): Agent { env["AO_ISSUE_ID"] = config.issueId; } + // Point Bun's embedded shared-library extraction at an AO-owned temp + // dir so the cli-side janitor only needs to sweep our own files + // (issue #1046). Setting all three keys covers POSIX (TMPDIR) and + // Windows fallbacks; opencode itself ships POSIX-only today. + const tmpDir = ensureOpenCodeTmpDir(); + env["TMPDIR"] = tmpDir; + env["TMP"] = tmpDir; + env["TEMP"] = tmpDir; + // PATH and GH_PATH are injected by session-manager for all agents. return env; @@ -452,7 +442,7 @@ export function create(): Agent { export function detect(): boolean { try { - execFileSync("opencode", ["version"], { stdio: "ignore" }); + execFileSync("opencode", ["version"], { stdio: "ignore", env: getOpenCodeChildEnv() }); return true; } catch { return false;