mirror of
https://github.com/alphaXiv/OpenResearch.git
synced 2026-10-02 01:34:34 +08:00
OR-338: Restore vanished chat worktrees at their last checkout (#478)
* OR-338: Restore vanished chat worktrees at their last checkout Session worktrees have been reported disappearing mid-turn (Codex, 0.2.10) with no identified cause. Make recovery lossless for committed work: - When a session worktree is missing but still registered, recreate it on its last branch (or commit) instead of the baseline, and log it. - Replace the global `git worktree prune` with a targeted `worktree remove --force <dir>`, so one session's recovery or deletion no longer erases other vanished sessions' checkouts; prune stays as a last resort when the baseline add still fails. - Demo seeding keeps landing on its exact commit. - Refuse enabling GitHub sync (409) while a busy chat is still on the legacy worktree layout, whose migration would `git worktree move` it mid-turn. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * OR-338: Make legacy migration atomic with turn starts; drop fallback prune Addresses review on #478: - Run the legacy worktree migration inside ChatHost::while_idle, which holds the turn map and refuses if an affected session has a turn or durable lease, so no turn can start between the check and `git worktree move`. Replaces the racy handler-level guard. - Remove the last-resort global `git worktree prune`: an unrelated add failure could erase other vanished sessions' restorable checkouts. Remove an empty leftover dir up front instead, the one case the prune was covering. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
ddb603173b
commit
f336b12152
+19
-7
@@ -1502,7 +1502,7 @@ async fn create_project(
|
||||
.ok_or_else(|| bad_request("project deletion is in progress"))?;
|
||||
drop(create_admission);
|
||||
let (project, github_publication_error) = if github_sync_enabled {
|
||||
match push_project_for_sync(project.clone()).await {
|
||||
match push_project_for_sync(project.clone(), &state.chat).await {
|
||||
Ok((project, _)) => (project, None),
|
||||
Err(error) => {
|
||||
let project = Store::open()?
|
||||
@@ -1659,14 +1659,23 @@ fn github_push_was_rejected(error: &str) -> bool {
|
||||
|
||||
async fn create_independent_project_repository(
|
||||
mut project: local::model::LocalProject,
|
||||
chat: &ChatHost,
|
||||
) -> Result<local::model::LocalProject> {
|
||||
let store = Store::open()?;
|
||||
let session_ids = store
|
||||
let legacy = store
|
||||
.list_chat_sessions_by_project(&project.id)?
|
||||
.into_iter()
|
||||
.map(|session| session.id)
|
||||
.filter(|id| {
|
||||
local::git::existing_session_worktree_path(&project, id)
|
||||
!= local::git::session_worktree_path(&project.id, id)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
local::git::migrate_legacy_project_worktrees(&project, &session_ids)?;
|
||||
// `git worktree move` would pull a running turn's checkout out from under it.
|
||||
chat.while_idle(&legacy, || {
|
||||
local::git::migrate_legacy_project_worktrees(&project, &legacy)
|
||||
})
|
||||
.await?;
|
||||
let source_repository = project
|
||||
.has_github_repository()
|
||||
.then(|| (project.github_owner.clone(), project.github_repo.clone()));
|
||||
@@ -1690,6 +1699,7 @@ async fn create_independent_project_repository(
|
||||
|
||||
async fn push_project_for_sync(
|
||||
mut project: local::model::LocalProject,
|
||||
chat: &ChatHost,
|
||||
) -> Result<(local::model::LocalProject, local::github::Status)> {
|
||||
let github_status = local::github::status().await;
|
||||
if !github_status.installed {
|
||||
@@ -1707,11 +1717,11 @@ async fn push_project_for_sync(
|
||||
.await?
|
||||
.is_some_and(|meta| meta.can_push && !meta.archived);
|
||||
if !can_push {
|
||||
project = create_independent_project_repository(project).await?;
|
||||
project = create_independent_project_repository(project, chat).await?;
|
||||
using_existing_repository = false;
|
||||
}
|
||||
} else {
|
||||
project = create_independent_project_repository(project).await?;
|
||||
project = create_independent_project_repository(project, chat).await?;
|
||||
using_existing_repository = false;
|
||||
}
|
||||
|
||||
@@ -1727,7 +1737,7 @@ async fn push_project_for_sync(
|
||||
if !using_existing_repository || !github_push_was_rejected(&error.to_string()) {
|
||||
return Err(error);
|
||||
}
|
||||
project = create_independent_project_repository(project).await?;
|
||||
project = create_independent_project_repository(project, chat).await?;
|
||||
push_once(&project)
|
||||
.await
|
||||
.map_err(|error| anyhow!("Git push task failed: {error}"))??;
|
||||
@@ -1750,7 +1760,9 @@ async fn enable_project_github(State(state): State<AppState>, Path(id): Path<Str
|
||||
let project = store
|
||||
.get_local_project(&id)?
|
||||
.ok_or_else(|| not_found("project"))?;
|
||||
let (project, github_status) = push_project_for_sync(project).await.map_err(bad_request)?;
|
||||
let (project, github_status) = push_project_for_sync(project, &state.chat)
|
||||
.await
|
||||
.map_err(bad_request)?;
|
||||
let git_status = project_git_json(&project, github_status);
|
||||
Ok(Json(
|
||||
json!({ "project": project_json(&project), "git": git_status }),
|
||||
|
||||
@@ -3615,6 +3615,24 @@ impl ChatHost {
|
||||
self.turns.lock().await.contains_key(session_id)
|
||||
}
|
||||
|
||||
/// Run `f` only if none of `session_ids` has a turn, holding the turn map so none can start
|
||||
/// until it returns.
|
||||
pub async fn while_idle<T>(
|
||||
&self,
|
||||
session_ids: &[String],
|
||||
f: impl FnOnce() -> Result<T>,
|
||||
) -> Result<T> {
|
||||
let turns = self.turns.lock().await;
|
||||
for session_id in session_ids {
|
||||
if turns.contains_key(session_id) || Store::open()?.chat_turn_leased(session_id)? {
|
||||
return Err(anyhow!(
|
||||
"Wait for the running chats to finish, then try again."
|
||||
));
|
||||
}
|
||||
}
|
||||
f()
|
||||
}
|
||||
|
||||
fn claim_durable_turn(&self, session_id: &str) -> bool {
|
||||
let mut claims = self.durable_turns.lock().unwrap();
|
||||
if claims.contains_key(session_id) {
|
||||
@@ -9555,6 +9573,25 @@ mod bridge_tests {
|
||||
)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn while_idle_refuses_a_running_session_and_runs_otherwise() {
|
||||
let host = test_host();
|
||||
host.turns
|
||||
.lock()
|
||||
.await
|
||||
.insert("busy".into(), TurnState::Reserved { turn_id: None });
|
||||
let mut ran = false;
|
||||
assert!(host
|
||||
.while_idle(&["busy".into()], || {
|
||||
ran = true;
|
||||
Ok(())
|
||||
})
|
||||
.await
|
||||
.is_err());
|
||||
assert!(!ran);
|
||||
assert_eq!(host.while_idle(&[], || Ok(7)).await.unwrap(), 7);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn claude_permission_reviews_are_serialized_per_session() {
|
||||
let host = test_host();
|
||||
|
||||
+173
-13
@@ -1272,7 +1272,7 @@ pub fn ensure_session_worktree(
|
||||
)
|
||||
.unwrap_or(&project.baseline_branch);
|
||||
git(Some(repo_path), &["rev-parse", "--verify", start_ref])?;
|
||||
ensure_worktree_from(repo_path, dir, start_ref)
|
||||
ensure_worktree_from(repo_path, dir, start_ref, true)
|
||||
}
|
||||
|
||||
pub(crate) fn ensure_session_worktree_in(
|
||||
@@ -1285,7 +1285,7 @@ pub(crate) fn ensure_session_worktree_in(
|
||||
) -> Result<PathBuf> {
|
||||
let start_ref = super::demo::session_start_ref(repo, owner, repo_name, session_id)
|
||||
.unwrap_or(baseline_branch);
|
||||
ensure_worktree_from(repo, dir.to_path_buf(), start_ref)
|
||||
ensure_worktree_from(repo, dir.to_path_buf(), start_ref, true)
|
||||
}
|
||||
|
||||
pub fn ensure_worktree_at(repo: &Path, dir: &Path, start_ref: &str) -> Result<PathBuf> {
|
||||
@@ -1301,10 +1301,17 @@ pub fn ensure_worktree_at(repo: &Path, dir: &Path, start_ref: &str) -> Result<Pa
|
||||
));
|
||||
}
|
||||
}
|
||||
ensure_worktree_from(repo, dir.to_path_buf(), start_ref)
|
||||
ensure_worktree_from(repo, dir.to_path_buf(), start_ref, false)
|
||||
}
|
||||
|
||||
fn ensure_worktree_from(repo: &Path, dir: PathBuf, start_ref: &str) -> Result<PathBuf> {
|
||||
/// `restore_lost` recreates a vanished but still-registered worktree at its last
|
||||
/// checkout instead of `start_ref`, so an agent resumes its own work.
|
||||
fn ensure_worktree_from(
|
||||
repo: &Path,
|
||||
dir: PathBuf,
|
||||
start_ref: &str,
|
||||
restore_lost: bool,
|
||||
) -> Result<PathBuf> {
|
||||
if dir.join(".git").exists() {
|
||||
if git(Some(&dir), &["rev-parse", "--is-inside-work-tree"]).is_ok() {
|
||||
return Ok(dir);
|
||||
@@ -1314,14 +1321,36 @@ fn ensure_worktree_from(repo: &Path, dir: PathBuf, start_ref: &str) -> Result<Pa
|
||||
dir.display()
|
||||
));
|
||||
}
|
||||
// A manually deleted worktree dir leaves a stale registration behind that
|
||||
// would make `worktree add` at the same path fail.
|
||||
let _ = git(Some(repo), &["worktree", "prune"]);
|
||||
// Create the parent first so git and lost_worktree_head resolve symlinks to the registered path.
|
||||
if let Some(parent) = dir.parent() {
|
||||
std::fs::create_dir_all(parent)
|
||||
.map_err(|e| anyhow!("Could not create {}: {}", parent.display(), e))?;
|
||||
}
|
||||
// An empty dir left at the path would make git refuse to treat the worktree as missing.
|
||||
let _ = std::fs::remove_dir(&dir);
|
||||
let target = dir.to_string_lossy().to_string();
|
||||
// Read the lost checkout before the `worktree remove` below erases its registration.
|
||||
let lost = if restore_lost {
|
||||
lost_worktree_head(repo, &dir)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
// A manually deleted worktree dir leaves a stale registration that blocks `worktree add`
|
||||
// here; a prune would also erase other vanished sessions' checkouts before they restore.
|
||||
let _ = git(Some(repo), &["worktree", "remove", "--force", &target]);
|
||||
if let Some(head) = lost {
|
||||
// A commit sha checks out detached; a branch name checks out the branch.
|
||||
match git(Some(repo), &["worktree", "add", &target, &head]) {
|
||||
Ok(_) => {
|
||||
eprintln!("orx: restored missing worktree {} at {head}", dir.display());
|
||||
return Ok(dir);
|
||||
}
|
||||
Err(err) => eprintln!(
|
||||
"orx: could not restore missing worktree {} at {head}: {err}",
|
||||
dir.display()
|
||||
),
|
||||
}
|
||||
}
|
||||
git(
|
||||
Some(repo),
|
||||
&["worktree", "add", "--detach", &target, start_ref],
|
||||
@@ -1329,24 +1358,49 @@ fn ensure_worktree_from(repo: &Path, dir: PathBuf, start_ref: &str) -> Result<Pa
|
||||
Ok(dir)
|
||||
}
|
||||
|
||||
/// What a still-registered worktree at `dir` had checked out: its branch, else its commit.
|
||||
fn lost_worktree_head(repo: &Path, dir: &Path) -> Option<String> {
|
||||
let canonical_dir = dir
|
||||
.parent()
|
||||
.and_then(|parent| crate::paths::canonicalize(parent).ok())
|
||||
.zip(dir.file_name())
|
||||
.map(|(parent, name)| parent.join(name));
|
||||
let list = git(Some(repo), &["worktree", "list", "--porcelain"]).ok()?;
|
||||
list.split("\n\n").find_map(|entry| {
|
||||
let mut path = None;
|
||||
let mut sha = None;
|
||||
let mut branch = None;
|
||||
for line in entry.lines() {
|
||||
if let Some(value) = line.strip_prefix("worktree ") {
|
||||
path = Some(Path::new(value));
|
||||
} else if let Some(value) = line.strip_prefix("HEAD ") {
|
||||
sha = Some(value);
|
||||
} else if let Some(value) = line.strip_prefix("branch refs/heads/") {
|
||||
branch = Some(value);
|
||||
}
|
||||
}
|
||||
let path = path?;
|
||||
if path != dir && Some(path) != canonical_dir.as_deref() {
|
||||
return None;
|
||||
}
|
||||
branch.or(sha).map(str::to_string)
|
||||
})
|
||||
}
|
||||
|
||||
/// Remove a session's worktree (on session/project delete). Uncommitted
|
||||
/// scratch is discarded deliberately — real work is committed per the
|
||||
/// playbook contract. Best-effort: cleanup must never block the delete.
|
||||
pub fn remove_session_worktree(project: &crate::local::model::LocalProject, session_id: &str) {
|
||||
let repo_path = Path::new(&project.repo_path);
|
||||
let dir = existing_session_worktree_path(project, session_id);
|
||||
if !dir.exists() {
|
||||
return;
|
||||
}
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
// Once the dir is gone, `worktree remove` drops only this registration (unlike prune).
|
||||
if is_repository(repo_path) {
|
||||
let _ = git(
|
||||
Some(repo_path),
|
||||
&["worktree", "remove", "--force", &dir.to_string_lossy()],
|
||||
);
|
||||
let _ = git(Some(repo_path), &["worktree", "prune"]);
|
||||
}
|
||||
// Hub gone (cache wiped) or `worktree remove` refused: take the dir anyway.
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Seed a fresh (empty) GitHub repo from the tip of another repo — the
|
||||
@@ -2625,4 +2679,110 @@ mod tests {
|
||||
);
|
||||
assert!(public_clone_history_args(false).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vanished_worktree_is_restored_on_its_branch() {
|
||||
let hub = temp_repo();
|
||||
let dir = hub.with_extension("session");
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
run(&dir, &["switch", "-q", "-c", "exp/square"]);
|
||||
write(&dir, "result.txt", "42\n");
|
||||
run(&dir, &["add", "-A"]);
|
||||
run(&dir, &["commit", "-q", "-m", "result"]);
|
||||
std::fs::remove_dir_all(&dir).unwrap();
|
||||
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
assert_eq!(run(&dir, &["branch", "--show-current"]), "exp/square");
|
||||
assert!(dir.join("result.txt").is_file());
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let _ = std::fs::remove_dir_all(&hub);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vanished_detached_worktree_is_restored_at_its_commit() {
|
||||
let hub = temp_repo();
|
||||
let dir = hub.with_extension("session");
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
write(&dir, "scratch.txt", "draft\n");
|
||||
run(&dir, &["add", "-A"]);
|
||||
run(&dir, &["commit", "-q", "-m", "detached work"]);
|
||||
let head = run(&dir, &["rev-parse", "HEAD"]);
|
||||
std::fs::remove_dir_all(&dir).unwrap();
|
||||
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
assert_eq!(run(&dir, &["rev-parse", "HEAD"]), head);
|
||||
assert_eq!(run(&dir, &["branch", "--show-current"]), "");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let _ = std::fs::remove_dir_all(&hub);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn seeded_worktree_ignores_a_vanished_checkout() {
|
||||
let hub = temp_repo();
|
||||
let dir = hub.with_extension("seeded");
|
||||
let seed = run(&hub, &["rev-parse", "main"]);
|
||||
ensure_worktree_at(&hub, &dir, &seed).unwrap();
|
||||
run(&dir, &["switch", "-q", "-c", "exp/other"]);
|
||||
std::fs::remove_dir_all(&dir).unwrap();
|
||||
|
||||
ensure_worktree_at(&hub, &dir, &seed).unwrap();
|
||||
assert_eq!(run(&dir, &["rev-parse", "HEAD"]), seed);
|
||||
assert_eq!(run(&dir, &["branch", "--show-current"]), "");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let _ = std::fs::remove_dir_all(&hub);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn restoring_one_vanished_worktree_keeps_another_restorable() {
|
||||
let hub = temp_repo();
|
||||
let first = hub.with_extension("first");
|
||||
let second = hub.with_extension("second");
|
||||
for (dir, branch) in [(&first, "exp/first"), (&second, "exp/second")] {
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
run(dir, &["switch", "-q", "-c", branch]);
|
||||
std::fs::remove_dir_all(dir).unwrap();
|
||||
}
|
||||
|
||||
ensure_worktree_from(&hub, first.clone(), "main", true).unwrap();
|
||||
ensure_worktree_from(&hub, second.clone(), "main", true).unwrap();
|
||||
assert_eq!(run(&second, &["branch", "--show-current"]), "exp/second");
|
||||
for dir in [&first, &second, &hub] {
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn vanished_worktree_under_a_symlink_is_restored_without_its_parent() {
|
||||
let hub = temp_repo();
|
||||
let real = hub.with_extension("real");
|
||||
let link = hub.with_extension("link");
|
||||
std::fs::create_dir_all(&real).unwrap();
|
||||
std::os::unix::fs::symlink(&real, &link).unwrap();
|
||||
let dir = link.join("project").join("session");
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
run(&dir, &["switch", "-q", "-c", "exp/linked"]);
|
||||
std::fs::remove_dir_all(real.join("project")).unwrap();
|
||||
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
assert_eq!(run(&dir, &["branch", "--show-current"]), "exp/linked");
|
||||
for path in [&link, &real, &hub] {
|
||||
let _ = std::fs::remove_dir_all(path);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vanished_worktree_behind_an_empty_dir_is_restored() {
|
||||
let hub = temp_repo();
|
||||
let dir = hub.with_extension("session");
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
run(&dir, &["switch", "-q", "-c", "exp/empty"]);
|
||||
std::fs::remove_dir_all(&dir).unwrap();
|
||||
std::fs::create_dir(&dir).unwrap();
|
||||
|
||||
ensure_worktree_from(&hub, dir.clone(), "main", true).unwrap();
|
||||
assert_eq!(run(&dir, &["branch", "--show-current"]), "exp/empty");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
let _ = std::fs::remove_dir_all(&hub);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user