fix(maestro): roll back partial SessionManager::create on failure (NAP-260)
ci / rust (push) Failing after 5m47s
ci / rust (push) Failing after 5m47s
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-<uuid>` 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<PathBuf>, path: &Option<PathBuf>) {
|
||||
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> {
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user