From 44841c8b3c6a12722146336698502f39b5c797c3 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Wed, 23 Sep 2026 08:13:47 -0700 Subject: [PATCH] fix(sessions): say which guard refused a session store switch Refs #6418. A refused session switch always said the saved Runtime store "belongs to a different host", whatever the actual guard was. When another running Codewhale process holds the store, the suggested `codewhale resume` meets the same holder, so the advice was wrong. RuntimeStoreBinding::adoption_refusal returns the first guard that did not provably hold (unconfined, not a directory, held by a live process, durable work in a named directory, scope-pinned automation). It fails closed and never widens what is adoptable. The switch path now names the reason, and a live holder gets the step that actually frees the store. has_no_durable_work lost its last production consumer, so its two test assertions move to adoption_refusal and the helper is removed instead of suppressed. Mined from the unreviewed 0.10.1 WIP branch (runtime_threads.rs and its adoption_refusal test module); the UI consumer migration is new. Evidence: 17 passed, 0 failed (13,233 skipped) across runtime_store_binding, adoption_refusal and adoptable selections. TUI all-target/all-feature Clippy with CI flags and fmt passed; dead-code budget stays at 279. This is a diagnostic, not a fix for the #6418 reporter's store; that still needs their store/binding evidence. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/tui/src/runtime_threads.rs | 104 +++++++--- crates/tui/src/runtime_threads/tests.rs | 181 ++++++++++++++++++ crates/tui/src/tui/ui/apply.rs | 29 ++- .../src/tui/ui/tests/runtime_store_binding.rs | 10 +- 4 files changed, 284 insertions(+), 40 deletions(-) diff --git a/crates/tui/src/runtime_threads.rs b/crates/tui/src/runtime_threads.rs index 6da7506764..1d532bedf1 100644 --- a/crates/tui/src/runtime_threads.rs +++ b/crates/tui/src/runtime_threads.rs @@ -3073,6 +3073,44 @@ pub struct RuntimeThreadManagerConfig { pub max_active_threads: usize, } +/// Why a session switch refused to adopt an existing Runtime store. +/// +/// Returned by [`RuntimeStoreBinding::adoption_refusal`]; the first guard +/// that did not provably hold wins. Known limitation: it names one reason, +/// not every one — a store both held and non-empty reports only the hold. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum StoreAdoptionRefusal { + /// The store is not at `/sessions//runtime` (or a + /// `runtime-recovered-*` sibling), or a symlink sits on the way down. + Unconfined, + /// The confined store path is not an existing directory. + NotADirectory, + /// Another live process holds the store's process-owner lock. + HeldByLiveProcess, + /// The named store directory holds work a switch would abandon. + HasDurableWork { dir: &'static str }, + /// An automation is pinned to this store's execution scope. + ScopePinnedAutomation, +} + +impl std::fmt::Display for StoreAdoptionRefusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unconfined => f.write_str("the saved store is outside the session directory"), + Self::NotADirectory => f.write_str("the saved store path is not an existing directory"), + Self::HeldByLiveProcess => { + f.write_str("another running Codewhale process holds the saved store") + } + Self::HasDurableWork { dir } => { + write!(f, "the saved store still holds work in `{dir}`") + } + Self::ScopePinnedAutomation => { + f.write_str("an automation is pinned to the saved store") + } + } + } +} + /// Durable host authority shared by conversations created in that host. /// A conversation id can change at launch; the locked Runtime store cannot. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -3122,8 +3160,8 @@ impl RuntimeStoreBinding { } } - /// True when a confined store exists but holds nothing a session switch - /// could abandon. + /// The first store directory holding durable work, or `None` when the + /// store holds nothing a session switch could abandon. /// /// The switch path can rebind a conversation but cannot carry a store's /// durable work across — queued tasks, pending approvals, agent mail — @@ -3132,22 +3170,16 @@ impl RuntimeStoreBinding { /// there is nothing to abandon, so refusing protects nothing, and a /// force-quit leaves exactly this shape (#6207). /// - /// Fails closed: anything unreadable, unconfined, or non-empty is treated - /// as work worth keeping. Scope-pinned automations live outside the store - /// directories and are covered by [`Self::has_scope_pinned_automation`], - /// not here. - pub(crate) fn has_no_durable_work(&self) -> Result { - if !self.is_confined_session_store()? { - return Ok(false); - } - if !self.data_dir.is_dir() { - return Ok(false); - } + /// Callers establish confinement and that `data_dir` is a directory + /// first; this only reads. Scope-pinned automations live outside the + /// store directories and are covered by + /// [`Self::has_scope_pinned_automation`], not here. + fn first_durable_work_dir(&self) -> Result> { for name in RUNTIME_STORE_WORK_DIRS { match fs::read_dir(self.data_dir.join(name)) { Ok(mut entries) => { if entries.next().is_some() { - return Ok(false); + return Ok(Some(name)); } } // A store opened by an older build may predate a directory; @@ -3157,13 +3189,13 @@ impl RuntimeStoreBinding { } } // A sequence past its initial value means events were appended, even - // if those files have since been pruned. + // if those files have since been pruned — reported as `events`. match fs::read_to_string(self.data_dir.join("state.json")) { Ok(raw) => { let state: RuntimeStoreState = serde_json::from_str(&raw)?; - Ok(state.next_seq <= 1) + Ok((state.next_seq > 1).then_some("events")) } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(true), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), Err(err) => Err(err.into()), } } @@ -3199,7 +3231,7 @@ impl RuntimeStoreBinding { /// True when an automation's execution scope matches this binding. /// /// Scope-pinned automations are recorded outside the store directories, so - /// [`Self::has_no_durable_work`] cannot see them — adopting their store + /// [`Self::first_durable_work_dir`] cannot see them — adopting their store /// would orphan their scheduled work. A missing automations directory /// means no definitions exist. Read-only: the manager is only opened when /// the directory exists, and listing takes no locks. @@ -3220,27 +3252,39 @@ impl RuntimeStoreBinding { })) } - /// True when the bound store exists and a switch may adopt it: confined, - /// empty, unheld, with no scope-pinned automation. Liveness is checked - /// before emptiness — a live holder's disk state moves under the read — - /// and the automation check runs last because it parses every definition. - pub(crate) fn is_adoptable_empty_store(&self) -> Result { + /// Why a session switch may not adopt the bound store, or `None` when it + /// may: confined, a directory, unheld, empty, with no scope-pinned + /// automation. Liveness is checked before emptiness — a live holder's + /// disk state moves under the read — and the automation check runs last + /// because it parses every definition. + /// + /// Fails closed: every refusal is the first condition that did not + /// provably hold, and an unexpected IO or parse error is an `Err`, which + /// callers treat as a refusal. The reason exists so a user can be told + /// *which* guard refused (#6418); it never widens what is adoptable. + pub(crate) fn adoption_refusal(&self) -> Result> { if !self.is_confined_session_store()? { - return Ok(false); + return Ok(Some(StoreAdoptionRefusal::Unconfined)); } if !self.data_dir.is_dir() { - return Ok(false); + return Ok(Some(StoreAdoptionRefusal::NotADirectory)); } if self.has_live_holder()? { - return Ok(false); + return Ok(Some(StoreAdoptionRefusal::HeldByLiveProcess)); } - if !self.has_no_durable_work()? { - return Ok(false); + if let Some(dir) = self.first_durable_work_dir()? { + return Ok(Some(StoreAdoptionRefusal::HasDurableWork { dir })); } if self.has_scope_pinned_automation()? { - return Ok(false); + return Ok(Some(StoreAdoptionRefusal::ScopePinnedAutomation)); } - Ok(true) + Ok(None) + } + + /// True when the bound store exists and a switch may adopt it; see + /// [`Self::adoption_refusal`] for the reason when it may not. + pub(crate) fn is_adoptable_empty_store(&self) -> Result { + Ok(self.adoption_refusal()?.is_none()) } pub(crate) fn validate_existing_store(&self) -> Result<()> { diff --git a/crates/tui/src/runtime_threads/tests.rs b/crates/tui/src/runtime_threads/tests.rs index ea0e24f477..005f212f37 100644 --- a/crates/tui/src/runtime_threads/tests.rs +++ b/crates/tui/src/runtime_threads/tests.rs @@ -17657,3 +17657,184 @@ async fn engine_plumbing_items_are_tagged_internal_and_retry_hints_are_dropped() } Ok(()) } + +/// #6418: a refused session switch must say *which* guard refused. One case +/// per `StoreAdoptionRefusal` variant, each built from a real store so the +/// layout under test is the product's. +mod adoption_refusal { + use super::*; + use crate::automation_manager::{AutomationManager, AutomationStatus, CreateAutomationRequest}; + + /// Fields drop in declaration order: the env guards restore before the + /// env lock releases. + struct Fixture { + _home: crate::test_support::EnvVarGuard, + _runtime: crate::test_support::EnvVarGuard, + _legacy: crate::test_support::EnvVarGuard, + root: tempfile::TempDir, + _env: crate::test_support::TestEnvLock, + } + + fn fixture() -> Result { + let env = crate::test_support::lock_test_env(); + let root = tempfile::tempdir()?; + let home = crate::test_support::EnvVarGuard::set("CODEWHALE_HOME", root.path()); + let runtime = crate::test_support::EnvVarGuard::remove("CODEWHALE_RUNTIME_DIR"); + let legacy = crate::test_support::EnvVarGuard::remove("DEEPSEEK_RUNTIME_DIR"); + Ok(Fixture { + root, + _env: env, + _home: home, + _runtime: runtime, + _legacy: legacy, + }) + } + + fn opened_binding( + fixture: &Fixture, + session: &str, + scope: &str, + ) -> Result { + let data_dir = fixture + .root + .path() + .join("sessions") + .join(session) + .join("runtime"); + drop(RuntimeThreadStore::open(data_dir.clone())?); + Ok(RuntimeStoreBinding { + data_dir, + execution_scope: scope.to_string(), + }) + } + + #[test] + fn empty_unheld_store_has_no_refusal() -> Result<()> { + let fixture = fixture()?; + let binding = opened_binding(&fixture, "adoptable", &"0".repeat(64))?; + assert_eq!(binding.adoption_refusal()?, None); + assert!(binding.is_adoptable_empty_store()?); + Ok(()) + } + + #[test] + fn store_outside_the_sessions_dir_is_unconfined() -> Result<()> { + let fixture = fixture()?; + let data_dir = fixture.root.path().join("elsewhere").join("runtime"); + drop(RuntimeThreadStore::open(data_dir.clone())?); + let binding = RuntimeStoreBinding { + data_dir, + execution_scope: "0".repeat(64), + }; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::Unconfined) + ); + assert!(!binding.is_adoptable_empty_store()?); + Ok(()) + } + + /// A confined path that does not exist reaches `NotADirectory`; a file + /// in its place never gets that far — confinement rejects any + /// non-directory on the way down as an `Err`, which is still a refusal. + #[test] + fn missing_confined_store_is_not_a_directory() -> Result<()> { + let fixture = fixture()?; + let session_dir = fixture.root.path().join("sessions").join("absent"); + std::fs::create_dir_all(&session_dir)?; + let binding = RuntimeStoreBinding { + data_dir: session_dir.join("runtime"), + execution_scope: "0".repeat(64), + }; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::NotADirectory) + ); + assert!(!binding.is_adoptable_empty_store()?); + + std::fs::write(&binding.data_dir, "not a store")?; + assert!( + binding.adoption_refusal().is_err(), + "a file in the store's place fails closed" + ); + Ok(()) + } + + #[test] + fn live_owner_lock_is_held_by_live_process() -> Result<()> { + let fixture = fixture()?; + let binding = opened_binding(&fixture, "held", &"0".repeat(64))?; + let held = RuntimeProcessOwnerLock::acquire(&binding.data_dir)?; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::HeldByLiveProcess) + ); + assert!(!binding.is_adoptable_empty_store()?); + drop(held); + assert_eq!(binding.adoption_refusal()?, None); + Ok(()) + } + + #[test] + fn each_work_dir_is_named_as_durable_work() -> Result<()> { + let fixture = fixture()?; + let binding = opened_binding(&fixture, "busy", &"0".repeat(64))?; + for dir in RUNTIME_STORE_WORK_DIRS { + let marker = binding.data_dir.join(dir).join("work.json"); + std::fs::write(&marker, "{}")?; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::HasDurableWork { dir }), + "{dir} holds work" + ); + assert!(!binding.is_adoptable_empty_store()?); + std::fs::remove_file(&marker)?; + } + // Events appended and since pruned still count, reported as `events`. + let state_path = binding.data_dir.join("state.json"); + let mut state: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&state_path)?)?; + state["next_seq"] = serde_json::json!(5); + std::fs::write(&state_path, serde_json::to_vec(&state)?)?; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::HasDurableWork { dir: "events" }) + ); + Ok(()) + } + + #[test] + fn automation_pinned_to_the_scope_refuses() -> Result<()> { + let fixture = fixture()?; + let scope = "ab".repeat(32); + let binding = opened_binding(&fixture, "pinned", &scope)?; + let automations = AutomationManager::open(fixture.root.path().join("automations"))?; + let created = automations.create_automation(CreateAutomationRequest { + name: "scope fixture".into(), + prompt: "local fixture only".into(), + rrule: "FREQ=HOURLY;INTERVAL=1".into(), + cwds: vec![fixture.root.path().into()], + model: None, + model_provider: None, + model_provider_id: None, + mode: None, + allow_shell: Some(false), + trust_mode: Some(false), + auto_approve: Some(false), + delivery_mode: None, + status: Some(AutomationStatus::Paused), + })?; + automations.edit_automation(&created.id, |record| { + let mut record = + record.ok_or_else(|| anyhow::anyhow!("fresh automation must exist"))?; + record.execution_scope = Some(scope.clone()); + Ok(Some(record)) + })?; + assert_eq!( + binding.adoption_refusal()?, + Some(StoreAdoptionRefusal::ScopePinnedAutomation) + ); + assert!(!binding.is_adoptable_empty_store()?); + Ok(()) + } +} diff --git a/crates/tui/src/tui/ui/apply.rs b/crates/tui/src/tui/ui/apply.rs index aa8d261b0f..52a6bfb4f0 100644 --- a/crates/tui/src/tui/ui/apply.rs +++ b/crates/tui/src/tui/ui/apply.rs @@ -3592,16 +3592,33 @@ pub(crate) fn apply_loaded_session_with_goal( // scope-pinned automation. A force-quit leaves the second shape — the // store is on disk, ownerless and holding zero events — and refusing // it protected nothing while making the session unopenable (#6207). - let nothing_to_abandon = binding + let refusal = if binding .is_missing_session_store() .map_err(|error| error.to_string())? - || binding - .is_adoptable_empty_store() - .map_err(|error| error.to_string())?; - if nothing_to_abandon { + { + None + } else { + binding + .adoption_refusal() + .map_err(|error| error.to_string())? + }; + if refusal.is_none() { recovered_binding = tasks.session_store_binding(); } + if let Some(crate::runtime_threads::StoreAdoptionRefusal::HeldByLiveProcess) = refusal { + // A fresh `codewhale resume` would meet the same live holder, so + // name the step that actually frees the store (#6418). + return Err(format!( + "This session's saved Runtime store is open in another running \ + Codewhale process. Close that session there, then open this one \ + again, or run `codewhale resume {}` after it exits.", + session.metadata.id + )); + } if recovered_binding.is_none() { + let reason = refusal + .map(|refusal| format!(" ({refusal})")) + .unwrap_or_default(); // Name the real condition and the path that actually works. The // old wording ("resume it in a new Codewhale process") sent users // in circles: starting a new process and then picking the session @@ -3612,7 +3629,7 @@ pub(crate) fn apply_loaded_session_with_goal( // adopts it (runtime_threads.rs, `validate_existing_store` then // `open_inner`). So the advice has to say which one (#6207, #6225). return Err(format!( - "This session's saved Runtime store belongs to a different host. \ + "This session's saved Runtime store belongs to a different host{reason}. \ Switching to it from inside a running session cannot carry that \ store's queued work across, but opening it directly can: run \ `codewhale resume {}` from your shell.", diff --git a/crates/tui/src/tui/ui/tests/runtime_store_binding.rs b/crates/tui/src/tui/ui/tests/runtime_store_binding.rs index 6abb1c1540..701dc22843 100644 --- a/crates/tui/src/tui/ui/tests/runtime_store_binding.rs +++ b/crates/tui/src/tui/ui/tests/runtime_store_binding.rs @@ -644,8 +644,9 @@ fn adoptable_empty_store_reports_nothing_to_abandon() -> anyhow::Result<()> { !binding.is_missing_session_store()?, "the store exists, so the old predicate cannot recover it" ); - assert!( - binding.has_no_durable_work()?, + assert_eq!( + binding.adoption_refusal()?, + None, "a freshly opened store holds nothing to abandon" ); assert!( @@ -675,8 +676,9 @@ fn adoptable_empty_store_reports_nothing_to_abandon() -> anyhow::Result<()> { ] { let marker = store_dir.join(dir).join("work.json"); std::fs::write(&marker, "{}")?; - assert!( - !binding.has_no_durable_work()?, + assert_eq!( + binding.adoption_refusal()?, + Some(crate::runtime_threads::StoreAdoptionRefusal::HasDurableWork { dir }), "{dir} holds work; the store must not be adopted" ); assert!(