mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-11 01:51:34 -10:00
fix(tui): adopt an existing Runtime store that holds no work (#6207)
Opening a saved session could fail with "This session belongs to another Runtime host" while the store it names was sitting on disk, validating, and held by nobody: `validate_existing_store` accepted it, the process-owner lock was free, and no other Codewhale process was running. The predicate was not wrong, it was incomplete. The switch path can rebind a conversation but cannot carry a store's durable work into the running host — queued tasks, pending approvals, agent mail, scope-pinned automations — so recovery was scoped to "there is nothing on disk to abandon", i.e. a *missing* store. That policy has no case for the state a force-quit actually leaves: a store that exists, is ownerless, and is empty. Refusing it protected nothing and made the session unopenable. `RuntimeStoreBinding::has_no_durable_work` adds that case. A store qualifies only when it is confined to `<state>/sessions/<id>/runtime[-recovered-*]` with no symlink on the path, every work directory is empty, and `state.json`'s sequence is still at its initial value — because events can be appended and later pruned, and the sequence remembers. Anything unreadable, unconfined or non-empty fails closed, so the guarantee the guard exists to protect is unchanged; the seven pre-existing tests that pin it still pass untouched. Deliberately NOT used as the predicate: lock-freeness. A crashed host leaves a store unlocked whether or not it holds work, so adopting on "the owner lock is free" would be a policy reversal that silently drops queued work, not a bug fix. Emptiness is the discriminator that fixes the reported case without weakening anything. The refusal message was also describing the wrong thing — it claimed another host owned the session, when what it means is that the saved store holds work this process cannot carry across. It now says that. Confinement checks are now shared by both predicates via `is_confined_session_store` rather than duplicated. Possibly also #6185(a): an empty transcript on resume with an intact journal is what every early return in `apply_loaded_session_with_goal` produces, and this guard is one of them. Asked the reporter there to confirm the status-line text before claiming it. Checks: `./scripts/dev-test.sh tui` — 12708 tests run, 12708 passed, 19 skipped. `./scripts/dev-test.sh tui runtime_store_binding` — 8 passed, including the new `empty_existing_runtime_store_reports_no_durable_work`, which asserts each work directory is load-bearing on its own so a directory added to `open` without being added to the list is caught. CI clippy gate exits 0. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
1 parent
0d0a1196e3
commit
68431ba097
3 files changed
+159
-6
No files matched your search
@@ -84,6 +84,18 @@ const EVENT_TRANSACTION_LOCK_FILE: &str = "events.lock";
|
||||
const RUNTIME_PROCESS_OWNER_LOCK_FILE: &str = "runtime-process.owner.lock";
|
||||
const RUNTIME_PROCESS_OWNER_LOCK_HELD: &str = "This runtime is already active in another process. Close the other Codewhale session and try again, or set CODEWHALE_RUNTIME_DIR to a different directory.";
|
||||
const AGENT_MAIL_OWNER_FILE: &str = "owner.json";
|
||||
/// Every directory `RuntimeThreadStore::open` creates to hold work. Emptiness
|
||||
/// across all of them is what lets a switch adopt an existing store (#6207);
|
||||
/// `fresh_runtime_store_reports_no_durable_work` pins this list against `open`.
|
||||
const RUNTIME_STORE_WORK_DIRS: [&str; 7] = [
|
||||
"threads",
|
||||
"turns",
|
||||
"items",
|
||||
"events",
|
||||
"goals",
|
||||
"agent-mail",
|
||||
"turn-operations",
|
||||
];
|
||||
const TURN_OPERATION_BINDING_SCHEMA_VERSION: u32 = 1;
|
||||
const REQUEST_USER_INPUT_TOOL_NAME: &str = "request_user_input";
|
||||
const REDACTED_USER_INPUT_RECEIPT: &str = "User input submitted";
|
||||
@@ -2886,9 +2898,11 @@ pub struct RuntimeStoreBinding {
|
||||
}
|
||||
|
||||
impl RuntimeStoreBinding {
|
||||
/// Only a missing, confined session store can recover from its transcript.
|
||||
/// Existing stores with a wrong owner, symlinks and external paths fail closed.
|
||||
pub(crate) fn is_missing_session_store(&self) -> Result<bool> {
|
||||
/// The confinement shared by every store-recovery predicate: the bound
|
||||
/// store must sit at `<state>/sessions/<session-id>/runtime` (or a
|
||||
/// `runtime-recovered-*` sibling) with no symlink on the way down. Wrong
|
||||
/// owner, symlinks and external paths fail closed for all callers.
|
||||
fn is_confined_session_store(&self) -> Result<bool> {
|
||||
let sessions = codewhale_config::resolve_state_dir("sessions")?;
|
||||
let Some(session_dir) = self.data_dir.parent() else {
|
||||
return Ok(false);
|
||||
@@ -2908,6 +2922,15 @@ impl RuntimeStoreBinding {
|
||||
for path in [&sessions, session_dir, &self.data_dir] {
|
||||
reject_symlinked_store_dir(path)?;
|
||||
}
|
||||
Ok(true)
|
||||
}
|
||||
|
||||
/// Only a missing, confined session store can recover from its transcript.
|
||||
/// Existing stores with a wrong owner, symlinks and external paths fail closed.
|
||||
pub(crate) fn is_missing_session_store(&self) -> Result<bool> {
|
||||
if !self.is_confined_session_store()? {
|
||||
return Ok(false);
|
||||
}
|
||||
match fs::symlink_metadata(&self.data_dir) {
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(true),
|
||||
Err(err) => Err(err.into()),
|
||||
@@ -2915,6 +2938,50 @@ impl RuntimeStoreBinding {
|
||||
}
|
||||
}
|
||||
|
||||
/// True when a confined store exists but 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,
|
||||
/// scope-pinned automations — which is why only a *missing* store was ever
|
||||
/// allowed to recover. A store that exists and is empty is the case that
|
||||
/// policy never covered: 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.
|
||||
pub(crate) fn has_no_durable_work(&self) -> Result<bool> {
|
||||
if !self.is_confined_session_store()? {
|
||||
return Ok(false);
|
||||
}
|
||||
if !self.data_dir.is_dir() {
|
||||
return Ok(false);
|
||||
}
|
||||
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);
|
||||
}
|
||||
}
|
||||
// A store opened by an older build may predate a directory;
|
||||
// absent is empty.
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
|
||||
Err(err) => return Err(err.into()),
|
||||
}
|
||||
}
|
||||
// A sequence past its initial value means events were appended, even
|
||||
// if those files have since been pruned.
|
||||
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)
|
||||
}
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(true),
|
||||
Err(err) => Err(err.into()),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn validate_existing_store(&self) -> Result<()> {
|
||||
anyhow::ensure!(
|
||||
self.data_dir.is_absolute(),
|
||||
|
||||
@@ -3619,14 +3619,24 @@ pub(crate) fn apply_loaded_session_with_goal(
|
||||
&& let Some(tasks) = app.runtime_services.task_manager.as_ref()
|
||||
&& tasks.session_store_binding().as_ref() != Some(binding)
|
||||
{
|
||||
if binding
|
||||
// A switch can rebind the conversation but cannot carry the saved
|
||||
// store's durable work into the running host, so it may only adopt a
|
||||
// store there is nothing to lose from leaving: one that is missing, or
|
||||
// one that exists and is provably empty. 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
|
||||
.is_missing_session_store()
|
||||
.map_err(|error| error.to_string())?
|
||||
{
|
||||
|| binding
|
||||
.has_no_durable_work()
|
||||
.map_err(|error| error.to_string())?;
|
||||
if nothing_to_abandon {
|
||||
recovered_binding = tasks.session_store_binding();
|
||||
}
|
||||
if recovered_binding.is_none() {
|
||||
return Err("This session belongs to another Runtime host. Resume it in a new Codewhale process to reopen its saved store.".into());
|
||||
return Err("This session's saved Runtime store still holds work — queued tasks, approvals, agent mail or scope-pinned automations — that this process cannot carry across. Resume it in a new Codewhale process to reopen that store.".into());
|
||||
}
|
||||
}
|
||||
if app.session_transition_blocked() {
|
||||
|
||||
@@ -552,3 +552,79 @@ async fn picker_recovers_missing_store_into_the_idle_host_and_persists_before_re
|
||||
tasks.shutdown_and_wait().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// #6207: a store that exists but is empty holds nothing a session switch could
|
||||
/// abandon. A force-quit leaves exactly that shape — the directory is on disk,
|
||||
/// ownerless, with zero events — and refusing it left the session unopenable
|
||||
/// while protecting nothing.
|
||||
#[test]
|
||||
fn empty_existing_runtime_store_reports_no_durable_work() -> anyhow::Result<()> {
|
||||
let _environment = 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");
|
||||
|
||||
let store_dir = root.path().join("sessions/interrupted/runtime");
|
||||
// Open a real store, so the layout under test is the product's rather than
|
||||
// the test's idea of it.
|
||||
drop(crate::runtime_threads::RuntimeThreadStore::open(
|
||||
store_dir.clone(),
|
||||
)?);
|
||||
|
||||
let binding = crate::runtime_threads::RuntimeStoreBinding {
|
||||
data_dir: store_dir.clone(),
|
||||
execution_scope: "0".repeat(64),
|
||||
};
|
||||
assert!(
|
||||
!binding.is_missing_session_store()?,
|
||||
"the store exists, so the old predicate cannot recover it"
|
||||
);
|
||||
assert!(
|
||||
binding.has_no_durable_work()?,
|
||||
"a freshly opened store holds nothing to abandon"
|
||||
);
|
||||
|
||||
// Each work directory must be load-bearing on its own. If `open` gains a
|
||||
// directory that RUNTIME_STORE_WORK_DIRS misses, this is the assertion that
|
||||
// notices, instead of the miss silently widening what a switch will adopt.
|
||||
for dir in [
|
||||
"threads",
|
||||
"turns",
|
||||
"items",
|
||||
"events",
|
||||
"goals",
|
||||
"agent-mail",
|
||||
"turn-operations",
|
||||
] {
|
||||
let marker = store_dir.join(dir).join("work.json");
|
||||
std::fs::write(&marker, "{}")?;
|
||||
assert!(
|
||||
!binding.has_no_durable_work()?,
|
||||
"{dir} holds work; the store must not be adopted"
|
||||
);
|
||||
std::fs::remove_file(&marker)?;
|
||||
assert!(binding.has_no_durable_work()?, "{dir} is empty again");
|
||||
}
|
||||
|
||||
// Events can be appended and later pruned; the sequence still remembers.
|
||||
let state_path = store_dir.join("state.json");
|
||||
let original = std::fs::read_to_string(&state_path)?;
|
||||
std::fs::write(&state_path, r#"{"next_seq":5}"#)?;
|
||||
assert!(
|
||||
!binding.has_no_durable_work()?,
|
||||
"an advanced sequence means events were appended"
|
||||
);
|
||||
std::fs::write(&state_path, &original)?;
|
||||
|
||||
// Confinement still governs: an unconfined path fails closed either way.
|
||||
let foreign = crate::runtime_threads::RuntimeStoreBinding {
|
||||
data_dir: root.path().join("elsewhere/runtime"),
|
||||
execution_scope: "0".repeat(64),
|
||||
};
|
||||
assert!(
|
||||
!foreign.has_no_durable_work()?,
|
||||
"a store outside the sessions tree must never be adopted"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
Reference in new issue
Block a user