test(terminal): stabilize Windows Ctrl-C cleanup assertion - #4290
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Overall verdict
No material findings. The bounded polling addresses the observed teardown race while preserving a failure when the child remains alive. The helper uses a monotonic deadline, performs a final state check at the deadline, and the existing finally cleanup remains intact. The exact-head Windows suite completed successfully, including test_windows_ctrl_c_interrupt_kills_child_process.
Risk assessment
Low risk. This is a Windows-only test change with no production/runtime behavior changes, no security-sensitive code, and no API or compatibility impact. The principal residual risk is ordinary timing sensitivity on unusually slow Windows hosts, bounded by both the five-second polling window and the existing test timeout.
This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
Locally verified the terminal Ctrl-C runtime path remains unchanged and reproduced the timing pattern this PR is trying to stabilize, but I could not manually exercise the native Windows-only test path from this Linux runner.
Does this PR achieve its stated goal?
Partially verified: the PR's goal is to stabilize a Windows-only Ctrl-C cleanup assertion without changing terminal runtime behavior. I confirmed the runtime Ctrl-C behavior is unchanged between origin/main and the PR using a real TerminalSession, and a representative short-lived process scenario shows why an immediate process-existence assertion can fail while a bounded wait succeeds. Native Windows validation could not be run from this host, but the PR's windows-tests CI check was green when checked.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run python created the project environment and imported the terminal tool API successfully. |
| CI Status | 🟡 windows-tests and most core checks were green; several agent-server build/publish checks plus this QA job were still in progress, with no failed checks observed. |
| Functional Verification | 🟡 Real terminal Ctrl-C behavior verified on Linux; native Windows Get-Process teardown race not locally executable. |
Functional Verification
Test 1: Terminal Ctrl-C runtime behavior is unchanged
Step 1 — Establish baseline on origin/main:
Ran git checkout --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_terminal_ctrl_c.py; git checkout agent/stabilize-windows-ctrl-c-test:
pid=4330
exists_before_ctrl_c=True
exists_after_wait=False
first_status=None
interrupt_status=None
This shows the base branch's real terminal backend can start a long-running foreground process, accept C-c input, and leave that process gone after a bounded wait.
Step 2 — Apply the PR's changes:
Checked back out agent/stabilize-windows-ctrl-c-test at 9c6e06444d78d8883527510977fa7d999c696c14.
Step 3 — Re-run with the PR in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_terminal_ctrl_c.py:
pid=4200
exists_before_ctrl_c=True
exists_after_wait=False
first_status=None
interrupt_status=None
This shows the PR did not regress the actual terminal Ctrl-C user path I could exercise locally; the long-running process still exists before interrupt and is gone after C-c.
Test 2: Representative process teardown timing race
Step 1 — Reproduce the old immediate-check failure mode:
Ran python /tmp/qa_process_wait_representative.py using a real short-lived sleep process:
pid=4901
immediate_process_exists=True
old_immediate_assertion_would_pass=False
new_bounded_wait_result=True
process_returncode=0
This confirms the race shape: a process can still be observable immediately after teardown/interrupt timing, so an immediate assert not exists style check can produce a false failure.
Step 2 — Apply the PR's changed behavior:
The PR replaces the immediate assertion with a bounded wait before failing the process-liveness check.
Step 3 — Re-run with bounded-wait behavior represented:
The same run above reports new_bounded_wait_result=True after polling up to five seconds.
This demonstrates the intended stabilization behavior: transient process visibility is tolerated, but the process still has to exit within the bounded window.
Unable to Verify
Native Windows execution was not available in this runner:
platform_system= Linux
pwsh= /usr/bin/pwsh
powershell.exe= None
Because the changed test is explicitly Windows-only and uses native Windows process semantics, I did not run the target pytest file or any test suite locally. Future QA would be stronger with AGENTS.md guidance for obtaining a native Windows QA runner or a documented non-pytest command that exercises the Windows terminal backend directly.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
Verdict: PARTIAL
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
👋 I'm an AI agent running on Claude Opus 5, reviewing on behalf of @enyst using the codereview-roasted skill.
Taste Rating: 🟡 Acceptable — the race fix is correct and obvious in hindsight, which is the best kind. Two things went along for the ride that deserve to be said out loud rather than absorbed in a "stabilize" commit.
[IMPROVEMENT OPPORTUNITIES]
-
[
tests/tools/terminal/test_windows_ctrl_c.py, L64] Scope Creep In A Rename:test_windows_ctrl_c_interrupt_kills_child_process_tree→..._kills_child_process. Dropping_treequietly shrinks what this suite promises. The old docstring was explicit that the backend "does not ensure child processes launched by the command are terminated", and that paragraph is now gone too. If the tree case is genuinely still unhandled, deleting the sentence that said so doesn't make it handled — it makes it undiscoverable. Either note in the PR body that tree termination remains uncovered, or leave a one-line comment where the old docstring was.(Worth noting the old file was self-contradictory: the docstring described a known gap while the assertion demanded the gap be closed. Your version is at least internally consistent.)
-
[
tests/tools/terminal/test_windows_ctrl_c.py, L54-61] Simplification: the trailingreturn not _powershell_process_exists(pid)re-runs the exact check the loop just ran, costing one morepowershell.exespawn on the failure path. The loop already answers the question:while time.monotonic() < deadline: if not _powershell_process_exists(pid): return True time.sleep(0.1) return False
-
[same helper] Pragmatism:
time.sleep(0.1)implies ~50 samples in 5s, but each sample spawns a freshpowershell.exe -NoLogo -NoProfile(L20-34), which on Windows costs meaningfully more than the sleep. Real cadence is process-spawn-bound, so you get maybe half that many samples and burn CPU doing it. Fine for a 20s-budget test — just don't tune the0.1expecting it to matter.
[TESTING GAPS]
None — this is the test. pytestmark correctly gates on platform.system() != "Windows", so this only proves anything on Windows CI; I can't exercise it from here and I'm not going to pretend otherwise.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — test-only, Windows-only, no production code touched. Worst case it stays flaky and you're no worse off than today.
VERDICT: ✅ Worth merging.
KEY INSIGHT: Asserting on an asynchronous kill the instant you request it was always going to be a coin flip; polling is the right fix. Just don't let the rename smuggle a reduced promise past the reviewer — waiting on a PID is also, strictly, a bet that Windows won't recycle that PID inside your 5s window.
|
Addressed in 0794240.
The hard failure for a real leaked child is preserved ( — created by an AI agent (OpenHands) on behalf of the user. |
…ershell spawns Use psutil.Process.wait/pid_exists for the child liveness wait rather than spawning a fresh powershell.exe on every poll, which itself could exhaust the old 20s watchdog on a slow hosted runner (second failure mode observed in PR #4280). Raise the pytest timeout to 45s to cover the bounded slow path plus finally cleanup. Note in the docstring that the assertion covers only the directly spawned child, not the whole process tree. Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Taste Rating: Good taste - this is a small, focused stabilization of a Windows-only regression test without touching terminal runtime behavior.
The change removes slow per-poll PowerShell subprocess checks and replaces them with psutil process waiting, while keeping a hard assertion that the directly spawned child exits after Ctrl-C. The narrowed test name/docstring now matches what the test actually proves, and the current PR-specific Run tests/windows-tests check is passing.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Test-only change, scoped to a Windows-specific terminal interrupt regression test. No production code, public API, dependency, or persisted data behavior is changed; the main risk is reduced flakiness coverage breadth, but the test still verifies the real child-process cleanup condition it exercises.
VERDICT:
✅ Worth merging: Core test logic is sound and the target Windows test workflow passed.
KEY INSIGHT:
The PR makes the assertion wait for the OS to observe process exit instead of sampling immediately through repeated PowerShell launches, which is the right simplification for this flake.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
4 participants
HUMAN:
CI records show that windows tests are passing.
AGENT:
Why
Release PR #4283 intermittently failed
test_windows_ctrl_c_interrupt_kills_child_process_treebecause the test queriedGet-Processimmediately after Windows process teardown. The unchanged test passed in the 15 most recent sampled Windows jobs, including PR #4284 and three recent main pushes, confirming a timing race rather than a deterministic terminal regression.Failing run: https://github.com/OpenHands/software-agent-sdk/actions/runs/30303472402/job/90109549429
Summary
psutil.Process.wait(replaces per-pollpowershell.exespawns, which themselves could blow the old 20s watchdog on a slow hosted runner)finallycleanupREST API contract changes
Compared with base OpenAPI
395b94b0c3c4for public/api/**paths.Issue Number
Related release PR: #4283
How to Test
Native Windows validation:
Local validation on Linux:
The test is intentionally Windows-only. Ruff format/lint, pycodestyle, Pyright, import rules, and tool registration checks all pass locally. The PR Windows workflow provides end-to-end validation on the target OS.
Video/Screenshots
Not applicable; this is a process-lifecycle test stabilization.
Type
Notes
This is intentionally test-only. It does not hide genuine child-process leaks: the assertion still fails after the bounded five-second wait. The assertion covers only the directly spawned child process — the PowerShell backend does not guarantee termination of an entire process tree, so a leaked grandchild would not fail this test.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:0794240-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0794240-python) is a multi-arch manifest supporting both amd64 and arm640794240-python-amd64) are also available if neededCloses #4309