From 2f4dde85acbf9576a241433e59d7d5c5140286fc Mon Sep 17 00:00:00 2001 From: naps62-yolo Date: Sun, 31 May 2026 22:15:02 +0000 Subject: [PATCH] fix(maestro): roll back partial SessionManager::create on failure (NAP-260) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit create() ran worktree-add → tmux spawn → sidecar write with no unwinding. A failure in a later step leaked earlier ones — worst case a sidecar write failure (disk full / EIO / permission) left a ghost `maestro-` tmux session running with no in-memory record, silently adopted with empty metadata on the next daemon restart. Each step now unwinds the ones before it: bridge/tmux failures remove the worktree (+ FIFO), and a sidecar-write failure kills the just-spawned tmux session before removing the FIFO + worktree. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/maestro-daemon/src/session.rs | 53 ++++++++++++++----- .../maestro-daemon/tests/session_manager.rs | 45 ++++++++++++++++ 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index c3ce31a..b6cef9e 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -217,17 +217,28 @@ impl SessionManager { // `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")); + // Each side effect below unwinds the ones before it on failure, so a + // partial create never leaks a worktree or — worse — a ghost tmux + // session with no in-memory record (NAP-260). let bridge = - PtyBridge::start_with_replay(fifo_path.clone(), self.inner.replay_bytes).await?; - self.inner - .tmux - .new_session_piped( - id, - &effective_cwd, - &req.command, - &pipe_pane_command(&fifo_path), - ) - .map_err(SessionError::Tmux)?; + match PtyBridge::start_with_replay(fifo_path.clone(), self.inner.replay_bytes).await { + Ok(b) => b, + Err(e) => { + self.rollback_worktree(&worktree_base_repo, &worktree_path); + return Err(e.into()); + } + }; + if let Err(e) = self.inner.tmux.new_session_piped( + id, + &effective_cwd, + &req.command, + &pipe_pane_command(&fifo_path), + ) { + // `bridge` drops at the end of this block, closing the FIFO reader. + let _ = std::fs::remove_file(&fifo_path); + self.rollback_worktree(&worktree_base_repo, &worktree_path); + return Err(SessionError::Tmux(e)); + } let session = Session { id, @@ -240,10 +251,14 @@ impl SessionManager { created_at: Utc::now(), }; - self.inner - .sidecar - .write(&session) - .map_err(SessionError::Sidecar)?; + if let Err(e) = self.inner.sidecar.write(&session) { + // tmux already spawned the session; kill it so no ghost survives a + // failed sidecar write, then unwind the FIFO + worktree. + let _ = self.inner.tmux.kill_session(id); + let _ = std::fs::remove_file(&fifo_path); + self.rollback_worktree(&worktree_base_repo, &worktree_path); + return Err(SessionError::Sidecar(e)); + } { let mut sessions = self.inner.sessions.lock().await; @@ -263,6 +278,16 @@ impl SessionManager { Ok(session) } + /// Best-effort removal of a worktree created earlier in `create` when a + /// later step fails. No-op when the request had no worktree. + fn rollback_worktree(&self, base: &Option, path: &Option) { + if let (Some(base), Some(path)) = (base, path) { + if let Err(e) = self.inner.worktree_mgr.remove(base, path) { + tracing::warn!(error = %e, ?path, "rollback: failed to remove worktree after partial create"); + } + } + } + /// Kill a session and remove its on-disk artifacts. The worktree is only /// pruned if the create request set `cleanup_on_kill: true`. pub async fn kill(&self, id: Uuid) -> Result<(), SessionError> { diff --git a/crates/maestro-daemon/tests/session_manager.rs b/crates/maestro-daemon/tests/session_manager.rs index 45581c5..22b7c2d 100644 --- a/crates/maestro-daemon/tests/session_manager.rs +++ b/crates/maestro-daemon/tests/session_manager.rs @@ -69,6 +69,51 @@ async fn rejects_wrong_daemon_in_request() { assert!(format!("{err}").contains("targets daemon"), "{err}"); } +/// 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("laptop"); + 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(); + + let res = fx + .manager + .create(CreateSessionRequest { + daemon_id: "laptop".into(), + 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("laptop");