diff --git a/assets/coverage.svg b/assets/coverage.svg index 932bbcc..3a0cf59 100644 --- a/assets/coverage.svg +++ b/assets/coverage.svg @@ -1,5 +1,5 @@ - - coverage: 92.59% + + coverage: 92.43% @@ -17,7 +17,7 @@ coverage coverage - 92.59% - 92.59% + 92.43% + 92.43% \ No newline at end of file diff --git a/docs/development.md b/docs/development.md index a2f5ed8..501a7d0 100644 --- a/docs/development.md +++ b/docs/development.md @@ -25,6 +25,10 @@ convoy/ │ ├── cli.ts # flag parsing │ ├── runner.ts # pipeline orchestration │ ├── opencode.ts # startup/control via SDK +│ ├── managed-server.ts # owned `opencode serve` child launch + bounded observed stop +│ ├── process-stop.ts # shared SIGTERM→SIGKILL stop state machine +│ ├── process-identity.ts # kernel birth/UID/executable probes (Linux /proc, macOS libproc) +│ ├── process-records.ts # private lifecycle records under ~/.convoy/processes + reconciliation │ ├── agents.ts # prompt loading, agent config, bash policy │ ├── project-context.ts # automatic .convoy/rules.md, AGENTS.md, CLAUDE.md discovery │ ├── permissions.ts # live permission gate for tool calls that fall outside the allowlist diff --git a/docs/running.md b/docs/running.md index 4c2484a..934a78e 100644 --- a/docs/running.md +++ b/docs/running.md @@ -102,7 +102,7 @@ convoy --prompt-file prd.md --base develop convoy --prompt-file prd.md --include-dirty ``` -In interactive terminals, Convoy shows a full-screen OpenTUI dashboard headed by a compact run summary (clock, elapsed, cost, tokens). The `pipeline` panel on the left is a tab selector: every step — done, running, or still scheduled — is a row you move through with `↑`/`↓` (or `j`/`k`), or by clicking, with `▸` marking the focused one. Focusing a step drives the whole right side to it: a detail panel (name; whether it's ongoing, done, failed, or scheduled; model; cost; tokens; attempt; files changed) over that step's todo list and a three-tab content panel — switched with `←`/`→`, `Tab`, the number keys `1`/`2`/`3`, or by clicking the tab strip. The tabs are `logs` (the step's color-coded activity feed), `reports` (the markdown report that step wrote, if any, scrollable with `PgUp`/`PgDn` — available live the moment a step finishes, not only at the end), and `session` (a read-only "follow along" view of that step's OpenCode session: its live state — reasoning, running a command, editing, applying a diff — model, attempt, cost, diff summary, and a scrolling transcript of what the model is doing, newest at the bottom). A not-yet-started step reads as `scheduled` with its planned model and zeroed usage, so you can inspect what's coming; focus auto-follows the active step until you navigate, and `Esc` hands it back to auto-follow. The dashboard never paints backgrounds: the canvas is your terminal's own background and panels are delineated by borders alone, derived as subtle elevations of the terminal's reported background color, with dark or light accents picked by its brightness (and a neutral fallback when the terminal doesn't answer); floating modals repaint the reported color exactly to mask the content beneath them. It follows live theme changes. For full interactivity, press `o` (or click the detail panel) to open the focused step's OpenCode session in a new terminal window attached to Convoy's running OpenCode server; clicking a pipeline row only focuses that step — it no longer opens the session. Inside Herdr or Zellij that session opens in a sibling pane instead (see below); otherwise Ghostty is preferred when installed and Terminal.app is the fallback (`CONVOY_TERMINAL=herdr|zellij|ghostty|terminal` forces a backend). Press `Shift+Tab` to cycle auto-accept modes — off, auto-accept, smart (see the permission gate below). Press `Ctrl+C` once to abort the active OpenCode session and shut down Convoy cleanly; press it again to force exit if cleanup hangs. Human gates stay inside the dashboard (`c` continue · `o` open OpenCode · `a` abort); without a TTY dashboard they fall back to plain terminal prompts. A step that fails now waits for you instead of retrying: the dashboard shows a `step failed` gate with `r` retry clean (restore the baseline and run again), `o` open the OpenCode session and fix it by hand, `a` abort — no auto-retry, no lost work. Once you open the session (`o`), the gate becomes the interactive one and `c` unlocks; `c` delivers the step's report (including one written in the reopened session), and without any valid report it re-opens the gate instead of advancing to the next step. Use `--no-tui` to fall back to plain logs. +In interactive terminals, Convoy shows a full-screen OpenTUI dashboard headed by a compact run summary (clock, elapsed, cost, tokens). The `pipeline` panel on the left is a tab selector: every step — done, running, or still scheduled — is a row you move through with `↑`/`↓` (or `j`/`k`), or by clicking, with `▸` marking the focused one. Focusing a step drives the whole right side to it: a detail panel (name; whether it's ongoing, done, failed, or scheduled; model; cost; tokens; attempt; files changed) over that step's todo list and a three-tab content panel — switched with `←`/`→`, `Tab`, the number keys `1`/`2`/`3`, or by clicking the tab strip. The tabs are `logs` (the step's color-coded activity feed), `reports` (the markdown report that step wrote, if any, scrollable with `PgUp`/`PgDn` — available live the moment a step finishes, not only at the end), and `session` (a read-only "follow along" view of that step's OpenCode session: its live state — reasoning, running a command, editing, applying a diff — model, attempt, cost, diff summary, and a scrolling transcript of what the model is doing, newest at the bottom). A not-yet-started step reads as `scheduled` with its planned model and zeroed usage, so you can inspect what's coming; focus auto-follows the active step until you navigate, and `Esc` hands it back to auto-follow. The dashboard never paints backgrounds: the canvas is your terminal's own background and panels are delineated by borders alone, derived as subtle elevations of the terminal's reported background color, with dark or light accents picked by its brightness (and a neutral fallback when the terminal doesn't answer); floating modals repaint the reported color exactly to mask the content beneath them. It follows live theme changes. For full interactivity, press `o` (or click the detail panel) to open the focused step's OpenCode session in a new terminal window attached to Convoy's running OpenCode server; clicking a pipeline row only focuses that step — it no longer opens the session. Inside Herdr or Zellij that session opens in a sibling pane instead (see below); otherwise Ghostty is preferred when installed and Terminal.app is the fallback (`CONVOY_TERMINAL=herdr|zellij|ghostty|terminal` forces a backend). Press `Shift+Tab` to cycle auto-accept modes — off, auto-accept, smart (see the permission gate below). Press `Ctrl+C` once to abort the active OpenCode session and shut down Convoy cleanly; press it again to force-stop the owned OpenCode server (SIGKILL) and exit within a bounded second instead of leaving it behind. Human gates stay inside the dashboard (`c` continue · `o` open OpenCode · `a` abort); without a TTY dashboard they fall back to plain terminal prompts. A step that fails now waits for you instead of retrying: the dashboard shows a `step failed` gate with `r` retry clean (restore the baseline and run again), `o` open the OpenCode session and fix it by hand, `a` abort — no auto-retry, no lost work. Once you open the session (`o`), the gate becomes the interactive one and `c` unlocks; `c` delivers the step's report (including one written in the reopened session), and without any valid report it re-opens the gate instead of advancing to the next step. Use `--no-tui` to fall back to plain logs. When Convoy runs inside Herdr or Zellij (including over SSH), `o` and `i` open OpenCode in a focused sibling pane rather than a macOS window, named for what it holds (`opencode session`, `opencode iterate`, `claude session`). Inside Herdr the pane splits the current one to the right; inside Zellij it is a new pane. The multiplexer's normal focus shortcut returns to Convoy without closing the pane. When OpenCode exits the pane deliberately stays, showing the exit code — so a session that failed to start is readable instead of vanishing; press `Ctrl+C` there to close the pane, or `Enter` to run it again. Set `CONVOY_TERMINAL=herdr`, `zellij`, `ghostty`, or `terminal` to override automatic backend selection — any other value is rejected with an error rather than silently ignored. When both multiplexers are detected, Herdr wins because the session runs inside it — and a failed Herdr open never falls through to Zellij, which would talk to the outer session and hang or open a pane you cannot see. If Convoy is inside a multiplexer but can't find its binary on its own `PATH`, it falls back to a macOS window rather than losing session opening altogether. @@ -178,4 +178,52 @@ Every interactive manual run now displays its fully resolved plan before reposit --- +## OpenCode server lifecycle + +Convoy spawns and owns every `opencode serve` it starts — the run's server, +short-lived helpers (model discovery, commit messages, branch naming, +conversation session queries), and the repository's independently persistent +authoring service. It keeps the actual child handle, so shutdown is observed +rather than assumed. + +- **Bounded, observed stop.** Ending an owning operation sends SIGTERM, waits + 2 seconds, escalates to SIGKILL, then observes for 1 second. A delivered + signal is never reported as a confirmed stop; if exit cannot be observed the + outcome is reported as unresolved (with the identity retained) instead of + claiming success. +- **Ownership spans startup.** Child ownership is registered at spawn and the + child's process identity is published before its URL is exposed, so a boot + that fails, exits early, reports a malformed readiness line, times out, or is + cancelled still cleans up through the same handle. +- **Controller departure releases the finish screen.** A completed run parked + on its finish screen is released when its controller explicitly leaves or its + 15-second heartbeat lease expires (detected within about a second without any + further request), then the run's server stops. Active, paused, permission, + and human-gate runs are unaffected, as are independent authoring services. +- **Eventual orphan recovery.** Run/helper lifetimes are recorded under + `~/.convoy/processes/` (private, versioned, atomically written). A later + managed launch inspects a bounded number of records (32 records / 5 seconds + per pass, with a fair continuation cursor) and terminates a recorded child + only after independently confirming its original owner incarnation is gone + and the target still matches the recorded child identity and executable role, + revalidated immediately before each signal. A global name, port, or PPID + scan is never used. The identity recheck narrows but cannot eliminate the + residual POSIX window between the check and the delivered signal (most + visible on macOS); recovery is a best-effort lifecycle cleanup, not an atomic + security boundary, so do not treat it as authorization to target processes it + did not record. +- **Legacy and uncertain evidence is never a kill target.** Records without a + child identity (older runs' coordinator PID metadata), unreadable probes, and + reused PIDs are reported as skipped/uncertain and left alone. Confirmed + stops remove their record; unresolved ones are retained for a later pass. +- **Diagnostics.** Unresolved recovery prints the record location under + `~/.convoy/processes/`. Records hold only process identity, lifetime class, + lifecycle state, and a bounded outcome — never tokens, environment, config, + or prompts. +- **Limitation.** This guarantees the owned `serve` child only. MCP/tool + subprocesses can create their own sessions and process groups; stopping the + server is not a universal process-tree reaper. + +--- + [Back to documentation](README.md) diff --git a/openspec/changes/fix-opencode-server-lifecycle/.openspec.yaml b/openspec/changes/fix-opencode-server-lifecycle/.openspec.yaml new file mode 100644 index 0000000..96db9a4 --- /dev/null +++ b/openspec/changes/fix-opencode-server-lifecycle/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-15 diff --git a/openspec/changes/fix-opencode-server-lifecycle/design.md b/openspec/changes/fix-opencode-server-lifecycle/design.md new file mode 100644 index 0000000..150b267 --- /dev/null +++ b/openspec/changes/fix-opencode-server-lifecycle/design.md @@ -0,0 +1,178 @@ +## Context + +See `proposal.md` for motivation and `specs/opencode-server-lifecycle/spec.md` for the contract. This design is required because shutdown crosses the server launcher, runner, coordinator, control protocol, and runtime state; recovery also introduces process-identity and migration concerns. + +### Corroborated evidence and corrections + +The baseline audited is `e7061bb` (Convoy 0.9.0). These are source-confirmed mechanisms, not proof of the cause of every historical accumulation: + +| Evidence | Finding | Consequence | +| --- | --- | --- | +| `src/runner.ts:125-140` | Repeated requests and the 15-second timer call `process.exit(130)` | Async server release can be skipped entirely | +| `src/runner.ts:1044-1112`, `src/coordinate.ts:366-390` | Signal handlers are removed at entry to the runner's finally; hosted server release happens later, after the coordinator's finish hold | Catchable signals during teardown/hold can bypass owned-child cleanup | +| `src/opencode.ts:142-179,182-215` | Direct boot uses a single SIGTERM; SDK boot returns only URL/client/close | No awaited termination or actual run-child identity | +| Published `@opencode-ai/sdk@1.18.4`, `dist/v2/server.js:69-75`, `dist/process.js:4-13` | SDK close calls `stop`, which calls `proc.kill()` on POSIX | SDK close is not an exit acknowledgement or bounded escalation | +| `src/control-progress.ts:113-121`, `src/control-server.ts:143-153,274-327,382-393` | Controller presence is checked only before entering a finish hold; `/bye` and heartbeat expiry do not resolve it | A parked terminal hold can survive the controller indefinitely | +| `src/metadata.ts:387-397` | `server.pid` is `process.pid`, not the SDK child PID; metadata is cleared before close | Legacy fields cannot authorize orphan-child termination | +| `src/coordinate.ts:89-119` | Pending sweep removes directories for dead owners | No run-child reconciliation, and pending logs are disposable | +| `src/cli.ts:1623-1724` | Proposal fallback boots a server; unknown command discovery returns before `boundedClose`, while successful authoring returns without an explicit ownership transfer | Read-only failure paths need guaranteed close; active authoring needs an independent owner, not an indiscriminate helper stop | + +SDK evidence was read from the published [1.18.4 package tarball](https://registry.npmjs.org/@opencode-ai/sdk/-/sdk-1.18.4.tgz), extracted during the preceding investigation, not from installed workspace dependencies. Recheck the same behavior if the pinned version changes during implementation. + +The following earlier interpretations must NOT drive implementation: + +- A live process adopted by PID 1 is not necessarily abandoned; true zombies have exited and normally do not retain substantial resident memory. Historical server counts and their specific states were not reproduced in this corroboration. +- The ratio of OpenCode `creating instance`/`disposing instance` log messages is not a process leak measurement; internal instances and signal exits do not map one-to-one to those messages. +- Waiting permission/human gates and detached active runs are intentional and recoverable, not candidates for automatic cancellation. +- `ControlProgress.runFinished` currently awaits a resolve-only promise. A hypothetical rejection is a reason for defensive release placement, not an observed production failure. +- Independent authoring service persistence is compatible with `work-conversations`; its stop helper lacking production callers does not prove a leak. +- Earlier ad-hoc probes reported that a local OpenCode binary exited on SIGTERM, and that a shell child could survive it. These are limited observations, not deterministic regression tests. No new OpenCode process or model request is needed to author these artifacts. + +## Goals / Non-Goals + +**Goals:** + +- Make process ownership cover the entire interval from spawn to confirmed exit, including helper callers outside a coordinator. +- Separate best-effort application/session cleanup from compulsory bounded server cleanup. +- Recover provable run/helper orphans at a later managed startup without conflating them with durable services or old metadata. +- Test process behavior with isolated fixture children, without models, credentials, or user process mutation. + +**Non-Goals:** + +- Automatically stop authoring services on idle, terminal closure, or creator death; add a new public process-management command; or change background run semantics. +- Immediately recover from SIGKILL when no later Convoy process runs. A watchdog/daemon is deliberately not introduced. +- Kill arbitrary descendants or promise complete process-tree containment. MCP/tool subprocesses can create new sessions/process groups; stopping one group cannot guarantee their removal. This change guarantees the owned `serve` child lifecycle, not a universal tool-process reaper. +- Kill unrecorded legacy processes, treat PPID 1 as ownership, shrink OpenCode's database, or change durable run/session history. + +## Decisions + +### D1. One owned launch primitive, explicit lifetime classes + +Introduce a shared managed-server module underneath `startOpencode` and `bootOpencodeServerFrom`. Convoy owns the `node:child_process` handle, uses the existing SDK for clients, and returns an awaitable `close(): Promise` plus explicit child identity. Preserve wrapper names if useful for callers, but migrate types, fakes, and every owned-call `finally` to await close. + +Each launch must explicitly select `run`, `helper`, or `authoring-service`. Only the first two join the owner shutdown registry and dead-owner reconciliation policy. An injected service URL conveys no shutdown right. Authoring boot failures still clean up their newly spawned child; after successful publication the independent discovery record controls its lifetime. + +Classify by execution ownership, not by whether a filename contains "conversation": + +| Caller | Lifetime and handoff | +| --- | --- | +| Pipeline server | `run`, owned through coordinator release | +| Provider/model discovery, commit/branch naming | `helper`, stopped on operation success, failure, or timeout | +| Per-call session create/validate/status/command-list queries | `helper`; session creation alone does not start independent execution, and the durable session reference does not extend the helper's lifetime | +| Borrowed authoring service | No close right for the borrower; independent service owner remains authoritative | +| Proposal fallback before command invocation | `helper` for discovery, with unconditional cleanup on early return/error; before starting an authoring command it must be published/transferred to the repository's independent conversation service under its discovery lock, or reuse a service that won that race and close the unused child | + +If the proposal fallback cannot safely establish/reuse independent service ownership, refuse command invocation, stop the still-owned helper, and leave ordinary standalone conversation available. Update persisted lifetime and unregister creator-death cleanup as part of the transfer before any command execution; failed/ambiguous publication must not leave a helper record authorizing a later kill of an active authoring service. Successful authoring outlives the view under the existing conversation contract. No automatic transfer is inferred from external clients: attaching to a **run-owned** server is inspection of that run, not an independent lifetime lease; its owner stop/death can close that connection. + +Preserve existing launch semantics: loopback binding, allocated port, 30-second readiness timeout, strict readiness-line parsing, supplied config via `OPENCODE_CONFIG_CONTENT`, inherited cwd for SDK-style callers versus explicit checkout for conversation helpers, and current `HERDR_*` stripping behavior of `startOpencode`. Use explicit per-child env rather than widening a global env mutation window. Keep streaming fetch timeout behavior unchanged. Install spawn/error/exit/output listeners before waiting; bound diagnostic stdout/stderr tails while continuing to drain pipes. + +Register the live handle synchronously after spawn, capture birth identity, persist ownership before exposing readiness, and clean up if identity capture, record publication, URL parsing, or client construction fails. Handle already-aborted signals and readiness/abort races. No branch may lose its reference to the spawned child. + +**Alternative rejected:** wrapping SDK `close()` alone. The pinned factory does not expose the child handle or awaited exit, so it cannot implement the required guarantees. Do not infer PID from a port listener. + +### D2. Bounded, shared stop state machine + +Use one stop promise and explicit outcomes: + +``` +starting -> ready -> stopping (SIGTERM) -> stopped + | | + +-- failure --------+-- grace expired -> forcing (SIGKILL) + | | + stopped unresolved +``` + +Default server grace is 2 seconds, followed by at most 1 second for forced-exit observation. A forced request bypasses the remaining graceful interval and shares the existing promise. Tests inject timers and signal/exit seams. A returned `kill()` boolean is only signal delivery evidence, never successful stop. + +For a direct child, attach exit observation before signaling and reap through the child-process API. Already-exited children settle without another signal. Bound waiting for pipe closure separately from child exit: inherited pipes in unrelated surviving descendants must not keep server shutdown open forever. Dispose listeners, drainers, and timers when they no longer serve the state machine. Optional bookkeeping errors must not skip the signal/exit path. + +Target the owned child, not the coordinator's process group and not all children by name. No group kill is necessary for the direct-serve guarantee; adding one would create a false claim about descendants that detach themselves. + +**Alternative rejected:** SIGTERM-and-return or unconditional SIGKILL. The former never verifies resource release; the latter discards a useful graceful window for normal exits. + +An outer operation-timeout race is not sufficient cleanup ownership. In particular, preflight's timeout can return before its discovery coroutine's `finally` settles. Timeout paths must explicitly cancel the operation and await the owned stop outcome without waiting indefinitely for the original request promise. Standalone helper operation deadlines can be followed by at most the 3-second cleanup allowance; helpers running under a coordinator use its remaining global shutdown budget instead. Verify both successful and timeout-return paths rather than only replacing `close()` calls with `await close()`. + +### D3. Ownership follows the coordinator through final release + +Place the production coordinator's lifecycle/signal scope outside `run()`, spanning initialization through `result.release()`/error teardown. Direct programmatic runs retain equivalent local scope. Helper-only CLI operations use the shared owner registry while a managed helper is present; installing it must not compete with the coordinator's handler or turn normal view detachment into cancellation. Raw-mode UI abort and process SIGINT/SIGTERM/SIGHUP route to the same owner shutdown state. Do not forward parent-terminal SIGHUP into the deliberately detached coordinator. + +Concretely, pass the coordinator-owned shutdown context into hosted `run()` and skip its current local `installShutdownSignals`/`dispose` path in that mode. Hosted `run()` may unregister its execution callbacks when complete, but not the owner's process handlers or registry. Direct calls create and dispose a local owner scope; nested helpers register children with the already-active scope rather than installing another handler set. A single incoming process signal must increment exactly one shutdown request counter. + +Keep the current 15-second overall graceful shutdown ceiling. Reserve the last 3 seconds for server stop: session cancellation and optional cleanup cannot consume that reserve. A second abort transitions directly to forced server stop, observes for at most 1 second, then exits with the existing abort code. A deadline-triggered force path likewise attempts termination before exit. Repeated signals while forcing are idempotent, not new immediate `process.exit` calls. An unresponsive JS event loop remains outside the guarantee; hard-kill recovery is D5. + +Retain hosted release from both result and error paths, but guard it with a coordinator-level `finally`. During terminal hold, a signal resolves only that presentation wait and triggers release. During abort cleanup, failures/hangs in session cancellation, notifiers, permissions, metadata, or leases cannot prevent server termination. Keep bridge shutdown before terminating the server where possible, but do not let bridge or persistence failures bypass termination. Keep abort handling installed until owned stop attempts and bounded final cleanup settle. + +Do not make `serverStopped()` or writer-claim release proof of actual death. Confirm child exit before clearing live-server metadata/releasing execution ownership on the ordinary path. If stop remains unresolved, persist the unresolved process record before exiting where possible, and do not erase it while cleaning the workspace. Preserve existing writer-conflict reconciliation rules rather than inventing a claim takeover policy in this change. + +**Alternative rejected:** adding an asynchronous `process.on('exit')` handler. Exit handlers cannot await cleanup and cannot handle SIGKILL. The last-resort deadline must invoke the synchronous signaling edge before deciding to exit, not merely queue an async cleanup job. + +### D4. Finish holds subscribe to controller lease state + +Add a lease-state notification/timer owned by the control server. Refresh, claim, valid `/bye`, and expiry update this state; expiry runs even when no HTTP request arrives. Reuse the existing 15-second timeout and detect expiry within one additional second under normal scheduling. Dispose the timer with the server. + +`ControlProgress.runFinished` registers a finish-only waiter against current controller state and rechecks immediately after registration to close the check/subscribe race. Valid departure resolves it immediately. Silent expiry resolves it after checking that no replacement controller currently owns the slot. Expired credentials cannot resurrect their old lease without a new claim; a delayed old `/bye` or timer cannot dismiss a replacement controller. Explicit abort/dismiss remains idempotent. Observer requests neither hold nor dismiss the terminal screen. + +This notification does not resolve permission/human queues, abort paused/executing runs, or release writer claims. If no controller is present at terminal-hold entry, preserve immediate release. Connected inspection retains the live run server; once terminal cleanup has started, later viewers use the existing historical/stored-session fallback. + +**Alternative rejected:** a blanket no-client TTL for all runs/services. It would cancel legitimate background execution and violate conversation and writer-lifetime contracts. + +### D5. Durable transient records and conservative reconciliation + +Store private, versioned lifecycle records under `~/.convoy/processes/`, independent of `pending/` and disposable workspaces. These are transient execution evidence, not worktree ownership or a feature registry. Use atomic writes, private directory/file modes, a unique record ID, and a per-record exclusive reconciliation lock. Fields include lifetime, owner identity, child identity, run ID if available, URL when ready, state, and bounded last-stop outcome. Do not store credentials, prompts, full env, or config. + +Process identity comprises PID plus kernel-derived birth identity (including a boot discriminator), UID, and expected executable/serve role. Use an injected platform adapter: Linux `/proc` start ticks plus `/proc/sys/kernel/random/boot_id` and process/executable observations; macOS system `libproc` (Bun FFI) for process start seconds/microseconds and executable observations, paired with kernel boot time from `sysctl kern.boottime` as the boot discriminator. The macOS boot discriminator is not supplied by libproc itself. Load platform support lazily and verify standalone builds on both supported OSes. `Date.now()` at record creation or human-formatted `ps lstart` alone is not a birth identity. Unsupported platforms or failed probes return unknown and never become destructive targets. Startup cannot expose a new managed run/helper without publishing its required identity; failure uses the still-owned child handle for bounded cleanup. + +Create a provisional record before spawn, update it with child identity immediately after spawn and before readiness, and retain unresolved records on failure. There is an unavoidable interval between OS spawn and durable child identity publication; SIGKILL in that interval may leave an unattributable child. Do not invent a safe automatic kill for that interval. Record/report incomplete evidence and document the limitation rather than introduce a supervisor in this change. + +Before a managed run/helper boot, perform a bounded pass over these records (initial defaults: at most 32 inspected records and 5 seconds total). Maintain a fair cursor across passes; do not always revisit only the oldest uncertain records. Check remaining budget before starting a full stop attempt; preserve deferred entries. One failed or inaccessible record must not stop unrelated processing or fail a new launch merely because recovery could not classify an old record. + +Under the record's exclusive lock, reread and verify: + +1. Version and lifetime are eligible (`run`/`helper`). +2. The original owner incarnation is provably gone, not merely UI-less or slow; a different process at the old owner PID is not that original owner. Permission/probe errors mean uncertain. +3. The target is the recorded child incarnation and expected same-user executable/serve role. +4. Revalidate owner and child immediately before each destructive transition, including forced escalation. + +Only then apply the bounded stop algorithm. Independently proven ownership permits terminating a wedged server whose HTTP endpoint does not answer. A responding HTTP endpoint adds diagnostic context but is neither necessary nor sufficient kill authority. Identity mismatch receives no signal; preserve a bounded skipped/mismatch diagnostic. Already-gone children and confirmed stops can have their transient records removed. Locks and cursor updates must themselves be bounded; use fail-closed contention handling, not read-then-unlink reclamation of another live worker's lock. + +Normal stop updates outcome before removing records; unresolved records survive later pending/workspace sweeps. A normal success emits a compact lifecycle log line, while uncertain recovery emits a warning with record location and safe inspection guidance. Do not persist a log for every successful short-lived server indefinitely. Existing logs and process records must exclude tokens, full environment, and prompts. + +**Alternatives rejected:** kill by PPID/name/port; reinterpret legacy `server.pid`; require healthy HTTP before stopping a proven orphan; add a daemon for immediate owner-death detection. These respectively risk unrelated work, target the wrong PID, miss wedged orphans, or substantially expand deployment scope. + +### D6. Backward-compatible metadata and independent authoring boundary + +Keep `metadata.server.pid` as the historical coordinator/owner anchor. Add optional explicit child identity/process-record reference for new runs, or link it from the runtime record by run ID; do not change legacy reader semantics. Lifecycle authority comes from the new runtime record, not inferred run completion, a stale lease, or a port scan. History readers ignore missing optional identity fields as before. + +The authoring service continues to use its independent discovery record, not creator-death recovery. Adapt its handle types and explicit stop outcome where touched by D1/D2, but do not add automatic eviction or a new CLI stop policy. For newly booted authoring services, retain enough identity to avoid weakening existing guarded-stop intent. Legacy authoring records still support non-destructive discovery; insufficient identity must not authorize a new automatic stop. Publication failure must close the unpublished child rather than leaking it. + +**Alternative rejected:** converting all authoring services to creator-owned helpers. That would terminate independent execution merely because a view exits, contrary to `work-conversations`. + +### D7. Deterministic regression and subprocess verification + +Extend existing tests around `opencode`, `runner`, `runner-hosted`, `coordinate`, `control-server`, `attach-controller`, `coordinated-hold`, and conversation-service boundaries. Unit tests inject process probes, deadlines, clocks, persistence failures, and exit behavior; no test may call real `process.exit` in the test runner. + +Subprocess fixtures, invoked only during implementation/verification, exercise real OS boundaries: normal exit, ignored SIGTERM, malformed/no readiness output, failed client construction, abort/readiness race, owner force-exit, and orphan recovery on a subsequent launch. Test children must never load user OpenCode configuration, use model credentials, or match broad user-process scans. Record exact fixture PIDs/birth identities and clean them in test teardown even when assertions fail. Simulate identity mismatches and probe errors without signaling unrelated real processes. + +Add a no-client-requests expiry test, valid `/bye` during hold, replacement/observer/stale-controller cases, plus negative tests showing permission/human gates and active runs stay alive. Add repeated helper boot/stop cycles and assert no fixture children/records/listeners remain after confirmed shutdown; memory snapshots alone are not leak assertions. + +## Risks / Trade-offs + +- **PID reuse and probe/signal races** → Prefer direct child handles while the owner lives; use kernel birth identity and immediate revalidation for recovery. Portable POSIX PID signaling still has a residual TOCTOU window, especially on macOS; do not claim it is an atomic security boundary or use low-resolution identity fallbacks. +- **Platform identity support and compiled Bun builds** → Test Linux and macOS adapters and standalone release paths; unknown observations stay non-destructive. No native third-party addon is introduced. +- **SIGKILL/crash before publication or no future launch** → Recovery is eventual and limited to attributable records; immediate containment needs a future supervisor design. +- **Forced stop can interrupt writes** → Use bounded graceful session/server stop first; never compact or report a successful run merely because shutdown completed; preserve recovery/history evidence. +- **Raw-mode signals, helper timeouts, and owner handlers can overlap** → One owner shutdown state and idempotent stop; regression-test signal scopes rather than installing competing handlers per module. +- **Abandoned finish cleanup removes live inspection availability** → Keep the existing heartbeat lease, preserve connected controllers, and use existing stored-session reopening after release. Active/gated runs remain unaffected. +- **Descendant processes can outlive the server** → Explicitly out of the server-lifecycle guarantee; do not sell direct-child cleanup as complete MCP/LSP/tool containment. +- **Persistent authoring services still consume memory** → Intentional independent lifetime; visibility/explicit idle-stop policy is a separate product decision, not an automatic kill in this fix. +- **Uncertain records can persist** → Bound scanning and diagnostics, ensure fairness, remove proven stopped records, and retain uncertainty rather than deleting kill-safety evidence. + +## Migration Plan + +1. Implement process identity/record storage and owned launch/stop primitives with isolated tests, leaving existing metadata interpretation intact. +2. Migrate run/helper call sites and the coordinator's whole-lifetime shutdown scope; explicitly classify authoring-service and injected handles. +3. Add finish-only lease notifications and safe startup reconciliation; validate preservation of active/background/gated execution. +4. Document the automatic cleanup boundary, eventual recovery after owner death, legacy/incomplete-record exclusions, and how to inspect PID/PPID/state/birth evidence without broad kill commands. +5. Run targeted and full tests, typechecking, and builds on macOS/Linux before shipping. Optional smoke tests with an installed OpenCode binary require separate implementation-time execution and isolated state, not the user's active servers. + +Rollback is code-only: older versions ignore the new private process records and optional metadata. Do not delete unresolved records during rollback; they may be reconciled after re-upgrade. Legacy already-orphaned servers remain manual inspection cases because no trustworthy child identity was recorded at launch. diff --git a/openspec/changes/fix-opencode-server-lifecycle/proposal.md b/openspec/changes/fix-opencode-server-lifecycle/proposal.md new file mode 100644 index 0000000..ad41bcb --- /dev/null +++ b/openspec/changes/fix-opencode-server-lifecycle/proposal.md @@ -0,0 +1,31 @@ +## Why + +Convoy has code-confirmed paths that bypass shutdown of its OpenCode servers or retain a completed run after its controller disappears; the current close primitive sends a signal without confirming termination. Operators report accumulating memory-consuming `opencode serve` processes, so Convoy needs verifiable lifecycle ownership and safe recovery, without claiming that these mechanisms explain every historical report or confusing live orphans with OS zombies. + +## What Changes + +- Give run-owned and short-lived helper servers a shared, awaitable, idempotent stop operation with bounded SIGTERM grace, SIGKILL escalation, and an observed termination outcome; handle failed and interrupted startup as part of the same lifetime. +- Close proposal-discovery fallback helpers on all exits, and require an explicit transfer to independent service ownership before invoking an authoring command so recovery cannot mistake active authoring for an abandoned helper. +- Keep shutdown ownership and signal handling in effect through the coordinator's terminal hold and final release. Repeated aborts and the shutdown deadline force-stop owned servers before exiting rather than abandoning cleanup immediately. +- Release a completed/failed run's terminal hold when its controller explicitly leaves or its existing heartbeat lease expires. Do not apply this rule to active runs, permission gates, human gates, or authoring execution. +- Record the actual server process identity separately from the coordinator identity, outside disposable run workspaces. Reconcile attributable run/helper orphans during subsequent managed-server startup with bounded work, strict identity checks, and diagnostics for anything uncertain. +- Preserve lifecycle evidence until termination is observed; retain compatibility with existing run metadata and treat legacy records without child identity as ineligible for automatic killing. +- Preserve standalone OpenCode windows, independently persistent authoring services, and deliberate background execution. Do not add a global `pkill`, a general process-tree janitor, or an automatic authoring-service idle eviction policy. + +## Capabilities + +### New Capabilities + +- `opencode-server-lifecycle`: Ownership, bounded shutdown, terminal-hold release, and fail-closed orphan recovery for Convoy-managed OpenCode servers, with explicit exclusions for independent lifetimes. + +### Modified Capabilities + +None. Existing `work-conversations` lifetime independence, `session-transcripts` historical fallback, and run-finalization/history contracts remain unchanged; the new capability adds process-lifecycle guarantees around them. + +## Impact + +- Server launch and client construction in `src/opencode.ts`; run shutdown/release in `src/runner.ts` and `src/coordinate.ts`; controller lease/finish-hold handling in `src/control-server.ts`, `src/control-progress.ts`, and `src/attach.ts`. +- Short-lived callers in preflight, model catalog, commit-message generation, branch naming, and conversation adapters must await cleanup. Authoring discovery must explicitly retain its independent lifetime rather than inherit run-owned recovery policy. +- Additive runtime process records and lifecycle diagnostics under Convoy's user-state directory; existing coordinator PID semantics in run metadata remain backward compatible. +- SDK clients remain in use, but the pinned SDK's server factory lacks the process handle and confirmed-stop contract needed here. Launching must be owned by Convoy while preserving config, environment filtering, cwd, URL parsing, and boot-abort behavior. +- Regression and subprocess integration tests on macOS and Linux, plus lifecycle and recovery documentation. No new external daemon or dependency is planned; no application code is changed by this proposal. diff --git a/openspec/changes/fix-opencode-server-lifecycle/specs/opencode-server-lifecycle/spec.md b/openspec/changes/fix-opencode-server-lifecycle/specs/opencode-server-lifecycle/spec.md new file mode 100644 index 0000000..c3f77c0 --- /dev/null +++ b/openspec/changes/fix-opencode-server-lifecycle/specs/opencode-server-lifecycle/spec.md @@ -0,0 +1,141 @@ +## Purpose + +Prevent abandoned Convoy-owned OpenCode servers from accumulating while preserving deliberate background execution and independent conversations. Make shutdown and orphan recovery bounded, attributable, and truthful about uncertain process state. + +## ADDED Requirements + +### Requirement: Managed servers have explicit lifetime ownership + +Convoy SHALL distinguish run-owned servers, short-lived helper servers, and independently persistent authoring services. For newly launched run/helper servers it SHALL retain the actual child process identity, owner process identity, lifetime class, and lifecycle state independently of disposable run workspaces. A PID alone, process name, parent PID of 1, or answering network port SHALL NOT authorize termination. Existing coordinator identity fields SHALL retain their historical interpretation. + +#### Scenario: Run server and coordinator are different processes +- **WHEN** a coordinator launches a run server +- **THEN** lifecycle evidence distinguishes the two process incarnations and can identify the child after the coordinator exits without reinterpreting historical coordinator PID fields + +#### Scenario: Helper startup cannot establish recoverable ownership +- **WHEN** a helper child starts but its required ownership evidence cannot be persisted +- **THEN** Convoy refuses to expose the helper as ready, attempts bounded cleanup through its owned child handle, and reports any unresolved outcome + +#### Scenario: A temporary authoring helper would start independent execution +- **WHEN** a proposal fallback intends to invoke an authoring command on a temporary server +- **THEN** Convoy first transfers that server to verified independent service ownership or reuses such a service and stops the unused helper; if neither is safe it refuses command execution and cleans up the temporary server + +### Requirement: Managed shutdown observes termination within bounded waits + +Convoy SHALL attempt graceful termination of run/helper servers when their owning operation ends, escalate to forced termination after a finite grace period, and await an observed outcome within a finite total shutdown budget. A successful signal submission SHALL NOT be reported as confirmed termination. Concurrent or repeated stop requests SHALL share one lifecycle outcome. Confirmed disappearance SHALL be treated as already stopped; inability to confirm termination SHALL retain diagnostic evidence rather than wait forever or claim success. + +#### Scenario: Helper completes normally +- **WHEN** a short-lived provider, naming, or conversation helper completes its operation +- **THEN** its caller waits for bounded server cleanup before returning, without closing an injected independently owned service + +#### Scenario: Helper operation times out +- **WHEN** an outer timeout ends a helper operation while its request remains pending +- **THEN** the timeout path cancels the operation and waits for the bounded owned-server stop outcome before returning, without waiting indefinitely for the original request to settle + +#### Scenario: Server ignores graceful termination +- **WHEN** a managed server remains alive beyond the graceful-stop deadline +- **THEN** Convoy attempts forced termination, observes the result within the remaining budget, and reports whether it stopped or remains unresolved + +#### Scenario: Multiple callers request stop +- **WHEN** normal release and an interrupt request cleanup concurrently +- **THEN** cleanup is idempotent and neither caller repeats destructive effects against a later process incarnation + +### Requirement: Startup failures retain shutdown ownership + +Convoy SHALL apply the same bounded cleanup guarantees from child creation through readiness, including boot timeout, spawn error, early exit, malformed readiness output, cancellation, and failure to construct a client after server readiness. It SHALL drain child output without unbounded diagnostic accumulation and SHALL NOT hand out a successful server connection after cancellation has won. + +#### Scenario: Cancellation races with readiness +- **WHEN** an abort arrives while a child is starting or emitting its readiness URL +- **THEN** Convoy either exposes a live owned handle before cancellation takes effect or completes bounded cancellation cleanup; it never loses ownership of the spawned child + +#### Scenario: Startup fails after spawning +- **WHEN** a child never reports a valid readiness URL or client creation fails after readiness +- **THEN** Convoy reports startup failure only with a bounded cleanup outcome and retains evidence if the child could not be confirmed stopped + +### Requirement: Interrupt protection spans the complete owned-server lifetime + +For an explicit run abort, Convoy SHALL attempt bounded session cancellation followed by bounded server shutdown. Catchable process termination signals SHALL remain handled through boot, execution, terminal hold, and final release. A repeated abort or the shutdown deadline SHALL accelerate termination of owned run/helper servers before Convoy exits, without indefinitely waiting for session APIs, metadata writes, or optional cleanup. Uncatchable termination SHALL be handled by subsequent orphan reconciliation rather than a promise of in-process cleanup. + +#### Scenario: Session cancellation does not answer +- **WHEN** an explicitly aborted run's session-cancellation request hangs +- **THEN** Convoy proceeds to server termination within the shutdown budget instead of exiting while skipping that attempt + +#### Scenario: Second interrupt or shutdown deadline +- **WHEN** a second abort arrives or graceful shutdown exhausts its budget +- **THEN** Convoy enters bounded forced server cleanup before exiting with the abort outcome, retaining unresolved process evidence if termination cannot be verified + +#### Scenario: Signal during terminal hold or release +- **WHEN** the coordinator receives SIGTERM after execution finishes but before its owned server has stopped +- **THEN** it releases the terminal wait and performs bounded server cleanup rather than reverting to an unprotected immediate exit + +### Requirement: Completed-run terminal holds follow controller lifetime + +A completed or failed run awaiting only terminal-screen dismissal SHALL release that hold on valid controller departure or controller heartbeat expiry, and then perform normal run-owned server cleanup. Silent expiry SHALL be detected without requiring a new client request. A connected controller SHALL retain inspection access until dismissal, departure, expiry, or explicit abort. Controller replacement races SHALL NOT let an old controller's departure release a new live controller's hold. + +#### Scenario: Controller explicitly leaves a finish screen +- **WHEN** the valid controlling client releases its claim while a completed run is waiting for dismissal +- **THEN** the terminal hold resolves and the coordinator shuts down its run server + +#### Scenario: Controller disappears without goodbye +- **WHEN** the client dies during a terminal hold and sends no further requests +- **THEN** the coordinator detects expiry using the existing 15-second heartbeat lease, releases the hold within one additional second under normal scheduling, and begins bounded cleanup + +#### Scenario: Controller replacement wins before expiry handling +- **WHEN** a new live controller acquires the slot before an old lease's expiry callback runs +- **THEN** the callback revalidates current ownership and does not dismiss the new controller's finish screen + +#### Scenario: Departure and expiry coincide +- **WHEN** valid controller departure races with lease expiry for the same terminal hold +- **THEN** the hold resolves once and run-owned cleanup remains idempotent + +### Requirement: Background execution and independent services remain protected + +Controller departure or silence SHALL NOT abort an executing or paused run, answer/reject a permission or human gate, release an active writer claim, or terminate a required authoring service. Run/helper orphan recovery SHALL exclude independently persistent services and standalone OpenCode clients. Run history and stored-session reopening SHALL remain available after process cleanup under their existing contracts. + +#### Scenario: Terminal closes during active or waiting execution +- **WHEN** the controlling view disappears while a run is executing, paused, or waiting for a permission or human decision +- **THEN** execution and pending decisions retain their existing background/reattach behavior and are not classified as abandoned solely from client absence + +#### Scenario: Run cleanup coexists with authoring and standalone windows +- **WHEN** a run ends while independent authoring or standalone OpenCode clients exist +- **THEN** run cleanup leaves those services and clients untouched and does not invalidate durable session references + +### Requirement: Orphan reconciliation is attributable and bounded + +During subsequent managed-server startup, Convoy SHALL perform bounded reconciliation of its own recorded run/helper lifetimes. It SHALL terminate a recorded child only after independently confirming that its original owner incarnation is gone and that the target still matches its recorded child incarnation and expected executable role. It SHALL revalidate immediately before signaling, serialize concurrent attempts on the same record, and retain uncertain evidence. Failure of an HTTP probe SHALL NOT alone prevent cleanup when process ownership is independently verified, nor SHALL HTTP success substitute for that ownership. Recovery SHALL NOT discover kill targets by a global name, port, or PPID scan. + +#### Scenario: Owner was killed uncatchably +- **WHEN** a later managed startup finds a recorded run/helper server with a provably dead original owner and matching child identity +- **THEN** it performs bounded server termination even if the server's HTTP endpoint no longer responds + +#### Scenario: Owner is alive but no UI remains +- **WHEN** reconciliation finds the same live owner process for a detached run +- **THEN** it leaves the server untouched regardless of client count + +#### Scenario: PID reuse or incomplete evidence +- **WHEN** a PID now belongs to a different incarnation, the record is legacy/incomplete, or a required identity probe is unavailable +- **THEN** Convoy does not signal that PID and reports a skipped or uncertain recovery outcome rather than guessing ownership + +#### Scenario: Concurrent reconciliation +- **WHEN** two Convoy instances inspect the same orphan record +- **THEN** only one performs its destructive transition at a time and both respect refreshed identity and termination evidence + +#### Scenario: Many records or slow probes +- **WHEN** recovery encounters more records than its bounded startup work budget permits +- **THEN** it preserves unprocessed records for subsequent passes without blocking startup indefinitely or repeatedly starving the same records + +### Requirement: Cleanup evidence is truthful and privacy-conscious + +Lifecycle diagnostics SHALL distinguish requested stop, graceful exit, forced exit, already gone, identity mismatch, and unresolved termination. Unresolved recovery evidence SHALL survive ordinary pending-launch and workspace cleanup. Confirmed-stop records SHALL be eligible for removal so successful lifetimes do not accumulate forever. Diagnostics SHALL NOT persist authentication tokens, full environment/configuration contents, or prompt transcripts. Existing records without child identity SHALL remain readable but SHALL NOT silently become automatic-kill authority. + +#### Scenario: Termination cannot be confirmed +- **WHEN** the stop deadline expires without conclusive child-exit evidence +- **THEN** Convoy retains the relevant identity and bounded diagnostic reason and does not describe the process as successfully stopped + +#### Scenario: Pending directory is swept +- **WHEN** a dead coordinator's disposable launch directory is removed +- **THEN** unresolved child lifecycle evidence remains available independently and immutable run history is not deleted by process recovery + +#### Scenario: Legacy run is inspected +- **WHEN** historical metadata has only a coordinator PID and server URL +- **THEN** history remains readable, the legacy PID is not relabeled as the child, and automatic recovery does not target a process from that record diff --git a/openspec/changes/fix-opencode-server-lifecycle/tasks.md b/openspec/changes/fix-opencode-server-lifecycle/tasks.md new file mode 100644 index 0000000..511d3b8 --- /dev/null +++ b/openspec/changes/fix-opencode-server-lifecycle/tasks.md @@ -0,0 +1,42 @@ +## 1. Process identity and lifecycle evidence + +- [x] 1.1 Define lifetime classes, process-identity and stop-outcome types, and an injectable identity-probe interface (design D1/D5); verify unit tests reject PID-only/low-resolution identities and distinguish alive, gone, mismatch, and unknown observations. +- [x] 1.2 Implement the Linux birth/boot/UID/executable identity adapter (D5); verify fixture-process tests on Linux plus mocked PID reuse, reboot, missing `/proc`, and permission-denied cases produce the expected non-destructive outcomes. (`linuxProcessIdentity` reads start ticks + `boot_id` + uid + `exe`; guarded tests cover parsing, reuse, reboot, missing `/proc`, and permission-denied. Real-fixture coverage comes from the platform-agnostic subprocess suite through `defaultIdentityProbe()`. Linux-only execution is verified on ubuntu CI, not on this macOS host.) +- [x] 1.3 Implement the lazily loaded macOS kernel birth/UID/executable adapter via system libproc and kernel boot-time discriminator (D5); verify fixture-process tests on macOS and ensure unsupported or failed native/boot observations return unknown rather than authorizing a signal. +- [x] 1.4 Add versioned private lifecycle record storage under Convoy user state, with provisional/ready/stopping/unresolved states, atomic publication and record-specific locks (D5); verify tests cover concurrent writers, corrupt/legacy records, bounded lock contention, restrictive permissions, publication failure, and no credentials/config/prompts in persisted data. + +## 2. Owned launch and bounded shutdown + +- [x] 2.1 Introduce the shared child-owning launcher and adapt `startOpencode`/`bootOpencodeServerFrom` to it (D1); verify `test/opencode.test.ts` and new launcher tests preserve explicit/inherited cwd, loopback/readiness behavior, config injection, HERDR filtering, and client fetch semantics. +- [x] 2.2 Register child ownership at spawn and publish identity before readiness; implement bounded output draining and startup cleanup (D1); verify tests for missing executable, early exit, malformed/no readiness, pre-aborted signal, abort/readiness races, identity/record write failure, and client-construction failure leave no untracked exposed handle. +- [x] 2.3 Implement the shared idempotent async stop operation with 2-second TERM grace, forced escalation, and 1-second final observation (D2); verify fake-clock tests for normal exit, ignored TERM, concurrent stops, already-exited children, signal errors, unresolved outcomes, timer/listener disposal, and inherited pipes not delaying child-exit recognition indefinitely. +- [x] 2.4 Migrate preflight, model catalog, commit writer, branch naming, read-only conversation helpers, CLI bounded boots, and test doubles to await owned close; restructure outer timeout races to wait for bounded stop without awaiting a hung original request (D1/D2); verify success, early-return, rejection, and timeout tests prove cleanup settles before caller return within the stated operation-plus-cleanup budget and borrowers never close injected services. (All listed callers `await close()`; `withinPreflightTimeout` runs its cancel hook to completion before the timeout rejection settles, and `conversations.ts` only closes handles it booted. Cleanup-ownership tests in `test/preflight.test.ts`.) +- [x] 2.5 Explicitly classify independently persistent authoring boots and adapt publication/guarded-stop outcomes without adding creator-death eviction (D6); verify conversation-service tests preserve reuse, client/run lifetime independence, legacy non-destructive discovery, and cleanup of an unpublished child when discovery persistence fails. (Authoring boots declare `lifetime: "authoring-service"`; discovery-persistence failure closes the unpublished child, and the guarded stop re-verifies recorded `childBirth` before killing. Tests: `test/conversation-service.test.ts` reuse/lifetime-boundary/publication-failure/recycled-pid cases; no automatic eviction added.) +- [x] 2.6 Make proposal fallback ownership explicit: close discovery-only helpers on every early return, and publish/transfer to or reuse the independent repository service before command invocation (D1); verify tests for unknown/missing commands, discovery/publish failure, concurrent service publication, and active-authoring view closure prove no command runs under recoverable helper ownership and no independent service is reaped by creator-death policy. + +## 3. Coordinator-wide shutdown ownership + +- [x] 3.1 Establish one owner shutdown scope spanning coordinator boot, execution, terminal hold, and release; inject it into hosted `run()` instead of installing/disposing a second local handler set, while direct runs and helper-only CLI lifetimes retain local ownership (D3); verify each SIGINT/SIGTERM/SIGHUP increments exactly one request counter, nested helpers add no duplicate handlers, and parent view detachment does not stop a detached coordinator. +- [x] 3.2 Bound session cancellation and optional cleanup within the existing 15-second shutdown ceiling, reserving server-stop time; replace repeated-abort/deadline immediate exits with bounded forced server-stop transitions (D3); verify injected-exit/fake-clock tests assert signaling before exit, no real test-runner exit, and no deadline extension from repeated interrupts. +- [x] 3.3 Make hosted result/error release idempotent and guaranteed through coordinator cleanup, with a terminal-hold signal path (D3); verify `test/runner-hosted.test.ts`, `test/coordinate.test.ts`, and extended signal-during-finally/finish/release tests close each owned server despite errors or timeouts in optional cleanup. +- [x] 3.4 Preserve historical coordinator PID semantics and link new run records to explicit child evidence; clear live-server metadata and normal execution ownership only after confirmed stop, retaining unresolved evidence outside disposable workspaces (D3/D6); verify metadata/history compatibility tests, stop-order assertions, writer-claim non-regressions, and pending/workspace sweep survival tests. + +## 4. Controller-aware terminal holds + +- [x] 4.1 Add control-server lease notifications and autonomous expiry scheduling using the existing 15-second lease (D4); verify `test/control-server.test.ts` covers no further client requests, valid versus stale `/bye`, expired credentials, replacement claims, observer isolation, and timer disposal. +- [x] 4.2 Bind only terminal finish holds to lease loss with a registration/recheck race guard and idempotent abort/dismiss resolution (D4); verify `test/coordinated-hold.test.ts` and `test/attach-controller.test.ts` cover normal dismissal, departure while held, concurrent valid goodbye/expiry resolving once, silent expiry within lease plus one second, and replacement winning before old expiry handling. +- [x] 4.3 Preserve active, paused, permission, human-gate, background, and independent authoring behavior (D4/D6); verify negative tests leave each execution/decision pending across controller departure and verify historical/stored-session fallback remains available after completed-run cleanup. + +## 5. Safe orphan reconciliation and diagnostics + +- [x] 5.1 Implement record-driven reconciliation under per-record locks, verifying original owner death and child incarnation/UID/executable role immediately before TERM and KILL (D5); verify unit tests never signal live owners' children, reused PIDs, unknown probes, legacy/incomplete records, authoring services, or standalone clients, but can stop a verified orphan with an unreachable HTTP endpoint. +- [x] 5.2 Invoke bounded reconciliation before managed run/helper startup with the 32-record/5-second initial budgets and a fair continuation cursor (D5); verify tests cover slow probes, concurrent passes, lock contention, deferred candidates, no starvation, and old-record failures not failing unrelated startup. +- [x] 5.3 Persist and emit bounded lifecycle outcomes without secrets, removing confirmed-stop records and retaining uncertainty independently of pending cleanup (D5/D6); verify outcome/redaction tests and repeated successful helper cycles leave no growing unresolved registry or permanent success-record backlog. + +## 6. End-to-end verification and documentation + +- [x] 6.1 Add isolated subprocess fixtures for normal/ignored-TERM shutdown, helper boot failure, repeated abort/deadline, and signals during terminal hold/release (D7); verify real child exit and fixture-only cleanup on macOS and Linux without reading user OpenCode state, invoking models, or signaling broad process matches. +- [x] 6.2 Add a hard-owner-death fixture followed by a fresh reconciliation pass and negative identity/independent-lifetime fixtures (D5/D7); verify recorded orphans stop, protected fixtures survive, publication-gap records remain non-destructive, and all fixture processes are removed by test teardown even after assertion failures. +- [x] 6.3 Update `docs/running.md` and `docs/development.md` with stop semantics, finish-only expiry, eventual recovery, legacy/incomplete exclusions, diagnostic locations, and the descendant-containment limitation; verify documentation matches the capability scenarios and contains no blanket process-kill recommendation. +- [x] 6.4 Run the existing targeted lifecycle suites and every new process-identity/managed-server/reconciliation suite; then run `bun run typecheck`, `bun test`, and `bun run build` on macOS and Linux, including standalone identity-probe smoke coverage; record exact commands/results and confirm no fixture children remain. Use skips only for genuinely unavailable OS execution and report them explicitly rather than claiming cross-platform verification. (macOS: `bun test test/process-lifecycle.test.ts test/process-teardown.test.ts test/preflight.test.ts test/conversation-service.test.ts test/opencode.test.ts` → 187 pass / 0 fail; `bun run typecheck` → clean; `bun run test:coverage` → 3389 pass / 0 fail, 92.43% lines / 92.54% funcs; `bun run build` → `convoy (darwin-arm64) v0.9.0-local+3d82f51`; standalone smoke passed both via `bun run scripts/identity-smoke.ts` and compiled (`bun build scripts/identity-smoke.ts --compile`), proving libproc/FFI loads in a bundled binary. No fixture children remained. **Linux execution was skipped: this host is macOS.** The linux-only suites and ubuntu CI leg remain unverified locally; `test/process-lifecycle.test.ts` guards them with `process.platform !== "linux"` early returns.) +- [x] 6.5 Run `openspec validate fix-opencode-server-lifecycle --strict` and review the implementation diff against every lifecycle scenario; verify the change neither modifies independent authoring/background contracts nor enables killing by name, PPID, port alone, or legacy coordinator PID. diff --git a/openspec/specs/opencode-server-lifecycle/spec.md b/openspec/specs/opencode-server-lifecycle/spec.md new file mode 100644 index 0000000..d5a1c6a --- /dev/null +++ b/openspec/specs/opencode-server-lifecycle/spec.md @@ -0,0 +1,169 @@ +# opencode-server-lifecycle Specification + +## Purpose + +Prevent abandoned Convoy-owned OpenCode servers from accumulating while preserving deliberate background execution and independent conversations. Make shutdown and orphan recovery bounded, attributable, and truthful about uncertain process state. + +## Requirements + +### Requirement: Managed servers have explicit lifetime ownership + +Convoy SHALL distinguish run-owned servers, short-lived helper servers, and independently persistent authoring services. For newly launched run/helper servers it SHALL retain the actual child process identity, owner process identity, lifetime class, and lifecycle state independently of disposable run workspaces. A PID alone, process name, parent PID of 1, or answering network port SHALL NOT authorize termination. Existing coordinator identity fields SHALL retain their historical interpretation. + +#### Scenario: Run server and coordinator are different processes + +- **WHEN** a coordinator launches a run server +- **THEN** lifecycle evidence distinguishes the two process incarnations and can identify the child after the coordinator exits without reinterpreting historical coordinator PID fields + +#### Scenario: Helper startup cannot establish recoverable ownership + +- **WHEN** a helper child starts but its required ownership evidence cannot be persisted +- **THEN** Convoy refuses to expose the helper as ready, attempts bounded cleanup through its owned child handle, and reports any unresolved outcome + +#### Scenario: A temporary authoring helper would start independent execution + +- **WHEN** a proposal fallback intends to invoke an authoring command on a temporary server +- **THEN** Convoy first transfers that server to verified independent service ownership or reuses such a service and stops the unused helper; if neither is safe it refuses command execution and cleans up the temporary server + +### Requirement: Managed shutdown observes termination within bounded waits + +Convoy SHALL attempt graceful termination of run/helper servers when their owning operation ends, escalate to forced termination after a finite grace period, and await an observed outcome within a finite total shutdown budget. A successful signal submission SHALL NOT be reported as confirmed termination. Concurrent or repeated stop requests SHALL share one lifecycle outcome. Confirmed disappearance SHALL be treated as already stopped; inability to confirm termination SHALL retain diagnostic evidence rather than wait forever or claim success. + +#### Scenario: Helper completes normally + +- **WHEN** a short-lived provider, naming, or conversation helper completes its operation +- **THEN** its caller waits for bounded server cleanup before returning, without closing an injected independently owned service + +#### Scenario: Helper operation times out + +- **WHEN** an outer timeout ends a helper operation while its request remains pending +- **THEN** the timeout path cancels the operation and waits for the bounded owned-server stop outcome before returning, without waiting indefinitely for the original request to settle + +#### Scenario: Server ignores graceful termination + +- **WHEN** a managed server remains alive beyond the graceful-stop deadline +- **THEN** Convoy attempts forced termination, observes the result within the remaining budget, and reports whether it stopped or remains unresolved + +#### Scenario: Multiple callers request stop + +- **WHEN** normal release and an interrupt request cleanup concurrently +- **THEN** cleanup is idempotent and neither caller repeats destructive effects against a later process incarnation + +### Requirement: Startup failures retain shutdown ownership + +Convoy SHALL apply the same bounded cleanup guarantees from child creation through readiness, including boot timeout, spawn error, early exit, malformed readiness output, cancellation, and failure to construct a client after server readiness. It SHALL drain child output without unbounded diagnostic accumulation and SHALL NOT hand out a successful server connection after cancellation has won. + +#### Scenario: Cancellation races with readiness + +- **WHEN** an abort arrives while a child is starting or emitting its readiness URL +- **THEN** Convoy either exposes a live owned handle before cancellation takes effect or completes bounded cancellation cleanup; it never loses ownership of the spawned child + +#### Scenario: Startup fails after spawning + +- **WHEN** a child never reports a valid readiness URL or client creation fails after readiness +- **THEN** Convoy reports startup failure only with a bounded cleanup outcome and retains evidence if the child could not be confirmed stopped + +### Requirement: Interrupt protection spans the complete owned-server lifetime + +For an explicit run abort, Convoy SHALL attempt bounded session cancellation followed by bounded server shutdown. Catchable process termination signals SHALL remain handled through boot, execution, terminal hold, and final release. A repeated abort or the shutdown deadline SHALL accelerate termination of owned run/helper servers before Convoy exits, without indefinitely waiting for session APIs, metadata writes, or optional cleanup. Uncatchable termination SHALL be handled by subsequent orphan reconciliation rather than a promise of in-process cleanup. + +#### Scenario: Session cancellation does not answer + +- **WHEN** an explicitly aborted run's session-cancellation request hangs +- **THEN** Convoy proceeds to server termination within the shutdown budget instead of exiting while skipping that attempt + +#### Scenario: Second interrupt or shutdown deadline + +- **WHEN** a second abort arrives or graceful shutdown exhausts its budget +- **THEN** Convoy enters bounded forced server cleanup before exiting with the abort outcome, retaining unresolved process evidence if termination cannot be verified + +#### Scenario: Signal during terminal hold or release + +- **WHEN** the coordinator receives SIGTERM after execution finishes but before its owned server has stopped +- **THEN** it releases the terminal wait and performs bounded server cleanup rather than reverting to an unprotected immediate exit + +### Requirement: Completed-run terminal holds follow controller lifetime + +A completed or failed run awaiting only terminal-screen dismissal SHALL release that hold on valid controller departure or controller heartbeat expiry, and then perform normal run-owned server cleanup. Silent expiry SHALL be detected without requiring a new client request. A connected controller SHALL retain inspection access until dismissal, departure, expiry, or explicit abort. Controller replacement races SHALL NOT let an old controller's departure release a new live controller's hold. + +#### Scenario: Controller explicitly leaves a finish screen + +- **WHEN** the valid controlling client releases its claim while a completed run is waiting for dismissal +- **THEN** the terminal hold resolves and the coordinator shuts down its run server + +#### Scenario: Controller disappears without goodbye + +- **WHEN** the client dies during a terminal hold and sends no further requests +- **THEN** the coordinator detects expiry using the existing 15-second heartbeat lease, releases the hold within one additional second under normal scheduling, and begins bounded cleanup + +#### Scenario: Controller replacement wins before expiry handling + +- **WHEN** a new live controller acquires the slot before an old lease's expiry callback runs +- **THEN** the callback revalidates current ownership and does not dismiss the new controller's finish screen + +#### Scenario: Departure and expiry coincide + +- **WHEN** valid controller departure races with lease expiry for the same terminal hold +- **THEN** the hold resolves once and run-owned cleanup remains idempotent + +### Requirement: Background execution and independent services remain protected + +Controller departure or silence SHALL NOT abort an executing or paused run, answer/reject a permission or human gate, release an active writer claim, or terminate a required authoring service. Run/helper orphan recovery SHALL exclude independently persistent services and standalone OpenCode clients. Run history and stored-session reopening SHALL remain available after process cleanup under their existing contracts. + +#### Scenario: Terminal closes during active or waiting execution + +- **WHEN** the controlling view disappears while a run is executing, paused, or waiting for a permission or human decision +- **THEN** execution and pending decisions retain their existing background/reattach behavior and are not classified as abandoned solely from client absence + +#### Scenario: Run cleanup coexists with authoring and standalone windows + +- **WHEN** a run ends while independent authoring or standalone OpenCode clients exist +- **THEN** run cleanup leaves those services and clients untouched and does not invalidate durable session references + +### Requirement: Orphan reconciliation is attributable and bounded + +During subsequent managed-server startup, Convoy SHALL perform bounded reconciliation of its own recorded run/helper lifetimes. It SHALL terminate a recorded child only after independently confirming that its original owner incarnation is gone and that the target still matches its recorded child incarnation and expected executable role. It SHALL revalidate immediately before signaling, serialize concurrent attempts on the same record, and retain uncertain evidence. Failure of an HTTP probe SHALL NOT alone prevent cleanup when process ownership is independently verified, nor SHALL HTTP success substitute for that ownership. Recovery SHALL NOT discover kill targets by a global name, port, or PPID scan. + +#### Scenario: Owner was killed uncatchably + +- **WHEN** a later managed startup finds a recorded run/helper server with a provably dead original owner and matching child identity +- **THEN** it performs bounded server termination even if the server's HTTP endpoint no longer responds + +#### Scenario: Owner is alive but no UI remains + +- **WHEN** reconciliation finds the same live owner process for a detached run +- **THEN** it leaves the server untouched regardless of client count + +#### Scenario: PID reuse or incomplete evidence + +- **WHEN** a PID now belongs to a different incarnation, the record is legacy/incomplete, or a required identity probe is unavailable +- **THEN** Convoy does not signal that PID and reports a skipped or uncertain recovery outcome rather than guessing ownership + +#### Scenario: Concurrent reconciliation + +- **WHEN** two Convoy instances inspect the same orphan record +- **THEN** only one performs its destructive transition at a time and both respect refreshed identity and termination evidence + +#### Scenario: Many records or slow probes + +- **WHEN** recovery encounters more records than its bounded startup work budget permits +- **THEN** it preserves unprocessed records for subsequent passes without blocking startup indefinitely or repeatedly starving the same records + +### Requirement: Cleanup evidence is truthful and privacy-conscious + +Lifecycle diagnostics SHALL distinguish requested stop, graceful exit, forced exit, already gone, identity mismatch, and unresolved termination. Unresolved recovery evidence SHALL survive ordinary pending-launch and workspace cleanup. Confirmed-stop records SHALL be eligible for removal so successful lifetimes do not accumulate forever. Diagnostics SHALL NOT persist authentication tokens, full environment/configuration contents, or prompt transcripts. Existing records without child identity SHALL remain readable but SHALL NOT silently become automatic-kill authority. + +#### Scenario: Termination cannot be confirmed + +- **WHEN** the stop deadline expires without conclusive child-exit evidence +- **THEN** Convoy retains the relevant identity and bounded diagnostic reason and does not describe the process as successfully stopped + +#### Scenario: Pending directory is swept + +- **WHEN** a dead coordinator's disposable launch directory is removed +- **THEN** unresolved child lifecycle evidence remains available independently and immutable run history is not deleted by process recovery + +#### Scenario: Legacy run is inspected + +- **WHEN** historical metadata has only a coordinator PID and server URL +- **THEN** history remains readable, the legacy PID is not relabeled as the child, and automatic recovery does not target a process from that record diff --git a/scripts/identity-smoke.ts b/scripts/identity-smoke.ts new file mode 100644 index 0000000..83535c2 --- /dev/null +++ b/scripts/identity-smoke.ts @@ -0,0 +1,72 @@ +/** + * Standalone smoke test for the process-identity probe (change + * `fix-opencode-server-lifecycle`, design D5/D7). + * + * bun run scripts/identity-smoke.ts + * # or, to exercise the *compiled-binary* FFI path (macOS libproc via + * # bun:ffi, Linux /proc) rather than the dev runtime: + * bun build scripts/identity-smoke.ts --compile --outfile /tmp/identity-smoke && /tmp/identity-smoke + * + * The managed-server tests prove the probe works inside the test runtime; this + * proves the same adapter still loads and answers when Convoy is bundled into a + * standalone executable, which is how releases ship. + * + * It spawns only a throwaway `sleep` child — never OpenCode — and touches no + * user configuration or `CONVOY_HOME` state. Exit code 0 means every probe + * behaved; 1 means at least one did not. + */ +import { spawn } from "node:child_process" + +import { captureIdentity, defaultIdentityProbe, identityMismatchReason, sameIdentity } from "../src/process-identity" + +const failures: string[] = [] +const check = (ok: boolean, message: string): void => { + process.stdout.write(`${ok ? "ok " : "FAIL"} ${message}\n`) + if (!ok) failures.push(message) +} + +const probe = defaultIdentityProbe() +process.stdout.write(`platform: ${process.platform}\n`) + +// 1. The probe must answer for a process that certainly exists: this one. +const self = await probe.observe(process.pid) +check(self.status === "alive", `self probe reports alive (got ${self.status})`) +if (self.status === "alive") { + check(self.identity.pid === process.pid, "self identity names this pid") + check(self.identity.birth.includes(":"), `self birth carries a boot discriminator (${self.identity.birth})`) + check(self.identity.executable.length > 0, `self executable observed (${self.identity.executable})`) +} else if (self.status !== "gone") { + process.stderr.write(` reason: ${self.reason}\n`) +} + +// 2. A real child fixture, the same shape a managed server has. +const child = spawn("sleep", ["30"], { stdio: "ignore" }) +const childPid = child.pid ?? 0 +check(childPid > 0, "spawned a fixture child") +const captured = childPid > 0 ? await captureIdentity(childPid, probe) : undefined +check(Boolean(captured), "captured the fixture child's identity") +if (captured) { + const observed = await probe.observe(captured.pid) + check( + observed.status === "alive" && sameIdentity(captured, observed.identity), + "fixture identity re-observes identically (pid + birth + uid)", + ) + + // 3. A reboot changes the boot discriminator, so the old incarnation can + // never match — the reason recovery must refuse to signal it. + const startTicks = captured.birth.slice(captured.birth.lastIndexOf(":") + 1) + const afterReboot = { ...captured, birth: `rebooted-boot:${startTicks}` } + check(!sameIdentity(captured, afterReboot), "a reboot never matches the recorded incarnation") + check(Boolean(identityMismatchReason(captured, afterReboot)), "the reboot mismatch explains itself") +} + +// Teardown: stop the fixture and observe it gone. Never leave it behind. +if (childPid > 0) { + child.kill("SIGTERM") + await new Promise((resolve) => child.once("exit", () => resolve())) + const gone = await probe.observe(childPid) + check(gone.status === "gone", `fixture child is gone after teardown (got ${gone.status})`) +} + +process.stdout.write(failures.length === 0 ? "\nidentity smoke: PASS\n" : `\nidentity smoke: FAIL (${failures.length})\n`) +process.exit(failures.length === 0 ? 0 : 1) diff --git a/src/cli.ts b/src/cli.ts index e2e907d..7a78b6a 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1621,106 +1621,97 @@ async function openCheckoutConversationExternal(input: { launchDir: string; chec async function proposeInCheckout(input: { launchDir: string; route: TuiRoute; checkout: string; branch: string; displayName: string }): Promise { const { bootOpencodeServerFrom } = await import("./opencode") const { createAuthoringConversation, listAuthoringCommands, invokeAuthoringCommand, openConversationForeground } = await import("./conversations") + const { startProposalCommand } = await import("./propose-service") const serverResolution = await resolveAuthoringServer({ launchDir: input.launchDir, checkout: input.checkout, route: input.route }) if (serverResolution.status === "blocked") return - let serviceHandle: { url: string; close?(): void } | undefined - let boundedClose: (() => void) | undefined + let serviceHandle: { url: string } | undefined + let helper: { url: string; close?(): void | Promise } | undefined if (serverResolution.status === "service") { + // An injected service URL conveys no shutdown right: the independent + // service owner remains authoritative (design D1). serviceHandle = { url: serverResolution.url } } else { const booted = await bootOpencodeServerFrom(input.checkout).catch(() => undefined) if (booted) { serviceHandle = booted - boundedClose = () => booted.close() - } - } - - // Command discovery through the supported API, before any session exists: - // an absent workflow disables the action instead of imitating success. - let commandName: string | undefined - if (serviceHandle) { - const commands = await listAuthoringCommands({ checkout: input.checkout, server: serviceHandle }) - if (commands === "unknown") { - await reportHandoffBlocker("the project's authoring commands could not be discovered", ["check the project's .opencode/commands/ directory — Convoy does not install commands into it"], input.route) - return + helper = booted } - commandName = commands.find((name) => name === "opsx-propose") ?? commands.find((name) => name.endsWith("propose")) } - if (!commandName) { + if (!serviceHandle) { await reportHandoffBlocker( - "this project has no supported proposal workflow command (looked for opsx-propose under .opencode/commands/)", - ["author the change manually in a conversation"], + "the authoring workflow could not start: no server could be established for this checkout", + ["open an ordinary conversation in the worktree instead"], input.route, ) - boundedClose?.() return } // The writer claim precedes any writer work (capability work-conversations: // a conflicting managed writer is refused before a second writer starts). const { repoCommonDir } = await import("./repo-store") - const { acquireWriterClaim, writerConflictGuidance } = await import("./writer-claims") + const { acquireWriterClaim, writerConflictGuidance, releaseWriterClaim } = await import("./writer-claims") const commonDir = await repoCommonDir(input.launchDir).catch(() => undefined) let claimed = false - if (commonDir) { - const acquired = await acquireWriterClaim({ - commonDir, - branch: input.branch, - checkoutPath: input.checkout, - kind: "authoring", - // The claim is taken before any session exists, under the "convoy" - // pre-session owner: the failure-path release and the post-session - // re-own (`reconcileOwner: "convoy"`) match this owner, so a failed - // propose always releases what it claimed. - owner: "convoy", - }) - if (acquired.status === "acquired") { - claimed = true - } else { + + // Discovery, ownership transfer, and command invocation live in one tested + // helper (design D1/D6): the fallback helper is stopped on every early + // return, and the independent service is established strictly before the + // command runs, so no command ever executes under recoverable helper + // ownership. + const outcome = await startProposalCommand(helper ? { kind: "helper", url: serviceHandle.url } : { kind: "independent", url: serviceHandle.url }, { + listCommands: (server) => listAuthoringCommands({ checkout: input.checkout, server }), + transfer: async () => { + if (!commonDir) return { status: "unavailable" as const, reason: "this checkout has no repository storage for an independent authoring service" } + const { ensureConversationService } = await import("./conversation-service") + return await ensureConversationService({ commonDir, checkout: input.checkout }).catch((error: unknown) => ({ + status: "unavailable" as const, + reason: error instanceof Error ? error.message : String(error), + })) + }, + stopHelper: () => Promise.resolve(helper?.close?.()), + // The claim is taken before any session exists, under the "convoy" + // pre-session owner: the failure-path release and the post-session + // re-own (`reconcileOwner: "convoy"`) match this owner, so a failed + // propose always releases what it claimed. + acquireClaim: async () => { + if (!commonDir) return { ok: true as const } + const acquired = await acquireWriterClaim({ commonDir, branch: input.branch, checkoutPath: input.checkout, kind: "authoring", owner: "convoy" }) + if (acquired.status === "acquired") { + claimed = true + return { ok: true as const } + } const guidance = acquired.status === "conflict" ? writerConflictGuidance(acquired.existing) : ["a writer claim for this checkout is in an uncertain state — reconcile it before starting another writer"] - await reportHandoffBlocker(guidance[0], guidance.slice(1), input.route) - boundedClose?.() - return - } - } + return { ok: false as const, reason: guidance[0]!, remediation: guidance.slice(1) } + }, + // Release by pid: the re-own to the session id only runs after success, + // so on a failure path the claim is still this process's (a mismatched + // owner release is a no-op and would wedge the checkout behind a claim). + releaseClaim: async () => { + if (claimed && commonDir) await releaseWriterClaim({ commonDir, branch: input.branch, ownerPid: process.pid }).catch(() => {}) + }, + createConversation: (server) => createAuthoringConversation({ checkout: input.checkout, title: input.displayName, server }), + invokeCommand: ({ ref, server, command }) => invokeAuthoringCommand({ ref, server, command }), + }) - let ref: { harness: "opencode"; sessionId: string } | undefined - try { - if (!serviceHandle) throw new Error("no authoring server is available") - ref = await createAuthoringConversation({ checkout: input.checkout, title: input.displayName, server: serviceHandle }) - await invokeAuthoringCommand({ ref, server: serviceHandle, command: commandName }) - } catch (error) { - await reportHandoffBlocker( - `the authoring workflow could not start: ${error instanceof Error ? error.message : String(error)}`, - ["open an ordinary conversation in the worktree instead"], - input.route, - ) - boundedClose?.() - if (claimed && commonDir) { - const { releaseWriterClaim } = await import("./writer-claims") - // The re-own to the session id only runs after success, so on this - // path the claim is still this process's: release by pid, which - // matches regardless of the owner string (a mismatched-owner release - // is a no-op and would wedge the checkout behind an authoring claim). - await releaseWriterClaim({ commonDir, branch: input.branch, ownerPid: process.pid }).catch(() => {}) - } + if (outcome.status === "blocked") { + await reportHandoffBlocker(outcome.reason, outcome.remediation, input.route) return } - if (!ref) return + // Re-own the claim by the session id so the idle release and conflict // guidance keep naming the actual writer. if (claimed && commonDir) { const { acquireWriterClaim } = await import("./writer-claims") - await acquireWriterClaim({ commonDir, branch: input.branch, checkoutPath: input.checkout, kind: "authoring", owner: ref.sessionId, reconcileOwner: "convoy" }).catch(() => {}) + await acquireWriterClaim({ commonDir, branch: input.branch, checkoutPath: input.checkout, kind: "authoring", owner: outcome.ref.sessionId, reconcileOwner: "convoy" }).catch(() => {}) } const renderer = input.route.session.renderer - const exitCode = await openConversationForeground({ checkout: input.checkout, ref, suspend: () => renderer.suspend(), resume: () => renderer.resume() }) - await releaseAuthoringWriterIfIdle({ launchDir: input.launchDir, checkout: input.checkout, branch: input.branch, sessionId: ref.sessionId }) + const exitCode = await openConversationForeground({ checkout: input.checkout, ref: outcome.ref, suspend: () => renderer.suspend(), resume: () => renderer.resume() }) + await releaseAuthoringWriterIfIdle({ launchDir: input.launchDir, checkout: input.checkout, branch: input.branch, sessionId: outcome.ref.sessionId }) if (exitCode !== 0) { await reportHandoffBlocker(`the authoring client exited with code ${exitCode}`, ["reopen the worktree to continue"], input.route) } diff --git a/src/commit-message.ts b/src/commit-message.ts index 7aea22d..2c15a38 100644 --- a/src/commit-message.ts +++ b/src/commit-message.ts @@ -3,6 +3,7 @@ import type { AgentConfig, Config, OpencodeClient } from "@opencode-ai/sdk/v2" import { capSubjectWithin, firstMeaningfulLine, maxCommitSubjectLength } from "./commit-text" import { log } from "./log" import { startOpencode } from "./opencode" +import type { StopPolicy } from "./process-stop" import { splitModelVariant } from "./pipeline" import { parseModel } from "./runner" import { excerpt } from "./worktree" @@ -41,6 +42,12 @@ export type CommitMessageInput = { /** Override the model that writes the message (provider/model[#variant]). */ model?: string signal?: AbortSignal + /** + * Shared cleanup budget for the writer's owned helper (design D2). The + * coordinator passes a resolver drawing from its remaining shutdown deadline + * so this helper's stop cannot restart or exceed that budget. + */ + stopPolicy?: () => StopPolicy } export type CommitMessageProposal = { @@ -122,14 +129,20 @@ export async function proposeCommitMessage( ): Promise { let error: string | undefined try { - const handle = await deps.startOpencode(writerOpencodeConfig(), AbortSignal.timeout(commitMessageTimeoutMs)) + const handle = await deps.startOpencode( + writerOpencodeConfig(), + AbortSignal.timeout(commitMessageTimeoutMs), + // A helper under a coordinator gets its shared budget resolver so its + // bounded stop draws from the coordinator's remaining deadline. + input.stopPolicy ? { stopPolicy: input.stopPolicy } : undefined, + ) try { const reply = await askForCommitMessage(handle.client, { ...input, model: input.model ?? defaultCommitMessageModel }) const message = readCommitMessage(reply) if (message) return { message, source: "model" } error = `the commit writer's reply had no usable message: ${truncate(reply, 160)}` } finally { - handle.close() + await handle.close() } } catch (cause) { error = cause instanceof Error ? cause.message : String(cause) diff --git a/src/control-progress.ts b/src/control-progress.ts index e39ddea..73a4436 100644 --- a/src/control-progress.ts +++ b/src/control-progress.ts @@ -112,12 +112,44 @@ export class ControlProgress implements ProgressUI { async runFinished(outcome: RunOutcome): Promise { if (!this.server.hasController()) return - await this.server.pending.holdFinish({ + const dismissed = this.server.pending.holdFinish({ status: outcome.status, ...(outcome.error !== undefined ? { error: outcome.error } : {}), ...(outcome.goalLoop ? { goalLoop: outcome.goalLoop } : {}), ...(outcome.finalization ? { finalization: outcome.finalization } : {}), }) + await this.waitForDismissalOrLeaseLoss(dismissed) + } + + /** + * A completed run's terminal hold follows controller lifetime (design D4): + * an explicit departure or a silent heartbeat expiry resolves it, while a + * replacement that has already claimed the slot is never dismissed by the + * old lease's timer or a delayed `/bye`. The subscription is registered + * before the immediate recheck so a departure in the gap is not missed. + */ + private waitForDismissalOrLeaseLoss(dismissed: Promise): Promise { + return new Promise((resolve) => { + let settled = false + const finish = () => { + if (settled) return + settled = true + unsubscribe() + // Clear the hold so a later viewer does not inherit a dismissed + // finish screen; a no-op when the hold already resolved. + this.server.pending.resolveFinish() + resolve() + } + const unsubscribe = this.server.onControllerLease((event) => { + if (event === "claimed") return + if (!this.server.hasController()) finish() + }) + if (!this.server.hasController()) { + finish() + return + } + void dismissed.then(finish) + }) } keepRunDirRequested(): boolean { diff --git a/src/control-server.ts b/src/control-server.ts index 46ca761..28e3103 100644 --- a/src/control-server.ts +++ b/src/control-server.ts @@ -261,6 +261,13 @@ export type ControlServer = { * only by its own /bye, and expired by silence. */ hasController(): boolean + /** + * Subscribes to controller-lease transitions. Expiry is emitted by the + * server's own timer even when no HTTP request ever arrives again, so a + * terminal hold can follow a silent controller death (design D4). Returns + * an unsubscribe function; disposed with the server. + */ + onControllerLease(listener: (event: ControllerLeaseEvent) => void): () => void /** Phases the resident armed with [i] through the control channel. */ isInteractiveArmed(phase: string): boolean /** Repoints the command handlers; the ControlProgress adapter wires run objects late. */ @@ -268,6 +275,8 @@ export type ControlServer = { close(): void } +export type ControllerLeaseEvent = "claimed" | "released" | "expired" + /** Header the controller client echoes on every request; doubles as the heartbeat. */ export const CONTROLLER_ID_HEADER = "x-convoy-controller" @@ -284,9 +293,33 @@ export async function startControlServer(options: ControlServerOptions = {}): Pr let controllerId: string | undefined let controllerLastSeen = 0 const interactiveArmed = new Map() + const leaseListeners = new Set<(event: ControllerLeaseEvent) => void>() const controllerActive = () => controllerId !== undefined && Date.now() - controllerLastSeen < controllerTimeoutMs + const notifyLease = (event: ControllerLeaseEvent) => { + for (const listener of [...leaseListeners]) { + try { + listener(event) + } catch (error) { + log.warn(`[control] lease listener failed: ${error instanceof Error ? error.message : String(error)}`) + } + } + } + + // Autonomous expiry (design D4): a controller that dies without /bye is + // detected even when it never sends another request. The interval is + // bounded by the same lease the request path uses, and it is disposed with + // the server so no timer outlives a finished run. + const leaseTimer = setInterval(() => { + if (controllerId === undefined) return + if (Date.now() - controllerLastSeen < controllerTimeoutMs) return + controllerId = undefined + controllerLastSeen = 0 + notifyLease("expired") + }, 1_000) + leaseTimer.unref?.() + const server = Bun.serve({ hostname, port, @@ -297,12 +330,14 @@ export async function startControlServer(options: ControlServerOptions = {}): Pr claimController: () => { controllerId = crypto.randomUUID() controllerLastSeen = Date.now() + notifyLease("claimed") return controllerId }, releaseController: (id) => { if (!controllerActive() || id !== controllerId) return false controllerId = undefined controllerLastSeen = 0 + notifyLease("released") return true }, refreshController: (id) => { @@ -320,11 +355,19 @@ export async function startControlServer(options: ControlServerOptions = {}): Pr token, pending, hasController: () => controllerActive(), + onControllerLease: (listener) => { + leaseListeners.add(listener) + return () => leaseListeners.delete(listener) + }, isInteractiveArmed: (phase) => interactiveArmed.get(phase) === true, setHandlers: (next) => { handlers = next }, - close: () => server.stop(true), + close: () => { + clearInterval(leaseTimer) + leaseListeners.clear() + server.stop(true) + }, } } diff --git a/src/conversation-service.ts b/src/conversation-service.ts index ba1cdd2..b41e22c 100644 --- a/src/conversation-service.ts +++ b/src/conversation-service.ts @@ -1,6 +1,7 @@ import { join } from "node:path" import { bootOpencodeServerFrom } from "./opencode" +import { captureIdentity, defaultIdentityProbe, type IdentityProbe, type ProcessIdentity } from "./process-identity" import { readJsonFile, removePath, withExclusiveLock, writeJsonFile, type StoreRead } from "./repo-store" /** The discovery record's schema version; bumped only for a wire-format change. */ @@ -40,6 +41,14 @@ export type ConversationServiceRecord = { pid: number bootCheckout: string startedAt: number + /** + * Kernel birth identity of the recorded child for servers booted by this + * build (design D6). Optional and additive: legacy records simply omit it, + * and an explicit stop only re-verifies it when present so a recycled PID + * can never be killed as though it were the recorded server. The probe is + * not atomic, so this strengthens — never overstates — the guarded stop. + */ + childBirth?: string } export function validateConversationServiceRecord(value: unknown): ConversationServiceRecord | undefined { @@ -54,12 +63,14 @@ export function validateConversationServiceRecord(value: unknown): ConversationS if (typeof record.pid !== "number" || !Number.isInteger(record.pid) || record.pid <= 0) return undefined if (typeof record.bootCheckout !== "string" || record.bootCheckout === "") return undefined if (typeof record.startedAt !== "number") return undefined + if (record.childBirth !== undefined && typeof record.childBirth !== "string") return undefined return { schemaVersion: schemaVersion, url: record.url, pid: record.pid, bootCheckout: record.bootCheckout, startedAt: record.startedAt, + ...(typeof record.childBirth === "string" && record.childBirth !== "" ? { childBirth: record.childBirth } : {}), } } @@ -148,11 +159,16 @@ export async function ensureConversationService(input: { checkout: string bootTimeoutMs?: number /** Injected boot (tests); defaults to the detached OpenCode server boot. */ - boot?: (checkout: string, timeoutMs?: number) => Promise<{ url: string; close(): void; pid: number }> + boot?: (checkout: string, timeoutMs?: number) => Promise<{ url: string; close(): void | Promise; pid: number; identity?: ProcessIdentity }> /** Injected probe (tests); defaults to the PID + URL liveness probe. */ probe?: (record: ConversationServiceRecord) => Promise<"live" | "stale" | "uncertain"> }): Promise { - const boot = input.boot ?? ((checkout: string, timeoutMs?: number) => bootOpencodeServerFrom(checkout, timeoutMs ?? 30_000)) + const boot = + input.boot ?? + ((checkout: string, timeoutMs?: number) => + // An independently persistent authoring service: explicitly excluded + // from run/helper orphan reconciliation (design D1/D6). + bootOpencodeServerFrom(checkout, timeoutMs ?? 30_000, { lifetime: "authoring-service" })) const probe = input.probe ?? probeConversationService let outcome: ConversationServiceOutcome = { status: "uncertain", reason: "the authoring service lock was lost" } await withExclusiveLock(join(input.commonDir, "convoy", "authoring-service"), async () => { @@ -179,7 +195,7 @@ export async function ensureConversationService(input: { outcome = { status: "uncertain", reason: `the authoring service discovery record is ${read.status === "unreadable" ? `unreadable: ${read.reason}` : read.status} — inspect ${discoveryPath(input.commonDir)} before conversation work` } return } - let booted: { url: string; close(): void; pid: number } + let booted: { url: string; close(): void | Promise; pid: number; identity?: ProcessIdentity } try { booted = await boot(input.checkout, input.bootTimeoutMs) } catch (error) { @@ -192,8 +208,19 @@ export async function ensureConversationService(input: { pid: booted.pid, bootCheckout: input.checkout, startedAt: Date.now(), + // Retain the child's kernel birth identity for newly booted services so + // the guarded stop can refuse a recycled PID (design D6). + ...(booted.identity?.birth ? { childBirth: booted.identity.birth } : {}), + } + try { + await writeJsonFile(discoveryPath(input.commonDir), record) + } catch (error) { + // An unpublished child has no discovery record to control its lifetime: + // close it rather than leak an unreachable server (design D6). + await Promise.resolve(booted.close()).catch(() => {}) + outcome = { status: "unavailable", reason: `the authoring service discovery record could not be written: ${error instanceof Error ? error.message : String(error)}` } + return } - await writeJsonFile(discoveryPath(input.commonDir), record) outcome = { status: "live", url: booted.url, record, reused: false } }) return outcome @@ -214,6 +241,8 @@ export async function stopConversationService(input: { activity: "idle" | "busy" | "unknown" /** Injected probe (tests); defaults to the PID + URL liveness probe. */ probe?: (record: ConversationServiceRecord) => Promise<"live" | "stale" | "uncertain"> + /** Injected identity probe (tests); defaults to the platform kernel probe. */ + identityProbe?: IdentityProbe /** Injected kill (tests); defaults to SIGTERM on the recorded PID. */ kill?: (record: ConversationServiceRecord) => Promise | void }): Promise<{ status: "stopped" } | { status: "kept"; reason: string } | { status: "missing" }> { @@ -239,6 +268,19 @@ export async function stopConversationService(input: { return { status: "kept", reason: `the authoring server (pid ${record.pid}, ${record.url}) is in an unverified state — it is kept until it answers or its process is gone` } } if (liveness === "live") { + // Newly booted services record their child's kernel birth identity. An + // explicit stop re-verifies it where present so a recycled PID behind a + // stale URL answer is never killed as though it were the recorded server + // (design D6). Legacy records without identity keep the PID+URL intent. + if (record.childBirth) { + const observed = await captureIdentity(record.pid, input.identityProbe ?? defaultIdentityProbe()) + if (!observed || observed.birth !== record.childBirth) { + return { + status: "kept", + reason: `the authoring server pid ${record.pid} no longer matches its recorded child identity — it is kept rather than killed`, + } + } + } const kill = input.kill ?? (async (target: ConversationServiceRecord) => { process.kill(target.pid, "SIGTERM") }) diff --git a/src/conversations.ts b/src/conversations.ts index 740a4a5..70130db 100644 --- a/src/conversations.ts +++ b/src/conversations.ts @@ -26,7 +26,7 @@ export type AuthoringSessionRef = { * is stopped only through the service's explicit, guarded stop. Adapters * never close an injected handle. */ -type ServerHandle = { url: string; close?(): void } +type ServerHandle = { url: string; close?(): void | Promise } /** * Boots a bounded server rooted at the checkout, creates a session, and @@ -53,7 +53,7 @@ export async function createAuthoringConversation(input: { return { harness: "opencode", sessionId: created.data.id } } finally { // Only close servers this call booted; an injected one belongs to its owner. - if (!input.server) server.close?.() + if (!input.server) await server.close?.() } } @@ -80,7 +80,7 @@ export async function validateAuthoringSession(input: { } return { status: "available", ...(got.data.title ? { title: got.data.title } : {}) } } finally { - if (!input.server) server.close?.() + if (!input.server) await server.close?.() } } @@ -180,7 +180,7 @@ export async function sessionActivity(input: { } catch { return "unknown" } finally { - if (!input.server) server.close?.() + if (!input.server) await server.close?.() } } @@ -203,7 +203,7 @@ export async function listAuthoringCommands(input: { } catch { return "unknown" } finally { - if (!input.server) server.close?.() + if (!input.server) await server.close?.() } } diff --git a/src/coordinate.ts b/src/coordinate.ts index f58da45..7b9cfcc 100644 --- a/src/coordinate.ts +++ b/src/coordinate.ts @@ -2,11 +2,11 @@ import { closeSync, openSync } from "node:fs" import { mkdir, open, readFile, readdir, rm, stat, writeFile, type FileHandle } from "node:fs/promises" import { join, resolve, sep } from "node:path" -import { startControlServer } from "./control-server" +import { startControlServer, type ControlServer } from "./control-server" import { ControlProgress, type ControlProgressOptions } from "./control-progress" import { hasWritableStep } from "./pipeline" import { pidAlive } from "./runs" -import { hostedTeardownFromError, isUserAbortError, run } from "./runner" +import { hostedTeardownFromError, installShutdownSignals, isUserAbortError, run, RunShutdown } from "./runner" import { isOfficialStandaloneExecutable } from "./update" import { convoyHome } from "./workspace" import type { RunOptions, RunPlan } from "./types" @@ -54,7 +54,9 @@ export function pendingRoot(): string { /** Strips functions so the launch payload survives JSON round-tripping. */ export function launchPayload(options: RunOptions, plan: RunPlan | undefined): LaunchFile { - const { progress: _progress, ...rest } = options + // `shutdown` is a runtime-only ownership handle the coordinator installs in + // memory; it must never ride the persisted launch file. + const { progress: _progress, shutdown: _shutdown, ...rest } = options return { schemaVersion: 1, options: rest, @@ -286,6 +288,10 @@ export type CoordinateBootDeps = { hostedTeardownFromError: typeof hostedTeardownFromError /** Override for `assertInternalLaunchPath`; tests point this at a scratch dir. */ launchRoot?: string + /** Test seam: the coordinator-owned shutdown scope (design D3). */ + createShutdown?: () => RunShutdown + /** Test seam: installs the process signal handlers; returns their remover. */ + installSignals?: (shutdown: RunShutdown) => () => void } const defaultCoordinateBootDeps: CoordinateBootDeps = { @@ -295,6 +301,17 @@ const defaultCoordinateBootDeps: CoordinateBootDeps = { hostedTeardownFromError, } +/** + * Awaits a terminal hold but yields to the coordinator's shutdown scope: a + * SIGTERM during the finish hold must fall through to the bounded owned-server + * stop instead of parking forever (design D3). An abort that already landed is + * checked first so no hold is entered after cancellation. + */ +async function holdUntilAbort(hold: Promise, signal: AbortSignal): Promise { + if (signal.aborted) return + await Promise.race([hold, new Promise((resolve) => signal.addEventListener("abort", () => resolve(), { once: true }))]) +} + /** * The child side of `--coordinate `. The control server starts * here, before run() boot, so a client can attach during OpenCode boot. The @@ -312,7 +329,14 @@ export async function runCoordinateBoot( const plan = launch.plan if (!plan) throw new Error(`launch file ${launchPath} carries no reviewed plan`) - const server = await deps.startControlServer() + // The coordinator owns one shutdown scope for its whole lifetime — boot, + // execution, the terminal finish hold, and final release (design D3). Its + // signal handlers must outlive run(), whose finally no longer disposes an + // injected scope; a SIGTERM during the finish hold therefore releases the + // wait and runs the bounded owned-server stop instead of exiting blindly. + const shutdown = (deps.createShutdown ?? (() => new RunShutdown()))() + const removeSignals = (deps.installSignals ?? installShutdownSignals)(shutdown) + let server: ControlServer | undefined // Managed writer ownership (capability work-conversations, design D5): the // coordinator is the run's writer, so its claim lives exactly as long as // the run does — acquired here (the coordinator's own PID is the liveness @@ -320,30 +344,31 @@ export async function runCoordinateBoot( // in the same checkout before this child ever spawns; an acquisition // conflict or persistence failure stops the run (fail closed). let writerClaim: { branch: string } | undefined - if (hasWritableStep(plan.pipeline)) { - const { repoCommonDir } = await import("./repo-store") - const { acquireWriterClaim, writerConflictGuidance } = await import("./writer-claims") - const commonDir = await repoCommonDir(plan.target.directory) - if (commonDir) { - const { currentBranch } = await import("./git") - const branch = plan.target.branch ?? (await currentBranch(plan.target.directory).catch(() => undefined)) - if (branch) { - const acquired = await acquireWriterClaim({ - commonDir, - branch, - checkoutPath: plan.target.directory, - kind: "pipeline", - }) - if (acquired.status === "acquired") { - writerClaim = { branch } - } else { - const detail = acquired.status === "conflict" ? writerConflictGuidance(acquired.existing).join(" ") : "a writer claim for this checkout is in an uncertain state — reconcile it before starting another writer" - throw new Error(`another managed writer owns ${plan.target.directory}: ${detail}`) + try { + server = await deps.startControlServer() + if (hasWritableStep(plan.pipeline)) { + const { repoCommonDir } = await import("./repo-store") + const { acquireWriterClaim, writerConflictGuidance } = await import("./writer-claims") + const commonDir = await repoCommonDir(plan.target.directory) + if (commonDir) { + const { currentBranch } = await import("./git") + const branch = plan.target.branch ?? (await currentBranch(plan.target.directory).catch(() => undefined)) + if (branch) { + const acquired = await acquireWriterClaim({ + commonDir, + branch, + checkoutPath: plan.target.directory, + kind: "pipeline", + }) + if (acquired.status === "acquired") { + writerClaim = { branch } + } else { + const detail = acquired.status === "conflict" ? writerConflictGuidance(acquired.existing).join(" ") : "a writer claim for this checkout is in an uncertain state — reconcile it before starting another writer" + throw new Error(`another managed writer owns ${plan.target.directory}: ${detail}`) + } } } } - } - try { const progress = deps.createProgress({ server, readyPath }) // The gate/control cycle shares exactly the adapter's AutoAccept object. // Seed it from the launch flags: run() uses options.autoAccept when @@ -355,7 +380,7 @@ export async function runCoordinateBoot( // launch options carry the unresolved config; run() only swaps in the // reviewed steps when options.plan is set. Dropping it here silently // turns every advised pipeline into an unadvised one. - const options: RunOptions = { ...launch.options, plan, progress, autoAccept: progress.autoAccept, tui: false } + const options: RunOptions = { ...launch.options, plan, progress, autoAccept: progress.autoAccept, tui: false, shutdown } // metadata.server gains the control URL (no token) for liveness/debug. process.env.CONVOY_CONTROL_URL = server.url @@ -365,17 +390,20 @@ export async function runCoordinateBoot( // metadata store, one server, one hook lifecycle, one finish hold. try { const result = await deps.run(options) - await progress.runFinished({ status: "completed", runDir: result.dir }) + await holdUntilAbort(progress.runFinished({ status: "completed", runDir: result.dir }), shutdown.signal) await result.release?.() return 0 } catch (error) { const teardown = deps.hostedTeardownFromError(error) if (!isUserAbortError(error)) { - await progress.runFinished({ - status: "failed", - runDir: teardown?.runDir ?? "", - ...(error instanceof Error ? { error: error.message } : { error: String(error) }), - }) + await holdUntilAbort( + progress.runFinished({ + status: "failed", + runDir: teardown?.runDir ?? "", + ...(error instanceof Error ? { error: error.message } : { error: String(error) }), + }), + shutdown.signal, + ) } await teardown?.release?.() throw error @@ -387,6 +415,8 @@ export async function runCoordinateBoot( const commonDir = await repoCommonDir(plan.target.directory).catch(() => undefined) if (commonDir) await releaseWriterClaim({ commonDir, branch: writerClaim.branch, ownerPid: process.pid }) } - server.close() + server?.close() + removeSignals() + shutdown.dispose() } } diff --git a/src/finalization/compact.ts b/src/finalization/compact.ts index 88d355b..d86d190 100644 --- a/src/finalization/compact.ts +++ b/src/finalization/compact.ts @@ -6,6 +6,7 @@ import { currentHead, diffStat, execFile, resetSoft, resolveCommit } from "../gi import { log } from "../log" import { readPersistedRunTitle } from "../run-title" import { convoyHome } from "../workspace" +import type { StopPolicy } from "../process-stop" import type { FeaturePlanLink } from "../types" import { boundedCommitAsOperator } from "./executor" import { verifyRunInterval, type RunInterval } from "./interval" @@ -71,6 +72,12 @@ export type RunFinalizationInput = { feature?: FeaturePlanLink commitMessageModel?: string signal?: AbortSignal + /** + * Shared cleanup budget for the model-backed commit writer's helper (design + * D2). A coordinator passes a resolver drawing from its remaining shutdown + * deadline so this helper cannot restart or exceed that budget. + */ + stopPolicy?: () => StopPolicy progress?: FinalizationProgress /** * Overrides message composition (hermetic tests inject a deterministic @@ -566,6 +573,7 @@ async function composeMessage(input: RunFinalizationInput, interval: Extract + /** + * Last-resort synchronous edge for a repeated abort or the shutdown + * deadline: delivers SIGKILL immediately and starts the same observed stop, + * so a coordinator can exit on a bounded timer instead of abandoning the + * child. Safe to call repeatedly and after `stop()`. + */ + forceStop(): void +} + +export type SpawnChildFn = (command: string, args: string[], options: SpawnOptions) => ChildProcess + +export type ManagedServerOptions = { + command: string + args: string[] + cwd: string + env: NodeJS.ProcessEnv + lifetime: LifetimeClass + /** Human label for diagnostics (e.g. "run server", "model catalog helper"). */ + label: string + /** Readiness timeout; the SDK used 30s. */ + timeoutMs?: number + signal?: AbortSignal + /** Readiness line parser; returns the URL or undefined for a non-readiness line. */ + parseLine?: (line: string) => { url: string } | "malformed" | undefined + /** Recorded in the lifetime record so an orphan can be tied back to its run. */ + runId?: string + deps?: { + spawn?: SpawnChildFn + probe?: IdentityProbe + store?: ProcessRecordStore + now?: () => number + /** + * Stop policy for this child. A resolver is evaluated at stop time so an + * owned helper can draw from a shared, shrinking shutdown budget (design + * D2) instead of starting a fresh standalone allowance. + */ + policy?: StopPolicy | (() => StopPolicy) + /** + * Bounded orphan-recovery pass run before a run/helper boot (design D5). + * Injectable so tests assert the wiring without touching real processes; + * defaults to {@link reconcileProcessRecords}. + */ + reconcile?: (deps: { store: ProcessRecordStore; probe: IdentityProbe }) => Promise + } +} + +export const defaultReadinessTimeoutMs = 30_000 + +/** + * Strict readiness parser matching the CLI's own line (`opencode server + * listening on http://…`). A listening line without a parseable URL is + * malformed and fails the boot rather than being ignored. + */ +export function parseReadinessLine(line: string): { url: string } | "malformed" | undefined { + if (!line.startsWith("opencode server listening")) return undefined + const match = line.match(/on\s+(https?:\/\/[^\s]+)/) + if (!match?.[1]) return "malformed" + return { url: match[1] } +} + +/** Bounds diagnostic stdout/stderr retained for error messages. */ +const diagnosticTailLimit = 4_000 + +/** + * Bounds the unterminated stdout line retained while waiting for a newline. A + * readiness line is short, so a chatty child that never emits a newline cannot + * grow the line buffer without bound; only the tail can still complete a line. + */ +const stdoutLineLimit = 64 * 1024 + +export class ManagedServerStartupError extends Error { + readonly cleanup?: StopOutcome + constructor(message: string, cleanup?: StopOutcome) { + super(message) + this.name = "ManagedServerStartupError" + if (cleanup) this.cleanup = cleanup + } +} + +/** + * Spawns and owns one `opencode serve` child. Resolves only once the child has + * reported readiness *and* its ownership record has been published; rejects + * (after bounded cleanup) for spawn errors, early exit, malformed readiness, + * timeout, abort, or record-publication failure. + */ +export async function launchManagedServer(options: ManagedServerOptions): Promise { + const deps = options.deps ?? {} + const spawnChild = deps.spawn ?? (nodeSpawn as unknown as SpawnChildFn) + const probe = deps.probe ?? defaultIdentityProbe() + const store = deps.store ?? createProcessRecordStore() + const now = deps.now ?? Date.now + const timeoutMs = options.timeoutMs ?? defaultReadinessTimeoutMs + const reconciledLifetime = options.lifetime === "authoring-service" ? undefined : options.lifetime + + if (options.signal?.aborted) { + throw new ManagedServerStartupError(`${options.label} was cancelled before it started`) + } + + // Bounded orphan recovery before an owned run/helper boot (design D5): a + // later managed launch is the only trigger that reclaims attributable + // run/helper orphans left by an uncatchably-killed owner. It is bounded, + // never signals a live owner's child, and never fails this launch — a + // recovery pass that cannot classify an old record is not a reason to + // refuse unrelated work. + if (reconciledLifetime) { + await (deps.reconcile ?? reconcileProcessRecords)({ store, probe }).catch(() => {}) + } + + // Provisional record before spawn (design D5): a crash between OS spawn and + // identity publication must still leave evidence that a child may exist. + let record: ProcessRecord | undefined + if (reconciledLifetime) { + const owner = await captureIdentity(process.pid, probe).catch(() => undefined) + record = { + ...newProcessRecord({ lifetime: reconciledLifetime, now: now() }), + ...(owner ? { owner } : {}), + ...(options.runId ? { runId: options.runId } : {}), + } + try { + await store.put(record) + } catch (error) { + // Ownership evidence that cannot be persisted must not be papered over: + // refuse to launch rather than create an unattributable child. + throw new ManagedServerStartupError( + `${options.label} ownership cannot be persisted under ${store.dir}: ${describe(error)}`, + ) + } + } + + const child = spawnChild(options.command, options.args, { + cwd: options.cwd, + env: options.env, + stdio: ["ignore", "pipe", "pipe"], + }) + + return await new Promise((resolve, reject) => { + let settled = false + let ready = false + let stdoutBuffer = "" + let stderrTail = "" + let stdoutTail = "" + let childIdentity: ProcessIdentity | undefined + let stopPromise: Promise | undefined + let abortListener: (() => void) | undefined + let readinessTimer: ReturnType | undefined + + const cleanupListeners = () => { + if (readinessTimer) clearTimeout(readinessTimer) + child.removeListener?.("exit", onExit as never) + if (abortListener && options.signal) options.signal.removeEventListener("abort", abortListener) + } + + const performStop = (): Promise => { + stopPromise ??= (async () => { + const pid = child.pid ?? 0 + const target = childStopTarget({ + child, + pid, + ...(childIdentity + ? { + verify: async (): Promise<{ ok: true } | { ok: false; reason: string }> => { + const observed = await probe.observe(pid) + if (observed.status === "gone") return { ok: true } + if (observed.status !== "alive") return { ok: false, reason: `child identity became ${observed.status}` } + return observed.identity.birth === childIdentity!.birth + ? { ok: true } + : { ok: false, reason: "child incarnation changed before escalation" } + }, + } + : {}), + }) + const policy = typeof deps.policy === "function" ? deps.policy() : deps.policy + const outcome = await stopTarget(target, policy) + destroyPipes() + // Release the record only on a confirmed stop; an unresolved child + // keeps its evidence for a later reconciliation pass. + if (record) { + if (outcome.status === "stopped") await store.remove(record.id).catch(() => {}) + else await store.put({ ...record, state: "unresolved", updatedAt: now(), lastOutcome: outcome.reason }).catch(() => {}) + } + return outcome + })() + return stopPromise + } + + const destroyPipes = () => { + try { + child.stdout?.destroy?.() + } catch { + /* best effort */ + } + try { + child.stderr?.destroy?.() + } catch { + /* best effort */ + } + } + + const forceStop = () => { + try { + child.kill("SIGKILL") + } catch { + /* the child may already be gone; the stop state machine rechecks */ + } + void performStop() + } + + const onExit = (code: number | null, signal: NodeJS.Signals | null) => { + if (settled) return + settled = true + cleanupListeners() + if (record && reconciledLifetime) { + // The child exited before readiness; its record is resolved. + void store.remove(record.id).catch(() => {}) + } + reject( + new ManagedServerStartupError( + `${options.label} exited before it was ready (code ${code ?? "null"}${signal ? `, ${signal}` : ""})${diagnosticSuffix()}`, + ), + ) + } + + const onError = (error: Error) => { + if (settled) return + settled = true + cleanupListeners() + if (record && reconciledLifetime) void store.remove(record.id).catch(() => {}) + reject(new ManagedServerStartupError(`${options.label} could not start: ${describe(error)}`)) + } + + const diagnosticSuffix = () => { + const tail = (stderrTail || stdoutTail).trim() + return tail ? `: ${tail.slice(-diagnosticTailLimit)}` : "" + } + + const finish = () => { + if (settled) return + settled = true + ready = true + cleanupListeners() + resolve({ + url: parsedUrl!, + pid: child.pid ?? 0, + ...(childIdentity ? { identity: childIdentity } : {}), + ...(record ? { recordId: record.id } : {}), + stop: performStop, + forceStop, + }) + } + + let parsedUrl: string | undefined + + const onLine = (line: string) => { + if (settled || parsedUrl) return + const parsed = (options.parseLine ?? parseReadinessLine)(line) + if (!parsed) return + if (parsed === "malformed") { + settled = true + cleanupListeners() + void performStop().then((cleanup) => { + reject(new ManagedServerStartupError(`${options.label} reported a malformed readiness line: ${line.trim()}`, cleanup)) + }) + return + } + parsedUrl = parsed.url + // Publish child identity and readiness before exposing the URL. + void publishOwnershipThenResolve() + } + + const publishOwnershipThenResolve = async () => { + if (settled) return + childIdentity = await captureIdentity(child.pid ?? 0, probe).catch(() => undefined) + if (record && reconciledLifetime) { + if (!childIdentity) { + // A run/helper whose required ownership evidence cannot be captured + // must never be exposed as ready (design D1/D5): refuse readiness and + // clean up through the owned child handle instead of leaking an + // unattributable server. + settled = true + cleanupListeners() + const cleanup = await performStop() + reject(new ManagedServerStartupError(`${options.label} child identity could not be captured under ${store.dir}`, cleanup)) + return + } + const published = await publishChildIdentity(store, record, child.pid ?? 0, probe).catch(() => undefined) + if (!published) { + // Persisting ownership failed: refuse readiness and clean up. Settle + // before the stop so the child's own exit cannot win the rejection + // race and mask the ownership failure. + settled = true + cleanupListeners() + const cleanup = await performStop() + reject(new ManagedServerStartupError(`${options.label} ownership could not be published under ${store.dir}`, cleanup)) + return + } + record = published.record + record = { ...record, state: "ready", url: parsedUrl, updatedAt: now() } + // Awaited so a stop immediately after readiness cannot be overtaken by + // this write and lose the unresolved outcome. + await store.put(record).catch(() => {}) + } + finish() + } + + const onStdout = (chunk: Buffer | string) => { + const text = chunk.toString() + stdoutBuffer += text + stdoutTail = (stdoutTail + text).slice(-diagnosticTailLimit) + for (;;) { + const newline = stdoutBuffer.indexOf("\n") + if (newline === -1) break + const line = stdoutBuffer.slice(0, newline) + stdoutBuffer = stdoutBuffer.slice(newline + 1) + onLine(line) + } + // Bound the partial line a newline-less child can accumulate before its + // next newline arrives (only the tail can still complete a readiness + // line), so stdout cannot grow without bound. + if (stdoutBuffer.length > stdoutLineLimit) stdoutBuffer = stdoutBuffer.slice(-stdoutLineLimit) + } + + const onStderr = (chunk: Buffer | string) => { + const text = chunk.toString() + stderrTail = (stderrTail + text).slice(-diagnosticTailLimit) + } + + child.stdout?.on("data", onStdout) + child.stderr?.on("data", onStderr) + child.once("exit", onExit as never) + child.on("error", onError as never) + + const timer = setTimeout(() => { + if (settled) return + settled = true + cleanupListeners() + void performStop().then((cleanup) => { + reject(new ManagedServerStartupError(`${options.label} did not report a readiness URL within ${timeoutMs}ms${diagnosticSuffix()}`, cleanup)) + }) + }, timeoutMs) + timer.unref?.() + readinessTimer = timer + + if (options.signal) { + abortListener = () => { + if (settled) return + settled = true + cleanupListeners() + void performStop().then((cleanup) => { + reject(new ManagedServerStartupError(`${options.label} was cancelled before it was ready`, cleanup)) + }) + } + // A signal that fired during the async gaps before this listener existed + // (reconciliation, owner capture, provisional publication) never replays + // its event. Check `aborted` explicitly so a cancellation race can never + // resolve a live owned server (design D1). + if (options.signal.aborted) abortListener() + else options.signal.addEventListener("abort", abortListener, { once: true }) + } + + // A child that never emits a readiness line but exits cleanly is handled + // by onExit; one whose stdout closes early is handled by the timeout. + }) +} + +function describe(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} diff --git a/src/metadata.ts b/src/metadata.ts index f3bf72b..d9f84f1 100644 --- a/src/metadata.ts +++ b/src/metadata.ts @@ -101,8 +101,28 @@ export type RunMetadata = { */ plannedPhases?: ProgressPhase[] modelRouting?: { gateway: ModelGateway } - /** The live opencode server for this run while it executes; cleared on shutdown, so a lingering entry means the run process died mid-flight. Lets `convoy runs` attach to a running run. */ - server?: { url: string; pid: number; startedAt: number; controlUrl?: string } + /** + * The live opencode server for this run while it executes; cleared on + * shutdown, so a lingering entry means the run process died mid-flight. Lets + * `convoy runs` attach to a running run. + * + * `pid` keeps its historical meaning: the coordinator/owner anchor, not the + * child. New runs add explicit child evidence (design D6) — the managed + * server's runtime process-record id and captured child incarnation — while + * legacy records simply omit those optional fields and stay readable. + */ + server?: { + url: string + pid: number + startedAt: number + controlUrl?: string + /** Id of the durable `~/.convoy/processes` record that owns the child. */ + recordId?: string + /** The actual `opencode serve` child PID (distinct from the coordinator pid). */ + childPid?: number + /** Kernel birth identity of the child, so a reused PID is not mistaken for it. */ + childBirth?: string + } control: { state: RunControlState; requestedAt?: number; pausedAt?: number } phases: Record /** The durable goal-cycle record, present when the pipeline declares a terminal goal step (schema v4). */ @@ -158,6 +178,12 @@ export type RunMetadataStore = { setFinalization(record: FinalizationRecord): Promise /** Records the run's live opencode server URL so `convoy runs` can attach; cleared by serverStopped. */ serverStarted(url: string): void + /** + * Attaches explicit child evidence (record id + child PID/birth) to the live + * server block so a leftover entry identifies the actual `serve` child + * without relabeling the historical coordinator PID (design D6). + */ + recordServerChild(evidence: { recordId?: string; pid: number; birth?: string }): void serverStopped(): Promise phaseStarted(name: string): Promise phaseSession(name: string, sessionID: string): void @@ -392,6 +418,16 @@ export async function openRunMetadata( data.server = { url, pid: process.pid, startedAt: Date.now(), ...(controlUrl ? { controlUrl } : {}) } void persist() }, + recordServerChild(evidence) { + // Annotates the live block only; `serverStarted` keeps the historical + // coordinator `pid`, and a legacy record that never got evidence is + // still read by the same history readers (design D6). + if (!data.server) return + if (evidence.recordId) data.server.recordId = evidence.recordId + data.server.childPid = evidence.pid + if (evidence.birth) data.server.childBirth = evidence.birth + void persist() + }, async serverStopped() { data.server = undefined await persist({ throwOnError: true }) diff --git a/src/model-catalog.ts b/src/model-catalog.ts index e603541..1cbe6dc 100644 --- a/src/model-catalog.ts +++ b/src/model-catalog.ts @@ -71,7 +71,9 @@ async function listModelsFromSdk(targetDir: string, start: NonNullable + /** Last-resort synchronous SIGKILL edge used by a repeated abort/deadline. */ + forceStop(): void + close(): Promise +} + +/** A bounded per-call server boot (conversation helpers, CLI discovery). */ +export type BootedOpencodeServer = { + url: string + pid: number + identity?: ProcessIdentity + /** Id of the durable helper record that owns this child (design D6). */ + recordId?: string + stop(): Promise + forceStop(): void + close(): Promise } type StartOpencodeDeps = { getFreePort(): Promise - createServer(options: Parameters[0]): Promise<{ url: string; close(): void }> createClient(options: Parameters[0]): OpencodeClient + /** Test seam: replaces the real owned spawn. */ + launch(options: Parameters[0]): Promise + /** Lifetime class; the executor's run server is `run`, helpers default to `helper`. */ + lifetime?: LifetimeClass + /** Run id recorded on the lifetime record so an orphan can be tied to its run (design D6). */ + runId?: string + probe?: IdentityProbe + store?: ProcessRecordStore + /** + * Stop policy for the owned helper. A resolver lets a helper under a + * coordinator draw from that coordinator's remaining shutdown budget (design + * D2) instead of a fresh standalone allowance. + */ + stopPolicy?: StopPolicy | (() => StopPolicy) } // Async on purpose: this is called from the TUI's render path, and a sync @@ -133,49 +177,44 @@ async function freePort() { /** * Boots a bounded OpenCode server rooted at an explicit checkout (capability - * work-conversations, design D5): unlike `startOpencode`, which inherits + * work-conversations, design D5). Unlike `startOpencode`, which inherits * Convoy's cwd, this spawns the CLI with an explicit cwd so the server's * project scope is that checkout's repository. The URL is parsed from the - * server's own startup output, and `close()` terminates the child; a boot - * that fails or stalls within the timeout rejects instead of hanging. + * server's own startup output, ownership is published before readiness, and + * `close()` performs a bounded observed stop; a boot that fails or stalls + * within the timeout rejects instead of hanging. + * + * Lifetime defaults to `helper` (a short-lived per-call server). An + * independently persistent authoring service passes `authoring-service`, which + * is deliberately excluded from orphan reconciliation. */ -export async function bootOpencodeServerFrom(checkout: string, timeoutMs = 30_000): Promise<{ url: string; close(): void; pid: number }> { - const { spawn } = await import("node:child_process") - const port = await freePort() - const child = spawn("opencode", ["serve", "--hostname=127.0.0.1", `--port=${port}`], { +export async function bootOpencodeServerFrom( + checkout: string, + timeoutMs = 30_000, + options: { lifetime?: LifetimeClass; deps?: Partial } = {}, +): Promise { + const lifetime = options.lifetime ?? "helper" + const server = await (options.deps?.launch ?? launchManagedServer)({ + command: "opencode", + args: ["serve", "--hostname=127.0.0.1", `--port=${await (options.deps?.getFreePort ?? freePort)()}`], cwd: checkout, - stdio: ["ignore", "pipe", "pipe"], - }) - const url = await new Promise((resolve, reject) => { - let stdout = "" - let stderr = "" - const timer = setTimeout(() => { - child.kill("SIGTERM") - reject(new Error(`opencode server did not report a URL within ${timeoutMs}ms (stderr: ${stderr.trim().slice(0, 300)})`)) - }, timeoutMs) - child.stdout!.on("data", (chunk: Buffer) => { - stdout += chunk.toString() - const match = stdout.match(/http:\/\/[^\s]+/) - if (match) { - clearTimeout(timer) - resolve(match[0]) - } - }) - child.stderr!.on("data", (chunk: Buffer) => { - stderr += chunk.toString() - }) - child.on("exit", (code) => { - clearTimeout(timer) - reject(new Error(`opencode server exited with code ${code}${stderr.trim() ? `: ${stderr.trim().slice(0, 300)}` : ""}`)) - }) + env: withoutHerdrEnv(process.env), + lifetime, + label: lifetime === "authoring-service" ? "opencode authoring service" : "opencode helper", + timeoutMs, + deps: { + ...(options.deps?.probe ? { probe: options.deps.probe } : {}), + ...(options.deps?.stopPolicy ? { policy: options.deps.stopPolicy } : {}), + }, }) return { - url, - close() { - child.kill("SIGTERM") - }, - // The spawned server process — the conversation service's liveness anchor. - pid: child.pid ?? 0, + url: server.url, + pid: server.pid, + ...(server.identity ? { identity: server.identity } : {}), + ...(server.recordId ? { recordId: server.recordId } : {}), + stop: server.stop, + forceStop: server.forceStop, + close: server.stop, } } @@ -185,60 +224,52 @@ export async function startOpencode( deps?: Partial, ): Promise { const port = await (deps?.getFreePort ?? freePort)() - // The SDK hands the server child Convoy's environment at spawn time, and - // ServerOptions has no env override (confirmed against @opencode-ai/sdk). - // A global `herdr integration install opencode` plugin would otherwise - // inherit HERDR_PANE_ID and claim the pane as an "opencode" agent. - // - // This wrapper is synchronous: `finally` restores process.env when `fn` - // returns, which for an async createOpencodeServer is when the Promise is - // *created*, not when it settles. That is enough because @opencode-ai/sdk - // spreads `{...process.env}` in launch()/cross-spawn before its first - // `await`. Re-verify that on SDK upgrades — if spawn moves past an await, - // the child would inherit the restored HERDR_* keys. Do not make this - // helper async: awaiting would widen the global-mutation window. - const server = await withProcessHerdrEnvStripped(() => - (deps?.createServer ?? createOpencodeServer)({ - hostname: "127.0.0.1", - port, - timeout: 30_000, - signal, - config, - }), - ) - const client = (deps?.createClient ?? createOpencodeClient)({ baseUrl: server.url, fetch: fetchWithoutIdleTimeout as typeof fetch }) + const lifetime = deps?.lifetime ?? "helper" + const args = ["serve", "--hostname=127.0.0.1", `--port=${port}`] + const logLevel = (config as { logLevel?: unknown } | undefined)?.logLevel + if (typeof logLevel === "string" && logLevel) args.push(`--log-level=${logLevel}`) - return { - client, - url: server.url, - close: server.close, - } -} + // The child's environment is built explicitly per launch (design D1): HERDR_* + // keys are stripped from the copy handed to the child instead of mutating — + // and restoring — the parent's process.env around an SDK call. A herdr + // integration plugin would otherwise inherit HERDR_PANE_ID and claim the + // pane as an "opencode" agent. OPENCODE_CONFIG_CONTENT matches the SDK's own + // injection so project/global config keeps deep-merging identically. + const server = await (deps?.launch ?? launchManagedServer)({ + command: "opencode", + args, + cwd: process.cwd(), + env: { ...withoutHerdrEnv(process.env), OPENCODE_CONFIG_CONTENT: JSON.stringify(config ?? {}) }, + lifetime, + label: lifetime === "run" ? "opencode run server" : "opencode helper", + timeoutMs: 30_000, + ...(signal ? { signal } : {}), + ...(deps?.runId ? { runId: deps.runId } : {}), + deps: { + ...(deps?.probe ? { probe: deps.probe } : {}), + ...(deps?.store ? { store: deps.store } : {}), + ...(deps?.stopPolicy ? { policy: deps.stopPolicy } : {}), + }, + }) -/** - * Runs `fn` with every `HERDR_*` key removed from `process.env`, then restores - * them when `fn` returns (not when a returned Promise settles). See the - * call-site comment: the strip only covers the SDK's synchronous spawn. - */ -function withProcessHerdrEnvStripped(fn: () => T): T { - // Reuses the same filter as the reporter's env injection so the set of - // stripped keys stays in one place. The kept object is a shallow copy of the - // non-HERDR entries; any key absent from it is a HERDR_* key to save/delete. - const kept = withoutHerdrEnv(process.env) - const saved = new Map() - for (const key of Object.keys(process.env)) { - if (!(key in kept)) { - saved.set(key, process.env[key]) - delete process.env[key] - } - } try { - return fn() - } finally { - for (const [key, value] of saved) { - if (value === undefined) delete process.env[key] - else process.env[key] = value + const client = (deps?.createClient ?? createOpencodeClient)({ baseUrl: server.url, fetch: fetchWithoutIdleTimeout as typeof fetch }) + return { + client, + url: server.url, + pid: server.pid, + ...(server.identity ? { identity: server.identity } : {}), + ...(server.recordId ? { recordId: server.recordId } : {}), + stop: server.stop, + forceStop: server.forceStop, + close: server.stop, } + } catch (error) { + // Never hand out a live child without a client: fail closed and confirm + // the bounded stop before surfacing the construction failure. + const cleanup = await server.stop() + const reason = error instanceof Error ? error.message : String(error) + throw new Error(`opencode server client construction failed: ${reason} (cleanup: ${cleanup.status === "stopped" ? cleanup.via : `unresolved — ${cleanup.reason}`})`) } } diff --git a/src/preflight.ts b/src/preflight.ts index 10b0e36..7f58751 100644 --- a/src/preflight.ts +++ b/src/preflight.ts @@ -1,4 +1,4 @@ -import { startOpencode } from "./opencode" +import { startOpencode, type OpencodeHandle } from "./opencode" import type { RunPlan } from "./types" import { type ProviderCatalog, preflightTargets, validatePreflightTargets } from "./preflight-validation" @@ -6,17 +6,50 @@ const preflightTimeoutMs = 15_000 export type PreflightDiscovery = (directory: string, signal: AbortSignal) => Promise /** Validate the exact physical OpenCode targets after approval and before run/worktree creation. */ -export async function preflightRunPlan(plan: RunPlan, discover: PreflightDiscovery = discoverProviderCatalog): Promise { +export async function preflightRunPlan(plan: RunPlan, discover?: PreflightDiscovery): Promise { const targets = preflightTargets(plan) if (targets.length === 0) return const timeout = AbortSignal.timeout(preflightTimeoutMs) + if (!discover) { + // Production path: discovery owns a bounded helper whose stop must settle + // before a timeout-returning caller resumes (design D2). + const tracked = createTrackedDiscovery() + try { + const catalog = await withinPreflightTimeout(tracked.discover(plan.target.directory, timeout), timeout, tracked.cancel) + validatePreflightTargets(targets, catalog) + } finally { + await tracked.cancel() + } + return + } const catalog = await withinPreflightTimeout(discover(plan.target.directory, timeout), timeout) validatePreflightTargets(targets, catalog) } -async function discoverProviderCatalog(directory: string, signal: AbortSignal): Promise { +/** + * Discovery owns a bounded helper server. The timeout path must not return + * while that server is still shutting down (design D2): the helper handle is + * recorded here and cancelled before the timeout rejection reaches the caller. + */ +export function createTrackedDiscovery(): { discover: PreflightDiscovery; cancel: () => Promise } { + let handle: OpencodeHandle | undefined + return { + discover: (directory, signal) => discoverProviderCatalog(directory, signal, (next) => (handle = next)), + cancel: async () => { + await handle?.stop() + handle = undefined + }, + } +} + +async function discoverProviderCatalog( + directory: string, + signal: AbortSignal, + track?: (handle: OpencodeHandle) => void, +): Promise { const handle = await startOpencode({}, signal) + track?.(handle) try { // Runs use the classic session API, whose provider catalog owns the // credential connections and exact model IDs accepted by session.prompt. @@ -26,16 +59,51 @@ async function discoverProviderCatalog(directory: string, signal: AbortSignal): if (providerResult.error || !providerResult.data) throw new Error("OpenCode could not list connected providers and models") return providerResult.data } finally { - handle.close() + await handle.close() } } -function withinPreflightTimeout(operation: Promise, signal: AbortSignal): Promise { +/** + * Races the operation against its timeout. On timeout the cancel hook runs to + * completion *before* the rejection settles, so the caller never returns ahead + * of the bounded owned-server stop — and never awaits the original request. + * The operation's own `finally` also closes the handle; the stop is idempotent. + */ +export function withinPreflightTimeout( + operation: Promise, + signal: AbortSignal, + cancel?: () => Promise, +): Promise { if (signal.aborted) return Promise.reject(new Error("OpenCode preflight timed out")) - return Promise.race([ - operation, - new Promise((_, reject) => { - signal.addEventListener("abort", () => reject(new Error("OpenCode preflight timed out")), { once: true }) - }), - ]) + return new Promise((resolve, reject) => { + let settled = false + signal.addEventListener( + "abort", + () => { + if (settled) return + settled = true + void (async () => { + try { + await cancel?.() + } catch { + // Cleanup failure never masks the timeout itself. + } + reject(new Error("OpenCode preflight timed out")) + })() + }, + { once: true }, + ) + operation.then( + (value) => { + if (settled) return + settled = true + resolve(value) + }, + (error) => { + if (settled) return + settled = true + reject(error) + }, + ) + }) } diff --git a/src/process-identity.ts b/src/process-identity.ts new file mode 100644 index 0000000..321dc5c --- /dev/null +++ b/src/process-identity.ts @@ -0,0 +1,306 @@ +/** + * Process identity for Convoy-managed OpenCode servers (change + * `fix-opencode-server-lifecycle`, design D1/D5). + * + * A PID alone is never enough to authorize a signal: PIDs are reused, and the + * kernel tells us nothing durable about which incarnation a PID names. An + * identity therefore pairs the PID with a *kernel-derived birth identity* + * (start time plus a boot discriminator), the owning UID, and the executable + * observed for that process. Any adapter that cannot produce every one of + * those facts returns `unknown`; unknown is never a destructive target. + * + * Platform support is loaded lazily so an unsupported host pays nothing until + * a managed launch actually needs identity — and never at import time. + */ + +import { readFile, readlink } from "node:fs/promises" + +/** How a Convoy-managed server's lifetime is owned (design D1). */ +export type LifetimeClass = "run" | "helper" | "authoring-service" + +/** Kernel-derived identity of one process incarnation. */ +export type ProcessIdentity = { + pid: number + /** Opaque, comparable birth token: `:` (never `Date.now()`). */ + birth: string + /** Real UID observed for the process. */ + uid: number + /** Basename of the observed executable, used as a role check. */ + executable: string +} + +export type IdentityObservation = + | { status: "alive"; identity: ProcessIdentity } + | { status: "gone" } + /** The PID exists but is demonstrably not the recorded incarnation. */ + | { status: "mismatch"; reason: string } + /** The platform could not answer; never authorizes a signal. */ + | { status: "unknown"; reason: string } + +export type IdentityProbe = { + /** Observe the current incarnation of `pid`. Never throws. */ + observe(pid: number): Promise +} + +/** True when `observed` is the exact recorded incarnation of `expected`. */ +export function sameIdentity(expected: ProcessIdentity, observed: ProcessIdentity): boolean { + return expected.pid === observed.pid && expected.birth === observed.birth && expected.uid === observed.uid +} + +/** + * Why `observed` does not match `expected`, or undefined when it does. The + * executable is a role hint: a kernel birth mismatch alone already + * disqualifies the target. + */ +export function identityMismatchReason(expected: ProcessIdentity, observed: ProcessIdentity): string | undefined { + if (observed.pid !== expected.pid) return `pid changed from ${expected.pid} to ${observed.pid}` + if (observed.birth !== expected.birth) return `pid ${expected.pid} was reused (birth ${observed.birth} ≠ recorded ${expected.birth})` + if (observed.uid !== expected.uid) return `uid changed from ${expected.uid} to ${observed.uid}` + if (observed.executable !== expected.executable) return `executable changed from ${expected.executable} to ${observed.executable}` + return undefined +} + +/** Identity probe unavailable: every observation is `unknown`. */ +export const unknownIdentityProbe: IdentityProbe = { + async observe(pid) { + return { status: "unknown", reason: `process identity probing is unavailable on this platform (pid ${pid})` } + }, +} + +function executableBasename(path: string): string { + const trimmed = path.replace(/\/+$/, "") + const index = trimmed.lastIndexOf("/") + return index === -1 ? trimmed : trimmed.slice(index + 1) +} + +async function readText(path: string): Promise { + try { + return await readFile(path, "utf8") + } catch { + return undefined + } +} + +/** + * How `kill(pid, 0)` classifies a PID: ESRCH proves the process is absent; + * success or EPERM proves it exists (unreadable or not). + */ +export type ExistenceCheck = (pid: number) => "absent" | "present" + +/** + * Distinguishes "no such process" from "exists but unreadable" via + * `kill(pid, 0)`. Probe/permission failures must never become kill authority + * (design D5): only kernel-level absence may report `gone`. + */ +function existenceByKill0(pid: number): "absent" | "present" { + try { + process.kill(pid, 0) + return "present" + } catch (error) { + return (error as NodeJS.ErrnoException).code === "ESRCH" ? "absent" : "present" + } +} + +// --------------------------------------------------------------------------- +// Linux: /proc-derived identity. +// --------------------------------------------------------------------------- + +/** + * Parses `/proc//stat`. The comm field is parenthesized and may contain + * spaces and parentheses, so split on the *last* `)` and index the remaining + * fields from there: `rest[0]` is field 3 (state), making starttime (field 22) + * `rest[19]` and ppid (field 4) `rest[1]`. + */ +export function parseProcStat(stat: string): { ppid: number; startTicks: number } | undefined { + const close = stat.lastIndexOf(")") + if (close === -1) return undefined + const rest = stat.slice(close + 1).trim().split(/\s+/) + const startTicks = Number(rest[19]) + const ppid = Number(rest[1]) + if (!Number.isInteger(startTicks) || startTicks <= 0) return undefined + if (!Number.isInteger(ppid)) return undefined + return { ppid, startTicks } +} + +/** Parses the first `Uid:` line of `/proc//status` (real uid first). */ +export function parseProcStatusUid(status: string): number | undefined { + const match = status.match(/^Uid:\s+(\d+)/m) + if (!match?.[1]) return undefined + const uid = Number(match[1]) + return Number.isInteger(uid) ? uid : undefined +} + +export async function linuxProcessIdentity(pid: number, exists: ExistenceCheck = existenceByKill0): Promise { + if (!Number.isInteger(pid) || pid <= 0) return { status: "unknown", reason: `invalid pid ${pid}` } + const stat = await readText(`/proc/${pid}/stat`) + if (stat === undefined) { + // /proc being unmounted or unreadable means the platform cannot answer. + if ((await readText("/proc/self/stat")) === undefined) { + return { status: "unknown", reason: "/proc is not readable" } + } + // A missing /proc/ usually means the process is gone, but a + // permission failure (e.g. hidepid) looks identical. `kill(pid, 0)` + // disambiguates: only ESRCH proves absence; anything else is uncertain. + if (exists(pid) === "absent") return { status: "gone" } + return { status: "unknown", reason: `/proc/${pid} could not be read` } + } + const parsed = parseProcStat(stat) + if (!parsed) return { status: "unknown", reason: `could not parse /proc/${pid}/stat` } + const [boot, status, exe] = await Promise.all([ + readText("/proc/sys/kernel/random/boot_id"), + readText(`/proc/${pid}/status`), + readSymlink(`/proc/${pid}/exe`), + ]) + const uid = status === undefined ? undefined : parseProcStatusUid(status) + if (!boot || uid === undefined || exe === undefined) { + return { status: "unknown", reason: `incomplete /proc evidence for pid ${pid}` } + } + return { + status: "alive", + identity: { pid, birth: `${boot.trim()}:${parsed.startTicks}`, uid, executable: executableBasename(exe) }, + } +} + +async function readSymlink(path: string): Promise { + try { + return await readlink(path) + } catch { + return undefined + } +} + +// --------------------------------------------------------------------------- +// macOS: libproc + kernel boot time. +// --------------------------------------------------------------------------- + +/** Offsets into `struct proc_bsdinfo` (Darwin). Verified against the header. */ +const BSDINFO_SIZE = 136 +const BSDINFO_PID = 12 +const BSDINFO_UID = 20 +const BSDINFO_START_SEC = 120 +const BSDINFO_START_USEC = 128 +const PROC_PIDTBSDINFO = 3 + +type MacosLibproc = { + proc_pidinfo(pid: number, flavor: number, arg: bigint, buffer: Uint8Array, size: number): number + proc_pidpath(pid: number, buffer: Uint8Array, size: number): number +} + +let macosLibprocPromise: Promise | undefined + +/** + * Opens libproc on first use. Any failure (non-Bun host, missing library) + * resolves to undefined so observations stay `unknown` rather than throwing. + */ +export async function loadMacosLibproc(): Promise { + macosLibprocPromise ??= (async () => { + try { + const { dlopen, FFIType } = await import("bun:ffi") + const lib = dlopen("libproc.dylib", { + proc_pidinfo: { + args: [FFIType.i32, FFIType.i32, FFIType.u64, FFIType.ptr, FFIType.i32], + returns: FFIType.i32, + }, + proc_pidpath: { + args: [FFIType.i32, FFIType.ptr, FFIType.u32], + returns: FFIType.i32, + }, + }) + return { + proc_pidinfo: (pid, flavor, arg, buffer, size) => lib.symbols.proc_pidinfo(pid, flavor, arg, buffer, size), + proc_pidpath: (pid, buffer, size) => lib.symbols.proc_pidpath(pid, buffer, size), + } + } catch { + return undefined + } + })() + return macosLibprocPromise +} + +/** Test seam: forget the loaded library and cached boot discriminator. */ +export function resetMacosIdentityCaches(): void { + macosLibprocPromise = undefined + cachedBoottime = null +} + +/** Parses `sysctl -n kern.boottime` output (`{ sec = 1, usec = 2 } …`). */ +export function parseBoottime(output: string): string | undefined { + const match = output.match(/sec\s*=\s*(\d+)[^}]*usec\s*=\s*(\d+)/) + if (!match) return undefined + return `${match[1]}.${match[2]}` +} + +let cachedBoottime: string | undefined | null = null + +async function macosBoottime(): Promise { + if (cachedBoottime !== null) return cachedBoottime + try { + const { spawnSync } = await import("node:child_process") + const result = spawnSync("sysctl", ["-n", "kern.boottime"], { encoding: "utf8" }) + cachedBoottime = parseBoottime(result.stdout ?? "") ?? undefined + } catch { + cachedBoottime = undefined + } + return cachedBoottime +} + +export async function macosProcessIdentity(pid: number, libproc?: MacosLibproc, exists: ExistenceCheck = existenceByKill0): Promise { + if (!Number.isInteger(pid) || pid <= 0) return { status: "unknown", reason: `invalid pid ${pid}` } + const lib = libproc ?? (await loadMacosLibproc()) + if (!lib) return { status: "unknown", reason: "libproc could not be loaded" } + + const buffer = new Uint8Array(BSDINFO_SIZE) + const written = lib.proc_pidinfo(pid, PROC_PIDTBSDINFO, 0n, buffer, BSDINFO_SIZE) + if (written !== BSDINFO_SIZE) { + // ESRCH (no such process) and EPERM (another user's process) both land + // here, and neither alone proves a specific incarnation. Only + // kernel-level absence (`kill(pid, 0)` → ESRCH) may report `gone`; + // every other failure is uncertain and never kill authority (design D5). + if (exists(pid) === "absent") return { status: "gone" } + return { status: "unknown", reason: `proc_pidinfo could not read pid ${pid}` } + } + const view = new DataView(buffer.buffer) + const observedPid = view.getUint32(BSDINFO_PID, true) + if (observedPid !== pid) return { status: "unknown", reason: `libproc returned pid ${observedPid} for ${pid}` } + const uid = view.getUint32(BSDINFO_UID, true) + const startSec = view.getBigUint64(BSDINFO_START_SEC, true) + const startUsec = view.getBigUint64(BSDINFO_START_USEC, true) + if (startSec === 0n) return { status: "unknown", reason: `libproc returned no start time for pid ${pid}` } + const boot = await macosBoottime() + if (!boot) return { status: "unknown", reason: "kernel boot time is unavailable" } + + const pathBuffer = new Uint8Array(4096) + const pathLength = lib.proc_pidpath(pid, pathBuffer, pathBuffer.length) + if (pathLength <= 0) return { status: "unknown", reason: `libproc could not read the executable for pid ${pid}` } + const executable = executableBasename(new TextDecoder().decode(pathBuffer.subarray(0, pathLength))) + + return { status: "alive", identity: { pid, birth: `${boot}:${startSec}.${startUsec}`, uid, executable } } +} + +// --------------------------------------------------------------------------- +// Platform dispatch. +// --------------------------------------------------------------------------- + +/** + * Captures a just-spawned child's identity. macOS `proc_pidinfo` can lag the + * spawn by a scheduling tick, so this retries briefly; giving up is always + * non-destructive. `gone` is only retried while attempts remain. + */ +export async function captureIdentity(pid: number, probe: IdentityProbe, attempts = 20): Promise { + for (let attempt = 0; attempt < attempts; attempt++) { + const observation = await probe.observe(pid) + if (observation.status === "alive") return observation.identity + if (observation.status === "gone" && attempt < attempts - 1) { + await new Promise((resolve) => setTimeout(resolve, 25)) + continue + } + return undefined + } + return undefined +} + +export function defaultIdentityProbe(platform: NodeJS.Platform = process.platform): IdentityProbe { + if (platform === "linux") return { observe: (pid) => linuxProcessIdentity(pid) } + if (platform === "darwin") return { observe: (pid) => macosProcessIdentity(pid) } + return unknownIdentityProbe +} diff --git a/src/process-records.ts b/src/process-records.ts new file mode 100644 index 0000000..3454f6d --- /dev/null +++ b/src/process-records.ts @@ -0,0 +1,446 @@ +/** + * Durable, private lifecycle records for Convoy-managed OpenCode servers + * (change `fix-opencode-server-lifecycle`, design D5). + * + * These records are transient execution evidence, not worktree ownership and + * not a feature registry. They live under `~/.convoy/processes/`, outside + * disposable run workspaces and outside `pending/`, so ordinary workspace or + * pending cleanup cannot erase an unresolved child. They never carry + * credentials, prompts, full environment, or config: only process identity, + * lifetime class, lifecycle state, and a bounded outcome string. + * + * Reconciliation only ever signals a recorded child once the original owner + * incarnation is provably gone *and* the target still matches the recorded + * child incarnation and executable role, revalidated immediately before each + * destructive transition. + * + * Residual risk (design D5): that revalidation narrows but does not eliminate + * the portable POSIX time-of-check/time-of-use window between an identity + * probe and the delivered signal, especially on macOS. Reconciliation is a + * best-effort lifecycle cleanup, never an atomic security boundary, and it is + * never a reason to signal a PID that this recovery did not record. Unknown, + * unreadable, or mismatched evidence stays non-destructive. + */ + +import { mkdir, open, readFile, readdir, unlink, writeFile } from "node:fs/promises" +import { join } from "node:path" + +import { log } from "./log" +import { + captureIdentity, + defaultIdentityProbe, + identityMismatchReason, + sameIdentity, + type IdentityProbe, + type ProcessIdentity, +} from "./process-identity" +import { observedStopTarget, stopTarget, type StopOutcome, type StopPolicy } from "./process-stop" +import { isFound, readJsonFile, writeJsonFile } from "./repo-store" +import { convoyHome } from "./workspace" + +export const PROCESS_RECORD_VERSION = 1 + +/** Only owned lifetimes participate in orphan reconciliation (design D1). */ +export type RecordLifetime = "run" | "helper" + +export type ProcessRecordState = "provisional" | "ready" | "stopping" | "unresolved" + +export type ProcessRecord = { + version: typeof PROCESS_RECORD_VERSION + id: string + lifetime: RecordLifetime + state: ProcessRecordState + createdAt: number + updatedAt: number + /** The process that owns the child's lifetime. Absent until captured. */ + owner?: ProcessIdentity + /** The recorded server child. Absent in the pre-spawn provisional record. */ + child?: ProcessIdentity + runId?: string + url?: string + /** Bounded last-stop outcome; never secrets, env, or prompts. */ + lastOutcome?: string +} + +export function processRecordsDir(home = convoyHome()): string { + return join(home, "processes") +} + +export function newProcessRecord(input: { lifetime: RecordLifetime; id?: string; now?: number }): ProcessRecord { + const now = input.now ?? Date.now() + return { + version: PROCESS_RECORD_VERSION, + id: input.id ?? crypto.randomUUID(), + lifetime: input.lifetime, + state: "provisional", + createdAt: now, + updatedAt: now, + } +} + +/** Validator for one stored record, matching the repo's `validate` store convention. */ +export function validateProcessRecord(value: unknown): ProcessRecord | undefined { + if (!value || typeof value !== "object") return undefined + const record = value as Partial + if (record.version !== PROCESS_RECORD_VERSION) return undefined + if (typeof record.id !== "string") return undefined + if (record.lifetime !== "run" && record.lifetime !== "helper") return undefined + if (typeof record.createdAt !== "number") return undefined + return value as ProcessRecord +} + +export type ProcessRecordStore = { + dir: string + put(record: ProcessRecord): Promise + get(id: string): Promise + remove(id: string): Promise + list(): Promise + /** Runs `fn` under the record's exclusive lock. Returns undefined when the lock is contended. */ + withLock( + id: string, + fn: () => Promise, + opts?: { timeoutMs?: number; probe?: IdentityProbe; owner?: ProcessIdentity }, + ): Promise + readCursor(): Promise + writeCursor(id: string | undefined): Promise +} + +const recordPath = (dir: string, id: string) => join(dir, `${id}.json`) +const lockPath = (dir: string, id: string) => join(dir, `${id}.lock`) + +export function createProcessRecordStore(dir = processRecordsDir()): ProcessRecordStore { + return { + dir, + async put(record) { + // 0700 keeps the evidence directory private; `writeJsonFile` publishes + // atomically (temp + rename) with 0600, so a reader never sees a torn + // record and the file never passes through a world-readable state. + await mkdir(dir, { recursive: true, mode: 0o700 }) + await writeJsonFile(recordPath(dir, record.id), record, { mode: 0o600 }) + }, + async get(id) { + const read = await readJsonFile(recordPath(dir, id), validateProcessRecord) + return isFound(read) ? read.value : undefined + }, + async remove(id) { + // File-only: a record is one JSON file, and a directory at that path + // must never be recursively deleted by a stop/cleanup path. + await unlink(recordPath(dir, id)).catch(() => {}) + }, + async list() { + let names: string[] + try { + names = await readdir(dir) + } catch { + return [] + } + const records: ProcessRecord[] = [] + for (const name of names) { + if (!name.endsWith(".json")) continue + const parsed = await this.get(name.slice(0, -".json".length)) + if (parsed) records.push(parsed) + } + records.sort((a, b) => a.createdAt - b.createdAt || a.id.localeCompare(b.id)) + return records + }, + async withLock(id, fn, opts = {}) { + const timeoutMs = opts.timeoutMs ?? 200 + const path = lockPath(dir, id) + await mkdir(dir, { recursive: true, mode: 0o700 }) + const deadline = Date.now() + timeoutMs + for (;;) { + try { + const handle = await open(path, "wx", 0o600) + await handle.close() + await writeLockOwner(path, opts.owner) + break + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") return undefined + // Fail closed: never unlink a lock while its owner may be alive. + // A lock whose recorded owner is provably gone may be reclaimed. + const reclaimed = await reclaimDeadLock(path, opts.probe) + if (!reclaimed) { + if (Date.now() >= deadline) return undefined + await sleep(20) + } + } + } + try { + return await fn() + } finally { + await unlink(path).catch(() => {}) + } + }, + async readCursor() { + try { + return (await readFile(join(dir, ".cursor"), "utf8")).trim() || undefined + } catch { + return undefined + } + }, + async writeCursor(id) { + await mkdir(dir, { recursive: true, mode: 0o700 }) + await writeFile(join(dir, ".cursor"), id ?? "", { mode: 0o600 }).catch(() => {}) + }, + } +} + +async function reclaimDeadLock(path: string, probe: IdentityProbe | undefined): Promise { + if (!probe) return false + let info: { pid?: number; birth?: string } + try { + info = JSON.parse(await readFile(path, "utf8")) as { pid?: number; birth?: string } + } catch { + return false + } + if (!info.pid) return false + const observation = await probe.observe(info.pid) + if (observation.status !== "gone") return false + await unlink(path).catch(() => {}) + return true +} + +/** The owner block written into a lock file so a dead holder can be reclaimed. */ +async function writeLockOwner(path: string, identity: ProcessIdentity | undefined): Promise { + await writeFile(path, JSON.stringify({ pid: identity?.pid, birth: identity?.birth }), { mode: 0o600 }).catch(() => {}) +} + +// --------------------------------------------------------------------------- +// Reconciliation +// --------------------------------------------------------------------------- + +export type ReconcileOutcome = { + inspected: number + removed: string[] + stopped: string[] + skipped: { id: string; reason: string }[] + uncertain: { id: string; reason: string }[] + deferred: string[] +} + +export type ReconcileDeps = { + store?: ProcessRecordStore + probe?: IdentityProbe + /** Signals a recorded child by pid; injectable so tests never touch real processes. */ + signal?: (pid: number, signal: "SIGTERM" | "SIGKILL") => void + policy?: StopPolicy + maxRecords?: number + budgetMs?: number + now?: () => number + /** Bounded diagnostic sink; defaults to the structured logger. */ + warn?: (line: string) => void + controllerPid?: number +} + +const defaultReconcileBudgets = { maxRecords: 32, budgetMs: 5_000 } as const + +/** + * Printed with an unresolved recovery so an operator cannot read reconciliation + * as an atomic kill guarantee (design D5): identity is revalidated at each + * destructive edge, but the POSIX probe-to-signal window is inherently racy. + */ +const reconcileToctouNotice = + "identity is rechecked immediately before each signal, but the portable probe-to-signal window is not atomic; treat recovery as best-effort cleanup, not kill authority for unrecorded processes" + +/** + * Bounded pass over recorded run/helper lifetimes. Never throws and never + * fails an unrelated launch: every per-record error becomes a `skipped` or + * `uncertain` entry, and the caller's startup continues. + */ +export async function reconcileProcessRecords(deps: ReconcileDeps = {}): Promise { + const store = deps.store ?? createProcessRecordStore() + const probe = deps.probe ?? defaultIdentityProbe() + const signal = deps.signal ?? ((pid, sig) => process.kill(pid, sig)) + const now = deps.now ?? Date.now + const warn = deps.warn ?? ((line: string) => log.warn(line)) + const maxRecords = deps.maxRecords ?? defaultReconcileBudgets.maxRecords + const budgetMs = deps.budgetMs ?? defaultReconcileBudgets.budgetMs + const deadline = now() + budgetMs + + const outcome: ReconcileOutcome = { inspected: 0, removed: [], stopped: [], skipped: [], uncertain: [], deferred: [] } + const all = await store.list() + if (all.length === 0) return outcome + + // The lock owner is this process, so a crashed reconciler's lock can be + // reclaimed by a later pass without ever unlinking a live worker's lock. + const owner = await captureIdentity(process.pid, probe).catch(() => undefined) + + // Fair cursor: continue after the last inspected record so a long tail of + // uncertain records is never starved and the oldest is never revisited + // forever at the expense of newer ones. + const cursor = await store.readCursor() + const start = cursor ? (all.findIndex((record) => record.id === cursor) + 1) % all.length : 0 + + let lastInspected: ProcessRecord | undefined + let offset = 0 + let budgetExhausted = false + for (; offset < all.length && outcome.inspected < maxRecords; offset++) { + if (now() >= deadline) { + budgetExhausted = true + break + } + const record = all[(start + offset) % all.length]! + lastInspected = record + outcome.inspected++ + try { + await reconcileOne(record, { store, probe, signal, policy: deps.policy, now, warn, ...(owner ? { owner } : {}), outcome }) + } catch (error) { + outcome.uncertain.push({ id: record.id, reason: error instanceof Error ? error.message : String(error) }) + } + } + // Whatever remains unprocessed stays for a later pass, and the cursor now + // points at the next record so the same head is not starved next time. + if (budgetExhausted || offset < all.length) { + for (let index = offset; index < all.length; index++) { + outcome.deferred.push(all[(start + index) % all.length]!.id) + } + } + if (lastInspected) await store.writeCursor(lastInspected.id) + return outcome +} + +async function reconcileOne( + record: ProcessRecord, + context: { + store: ProcessRecordStore + probe: IdentityProbe + signal: (pid: number, signal: "SIGTERM" | "SIGKILL") => void + policy?: StopPolicy + now: () => number + warn: (line: string) => void + owner?: ProcessIdentity + outcome: ReconcileOutcome + }, +): Promise { + const { store, probe, signal, outcome, warn } = context + + await store.withLock( + record.id, + async () => { + // Reread under the lock: another Convoy incarnation may have resolved it. + const current = await store.get(record.id) + if (!current) return + if (current.lifetime !== "run" && current.lifetime !== "helper") { + outcome.skipped.push({ id: current.id, reason: `lifetime ${current.lifetime} is not eligible for recovery` }) + return + } + if (!current.child) { + outcome.uncertain.push({ id: current.id, reason: "record carries no child identity (legacy/incomplete); never a kill target" }) + // An incomplete record can never be resolved automatically; remove it + // only if it is clearly ancient so the registry does not grow forever. + if (context.now() - current.createdAt > 24 * 60 * 60 * 1_000) await store.remove(current.id) + return + } + if (!current.owner) { + outcome.uncertain.push({ id: current.id, reason: "record carries no owner identity; cannot prove owner death" }) + return + } + + // Owner must be provably gone; a different process at the old PID is + // not the original owner, and probe errors are uncertain. + const ownerObservation = await probe.observe(current.owner.pid) + if (ownerObservation.status === "alive") { + if (sameIdentity(current.owner, ownerObservation.identity)) { + outcome.skipped.push({ id: current.id, reason: "owner is still alive; detached execution is preserved" }) + } else { + outcome.skipped.push({ id: current.id, reason: identityMismatchReason(current.owner, ownerObservation.identity) ?? "owner pid was reused" }) + } + return + } + if (ownerObservation.status === "unknown") { + outcome.uncertain.push({ id: current.id, reason: `owner identity could not be probed: ${ownerObservation.reason}` }) + return + } + + // The target must still be the recorded child incarnation. + const childObservation = await probe.observe(current.child.pid) + if (childObservation.status === "gone") { + await store.remove(current.id) + outcome.removed.push(current.id) + return + } + if (childObservation.status === "unknown") { + outcome.uncertain.push({ id: current.id, reason: `child identity could not be probed: ${childObservation.reason}` }) + return + } + if (childObservation.status === "mismatch") { + outcome.skipped.push({ id: current.id, reason: childObservation.reason }) + return + } + if (!sameIdentity(current.child, childObservation.identity)) { + outcome.skipped.push({ + id: current.id, + reason: identityMismatchReason(current.child, childObservation.identity) ?? "child pid was reused", + }) + return + } + + // Revalidate immediately before each destructive transition: TERM and + // KILL both go through this verify hook. + const verify = async (): Promise<{ ok: true } | { ok: false; reason: string }> => { + const observed = await probe.observe(current.child!.pid) + if (observed.status === "gone") return { ok: true } + if (observed.status !== "alive") return { ok: false, reason: `child identity became ${observed.status}` } + return sameIdentity(current.child!, observed.identity) ? { ok: true } : { ok: false, reason: "child incarnation changed before escalation" } + } + + const target = observedStopTarget({ + pid: current.child.pid, + observe: async () => { + const observed = await probe.observe(current.child!.pid) + return observed.status === "mismatch" ? "unknown" : observed.status + }, + signal: (sig) => signal(current.child!.pid, sig), + verify, + }) + const stop = await stopTarget(target, context.policy) + + if (stop.status === "stopped") { + await store.remove(current.id) + outcome.stopped.push(current.id) + outcome.removed.push(current.id) + warn( + `[processes] recovered orphan ${current.lifetime} server pid ${current.child.pid} (${stop.via}); record ${current.id} removed`, + ) + } else { + const stopping: ProcessRecord = { + ...current, + state: "unresolved", + updatedAt: context.now(), + lastOutcome: boundedOutcome(stop), + } + await store.put(stopping) + outcome.uncertain.push({ id: current.id, reason: stop.reason }) + warn(`[processes] could not confirm stop for record ${current.id} at ${store.dir}; retained for a later pass: ${stop.reason}. ${reconcileToctouNotice}`) + } + }, + { probe, ...(context.owner ? { owner: context.owner } : {}) }, + ) +} + +function boundedOutcome(outcome: StopOutcome): string { + const text = outcome.status === "stopped" ? `stopped (${outcome.via})` : `unresolved: ${outcome.reason}` + return text.slice(0, 400) +} + +/** + * Publishes a record's child identity before readiness and returns it. A + * record that cannot be persisted is a launch failure (fail closed): the + * caller must fall back to bounded cleanup through the owned child handle. + */ +export async function publishChildIdentity( + store: ProcessRecordStore, + record: ProcessRecord, + pid: number, + probe: IdentityProbe, +): Promise<{ record: ProcessRecord; identity: ProcessIdentity } | undefined> { + const identity = await captureIdentity(pid, probe) + if (!identity) return undefined + const next: ProcessRecord = { ...record, child: identity, state: "ready", updatedAt: Date.now() } + await store.put(next) + return { record: next, identity } +} + +async function sleep(ms: number): Promise { + await new Promise((resolve) => setTimeout(resolve, ms)) +} diff --git a/src/process-stop.ts b/src/process-stop.ts new file mode 100644 index 0000000..ab9d27d --- /dev/null +++ b/src/process-stop.ts @@ -0,0 +1,184 @@ +/** + * Bounded process-stop state machine shared by owned children and orphan + * reconciliation (change `fix-opencode-server-lifecycle`, design D2/D5). + * + * A delivered signal is never a confirmed stop. Every stop attempt observes + * the outcome within a finite budget: graceful termination first, forced + * escalation after the grace window, then a bounded observation window. The + * state machine is idempotent — concurrent callers share one outcome — and it + * never signals a target it cannot re-verify immediately before escalation. + */ + +export type StopOutcome = + | { status: "stopped"; via: "already-gone" | "graceful" | "forced" } + | { status: "unresolved"; reason: string } + +export type StopPolicy = { + /** Graceful window after SIGTERM (default 2s). */ + graceMs?: number + /** Observation window after SIGKILL (default 1s). */ + forceObservationMs?: number + /** Poll interval for pid-observed targets (default 50ms). */ + pollMs?: number +} + +export const defaultStopPolicy = { graceMs: 2_000, forceObservationMs: 1_000, pollMs: 50 } as const + +export type StopTarget = { + pid: number + /** True once the target is known to have exited. */ + isExited(): boolean + /** Resolves true when exit is observed within `timeoutMs`. */ + waitForExit(timeoutMs: number): Promise + /** Best-effort signal delivery. May throw; the caller rechecks exit after. */ + signal(signal: "SIGTERM" | "SIGKILL"): void + /** + * Revalidates that this is still the recorded incarnation. Called before + * forced escalation; a false result abandons the escalation (never signal a + * process that may have been replaced). + */ + verify?(): Promise<{ ok: true } | { ok: false; reason: string }> + /** Records bounded diagnostic evidence when the outcome is unresolved. */ + onUnresolved?(reason: string): void +} + +/** + * Runs the bounded stop algorithm. Never throws: signal/probe failures become + * an `unresolved` outcome with a bounded reason. + */ +export async function stopTarget(target: StopTarget, policy: StopPolicy = {}): Promise { + const graceMs = policy.graceMs ?? defaultStopPolicy.graceMs + const forceObservationMs = policy.forceObservationMs ?? defaultStopPolicy.forceObservationMs + + if (target.isExited()) return { status: "stopped", via: "already-gone" } + + // A signal error (ESRCH) is only meaningful after rechecking exit: if the + // target is gone the stop succeeded, otherwise the failure is real. + const alreadyGoneAfter = (): StopOutcome | undefined => (target.isExited() ? { status: "stopped", via: "already-gone" } : undefined) + + try { + target.signal("SIGTERM") + } catch (error) { + const settled = alreadyGoneAfter() + if (settled) return settled + return unresolved(target, `could not deliver SIGTERM: ${describeError(error)}`) + } + + if (await target.waitForExit(graceMs)) return { status: "stopped", via: "graceful" } + if (target.isExited()) return { status: "stopped", via: "graceful" } + + if (target.verify) { + const verified = await target.verify() + if (!verified.ok) return unresolved(target, `refused forced escalation: ${verified.reason}`) + } + + try { + target.signal("SIGKILL") + } catch (error) { + const settled = alreadyGoneAfter() + if (settled) return settled + return unresolved(target, `could not deliver SIGKILL: ${describeError(error)}`) + } + + if (await target.waitForExit(forceObservationMs)) return { status: "stopped", via: "forced" } + if (target.isExited()) return { status: "stopped", via: "forced" } + return unresolved(target, `termination was requested but exit was not observed within the shutdown budget`) +} + +function unresolved(target: StopTarget, reason: string): StopOutcome { + target.onUnresolved?.(reason) + return { status: "unresolved", reason } +} + +/** + * A `StopTarget` for a `node:child_process` child Convoy owns: exit is + * observed through the child API rather than by polling a PID. + */ +export function childStopTarget(options: { + child: { + pid?: number + kill(signal?: NodeJS.Signals | number): boolean + once(event: "exit", listener: () => void): unknown + removeListener(event: "exit", listener: () => void): unknown + } + pid: number + /** Optional re-verification hook before forced escalation. */ + verify?: StopTarget["verify"] + onUnresolved?: (reason: string) => void +}): StopTarget { + let exited = false + const listeners = new Set<() => void>() + const onExit = () => { + exited = true + for (const listener of [...listeners]) listener() + } + options.child.once("exit", onExit) + return { + pid: options.pid, + isExited: () => exited, + waitForExit: (timeoutMs) => + new Promise((resolve) => { + if (exited) { + resolve(true) + return + } + let settled = false + const done = (value: boolean) => { + if (settled) return + settled = true + listeners.delete(onExitListener) + clearTimeout(timer) + resolve(value) + } + const onExitListener = () => done(true) + listeners.add(onExitListener) + const timer = setTimeout(() => done(false), timeoutMs) + timer.unref?.() + }), + signal: (signal) => { + options.child.kill(signal) + }, + ...(options.verify ? { verify: options.verify } : {}), + ...(options.onUnresolved ? { onUnresolved: options.onUnresolved } : {}), + } +} + +/** + * A `StopTarget` for a recorded PID observed through the injected probe — the + * reconciliation path, where no child handle exists. + */ +export function observedStopTarget(options: { + pid: number + observe: () => Promise<"alive" | "gone" | "unknown"> + signal: (signal: "SIGTERM" | "SIGKILL") => void + pollMs?: number + verify?: StopTarget["verify"] + onUnresolved?: (reason: string) => void +}): StopTarget { + const pollMs = options.pollMs ?? defaultStopPolicy.pollMs + let exited = false + return { + pid: options.pid, + isExited: () => exited, + waitForExit: async (timeoutMs) => { + const deadline = Date.now() + timeoutMs + for (;;) { + const observation = await options.observe() + if (observation === "gone") { + exited = true + return true + } + if (Date.now() >= deadline) return false + await new Promise((resolve) => setTimeout(resolve, Math.min(pollMs, Math.max(0, deadline - Date.now())))) + } + }, + signal: options.signal, + ...(options.verify ? { verify: options.verify } : {}), + ...(options.onUnresolved ? { onUnresolved: options.onUnresolved } : {}), + } +} + +function describeError(error: unknown): string { + if (error instanceof Error) return error.message + return String(error) +} diff --git a/src/propose-service.ts b/src/propose-service.ts new file mode 100644 index 0000000..b546fff --- /dev/null +++ b/src/propose-service.ts @@ -0,0 +1,132 @@ +/** + * Proposal-command ownership (change `fix-opencode-server-lifecycle`, design + * D1/D6). + * + * The proposal fallback boots a temporary, run/helper-classified `opencode + * serve` to discover the project's authoring commands. That helper must never + * run the command: an authoring command can start independent execution, so it + * runs under the repository's independently persistent conversation service + * (published or reused under that service's discovery lock). The unused helper + * is stopped on every path — unknown discovery, no usable command, a lost + * writer claim, transfer failure, and transfer success — so recovery can never + * mistake active authoring for an abandoned helper. + * + * Extracted from `cli.ts` so the ordering (transfer strictly before command + * invocation) and the guaranteed helper stop are unit-testable without booting + * a real server or TUI. + */ + +export type AuthoringServiceHandle = { url: string } + +export type AuthoringConversationRef = { harness: "opencode"; sessionId: string } + +/** The server availability the caller resolved before invoking the workflow. */ +export type ProposalServer = + | { kind: "independent"; url: string } + | { kind: "helper"; url: string } + +export type ProposalCommandDeps = { + /** The project's authoring commands on a server; `"unknown"` means unreadable. */ + listCommands: (server: AuthoringServiceHandle) => Promise + /** Publishes or reuses the repository's independent authoring service. */ + transfer: () => Promise<{ status: "live"; url: string } | { status: "unavailable" | "uncertain"; reason: string }> + /** Stops the owned temporary helper; idempotent and only called for a helper. */ + stopHelper: () => Promise + /** Takes the checkout's managed-writer claim; `ok: false` carries the guidance. */ + acquireClaim: () => Promise<{ ok: true } | { ok: false; reason: string; remediation: string[] }> + /** Releases a claim this operation took on a failure path. */ + releaseClaim: () => Promise + createConversation: (server: AuthoringServiceHandle) => Promise + invokeCommand: (input: { ref: AuthoringConversationRef; server: AuthoringServiceHandle; command: string }) => Promise +} + +export type ProposalCommandOutcome = + | { status: "started"; ref: AuthoringConversationRef; service: AuthoringServiceHandle } + | { status: "blocked"; reason: string; remediation: string[] } + +const checkCommandsRemediation = ["check the project's .opencode/commands/ directory — Convoy does not install commands into it"] +const manualRemediation = ["open an ordinary conversation in the worktree instead"] +const discoverCommandRemediation = ["author the change manually in a conversation"] + +/** The proposal workflow command the fallback looks for. */ +export function findProposalCommand(commands: readonly string[]): string | undefined { + return commands.find((name) => name === "opsx-propose") ?? commands.find((name) => name.endsWith("propose")) +} + +/** + * Runs the full proposal workflow under the correct process ownership: + * discovery → writer claim → (helper) transfer to the independent service → + * conversation creation → command invocation. A temporary helper is stopped on + * every path that does not need it, including just before a command runs under + * the transferred service. + */ +export async function startProposalCommand(server: ProposalServer, deps: ProposalCommandDeps): Promise { + const owned = server.kind === "helper" + let helperStopped = false + const stopOwnedHelper = async () => { + if (!owned || helperStopped) return + helperStopped = true + await deps.stopHelper() + } + + // A function-level finally guarantees the owned helper is stopped on every + // path, including a `listCommands` or `acquireClaim` that throws before the + // post-claim try below (design D1). `stopOwnedHelper` is idempotent, so the + // explicit calls on the return paths remain the primary stop and this is the + // safety net for a throw. + try { + const commands = await deps.listCommands({ url: server.url }) + if (commands === "unknown") { + await stopOwnedHelper() + return { status: "blocked", reason: "the project's authoring commands could not be discovered", remediation: checkCommandsRemediation } + } + const command = findProposalCommand(commands) + if (!command) { + await stopOwnedHelper() + return { + status: "blocked", + reason: "this project has no supported proposal workflow command (looked for opsx-propose under .opencode/commands/)", + remediation: discoverCommandRemediation, + } + } + + const claim = await deps.acquireClaim() + if (!claim.ok) { + await stopOwnedHelper() + return { status: "blocked", reason: claim.reason, remediation: claim.remediation } + } + + let service: AuthoringServiceHandle = { url: server.url } + try { + if (owned) { + // The helper may only discover. Publish or reuse the independent service + // before any command runs, then stop the helper whether the transfer + // succeeded or not — it is never the command's owner. + const transfer = await deps.transfer() + await stopOwnedHelper() + if (transfer.status !== "live") { + await deps.releaseClaim() + return { + status: "blocked", + reason: `the authoring service could not be established independently: ${transfer.reason}`, + remediation: manualRemediation, + } + } + service = { url: transfer.url } + } + const ref = await deps.createConversation(service) + await deps.invokeCommand({ ref, server: service, command }) + return { status: "started", ref, service } + } catch (error) { + await stopOwnedHelper() + await deps.releaseClaim() + return { + status: "blocked", + reason: `the authoring workflow could not start: ${error instanceof Error ? error.message : String(error)}`, + remediation: manualRemediation, + } + } + } finally { + await stopOwnedHelper() + } +} diff --git a/src/runner.ts b/src/runner.ts index ee9acb9..b850881 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -28,6 +28,7 @@ import { log } from "./log" import { LoopGuard, LoopGuardError, observationFromSessionEvent, resolveLoopGuard, type LoopGuardConfig } from "./loop-guard" import { openRunMetadata, recordProgress, type RunMetadataStore } from "./metadata" import { openOpencodeSessionWindow, startOpencode } from "./opencode" +import { defaultStopPolicy, type StopOutcome, type StopPolicy } from "./process-stop" import { HerdrReporter } from "./herdr" import { defaultNotificationSettings, Notifier } from "./notifications" import { startPermissionGate, type PermissionGate } from "./permissions" @@ -99,12 +100,36 @@ export function isIgnorableRejection(reason: unknown): boolean { return false } +/** + * Injected seams for the shutdown state machine. `exit` lets tests assert the + * force path without killing the runner; `graceMs` and `forceObservationMs` + * let a test exercise the deadline/observation timers without waiting the + * production 15s + 1s budgets (design D2: tests inject timers and seams). + */ +export type RunShutdownDeps = { + exit?: (code: number) => void + /** Overall cleanup deadline before the forced owned-server edge; default 15s. */ + graceMs?: number + /** Observation window after the forced edge before exiting; default 1s. */ + forceObservationMs?: number + /** Injectable clock so the shared stop budget is deterministically testable. */ + now?: () => number +} + export class RunShutdown { private readonly controller = new AbortController() private readonly activeSessions = new Map() private abortingSessions: Promise | undefined private requests = 0 private forceTimer: ReturnType | undefined + private exitTimer: ReturnType | undefined + private forceHandler?: () => void + private forcing = false + private readonly exit: (code: number) => void + private readonly graceMs: number + private readonly forceObservationMs: number + private readonly now: () => number + private requestedAt: number | undefined /** * Set by {@link dispose} so a signal routed to a shutdown whose loop already * exited (a borrowed dashboard's abort handler still pointing at it) is a @@ -114,6 +139,13 @@ export class RunShutdown { */ private disposed = false + constructor(deps: RunShutdownDeps = {}) { + this.exit = deps.exit ?? ((code) => process.exit(code)) + this.graceMs = deps.graceMs ?? 15_000 + this.forceObservationMs = deps.forceObservationMs ?? 1_000 + this.now = deps.now ?? Date.now + } + get signal() { return this.controller.signal } @@ -122,23 +154,70 @@ export class RunShutdown { return this.controller.signal.aborted } + /** + * Registers the owned-server force edge. It runs synchronously when a + * repeated abort or the shutdown deadline arrives, immediately before the + * bounded last-resort exit, so a wedged cleanup cannot abandon the child + * (design D3). + */ + setForceHandler(handler: (() => void) | undefined) { + this.forceHandler = handler + } + request(source: string) { if (this.disposed) return this.requests++ if (this.requests > 1) { - log.warn(`${source} received again; forcing exit`) - process.exit(130) + log.warn(`${source} received again; forcing owned-server cleanup before exit`) + this.forceShutdown() + return } log.warn(`${source} received; aborting active OpenCode session(s) and shutting down`) + this.requestedAt = this.now() this.controller.abort(new UserAbortError(`${source} received`)) this.forceTimer = setTimeout(() => { - log.warn("Shutdown cleanup timed out; forcing exit") - process.exit(130) - }, 15_000) + log.warn("Shutdown cleanup timed out; forcing owned-server cleanup before exit") + this.forceShutdown() + }, this.graceMs) this.forceTimer.unref?.() } + /** + * The shared cleanup budget a helper owned by this shutdown must respect + * (design D2): helpers running under a coordinator draw from its remaining + * global deadline instead of starting a fresh standalone allowance. Before a + * shutdown is requested this returns the standalone defaults; once requested + * it clamps the graceful window and the forced-observation window so their + * sum never exceeds the time left before the forced edge — and therefore + * never restarts or extends the coordinator's budget. + */ + stopBudget(): StopPolicy { + if (this.requestedAt === undefined) return {} + const remaining = Math.max(0, this.graceMs - (this.now() - this.requestedAt)) + const graceMs = Math.min(defaultStopPolicy.graceMs, remaining) + const forceObservationMs = Math.min(this.forceObservationMs, Math.max(0, remaining - graceMs)) + return { graceMs, forceObservationMs } + } + + /** + * Idempotent last resort: deliver the synchronous SIGKILL edge to owned + * servers, observe for at most the reserved second, then exit with the + * existing abort code. Repeated signals while forcing are no-ops rather than + * new immediate exits, so a burst of signals cannot skip the server stop. + */ + private forceShutdown() { + if (this.forcing || this.disposed) return + this.forcing = true + try { + this.forceHandler?.() + } catch (error) { + log.warn(`couldn't force owned-server cleanup: ${String(error)}`) + } + this.exitTimer = setTimeout(() => this.exit(130), this.forceObservationMs) + this.exitTimer.unref?.() + } + throwIfRequested() { if (this.aborted) throw this.abortError() } @@ -185,6 +264,7 @@ export class RunShutdown { dispose() { if (this.forceTimer) clearTimeout(this.forceTimer) + if (this.exitTimer) clearTimeout(this.exitTimer) this.disposed = true } } @@ -385,11 +465,23 @@ export function createGitLock(): GitLock { export function installShutdownSignals(shutdown: RunShutdown) { // Bun delivers the numeric signal value to handlers; normalize for logs. - const handler = (signal: NodeJS.Signals | number) => - shutdown.request(typeof signal === "number" ? (signal === 15 ? "SIGTERM" : signal === 2 ? "SIGINT" : `signal ${signal}`) : signal) + const nameOf = (signal: NodeJS.Signals | number) => { + if (typeof signal !== "number") return signal + if (signal === 1) return "SIGHUP" + if (signal === 2) return "SIGINT" + if (signal === 15) return "SIGTERM" + return `signal ${signal}` + } + const handler = (signal: NodeJS.Signals | number) => shutdown.request(nameOf(signal)) + // SIGINT/SIGTERM/SIGHUP all route to the one owner shutdown state. The + // coordinator is spawned detached and unref'd, so closing the parent terminal + // never forwards its SIGHUP here; this handler only answers a HUP delivered + // directly to the owning process (design D3). + process.on("SIGHUP", handler) process.on("SIGINT", handler) process.on("SIGTERM", handler) return () => { + process.off("SIGHUP", handler) process.off("SIGINT", handler) process.off("SIGTERM", handler) } @@ -510,14 +602,38 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { // Hosted runs defer the server/lease teardown here; the loop calls it between // iterations and after its finish hold. let deferredRelease: (() => Promise) | undefined - const shutdown = new RunShutdown() - const removeSignalHandlers = installShutdownSignals(shutdown) + const shutdown = options.shutdown ?? new RunShutdown() + // A coordinated run injects the coordinator-owned scope so the process + // signal handlers stay installed through the terminal finish hold and final + // release; direct runs own their scope locally (design D3). + const ownsShutdown = options.shutdown === undefined + const removeSignalHandlers = ownsShutdown ? installShutdownSignals(shutdown) : () => {} const autoAccept: AutoAccept = options.autoAccept ?? options.progress?.autoAccept ?? { mode: options.yolo ? "all" : options.smart ? "smart" : "off" } // cli.ts always resolves a concrete model string (--smart-model → config → // --model → defaults.model), so smart mode never lacks a judge. const judgeModel = parseModel(splitModelVariant(options.smartJudgeModel).model) + /** + * Stops the owned server and only then clears the live-server pointer and + * the run's execution ownership. Awaiting `stop()` is not proof of exit: an + * `unresolved` outcome keeps both as evidence rather than reporting a clean + * shutdown, and the child's identity record survives independently under + * `~/.convoy/processes` (design D3). + */ + const stopOwnedServer = async (): Promise => { + const stop = await opencode?.stop() + if (stop?.status === "unresolved") { + log.warn(`run server stop unresolved; keeping live-server metadata and execution ownership: ${stop.reason}`) + await metadata?.flush().catch((error) => log.warn(`couldn't flush run metadata: ${String(error)}`)) + return stop + } + await metadata?.serverStopped().catch((error) => log.warn(`couldn't persist server-stopped metadata: ${String(error)}`)) + await metadata?.flush().catch((error) => log.warn(`couldn't flush run metadata: ${String(error)}`)) + await releaseLease?.().catch((error) => log.warn(`couldn't release run lease: ${String(error)}`)) + return stop + } + try { releaseLease = await acquireRunLease(workspace) // The run's branch, resolved once: the confirmed worktree branch for an @@ -719,11 +835,29 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { throughputModels, }), boot.signal, + // The run server is owned through the coordinator's release; helpers + // default to their own short-lived lifetime class. The run id rides the + // lifetime record so a later orphan recovery can tie the child back to + // its run (design D6). + { lifetime: "run", runId: workspace.runID }, ) } finally { shutdown.signal.removeEventListener("abort", abortBoot) } + // A repeated abort or the shutdown deadline forces this owned server + // before the process exits; without this edge a wedged cleanup would + // abandon the child (design D3). + shutdown.setForceHandler(() => opencode?.forceStop()) progress.serverReady(opencode.url) + // Link the live-server block to the actual child identity (design D6): the + // historical coordinator `pid` stays, and the record id / child PID/birth + // let an operator (or later recovery) identify the `serve` child without + // reinterpreting legacy metadata. An unresolved stop keeps this block. + metadata.recordServerChild({ + ...(opencode.recordId ? { recordId: opencode.recordId } : {}), + pid: opencode.pid, + ...(opencode.identity?.birth ? { birth: opencode.identity.birth } : {}), + }) // serverReady persists asynchronously through the progress adapter. Flush // before phases (or a hosted return) can expose this run for [o] attach. await metadata.flush() @@ -897,6 +1031,10 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { // The reviewed feature link survives workspace cleanup (task 5.1). ...(options.plan?.feature ? { feature: options.plan.feature } : {}), signal: shutdown.signal, + // The commit writer's helper runs under this coordinator, so its + // bounded stop draws from the coordinator's remaining shutdown + // budget instead of a fresh standalone allowance (design D2). + stopPolicy: () => shutdown.stopBudget(), progress: { activity: (detail, kind) => progress.phaseActivity(compactRunRowName, detail, kind), }, @@ -1059,12 +1197,9 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { // process.env), and they point at a bridge that no longer exists. reportBridge?.close() advisorBridge?.close() - // The server dies at the end of this block; clear its metadata entry now - // so `convoy runs` stops offering to attach to a run that's shutting down. - await metadata?.serverStopped().catch((error) => log.warn(`couldn't persist server-stopped metadata: ${String(error)}`)) - await metadata?.flush().catch((error) => log.warn(`couldn't flush run metadata: ${String(error)}`)) - await releaseLease?.().catch((error) => log.warn(`couldn't release run lease: ${String(error)}`)) - opencode?.close() + // Bridges first, then confirm the owned child exited before clearing + // the live-server pointer or releasing execution ownership (design D3). + await stopOwnedServer() // Hosted teardown runs after the coordinator's finish hold, so an // attached [i]/[o] can still flip keepRunDirRequested before we decide // whether the workspace may go. In-process runs settle in the finally @@ -1085,11 +1220,9 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { // fail with a fetch error, which it handles gracefully. reportBridge?.close() advisorBridge?.close() - // The server dies at the end of this block; clear its metadata entry now so - // `convoy runs` stops offering to attach to a run that's shutting down. - await metadata?.serverStopped().catch((error) => log.warn(`couldn't persist server-stopped metadata: ${String(error)}`)) - await metadata?.flush().catch((error) => log.warn(`couldn't flush run metadata: ${String(error)}`)) - await releaseLease?.().catch((error) => log.warn(`couldn't release run lease: ${String(error)}`)) + // The owned server stops and its exit is confirmed before the + // live-server pointer and execution ownership are cleared (design D3). + await stopOwnedServer() progress.stop() } // In-process runs stop the tracker (and publish idle/blocked) above; @@ -1098,18 +1231,14 @@ export async function run(options: RunOptions, deps: RunDeps = defaultRunDeps) { // After the renderer is gone: restoring the title while it still owns the // alternate screen would be overwritten by its teardown. if (titleSaved) popTerminalTitle() - shutdown.dispose() + // A coordinator-owned scope outlives this run: it stays live through the + // finish hold and release, and its caller disposes it (design D3). + if (ownsShutdown) shutdown.dispose() // Hosted runs settle the workspace in release(), after the coordinator // (or goal loop) has held the finish screen. Doing it here would delete // the run dir before [i] iterate can ask to keep it. if (!options.progress) await settleRunWorkspace(workspace, options, progress, runErr) - - // Kill the server last and return immediately: once it dies, any event - // stream still held open by the SDK starts failing, and those failures - // must not get a chance to surface mid-cleanup. Hosted runs defer this to - // their release so the goal loop's finish hold can still serve [o]. - if (!options.progress) opencode?.close() } } diff --git a/src/types.ts b/src/types.ts index 9bc8304..ecc1297 100644 --- a/src/types.ts +++ b/src/types.ts @@ -23,6 +23,7 @@ export type FeaturePlanLink = { worktreeDir?: string } import type { AutoAccept, ProgressUI } from "./progress" +import type { RunShutdown } from "./runner" import type { StepRunnerId } from "./step-runners" import type { ModelGateway, ModelRoutingOverrides, ResolvedModel } from "./model-routing" @@ -86,6 +87,14 @@ export type RunOptions = { * instead of doing it in the finally. */ progress?: ProgressUI + /** + * The shutdown scope whose process signal handlers span boot through final + * release. Set by the coordinator so a SIGTERM during the terminal finish + * hold or release resolves that wait and runs the bounded owned-server stop + * (design D3). When set, the runner uses it instead of creating and + * disposing its own; the caller owns its lifetime. + */ + shutdown?: RunShutdown /** * The shared auto-accept reference to use for the permission gate. When set, * the gate uses exactly this object (so a dashboard shift+tab toggle reaches diff --git a/src/worktree.ts b/src/worktree.ts index 407a3d3..f33afc4 100644 --- a/src/worktree.ts +++ b/src/worktree.ts @@ -396,7 +396,7 @@ export async function proposeBranchName(input: BranchNameInput): Promise, + stopPolicies: [] as unknown[], creates: [] as unknown[], prompts: [] as unknown[], deletes: [] as unknown[], @@ -50,14 +51,19 @@ function createProposalHarness(reply: string | Error) { }, } as unknown as OpencodeClient const deps: ProposalDeps = { - async startOpencode(config, signal) { + async startOpencode(config, signal, deps) { calls.configs.push(config) calls.startSignals.push(signal) + calls.stopPolicies.push(deps?.stopPolicy) return { client, url: "http://localhost:0", - close() { + pid: 0, + stop: async () => ({ status: "stopped" as const, via: "already-gone" as const }), + forceStop: () => {}, + close: async () => { calls.close++ + return { status: "stopped" as const, via: "already-gone" as const } }, } }, @@ -113,6 +119,18 @@ describe("proposeCommitMessage", () => { expect(calls.close).toBe(1) }) + test("forwards a shared stop budget resolver to the writer helper (design D2)", async () => { + const { calls, deps } = createProposalHarness('{"type":"feat","subject":"add login","body":[]}') + const resolver = () => ({ graceMs: 100, forceObservationMs: 0 }) + + await proposeCommitMessage({ ...proposalInput, stopPolicy: resolver }, deps) + + // The resolver travels to the owned helper so its bounded stop draws from + // the coordinator's remaining deadline instead of a fresh allowance. + expect(calls.stopPolicies).toHaveLength(1) + expect(calls.stopPolicies[0]).toBe(resolver) + }) + test("uses the deterministic template and closes the handle when the writer errors", async () => { const { calls, deps } = createProposalHarness(new Error("model unavailable")) diff --git a/test/control-lease.test.ts b/test/control-lease.test.ts new file mode 100644 index 0000000..0f077a3 --- /dev/null +++ b/test/control-lease.test.ts @@ -0,0 +1,186 @@ +import { describe, expect, test } from "bun:test" + +import { createControlClient } from "../src/control-client" +import { ControlProgress } from "../src/control-progress" +import { startControlServer, type ControllerLeaseEvent, type ControlServer } from "../src/control-server" + +/** + * Controller-lease notifications (change fix-opencode-server-lifecycle, + * design D4): a completed run's terminal hold must follow a controller that + * leaves or silently dies, without waiting for a request that will never come. + */ + +async function claim(server: Awaited>) { + const client = createControlClient({ url: server.url, token: server.token }) + await client.claimController() + return client +} + +describe("control-server lease notifications", () => { + test("a silent controller expires without any further request", async () => { + const server = await startControlServer({ controllerTimeoutMs: 100 }) + const events: ControllerLeaseEvent[] = [] + const unsubscribe = server.onControllerLease((event) => events.push(event)) + try { + await claim(server) + expect(server.hasController()).toBe(true) + // No further requests: the server's own timer must detect expiry. + await Bun.sleep(1_300) + expect(server.hasController()).toBe(false) + expect(events).toEqual(["claimed", "expired"]) + } finally { + unsubscribe() + server.close() + } + }) + + test("a valid /bye emits a release and no later expiry", async () => { + const server = await startControlServer({ controllerTimeoutMs: 100 }) + const events: ControllerLeaseEvent[] = [] + server.onControllerLease((event) => events.push(event)) + try { + const client = await claim(server) + await client.bye() + expect(server.hasController()).toBe(false) + await Bun.sleep(1_300) + expect(events).toEqual(["claimed", "released"]) + } finally { + server.close() + } + }) +}) + +describe("terminal hold follows the controller lease", () => { + test("a silent expiry releases the finish hold", async () => { + const server = await startControlServer({ controllerTimeoutMs: 100 }) + const progress = new ControlProgress({ server }) + try { + await claim(server) + let settled = false + const held = progress.runFinished({ status: "completed", runDir: "/tmp/run" }).then(() => { + settled = true + }) + expect(server.pending.snapshot().finish?.status).toBe("completed") + await Bun.sleep(1_400) + expect(settled).toBe(true) + expect(server.pending.snapshot().finish).toBeUndefined() + await held + } finally { + server.close() + } + }, 10_000) + + test("an explicit departure releases the hold", async () => { + const server = await startControlServer() + const progress = new ControlProgress({ server }) + try { + const client = await claim(server) + const held = progress.runFinished({ status: "completed", runDir: "/tmp/run" }) + await Bun.sleep(20) + expect(server.pending.snapshot().finish).toBeDefined() + await client.bye() + const settled = await Promise.race([held.then(() => true), Bun.sleep(250).then(() => false)]) + expect(settled).toBe(true) + } finally { + server.close() + } + }) + + test("no controller at hold entry releases immediately", async () => { + const server = await startControlServer() + const progress = new ControlProgress({ server }) + try { + await progress.runFinished({ status: "completed", runDir: "/tmp/run" }) + expect(server.pending.snapshot().finish).toBeUndefined() + } finally { + server.close() + } + }) +}) + +describe("lease notification lifecycle", () => { + test("unsubscribe stops further delivery", async () => { + const server = await startControlServer({ controllerTimeoutMs: 100 }) + const events: ControllerLeaseEvent[] = [] + const unsubscribe = server.onControllerLease((event) => events.push(event)) + unsubscribe() + try { + await claim(server) + await Bun.sleep(1_300) + expect(events).toEqual([]) + } finally { + server.close() + } + }) + + test("observer traffic neither holds nor dismisses a controller's finish screen", async () => { + const server = await startControlServer() + const progress = new ControlProgress({ server }) + try { + const controller = await claim(server) + const observer = createControlClient({ url: server.url, token: server.token }) + const held = progress.runFinished({ status: "completed", runDir: "/tmp/run" }) + await Bun.sleep(20) + await observer.pending() + await Bun.sleep(20) + // An observer may inspect pending state without acquiring or releasing + // the terminal hold. + expect(server.pending.snapshot().finish).toBeDefined() + await controller.bye() + const settled = await Promise.race([held.then(() => true), Bun.sleep(250).then(() => false)]) + expect(settled).toBe(true) + } finally { + server.close() + } + }) + + test("closing the server disposes the lease timer and listeners", async () => { + const server = await startControlServer({ controllerTimeoutMs: 50 }) + const events: ControllerLeaseEvent[] = [] + server.onControllerLease((event) => events.push(event)) + await claim(server) + server.close() + await Bun.sleep(1_200) + expect(events).toEqual(["claimed"]) + }) +}) + +describe("terminal hold against controller replacement", () => { + test("an old lease's expiry never dismisses a replacement controller's finish screen", async () => { + let controllerPresent = true + let leaseListener: ((event: ControllerLeaseEvent) => void) | undefined + let resolvedFinish = 0 + let releaseHold: (() => void) | undefined + const fakeServer = { + token: "t", + setHandlers: () => {}, + hasController: () => controllerPresent, + onControllerLease: (listener: (event: ControllerLeaseEvent) => void) => { + leaseListener = listener + return () => { + leaseListener = undefined + } + }, + pending: { + holdFinish: () => new Promise((resolve) => (releaseHold = resolve)), + resolveFinish: () => { + resolvedFinish++ + releaseHold?.() + }, + }, + } as unknown as ControlServer + const progress = new ControlProgress({ server: fakeServer }) + const held = progress.runFinished({ status: "completed", runDir: "/tmp/run" }) + await Bun.sleep(5) + // A replacement claimed the slot before the old lease's expiry ran: the + // callback must revalidate current ownership and leave the hold alone. + leaseListener?.("expired") + await Bun.sleep(5) + expect(resolvedFinish).toBe(0) + // The replacement's own departure then releases it, once. + controllerPresent = false + leaseListener?.("released") + expect(await Promise.race([held.then(() => true), Bun.sleep(100).then(() => false)])).toBe(true) + expect(resolvedFinish).toBe(1) + }) +}) diff --git a/test/control-server.test.ts b/test/control-server.test.ts index a87f4d5..b0c08bb 100644 --- a/test/control-server.test.ts +++ b/test/control-server.test.ts @@ -331,6 +331,42 @@ describe("control server", () => { } }) + test("controller lease expiry leaves permission and human gates pending", async () => { + const pending = new ControlPendingQueue() + const server = await startControlServer({ pending, controllerTimeoutMs: 120 }) + try { + const id = await claim(server) + expect(id).toBeTruthy() + let permissionSettled = false + let humanSettled = false + const permission = pending.holdPermission({ ...permissionInfo }) + const human = pending.holdHuman({ stepName: "review-a", iterations: 0 }) + void permission.then(() => { + permissionSettled = true + }) + void human.then(() => { + humanSettled = true + }) + expect(pending.snapshot().permission?.requestId).toBe("perm-1") + expect(pending.snapshot().human?.stepName).toBe("review-a") + + // The controller goes silent; the lease expires with no further request. + await Bun.sleep(250) + expect(server.hasController()).toBe(false) + + // A departed controller never answers or rejects a pending decision, and + // the gates are not auto-cancelled: they wait for the next controller + // (spec R5 "Background execution and independent services remain + // protected"). Only the finish hold follows the lease. + expect(permissionSettled).toBe(false) + expect(humanSettled).toBe(false) + expect(pending.snapshot().permission?.requestId).toBe("perm-1") + expect(pending.snapshot().human?.stepName).toBe("review-a") + } finally { + server.close() + } + }) + test("concurrent human gates resolve by id, not by queue head", async () => { const pending = new ControlPendingQueue() const server = await startControlServer({ pending }) diff --git a/test/conversation-service.test.ts b/test/conversation-service.test.ts index 92a5bdb..d6270bd 100644 --- a/test/conversation-service.test.ts +++ b/test/conversation-service.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test" -import { mkdir, mkdtemp, readFile, rm } from "node:fs/promises" +import { chmod, mkdir, mkdtemp, readFile, rm } from "node:fs/promises" import { tmpdir } from "node:os" import { join } from "node:path" @@ -218,6 +218,51 @@ describe("conversation-service discovery and reuse (task 4.3)", () => { expect(read2.status).toBe("missing") }) + test("an explicit stop refuses a recycled pid whose recorded birth identity no longer matches (task 2.5)", async () => { + const seeded: ConversationServiceRecord = { + schemaVersion: schemaVersion, + url: "http://127.0.0.1:51006", + pid: 123_461, + bootCheckout: repoDir, + startedAt: Date.now(), + childBirth: "boot:original", + } + const { writeJsonFile } = await import("../src/repo-store") + await writeJsonFile(join(commonDir, "convoy", "authoring-server.json"), seeded) + let killed = 0 + // The PID answers (URL liveness "live") but the kernel reports a different + // birth: a recycled PID must never be killed as the recorded server. + const stopped = await stopConversationService({ + commonDir, + activity: "idle", + probe: async () => "live", + identityProbe: { + observe: async (pid) => ({ status: "alive", identity: { pid, birth: "boot:recycled", uid: 501, executable: "opencode" } }), + }, + kill: async () => { + killed += 1 + }, + }) + expect(stopped.status).toBe("kept") + expect(killed).toBe(0) + expect((await readConversationServiceDiscovery(commonDir)).status).toBe("found") + + // A matching identity still stops normally. + const matched = await stopConversationService({ + commonDir, + activity: "idle", + probe: async () => "live", + identityProbe: { + observe: async (pid) => ({ status: "alive", identity: { pid, birth: "boot:original", uid: 501, executable: "opencode" } }), + }, + kill: async () => { + killed += 1 + }, + }) + expect(matched.status).toBe("stopped") + expect(killed).toBe(1) + }) + test("an uncertain probe during stop keeps both the process and its record", async () => { const seeded: ConversationServiceRecord = { schemaVersion: schemaVersion, @@ -341,3 +386,35 @@ describe("conversation-service shutdown boundaries (task 4.3, real server)", () await stopConversationService({ commonDir: realCommonDir, activity: "idle" }) }) }) + +describe("conversation-service publication failure (design D6)", () => { + test("closes an unpublished child when the discovery record cannot be written", async () => { + const repo = await mkdtemp(join(tmpdir(), "convoy-service-fail-")) + dirs.push(repo) + await Bun.write(join(repo, "README.md"), "# repo\n") + await git(repo, ["init", "-q", "-b", "main"]) + await git(repo, ["add", "."]) + await git(repo, ["-c", "user.email=t@x", "-c", "user.name=T", "commit", "-q", "-m", "init"]) + const common = (await repoCommonDir(repo))! + const convoyDir = join(common, "convoy") + // Pre-create the lock directory, then make its parent read-only so the + // discovery record cannot be written while reads still see "missing". + await mkdir(join(convoyDir, "authoring-service"), { recursive: true }) + await chmod(convoyDir, 0o500) + let closed = 0 + const boot = async () => ({ + url: "http://127.0.0.1:51999", + pid: 999_999, + close: async () => { + closed++ + }, + }) + try { + const outcome = await ensureConversationService({ commonDir: common, checkout: repo, boot, probe: async () => "stale" }) + expect(outcome.status).toBe("unavailable") + expect(closed).toBe(1) + } finally { + await chmod(convoyDir, 0o700) + } + }) +}) diff --git a/test/coordinate.test.ts b/test/coordinate.test.ts index b51ce55..6c37993 100644 --- a/test/coordinate.test.ts +++ b/test/coordinate.test.ts @@ -20,7 +20,7 @@ import { writePendingLaunch, } from "../src/coordinate" import type { AutoAccept, RunOutcome } from "../src/progress" -import { UserAbortError } from "../src/runner" +import { RunShutdown, UserAbortError } from "../src/runner" import type { AgentStep, RunOptions, RunPlan } from "../src/types" const dirs: string[] = [] @@ -435,6 +435,55 @@ describe("runCoordinateBoot", () => { expect(released).toBe(1) expect(finished).toEqual([]) }) + + test("SC-1: a signal during the finish hold releases it and runs the owned stop", async () => { + // The coordinator's shutdown scope must outlive run(): a SIGTERM after + // execution finishes but before the owned server stops has to release the + // parked terminal wait and fall through to the bounded stop (spec R4). The + // scope is injected so the test delivers the signal without killing the + // test runner. + const root = await scratch() + const pending = await writeBootLaunch(root) + const order: string[] = [] + const shutdown = new RunShutdown({ exit: () => {} }) + let holdRegistered!: () => void + const registered = new Promise((resolve) => { + holdRegistered = resolve + }) + + const promise = runCoordinateBoot(pending.launchPath, pending.readyPath, { + launchRoot: root, + createShutdown: () => shutdown, + // The test's manual `request` stands in for the process signal; no real + // handler is installed. + installSignals: () => () => {}, + createProgress: (opts) => { + const progress = new ControlProgress(opts) + progress.runFinished = async () => { + order.push("hold") + holdRegistered() + await new Promise(() => {}) + } + return progress + }, + run: async () => ({ + runID: "20260101-000000-ab12", + dir: "/tmp/run", + release: async () => { + order.push("release") + }, + }), + }) + + await registered + shutdown.request("SIGTERM") + const code = await promise + expect(code).toBe(0) + expect(order).toEqual(["hold", "release"]) + // The scope is disposed after release: a later request is inert. + shutdown.request("SIGTERM") + expect(order).toEqual(["hold", "release"]) + }) }) function advisedImplementerStep(): AgentStep { diff --git a/test/coordinator-hold-subprocess.test.ts b/test/coordinator-hold-subprocess.test.ts new file mode 100644 index 0000000..ceb7cd5 --- /dev/null +++ b/test/coordinator-hold-subprocess.test.ts @@ -0,0 +1,136 @@ +import { access, mkdtemp, readFile, rm, writeFile } from "node:fs/promises" +import { tmpdir } from "node:os" +import { join } from "node:path" + +import { afterAll, describe, expect, test } from "bun:test" + +/** + * Isolated subprocess regression for the coordinator finish hold (change + * fix-opencode-server-lifecycle, design D7 / spec R4). + * + * The fixture runs the production `runCoordinateBoot` with the real + * SIGINT/SIGTERM/SIGHUP scope installed and owns a real managed `serve` child. + * A real signal delivered while the finish hold is parked must release the + * hold and run the bounded owned-server stop with an observed child exit. The + * fixture runs in its own process so the signal never reaches the test runner. + */ + +const dirs: string[] = [] +const spawned = new Set() + +afterAll(async () => { + for (const pid of spawned) { + try { + process.kill(pid, "SIGKILL") + } catch { + /* already gone */ + } + } + await Promise.all(dirs.map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +async function scratchDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), "convoy-hold-")) + dirs.push(dir) + return dir +} + +const READY_LINE = 'console.log("opencode server listening on http://127.0.0.1:1")\n' + +function pidAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +async function waitForExit(pid: number, timeoutMs = 5_000): Promise { + const deadline = Date.now() + timeoutMs + while (Date.now() < deadline) { + if (!pidAlive(pid)) return true + await Bun.sleep(25) + } + return !pidAlive(pid) +} + +async function fileExists(path: string): Promise { + try { + await access(path) + return true + } catch { + return false + } +} + +/** Runs the fixture, parks its finish hold, delivers a real signal, and asserts the observed stop. */ +async function runFinishHold(signal: number): Promise { + const dir = await scratchDir() + const childScript = join(dir, "hold-child.js") + await writeFile(childScript, `${READY_LINE}setInterval(() => {}, 1000)\n`) + const storeDir = join(dir, "processes") + const recordPath = join(dir, "record.json") + const holdingPath = join(dir, "holding") + const pidPath = join(dir, "child.pid") + const fixture = join(import.meta.dir, "fixtures", "coordinator-hold-fixture.ts") + + const proc = Bun.spawn([process.execPath, fixture, childScript, storeDir, recordPath, holdingPath, pidPath], { + cwd: process.cwd(), + stdout: "ignore", + stderr: "pipe", + env: { ...process.env, CONVOY_HOME: join(dir, "home") }, + }) + if (proc.pid) spawned.add(proc.pid) + + const drainStderr = async () => new Response(proc.stderr).text().catch(() => "") + + try { + // Wait until the finish hold is parked and the owned child is running. + const deadline = Date.now() + 15_000 + while (!(await fileExists(holdingPath)) && Date.now() < deadline) await Bun.sleep(25) + if (!(await fileExists(holdingPath))) { + proc.kill(9) + throw new Error(`fixture never parked its finish hold. stderr: ${await drainStderr()}`) + } + + const childPid = Number(await readFile(pidPath, "utf8")) + expect(childPid).toBeGreaterThan(0) + spawned.add(childPid) + expect(pidAlive(childPid)).toBe(true) + + // The real signal the coordinator's owner scope must answer. + proc.kill(signal) + const code = await proc.exited + if (code !== 0) throw new Error(`fixture exited with ${code}. stderr: ${await drainStderr()}`) + + const record = JSON.parse(await readFile(recordPath, "utf8")) as { pid: number; outcome: { status: string } } + expect(record.pid).toBe(childPid) + // A delivered signal is not a stop; the outcome is recorded only after the + // bounded stop confirms the child exited. + expect(record.outcome.status).toBe("stopped") + expect(await waitForExit(childPid)).toBe(true) + } finally { + proc.kill(9) + const childPid = await readFile(pidPath, "utf8") + .then(Number) + .catch(() => 0) + if (Number.isFinite(childPid) && childPid > 0) { + try { + process.kill(childPid, "SIGKILL") + } catch { + /* gone */ + } + } + } +} + +describe("coordinator finish-hold subprocess", () => { + test("a real SIGTERM during the finish hold stops the owned child and observes its exit", async () => { + await runFinishHold(15) + }, 30_000) + + test("a real SIGHUP during the finish hold stops the owned child and observes its exit", async () => { + await runFinishHold(1) + }, 30_000) +}) diff --git a/test/env.ts b/test/env.ts index 4802b90..4c5e7be 100644 --- a/test/env.ts +++ b/test/env.ts @@ -1,8 +1,10 @@ +import { afterAll } from "bun:test" import { mkdirSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { setLimitsFetcherForTests } from "../src/limits" +import { stopOwnedTestServers } from "./process-teardown" // Isolate every test run from the developer's real ~/.convoy so tests never // read or write the user's actual config, runs, or agent prompts. CONVOY_HOME @@ -15,3 +17,13 @@ mkdirSync(process.env.CONVOY_HOME, { recursive: true }) // pick up the real ChatGPT/OpenRouter meters. Tests that need a snapshot // assign it to `dashboard.limits` directly. setLimitsFetcherForTests(async () => ({})) + +// Run-level teardown: this preload `afterAll` runs once for the whole `bun test` +// invocation, while the owner process is still alive. It stops any managed +// OpenCode server a test still owns and drops its record, so a helper whose +// promise outlived its test file can never leave an orphan the production +// reconciliation in `~/.convoy/processes/` would never see (this run's records +// live under the throwaway test `CONVOY_HOME`, above). +afterAll(async () => { + await stopOwnedTestServers().catch(() => {}) +}) diff --git a/test/fixtures/coordinator-hold-fixture.ts b/test/fixtures/coordinator-hold-fixture.ts new file mode 100644 index 0000000..3df2e24 --- /dev/null +++ b/test/fixtures/coordinator-hold-fixture.ts @@ -0,0 +1,116 @@ +/** + * Isolated coordinator finish-hold regression fixture (change + * fix-opencode-server-lifecycle, design D7). + * + * Runs the production `runCoordinateBoot` with the real signal scope installed + * and a real managed `opencode serve` child. When the parent test sends a real + * SIGTERM while the finish hold is parked, the coordinator must release the + * hold, run the bounded owned-server stop, and observe the child's exit. The + * outcome and child PID are recorded for the parent to assert. + * + * It never reads user OpenCode config, uses model credentials, or matches a + * broad process scan. + * + * argv: + */ +import { mkdtemp, writeFile } from "node:fs/promises" +import { dirname, join } from "node:path" + +import { launchPayload, runCoordinateBoot, writePendingLaunch } from "../../src/coordinate" +import { launchManagedServer } from "../../src/managed-server" +import { createProcessRecordStore } from "../../src/process-records" +import type { RunOptions, RunPlan } from "../../src/types" + +const [childScript, storeDir, recordPath, holdingPath, pidPath] = process.argv.slice(2) +if (!childScript || !storeDir || !recordPath || !holdingPath || !pidPath) { + console.error("usage: coordinator-hold-fixture ") + process.exit(2) +} + +// Scratch dirs live under the parent test's removable directory so a passing +// or failing run leaves no temp state behind. +const scratch = dirname(recordPath) +const runDir = await mkdtemp(join(scratch, "run-")) +const launchRoot = await mkdtemp(join(scratch, "launch-")) + +const options: RunOptions = { + prompt: "hold", + prdHistory: false, + files: [], + onlySteps: [], + skipSteps: [], + resumeRunID: "", + keepRunDir: true, + modelOverride: "", + advisorOverride: "", + advisorDisabled: false, + tui: false, + notifications: {}, + humanReview: false, + baseRef: "main", + targetDir: runDir, + worktree: false, + includeDirty: false, + yolo: false, + smart: false, + smartJudgeModel: "openai/gpt-5", + pipeline: { name: "hold", steps: [] }, + agents: [], + permissions: { allow: [], deny: [] }, + hooks: { pre: [], post: [], pipelines: {} }, +} + +const plan: RunPlan = { + prompt: { source: "inline", text: "hold" }, + target: { directory: runDir, baseRef: "main", worktree: false, dirty: false }, + pipeline: { name: "hold", steps: [] }, + modelRouting: { gateway: "openrouter" }, + hooks: { pre: [], post: [] }, + attachments: [], + permissions: "interactive", +} + +const pending = await writePendingLaunch(launchPayload(options, plan), launchRoot) + +let server: Awaited> | undefined + +// The finish hold: record that the hold is parked, then never resolve. The +// parent test sends its signal after seeing the holding marker. +const hold = { + autoAccept: { mode: "off" as "off" | "all" | "smart" }, + runFinished: async (): Promise => { + await writeFile(holdingPath, "holding") + await new Promise(() => {}) + }, +} + +const code = await runCoordinateBoot(pending.launchPath, pending.readyPath, { + launchRoot, + // The production `installSignals` (real SIGINT/SIGTERM/SIGHUP handlers) and + // `startControlServer` are deliberately left at their defaults: this fixture + // exercises the real wiring, not an injected one. + createProgress: () => hold as never, + run: async () => { + server = await launchManagedServer({ + command: process.execPath, + args: [childScript], + cwd: runDir, + env: {}, + lifetime: "helper", + label: "hold fixture child", + timeoutMs: 5_000, + deps: { store: createProcessRecordStore(storeDir), policy: { graceMs: 2_000, forceObservationMs: 1_000 } }, + }) + await writeFile(pidPath, String(server.pid)) + return { + runID: "20260101-000000-hold", + dir: runDir, + release: async () => { + const outcome = await server!.stop() + await writeFile(recordPath, JSON.stringify({ pid: server!.pid, outcome })) + }, + } + }, +}) + +process.exit(code) diff --git a/test/home-tui.test.ts b/test/home-tui.test.ts index 3fda862..6f24d1a 100644 --- a/test/home-tui.test.ts +++ b/test/home-tui.test.ts @@ -104,12 +104,23 @@ function viewDir(): string { return "/work/acme" } +/** + * Hermetic default for the branch-name proposal. The production proposer boots + * a real `opencode serve`, so an unprompted fallback to it would start a server + * from inside the test runner — exactly the child this file must never leak. + * Tests that assert a model-derived name inject their own `proposeBranchName`. + */ +const hermeticProposeBranchName = async ({ prompt }: { prompt: string }): Promise<{ branch: string }> => ({ + branch: `feat/${prompt.trim().toLowerCase().replace(/\s+/g, "-") || "work"}`, +}) + async function openHome(options: { worktrees?: BoardWorktree[]; width?: number; height?: number; targetDir?: string; proposeBranchName?: (input: { prompt: string }) => Promise<{ branch: string }>; observePr?: (worktree: BoardWorktree) => Promise; listRunsForWorktree?: (worktree: BoardWorktree) => Promise } = {}) { const testRenderer = await createTestRenderer({ width: options.width ?? 110, height: options.height ?? 30 }) const instance = new HomeLauncher(testRenderer.renderer, options.targetDir ?? viewDir(), { scene: undefined, worktrees: options.worktrees ?? worktrees, - proposeBranchName: options.proposeBranchName, + // Hermetic default: never let an omitted proposal reach the real namer. + proposeBranchName: options.proposeBranchName ?? hermeticProposeBranchName, // Hermetic default: no test talks to `gh` unless it injects its own observer. observePr: options.observePr ?? (async () => ({ availability: "unknown", reason: "no PR observation requested by this test", observedAt: 0 })), // Hermetic default: no test reads run history unless it injects its own source. diff --git a/test/managed-server-subprocess.test.ts b/test/managed-server-subprocess.test.ts new file mode 100644 index 0000000..ec61c65 --- /dev/null +++ b/test/managed-server-subprocess.test.ts @@ -0,0 +1,310 @@ +import { spawn as rawSpawn } from "node:child_process" +import { afterAll, describe, expect, test } from "bun:test" +import { mkdtemp, rm, writeFile } from "node:fs/promises" +import { tmpdir } from "node:os" +import { join } from "node:path" + +import { launchManagedServer } from "../src/managed-server" +import { captureIdentity, defaultIdentityProbe, type IdentityProbe } from "../src/process-identity" +import { createProcessRecordStore, newProcessRecord, reconcileProcessRecords } from "../src/process-records" +import { RunShutdown } from "../src/runner" + +/** + * Real-OS subprocess fixtures (change fix-opencode-server-lifecycle, D7). + * + * These children are plain local scripts: they never read OpenCode config, use + * model credentials, or match a broad process scan. Every spawned PID is + * tracked and killed in teardown even when an assertion fails. + */ + +const dirs: string[] = [] +const spawned = new Set() + +afterAll(async () => { + for (const pid of spawned) { + try { + process.kill(pid, "SIGKILL") + } catch { + /* already gone */ + } + } + await Promise.all(dirs.map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +async function scratchDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), "convoy-subprocess-")) + dirs.push(dir) + return dir +} + +function track(pid: number | undefined): number { + if (pid) spawned.add(pid) + return pid ?? 0 +} + +/** Reads the readiness line the managed launcher waits for. */ +const READY_LINE = 'console.log("opencode server listening on http://127.0.0.1:1")\n' + +async function fixtureScript(dir: string, name: string, body: string): Promise { + const path = join(dir, `${name}.js`) + await writeFile(path, body) + return path +} + +function pidAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +async function waitForExit(pid: number, timeoutMs = 3_000): Promise { + const deadline = Date.now() + timeoutMs + while (Date.now() < deadline) { + if (!pidAlive(pid)) return true + await Bun.sleep(25) + } + return !pidAlive(pid) +} + +describe("owned subprocess lifecycle", () => { + test("terminates a real child and observes the exit", async () => { + const dir = await scratchDir() + const script = await fixtureScript(dir, "normal", `${READY_LINE}setInterval(() => {}, 1000)\n`) + const store = createProcessRecordStore(join(dir, "processes")) + const server = await launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 5_000, + deps: { store, policy: { graceMs: 2_000, forceObservationMs: 1_000 } }, + }) + track(server.pid) + expect(server.url).toBe("http://127.0.0.1:1") + expect(pidAlive(server.pid)).toBe(true) + + const outcome = await server.stop() + expect(outcome.status).toBe("stopped") + expect(await waitForExit(server.pid)).toBe(true) + // The record of a confirmed stop is removed. + expect(await store.list()).toEqual([]) + }, 15_000) + + test("escalates to SIGKILL for a child that ignores SIGTERM", async () => { + const dir = await scratchDir() + const script = await fixtureScript( + dir, + "stubborn", + `${READY_LINE}process.on("SIGTERM", () => {})\nsetInterval(() => {}, 1000)\n`, + ) + const store = createProcessRecordStore(join(dir, "processes")) + const server = await launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 5_000, + deps: { store, policy: { graceMs: 300, forceObservationMs: 2_000 } }, + }) + track(server.pid) + const outcome = await server.stop() + expect(outcome).toEqual({ status: "stopped", via: "forced" }) + expect(await waitForExit(server.pid)).toBe(true) + }, 15_000) + + test("a later reconciliation pass stops a real orphan whose owner is gone", async () => { + const dir = await scratchDir() + const script = await fixtureScript(dir, "orphan", `${READY_LINE}setInterval(() => {}, 1000)\n`) + const probe: IdentityProbe = defaultIdentityProbe() + + // A real, already-dead owner: spawn and reap a short-lived process. + const deadOwner = Bun.spawn([process.execPath, "-e", "process.exit(0)"], { stdout: "ignore", stderr: "ignore" }) + const deadOwnerPid = track(deadOwner.pid) + await deadOwner.exited + const ownerIdentity = await captureIdentity(deadOwnerPid, probe) + expect(ownerIdentity).toBeUndefined() + + // The orphan child is a real live process; its old owner PID is provably gone. + const orphan = Bun.spawn([process.execPath, script], { stdout: "pipe", stderr: "ignore" }) + const orphanPid = track(orphan.pid) + const childIdentity = await captureIdentity(orphanPid, probe) + expect(childIdentity).toBeDefined() + + const store = createProcessRecordStore(join(dir, "processes")) + await store.put({ + ...newProcessRecord({ lifetime: "run" }), + // Reuse the dead PID as the recorded owner: the probe reports `gone`. + owner: { pid: deadOwnerPid, birth: "gone", uid: childIdentity!.uid, executable: "bun" }, + child: childIdentity!, + state: "ready", + }) + + const outcome = await reconcileProcessRecords({ store, probe, policy: { graceMs: 2_000, forceObservationMs: 1_000, pollMs: 25 } }) + expect(outcome.stopped.length).toBe(1) + expect(await waitForExit(orphanPid)).toBe(true) + }, 20_000) + + test("reconciliation leaves a live owner's child untouched", async () => { + const dir = await scratchDir() + const script = await fixtureScript(dir, "owned", `${READY_LINE}setInterval(() => {}, 1000)\n`) + const probe: IdentityProbe = defaultIdentityProbe() + + const owner = Bun.spawn([process.execPath, "-e", "setInterval(() => {}, 1000)"], { stdout: "ignore", stderr: "ignore" }) + const ownerPid = track(owner.pid) + const ownerIdentity = await captureIdentity(ownerPid, probe) + expect(ownerIdentity).toBeDefined() + + const child = Bun.spawn([process.execPath, script], { stdout: "pipe", stderr: "ignore" }) + const childPid = track(child.pid) + const childIdentity = await captureIdentity(childPid, probe) + expect(childIdentity).toBeDefined() + + const store = createProcessRecordStore(join(dir, "processes")) + await store.put({ ...newProcessRecord({ lifetime: "run" }), owner: ownerIdentity!, child: childIdentity!, state: "ready" }) + + const outcome = await reconcileProcessRecords({ store, probe }) + expect(outcome.stopped).toEqual([]) + expect(pidAlive(childPid)).toBe(true) + expect(pidAlive(ownerPid)).toBe(true) + owner.kill("SIGKILL") + child.kill("SIGKILL") + }, 20_000) +}) + +describe("owned subprocess force edge and startup race", () => { + test("the force edge kills a real child that ignores SIGTERM", async () => { + const dir = await scratchDir() + const script = await fixtureScript( + dir, + "stubborn-force", + `${READY_LINE}process.on("SIGTERM", () => {})\nsetInterval(() => {}, 1000)\n`, + ) + const store = createProcessRecordStore(join(dir, "processes")) + const server = await launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 5_000, + deps: { store, policy: { graceMs: 5_000, forceObservationMs: 1_000 } }, + }) + track(server.pid) + expect(pidAlive(server.pid)).toBe(true) + // The synchronous last-resort edge a repeated abort/deadline delivers. + server.forceStop() + expect(await waitForExit(server.pid)).toBe(true) + const outcome = await server.stop() + expect(outcome.status).toBe("stopped") + }, 15_000) + + test("a helper that exits before readiness fails with bounded cleanup and no lingering record", async () => { + const dir = await scratchDir() + const script = await fixtureScript(dir, "early-exit", "process.exit(3)\n") + const store = createProcessRecordStore(join(dir, "processes")) + // The managed launcher owns the child from spawn: a helper that dies before + // reporting a URL must reject with a bounded cleanup outcome and leave no + // record behind (design D1, spec R2 "Startup fails after spawning"). + await expect( + launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 5_000, + deps: { store, policy: { graceMs: 500, forceObservationMs: 500 } }, + }), + ).rejects.toThrow(/exited before it was ready/) + expect(await store.list()).toEqual([]) + }, 15_000) + + test("the shutdown deadline force edge kills a real owned child when session cancellation hangs", async () => { + const dir = await scratchDir() + const script = await fixtureScript( + dir, + "stubborn-deadline", + `${READY_LINE}process.on("SIGTERM", () => {})\nsetInterval(() => {}, 1000)\n`, + ) + const store = createProcessRecordStore(join(dir, "processes")) + const server = await launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 5_000, + deps: { store, policy: { graceMs: 5_000, forceObservationMs: 1_000 } }, + }) + track(server.pid) + expect(pidAlive(server.pid)).toBe(true) + + // An explicitly aborted run whose session-cancellation request never + // answers: the injected deadline must still deliver the owned-server force + // edge before the process exits (spec R4 "Session cancellation does not + // answer"; task 6.1 repeated-abort/deadline). + let exitCode: number | undefined + const shutdown = new RunShutdown({ exit: (code) => { exitCode = code }, graceMs: 40, forceObservationMs: 200 }) + shutdown.setForceHandler(() => server.forceStop()) + shutdown.setActiveSession({ + client: { session: { abort: () => new Promise(() => {}) } } as never, + sessionID: "ses_hung", + directory: dir, + phaseName: "implementer", + }) + const hung = shutdown.abortActiveSessions() + shutdown.request("SIGTERM") + expect(await waitForExit(server.pid)).toBe(true) + await Bun.sleep(250) + expect(exitCode).toBe(130) + // The forced stop is observed, so the helper's record is released. + const recordDeadline = Date.now() + 2_000 + while ((await store.list()).length > 0 && Date.now() < recordDeadline) await Bun.sleep(25) + expect(await store.list()).toEqual([]) + shutdown.dispose() + void hung + }, 15_000) + + test("an abort racing startup cleans up the real child it spawned", async () => { + const dir = await scratchDir() + const script = await fixtureScript(dir, "hang", "setInterval(() => {}, 1000)\n") + const store = createProcessRecordStore(join(dir, "processes")) + const controller = new AbortController() + let childPid = 0 + const promise = launchManagedServer({ + command: process.execPath, + args: [script], + cwd: dir, + env: {}, + lifetime: "helper", + label: "fixture helper", + timeoutMs: 10_000, + signal: controller.signal, + deps: { + store, + spawn: (command, args, options) => { + const child = rawSpawn(command, args, options) + childPid = track(child.pid ?? 0) + return child + }, + policy: { graceMs: 2_000, forceObservationMs: 1_000 }, + }, + }) + // Cancel while the launch is still in its pre-spawn gaps: the abort event + // never replays, so the launcher must recheck `aborted` after spawn. + controller.abort(new Error("cancelled")) + await expect(promise).rejects.toThrow(/cancelled/) + expect(childPid).toBeGreaterThan(0) + expect(await waitForExit(childPid)).toBe(true) + }, 15_000) +}) diff --git a/test/metadata.test.ts b/test/metadata.test.ts index 7b787ee..d134d00 100644 --- a/test/metadata.test.ts +++ b/test/metadata.test.ts @@ -405,6 +405,51 @@ describe("openRunMetadata", () => { } }) + test("recordServerChild links the live block to explicit child evidence (task 3.4)", async () => { + const { dir, ws, cleanup } = await withDir("srv-child") + const store = await openRunMetadata(ws, "/target", validPipeline([validAgentStep("design")])) + try { + store.serverStarted("http://localhost:8080") + store.recordServerChild({ recordId: "rec-123", pid: 4321, birth: "boot:4321" }) + await store.flush() + + const raw = await readRunMetadata(`${dir}/metadata.json`) + // The historical coordinator pid is preserved; the child evidence is + // recorded separately so neither is read as the other. + expect(raw!.server?.pid).toBeGreaterThan(0) + expect(raw!.server?.recordId).toBe("rec-123") + expect(raw!.server?.childPid).toBe(4321) + expect(raw!.server?.childBirth).toBe("boot:4321") + + // A confirmed stop clears the whole live block, evidence included. + await store.serverStopped() + expect((await readRunMetadata(`${dir}/metadata.json`))!.server).toBeUndefined() + } finally { + await cleanup() + } + }) + + test("a legacy live-server block without child evidence stays readable (task 3.4)", async () => { + const { dir, cleanup } = await withDir("srv-legacy") + try { + const legacy = { + ...baseV3, + schemaVersion: 5, + runID: "run-legacy", + server: { url: "http://127.0.0.1:4123", pid: process.pid, startedAt: 1_700_000_000_000 }, + } + await Bun.write(`${dir}/metadata.json`, JSON.stringify(legacy)) + const raw = await readRunMetadata(`${dir}/metadata.json`) + expect(raw!.server?.url).toBe("http://127.0.0.1:4123") + expect(raw!.server?.pid).toBe(process.pid) + // Legacy readers ignore the missing optional fields instead of guessing. + expect(raw!.server?.recordId).toBeUndefined() + expect(raw!.server?.childPid).toBeUndefined() + } finally { + await cleanup() + } + }) + test("setControlState from running to pausing to paused", async () => { const { dir, ws, cleanup } = await withDir("ctrl") const store = await openRunMetadata(ws, "/target", validPipeline([validAgentStep("design")])) @@ -944,6 +989,9 @@ describe("recordProgress", () => { recordPlannedPhases: () => Promise.resolve(), phaseOutput: (name: string) => { storeCalls.push(`phaseOutput(${name})`) }, serverStarted: (url: string) => { storeCalls.push(`serverStarted(${url})`) }, + recordServerChild: (evidence: { recordId?: string; pid: number; birth?: string }) => { + storeCalls.push(`recordServerChild(${evidence.pid})`) + }, serverStopped: () => Promise.resolve(), phaseStarted: (name: string) => { storeCalls.push(`phaseStarted(${name})`); return Promise.resolve() }, phaseSession: (name: string) => { storeCalls.push(`phaseSession(${name})`) }, diff --git a/test/opencode.test.ts b/test/opencode.test.ts index e5747d9..8dfb817 100644 --- a/test/opencode.test.ts +++ b/test/opencode.test.ts @@ -14,6 +14,7 @@ import { openIterateOpencodeWindow, connectOpencode, startOpencode, + bootOpencodeServerFrom, } from "../src/opencode" import type { OpencodeHandle } from "../src/opencode" @@ -1332,21 +1333,24 @@ describe("connectOpencode", () => { }) describe("startOpencode", () => { - test("returns the SDK client and closes the injected server", async () => { + test("owns the spawned server and awaits a bounded stop", async () => { const client = { session: {} } as unknown as OpencodeHandle["client"] - let closed = false - let serverOptions: Record | undefined + let stopped = 0 + let launchOptions: Record | undefined let clientOptions: Record | undefined const handle: OpencodeHandle = await startOpencode({}, undefined, { - createServer: async (options) => { - if (!options) throw new Error("expected server options") - serverOptions = options as unknown as Record + getFreePort: async () => 41234, + launch: async (options) => { + launchOptions = options as unknown as Record return { - url: `http://127.0.0.1:${options.port}`, - close() { - closed = true + url: "http://127.0.0.1:41234", + pid: 4242, + stop: async () => { + stopped++ + return { status: "stopped" as const, via: "graceful" as const } }, + forceStop: () => {}, } }, createClient: (options) => { @@ -1356,27 +1360,40 @@ describe("startOpencode", () => { }) expect(handle.client).toBe(client) - expect(handle.url).toMatch(/^http:\/\/127\.0\.0\.1:\d+$/) - expect(serverOptions).toMatchObject({ hostname: "127.0.0.1", timeout: 30_000, config: {} }) - expect(serverOptions?.port).toBeGreaterThan(0) + expect(handle.url).toBe("http://127.0.0.1:41234") + expect(handle.pid).toBe(4242) + expect(launchOptions).toMatchObject({ command: "opencode", cwd: process.cwd(), lifetime: "helper" }) + expect(launchOptions?.args).toContain("serve") + expect(launchOptions?.args).toContain("--hostname=127.0.0.1") + expect(launchOptions?.args).toContain("--port=41234") + expect(launchOptions?.env).toMatchObject({ OPENCODE_CONFIG_CONTENT: "{}" }) expect(clientOptions?.baseUrl).toBe(handle.url) expect(typeof clientOptions?.fetch).toBe("function") - handle.close() - expect(closed).toBe(true) + const outcome = await handle.close() + expect(outcome.status).toBe("stopped") + expect(stopped).toBe(1) }) - test("strips HERDR_* from process.env around the server call and restores them after", async () => { + test("passes a log level through and strips HERDR_* from the child env only", async () => { process.env.HERDR_ENV = "1" process.env.HERDR_PANE_ID = "w1:p1" const client = { session: {} } as unknown as OpencodeHandle["client"] let observed: Record | undefined + let observedArgs: string[] | undefined try { - const handle = await startOpencode({}, undefined, { - createServer: async () => { - observed = { ...process.env } - return { url: "http://127.0.0.1:1", close() {} } + const handle = await startOpencode({ logLevel: "warn" } as never, undefined, { + getFreePort: async () => 1, + launch: async (options) => { + observed = { ...options.env } + observedArgs = options.args + return { + url: "http://127.0.0.1:1", + pid: 1, + stop: async () => ({ status: "stopped" as const, via: "already-gone" as const }), + forceStop: () => {}, + } }, createClient: () => client, }) @@ -1384,12 +1401,106 @@ describe("startOpencode", () => { expect(observed).toBeDefined() expect(observed).not.toHaveProperty("HERDR_ENV") expect(observed).not.toHaveProperty("HERDR_PANE_ID") - // The caller's environment is untouched once the server exists. + expect(observed?.OPENCODE_CONFIG_CONTENT).toContain("warn") + expect(observedArgs).toContain("--log-level=warn") + // The caller's environment is untouched: the strip is per-child now. expect(process.env.HERDR_ENV).toBe("1") expect(process.env.HERDR_PANE_ID).toBe("w1:p1") - handle.close() + await handle.close() } finally { + delete process.env.HERDR_ENV delete process.env.HERDR_PANE_ID } }) }) + +describe("owned-server factory failure and boot", () => { + test("a client construction failure stops the owned child before surfacing", async () => { + let stopped = 0 + await expect( + startOpencode({}, undefined, { + getFreePort: async () => 1, + launch: async () => ({ + url: "http://127.0.0.1:1", + pid: 77, + stop: async () => { + stopped++ + return { status: "stopped" as const, via: "graceful" as const } + }, + forceStop: () => {}, + }), + createClient: () => { + throw new Error("bad base url") + }, + }), + ).rejects.toThrow(/client construction failed/) + // The live child is never handed out without a client: it is stopped first. + expect(stopped).toBe(1) + }) + + test("a client construction failure reports an unresolved cleanup honestly", async () => { + await expect( + startOpencode({}, undefined, { + getFreePort: async () => 1, + launch: async () => ({ + url: "http://127.0.0.1:1", + pid: 78, + stop: async () => ({ status: "unresolved" as const, reason: "exit was not observed" }), + forceStop: () => {}, + }), + createClient: () => { + throw new Error("boom") + }, + }), + ).rejects.toThrow(/unresolved — exit was not observed/) + }) + + test("bootOpencodeServerFrom owns a helper rooted at the explicit checkout", async () => { + let launchOptions: Record | undefined + let stopped = 0 + const handle = await bootOpencodeServerFrom("/tmp/some-checkout", 1_234, { + deps: { + getFreePort: async () => 4321, + launch: async (options) => { + launchOptions = options as unknown as Record + return { + url: "http://127.0.0.1:4321", + pid: 99, + stop: async () => { + stopped++ + return { status: "stopped" as const, via: "graceful" as const } + }, + forceStop: () => {}, + } + }, + }, + }) + expect(launchOptions?.cwd).toBe("/tmp/some-checkout") + expect(launchOptions?.lifetime).toBe("helper") + expect(launchOptions?.args).toContain("--hostname=127.0.0.1") + expect(launchOptions?.args).toContain("--port=4321") + // A bounded per-call boot closes through the same awaitable stop. + expect(await handle.close()).toEqual({ status: "stopped", via: "graceful" }) + expect(stopped).toBe(1) + }) + + test("an authoring-service boot is explicitly classified and independently persistent", async () => { + let launchOptions: Record | undefined + await bootOpencodeServerFrom("/tmp/checkout", 500, { + lifetime: "authoring-service", + deps: { + getFreePort: async () => 1, + launch: async (options) => { + launchOptions = options as unknown as Record + return { + url: "http://127.0.0.1:1", + pid: 1, + stop: async () => ({ status: "stopped" as const, via: "already-gone" as const }), + forceStop: () => {}, + } + }, + }, + }) + expect(launchOptions?.lifetime).toBe("authoring-service") + }) +}) diff --git a/test/preflight.test.ts b/test/preflight.test.ts index 1445511..b161893 100644 --- a/test/preflight.test.ts +++ b/test/preflight.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test, mock } from "bun:test" import { preflightTargets, validatePreflightTargets } from "../src/preflight-validation" -import { preflightRunPlan } from "../src/preflight" +import { preflightRunPlan, withinPreflightTimeout } from "../src/preflight" import type { ResolvedModel } from "../src/model-routing" import type { RunPlan } from "../src/types" @@ -473,4 +473,37 @@ describe("withinPreflightTimeout edge cases (through preflightRunPlan)", () => { }), ).rejects.toThrow("discovery failure") }) -}) \ No newline at end of file +}) +describe("withinPreflightTimeout cleanup ownership (design D2)", () => { + test("awaits the cancel hook before the timeout rejection settles", async () => { + const controller = new AbortController() + let cancelled = false + const settled = withinPreflightTimeout(new Promise(() => {}), controller.signal, async () => { + await Bun.sleep(20) + cancelled = true + }).then( + () => "resolved", + (error: Error) => error.message, + ) + controller.abort() + expect(await settled).toBe("OpenCode preflight timed out") + // The caller never returns ahead of the bounded owned-server stop. + expect(cancelled).toBe(true) + }) + + test("never waits for the original request to settle", async () => { + const controller = new AbortController() + const pending = withinPreflightTimeout(new Promise(() => {}), controller.signal, async () => {}) + controller.abort() + await expect(pending).rejects.toThrow("OpenCode preflight timed out") + }) + + test("a failing cancel hook never masks the timeout itself", async () => { + const controller = new AbortController() + const pending = withinPreflightTimeout(new Promise(() => {}), controller.signal, async () => { + throw new Error("cancel exploded") + }) + controller.abort() + await expect(pending).rejects.toThrow("OpenCode preflight timed out") + }) +}) diff --git a/test/process-lifecycle.test.ts b/test/process-lifecycle.test.ts new file mode 100644 index 0000000..f869425 --- /dev/null +++ b/test/process-lifecycle.test.ts @@ -0,0 +1,1037 @@ +import { afterAll, describe, expect, test } from "bun:test" +import { mkdir, mkdtemp, readFile, readdir, rm, stat, writeFile } from "node:fs/promises" +import { tmpdir } from "node:os" +import { join } from "node:path" + +import { launchManagedServer, parseReadinessLine } from "../src/managed-server" +import { + captureIdentity, + defaultIdentityProbe, + identityMismatchReason, + linuxProcessIdentity, + macosProcessIdentity, + parseBoottime, + parseProcStat, + parseProcStatusUid, + sameIdentity, + type IdentityObservation, + type IdentityProbe, + type ProcessIdentity, +} from "../src/process-identity" +import { + createProcessRecordStore, + newProcessRecord, + processRecordsDir, + reconcileProcessRecords, + type ProcessRecord, + type ProcessRecordStore, +} from "../src/process-records" +import { sweepPendingLaunches } from "../src/coordinate" +import { cleanupWorkspace } from "../src/workspace" +import { stopTarget } from "../src/process-stop" + +const dirs: string[] = [] +afterAll(async () => Promise.all(dirs.map((dir) => rm(dir, { recursive: true, force: true })))) + +async function scratchDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), "convoy-process-lifecycle-")) + dirs.push(dir) + return dir +} + +const identity = (pid: number, overrides: Partial = {}): ProcessIdentity => ({ + pid, + birth: `boot:${pid}`, + uid: 501, + executable: "opencode", + ...overrides, +}) + +function probeOf(map: Record): IdentityProbe { + return { observe: async (pid) => map[pid] ?? { status: "gone" } } +} + +describe("process identity parsing", () => { + test("parses /proc stat past a comm containing spaces and parentheses", () => { + // comm = "(a b) c)"; field 4 (ppid) = 42, field 22 (starttime) = 12345. + const fields = ["S", "42", "1", "1", "0", "-1", "4194304"] + while (fields.length < 20) fields.push("0") + fields[19] = "12345" + const stat = `7 (a b) c) ${fields.join(" ")}` + expect(parseProcStat(stat)).toEqual({ ppid: 42, startTicks: 12345 }) + }) + + test("rejects a stat line without a usable starttime", () => { + expect(parseProcStat("not a stat line")).toBeUndefined() + }) + + test("reads the real uid from /proc status", () => { + expect(parseProcStatusUid("Name:\topencode\nUid:\t501\t501\t501\t501\n")).toBe(501) + expect(parseProcStatusUid("Name:\topencode\n")).toBeUndefined() + }) + + test("parses the macOS boottime discriminator", () => { + expect(parseBoottime("{ sec = 1787332370, usec = 36769 } Fri Aug 21")).toBe("1787332370.36769") + expect(parseBoottime("nonsense")).toBeUndefined() + }) + + test("low-resolution or partial identities never compare equal", () => { + const expected = identity(7, { birth: "boot:1" }) + expect(sameIdentity(expected, identity(7, { birth: "boot:1" }))).toBe(true) + expect(sameIdentity(expected, identity(7, { birth: "boot:2" }))).toBe(false) + expect(sameIdentity(expected, identity(7, { uid: 0 }))).toBe(false) + expect(identityMismatchReason(expected, identity(7, { birth: "boot:2" }))).toContain("reused") + }) +}) + +describe("platform adapters", () => { + test("macOS observes the current process with a kernel birth token", async () => { + if (process.platform !== "darwin") return + const observation = await macosProcessIdentity(process.pid) + expect(observation.status).toBe("alive") + if (observation.status === "alive") { + expect(observation.identity.pid).toBe(process.pid) + expect(observation.identity.birth).toMatch(/^\d+\.\d+:\d+\.\d+$/) + expect(observation.identity.executable.length).toBeGreaterThan(0) + } + }) + + test("a failed libproc read on a live PID is uncertain, never gone", async () => { + // proc_pidinfo writing nothing models an unreadable/refused PID; the + // process still exists, so the observation must not become kill authority. + const failed = await macosProcessIdentity(process.pid, { proc_pidinfo: () => 0, proc_pidpath: () => 0 }) + expect(failed.status).toBe("unknown") + }) + + test("a failed libproc read reports gone only when the kernel proves absence", async () => { + const gone = await macosProcessIdentity(999_999, { proc_pidinfo: () => 0, proc_pidpath: () => 0 }, () => "absent") + expect(gone.status).toBe("gone") + const unreadable = await macosProcessIdentity(999_999, { proc_pidinfo: () => 0, proc_pidpath: () => 0 }, () => "present") + expect(unreadable.status).toBe("unknown") + }) + + test("linux adapter reports gone only when the kernel proves absence", async () => { + if (process.platform !== "linux") return + const missing = 4_194_000 // beyond typical pid_max; /proc/ does not exist + const gone = await linuxProcessIdentity(missing, () => "absent") + expect(gone.status).toBe("gone") + const hidden = await linuxProcessIdentity(missing, () => "present") + expect(hidden.status).toBe("unknown") + }) + + test("linux adapter reads birth, uid, and executable from a live process", async () => { + if (process.platform !== "linux") return + const observation = await linuxProcessIdentity(process.pid) + expect(observation.status).toBe("alive") + if (observation.status !== "alive") return + const bootId = (await readFile("/proc/sys/kernel/random/boot_id", "utf8")).trim() + expect(observation.identity.pid).toBe(process.pid) + // The birth token must carry the kernel boot discriminator, not just a + // start time: a reboot has to invalidate the recorded incarnation. + expect(observation.identity.birth.startsWith(`${bootId}:`)).toBe(true) + expect(observation.identity.uid).toBe(process.getuid?.() ?? -1) + expect(observation.identity.executable.length).toBeGreaterThan(0) + }) + + test("linux reboot invalidates the recorded incarnation, never authorizing a signal", async () => { + if (process.platform !== "linux") return + const observation = await linuxProcessIdentity(process.pid) + expect(observation.status).toBe("alive") + if (observation.status !== "alive") return + const recorded = observation.identity + const startTicks = recorded.birth.slice(recorded.birth.lastIndexOf(":") + 1) + // Same pid and start ticks, different boot: not the same incarnation. + const afterReboot: ProcessIdentity = { ...recorded, birth: `rebooted-boot:${startTicks}` } + expect(sameIdentity(recorded, afterReboot)).toBe(false) + expect(identityMismatchReason(recorded, afterReboot)).toContain("reused") + }) + + test("captureIdentity gives up non-destructively when a probe cannot answer", async () => { + const captured = await captureIdentity(4242, { observe: async () => ({ status: "unknown", reason: "no probe" }) }, 3) + expect(captured).toBeUndefined() + }) + + test("the default probe is platform-selected and never throws", async () => { + const probe = defaultIdentityProbe("sunos") + const observation = await probe.observe(1) + expect(observation.status).toBe("unknown") + }) +}) + +describe("bounded stop state machine", () => { + test("observes a graceful exit", async () => { + let exited = false + const outcome = await stopTarget( + { + pid: 1, + isExited: () => exited, + waitForExit: async () => { + exited = true + return true + }, + signal: () => {}, + }, + { graceMs: 10, forceObservationMs: 10 }, + ) + expect(outcome).toEqual({ status: "stopped", via: "graceful" }) + }) + + test("escalates to SIGKILL when the target ignores SIGTERM", async () => { + const signals: string[] = [] + let exited = false + const outcome = await stopTarget( + { + pid: 2, + isExited: () => exited, + waitForExit: async () => exited, + signal: (signal) => { + signals.push(signal) + if (signal === "SIGKILL") exited = true + }, + }, + { graceMs: 5, forceObservationMs: 5 }, + ) + expect(signals).toEqual(["SIGTERM", "SIGKILL"]) + expect(outcome).toEqual({ status: "stopped", via: "forced" }) + }) + + test("an already-exited target settles without another signal", async () => { + let signalled = false + const outcome = await stopTarget( + { + pid: 3, + isExited: () => true, + waitForExit: async () => true, + signal: () => { + signalled = true + }, + }, + { graceMs: 5, forceObservationMs: 5 }, + ) + expect(outcome).toEqual({ status: "stopped", via: "already-gone" }) + expect(signalled).toBe(false) + }) + + test("refuses forced escalation when the target no longer verifies", async () => { + const signals: string[] = [] + const outcome = await stopTarget( + { + pid: 4, + isExited: () => false, + waitForExit: async () => false, + signal: (signal) => void signals.push(signal), + verify: async () => ({ ok: false, reason: "pid was reused" }), + }, + { graceMs: 5, forceObservationMs: 5 }, + ) + expect(signals).toEqual(["SIGTERM"]) + expect(outcome.status).toBe("unresolved") + if (outcome.status === "unresolved") expect(outcome.reason).toContain("pid was reused") + }) + + test("reports unresolved rather than claiming a stop it could not observe", async () => { + const outcome = await stopTarget( + { + pid: 5, + isExited: () => false, + waitForExit: async () => false, + signal: () => {}, + }, + { graceMs: 5, forceObservationMs: 5 }, + ) + expect(outcome.status).toBe("unresolved") + }) +}) + +describe("process record storage", () => { + test("publishes atomically with private modes and never stores secrets", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const record = { ...newProcessRecord({ lifetime: "helper" }), runId: "20260101-000000-ab12", url: "http://127.0.0.1:1" } + await store.put(record) + const raw = await readFile(join(store.dir, `${record.id}.json`), "utf8") + expect(JSON.parse(raw)).toMatchObject({ version: 1, lifetime: "helper", state: "provisional" }) + expect(raw).not.toContain("token") + expect(raw).not.toContain("OPENCODE_CONFIG_CONTENT") + expect((await stat(store.dir)).mode & 0o777).toBe(0o700) + expect((await stat(join(store.dir, `${record.id}.json`))).mode & 0o777).toBe(0o600) + // No temp files survive a successful publication. + expect((await readdir(store.dir)).filter((name) => name.endsWith(".tmp"))).toEqual([]) + }) + + test("ignores corrupt and legacy records", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + await mkdir(store.dir, { recursive: true }) + await writeFile(join(store.dir, "bad.json"), "{not json") + await writeFile(join(store.dir, "old.json"), JSON.stringify({ version: 0, id: "old", lifetime: "run" })) + expect(await store.list()).toEqual([]) + expect(await store.get("bad")).toBeUndefined() + }) + + test("serializes concurrent lock attempts without stealing a live lock", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const record = newProcessRecord({ lifetime: "run" }) + let release: (() => void) | undefined + const first = store.withLock(record.id, async () => { + await new Promise((resolve) => (release = resolve)) + return "first" + }) + await Bun.sleep(5) + const second = await store.withLock(record.id, async () => "second", { timeoutMs: 30 }) + expect(second).toBeUndefined() + release?.() + expect(await first).toBe("first") + // The lock is released for the next caller. + expect(await store.withLock(record.id, async () => "third", { timeoutMs: 30 })).toBe("third") + }) +}) + +describe("orphan reconciliation", () => { + async function seed(record: ProcessRecord): Promise<{ store: ReturnType; record: ProcessRecord }> { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + await store.put(record) + return { store, record } + } + + test("stops a verified orphan whose owner is provably gone", async () => { + const child = identity(900, { birth: "boot:900" }) + const owner = identity(901, { birth: "boot:901", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "run" }), owner, child, state: "ready" as const } + const { store } = await seed(record) + const signals: string[] = [] + const observations: Record = { 901: { status: "gone" }, 900: { status: "alive", identity: child } } + const outcome = await reconcileProcessRecords({ + store, + probe: { observe: async (pid) => observations[pid] ?? { status: "gone" } }, + signal: (pid, signal) => { + signals.push(signal) + observations[pid] = { status: "gone" } + }, + policy: { graceMs: 5, forceObservationMs: 5, pollMs: 1 }, + }) + expect(outcome.stopped).toEqual([record.id]) + expect(signals).toEqual(["SIGTERM"]) + expect(await store.get(record.id)).toBeUndefined() + }) + + test("leaves a detached run alone while its owner is alive", async () => { + const child = identity(910, { birth: "boot:910" }) + const owner = identity(911, { birth: "boot:911", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "run" }), owner, child, state: "ready" as const } + const { store } = await seed(record) + const signals: string[] = [] + const outcome = await reconcileProcessRecords({ + store, + probe: probeOf({ 911: { status: "alive", identity: owner }, 910: { status: "alive", identity: child } }), + signal: (_pid, signal) => void signals.push(signal), + }) + expect(outcome.skipped.map((entry) => entry.id)).toEqual([record.id]) + expect(signals).toEqual([]) + expect(await store.get(record.id)).toBeDefined() + }) + + test("never signals a reused child PID", async () => { + const child = identity(920, { birth: "boot:920" }) + const owner = identity(921, { birth: "boot:921", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "helper" }), owner, child, state: "ready" as const } + const { store } = await seed(record) + const signals: string[] = [] + const outcome = await reconcileProcessRecords({ + store, + probe: probeOf({ + 921: { status: "gone" }, + 920: { status: "alive", identity: identity(920, { birth: "boot:other" }) }, + }), + signal: (_pid, signal) => void signals.push(signal), + }) + expect(signals).toEqual([]) + expect(outcome.skipped.length).toBe(1) + }) + + test("an unknown probe is uncertain, never a kill target", async () => { + const child = identity(930, { birth: "boot:930" }) + const owner = identity(931, { birth: "boot:931", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "helper" }), owner, child, state: "ready" as const } + const { store } = await seed(record) + const outcome = await reconcileProcessRecords({ + store, + probe: probeOf({ 931: { status: "unknown", reason: "no /proc" } }), + signal: () => { + throw new Error("must not signal") + }, + }) + expect(outcome.uncertain.map((entry) => entry.id)).toEqual([record.id]) + expect(await store.get(record.id)).toBeDefined() + }) + + test("a legacy record without child identity is never a kill target", async () => { + const owner = identity(941, { birth: "boot:941", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "run" }), owner, state: "ready" as const } + const { store } = await seed(record) + const outcome = await reconcileProcessRecords({ + store, + probe: probeOf({ 941: { status: "gone" } }), + signal: () => { + throw new Error("must not signal") + }, + }) + expect(outcome.uncertain.map((entry) => entry.id)).toEqual([record.id]) + }) + + test("bounds the pass and preserves unprocessed records", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + for (let index = 0; index < 5; index++) { + await store.put({ ...newProcessRecord({ lifetime: "helper", now: index }), state: "ready" }) + } + const outcome = await reconcileProcessRecords({ + store, + probe: { observe: async () => ({ status: "unknown", reason: "slow" }) }, + maxRecords: 2, + }) + expect(outcome.inspected).toBe(2) + expect(outcome.deferred.length).toBe(3) + }) + + test("a passing budget defers without failing startup", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + await store.put(newProcessRecord({ lifetime: "helper" })) + const outcome = await reconcileProcessRecords({ store, budgetMs: -1, probe: { observe: async () => ({ status: "unknown", reason: "x" }) } }) + expect(outcome.deferred).toContain((await store.list())[0]!.id) + }) + + test("two concurrent passes serialize on the record lock and signal once", async () => { + const child = identity(950, { birth: "boot:950" }) + const owner = identity(951, { birth: "boot:951", executable: "bun" }) + const record = { ...newProcessRecord({ lifetime: "run" }), owner, child, state: "ready" as const } + const { store } = await seed(record) + const observations: Record = { 951: { status: "gone" }, 950: { status: "alive", identity: child } } + const probe: IdentityProbe = { + observe: async (pid) => { + // The reconciler's own live lock must never be reclaimed as dead. + if (pid === process.pid) return { status: "alive", identity: identity(process.pid, { executable: "bun" }) } + return observations[pid] ?? { status: "gone" } + }, + } + let signals = 0 + const signal = (pid: number, _signal: string) => { + signals++ + observations[pid] = { status: "gone" } + } + const policy = { graceMs: 5, forceObservationMs: 5, pollMs: 1 } + const [first, second] = await Promise.all([ + reconcileProcessRecords({ store, probe, signal, policy }), + reconcileProcessRecords({ store, probe, signal, policy }), + ]) + expect(signals).toBe(1) + expect(first.stopped.length + second.stopped.length).toBe(1) + }) + + test("the continuation cursor advances so deferred records are not starved", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + for (let index = 0; index < 3; index++) { + await store.put({ ...newProcessRecord({ lifetime: "helper", now: index }), state: "ready" }) + } + const probe: IdentityProbe = { observe: async () => ({ status: "unknown", reason: "slow probe" }) } + const inspected: string[] = [] + for (let pass = 0; pass < 3; pass++) { + const outcome = await reconcileProcessRecords({ store, probe, maxRecords: 1 }) + inspected.push(outcome.uncertain[0]!.id) + } + expect(new Set(inspected).size).toBe(3) + }) + + test("an ancient incomplete record is reported uncertain and removed without a signal", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const record = { + ...newProcessRecord({ lifetime: "run", now: Date.now() - 25 * 60 * 60 * 1_000 }), + state: "ready" as const, + } + await store.put(record) + const outcome = await reconcileProcessRecords({ + store, + probe: { observe: async () => ({ status: "gone" }) }, + signal: () => { + throw new Error("must not signal an incomplete record") + }, + }) + expect(outcome.uncertain.map((entry) => entry.id)).toContain(record.id) + expect(await store.get(record.id)).toBeUndefined() + }) +}) + +describe("managed server launch and stop", () => { + type FakeChild = { + pid: number + stdout: { on(event: string, listener: (chunk: Buffer) => void): void; destroy(): void } + stderr: { on(event: string, listener: (chunk: Buffer) => void): void; destroy(): void } + kill(signal?: string): boolean + once(event: string, listener: (...args: unknown[]) => void): void + on(event: string, listener: (...args: unknown[]) => void): void + removeListener(event: string, listener: (...args: unknown[]) => void): void + } + + function fakeChild(options: { pid?: number; ignoreTerm?: boolean } = {}): { child: FakeChild; emitExit: () => void; emitData: (line: string) => void } { + const exitListeners: Array<(...args: unknown[]) => void> = [] + const dataListeners: Array<(chunk: Buffer) => void> = [] + // The launcher awaits identity capture and record publication before it + // spawns, so a test's emission can precede listener registration. Replay + // everything emitted so far to each late listener. + const pendingData: Buffer[] = [] + let pendingExit = false + let exited = false + const child: FakeChild = { + pid: options.pid ?? 5555, + stdout: { + on: (_event, listener) => { + dataListeners.push(listener) + for (const chunk of pendingData.splice(0)) listener(chunk) + }, + destroy: () => {}, + }, + stderr: { on: () => {}, destroy: () => {} }, + kill: (signal) => { + if (signal === "SIGTERM" && options.ignoreTerm) return true + if (exited) return false + exited = true + pendingExit = true + setTimeout(() => exitListeners.splice(0).forEach((listener) => listener(0, null)), 0) + return true + }, + once: (event, listener) => { + if (event !== "exit") return + exitListeners.push(listener) + if (pendingExit) setTimeout(listener, 0) + }, + on: () => {}, + removeListener: (event, listener) => { + if (event !== "exit") return + const index = exitListeners.indexOf(listener) + if (index !== -1) exitListeners.splice(index, 1) + }, + } + return { + child, + emitExit: () => { + pendingExit = true + exitListeners.splice(0).forEach((listener) => listener(0, null)) + }, + emitData: (line) => { + const chunk = Buffer.from(line) + if (dataListeners.length === 0) pendingData.push(chunk) + else dataListeners.forEach((listener) => listener(chunk)) + }, + } + } + + const probe: IdentityProbe = { observe: async (pid) => ({ status: "alive", identity: identity(pid, { executable: "opencode" }) }) } + + test("parses the strict readiness line and rejects a malformed one", () => { + expect(parseReadinessLine("opencode server listening on http://127.0.0.1:4123")).toEqual({ url: "http://127.0.0.1:4123" }) + expect(parseReadinessLine("opencode server listening somewhere")).toBe("malformed") + expect(parseReadinessLine("some other output")).toBeUndefined() + }) + + test("publishes ownership before resolving and stops idempotently", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = fakeChild() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe, store, policy: { graceMs: 5, forceObservationMs: 5 } }, + }) + emitData("opencode server listening on http://127.0.0.1:9999\n") + const server = await serverPromise + expect(server.url).toBe("http://127.0.0.1:9999") + expect(server.recordId).toBeDefined() + const records = await store.list() + expect(records.length).toBe(1) + expect(records[0]!.state).toBe("ready") + expect(records[0]!.child?.pid).toBe(5555) + + const [first, second] = await Promise.all([server.stop(), server.stop()]) + expect(first.status).toBe("stopped") + expect(second.status).toBe("stopped") + // A confirmed stop removes the transient record. + expect(await store.list()).toEqual([]) + }) + + test("a malformed readiness line fails the boot after bounded cleanup", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = fakeChild() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe, store, policy: { graceMs: 5, forceObservationMs: 5 } }, + }) + emitData("opencode server listening somewhere\n") + await expect(serverPromise).rejects.toThrow(/malformed readiness/) + }) + + test("bounds an unterminated stdout line without losing a later readiness line", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = fakeChild() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe, store, policy: { graceMs: 5, forceObservationMs: 5 } }, + }) + // A chatty child that never emits a newline must not grow the line buffer + // without bound. The cap keeps only the tail, so a readiness line on its + // own line still parses. + emitData("x".repeat(200_000)) + emitData("\nopencode server listening on http://127.0.0.1:4321\n") + const server = await serverPromise + expect(server.url).toBe("http://127.0.0.1:4321") + await server.stop() + }) + + test("an early exit fails the boot and leaves no record", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitExit } = fakeChild() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe, store, policy: { graceMs: 5, forceObservationMs: 5 } }, + }) + emitExit() + await expect(serverPromise).rejects.toThrow(/exited before it was ready/) + await Bun.sleep(10) + expect(await store.list()).toEqual([]) + }) + + test("a pre-aborted signal never spawns", async () => { + const root = await scratchDir() + const controller = new AbortController() + controller.abort(new Error("cancelled")) + let spawned = false + await expect( + launchManagedServer({ + command: "opencode", + args: [], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + signal: controller.signal, + deps: { + spawn: () => { + spawned = true + return fakeChild().child as never + }, + }, + }), + ).rejects.toThrow(/cancelled/) + expect(spawned).toBe(false) + }) + + test("an authoring-service launch publishes no reconciliation record", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = fakeChild() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "authoring-service", + label: "authoring service", + deps: { spawn: () => child as never, probe, store, policy: { graceMs: 5, forceObservationMs: 5 } }, + }) + emitData("opencode server listening on http://127.0.0.1:8888\n") + const server = await serverPromise + expect(server.recordId).toBeUndefined() + expect(await store.list()).toEqual([]) + await server.stop() + }) +}) + +describe("managed server ownership evidence and startup recovery", () => { + // Reuse the fake-child shape by redeclaring a minimal version here; the + // launcher only needs pid/stdout/stderr/kill/once/on/removeListener. + function childFixture(options: { pid?: number; ignoreTerm?: boolean; ignoreKill?: boolean } = {}) { + const exitListeners: Array<(...args: unknown[]) => void> = [] + const dataListeners: Array<(chunk: Buffer) => void> = [] + const pendingData: Buffer[] = [] + let pendingExit = false + let exited = false + let killed = 0 + const child = { + pid: options.pid ?? 6001, + stdout: { + on: (_event: string, listener: (chunk: Buffer) => void) => { + dataListeners.push(listener) + for (const chunk of pendingData.splice(0)) listener(chunk) + }, + destroy: () => {}, + }, + stderr: { on: () => {}, destroy: () => {} }, + kill: (signal?: string) => { + killed++ + if (signal === "SIGTERM" && options.ignoreTerm) return true + if (options.ignoreKill) return true + if (exited) return false + exited = true + pendingExit = true + setTimeout(() => exitListeners.splice(0).forEach((listener) => listener(0, null)), 0) + return true + }, + once: (event: string, listener: (...args: unknown[]) => void) => { + if (event !== "exit") return + exitListeners.push(listener) + if (pendingExit) setTimeout(listener, 0) + }, + on: () => {}, + removeListener: (event: string, listener: (...args: unknown[]) => void) => { + if (event !== "exit") return + const index = exitListeners.indexOf(listener) + if (index !== -1) exitListeners.splice(index, 1) + }, + get killedTimes() { + return killed + }, + } + return { + child, + emitData: (line: string) => { + const chunk = Buffer.from(line) + if (dataListeners.length === 0) pendingData.push(chunk) + else dataListeners.forEach((listener) => listener(chunk)) + }, + } + } + + const probe: IdentityProbe = { observe: async (pid) => ({ status: "alive", identity: identity(pid, { executable: "opencode" }) }) } + const stopPolicy = { graceMs: 5, forceObservationMs: 5 } + + test("refuses to spawn when the provisional ownership record cannot be persisted", async () => { + const root = await scratchDir() + // A file where the record directory must be makes `mkdir`/`put` fail. + await writeFile(join(root, "processes"), "not a directory") + const store = createProcessRecordStore(join(root, "processes")) + let spawned = false + await expect( + launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { + spawn: () => { + spawned = true + return childFixture().child as never + }, + probe, + store, + }, + }), + ).rejects.toThrow(/ownership cannot be persisted/) + expect(spawned).toBe(false) + }) + + test("refuses readiness and cleans up when the child identity cannot be captured", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = childFixture() + const unknownProbe: IdentityProbe = { observe: async () => ({ status: "unknown", reason: "no probe" }) } + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe: unknownProbe, store, policy: stopPolicy }, + }) + emitData("opencode server listening on http://127.0.0.1:7777\n") + await expect(serverPromise).rejects.toThrow(/identity could not be captured/) + // The still-owned child is cleaned up and its transient record removed. + expect(child.killedTimes).toBe(1) + await Bun.sleep(10) + expect(await store.list()).toEqual([]) + }) + + test("refuses readiness and cleans up when publishing the child identity fails", async () => { + const root = await scratchDir() + const real = createProcessRecordStore(join(root, "processes")) + let puts = 0 + const flaky: ProcessRecordStore = { + ...real, + put: async (record: ProcessRecord) => { + puts++ + if (puts >= 2) throw new Error("no space left on device") + await real.put(record) + }, + } + const { child, emitData } = childFixture() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "run", + label: "test run server", + deps: { spawn: () => child as never, probe, store: flaky, policy: stopPolicy }, + }) + emitData("opencode server listening on http://127.0.0.1:7778\n") + await expect(serverPromise).rejects.toThrow(/ownership could not be published/) + expect(child.killedTimes).toBe(1) + }) + + test("runs a bounded reconciliation pass before a run/helper boot", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const order: string[] = [] + const passes: Array<{ store: unknown; probe: unknown }> = [] + const { child, emitData } = childFixture() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { + spawn: () => { + order.push("spawn") + return child as never + }, + probe, + store, + reconcile: async (deps) => { + order.push("reconcile") + passes.push(deps) + }, + policy: stopPolicy, + }, + }) + emitData("opencode server listening on http://127.0.0.1:6666\n") + const server = await serverPromise + expect(order).toEqual(["reconcile", "spawn"]) + expect(passes.length).toBe(1) + expect(passes[0]!.store).toBe(store) + expect(passes[0]!.probe).toBe(probe) + await server.stop() + }) + + test("an authoring-service boot runs no reconciliation pass", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + let passes = 0 + const { child, emitData } = childFixture() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "authoring-service", + label: "authoring service", + deps: { + spawn: () => child as never, + probe, + store, + reconcile: async () => { + passes++ + }, + policy: stopPolicy, + }, + }) + emitData("opencode server listening on http://127.0.0.1:6667\n") + const server = await serverPromise + expect(passes).toBe(0) + await server.stop() + }) + + test("a failing reconciliation pass does not fail an unrelated launch", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = childFixture() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { + spawn: () => child as never, + probe, + store, + reconcile: async () => { + throw new Error("slow record could not be classified") + }, + policy: stopPolicy, + }, + }) + emitData("opencode server listening on http://127.0.0.1:6668\n") + const server = await serverPromise + expect(server.url).toBe("http://127.0.0.1:6668") + await server.stop() + }) + + test("an unobserved stop retains the record as unresolved evidence", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + // A child that ignores both signals: the stop cannot confirm exit and must + // not claim one (design D2/D5). + const { child, emitData } = childFixture({ ignoreKill: true }) + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { spawn: () => child as never, probe, store, policy: stopPolicy }, + }) + emitData("opencode server listening on http://127.0.0.1:4444\n") + const server = await serverPromise + const outcome = await server.stop() + expect(outcome.status).toBe("unresolved") + const record = await store.get(server.recordId!) + expect(record?.state).toBe("unresolved") + expect(record?.lastOutcome).toContain("not observed") + }) + + test("a helper stopped under a shared budget uses the resolver and cannot restart it", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const { child, emitData } = childFixture({ ignoreKill: true }) + let policyCalls = 0 + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + deps: { + spawn: () => child as never, + probe, + store, + // The shared budget a coordinator under its deadline would report: + // no time left, so no fresh graceful/observation window (design D2). + policy: () => { + policyCalls++ + return { graceMs: 0, forceObservationMs: 0 } + }, + }, + }) + emitData("opencode server listening on http://127.0.0.1:7777\n") + const server = await serverPromise + const started = Date.now() + const outcome = await server.stop() + const elapsed = Date.now() - started + // The resolver is evaluated at stop time, and the default 2s+1s standalone + // allowance was not restarted: an unresponsive child settles unresolved in + // well under it. + expect(policyCalls).toBeGreaterThan(0) + expect(outcome.status).toBe("unresolved") + expect(elapsed).toBeLessThan(500) + }) + + test("a signal aborted during startup still cleans up the owned child", async () => { + const root = await scratchDir() + const store = createProcessRecordStore(join(root, "processes")) + const controller = new AbortController() + const { child } = childFixture() + const serverPromise = launchManagedServer({ + command: "opencode", + args: ["serve"], + cwd: root, + env: {}, + lifetime: "helper", + label: "test helper", + signal: controller.signal, + deps: { + spawn: () => child as never, + probe, + store, + // Cancel during the pre-spawn gap: the abort event never replays, so + // the launcher must recheck `aborted` after spawn to avoid exposing a + // live server after cancellation won (design D1). + reconcile: async () => { + controller.abort(new Error("cancelled")) + }, + policy: stopPolicy, + }, + }) + await expect(serverPromise).rejects.toThrow(/cancelled/) + expect(child.killedTimes).toBe(1) + await Bun.sleep(10) + expect(await store.list()).toEqual([]) + }) +}) + +describe("process records survive ordinary cleanup", () => { + test("an unresolved record outlives a pending-launch sweep and its workspace removal", async () => { + const root = await scratchDir() + const home = join(root, "home") + const previousHome = process.env.CONVOY_HOME + process.env.CONVOY_HOME = home + try { + // The record lives under /.convoy/processes, independently + // of the disposable `pending/` and run workspaces (design D5). + const store = createProcessRecordStore(processRecordsDir()) + const record: ProcessRecord = { + ...newProcessRecord({ lifetime: "run" }), + state: "unresolved", + runId: "20260101-000000-ab12", + child: identity(4321), + } + await store.put(record) + + // A dead-pid pending launch dir is swept away. + const pendingRoot = join(home, ".convoy", "pending") + const dirName = "22222222-2222-4222-8222-222222222222" + await mkdir(join(pendingRoot, dirName), { recursive: true }) + const gone = Bun.spawn(["true"]) + await gone.exited + await writeFile(join(pendingRoot, dirName, "pid"), String(gone.pid)) + await sweepPendingLaunches(pendingRoot) + expect(await readdir(pendingRoot)).toEqual([]) + + // The run workspace is removed as ordinary cleanup does. + const runDir = join(home, ".convoy", "runs", record.runId!) + await mkdir(runDir, { recursive: true }) + await cleanupWorkspace({ dir: runDir, runID: record.runId! }) + + // Unresolved evidence is retained for a later reconciliation pass. + const retained = await store.get(record.id) + expect(retained?.state).toBe("unresolved") + expect(retained?.child?.pid).toBe(4321) + expect(await readFile(join(processRecordsDir(), `${record.id}.json`), "utf8")).toContain(record.runId!) + } finally { + if (previousHome === undefined) delete process.env.CONVOY_HOME + else process.env.CONVOY_HOME = previousHome + } + }) +}) diff --git a/test/process-teardown.test.ts b/test/process-teardown.test.ts new file mode 100644 index 0000000..6f53db2 --- /dev/null +++ b/test/process-teardown.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, test } from "bun:test" +import { mkdtemp, rm } from "node:fs/promises" +import { tmpdir } from "node:os" +import { join } from "node:path" + +import type { IdentityObservation, IdentityProbe, ProcessIdentity } from "../src/process-identity" +import { createProcessRecordStore, newProcessRecord, type ProcessRecord } from "../src/process-records" +import { stopOwnedTestServers } from "./process-teardown" + +/** This test process, as the probe reports it — the owner of the records under test. */ +const self: ProcessIdentity = { pid: process.pid, birth: "boot:self", uid: 501, executable: "bun" } +const child: ProcessIdentity = { pid: 4242, birth: "boot:child", uid: 501, executable: "opencode" } +const foreignOwner: ProcessIdentity = { pid: 999, birth: "boot:foreign", uid: 501, executable: "bun" } + +type ProbeState = { + /** Identity reported for each pid; a pid absent from here observes as gone. */ + identities: Map + alive: Set + /** Pids that answer with an inconclusive `unknown` instead of alive/gone. */ + unknown?: Set +} + +function fakeProbe(state: ProbeState): IdentityProbe { + return { + async observe(pid): Promise { + if (state.unknown?.has(pid)) return { status: "unknown", reason: `inconclusive pid ${pid}` } + const identity = state.identities.get(pid) + if (!identity) return { status: "gone" } + return state.alive.has(pid) ? { status: "alive", identity } : { status: "gone" } + }, + } +} + +async function withTempDir(fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), "convoy-teardown-test-")) + try { + return await fn(dir) + } finally { + await rm(dir, { recursive: true, force: true }) + } +} + +function record(overrides: Partial & Pick): ProcessRecord { + return { + ...newProcessRecord({ lifetime: "helper" }), + state: "ready", + url: "http://127.0.0.1:1", + ...overrides, + } +} + +const fastPolicy = { graceMs: 200, forceObservationMs: 200, pollMs: 10 } as const + +describe("stopOwnedTestServers", () => { + test("stops a live child this process recorded and removes its record", async () => { + await withTempDir(async (dir) => { + const store = createProcessRecordStore(dir) + const published = record({ owner: self, child }) + await store.put(published) + + const state: ProbeState = { + identities: new Map([ + [self.pid, self], + [child.pid, child], + ]), + alive: new Set([self.pid, child.pid]), + } + const signals: Array<{ pid: number; signal: string }> = [] + const outcomes: string[] = [] + + const stopped = await stopOwnedTestServers(dir, { + store, + probe: fakeProbe(state), + signal: (pid, signal) => { + signals.push({ pid, signal }) + if (pid === child.pid && signal === "SIGTERM") state.alive.delete(child.pid) + }, + onOutcome: (_id, outcome) => outcomes.push(outcome.status), + policy: fastPolicy, + }) + + expect(stopped).toEqual([published.id]) + expect(signals).toEqual([{ pid: child.pid, signal: "SIGTERM" }]) + expect(outcomes).toEqual(["stopped"]) + expect(await store.get(published.id)).toBeUndefined() + }) + }) + + test("never signals a child owned by another process", async () => { + await withTempDir(async (dir) => { + const store = createProcessRecordStore(dir) + const published = record({ owner: foreignOwner, child }) + await store.put(published) + + const state: ProbeState = { + identities: new Map([ + [self.pid, self], + [child.pid, child], + ]), + alive: new Set([self.pid, child.pid, foreignOwner.pid]), + } + const signals: number[] = [] + + const stopped = await stopOwnedTestServers(dir, { + store, + probe: fakeProbe(state), + signal: (pid) => signals.push(pid), + policy: fastPolicy, + }) + + expect(stopped).toEqual([]) + expect(signals).toEqual([]) + expect(await store.get(published.id)).toBeDefined() + }) + }) + + test("removes a record whose child already exited, without signaling", async () => { + await withTempDir(async (dir) => { + const store = createProcessRecordStore(dir) + const published = record({ owner: self, child }) + await store.put(published) + + const state: ProbeState = { + identities: new Map([ + [self.pid, self], + [child.pid, child], + ]), + alive: new Set([self.pid]), + } + const signals: number[] = [] + + const stopped = await stopOwnedTestServers(dir, { + store, + probe: fakeProbe(state), + signal: (pid) => signals.push(pid), + policy: fastPolicy, + }) + + expect(stopped).toEqual([]) + expect(signals).toEqual([]) + expect(await store.get(published.id)).toBeUndefined() + }) + }) + + test("leaves evidence untouched when this process cannot prove its own identity", async () => { + await withTempDir(async (dir) => { + const store = createProcessRecordStore(dir) + const published = record({ owner: self, child }) + await store.put(published) + + const state: ProbeState = { + identities: new Map([ + [self.pid, self], + [child.pid, child], + ]), + alive: new Set([self.pid, child.pid]), + unknown: new Set([self.pid]), + } + const signals: number[] = [] + + const stopped = await stopOwnedTestServers(dir, { + store, + probe: fakeProbe(state), + signal: (pid) => signals.push(pid), + policy: fastPolicy, + }) + + expect(stopped).toEqual([]) + expect(signals).toEqual([]) + expect(await store.get(published.id)).toBeDefined() + }) + }) +}) diff --git a/test/process-teardown.ts b/test/process-teardown.ts new file mode 100644 index 0000000..4c37a1c --- /dev/null +++ b/test/process-teardown.ts @@ -0,0 +1,97 @@ +/** + * Test-only lifecycle safety net (change `fix-opencode-server-lifecycle`). + * + * A test that abandons an in-flight helper — for example a real branch-name + * proposal whose promise outlives the test file — leaves its owned + * `opencode serve` child alive after `bun test` exits. Because the harness + * points `CONVOY_HOME` at a throwaway directory (`test/env.ts`), the + * production reconciliation under the developer's real `~/.convoy/processes/` + * can never see that record once the run ends: the orphan would be + * unattributable from then on. + * + * This teardown runs while the owner (this test process) is still alive, so it + * can stop exactly the children this process recorded — never a PID it did not + * spawn — and remove their records. It revalidates the recorded ownership and + * child incarnation immediately before the signal, the same way the production + * stop paths do. + */ + +import { captureIdentity, defaultIdentityProbe, sameIdentity, type IdentityProbe } from "../src/process-identity" +import { createProcessRecordStore, processRecordsDir, type ProcessRecordStore } from "../src/process-records" +import { observedStopTarget, stopTarget, type StopOutcome, type StopPolicy } from "../src/process-stop" + +export type StopOwnedTestServersDeps = { + /** Store to sweep; defaults to the process-records dir under the test `CONVOY_HOME`. */ + store?: ProcessRecordStore + probe?: IdentityProbe + /** Injectable signal delivery, so the unit test never touches a real process. */ + signal?: (pid: number, signal: "SIGTERM" | "SIGKILL") => void + policy?: StopPolicy + /** Diagnostics sink; defaults to silent because this runs during teardown. */ + onOutcome?: (recordId: string, outcome: StopOutcome) => void +} + +/** + * Stops every recorded `run`/`helper` child owned by this process and removes + * the resolved records. Returns the ids it confirmed stopped. Records owned by + * another process, already-gone children, and children whose incarnation no + * longer matches the record are left in a safe state (matched records whose + * child is gone are removed; everything else is untouched). + */ +export async function stopOwnedTestServers( + dir: string = processRecordsDir(), + deps: StopOwnedTestServersDeps = {}, +): Promise { + const store = deps.store ?? createProcessRecordStore(dir) + const records = await store.list().catch(() => []) + if (records.length === 0) return [] + + const probe = deps.probe ?? defaultIdentityProbe() + const signal = deps.signal ?? ((pid, sig) => process.kill(pid, sig)) + const self = await captureIdentity(process.pid, probe).catch(() => undefined) + // Without our own identity we cannot prove ownership; leave evidence intact + // rather than signal a child we cannot attribute to this process. + if (!self) return [] + + const stopped: string[] = [] + for (const record of records) { + const child = record.child + if (!record.owner || !child) continue + if (!sameIdentity(record.owner, self)) continue + + const observed = await probe.observe(child.pid) + if (observed.status === "gone") { + await store.remove(record.id) + continue + } + if (observed.status !== "alive" || !sameIdentity(child, observed.identity)) continue + + const verify = async (): Promise<{ ok: true } | { ok: false; reason: string }> => { + const recheck = await probe.observe(child.pid) + if (recheck.status === "gone") return { ok: true } + if (recheck.status !== "alive") return { ok: false, reason: `child identity became ${recheck.status}` } + return sameIdentity(child, recheck.identity) + ? { ok: true } + : { ok: false, reason: "child incarnation changed before escalation" } + } + + const outcome = await stopTarget( + observedStopTarget({ + pid: child.pid, + observe: async () => { + const now = await probe.observe(child.pid) + return now.status === "mismatch" ? "unknown" : now.status + }, + signal: (sig) => signal(child.pid, sig), + verify, + }), + deps.policy, + ) + deps.onOutcome?.(record.id, outcome) + if (outcome.status === "stopped") { + await store.remove(record.id) + stopped.push(record.id) + } + } + return stopped +} diff --git a/test/propose-service.test.ts b/test/propose-service.test.ts new file mode 100644 index 0000000..86f7c60 --- /dev/null +++ b/test/propose-service.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, test } from "bun:test" + +import { findProposalCommand, startProposalCommand, type ProposalCommandDeps } from "../src/propose-service" + +/** + * Proposal-command ownership (change fix-opencode-server-lifecycle, design + * D1/D6). These tests fail if the discovery helper is not stopped on an early + * return or if the independent service is not established before the command + * runs. + */ + +function recorder(overrides: Partial = {}) { + const events: string[] = [] + const base: ProposalCommandDeps = { + listCommands: async () => { + events.push("list") + return ["opsx-propose"] + }, + transfer: async () => { + events.push("transfer") + return { status: "live" as const, url: "http://service" } + }, + stopHelper: async () => { + events.push("stopHelper") + }, + acquireClaim: async () => { + events.push("claim") + return { ok: true as const } + }, + releaseClaim: async () => { + events.push("releaseClaim") + }, + createConversation: async () => { + events.push("create") + return { harness: "opencode" as const, sessionId: "ses_1" } + }, + invokeCommand: async ({ command }) => { + events.push(`invoke:${command}`) + }, + } + return { events, deps: { ...base, ...overrides } } +} + +const helper = { kind: "helper", url: "http://helper" } as const +const independent = { kind: "independent", url: "http://service" } as const + +describe("findProposalCommand", () => { + test("prefers opsx-propose over any other propose-suffixed command", () => { + expect(findProposalCommand(["propose", "opsx-propose"])).toBe("opsx-propose") + expect(findProposalCommand(["help", "my-propose"])).toBe("my-propose") + expect(findProposalCommand(["help"])).toBeUndefined() + }) +}) + +describe("startProposalCommand fallback ownership", () => { + test("unknown command discovery stops the helper and never claims or invokes", async () => { + const { events, deps } = recorder({ + listCommands: async () => { + events.push("list") + return "unknown" + }, + }) + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("blocked") + expect(events).toEqual(["list", "stopHelper"]) + if (outcome.status === "blocked") expect(outcome.reason).toContain("could not be discovered") + }) + + test("no supported command stops the helper", async () => { + const { events, deps } = recorder({ + listCommands: async () => { + events.push("list") + return ["help"] + }, + }) + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("blocked") + expect(events).toEqual(["list", "stopHelper"]) + }) + + test("a failed independent-service transfer stops the helper and releases the claim before blocking", async () => { + const { events, deps } = recorder({ + transfer: async () => { + events.push("transfer") + return { status: "unavailable" as const, reason: "publish failed" } + }, + }) + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("blocked") + // The helper is stopped before the claim is released, and the command + // never runs. + expect(events).toEqual(["list", "claim", "transfer", "stopHelper", "releaseClaim"]) + if (outcome.status === "blocked") expect(outcome.reason).toContain("publish failed") + }) + + test("a successful transfer stops the unused helper and invokes under the independent service", async () => { + const { events, deps } = recorder() + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("started") + // Transfer strictly precedes command invocation; the helper is stopped + // before either the conversation is created or the command runs. + expect(events).toEqual(["list", "claim", "transfer", "stopHelper", "create", "invoke:opsx-propose"]) + expect(events.indexOf("transfer")).toBeLessThan(events.indexOf("invoke:opsx-propose")) + expect(events.indexOf("stopHelper")).toBeLessThan(events.indexOf("invoke:opsx-propose")) + if (outcome.status === "started") expect(outcome.service.url).toBe("http://service") + }) + + test("an independent service is used as-is: no transfer and no helper stop", async () => { + const { events, deps } = recorder() + const outcome = await startProposalCommand(independent, deps) + expect(outcome.status).toBe("started") + expect(events).toEqual(["list", "claim", "create", "invoke:opsx-propose"]) + expect(events).not.toContain("transfer") + expect(events).not.toContain("stopHelper") + if (outcome.status === "started") expect(outcome.service.url).toBe("http://service") + }) + + test("a lost writer claim stops the helper before blocking", async () => { + const { events, deps } = recorder({ + acquireClaim: async () => { + events.push("claim") + return { ok: false as const, reason: "a managed writer already owns this checkout", remediation: ["stop it first"] } + }, + }) + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("blocked") + expect(events).toEqual(["list", "claim", "stopHelper"]) + if (outcome.status === "blocked") { + expect(outcome.reason).toContain("managed writer") + expect(outcome.remediation).toEqual(["stop it first"]) + } + }) + + test("a throwing discovery still stops the owned helper before the error escapes", async () => { + let stops = 0 + const { deps } = recorder({ + listCommands: async () => { + throw new Error("command discovery exploded") + }, + stopHelper: async () => { + stops += 1 + }, + }) + await expect(startProposalCommand(helper, deps)).rejects.toThrow("command discovery exploded") + // The pre-claim path now shares the function-level finally, so a throw + // can never skip the owned helper's bounded stop. + expect(stops).toBe(1) + }) + + test("a throwing writer-claim acquisition still stops the owned helper", async () => { + let stops = 0 + const { deps } = recorder({ + acquireClaim: async () => { + throw new Error("claim store unavailable") + }, + stopHelper: async () => { + stops += 1 + }, + }) + await expect(startProposalCommand(helper, deps)).rejects.toThrow("claim store unavailable") + expect(stops).toBe(1) + }) + + test("a conversation failure after a successful transfer stops the helper once and releases the claim", async () => { + let stops = 0 + const { events, deps } = recorder({ + stopHelper: async () => { + stops += 1 + events.push("stopHelper") + }, + createConversation: async () => { + events.push("create") + throw new Error("session create failed") + }, + }) + const outcome = await startProposalCommand(helper, deps) + expect(outcome.status).toBe("blocked") + expect(stops).toBe(1) + expect(events).toEqual(["list", "claim", "transfer", "stopHelper", "create", "releaseClaim"]) + if (outcome.status === "blocked") expect(outcome.reason).toContain("session create failed") + }) +}) diff --git a/test/runner-hosted.test.ts b/test/runner-hosted.test.ts index de7dd5a..7c50a39 100644 --- a/test/runner-hosted.test.ts +++ b/test/runner-hosted.test.ts @@ -36,11 +36,16 @@ let throwPrimitiveFromStart = false const fakeStartOpencode: RunDeps["startOpencode"] = async () => { if (throwPrimitiveFromStart) throw "primitive boom" - return { - client: fakeClient as never, - url: "http://127.0.0.1:41234", - close: () => {}, - } + return fakeOpencodeHandle(fakeClient, "http://127.0.0.1:41234") +} + +/** + * A stop-able fake handle. Owned servers now return an awaited, idempotent + * `stop()`, so every fake must provide one; these tests never spawn a child. + */ +function fakeOpencodeHandle(client: unknown, url: string) { + const stop = async (): Promise<{ status: "stopped"; via: "already-gone" }> => ({ status: "stopped", via: "already-gone" }) + return { client: client as never, url, pid: 0, stop, forceStop: () => {}, close: stop } } // Bind the fake opencode handle so every test in this file exercises the @@ -164,6 +169,30 @@ describe("run() with a hosted progress", () => { } }) + test("an unresolved server stop keeps live-server metadata instead of reporting a clean shutdown", async () => { + const repo = await cleanRepo() + const dashboard = fakeDashboard() + // Awaiting stop() is not proof of exit: a stop that cannot confirm the + // child's exit must leave the live-server pointer in place as evidence. + const unresolvedStop: RunDeps["startOpencode"] = async () => ({ + client: fakeClient as never, + url: "http://127.0.0.1:41237", + pid: 0, + stop: async () => ({ status: "unresolved" as const, reason: "exit was not observed within the budget" }), + forceStop: () => {}, + close: async () => ({ status: "unresolved" as const, reason: "exit was not observed within the budget" }), + }) + try { + const result = await realRun(makeOptions(repo, { progress: dashboard.progress }), { startOpencode: unresolvedStop }) + await result.release?.() + const after = JSON.parse(await readFile(join(result.dir, "metadata.json"), "utf8")) + expect(after.server).toBeDefined() + expect(after.server.url).toBe("http://127.0.0.1:41237") + } finally { + await rm(repo, { recursive: true, force: true }) + } + }) + test("keeps the Herdr agent claimed through the finish hold and only then release-agent", async () => { // Hosted run() used to herdr.stop() in its finally — before the coordinator // holds the finish screen — so Herdr dropped Convoy from the agents list @@ -380,11 +409,7 @@ describe("run() with a hosted progress", () => { }, }, } - const gatedStart: RunDeps["startOpencode"] = async () => ({ - client: permissionClient as never, - url: "http://127.0.0.1:41235", - close: () => {}, - }) + const gatedStart: RunDeps["startOpencode"] = async () => fakeOpencodeHandle(permissionClient, "http://127.0.0.1:41235") const progress: ProgressUI = { ...noopProgress, @@ -433,7 +458,7 @@ describe("run() with a hosted progress", () => { let captured: Awaited[0]> | undefined const capturingStart: RunDeps["startOpencode"] = async (config) => { captured = config - return { client: fakeClient as never, url: "http://127.0.0.1:41235", close: () => {} } + return fakeOpencodeHandle(fakeClient, "http://127.0.0.1:41235") } try { const options = makeOptions(repo, { @@ -477,7 +502,7 @@ describe("run() with a hosted progress", () => { let captured: Awaited[0]> | undefined const capturingStart: RunDeps["startOpencode"] = async (config) => { captured = config - return { client: fakeClient as never, url: "http://127.0.0.1:41236", close: () => {} } + return fakeOpencodeHandle(fakeClient, "http://127.0.0.1:41236") } try { const options = makeOptions(repo, { diff --git a/test/runner.test.ts b/test/runner.test.ts index 019bde7..0c58d25 100644 --- a/test/runner.test.ts +++ b/test/runner.test.ts @@ -23,6 +23,7 @@ import { describeSessionActivity, extractAssistantText, finalizePhaseRepository, + installShutdownSignals, isIgnorableRejection, isMessageAbortedError, isUserAbortError, @@ -3294,6 +3295,37 @@ describe("isIgnorableRejection", () => { }) }) +describe("installShutdownSignals owner scope", () => { + test("registers and removes one handler for SIGINT, SIGTERM, and SIGHUP", () => { + const signals = ["SIGHUP", "SIGINT", "SIGTERM"] as const + // Erase the per-signal listener type so identity deltas compare cleanly. + const listenersOf = (signal: string) => process.listeners(signal as never) as unknown as Array<(...args: unknown[]) => unknown> + + const shutdown = trackedShutdown() + const before = new Map(signals.map((signal) => [signal, new Set(listenersOf(signal))])) + + const remove = installShutdownSignals(shutdown) + for (const signal of signals) { + const added = listenersOf(signal).filter((listener) => !before.get(signal)!.has(listener)) + expect(added).toHaveLength(1) + } + + // Invoke the captured SIGHUP handler directly instead of emitting a + // process-wide signal: a detached coordinator only ever sees a HUP + // delivered to its own process, and emitting here would abort unrelated + // in-flight runs sharing the test process. + const hup = listenersOf("SIGHUP").find((listener) => !before.get("SIGHUP")!.has(listener)) as (signal: number) => void + hup(1) + expect(shutdown.aborted).toBe(true) + + remove() + for (const signal of signals) { + expect(listenersOf(signal).filter((listener) => !before.get(signal)!.has(listener))).toHaveLength(0) + } + shutdown.dispose() + }) +}) + describe("RunShutdown methods", () => { test("signal returns an AbortSignal", () => { const shutdown = trackedShutdown() @@ -3822,3 +3854,86 @@ describe("watchSession transcript backfill", () => { } }) }) + +describe("RunShutdown force path", () => { + test("repeated requests force the owned server once and exit after the bounded observation", async () => { + const exits: number[] = [] + let forced = 0 + const shutdown = new RunShutdown({ exit: (code) => void exits.push(code) }) + shutdown.setForceHandler(() => { + forced++ + }) + shutdown.request("SIGINT") + shutdown.request("SIGINT") + shutdown.request("SIGINT") + // Idempotent: a burst of signals never repeats the destructive edge or + // exits immediately. + expect(forced).toBe(1) + expect(exits).toEqual([]) + await Bun.sleep(1_100) + expect(exits).toEqual([130]) + shutdown.dispose() + }, 10_000) + + test("the shared helper budget clamps to the coordinator's remaining deadline", () => { + let now = 0 + const shutdown = new RunShutdown({ exit: () => {}, graceMs: 10_000, forceObservationMs: 1_000, now: () => now }) + // Before any request there is no coordinator shutdown in progress, so a + // helper keeps the standalone defaults. + expect(shutdown.stopBudget()).toEqual({}) + + shutdown.request("SIGINT") + // At the instant of the request the full standalone allowance fits inside + // the remaining deadline. + expect(shutdown.stopBudget()).toEqual({ graceMs: 2_000, forceObservationMs: 1_000 }) + + // Near the deadline the windows shrink so their sum never exceeds what is + // left — cleanup cannot restart a fresh budget or run past the deadline. + now = 9_000 + expect(shutdown.stopBudget()).toEqual({ graceMs: 1_000, forceObservationMs: 0 }) + now = 10_000 + expect(shutdown.stopBudget()).toEqual({ graceMs: 0, forceObservationMs: 0 }) + now = 12_000 + expect(shutdown.stopBudget()).toEqual({ graceMs: 0, forceObservationMs: 0 }) + shutdown.dispose() + }) + + test("a hung session cancellation still yields bounded termination via the deadline force path", async () => { + const exits: number[] = [] + let forced = 0 + // Injected timers (design D2): the production 15s deadline and 1s + // observation are compressed so the test never waits them out. + const shutdown = new RunShutdown({ exit: (code) => void exits.push(code), graceMs: 25, forceObservationMs: 25 }) + shutdown.setForceHandler(() => { + forced++ + }) + // An explicitly aborted run whose session-cancellation request never + // answers (spec R4: "Session cancellation does not answer"). + shutdown.setActiveSession({ + client: { session: { abort: () => new Promise(() => {}) } } as never, + sessionID: "ses_hung", + directory: "/tmp", + phaseName: "implementer", + }) + const hung = shutdown.abortActiveSessions() + shutdown.request("SIGINT") + expect(forced).toBe(0) + await Bun.sleep(140) + // The deadline still delivered the owned-server force edge and exited with + // the abort code instead of waiting on the hung session API forever. + expect(forced).toBe(1) + expect(exits).toEqual([130]) + shutdown.dispose() + void hung + }, 10_000) + + test("dispose cancels a pending forced exit", async () => { + const exits: number[] = [] + const shutdown = new RunShutdown({ exit: (code) => void exits.push(code) }) + shutdown.request("SIGINT") + shutdown.request("SIGINT") + shutdown.dispose() + await Bun.sleep(1_100) + expect(exits).toEqual([]) + }, 10_000) +})