fix(cli): restore terminal from cbreak mode after run exits#294
Open
hertznsk wants to merge 2 commits into
Open
fix(cli): restore terminal from cbreak mode after run exits#294hertznsk wants to merge 2 commits into
hertznsk wants to merge 2 commits into
Conversation
…xits A second KeyboardListener started while the terminal was already in cbreak mode (e.g. after an Esc pause/resume cycle or two listeners in one process) captured the cbreak state as its "original" settings and restored it on stop(), leaving the user's terminal without echo/ICANON. - capture the tty baseline once per process in a module-level cache, captured before the first tty.setcbreak() and reused by every later listener instance; - make start() idempotent while the listener is truly active so a duplicate call can no longer overwrite the baseline; - restore with TCSANOW instead of TCSADRAIN so a blocked output drain cannot delay the restore; - clear the saved baseline only after a successful tcsetattr so a transient restore failure can be retried by atexit/SIGTERM/stop(). Adds real-pty integration tests reproducing both corruption scenarios. Fixes microsoft#290
…f-recursion The SIGTERM cleanup handler restored the terminal but then swallowed the signal, so a SIGTERM during a run left the process alive instead of terminating with the default action. Re-registration could also capture the listener's own handler as the "previous" disposition, recursing forever when invoked. - delegate to the previously-installed disposition after restoring the terminal: SIG_DFL resets and re-raises via os.kill, SIG_IGN stays ignored, a callable previous handler is invoked; - capture the previous disposition in the handler closure at registration time instead of the mutable instance field; - store each instance's own handler and skip re-registration only when that exact handler is still installed, so a stopped listener's stale closure never blocks a new listener from protecting its terminal. Part of the terminal-safety fix for microsoft#290 Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
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.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.Changes
Two atomic commits, each leaving the suite fully green:
Commit 1 — prevent cbreak capture (
e5205b5)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().Commit 2 — SIGTERM correctness (
8ebd92f)SIG_DFLresets and re-raises viaos.kill,SIG_IGNstays ignored, a callable previous handler is invoked.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. Both fail on the old code and pass on the new. Gatedskipif(win32); CI runs them onubuntu-latest.TCSANOWrestore paths, restore retry semantics, and all SIGTERM delegation cases (DFL re-raise, IGN no-op, callable delegation, self-recursion guard, cross-instance registration).make lintandmake typecheckclean.conductor run examples/wait-smoke.yamlon a real pty leavesICANON|ECHOintact.Scope
Only
src/conductor/interrupt/listener.pyplus its tests. No CLI/engine/web/dashboard changes, no new dependencies, no new CLI flags, no behavior change forsuspend()/resume()semantics. Windows is unaffected (the listener is already Unix-only).