Read-modify-write spans the metadata seam, so a concurrent field update is lost #400

Activity

1. blooop

   added

   bugSomething isn't workingSomething isn't working

   wayfinder:grilling

   and removed

   bugSomething isn't workingSomething isn't working

   on Aug 24, 2026
2. blooop

   added a parent issue on Aug 24, 2026

   • Close out the 2026-08-24 architecture review #406
3. blooop

   self-assigned this

   on Aug 24, 2026
4. blooop commented on Aug 24, 2026

   

   blooop

   on Aug 24, 2026

   OwnerAuthor

   More actions

   agent-decided (long-term maintainability):

   The ticket asks one thing — establish whether the window is reachable before paying for the fix. Answer in five parts: the launch flock is not what guards these writes, only two of the four sites are the shape described, the window is wide open, nothing observable travels through it, and the interface should change anyway.

   1. The launch flock covers none of the four writes

   serialize_launch is taken at flows/launch.rs:1089, inside up_under_stage — stage four. Every metadata write on the launch path happens in stage three: place_triple calls prepare (launch.rs:2656), prepare delegates whole to prepare_cold (launch.rs:2332), and carry_out — the function that eventually reaches workspace_up — runs only after place has returned a Placement. The launch lock is acquired after the last metadata write of the launch, never across one.

   So the hedge in "How real is it" should be struck: serialize_launch holds nothing across any of these four writes, and the per-workspace lock drops out of the analysis entirely.

   What does guard them is the per-repo lock, partially:

   • workspace_clone.rs:961 — under hold_repo_lock (workspace_clone.rs:535).
   • repo_manager.rs:1431 — under the repo lock too, from the other side: the only production caller is lazy_fetch ← sweep_repo_fetches, wrapped in locks::run_if_lock_free(&repos.lock_path(&owner, &repo), …) (lifecycle.rs:485), and lock_path (repo_manager.rs:1107) is the same path hold_repo_lock takes (:1132).
   • lifecycle.rs:3070 — no repo lock. Nothing but the metadata lock.
   • migration.rs:306 — see below.

   2. Two of the four sites are not the shape the ticket describes

   migration.rs:306 is already the proposed fix. commit_migration (metadata.rs:674) takes the metadata lock, reloads at :688, and hands the reloaded IndexMap to the edit closure; migrate_cache iterates worktrees.values_mut() (migration.rs:192) and migrate_record mutates the borrowed record in place. That is update_worktree(key, |w| …) under another name, already in this file and already load-bearing. Whatever gets built should generalise it, not invent a second mechanism beside it.

   workspace_clone.rs:960-961 is not a read-modify-write. WorktreeInfo::new (model.rs:283) builds the record out of the launch's own inputs; nothing is read from storage. It is wholesale re-registration, which is what the comment above it says it is. It clobbers, but only with values it owns.

   Two genuine cross-seam read-modify-writes remain: repo_manager.rs:1429-1436 and lifecycle.rs:3068-3070.

   3. The window is reachable, and not by microseconds

   The staleness is not the read-to-write distance — at repo_manager.rs:1429-1431 those are adjacent lines. It is the load-to-write distance, because get_repository reads the in-memory map, refreshed only by whichever exclusive() ran most recently (metadata.rs:562). Two measurements:

   • The sweeper. child_work (lifecycle.rs:381-387) is a detached child spawned on nearly every command. It loads its map at startup and then runs a full git fetch --all bounded only by BACKGROUND_FETCH_TIMEOUT = 300s (repo_manager.rs:82) before writing last_fetched back from that pre-fetch copy. Two dl processes writing metadata at once is the designed steady state here, not an edge case — domain/locks.rs:4-10 says so in as many words.
   • --reconcile. The plan is built at dl/src/commands.rs:997 and applied at :1020, with confirmed("Re-point these? [y/N] ") sitting at :1012 in between. The read-to-write window contains a human prompt, so it is unbounded.

   Reachable, then, and for minutes.

   4. And yet nothing is lost through it, because there is no field to lose

   I enumerated every field a second process can move while one of the two sites holds its stale copy.

   BaseRepository, at the sweeper's write. The sweeper holds the repo lock, and every writer of remote_url, local_path and default_branch holds it too — all of them reach storage through record (repo_manager.rs:1291), from clone_if_missing/register_existing_bare, which are RepoLock-gated. The one field a non-holder can move is worktrees, pushed by add_worktree (metadata.rs:508-510) and popped by remove_worktree (:532-534) on the reloaded record — and remove_clone (workspace_clone.rs:1278, the rm/--prune path) takes no repo lock, so it genuinely races the sweeper. But BaseRepository.worktrees is written by dl and read by nothing. The only non-test mentions in the tree are those two mutators and serde (model.rs:274). It survives because the on-disk shape is pinned against what Python wrote, and Python is gone — nothing outside test/ is a .py file any more.

   WorktreeInfo, at apply_reconciliation's write. The revertible fields are local_path, workspace_id, devpod_workspace_id, created_at, last_used. created_at and last_used have no production reader either: dl ls takes last_used off the devpod listing (clients/devpod.rs:726 → listing.rs:883, :1114), never off the record. The other three are derived deterministically from (owner, repo, branch) through WorkspaceId, so the only concurrent writer of that key (workspace_clone.rs:960) writes byte-identical values.

   That is the answer. The mechanism the finding describes is real; the "different field of the same record" it needs does not exist today. (b): the window is reachable, the bug is not.

   5. What is not nothing

   Two residuals through the same seam. Both real; neither prices a fix on its own.

   • add_worktree's insert (metadata.rs:506) is unconditional, so it resurrects a record a concurrent writer deleted. Plan a --reconcile, leave the prompt sitting, dl <ws> rm a workspace in that plan from another terminal, answer y — the record comes back naming a directory that is gone. No data is lost: the filesystem wins by design (repo_manager.rs:1572-1576) and the next --prune clears the row. But it is a lost delete, and update_worktree(key, |w| …) is a no-op on an absent key rather than a resurrection.
   • A concurrent --reconcile could revert local_path over migrate_cache's rewrite, and that one is observable, since rm follows local_path. It is also once per machine, on a pre-Build: parse workspace ids at one boundary (injective scheme) #64 cache, against a --reconcile running at that instant.

   6. Change the interface anyway

   The safety of repo_manager.rs:1431 rests on an argument I had to reassemble out of six files: which lock the sweeper happens to hold, which callers happen to be RepoLock-gated, and which fields happen to have no reader. None of it is written down at the call site and none of it is pinned by a test — a_mutation_reloads_under_the_lock_rather_than_rewriting_a_stale_copy (metadata.rs:2443) pins the neighbouring-records half and stops there. The day somebody reads worktrees, or adds a field to BaseRepository written from a path that takes no repo lock, the bug goes live and nothing goes red. That is exactly the reader-in-a-year cost principle 1 is about, and it is why "no live bug" is not the end of this ticket.

   The interface is what makes it possible. add_worktree(WorktreeInfo) (metadata.rs:498) takes a whole record, so a caller with one field to change has no move but read-modify-write across the seam. The four call sites did not choose badly; there was nothing else to choose.

   Principle 2 agrees rather than objects, because the change is a deletion. Two call sites lose their .cloned() + field-set + add_* and become one closure each. add_* stays — wholesale registration (workspace_clone.rs:960) is a genuinely different intent from moving a field, and add beside update is the sum of the two, not an extra verb. With the flows layer no longer constructing records it never read, the fields on BaseRepository/WorktreeInfo (model.rs:199-223) can drop to module-private, which is principle 3: "a record whose unseen fields the caller invented" stops being representable.

   7. How it gets tested, and not the way "Proving it" says

   The ticket proposes two processes, one worktree key, one field each. That test passes today — it has to, because §4 is the finding that no such pair of fields exists. Writing it would prove nothing and would leave the next reader believing a race was closed that was never open.

   The behaviour genuinely absent needs no second process: domain/locks.rs:14-17 notes flock is per open file description, so a second MetadataStorage on the same path inside one test is a real second writer.

   • A field written to disk by another writer between this store's load() and its update_* survives the update, and only the closure's field moves. Red today under add_*, because the pre-lock clone overwrites it.
   • update_worktree on a key another writer has deleted writes nothing and reports so, rather than resurrecting the row. Red today at metadata.rs:506.

   Graduating as a maintainability-and-modelling slice, explicitly not a bug fix, so nobody reads the resulting PR as having closed a live race.
5. blooop commented on Aug 24, 2026

   

   blooop

   on Aug 24, 2026

   OwnerAuthor

   More actions

   Graduated as #412 on the map — update_repository/update_worktree as closures over the record loaded under the lock, the two genuine call sites converted, the other two left alone with the reason written down, and the record fields narrowed. Filed as maintainability and modelling, not as a bug fix: §4 above is why there is nothing to fix, and #412 carries the two seam tests that actually go red instead of the two-process one proposed here.
6. blooop

   closed this as completedon Aug 24, 2026