Skip to content

fix(cli): restore terminal from cbreak mode after run exits#294

Open
hertznsk wants to merge 2 commits into
microsoft:mainfrom
hertznsk:issue-290-terminal-cbreak
Open

fix(cli): restore terminal from cbreak mode after run exits#294
hertznsk wants to merge 2 commits into
microsoft:mainfrom
hertznsk:issue-290-terminal-cbreak

Conversation

@hertznsk

Copy link
Copy Markdown
Contributor

Summary

Fixes #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.

Root cause

KeyboardListener.start() captured the tty settings on every call. Under specific interleavings — a second listener instance in the same process, or a second start() on an already-active listener — the captured "original" settings were the already-broken cbreak state, which stop() then faithfully restored.

Changes

Two atomic commits, each leaving the suite fully green:

Commit 1 — prevent cbreak capture (e5205b5)

  • Capture the tty baseline once per process into a module-level cache before the first tty.setcbreak(); every later listener instance reuses it instead of re-snapshotting the live tty.
  • Make 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.
  • Restore with TCSANOW instead of TCSADRAIN so a blocked output drain can't delay the restore.
  • Clear the saved baseline only after a successful tcsetattr, so a transient restore failure can be retried by atexit/SIGTERM/stop().

Commit 2 — SIGTERM correctness (8ebd92f)

  • The SIGTERM cleanup handler restored the terminal but then swallowed the signal, leaving the process alive instead of terminating. It now delegates to the previously-installed disposition after restoring: SIG_DFL resets and re-raises via os.kill, SIG_IGN stays ignored, a callable previous handler is invoked.
  • The previous disposition is captured into the handler closure at registration time (not the mutable instance field), and each instance stores its own handler, skipping 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, and a handler can never capture itself as "previous" (infinite recursion).

Tests

  • New real-pty integration 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. Gated skipif(win32); CI runs them on ubuntu-latest.
  • New mocked unit tests for the baseline cache, idempotent start, TCSANOW restore paths, restore retry semantics, and all SIGTERM delegation cases (DFL re-raise, IGN no-op, callable delegation, self-recursion guard, cross-instance registration).
  • Full suite: 3670 passed, 13 skipped (excl. integration/perf/real_api/install_scripts); make lint and make typecheck clean.
  • End-to-end pty smoke: conductor run examples/wait-smoke.yaml on a real pty leaves ICANON|ECHO intact.

Scope

Only src/conductor/interrupt/listener.py plus its tests. No CLI/engine/web/dashboard changes, no new dependencies, no new CLI flags, no behavior change for suspend()/resume() semantics. Windows is unaffected (the listener is already Unix-only).

Note for the release-prep PR: this change intentionally does not add a CHANGELOG entry — please backfill it under [Unreleased] at release time.

Genadij Blinov and others added 2 commits July 13, 2026 18:27
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terminal left in cbreak mode (no echo) after conductor run exits

1 participant