Repository navigation
🤖 refactor: the target mutation and rollback locks take their file lock from SessionLocks - #7423
Merged
Merged
Conversation
… from SessionLocks withTargetMutationLock(s), acquireRollbackFileLock and rollbackRefinement take an optional SessionLocks (default: the Node layer). Removal, its tombstone rollback, WorkspaceService's refine serialization lock and SessionUsageService's headless usage write pass their history service's locks. The 9 other target-lock callers, the rollback tool and the CLI keep the default.
…ocks to the rollback turnRequestBuilder puts its history service's locks in the tool assembly's sandbox context, and refinement_rollback passes them to rollbackRefinement for the rollback and target 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 · 222.0k tokens (74% 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 (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 18.0s · 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 |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Oct 24, 2026
This was referenced Oct 24, 2026
Merged
Closed
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…O from SessionLocks (coder#7425) ## Summary The report, failure, transcript and git patch artifact index writes take the workspace FIFO from `SessionLocks`. It is PR 14 of phase 4. Lock keys and order do not change: the Node layer gives today's `workspaceFileLocks`. ## Order - PR 13 (coder#7423), which the plan puts below it, has merged, so the base is main. PR 18 (the branch-summary marker, landing note 15) stacks on it in the native stack. - Phase 5b PR 3 waits for it. ## Implementation - `subagentFailureArtifacts.ts`, `subagentReportArtifacts.ts`, `subagentTranscriptArtifacts.ts`, `subagentGitPatchArtifacts.ts`: each index write takes an optional `locks` (Node default). In `subagentReportArtifacts.ts`, the helper `workspaceFileLocksOf` holds the fallback, so the CC of `upsertSubagentReportArtifact` does not grow. - `workspaceService.ts`: the child-artifact rollup passes `this.historyService.locks` for the parent's index writes. Files beyond the plan's list (landing note 14). The plan said that `taskService.ts`, `gitPatchArtifactService.ts` and `taskGitPatchEngine.ts` do not change. These changes close the gap of a production caller that has a service but keeps the Node default: - `taskService.ts`: the failure and report upserts pass `this.historyService.locks`, and so does `new GitPatchArtifactService(config, locks)`. - `gitPatchArtifactService.ts`: the constructor requires `locks`, so the type checker proves that no caller is left. Its 4 upserts pass them. `gitPatchArtifactService.test.ts` passes `nodeSessionLocks` at its 4 call sites. One test now records the locks that the service takes. - `taskGitPatchEngine.ts`: `TaskGitPatchApplyConfig` picks `sessionLocks`, and the applied watermark write passes it. - `common/utils/tools/tools.ts`: the `sessionLocks` comment names the watermark write. The `task_apply_git_patch` tool gets the locks through its tool config, which `turnRequestBuilder` already sets. - `turnRequestBuilder.ts`, `workflows/WorkflowService.ts`, `cli/workflow.ts`: each workflow patch config sets `sessionLocks` from its history service. `WorkflowServiceContext` gains `historyService: Pick<HistoryService, "locks">`. The ORPC context already has it. `WorkflowService.context.test.ts` adds it to its test context. The 4 artifact functions keep an optional `locks`: a required one breaks about 25 test files. ## Validation On the stack head `c0831c924e` (PR 14 on PR 13's final head `41aab3e5b4`), every gate passed: - `make static-check` and `make lint-eslint` pass. `make crap-check`: PASS, 0 violations, 7 warnings on touched hotspots (total CC of the changed files 10107 -> 10112: the 4 artifact functions each gain one `locks ?? nodeSessionLocks` fallback, and `workspaceFileLocksOf` holds the report one). - `bun test` of the 4 `subagent*Artifacts.test.ts`, `historyService.test.ts`, `taskService.agentReports.test.ts`, `taskService.handoffAndCleanup.test.ts`, `gitPatchArtifactService.test.ts`, `taskGitPatchEngine.test.ts`, `workflows/WorkflowService.context.test.ts`, `workflows/WorkflowTaskServiceAdapter.test.ts` and `tools/task_apply_git_patch.test.ts`: 589 pass, 0 fail. - The tripwire (`scripts/perf/loopTestBaseline.ts --runs 3 --base 41aab3e`): 0 trips. - `make test`: 0 fail in every run. The head `0ae88d7ef7` is the same 5 commits rebased on main `d1f9d61489` (after coder#7423). Its diff against main has the 14 files above. On this head, `make static-check` passes and the plan tests give 589 pass, 0 fail. Test: `gitPatchArtifactService.test.ts` gives the service a `SessionLocks` whose workspace FIFO records each key, and checks that the parent's index writes take it. Revert proof: with the 4 upserts back on the Node default, that test fails. ## Mutation check `make mutate-check` (base `6887934d00`, so PR 13's lines are in scope too): 167 killed, 128 survived, 4 equivalent, 1 without a test, 682 not run. No survivor is on a line that this PR changes. I list them in coder#7326 for PR 17. - The 4 equivalents are the `locks ?? nodeSessionLocks` fallbacks, each with a `mutation-equivalent` marker: `locks` is a `SessionLocks` object or undefined, so `??` and `||` agree. - The 1 without a test is `cli/workflow.ts`, and the local baseline reports it as failed. That is a local limit: mutate's worker copies hold only tracked files, and `cli/workflow.test.ts` runs the built `dist/cli/index.js`, which they lack. The test passes in the checkout (10 pass, 0 fail). The PR adds one `sessionLocks` field there, with no operator to mutate. CI's Quality / Mutation is the check of record. - Survivors: `GitPatchArtifactService.generate` 29, `maybeStartGenerationUnlocked` 20, `taskGitPatchEngine.applyProjectPatch` 31 and `applyPatch` 21, `upsertSubagentReportArtifact` 7, `upsertSubagentFailureArtifact` 3, and 5 more in the other artifact functions. PR 13's survivors (`rollbackRefinement` 9, `withTargetMutationLocks` 2, `acquireRollbackFileLock` 1) repeat and are already listed. ## Risks - None for behavior: in the app, `historyService.locks` is `nodeSessionLocks`, which gives the same `workspaceFileLocks` FIFO. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$163.55`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=163.55 -->
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…ugh the HistoryService's SessionFiles (coder#7428) ## Summary 5b PR 5 of phase 5 (Loop host I/O) of the agent-loop refactor. `SessionUsageService` reads and writes its files through the `SessionFiles` of its HistoryService (phase 2) instead of `fs/promises` and `writeFileAtomic`: - the settled ledger `session-usage.json`, - the in-flight files of running streams (`session-usage.in-flight/<key>.json`), - the headless sidecar `headless-usage.jsonl`. All three live in the session dir, so they are session files. The service already takes its locks from the same HistoryService (phase 4). Now a memory `SessionFiles` layer keeps these files off the disk too. Behavior does not change: the default is the Node layer (`nodeSessionFiles`), which calls the same `fs/promises` functions and `writeFileAtomic`. Its members read their bindings at call time, so the existing tests that spy on `fs/promises` still intercept these calls. ## Order Plan order: stack 5b is PR 1 to PR 9. This PR waits for phase 4 PR 13 (coder#7423, merged), which also changed `sessionUsageService.ts`. It uses no code of 5b PRs 3 and 4, so it opens on its own, with base main. ## Implementation - `services/sessionUsageService.ts`: a private `files` getter returns `historyService.files`. Like the `fileLocks` getter beside it, it reads at each use, not at construction. The 11 `fs/promises` calls and the 2 `writeFileAtomic` calls go through it, and the module-level `readInFlightUsageFile` takes the files as its first argument. The file has no `fs` import now. - `recordHeadlessUsage` checks the removal tombstone through the same files (`isWorkspaceRemovalTombstoned(rootDir, workspaceId, this.files)`), like HistoryService's own check. The files argument of that function came with 5b PR 4 (coder#7426, merged), so a later commit of this PR adds it after a merge of main. The plan estimated about 420 lines. This PR adds 115 and removes 18, because the service reaches the files through the HistoryService it already has, so no caller changes. ## Validation - `sessionUsageService.test.ts`: a second new case puts a tombstone only in the memory volume, and `recordHeadlessUsage` refuses the write (undefined) and creates no session dir. Without the files argument of the check, the write succeeds and the case fails. - `sessionUsageService.test.ts`: a new case builds HistoryService on memory `SessionFiles`. It records a ledger entry, an in-flight step and a headless sidecar line. The memory session dir holds all three, and a restarted service folds the in-flight file into the ledger from memory (120 input tokens) and deletes it there. The session dir on disk does not exist. With main's `sessionUsageService.ts`, the case fails (ENOENT on the memory readdir). - The plan's tests (`sessionUsageService.test.ts`, `sessionUsageService.inFlight.test.ts`, `streamManager*.test.ts`) plus `agentStatusService.test.ts` and `tokenizerService.test.ts`: 513 pass, 0 fail. The 8 other test files that use `SessionUsageService`: 520 pass, 0 fail. The on-disk tests read `session-usage.json` and the in-flight files from disk, and they pass unchanged. - `make static-check`, `make crap-check` and `make dry-check` pass. `make lint-eslint` first flagged an any-typed matcher (`expect.arrayContaining`) in the new case, and a test-only commit replaced it with `toContain` checks. - `make test` (host suite lock) on head 6ce1a31: `bun test src` had 27905 pass, 7 skip, 0 fail. The shared scripts/storybook/vscode run had 729 pass, 1 skip, 0 fail, and the isolated files had 0 fail. Loop-test tripwire, 3 runs against main: 0 trips. - After the lint fix and a merge of main be78b8f: `make static-check` and `make lint-eslint` pass, and the plan's tests have 416 pass, 0 fail. - After the tombstone commit and a merge of main 9c841fa (coder#7426): typecheck, `make static-check`, `make lint-eslint`, `make crap-check` and `make dry-check` pass, and the plan's tests plus `workspaceRemoval.test.ts` have 428 pass, 0 fail. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$195.58`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=195.58 -->
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 target mutation lock and the refinement rollback lock take their file lock from
SessionLocks. It is PR 13 of phase 4. Lock paths, the 2 s waits, order and messages do not change: the Node layer gives today'sacquireProcessFileLock. The in-process mutexes stay in their modules.Order
Implementation
refinement/targetMutationLocks.ts:withTargetMutationLockandwithTargetMutationLockstake an optional lastlocksargument (Node default) for the file lock.refinement/refinementRollback.ts:rollbackRefinementtakeslocksin its options for the rollback lock and the target locks.acquireRollbackFileLock(sessionDir, locks)defaults to the Node layer.workspaceRemoval.ts: the removal target lock androllbackRemovalTombstoneIfOwneduse the caller'slocks.workspaceService.ts: the 2 tombstone rollbacks passthis.historyService.locks. The plan also lists the refine serialization lock here (workspaceService.ts:4478). PR 12 (🤖 refactor: removal, refine and the durable journal take their locks from SessionLocks #7404) moved it in its round 5, because Codex found it split from its partner lock.sessionUsageService.ts: the headless usage target lock passesthis.historyService.locks.The 9 other callers of the target lock (plan claim 2.1.1) keep the default with no edit:
reviewStateService.ts,mcpAppResultStore.ts,memoryService.ts,tools/agent_skill_write.ts,tools/agent_skill_delete.ts,devToolsService.ts,creationDraftService.ts,draftService.tsandbackup/adapters.ts. The debug CLIcli/debug/refinements.tshas no service, so its rollback keeps the default too.Files beyond the plan's list, each a pass-through of locks that a caller already has (landing note 14):
tools/refinement_rollback.ts,toolAssembly.ts,turnRequestBuilder.ts: therefinement_rollbacktool passes locks torollbackRefinement.turnRequestBuilderputshistoryService.locksin the tool assembly's sandbox context. Why: the tool is a production caller of the new option, and its turn has a history service.tools/refinement_rollback.test.ts: a new case that gives the tool a context whoselocksrecord each acquire, and checks that the rollback lock comes from them. Revert proof: withoutlocks: ctx.locksin the tool, the case fails.Validation
On the stack head
2456cafda9(PR 13 on PR 12's final head4eabff0f7b), every gate passed:make static-checkandmake lint-eslintpass.make crap-check: PASS, 0 violations, 5 warnings on touched hotspots with unchanged CC (total CC of the changed files 5117 -> 5118).bun testofrefinement/,workspaceRemoval.test.ts,backup/,sessionUsageService*.test.ts,toolAssembly.test.ts,turnRequestBuilder.test.tsandtools/refinement_rollback.test.ts: 686 pass, 0 fail.scripts/perf/loopTestBaseline.ts --runs 3 --base 4eabff0f7b): 0 trips.make test: 27851 pass, 0 fail in the main run, and every other run passes.The head
41aab3e5b4is the same 3 commits rebased on main6887934d00(after #7404 and #7417). Its diff against main has the 9 files above. On this head,make static-checkpasses and the plan tests give 686 pass, 0 fail.Mutation check
make mutate-check(base214ec69d6a, so PR 12's lines are in scope too): 187 killed, 113 survived, 0 without a test, 466 not run. No survivor is on a line that this PR changes. I list them in #7326 for PR 17:prepareOrThrow(turnRequestBuilder.ts): 45. The PR adds onelocksfield.RefineService.applyLocked: 22,RefineService.runLocked: 12, andcacheAuditCommand: 20. These are PR 12's, already listed.rollbackRefinement: 9, andacquireRollbackFileLock: 1 (refinementRollback.ts).withTargetMutationLocks: 2, andcreateRefinementRollbackTool: 2.Risks
historyService.locksisnodeSessionLocks, which calls the sameacquireProcessFileLockwith the same arguments.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$163.55