mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-11 01:51:34 -10:00
Revert "fix(tui): adopt an existing Runtime store that holds no work (#6207)"
This reverts commit 68431ba097.
CI caught what my local runs did not: `runtime_store_binding_survives_launch_snapshot_and_resume`
fails at its line 284 assertion on macOS and ubuntu, while passing in isolation
here even under `--all-features`. Passing alone and failing under the full
suite is the signature of a race, and the race is in the predicate I added.
`has_no_durable_work` reads emptiness at one instant. It has no liveness
check, so a store that is empty *right now* because the host holding it has
not flushed yet is indistinguishable from a store that is empty because it was
abandoned. That test has a live foreign TaskManager holding the store; under
load the adopt path wins the race, the switch succeeds where it must refuse,
and the assertion sees a different error.
Two further gaps, both mine:
- The predicate only walks the runtime store's own directories. Scope-pinned
automations are recorded outside them, so an automation whose execution
scope matches the binding is durable work my check cannot see. The review
that scoped this said the predicate needed "no automation whose
execution_scope matches the binding"; I did not implement that.
- I reported that the seven pre-existing tests "pass untouched". That was true
of the run I did and false in general, and the distinction matters precisely
here.
This sits on a data-loss boundary — queued tasks, approvals, agent mail,
automations — so it comes out rather than being patched forward at speed.
#6207 stays open with the corrected design: adopt only when the store is both
provably empty *and* provably unheld, with automation scope included in
"empty", and the shape of the liveness check settled before any of it is
written.
The behaviour returns to refusing, which is the pre-existing bug the issue
reports — a worse user experience than the fix, and a better one than silently
dropping someone's scheduled work.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01NWzjx9Q7Mw2G7K8rpiJy9p
This commit is contained in:
1 parent
68431ba097
commit
bc77a0ff5d
3 files changed
+6
-159
No files matched your search
@@ -84,18 +84,6 @@ 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";
|
||||
@@ -2898,11 +2886,9 @@ pub struct RuntimeStoreBinding {
|
||||
}
|
||||
|
||||
impl RuntimeStoreBinding {
|
||||
/// 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> {
|
||||
/// 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> {
|
||||
let sessions = codewhale_config::resolve_state_dir("sessions")?;
|
||||
let Some(session_dir) = self.data_dir.parent() else {
|
||||
return Ok(false);
|
||||
@@ -2922,15 +2908,6 @@ 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()),
|
||||
@@ -2938,50 +2915,6 @@ 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,24 +3619,14 @@ 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)
|
||||
{
|
||||
// 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
|
||||
if 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'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());
|
||||
return Err("This session belongs to another Runtime host. Resume it in a new Codewhale process to reopen its saved store.".into());
|
||||
}
|
||||
}
|
||||
if app.session_transition_blocked() {
|
||||
|
||||
@@ -552,79 +552,3 @@ 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