Repository navigation
🤖 refactor: the branch-summary pending marker takes its lock and a read-only probe from SessionLocks - #7427
Merged
Conversation
… file lock Every layer answers it: the Node layer stats the lock file, the memory layer reads its table, and the foreign holders see their own entries. A contract case checks own, live and dead holders, and that the probe takes no lock.
… from SessionLocks The writer and the reader move together: the reader probes the marker through the same locks as the writer's acquire, since a memory lock leaves no file to stat. agentSession and workspaceService pass the history service's locks.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 31 model calls · 218.7k tokens (70% cached) All 14 tests in 9 files
e2e 0.20.0 · 2m 55s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.1k tokens (21% cached) All 3 tests in 3 files
e2e 0.20.0 · 18.6s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
This was referenced Oct 25, 2026
Closed
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 26, 2026
…rker, subagent artifacts and tool configs (coder#7506) ## Summary No-behavior-change refactor for the readiness-advisor follow-ups of phase 4 PRs 13, 14 and 18 in coder#7326. The branch-summary marker, the four subagent artifact writers, the patch-apply watermark and `ToolConfiguration` now require the caller's `SessionLocks` instead of falling back to the Node layer, and the two skill tools pass the tool config's locks to the target mutation lock. Two production sites that built a `ToolConfiguration` without locks now pass `historyService.locks`. In the app every one of these resolves to `nodeSessionLocks`, so the locks taken do not change. Refs coder#7326 ## Items | coder#7326 item | Result | | ---------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | PR 18 (coder#7427) advisor 1: `startAbandonedBranchSummaryInBackground` and `awaitPendingBranchSummary` fall back to `nodeSessionLocks` | Done. Both take an optional `marker: BranchSummaryMarker` (`{ sessionDir, locks }`) instead of `sessionDir?` plus an optional or defaulted `locks`. The locks are needed only for the marker, so a caller that names the marker must name its locks, and the calls without a session dir stay as they are. | | PR 18 (coder#7427) advisor 2: the call-record branch-summary scenario uses the default locks | Done. `oneFork` passes `historyService.locks` to the writer and the reader. The recorded fixture does not change. | | PR 14 (coder#7425) advisor 1: the 4 artifact functions keep an optional `locks` | Done. `locks` is required in `subagentFailureArtifacts.ts`, `subagentReportArtifacts.ts`, `subagentTranscriptArtifacts.ts` and `subagentGitPatchArtifacts.ts`, and in `applyProjectPatch` (`taskGitPatchEngine.ts`), which passes it through. Every production call already passed locks. The test calls pass `nodeSessionLocks`. | | PR 14 (coder#7425) advisor 2: `ToolConfiguration.sessionLocks` is optional | Done. It is required. `createTestToolConfig` supplies `nodeSessionLocks`. Two production configs left it out and now pass `historyService.locks`: the skill-write config of `RefineService` and the IPC bash tool of `WorkspaceService`. `todo.ts` and `review_pane.ts` drop their `?? nodeSessionLocks`. | | PR 13 (coder#7423) advisor: 9 callers of the target mutation lock take the Node default | Partly done. `agent_skill_write.ts` and `agent_skill_delete.ts` pass `config.sessionLocks` (6 sites), with a test each. The other 7 (`memoryService.ts`, `reviewStateService.ts`, `mcpAppResultStore.ts`, `devToolsService.ts`, `draftService.ts`, `creationDraftService.ts`, `backup/adapters.ts`) have no `SessionLocks` in scope: each needs a new constructor dependency and about 300 test constructions, for no change in the app. Reasoned close in coder#7326. | | PR 12 (coder#7404) advisor: the durable journal callers | Not in this PR. They touch the files of coder#7485, which is still open. | ## Validation - The two new tests ("takes the target lock from the config's session locks", in `agent_skill_write.test.ts` and `agent_skill_delete.test.ts`) fail when the tools drop `config.sessionLocks` and pass with it. - The other changes are type-level: `tsc` fails on any caller that leaves the locks out. - `sessionLocks.callOptions.test.ts` passes with the fixture unchanged. - `git merge-tree` of this head with each open `turn-engine/*` PR and with coder#7491 gives no conflict that this PR causes. coder#7488 conflicts with main in `streamManager.ts`, a file that this PR does not change. - Mutation gate: the final `make mutate-check` run (`--max-mutants 900`, base e803893) found 287 survivors, all in `AgentSession.prepareMessage`. That is the hotspot that coder#7475 lists, and this PR changes only one argument there. I did not chase them. The run stopped at its time budget with 438 mutants not run, so it did not reach the other changed functions. `MUTATION_MODE` is `warn`. ## Risks Low. In the app the injected locks are `nodeSessionLocks` everywhere, so no lock path or lock kind changes. Most of the diff is test call sites that now name `nodeSessionLocks`. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$209.75`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=209.75 -->
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.
Summary
The branch-summary pending marker takes its lock from
SessionLocks, and its reader finds a held marker through a new read-only probe,SessionLocks.processFileLockExists. It is PR 18 of phase 4 (landing note 15). In the app nothing changes: the Node layer's probe is the same singlefs.statof the lock file that the reader makes today, and the writer and reader getacquireProcessFileLockwith today's arguments.Why
PR 12 (#7404) first moved the marker's writer and reader to
SessionLocks. Codex found that the reader found a held marker withfs.staton the lock file, which an injected memorySessionLocksnever creates. So the reader stopped seeing the writer in that layer. #7404 left the marker as it is on main, and this PR moves the writer and the reader together, with a probe that every layer answers. Tracked in #7326.The existing API does not cover the probe. Both the Node and the memory
acquireProcessFileLockasserttimeoutMs > 0, so a try-acquire with timeout 0 is refused. A short try-acquire would also create the lock's folder and lock file on every send and contend with the writer, which changes what the writer sees.Order
awaitPendingBranchSummary.Implementation
sessionLocks/sessionLocks.ts:SessionLocksgainsprocessFileLockExists(lockPath): true while any holder (this process or another, live or dead) has the lock. It never takes or reclaims the lock.utils/concurrency/fileLock.ts:processFileLockExists, onefs.statof the lock path.nodeSessionLocks.tsforwards to it.sessionLocks/memorySessionLocks.testHarness.ts: the memory layer answers from its lock table. A dead holder stays until an acquire reclaims it, as its lock file would on the disk.sessionLocks/foreignHolders.testHarness.ts: a fake foreign holder, live or dead, makes the probe true. Otherwise the inner layer answers.sessionLocks/sessionLocks.contract.testHarness.ts: a contract case for all 4 layers (Node, memory, and each with foreign holders): own, live and dead holders, and the probe takes no lock.branchSummary.ts:startAbandonedBranchSummaryInBackgroundtakeslocksin its input (Node default) for the writer.awaitPendingBranchSummarytakeslocks(Node default) for the probe and the wait.agentSession.tsandworkspaceService.ts: they passthis.historyService.locksto the reader and the writer.Validation
On the stack head
d24c5642c4(PR 18 on PR 14's stack headc0831c924e), every gate passed:make static-checkandmake lint-eslintpass.make crap-check: PASS, 0 violations, 9 warnings on touched hotspots (total CC of the changed files 13439 -> 13446).bun testofsessionLocks/,branchSummary.test.ts,agentSession*.test.ts,workspaceService.*.test.tsandutils/concurrency/: 2668 pass, 0 fail.scripts/perf/loopTestBaseline.ts --runs 3 --base c0831c924e): 0 trips.make test: 27903 pass, 0 fail in the main run, and every other run passes.The head
6a6948c03eis the same 2 commits rebased on mainbe78b8fed3(after #7425). Its diff against main has the 10 files above, 103 insertions and 25 deletions. On this head,make static-checkpasses, andsessionLocks/,branchSummary.test.tsandutils/concurrency/give 191 pass, 0 fail.Tests
branchSummary.test.ts: the r48 cross-process wait runs on the Node layer and on the memory layer, through one helper. The memory case is new.fs.stat: the memory r48 case fails.nodeSessionLocks: the memory r48 case fails.Mutation check
make mutate-check BASE=<PR 14 head>: 103 killed, 197 survived, 287 not run. Every survivor is inAgentSession.prepareMessage(CC 421), where the PR adds one argument, and the 300-mutant budget runs out there. So I ran a second, scoped check on a scratch commit that leavesagentSession.tsas on main: 38 killed, 81 survived, 1 equivalent, 0 without a test, 13 not run. No survivor of either run is on a line that this PR changes. I list them in #7326 for PR 17:WorkspaceService.fork: 78. The PR adds onelocksfield.awaitPendingBranchSummary: 2, andstartAbandonedBranchSummaryInBackground: 1.input.locks ?? nodeSessionLocks, with amutation-equivalentmarker.Risks
historyService.locksisnodeSessionLocks. Its probe is the samefs.statas before, and its acquire calls the sameacquireProcessFileLockwith the same arguments.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$167.23