fix(cli): restore terminal from cbreak mode after run exits - #294
Open
hertznsk wants to merge 1 commit into
Open
fix(cli): restore terminal from cbreak mode after run exits#294hertznsk wants to merge 1 commit into
hertznsk wants to merge 1 commit into
Conversation
hertznsk
force-pushed
the
issue-290-terminal-cbreak
branch
from
September 9, 2026 00:06
8ebd92f to
9983ccb
Compare
hertznsk
marked this pull request as draft
September 10, 2026 12:52
hertznsk
force-pushed
the
issue-290-terminal-cbreak
branch
2 times, most recently
from
September 14, 2026 08:43
0679668 to
6f0fbd0
Compare
Fixes microsoft#290 — Conductor could leave the invoking terminal in cbreak mode (no echo / no line editing) after a workflow exited, requiring a blind `reset` or `stty sane` to recover. The tty baseline is now captured once per process, before the first `tty.setcbreak()`, and reused by every later listener; `start()` is idempotent while active so a duplicate call can't overwrite the baseline; restore uses `TCSANOW` so a blocked output drain can't delay it; and the saved baseline is cleared only after a successful `tcsetattr` so a transient failure can be retried. The SIGTERM cleanup handler also no longer swallows the signal: after restoring the terminal it delegates to the previously-installed disposition (reset-and-re-raise for `SIG_DFL`, ignore for `SIG_IGN`, invoke a callable previous handler), and re-registration captures the previous disposition in the handler closure so the listener can no longer recurse into itself. The run and resume commands now also reapply and retire the baseline at their outermost cleanup boundary, after provider shutdown and every other teardown step. This closes a later race where normal completion or Ctrl+C could restore the terminal correctly and then a provider's cleanup could put it back into cbreak; SIGTERM during that same late-cleanup window restores the process baseline before terminating.
hertznsk
force-pushed
the
issue-290-terminal-cbreak
branch
from
September 14, 2026 08:46
6f0fbd0 to
5981c19
Compare
hertznsk
marked this pull request as ready for review
September 14, 2026 08:46
Contributor
Author
|
Jason Robert (@jrob5756) please take a look at this PR |
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
Fixes #290 — Conductor could leave the invoking terminal in cbreak mode (no echo / no line editing) after a workflow exited, requiring a blind
resetorstty saneto recover.Why this was reworked
An earlier revision of this PR only fixed the SIGTERM cleanup handler (delegating to the previous disposition instead of swallowing the signal, and capturing the disposition into the handler closure to avoid self-recursion). Verified against a real pseudo-terminal, that alone did not fix the issue: the terminal was still left in cbreak after ordinary exits and Ctrl+C, because the two actual root causes were elsewhere. The implementation was therefore reworked around them; the SIGTERM correctness fix is retained as one part of the whole.
Root cause
KeyboardListener.start()captured the tty settings on every call. Under specific interleavings — a second listener instance in the same process, or a secondstart()on an already-active listener — the captured "original" settings were the already-broken cbreak state, whichstop()then faithfully restored.listener.stop()had completed, so even a correct restore at the listener level did not guarantee a sane terminal at process exit.Additionally, the SIGTERM handler swallowed the signal, leaving the process running.
Changes
tty.setcbreak(); every later listener instance reuses it instead of re-snapshotting the live tty.start()idempotent while the listener is truly active (baseline held AND reader thread running), so a duplicate call can't overwrite the baseline or spawn duplicate threads. Start-after-suspend()still restarts correctly.TCSANOWinstead ofTCSADRAINso a blocked output drain can't delay the restore.tcsetattr, so a transient restore failure can be retried byatexit/SIGTERM/stop().SIG_DFLresets and re-raises viaos.kill,SIG_IGNstays ignored, a callable previous handler is invoked. The previous disposition is captured into the handler closure at registration time to prevent infinite self-recursion.restore_terminal_baseline(clear=True)). This ensures that even if a provider's cleanup puts the terminal back into cbreak, normal exit, Ctrl+C, and late cleanup failures all return a sane terminal to the shell — and that a later invocation in the same process never applies a stale baseline to a new terminal.Tests
tests/test_interrupt/test_listener_pty.py) reproduce both corruption scenarios on a genuine pseudo-terminal: two listener instances in one process, and double-start()on one instance.tests/test_interrupt/test_cli_terminal_pty.py) verify that provider teardown cannot bypass final TTY restoration, that a later non-interactive run doesn't reuse an old TTY baseline, and that the real CLI restores exact TTY attrs on exit and Ctrl+C.TCSANOWrestore paths, restore retry semantics, all SIGTERM delegation cases, and process baseline restoration after listener stop.make lintandmake typecheckclean.Scope
src/conductor/interrupt/listener.pysrc/conductor/cli/run.pytests/test_interrupt/CHANGELOG.mdNo behavior change for
suspend()/resume()semantics. Windows is unaffected (the listener is already Unix-only).