Skip to content

Commit de19329

Browse files
okwasniewskithymikee
authored andcommitted
fix(client): confirm a companion exit before dropping its state
The host refusing SIGTERM was reported as a completed stop, so the profile forgot a live companion and a second one could start beside it. An unreadable identity had the same effect through a different door: on Windows an elevated process answers the command-line query with nothing, and the stop path read that as 'not our companion' and cleared the state. Signals now settle against the host in one place. stopProcessAndConfirmExit takes the identity the caller recorded - the start time and a command matcher - and reports how the process ended: only a released pid, a zombie, or an identity the host contradicts proves the exit; a refusal is retained with its errno, and a pid nobody can describe is retained as identity-unverified without being signalled at all. The companion state is dropped only on 'stopped', and the failure names the pid, the reason, and the state file so a retry stops the same process.
1 parent a95d2d5 commit de19329

7 files changed

Lines changed: 704 additions & 188 deletions

File tree

Lines changed: 281 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,281 @@
1+
import assert from 'node:assert/strict';
2+
import { afterEach, test, vi } from 'vitest';
3+
4+
// Every claim the stopper makes is about what the host answers, so one double answers both syscalls it
5+
// reads: `kill(pid, signal)` for the signals and liveness probes, and `ps` for the identity fields.
6+
// One host state drives both, so no case can be live while its identity says otherwise.
7+
const { mockRunCmd } = vi.hoisted(() => ({ mockRunCmd: vi.fn() }));
8+
vi.mock('./exec.ts', async () => ({
9+
...(await vi.importActual<typeof import('./exec.ts')>('./exec.ts')),
10+
runCmd: mockRunCmd,
11+
}));
12+
13+
import {
14+
stopProcessAndConfirmExit,
15+
type ConfirmedProcessStop,
16+
type ProcessStopTarget,
17+
} from './host-process.ts';
18+
19+
const PID = 4242;
20+
const BUDGET = { termTimeoutMs: 1_000, killTimeoutMs: 500 };
21+
type Signal = 'SIGTERM' | 'SIGKILL';
22+
23+
const START = 'Mon Oct 12 00:00:00 2026';
24+
const OTHER_START = 'Tue Oct 13 09:00:00 2026';
25+
const COMMAND = '/usr/bin/node /app/dist/src/client/companion-tunnel.js --run-metro-companion';
26+
27+
const STOPPED: ConfirmedProcessStop = { status: 'stopped' };
28+
/** `ps` found the row and told nothing for that column, which is unknown rather than absent. */
29+
const EMPTY = { stdout: '', stderr: '', exitCode: 0 };
30+
const PS_REFUSED = { stdout: '', stderr: 'ps: not permitted', exitCode: 1 };
31+
const psAnswer = (value: string) => ({ stdout: `${value}\n`, stderr: '', exitCode: 0 });
32+
const refused = (signal: Signal): ConfirmedProcessStop => ({
33+
status: 'retained',
34+
reason: 'signal-failed',
35+
signal,
36+
errno: 'EPERM',
37+
});
38+
39+
/** What the host will say about the pid, overriding what the recorded process would show. */
40+
type HostIdentity = Readonly<{ startTime?: string | null; command?: string | null }>;
41+
42+
type HostCase = Readonly<{
43+
name: string;
44+
/** The identity the caller recorded beside the pid; absent means it tracks only a pid. */
45+
recorded?: boolean;
46+
/** What the host answers each signal with: a string is the refusing errno, absent accepts. */
47+
answers?: Partial<Record<Signal, string>>;
48+
/** The signal after which the process has run its course, when it ends because it was asked. */
49+
exitsOnSignal?: Signal;
50+
/** How many liveness probes it survives before somebody else ends it. */
51+
exitsAfterProbes?: number;
52+
/**
53+
* How a process that has run its course keeps answering to its pid, which is where liveness and
54+
* identity disagree: a POSIX zombie is `EPERM` beside a `Z` row, and on Windows a process that
55+
* exited while its object still had handles is `EPERM` with no row at all. `'held'` never lets go.
56+
*/
57+
afterExit?: 'zombie' | 'no-row' | 'held';
58+
identity?: HostIdentity;
59+
/** A `ps` the host refuses outright, so identity cannot be read at all. */
60+
psFails?: boolean;
61+
/** The signals the stop must spend, in order, and the verdict each host answer earns. */
62+
expect: { outcome: ConfirmedProcessStop; signals: Signal[] };
63+
}>;
64+
65+
type HostModel = Omit<HostCase, 'name' | 'expect'>;
66+
type Expect = HostCase['expect'];
67+
const HELD = { afterExit: 'held' } satisfies HostModel;
68+
const DENIED_ON_TERM = { answers: { SIGTERM: 'EPERM' } } satisfies HostModel;
69+
const UNREADABLE = { identity: { startTime: null, command: null } } satisfies HostModel;
70+
const RECORDED = { recorded: true } satisfies HostModel;
71+
/** The process has run its course: liveness stops answering for it before anything is signaled. */
72+
const GONE = { exitsAfterProbes: 0 } satisfies HostModel;
73+
const ZOMBIE = { ...GONE, afterExit: 'zombie' } satisfies HostModel;
74+
const NO_ROW = { ...GONE, afterExit: 'no-row' } satisfies HostModel;
75+
const STOPPED_BY_TERM: Expect = { outcome: STOPPED, signals: ['SIGTERM'] };
76+
const STOPPED_BY_KILL: Expect = { outcome: STOPPED, signals: ['SIGTERM', 'SIGKILL'] };
77+
const NEVER_SIGNALED: Expect = { outcome: STOPPED, signals: [] };
78+
const REFUSED_ON_TERM: Expect = { outcome: refused('SIGTERM'), signals: ['SIGTERM'] };
79+
const REFUSED_ON_KILL: Expect = { outcome: refused('SIGKILL'), signals: STOPPED_BY_KILL.signals };
80+
const EXIT_TIMED_OUT: Expect = {
81+
outcome: { status: 'retained', reason: 'exit-timeout', signal: 'SIGKILL' },
82+
signals: STOPPED_BY_KILL.signals,
83+
};
84+
const UNVERIFIED: Expect = {
85+
outcome: { status: 'retained', reason: 'identity-unverified' },
86+
signals: [],
87+
};
88+
89+
const STOP_CASES: HostCase[] = [
90+
// A caller that tracks only a pid — Metro spawns the process itself — can prove the exit only from
91+
// the pid being released or from a zombie's row: it holds no identity to compare an answer against.
92+
{ name: 'leaves on SIGTERM', exitsOnSignal: 'SIGTERM', expect: STOPPED_BY_TERM },
93+
{ name: 'ignores SIGTERM and takes SIGKILL', exitsOnSignal: 'SIGKILL', expect: STOPPED_BY_KILL },
94+
{ name: 'was gone before anything was signaled', ...GONE, expect: NEVER_SIGNALED },
95+
{ name: 'is held by another owner', ...DENIED_ON_TERM, ...HELD, expect: REFUSED_ON_TERM },
96+
{ name: 'refuses SIGKILL', answers: { SIGKILL: 'EPERM' }, ...HELD, expect: REFUSED_ON_KILL },
97+
{
98+
name: 'is released inside the budget',
99+
...DENIED_ON_TERM,
100+
exitsAfterProbes: 3,
101+
expect: STOPPED_BY_TERM,
102+
},
103+
{ name: 'leaves a zombie the budget cannot wait out', ...ZOMBIE, expect: STOPPED_BY_TERM },
104+
{
105+
// An absent row is not proof of absence: a hidden or protected process is absent from `ps` too,
106+
// so the pid that still answers `kill(pid, 0)` is retained rather than forgotten.
107+
name: 'leaves no row while its pid still answers',
108+
...NO_ROW,
109+
...DENIED_ON_TERM,
110+
expect: REFUSED_ON_TERM,
111+
},
112+
{ name: 'outlives both budgets', ...HELD, expect: EXIT_TIMED_OUT },
113+
114+
// A caller that recorded an identity — the companion's state file — additionally proves a stale pid,
115+
// and an unreadable identity stops the stopper before it signals a process nobody can name.
116+
{ name: 'recorded process was gone first', ...RECORDED, ...GONE, expect: NEVER_SIGNALED },
117+
{ name: 'recorded process left a zombie', ...RECORDED, ...ZOMBIE, expect: NEVER_SIGNALED },
118+
{
119+
name: 'recorded process refuses SIGTERM',
120+
...RECORDED,
121+
...DENIED_ON_TERM,
122+
...HELD,
123+
expect: REFUSED_ON_TERM,
124+
},
125+
{
126+
// Windows hides only the command line of an elevated or other-user process, and that is the #3540
127+
// companion: it must be signaled and retained, never reported stopped.
128+
name: 'recorded process hides its command line, as an elevated one does',
129+
...RECORDED,
130+
identity: { command: null },
131+
...DENIED_ON_TERM,
132+
...HELD,
133+
expect: REFUSED_ON_TERM,
134+
},
135+
{
136+
name: 'recorded process hides its start time',
137+
...RECORDED,
138+
identity: { startTime: null },
139+
...DENIED_ON_TERM,
140+
...HELD,
141+
expect: REFUSED_ON_TERM,
142+
},
143+
{
144+
// POSIX `hidepid`, or a `ps` that times out: the pid answers `kill(pid, 0)` and nothing else.
145+
name: 'live recorded pid whose whole identity is unreadable',
146+
...RECORDED,
147+
...UNREADABLE,
148+
...HELD,
149+
expect: UNVERIFIED,
150+
},
151+
{
152+
name: 'live recorded pid whose process tool the host refuses',
153+
...RECORDED,
154+
psFails: true,
155+
...HELD,
156+
expect: UNVERIFIED,
157+
},
158+
{
159+
name: 'pid the host has handed to another lifetime',
160+
...RECORDED,
161+
identity: { startTime: OTHER_START },
162+
expect: NEVER_SIGNALED,
163+
},
164+
{
165+
name: 'pid whose command line names a different program',
166+
...RECORDED,
167+
identity: { command: '/usr/bin/other --serve' },
168+
expect: NEVER_SIGNALED,
169+
},
170+
];
171+
172+
function driveHost(model: HostCase): { signals: Signal[] } {
173+
const signals: Signal[] = [];
174+
let ended = false;
175+
let probes = 0;
176+
const hasEnded = () =>
177+
ended ||
178+
(model.afterExit !== 'held' && probes >= (model.exitsAfterProbes ?? Number.POSITIVE_INFINITY));
179+
vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string | number) => {
180+
if (signal === 0) {
181+
probes += 1;
182+
// A released pid answers `ESRCH`. A zombie, and on Windows a process whose object still has
183+
// handles, both keep answering `EPERM`, and only the process table tells those apart.
184+
if (hasEnded() && model.afterExit === undefined)
185+
throw Object.assign(new Error('no such process'), { code: 'ESRCH' });
186+
if (hasEnded()) throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' });
187+
return true;
188+
}
189+
const name = signal as Signal;
190+
signals.push(name);
191+
const errno = model.answers?.[name];
192+
if (errno) throw Object.assign(new Error(`kill refused: ${errno}`), { code: errno });
193+
if (model.exitsOnSignal === name) ended = true;
194+
return true;
195+
}) as typeof process.kill);
196+
197+
// Absent means the recorded process, `null` means the host withholds the column, and a string is
198+
// what it says instead.
199+
const identity = {
200+
startTime: model.identity?.startTime === undefined ? START : model.identity.startTime,
201+
command: model.identity?.command === undefined ? COMMAND : model.identity.command,
202+
};
203+
mockRunCmd.mockImplementation(async (_command: string, args: readonly string[]) => {
204+
if (model.psFails) return PS_REFUSED;
205+
// A process the host has fully released, and one that left no row, both answer no fields.
206+
if (hasEnded() && model.afterExit !== 'zombie') return EMPTY;
207+
const field = String(args[3]);
208+
if (field === 'state=') return psAnswer(hasEnded() && model.afterExit === 'zombie' ? 'Z' : 'S');
209+
// Withheld is its own answer: `ps` finds the row and reports nothing for that column, which is
210+
// how Windows answers `CommandLine` for another user's process.
211+
const column = field === 'lstart=' ? identity.startTime : identity.command;
212+
return column === null ? EMPTY : psAnswer(column);
213+
});
214+
return { signals };
215+
}
216+
217+
function targetOf(model: HostCase): ProcessStopTarget {
218+
if (!model.recorded) return { pid: PID };
219+
return { pid: PID, startTime: START, matches: (command) => command === COMMAND };
220+
}
221+
222+
afterEach(() => {
223+
vi.useRealTimers();
224+
vi.restoreAllMocks();
225+
mockRunCmd.mockReset();
226+
});
227+
228+
// The host model above answers with POSIX `ps` fields; a Windows host answers the same questions from
229+
// CIM, which never reports a zombie state. win32 branches live in host-process-windows.test.ts.
230+
test.skipIf(process.platform === 'win32').each(STOP_CASES)(
231+
'stopProcessAndConfirmExit $name',
232+
async (testCase) => {
233+
const host = driveHost(testCase);
234+
// Both stage budgets, advanced on a fake clock: a retained outcome must not cost real time.
235+
vi.useFakeTimers();
236+
const stop = stopProcessAndConfirmExit(targetOf(testCase), BUDGET);
237+
await vi.advanceTimersByTimeAsync(BUDGET.termTimeoutMs + BUDGET.killTimeoutMs);
238+
239+
assert.deepEqual(await stop, testCase.expect.outcome);
240+
assert.deepEqual(host.signals, testCase.expect.signals);
241+
},
242+
);
243+
244+
// `kill(0, SIGTERM)` addresses this very process group, so an unvalidated pid must not reach it.
245+
test.each([0, -1, 1.5, Number.NaN])(
246+
'never signals a pid that is not a process: %s',
247+
async (pid) => {
248+
const signals: string[] = [];
249+
vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string) => {
250+
signals.push(signal);
251+
throw Object.assign(new Error('no such process'), { code: 'ESRCH' });
252+
}) as typeof process.kill);
253+
assert.deepEqual(await stopProcessAndConfirmExit({ pid }, BUDGET), STOPPED);
254+
assert.deepEqual(signals, []);
255+
},
256+
);
257+
258+
// Stale or malformed process state can name the caller's own pid, and SIGTERM would end the very
259+
// process trying to stop it.
260+
test('refuses to stop the current process', async () => {
261+
const signals: string[] = [];
262+
vi.spyOn(process, 'kill').mockImplementation(((_pid: number, signal: string) => {
263+
signals.push(signal);
264+
return true;
265+
}) as typeof process.kill);
266+
await assert.rejects(() => stopProcessAndConfirmExit({ pid: process.pid }, BUDGET), RangeError);
267+
assert.deepEqual(signals, []);
268+
});
269+
270+
// Node's own argument faults carry a `code` that is not an errno, and reading one as a refusal would
271+
// send the caller off to stop a process that never existed. The fault itself has to reach it.
272+
test('propagates an argument fault instead of reporting it as a host refusal', async () => {
273+
const failure = Object.assign(new TypeError('bad signal'), { code: 'ERR_INVALID_ARG_TYPE' });
274+
vi.spyOn(process, 'kill').mockImplementation(() => {
275+
throw failure;
276+
});
277+
const settled = await stopProcessAndConfirmExit({ pid: PID }, BUDGET).catch(
278+
(error: unknown) => error,
279+
);
280+
assert.equal(settled, failure);
281+
});

0 commit comments

Comments
 (0)