From a6cae40c9b03b263e34f9b6c7bb22f5334abd32e Mon Sep 17 00:00:00 2001 From: endlessblink Date: Thu, 30 Jul 2026 14:08:51 +0300 Subject: [PATCH] [TC-069] Stop invalid Codex resume loops Persist missing-session resume failures, ignore replayed historical errors, and reopen the pane as a regular shell instead of retrying forever. Constraint: Preserve per-pane recovery identity and never resume one conversation in multiple panes. Rejected: Retrying the same missing session or suppressing PTY output. Tested: cargo test --lib; cargo check; rustfmt --check src-tauri/src/pty.rs; cargo clippy --lib; TMPDIR=/tmp npm run verify:restart-restore; TMPDIR=/tmp npm run verify:standalone-daemon --- MASTER_PLAN.md | 39 ++++++++ docs/regression-matrix.md | 1 + src-tauri/src/pty.rs | 206 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 239 insertions(+), 7 deletions(-) diff --git a/MASTER_PLAN.md b/MASTER_PLAN.md index de914d7..37479ea 100644 --- a/MASTER_PLAN.md +++ b/MASTER_PLAN.md @@ -152,6 +152,21 @@ Do not split them into unrelated cleanup/design buckets; execute them in order s the visual system, shell, navigation, terminal surface, map, command layer, run state, and visual QA converge on one product direction. +### ~~TC-069~~: ✅ Stop invalid agent resume loops + +**Priority:** P0 +**Status:** ✅ **DONE** (2026-07-30) + +Codex panes now stop retrying when their saved conversation no longer exists. +Recovery records the invalid target, ignores stale errors replayed from old +scrollback, and returns the pane to a regular shell after the failed attempt. + +Fresh evidence: + +- `CARGO_BUILD_JOBS=1 cargo test --lib` — 148 passed. +- `TMPDIR=/tmp npm run verify:restart-restore` — passed all three recovery layers. +- `TMPDIR=/tmp npm run verify:standalone-daemon` — passed the headed cold-restore smoke. + ### ~~TC-068~~: ✅ Make expensive chats unmistakable **Priority:** P1 @@ -6660,3 +6675,27 @@ never takes, which is why it reported "240/240 clean" while four panes were visi broken on screen; and `npm run doctor` now resolves the dock launcher through its wrapper and symlink, so its advice names the artifact the operator actually launches (dev mode) instead of a release binary that is never started. + +### ~~TC-069~~: ✅ Stop invalid agent resume loops + +**Priority:** P0 +**Status:** DONE (2026-07-30) + +When Codex reports that a pane's saved conversation no longer exists, recovery now +records the failed target and returns the pane to a regular shell instead of repeatedly +launching the same invalid resume command. Classification is limited to output from +the current attempt so an old error in replayed scrollback cannot poison a later +valid resume. The reported screenshot is preserved at +`/media/endlessblink/data/.dev-tmp/endlessblink/codex-clipboard-sqLjkQ.png` +(SHA-256 `c5983e4358a6ce6816d9cd6177d5a8b8593cb354806b40e507c8c543f6e60397`). + +**Evidence:** the regression failed before the production change. +`CARGO_BUILD_JOBS=1 cargo test failed_agent_resume_is_persisted_and_not_planned_again --lib` +and `CARGO_BUILD_JOBS=1 cargo test replayed_old_resume_error_does_not_poison_a_new_attempt --lib` +each pass; `CARGO_BUILD_JOBS=1 cargo test --lib` passes 148 tests; and +`CARGO_BUILD_JOBS=1 cargo check` passes. With `TMPDIR=/tmp` keeping the private Unix +socket below the platform path limit, `npm run verify:restart-restore` passes live +reattach, cold restore, and agent resume/reconstruction. `npm run verify:standalone-daemon` +passes live app restart, daemon cold restore, visible repaint, and post-restart input +against the rebuilt release app. `rustfmt --check` passes for the changed backend file; +`cargo clippy --lib` completes with the repository's ten pre-existing warnings. diff --git a/docs/regression-matrix.md b/docs/regression-matrix.md index 29896ae..df6f7c4 100644 --- a/docs/regression-matrix.md +++ b/docs/regression-matrix.md @@ -54,6 +54,7 @@ Playwright suite; the per-row specs are the precise guards. | 3.5 | Daemon/PTY latency regression | p95 budget (~1ms). | `verify:daemon-latency` | ✅ | | 3.6 | **All panes remain at shell prompts after their agent processes are killed, even though each pane has a saved conversation** | A surviving daemon correctly keeps each PTY alive, so cold-restore never runs and relaunching the window only reattaches to the idle shells. The Sessions panel now offers **Reconnect agents**: it keys recovery by pane, skips processes already running, verifies local Codex/Claude records, rejects unsafe ids, and writes each valid provider resume command only to its original idle pane. | `tests/agent-reconnect.spec.ts`, `tests/agent-reconnect-button.spec.ts`, `verify:map-terminals`, live desktop multi-pane action | ✅ | | 3.7 | Private verifier/runtime directory has no user systemd bus | Do not launch a transient systemd daemon unit unless the runtime directory exposes the user bus; fall back to the detached binary so the daemon socket still appears. | `cargo test platform_process`, `verify:standalone-daemon` | ✅ | +| 3.8 | **A pane repeatedly prints “No saved session found” after restart** | A failed Codex resume was never persisted as terminal recovery state, so every ensure replaced the ended PTY with the same invalid resume command. Missing-session output from the current attempt now marks the target `resume-failed`; replayed historical errors are ignored, and the next ensure opens a regular shell instead of retrying that conversation id. | `cargo test failed_agent_resume_is_persisted_and_not_planned_again`, `cargo test replayed_old_resume_error_does_not_poison_a_new_attempt`, `verify:restart-restore`, `verify:standalone-daemon` | ✅ lifecycle Rust guards + live restart and standalone-daemon cold restore | ## 4. Map (operations canvas) ↔ split diff --git a/src-tauri/src/pty.rs b/src-tauri/src/pty.rs index cc2074a..9cb5fc6 100644 --- a/src-tauri/src/pty.rs +++ b/src-tauri/src/pty.rs @@ -504,9 +504,19 @@ impl PtyManager { .map(|entry| plan_agent_restore(entry, false)); let recovery_command = recovery_plan.as_ref().and_then(|plan| plan.command.clone()); let cwd = cwd.or_else(|| persisted.as_ref().and_then(|entry| entry.cwd.clone())); - let command = recovery_command - .or(command) - .or_else(|| persisted.as_ref().and_then(|entry| entry.command.clone())); + let suppress_agent_relaunch = recovery_plan.as_ref().is_some_and(|plan| { + matches!( + plan.status, + AgentRestoreStatus::ResumeFailed | AgentRestoreStatus::NeedsAuth + ) + }); + let command = if suppress_agent_relaunch { + None + } else { + recovery_command + .or(command) + .or_else(|| persisted.as_ref().and_then(|entry| entry.command.clone())) + }; let pty_system = native_pty_system(); // Open the PTY at the caller's measured size when known so a freshly @@ -641,6 +651,7 @@ impl PtyManager { write_agent_restore_status(dir, &id, plan.status.clone(), plan.reason.as_deref()); } } + let resume_output_start = initial_buffer.data.len(); let output = Arc::new(Mutex::new(initial_buffer)); let output_reader = output.clone(); let subscribers: Arc>> = Arc::new(Mutex::new(Vec::new())); @@ -655,6 +666,10 @@ impl PtyManager { let reader_events = self.session_events.clone(); let reader_event_id = id.clone(); let reader_pid = child_pid; + let reader_persist_dir = self.persist_dir.clone(); + let reader_was_resuming = recovery_plan + .as_ref() + .is_some_and(|plan| plan.status == AgentRestoreStatus::Resuming); let reader_handle = std::thread::Builder::new() .name(format!("pty-reader-{id}")) @@ -728,6 +743,24 @@ impl PtyManager { subscribers_reader.lock().unwrap().clear(); ended_reader.store(true, Ordering::Release); event = event.with_exit_status(exit_status); + if reader_was_resuming { + let resume_failure = { + let output = output_reader.lock().unwrap(); + classify_agent_resume_failure( + output.data.get(resume_output_start..).unwrap_or_default(), + ) + }; + if let (Some(dir), Some(reason)) = + (reader_persist_dir.as_deref(), resume_failure) + { + write_agent_restore_status( + dir, + &reader_event_id, + AgentRestoreStatus::ResumeFailed, + Some(reason), + ); + } + } } trace_pty( "pty.session.event", @@ -1246,6 +1279,14 @@ fn plan_agent_restore(persisted: &PersistedSession, live_pty_exists: bool) -> Ag }; } + if persisted.restore_status == Some(AgentRestoreStatus::ResumeFailed) { + return AgentRestorePlan { + status: AgentRestoreStatus::ResumeFailed, + command: None, + reason: persisted.restore_failure_reason.clone(), + }; + } + if let Some(command) = persisted .sanitized_resume_command .as_deref() @@ -1315,7 +1356,10 @@ fn plan_agent_restore(persisted: &PersistedSession, live_pty_exists: bool) -> Ag { return AgentRestorePlan { status: AgentRestoreStatus::Resuming, - command: Some(format!("opencode --session {}", shell_quote_arg(session_id))), + command: Some(format!( + "opencode --session {}", + shell_quote_arg(session_id) + )), reason: None, }; } @@ -1344,6 +1388,14 @@ fn plan_agent_restore(persisted: &PersistedSession, live_pty_exists: bool) -> Ag } } +fn classify_agent_resume_failure(output: &str) -> Option<&'static str> { + if output.contains("No saved session found with ID") { + Some("saved agent session no longer exists") + } else { + None + } +} + fn shell_quote_arg(value: &str) -> String { if value .chars() @@ -1687,9 +1739,9 @@ fn discard_partial_replay_prefix(base_offset: u64, data: String) -> (u64, String #[cfg(test)] mod tests { use super::{ - agent_recovery_from_sidecar, discard_partial_replay_prefix, fnv1a_hex, plan_agent_restore, - replay_boundary_at_or_after, AgentRecoveryManifestUpdate, AgentRestoreStatus, - PersistedSession, PtyManager, SessionMeta, SessionRecoveryKind, + agent_recovery_from_sidecar, classify_agent_resume_failure, discard_partial_replay_prefix, + fnv1a_hex, plan_agent_restore, replay_boundary_at_or_after, AgentRecoveryManifestUpdate, + AgentRestoreStatus, PersistedSession, PtyManager, SessionMeta, SessionRecoveryKind, }; fn wait_for_snapshot_containing(manager: &PtyManager, id: &str, needle: &str) -> String { @@ -1780,6 +1832,33 @@ mod tests { assert_eq!(plan.reason, None); } + #[test] + fn agent_restore_planner_does_not_retry_a_failed_resume() { + let mut checkpoint = codex_agent_checkpoint(Some("019f-missing-session")); + checkpoint.restore_status = Some(AgentRestoreStatus::ResumeFailed); + checkpoint.restore_failure_reason = Some("resume command exited".to_string()); + + let plan = plan_agent_restore(&checkpoint, false); + + assert_eq!(plan.status, AgentRestoreStatus::ResumeFailed); + assert_eq!(plan.command, None); + assert_eq!(plan.reason.as_deref(), Some("resume command exited")); + } + + #[test] + fn missing_codex_session_output_marks_the_resume_as_failed() { + assert_eq!( + classify_agent_resume_failure( + "ERROR: No saved session found with ID 019f-missing-session." + ), + Some("saved agent session no longer exists") + ); + assert_eq!( + classify_agent_resume_failure("agent completed normally"), + None + ); + } + #[test] fn agent_restore_planner_builds_claude_resume_from_durable_session_id() { let plan = plan_agent_restore(&claude_agent_checkpoint(Some("97f9-claude-session")), false); @@ -2599,6 +2678,119 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn failed_agent_resume_is_persisted_and_not_planned_again() { + use std::path::PathBuf; + + let dir: PathBuf = std::env::temp_dir().join(format!( + "tw-agent-resume-failure-test-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create persistence dir"); + let id = "agent-resume-failure-test".to_string(); + + let mut scrollback = Vec::new(); + scrollback.extend_from_slice(&0_u64.to_le_bytes()); + scrollback.extend_from_slice(b"previous agent transcript\n"); + super::atomic_write(&super::scrollback_path(&dir, &id), &scrollback) + .expect("seed scrollback"); + let meta = SessionMeta { + cwd: Some("/tmp".to_string()), + command: Some("codex".to_string()), + recovery_kind: Some(SessionRecoveryKind::AgentTerminal), + provider: Some("codex".to_string()), + provider_session_id: Some("019f-missing-session".to_string()), + sanitized_resume_command: Some( + "printf 'ERROR: No saved session found with ID 019f-missing-session.\\n'" + .to_string(), + ), + ..SessionMeta::default() + }; + let meta_bytes = serde_json::to_vec(&meta).expect("encode seeded meta"); + super::atomic_write(&super::meta_path(&dir, &id), &meta_bytes).expect("seed metadata"); + + let manager = super::PtyManager::with_persistence_dir(dir.clone()); + manager + .ensure_detached(Some(id.clone()), None, None, None, None) + .expect("attempt saved resume"); + + let updated = (0..80) + .find_map(|_| { + let meta = std::fs::read(super::meta_path(&dir, &id)) + .ok() + .and_then(|bytes| serde_json::from_slice::(&bytes).ok())?; + if meta.restore_status == Some(AgentRestoreStatus::ResumeFailed) { + Some(meta) + } else { + std::thread::sleep(std::time::Duration::from_millis(25)); + None + } + }) + .expect("failed resume status was not persisted"); + assert_eq!( + updated.restore_failure_reason.as_deref(), + Some("saved agent session no longer exists") + ); + + let persisted = super::load_persisted(&dir, &id).expect("reload failed checkpoint"); + let plan = super::plan_agent_restore(&persisted, false); + assert_eq!(plan.status, AgentRestoreStatus::ResumeFailed); + assert_eq!(plan.command, None, "failed resume must not be retried"); + + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn replayed_old_resume_error_does_not_poison_a_new_attempt() { + use std::path::PathBuf; + + let dir: PathBuf = std::env::temp_dir().join(format!( + "tw-agent-old-resume-error-test-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("create persistence dir"); + let id = "agent-old-resume-error-test".to_string(); + + let mut scrollback = Vec::new(); + scrollback.extend_from_slice(&0_u64.to_le_bytes()); + scrollback.extend_from_slice(b"ERROR: No saved session found with ID old-session.\n"); + super::atomic_write(&super::scrollback_path(&dir, &id), &scrollback) + .expect("seed stale error scrollback"); + let meta = SessionMeta { + cwd: Some("/tmp".to_string()), + command: Some("codex".to_string()), + recovery_kind: Some(SessionRecoveryKind::AgentTerminal), + provider: Some("codex".to_string()), + provider_session_id: Some("019f-valid-session".to_string()), + sanitized_resume_command: Some("printf 'resume completed\\n'".to_string()), + ..SessionMeta::default() + }; + let meta_bytes = serde_json::to_vec(&meta).expect("encode seeded meta"); + super::atomic_write(&super::meta_path(&dir, &id), &meta_bytes).expect("seed metadata"); + + let manager = super::PtyManager::with_persistence_dir(dir.clone()); + manager + .ensure_detached(Some(id.clone()), None, None, None, None) + .expect("run clean resume attempt"); + std::thread::sleep(std::time::Duration::from_millis(150)); + + let updated = std::fs::read(super::meta_path(&dir, &id)) + .ok() + .and_then(|bytes| serde_json::from_slice::(&bytes).ok()) + .expect("read updated metadata"); + assert_eq!( + updated.restore_status, + Some(AgentRestoreStatus::Resuming), + "an error from replayed scrollback must not poison the new attempt" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn detached_sessions_are_owned_and_listed_without_tauri() { let manager = PtyManager::new();