Fix Windows compatibility: /dev/tty fallback and process tree termination - #54933
Closed
PowerSynth-max wants to merge 1 commit into
Closed
Fix Windows compatibility: /dev/tty fallback and process tree termination#54933PowerSynth-max wants to merge 1 commit into
PowerSynth-max wants to merge 1 commit into
Conversation
问题描述:
在 Windows 平台上发现两个兼容性问题,影响 Hermes 的稳定性和资源管理。
修复内容:
1. cli.py: 修复 Windows 上 /dev/tty 不存在的问题
- 问题:_reset_terminal_input_modes() 函数在 stdout 被重定向时会尝试打开
/dev/tty 来重置终端输入模式,但 Windows 上没有 /dev/tty 设备,导致
FileNotFoundError
- 修复:在 Windows 上使用 CON 设备(Windows 控制台设备)替代 /dev/tty
- 影响:修复 TUI 退出时的终端状态重置,避免终端状态异常
2. tools/environments/local.py: 修复 Windows 上进程终止不彻底的问题
- 问题:_kill_process() 在 Windows 分支中只调用 proc.terminate(),这只会
终止父进程,子进程会变成孤儿进程继续运行,导致资源泄漏
- 修复:使用 taskkill /T /F /PID 命令终止整个进程树(包括所有子进程),
与 POSIX 分支的 os.killpg() 行为对齐
- 影响:确保终止进程时不会留下孤儿进程,避免资源泄漏
测试验证:
- 测试1:确认 /dev/tty 在 Windows 上报 FileNotFoundError,使用 CON 后正常
- 测试2:确认 proc.terminate() 会留下孤儿进程,taskkill /T 能终止整个进程树
技术细节:
- cli.py 第 1144 行:添加平台判断,Windows 使用 "CON",其他平台使用 "/dev/tty"
- local.py 第 660-661 行:Windows 分支改用 subprocess.run 调用 taskkill /T,
失败时 fallback 到 proc.terminate(),并等待进程退出
相关文件:
- cli.py
- tools/environments/local.py
注:部分代码由 AI 辅助生成,经人工审核和测试验证。
Collaborator
Duplicate of #49460 (and competing with #54235): the Note: this PR ALSO bundles a distinct, independently-useful fix in |
Author
|
Hi @alt-glitch,
Thanks for the quick review and for identifying the overlap!
You're absolutely right — the `local.py` change is indeed a duplicate of #49460 and #54235. I wasn't aware those PRs existed when I submitted this.
However, as you noted, the `cli.py` fix (using `CON` instead of `/dev/tty` on Windows) is a distinct issue that those other PRs don't address. This was discovered during actual testing on Windows 11.
**Questions for maintainers:**
1. Should I close this PR and submit a new one with only the `cli.py` changes?
2. Or would you prefer to keep this PR open and cherry-pick the `cli.py` hunk separately?
3. Or should I just remove the `local.py` changes from this PR and keep the `cli.py` fix here?
Happy to follow whatever workflow makes the most sense.
Thanks again for the review!
…---
**Note**: This reply was sent via email from the PR author.
|
13 tasks
13 tasks
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.
What does this PR do?
Fix two Windows-specific compatibility issues that affect Hermes stability and resource management on Windows platforms.
Related Issue
No existing issue found. This PR addresses real-world Windows compatibility problems discovered during local development and testing.
Type of Change
[x] 🐛 Bug fix (non-breaking change that fixes an issue)
Changes Made
cli.py (line ~1144): Added platform check in _reset_terminal_input_modes() to use CON device on Windows instead of /dev/tty
tools/environments/local.py (line ~660): Changed Windows branch in _kill_process() to use taskkill /T /F /PID for terminating the entire process tree, with fallback to proc.terminate()
How to Test
Test 1 (cli.py): Redirect stdout and verify no FileNotFoundError on Windows when exiting TUI
Test 2 (local.py): Create a process with child processes, call _kill_process(), verify all child processes are terminated (no orphan processes remain)
Test 3: Run existing test suite on Windows to ensure no regressions
Checklist
Code
[x] I've read the Contributing Guide
[x] My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
[x] I searched for existing PRs to make sure this isn't a duplicate
[x] My PR contains only changes related to this fix/feature (no unrelated commits)
[ ] I've run pytest tests/ -q and all tests pass (Note: Full test suite not run due to environment limitations)
[ ] I've added tests for my changes (Note: Manual testing performed, automated tests not added)
[x] I've tested on my platform: Windows 11
Documentation & Housekeeping
[x] I've updated relevant documentation — or N/A
[x] I've updated cli-config.yaml.example if I added/changed config keys — or N/A
[x] I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
[x] I've considered cross-platform impact (Windows, macOS) per the compatibility guide
[x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
Note: Some code was AI-assisted, reviewed and tested by human.