[Fix] Write a site's metadata as one read-change-write so a ticket's base survives an overlapping write, Fixes #172

[juanmaguitar]

Why

Every write to a site's stored metadata read the whole record, changed one field and wrote it back. Two writes that overlap lose one of the two changes. Harmless while the fields were labels and flags; since #108 one of them is a branch's recorded trunk base, written once when the branch starts and read for the rest of its life as the base every patch for that ticket is measured against. Lose it and the patch is generated against the wrong trunk: not empty, not refused, wrong.

The overlap the issue names is a ticket started while a trunk update is finishing. It is reproduced here, and so are five more windows of the same shape that the self-review turned up.

What changes

The root cause is narrower than the issue's framing. The store's get and set are synchronous, so the event loop already serialises writers, and mergeSiteMeta on its own was atomic: nothing yields between its read and its write. What was not atomic was every writer that computed what it stored from a read taken an await earlier.

changeSiteMeta(sitePath, change) is now the helper for those: it awaits the store, then reads, applies the caller's function to the record as it is at that moment, and writes, with no yield in between. changeWorkMetaOn is the same idea one layer up, for the per-branch work meta whose scope has to be resolved before anything can be written.

Six writers were losing changes, each a real lost write rather than a theoretical one:

• mergeBranchMeta read the branches map, awaited the store, and wrote that map back. A branch another flow started inside that await was written out of existence, base and all. This is the one the issue describes.
• wordpress:setup held the whole site map, not one record, across a Git spawn that reads the new clone's trunk info. A write to any other site during it was undone. The read now happens before the record is touched.
• sites:set-ticket read the branch's recorded base on the existing-branch path and wrote it straight back, which rolled back a branches:rebase that moved the base while the switch was running. The base is now written only by the flow that created the branch; the other path leaves it alone rather than rewriting it to the value it just read.
• branches:rebase keeps the applied-patch record and drops only its text, and it read that record, resolved the write scope with a Git spawn, then wrote back what it had read. A discard or an apply landing in that window was replaced by the record from before it, which is a revert banner for a patch that is not on disk and a Revert with no hunks to find. This is the one that costs a contributor a patch rather than a base.
• branches:delete rebuilt the map from an earlier read, the same way mergeBranchMeta did.
• migrateSiteToBranches is the one write that cannot be a single read-change-write, because its Git work sits between the read and the write. It now yields to a migration that finished in the meantime, and its catch re-reads instead of handing back the record it opened with. That last part was a bug of its own: a migration that loses the race throws branch-exists before the guard, so the loser reported an unmigrated site and branches:rebase refused a ticket whose base was on record.

AGENTS.md carries the rule under the persistence bullet, stated as the rule itself rather than as a claim about which helper gets called: nothing may yield between reading the store and writing it back.

Nothing about what is stored changes. Same keys, same shapes, no migration needed.

How to test this

Platforms: any. Nothing here touches spawning, paths, or line endings. Every command below runs in the repository directory.

Six new layer-3 tests in tests/unit/ipc-wiring.test.cjs, each verified red against trunk f31e5df and green at head:

node --test --test-name-pattern="issue #172" tests/unit/ipc-wiring.test.cjs
npm test
npm run test:electron

The instrument is the stubbed getStore. Three of the tests run one handler and land a second handler inside each store access the first one makes, one subtest per access, holding the first until the second has run to completion; AsyncLocalStorage is what lets a single stub tell the two flows apart. Iterating over every access rather than picking one matters: the count is an implementation detail, and these fixes changed it, so a test pinned to the access that fails today would pass vacuously tomorrow.

Test	What trunk loses
a ticket started while a trunk update is finishing keeps its recorded base	the new branch's base, at access 5 of 8
linking a ticket does not roll back a rebase that lands while it switches	the rebase's new base, at accesses 6 and 7
a rebase does not restore an applied-patch record a discard cleared while it ran	the discard's clear, at five of eleven accesses
registering a finished clone does not undo another site's write while it reads the trunk info	the other site's whole record
a migration that finished first is not overwritten by one still doing its git work	the winner's map, replaced by the loser's
a migration that lost the race still reports the record the winner wrote	the answer, not the record: branches:rebase says "no recorded starting point"

