Repository navigation
fix(workflow): settle a script run on its exit, not on pipe EOF - #895
Open
harshitwandhare wants to merge 1 commit into
Open
harshitwandhare wants to merge 1 commit into
harshitwandhare wants to merge 1 commit into
Conversation
A script that starts a background process and exits 0 left both pipes held open by that process. On Python 3.11+ process.wait() started before the exit waits for pipe EOF as well, so the run sat out the whole WORKFLOW_SCRIPT_TIMEOUT and was journaled FAILED, kind=timeout with its output dropped. On 3.10 wait() returned at the exit, but the drain after it had no bound, so the run did not settle until the helper exited. Wait for the exit with wait() raced against returncode, which asyncio sets at the exit on every supported version, then give the drain WORKFLOW_SCRIPT_TERM_GRACE and keep what was read, with a warning. A successful run now settles within WORKFLOW_SCRIPT_TIMEOUT + WORKFLOW_SCRIPT_TERM_GRACE, the same envelope as the timeout arm. Fixes awslabs#894
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The implementation has a timeout-boundary race and retains pipe resources after cancelling readers.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Fixes script workflows hanging when background processes retain stdout/stderr pipes.
Changes:
- Detects script exit independently of pipe EOF.
- Bounds post-exit pipe draining and preserves captured output.
- Adds unit, end-to-end, and changelog coverage.
| File | Description |
|---|---|
src/cli_agent_orchestrator/services/script_runner.py |
Adds exit polling and bounded pipe draining. |
test/services/test_script_runner.py |
Adds held-pipe regression tests. |
test/e2e/script_runner/test_script_runner_e2e.py |
Tests a real background helper. |
CHANGELOG.md |
Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+347
to
348
| await asyncio.wait_for(_wait_for_exit(process), timeout=timeout) | ||
| except asyncio.TimeoutError as e: |
Comment on lines
+1121
to
+1126
| await asyncio.wait_for(asyncio.gather(*drain), timeout=WORKFLOW_SCRIPT_TERM_GRACE) | ||
| except asyncio.TimeoutError: | ||
| drain_warnings.append( | ||
| f"stdout/stderr were still open {WORKFLOW_SCRIPT_TERM_GRACE}s after the script " | ||
| "exited, most likely held by a process it started; output read up to then was kept" | ||
| ) |
Comment on lines
+182
to
+198
| ``returncode`` is set from the start, as asyncio sets it when the process | ||
| exits. ``wait_holds_for_pipes`` picks the ``wait()`` behaviour: on Python | ||
| 3.10 it returns at exit, and from 3.11 a ``wait()`` started before the exit | ||
| does not return until the pipes reach EOF, which here is never. | ||
| """ | ||
|
|
||
| def __init__(self, *, stdout: bytes, wait_holds_for_pipes: bool): | ||
| self.returncode: Optional[int] = 0 | ||
| self.stdout = _HeldOpenStream(stdout) | ||
| self.stderr = _HeldOpenStream(b"") | ||
| self._wait_holds_for_pipes = wait_holds_for_pipes | ||
| self.signals: List[str] = [] | ||
|
|
||
| async def wait(self) -> int: | ||
| if self._wait_holds_for_pipes: | ||
| await asyncio.Event().wait() | ||
| return 0 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
=======================================
Coverage ? 92.69%
=======================================
Files ? 244
Lines ? 41354
Branches ? 0
=======================================
Hits ? 38332
Misses ? 3022
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #894
What was broken
A script workflow that starts a background process and exits 0 was not treated as finished at its exit. The background process inherits the script's stdout and stderr, so both pipes stay open after the script is gone, and
_drive_processwaited for them:process.wait()started before the exit also waits for pipe EOF, so the run sat out all ofWORKFLOW_SCRIPT_TIMEOUTand was journaledFAILED,kind=timeout, with itsCAO_WORKFLOW_OUTPUTdropped.wait()returns at the exit, but theasyncio.gather(*drain)after it had no bound, so the run did not settle until the helper exited.Measurements and the asyncio timings for all three versions are in #894.
The fix
_wait_for_exitracesprocess.wait()againstprocess.returncode, which asyncio sets at the exit on 3.10, 3.11 and 3.12 whether or not the pipes are open._await_exit_within_boundnow wraps it instead ofwait().WORKFLOW_SCRIPT_TERM_GRACE. If the pipes are still open after that, the readers are cancelled, the output already read is kept, and a warning says the pipes were still open.A successful run now settles within
WORKFLOW_SCRIPT_TIMEOUT + WORKFLOW_SCRIPT_TERM_GRACE, the same envelope as the timeout arm, so the invariant inconstants.pyholds again. A script that closes its pipes normally is unaffected: the drain finishes at once, as before.The background process itself is left running, the same as today, since
_terminatesignals the script's own process and not its group.Tests
test_exit_with_pipes_held_open_completes, parametrized over the twowait()behaviours (returns at exit, holds for the pipes), with a fake process whose streams never reach EOF.test_real_background_child_holding_stdout_does_not_hold_the_runin the e2e script-runner suite, with a real helper that outlives the script.All three fail on
mainand pass with the fix, on each CI Python:main+ tests onlyVerification
In WSL Ubuntu 24.04 on 3.12, with
uv sync --locked --all-extras --dev:ci.ymlunit command (pytest test/ examples/workflow/tests/ --ignore=test/providers/test_kiro_cli_integration.py --ignore=test/e2e -m "not e2e" --cov=...): 14184 passed / 1 failed onmainatfd5113ea, 14187 passed / 0 failed on this branch (the 14185 testsmainran plus the 2 new parametrized cases). The onemainfailure,test_fifo_reader.py::TestReaderThreadLifecycle::test_data_received_across_writer_reconnects, is a timing flake under load: it passed 5 of 5 run on its own onmainpytest test/e2e/script_runner -m e2e: 6 passedpytest test/test_http_only_boundary.py: 2 passedscripts/validate_markdown_links.pycleanscript_runner.pyare all coveredtrivy was not installed in my environment, for both runs.
CHANGELOG entry added under Unreleased / Fixed.