diff --git a/crates/repo/src/ws_refresh.rs b/crates/repo/src/ws_refresh.rs index ce366350..cbd9bbe2 100644 --- a/crates/repo/src/ws_refresh.rs +++ b/crates/repo/src/ws_refresh.rs @@ -325,11 +325,23 @@ fn plan_checkout( identity: Option<&scl_crypto::SecretKey>, sparse: &Sparse, cache: &mut ProtectedCache, - resuming: bool, + prior_writers: Option<&[u32]>, ) -> Result { + let resuming = prior_writers.is_some(); // Bounded by the view on both sides: a partial clone does not hold the rest. let old = tree_file_entries_with_perms_sparse(store, old_root, sparse)?; let new = tree_file_entries_with_perms_sparse(store, new_root, sparse)?; + // A killed writer may have left staging files with protected plaintext beside + // the paths it was writing (any path the result changes, skipped ones + // included: the first attempt may have had a key). They go first: the ignore + // comparison below would count one as a file the agent added. + if let Some(writers) = prior_writers { + for (p, _) in new.iter().filter(|(p, e)| old.get(*p) != Some(e)) { + if !under_symlink(root, p) { + sweep_stale_temps(&safe_join(root, p)?, writers); + } + } + } hidden_additions( store, root, @@ -437,7 +449,17 @@ fn plan_checkout( // empty one the refresh did not empty is the user's (as in any // forward op), whose working directory a process may hold. let under = format!("{p}/"); - let emptied = removals.iter().any(|r| r.0.starts_with(&under)); + // On a retry the husk may be the work of the first attempt: the + // candidate tracked files under it, the result drops them all and + // none is left on disk to remove (`dir_clearable` below still + // refuses anything else inside). + let emptied = removals.iter().any(|r| r.0.starts_with(&under)) + || (resuming + && old.iter().any(|(o, e)| { + // A protected file may never have been laid out (no key at + // fork), so it proves nothing about what was on disk. + o.starts_with(&under) && !new.contains_key(o) && e.2 & PROTECTED == 0 + })); if !emptied || !dir_clearable(&full, p, &|c| removable.contains(c)) { return Err(Error::InvalidArgument(format!( "cannot write {p}: a directory with untracked or ignored content \ @@ -897,7 +919,7 @@ impl Repo { identity, &sparse, &mut prep.cache, - prior_pending.is_some(), + prior_pending.as_ref().map(|p| &p.writers[..]), ) }; match plan { @@ -942,23 +964,6 @@ impl Repo { }); } Some(p) => { - // A killed writer may have left staging files with protected - // plaintext: remove those of the pids that wrote before. - for job in &jobs { - // Paths skipped for lack of a key included: the first attempt - // may have had one. - let paths = job - .plan - .writes - .iter() - .map(|w| &w.0) - .chain(&job.plan.skipped); - for path in paths { - if !under_symlink(&job.dir, path) { - sweep_stale_temps(&safe_join(&job.dir, path)?, &p.writers); - } - } - } if !p.writers.contains(&me) { p.writers.push(me); } @@ -1696,6 +1701,95 @@ mod tests { cleanup(repo, &root); } + #[test] + fn a_stale_staging_file_the_new_ignore_rules_expose_is_swept_before_they_are_compared() { + let root = tmp_root("stale-tmp-scignore"); + let repo = Repo::init(&root).unwrap(); + std::fs::write(root.join(".scignore"), "*.tmp\n").unwrap(); + std::fs::write(root.join("a.txt"), LINES).unwrap(); + repo.commit("t", "base").unwrap(); + let s = repo.ws_fork(None, 1, "t", None).unwrap(); + let dir = s.workspaces[0].dir.clone(); + // The tip stops hiding `*.tmp` and adds b.txt; the apply phase dies after + // .scignore, in the middle of b.txt's write. + host_commit( + &repo, + &root, + &[(".scignore", Some("other\n")), ("b.txt", Some("b\n"))], + ); + { + let _g = FailGuard::after(1); + repo.ws_refresh(None, None, None).unwrap_err(); + } + assert_eq!(read(&dir, ".scignore"), "other\n"); + // The killed writer's staging file: hidden by the rules the checkout was + // read under, visible under the new ones. + let stale = dir.join("b.4000000.7.tmp"); + std::fs::write(&stale, "half\n").unwrap(); + let mut session = repo.ws_session(None).unwrap().unwrap(); + session + .pending_refresh + .as_mut() + .unwrap() + .writers + .push(4_000_000); + write_manifest(repo.layout(), &session).unwrap(); + repo.ws_refresh(None, None, None).unwrap(); + assert!(!stale.exists()); + assert_eq!(read(&dir, "b.txt"), "b\n"); + cleanup(repo, &root); + } + + #[test] + fn a_retry_replaces_the_empty_directory_its_own_removals_left() { + let (repo, root) = init("dir-replaced"); + host_commit(&repo, &root, &[("dir/file", Some("f\n"))]); + let s = repo.ws_fork(None, 1, "t", None).unwrap(); + let dir = s.workspaces[0].dir.clone(); + // The host replaces the directory with a file. + std::fs::remove_dir_all(root.join("dir")).unwrap(); + std::fs::write(root.join("dir"), "now a file\n").unwrap(); + let tip = repo.commit("t", "host").unwrap(); + { + // dir/file is removed, then the write of `dir` dies. + let _g = FailGuard::after(1); + repo.ws_refresh(None, None, None).unwrap_err(); + } + assert!(dir.join("dir").is_dir() && !dir.join("dir/file").exists()); + // Something of the user's in the husk is not ours to clear. + std::fs::write(dir.join("dir/mine"), "mine\n").unwrap(); + let err = repo.ws_refresh(None, None, None).unwrap_err().to_string(); + assert!(err.contains("cannot write dir"), "{err}"); + assert_eq!(read(&dir, "dir/mine"), "mine\n"); + std::fs::remove_file(dir.join("dir/mine")).unwrap(); + repo.ws_refresh(None, None, None).unwrap(); + assert_eq!(read(&dir, "dir"), "now a file\n"); + assert_eq!(base_of(&repo, None), tip); + cleanup(repo, &root); + } + + #[test] + fn a_retry_still_refuses_a_users_empty_directory_the_candidate_never_tracked() { + let (repo, root) = init("dir-users-empty"); + let s = repo.ws_fork(None, 1, "t", None).unwrap(); + let dir = s.workspaces[0].dir.clone(); + host_commit( + &repo, + &root, + &[("b.txt", Some("b\n")), ("mine", Some("a file\n"))], + ); + { + // b.txt is written, then the write of `mine` dies. + let _g = FailGuard::after(1); + repo.ws_refresh(None, None, None).unwrap_err(); + } + std::fs::create_dir(dir.join("mine")).unwrap(); + let err = repo.ws_refresh(None, None, None).unwrap_err().to_string(); + assert!(err.contains("cannot write mine"), "{err}"); + assert!(dir.join("mine").is_dir()); + cleanup(repo, &root); + } + #[test] fn a_convergent_protected_file_edited_after_the_plan_is_not_overwritten() { let (repo, root) = init("late-convergent"); diff --git a/docs/adr/0056-named-concurrent-ws-sessions.md b/docs/adr/0056-named-concurrent-ws-sessions.md index 50cebe51..5fb5d905 100644 --- a/docs/adr/0056-named-concurrent-ws-sessions.md +++ b/docs/adr/0056-named-concurrent-ws-sessions.md @@ -56,11 +56,11 @@ Reading the code turned up one fact that shapes the design. `Repo::open` takes t 7. **Crash behaviour.** Write order: a `pending_refresh` marker in the manifest (durable, before the first file), every checkout, then the manifest with the new base and no marker. The marker records the target tip and its branch and, per workspace, two snapshots: the candidate (the checkout as the refresh read it, parent the old base) and the result (the candidate replayed onto the target; a snapshot so its wrap lists come with it). All of them are gc roots. If the first file operation is refused, the marker is cleared again (or, on a retry, left as it was). In-place re-materialization is not atomic across files, and no in-place scheme can be. A crash leaves the old base, the marker, and each checkout a mix of files at their pre-refresh state and at their result state. - A mix cannot be re-read as a fresh checkout. A merged file (the agent's line 4 and the host's line 5, say) diffed against the old base fuses the two edits into one hunk that overlaps the tip's, and conflicts with the very tip it was merged with; and a harvest would read the refreshed files as the agent's own work, re-applying a part of the tip the host has since reverted, or diverging on a refreshed protected file that has no cache proof against the old base. So while the marker is set, `sc ws run` and `sc ws harvest` refuse ("run `sc ws refresh` to finish it"), `sc ws list` says so, and abandon still works. `sc ws refresh` run again does not recompute anything from the checkouts: it re-plans from the recorded pairs, over the same candidate and result, and accepts at each path either the bytes the candidate read or the bytes the result holds (a plain file by bytes; a protected file by decrypting the result's blob and comparing plaintext when a key is given, which needs no record, else a randomized one by the workspace cache and a convergent one by re-encrypting; the same holds for what the candidate read, opened with the wrap lists of the candidate snapshot, so the agent's re-sealed edit of a protected file is recognised even when the cache on disk predates the refresh; the caches are also saved before the marker is written and right after each protected write, because a kill never reaches a later save). Without a key a retry that finds a protected file it cannot verify refuses with "pass --identity (an unfinished refresh needs the key to be resumed)". A path already holding the result is left alone, including a file the first attempt added, so an interruption is resumed to the same end state; an edit made to a file the first attempt wrote or was about to write is neither of the two and still refuses. Only then does it move on to the requested tip, as a fresh refresh from the finished state, so a host revert in between is honoured. A protected path the first attempt had not yet reached still needs `--identity`. The target branch is part of the marker, so the unfinished step is labelled as the branch it was moving to. + A mix cannot be re-read as a fresh checkout. A merged file (the agent's line 4 and the host's line 5, say) diffed against the old base fuses the two edits into one hunk that overlaps the tip's, and conflicts with the very tip it was merged with; and a harvest would read the refreshed files as the agent's own work, re-applying a part of the tip the host has since reverted, or diverging on a refreshed protected file that has no cache proof against the old base. So while the marker is set, `sc ws run` and `sc ws harvest` refuse ("run `sc ws refresh` to finish it"), `sc ws list` says so, and abandon still works. `sc ws refresh` run again does not recompute anything from the checkouts: it re-plans from the recorded pairs, over the same candidate and result, and accepts at each path either the bytes the candidate read or the bytes the result holds (a plain file by bytes; a protected file by decrypting the result's blob and comparing plaintext when a key is given, which needs no record, else a randomized one by the workspace cache and a convergent one by re-encrypting; the same holds for what the candidate read, opened with the wrap lists of the candidate snapshot, so the agent's re-sealed edit of a protected file is recognised even when the cache on disk predates the refresh; the caches are also saved before the marker is written and right after each protected write, because a kill never reaches a later save). Without a key a retry that finds a protected file it cannot verify refuses with "pass --identity (an unfinished refresh needs the key to be resumed)". A path already holding the result is left alone, including a file the first attempt added, so an interruption is resumed to the same end state (a directory the first attempt emptied, because the result replaces `dir/file` with a file `dir` and the crash came between the removals and the write, is replaced on the retry: on a retry an empty directory counts as the refresh's own when the candidate tracked files under it that the result drops; a directory with anything of the user's in it still refuses, and a protected file the candidate tracked does not count, since it may never have been laid out, #275); an edit made to a file the first attempt wrote or was about to write is neither of the two and still refuses. Only then does it move on to the requested tip, as a fresh refresh from the finished state, so a host revert in between is honoured. A protected path the first attempt had not yet reached still needs `--identity`. The target branch is part of the marker, so the unfinished step is labelled as the branch it was moving to. - `atomic_write_durable` stages each write in `...tmp` beside the file; a writer killed mid-write leaves one, possibly holding protected plaintext, where a harvest would take it for the agent's file. The marker records the pid of every process that wrote for the refresh (the first attempt and each retry); the retry removes files of exactly that name, for the paths it is about to write, under one of those pids once it is provably dead, and nothing else. The alternative that leaves the old checkout whole (build the new one in a sibling directory and swap) was rejected: it drops untracked and ignored files, breaks any open handle or working directory, and is no more atomic in the face of a crash during the swap. While the marker is set the manifest's `base_snapshot` holds the text `refresh-unfinished` (the real base is in the marker), so a binary from before this change fails to parse the manifest instead of harvesting a half-refreshed checkout as clean, and so does its gc. + `atomic_write_durable` stages each write in `...tmp` beside the file; a writer killed mid-write leaves one, possibly holding protected plaintext, where a harvest would take it for the agent's file. The marker records the pid of every process that wrote for the refresh (the first attempt and each retry); the retry removes files of exactly that name, for every path the result changes (skipped ones included), under one of those pids once it is provably dead, and nothing else, and does so before the `.scignore` comparison, so a refresh that turned `*.tmp` from hidden to visible does not count its own staging file as a file the agent added (#275). The sweep runs while the retry is planned, so a retry that then refuses may already have removed those dead writers' files; nothing else is touched. The alternative that leaves the old checkout whole (build the new one in a sibling directory and swap) was rejected: it drops untracked and ignored files, breaks any open handle or working directory, and is no more atomic in the face of a crash during the swap. While the marker is set the manifest's `base_snapshot` holds the text `refresh-unfinished` (the real base is in the marker), so a binary from before this change fails to parse the manifest instead of harvesting a half-refreshed checkout as clean, and so does its gc. -Known limits: `sc ws run --with-secrets` resolves the secret environment from the host's checked-out branch, as it did before refresh existed, not from the session's base; on a case-insensitive filesystem a target that renames a tracked path only by case is refused (the old spelling is found on disk but not tracked under the new one; fail closed, as the other untracked-file guards are); the marker records no per-file progress, so a file the interrupted refresh added and the agent then deleted before the retry comes back (the retry cannot tell its own earlier write from a file the result still owes); deletions and the pruning of emptied directories are not fsynced before the manifest is written, and a file's mode is applied just after its rename, so a power loss (not a process crash) can leave a refreshed file briefly without its mode or a deleted file back, both of which the retry redoes from its record. When a retry finishes an older target first and the requested one is then refused, the older target's progress is kept but not reported. A mode-only edit in a workspace is not seen as a change (harvest's diff compares content only, `diff_worktree`), so where the new tip also changes that file's content the tip's mode wins, as a harvest would drop the mode edit. Not built: refreshing one workspace by index. +Known limits: `sc ws run --with-secrets` resolves the secret environment from the host's checked-out branch, as it did before refresh existed, not from the session's base; on a case-insensitive filesystem a target that renames a tracked path only by case is refused (the old spelling is found on disk but not tracked under the new one; fail closed, as the other untracked-file guards are); the marker records no per-file progress, so a file the interrupted refresh added and the agent then deleted before the retry comes back (the retry cannot tell its own earlier write from a file the result still owes); for the same reason, if a crash came before the removals, a user who then deletes the tracked files under `dir` and puts an empty directory of their own at `dir` loses that empty directory to the retry, which takes it for the husk of its own removals (no content is ever lost); deletions and the pruning of emptied directories are not fsynced before the manifest is written, and a file's mode is applied just after its rename, so a power loss (not a process crash) can leave a refreshed file briefly without its mode or a deleted file back, both of which the retry redoes from its record. When a retry finishes an older target first and the requested one is then refused, the older target's progress is kept but not reported. A mode-only edit in a workspace is not seen as a change (harvest's diff compares content only, `diff_worktree`), so where the new tip also changes that file's content the tip's mode wins, as a harvest would drop the mode edit. Not built: refreshing one workspace by index. Tests: `ws_refresh.rs` (no edits; already at the tip; uncommitted edits and a same-file merge, then a harvest that lands as a fast-forward of the new tip; host and workspace deletions; a conflict refuses everything and harvest is unchanged; a running command; two named sessions refresh independently; `--onto`; an ignored file survives and a user file in the way refuses; the scanner; protected paths need `--identity`, the cache keeps an untouched workspace unchanged and an edited protected file changed; a narrow recorded view; a file edited after the plan (plain and protected) is not overwritten and leaves nothing pending; an injected mid-apply failure blocks harvest, can be abandoned, can be run again, and finishes onto the old target after the host reverted part of it; the same with a protected file that needs the key; emptied directories go), `partial_sessions.rs` (a partial clone: the view is refreshed and `docs/` is carried and never fetched; a gap refuses before any write), `crates/cli/tests/ws_sessions.rs` and `demo/run_ws_demo.sh` section 10.