One thing to know when reading the staged tests, because it is not obvious: the hold sits inside the stubbed getStore, never between a get and its set, so the nested flow never sees a half-written store, which is a state the real one cannot be in. For the two pairs that live in one process the real window between read and write is microtask-only, so the staging opens a wider window than the app can. The lost-write shape is identical and reachable by two overlapping microtask chains; the clone and migration tests sit across genuine Git spawns, where the window is as wide as it looks.

By hand, on either platform, on a packaged build of this branch's head: link a ticket, run Update to latest trunk, and as soon as Updated to the latest trunk appears link a second ticket. Open the second ticket's card, and Create patch must produce a diff of that ticket's changes only. The window is a few milliseconds, so a manual pass is not evidence either way. It was not run for that reason, and the tests are what covers this.

What must not have happened:

• No branch may have lost its recorded base. That is the whole issue: a base silently replaced produces a patch that is wrong rather than missing, and nobody finds out until someone tries to apply it.
• A site registered while another site's record was being written must not have blanked it. The clone path held every site's record at once, so the blast radius was the whole registry, not one site.
• A rebase must still keep the applied-patch record and still drop its text. The record is what "Save" puts my username on a patch when I just applied someone else's PR #328 reads to refuse publishing another author's hunks as your own, and a record without a text is exactly "applied, not revertable" to the card.
• The update's own writes must still land: the trunk oid, the incomplete flag on the returning branch, and the return to the ticket. The existing Working on a second ticket means rebuilding the world: tickets should be branches, not sites #108 and False "Update incomplete" banner on trunk after an update run from a ticket #419 tests cover these and are green.
• A branch whose entry is created by a switch rather than by a start must still be readable. It now has no baseOid key at all rather than an explicit null; every reader treats the two the same, which is why branches:list and the patch refusal are unchanged.

Risks and limitations

Review outcome: 5 [fix here] · 6 [follow-up] over three passes, all 5 fixed and 4 of the follow-ups taken. Details in the collapsed section.

