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.
fix: recover tmux terminal streams after server restart #597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: recover tmux terminal streams after server restart #597
Changes from all commits
d9597deFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Seeding bypasses the pyte screen, so a worker that was mid-turn at restart stays
PROCESSINGforeverseed_from_snapshotwrites the snapshot intoself._buffersand calls_detect_status— the raw path. It never touchesself._screens. But_process_chunk:139-142routes every provider withsupports_screen_detection = Truethrough the pyte screen wheneverCAO_PYTE_STATUSis on, and that is the default (constants.py:228). Five providers opt in, includingclaude_code.So after recovery the pyte screen is blank. The next real chunk off the re-armed FIFO is composited onto nothing, and TUI output is overwhelmingly cursor-addressed in-place repainting — which is the entire reason the screen path exists.
Reproduced at
d9597de(pane mid-turn at restart; Ink then repaints row 2 in place to finish the turn):In (A) the truncated viewport yields
UNKNOWN, which_apply_detectioncorrectly suppresses because a known status is already latched — so the terminal is pinned atPROCESSING. An idle agent emits nothing further, so nothing ever re-triggers detection. The pending callback is never delivered, and only another restart clears it. That is precisely the failure this PR set out to fix; it just moved from "was idle at restart" to "was busy at restart", which is the more likely restart scenario.The codebase already solved this.
fifo_reader._rearm_stalled_pipe:686-688recovers a dead forwarder by replaying the pane through the normal pipeline, and its docstring even documents the\r\nrequirement:That
\r\nis not cosmetic —get_historyjoins with a bare\nand pyte defaults LNM off, so rawcapture-paneoutput staircases:Please reuse that primitive instead of adding a parallel one. Keeping it synchronous (which you need, so the seed lands before the
reconcile_orphaned_messagescall atapi/main.py:1102) is easy —_schedule_screen_detectiondetects on the rising edge inline when_burstingisFalse, which you already set:And take the snapshot after
_rearm_pipe()interminal_service.py(Copilot's point at:240) so output produced during reattachment is included rather than lost to the old FIFO.Whichever shape you land on, this needs a test that actually runs the real
StatusMonitor— see my note on the test file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Recovered idle terminals trip the cold-start watchdog and get permanently dropped from it
create_readerunconditionally sets_ever_delivered[terminal_id] = Falseand_registered_at[terminal_id] = now(fifo_reader.py:180-182). For a newly launched terminal that is a correct assumption — a starting CLI always emits something. For a recovered idle terminal it is wrong: the pane is quiescent by definition, so the FIFO will never deliver a byte.The cold-start branch at
fifo_reader.py:490-497then matches on exactly that shape —not ever_deliveredand past the grace window andcontent.strip()(a recovered pane is full of prior conversation) — and concludes "the forwarder never started, full stop." Note the self-ROAST comment at:498-508: the replay publishes straight to the bus and never flips_ever_delivered, so each attempt re-qualifies.Reproduced at
d9597dewith the exact probe/rearm pair this function registers and an idle pane:So every recovered idle supervisor — the exact population this PR targets — gets ~5 spurious
stop_pipe_pane/pipe_panecycles plus 5 full-pane replays in the first ~20s (PIPE_LIVENESS_CHECK_INTERVAL_S4.0s), anERRORlog, and is then unenrolled from the issue #388 liveness watchdog for the life of the process. If its pipe genuinely stalls later, nothing repairs it — silently, since the give-up already logged and won't log again.P2 rather than P1 because the immediate effect is loud and benign-to-helpful; what's lost is a safety net, quietly.
Recovery knows the pipe was just re-armed against a live pane, so the cold-start check is not applicable here. Give
create_readera way to say so and have recovery pass it — the ordinary divergence check still protects these terminals:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2]
blackfails here (CI red), and mockingstatus_monitorleaves the new logic completely untestedThe Code Quality job fails on this file. Reproduced locally with
uv run black --check:Separately — and this is why the P1 shipped unnoticed — the test patches
terminal_service.status_monitorwholesale, sostatus_monitor.seed_from_snapshot.assert_called_once_with(...)only asserts that aMagicMockwas called.seed_from_snapshotis referenced nowhere else intest/, so the real implementation is executed by zero tests. What the suite currently pins is the call wiring, not the recovery.At minimum, add a test that drives the genuine
StatusMonitorwith a real provider:The failure paths in
recover_persisted_terminal_output_streamsare also untested —get_historyraising (stale DB row),create_readerraising, and thelist_sessionsguard — and thoseexceptblocks are the load-bearing part of the "must never prevent server startup" contract in the docstring.Uh oh!
There was an error while loading. Please reload this page.