From 30452552fc3cfd16e8ab979434afb227e3d22c02 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 18:28:01 +0000 Subject: [PATCH 1/3] A field change no longer crosses the metadata seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MetadataStorage` offered `get_*` and `add_*` and no update, so a caller with one field to move had to read the record, clone it, set the field and hand the whole clone back. The clone is taken before the lock exists and `add_*` inserts it over the map `exclusive` has just reloaded, so the write carries this process's copy of every *other* field over whatever another dl run had written to them. Nothing is lost through that today, and this fixes no bug. #400 §4 enumerated every field a second process can move while one of these two sites holds its stale copy: the only concurrently-writable field of `BaseRepository` is `worktrees`, which nothing reads, and the revertible fields of `WorktreeInfo` are either unread or derived byte-identically by the only other writer of that key. What changes here is that the argument stops having to be reassembled out of six files before the interface can be trusted. `update_repository` and `update_worktree` run an edit closure on the record `exclusive` loaded, and save only if the key was there. Both are shaped after `commit_migration`, which has reloaded under the lock and handed the reloaded records to a closure since the v1 to v2 migration was written; a second mechanism beside it would have been the wrong answer. `RecordUpdate::{Applied, Absent}` is the return, so "no such record" is an arm a caller can see rather than a silent insert that undoes another run's delete. Two sites converted: the sweeper's `last_fetched` stamp in repo_manager, and `--reconcile`'s `devpod_workspace_id` in lifecycle. The first also stops consulting the stale map to decide whether there is a record at all, since the key is now checked under the lock. Two left alone, deliberately. migration.rs already runs inside `commit_migration`'s closure on reloaded records, so it is not a cross-seam read-modify-write. workspace_clone.rs reads nothing — `WorktreeInfo::new` builds the record out of the launch's own inputs — and wholesale registration is a different intent from moving a field, so it keeps calling `add_worktree`. Three record fields narrowed to the domain module now that no flows-layer site builds a record it did not read: `BaseRepository::worktrees` and `WorktreeInfo::{created_at, last_used}`. The two `BaseRepository` literals in repo_manager both invented `worktrees: Vec::new()`; they go through `BaseRepository::new` now, which stops being dead code outside tests. The other seven fields stay `pub(crate)`: each has a production reader in flows, and Rust has no visibility that separates reading a field from writing it, so narrowing them means ten accessors and sixty-odd rewritten reads for no modelling gain. Rejected: folding `devpod_workspace_id` into `WorktreeInfo::new` so that field could narrow too. `new` would then always write `Some`, and the reconcile tests need a record without one — which is exactly the pre-#88 record `--reconcile` exists to adopt. Two tests at the metadata seam, red before the closures existed. Not the two-process race #400 first proposed: that one passes on today's code, for the reason in §4, and would have left the next reader believing a race was closed that was never open. flock is per open file description, so a second `MetadataStorage` over the same path is a genuine second writer inside one test. --- rust/devlaunch-core/src/domain/metadata.rs | 147 +++++++++++++++++- rust/devlaunch-core/src/domain/model.rs | 26 +++- rust/devlaunch-core/src/flows/lifecycle.rs | 18 ++- rust/devlaunch-core/src/flows/repo_manager.rs | 50 +++--- .../src/flows/workspace_clone.rs | 9 +- 5 files changed, 206 insertions(+), 44 deletions(-) diff --git a/rust/devlaunch-core/src/domain/metadata.rs b/rust/devlaunch-core/src/domain/metadata.rs index 92dcac27..0e04fbb9 100644 --- a/rust/devlaunch-core/src/domain/metadata.rs +++ b/rust/devlaunch-core/src/domain/metadata.rs @@ -290,6 +290,21 @@ pub(crate) enum MigrationCommit { AlreadyCurrent, } +/// Whether an update found the record it names, once the lock was held. +/// +/// The reload under the lock can show the record gone, because another dl run +/// removed it while this one was holding its copy. There is then nothing to +/// edit, and inserting the record back would turn that run's delete into a row +/// naming a clone directory that no longer exists. Saying so is what lets a +/// caller tell "changed" from "there was nothing there". +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum RecordUpdate { + /// The edit ran on the reloaded record and the store was saved. + Applied, + /// No such record when the lock was taken; nothing was written. + Absent, +} + /// Which worktrees a listing asks for. /// /// A sum type rather than Python's two optional arguments, where "a repo with no @@ -470,6 +485,42 @@ impl MetadataStorage { .map(|((), notices)| notices) } + /// Change a repository in place, editing the record loaded under the lock. + /// + /// The counterpart to [`MetadataStorage::add_repository`], and the one to + /// reach for when a caller has a field to move rather than a whole record + /// to register. `add_*` takes the record by value, so a caller with one + /// field to change had to read it, clone it and hand the clone back — and + /// that clone was taken before the lock existed, so it carried this + /// process's copy of every *other* field over whatever a concurrent dl run + /// had written to them. The edit crossed the seam; this keeps it inside, + /// where the reload has already happened. + /// + /// Shaped after [`MetadataStorage::commit_migration`], which has reloaded + /// under the lock and handed the reloaded records to an edit closure since + /// the v1 to v2 migration was written. Same mechanism, one record instead + /// of all of them. + /// + /// Nothing is written when the key is absent (#412): a store that inserted + /// would undo a delete another run had just committed. Like every mutator + /// here it is not reentrant — `edit` must not call one. + pub(crate) fn update_repository( + &mut self, + owner: &str, + repo: &str, + edit: impl FnOnce(&mut BaseRepository), + ) -> Result<(RecordUpdate, Vec), MetadataError> { + let key = repository_key(owner, repo); + self.exclusive(move |storage| { + let Some(recorded) = storage.repositories.get_mut(&key) else { + return Ok(RecordUpdate::Absent); + }; + edit(recorded); + storage.save()?; + Ok(RecordUpdate::Applied) + }) + } + /// Remove a repository, writing only if it was there. /// /// Held for the #251 §7 public-API freeze — what the `remove` verb writes. @@ -514,6 +565,29 @@ impl MetadataStorage { .map(|((), notices)| notices) } + /// Change a worktree in place, editing the record loaded under the lock. + /// + /// [`MetadataStorage::update_repository`] carries the argument for both. + /// The branch list on the repository is not touched, because an edit that + /// cannot move the key cannot put the two out of step. + pub(crate) fn update_worktree( + &mut self, + owner: &str, + repo: &str, + branch: &str, + edit: impl FnOnce(&mut WorktreeInfo), + ) -> Result<(RecordUpdate, Vec), MetadataError> { + let key = worktree_key(owner, repo, branch); + self.exclusive(move |storage| { + let Some(recorded) = storage.worktrees.get_mut(&key) else { + return Ok(RecordUpdate::Absent); + }; + edit(recorded); + storage.save()?; + Ok(RecordUpdate::Applied) + }) + } + /// Remove a worktree and its entry in the repository's branch list. pub(crate) fn remove_worktree( &mut self, @@ -1115,7 +1189,7 @@ mod tests { //! makes "byte-compatible with Python" an assertion rather than a claim. use super::*; - use crate::domain::model::Timestamp; + use crate::domain::model::{RecordedDefaultBranch, Timestamp}; use jiff::civil; use serde_json::json; @@ -2461,6 +2535,77 @@ mod tests { assert_eq!(keys, vec!["owner1/first", "owner2/second"]); } + #[test] + fn an_update_keeps_the_field_another_writer_moved_since_this_store_loaded() { + // The staleness is the load-to-write distance and not the read-to-write + // one: this store's copy of the record dates from whenever it opened, + // and another dl run has moved a different field of that record since. + // flock is per open file description (see [`super::locks`]), so a + // second store over the same file is a genuine second writer. + let dir = temp_dir(); + let mut seeded = quiet_storage(dir.path()); + seeded + .add_repository(repository("owner1", "repo1")) + .expect("saved"); + + let mut updating = quiet_storage(dir.path()); + let mut other = quiet_storage(dir.path()); + let mut moved = repository("owner1", "repo1"); + moved.default_branch = RecordedDefaultBranch::Named("develop".to_owned()); + other.add_repository(moved).expect("saved"); + + let fetched = Timestamp::from_civil(civil::datetime(2026, 8, 24, 9, 0, 0, 0)); + let (update, _) = updating + .update_repository("owner1", "repo1", |recorded| { + recorded.last_fetched = Some(fetched.clone()); + }) + .expect("saved"); + + assert_eq!(update, RecordUpdate::Applied); + let reloaded = quiet_storage(dir.path()); + let recorded = reloaded + .get_repository("owner1", "repo1") + .expect("the record"); + assert_eq!(recorded.last_fetched.as_ref(), Some(&fetched)); + assert_eq!( + recorded.default_branch.named(), + Some("develop"), + "the other run's field, which this update never named" + ); + } + + #[test] + fn an_update_does_not_resurrect_a_record_another_writer_removed() { + // The other run's delete has to win: a record put back names a clone + // directory that is gone. + let dir = temp_dir(); + let mut seeded = quiet_storage(dir.path()); + seeded + .add_worktree(worktree("owner1", "repo1", "branch1")) + .expect("saved"); + + let mut updating = quiet_storage(dir.path()); + let mut other = quiet_storage(dir.path()); + other + .remove_worktree("owner1", "repo1", "branch1") + .expect("saved"); + + let (update, _) = updating + .update_worktree("owner1", "repo1", "branch1", |recorded| { + recorded.devpod_workspace_id = Some("adopted".to_owned()); + }) + .expect("nothing to write and nothing to refuse"); + + assert_eq!(update, RecordUpdate::Absent); + let reloaded = quiet_storage(dir.path()); + assert!( + reloaded + .get_worktree("owner1", "repo1", "branch1") + .is_none(), + "the update found no record and wrote nothing" + ); + } + #[test] fn a_run_that_has_to_queue_can_say_so_before_it_blocks() { // The one thing a returned notice cannot cover, because the point of diff --git a/rust/devlaunch-core/src/domain/model.rs b/rust/devlaunch-core/src/domain/model.rs index ee8681ee..450b694b 100644 --- a/rust/devlaunch-core/src/domain/model.rs +++ b/rust/devlaunch-core/src/domain/model.rs @@ -205,7 +205,13 @@ pub(crate) struct BaseRepository { pub(crate) default_branch: RecordedDefaultBranch, pub(crate) last_fetched: Option, /// The branch names this repository has workspace clones for. - pub(crate) worktrees: Vec, + /// + /// Narrower than its neighbours on purpose: [`super::metadata`] keeps it in + /// step with the worktree map on every add and remove, and it is the one + /// field of this record a dl run that holds no repo lock can move (#400 + /// §4). Out of reach of `flows`, so a caller with one field to change has + /// to say which field, and cannot carry a stale copy of this one along. + pub(super) worktrees: Vec, } /// A workspace clone of one branch. @@ -217,8 +223,13 @@ pub struct WorktreeInfo { #[serde(serialize_with = "as_string")] pub(crate) local_path: PathBuf, pub(crate) workspace_id: String, - pub(crate) created_at: Timestamp, - pub(crate) last_used: Timestamp, + /// Written by [`WorktreeInfo::new`] and by nothing else. Narrower than + /// their neighbours because no production caller outside this module reads + /// either one (#400 §4) — `dl ls` takes the last-used column off the devpod + /// listing, not off the record — so the day one wants them, it asks here + /// rather than reaching in. + pub(super) created_at: Timestamp, + pub(super) last_used: Timestamp, pub(crate) devpod_workspace_id: Option, } @@ -245,9 +256,12 @@ pub(crate) struct NotRebuilt { impl BaseRepository { /// A repository with the defaults `models.py` declares. /// - /// Held for the #251 §7 public-API freeze — the record `up` writes on a first - /// clone. Only tests build one today. - #[cfg_attr(not(test), allow(dead_code))] + /// The record `up` writes on a first clone, and now the only way to build + /// one outside this module: `worktrees` is out of `flows`' reach (#412), so + /// a caller registering a repository no longer picks what its branch list + /// says. Both flows-layer sites used to spell the record out as a literal + /// and both wrote `worktrees: Vec::new()`. That is still the value; it is + /// this module's answer now rather than one invented at the call site. pub(crate) fn new(owner: &str, repo: &str, remote_url: &str, local_path: PathBuf) -> Self { Self { owner: owner.to_owned(), diff --git a/rust/devlaunch-core/src/flows/lifecycle.rs b/rust/devlaunch-core/src/flows/lifecycle.rs index 0d77b8c7..41e9f44f 100644 --- a/rust/devlaunch-core/src/flows/lifecycle.rs +++ b/rust/devlaunch-core/src/flows/lifecycle.rs @@ -3065,10 +3065,20 @@ pub fn apply_reconciliation( // The second copy of the id, which is what stops this happening again: // after this the workspace is reachable from the record, so the next // derivation change costs nothing. - let mut record = adoptable.record.clone(); - record.devpod_workspace_id = Some(adoptable.workspace_id.clone()); - match storage.add_worktree(record) { - Ok(store_notices) => extend_with_store(notices, store_notices), + // + // Written into the record the metadata lock reloaded rather than into a + // copy taken while the plan was being confirmed. The confirmation + // prompt puts an unbounded wait between the read and the write, and a + // whole-record write would carry every other field back across it. + // `Absent` is the workspace having been removed while the plan sat + // there, and re-inserting the record would undo that removal. + match storage.update_worktree( + &adoptable.record.owner, + &adoptable.record.repo, + &adoptable.record.branch, + |record| record.devpod_workspace_id = Some(adoptable.workspace_id.clone()), + ) { + Ok((_, store_notices)) => extend_with_store(notices, store_notices), Err(error) => notices.say(LifecycleNotice::RecordNotDropped { path: adoptable.record.local_path.clone(), refusal: error, diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index 6416ffec..92210e6a 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -1241,15 +1241,10 @@ impl<'r> RepositoryManager<'r> { }); } - let repository = BaseRepository { - owner: owner.to_owned(), - repo: repo.to_owned(), - remote_url: remote_url.to_owned(), - local_path: bare.clone(), - default_branch: RecordedDefaultBranch::from_stored(self.default_branch_of(&bare)), - last_fetched: Some(Timestamp::now()), - worktrees: Vec::new(), - }; + let mut repository = BaseRepository::new(owner, repo, remote_url, bare.clone()); + repository.default_branch = + RecordedDefaultBranch::from_stored(self.default_branch_of(&bare)); + repository.last_fetched = Some(Timestamp::now()); let recorded = self.record(storage, repository, notices)?; // After the record, as Python logs it: what the line reports is a clone // that is both on disk and known about. @@ -1270,18 +1265,13 @@ impl<'r> RepositoryManager<'r> { bare: &Path, notices: &mut dyn Notices, ) -> Result { - let repository = BaseRepository { - owner: owner.to_owned(), - repo: repo.to_owned(), - remote_url: remote_url.to_owned(), - local_path: bare.to_path_buf(), - // Read off the adopted clone, not defaulted: a repository whose - // default branch is `master` and one this could read nothing at all - // from would otherwise get the same answer. - default_branch: RecordedDefaultBranch::from_stored(self.default_branch_of(bare)), - last_fetched: Some(Timestamp::now()), - worktrees: Vec::new(), - }; + let mut repository = BaseRepository::new(owner, repo, remote_url, bare.to_path_buf()); + // Read off the adopted clone, not defaulted: a repository whose default + // branch is `master` and one this could read nothing at all from would + // otherwise get the same answer. + repository.default_branch = + RecordedDefaultBranch::from_stored(self.default_branch_of(bare)); + repository.last_fetched = Some(Timestamp::now()); // The record *is* the point of this call, so a write that fails is the // call failing: there is nothing else it accomplished. self.record(storage, repository, notices) @@ -1426,14 +1416,19 @@ impl<'r> RepositoryManager<'r> { }); } - if let Some(mut recorded) = storage.get_repository(owner, repo).cloned() { + // The stamp is the only field this touches, so it moves inside the + // metadata lock rather than riding back in a copy of the whole record + // taken before the lock existed. `Absent` is the "no record, nothing to + // stamp" this used to spell as an `if let Some`, and stays silent for + // the same reason: the sweeper is bookkeeping behind a fetch that has + // already happened. + match storage.update_repository(owner, repo, |recorded| { recorded.last_fetched = Some(Timestamp::now()); - match storage.add_repository(recorded) { - Ok(store_notices) => { - notices.say_all(store_notices.into_iter().map(CacheNotice::Metadata)); - } - Err(error) => return Err(FetchRepoError::NotRecorded(error)), + }) { + Ok((_, store_notices)) => { + notices.say_all(store_notices.into_iter().map(CacheNotice::Metadata)); } + Err(error) => return Err(FetchRepoError::NotRecorded(error)), } // Last, where Python logs it: past the fetch and past the bookkeeping, so // the line means both are done. @@ -2233,7 +2228,6 @@ pub(crate) mod tests { cloned.last_fetched.is_some(), "the sweep's clock starts here" ); - assert!(cloned.worktrees.is_empty()); assert_eq!( cache.storage.get_repository("owner", "repo"), Some(&cloned), diff --git a/rust/devlaunch-core/src/flows/workspace_clone.rs b/rust/devlaunch-core/src/flows/workspace_clone.rs index 3824ad6f..052a6be6 100644 --- a/rust/devlaunch-core/src/flows/workspace_clone.rs +++ b/rust/devlaunch-core/src/flows/workspace_clone.rs @@ -1350,7 +1350,7 @@ mod tests { use super::*; use crate::domain::locks; - use crate::domain::model::{RecordedDefaultBranch, Timestamp}; + use crate::domain::model::RecordedDefaultBranch; use crate::flows::repo_manager::{ Cleanup, RemoveTreeError, bare_dir, clone_dir, repo_dir, repo_lock_path, tests::{ @@ -3083,10 +3083,9 @@ mod tests { /// A record pointing at `local_path`, as `metadata.json` holds one. fn a_record(branch: &str, local_path: PathBuf) -> WorktreeInfo { - let mut recorded = WorktreeInfo::new("owner", "repo", branch, local_path, &leaf(branch)); - recorded.created_at = Timestamp::from_civil(jiff::civil::datetime(2024, 1, 1, 10, 0, 0, 0)); - recorded.last_used = recorded.created_at.clone(); - recorded + // The stamps are whatever the constructor writes: nothing below reads + // them, and they are out of this module's reach now (#412). + WorktreeInfo::new("owner", "repo", branch, local_path, &leaf(branch)) } #[test] From ef3123b4941862ab887b1c38886696b2a364615c Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 19:31:18 +0000 Subject: [PATCH 2/3] A re-point that wrote nothing stops being reported as one that landed `apply_reconciliation` destructured the update as `Ok((_, store_notices))` and pushed `Adoption::Repointed` regardless, so the `Absent` arm this branch added -- the row gone when the metadata lock was taken, which is `dl rm` in another terminal while the confirmation prompt sits there -- re-pointed devpod, wrote no metadata, and still printed `Re-pointed ...` about a record dl never touched, with `finished()` true and an exit code of 0. The arm the PR body sold as "an arm the caller can see" was read by neither call site. `Adoption::Unrecorded { workspace_id }` is that ending: devpod re-pointed, dl's record not written. The store refusing the write lands there too, because it is the same half-done adoption and the notice already names the refusal; reporting one of the two as landed while the other was not would have been a distinction with nothing behind it. `finished()` is a `match` over the arms rather than a `matches!` for `Refused`, so the arm after this one has to say which side of "landed" it falls on instead of inheriting the answer that caused this. `dl --reconcile` prints the new ending on stderr beside the refusals. repo_manager's discard was the defensible one and stays silent -- the sweeper is bookkeeping behind a fetch that has already happened -- but it says so as an arm now rather than dropping the answer with `_`, since a caller that cannot say what it does with an answer is exactly the shape that produced the bug above. Three tests, red first. At the reconcile seam, the plan built and the row removed before it is applied: `left: 1, right: 0` on the re-pointed count, which is the report claiming an adoption that wrote nothing. At the metadata seam, `update_repository` -> `Absent` had no test at all, and it is the arm the dropped `if let Some(get_repository(..))` newly reaches; its sibling pins the "no longer skipped" behaviour the body advertises, a repository another run registered since this store loaded. Both go red on the read-clone-set-`add_*` shape, one with `Absent` where `Applied` was wanted and one the other way round. `RecordUpdate` is `#[must_use]`, which is worth its line and not a guarantee: it fires on the answer thrown away whole, and the `_` in a tuple destructure that actually threw it away is still silent. Both call sites naming both arms is what holds. The doc gains the one misuse the compiler does not catch: `edit` calling a mutator on the same store is E0499, but `edit` capturing a second `MetadataStorage` over the same file compiles and blocks forever on `flock` -- inherited from `exclusive`, so a doc line rather than a fix. A variant on a `pub` enum is two rows in `public-api.rest.txt`, the tripwire tier the crate docs describe as regenerated freely. `public-api.api.txt` is untouched. Not taken: a second `BaseRepository` constructor pre-setting the two fields both repo_manager sites overwrite. It closes a transient three lines wide inside one function, and costs `BaseRepository::new` its only non-test callers -- back to dead code outside tests, with two constructors for one record. --- rust/devlaunch-core/public-api.rest.txt | 2 + rust/devlaunch-core/src/domain/metadata.rs | 79 ++++++++++- rust/devlaunch-core/src/flows/lifecycle.rs | 125 ++++++++++++++++-- rust/devlaunch-core/src/flows/repo_manager.rs | 31 +++-- rust/dl/src/commands.rs | 6 + 5 files changed, 216 insertions(+), 27 deletions(-) diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 0096b7af..2ba09d32 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1045,6 +1045,8 @@ pub devlaunch_core::flows::lifecycle::Adoption::Refused pub devlaunch_core::flows::lifecycle::Adoption::Refused::failure: devlaunch_core::flows::lifecycle::RepointFailure pub devlaunch_core::flows::lifecycle::Adoption::Refused::workspace_id: alloc::string::String pub devlaunch_core::flows::lifecycle::Adoption::Repointed(alloc::boxed::Box) +pub devlaunch_core::flows::lifecycle::Adoption::Unrecorded +pub devlaunch_core::flows::lifecycle::Adoption::Unrecorded::workspace_id: alloc::string::String impl core::clone::Clone for devlaunch_core::flows::lifecycle::Adoption pub fn devlaunch_core::flows::lifecycle::Adoption::clone(&self) -> devlaunch_core::flows::lifecycle::Adoption impl core::cmp::Eq for devlaunch_core::flows::lifecycle::Adoption diff --git a/rust/devlaunch-core/src/domain/metadata.rs b/rust/devlaunch-core/src/domain/metadata.rs index 0e04fbb9..ea681130 100644 --- a/rust/devlaunch-core/src/domain/metadata.rs +++ b/rust/devlaunch-core/src/domain/metadata.rs @@ -297,6 +297,13 @@ pub(crate) enum MigrationCommit { /// edit, and inserting the record back would turn that run's delete into a row /// naming a clone directory that no longer exists. Saying so is what lets a /// caller tell "changed" from "there was nothing there". +/// +/// `#[must_use]` because the first caller written against this dropped the +/// answer and reported a write that never happened. It is not a guarantee: the +/// lint fires on a value thrown away whole, and a `_` in the tuple destructure +/// that actually did it is still silent. What stops that is the two call sites +/// naming both arms, which is why they do. +#[must_use] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum RecordUpdate { /// The edit ran on the reloaded record and the store was saved. @@ -503,7 +510,13 @@ impl MetadataStorage { /// /// Nothing is written when the key is absent (#412): a store that inserted /// would undo a delete another run had just committed. Like every mutator - /// here it is not reentrant — `edit` must not call one. + /// here it is not reentrant — `edit` must not call one. Calling one on + /// *this* store is a borrow error, so the rule the compiler does not keep + /// is the other shape: `edit` capturing a second [`MetadataStorage`] over + /// the same file compiles, and then blocks forever, because the lock is one + /// flock per open file description and [`super::locks`] waits without a + /// timeout. Inherited from [`MetadataStorage::exclusive`] rather than new + /// here, and worth naming because this is the first closure `flows` writes. pub(crate) fn update_repository( &mut self, owner: &str, @@ -2606,6 +2619,70 @@ mod tests { ); } + #[test] + fn an_update_reaches_a_repository_registered_since_this_store_loaded() { + // What the sweeper's dropped `if let Some(get_repository(..))` used to + // decide off a map loaded minutes and one `git fetch --all` ago: a + // repository another run registered in that window read as "not there, + // nothing to stamp". The lookup happens under the lock now, on the + // reload, so the record it has to find is the one on disk. + let dir = temp_dir(); + let mut updating = quiet_storage(dir.path()); + let mut other = quiet_storage(dir.path()); + other + .add_repository(repository("owner1", "repo1")) + .expect("saved"); + + let fetched = Timestamp::from_civil(civil::datetime(2026, 8, 24, 9, 0, 0, 0)); + let (update, _) = updating + .update_repository("owner1", "repo1", |recorded| { + recorded.last_fetched = Some(fetched.clone()); + }) + .expect("saved"); + + assert_eq!(update, RecordUpdate::Applied); + let reloaded = quiet_storage(dir.path()); + assert_eq!( + reloaded + .get_repository("owner1", "repo1") + .expect("the record") + .last_fetched + .as_ref(), + Some(&fetched), + "stamped, where the pre-lock lookup would have skipped it" + ); + } + + #[test] + fn an_update_does_not_resurrect_a_repository_another_writer_removed() { + // The worktree half of this is above; a repository is the other half of + // the same rule, and it is the arm the sweeper newly reaches now that + // nothing guards the call. A row put back names a bare clone `dl rm` + // has already deleted. + let dir = temp_dir(); + let mut seeded = quiet_storage(dir.path()); + seeded + .add_repository(repository("owner1", "repo1")) + .expect("saved"); + + let mut updating = quiet_storage(dir.path()); + let mut other = quiet_storage(dir.path()); + other.remove_repository("owner1", "repo1").expect("saved"); + + let (update, _) = updating + .update_repository("owner1", "repo1", |recorded| { + recorded.last_fetched = Some(Timestamp::now()); + }) + .expect("nothing to write and nothing to refuse"); + + assert_eq!(update, RecordUpdate::Absent); + let reloaded = quiet_storage(dir.path()); + assert!( + reloaded.get_repository("owner1", "repo1").is_none(), + "the other run's delete stands" + ); + } + #[test] fn a_run_that_has_to_queue_can_say_so_before_it_blocks() { // The one thing a returned notice cannot cover, because the point of diff --git a/rust/devlaunch-core/src/flows/lifecycle.rs b/rust/devlaunch-core/src/flows/lifecycle.rs index 41e9f44f..ae7a6eda 100644 --- a/rust/devlaunch-core/src/flows/lifecycle.rs +++ b/rust/devlaunch-core/src/flows/lifecycle.rs @@ -72,7 +72,7 @@ use crate::clients::devpod::{ use crate::clients::docker; use crate::clients::git::Git; use crate::domain::locks::{self, LockError}; -use crate::domain::metadata::{self, MetadataStorage, WorktreeFilter}; +use crate::domain::metadata::{self, MetadataStorage, RecordUpdate, WorktreeFilter}; use crate::domain::model::WorktreeInfo; use crate::domain::workspace_state::{self, CouldNotTell, Losses, NonEmpty, Unsaved}; use crate::flows::completion_cache; @@ -3002,6 +3002,15 @@ pub enum Adoption { workspace_id: String, failure: RepointFailure, }, + /// devpod's record was re-pointed and metadata was not, so the id has the + /// one copy a finished adoption leaves two of. Either the row was gone when + /// the metadata lock was taken — another run removed the workspace while + /// the plan sat at its prompt, and writing would have put the row back — or + /// the store refused the write, and then a notice names the refusal. Half + /// an adoption, reported as one: [`ReconcileReport::finished`] is false + /// here, because the alternative is a line reading "Re-pointed" about a + /// record dl never touched. + Unrecorded { workspace_id: String }, } /// What applying a reconcile plan did: one [`Adoption`] per adoption the plan @@ -3025,16 +3034,21 @@ impl ReconcileReport { pub(crate) fn repointed(&self) -> impl Iterator { self.adoptions.iter().filter_map(|adoption| match adoption { Adoption::Repointed(adoptable) => Some(adoptable.as_ref()), - Adoption::Refused { .. } => None, + Adoption::Refused { .. } | Adoption::Unrecorded { .. } => None, }) } /// Whether every adoption landed. The one distinction an exit code carries. + /// + /// A `match` rather than the `matches!` this used to be, so an arm added to + /// [`Adoption`] has to say which side of that distinction it falls on + /// instead of defaulting to "landed" — which is how a re-point that wrote + /// no metadata came to leave this true. pub fn finished(&self) -> bool { - !self - .adoptions - .iter() - .any(|adoption| matches!(adoption, Adoption::Refused { .. })) + self.adoptions.iter().all(|adoption| match adoption { + Adoption::Repointed(_) => true, + Adoption::Refused { .. } | Adoption::Unrecorded { .. } => false, + }) } } @@ -3072,19 +3086,39 @@ pub fn apply_reconciliation( // whole-record write would carry every other field back across it. // `Absent` is the workspace having been removed while the plan sat // there, and re-inserting the record would undo that removal. - match storage.update_worktree( + // + // Which arm comes back is what the report says, and that is the point + // of there being arms: an adoption is "re-pointed" when both writes + // happened, and the two endings where the second one did not are + // `Unrecorded`. Reporting them as `Repointed` — which discarding the + // answer amounts to — prints a line about a record dl never touched. + let ending = match storage.update_worktree( &adoptable.record.owner, &adoptable.record.repo, &adoptable.record.branch, |record| record.devpod_workspace_id = Some(adoptable.workspace_id.clone()), ) { - Ok((_, store_notices)) => extend_with_store(notices, store_notices), - Err(error) => notices.say(LifecycleNotice::RecordNotDropped { - path: adoptable.record.local_path.clone(), - refusal: error, - }), - } - adoptions.push(Adoption::Repointed(Box::new(adoptable.clone()))); + Ok((RecordUpdate::Applied, store_notices)) => { + extend_with_store(notices, store_notices); + Adoption::Repointed(Box::new(adoptable.clone())) + } + Ok((RecordUpdate::Absent, store_notices)) => { + extend_with_store(notices, store_notices); + Adoption::Unrecorded { + workspace_id: adoptable.workspace_id.clone(), + } + } + Err(error) => { + notices.say(LifecycleNotice::RecordNotDropped { + path: adoptable.record.local_path.clone(), + refusal: error, + }); + Adoption::Unrecorded { + workspace_id: adoptable.workspace_id.clone(), + } + } + }; + adoptions.push(ending); } // devpod's records just changed, so any listing dl is holding describes the // world before the repair. @@ -7258,6 +7292,69 @@ pub(crate) mod tests { ); } + #[test] + fn a_record_removed_while_the_plan_sat_there_is_not_reported_as_re_pointed() { + // The confirmation prompt is an unbounded wait, and `dl rm` in + // another terminal is what walks through it. devpod's record is + // re-pointed either way — that write is done before metadata is + // reloaded — but the id is not written, because writing it would put + // back a row the other run deleted. What must not happen is the run + // reporting an adoption that landed anyway. + let mut world = World::empty(); + let devpod_home = world.tmp().join("devpod"); + let clone = a_bare_clone_directory(&world.repo_dir.join("r-feature-auth-aaa")); + world.record("r-feature-auth-aaa", "feature/auth", &clone); + let old = world.repo_dir.join("feature-auth"); + let record = devpod_record(&devpod_home, "ws-old", &old); + world.devpod.lists(&[listed("ws-old", &old)]); + let plan = reconcile_for(&world); + world + .storage + .remove_worktree(OWNER, REPO, "feature/auth") + .expect("the other run's delete"); + let updater = SelfInvocation::new("dl"); + let cache_path = fresh_cache(world.tmp()); + let mut context = CommandContext::new(&world.devpod); + let mut refresh = Refresh::new(&updater, &cache_path); + + let report = apply_reconciliation( + &mut context, + &mut refresh, + &mut world.storage, + &devpod_home, + &plan, + &mut ignoring(), + ); + + assert_eq!( + report.adoptions(), + [Adoption::Unrecorded { + workspace_id: "ws-old".to_owned() + }], + "the ending the report carries is the one that happened" + ); + assert_eq!(report.repointed().count(), 0, "nothing was recorded"); + assert!( + !report.finished(), + "an adoption that wrote nothing is not an adoption that landed" + ); + assert!( + world + .storage + .get_worktree(OWNER, REPO, "feature/auth") + .is_none(), + "the other run's delete stands" + ); + assert_eq!( + sourced_at(&record), + canonical(&clone.to_string_lossy()) + .expect("the clone") + .display() + .to_string(), + "devpod's record was re-pointed before the reload found the row gone" + ); + } + #[test] fn reconciling_never_reaches_devpod_delete() { // A wrongly-adopted workspace costs a rebuild; a wrongly-deleted one costs diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index 92210e6a..90c2e422 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -40,7 +40,7 @@ use std::time::Duration; use crate::clients::git::{self, Failure, Git, GitRefused}; use crate::domain::locks::{self, Contention, LockError, LockGuard}; -use crate::domain::metadata::{self, MetadataError, MetadataStorage}; +use crate::domain::metadata::{self, MetadataError, MetadataStorage, RecordUpdate}; use crate::domain::model::{BaseRepository, RecordedDefaultBranch, Timestamp}; use crate::domain::workspace_id::{NamePart, UnsafeName, validate_ref_name}; use crate::domain::workspace_state::NonEmpty; @@ -1418,17 +1418,24 @@ impl<'r> RepositoryManager<'r> { // The stamp is the only field this touches, so it moves inside the // metadata lock rather than riding back in a copy of the whole record - // taken before the lock existed. `Absent` is the "no record, nothing to - // stamp" this used to spell as an `if let Some`, and stays silent for - // the same reason: the sweeper is bookkeeping behind a fetch that has - // already happened. - match storage.update_repository(owner, repo, |recorded| { - recorded.last_fetched = Some(Timestamp::now()); - }) { - Ok((_, store_notices)) => { - notices.say_all(store_notices.into_iter().map(CacheNotice::Metadata)); - } - Err(error) => return Err(FetchRepoError::NotRecorded(error)), + // taken before the lock existed. + let (stamped, store_notices) = storage + .update_repository(owner, repo, |recorded| { + recorded.last_fetched = Some(Timestamp::now()); + }) + .map_err(FetchRepoError::NotRecorded)?; + notices.say_all(store_notices.into_iter().map(CacheNotice::Metadata)); + match stamped { + RecordUpdate::Applied => {} + // The "no record, nothing to stamp" this used to spell as an `if + // let Some`, and silent for the same reason it was silent then: the + // sweeper is bookkeeping behind a fetch that has already happened, + // and a repository dropped from the store while that fetch ran is + // not news about the fetch. Spelled as an arm rather than dropped + // with `_`, because a caller that cannot say what it does with an + // answer is the shape that turned a no-op into a reported success + // in `apply_reconciliation`. + RecordUpdate::Absent => {} } // Last, where Python logs it: past the fetch and past the bookkeeping, so // the line means both are done. diff --git a/rust/dl/src/commands.rs b/rust/dl/src/commands.rs index 52b07493..416ddf2e 100644 --- a/rust/dl/src/commands.rs +++ b/rust/dl/src/commands.rs @@ -1042,6 +1042,12 @@ fn render_reconcile( "Could not re-point {workspace_id}: {}", render::repoint_failure(failure) ), + // stderr with the refusals rather than stdout with the adoptions: + // the workspace opens the right clone now, but dl's half of the + // repair did not happen, and the exit code says so. + lifecycle::Adoption::Unrecorded { workspace_id } => { + eprintln!("Re-pointed {workspace_id}; dl's own record was not updated") + } } } say(¬ices); From 9288658a2a0e7c9033be7bd212d24f7f2c5cd474 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Mon, 24 Aug 2026 19:35:38 +0000 Subject: [PATCH 3/3] Changelog: the reconcile outcome that reports itself unlanded The refactor this rides on changes no behaviour, which is why it was told to add nothing here. Fixing the review's first finding changed that: --reconcile now has a third thing it can say, and a run that says it is not finished where it used to say it was. That is the kind of change the file exists for, so the instruction not to add one had gone stale. --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ab2e010..94d20b24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`dl --reconcile` no longer reports an adoption as landed when it wrote no + record.** Re-pointing devpod at the clone and recording the worktree are two + steps, and the second can find nothing to update — another run removed the + record while the plan sat there waiting to be applied. That case was reported + as `Repointed` like any other, with the run finishing successfully, so the one + outcome worth knowing about looked identical to the ordinary one. It is now its + own `Unrecorded` arm: devpod re-pointed, dl's record not written, said on + stderr, and the run does not report itself finished. A store that refused the + write lands in the same arm for the same reason — it is the same half-done + adoption — and the refusal is named beside it. + ### Added - **CI fails when nothing reviewed a pull request.** Sourcery answers a quota