feat(maestro): put a Panes seam behind Tmux
ci / rust (pull_request) Successful in 1m45s
ci / rust (push) Successful in 1m38s

SessionManager held a concrete Tmux, so its decisions — spawn rollback
ordering, liveness diffing, activity change-detection — could only be
reached by forking a real tmux server. Inner now holds an Arc<dyn Panes>;
Tmux is the production adapter, FakePanes the one tests drive.

Arc<dyn Panes> rather than a type parameter: a generic would spread
through LocalApiState, attach and cli, and one vtable hop is free next to
a fork+exec.

Adds tests/session_decisions.rs — six cases, no subprocess, 0.24s.
Replaces the chmod-0o500 rollback test, which detected root and returned
early, so on CI it asserted nothing; the failure is now forced by a
regular file sitting where the sidecar directory's parent must be, which
no uid can work around. It also asserts the rollback *order*, not just
the end state.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit was merged in pull request #41.
This commit is contained in:
Miguel Palhas
2026-08-22 12:44:13 +01:00
co-authored by Claude Opus 5
parent fc53d74850
commit 63898cef25
7 changed files with 545 additions and 65 deletions
+1 -1
View File
@@ -57,7 +57,7 @@ async fn run(config_override: Option<PathBuf>) -> Result<(), Box<dyn Error>> {
None => Tmux::new(),
};
let manager = SessionManager::new_with_replay(
tmux,
std::sync::Arc::new(tmux),
sidecar,
worktree_mgr,
config.fifo_dir(),
+1
View File
@@ -15,6 +15,7 @@ pub mod dirlist;
pub mod gitroot;
pub mod local_api;
pub mod local_events;
pub mod panes;
pub mod proc;
pub mod pty;
pub mod session;
+274
View File
@@ -0,0 +1,274 @@
//! The tmux operations [`crate::session::SessionManager`] depends on.
//!
//! Behind a trait so the manager's *decisions* — spawn rollback ordering,
//! liveness diffing, activity change-detection — can be exercised without
//! forking a tmux server. [`Tmux`] is the production adapter; [`FakePanes`] is
//! the one tests drive.
use std::collections::{HashMap, HashSet};
use std::path::Path;
use std::sync::Mutex;
use uuid::Uuid;
use crate::tmux::{PaneInfo, Tmux, TmuxError};
/// Every tmux call the session manager makes.
///
/// Object-safe on purpose: `Inner` holds an `Arc<dyn Panes>` rather than a type
/// parameter, which would otherwise spread through `LocalApiState`, `attach`
/// and `cli`. One vtable hop is free next to a `fork`+`exec`.
pub trait Panes: std::fmt::Debug + Send + Sync + 'static {
fn new_session_piped(
&self,
id: Uuid,
cwd: &Path,
command: &str,
sink_command: &str,
) -> Result<(), TmuxError>;
fn kill_session(&self, id: Uuid) -> Result<(), TmuxError>;
fn list_sessions(&self) -> Result<Vec<Uuid>, TmuxError>;
fn pane_info(&self, id: Uuid) -> Result<Option<PaneInfo>, TmuxError>;
fn send_keys(&self, id: Uuid, bytes: &[u8]) -> Result<(), TmuxError>;
fn pipe_pane(&self, id: Uuid, sink_command: &str) -> Result<(), TmuxError>;
fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError>;
}
impl Panes for Tmux {
fn new_session_piped(
&self,
id: Uuid,
cwd: &Path,
command: &str,
sink_command: &str,
) -> Result<(), TmuxError> {
Tmux::new_session_piped(self, id, cwd, command, sink_command)
}
fn kill_session(&self, id: Uuid) -> Result<(), TmuxError> {
Tmux::kill_session(self, id)
}
fn list_sessions(&self) -> Result<Vec<Uuid>, TmuxError> {
Tmux::list_sessions(self)
}
fn pane_info(&self, id: Uuid) -> Result<Option<PaneInfo>, TmuxError> {
Tmux::pane_info(self, id)
}
fn send_keys(&self, id: Uuid, bytes: &[u8]) -> Result<(), TmuxError> {
Tmux::send_keys(self, id, bytes)
}
fn pipe_pane(&self, id: Uuid, sink_command: &str) -> Result<(), TmuxError> {
Tmux::pipe_pane(self, id, sink_command)
}
fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError> {
Tmux::resize_window(self, id, cols, rows)
}
}
// ---------------------------------------------------------------------------
// Test adapter
// ---------------------------------------------------------------------------
/// Which [`Panes`] call a [`FakePanes`] expectation targets.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum Op {
NewSessionPiped,
KillSession,
ListSessions,
PaneInfo,
SendKeys,
PipePane,
ResizeWindow,
}
/// One recorded call, in the order the manager made it. Rollback ordering is a
/// sequence property, so the tests that matter assert on this.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Call {
NewSessionPiped { id: Uuid, command: String },
KillSession(Uuid),
ListSessions,
PaneInfo(Uuid),
SendKeys { id: Uuid, bytes: Vec<u8> },
PipePane(Uuid),
ResizeWindow { id: Uuid, cols: u16, rows: u16 },
}
/// In-memory [`Panes`] for tests.
///
/// Lives in `src/` rather than a test module because the daemon's integration
/// tests are separate crates, which `#[cfg(test)]` cannot reach and which
/// cannot enable a feature on their own crate.
#[derive(Debug, Default)]
pub struct FakePanes {
state: Mutex<FakeState>,
}
#[derive(Debug, Default)]
struct FakeState {
live: HashSet<Uuid>,
panes: HashMap<Uuid, PaneInfo>,
calls: Vec<Call>,
fail: HashMap<Op, String>,
}
impl FakePanes {
pub fn new() -> Self {
Self::default()
}
/// Make every call to `op` fail with `stderr`.
pub fn fail(&self, op: Op, stderr: impl Into<String>) {
self.lock().fail.insert(op, stderr.into());
}
/// Pretend a session exists without the manager having created it — the
/// recovery path's starting condition.
pub fn add_live(&self, id: Uuid) {
self.lock().live.insert(id);
}
/// Drop a session behind the manager's back, the way a command exiting or
/// an external `tmux kill-session` would.
pub fn remove_live(&self, id: Uuid) {
self.lock().live.remove(&id);
}
pub fn is_live(&self, id: Uuid) -> bool {
self.lock().live.contains(&id)
}
pub fn set_pane_info(&self, id: Uuid, info: PaneInfo) {
self.lock().panes.insert(id, info);
}
/// Every call so far, in order.
pub fn calls(&self) -> Vec<Call> {
self.lock().calls.clone()
}
fn lock(&self) -> std::sync::MutexGuard<'_, FakeState> {
self.state.lock().expect("FakePanes mutex poisoned")
}
fn record(&self, call: Call, op: Op) -> Result<(), TmuxError> {
let mut st = self.lock();
st.calls.push(call);
match st.fail.get(&op) {
Some(stderr) => Err(TmuxError::NonZero {
code: 1,
stderr: stderr.clone(),
}),
None => Ok(()),
}
}
}
impl Panes for FakePanes {
fn new_session_piped(
&self,
id: Uuid,
_cwd: &Path,
command: &str,
_sink_command: &str,
) -> Result<(), TmuxError> {
self.record(
Call::NewSessionPiped {
id,
command: command.to_owned(),
},
Op::NewSessionPiped,
)?;
self.lock().live.insert(id);
Ok(())
}
fn kill_session(&self, id: Uuid) -> Result<(), TmuxError> {
self.record(Call::KillSession(id), Op::KillSession)?;
self.lock().live.remove(&id);
Ok(())
}
fn list_sessions(&self) -> Result<Vec<Uuid>, TmuxError> {
self.record(Call::ListSessions, Op::ListSessions)?;
Ok(self.lock().live.iter().copied().collect())
}
fn pane_info(&self, id: Uuid) -> Result<Option<PaneInfo>, TmuxError> {
self.record(Call::PaneInfo(id), Op::PaneInfo)?;
Ok(self.lock().panes.get(&id).cloned())
}
fn send_keys(&self, id: Uuid, bytes: &[u8]) -> Result<(), TmuxError> {
self.record(
Call::SendKeys {
id,
bytes: bytes.to_vec(),
},
Op::SendKeys,
)
}
fn pipe_pane(&self, id: Uuid, _sink_command: &str) -> Result<(), TmuxError> {
self.record(Call::PipePane(id), Op::PipePane)
}
fn resize_window(&self, id: Uuid, cols: u16, rows: u16) -> Result<(), TmuxError> {
self.record(Call::ResizeWindow { id, cols, rows }, Op::ResizeWindow)
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn records_calls_in_order_and_tracks_liveness() {
let fake = FakePanes::new();
let id = Uuid::new_v4();
fake.new_session_piped(id, Path::new("/tmp"), "bash", "cat >> /x")
.unwrap();
assert!(fake.is_live(id));
fake.kill_session(id).unwrap();
assert!(!fake.is_live(id));
assert_eq!(
fake.calls(),
vec![
Call::NewSessionPiped {
id,
command: "bash".into()
},
Call::KillSession(id),
]
);
}
#[test]
fn injected_failure_still_records_the_attempt() {
let fake = FakePanes::new();
let id = Uuid::new_v4();
fake.fail(Op::NewSessionPiped, "boom");
assert!(fake
.new_session_piped(id, Path::new("/tmp"), "bash", "cat")
.is_err());
assert!(
!fake.is_live(id),
"a failed spawn must not register a session"
);
assert_eq!(fake.calls().len(), 1);
}
}
+5 -4
View File
@@ -17,10 +17,11 @@ use maestro_protocol::{
use tokio::sync::{broadcast, Mutex};
use uuid::Uuid;
use crate::panes::Panes;
use crate::pty::{pipe_pane_command, PtyBridge};
use crate::sidecar::{SidecarError, SidecarStore};
use crate::spawn_history::{RecentDir, SpawnHistory, SpawnHistoryRecord};
use crate::tmux::{Tmux, TmuxError};
use crate::tmux::TmuxError;
use crate::worktree::{WorktreeError, WorktreeManager};
/// Capacity of the session-events broadcast channel. The `/api/events`
@@ -93,7 +94,7 @@ pub struct SessionManager {
#[derive(Debug)]
struct Inner {
tmux: Tmux,
tmux: Arc<dyn Panes>,
sidecar: SidecarStore,
worktree_mgr: WorktreeManager,
fifo_dir: PathBuf,
@@ -110,7 +111,7 @@ struct Inner {
impl SessionManager {
pub fn new(
tmux: Tmux,
tmux: Arc<dyn Panes>,
sidecar: SidecarStore,
worktree_mgr: WorktreeManager,
fifo_dir: PathBuf,
@@ -127,7 +128,7 @@ impl SessionManager {
/// Variant that lets the caller override the per-session replay buffer
/// cap. Pass `0` to disable replay (subscribers see only live frames).
pub fn new_with_replay(
tmux: Tmux,
tmux: Arc<dyn Panes>,
sidecar: SidecarStore,
worktree_mgr: WorktreeManager,
fifo_dir: PathBuf,
+2 -1
View File
@@ -7,6 +7,7 @@
use std::future::Future;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::Arc;
use std::thread;
use std::time::{Duration, Instant};
@@ -135,7 +136,7 @@ impl DaemonFixture {
let sidecar = SidecarStore::new(state_dir.path().join("sessions"));
let worktree_mgr = WorktreeManager::new(worktree_root.path().to_path_buf());
let mut manager = SessionManager::new(
tmux.tmux.clone(),
Arc::new(tmux.tmux.clone()),
sidecar,
worktree_mgr,
state_dir.path().join("fifos"),
@@ -0,0 +1,258 @@
//! The session manager's decisions, driven through [`FakePanes`].
//!
//! These are the branches that used to need a forked tmux server to reach:
//! spawn rollback ordering, liveness diffing, and activity change-detection.
//! Nothing here spawns a process, so they run everywhere and in milliseconds.
use std::collections::HashMap;
use std::path::PathBuf;
use std::sync::Arc;
use std::time::Duration;
use maestro_daemon::panes::{Call, FakePanes, Op};
use maestro_daemon::session::{SessionError, SessionEvent, SessionManager};
use maestro_daemon::sidecar::SidecarStore;
use maestro_daemon::tmux::PaneInfo;
use maestro_daemon::worktree::WorktreeManager;
use maestro_protocol::{ActivityState, CreateSessionRequest};
use tempfile::TempDir;
use tokio::sync::broadcast::error::TryRecvError;
use uuid::Uuid;
struct Fx {
manager: SessionManager,
fake: Arc<FakePanes>,
tmp: TempDir,
}
impl Fx {
fn new() -> Self {
Self::with_sidecar_dir(|tmp| tmp.join("sidecars"))
}
fn with_sidecar_dir(pick: impl Fn(&std::path::Path) -> PathBuf) -> Self {
let tmp = TempDir::new().expect("tempdir");
let fake = Arc::new(FakePanes::new());
let fifo_dir = tmp.path().join("fifos");
std::fs::create_dir_all(&fifo_dir).expect("fifo dir");
let manager = SessionManager::new(
fake.clone(),
SidecarStore::new(pick(tmp.path())),
WorktreeManager::new(tmp.path().join("worktrees")),
fifo_dir,
);
Self { manager, fake, tmp }
}
fn request(&self, command: &str) -> CreateSessionRequest {
CreateSessionRequest {
cwd: self.tmp.path().to_path_buf(),
command: command.into(),
env: HashMap::new(),
metadata: HashMap::new(),
worktree: None,
}
}
}
/// Poll `manager.snapshot()` until it holds `n` sessions.
async fn wait_for_len(manager: &SessionManager, n: usize) {
let deadline = tokio::time::Instant::now() + Duration::from_secs(3);
while tokio::time::Instant::now() < deadline {
if manager.snapshot().await.len() == n {
return;
}
tokio::time::sleep(Duration::from_millis(10)).await;
}
panic!(
"timed out waiting for {n} sessions, have {}",
manager.snapshot().await.len()
);
}
/// NAP-260 rollback, asserted as an *ordering* rather than an end state.
///
/// Replaces a test that forced the sidecar failure with `chmod 0o500` and then
/// returned early when it detected root — so on CI it asserted nothing. Here
/// the sidecar directory cannot be created because a regular file sits where
/// its parent must be, which no uid can work around.
#[tokio::test]
async fn create_kills_the_tmux_session_when_the_sidecar_write_fails() {
let fx = Fx::with_sidecar_dir(|tmp| {
let blocker = tmp.join("blocker");
std::fs::write(&blocker, b"not a directory").expect("blocker file");
blocker.join("sidecars")
});
let res = fx.manager.create(fx.request("sleep 30")).await;
assert!(
matches!(res, Err(SessionError::Sidecar(_))),
"expected the sidecar write to fail, got {res:?}"
);
let calls = fx.fake.calls();
assert_eq!(
calls.len(),
2,
"expected spawn then rollback kill, got {calls:?}"
);
assert!(matches!(calls[0], Call::NewSessionPiped { .. }));
let Call::NewSessionPiped { id, .. } = calls[0] else {
unreachable!()
};
assert_eq!(
calls[1],
Call::KillSession(id),
"the rollback must kill the session it just spawned"
);
assert!(!fx.fake.is_live(id), "create left a ghost tmux session");
assert!(fx.manager.snapshot().await.is_empty());
}
/// A failed spawn must not roll back anything, least of all kill a session id
/// tmux never accepted.
#[tokio::test]
async fn create_does_not_roll_back_when_the_spawn_itself_fails() {
let fx = Fx::new();
fx.fake.fail(Op::NewSessionPiped, "no server running");
let res = fx.manager.create(fx.request("sleep 30")).await;
assert!(res.is_err(), "expected the spawn to fail, got {res:?}");
assert_eq!(
fx.fake.calls().len(),
1,
"only the failed spawn should have been attempted: {:?}",
fx.fake.calls()
);
assert!(fx.manager.snapshot().await.is_empty());
}
/// A restarted daemon adopts whatever tmux still holds, and must re-pipe each
/// one — without that the adopted session produces no output.
#[tokio::test]
async fn recover_adopts_live_sessions_and_repipes_them() {
let fx = Fx::new();
let orphan = Uuid::new_v4();
fx.fake.add_live(orphan);
let adopted = fx.manager.recover().await.expect("recover");
assert_eq!(adopted, 1);
let snap = fx.manager.snapshot().await;
assert_eq!(snap.len(), 1);
assert_eq!(snap[0].id, orphan);
assert!(
fx.fake.calls().contains(&Call::PipePane(orphan)),
"adopted session was never re-piped: {:?}",
fx.fake.calls()
);
}
#[tokio::test]
async fn liveness_reaps_only_the_session_tmux_lost() {
let fx = Fx::new();
let keep = fx.manager.create(fx.request("keep")).await.expect("keep");
let lose = fx.manager.create(fx.request("lose")).await.expect("lose");
let mut events = fx.manager.events();
// The command exited, or someone ran `tmux kill-session` behind our back.
fx.fake.remove_live(lose.id);
fx.manager.start_liveness_watcher(Duration::from_millis(25));
wait_for_len(&fx.manager, 1).await;
let survivors = fx.manager.snapshot().await;
assert_eq!(survivors.len(), 1);
assert_eq!(survivors[0].id, keep.id);
let mut removed = Vec::new();
while let Ok(ev) = events.try_recv() {
if let SessionEvent::Removed(id) = ev {
removed.push(id);
}
}
assert_eq!(
removed,
vec![lose.id],
"exactly one Removed, for the dead id"
);
}
/// If `list_sessions` fails the pass has no idea what is alive. Reaping on that
/// basis would kill the whole catalog.
#[tokio::test]
async fn liveness_reaps_nothing_when_tmux_cannot_be_listed() {
let fx = Fx::new();
let alive = fx.manager.create(fx.request("bash")).await.expect("create");
fx.fake.fail(Op::ListSessions, "connection refused");
fx.manager.start_liveness_watcher(Duration::from_millis(25));
// Give the watcher several passes to do the wrong thing.
tokio::time::sleep(Duration::from_millis(200)).await;
let snap = fx.manager.snapshot().await;
assert_eq!(
snap.len(),
1,
"a failed list must not be read as 'everything died'"
);
assert_eq!(snap[0].id, alive.id);
}
#[tokio::test]
async fn activity_pass_emits_updated_only_when_something_changed() {
let fx = Fx::new();
let session = fx.manager.create(fx.request("zsh")).await.expect("create");
let mut events = fx.manager.events();
fx.fake.set_pane_info(
session.id,
PaneInfo {
// A pid nothing can be reading from, so `awaiting_input` is a
// deterministic false rather than a probe of this machine.
pane_pid: i32::MAX,
command: "claude".into(),
tty: "/dev/null".into(),
current_path: "/srv/work".into(),
},
);
fx.manager.start_liveness_watcher(Duration::from_millis(25));
let updated = wait_for_updated(&mut events).await;
assert_eq!(updated.foreground.as_deref(), Some("claude"));
assert_eq!(
updated.current_cwd.as_deref(),
Some(std::path::Path::new("/srv/work"))
);
// Still inside `activity::ACTIVE_WINDOW` (3s) since the spawn, so the
// change here is `foreground`, not the activity state.
assert_eq!(updated.activity, ActivityState::Active);
// Drain anything already queued, then confirm a steady pane stays quiet.
// Stays well inside the 3s window, so no Active -> Idle transition either.
while events.try_recv().is_ok() {}
tokio::time::sleep(Duration::from_millis(200)).await;
assert!(
matches!(events.try_recv(), Err(TryRecvError::Empty)),
"an unchanged pane must not re-emit SessionUpdated every tick"
);
}
async fn wait_for_updated(
events: &mut tokio::sync::broadcast::Receiver<SessionEvent>,
) -> maestro_protocol::Session {
let deadline = tokio::time::Instant::now() + Duration::from_secs(3);
while tokio::time::Instant::now() < deadline {
match events.try_recv() {
Ok(SessionEvent::Updated(s)) => return s,
Ok(_) => continue,
Err(_) => tokio::time::sleep(Duration::from_millis(10)).await,
}
}
panic!("timed out waiting for SessionUpdated");
}
+4 -59
View File
@@ -4,6 +4,7 @@ mod common;
use std::collections::HashMap;
use std::path::PathBuf;
use std::sync::Arc;
use std::time::Duration;
use common::{wait_for_async, DaemonFixture};
@@ -101,63 +102,6 @@ async fn create_without_history_logs_nothing() {
assert!(!fx.spawn_history_path().exists());
}
/// NAP-260: when the sidecar write fails *after* tmux has spawned the session,
/// `create` must roll back — no ghost tmux session, no in-memory record left
/// behind. We force the failure by making the sidecar directory read-only (the
/// FIFO + tmux steps use other dirs and still succeed).
#[tokio::test]
async fn create_rolls_back_tmux_session_when_sidecar_write_fails() {
use std::os::unix::fs::PermissionsExt;
let fx = DaemonFixture::new();
let cwd = TempDir::new().unwrap();
let sidecar_dir = fx.sidecar_dir();
std::fs::create_dir_all(&sidecar_dir).unwrap();
std::fs::set_permissions(&sidecar_dir, std::fs::Permissions::from_mode(0o500)).unwrap();
// The failure is injected by making the sidecar dir read-only — but root
// bypasses directory permission bits, so the write would succeed and the
// rollback path never trigger. CI runs as root; detect that with a write
// probe and skip rather than report a spurious failure (the rollback is
// still covered in any non-root environment).
let probe = sidecar_dir.join(".perm_probe");
if std::fs::write(&probe, b"x").is_ok() {
let _ = std::fs::remove_file(&probe);
std::fs::set_permissions(&sidecar_dir, std::fs::Permissions::from_mode(0o700)).unwrap();
eprintln!("skipping: sidecar dir writable despite 0o500 (running as root?)");
return;
}
let res = fx
.manager
.create(CreateSessionRequest {
cwd: cwd.path().to_path_buf(),
command: "sleep 30".into(),
env: HashMap::new(),
metadata: HashMap::new(),
worktree: None,
})
.await;
// Restore perms so the TempDir can be cleaned up regardless of assertions.
std::fs::set_permissions(&sidecar_dir, std::fs::Permissions::from_mode(0o700)).unwrap();
assert!(
matches!(res, Err(maestro_daemon::SessionError::Sidecar(_))),
"expected the sidecar write to fail, got {res:?}"
);
// The session tmux spawned must have been killed — no ghost left running.
let live = fx.tmux.tmux.list_sessions().expect("list tmux sessions");
assert!(
live.is_empty(),
"create left a ghost tmux session behind: {live:?}"
);
// And nothing in the in-memory catalog.
assert!(fx.manager.snapshot().await.is_empty());
}
#[tokio::test]
async fn recover_adopts_existing_tmux_sessions() {
let fx = DaemonFixture::new();
@@ -182,7 +126,7 @@ async fn recover_adopts_existing_tmux_sessions() {
let worktree_mgr =
maestro_daemon::worktree::WorktreeManager::new(fx.worktree_root.path().to_path_buf());
let revived = maestro_daemon::SessionManager::new(
fx.tmux.tmux.clone(),
Arc::new(fx.tmux.tmux.clone()),
sidecar,
worktree_mgr,
fx.fifo_dir(),
@@ -280,7 +224,8 @@ async fn recover_re_pipes_pane_so_output_reaches_subscribers() {
let sidecar = maestro_daemon::sidecar::SidecarStore::new(sidecar_dir);
let worktree_mgr = maestro_daemon::worktree::WorktreeManager::new(worktree_root);
let revived = maestro_daemon::SessionManager::new(tmux, sidecar, worktree_mgr, fifo_dir);
let revived =
maestro_daemon::SessionManager::new(Arc::new(tmux), sidecar, worktree_mgr, fifo_dir);
assert_eq!(revived.recover().await.expect("recover"), 1);
let mut rx = revived.subscribe_pty(s.id).await.expect("subscribe");