fix(client): confirm a companion exit before dropping its state

[thymikee]

Problem

stopCompanionProcess reported a companion stopped in two cases where nothing proved it had left:

• the host refused SIGTERM (EPERM, another user or an elevated process — the Client stop paths treat EPERM as a confirmed exit and drop state for a live companion #3540 case);
• readProcessCommand returned null, which the stop read as "not our companion".

Both then ran clearCompanionArtifacts, so stopCompanionTunnel returned stopped: true and ensureCompanionTunnel started a second companion beside the live one. On Windows Win32_Process.CommandLine is empty for an elevated or other-user process, so that door was common; POSIX hits it on a ps timeout or under hidepid.

Change

stopProcessAndConfirmExit (host-kit) takes the recorded identity — startTime and a command matcher — and signals a pid only where a claim it recorded is confirmed by a readable fact. A released pid, a zombie row, or an identity the host contradicts proves stopped; a refusal is retained with its errno; claims resting on a silent host are retained as identity-unverified without being signalled. A missing table row proves nothing, matching waitForDaemonExit. A strict liveness probe means only ESRCH releases a pid, and a non-errno fault propagates instead of reading as "gone".

The verdict's details projection and never-signalled wording sit beside it, so #3575's builder reports it identically. The companion passes state.startTime plus isCompanionTunnelCommand, removing the fail-open precheck, and drops state only on stopped.

Metro is split to #3575. Migrating stopDaemonProcess and terminateOwnedProcess is a follow-up: it needs the identity-pinned wait moved out of daemon-contracts first.

Validated: check:affected --run exit 0 at 9d7f73b5f.

Closes #3540

[github-actions]

PR Preview Action v1.8.1
🚀 View preview at https://callstack.github.io/agent-device/pr-preview/pr-3551/
Built to branch gh-pages at 2026-10-13 17:00 UTC. Preview will be ready when the GitHub Pages deployment is complete.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.19 MB	5.19 MB	+2.9 kB
Package (unpacked)	5.19 MB	5.19 MB	+2.9 kB
Package (download)	1.56 MB	1.56 MB	+1.1 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	21.1 ms	21.8 ms	+0.7 ms
CLI --help	58.6 ms	58.0 ms	-0.6 ms

[cubic-dev-ai]

All reported issues were addressed across 11 files

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 10 files (changes from recent commits).

<sub>Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[thymikee]

Reviewed eb8b7a7. The change is close, but one case still drops a live companion's state, and that is the bug this PR sets out to close. CI is green with 22 of 22 checks passing, and there are no conflicts.

stopCompanionProcess still returns silently when the companion is alive but readProcessCommand(state.pid) is null. Both callers then run clearCompanionArtifacts, so stopCompanionTunnel reports stopped: true and ensureCompanionTunnel starts a second companion. A null command means the identity is unknown, not that the process is gone. On Windows, Win32_Process.CommandLine is empty for an elevated or other-user process when the caller is not elevated, and the row parser maps that to null. That is the "started with elevated privileges" companion from #3540, and it never reaches the new helper. The same branch fires on POSIX when ps times out or under hidepid. The rule the code must satisfy is that state is dropped only when the host proves the pid is gone, or proves it is a different process (a readable start time that differs from state.startTime, or a readable command that does not match). An unreadable identity must keep the state and return a typed reason such as identity-unverified, as the daemon stopper does. Please apply this in stopCompanionProcess for both callers, and compare state.startTime, which the state records and shouldReuseCompanion checks but the stop path ignores. The "never signals a pid whose command line is unreadable" test in client-metro-companion.test.ts currently asserts keepsState: false. It should assert keepsState: true with details.reason.

Could the host-kit stopper take an identity argument, such as stopProcessAndConfirmExit({ pid, startTime?, matches?(command) }, budget)? It would return retained with identity-unverified when identity cannot be read, and exited when the start time or command has provably changed. The companion would pass state.startTime and isCompanionTunnelCommand, which removes the fail-open precheck above. stopDaemonProcess and the app-log terminateOwnedProcess could then delegate to it, leaving one owner of "confirmed exit". That means moving the identity-pinned wait (the zombie and start-time logic in waitForDaemonExit) from daemon-contracts into host-kit first. The daemon migration can be a separate PR, but the identity argument and the fix above belong in this one.

Not blocking, take or leave: the new docs sentence in build-an-integration.md covers only stopMetroTunnel(), while the react-devtools companion goes through the same stop and ensure path and now fails the same way, so one line in its help or docs would help; and the zombie-row check in holdsHostProcessRecord plus the ConfirmedProcessStop shape repeat what waitForDaemonExit and DaemonTerminationResult already hold, which the question above would resolve.

Of the ten bot threads, the pid === process.pid RangeError is fixed at this commit with a test, and so are the P2 threads mockReturnValue in the test, the half-spawned test, the docs guarantee, the win32 skipIf and the persistUnstoppedCompanion retry. The P3 threads on the SIGKILL case, on the client-metro.ts doc and on the "still live afterwards" wording are fixed too, and the react-devtools.ts thread no longer applies because that file moved to #3559. None of these still apply, so you can resolve them all.

I did not reproduce the Windows behavior on a Windows host, and I did not rerun the mutation checks or the local gate. I also did not run a real other-user companion through disconnect. The host-kit behavior is covered only through the mocked exec and process.kill fakes, and no device-facing live run is needed for this change. A refused SIGTERM costs the full 1s termTimeoutMs plus one sync ps read before it retains. That is shorter than the spawn and readiness work it precedes, but I did not measure it. Before merge, make an unreadable companion identity retain its state with a typed reason, and flip that test to expect the state kept.

[thymikee]

Rebased onto current main; the required change is in at de19329.

The fail-open door is closed. stopCompanionProcess no longer inspects the process — it hands host-kit the identity it recorded and acts on the verdict:

stopProcessAndConfirmExit(
  {
    pid: state.pid,
    identity: {
      startTime: state.startTime,
      matches: (command) => isCompanionTunnelCommand(command, definition),
    },
  },
  budget,
);

ConfirmedProcessStop gained retained + identity-unverified, returned when the host will not describe the pid: nothing is signalled, state is kept, and details.reason says so. stopped now requires proof — a released pid (only ESRCH counts), a Z row, or an identity the host contradicts. state.startTime is compared, so a recycled pid is proven stale and never signalled. The unreadable-command case in client-metro-companion.test.ts is flipped to keepsState: true and asserts the reason and the operator's wording.

Liveness: isProcessAlive reads every answer but EPERM as absence, so the stopper uses a strict probe. That matters beyond this bug: a non-errno fault (ERR_INVALID_ARG_TYPE) was on a path to being read as "the process is gone".

Naming: your shape said exited; the verdict keeps this union's existing spelling, so a provably-changed start time or command returns { status: 'stopped' } — the caller may forget the process, which is what exited meant.

Size: 892 gross. The Metro half moved to #3575, stacked here, per your split option; the react-devtools line you allowed is one sentence in commands.md, since that companion shares this stop and ensure path. Daemon and app-log stoppers untouched, as directed — the body notes the daemon-contracts prerequisite.

identity is required rather than optional, so a caller cannot silently ask for a pid-only stop: an identity that claims nothing is itself retained as identity-unverified.

pnpm check:affected --run exit 0 at head e9c174d, under the full-gate lock.

[thymikee]

The code at e9c174d fixes most of what the earlier review raised (#3551 (comment)), but one defect remains. All 22 checks pass at this commit. Before merge, the stop-identity check needs the fix below.

In proveStopTargetIdentity (host-process.ts:640), the function returns 'unverified' only when the host withholds both the start time and the command. Take a target with identity {matches} and no start time. That is the companion state when readProcessStartTime(pid) returned null at spawn (client-companion-tunnel.ts:432). Now the host reports a start time but withholds the command line, for example Windows CIM for an elevated or other-user process, or one ps read timing out. Every check is skipped, the function returns 'ours', and SIGTERM/SIGKILL go to a pid that no recorded claim identifies. The pid may have been recycled. On POSIX, an unrelated process of the same user can be terminated. On Windows, the user gets 'belongs to another user… sudo kill ' advice aimed at an unrelated process. This breaks the PR's own rule that a pid nobody can describe is never signalled. The daemon stopper already keeps this case as missing-start-time (daemon-process.ts:120-123). Please make the rule: the target is 'ours' only when at least one recorded claim is confirmed by a readable fact (equal start time, or a readable command that matches) and no claim is contradicted. Every other live, non-zombie answer is 'unverified'. Apply it in proveStopTargetIdentity, the one place that authorises a signal. Then add a host-kit STOP_CASES row with a {matches}-only identity, a readable start time and a withheld command, expecting UNVERIFIED and no signals.

Not blocking: stopRequests() in src/__tests__/client-metro-companion.test.ts only projects pid, start time and budget, so removing matches from stopCompanionProcess leaves the companion suite green; asserting that target.identity.matches accepts COMPANION_TUNNEL_COMMAND and rejects an unrelated command would close that gap, and you can take or leave it.

Six cubic-dev-ai threads are fixed at this commit and can be resolved: the self-pid RangeError, per-case teardown mocks, the half-spawned write fault, the doc guarantee scope, the win32 test split, and retry after an unconfirmed stop. Four more can be resolved because the files are no longer in this PR: the startup cleanup test (Metro moved to #3575, and host-kit covers the SIGKILL refusal), react-devtools.ts (moved to #3559), and two client-metro.ts threads (moved to #3575).

I read the diff and did not run the tests or the gate, and I did not reproduce the trigger on Windows or with a partial ps timeout; it follows from reading proveStopTargetIdentity and the spawn-time start-time read. The companion tests mock stopProcessAndConfirmExit, so only the host-kit model proves the identity logic. I did not review the upstream rebase content (fallowrc, ci.yml, runner, mutation scripts).

[thymikee]

Fixed at 9d7f73b5f. The rule is now: ours requires a claim the caller recorded to be confirmed by a fact the host read out, and no claim contradicted.

if (target.identity.startTime && facts.startTime && facts.startTime !== target.identity.startTime)
  return 'gone';
if (target.identity.matches && facts.command && !target.identity.matches(facts.command))
  return 'gone';
const confirmedStartTime = Boolean(target.identity.startTime && facts.startTime);
const confirmedCommand = Boolean(target.identity.matches && facts.command);
return confirmedStartTime || confirmedCommand ? 'ours' : 'unverified';

Both contradictions run first, so reaching the confirmation step already means nothing was contradicted. The facts.startTime === null && facts.command === null guard is gone — redundant under this rule — as is the early "recorded nothing" return, since an identity that claims nothing can confirm nothing.

Two STOP_CASES rows added, both UNVERIFIED with signals: []:

• has only a command matcher and the host withholds the command line — your trigger, reachable today: startCompanionProcess stores startTime: readProcessStartTime(pid) ?? undefined (client-companion-tunnel.ts:432).
• has only a recorded start time and the host withholds the start time — the mirror, which is the identity shape fix(metro): report what the dev-server stop proved instead of assuming success #3575 hands the stopper, since Metro claims no command.

The table's recorded?: boolean became claims?: 'startTime' | 'matches' | 'both' | 'none' to express a half-recorded identity. Mutation-checked both directions: restoring the both-null guard fails the two new rows and the nothing-recorded row; the shipped rule keeps all 24 green.

Took the non-blocking item. stopRequests() now projects the matcher's verdicts — namesCompanion accepts COMPANION_TUNNEL_COMMAND, namesStranger refuses /usr/bin/nginx -g daemon off; — because a predicate compares by identity, so deepEqual cannot see it. Dropping matches from stopCompanionProcess now fails 8 companion tests; matches: () => true fails the same 8.

Your ten threads already each carry a reply and are resolved. Rebased onto f70224ad8 (vitest 5 landed, so the lockfile moved); suite green on 5.0.4. pnpm check:affected --run exit 0 recorded at 9d7f73b5f. Gross 984.

Two follow-ups you did not ask for, both driven by your own rule, on #3575: the retained-verdict details projection and the never-signalled sentence now live beside ConfirmedProcessStop, since the Metro builder and the companion builder were paraphrasing each other; and the bridge wrapper forwards verdict keys only, gated on the typed reason, instead of the whole inner details, which was leaking the companion's logPath and retry advice under a Metro command's error.

[thymikee]

The PR is ready at 9d7f73b. The earlier findings from e9c174d (#3551 (comment)) are fixed, and I found no new problems in the changes since then.

Not blocking: confirmedProcessStopDetails and describeUnconfirmedProcessStop in host-process.ts have one caller here, and confirmedProcessStopDetails takes a separate pid that the caller passes as retained.pid. This is fine if #3575, which has the second caller, lands next. Otherwise the helpers could move into #3575. Take it or leave it.

None of the cubic-dev-ai threads still applies, so please resolve them all. These are fixed at this commit: self-pid guard, afterEach reset, half-spawned write fault, docs wording, skipIf(win32) and persistUnstoppedCompanion. These no longer apply because their files are not in this PR: client-metro-startup-cleanup.test.ts (Metro moved to #3575, and the host-kit "refuses SIGKILL" row covers the case), react-devtools.ts (moved to #3559), and the two client-metro.ts threads (moved to #3575).

I did not run the host-kit or companion suites or the repo checks. I checked the identity rule by tracing it by hand against the new test rows. The companion tests mock stopProcessAndConfirmExit, so only the host-kit tests prove the rule. I did not reproduce the Windows case where the command line is withheld, or a partial ps timeout. I did not review the upstream rebase content in the diff stat. At this commit I spot-checked only the five fixed items above.

No conflicts. CI is still running: 12 checks are queued or in progress and none has failed. Unit, Integration and Coverage are the jobs that exercise this change. Merge waits on those finishing green at 9d7f73b.