fix(maestro): re-pipe pane on recover so attach sees output (NAP-228)
`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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -149,16 +149,18 @@ impl Tmux {
|
||||
check(out)
|
||||
}
|
||||
|
||||
/// `tmux pipe-pane -t maestro-<id> -o '<sink_command>'`.
|
||||
/// `tmux pipe-pane -t maestro-<id> '<sink_command>'`.
|
||||
///
|
||||
/// `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)
|
||||
}
|
||||
|
||||
@@ -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<u8> = 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");
|
||||
|
||||
Reference in New Issue
Block a user