diff --git a/.github/workflows/release-binaries.yml b/.github/workflows/release-binaries.yml index fcbee8ca..8133bc13 100644 --- a/.github/workflows/release-binaries.yml +++ b/.github/workflows/release-binaries.yml @@ -9,16 +9,24 @@ name: Release binaries # repository or dependency code. `build` runs that code with read-only # permissions and no OIDC. `attest` holds the signing identity but runs no repo # code at all — no checkout, no install — and signs only bytes it downloaded. -# `publish` writes the release and never touches OIDC. +# `publish` writes the release and never touches OIDC. The same split repeats +# for the Homebrew formula: `formula` renders it from the immutable tag with +# read-only permissions, and `formula-pr` holds the contents-write credential +# while executing no repository code. # # Release shape: this repository has immutable releases enabled, so assets and # the Git tag are frozen the moment a release is published. Assets are therefore # attached to a draft and the draft is published last. +# Manual-only, deliberately: two of the four matrix legs need macOS runners, +# which are billed at a premium on GitHub-hosted infrastructure, so binaries +# are built when a human dispatches this workflow for a tag rather than on +# every tag push. (The npm release in release.yml stays automatic — it runs on +# a Linux runner.) Immutable releases force this to be all-or-nothing anyway: +# every asset must exist before the one-shot publish, so the darwin legs could +# not be deferred independently. If a self-hosted macOS runner is ever +# registered, point the darwin legs at it and a tag-push trigger can return. on: - push: - tags: - - "v*.*.*" workflow_dispatch: inputs: tag: @@ -74,7 +82,12 @@ jobs: process.exit(1); } - const pkg = JSON.parse(readFileSync("package.json", "utf8")); + // From the tagged commit, not the checkout: on workflow_dispatch the + // checkout is the dispatch branch, whose package.json can disagree + // with the tag being released. + const pkg = JSON.parse( + execFileSync("git", ["show", `${tag}:package.json`], { encoding: "utf8" }), + ); if (tag !== `v${pkg.version}`) { console.error(`Tag ${tag} does not match package.json version ${pkg.version}.`); process.exit(1); @@ -121,7 +134,9 @@ jobs: include: - runner: macos-14 target: darwin-arm64 - - runner: macos-13 + # macos-13 was the last plain Intel label but is retired; the -intel + # variants are the supported Intel images. + - runner: macos-15-intel target: darwin-x64 - runner: ubuntu-24.04 target: linux-x64 @@ -159,6 +174,7 @@ jobs: - run: pnpm install --frozen-lockfile - run: pnpm run build + - run: pnpm run build:test # rollup-plugin-sbom writes dist-sea/sbom.json during this build, from the # bundler's own module graph. These two variables are what it records as @@ -209,6 +225,7 @@ jobs: exit 1 fi env -i HOME="$HOME" PATH=/usr/bin:/bin "$workdir/acpx" --help > /dev/null + ACPX_TEST_PACKAGE_BIN="$workdir/acpx" node --test dist-test/test/packaged-bin.test.js echo "Verified acpx $reported" - name: Name and check the SBOM @@ -324,7 +341,7 @@ jobs: # A convenience index only. Each tarball carries its own provenance and # SBOM attestation, so `gh attestation verify` — not this file — is the # integrity mechanism. - shasum -a 256 *.tar.gz *.cdx.json > SHA256SUMS + shasum -a 256 -- *.tar.gz *.cdx.json > SHA256SUMS cat SHA256SUMS - name: Attach assets to a draft, then publish @@ -364,9 +381,145 @@ jobs: gh release edit "$TAG" --draft=false echo "Published $TAG; assets and tag are now immutable." - - name: Formula checksums + # The formula pipeline mirrors the build/attest split above: `formula` + # executes the repository's generator script, so it gets no write credential; + # `formula-pr` holds the write credential, so it executes no repository code + # at all — it commits bytes it downloaded from the render job. + formula: + name: Render Homebrew formula + needs: publish + runs-on: ubuntu-24.04 + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The validated tag, not main: the gate proved this ref is on main + # and matches the released version, and it cannot move afterwards — + # a branch checkout would run whatever landed on main since. + ref: ${{ inputs.tag || github.ref_name }} + persist-credentials: false + + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: ${{ env.NODE_VERSION }} + check-latest: false + + - name: Fetch the release checksum manifest + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + TAG: ${{ inputs.tag || github.ref_name }} + run: | + set -euo pipefail + # From the published release, not the build artifacts: the release is + # immutable, so this is the manifest users can verify against. + gh release download "$TAG" --pattern SHA256SUMS --dir "${RUNNER_TEMP}" + + - name: Wait for the npm tarball, verify it, and record its checksum + id: npm + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + TAG: ${{ inputs.tag || github.ref_name }} + run: | + set -euo pipefail + version="${TAG#v}" + # release.yml publishes the npm package in a parallel workflow, so the + # tarball may lag this job by a few minutes. Fail after ~10 minutes: + # a formula whose fallback URL 404s must not be proposed. + for _ in $(seq 1 30); do + if curl -fsSL -o "${RUNNER_TEMP}/acpx.tgz" \ + "https://registry.npmjs.org/acpx/-/acpx-${version}.tgz"; then + # Not trust-on-first-use: whatever the registry served must carry + # this repository's provenance attestation — created by release.yml + # over the exact packed tarball before publish — before its + # checksum is pinned into a formula every brew user will install. + gh attestation verify "${RUNNER_TEMP}/acpx.tgz" --repo "$GH_REPO" + sha="$(sha256sum "${RUNNER_TEMP}/acpx.tgz" | cut -d' ' -f1)" + echo "sha256=${sha}" >> "$GITHUB_OUTPUT" + exit 0 + fi + echo "npm tarball for ${version} not available yet; retrying in 20s." + sleep 20 + done + echo "::error::acpx@${version} never appeared on the npm registry." + exit 1 + + - name: Regenerate the formula + env: + TAG: ${{ inputs.tag || github.ref_name }} + NPM_SHA256: ${{ steps.npm.outputs.sha256 }} run: | set -euo pipefail - # Paste into Formula/acpx.rb. A stale checksum makes `brew install` - # fail loudly instead of installing a wrong artifact. - grep 'tar\.gz$' assets/SHA256SUMS + node scripts/sea/generate-formula.mjs \ + --version "${TAG#v}" \ + --npm-sha256 "$NPM_SHA256" \ + --sums "${RUNNER_TEMP}/SHA256SUMS" + # The bot PR is created by GITHUB_TOKEN, so CI does not run on it and + # a syntax error would otherwise surface only at merge review. Ruby is + # on the runner image; a full `brew audit` is not available on Linux. + ruby -c Formula/acpx.rb + + - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: formula + path: Formula/acpx.rb + if-no-files-found: error + + formula-pr: + name: Propose formula update + needs: formula + runs-on: ubuntu-24.04 + # Writes a branch and opens a PR; requires "Allow GitHub Actions to create + # and approve pull requests" in the repository's Actions settings. + # + # This job holds the write credential, so it runs no repository code: the + # checkout is only a git work tree for the commit below, and every command + # is an inline git/gh invocation. The formula bytes come exclusively from + # the render job's artifact. + permissions: + contents: write + pull-requests: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # The PR targets main, so the branch forks from it. Nothing from this + # checkout is executed. + ref: main + persist-credentials: true + + - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: formula + path: Formula + + - name: Open a pull request + env: + GH_TOKEN: ${{ github.token }} + TAG: ${{ inputs.tag || github.ref_name }} + run: | + set -euo pipefail + if git diff --quiet -- Formula/acpx.rb; then + echo "Formula already matches $TAG; nothing to propose." + exit 0 + fi + branch="bot/homebrew-formula-${TAG}" + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git checkout -b "$branch" + git add Formula/acpx.rb + git commit -m "chore(brew): point the formula at $TAG" + # Plain --force: a fresh clone has no remote-tracking ref for the bot + # branch, so --force-with-lease would reject every re-run. The branch + # is namespaced to this job and carries generated content only. + git push --force origin "$branch" + # A PR rather than a direct push: the formula names the bytes every + # brew user installs, so it goes through the same review path as any + # other change to main. Note: PRs created with GITHUB_TOKEN get no CI + # runs — the render job's checks stand in; review before merging. + if ! gh pr view "$branch" >/dev/null 2>&1; then + gh pr create \ + --title "chore(brew): point the formula at $TAG" \ + --body "$(printf 'Regenerated by the formula jobs of release-binaries.yml from the %s release assets and the attestation-verified npm tarball.\n\nNote: bot PRs from GITHUB_TOKEN do not trigger CI; the render job syntax-checked the formula.\n\n🤖 Generated with [Claude Code](https://claude.com/claude-code)' "$TAG")" + fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 17cfd490..39fc31b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,12 @@ Repo: https://github.com/openclaw/acpx ### Fixes - Session queue owner: capture a bounded owner stderr tail and exit status during cold start only (stop retaining at first IPC accept; keep draining the pipe so long-lived owners are not killed by EPIPE) so a dead owner reports the real failure instead of a silent timeout. Thanks @SebTardif. +- Packaging/sessions: let Node single-executable builds self-spawn detached queue + owners without repeating the embedded executable path, restoring persistent + prompts for Homebrew and other SEA installs. +- Runtime/agents: terminate the owned adapter process group/tree during normal + and failed startup cleanup so package-exec wrappers cannot leave descendants + running after ACPX exits. ## 2026.7.4 (v0.12.0) diff --git a/Formula/acpx.rb b/Formula/acpx.rb index 721c3cc2..4be71374 100644 --- a/Formula/acpx.rb +++ b/Formula/acpx.rb @@ -1,43 +1,59 @@ +# Generated by scripts/sea/generate-formula.mjs — do not edit by hand. +# The `formula` job in .github/workflows/release-binaries.yml regenerates +# this file for every release and opens a PR with the result. class Acpx < Formula desc "Headless CLI client for the Agent Client Protocol (ACP)" homepage "https://github.com/artagon/acpx" - version "0.12.0" - license "MIT" - # Self-contained Node single-executable application: the bundle and a V8 - # startup snapshot are injected into a Node binary, so there is no runtime - # dependency on a system Node install and startup is ~50ms versus ~77ms for - # the npm package. + # Two install paths, resolved per platform: # - # Assets are built by .github/workflows/release-binaries.yml (`pnpm run sea` - # per target) and attached to the tagged release. Homebrew's own node is - # compiled without single-executable support and cannot build them, which is - # why this formula ships prebuilt binaries rather than building from source. + # Platforms with a published asset get a self-contained Node + # single-executable: the bundle and a V8 startup snapshot injected into an + # official Node binary, with no runtime dependency on a system Node and + # ~50ms startup versus ~77ms for the npm package. The snapshot is + # architecture-specific, so each asset is built on a native runner by + # release-binaries.yml; Homebrew's own node has SEA support compiled out + # and cannot build them from source. # - # Every asset carries build-provenance and SBOM attestations, and releases are - # immutable, so the sha256 below pins bytes that cannot be replaced upstream. - # See docs/verifying-releases.md. + # Every other platform installs the npm package below with Homebrew's + # node — same code, ordinary module resolution instead of a snapshot. # - # Only the platforms with a published asset are listed. Adding a url/sha256 - # pair for a platform whose asset does not exist turns a clear "unsupported" - # message into a download failure, so new platforms are added by the release - # workflow, not by hand. + # Binary assets carry build-provenance and SBOM attestations, and releases + # are immutable, so each sha256 pins bytes that cannot be replaced + # upstream. See docs/verifying-releases.md. + url "https://registry.npmjs.org/acpx/-/acpx-0.12.0.tgz" + version "0.12.0" + sha256 "1dd271ad09a39071b8305bdcdf6acddaa31c8f35ecf063e782dc9b5da8e193d7" + license "MIT" + on_macos do on_arm do url "https://github.com/artagon/acpx/releases/download/v0.12.0/acpx-0.12.0-darwin-arm64.tar.gz" sha256 "823fea276f249b73c9305f0b36299f0af8f8936966208e5b52ef73f6f97e2c58" end + on_intel do + depends_on "node" + end + end + + on_linux do + depends_on "node" end def install - bin.install "acpx" + if (buildpath/"acpx").exist? + bin.install "acpx" + else + system "npm", "install", *std_npm_args + bin.install_symlink Dir["#{libexec}/bin/*"] + end end test do assert_match version.to_s, shell_output("#{bin}/acpx --version") - # The binary must answer without a system Node on PATH — that is the - # property that justifies shipping a ~122MB single executable. + # On binary platforms this must answer without a system Node on PATH — + # that is the property that justifies shipping a ~122MB executable. assert_match "Usage", shell_output("#{bin}/acpx --help") end end diff --git a/scripts/sea/generate-formula.mjs b/scripts/sea/generate-formula.mjs new file mode 100644 index 00000000..574941ae --- /dev/null +++ b/scripts/sea/generate-formula.mjs @@ -0,0 +1,182 @@ +#!/usr/bin/env node +/** + * Renders Formula/acpx.rb from a release's checksum manifest. + * + * The formula serves two install paths and this script is the only writer of + * both, so they cannot drift apart: + * + * - Binary: platforms with a published single-executable asset get a + * url/sha256 block pointing at the immutable release tarball. + * - Node: every other platform falls back to the npm registry tarball and a + * Homebrew `node` dependency, installed with std_npm_args. + * + * Run by the `formula` job in .github/workflows/release-binaries.yml after the + * release is published. Runnable locally the same way: + * + * node scripts/sea/generate-formula.mjs \ + * --version 0.12.0 --npm-sha256 --sums SHA256SUMS + */ +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); + +function parseArgs(argv) { + const args = {}; + for (let i = 0; i < argv.length; i += 2) { + const key = argv[i]; + const value = argv[i + 1]; + if (!/^--[a-z0-9-]+$/.test(key) || value === undefined) { + throw new Error(`Malformed arguments near ${key ?? ""}.`); + } + args[key.slice(2)] = value; + } + return args; +} + +const args = parseArgs(process.argv.slice(2)); +const version = args.version ?? ""; +const npmSha = args["npm-sha256"] ?? ""; +const sumsPath = args.sums ?? ""; +const outPath = args.out ?? path.join(repoRoot, "Formula", "acpx.rb"); + +if (!/^\d+\.\d+\.\d+$/.test(version)) { + throw new Error(`--version must be X.Y.Z; received "${version}".`); +} +if (!/^[0-9a-f]{64}$/.test(npmSha)) { + throw new Error(`--npm-sha256 must be 64 hex characters; received "${npmSha}".`); +} + +// The four slots Homebrew can address with on_macos/on_linux × on_arm/on_intel. +// A manifest naming any other target fails the run: silently skipping it would +// publish a formula that pretends the platform does not exist. +const SLOTS = new Set(["darwin-arm64", "darwin-x64", "linux-arm64", "linux-x64"]); + +const shaByTarget = new Map(); +for (const line of fs.readFileSync(sumsPath, "utf8").split("\n")) { + if (line.trim() === "") { + continue; + } + const match = /^([0-9a-f]{64})\s+(\S+)$/.exec(line.trim()); + if (!match) { + throw new Error(`Unparseable checksum line: "${line}".`); + } + const [, sha, name] = match; + if (!name.endsWith(".tar.gz")) { + continue; + } + const asset = new RegExp(`^acpx-(\\d+\\.\\d+\\.\\d+)-([a-z0-9-]+)\\.tar\\.gz$`).exec(name); + if (!asset) { + throw new Error(`Tarball "${name}" does not match acpx--.tar.gz.`); + } + if (asset[1] !== version) { + throw new Error(`Tarball "${name}" is for version ${asset[1]}, expected ${version}.`); + } + if (!SLOTS.has(asset[2])) { + throw new Error(`Tarball "${name}" names unknown target "${asset[2]}".`); + } + if (shaByTarget.has(asset[2])) { + throw new Error(`Duplicate tarball for target "${asset[2]}"; refusing to pick one silently.`); + } + shaByTarget.set(asset[2], sha); +} + +if (shaByTarget.size === 0) { + throw new Error(`${sumsPath} lists no acpx tarballs; refusing to emit a binary-free formula.`); +} + +const releaseBase = `https://github.com/artagon/acpx/releases/download/v${version}`; + +/** One platform slot: a binary url/sha256 pair, or the node fallback. */ +function slotLines(target, indent) { + const sha = shaByTarget.get(target); + if (sha === undefined) { + return [`${indent}depends_on "node"`]; + } + return [ + `${indent}url "${releaseBase}/acpx-${version}-${target}.tar.gz"`, + `${indent}sha256 "${sha}"`, + ]; +} + +/** on_macos / on_linux block, collapsing when both arches take the same path. */ +function osBlock(os, armTarget, intelTarget) { + const bothMissing = !shaByTarget.has(armTarget) && !shaByTarget.has(intelTarget); + if (bothMissing) { + return [` on_${os} do`, ` depends_on "node"`, " end"]; + } + return [ + ` on_${os} do`, + " on_arm do", + ...slotLines(armTarget, " "), + " end", + " on_intel do", + ...slotLines(intelTarget, " "), + " end", + " end", + ]; +} + +const formula = [ + "# Generated by scripts/sea/generate-formula.mjs — do not edit by hand.", + "# The `formula` job in .github/workflows/release-binaries.yml regenerates", + "# this file for every release and opens a PR with the result.", + "class Acpx < Formula", + ' desc "Headless CLI client for the Agent Client Protocol (ACP)"', + ' homepage "https://github.com/artagon/acpx"', + "", + " # Two install paths, resolved per platform:", + " #", + " # Platforms with a published asset get a self-contained Node", + " # single-executable: the bundle and a V8 startup snapshot injected into an", + " # official Node binary, with no runtime dependency on a system Node and", + " # ~50ms startup versus ~77ms for the npm package. The snapshot is", + " # architecture-specific, so each asset is built on a native runner by", + " # release-binaries.yml; Homebrew's own node has SEA support compiled out", + " # and cannot build them from source.", + " #", + " # Every other platform installs the npm package below with Homebrew's", + " # node — same code, ordinary module resolution instead of a snapshot.", + " #", + " # Binary assets carry build-provenance and SBOM attestations, and releases", + " # are immutable, so each sha256 pins bytes that cannot be replaced", + " # upstream. See docs/verifying-releases.md.", + ` url "https://registry.npmjs.org/acpx/-/acpx-${version}.tgz"`, + ` version "${version}"`, + ` sha256 "${npmSha}"`, + ' license "MIT"', + "", + ...osBlock("macos", "darwin-arm64", "darwin-x64"), + "", + ...osBlock("linux", "linux-arm64", "linux-x64"), + "", + " def install", + ' if (buildpath/"acpx").exist?', + ' bin.install "acpx"', + " else", + ' system "npm", "install", *std_npm_args', + ' bin.install_symlink Dir["#{libexec}/bin/*"]', + " end", + " end", + "", + " test do", + ' assert_match version.to_s, shell_output("#{bin}/acpx --version")', + "", + " # On binary platforms this must answer without a system Node on PATH —", + " # that is the property that justifies shipping a ~122MB executable.", + ' assert_match "Usage", shell_output("#{bin}/acpx --help")', + " end", + "end", + "", +].join("\n"); + +fs.mkdirSync(path.dirname(outPath), { recursive: true }); +fs.writeFileSync(outPath, formula); + +const targets = [...shaByTarget.keys()] + .toSorted((left, right) => left.localeCompare(right)) + .join(", "); +process.stdout.write( + `Wrote ${outPath} for v${version} (binary: ${targets}; npm fallback elsewhere)\n`, +); diff --git a/src/acp/auth-env.ts b/src/acp/auth-env.ts index 35d20301..928997af 100644 --- a/src/acp/auth-env.ts +++ b/src/acp/auth-env.ts @@ -174,12 +174,14 @@ export function buildAgentSpawnOptions( sessionEnv?: Record, ): { cwd: string; + detached: true; env: NodeJS.ProcessEnv; stdio: ["pipe", "pipe", "pipe"]; windowsHide: true; } { return { cwd, + detached: true, env: buildAgentEnvironment(authCredentials, sessionEnv), stdio: ["pipe", "pipe", "pipe"], windowsHide: true, diff --git a/src/acp/client.ts b/src/acp/client.ts index 4ea353ef..f0fe5161 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -108,6 +108,14 @@ import { resolveRequestedModelId, type SessionModelState, } from "./model-support.js"; +import { + captureProcessTreePids, + createManagedProcessTree, + rememberProcessTreePids, + signalProcessTree, + waitForProcessTreeExit, + type ManagedProcessTree, +} from "./process-tree.js"; import { formatSessionControlAcpSummary, maybeWrapSessionControlError, @@ -414,6 +422,7 @@ export class AcpClient { private options: AcpClientOptions; private connection?: ClientSideConnection; private agent?: ChildProcessByStdio; + private agentProcessTree?: ManagedProcessTree; private initResult?: InitializeResponse; private loadedSessionId?: string; private eventHandlers: Pick< @@ -724,6 +733,11 @@ export class AcpClient { plan.args, buildSpawnCommandOptions(plan.spawnCommand, plan.spawnOptions), ) as ChildProcessByStdio; + const processTree = createManagedProcessTree(spawnedChild.pid, true); + this.agentProcessTree = processTree; + spawnedChild.once("exit", () => { + rememberProcessTreePids(processTree); + }); try { await waitForSpawn(spawnedChild); } catch (error) { @@ -851,7 +865,7 @@ export class AcpClient { params.startupStderr, ); try { - params.child.kill(); + await this.terminateAgentProcess(params.child); } catch { // best effort } @@ -1382,11 +1396,13 @@ export class AcpClient { this.initResult = undefined; this.connection = undefined; this.agent = undefined; + this.agentProcessTree = undefined; } private async terminateAgentProcess( child: ChildProcessByStdio, ): Promise { + const processTree = this.agentProcessTree ?? createManagedProcessTree(child.pid, true); const stdinCloseGraceMs = resolveAgentCloseAfterStdinEndMs(this.options.agentCommand); const termGraceMs = this.options.fastTeardown ? AGENT_CLOSE_FAST_TERM_GRACE_MS @@ -1394,12 +1410,17 @@ export class AcpClient { const killGraceMs = this.options.fastTeardown ? AGENT_CLOSE_FAST_KILL_GRACE_MS : AGENT_CLOSE_KILL_GRACE_MS; + await captureProcessTreePids(processTree, isChildProcessRunning(child)); this.endAgentStdin(child); - let exited = await waitForChildExit(child, stdinCloseGraceMs); - exited = await this.killAgentIfRunning(child, exited, "SIGTERM", termGraceMs); + let exited = await waitForProcessTreeExit( + processTree, + () => isChildProcessRunning(child), + stdinCloseGraceMs, + ); + exited = await this.killAgentIfRunning(child, processTree, exited, "SIGTERM", termGraceMs); if (!exited) { this.log(`agent did not exit after ${termGraceMs}ms; forcing SIGKILL`); - exited = await this.killAgentIfRunning(child, exited, "SIGKILL", killGraceMs); + exited = await this.killAgentIfRunning(child, processTree, exited, "SIGKILL", killGraceMs); } // Ensure stdio handles don't keep this process alive after close() returns. @@ -1420,19 +1441,20 @@ export class AcpClient { private async killAgentIfRunning( child: ChildProcessByStdio, + processTree: ManagedProcessTree, alreadyExited: boolean, signal: NodeJS.Signals, waitMs: number, ): Promise { - if (alreadyExited || !isChildProcessRunning(child)) { - return alreadyExited; + if (alreadyExited) { + return true; } try { - child.kill(signal); + await signalProcessTree(processTree, isChildProcessRunning(child), signal); } catch { // best effort } - return await waitForChildExit(child, waitMs); + return await waitForProcessTreeExit(processTree, () => isChildProcessRunning(child), waitMs); } private detachAgentHandles(agent: ChildProcess, unref: boolean): void { diff --git a/src/acp/process-tree.ts b/src/acp/process-tree.ts new file mode 100644 index 00000000..7cecb59b --- /dev/null +++ b/src/acp/process-tree.ts @@ -0,0 +1,290 @@ +import { spawn } from "node:child_process"; + +const PROCESS_TREE_POLL_MS = 25; + +export type ManagedProcessTree = { + rootPid: number | undefined; + killProcessGroup: boolean; + descendantPids: Set; + snapshotPromise?: Promise; +}; + +export function createManagedProcessTree( + rootPid: number | undefined, + killProcessGroup: boolean, +): ManagedProcessTree { + return { + rootPid, + killProcessGroup, + descendantPids: new Set(), + }; +} + +export function rememberProcessTreePids(tree: ManagedProcessTree): void { + tree.snapshotPromise = captureProcessTreePids(tree, false); +} + +export async function captureProcessTreePids( + tree: ManagedProcessTree, + rootRunning: boolean, +): Promise { + const rootPid = tree.rootPid; + // POSIX ownership is the process group created at spawn. Descendants that + // deliberately create another session are outside that ownership boundary. + if (!tree.killProcessGroup || !rootPid || process.platform !== "win32") { + return; + } + await waitForPriorSnapshot(tree, rootRunning); + + recordProcessTreePids(tree, await listDescendantPids(rootPid)); +} + +async function waitForPriorSnapshot(tree: ManagedProcessTree, rootRunning: boolean): Promise { + if (rootRunning) { + return; + } + await tree.snapshotPromise?.catch(() => { + // Process tree snapshots are best-effort because the root may already be gone. + }); +} + +function recordProcessTreePids(tree: ManagedProcessTree, pids: number[]): void { + for (const pid of pids) { + if (pid === tree.rootPid) { + continue; + } + tree.descendantPids.add(pid); + } +} + +export async function signalProcessTree( + tree: ManagedProcessTree, + rootRunning: boolean, + signal: NodeJS.Signals, +): Promise { + const rootPid = tree.rootPid; + if (!tree.killProcessGroup || !rootPid) { + if (rootPid) { + sendSignal(rootPid, signal); + } + return; + } + + await captureProcessTreePids(tree, rootRunning); + if (process.platform === "win32") { + await signalWindowsProcessTree(tree, rootRunning, signal); + return; + } + signalPosixProcessTree(tree, signal); +} + +export async function waitForProcessTreeExit( + tree: ManagedProcessTree, + rootRunning: () => boolean, + timeoutMs: number, +): Promise { + const deadline = Date.now() + Math.max(0, timeoutMs); + while (rootRunning() || hasLiveManagedProcessTree(tree)) { + if (Date.now() >= deadline) { + return false; + } + await waitMs(Math.min(PROCESS_TREE_POLL_MS, Math.max(0, deadline - Date.now()))); + } + return true; +} + +async function signalWindowsProcessTree( + tree: ManagedProcessTree, + rootRunning: boolean, + signal: NodeJS.Signals, +): Promise { + const rootPid = tree.rootPid; + if (rootRunning && rootPid) { + await killWindowsProcessTree(rootPid, signal); + return; + } + for (const descendantPid of tree.descendantPids) { + await killWindowsProcessTree(descendantPid, signal); + } +} + +function signalPosixProcessTree(tree: ManagedProcessTree, signal: NodeJS.Signals): void { + const rootPid = tree.rootPid; + if (rootPid && hasLiveProcessGroup(rootPid)) { + sendSignal(-rootPid, signal); + } +} + +function hasLiveManagedProcessTree(tree: ManagedProcessTree): boolean { + const rootPid = tree.rootPid; + if ( + tree.killProcessGroup && + rootPid && + process.platform !== "win32" && + hasLiveProcessGroup(rootPid) + ) { + return true; + } + return process.platform === "win32" && hasLivePid(tree.descendantPids); +} + +async function listDescendantPids(rootPid: number): Promise { + let output: string; + try { + output = await runProcessListCommand(); + } catch { + return []; + } + + const childrenByParent = new Map(); + for (const line of output.split("\n")) { + addProcessListLine(childrenByParent, line); + } + + const descendants: number[] = []; + const queue = [...(childrenByParent.get(rootPid) ?? [])]; + for (let index = 0; index < queue.length; index += 1) { + const pid = queue[index]; + descendants.push(pid); + queue.push(...(childrenByParent.get(pid) ?? [])); + } + return descendants; +} + +function addProcessListLine(childrenByParent: Map, line: string): void { + const parsed = parseProcessListLine(line); + if (!parsed) { + return; + } + + const children = childrenByParent.get(parsed.parentPid); + if (children) { + children.push(parsed.pid); + } else { + childrenByParent.set(parsed.parentPid, [parsed.pid]); + } +} + +function parseProcessListLine(line: string): { pid: number; parentPid: number } | undefined { + const match = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!match) { + return undefined; + } + + const pid = Number(match[1]); + const parentPid = Number(match[2]); + if (!Number.isInteger(pid) || !Number.isInteger(parentPid) || pid <= 0 || parentPid <= 0) { + return undefined; + } + return { pid, parentPid }; +} + +async function runProcessListCommand(): Promise { + if (process.platform === "win32") { + return await runWindowsProcessListCommand(); + } + return await runPsCommand(["-eo", "pid=,ppid="]); +} + +async function runPsCommand(args: string[]): Promise { + return await runCapturedCommand("ps", args, "ps"); +} + +async function runWindowsProcessListCommand(): Promise { + const command = [ + "Get-CimInstance Win32_Process |", + 'ForEach-Object { "$($_.ProcessId) $($_.ParentProcessId)" }', + ].join(" "); + return await runCapturedCommand( + "powershell.exe", + ["-NoProfile", "-NonInteractive", "-Command", command], + "powershell process list", + ); +} + +async function runCapturedCommand( + command: string, + args: string[], + description: string, +): Promise { + return await new Promise((resolve, reject) => { + const child = spawn(command, args, { + stdio: ["ignore", "pipe", "pipe"], + windowsHide: true, + }); + let stdout = ""; + let stderr = ""; + + child.stdout.setEncoding("utf8"); + child.stderr.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => { + stdout += chunk; + }); + child.stderr.on("data", (chunk: string) => { + stderr += chunk; + }); + child.once("error", reject); + child.once("close", (code, signal) => { + if (code === 0) { + resolve(stdout); + return; + } + reject( + new Error( + `${description} exited with code ${code ?? "null"} signal ${signal ?? "null"}: ${stderr}`, + ), + ); + }); + }); +} + +async function killWindowsProcessTree(pid: number, signal: NodeJS.Signals): Promise { + const args = ["/pid", String(pid), "/t"]; + if (signal === "SIGKILL") { + args.push("/f"); + } + await new Promise((resolve) => { + const child = spawn("taskkill", args, { + stdio: ["ignore", "ignore", "ignore"], + windowsHide: true, + }); + child.once("error", () => resolve()); + child.once("close", () => resolve()); + }); +} + +function sendSignal(pid: number, signal: NodeJS.Signals): void { + try { + process.kill(pid, signal); + } catch { + // Processes can exit between discovery and signaling. + } +} + +function hasLiveProcessGroup(processGroupId: number): boolean { + try { + process.kill(-processGroupId, 0); + return true; + } catch { + return false; + } +} + +function hasLivePid(pids: Set): boolean { + let live = false; + for (const pid of pids) { + try { + process.kill(pid, 0); + live = true; + } catch { + pids.delete(pid); + } + } + return live; +} + +function waitMs(ms: number): Promise { + return new Promise((resolve) => { + setTimeout(resolve, Math.max(0, ms)); + }); +} diff --git a/src/acp/terminal-manager.ts b/src/acp/terminal-manager.ts index 00c14a41..cfa5aeca 100644 --- a/src/acp/terminal-manager.ts +++ b/src/acp/terminal-manager.ts @@ -24,15 +24,20 @@ import { type TerminalSpawnCommand, } from "../spawn-command-options.js"; import type { ClientOperation, NonInteractivePermissionPolicy, PermissionMode } from "../types.js"; +import { + createManagedProcessTree, + rememberProcessTreePids, + signalProcessTree, + waitForProcessTreeExit, + type ManagedProcessTree, +} from "./process-tree.js"; const DEFAULT_TERMINAL_OUTPUT_LIMIT_BYTES = 64 * 1024; const DEFAULT_KILL_GRACE_MS = 1_500; type ManagedTerminal = { process: ChildProcessByStdio; - killProcessGroup: boolean; - descendantPids: Set; - processGroupSnapshotPromise?: Promise; + processTree: ManagedProcessTree; output: Buffer; truncated: boolean; outputByteLimit: number; @@ -147,12 +152,6 @@ function canPromptForPermission(): boolean { return process.stdin.isTTY && process.stderr.isTTY; } -function waitMs(ms: number): Promise { - return new Promise((resolve) => { - setTimeout(resolve, Math.max(0, ms)); - }); -} - export class TerminalManager { private readonly cwd: string; private permissionMode: PermissionMode; @@ -210,8 +209,7 @@ export class TerminalManager { const terminal: ManagedTerminal = { process: proc, - killProcessGroup: spawnCommand.killProcessGroup, - descendantPids: new Set(), + processTree: createManagedProcessTree(proc.pid, spawnCommand.killProcessGroup), output: Buffer.alloc(0), truncated: false, outputByteLimit, @@ -239,14 +237,11 @@ export class TerminalManager { proc.once("exit", (exitCode, signal) => { terminal.exitCode = exitCode; terminal.signal = signal; - terminal.processGroupSnapshotPromise = rememberProcessGroupPids(terminal); - void (async () => { - await terminal.processGroupSnapshotPromise; - terminal.resolveExit({ - exitCode: exitCode ?? null, - signal: signal ?? null, - }); - })(); + rememberProcessTreePids(terminal.processTree); + terminal.resolveExit({ + exitCode: exitCode ?? null, + signal: signal ?? null, + }); }); const terminalId = randomUUID(); @@ -440,23 +435,23 @@ export class TerminalManager { } private async killProcess(terminal: ManagedTerminal): Promise { - if (!this.isRunning(terminal) && !terminal.killProcessGroup) { + if (!this.isRunning(terminal) && !terminal.processTree.killProcessGroup) { return; } try { - await this.signalProcess(terminal, "SIGTERM"); + await signalProcessTree(terminal.processTree, this.isRunning(terminal), "SIGTERM"); } catch { return; } const exitedAfterTerm = await this.waitForCleanupAfterSignal(terminal); - if (exitedAfterTerm && !terminal.killProcessGroup) { + if (exitedAfterTerm) { return; } try { - await this.signalProcess(terminal, "SIGKILL"); + await signalProcessTree(terminal.processTree, this.isRunning(terminal), "SIGKILL"); } catch { return; } @@ -464,75 +459,12 @@ export class TerminalManager { await this.waitForCleanupAfterSignal(terminal); } - private async signalProcess(terminal: ManagedTerminal, signal: NodeJS.Signals): Promise { - const pid = terminal.process.pid; - if (terminal.killProcessGroup && pid && process.platform === "win32") { - await this.signalWindowsProcessGroup(terminal, pid, signal); - return; - } - if (terminal.killProcessGroup && pid) { - await this.signalPosixProcessGroup(terminal, pid, signal); - return; - } - terminal.process.kill(signal); - } - - private async signalWindowsProcessGroup( - terminal: ManagedTerminal, - pid: number, - signal: NodeJS.Signals, - ): Promise { - await this.captureDescendantPids(terminal, pid); - if (this.isRunning(terminal)) { - await killWindowsProcessTree(pid, signal); - return; - } - for (const descendantPid of terminal.descendantPids) { - await killWindowsProcessTree(descendantPid, signal); - } - } - - private async signalPosixProcessGroup( - terminal: ManagedTerminal, - pid: number, - signal: NodeJS.Signals, - ): Promise { - await this.captureDescendantPids(terminal, pid); - if (hasLiveProcessGroup(pid)) { - sendSignal(-pid, signal); - return; - } - for (const descendantPid of terminal.descendantPids) { - sendSignal(descendantPid, signal); - } - } - - private async captureDescendantPids(terminal: ManagedTerminal, pid: number): Promise { - if (!this.isRunning(terminal)) { - await terminal.processGroupSnapshotPromise?.catch(() => { - // ignore best-effort process group snapshot failures - }); - } - for (const descendantPid of await listDescendantPids(pid)) { - terminal.descendantPids.add(descendantPid); - } - } - private async waitForCleanupAfterSignal(terminal: ManagedTerminal): Promise { - return await Promise.race([ - this.waitForTerminalAndTrackedDescendants(terminal).then(() => true), - waitMs(this.killGraceMs).then(() => false), - ]); - } - - private async waitForTerminalAndTrackedDescendants(terminal: ManagedTerminal): Promise { - await terminal.exitPromise; - while (hasLiveTerminalProcessGroup(terminal)) { - await waitMs(25); - } - while (hasLivePid(terminal.descendantPids)) { - await waitMs(25); - } + return await waitForProcessTreeExit( + terminal.processTree, + () => this.isRunning(terminal), + this.killGraceMs, + ); } } @@ -627,258 +559,3 @@ function commandPathExists(command: string, cwd: string): boolean { const resolvedPath = path.isAbsolute(command) ? command : path.resolve(cwd, command); return fs.existsSync(resolvedPath); } - -async function listDescendantPids(rootPid: number): Promise { - let output: string; - try { - output = await runProcessListCommand(); - } catch { - return []; - } - - const childrenByParent = new Map(); - for (const line of output.split("\n")) { - addProcessListLine(childrenByParent, line); - } - - const descendants: number[] = []; - const queue = [...(childrenByParent.get(rootPid) ?? [])]; - for (let index = 0; index < queue.length; index += 1) { - const pid = queue[index]; - descendants.push(pid); - queue.push(...(childrenByParent.get(pid) ?? [])); - } - return descendants; -} - -function addProcessListLine(childrenByParent: Map, line: string): void { - const parsed = parseProcessListLine(line); - if (!parsed) { - return; - } - - const children = childrenByParent.get(parsed.parentPid); - if (children) { - children.push(parsed.pid); - } else { - childrenByParent.set(parsed.parentPid, [parsed.pid]); - } -} - -function parseProcessListLine(line: string): { pid: number; parentPid: number } | undefined { - const match = line.trim().match(/^(\d+)\s+(\d+)$/); - if (!match) { - return undefined; - } - - const pid = Number(match[1]); - const parentPid = Number(match[2]); - if (!Number.isInteger(pid) || !Number.isInteger(parentPid) || pid <= 0 || parentPid <= 0) { - return undefined; - } - return { pid, parentPid }; -} - -async function runProcessListCommand(): Promise { - if (process.platform === "win32") { - return await runWindowsProcessListCommand(); - } - - return await new Promise((resolve, reject) => { - const child = spawn("ps", ["-eo", "pid=,ppid="], { - stdio: ["ignore", "pipe", "pipe"], - }); - - let stdout = ""; - let stderr = ""; - - child.stdout.setEncoding("utf8"); - child.stderr.setEncoding("utf8"); - - child.stdout.on("data", (chunk: string) => { - stdout += chunk; - }); - child.stderr.on("data", (chunk: string) => { - stderr += chunk; - }); - - child.once("error", reject); - child.once("close", (code, signal) => { - if (code === 0) { - resolve(stdout); - return; - } - reject( - new Error(`ps exited with code ${code ?? "null"} signal ${signal ?? "null"}: ${stderr}`), - ); - }); - }); -} - -async function rememberProcessGroupPids(terminal: ManagedTerminal): Promise { - const processGroupId = terminal.process.pid; - if (!terminal.killProcessGroup || !processGroupId) { - return; - } - - if (process.platform === "win32") { - for (const pid of await listDescendantPids(processGroupId)) { - terminal.descendantPids.add(pid); - } - return; - } - - for (const pid of await listProcessGroupPids(processGroupId)) { - if (pid !== processGroupId) { - terminal.descendantPids.add(pid); - } - } -} - -async function listProcessGroupPids(processGroupId: number): Promise { - let output: string; - try { - output = await runProcessGroupListCommand(); - } catch { - return []; - } - - const pids: number[] = []; - for (const line of output.split("\n")) { - const match = line.trim().match(/^(\d+)\s+(\d+)$/); - if (!match) { - continue; - } - - const pid = Number(match[1]); - const pgid = Number(match[2]); - if (Number.isInteger(pid) && Number.isInteger(pgid) && pid > 0 && pgid === processGroupId) { - pids.push(pid); - } - } - return pids; -} - -async function runProcessGroupListCommand(): Promise { - return await new Promise((resolve, reject) => { - const child = spawn("ps", ["-eo", "pid=,pgid="], { - stdio: ["ignore", "pipe", "pipe"], - }); - - let stdout = ""; - let stderr = ""; - - child.stdout.setEncoding("utf8"); - child.stderr.setEncoding("utf8"); - - child.stdout.on("data", (chunk: string) => { - stdout += chunk; - }); - child.stderr.on("data", (chunk: string) => { - stderr += chunk; - }); - - child.once("error", reject); - child.once("close", (code, signal) => { - if (code === 0) { - resolve(stdout); - return; - } - reject( - new Error(`ps exited with code ${code ?? "null"} signal ${signal ?? "null"}: ${stderr}`), - ); - }); - }); -} - -async function runWindowsProcessListCommand(): Promise { - return await new Promise((resolve, reject) => { - const command = [ - "Get-CimInstance Win32_Process |", - 'ForEach-Object { "$($_.ProcessId) $($_.ParentProcessId)" }', - ].join(" "); - const child = spawn("powershell.exe", ["-NoProfile", "-NonInteractive", "-Command", command], { - stdio: ["ignore", "pipe", "pipe"], - windowsHide: true, - }); - - let stdout = ""; - let stderr = ""; - - child.stdout.setEncoding("utf8"); - child.stderr.setEncoding("utf8"); - - child.stdout.on("data", (chunk: string) => { - stdout += chunk; - }); - child.stderr.on("data", (chunk: string) => { - stderr += chunk; - }); - - child.once("error", reject); - child.once("close", (code, signal) => { - if (code === 0) { - resolve(stdout); - return; - } - reject( - new Error( - `powershell process list exited with code ${code ?? "null"} signal ${ - signal ?? "null" - }: ${stderr}`, - ), - ); - }); - }); -} - -async function killWindowsProcessTree(pid: number, signal: NodeJS.Signals): Promise { - const args = ["/pid", String(pid), "/t"]; - if (signal === "SIGKILL") { - args.push("/f"); - } - await new Promise((resolve) => { - const child = spawn("taskkill", args, { - stdio: ["ignore", "ignore", "ignore"], - windowsHide: true, - }); - child.once("error", () => resolve()); - child.once("close", () => resolve()); - }); -} - -function sendSignal(pid: number, signal: NodeJS.Signals): void { - try { - process.kill(pid, signal); - } catch { - // Process tree cleanup is best-effort because descendants can exit between ps and kill. - } -} - -function hasLiveProcessGroup(processGroupId: number): boolean { - try { - process.kill(-processGroupId, 0); - return true; - } catch { - return false; - } -} - -function hasLiveTerminalProcessGroup(terminal: ManagedTerminal): boolean { - const pid = terminal.process.pid; - return Boolean( - terminal.killProcessGroup && pid && process.platform !== "win32" && hasLiveProcessGroup(pid), - ); -} - -function hasLivePid(pids: Set): boolean { - for (const pid of pids) { - try { - process.kill(pid, 0); - return true; - } catch { - pids.delete(pid); - } - } - return false; -} diff --git a/src/cli/session/queue-owner-process.ts b/src/cli/session/queue-owner-process.ts index 42c787df..2bab0ea7 100644 --- a/src/cli/session/queue-owner-process.ts +++ b/src/cli/session/queue-owner-process.ts @@ -2,6 +2,7 @@ import { spawn } from "node:child_process"; import { mkdtempSync, realpathSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; +import { isSea } from "node:sea"; import type { SessionAgentOptions } from "../../runtime/engine/session-options.js"; import type { AuthPolicy, @@ -129,7 +130,11 @@ export function sanitizeQueueOwnerExecArgv( export function buildQueueOwnerArgOverride( entryPath: string, execArgv: readonly string[] = process.execArgv, + runningInSea: boolean = isSea(), ): string | null { + if (runningInSea) { + return null; + } const sanitized = sanitizeQueueOwnerExecArgv(execArgv); if (sanitized.length === 0) { return null; @@ -137,7 +142,10 @@ export function buildQueueOwnerArgOverride( return JSON.stringify([...sanitized, entryPath, "__queue-owner"]); } -export function resolveQueueOwnerSpawnArgs(argv: readonly string[] = process.argv): string[] { +export function resolveQueueOwnerSpawnArgs( + argv: readonly string[] = process.argv, + runningInSea: boolean = isSea(), +): string[] { const override = process.env.ACPX_QUEUE_OWNER_ARGS; if (override) { const parsed = JSON.parse(override) as unknown; @@ -147,6 +155,10 @@ export function resolveQueueOwnerSpawnArgs(argv: readonly string[] = process.arg throw new Error("acpx self-spawn failed: invalid ACPX_QUEUE_OWNER_ARGS"); } + if (runningInSea) { + return ["__queue-owner"]; + } + const entry = argv[1]; if (!entry || entry.trim().length === 0) { throw new Error("acpx self-spawn failed: missing CLI entry path"); diff --git a/test/client.test.ts b/test/client.test.ts index 3483c39f..629217f5 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -1,4 +1,6 @@ import assert from "node:assert/strict"; +import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { PassThrough } from "node:stream"; import test from "node:test"; @@ -1276,6 +1278,76 @@ test("AcpClient start fails fast when the agent exits during initialize", async assert(Date.now() - startedAt < 2_000); }); +test("AcpClient startup failure kills descendants left by an exited npx wrapper", async () => { + if (process.platform === "win32") { + return; + } + + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "acpx-startup-tree-")); + const packageDir = path.join(tempDir, "adapter-package"); + const processInfoPath = path.join(tempDir, "process-info.json"); + let descendantPid: number | undefined; + + try { + await fs.mkdir(packageDir, { recursive: true }); + await fs.writeFile( + path.join(packageDir, "package.json"), + `${JSON.stringify({ + name: "acpx-startup-tree-fixture", + version: "1.0.0", + bin: { "acpx-startup-tree-fixture": "adapter.cjs" }, + })}\n`, + ); + const adapterPath = path.join(packageDir, "adapter.cjs"); + await fs.writeFile( + adapterPath, + [ + "#!/usr/bin/env node", + 'const { spawn } = require("node:child_process");', + 'const fs = require("node:fs");', + 'const child = spawn(process.execPath, ["-e", "process.on(\\"SIGTERM\\", () => {}); setInterval(() => {}, 1000);"], { stdio: "ignore" });', + `fs.writeFileSync(${JSON.stringify(processInfoPath)}, JSON.stringify({ adapterPid: process.pid, descendantPid: child.pid }));`, + "setTimeout(() => process.exit(17), 100);", + "", + ].join("\n"), + ); + await fs.chmod(adapterPath, 0o755); + + const client = makeClient({ + agentCommand: `npx --yes --offline --package ${JSON.stringify(packageDir)} -- acpx-startup-tree-fixture`, + cwd: tempDir, + sessionOptions: { + env: { + HOME: tempDir, + npm_config_cache: path.join(tempDir, "npm-cache"), + }, + }, + }); + + await assert.rejects(() => client.start(), AgentStartupError); + const processInfo = JSON.parse(await fs.readFile(processInfoPath, "utf8")) as { + adapterPid: number; + descendantPid: number; + }; + descendantPid = processInfo.descendantPid; + assert.notEqual( + processInfo.adapterPid, + descendantPid, + "fixture must create a separate stubborn descendant", + ); + assert.equal( + isPidAlive(descendantPid), + false, + "startup cleanup must not leave the adapter descendant alive", + ); + } finally { + if (descendantPid) { + await terminateTestPid(descendantPid); + } + await fs.rm(tempDir, { recursive: true, force: true }); + } +}); + test("AcpClient close resets in-memory state and shuts down terminal manager", async () => { const client = makeClient(); const internals = asInternals(client); @@ -1354,6 +1426,26 @@ function makeClient( }); } +function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function terminateTestPid(pid: number): Promise { + if (!isPidAlive(pid)) { + return; + } + process.kill(pid, "SIGKILL"); + const deadline = Date.now() + 2_000; + while (isPidAlive(pid) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } +} + function asInternals(client: AcpClient): ClientInternals { return client as unknown as ClientInternals; } diff --git a/test/packaged-bin.test.ts b/test/packaged-bin.test.ts index e42f00d1..6a9735f1 100644 --- a/test/packaged-bin.test.ts +++ b/test/packaged-bin.test.ts @@ -6,6 +6,7 @@ import os from "node:os"; import path from "node:path"; import test from "node:test"; import { fileURLToPath } from "node:url"; +import { queuePaths } from "./queue-test-helpers.js"; const DIST_CLI_PATH = path.join(process.cwd(), "dist", "cli.js"); const MOCK_AGENT_PATH = fileURLToPath(new URL("./mock-agent.js", import.meta.url)); @@ -20,6 +21,7 @@ type PackageJson = { type CliRunResult = { code: number | null; + signal: NodeJS.Signals | null; stdout: string; stderr: string; }; @@ -50,6 +52,11 @@ function packageBinSpawnArgs(args: string[]): { command: string; args: string[]; } { + const override = process.env.ACPX_TEST_PACKAGE_BIN; + if (override) { + return { command: path.resolve(override), args }; + } + const binPath = readPackageBinPath(); if (process.platform === "win32") { return { command: process.execPath, args: [binPath, ...args] }; @@ -66,8 +73,12 @@ async function withTempHome(run: (homeDir: string) => Promise): Promise { - return await new Promise((resolve) => { +async function runPackageBin( + args: string[], + homeDir: string, + timeoutMs = 15_000, +): Promise { + return await new Promise((resolve, reject) => { const env: NodeJS.ProcessEnv = { ...process.env, HOME: homeDir, @@ -93,8 +104,18 @@ async function runPackageBin(args: string[], homeDir: string): Promise { - resolve({ code, stdout, stderr }); + const timer = setTimeout(() => { + child.kill("SIGKILL"); + reject(new Error(`packaged acpx timed out after ${timeoutMs}ms: ${args.join(" ")}`)); + }, timeoutMs); + + child.once("error", (error) => { + clearTimeout(timer); + reject(error); + }); + child.once("close", (code, signal) => { + clearTimeout(timer); + resolve({ code, signal, stdout, stderr }); }); }); } @@ -157,3 +178,82 @@ test("packaged bin runs a mock-agent exec command through package executable map assert.equal(result.stdout.trim(), "packaged-bin-ok"); }); }); + +test("packaged bin serializes concurrent cold prompts through one detached queue owner", async (t) => { + if (!process.env.ACPX_TEST_PACKAGE_BIN && !existsSync(DIST_CLI_PATH)) { + t.skip("run pnpm build or set ACPX_TEST_PACKAGE_BIN before packaged-bin smoke tests"); + return; + } + + await withTempHome(async (homeDir) => { + const cwd = path.join(homeDir, "workspace"); + await fs.mkdir(cwd, { recursive: true }); + const baseArgs = ["--agent", MOCK_AGENT_COMMAND, "--approve-all", "--cwd", cwd, "--ttl", "30"]; + + const created = await runPackageBin( + [...baseArgs, "--format", "json", "sessions", "new"], + homeDir, + ); + assert.equal(created.code, 0, created.stderr); + assert.equal(created.signal, null); + const createdPayload = JSON.parse(created.stdout.trim()) as { + acpxRecordId?: unknown; + }; + assert.equal(typeof createdPayload.acpxRecordId, "string"); + const sessionId = createdPayload.acpxRecordId as string; + + try { + const [first, concurrent] = await Promise.all([ + runPackageBin( + [...baseArgs, "--format", "quiet", "prompt", "echo packaged-owner-first"], + homeDir, + ), + runPackageBin( + [...baseArgs, "--format", "quiet", "prompt", "echo packaged-owner-concurrent"], + homeDir, + ), + ]); + assert.equal(first.code, 0, first.stderr); + assert.equal(first.signal, null); + assert.equal(first.stdout.trim(), "packaged-owner-first"); + assert.equal(concurrent.code, 0, concurrent.stderr); + assert.equal(concurrent.signal, null); + assert.equal(concurrent.stdout.trim(), "packaged-owner-concurrent"); + + const firstLease = JSON.parse( + await fs.readFile(queuePaths(homeDir, sessionId).lockPath, "utf8"), + ) as { + pid?: unknown; + }; + assert.equal(typeof firstLease.pid, "number"); + + const status = await runPackageBin([...baseArgs, "--format", "json", "status"], homeDir); + assert.equal(status.code, 0, status.stderr); + const statusPayload = JSON.parse(status.stdout.trim()) as { + status?: unknown; + }; + assert.equal(statusPayload.status, "alive"); + + const warm = await runPackageBin( + [...baseArgs, "--format", "quiet", "prompt", "echo packaged-owner-warm"], + homeDir, + ); + assert.equal(warm.code, 0, warm.stderr); + assert.equal(warm.signal, null); + assert.equal(warm.stdout.trim(), "packaged-owner-warm"); + + const secondLease = JSON.parse( + await fs.readFile(queuePaths(homeDir, sessionId).lockPath, "utf8"), + ) as { + pid?: unknown; + }; + assert.equal(secondLease.pid, firstLease.pid); + } finally { + const closed = await runPackageBin( + [...baseArgs, "--format", "json", "sessions", "close"], + homeDir, + ); + assert.equal(closed.code, 0, closed.stderr); + } + }); +}); diff --git a/test/queue-owner-lifecycle.test.ts b/test/queue-owner-lifecycle.test.ts index 67963b56..e2b152fd 100644 --- a/test/queue-owner-lifecycle.test.ts +++ b/test/queue-owner-lifecycle.test.ts @@ -15,7 +15,7 @@ import net from "node:net"; import path from "node:path"; import readline from "node:readline"; import { describe, it } from "node:test"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { isProcessAlive } from "../src/cli/queue/lease-store.js"; import { queueLockFilePath, queueSocketPath } from "../src/cli/queue/paths.js"; import { makeSessionRecord, withTempHome, writeSessionRecordFile } from "./runtime-test-helpers.js"; @@ -96,6 +96,45 @@ function waitForProcessExit( }); } +async function writeLocalNpxAdapter( + homeDir: string, + processInfoPath: string, +): Promise<{ binName: string; packageDir: string }> { + const packageDir = path.join(homeDir, "adapter-package"); + const binName = "acpx-process-tree-fixture"; + const adapterPath = path.join(packageDir, "adapter.cjs"); + await fs.mkdir(packageDir, { recursive: true }); + await fs.writeFile( + path.join(packageDir, "package.json"), + `${JSON.stringify({ + name: binName, + version: "1.0.0", + bin: { [binName]: "adapter.cjs" }, + })}\n`, + ); + await fs.writeFile( + adapterPath, + [ + "#!/usr/bin/env node", + 'const fs = require("node:fs");', + `fs.writeFileSync(${JSON.stringify(processInfoPath)}, JSON.stringify({ pid: process.pid, parentPid: process.ppid }));`, + "setInterval(() => {}, 1_000);", + `void import(${JSON.stringify(pathToFileURL(MOCK_AGENT_PATH).href)});`, + "", + ].join("\n"), + ); + await fs.chmod(adapterPath, 0o755); + return { binName, packageDir }; +} + +async function terminateFixturePid(pid: number): Promise { + if (!isProcessAlive(pid)) { + return; + } + process.kill(pid, "SIGKILL"); + await waitUntil(async () => !isProcessAlive(pid), 2_000); +} + describe("queue owner lifecycle — graceful SIGTERM shutdown", () => { it("exits with code 0 and releases its lease when it receives SIGTERM", async () => { if (process.platform === "win32") { @@ -612,4 +651,114 @@ describe("queue owner lifecycle — bridge process death on SIGTERM", () => { } }); }); + + it("kills a SIGTERM-resistant adapter grandchild launched through npx", async () => { + if (process.platform === "win32") { + return; + } + + await withTempHome("acpx-lifecycle-npx-tree-", async (homeDir) => { + const cwd = path.join(homeDir, "workspace"); + await fs.mkdir(cwd, { recursive: true }); + + const processInfoPath = path.join(homeDir, "adapter-process.json"); + const { binName: adapterBin, packageDir } = await writeLocalNpxAdapter( + homeDir, + processInfoPath, + ); + const record = makeSessionRecord({ + acpxRecordId: "lifecycle-npx-tree-test", + acpSessionId: "lifecycle-npx-tree-session", + agentCommand: `npx --yes --offline --package ${JSON.stringify(packageDir)} -- ${adapterBin} --ignore-sigterm`, + cwd, + }); + await writeSessionRecordFile(homeDir, record); + + const socketPath = queueSocketPath(record.acpxRecordId, homeDir); + const lockPath = queueLockFilePath(record.acpxRecordId, homeDir); + const child = spawn(process.execPath, [CLI_PATH, "__queue-owner"], { + env: { + ...process.env, + HOME: homeDir, + ACPX_QUEUE_OWNER_PAYLOAD: JSON.stringify({ + sessionId: record.acpxRecordId, + permissionMode: "approve-reads", + }), + }, + stdio: ["ignore", "ignore", "pipe"], + }); + const stderrChunks: Buffer[] = []; + child.stderr?.on("data", (chunk: Buffer) => stderrChunks.push(chunk)); + + let queueSocket: net.Socket | undefined; + let adapterPid: number | undefined; + let wrapperPid: number | undefined; + + try { + await waitUntil( + async () => + (await fileExists(socketPath)) || child.exitCode != null || child.signalCode != null, + ); + assert.equal( + await fileExists(socketPath), + true, + `queue owner must open its socket; stderr=${Buffer.concat(stderrChunks).toString("utf8")}`, + ); + queueSocket = await new Promise((resolve, reject) => { + const socket = net.createConnection(socketPath); + socket.once("connect", () => resolve(socket)); + socket.once("error", reject); + }); + queueSocket.write( + `${JSON.stringify({ + type: "submit_prompt", + requestId: "req-npx-tree-test", + message: "sleep 10000", + permissionMode: "approve-reads", + waitForCompletion: true, + })}\n`, + ); + + await waitUntil(() => fileExists(processInfoPath), 8_000); + const processInfo = JSON.parse(await fs.readFile(processInfoPath, "utf8")) as { + pid: number; + parentPid: number; + }; + adapterPid = processInfo.pid; + wrapperPid = processInfo.parentPid; + assert.notEqual( + wrapperPid, + child.pid, + "fixture must launch the adapter as an npx grandchild, not the queue owner's direct child", + ); + assert.equal(isProcessAlive(adapterPid), true, "adapter grandchild must be alive"); + assert.equal(isProcessAlive(wrapperPid), true, "npx wrapper must be alive"); + + child.kill("SIGTERM"); + const { code, signal } = await waitForProcessExit(child, 10_000); + const stderr = Buffer.concat(stderrChunks).toString("utf8"); + + assert.equal(signal, null, `queue owner should exit gracefully; stderr=${stderr}`); + assert.equal(code, 0, `expected queue owner exit code 0; stderr=${stderr}`); + assert.equal( + isProcessAlive(adapterPid), + false, + "SIGTERM-resistant adapter grandchild must not survive npx wrapper teardown", + ); + assert.equal(isProcessAlive(wrapperPid), false, "npx wrapper must be gone"); + assert.equal(await fileExists(lockPath), false, "queue owner lease must be released"); + } finally { + queueSocket?.destroy(); + if (child.exitCode == null && child.signalCode == null) { + child.kill("SIGKILL"); + } + if (adapterPid) { + await terminateFixturePid(adapterPid); + } + if (wrapperPid) { + await terminateFixturePid(wrapperPid); + } + } + }); + }); }); diff --git a/test/queue-owner-process.test.ts b/test/queue-owner-process.test.ts index 9207b783..1e0f511f 100644 --- a/test/queue-owner-process.test.ts +++ b/test/queue-owner-process.test.ts @@ -39,7 +39,7 @@ async function waitForCondition( } describe("resolveQueueOwnerSpawnArgs", () => { - it("prefers ACPX_QUEUE_OWNER_ARGS when provided", () => { + it("prefers ACPX_QUEUE_OWNER_ARGS when provided, including in a SEA", () => { const previous = process.env.ACPX_QUEUE_OWNER_ARGS; process.env.ACPX_QUEUE_OWNER_ARGS = JSON.stringify([ "--import", @@ -48,7 +48,7 @@ describe("resolveQueueOwnerSpawnArgs", () => { "__queue-owner", ]); try { - const args = resolveQueueOwnerSpawnArgs(["node", "ignored.js"]); + const args = resolveQueueOwnerSpawnArgs(["acpx", "acpx"], true); assert.deepEqual(args, ["--import", "tsx", "src/cli.ts", "__queue-owner"]); } finally { if (previous === undefined) { @@ -59,6 +59,14 @@ describe("resolveQueueOwnerSpawnArgs", () => { } }); + it("runs the embedded entrypoint directly in a SEA", () => { + assert.deepEqual(resolveQueueOwnerSpawnArgs(["acpx", "acpx"], true), ["__queue-owner"]); + }); + + it("does not require a JavaScript entry path in a SEA", () => { + assert.deepEqual(resolveQueueOwnerSpawnArgs(["acpx"], true), ["__queue-owner"]); + }); + it("returns and __queue-owner", async () => { await withTempDir(async (dir) => { const cliFile = path.join(dir, "cli.js"); @@ -111,6 +119,13 @@ describe("sanitizeQueueOwnerExecArgv", () => { }); describe("buildQueueOwnerArgOverride", () => { + it("does not create a JavaScript entry override in a SEA", () => { + assert.equal( + buildQueueOwnerArgOverride("/snapshot/acpx/cli.js", ["--import", "tsx"], true), + null, + ); + }); + it("returns null when no loader args remain after sanitization", () => { assert.equal( buildQueueOwnerArgOverride("/tmp/cli.js", [ @@ -272,6 +287,7 @@ describe("spawnQueueOwnerProcess startup capture lifecycle", () => { `; const ownerArgs = JSON.stringify(["--input-type=module", "-e", ownerCode]); const probe = ` + import { writeSync } from "node:fs"; import { spawnQueueOwnerProcess } from ${JSON.stringify(moduleUrl)}; process.env.ACPX_QUEUE_OWNER_ARGS = ${JSON.stringify(ownerArgs)}; const handle = spawnQueueOwnerProcess({ @@ -279,7 +295,7 @@ describe("spawnQueueOwnerProcess startup capture lifecycle", () => { permissionMode: "approve-reads", }); handle.stopStartupCapture(); - console.log(handle.pid); + writeSync(1, String(handle.pid)); `; const result = spawnSync(process.execPath, ["--input-type=module", "--eval", probe], { @@ -293,7 +309,10 @@ describe("spawnQueueOwnerProcess startup capture lifecycle", () => { 0, `submitter did not exit independently: ${result.stderr || String(result.signal)}`, ); - assert.ok(Number.isInteger(ownerPid) && ownerPid > 0, "expected detached owner pid"); + assert.ok( + Number.isInteger(ownerPid) && ownerPid > 0, + `expected detached owner pid; stdout=${JSON.stringify(result.stdout)} stderr=${JSON.stringify(result.stderr)}`, + ); assert.doesNotThrow(() => process.kill(ownerPid, 0), "owner should still be running"); } finally { if (Number.isInteger(ownerPid) && ownerPid > 0) { diff --git a/test/release-workflow.test.ts b/test/release-workflow.test.ts index 9aa03759..bf5ab82f 100644 --- a/test/release-workflow.test.ts +++ b/test/release-workflow.test.ts @@ -73,7 +73,10 @@ function jobBlocks(workflow: string, keepComments = false): Map test("release-binaries separates the signing identity from repository code", () => { const jobs = jobBlocks(readWorkflow("release-binaries.yml")); - assert.deepEqual([...jobs.keys()], ["gate", "build", "attest", "publish"]); + assert.deepEqual( + [...jobs.keys()], + ["gate", "build", "attest", "publish", "formula", "formula-pr"], + ); // `build` runs pnpm install, the bundler, and the freshly built binary. OIDC // there would let any of that code mint provenance for bytes the workflow @@ -89,6 +92,33 @@ test("release-binaries separates the signing identity from repository code", () // `publish` writes the release; it signs nothing. assert.doesNotMatch(jobs.get("publish") ?? "", /id-token/); assert.doesNotMatch(jobs.get("publish") ?? "", /actions\/checkout|pnpm install/); + + // `formula` executes the repository's generator script, so it must be + // read-only: no OIDC, no write permission, no persisted credential, and it + // must render from the immutable tag rather than a movable branch. + assert.doesNotMatch(jobs.get("formula") ?? "", /id-token/); + assert.match(jobs.get("formula") ?? "", /permissions:\n\s+contents: read/); + assert.match(jobs.get("formula") ?? "", /persist-credentials: false/); + assert.doesNotMatch(jobs.get("formula") ?? "", /ref: main/); + assert.doesNotMatch(jobs.get("formula") ?? "", /pnpm install|npm install|npm ci\b/); + + // `formula-pr` holds the contents-write credential, so it must execute no + // repository code: no script invocations, no dependency installs, no signing. + assert.doesNotMatch(jobs.get("formula-pr") ?? "", /id-token/); + assert.doesNotMatch(jobs.get("formula-pr") ?? "", /node scripts|pnpm|npm install|npm ci\b/); +}); + +test("the formula pins only verified bytes", () => { + const formula = jobBlocks(readWorkflow("release-binaries.yml")).get("formula") ?? ""; + + // The binary checksums come from the immutable release's manifest, not from + // build artifacts that could be swapped between jobs. + assert.match(formula, /gh release download .*SHA256SUMS/); + + // The npm checksum must not be trust-on-first-use: the tarball has to carry + // this repository's provenance attestation (created by release.yml before + // publish) before its sha256 is pinned into the formula. + assert.match(formula, /gh attestation verify/); }); test("every artifact gets both provenance and an SBOM attestation", () => { @@ -129,6 +159,17 @@ test("the packaged artifact is proven to be a SEA, not a bare Node copy", () => assert.match(build, /reported.*!=.*expected|\[ "\$reported" != "\$expected" \]/s); }); +test("each release artifact runs the packaged persistent-session smoke tests", () => { + const build = jobBlocks(readWorkflow("release-binaries.yml")).get("build") ?? ""; + const compileTests = build.indexOf("pnpm run build:test"); + const packagedTests = build.indexOf( + 'ACPX_TEST_PACKAGE_BIN="$workdir/acpx" node --test dist-test/test/packaged-bin.test.js', + ); + + assert.ok(compileTests >= 0, "the release build must compile packaged-bin tests"); + assert.ok(packagedTests > compileTests, "the built SEA must run the packaged-bin test suite"); +}); + test("the SBOM describes the artifact this job actually built", () => { const build = jobBlocks(readWorkflow("release-binaries.yml")).get("build") ?? ""; diff --git a/test/spawn-options.test.ts b/test/spawn-options.test.ts index 93c781b7..bff0757a 100644 --- a/test/spawn-options.test.ts +++ b/test/spawn-options.test.ts @@ -111,6 +111,11 @@ test("buildAgentSpawnOptions hides Windows console windows and preserves auth en }); assert.equal(options.cwd, "/tmp/acpx-agent"); + assert.equal( + (options as { detached?: boolean }).detached, + true, + "agent wrappers must lead an owned process group/tree for descendant cleanup", + ); assert.deepEqual(options.stdio, ["pipe", "pipe", "pipe"]); assert.equal(options.windowsHide, true); assert.equal(options.env.ACPX_AUTH_TOKEN, "secret-token");