Repository navigation
🤖 refactor: the workspace-turn manager and its handle store take their locks from SessionLocks - #7403
Merged
Merged
Conversation
…ends take their locks through SessionLocks
# Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
…ontract SessionLocksMemory keeps lock state in a table per build and waits on the HostClock, so a TestClock test moves time instead of waiting. withForeignHolders plays other backends: a live holder refuses at once with today's text, and a dead one is reclaimed. The contract now runs on node, memory, node + foreign and memory + foreign.
…nts, gate order and holder text Codex round 1 on #7367: an aborted signal wins over a fake live holder, the memory layer rejects with ENOENT when createParentDirectory is false and the parent is gone, the fake gate is probed after the lease is published, fake holders record an identity so the text equals the Node text for a current record, and fake pids are offsets from process.pid.
…the memory record lock Codex round 2 on #7367: an aborted signal wins over a missing parent, and a parent removed after the first check (or during the wait) refuses with ENOENT and frees the lock, as Node's write would.
…ake leases in isHeld Codex round 3 on #7367. The memory layer's parent checks did not converge, so a record lock with createParentDirectory false now goes to the Node locks, whose own write gives WorkflowRunStore the ENOENT it needs. Other record locks create their parent as Node does. The foreign layer's isHeld counts a live fake lease in the workspace's lease folder.
@typescript-eslint/await-thenable (CI lint-full): bun types rejects.toThrow and rejects.toBeInstanceOf as void.
…ild SessionLocks without bun:test The memory acquireProcessFileLock creates the lock's parent as fileLock.ts does, and writes no lock file (#7326 follow-up from #7367 round 4). buildSessionLocks moves to its own module so testHistoryService.ts, which the Jest integration tests load, can build a layer.
HistoryService gets a third constructor argument, locks (default nodeSessionLocks), and exposes it as readonly locks. The workspace FIFO and the history write lock go through it, so historyService.ts imports no lock module. HistoryLive yields SessionLocksTag, and createTestHistoryService takes a locks layer. historyService.foreignHolder.test.ts plays a live and a dead foreign writer, and the golden leak test also runs on SessionLocksMemory.
…m SessionLocks SessionUsageService and WorkspaceGoalService read historyService.locks.workspaceFiles at each use. SessionTimingService takes an optional locks argument (default nodeSessionLocks), and desktop.ts keeps the default. The three files import no lock module.
The turn use lease now comes from this.historyService.locks.useLeasesFor(config), so agentSession.ts makes no value import of workspaceUseLeases. The Node layer returns the same per-Config object as workspaceUseLeasesFor, so the lease files and the refusal text do not change. agentSession.useLease.test.ts gains a live foreign gate case: the foreign-holder layer plays another backend's mutation gate with no file on disk, and the turn is refused at once with today's text and never reaches the provider.
The memory layer's folder side effect (e9272f9) awaited fs.mkdir before a waiter joined the FIFO, so a later caller could queue first. The contract case "a release hands the file lock to the waiters in FIFO order" failed 14 of 1500 reruns that way, and once in make test. A synchronous mkdir keeps the call order: 0 of 1500 fail.
# Conflicts: # src/node/services/sessionLocks/memorySessionLocks.testHarness.ts # src/node/services/sessionLocks/sessionLocks.contract.test.ts # src/node/services/sessionLocks/sessionLocks.contract.testHarness.ts
…hods c0e84fa made both memory-layer folder writes synchronous so waiters queue in call order. The local no-sync-fs-methods rule refuses mkdirSync, so each call gets a disable comment with that reason, as serverCrashLogging.ts does.
# Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
…sion no longer imports
…locks from SessionLocks WorkspaceTurnManager takes the live-owner lock and its read-only probe from this.historyService.locks, and passes the same locks to its handle store. TaskHandleStore takes the publication lock from an optional locks argument (Node default), so its other callers do not change. Paths, timeouts and messages stay the same. Tests: - workspaceTurnManager.foreignOwner.test.ts writes no lock file. A dead owner is a foreign-holder fake that backend B reclaims, and a new case shows a fake live owner keeps the handle through keepIfOwnedElsewhere. - The harness gains a locks option, through historyServiceWithLocks in testHistoryService.ts. - The SessionLocks contract gains an inspect case on all four layers: an own record reads held, and a probe never reclaims a dead one.
…the merge of #7355's squash restored
# Conflicts: # .oxlintrc.json # eslint.config.mjs # scripts/noHostImports.test.ts
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.8k tokens (70% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 0s · 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 · 21.8s · 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
azaj01
pushed a commit
to azaj01/mux
that referenced
this pull request
Oct 25, 2026
…ake their locks from SessionLocks (coder#7404) ## Summary Workspace removal, the refine service and the durable event journal take their locks from `SessionLocks`. It is PR 12 of phase 4. Lock paths, timeouts, order and messages do not change: the Node layer gives today's `acquireProcessFileLock`. The branch-summary pending marker is not in this PR. It keeps its direct `acquireProcessFileLock`. Phase 4 PR 18 moves its writer and its reader together (see Scope). ## Order - Stacked on PR 11 (coder#7403), which has merged, in the native stack, as the plan orders. - It uses `historyService.locks` from PR 9 (coder#7372), which has merged. Phase 5b PR 4 waits for it. ## Implementation - `workspaceRemoval.ts`: `withRemovalLocks` takes the refine and history locks from an optional `locks` (Node default). `removeSessionDirUnderMemoryLocks` and `sealSubAgentForRemovalUnderMemoryLocks` forward it. - `workspaceService.ts`: its 2 removal calls pass `this.historyService.locks`. `acquireRefineSerializationLock` takes its file lock from `this.historyService.locks` too. Why: it locks the same path as the refine service's apply lock, so both must use one layer or a reset stops contending with a refine apply. The plan had this line in PR 13. Codex found the split on round 4, so round 5 moves it here. - `refinement/refineService.ts`: the apply lock and the staging lock use `this.historyService.locks`. - `utils/journal/durableEventJournal.ts`: the constructor takes `locks` (Node default) for the blob lock and passes it to its `Journal` for the append lock. Files beyond the plan's list, each a pass-through of locks that a caller already has (landing note 14): - `cli/debug/cache-audit.ts` and `replay/replayFixture.ts`: the journal gets the history service's locks that each one already builds. Why: they are production callers with a service in scope. Callers that keep the Node default, because no service is in scope: the process-wide journal registry (`sharedDurableEventJournal`), `replayVerifySession`, and the call-record scenarios, so the PR 2 and 3 fixture stays unchanged. ## Scope: the branch-summary marker moves to PR 18 The plan lists `branchSummary.ts` for this PR. Codex found a real defect in the first version (thread on `branchSummary.ts`): the marker reader finds a held marker with `fs.stat` on the lock file. An injected memory `SessionLocks` never creates that file, so the reader stops seeing the writer. The correct fix needs a held-lock probe on every `SessionLocks` layer. By AGENTS.md ("remove the surface from this PR instead of growing it"), this PR leaves `branchSummary.ts`, `agentSession.ts` and the branch-summary call in `workspaceService.ts` as they are on main. Phase 4 PR 18 delivers the marker move with the probe. The finding is tracked in coder#7326. ## Validation On the head `4eabff0f7b` (PR 11 merged on main as `214ec69d6a`, plus this PR), every gate passed: - `make static-check` and `make lint-eslint` pass. `make crap-check`: PASS, 0 violations, 3 warnings on touched hotspots (base `214ec69d6a`, total CC of the changed files 4233 -> 4234). - `bun test` of `workspaceRemoval.test.ts`, `refinement/`, `branchSummary.test.ts`, `utils/journal/`, `workspaceService.*.test.ts`, `replay/` and `cli/debug/cache-audit.test.ts`: 1409 pass, 0 fail. - The tripwire (`scripts/perf/loopTestBaseline.ts --runs 3 --base origin/main`): 0 trips. - `make test`: 0 fail. The plan adds no test for this PR: the moved files keep their tests, which pass with no edit. The refine serialization line is a one-argument pass-through with no test of its own: in the app, both sides of the lock are `nodeSessionLocks`. ## Mutation check `make mutate-check` (base `214ec69d6a`): 130 killed, 99 survived, 1 equivalent, 0 without a test, 93 not run. No survivor is on a line that this PR changes. I list them in coder#7326 for PR 17: - `removeSingleWorkspaceUnlocked` (`workspaceService.ts`): 35. - `RefineService.applyLocked`: 22, and `RefineService.runLocked`: 12. - `cacheAuditCommand` (`cli/debug/cache-audit.ts`): 20. - `removeSessionDirUnderMemoryLocks` (`workspaceRemoval.ts`): 10. - Not run (93): past the default budget of 300 mutants per run. ## Risks - None for behavior: in the app, `historyService.locks` is `nodeSessionLocks`, which calls the same `acquireProcessFileLock` with the same arguments. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$161.10`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=161.10 -->
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 workspace-turn manager and its handle store take their locks from the history service's
SessionLocks. It is PR 11 of phase 4. Lock paths, timeouts and refusal texts do not change: the Node layer gives today'sacquireCrossProcessLock,inspectCrossProcessLockandacquireProcessFileLock.Order
historyService.locksfrom PR 9 (🤖 refactor: HistoryService takes its session locks from SessionLocks #7372), which has merged.Implementation
workspaceTurnManager.ts: the live-owner lock (acquireTurnOwnerLock) and its read-only probe (keepIfOwnedElsewhere) callthis.historyService.locks. The manager passes the same locks to its handle store. The file imports no lock module now.taskHandleStore.ts: the publication lock comes from an optionallocksconstructor argument (Node default). The workspace-turn manager, its only production caller, passes its history service's locks. The one other caller, the call-record scenario harness (sessionLocks.callOptions.scenarios.testHarness.ts), keeps the default, so the PR 2 and 3 call-record fixture stays unchanged.workspaceTurnManager.testHarness.ts: alocksoption, so a test can give one backend a foreign-holder layer.workspaceTurnManager.foreignOwner.test.ts: it writes no lock file now. A dead owner is a foreign-holder fake that backend B reclaims. A new case shows that a fake live owner keeps the handle throughkeepIfOwnedElsewhere.sessionLocks.contract.testHarness.ts: an inspect case on all four layers. A record this backend holds readsheld, and a probe never takes or reclaims a dead record.One file goes beyond the plan's list:
testHistoryService.ts:historyServiceWithLocks(config, layer), a HistoryService on an existing root with the locks of a given layer. Why: the foreign-owner test runs two backends on one root, and only one of them gets the foreign-holder layer.createTestHistoryServicemakes its own temp root, so it cannot do that.Validation
On the head
a1de29a8f4(PR 10's final head merged in), every gate passed:make static-checkandmake lint-eslintpass.make crap-check: PASS, 0 violations.bun testofworkspaceTurnManager*.test.ts,taskHandleStore*.test.ts,taskService*.test.tsandsessionLocks/: 1647 pass, 0 fail.scripts/perf/loopTestBaseline.ts --runs 3 --base phase4/agent-lease): 0 trips.make test: 27842 pass, 0 fail in the main run, and every other run passes.The head
14c059df97merges main after #7394's squash. The merge brings in only main's commits, and the diff against main has the 6 files above. On this head,make static-checkpasses and the plan tests give 1647 pass, 0 fail.Revert check: I restored the direct lock calls in
workspaceTurnManager.tsandtaskHandleStore.ts. Then 2 cases ofworkspaceTurnManager.foreignOwner.test.tsfailed: "another backend settles a handle whose owner died" and "a fake live owner on the lock keeps the handle, with no lock file". The contract's inspect case adds coverage ofinspectCrossProcessLockon all four layers. It tests no line that this PR changes.Mutation check
make mutate-check: 1 killed, 2 survived. Both survivors are inacquireTurnOwnerLock, on lines that this PR does not change. I list them in #7326 for PR 17::746acquireTimeoutMs: 0 -> 1. A 1 ms wait does not change the outcome of any test.:757delete !onerror instanceof CrossProcessLockTimeoutError. This only changes which errors write a warning to the log.Risks
historyService.locksisnodeSessionLocks, which calls the same lock functions with the same arguments.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$149.51