From 6f8d3a75a802cc2cc6a1b47ec0be580bc3b0f63e Mon Sep 17 00:00:00 2001 From: naps62-yolo Date: Wed, 27 May 2026 07:35:05 +0000 Subject: [PATCH] fix(maestro): re-pipe pane on recover so attach sees output (NAP-228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Tmux::pipe_pane` used `tmux pipe-pane -o`, which tmux documents as "only open a new pipe if no previous pipe exists." When `maestrod` restarts and `SessionManager::recover` re-issues `pipe-pane` against the adopted tmux session, the previous daemon process's pipe to a now-orphan FIFO is still active — so `-o` makes the call a no-op. The revived manager's PtyBridge reads from a fresh FIFO that tmux never feeds, and attach clients see input land in the pane but no output flow back. Drop the `-o`. tmux closes any existing pipe before opening the new one, which is what `pipe_pane` callers always wanted: `create` runs on a fresh session (nothing to clobber), `recover` actively needs the clobber. Test: `recover_re_pipes_pane_so_output_reaches_subscribers` creates a bash session under one manager, drops it, recovers under a second manager, sends keys, and asserts the echo bytes reach a fresh subscriber within 5s. Fails before the fix (stale pipe), passes after. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/maestro-daemon/src/session.rs | 4 +- crates/maestro-daemon/src/tmux.rs | 10 +-- .../maestro-daemon/tests/session_manager.rs | 64 +++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 8c85602..1373293 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -335,7 +335,9 @@ impl SessionManager { let fifo_path = self.inner.fifo_dir.join(format!("{id}.fifo")); let bridge = PtyBridge::start_with_replay(fifo_path.clone(), self.inner.replay_bytes).await?; - // Best-effort: re-pipe so output starts flowing again. + // Re-pipe so output starts flowing again. `Tmux::pipe_pane` runs + // without tmux's `-o` flag, so any stale pipe the previous daemon + // left running to a now-unread FIFO is replaced (NAP-228). let _ = self .inner .tmux diff --git a/crates/maestro-daemon/src/tmux.rs b/crates/maestro-daemon/src/tmux.rs index 1f017de..6eb99d9 100644 --- a/crates/maestro-daemon/src/tmux.rs +++ b/crates/maestro-daemon/src/tmux.rs @@ -149,16 +149,18 @@ impl Tmux { check(out) } - /// `tmux pipe-pane -t maestro- -o ''`. + /// `tmux pipe-pane -t maestro- ''`. /// /// `sink_command` is a shell command that receives pane output on stdin - /// (e.g. `cat >> /tmp/sess.log`). The `-o` flag makes the call idempotent: - /// if a pipe is already running on the pane, this is a no-op. + /// (e.g. `cat >> /tmp/sess.log`). Without `-o`, tmux closes any pipe + /// already running on the pane before opening this one — important on + /// `SessionManager::recover`, where the previous daemon process may have + /// left a `pipe-pane` writing to a FIFO that no one is reading anymore. pub fn pipe_pane(&self, id: Uuid, sink_command: &str) -> Result<(), TmuxError> { let name = session_name(id); let out = self .cmd() - .args(["pipe-pane", "-t", &name, "-o", sink_command]) + .args(["pipe-pane", "-t", &name, sink_command]) .output()?; check(out) } diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 33b1270..c73c5cc 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -108,6 +108,70 @@ async fn recover_adopts_existing_tmux_sessions() { assert_eq!(snap[0].command, "sleep 30"); } +/// Regression for NAP-228: after `recover`, PTY output must reach new +/// subscribers. Before the fix tmux's stale `pipe-pane` (with `-o`) silently +/// kept feeding the old FIFO and the revived manager's subscribers saw +/// nothing. +#[tokio::test] +async fn recover_re_pipes_pane_so_output_reaches_subscribers() { + let fx = DaemonFixture::new("laptop"); + let cwd = TempDir::new().unwrap(); + + let s = fx + .manager + .create(CreateSessionRequest { + daemon_id: "laptop".into(), + cwd: cwd.path().to_path_buf(), + command: "bash --noprofile --norc".into(), + env: HashMap::new(), + metadata: HashMap::new(), + worktree: None, + }) + .await + .expect("create"); + + // Drop the original manager. The previous daemon's `pipe-pane` is still + // running against tmux, pointing at a FIFO no one reads now. + drop(fx.manager); + + let sidecar = maestro_daemon::sidecar::SidecarStore::new(fx.sidecar_dir()); + let worktree_mgr = + maestro_daemon::worktree::WorktreeManager::new(fx.worktree_root.path().to_path_buf()); + let revived = maestro_daemon::SessionManager::new( + "laptop".to_string(), + fx.tmux.tmux.clone(), + sidecar, + worktree_mgr, + fx.fifo_dir(), + ); + assert_eq!(revived.recover().await.expect("recover"), 1); + + let mut rx = revived.subscribe_pty(s.id).await.expect("subscribe"); + revived + .send_keys(s.id, b"echo NAP_228_RECOVERED\n") + .await + .unwrap(); + + let want = b"NAP_228_RECOVERED"; + let mut buf: Vec = Vec::new(); + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while std::time::Instant::now() < deadline { + if let Ok(Ok(bytes)) = tokio::time::timeout(Duration::from_millis(50), rx.recv()).await { + buf.extend_from_slice(&bytes); + if buf.windows(want.len()).any(|w| w == want) { + break; + } + } + } + assert!( + buf.windows(want.len()).any(|w| w == want), + "recover() did not restore pty output flow; buf={:?}", + String::from_utf8_lossy(&buf), + ); + + revived.kill(s.id).await.unwrap(); +} + #[tokio::test] async fn pty_broadcast_delivers_bytes_to_subscribers() { let fx = DaemonFixture::new("laptop");