Repository navigation
Conversation
Two writers that both found the store lock naming a dead pid could both take it. Each read the dead holder, then ran unlink and an exclusive create. When the second writer's unlink ran after the first writer's create, it deleted the first writer's live lock and created its own, so both held the lock and their read-modify-write of units.tsv or ledger.tsv could drop rows. A takeover now runs behind a claim: a directory, .orch.lock.takeover, holding one file named for the claimant's pid. The claimant stages that directory and renames it into place. rename cannot replace a directory that holds a file, so only one writer claims at a time and the others fail with "being replaced by another writer; retry". Under the claim, the writer reads the lock again and unlinks it only if it still names the holder it judged replaceable. A claim left by a dead writer is removed by its pid-named file, which cannot remove a live claimant's. The forced takeover goes through the same claim. store.ts and orch.test.ts are now declared forks in tools/forks.json.
orch frontier set lists each branch's pull requests with --state all, because a merged or closed PR still anchors the chain below the current branch. A branch that had a merged or closed PR and now has a new open one returned all of them, and frontier resolution failed with "has multiple pull requests (chhoumann#11, chhoumann#9, chhoumann#8); close the stale ones first". Closing does not help, since the old PRs are already closed or merged. When a branch has an open PR, that PR is the branch's current one, and its merged and closed PRs are ignored. Two open PRs on one branch still fail, and the error now names only the open ones. A branch with no open PR keeps the old rule: exactly one PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in the orchestrate store,
plugins/pstack/skills/poteto-mode/scripts/orch/store.ts. Both are in Lauren's currentstore.ts(unchanged upstream sincece649c0), so the fix makesstore.tsandorch.test.tsdeclared forks intools/forks.json.1. Two writers can both take a stale lock
When
.orch.locknames a dead pid,acquireLockreads the holder, then runsunlinkand an exclusive create. Two writers can both read the dead holder. If the second one'sunlinkruns after the first one's create, it deletes the first writer's live lock and creates its own. Both then hold the lock, and their read-modify-write ofunits.tsvorledger.tsvdrops rows.Reproduction
A harness writes a dead pid into
.orch.lock, starts 6 processes that each open the store and rununits.addwith their own id, and counts the rows inunits.tsvagainst the processes that reported success. Onchhoumann/main, 200 trials:17 successful
units.addcalls left no row, so two writers held the lock at once. With this branch:The new test "keeps a lock another writer took over from the same dead holder" pins the interleaving deterministically: in
onStaleLock, which runs after the late writer reads the dead holder, it lets a rival finish its takeover. Onmainthe late writer then deletes the rival's lock and succeeds.Fix
A takeover now runs behind a claim: a directory,
.orch.lock.takeover, that holds one file named for the claimant's pid. The writer stages that directory and renames it into place.renamecannot replace a directory that holds a file, so one writer claims at a time and the others fail withstore lock held by pid <pid> is being replaced by another writer; retry. Under the claim the writer reads the lock again and unlinks it only if it still names the holder it judged replaceable. A claim left by a dead writer is removed by its pid-named file, which cannot remove a live claimant's file. The forced takeover goes through the same claim. The lock file format does not change.I chose this over renaming the stale lock aside first. A rename takes whatever file is at the path, including a lock another writer just created, and putting it back leaves a gap in which a third writer can create the lock. An exclusive-create claim file would avoid that, but a writer that dies holding it leaves nothing that says whether the claimant is alive. The directory claim is the smallest form I found that recovers from a dead claimant.
2. A reused branch fails frontier resolution
orch frontier setlists each branch's PRs with--state all, because a merged or closed PR still anchors the chain below the current branch. A branch that had a merged or closed PR and now has a new open one returns all of them, andbranchPullRequestrefuses:Closing does not help: #9 and #8 are already closed or merged.
Fix
When a branch has an open PR, that PR is the branch's current one, and its merged and closed PRs are ignored. Two open PRs on one branch still fail, and the error names only the open ones. A branch with no open PR keeps the old rule of exactly one PR.
Tests
Six new tests in
orch/orch.test.ts: the two lock tests, and for bothghandorigin, a reused branch resolving to its open PR and a branch with two open PRs failing. Onchhoumann/main'sstore.ts, all six fail:On this branch:
GIT_CONFIG_GLOBAL=/dev/null bun test orch watch-pr check-plan.test.ts: 125 pass, 0 failbun run typecheck: cleanbunx prettier@3.6.2 --check watch-pr/settle.ts watch-pr/settle.test.ts resolve-codex-transcript.ts: cleanGIT_CONFIG_GLOBAL=/dev/null bun test tests/ --timeout 30000: 550 pass, 46 skip, 0 failbun tools/generate.mjs --check: 17 generated files current, all checks passbun tools/sync.mjs {pstack,cursor-team-kit,thermos} 88d4751 --dry-run: exit 0 for each; pstack listsorch/store.tsandorch/orch.test.tsas declaredpolicyforksRelease
This PR does not bump
VERSIONor add aCHANGES.mdentry, so the fix is inert on installed copies until a release. I left the release to you, since other fix PRs are open at the same time. Thesincefield intools/forks.jsonsays2.0.0-rc.16; change it if the fix ships in a different release.Race harness
race.ts:worker.ts: