Skip to content

fix(background-terminals): retain live children after signal errors - #592

Open
ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/background-operational-errors
Open

ooiuuii wants to merge 1 commit into
openpi-dev:mainfrom
ooiuuii:fix/background-operational-errors

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

fix(background-terminals): retain live children after signal errors

Fixes #591.

Problem

An error event does not always mean a child failed to start or has exited. A failed signal can emit it after spawning. The background manager previously classified that as terminal failure, flushed/closed full-log streams and published completion while the actual process was still alive. Its one-shot error listener also disappeared after that first event.

Value

Keep background-terminal status and completion evidence aligned with the real child lifecycle, including output produced after a failed signal. Avoid leaving repeated operational errors unobserved.

Approach

  • Keep the child error listener installed for repeated operational failures.
  • Preserve the existing bounded error diagnostic, but do not mark an entry exited or settle it when the child has a PID.
  • Keep the existing true spawn-failure path when no PID was assigned. Leave real exit/close responsible for the final exit code and output flush.
  • Add real manager/child regression coverage for repeated failed signals followed by either exit 0 or exit 7, one completion event, and complete post-error spill output.

Validation

  • RED on unchanged production: both new tests fail because the error listener disappears while the real child is still alive.
  • GREEN: 18 focused manager tests, including spawn failure, normal/nonzero exit and synthetic Windows signal-helper contracts.
  • GREEN: 44 output, result-delivery, tool-result and watch tests.
  • GREEN: bun run check (contracts, web build/typecheck, formatting, lint, repository typecheck).
  • Scoped structured code review completed with no actionable findings.
  • Real-child proof changed from alive=true, status=failed, error listeners=0 to alive=true, status=running, error listeners=1.

The signal failure is deliberately injected at Node's native kill binding. The test invokes the real ChildProcess.kill method and verifies the actual child is still alive; it is not a naturally occurring OS-denial E2E.

Full-suite limitation: an initial broader test filter also selected the existing Windows descendant-tree taskkill test, which failed its result.killed assertion on this host. Its owned processes were cleaned up, and the final focused run excludes that real-tree test. This PR does not alter taskkill timeouts/fallback policy or claim the entire suite passed.

Impact

  • User/model-visible: an operational signal error alone no longer causes a premature completion report; its diagnostic remains visible.
  • Runtime/lifecycle: retain observation and output capture until real exit/close. True spawn failures are unchanged.
  • Persisted config/data, tool schemas and dependencies: none.
  • Risk boundary: no new retries, signal authority, process-tree escalation policy, or shared-process refactor.

@github-actions github-actions Bot added the area:background-terminals Background terminal runtime, skill, or tests label Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:background-terminals Background terminal runtime, skill, or tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(background-terminals): signal errors can settle a still-running child

1 participant