fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races#17118
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the TUI gateway’s stdio transport and shutdown path to avoid deadlocks/zombie processes when the parent TUI half-closes pipes and termination signals race with concurrent stdout writes.
Changes:
- Make stdio JSON frame emission more robust by reducing lock hold time and treating more write/flush failures as a clean “peer is gone” (
False) result. - Add an env knob to skip explicit
flush()inStdioTransportto mitigate environments whereflush()can block indefinitely. - Add a SIGTERM/SIGHUP shutdown grace timer that falls back to
os._exit(0)if normalsys.exit(0)hangs during interpreter shutdown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tui_gateway/transport.py | Refactors StdioTransport.write() error handling and introduces HERMES_TUI_GATEWAY_NO_FLUSH. |
| tui_gateway/entry.py | Adds a 1s hard-exit safety net around sys.exit(0) in the signal handler. |
| tests/tui_gateway/test_protocol.py | Adds regression tests for closed-stream/flush OSError handling and the no-flush env knob. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9dc2398 to
1c498db
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the TUI gateway’s stdio JSON-RPC transport and shutdown behavior to avoid deadlocks/zombie processes when pipes are half-closed and SIGTERM lands during concurrent writes.
Changes:
- Moves JSON serialization outside the stdout lock and treats write/flush I/O failures as
False(peer gone) instead of exceptions. - Adds
HERMES_TUI_GATEWAY_NO_FLUSH=1to skipflush()in environments where flush can hang on half-closed pipes. - Adds a SIGTERM/SIGHUP shutdown watchdog: start a 1s daemon timer that falls back to
os._exit(0)ifsys.exit(0)shutdown gets stuck.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tui_gateway/transport.py | Refactors stdio write path: serialize outside lock, split write vs flush exception handling, add no-flush env knob, and add logging. |
| tui_gateway/entry.py | Adds shutdown grace timer + hard-exit fallback in the signal handler to prevent wedged shutdowns. |
| tests/tui_gateway/test_protocol.py | Adds regression tests for closed stream handling, flush OSError handling, and the no-flush env knob behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the TUI gateway’s stdio transport and signal shutdown path to avoid deadlocks/zombie processes when stdout is half-closed or a flush wedges during SIGTERM/SIGHUP handling.
Changes:
StdioTransport.write()now serializes JSON outside the write lock, and treats write/flush I/O failures asFalsereturns (plus adds an opt-out flush env knob).- Signal handler
_log_signaladds a configurable grace window, then force-exits via a daemon timer to avoid shutdown hangs. - Adds protocol tests covering closed-stream and flush-OSError cases, plus the no-flush behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tui_gateway/transport.py | Moves serialization outside lock; handles write/flush exceptions; adds HERMES_TUI_GATEWAY_NO_FLUSH behavior knob. |
| tui_gateway/entry.py | Adds configurable shutdown grace and a timer-based os._exit(0) fallback after SIGTERM/SIGHUP. |
| tests/tui_gateway/test_protocol.py | Adds regression tests for closed stdout, flush failures, and skipping flush when the knob is enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…SIGTERM races
`tui_gateway` reports `tui_gateway_crash.log` traces where the main
thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid-
flush, and SIGTERM then calls `sys.exit(0)` while the lock is still
held — the interpreter shutdown stalls behind the wedged write.
Two narrowly scoped hardenings:
**`tui_gateway/transport.py`**
* Move JSON serialisation outside the lock — long messages no longer
block sibling writers while we serialise.
* Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and
generic `OSError` from both `write` and `flush` as "peer is gone":
return `False` instead of bubbling, matching what `write_json`'s
callers in `entry.py` already expect.
* Split `flush` into its own try block so a stuck flush never strands
a partial write or holds the lock indefinitely on its way out.
* Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit
`flush()` entirely on environments where a half-closed read pipe
produces an indefinite kernel-level block. Default unchanged.
**`tui_gateway/entry.py`**
* `_log_signal` now spawns a 1-second daemon timer that calls
`os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck
behind a wedged worker. Atexit handlers run inside the grace
window when they can; the timer is the safety net so a deadlocked
flush no longer strands the gateway process.
Tests:
* `test_write_json_closed_stream_returns_false` — ValueError path.
* `test_write_json_oserror_on_flush_returns_false` — OSError on flush
must not strand the lock; the write portion still landed before the
flush failure.
* `test_write_json_no_flush_env_skips_flush` — env knob bypass.
Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py`
(42/42 pass; one pre-existing failure on
`test_session_resume_returns_hydrated_messages` is unrelated to this
change — same `include_ancestors` mock kwarg issue tracked elsewhere).
`scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass.
…EncodeError Copilot review on PR #17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests.
f508dca to
14e7af2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the stdio-based tui_gateway process against deadlocks and shutdown races observed under high concurrency, focusing on safer stdout writes/flushes and ensuring SIGTERM can’t strand a zombie gateway.
Changes:
- Refactors
StdioTransport.write()to serialize outside the stdout lock, and to treat common stream/flush failures asFalsereturns. - Adds
HERMES_TUI_GATEWAY_NO_FLUSHknob to skipflush()in environments where flush can hang on half-closed pipes. - Updates SIGTERM/SIGHUP handling to enforce a configurable shutdown grace period, then force-exit via
os._exit(0)if orderly shutdown stalls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tui_gateway/transport.py | Moves JSON serialization outside the lock; adds robust handling for write/flush failures and a no-flush configuration knob. |
| tui_gateway/entry.py | Adds a configurable shutdown grace window and a hard-exit fallback timer in termination signal handling. |
| tests/tui_gateway/test_protocol.py | Adds regression tests for closed stream/flush errors, encoding errors, and the no-flush behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…amming errors Round 2 Copilot review on PR #17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test.
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the TUI gateway’s stdio transport and termination behavior to avoid deadlocks/zombie processes when pipes become half-closed and SIGTERM arrives during a blocked flush, improving reliability under heavy concurrency.
Changes:
- Refactors
StdioTransport.write()to serialize JSON outside the stdout lock and to treat write/flush I/O failures as a clean “peer gone” signal. - Adds an opt-in
HERMES_TUI_GATEWAY_NO_FLUSHknob to skipflush()for environments where flush can hang. - Adds a configurable shutdown grace window and a daemon timer fallback to
os._exit(0)in the SIGTERM/SIGHUP handler to avoid shutdown hangs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tui_gateway/transport.py | Moves JSON serialization outside the lock, adds flush-disable knob, and hardens write/flush error handling. |
| tui_gateway/entry.py | Adds a shutdown grace config and watchdog timer to ensure SIGTERM/SIGHUP cannot strand the process. |
| tests/tui_gateway/test_protocol.py | Adds tests for closed-stream, encoding/value errors, flush OSError handling, and flush-disable behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Round 3 Copilot review on PR #17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the TUI gateway’s stdio transport and shutdown path to avoid deadlocks/zombie processes when stdout/stderr pipes are half-closed and SIGTERM/SIGHUP lands while worker threads are mid-flush.
Changes:
- Refactors
StdioTransport.write()to serialize JSON outside the stdout lock and to returnFalseonly for verified “peer gone” I/O failures (with errno filtering), while re-raising programming/configuration errors. - Adds an opt-in
HERMES_TUI_GATEWAY_NO_FLUSH=1mode to skip explicit flushes in environments where flush can hang (with clear unbuffered-stdout requirement). - Updates signal handling to enforce a configurable shutdown grace window, then force-exit via
os._exit(0)as a safety net.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tui_gateway/transport.py | Narrows peer-gone handling, moves JSON serialization out of the lock, and adds the no-flush env knob. |
| tui_gateway/entry.py | Adds configurable shutdown grace and a daemon timer fallback to os._exit(0) on stuck shutdown. |
| tests/tui_gateway/test_protocol.py | Adds regression tests covering closed-stream, encoding/value error re-raise behavior, errno-based peer-gone handling, and no-flush wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ulasbilgen
pushed a commit
to ulasbilgen/hermes-adhd-agent
that referenced
this pull request
May 1, 2026
…SIGTERM races (NousResearch#17118) * fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races `tui_gateway` reports `tui_gateway_crash.log` traces where the main thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid- flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held — the interpreter shutdown stalls behind the wedged write. Two narrowly scoped hardenings: **`tui_gateway/transport.py`** * Move JSON serialisation outside the lock — long messages no longer block sibling writers while we serialise. * Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and generic `OSError` from both `write` and `flush` as "peer is gone": return `False` instead of bubbling, matching what `write_json`'s callers in `entry.py` already expect. * Split `flush` into its own try block so a stuck flush never strands a partial write or holds the lock indefinitely on its way out. * Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit `flush()` entirely on environments where a half-closed read pipe produces an indefinite kernel-level block. Default unchanged. **`tui_gateway/entry.py`** * `_log_signal` now spawns a 1-second daemon timer that calls `os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck behind a wedged worker. Atexit handlers run inside the grace window when they can; the timer is the safety net so a deadlocked flush no longer strands the gateway process. Tests: * `test_write_json_closed_stream_returns_false` — ValueError path. * `test_write_json_oserror_on_flush_returns_false` — OSError on flush must not strand the lock; the write portion still landed before the flush failure. * `test_write_json_no_flush_env_skips_flush` — env knob bypass. Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py` (42/42 pass; one pre-existing failure on `test_session_resume_returns_hydrated_messages` is unrelated to this change — same `include_ancestors` mock kwarg issue tracked elsewhere). `scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass. * review(copilot): tighten transport hardening comments + test cleanup * review(copilot): narrow exception capture, configurable grace, simpler no-flush test * fix(tui-gateway): narrow ValueError to closed-stream; surface UnicodeEncodeError Copilot review on PR NousResearch#17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests. * fix(tui-gateway): reserve write() False for peer-gone; re-raise programming errors Round 2 Copilot review on PR NousResearch#17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test. * fix(tui-gateway): narrow OSError to peer-gone errnos; honest test naming Round 3 Copilot review on PR NousResearch#17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
02356abc
pushed a commit
to 02356abc/hermes-agent
that referenced
this pull request
May 14, 2026
…SIGTERM races (NousResearch#17118) * fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races `tui_gateway` reports `tui_gateway_crash.log` traces where the main thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid- flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held — the interpreter shutdown stalls behind the wedged write. Two narrowly scoped hardenings: **`tui_gateway/transport.py`** * Move JSON serialisation outside the lock — long messages no longer block sibling writers while we serialise. * Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and generic `OSError` from both `write` and `flush` as "peer is gone": return `False` instead of bubbling, matching what `write_json`'s callers in `entry.py` already expect. * Split `flush` into its own try block so a stuck flush never strands a partial write or holds the lock indefinitely on its way out. * Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit `flush()` entirely on environments where a half-closed read pipe produces an indefinite kernel-level block. Default unchanged. **`tui_gateway/entry.py`** * `_log_signal` now spawns a 1-second daemon timer that calls `os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck behind a wedged worker. Atexit handlers run inside the grace window when they can; the timer is the safety net so a deadlocked flush no longer strands the gateway process. Tests: * `test_write_json_closed_stream_returns_false` — ValueError path. * `test_write_json_oserror_on_flush_returns_false` — OSError on flush must not strand the lock; the write portion still landed before the flush failure. * `test_write_json_no_flush_env_skips_flush` — env knob bypass. Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py` (42/42 pass; one pre-existing failure on `test_session_resume_returns_hydrated_messages` is unrelated to this change — same `include_ancestors` mock kwarg issue tracked elsewhere). `scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass. * review(copilot): tighten transport hardening comments + test cleanup * review(copilot): narrow exception capture, configurable grace, simpler no-flush test * fix(tui-gateway): narrow ValueError to closed-stream; surface UnicodeEncodeError Copilot review on PR NousResearch#17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests. * fix(tui-gateway): reserve write() False for peer-gone; re-raise programming errors Round 2 Copilot review on PR NousResearch#17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test. * fix(tui-gateway): narrow OSError to peer-gone errnos; honest test naming Round 3 Copilot review on PR NousResearch#17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
dannyJ848
pushed a commit
to dannyJ848/hermes-agent
that referenced
this pull request
May 17, 2026
…SIGTERM races (NousResearch#17118) * fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races `tui_gateway` reports `tui_gateway_crash.log` traces where the main thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid- flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held — the interpreter shutdown stalls behind the wedged write. Two narrowly scoped hardenings: **`tui_gateway/transport.py`** * Move JSON serialisation outside the lock — long messages no longer block sibling writers while we serialise. * Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and generic `OSError` from both `write` and `flush` as "peer is gone": return `False` instead of bubbling, matching what `write_json`'s callers in `entry.py` already expect. * Split `flush` into its own try block so a stuck flush never strands a partial write or holds the lock indefinitely on its way out. * Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit `flush()` entirely on environments where a half-closed read pipe produces an indefinite kernel-level block. Default unchanged. **`tui_gateway/entry.py`** * `_log_signal` now spawns a 1-second daemon timer that calls `os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck behind a wedged worker. Atexit handlers run inside the grace window when they can; the timer is the safety net so a deadlocked flush no longer strands the gateway process. Tests: * `test_write_json_closed_stream_returns_false` — ValueError path. * `test_write_json_oserror_on_flush_returns_false` — OSError on flush must not strand the lock; the write portion still landed before the flush failure. * `test_write_json_no_flush_env_skips_flush` — env knob bypass. Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py` (42/42 pass; one pre-existing failure on `test_session_resume_returns_hydrated_messages` is unrelated to this change — same `include_ancestors` mock kwarg issue tracked elsewhere). `scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass. * review(copilot): tighten transport hardening comments + test cleanup * review(copilot): narrow exception capture, configurable grace, simpler no-flush test * fix(tui-gateway): narrow ValueError to closed-stream; surface UnicodeEncodeError Copilot review on PR NousResearch#17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests. * fix(tui-gateway): reserve write() False for peer-gone; re-raise programming errors Round 2 Copilot review on PR NousResearch#17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test. * fix(tui-gateway): narrow OSError to peer-gone errnos; honest test naming Round 3 Copilot review on PR NousResearch#17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
gweeteve
pushed a commit
to gweeteve/hermes-agent
that referenced
this pull request
Jun 2, 2026
…SIGTERM races (NousResearch#17118) * fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races `tui_gateway` reports `tui_gateway_crash.log` traces where the main thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid- flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held — the interpreter shutdown stalls behind the wedged write. Two narrowly scoped hardenings: **`tui_gateway/transport.py`** * Move JSON serialisation outside the lock — long messages no longer block sibling writers while we serialise. * Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and generic `OSError` from both `write` and `flush` as "peer is gone": return `False` instead of bubbling, matching what `write_json`'s callers in `entry.py` already expect. * Split `flush` into its own try block so a stuck flush never strands a partial write or holds the lock indefinitely on its way out. * Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit `flush()` entirely on environments where a half-closed read pipe produces an indefinite kernel-level block. Default unchanged. **`tui_gateway/entry.py`** * `_log_signal` now spawns a 1-second daemon timer that calls `os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck behind a wedged worker. Atexit handlers run inside the grace window when they can; the timer is the safety net so a deadlocked flush no longer strands the gateway process. Tests: * `test_write_json_closed_stream_returns_false` — ValueError path. * `test_write_json_oserror_on_flush_returns_false` — OSError on flush must not strand the lock; the write portion still landed before the flush failure. * `test_write_json_no_flush_env_skips_flush` — env knob bypass. Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py` (42/42 pass; one pre-existing failure on `test_session_resume_returns_hydrated_messages` is unrelated to this change — same `include_ancestors` mock kwarg issue tracked elsewhere). `scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass. * review(copilot): tighten transport hardening comments + test cleanup * review(copilot): narrow exception capture, configurable grace, simpler no-flush test * fix(tui-gateway): narrow ValueError to closed-stream; surface UnicodeEncodeError Copilot review on PR NousResearch#17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests. * fix(tui-gateway): reserve write() False for peer-gone; re-raise programming errors Round 2 Copilot review on PR NousResearch#17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test. * fix(tui-gateway): narrow OSError to peer-gone errnos; honest test naming Round 3 Copilot review on PR NousResearch#17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
Egavasyug
pushed a commit
to Egavasyug/hermes-agent
that referenced
this pull request
Jun 10, 2026
…SIGTERM races (NousResearch#17118) * fix(tui-gateway): harden stdio transport against half-closed pipes + SIGTERM races `tui_gateway` reports `tui_gateway_crash.log` traces where the main thread sits in `sys.stdin` while a worker holds `_stdout_lock` mid- flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held — the interpreter shutdown stalls behind the wedged write. Two narrowly scoped hardenings: **`tui_gateway/transport.py`** * Move JSON serialisation outside the lock — long messages no longer block sibling writers while we serialise. * Treat `BrokenPipeError`, `ValueError` ("I/O on closed file") and generic `OSError` from both `write` and `flush` as "peer is gone": return `False` instead of bubbling, matching what `write_json`'s callers in `entry.py` already expect. * Split `flush` into its own try block so a stuck flush never strands a partial write or holds the lock indefinitely on its way out. * Optional `HERMES_TUI_GATEWAY_NO_FLUSH=1` env knob to skip explicit `flush()` entirely on environments where a half-closed read pipe produces an indefinite kernel-level block. Default unchanged. **`tui_gateway/entry.py`** * `_log_signal` now spawns a 1-second daemon timer that calls `os._exit(0)` if the orderly `sys.exit(0)` path is itself stuck behind a wedged worker. Atexit handlers run inside the grace window when they can; the timer is the safety net so a deadlocked flush no longer strands the gateway process. Tests: * `test_write_json_closed_stream_returns_false` — ValueError path. * `test_write_json_oserror_on_flush_returns_false` — OSError on flush must not strand the lock; the write portion still landed before the flush failure. * `test_write_json_no_flush_env_skips_flush` — env knob bypass. Validation: `scripts/run_tests.sh tests/tui_gateway/test_protocol.py` (42/42 pass; one pre-existing failure on `test_session_resume_returns_hydrated_messages` is unrelated to this change — same `include_ancestors` mock kwarg issue tracked elsewhere). `scripts/run_tests.sh tests/test_tui_gateway_server.py` 90/90 pass. * review(copilot): tighten transport hardening comments + test cleanup * review(copilot): narrow exception capture, configurable grace, simpler no-flush test * fix(tui-gateway): narrow ValueError to closed-stream; surface UnicodeEncodeError Copilot review on PR NousResearch#17118: `UnicodeEncodeError` is a ValueError subclass, so a non-UTF-8 stdout (mismatched PYTHONIOENCODING / locale) would have been silently swallowed as 'peer gone' under `except ValueError`. That hides a real environment bug. Now: - UnicodeEncodeError → log with exc_info (warning) and drop the frame - ValueError where str(e) contains 'closed file' → peer gone, return False - Any other ValueError → log loudly, drop frame (defensive, but visible) Same shape applied to flush. Adds two regression tests. * fix(tui-gateway): reserve write() False for peer-gone; re-raise programming errors Round 2 Copilot review on PR NousResearch#17118: `Transport.write()` returning `False` is documented as 'peer is gone', and `entry.py` reacts by calling `sys.exit(0)`. But the implementation also returned False for non-IO conditions (non-JSON-safe payloads, UnicodeEncodeError, unrelated ValueErrors), so a programming error or local env bug would present as a clean disconnect — exactly the diagnosis pain we wanted to eliminate. Now: - `json.dumps` failure → re-raises (TypeError/ValueError surfaces in crash log) - `BrokenPipeError` → False (peer gone) - `ValueError('...closed file...')` → False (peer gone) - `UnicodeEncodeError` and any other ValueError → re-raise - `OSError` → False (existing IO-failure semantics, debug-logged) Tests updated to assert the re-raise behaviour and added a non-serializable-payload regression test. * fix(tui-gateway): narrow OSError to peer-gone errnos; honest test naming Round 3 Copilot review on PR NousResearch#17118: - Docstring claimed False = peer gone, but generic OSError on write/flush also returned False — meaning ENOSPC/EACCES/EIO would silently exit. Added `_PEER_GONE_ERRNOS = {EPIPE, ECONNRESET, EBADF, ESHUTDOWN, +WSA}` and narrowed the OSError handlers; non-peer-gone errnos re-raise. Docstring now lists OSError as peer-gone branch with the errno set. - The `_DISABLE_FLUSH` test was named after the env var but actually patched the module constant. Renamed it to reflect the contract being tested (skips flush when constant is true) AND added a real end-to-end test that sets the env var, reloads transport.py, and asserts the constant flips. Cleanup reload restores defaults so parallel tests stay isolated. Self-review (avoid round 4): - Verified TeeTransport's secondary-swallow stays intentional. - _log_signal grace path already covered by separate tests.
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.
Summary
Crash log traces from heavy-concurrency TUI sessions show the main thread blocked inside `sys.stdin` while a worker holds `_stdout_lock` mid-flush, and SIGTERM then calls `sys.exit(0)` while the lock is still held. The interpreter shutdown stalls behind the wedged write, leaving zombie gateway processes.
This is a narrow hardening pass that doesn't touch the dispatcher or message semantics.
Changes
`tui_gateway/transport.py`
`tui_gateway/entry.py`
Test plan
Refs
Audit comment (asheriif): "tui_gateway multi-threaded write lock deadlock and SIGTERM crash". Crash log signature includes `StdioTransport.write` blocked on `stream.flush()` and SIGTERM landing on the main thread.