fix: guard terminateStaleProcessesSync against non-ESRCH kill errors by krissding · Pull Request #109702 · openclaw/openclaw

krissding · 2026-07-17T06:06:20Z

What Problem This Solves

terminateStaleProcessesSync() in src/infra/restart-stale-pids.ts has two catch blocks after process.kill() that silently swallow ALL errors, not just ESRCH. If a non-ESRCH error occurs (e.g. EPERM), the process is still alive but the code proceeds as if it were killed — potentially leaving a stale process on the port while waiting for the port to free.

Same pattern as #109590 (gateway signal guard) and #109692 (port force-free guard).

Evidence

Tests

src/infra/restart-stale-pids.test.ts — 54 tests pass, including 1 new test:
- "swallows ESRCH but re-throws non-ESRCH errors from terminateStaleProcessesSync" — verifies EPERM causes cleanStaleGatewayProcessesSync to return [] (graceful failure via outer catch)
Existing "still polls for port-free when all stale pids were already dead at SIGTERM time" test confirms ESRCH path still works

CI

 Test Files  1 passed (1)
      Tests  54 passed (54)

Merge Risk

Low. +4 lines across two catch blocks inside a private function. Follows the established ESRCH-guard pattern already accepted in #109590.

The two catch blocks silently swallowed all errors, not just ESRCH. Non-ESRCH errors (e.g. EPERM) indicate the process is still alive but cannot be signaled — continuing as if it were killed risks leaving a stale process on the port. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

clawsweeper · 2026-07-17T06:15:27Z

Codex review: needs real behavior proof before merge. Reviewed July 17, 2026, 2:11 AM ET / 06:11 UTC.

Summary
The PR changes Unix stale-gateway termination to ignore only ESRCH from SIGTERM, liveness probes, and SIGKILL, with a regression test for EPERM.

PR surface: Source +6, Tests +14. Total +20 across 2 files.

Reproducibility: yes. at source level: with multiple stale PIDs, make an earlier SIGTERM succeed and a later process.kill throw EPERM; the new throw reaches the outer catch before escalation or waitForPortFreeSync. This was not executed as a live current-head reproduction.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Preserve processing and port polling when one PID returns a non-ESRCH error, with mixed-success regression coverage.
[P1] Add redacted terminal proof from a real process setup showing the revised path does not falsely complete cleanup or skip port handling.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body provides mocked unit-test results and CI output, not after-fix proof from a real process/restart setup; add a redacted terminal transcript or recording, then update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

[P1] Merging as written can make gateway restart proceed without waiting for the occupied port to clear when process.kill returns EPERM or another non-ESRCH error, recreating the EADDRINUSE restart-loop failure this module is designed to prevent.
[P1] The PR supplies mocked unit-test output but no after-fix terminal or live restart evidence from a real process setup.

Maintainer options:

Preserve cleanup completion (recommended)
Revise both kill loops so one non-ESRCH failure cannot skip later PIDs, escalation, or the final port-release poll, and add mixed-PID regression coverage.
Pause until runtime proof
Keep the PR unmerged until a redacted terminal run demonstrates the revised path with a real permission-style signal failure and confirms restart cleanup does not return before port handling completes.

Next step before merge

[P1] Contributor revision and real-environment proof are required; ClawSweeper should not open an automated repair lane while the external proof gate remains unmet.

Security
Cleared: The diff changes local process-error handling and tests only; it introduces no concrete credential, authorization, dependency, workflow-permission, or supply-chain concern.

Review findings

[P1] Do not abort before the port-release poll — src/infra/restart-stale-pids.ts:502

Review details

Best possible solution:

Handle errors per PID: ignore ESRCH, retain observable diagnostics for other errors, continue safe cleanup of the remaining PIDs, and guarantee waitForPortFreeSync still executes; cover mixed-success multi-PID cases and demonstrate the result with redacted terminal proof.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: with multiple stale PIDs, make an earlier SIGTERM succeed and a later process.kill throw EPERM; the new throw reaches the outer catch before escalation or waitForPortFreeSync. This was not executed as a live current-head reproduction.

Is this the best way to solve the issue?

No. ESRCH-specific classification is appropriate, but throwing from the private PID loop is not the narrowest safe fix because the outer best-effort wrapper suppresses the error and skips required cleanup completion.

Full review comments:

[P1] Do not abort before the port-release poll — src/infra/restart-stale-pids.ts:502
Rethrowing here does not surface the error: cleanStaleGatewayProcessesSync catches it and returns [], which skips later PIDs, any SIGKILL escalation for PIDs already signaled, and the unconditional waitForPortFreeSync call. An EPERM on one listener can therefore make restart continue while the port remains bound, recreating the EADDRINUSE loop this module prevents. Handle the error per PID while preserving the final poll, and add a mixed multi-PID regression test; the same applies to the new SIGKILL throw.
Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c80b236a09d6.

Label changes

Label changes:

