Skip to content

[build-tools] Ignore already-exited processes when killing timed-out phases - #4561

Open
ahmdshrif wants to merge 2 commits into
expo:mainfrom
ahmdshrif:fix-kill-esrch-timeout
Open

ahmdshrif wants to merge 2 commits into
expo:mainfrom
ahmdshrif:fix-kill-esrch-timeout

Conversation

@ahmdshrif

Copy link
Copy Markdown

Why

Fixes #4479.

When expo doctor hits its 30s timeout, runExpoDoctor in packages/build-tools/src/common/setup.ts collects the process tree with getParentAndDescendantProcessPidsAsync and then calls process.kill(pid) on each pid inside an async setTimeout callback. The tree is a snapshot: a pid can exit between pgrep and the kill (a child finishing on its own, or exiting because its parent was just killed). process.kill then throws ESRCH, the async callback rejects, nothing handles it, and Node terminates the whole build with the stack from the issue:

Error: kill ESRCH
    at process.kill (node:internal/process/per_thread:225:13)
    at .../@expo/build-tools/dist/common/setup.js:168:25
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (.../@expo/build-tools/dist/common/setup.js:167:18)

So a timeout that is supposed to be skipped with a warning ("expo doctor" timed out, skipping...) fails the build instead. The same loop is copy-pasted in the install-dependencies kill timeout (setup.ts) and the install-pods kill timeout (builders/ios.ts), with the same race.

How

Add killProcesses(pids, signal = 'SIGTERM') to utils/processes.ts, next to getParentAndDescendantProcessPidsAsync. It kills each pid and skips ESRCH (the process is already gone, which is the goal), rethrowing anything else. This matches the existing killProcessGroup helper in the same file, which already treats ESRCH as "nothing left to stop". The three pids.forEach(pid => process.kill(pid)) loops now call it. A side effect is that one missing pid no longer stops the remaining pids from being killed.

No change to the timeouts, the signal, or the error reporting.

Test Plan

  • Extended src/utils/__tests__/processes.test.ts with killProcesses cases: kills every pid with the given signal; an ESRCH on one pid is ignored and the next pid is still killed; a real process that already exited does not throw; other errors (EPERM) are rethrown.
  • jest --config jest/unit-config.ts src/utils/__tests__/processes.test.ts: 11 tests pass.
  • jest --config jest/unit-config.ts src/utils src/common src/builders: 341 tests pass. The 3 suites that fail locally (custom, hookableBuildPhase, jobHooks) fail the same way on main in my checkout (a local @types/node / TypeScript mismatch in androidLocalEgress.ts), unrelated to this change.
  • Standalone Node 22 script reproducing the timer + kill loop against an exited pid: the old loop crashes with the exact Error: kill ESRCH ... at Array.forEach ... at Timeout._onTimeout stack from the issue (exit code 1); with killProcesses the callback finishes and the process exits 0.
  • oxfmt --check and oxlint clean on the changed files. tsc reports no errors in the changed files.

@douglowder

Copy link
Copy Markdown
Contributor

eas-cli now uses changesets for changelogs (#4580). The required "Check for a changeset" check fails on PRs that edit CHANGELOG.md.

To update this PR:

  1. Revert the changes to CHANGELOG.md.
  2. Run yarn changeset, select the changed packages and the bump type, and write the summary.
  3. Commit the new file in .changeset/.

The release adds the PR link and the author to the entry. If this PR needs no changelog entry, revert the CHANGELOG.md changes and add the "no changelog" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

expo doctor timeout crashes the build instead of skipping it

2 participants