fix(core): stop a surviving grandchild from hanging code execution

[ScottMansfield]

Link to Issue or Description of Change

• Bug: Flaky CI: app_loader and unsafe_local_code_executor time out intermittently on macOS/Windows (7 PRs affected) #622 (root cause; this is 1 of 2 — see the stack below)
• Related: LocalEnvironment.execute does not own the process tree: a timeout leaks forked descendants, and there is no way to cancel #782 (same defect class in LocalEnvironment, still open)

Problem:

UnsafeLocalCodeExecutor used spawn's own timeout option, which kills the interpreter and then leaves the promise waiting on 'close'. 'close' only fires once every stdio stream is closed, so an interpreter that forked rather than exec'd its work leaves a survivor holding those pipes — 'close' never arrives, the promise never settles, and the executor's own timeout branch never runs.

No timeout value bounds that wait. That is why raising the harness budget twice (#633 5s→…, #662 →30s) never stopped the flake: the wait is unbounded at 5s, at 30s, and at any other number. Windows is where it surfaces because powershell and python there are far likelier to leave a live descendant than bash/python3.

Reproduced locally with the executor's exact spawn shape:

grandchild holds pipes: { how: 'NEVER-CLOSED', ms: 7004 }   <- timeout was 3000ms
control, no grandchild: { how: 'close',  ms: 3004, signal: 'SIGKILL' }

Evidence from CI run 32415850940 (windows-latest, sha 540fa763) — all five failures are this executor, and three return empty output rather than slow output:

unsafe_local_code_executor_test › execute python code   Test timed out in 30000ms
unsafe_local_code_executor_test › execute shell code    Test timed out in 30000ms
run_skill_inline_script_tool_test › real Shell script   expected '' to contain 'hello from real sh'
run_skill_script_tool_test › real Python skill script   expected '' to contain 'hello from skill python'
run_skill_script_tool_test › failing Python script      expected '\nCode execution timed out after 30 s…'

Solution:

Run the timer here rather than delegating to spawn, and release the read ends along with the kill so the timeout is enforced rather than merely requested.

This is exactly the treatment LocalEnvironment.execute already applies, for the same reason, with a comment describing the same failure (core/src/environment/local_environment.ts:139-148). The executor simply never got it.

Also prefers an explicit timedOut flag over inferring the timeout from the close signal — Windows does not report a terminating signal the way POSIX does, so a killed child can close there with signal === null and silently skip the timeout message.

Alternative considered and rejected. Keeping spawn's timeout and destroying the pipes on 'exit' is a smaller diff and also works ({how:'close', ms:3006}), but 'exit' fires on every exit including healthy ones, and Node does not guarantee all 'data' events have been delivered by then. It survived 3×20 000 lines locally without truncating — but that is a timing assumption, which is precisely how you get a Windows-only flake. Destroying only on the timeout path is correct by construction.

The stack

	PR	What
1	this PR	fix the root cause
2	#NEXT	restore the test timeout the flake forced up

PR 2 is stacked on this branch and should merge after it. It is deliberately separate: this one is a behaviour fix worth reviewing on its own, and the revert is only safe once this lands.

Testing Plan

Unit Tests:

• I have added or updated unit tests for my change.
• All unit tests pass locally.

Added times out even when the script leaves a child holding the pipes open, mirroring the existing LocalEnvironment regression test. It reproduces the CI symptom exactly — on the unfixed executor it fails with Test timed out in 30000ms, the same string seen on windows-latest:

# without the fix
❯ unsafe_local_code_executor_test.ts (29 tests | 1 failed | 28 skipped) 30010ms
  → Test timed out in 30000ms.

# with the fix
✓ UnsafeLocalCodeExecutor > times out even when the script leaves a child holding the pipes open  509ms
  Tests  29 passed (29)

npm run test:unit — 241 files, 3643 tests passed. Also clean: build, ts:check, lint, format:check.

Manual End-to-End (E2E) Tests:

Not applicable — the behaviour change is entirely inside the spawn teardown and is covered by the regression test above.

Checklist

• I have read the CONTRIBUTING.md document.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have manually tested my changes end-to-end.
• Any dependent changes have been merged and published in downstream modules.

Additional context

This is the same defect class as #782, now confirmed in a second location. Worth considering a shared spawn helper that owns teardown once, rather than open-coding it a third time. #782 is still worth fixing, but it will not fix #622 — different file, no shared code, and its proposed detached + process-group remedy relies on POSIX process groups that Node does not implement on Windows.

[AmaadMartin]

Approve. The fix runs the timeout timer in the executor and destroys the read ends on the kill, so 'close' fires even when a forked grandchild holds the pipes. I verified this mirrors LocalEnvironment.execute (core/src/environment/local_environment.ts:136-148) field for field, and the new regression test reproduces the #622 symptom and would hang on the unfixed code. No suppressions, no instanceof, and no new public types; all run-tests jobs, including windows-latest, passed at review time.