add P2: This is a bounded gateway-restart reliability fix with limited scope, but no demonstrated widespread current outage.
add merge-risk: 🚨 availability: The new rethrows can bypass the established port-release wait and allow a restarted gateway to collide with a still-bound port.
add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides mocked unit-test results and CI output, not after-fix proof from a real process/restart setup; add a redacted terminal transcript or recording, then update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.

Label justifications:

P2: This is a bounded gateway-restart reliability fix with limited scope, but no demonstrated widespread current outage.
merge-risk: 🚨 availability: The new rethrows can bypass the established port-release wait and allow a restarted gateway to collide with a still-bound port.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides mocked unit-test results and CI output, not after-fix proof from a real process/restart setup; add a redacted terminal transcript or recording, then update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +6, Tests +14. Total +20 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	8	2	+6
Tests	1	14	0	+14
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	22	2	+20

What I checked:

Current-main behavior: The current Unix termination loops catch every process.kill error, so the reported non-ESRCH misclassification is still present on main. (src/infra/restart-stale-pids.ts:500, c80b236a09d6)
Cleanup boundary: cleanStaleGatewayProcessesSync calls waitForPortFreeSync only after terminateStaleProcessesSync returns; its outer catch instead returns an empty result, so a rethrow bypasses the port-release confirmation entirely. (src/infra/restart-stale-pids.ts, c80b236a09d6)
Introduced regression: The new throw aborts the whole PID loop on the first non-ESRCH error, including after earlier PIDs may already have received SIGTERM; the identical issue exists in the new SIGKILL catch. (src/infra/restart-stale-pids.ts:502, ceb4c2f998ea)
Coverage gap: The added test uses one PID and asserts the outer empty result, so it does not verify continued cleanup of other PIDs or the mandatory port-release polling invariant. (src/infra/restart-stale-pids.test.ts:836, ceb4c2f998ea)
Feature provenance: Commit 1be39d4 introduced the port-release polling and lifecycle regression coverage specifically to prevent EADDRINUSE restart loops; bypassing that poll conflicts with the established invariant. (src/infra/restart-stale-pids.ts, 1be39d4250)
Related implementation pattern: The merged fix(gateway): guard process.kill ESRCH race in signalVerifiedGatewayPidSync #109590 established ESRCH-specific handling in another signaling path, while fix: guard forceFreePort and killPids against ESRCH race #109692 covers separate CLI port-kill functions; neither supersedes this cleanup path. (src/infra/restart-stale-pids.ts, 853b1a8d112c)

Likely related people:

Takhoffman: Commit 1be39d4 introduced the central stale-process termination, port-release polling, and restart-loop regression invariants affected by this PR. (role: introduced behavior; confidence: high; commits: 1be39d4250; files: src/infra/restart-stale-pids.ts, src/infra/restart-stale-pids.test.ts)
HCL: The same lifecycle commit credits HCL for the stale-PID audit, branch coverage, and follow-up fixes around terminateStaleProcessesSync. (role: feature-history contributor; confidence: medium; commits: 1be39d4250; files: src/infra/restart-stale-pids.ts, src/infra/restart-stale-pids.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Co-authored-by: 丁宇婷0668001435 <[email protected]>

steipete · 2026-07-17T09:06:42Z

Merged via squash.

Prepared head SHA: 98f3a3f915c97444a82093280f1453f6fdb00bbc
Landed commit: 4612b1692c15086ab4816047a257dc04fd3fbc18

…penclaw#109702) * fix: guard terminateStaleProcessesSync against non-ESRCH kill errors The two catch blocks silently swallowed all errors, not just ESRCH. Non-ESRCH errors (e.g. EPERM) indicate the process is still alive but cannot be signaled — continuing as if it were killed risks leaving a stale process on the port. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix: continue stale PID cleanup after signal errors Co-authored-by: 丁宇婷0668001435 <[email protected]> --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

openclaw-barnacle Bot added the size: XS label Jul 17, 2026

fix: continue stale PID cleanup after signal errors

98f3a3f

Co-authored-by: 丁宇婷0668001435 <[email protected]>

openclaw-barnacle Bot added size: S and removed size: XS labels Jul 17, 2026

steipete merged commit 4612b16 into openclaw:main Jul 17, 2026
119 of 124 checks passed

github-actions Bot mentioned this pull request Jul 17, 2026

📡 Upstream Digest — 2026-07-17 09:48 UTC curtismercier/openclaw-mods#1321

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: guard terminateStaleProcessesSync against non-ESRCH kill errors#109702

fix: guard terminateStaleProcessesSync against non-ESRCH kill errors#109702
steipete merged 2 commits into
openclaw:mainfrom
krissding:fix/esrch-stale-pid-kill

krissding commented Jul 17, 2026

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Uh oh!

steipete commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

krissding commented Jul 17, 2026

What Problem This Solves

Evidence

Tests

CI

Merge Risk

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Uh oh!

steipete commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants