2 Commits

Include renames

Author	SHA1	Message	Date
3 people	2b63091421	fix(runner): adopt untracked runner webhooks only from the reporting process generation (#1943) ... * feat(cli): report the CLI process start marker in session metadata The session webhook only names hostPid. A PID alone does not tell the runner which process generation reported it: once the CLI exits, the OS can hand the same PID to any other process, including another runner-spawned CLI. Add metadata.hostStartMarker, the reporting process's own start marker (getProcessStartMarker(process.pid), the same probe the runner uses for its resume records and the stop-session generation check). buildSessionMetadata is the only place that sets hostPid, so every bootstrap path (fresh, existing, lazy, shared Codex) reports it. The runner uses it to decide whether an untracked runner webhook may be adopted (decideUntrackedRunnerWebhook). * fix(runner): adopt untracked runner webhooks only from the reporting process generation Since #1911 the runner adopts an untracked `startedBy: 'runner'` webhook (runner restart before the webhook; Claude has no HAPI id on argv) and persists getProcessStartMarker(pid) of whatever currently owns hostPid, so that a later stopSession can tree-kill that generation. If the webhook is late - the reporting CLI has exited and the OS has already reused its PID - the persisted marker belongs to a foreign process, possibly another runner-spawned CLI. The generation check in stopSession then passes and the foreign process tree is killed. The webhook now has to prove the generation itself: the CLI reports its own marker as metadata.hostStartMarker and decideUntrackedRunnerWebhook adopts only when it matches the live PID. A marker that names another generation, or a failed probe, is ignored without killing anything, also on the webhook-timeout path: a proven mismatch means the PID is not the ghost. A webhook without a marker (older CLI) is left untracked instead of adopted blindly; the timeout kill for those CLIs is unchanged. The integration test drives a real runner over its control server: a webhook carrying the live marker is adopted, listed and stoppable; a webhook with a stale or missing marker leaves the PID untracked and alive, and stop-session reports unknown. Follow-up to #1911 (#1910). * fix(cli): probe the process start marker only for runner-started sessions buildSessionMetadata probed getProcessStartMarker(process.pid) on every bootstrap, terminal starts included. The runner reads metadata.hostStartMarker only in the untracked `startedBy: 'runner'` path (decideUntrackedRunnerWebhook); a terminal webhook never consumes it, and neither do hub or web. The probe is a synchronous spawn: `ps` on POSIX, `tasklist` plus a PowerShell Get-CimInstance call on Windows, each with a 10 s timeout. A terminal `hapi` start on Windows therefore blocked on a PowerShell cold start before the session row existed, for a value nobody reads. With shared Codex the probe also ran per /new and /fork on the event loop shared by all sibling roots. Set hostStartMarker only when startedBy is 'runner'; that is the one case the runner evaluates. This narrows the earlier commit: every bootstrap path reports the marker for runner-started sessions, not for terminal ones. * fix(codex): keep hostStartMarker in the shared runtime's runner webhook The shared Codex runtime bootstraps its roots with reportStarted: false and reports them to the runner itself, from root.session.getMetadata(). That is the hub's view of the metadata: the hub parses stored metadata through MetadataSchema, so a hub that predates hostStartMarker strips the field and the webhook arrives without it. The hub's update-metadata merge never returns it either, so the loss lasts for the root's lifetime. With the generation check, an untracked runner-started webhook without a marker is ignored instead of adopted. A runner restart (for example the version handoff of a single-host upgrade, where the hub process keeps running the old code) followed by /new or /fork in a long-lived shared root therefore left the root unlisted and untracked, the very situation the adoption from #1911 addresses. The Claude and lazy bootstrap paths are unaffected: they report the local metadata. Take the marker from the local bootstrap metadata instead. The opt-in runtime integration test models the stripped hub view in its session mock and asserts the reported payload still carries the marker. The mock also gains the `on` subscription that SharedCodexRoot requires since #1911, without which the suite's bind test could not run. * test(runner): run the late-webhook suite in the serial integration project lateRunnerWebhook.integration.test.ts starts a real runner and a detached child process per case, but the default vitest project picked it up and `bun run test` ran it in a parallel worker. Tests that spawn runner process trees belong to the serial integration project (vitest.integration.config.ts, CI job `integration`), as the config comments and .github/workflows/test.yml require. Include the file in the integration project, exclude it from the default one, and remove the temporary HAPI_HOME of each case, which was left behind in the temp directory. --------- Co-authored-by: Benedict Groppe <195190800+BGGBTAC@users.noreply.github.com>	2026-10-28 12:10:21 +08:00
SSU-WEI HUANGandGitHub	291e7bc40b	fix(test): stop runner integration suite from leaking detached process trees (#1515) (#1521) ... * fix(test): stop runner integration suite from leaking detached process trees (#1515) The default CLI test run included runner.integration.test.ts, which spawns real detached runner/session process trees. A failing, timed-out, or interrupted test (or a plain runner stop) left those trees alive under PID 1 — on the Mac this accumulated ~600 Node/Bun/agent processes and several GiB of RSS over repeated runs. Test harness changes only; production runner session-preservation semantics are untouched: - Exclude runner.integration.test.ts from the default parallel unit-test suite; move it into a dedicated serial integration project (vitest.integration.config.ts, 'bun run test:integration'). The 20-session stress test is opt-in via HAPI_RUN_STRESS_TESTS=true. - Add a test-owned process/session registry (processRegistry.ts): every runner, runner-spawned session, and terminal-style child is registered immediately after spawn; afterEach/afterAll run two-stage cleanup (logical stopRunnerSession first, then bounded process-tree kill), followed by a marker sweep for agent grandchildren reparented to PID 1. - Add a per-run HAPI_TEST_MARKER env stamp + identity/secret env neutralization for test children (integrationEnv.ts) so outer HAPI/pi session variables never leak into test processes and the final audit can recognize test-owned processes by env alone. - Final suite audit in globalSetup teardown: reap anything still carrying the run marker and fail with PID/command diagnostics if anything cannot be reaped, before removing the temp home. - Regression coverage: a deliberately failing test registers a detached child and the follow-up audit must find zero test-owned processes. - CI: replace the dead .env.integration-test step with a dedicated integration job running the serial project. * refactor(test): drop unused killByChildProcess import and child field from registry * chore(test): raise integration hookTimeout to 60s for slow teardown hosts * fix(test): fail loudly when the process-table audit cannot scan; assert regression child death Bot review #1521 findings: - A failed `ps` scan (unsupported flags, buffer exhaustion, permissions) previously returned [] and silently disabled both teardown audit layers. It now throws; globalSetup teardown catches the scan error into the audit error (temp home is still removed) so the run fails visibly. - The regression audit test cleaned the leak with the reaper before asserting, and force-killed the fresh marked runner. The failing test's direct child PID is now asserted dead in afterEach right after registry cleanup (before the marker sweep), and the audit test stops its own runner gracefully before reaping. * fix(test): bound the logical cleanup phase so a hung runner cannot stall the hook Bot review #1521: stopRunnerSession carries the worker's 60s HTTP timeout (setup.ts raises HAPI_RUNNER_HTTP_TIMEOUT for the stress test), and the integration hook timeout is also 60s — N sequential stops could exhaust the hook budget before the process-tree fallback and marker sweep ran, recreating the very leak this change prevents. Logical shutdown is now parallel (Promise.allSettled over all tracked sessions) and the whole phase (stops + PID resolution) races against a 15s budget, so stage-2 tree-kill and the marker sweep always get their share of the hook window. * fix(test): bound graceful runner stop in hooks; keep credentials out of audit diagnostics Bot review #1521 (follow-up): - stopRunner()'s HTTP stop can burn the worker-wide 60s timeout on a hung-but-live runner, starving the marker sweep within the hook budget. afterEach/afterAll now race the graceful stop against a 10s bound; a runner that does not stop in time is force-reaped by the sweep (it carries the run marker) and the next beforeEach's alive-PID guard ignores any stale state file. - The env-bearing ps scan (ps eww) was also used for diagnostics, so the first 500 chars of a short-command process could print inherited credentials (CLI_API_TOKEN etc.) into teardown error logs. The scan now only identifies marked PIDs; command lines are fetched separately without 'e', falling back to '(command unavailable)' instead of the env dump. * fix(test): reap runner model-probe orphans before the zero-survivor inspection Bot review #1521 (Minor): inspect-before-reap. Applying it exposed a real race: each test's runner legitimately spawns marker-carrying children at startup (agent acp + agent --list-models model-catalog probes). Stopping the runner orphans them (ppid 1) with the run marker, so the audit test's OWN runner polluted the pure inspection with fresh probes spawned after the failing test's sweep window. - reapTestOwnedProcesses now re-kills every re-scan iteration instead of killing once and only re-scanning, so a process that survived its first SIGKILL (mid-exec) or spawned mid-kill is not given a free pass. - The regression audit test stops its runner, reaps (clearing its own legitimate orphan probes), then inspects: anything still marked is a genuine survivor the bounded reaper could not remove and fails the suite. Killable leaks from the failing test are already asserted dead in afterEach before the sweep runs. * fix(test): strictly bound the marker reaper; make per-test sweep unconditional and verified Bot review #1521 (follow-up): - The 10s reap deadline did not bound the awaited per-tree kills: each killProcessTreeByPid can wait up to 2s per PID, so several stuck processes could still exceed the 60s hook budget. Every process in a test-owned tree carries the marker (env is inherited), so tree-walking is unnecessary: the reaper now SIGKILLs every marked PID found by each scan, fire-and-forget, and re-scans every 250ms — the deadline strictly bounds the function. - The per-test sweep was skipped when the direct-child assertion failed first, and its survivors were ignored. afterEach now snapshots the regression-child state BEFORE the unconditional sweep, then verifies both the registry result and the sweep leftovers. * fix(test): replace it.fails regression with a direct assertion test Bot review #1521 (Minor): Vitest applies the it.fails expected-failure inversion after afterEach, so a broken registry assertion inside the hook would be masked as an expected failure, and the marker sweep would erase the evidence before the follow-up audit ran. The regression is now a normal test that registers a detached child at spawn time, deliberately performs NO per-test teardown, runs only the spawn-time registered cleanup, and asserts the child PID is dead. The afterEach no longer carries the registry-leak assertion (moved into the test body where it cannot be inverted); the per-test sweep assertion and the final audit test are unchanged. * fix(test): bound registry stage-2 tree-kills; require live regression fixture Bot review #1521 (follow-up): - Stage-2 killProcessTreeByPid awaits per descendant serially and can consume the whole 60s hook for a large/stuck tree. Signals are all delivered synchronously (children first) before any waiting, so racing the awaits against a 5s budget bounds the phase without skipping any kill; waitForAllDead still verifies the outcome. - The regression test could pass vacuously if its fixture exited during the startup delay (the registry exit listener would remove it before cleanup). It now asserts the child is alive before running cleanup. * fix(test): kill registered roots with bare synchronous SIGKILL, no pgrep walk Bot review #1521 (follow-up): racing the mapped killProcessTreeByPid calls against a timer does not bound the phase — evaluating the map invokes each call immediately, and each runs the recursive synchronous pgrep walk before its first await, which can consume the hook before the timer, runner stop, or marker sweep run. Stage-2 now SIGKILLs registered roots directly (fire-and-forget, no tree walk, no per-PID waits) and waits a bounded 5s for death. Descendants are reaped by the unconditional marker sweep immediately afterward — every descendant inherits the run marker, so tree-walking is unnecessary. * fix(test): drop duplicate process-death wait in registry cleanup Bot review #1521 (Minor): the duplicated waitForAllDead delayed the authoritative marker sweep by another 5s under the exact stuck-process condition the harness must handle. Keep the single bounded wait; the afterEach marker sweep remains the guarantee.	2026-08-12 09:29:59 +08:00