fix(host-kit): a killed command settles on its exit, and background exec cannot take a timeout - #2599
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Settling on A smaller-design question: could the timer, the abort and
The CodeQL alert on |
|
Review follow-up in ca2108d. The hang the added The detached group is still killed. Settling closes our end of the pipes. Tests: Mutation evidence: removing the settlement from the kill request turns 5 tests red, with the three deadline tests waiting the full 2 s; dropping the reachability probe turns the group-empty test red; restoring the post-reap early return turns the group-kill test red. CodeQL improper sanitization on the fake Gate at ca2108d: |
|
Two follow-ups after the Coverage job came back red on The new module was the problem, not the behavior. The eager-closure gate measured 27 entries evaluating one more module once The branch was behind. Gate after the merge at |
|
At a36dd28 the deadline, the abort, One part of #2522 is still open. The
Smoke Tests are still running, and there are no conflicts. |
|
Pushed
It could not stay in
So the seam moved down, into On mocking it. A That was with the raw write restored; Gate: |
|
Verified at
|
|
Smoke status, since the lane has been unstable rather than consistent:
Three attempts, three different steps, all in timing-sensitive device areas, and the third one is a Swift runner XCTest this branch cannot reach — it changes Rerun 3 is queued. If it lands red again on yet another step I'll stop burning the lane and we should treat the alert-observation XCTest's behaviour under load as its own issue rather than a signal about this diff. |
|
Reviewed at 3678353. The host app-log command now takes Two of the new tests do not check what their names say. In exec-kill-settle.test.ts#L262, exec-kill-settle.test.ts#L204 regex-matches CI is green on 3678353, and there are no conflicts. The next step is to fix or remove these two tests. |
3678353 to
f4b87f9
Compare
|
Both tests fixed at The "cannot be signalled" case now really is exercised. Proof it bites now: deleting the Before this change that same mutation was green, which is the hole you found. The source-shape test is gone. You are right about what it failed to check: |
|
Reviewed at f4b87f9. I found one small problem left in the reworked kill-settle test. exec-kill-settle.test.ts:192 uses a default All checks pass on f4b87f9, including Smoke Tests, which runs every device command through the exec.ts settlement this PR changes. There are no conflicts. The next step is the small test-only fix above. |
f4b87f9 to
ad1e409
Compare
|
Reviewed at ad1e409, as a follow-up to the review at f4b87f9. One small test problem is left. The vanished-group ESRCH loop test at exec-kill-settle.test.ts#L236 repeats the case that line 218 already covers, with a second hand-written Typecheck & Package fails with TS2353 on Smoke Tests was still running at review time. It exercises this change's route, because every device command spawns through Next: the test-double cleanup and a green Smoke Tests run. |
ad1e409 to
85b4f11
Compare
|
Reviewed again at The mode is The Mutation: making the catch in Gate at |
…ot 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.
…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>
…rs 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>
Co-authored-by: Apex by Callstack <noreply@callstack.com>
… 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>
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.
85b4f11 to
725a225
Compare
|
Reviewed at 85b4f11. This is the ad1e409 patch rebased onto main, with every commit unchanged in the range-diff, so the reply above answers the f4b87f9 review, which ad1e409 already addressed. The question from the review at ad1e409 is still open: the delivery test at exec-kill-settle.test.ts#L202 and the vanished-group test at L217 still use hand-written All checks passed at 85b4f11 and there are no conflicts. The newer head 725a225 is again the same patch rebased onto main, so this applies to it unchanged. Next: build both tests on |
|
Rebased on Gate re-run on the new head: |
`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.
|
Reviewed at One double now answers every group write in that file. The invalid-pid checks folded into the same shape, and got a stronger proof on the way. They used to assert The stale clause is gone, along with the Mutations, one at a time: rethrowing Scope of the unification, stated exactly: Gate at |
|
Reviewed at 0089fb1, as a follow-up to the earlier review. Both group-signal tests now use |
thymikee
added
the
ready-for-human
|
2 participants
Summary
A command that
exec.tskilled at its deadline or on request cancellation settled onclose, 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. Foreground promises and backgroundwaitnow settle onexitonce this module asked for the kill, and still wait for a full drain onclosewhen it did not. Onefinish()owns the timer clear, the abort-listener release and the trace emit, so the stdin-failure rejection stops bypassing them.killProcessTreeno 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.execHostAdbspawns detached likeexecSerialAdbalready does, so a deadline can signal the group instead of only the client. Background runs lose thetimeoutMsfield they never armed —ExecBackgroundOptionsandAndroidAdbSpawnOptionsomit it, the two app-log call sites that forwarded it are dropped, and the app-log adb command contract no longer offers it.Closes #2522. Eleven files, one command family. A caller-initiated direct
kill()still settles onclose; that path is outside this issue's scope. No output cap was added, sobuildRunnerEarlyExitErrorkeeps its full capture.Validation
Rebased on
origin/mainate3cbc91a1a; tested at0089fb123b. Every CI check on that head passed,including Smoke, Coverage and Typecheck. The two
scripts/gates thatcheck:affecteddoes not selectpass locally (ratchet; eager-closure at 614 assertions).
Locally,
pnpm check:affected --runfinished with one failing test in each of two runs, a different testeach time — a wall-clock budget in
runner-client.test.ts, a startup-threshold inscripts/fuzz/harness.test.ts— with 5855/5856 and 10,603/10,605 passing. Both are green in isolation(50 tests, 13 tests) and green in CI; the host was at load average 50+ from other sessions, which is the
condition
AGENTS.mdwarns about for subprocess-backed suites. Nothing in those two tests touches thisPR's files.
The wedge tests were run red against
origin/main'sexec.tsfirst (3073 ms, 3036 ms). Removing thegroup-kill branch, and removing the reaped-child guard, each turn one test red. The group-write double
answers the way
process.killdoes —trueon delivery,ESRCH,EPERM— and rethrowingEPERMfromthe seam's catch reddens the table test alone; replacing the invalid-pid refusal reddens the refusal
test through the guard's empty write list.
Honest gap: with a real Android adb on this host the client closed 1 ms after exit, so the adb-specific
pipe wedge is not reproduced here — the synthetic inherited-pipe holder is the evidence for the generic
defect.