Repository navigation
fix: kill timed-out hook/shell process trees (Windows hang) + flaky and Windows test fixes - #30
Merged
Merged
Conversation
The screen-test harness snapshotted after a fixed 0.15s, which is racy on a slow CI runner: one job snapshotted before the first frame was drawn and indexed into an empty screen. Wait at least that long, then until the output has stopped changing, and fail with a clear message if nothing renders.
The hooks tests used POSIX shell syntax (echo x >&2; exit 2, sleep 5). In cmd.exe there is no ; separator or sleep, so the hook never exited with 2 and two tests failed on every Windows job. Run Python for the exit code, stderr output and sleep instead, and assert the timeout test really stops the hook.
subprocess.run(shell=True, timeout=...) only kills the shell. On Windows the command's own process kept running and held the output pipes open, so run() blocked until it finished anyway: a hook with a 1 second timeout took 30 seconds, and the shell tool's 120 second limit had the same flaw. Add utils.proc.run_shell, which starts the command in its own process group or session and kills the whole tree on timeout or Ctrl+C, and use it for hooks and the shell tool.
Amanbig
changed the title
An unrelated DevOrch session run in this directory replaced the command imports with 'from cli.commands import ask, edit, run', which imports modules instead of the command functions, and that edit was swept into the previous commit. Restore the original imports.
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.
Why
mainhas CI problems that were hidden by the matrix's fail-fast (the first failing job cancels the rest, so the Windows jobs were usually cancelled and never reported).1. Flaky screen tests
tests/test_boxed_input.py: the virtual-terminal harness snapshotted after a fixed 0.15 s. On a slow runner one Ubuntu job snapshotted before the first frame was drawn and hitIndexError. This is what failed the docs-only PR #29. Fix: wait at least that long, then until the output stops changing, with a clear error if nothing renders.2. Windows hooks tests
tests/test_hooks.py(from #5) used POSIX syntax (echo nope >&2; exit 2,sleep 5). cmd.exe has no;separator orsleep, so two tests failed on every Windows job. Fix: run Python for the exit code, stderr and sleep.3. A real bug the stricter timeout test exposed (Windows hang)
subprocess.run(cmd, shell=True, timeout=...)only kills the shell. On Windows the command's own process keeps running and keeps the output pipes open, sorun()blocks until it exits anyway. Observed in CI: a hook withtimeout=1ran for 30 s. A stuck hook would freeze DevOrch, andtools/shell.py(timeout=120) has the identical flaw.Fix: new
utils/proc.run_shellstarts the command in its own process group/session and kills the whole tree on timeout or Ctrl+C (taskkill /F /Ton Windows,killpgelsewhere), then reaps. It keepssubprocess.run's contract (raisesTimeoutExpired, stdin inherited unlessinputis given). Used by hooks and the shell tool.Tests
tests/test_proc.py: output/exit code, stdin, cwd, UTF-8, prompt return on timeout, a grandchild is killed on timeout, Ctrl+C kills the child and propagates, shell tool timeout and formattingsubprocess.runbehaviour back in makes the grandchild test failNot verified locally
I only run Linux. The Windows result of this PR's jobs is the real verification; the previous push already isolated the single remaining Windows failure to the timeout test, which this change targets.
Merge order
Merge this first; #28 and #29 then just need "Update branch".