• Two flows can still move HEAD at once. This fixes the store half of the concurrency. A rebase running while a link is inside its checkout is two checkouts on one worktree, and the mid-switch marker only refuses after a checkout has already failed. Out of scope for Overlapping writes to a site's metadata can drop a ticket branch's patch base #172, which is about what is stored, and deliberately not filed as an issue.
• Two flows still fight over currentBranch and tracTicket. With a link landing inside the update's return, whichever writes last names the active ticket, and HEAD may say the other. That is field-level last-writer-wins, not a lost write: both values are recomputable from HEAD, and activeBranch reads HEAD first. Recorded here rather than opened.
• workMetaScope still decides a scope by reading, then writes after a yield. If a branch entry appears in that window the flag is filed at site level while the reader looks on the branch. The result is a flag written where nobody reads it, which is the False "Update incomplete" banner on trunk after an update run from a ticket #419 shape, not a dropped base, and no other writer's data is destroyed. Left as is rather than folded into the change callback, because it would rework the work-meta path [Fix] the false Update incomplete banner on trunk after an update run from a ticket #446 has just settled.
• A lock was not added. See the collapsed section for why. If a future writer must do Git work between its read and its write, the migration's pattern is the shape to copy, not a lock.
• Two concurrent migrations can still record the safe answer instead of the true one. If the loser gets far enough to see the branch on disk it records baseOid: null for it, and if it writes first the winner yields to that. Null is the deliberate answer for a branch this app did not create (When a ticket's base is unknown, the app guesses instead of saying so #308): the ticket refuses patch operations rather than guessing. So the failure mode is a refusal a re-link clears, not a wrong patch, and closing it would mean ordering two migrations rather than just stopping them clobbering each other.
• Not driven by hand. The windows are milliseconds wide and no manual sequence hits them reliably, which is why the tests iterate over every store access instead.

Related

Fixes #172. Follows #446 (Fixes #419), which moved the incomplete flag onto the returning branch and named this mechanism on the way past. #108 introduced the recorded base, #308 is why a branch without one refuses rather than guesses, and #328 is why a rebase keeps the applied-patch record it strips.

---

Design decisions and alternatives considered

The issue offered three shapes: serialise writes to the record, make the write take the field rather than the record, or guard the one value that cannot be lost.

A per-site lock was rejected as more machinery than the bug needs. The store's own calls are already synchronous, so a lock adds nothing a synchronous read-change-write does not, and it brings the usual costs: every writer has to remember to take it, one that awaits inside it holds everyone else, and a forgotten one is invisible until it races. It would also have deadlocked the test that reproduces the bug, since the held update would own the lock the link waits for. That the test hangs under that fix rather than failing was itself a review finding, and the wait is now bounded so a future attempt gets a readable failure instead of a silent CI hang.

Per-field writes do not fit the value at stake. baseOid lives inside branches[ref], a nested map that is replaced whole, so a field-level write of branches is exactly the whole-map write that lost the base. A path-level write would work but is a second write API beside the merge, and the merge is already a per-field write at the top level.

A guard for baseOid alone, refusing to write a map that drops an entry the store still has, would have protected the one value and left the pattern in place for whatever is stored next, which the issue names as the reason not to. The applied-patch record the rebase used to restore is that next value, and it was already there.

changeSiteMeta is the smallest of the four: it changes where the read happens, not what is written, and it removes the pattern rather than guarding one instance of it.

One thing considered and declined: a runtime guard rejecting an async change function, which would otherwise store a Promise that serialises to {} and wipe the record. Every call site is in the same file and synchronous, the JSDoc says so, and a defensive check for a programmer error the surrounding code makes obvious is the speculative kind this repo's complexity rule asks not to add.

Review outcome (required — see AGENTS.md)

5 [fix here] · 6 [follow-up] over three passes. All 5 fixed; 4 of the 6 follow-ups taken as well because each was a few lines; the 2 deferred are in Risks and limitations with their reasons.

Fixed:

1. Architecture (pass 1, b5f24ff) [fix here]: wordpress:setup held the whole site map across the Git spawn that reads the clone's trunk info, so a write to any other site during it was undone. Read hoisted, write through changeSiteMeta. a651d4f, with a test red on b5f24ff.
2. Architecture (pass 1) [fix here]: sites:set-ticket read a branch's recorded base and wrote it back a yield later, rolling back a branches:rebase landing in between. The read is gone and the base is written only by the flow that created the branch. a651d4f, test red on trunk.
3. Architecture (pass 1) [fix here]: the migration's catch returned the record from before its git work, so a migration that lost the race reported an unmigrated site and branches:rebase refused a ticket whose base was on record. Re-reads now. a651d4f, test red on b5f24ff.
4. Architecture (pass 1) [fix here]: the AGENTS.md sentence claimed every write goes through changeSiteMeta; eight handlers write the store directly and correctly. Restated as the rule itself. a651d4f.
5. Architecture (pass 2, a651d4f) [fix here]: the migration's race-loser branch swallowed the error it returned past. Logged. b7303ee.
6. Tests (pass 1) [follow-up]: the staged test hangs rather than fails under a mutex-shaped fix, and node --test sets no timeout. The held wait is bounded with a message naming the deadlock. a651d4f.
7. Architecture (pass 2) [follow-up]: branches:rebase read the applied-patch record, resolved the scope with a Git spawn, then wrote back what it had read, so a discard or apply landing in the window was replaced by the old record. Taken as a fix rather than deferred: it is the same class, it was the one live counter-example to the rule this PR writes into AGENTS.md, and it costs a patch rather than a base. changeWorkMetaOn. b7303ee, test red on trunk and on a651d4f.
8. Tests (pass 2) [follow-up]: two guards pinned the number of store accesses a flow makes, which the tests' own comments call an implementation detail. Replaced by guards that the overlap happened. b7303ee.
9. Architecture (pass 3, b7303ee) [follow-up]: the work-meta change re-created a branch entry that had been deleted between the scope decision and the write. The write is conditional on the entry still being there. 6ec7e7e.
10. Two pass-3 observations, both taken in 6ec7e7e: the helper re-derived the branch from HEAD with a second Git spawn when its one caller had the ref in hand, so it takes the ref now like writeWorkMetaOn; and the rebase/discard test asserts the rebase's own write landed, the guard its sibling already had.

Deferred:

11. Architecture (pass 1) [follow-up]: workMetaScope decides a scope by reading, then the write follows a yield. The worst case is a flag written where nobody reads it, not a lost base; reworking the path [Fix] the false Update incomplete banner on trunk after an update run from a ticket #446 just settled is not worth it here.
12. Architecture (pass 2) [follow-up]: two flows can still move HEAD at once; this PR fixes the store half of that concurrency, and the mid-switch marker only refuses once a checkout has already failed.

• CodeRabbit: not run on 6ec7e7e. The check reads pass; its comment reads "Review limit reached, next included review available in 55 minutes". Merged inside that window on the author's decision, so no retry was requested; the three fresh-context passes above are the review coverage for this change.
• Review: completed. Separate agent context (Claude, fresh session per pass with the diff and .github/instructions/code-review.instructions.md only), three passes per step 3 of that file. Pass 1 head b5f24ff, pass 2 head a651d4f, pass 3 head b7303ee; base origin/trunk f31e5df throughout. Working tree clean at each pass; no uncommitted or untracked files in scope. Deterministic layer run by the author and reported to each pass: npm run lint clean, npm test and npm run test:electron green at every head (1312 tests at 6ec7e7e).
• Since review: b5f24ff → a651d4f → b7303ee, each re-reviewed in a fresh context rather than carried forward; pass 2 reconciled all four pass-1 fixes as resolved, pass 3 reconciled all five earlier fixes as still in place. b7303ee → 6ec7e7e is pass 3's own follow-up and observations: one function renamed to take a ref and drop a spawn, one guard line, one test assertion. No fourth pass was run; the six Overlapping writes to a site's metadata can drop a ticket branch's patch base #172 tests are red on trunk and green at 6ec7e7e, and the full suite passes on both runtimes.

Implementation notes

src/main.js

• changeSiteMeta(sitePath, change) sits above mergeSiteMeta, which is now that function with a shallow merge. It returns the record it wrote, which is what migrateSiteToBranches hands back.
• changeWorkMetaOn(sitePath, ref, change) resolves the scope first, because that is the only part that cannot be answered without yielding, then does one read-change-write against the ref its caller already has. Returning null from the change writes nothing, and so does an entry deleted between the scope decision and the write.
• mergeBranchMeta and branches:delete build the branches map inside the change function. wasActive in the delete is still decided from the HEAD read before it, because that is a question about the checkout, not about the record.
• sites:set-ticket leaves baseOid undefined on the existing-branch path, and the patch spreads it only when it is set. An entry created there has no baseOid key rather than an explicit null; patchBaseOid, branches:list, branches:rebase and site:status all read the two the same way.
• migrateSiteToBranches guards its write with now.branches ? now : … and its catch with a re-read, which are the same question asked at the two places the race is observable. The catch logs the error it returns past, so a genuine failure that coincides with another flow finishing the migration is not swallowed.

tests/unit/ipc-wiring.test.cjs

• The staging counts a flow's store accesses from the moment its Git work is done, then replays the whole flow once per access.
• The held wait is bounded at four seconds with a message naming the deadlock it expects, because node --test sets no timeout of its own.
• Each looping test carries a guard that the overlap actually happened, rather than one on how many times a flow reaches the store: some hold has to let the second flow through, and the first flow has to still have a write of its own to make afterwards.
• The migration tests hold the loser on its first call into ticket-branches, not on a flag the loser itself sets, or the loser wins and there is no race left to test.

Screenshots or recording

Nothing on screen changes. The fix is in what is stored, and the values stored are the same ones as before.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LEj32Dunw3y321YzBXNiWS

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 89f1bb12-c62c-4c9f-a312-b5033362cac9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}