fix(maestro): chain pipe-pane into new-session so initial output isn't lost (NAP-253)
ci / rust (push) Failing after 1m45s
ci / rust (push) Failing after 1m45s
`SessionManager::create` used to call `tmux new-session` (which exec's the user's command immediately), run an async `PtyBridge::start_with_replay` (mkfifo + open + spawn reader), then issue a *separate* `tmux pipe-pane`. Any output the child wrote between exec and that third tmux subprocess landing went into the pane but never into the FIFO. Hit in NAP-252 dogfood: `bash -c 'echo X; exec bash'` sessions showed only the trailing prompt, never the X. Two changes: 1. New `Tmux::new_session_piped(id, cwd, cmd, sink)` chains `; pipe-pane -t maestro-<id> <sink>` into the same tmux command argv as `new-session`. Within a tmux command chain, `pipe-pane` is processed immediately after `new-session` returns — before the child has finished `execve` — so the FIFO is wired up before the first write. 2. `SessionManager::create` now builds the FIFO + reader task **before** spawning the session, then calls `new_session_piped` instead of the two-step `new_session` + `pipe_pane`. `Tmux::pipe_pane` and the standalone `Tmux::new_session` stay around — `recover()` (NAP-228) still needs a standalone pipe-pane re-attach, and some integration tests want a bare session without a pipe. Tests: - Unit: argv assertion that the chained `pipe-pane` follows `new-session` when a sink is supplied. - Integration: `create_captures_initial_output_in_replay_buffer` spawns `sh -c 'echo NAP_253_FIRST_LINE_MARKER; exec sleep 30'` and polls `subscribe_pty_with_replay` until the marker shows up in the replay buffer (or fails after 5s). Fails before the fix, passes after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -211,17 +211,22 @@ impl SessionManager {
|
||||
.unwrap_or(false);
|
||||
|
||||
let id = Uuid::new_v4();
|
||||
self.inner
|
||||
.tmux
|
||||
.new_session(id, &effective_cwd, &req.command)
|
||||
.map_err(SessionError::Tmux)?;
|
||||
|
||||
// Build the FIFO + reader task *before* asking tmux to spawn the
|
||||
// command — otherwise the child's first bytes race the separate
|
||||
// pipe-pane call and get lost (NAP-253). `new_session_piped` chains
|
||||
// `pipe-pane` into the same tmux invocation as `new-session`, so the
|
||||
// pipe is attached before the child finishes `execve`.
|
||||
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?;
|
||||
self.inner
|
||||
.tmux
|
||||
.pipe_pane(id, &pipe_pane_command(&fifo_path))
|
||||
.new_session_piped(
|
||||
id,
|
||||
&effective_cwd,
|
||||
&req.command,
|
||||
&pipe_pane_command(&fifo_path),
|
||||
)
|
||||
.map_err(SessionError::Tmux)?;
|
||||
|
||||
let session = Session {
|
||||
|
||||
@@ -92,7 +92,28 @@ impl Tmux {
|
||||
let name = session_name(id);
|
||||
let out = self
|
||||
.cmd()
|
||||
.args(new_session_args(&name, cwd, command))
|
||||
.args(new_session_args(&name, cwd, command, None))
|
||||
.output()?;
|
||||
check(out)
|
||||
}
|
||||
|
||||
/// Like [`Self::new_session`], but chains `pipe-pane -t maestro-<id>
|
||||
/// <sink_command>` into the same tmux invocation. Use this from
|
||||
/// `SessionManager::create` so the pipe is in place before the freshly
|
||||
/// `exec`-ed child writes its first byte; calling `new_session` + a
|
||||
/// separate `pipe_pane` opens a window in which initial output is
|
||||
/// silently dropped (NAP-253).
|
||||
pub fn new_session_piped(
|
||||
&self,
|
||||
id: Uuid,
|
||||
cwd: &Path,
|
||||
command: &str,
|
||||
sink_command: &str,
|
||||
) -> Result<(), TmuxError> {
|
||||
let name = session_name(id);
|
||||
let out = self
|
||||
.cmd()
|
||||
.args(new_session_args(&name, cwd, command, Some(sink_command)))
|
||||
.output()?;
|
||||
check(out)
|
||||
}
|
||||
@@ -212,11 +233,18 @@ pub fn session_name(id: Uuid) -> String {
|
||||
format!("{SESSION_PREFIX}{id}")
|
||||
}
|
||||
|
||||
/// Args passed to `tmux` for `new_session`, excluding the binary itself and
|
||||
/// any socket/tmpdir flags injected by [`Tmux::cmd`]. Split out so the argv
|
||||
/// can be asserted without invoking tmux.
|
||||
fn new_session_args(name: &str, cwd: &Path, command: &str) -> Vec<OsString> {
|
||||
vec![
|
||||
/// Args passed to `tmux` for `new_session` / `new_session_piped`, excluding
|
||||
/// the binary itself and any socket/tmpdir flags injected by [`Tmux::cmd`].
|
||||
/// When `pipe_sink` is `Some`, a chained `pipe-pane -t <name> <sink>` is
|
||||
/// appended so the pipe attaches in the same tmux invocation that creates
|
||||
/// the session.
|
||||
fn new_session_args(
|
||||
name: &str,
|
||||
cwd: &Path,
|
||||
command: &str,
|
||||
pipe_sink: Option<&str>,
|
||||
) -> Vec<OsString> {
|
||||
let mut args = vec![
|
||||
OsString::from("set-option"),
|
||||
OsString::from("-g"),
|
||||
OsString::from("default-terminal"),
|
||||
@@ -233,7 +261,17 @@ fn new_session_args(name: &str, cwd: &Path, command: &str) -> Vec<OsString> {
|
||||
OsString::from("-c"),
|
||||
cwd.as_os_str().to_os_string(),
|
||||
OsString::from(command),
|
||||
]
|
||||
];
|
||||
if let Some(sink) = pipe_sink {
|
||||
args.extend([
|
||||
OsString::from(";"),
|
||||
OsString::from("pipe-pane"),
|
||||
OsString::from("-t"),
|
||||
OsString::from(name),
|
||||
OsString::from(sink),
|
||||
]);
|
||||
}
|
||||
args
|
||||
}
|
||||
|
||||
fn check(out: Output) -> Result<(), TmuxError> {
|
||||
@@ -253,7 +291,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn new_session_args_pass_256color_env_and_default_terminal() {
|
||||
let args = new_session_args("maestro-abc", Path::new("/tmp/proj"), "sleep 30");
|
||||
let args = new_session_args("maestro-abc", Path::new("/tmp/proj"), "sleep 30", None);
|
||||
let strs: Vec<&str> = args.iter().map(|s| s.to_str().unwrap()).collect();
|
||||
assert_eq!(
|
||||
strs,
|
||||
@@ -277,4 +315,22 @@ mod tests {
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_session_args_chain_pipe_pane_when_sink_present() {
|
||||
let args = new_session_args(
|
||||
"maestro-abc",
|
||||
Path::new("/tmp/proj"),
|
||||
"sleep 30",
|
||||
Some("cat >> '/tmp/fifo'"),
|
||||
);
|
||||
let strs: Vec<&str> = args.iter().map(|s| s.to_str().unwrap()).collect();
|
||||
// The pipe-pane chain must come after new-session in the same argv
|
||||
// so tmux attaches the pipe before the child writes its first byte
|
||||
// (NAP-253).
|
||||
assert_eq!(
|
||||
&strs[strs.len() - 5..],
|
||||
&[";", "pipe-pane", "-t", "maestro-abc", "cat >> '/tmp/fifo'"]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -108,6 +108,55 @@ async fn recover_adopts_existing_tmux_sessions() {
|
||||
assert_eq!(snap[0].command, "sleep 30");
|
||||
}
|
||||
|
||||
/// Regression for NAP-253: the very first bytes a freshly-spawned command
|
||||
/// writes must reach the replay buffer. Before the fix, `pipe-pane` was a
|
||||
/// separate tmux invocation that ran after `new-session` + an async FIFO
|
||||
/// setup, so the child's first output was buffered by tmux but never tee'd
|
||||
/// into the FIFO.
|
||||
#[tokio::test]
|
||||
async fn create_captures_initial_output_in_replay_buffer() {
|
||||
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(),
|
||||
// The shell prints the marker before doing anything else and
|
||||
// then sleeps so the session stays alive. Before NAP-253 this
|
||||
// marker was the canonical case that got swallowed.
|
||||
command: "sh -c 'echo NAP_253_FIRST_LINE_MARKER; exec sleep 30'".into(),
|
||||
env: HashMap::new(),
|
||||
metadata: HashMap::new(),
|
||||
worktree: None,
|
||||
})
|
||||
.await
|
||||
.expect("create");
|
||||
|
||||
let want = b"NAP_253_FIRST_LINE_MARKER";
|
||||
let deadline = std::time::Instant::now() + Duration::from_secs(5);
|
||||
let mut found = false;
|
||||
while std::time::Instant::now() < deadline {
|
||||
let (replay, _rx) = fx
|
||||
.manager
|
||||
.subscribe_pty_with_replay(s.id)
|
||||
.await
|
||||
.expect("subscribe");
|
||||
if replay.windows(want.len()).any(|w| w == want) {
|
||||
found = true;
|
||||
break;
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(50)).await;
|
||||
}
|
||||
assert!(
|
||||
found,
|
||||
"first-line marker never reached the replay buffer — pipe-pane raced past the child's initial output",
|
||||
);
|
||||
|
||||
fx.manager.kill(s.id).await.unwrap();
|
||||
}
|
||||
|
||||
/// 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
|
||||
|
||||
Reference in New Issue
Block a user