diff --git a/docs/superpowers/plans/2026-09-09-opencode-export-integrity.md b/docs/superpowers/plans/2026-09-09-opencode-export-integrity.md new file mode 100644 index 000000000..41bbfe25e --- /dev/null +++ b/docs/superpowers/plans/2026-09-09-opencode-export-integrity.md @@ -0,0 +1,160 @@ +# OpenCode Export Integrity + +Refs #845. Independent detour from Grok integration; no merge is authorized. + +The affected OpenCode 1.18.30 session exported invalid 65,536-byte piped stdout +but valid 14,566,990-byte file output with 804 messages and matching identity. +The installed-version source writes stdout without awaiting it, then calls +process.exit() in its CLI finally block. This is producer-side pipe truncation, +not Agent Code's 256 MiB buffer limit or a provider-specific translator bug. +The private probe exported read-only, reported metadata only, and deleted output. + +1. Add a real child-process regression that writes a large native-shaped JSON + export and exits immediately, plus genuine-invalid-JSON and command-failure + cases. Use synthetic text in the transport stimulus, never personal prompts. +2. Capture finite OpenCode CLI stdout directly into a private temporary regular + file, not an asynchronous pipe. Keep process execution shell-free; wait for + completion before reading; preserve bounded output/error handling and cleanup. + Apply at the shared CLI boundary so large resolved config output is protected + too, not just outbound session export. Do not repair JSON or repeatedly export. +3. Run switching/CLI integration tests and typecheck. Re-run the affected native + export through the fixed production API and report only size/identity/counts. +4. Read-only review, fix verified findings, publish a focused PR linked to #845. + No Grok changes, app launch, source-session mutation, or automatic merge. + +## Verification + +The large abrupt-exit export and config regressions failed at 65,536 bytes with +the old pipe transport. The regular-file capture passes both. Coverage also +exercises import/model compatibility, private permissions, cleanup, actual +spawn failure, nonzero exit, invalid JSON, sparse oversized output and bounded +stderr. Read-only review identified post-spawn error handling that could retire +capture ownership before close; a failing lifecycle regression reproduced it. +Errors are now recorded and settled at close, including failed-spawn close. + +The actual affected session passed the opted-in production export test without +printing or retaining its contents. The 256 MiB final-size check is a memory +bound; polling detects disk overflow during execution but is not a hard quota. +Versioned upstream evidence: anomalyco/opencode v1.18.30, +packages/opencode/src/cli/cmd/export.ts and packages/opencode/src/index.ts. + +## Merge with main (e9ac8bdf) + +Main's e9ac8bdf (Refs #864) had meanwhile bounded every CLI call with +execFile's `timeout` plus an owned abort listener, both SIGKILL, with a 30 s +default, so a hung empty-session import cannot hold OpenCode Terminal startup +or survive stop(). The merge (36bf9581) rebuilt both bounds by hand on +`spawn` and kept main's invariants: settle only from `close`, cancellation +wins over timeout or overflow, and an already-stopped caller creates nothing. +Timeout errors now read `timed out after N ms` instead of execFile's +`Command failed: ...`. + +## Review round (2026-09-12) + +Two independent reviews of the merged head (Codex: request changes; Claude: +approve with comments) led to these changes: + +- A spawn that fails before stdio exists (EMFILE/ENFILE) has no stderr stream. + The `child.stderr!` dereference ran before the lifecycle listeners, so the + next-tick child `error` became an uncaught exception, which the crash hooks + turn into an app exit. The listeners now come first and stderr is optional. +- Every kill path SIGKILLed only the direct child. OpenCode's npm launcher + spawns the native binary with inherited stdio and cannot forward SIGKILL, so + the native process kept the capture and stderr pipe open, and timeout, stop + and overflow never settled. The merge had also dropped execFile's stdio + destruction. Commands now run in a private process group on POSIX, and one + terminate() destroys stderr and then SIGKILLs the group. Resolving the native + binary was rejected: it would couple Agent Code to OpenCode's install layout + and still miss helpers a native CLI starts itself. +- The capture was removed only by an async finally, which an app quit does not + await. It is now unlinked, with its directory, before spawn and read through + the retained descriptor with positional reads, because the child moved the + shared offset. The import payload is still read by path and is out of scope. +- Transform export and import inherited the 30 s startup default. They now + pass 5 minutes, while the startup import and profile probes keep 30 s. The + 14.6 MB export's duration was never measured and was not measured in this + round, because no live run against real sessions was authorized. Transforms + carry no AbortSignal, so the deadline is their only bound. +- Declined: an app-wide shutdown drain for finite CLI operations. With the + unlink, nothing persists across a quit. An orphaned child on quit predates + this branch (execFile had the same exposure). An app-owned operation drain + belongs to the quit lifecycle and operation ownership work in #919 and #918. +- The comments no longer claim that `spawn` lacks `timeout`/`killSignal`. + Spawn failure must now report ENOENT, and the late-stop replay is covered. + +Regression evidence: real two-process launcher trees for timeout, stop, +overflow, and a descendant that left the group; a spawn-failure double without +stderr that errors on the next tick; a zero link count observed inside the +running child; and no named capture while an import child runs. With its fix +reverted, each regression failed for the expected reason: an uncaught EMFILE, +a call still pending past its bound, a surviving descendant, a link count of 1, +or a named capture directory. + +## Review round 2 (2026-09-12) + +Both reviewers re-checked 8c94da7e (Codex: request changes; Claude: approve +with comments). Every round-1 disposition held. Changes: + +- terminate() could signal a released process-group id. Node reaps the direct + child before `exit`, but `close` also waits for stderr EOF, so the deadline, + stop() and the size guard stay armed after the reap while an escaped helper + holds stderr, and by then the group id is free for reuse. A timeout or + overflow followed by stop() also signalled twice. The group is now signalled + at most once, and only while exitCode and signalCode are both null. After + the reap, terminate() only destroys stderr, which is enough to release + `close`. Trade-off: a same-group member that outlives an exited leader is no + longer killed. It can only append to the unlinked capture, and readCapture + is bounded by the fstat size. The npm launcher waits for its native child, + so the tree the group exists for is unaffected. +- The launcher-tree tests armed a 2 s deadline at spawn but allowed 5 s for + fixture readiness, and read the launcher pid from a possibly reparented + ppid. A failed readiness wait also left unobserved trees running. The tests + now live in opencodeCliSessions.processTree.system.test.ts behind a spawn + seam that returns only after the fixture reports, so readiness precedes + every bound and the bounds stay tight. The launcher passes its own pid, the + test owns the launcher's ChildProcess from spawn, and cleanup aborts the + command before awaiting it. +- The detached-group comment said a wedged child runs "to the deadline". The + deadline is a timer in Agent Code's main process and ends with it, so a + dev-terminal Ctrl+C that kills Electron leaves a wedged CLI unbounded. + +- Inherited readiness race in main's hung-import stop/timeout tests + (e9ac8bdf). The tests waited up to 2 s for the fake CLI's status while the + timeout case's 1 s deadline was already running. During round-2 + verification, at load average ~42 with most of swap in use, the stop case + failed before stop() was ever called. Those tests now use the same readiness + gate as the tree tests (testing/spawnReadiness.ts), own the child from spawn, + check its exit on the ChildProcess handle (SIGKILL within 2 s of the + trigger), and abort in cleanup. No budget was simply widened. + +Round-2 regression evidence, each with its fix reverted: +- Signalling after the reap: the reaped-leader case fails with a recorded + negative-pid SIGKILL. +- Removing the once-only guard: the timeout-then-stop case records two group + signals. +- The round-1 kill-path mutations still fail the moved tree tests: + - direct-child kill only: all six cases pending past their bound; + - no stderr destroy: both escaped-helper cases pending; + - no group kill: descendants survive, or the timeout-then-stop case never + triggers its stop. + +## Review round 3 (2026-09-12) + +Both reviewers approved 3096ed25 with comments. The production fixes were +verified with real processes, and the remaining items were test-only or record +keeping: + +- An escaped fixture descendant was not reclaimed when readiness failed, + because cleanup found descendants only through the ready status. The + launcher fixture now records the descendant's pid as soon as spawn() + returns. Cleanup kills that owned identity after the command settles, whether + or not readiness succeeded, and a forced-readiness-failure case for the + escaped fixture proves no survivor remains. +- A failed test could leave its queued spawn gate for the next test's spawn. + Both system files now reset the spawn mock after each test. +- The reaped-leader regression's spy passed calls through, so under the very + mutation it guards it would have delivered a real negative-pid SIGKILL. It + now records such requests without delivering them. +- The polling helpers that the file split duplicated now live in + testing/processWait.ts. +- The PR body was brought up to date for rounds 2 and 3. diff --git a/src/main/providerSwitch/transcriptEngine.test.ts b/src/main/providerSwitch/transcriptEngine.test.ts index 48734dd13..dcdbb8366 100644 --- a/src/main/providerSwitch/transcriptEngine.test.ts +++ b/src/main/providerSwitch/transcriptEngine.test.ts @@ -103,6 +103,12 @@ describe('host transcript adapter registry', () => { await expect(getHostTranscriptAdapter('opencode').read('/project', 'ses_source')) .rejects.toThrow('has an unfinished turn') + // Transform exports are the large ones #845 fixed; the startup 30 s bound + // must not apply to them. + expect(mocks.exportOpencodeSession).toHaveBeenCalledWith( + { binary: '/tool', cwd: '/project', timeoutMs: 300_000 }, + 'ses_source', + ) }) it('imports one projected OpenCode envelope instead of treating it as JSONL', async () => { @@ -111,8 +117,10 @@ describe('host transcript adapter registry', () => { await expect(getHostTranscriptAdapter('opencode').write('/project', [value])) .resolves.toBe('opencode://session/ses_target') + // A projected import carries a whole conversation, so it must not inherit + // the 30 s bound meant for the terminal's empty-session startup import. expect(mocks.importOpencodeSession).toHaveBeenCalledWith( - { binary: '/tool', cwd: '/project' }, + { binary: '/tool', cwd: '/project', timeoutMs: 300_000 }, value, ) }) diff --git a/src/main/providerSwitch/transcriptEngine.ts b/src/main/providerSwitch/transcriptEngine.ts index ecd5d99b3..40ab9035a 100644 --- a/src/main/providerSwitch/transcriptEngine.ts +++ b/src/main/providerSwitch/transcriptEngine.ts @@ -170,6 +170,24 @@ const codexAdapter: HostTranscriptAdapter = { }, } +// WHY transcript transforms get their own, much larger OpenCode deadline: +// runOpencode's 30 s default came from e9ac8bdf (Refs #864), where it bounds a +// hung *empty-session* import that would otherwise hold OpenCode Terminal +// startup. A transform exports or imports a whole conversation instead. The +// session that exposed #845 was a 14.6 MB / 804-message export, and its export +// duration was never measured (the live probe ran before any deadline existed). +// Applying the startup bound here could turn the fixed truncation into a +// timeout on exactly the large sessions #845 made switchable, duplicable and +// rewindable. Five minutes still ends a genuinely wedged CLI, and runOpencode +// SIGKILLs its whole process group when it does. +// +// These transform calls carry no AbortSignal (nothing between the IPC handlers +// and this adapter threads one), so the deadline is their only bound; stop() +// cancellation exists for the terminal startup import alone. The target-profile +// probes (`debug config`, `models`) keep the 30 s default because their cost +// does not grow with conversation size. +const OPENCODE_TRANSFORM_TIMEOUT_MS = 5 * 60_000 + const opencodeAdapter: HostTranscriptAdapter = { provider: 'opencode', async read(cwd, providerSessionId) { @@ -197,7 +215,7 @@ const opencodeAdapter: HostTranscriptAdapter = { throw new Error('Projected OpenCode resume must contain exactly one export object.') } const binary = getToolPath('opencode', 'opencode') - const sessionId = await importOpencodeSession({ binary, cwd }, values[0]) + const sessionId = await importOpencodeSession({ binary, cwd, timeoutMs: OPENCODE_TRANSFORM_TIMEOUT_MS }, values[0]) return opencodeTranscriptFile(sessionId) }, sessionId(values) { @@ -346,7 +364,7 @@ async function loadOpencodeSnapshot( providerSessionId: string, ): Promise { const binary = getToolPath('opencode', 'opencode') - const exported = await exportOpencodeSession({ binary, cwd }, providerSessionId) + const exported = await exportOpencodeSession({ binary, cwd, timeoutMs: OPENCODE_TRANSFORM_TIMEOUT_MS }, providerSessionId) assertStableOpencodeExport(exported, providerSessionId) const conversation = decodeOpencodeConversation(exported) // OpenCode exports one complete native message per array position. That diff --git a/src/providers/opencode/runtime/opencodeCliSessions.live.test.ts b/src/providers/opencode/runtime/opencodeCliSessions.live.test.ts new file mode 100644 index 000000000..ca3ba5e41 --- /dev/null +++ b/src/providers/opencode/runtime/opencodeCliSessions.live.test.ts @@ -0,0 +1,37 @@ +import { existsSync } from 'node:fs' +import { homedir } from 'node:os' +import { join } from 'node:path' +import { expect, it } from 'vitest' +import { claudeNativeResumeProjector, codexNativeResumeProjector, decodeOpencodeConversation, opencodeNativeResumeProjector } from 'agent-transcript-parser' +import { exportOpencodeSession } from './opencodeCliSessions.js' + +// Explicit read-only native verification. No source/target sessions are +// modified, and no personal transcript values or source IDs become fixtures. +it('exports an opted-in large native OpenCode session completely through the production capture boundary', async context => { + const sessionId = process.env.OPENCODE_EXPORT_SESSION_ID + if (!sessionId) context.skip('Set OPENCODE_EXPORT_SESSION_ID to opt into a large native session export') + const binary = process.env.OPENCODE_BINARY ?? join(homedir(), '.opencode', 'bin', 'opencode') + if (!existsSync(binary)) context.skip('Installed OpenCode CLI unavailable') + const value = await exportOpencodeSession({ binary, cwd: process.env.OPENCODE_EXPORT_CWD ?? process.cwd() }, sessionId!) + const info = value.info as { id?: string } | undefined + expect(info?.id === sessionId, 'export identity matches the requested native session').toBe(true) + expect(Array.isArray(value.messages)).toBe(true) + const bytes = Buffer.byteLength(JSON.stringify(value)) + expect(bytes, 'fixture source must exceed the observed pipe truncation boundary').toBeGreaterThan(128 * 1024) + const conversation = decodeOpencodeConversation(value) + expect(conversation.entries.length).toBeGreaterThan(0) + // Exercise the real conversion hub with the previously failing native + // export, but do NOT publish target sessions or execute any imported tools. + const target = { + cwd: process.env.OPENCODE_EXPORT_CWD ?? process.cwd(), + targetSessionId: '00000000-0000-4000-8000-000000000001', + now: '2026-09-09T00:00:00.000Z', version: 'fixture', cliVersion: 'fixture', + model: 'fixture-model', modelProvider: 'fixture-provider', + } + for (const projector of [claudeNativeResumeProjector, codexNativeResumeProjector, opencodeNativeResumeProjector]) { + const projection = projector.projectNativeResume(conversation, target) + expect(projection.values.length, `${projector.provider} native projection is nonempty`).toBeGreaterThan(0) + expect(projection.report.counts.preserved, `${projector.provider} retained native conversation content`).toBeGreaterThan(0) + } + console.info(`Native export verified: ${bytes} JSON bytes, ${(value.messages as unknown[]).length} messages; identity matches.`) +}, 60000) diff --git a/src/providers/opencode/runtime/opencodeCliSessions.processTree.system.test.ts b/src/providers/opencode/runtime/opencodeCliSessions.processTree.system.test.ts new file mode 100644 index 000000000..4ad27c78f --- /dev/null +++ b/src/providers/opencode/runtime/opencodeCliSessions.processTree.system.test.ts @@ -0,0 +1,262 @@ +import { existsSync, readFileSync } from 'node:fs' +import { mkdtemp, mkdir, readdir, realpath, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { performance } from 'node:perf_hooks' +import { spawn, type ChildProcess } from 'node:child_process' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// OpenCode's npm launcher is a Node process whose native child inherits stdout +// and stderr (see testing/launcherCli.mjs). A single-process fake cannot show +// that a kill path reaches a descendant, or that settlement stops waiting on +// one, so these cases run a real two-process tree against runOpencode. They +// live apart from opencodeCliSessions.system.test.ts because they own a spawn +// seam (startTree) and a fixture that the output-integrity cases do not need. +vi.mock('node:child_process', async importOriginal => { + const actual = await importOriginal() + return { ...actual, spawn: vi.fn(actual.spawn) } +}) +import { exportOpencodeSession } from './opencodeCliSessions.js' +import { holdNextSpawnUntilReady } from './testing/spawnReadiness.js' +import { alive, waitUntil, within } from './testing/processWait.js' + +const { spawn: realSpawn } = await vi.importActual('node:child_process') + +// Readiness gets a generous budget. The termination bounds stay tight because +// they only start once the tree is ready (see startTree), and the per-test +// timeout covers the readiness budget plus the bounds and cleanup. +const READY_BUDGET_MS = 10_000 +const SETTLE_MARGIN_MS = 2_000 +const TEST_TIMEOUT_MS = 30_000 + +type Tree = { pid: number; launcherPid: number } +type Run = { + statusFile: string + /** Where the launcher records the descendant's pid the moment it spawns it. */ + spawnedFile: string + controller: AbortController + outcome: Promise + launcher: { child?: ChildProcess; readyAt?: number } +} + +let root: string +let cwd: string +beforeEach(async () => { + root = await realpath(await mkdtemp(join(tmpdir(), 'opencode-tree-test-'))) + cwd = join(root, 'project') + await mkdir(cwd) + vi.stubEnv('TMPDIR', root) + vi.stubEnv('TMP', root) + vi.stubEnv('TEMP', root) +}) +afterEach(async () => { + // mockReset, not only clearAllMocks: mockClear keeps queued + // once-implementations. A test that fails after startTree queued its readiness + // gate, but before runOpencode reached spawn, would otherwise hand that gate + // to the next test's first spawn. Vitest 4 resets vi.fn(actual.spawn) back to + // the real spawn. + vi.mocked(spawn).mockReset() + vi.clearAllMocks() + vi.unstubAllEnvs() + await rm(root, { recursive: true, force: true }) +}) +async function expectClean() { expect(await readdir(root)).toEqual(['project']) } + +/** + * Start one export against a real launcher tree that is ready before any bound arms. + * + * runOpencode's spawn() call is held until the descendant reports (see + * holdNextSpawnUntilReady for why readiness must precede the bounds rather than + * race them). The test owns the launcher's ChildProcess from spawn. + */ +type TreeOptions = { + timeoutMs?: number + // What the readiness gate waits for. 'status' (the default) is the + // descendant's own report. 'spawned' is only the launcher's record that the + // descendant exists, used with descendantDelayMs to force a readiness failure + // on a descendant that is alive but has not reported. + readyWhen?: 'status' | 'spawned' + descendantDelayMs?: number +} + +function startTree(descendant: 'hang' | 'overflow' | 'escaped' | 'escaped-exit', { timeoutMs, readyWhen = 'status', descendantDelayMs }: TreeOptions = {}): Run { + const statusFile = join(cwd, 'descendant.json') + const spawnedFile = join(cwd, 'descendant.spawned') + const launcher = holdNextSpawnUntilReady(vi.mocked(spawn), realSpawn, () => existsSync(readyWhen === 'status' ? statusFile : spawnedFile), READY_BUDGET_MS) + const controller = new AbortController() + const env = { + ...process.env, + NODE_OPTIONS: `--import=${new URL('./testing/launcherCli.mjs', import.meta.url).href}`, + OPENCODE_LAUNCHER_DESCENDANT: descendant, + OPENCODE_LAUNCHER_STATUS: statusFile, + OPENCODE_LAUNCHER_SPAWNED: spawnedFile, + ...(descendantDelayMs === undefined ? {} : { OPENCODE_LAUNCHER_DESCENDANT_DELAY_MS: String(descendantDelayMs) }), + } + const outcome = exportOpencodeSession({ binary: process.execPath, cwd, env, signal: controller.signal, ...(timeoutMs === undefined ? {} : { timeoutMs }) }, 'ses_fixture') + .then(() => new Error('resolved'), (error: Error) => error) + return { statusFile, spawnedFile, controller, outcome, launcher } +} + +/** The descendant pid the launcher recorded at spawn, if it got that far. */ +function spawnedDescendant(run: Run): number | undefined { + return existsSync(run.spawnedFile) ? Number(readFileSync(run.spawnedFile, 'utf8')) : undefined +} + +async function ready(run: Run): Promise { + await waitUntil(() => run.launcher.readyAt !== undefined, READY_BUDGET_MS + 5_000, 'launcher spawn') + expect(existsSync(run.statusFile), 'fixture tree ready before runOpencode armed its bounds').toBe(true) + const tree = JSON.parse(readFileSync(run.statusFile, 'utf8')) as Tree + // The launcher reports its own pid. A descendant's ppid is not evidence: an + // escaped child whose launcher already exited has been reparented. + expect(tree.launcherPid).toBe(run.launcher.child?.pid) + // The reporting descendant must be the one recorded at spawn: that record is + // the identity release() relies on when readiness fails. + expect(tree.pid).toBe(spawnedDescendant(run)) + return tree +} + +/** + * Release everything a tree test started, on success and on failure. + * + * Abort comes first. When a test fails before it observed the tree (no status, + * or a failed assertion before its trigger), runOpencode's own terminate() is + * the only code that can end the group, and without the abort `outcome` would + * wait for the 30 s default deadline while the tree outlived the test. The + * launcher is killed through its ChildProcess, which Node makes a no-op once + * reaped, never through a raw pid that could since have been reused. + * + * The descendant is found through the pid the launcher recorded at spawn, not + * through the ready status. A test that failed in ready() has no status, and + * the group kill cannot reach an escaped descendant, so without that record it + * would outlive the test. The record is read only after the command has + * settled: by then the launcher is dead (or never ran) and cannot start another + * descendant. + */ +async function release(run: Run) { + run.controller.abort() + run.launcher.child?.kill('SIGKILL') + await run.outcome + const descendant = spawnedDescendant(run) + if (descendant !== undefined && alive(descendant)) process.kill(descendant, 'SIGKILL') +} + +describe('OpenCode CLI process-tree termination', () => { + it.each([ + { trigger: 'timeout', descendant: 'hang', message: 'timed out after 1000 ms' }, + { trigger: 'stop', descendant: 'hang', message: 'OpenCode command cancelled' }, + { trigger: 'output overflow', descendant: 'overflow', message: 'output exceeds' }, + ] as const)('settles on $trigger and kills a descendant holding stdout and stderr', async ({ trigger, descendant, message }) => { + const run = startTree(descendant, trigger === 'timeout' ? { timeoutMs: 1000 } : {}) + let tree: Tree | undefined + try { + tree = await ready(run) + if (trigger === 'stop') run.controller.abort() + // Every bound counts from spawn's return, which is also readiness, and + // sits far below the 30 s default: a broken kill path fails here instead + // of being rescued by the default deadline. + const settleBy = run.launcher.readyAt! + (trigger === 'timeout' ? 1000 : 0) + SETTLE_MARGIN_MS + const failure = await within(run.outcome, settleBy - performance.now()) + expect(failure, `${trigger} settles within its bound`).toBeInstanceOf(Error) + expect((failure as Error).message).toContain(message) + await waitUntil(() => !alive(tree!.pid), 2000, 'descendant exit') + await expectClean() + } finally { + await release(run) + } + }, TEST_TIMEOUT_MS) + + it('settles a timeout when a descendant left the process group but kept stderr', async () => { + // A group kill cannot reach a descendant in its own session. Settlement is + // still bounded because terminate() releases stderr, the pipe `close` would + // otherwise wait on. The survivor is out of reach by design, so release() + // kills it rather than the test asserting it dead. + const run = startTree('escaped', { timeoutMs: 1000 }) + let tree: Tree | undefined + try { + tree = await ready(run) + const failure = await within(run.outcome, run.launcher.readyAt! + 1000 + SETTLE_MARGIN_MS - performance.now()) + expect(failure, 'timeout settles within its bound').toBeInstanceOf(Error) + expect((failure as Error).message).toContain('timed out after 1000 ms') + expect(run.launcher.child!.signalCode).toBe('SIGKILL') + await expectClean() + } finally { + await release(run) + } + }, TEST_TIMEOUT_MS) + + it('reclaims an escaped descendant whose readiness never arrived', async () => { + // Forces the path a failed readiness wait takes. spawn returns once the + // launcher has recorded its escaped descendant, but the descendant delays + // its own report far past that, so ready() fails with no status and no + // tree. The group kill cannot reach an escaped descendant, so only the + // spawn-time record lets release() reclaim it. + const run = startTree('escaped', { readyWhen: 'spawned', descendantDelayMs: 60_000 }) + let descendant: number | undefined + try { + await expect(ready(run)).rejects.toThrow('fixture tree ready before runOpencode armed its bounds') + descendant = spawnedDescendant(run) + expect(descendant, 'launcher recorded the escaped descendant at spawn').toBeGreaterThan(0) + expect(alive(descendant!), 'descendant alive and unobserved before cleanup').toBe(true) + } finally { + await release(run) + } + await waitUntil(() => !alive(descendant!), 2000, 'escaped descendant reclaimed') + expect((await run.outcome).message).toContain('OpenCode command cancelled') + await expectClean() + }, TEST_TIMEOUT_MS) + + it('never signals the process group once its leader has been reaped', async () => { + // The launcher exits 0 at once while an escaped helper keeps stderr open, + // so the deadline fires after Node reaped the leader but before `close`. + // Nothing reserves that group id any more: a negative-pid SIGKILL would be + // aimed at whatever group reused the number. So a regression is recorded, + // never delivered: the spy swallows every negative-pid request and passes + // only positive pids (liveness probes, cleanup) to the real kill. + const realKill = process.kill.bind(process) + const kills = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => pid < 0 ? true : realKill(pid, signal)) + const run = startTree('escaped-exit', { timeoutMs: 1000 }) + let tree: Tree | undefined + try { + tree = await ready(run) + const launcher = run.launcher.child! + await waitUntil(() => launcher.exitCode !== null, 2000, 'launcher reaped') + expect(launcher.exitCode).toBe(0) + expect(performance.now() - run.launcher.readyAt!, 'leader reaped before the deadline fires').toBeLessThan(1000) + const failure = await within(run.outcome, run.launcher.readyAt! + 1000 + SETTLE_MARGIN_MS - performance.now()) + expect(failure, 'timeout still settles by releasing stderr').toBeInstanceOf(Error) + expect((failure as Error).message).toContain('timed out after 1000 ms') + expect(kills.mock.calls.filter(([pid]) => pid < 0)).toEqual([]) + await expectClean() + } finally { + kills.mockRestore() + await release(run) + } + }, TEST_TIMEOUT_MS) + + it('signals the process group only once when stop() follows a timeout before close', async () => { + // Stop is requested in the microtask right after the deadline's group + // signal: later than the timeout, earlier than any I/O callback, so always + // before `close` (which needs the reap). A second group signal there would + // reuse a number that the first SIGKILL may already have released. + const run = startTree('hang', { timeoutMs: 1000 }) + const realKill = process.kill.bind(process) + const kills = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => { + if (pid < 0 && !run.controller.signal.aborted) queueMicrotask(() => run.controller.abort()) + return realKill(pid, signal) + }) + let tree: Tree | undefined + try { + tree = await ready(run) + const failure = await within(run.outcome, run.launcher.readyAt! + 1000 + SETTLE_MARGIN_MS - performance.now()) + expect(failure, 'timeout then stop settles within its bound').toBeInstanceOf(Error) + // Cancellation still takes precedence over the timeout it raced. + expect((failure as Error).message).toContain('OpenCode command cancelled') + expect(kills.mock.calls.filter(([pid]) => pid < 0)).toEqual([[-tree.launcherPid, 'SIGKILL']]) + await waitUntil(() => !alive(tree!.pid), 2000, 'descendant exit') + await expectClean() + } finally { + kills.mockRestore() + await release(run) + } + }, TEST_TIMEOUT_MS) +}) diff --git a/src/providers/opencode/runtime/opencodeCliSessions.system.test.ts b/src/providers/opencode/runtime/opencodeCliSessions.system.test.ts index 73d541b86..99c42438b 100644 --- a/src/providers/opencode/runtime/opencodeCliSessions.system.test.ts +++ b/src/providers/opencode/runtime/opencodeCliSessions.system.test.ts @@ -1,13 +1,192 @@ import { existsSync, readFileSync } from 'node:fs' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, mkdir, readdir, realpath, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' +import { EventEmitter, getEventListeners } from 'node:events' +import { PassThrough } from 'node:stream' import { performance } from 'node:perf_hooks' -import { expect, it, vi } from 'vitest' +import { spawn } from 'node:child_process' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +// This file was created independently on two branches: output integrity for +// large exports (#845) and cancel/timeout bounds for the empty-session import +// (e9ac8bdf, Refs #864). Both suites spawn real children against the same +// runOpencode boundary, so they share one file. They also share the TMPDIR +// isolation below, which lets the cancel/timeout cases check that the new +// stdout capture directory is removed as well as the import payload. +vi.mock('node:child_process', async importOriginal => { + const actual = await importOriginal() + return { ...actual, spawn: vi.fn(actual.spawn) } +}) vi.mock('node-pty', () => ({ spawn: vi.fn() })) -import { createEmptyOpencodeSession } from './opencodeCliSessions.js' +import { + createEmptyOpencodeSession, + exportOpencodeSession, + importOpencodeSession, + listOpencodeModels, + readResolvedOpencodeConfig, +} from './opencodeCliSessions.js' import { OpencodeTerminalSession } from './opencodeTerminalSession.js' +import { holdNextSpawnUntilReady } from './testing/spawnReadiness.js' +import { alive, waitUntil, within } from './testing/processWait.js' + +const { spawn: realSpawn } = await vi.importActual('node:child_process') + +let root: string +let cwd: string +beforeEach(async () => { + root = await realpath(await mkdtemp(join(tmpdir(), 'opencode-export-test-'))) + cwd = join(root, 'project with spaces') + await mkdir(cwd) + vi.stubEnv('TMPDIR', root) + vi.stubEnv('TMP', root) + vi.stubEnv('TEMP', root) +}) +afterEach(async () => { + // mockReset, not only clearAllMocks: mockClear keeps queued + // once-implementations. A test that fails after queuing a readiness gate or a + // process double, but before runOpencode reaches spawn, would otherwise hand + // that implementation to the next test's first spawn. Vitest 4 resets + // vi.fn(actual.spawn) back to the real spawn. + vi.mocked(spawn).mockReset() + vi.clearAllMocks() + vi.unstubAllEnvs() + await rm(root, { recursive: true, force: true }) +}) +function options(mode = '') { + return { + binary: process.execPath, cwd, + env: { ...process.env, NODE_OPTIONS: `--import=${new URL('./testing/abruptCli.mjs', import.meta.url).href}`, OPENCODE_FIXTURE_MODE: mode, OPENCODE_FIXTURE_VALUE: 'environment forwarded' }, + } +} +async function expectClean() { expect(await readdir(root)).toEqual(['project with spaces']) } + +describe('OpenCode CLI output integrity', () => { + it('captures a complete large export from a process that exits immediately after writing stdout', async () => { + const id = 'ses_fixture;not-a-shell-command' + const value = await exportOpencodeSession(options(), id) + expect(value.info).toEqual({ id, directory: cwd }) + expect((value.messages as { parts: { text: string }[] }[])[0].parts[0].text).toHaveLength('fixture text '.length * 160000) + expect(value.fixtureEnv).toBe('environment forwarded') + expect(value.outputMode).toBe(0o600) + // Observed from inside the running child: its stdout already has no name, + // so a quit or crash mid-export has no plaintext file to leave behind. A + // cleanup check after settlement cannot see that difference. + expect(value.outputLinks).toBe(0) + await expectClean() + }) + + it('also preserves large resolved-config output used when OpenCode is the switch target', async () => { + const config = await readResolvedOpencodeConfig(options()) + expect(config.model).toBe('fixture/model') + expect(config.fixture).toHaveLength('fixture text '.length * 160000) + await expectClean() + }) + + it('preserves native import and removes both import and capture files', async () => { + await expect(importOpencodeSession(options(), { info: { id: 'ses_import' }, messages: [] })).resolves.toBe('ses_import') + await expectClean() + }) + + it('rejects failed native commands rather than parsing their stdout, and cleans up', async () => { + await expect(exportOpencodeSession(options('failure'), 'ses_fixture')).rejects.toThrow('fixture command refused') + await expectClean() + }) + + it('rejects genuinely invalid JSON without including transcript text in the error', async () => { + const failure = await exportOpencodeSession(options('invalid'), 'ses_fixture').catch((error: Error) => error) + expect(failure).toBeInstanceOf(Error) + expect((failure as Error).message).toContain('did not return valid JSON') + expect((failure as Error).message).not.toContain('PRIVATE') + await expectClean() + }) + + it('rejects oversized file output before allocating it for JSON parsing', async () => { + await expect(exportOpencodeSession(options('oversized'), 'ses_fixture')).rejects.toThrow('output exceeds') + await expectClean() + }) + + it('cleans up when the CLI cannot be spawned', async () => { + // `ENOENT`, not just `failed`: every runOpencode rejection carries the + // `OpenCode export failed:` prefix, so a timeout, overflow or parse error + // would satisfy a weaker assertion without exercising spawn failure. + await expect(exportOpencodeSession({ ...options(), binary: join(root, 'missing') }, 'ses_fixture')).rejects.toThrow('ENOENT') + await expectClean() + }) + + it('terminates over-limit capture while the child is still running', async () => { + await expect(exportOpencodeSession(options('oversized-running'), 'ses_fixture')).rejects.toThrow('output exceeds') + await expectClean() + }) + + it('drains large stderr but retains only a bounded diagnostic prefix', async () => { + const error = await exportOpencodeSession(options('stderr-overflow'), 'ses_fixture').catch((error: Error) => error) + expect(error).toBeInstanceOf(Error) + expect((error as Error).message).toContain('[stderr truncated]') + expect(Buffer.byteLength((error as Error).message)).toBeLessThan(64 * 1024 + 100) + await expectClean() + }) + + it('preserves line-oriented model discovery through the same capture boundary', async () => { + await expect(listOpencodeModels(options())).resolves.toEqual(['fixture/model-a', 'fixture/model-b']) + await expectClean() + }) + + it('retains capture ownership after a post-spawn error until process close', async () => { + // Only this otherwise hard-to-provoke lifecycle failure uses a process + // double. The truncation/config/overflow cases above spawn real children. + const child = Object.assign(new EventEmitter(), { stderr: new PassThrough(), kill: vi.fn() }) + vi.mocked(spawn).mockReturnValueOnce(child as unknown as ReturnType) + const result = exportOpencodeSession(options(), 'ses_fixture').catch((error: Error) => error) + let timer: ReturnType | undefined + let beforeClose: string | undefined + try { + await vi.waitFor(() => expect(vi.mocked(spawn).mock.calls.length).toBe(1)) + child.emit('spawn') + child.emit('error', new Error('controlled post-spawn error')) + beforeClose = await Promise.race([ + result.then(() => 'settled before close'), + new Promise(resolve => { timer = setTimeout(() => resolve('waiting for close'), 200) }), + ]) + } finally { + clearTimeout(timer) + child.stderr.end() + child.emit('close', 1, null) + await result + } + expect(beforeClose).toBe('waiting for close') + expect((await result as Error).message).toContain('controlled post-spawn error') + await expectClean() + }) + + it('rejects a spawn that failed before stdio existed without an uncaught child error', async () => { + // Node's spawn() returns early on EMFILE/ENFILE: no pid, no stderr stream, + // and `error` then `close` arrive on the next tick. Exhausting descriptors + // inside a shared test worker would break unrelated tests, so this double + // reproduces that exact shape instead. EventEmitter throws synchronously + // from emit('error') when nothing listens; catching it here turns what + // would be a process-level uncaught exception into an assertable value. + let uncaught: unknown + const child = Object.assign(new EventEmitter(), { kill: vi.fn() }) + vi.mocked(spawn).mockImplementationOnce(() => { + process.nextTick(() => { + try { child.emit('error', Object.assign(new Error('spawn opencode EMFILE'), { code: 'EMFILE' })) } catch (error) { uncaught = error } + child.emit('close', -24, null) + }) + return child as unknown as ReturnType + }) + const controller = new AbortController() + const failure = await exportOpencodeSession({ ...options(), signal: controller.signal, timeoutMs: 50 }, 'ses_fixture').catch((error: Error) => error) + expect(uncaught).toBeUndefined() + expect((failure as Error).message).toContain('EMFILE') + expect(getEventListeners(controller.signal, 'abort')).toEqual([]) + // Past the 50 ms deadline and two size-guard polls: a timer that survived + // settlement would try to kill the (never started) child. + await new Promise(resolve => setTimeout(resolve, 250)) + expect(child.kill).not.toHaveBeenCalled() + await expectClean() + }) +}) // A real child process is necessary: a promise mock cannot establish that // cancellation kills the import process or releases its private export file. @@ -20,45 +199,99 @@ async function fixture() { return { dir, binary, statusFile } } catch (error) { await rm(dir, { recursive: true, force: true }); throw error } } -function alive(pid: number) { - try { process.kill(pid, 0); return true } catch { return false } -} +// Generous, because the readiness gate spends it before the deadline or stop() +// can fire. The bounds asserted below stay as tight as main's originals. +const IMPORT_READY_BUDGET_MS = 10_000 it.each(['stop', 'timeout'] as const)('bounds a hung import on %s and removes its payload', async mode => { const { dir, binary, statusFile } = await fixture() const spawnPty = vi.fn() const prepareLaunch = vi.fn() const session = new OpencodeTerminalSession({ cwd: dir, binary, env: { IMPORT_STATUS_FILE: statusFile } }, { spawnPty, prepareLaunch }) - let status: { pid: number; file: string } | undefined + // main's version (e9ac8bdf) waited up to 2 s for this status while the + // timeout case's 1 s deadline was already running, and on a loaded machine + // the stop case failed before stop() was ever called. The gate makes the child + // report before either trigger can fire, and hands the test its ChildProcess + // at spawn. + const importChild = holdNextSpawnUntilReady(vi.mocked(spawn), realSpawn, () => existsSync(statusFile), IMPORT_READY_BUDGET_MS) + // Exists so cleanup can end a child the timeout case never observed. It is not + // aborted before the outcome is recorded, so that case still proves the deadline. + const controller = new AbortController() let starting: Promise | undefined try { - starting = mode === 'stop' ? session.start() : createEmptyOpencodeSession({ cwd: dir, binary, env: { IMPORT_STATUS_FILE: statusFile }, timeoutMs: 1000 }) + starting = mode === 'stop' ? session.start() : createEmptyOpencodeSession({ cwd: dir, binary, env: { IMPORT_STATUS_FILE: statusFile }, signal: controller.signal, timeoutMs: 1000 }) // Attach the rejection observer before waiting for startup so a timeout is // never reported as an unhandled rejection while the fixture is inspected. const outcome = starting.then(() => 'resolved', () => 'rejected') - await waitUntil(() => existsSync(statusFile), 2000, 'import child status') - status = JSON.parse(readFileSync(statusFile, 'utf8')) - expect(existsSync(status!.file)).toBe(true) + await waitUntil(() => importChild.readyAt !== undefined, IMPORT_READY_BUDGET_MS + 5000, 'import child spawn') + expect(existsSync(statusFile), 'import child ready before its deadline or stop() can fire').toBe(true) + const status = JSON.parse(readFileSync(statusFile, 'utf8')) as { pid: number; file: string } + const child = importChild.child! + expect(status.pid).toBe(child.pid) + expect(existsSync(status.file)).toBe(true) + // While the child runs, its import payload still needs a path (`opencode + // import ` reads it by name), but the stdout capture must already be + // unnamed: nothing an app quit at this moment could leave on disk. + expect((await readdir(root)).filter(name => name.startsWith('agent-code-opencode-output-'))).toEqual([]) const stoppedAt = performance.now() if (mode === 'stop') await session.stop() - await waitUntil(() => !alive(status!.pid), 2000, 'import child exit') + // Read from the ChildProcess handle, not a raw pid that could be reused once + // reaped. The fixture ignores SIGTERM, so only SIGKILL ends it in time. + await waitUntil(() => child.exitCode !== null || child.signalCode !== null, 2000, 'import child exit') expect(performance.now() - stoppedAt).toBeLessThan(2000) + expect(child.signalCode).toBe('SIGKILL') expect(await outcome).toBe(mode === 'stop' ? 'resolved' : 'rejected') - expect(existsSync(dirname(status!.file))).toBe(false) + expect(existsSync(dirname(status.file))).toBe(false) + // Since #845 the import also mints a private stdout capture directory + // next to the payload, and the timeout and stop kill paths are owned by + // runOpencode rather than delegated to spawn's built-in options. A + // leftover agent-code-opencode-* directory here means a kill path skipped + // cleanup. + expect((await readdir(root)).filter(name => name.startsWith('agent-code-opencode-'))).toEqual([]) expect(spawnPty).not.toHaveBeenCalled() expect(prepareLaunch).not.toHaveBeenCalled() } finally { - if (status && alive(status.pid)) process.kill(status.pid, 'SIGKILL') + // Abort before awaiting: runOpencode's own terminate() ends a child the test + // never observed, through `controller` for the direct helper call and + // through stop() for the session. The handle kill is a no-op once reaped. + controller.abort() await session.stop() + importChild.child?.kill('SIGKILL') await starting?.catch(() => {}) await rm(dir, { recursive: true, force: true }) } -}) +}, 30_000) -async function waitUntil(predicate: () => boolean, timeoutMs: number, label: string): Promise { - const deadline = performance.now() + timeoutMs - while (!predicate()) { - if (performance.now() >= deadline) throw new Error(`timed out waiting for ${label}`) - await new Promise(resolve => setImmediate(resolve)) +it('kills a CLI whose stop arrived while its capture was still being prepared', async () => { + // runOpencode runs synchronously until mkdtemp suspends, so an abort issued + // right after the call lands after the pre-abort check but before the abort + // listener exists. `once` listeners are not replayed for past aborts, so only + // the post-spawn `signal.aborted` replay can stop this child. Without it the + // SIGTERM-ignoring fixture would live until the 30 s deadline, and stop() + // during capture setup would silently do nothing. + const { dir, binary, statusFile } = await fixture() + const controller = new AbortController() + const outcome = exportOpencodeSession({ binary, cwd: dir, env: { IMPORT_STATUS_FILE: statusFile }, signal: controller.signal }, 'ses_fixture') + .then(() => new Error('resolved'), (error: Error) => error) + controller.abort() + const spawnedPid = () => vi.mocked(spawn).mock.results[0]?.value?.pid as number | undefined + try { + const failure = await within(outcome, 2000) + expect(failure, 'late stop settles promptly').toBeInstanceOf(Error) + expect((failure as Error).message).toContain('OpenCode command cancelled') + // A real child was spawned: this is the replay path, not the "already + // stopped, create nothing" path the pre-abort check handles. + expect(spawnedPid()).toBeGreaterThan(0) + await waitUntil(() => !alive(spawnedPid()!), 2000, 'cancelled child exit') + expect((await readdir(root)).filter(name => name.startsWith('agent-code-opencode-'))).toEqual([]) + } finally { + const pid = spawnedPid() + if (pid && alive(pid)) process.kill(pid, 'SIGKILL') + await outcome + await rm(dir, { recursive: true, force: true }) } -} +}) + +// The launcher process-tree termination cases live in +// opencodeCliSessions.processTree.system.test.ts, next to the spawn readiness +// seam they need. diff --git a/src/providers/opencode/runtime/opencodeCliSessions.ts b/src/providers/opencode/runtime/opencodeCliSessions.ts index f24f4bd17..e83a4531a 100644 --- a/src/providers/opencode/runtime/opencodeCliSessions.ts +++ b/src/providers/opencode/runtime/opencodeCliSessions.ts @@ -1,10 +1,12 @@ -import { execFile } from 'node:child_process' +import { spawn } from 'node:child_process' import { randomUUID } from 'node:crypto' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { fstatSync } from 'node:fs' +import { mkdtemp, open, rm, rmdir, unlink, writeFile, type FileHandle } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' const MAX_EXPORT_BYTES = 256 * 1024 * 1024 +const MAX_STDERR_BYTES = 64 * 1024 export type OpencodeCliSessionOptions = { binary: string @@ -32,9 +34,9 @@ export async function exportOpencodeSession( let value: unknown try { value = JSON.parse(stdout) - } catch (error) { + } catch { throw new Error( - `OpenCode export for ${sessionId} did not return valid JSON: ${errorMessage(error)}`, + `OpenCode export for ${sessionId} did not return valid JSON (${Buffer.byteLength(stdout)} bytes captured).`, ) } if (!isRecord(value)) { @@ -135,41 +137,274 @@ async function runOpencode( options: OpencodeCliSessionOptions, args: string[], ): Promise<{ stdout: string; stderr: string }> { + let directory: string | undefined + let output: FileHandle | undefined try { + // Checked before minting the capture directory so an already-stopped + // caller creates nothing on disk. It sits inside this try (as it did when + // runOpencode still used execFile) so cancellation keeps the same wrapped + // `OpenCode failed: ...` shape as every other failure. options.signal?.throwIfAborted() - const result = await new Promise<{ stdout: string; stderr: string }>((resolve, reject) => { - const child = execFile(options.binary, args, { - cwd: options.cwd, - env: options.env ?? process.env, - encoding: 'utf8', - maxBuffer: MAX_EXPORT_BYTES, - timeout: Math.max(1, options.timeoutMs ?? 30_000), - killSignal: 'SIGKILL', - }, (error, stdout, stderr) => { + directory = await mkdtemp(join(tmpdir(), 'agent-code-opencode-output-')) + const outputPath = join(directory, 'stdout') + // OpenCode 1.18.30 writes stdout, then process.exit() without awaiting the + // pipe flush (cli/cmd/export.ts + index.ts). Raising execFile.maxBuffer + // cannot recover bytes the producer never delivered: a real 14.6 MB export + // stopped at 64 KiB. A regular-file stdout descriptor avoids that async-pipe + // exit race. Keep this at the CLI boundary: resolved config can be large too. + // The same rule applies to any new OpenCode command: output that always + // fits one pipe buffer (`db path` in opencode-terminal-headless, + // `--version` in setup/cliVersion) is safe on a pipe, because a write + // smaller than the buffer completes before exit; anything that can exceed + // ~64 KiB must go through this capture. + // + // WHY the capture is unlinked, and its private directory removed, BEFORE + // the child starts: the child inherits the descriptor, not the path, and + // this handle is all the parent needs to poll and read it. A file with no + // name is freed by the kernel when its last descriptor closes, so an app + // quit, a main-process crash or an orphaned child can no longer leave a + // plaintext copy of a transcript in the temp directory. Before this, only + // the async finally below deleted it, and an ordinary quit does not wait + // for that: finite CLI commands are not part of any shutdown drain. The + // remaining window is mkdtemp -> open -> unlink, a few syscalls before any + // byte is written, inside a 0700 directory with a 0600 file. + // `wx+` rather than `wx`: the parent reads the result back through this + // same handle (see readCapture). + // + // Out of scope: the payload importOpencodeSession writes is handed to + // `opencode import ` BY PATH, so it must keep a name while the child + // runs and is still removed only by that function's finally. + output = await open(outputPath, 'wx+', 0o600) + await unlink(outputPath) + await rmdir(directory) + // Cleared only once nothing is left on disk. While it is still set, the + // finally below removes whatever a failed setup step left behind. + directory = undefined + const fd = output.fd + const stderrChunks: Buffer[] = [] + let stderrBytes = 0 + let stderrTruncated = false + const stderrText = () => Buffer.concat(stderrChunks).toString('utf8') + (stderrTruncated ? '\n[stderr truncated]' : '') + const timeoutMs = Math.max(1, options.timeoutMs ?? 30_000) + await new Promise((resolve, reject) => { + // Two changes met here when #846 was merged with main. Main (e9ac8bdf, + // Refs #864) bounded every CLI call with execFile's `timeout` plus an + // owned abort listener, both SIGKILL, so a hung empty-session import + // cannot hold OpenCode Terminal startup or survive stop(). This branch + // (#845) replaced execFile with spawn so stdout can be a regular file. + // + // WHY the deadline and the abort are owned here instead of passed as + // spawn's own `timeout` / `signal` / `killSignal` options, which Node + // 24's spawn does accept: + // - The deadline records a distinct `timed out after N ms` failure. With + // spawn's timeout, `close` would only report signal SIGKILL, which is + // indistinguishable from an external kill or the OOM killer. + // - spawn's `signal` path emits an AbortError `error` and, for an + // already-aborted signal, kills on nextTick. Owning the listener keeps + // ONE cancellation outcome, `OpenCode command cancelled`, which + // OpencodeTerminalSession relies on to treat stop() as expected. + // - None of spawn's built-in kill paths destroy stdio or reach + // descendants (execFile's kill did destroy stdio, which the merge + // first lost), so none of them would bound settlement when a + // descendant holds stderr. terminate() below does both. + // + // The invariant from main still holds: this promise settles ONLY from + // `close`, never from the abort or timeout event. Rejecting earlier + // would let importOpencodeSession's finally delete the import payload + // while the child is still reading it, and would return a capture the + // child is still writing. terminate() is what keeps that wait bounded. + const child = spawn(options.binary, args, { + cwd: options.cwd, env: options.env ?? process.env, + stdio: ['ignore', fd, 'pipe'], + // WHY a private process group (detached, deliberately WITHOUT unref) + // on POSIX: the `opencode` on PATH or in the cached tool path can be + // OpenCode's npm launcher (anomalyco/opencode v1.18.30, + // packages/opencode/bin/opencode). + // It is a Node script that spawns the native binary as ITS child with + // `stdio: "inherit"` and only forwards SIGINT/SIGTERM/SIGHUP. SIGKILL + // cannot be forwarded, so killing only the direct child left the + // native process alive, still holding the stdout capture and the + // stderr pipe: `close` never arrived and the timeout, stop and + // overflow bounds all hung while the native CLI kept exporting or + // mutating provider state. In its own group, terminate() reaches the + // whole tree with one signal. Skipping unref() keeps the parent + // tracking the child exactly as before; on POSIX `detached` only + // means setsid(). + // + // Why not resolve and exec the native binary instead: the launcher + // picks it from OPENCODE_BIN_PATH, a cached copy, or one of several + // platform/arch/AVX2-baseline/musl optional packages. Re-deriving that + // couples Agent Code to OpenCode's private install layout (npm, brew, + // curl installer all differ) and still would not cover a native CLI + // or plugin that starts helpers of its own. A group does not care how + // the tree was formed. + // + // Consequences: the child has no controlling terminal (these commands + // are non-interactive and stdin is ignored), and a Ctrl+C in a dev + // terminal no longer reaches it directly, so the CLI runs to its own + // end. The deadline is not a property of the child: it is a timer in + // Agent Code's main process and ends with it. A Ctrl+C that kills + // Electron therefore leaves a wedged CLI unbounded, where the old + // non-detached child sat in the terminal's foreground group and got the + // SIGINT (which the npm launcher forwards). Packaged quits already + // orphaned these children before this change. On Windows `detached` + // would open a console window and there is no process-group kill, so it + // keeps the direct-child kill. + detached: process.platform !== 'win32', + }) + let failure: Error | undefined + // WHY the lifecycle listeners come first, before timers and stdio: on + // EMFILE/ENFILE Node's spawn() returns early with no pid and NO stderr + // stream, then emits `error` (and `close`) on the next tick. Descriptor + // exhaustion is reachable in a busy Electron main process even though + // open() above succeeded, because the stderr pipe and the exec need more + // descriptors. The old order dereferenced `child.stderr!` first: that + // threw inside this executor, so the promise rejected with a TypeError, + // the timers and abort listener were never cleared, and the next-tick + // `error` had no listener. An unhandled ChildProcess `error` is an + // uncaught exception, and installCrashHooks exits the app on those. + // Nothing that can throw may run before these exist. + // + // Node also emits error when a kill fails, not just on failed spawn. + // Record it and still settle only at close (which follows spawn errors + // too), otherwise the import payload could be deleted, or a capture + // returned, while its producer is alive. + child.on('error', error => { failure ??= error }) + child.once('close', (code, signal) => { + // Referencing the timers declared below is safe: `close` is always + // emitted asynchronously, after this executor has run to completion. + clearInterval(sizeGuard) + clearTimeout(deadline) options.signal?.removeEventListener('abort', abort) - // execFile's callback waits for process/stdio teardown. Rejecting from - // the abort event itself would remove the payload and settle startup - // while the child can still be alive and reading that file. if (options.signal?.aborted) reject(new Error('OpenCode command cancelled')) - else if (error) reject(Object.assign(error, { stderr })) - else resolve({ stdout, stderr }) + else if (failure) reject(failure) + else if (code !== 0) reject(new Error(stderrText().trim() || `exited with ${signal ?? code}`)) + else resolve() }) - // Node's execFile signal path uses spawn's default SIGTERM independently - // of execFile's timeout killSignal. Own this abort listener so a CLI that - // ignores SIGTERM cannot outlive stop(); its payload is disposable. - const abort = () => { child.kill('SIGKILL') } + // ONE termination path for the deadline, stop() and the size guard, so + // the three bounds cannot drift apart again. + // (a) Destroy stderr first. Settlement waits for `close`, and `close` + // waits for stderr EOF. A descendant that inherited fd 2 and left + // the process group (so (b) cannot reach it) would otherwise hold + // the promise open for as long as it lives. This mirrors execFile's + // own kill(), which destroyed stdout/stderr before signalling. Only + // diagnostics written after the outcome was decided are lost. + // stdout is a plain descriptor, not a stream: nothing to destroy. + // (b) SIGKILL the whole group via the negative pid, at most once, and + // only while the direct child is still unreaped. + // A numeric group id is only an identity while something still holds + // it. Node reaps the child and sets exitCode/signalCode before it + // emits `exit`, but `close` can come much later because it also + // waits for stderr EOF. Timers and the abort listener stay armed + // until `close`. So once the leader has been reaped and every + // remaining stderr holder has left the group, the id is free for any + // new session or job to reuse, and a late deadline, overflow or + // stop() would SIGKILL an unrelated group. While exitCode and + // signalCode are both null the leader (running or a zombie) still + // reserves the id. libuv reaps and runs Node's exit callback in the + // same loop turn, so the check below is re-evaluated immediately + // before the signal with no event of this command in between. + // Trade-off: a same-group member that outlives an exited leader is + // no longer killed. It can only append to the unlinked capture, and + // readCapture is bounded by the fstat size taken after `close`. The + // npm launcher that motivated the group is unaffected: it waits for + // its native child, so the leader is alive whenever the tree is. + // At most once: a timeout or overflow followed by stop() before + // `close` must not signal the number again, because the first + // SIGKILL may already have released it. `signalled` is set before + // signalling, so re-entry from a listener cannot signal twice. + // The `pid > 0` check is load-bearing: process.kill(-0) would signal + // Agent Code's OWN group. If the group cannot be signalled, fall back + // to the direct child's handle, which Node never aims at a reaped pid. + let signalled = false + const terminate = () => { + child.stderr?.destroy() + if (signalled || child.exitCode !== null || child.signalCode !== null) return + signalled = true + if (process.platform !== 'win32' && typeof child.pid === 'number' && child.pid > 0) { + try { + process.kill(-child.pid, 'SIGKILL') + return + } catch { + // Group already gone or not signalable: fall through. + } + } + child.kill('SIGKILL') + } + const deadline = setTimeout(() => { + failure ??= new Error(`timed out after ${timeoutMs} ms`) + terminate() + }, timeoutMs) + // No `failure` is recorded for abort: close checks `signal.aborted` + // first, so cancellation wins over any overflow or timeout that raced it. + // That matches the execFile version, which callers such as + // OpencodeTerminalSession treat as an expected stop, not a failure. + const abort = () => { terminate() } options.signal?.addEventListener('abort', abort, { once: true }) + // Detect overflow while the producer runs, then check its final size + // before allocating a string. Polling is a soft disk bound, not a hard + // quota. Overflow fails the command; never parse a truncated prefix. + // Killing the whole tree is what makes the disk bound real: before the + // group kill, a surviving native writer kept growing the file after + // `failure` had already switched this guard off. + const sizeGuard = setInterval(() => { + if (failure) return + try { + if (fstatSync(fd).size > MAX_EXPORT_BYTES) throw new Error('output exceeds the 256 MiB capture limit') + } catch (error) { + failure = error instanceof Error ? error : new Error('Cannot inspect CLI output') + terminate() + } + }, 100) + sizeGuard.unref() + // Optional: absent when spawn failed before stdio setup (see above). + child.stderr?.on('data', (chunk: Buffer) => { + const keep = Math.min(chunk.length, MAX_STDERR_BYTES - stderrBytes) + if (keep > 0) { stderrChunks.push(Buffer.from(chunk.subarray(0, keep))); stderrBytes += keep } + if (keep < chunk.length) stderrTruncated = true + }) + // The signal may have fired while the capture was being minted, before + // the listener existed; `once` listeners are not replayed for past aborts. if (options.signal?.aborted) abort() }) - return { stdout: result.stdout, stderr: result.stderr } + // fstat through the retained handle: the capture has no path to stat. + const { size } = await output.stat() + if (size > MAX_EXPORT_BYTES) throw new Error('output exceeds the 256 MiB capture limit') + return { stdout: await readCapture(output, size), stderr: stderrText() } } catch (error) { - const detail = isRecord(error) && typeof error.stderr === 'string' - ? error.stderr.trim() - : '' throw new Error( - `OpenCode ${args[0] ?? 'command'} failed${detail ? `: ${detail}` : `: ${errorMessage(error)}`}`, + `OpenCode ${args[0] ?? 'command'} failed: ${errorMessage(error)}`, ) + } finally { + // Reached only after `close`, or before anything was spawned. Closing the + // last descriptor Agent Code owns frees the unnamed capture. `directory` + // is still set only when setup failed between mkdtemp and rmdir; removal + // failure stays non-fatal, matching the import payload policy, and native + // history is never touched. + try { await output?.close() } + finally { if (directory) await rm(directory, { recursive: true, force: true }).catch(() => undefined) } + } +} + +/** + * Read the whole unnamed capture through its retained handle. + * + * WHY positional reads rather than `handle.readFile()`: the child's stdout is a + * dup of this very descriptor, so both share one file offset, and the child's + * writes left it at end-of-file. FileHandle.readFile reads from the current + * offset and would return an empty string for a complete export. Positional + * reads ignore the shared offset. `size` is the fstat that enforced the limit, + * so the allocation stays bounded even if a stray writer appended afterwards. + */ +async function readCapture(handle: FileHandle, size: number): Promise { + const buffer = Buffer.allocUnsafe(size) + let offset = 0 + while (offset < size) { + const { bytesRead } = await handle.read(buffer, offset, size - offset, offset) + if (bytesRead === 0) break + offset += bytesRead } + return buffer.toString('utf8', 0, offset) } function errorMessage(error: unknown): string { diff --git a/src/providers/opencode/runtime/testing/abruptCli.mjs b/src/providers/opencode/runtime/testing/abruptCli.mjs new file mode 100644 index 000000000..22d7a6326 --- /dev/null +++ b/src/providers/opencode/runtime/testing/abruptCli.mjs @@ -0,0 +1,52 @@ +// Transport fault injection matching OpenCode 1.18.30's write-then-exit +// lifecycle. This is synthetic content, not a recorded personal transcript. +// NODE_OPTIONS preloads it before Node tries to resolve the CLI command name. +import { fstatSync, ftruncateSync, readFileSync, writeSync } from 'node:fs' +import { basename } from 'node:path' + +const command = basename(process.argv[1]) +const mode = process.env.OPENCODE_FIXTURE_MODE +if (mode === 'failure') { + writeSync(2, 'fixture command refused') + process.exit(7) +} +if (mode === 'invalid') { + writeSync(1, 'PRIVATE_FIXTURE_CONTENT is not JSON') + process.exit(0) +} +if (mode === 'oversized' || mode === 'oversized-running') { + // Sparse output tests the existing 256 MiB guard without allocating or + // physically writing hundreds of MiB in an ordinary regression suite. + ftruncateSync(1, 256 * 1024 * 1024 + 1) + if (mode === 'oversized-running') await new Promise(() => setTimeout(() => process.exit(7), 2000)) + process.exit(0) +} +if (mode === 'stderr-overflow') { + process.stderr.write('fixture diagnostic '.repeat(15000), () => process.exit(7)) + await new Promise(() => {}) +} +if (command === 'models') { + writeSync(1, 'fixture/model-a\nfixture/model-b\n') + process.exit(0) +} +if (command === 'import') { + const value = JSON.parse(readFileSync(process.argv[2], 'utf8')) + writeSync(1, value.info.id) + process.exit(0) +} +const text = 'fixture text '.repeat(160000) +const sessionID = process.argv[2] +const value = command === 'debug' + ? { model: 'fixture/model', fixture: text } + : { + info: { id: sessionID, directory: process.cwd() }, + messages: [{ info: { id: 'msg_fixture', sessionID, role: 'assistant', time: { created: 1, completed: 2 } }, parts: [{ type: 'text', text }] }], + fixtureEnv: process.env.OPENCODE_FIXTURE_VALUE, + outputMode: fstatSync(1).isFile() ? fstatSync(1).mode & 0o777 : null, + // Link count of the capture as the running child sees it. Zero means the + // parent unlinked it before spawn, so no name exists to leave behind. + outputLinks: fstatSync(1).isFile() ? fstatSync(1).nlink : null, + } +process.stdout.write(JSON.stringify(value)) +process.stdout.write('\n') +process.exit(0) diff --git a/src/providers/opencode/runtime/testing/launcherCli.mjs b/src/providers/opencode/runtime/testing/launcherCli.mjs new file mode 100644 index 000000000..d5f25d7be --- /dev/null +++ b/src/providers/opencode/runtime/testing/launcherCli.mjs @@ -0,0 +1,71 @@ +// Process-tree fault injection shaped like OpenCode's npm launcher +// (packages/opencode/bin/opencode at anomalyco/opencode v1.18.30): a Node +// script that spawns the native executable with `stdio: "inherit"` and only +// forwards SIGINT/SIGTERM/SIGHUP. SIGKILL cannot be forwarded, so killing the +// launcher alone leaves the "native" descendant holding the stdout capture and +// the stderr pipe. Both roles hang, so a CLI call can only settle if Agent Code +// terminates the tree, or at least stops waiting on the inherited stderr. +// +// NODE_OPTIONS preloads this file as the launcher, like abruptCli.mjs. The +// descendant runs the same file as its main module with the preload cleared, +// so it cannot recursively become another launcher. +// +// OPENCODE_LAUNCHER_DESCENDANT selects the descendant's behaviour: +// - hang: stays in the launcher's process group and never exits. +// - overflow: as hang, after growing stdout past the 256 MiB capture limit. +// - escaped: starts its own session, like a plugin helper that daemonizes, so +// a process-group kill cannot reach it and only releasing stderr lets the +// launcher's `close` arrive. +// - escaped-exit: as escaped, but the launcher then exits 0 at once, like a CLI +// whose root finished while a daemonized helper still holds stderr. The +// parent reaps the launcher long before `close`, and the launcher's group id +// is released while the command is still pending. +// +// OPENCODE_LAUNCHER_SPAWNED names the file where the launcher records the +// descendant's pid as soon as spawn() returns. OPENCODE_LAUNCHER_DESCENDANT_DELAY_MS +// delays only the descendant's readiness report, so a test can force a +// readiness failure while the descendant demonstrably exists. +import { spawn } from 'node:child_process' +import { ftruncateSync, renameSync, writeFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' + +const behaviour = process.env.OPENCODE_LAUNCHER_DESCENDANT +const statusFile = process.env.OPENCODE_LAUNCHER_STATUS +if (process.env.OPENCODE_LAUNCHER_ROLE === 'descendant') { + // The status file is the tree's readiness signal. It carries the launcher + // pid the launcher itself passed down: process.ppid is not reliable, because + // an escaped descendant whose launcher already exited has been reparented. + // Status comes before overflow so the test learns both pids before the size + // guard kills the tree. + const report = () => { + writeFileSync(`${statusFile}.tmp`, JSON.stringify({ pid: process.pid, launcherPid: Number(process.env.OPENCODE_LAUNCHER_PID) })) + renameSync(`${statusFile}.tmp`, statusFile) + // Sparse, as in abruptCli.mjs: over the limit without writing 256 MiB. + if (behaviour === 'overflow') ftruncateSync(1, 256 * 1024 * 1024 + 1) + } + const delay = Number(process.env.OPENCODE_LAUNCHER_DESCENDANT_DELAY_MS ?? 0) + if (delay > 0) setTimeout(report, delay) + else report() +} else { + const descendant = spawn(process.execPath, [fileURLToPath(import.meta.url)], { + env: { ...process.env, NODE_OPTIONS: '', OPENCODE_LAUNCHER_ROLE: 'descendant', OPENCODE_LAUNCHER_PID: String(process.pid) }, + stdio: 'inherit', + detached: behaviour === 'escaped' || behaviour === 'escaped-exit', + }) + // Record the descendant's pid the moment it exists, before it initializes or + // reports. A test whose readiness wait failed never gets the status file, and + // an escaped descendant is out of reach of the process-group kill, so this + // record is the only way cleanup can still find and reclaim it. + const spawned = process.env.OPENCODE_LAUNCHER_SPAWNED + writeFileSync(`${spawned}.tmp`, String(descendant.pid)) + renameSync(`${spawned}.tmp`, spawned) + // spawn() has already forked and exec'd, so the helper keeps its inherited + // descriptors after this exit. + if (behaviour === 'escaped-exit') process.exit(0) +} +// Neither role exits on SIGTERM, so only a SIGKILL path can end them. +process.on('SIGTERM', () => {}) +setInterval(() => {}, 1000) +// As the preload, this also stops Node from resolving the CLI "command name" +// (`export`) as a script to run. +await new Promise(() => {}) diff --git a/src/providers/opencode/runtime/testing/processWait.ts b/src/providers/opencode/runtime/testing/processWait.ts new file mode 100644 index 000000000..bce6193ce --- /dev/null +++ b/src/providers/opencode/runtime/testing/processWait.ts @@ -0,0 +1,45 @@ +import { performance } from 'node:perf_hooks' + +// Process polling helpers shared by the OpenCode CLI system tests +// (opencodeCliSessions.system.test.ts and its processTree sibling). They live in +// one module so a fix to how the tests wait cannot land in one file and +// silently miss the other. + +/** + * Whether a pid currently names a process. + * + * Only for processes a test cannot observe through a ChildProcess handle, such + * as a fixture's grandchild. A raw pid is not an identity: a zombie still + * answers, and a reaped pid can be reused. For direct children prefer the + * handle's `exitCode` / `signalCode`. + */ +export function alive(pid: number): boolean { + try { process.kill(pid, 0); return true } catch { return false } +} + +/** The settled value of `promise`, or 'pending' if it has not settled within `ms`. */ +export async function within(promise: Promise, ms: number): Promise { + let timer: ReturnType | undefined + try { + return await Promise.race([promise, new Promise<'pending'>(resolve => { timer = setTimeout(() => resolve('pending'), Math.max(0, ms)) })]) + } finally { + clearTimeout(timer) + } +} + +/** + * Poll `predicate` until it holds, failing with `label` after `timeoutMs`. + * + * WHY setImmediate rather than a fixed interval: each turn lets the child + * process and filesystem callbacks the predicate depends on run, without adding + * a delay to every check. The deadline is monotonic (performance.now), as + * docs/testing/standard.md requires, so a wall-clock change cannot stretch or + * cut it. + */ +export async function waitUntil(predicate: () => boolean, timeoutMs: number, label: string): Promise { + const deadline = performance.now() + timeoutMs + while (!predicate()) { + if (performance.now() >= deadline) throw new Error(`timed out waiting for ${label}`) + await new Promise(resolve => setImmediate(resolve)) + } +} diff --git a/src/providers/opencode/runtime/testing/spawnReadiness.ts b/src/providers/opencode/runtime/testing/spawnReadiness.ts new file mode 100644 index 000000000..24505e9fc --- /dev/null +++ b/src/providers/opencode/runtime/testing/spawnReadiness.ts @@ -0,0 +1,45 @@ +import type { ChildProcess, spawn } from 'node:child_process' +import { performance } from 'node:perf_hooks' +import type { MockedFunction } from 'vitest' + +export type HeldSpawn = { child?: ChildProcess; readyAt?: number } + +/** + * Let the next call to a mocked `spawn` start the real child, but return it only + * once that child has reported readiness (or `budgetMs` has passed). + * + * WHY: runOpencode arms its deadline, its size guard and its abort handling only + * after spawn() returns. A test that waits for readiness while those bounds are + * already running makes readiness compete with them. On a heavily loaded machine + * (which is how this was found: a stop case failed before stop() ever ran) Node + * can take longer to start a child than a 1 s deadline or a 2 s readiness wait. + * A correct deadline then kills the child before it reports, or the wait gives + * up first. Neither failure says anything about termination. + * + * Holding the synchronous spawn call makes readiness a precondition of every + * bound, so the bounds that ARE the contract stay tight instead of being + * widened. Atomics.wait is the only synchronous sleep available; the child keeps + * running while this worker waits. + * + * `child` is filled at spawn time, so the test owns the child even when + * readiness never arrives. When the budget runs out, spawn returns anyway and + * the caller's readiness assertion reports it. + */ +export function holdNextSpawnUntilReady( + mockedSpawn: MockedFunction, + realSpawn: typeof spawn, + isReady: () => boolean, + budgetMs: number, +): HeldSpawn { + const held: HeldSpawn = {} + mockedSpawn.mockImplementationOnce(((...args: Parameters) => { + const child = realSpawn(...args) + held.child = child + const readyBy = performance.now() + budgetMs + const pause = new Int32Array(new SharedArrayBuffer(4)) + while (!isReady() && performance.now() < readyBy) Atomics.wait(pause, 0, 0, 10) + held.readyAt = performance.now() + return child + }) as typeof spawn) + return held +}