Repository navigation
Give MetadataStorage an update_* so a field change stops crossing the seam #412
Description
Decided on #400, which established there is no live race — read the resolution comment there before starting, because it is what keeps this slice from being mis-sold. This is a maintainability and modelling change. Nothing here closes a bug, and the PR must not claim to.
Why
MetadataStorage offers get_* and add_* and no update (domain/metadata.rs:498, :461), so a caller with one field to change has no move but read-modify-write across the seam: the record is cloned before the lock exists, and add_* inserts it wholesale over the map exclusive freshly reloaded (:562). Two production sites do that today.
That it costs nothing today is an accident of which fields have readers, and the argument runs through six files. #400 §4 has it in full; the short version is that the only concurrently-writable field of BaseRepository is worktrees, which nothing reads, and the only concurrently-writable fields of WorktreeInfo are either unread or derived byte-identically by the other writer. Add a reader for worktrees, or a field to BaseRepository written off the repo-lock path, and it goes live with nothing going red.
The module already knows the right shape. commit_migration (metadata.rs:674) reloads under the lock and hands the reloaded IndexMap to an edit closure; migrate_cache mutates records through values_mut() (flows/migration.rs:192). This generalises that rather than adding a second mechanism.
Slice
update_repository(owner, repo, |r| …)andupdate_worktree(owner, repo, branch, |w| …)onMetadataStorage, running the closure on the record loaded under the lock, saving only if the key was there. Built onexclusive, shaped aftercommit_migration.- Convert the two genuine call sites:
flows/repo_manager.rs:1429-1436—last_fetched.flows/lifecycle.rs:3068-3070—devpod_workspace_id.
- Leave the other two alone, and say why in the PR body:
flows/migration.rs:306already runs insidecommit_migration's closure on reloaded records. It is not a cross-seam RMW.flows/workspace_clone.rs:960-961reads nothing —WorktreeInfo::new(domain/model.rs:283) builds the record from launch inputs. Wholesale registration is a different intent from updating a field, soadd_worktreestays and this site keeps calling it.
- With no flows-layer site constructing a record it did not read, narrow the fields on
BaseRepository/WorktreeInfo(domain/model.rs:199-223) to module-private wherever the borrow checker now allows. This is the constructive-modelling half: "a record whose unseen fields the caller invented" stops being representable.
Net lines should be negative in flows/.
Tests, red first
Not the two-process test #400 originally proposed — that passes today, for the reason in §4, and would mislead. Both of these live at the metadata seam and need no subprocess: flock is per open file description (domain/locks.rs:14-17), so a second MetadataStorage on the same path inside one test is a genuine second writer.
- A concurrent field update survives. Store A loads. Store B writes a different field of the same record and saves. Store A
update_*s its field. Assert both fields on disk. Red today withadd_*: the pre-lock clone overwrites B. - An update does not resurrect a deleted record. Store A loads. Store B removes the worktree. Store A
update_worktrees that key. Assert the key is still absent, and that the call reports it did nothing. Red today atmetadata.rs:506, whoseinsertis unconditional.
Out of scope
Making the repo lock cover apply_reconciliation and remove_clone. Those are real asymmetries (#400 §1) but a separate question, and the closure fix does not depend on the answer.
Activity
blooop commentedon Aug 24, 2026
Built as #428, on
refactor/metadata-update-closures. Open, not merged, and this ticket stays open.update_repository/update_worktreerun an edit closure on the recordexclusiveloaded under the lock, returningRecordUpdate::{Applied, Absent}so a key another run deleted is an arm rather than a resurrectinginsert. Shaped aftercommit_migration, not beside it. The two genuine sites are converted (flows/repo_manager.rs'slast_fetched,flows/lifecycle.rs'sdevpod_workspace_id);flows/migration.rsandflows/workspace_clone.rsare left alone with the reason in the PR body.Both seam tests were red first, with the sites still spelled as read-clone-set-
add_*; the verbatim failures are in the PR. The two-process race is not tested, for the reason in §4 above, and the PR says in its first section that this fixes no bug and what would make the gap live.Two departures from the slice as written, both in step 4:
BaseRepository::worktreesandWorktreeInfo::{created_at, last_used}are nowpub(super). The other seven each have a production reader inflows, and Rust has no visibility that separates reading a field from writing it, so narrowing them would cost about ten accessors and sixty-odd rewritten reads for no modelling gain.devpod_workspace_idis blocked for a second reason: the closure that writes it is written inflows, even though it runs under the lock. The twoBaseRepositoryliterals inrepo_managerdid go, throughBaseRepository::new, which stops being dead code outside tests.flows/are +3, not negative: −12 code lines and +15 comment lines. The code shrank as predicted; the comments explaining why the closure form is the one to reach for did not.No public surface change — the new methods and
RecordUpdatearepub(crate), the narrowed fields werepub(crate)and never in the snapshot, sopublic-api.rest.txtis untouched. Gate green:cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check.blooop commentedon Aug 24, 2026
wf-review on #428: Request changes — 2 blocking (Absent dropped in
apply_reconciliationso a no-op reconcile still reportsRepointed;update_repository→Absentuntested and newly reachable). Report on the PR.blooop commentedon Aug 24, 2026
Review findings on #428 addressed on
ef3123b, still open and not merged.Both blocking items were red first.
apply_reconciliationwas reportingAdoption::Repointedfor an update that returnedAbsent— devpod re-pointed, no metadata written,finished()true — so there is a third arm,Adoption::Unrecorded { workspace_id }, andfinished()is amatchover the arms rather than amatches!, which is what stops the next arm inheriting "landed" by default. The store refusing the write lands in the same arm.Absentis read at both call sites now; repo_manager's stays silent for the reason already in its comment, but says so as an arm.update_repository→Absentand the "a repository another run just registered is no longer skipped" behaviour change both have tests now; both go red on the read-clone-set-add_*shape. Non-blocking:#[must_use]onRecordUpdateand a doc line naming the second-storeflockdeadlock taken; a secondBaseRepositoryconstructor declined, because it costsnewits only non-test callers.One row of public surface where the first revision had none — a variant on a
pubenum, sopublic-api.rest.txtgains two rows;public-api.api.txtis untouched. The PR body's "no user-visible behaviour changes" and "no public surface change" are both corrected, and the stale red the review found is replaced.