Skip to content

Commit 354d351

Browse files
thymikeeApex by Callstack
andauthored
fix(host-kit): a killed command settles on its exit, and background exec cannot take a timeout (#2599)
* fix(host-kit): a killed command settles on exit; background exec cannot take a timeout A command killed by its own deadline or by request cancellation settled on 'close', which waits for the stdio pipes to drain. A descendant that inherited those pipes keeps them open after the direct child is gone, wedging the request and the device lock it owns. Both the foreground promise and the background wait now settle on 'exit' once this module asked for the kill, and still wait for a full drain on 'close' when it did not. One finish() owns the timer clear, the abort-listener release, and the trace emit, so the stdin-failure rejection stops bypassing them. killProcessTree no longer signals a child Node already reaped: its pid, and with it the process-group id a detached spawn handed out, is reusable by then. execHostAdb spawns detached like execSerialAdb already does, so a deadline can signal the group instead of only the client. Background runs lose the timeoutMs field they never armed: ExecBackgroundOptions and AndroidAdbSpawnOptions omit it, the two app-log call sites that forwarded it are dropped, and the app-log adb command contract no longer offers it. * fix(host-kit): both the kill request and the child exit can settle a command A deadline that fired after the child had already exited left the command waiting for a `close` its pipe-holding descendant would never release: the group kill that ends that descendant cannot run through a child Node already reaped. Kill requests and child exits now report to one settlement that does not care which arrives first, a detached process group is still killed while its members are reachable, and settling closes our end of the pipes instead of holding them open for a stranger. The fake adb reads its marker path from the environment instead of having it spliced into the source it is generated from. Co-authored-by: Apex by Callstack <noreply@callstack.com> * fix(host-kit): the kill settlement stays inside the closure its callers already pay The Coverage job's eager-closure gate measured 27 entries evaluating one more module once `command-kill-settlement.ts` landed, and its verdict names the remedy: a small module that every affected entry already evaluates belongs inside that module rather than behind a new static edge. The factory now sits in `exec.ts` next to the two commands that construct it; the ordering behavior and its tests are unchanged. Co-authored-by: Apex by Callstack <noreply@callstack.com> * fix(host-kit): the kill settlement is private to the command executor Co-authored-by: Apex by Callstack <noreply@callstack.com> * fix(host-kit): one group-signal seam, and no deadline an app-log tail will not honour `killProcessTree` probed a group with `process.kill(-pid, 0)` and then wrote to it by hand, so host-kit had two group-signal paths and the one every runner-tree kill already used was the one this file could not mock. The probe also decided nothing: EPERM made it report the group reachable and the following write was refused all the same. It is gone, and the detached branch is one call to `signalProcessGroupBestEffort`. That seam now lives in `exec.ts`, below the module that already reached it. `host-process.ts` imports `exec.ts` for `runCmd`, so importing its signal helper back up would close a production value-import cycle, which `check:layering` R4 rejects outright; and giving the two of them a new shared module below both is a module every one of those entries starts evaluating, which the eager-closure budgets reject. `@agent-device/host-kit/process` exports the same name from the new home, and nothing outside host-kit noticed. Two tests moved with the function. The two group-kill tests now answer writes at a guard which records what the kill aimed at and refuses to deliver it — the seam the hermetic signal setup points real kill paths at — and a source-shape test fails if a second `process.kill(-…)` ever appears in this module beside the seam. Reverting the detached branch to a raw group write turns that one red. `AppLogProcessCommand`'s host variant takes `Omit<HostCommandRequest, 'timeoutMs'>`. The background exec it feeds passes `allowFailure`, `cwd`, and `env` and cannot pass a timeout, so a producer that wrote one was writing a budget that never fires. Co-authored-by: Apex by Callstack <noreply@callstack.com> * test(host-kit): a vanished group answers with ESRCH, and no source-shape proxy * test(host-kit): a guarded group write answers the way process.kill does The default mode returned `false` for a write the kernel took, and the comment above it described that as a group with no reachable members. `process.kill` has three answers to a negative pid and none of them is `false`: `true` once the write is accepted, `ESRCH` when no member is left, `EPERM` when a member belongs to someone else. The assertions survived because the seam reads the throw and ignores the return value, so nothing was wrong with the behavior — but the next test written against this helper would copy an answer the kernel never gives, and the comment would keep promising a state the seam cannot observe. The mode is now `delivered` and returns `true`, and `EPERM` is a mode of its own, which is also the test that branch never had: `signalProcessGroupBestEffort` has one catch for both errno values, and only `ESRCH` was ever exercised. Making that catch rethrow `EPERM` turns the new test red and nothing else. * test(host-kit): one double answers every group write in these tests `guardGroupWrites` had been introduced as the seam every group write is answered at, and then two tests beside it went on answering the same question with hand-written `vi.spyOn(process, 'kill')` doubles. Two doubles for one seam is a slow disagreement waiting to happen: the spy said delivery and the guard said delivery differently, and only one of them was checked against what `process.kill` really answers. Both are built on the guard now, and the three answers it can give — `true` for a delivered write, `ESRCH`, `EPERM` — are one table with the report each must produce, so the delivered case and the two refusals cannot drift apart or be edited separately. The invalid-pid checks moved into the same shape rather than staying beside it, and their proof got stronger: the guard records every write it is asked about, so "nothing was signalled" is now an empty list rather than a spy call count, which is a claim about what the seam did rather than about how the spy was wired. The comment clause claiming only one of the three answers was tested is gone, since it was written before the third arrived. Mutations, one at a time: making the catch in `signalProcessGroupBestEffort` rethrow `EPERM` reddens the table test and nothing else; replacing its pid refusal with a NaN check reddens the invalid-pid test through that empty list. Running the second is safe precisely because the guard answers a negative pid itself and never forwards one to the kernel. --------- Co-authored-by: Apex by Callstack <noreply@callstack.com>
1 parent 55d02e9 commit 354d351

14 files changed

Lines changed: 805 additions & 237 deletions

packages/contracts/src/app-log-runtime.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,13 +110,18 @@ export type AppLogBackgroundProcess = AsyncDisposable &
110110
export type AppLogProcessCommand =
111111
| Readonly<{
112112
kind: 'host';
113-
request: HostCommandRequest;
113+
/**
114+
* A streamed log tail is stopped by its owner, so a host command here has no deadline to
115+
* honour: the background exec drops `timeoutMs`. It stays out of the type so a producer
116+
* cannot pass a budget that silently never fires.
117+
*/
118+
request: Omit<HostCommandRequest, 'timeoutMs'>;
114119
}>
115120
| Readonly<{
116121
kind: 'android-adb';
117122
serial: string;
118123
args: readonly string[];
119-
options?: Pick<HostCommandRequest, 'allowFailure' | 'cwd' | 'env' | 'timeoutMs'>;
124+
options?: Pick<HostCommandRequest, 'allowFailure' | 'cwd' | 'env'>;
120125
}>;
121126

122127
export type AppLogBackgroundProcessRequest = Readonly<{

packages/host-kit/src/internal/exec-boundary-faults.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ import { test } from 'vitest';
33
import { AppError } from '@agent-device/kernel/errors';
44
import {
55
runCmd,
6+
runCmdBackground,
7+
runCmdDetached,
8+
runCmdStreaming,
9+
runCmdSync,
610
withCommandExecutorOverride,
711
type CommandExecutorOverride,
812
} from '@agent-device/host-kit/command';
@@ -28,3 +32,27 @@ test('fail-Nth executor drives one deterministic command failure without hiding
2832

2933
assert.deepEqual(calls, [['first'], ['second'], ['third']]);
3034
});
35+
36+
test('the override seam covers the foreground commands and no other spawn path', async () => {
37+
const consulted: string[] = [];
38+
39+
await withCommandExecutorOverride(
40+
(command) => {
41+
consulted.push(command);
42+
return undefined;
43+
},
44+
async () => {
45+
runCmdSync(process.execPath, ['-e', 'process.stdout.write("sync")']);
46+
const background = runCmdBackground(process.execPath, [
47+
'-e',
48+
'process.stdout.write("background")',
49+
]);
50+
await background.wait;
51+
runCmdDetached(process.execPath, ['-e', 'process.exit(0)']);
52+
await runCmdStreaming(process.execPath, ['-e', 'process.stdout.write("streaming")']);
53+
await runCmd(process.execPath, ['-e', 'process.stdout.write("foreground")']);
54+
},
55+
);
56+
57+
assert.deepEqual(consulted, [process.execPath, process.execPath]);
58+
});

0 commit comments

Comments
 (0)