Repository navigation
🤖 tests: SessionLocksMemory keeps use leases in its table and makes lock folders through HostFiles - #7376
Merged
Conversation
…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.
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.
…ck folders through HostFiles The memory layer's useLeasesFor holds each lease as a table entry at the lease file's path: hold publishes the lease, then probes the gate entry, and refuses with today's WorkspaceMutationInProgressError text. isHeld sees every live lease of that kind on the table. The contract gets the gate and isHeld cases on all four layers, which replace the two node-only foreign tests. The lock folders go to an injected HostFiles (SessionLocksMemory now needs HostFilesTag), so the golden run on the memory volume leaves nothing under <root>/locks. A waiter takes its FIFO place before it awaits its folder, and a failed mkdir gives up the place or the lock. The FIFO case now holds the mkdir calls and readies the later folder first, so it fails at once without that order.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d305291a12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
🟢 e2e stories (Haiku): 14 passed27 agent steps · 9 replayed from cache · 26 model calls · 162.7k tokens (72% cached) All 14 tests in 9 files
e2e 0.20.0 · 2m 13s · 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 · 20.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 |
…kdir The Node lock checks the caller's signal after its mkdir. The memory layer took a free lock before the mkdir, so an abort during it still returned the lock. It now checks the signal after the mkdir and frees the lock it took.
This was referenced Oct 25, 2026
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Oct 26, 2026
…ck attempt count, temp lock folders, foreign ENOENT) (coder#7493) ## Summary Test-only PR for the open test items of coder#7326 (phase 4 review follow-ups). It fixes one temp-root leak, removes a host-load race from the try-lock test, cleans up the temp lock folders of `crossProcessLock.test.ts`, and makes the foreign-holder test layer give ENOENT for a missing parent as the Node lock does. No product code changes. Refs coder#7326 ## Items | coder#7326 item | Result | | --- | --- | | Finding 3: a failed seed append leaks the temp root of the refine scenario | Done. `oneRefineApply` (`sessionLocks.callOptions.scenarios.testHarness.ts`) now opens its `try` right after `createTestHistoryService()`, so the seeding (folder, config edit, appends) runs inside it and `cleanup()` always runs. The scenario is a function that a test calls, not a test file, so a `try` is its `beforeEach`. | | PR 17 advisor item 1: the T5 test races one try-lock attempt against a 1 s timer | Done. The test counts the attempts (each one links a record onto `lockPath`) and expects exactly 1. A second attempt moves the frozen clock past the deadline, so a retrying build still ends. | | PR 17 advisor item 2: `tempLockPath()` leaves its folders | Done. `tempLockPath()` records each folder, and a file-level `afterEach` removes them. | | PR 6 (coder#7367) follow-up: the memory layer does not create the missing parent of `lockPath` | Already done on main, no code here: coder#7376 added `acquireAfterFolder` (`memorySessionLocks.testHarness.ts`), which makes the folder through `HostFiles` before the take, and the contract case "a file lock creates its missing parent" (coder#7372) runs on the memory layer. | | PR 6 (coder#7367) follow-up: a live fake holder refuses instead of ENOENT when `createParentDirectory` is false | Done. `wrapWithForeignHolders` hands the call to the inner lock when `createParentDirectory` is false and the parent is gone. The inner lock then fails with ENOENT, as the Node lock does before it reads any holder. coder#7485 is still open, so `fileLock.ts` is not touched. | | Finding 4: Windows path separators | No code. Reasoned close in coder#7326: no CI lane runs this file on Windows. | ## Validation - T5 fails on the try-lock mutant: with `crossProcessLock.ts:585` changed from `>=` to `>`, the test fails with "Expected: 1, Received: 2". It passes on main code. - The new contract test "foreign: a missing parent wins over a live holder when createParentDirectory is false" fails without the wrapper change (it gets the timeout error), and its second half (parent back, live holder refuses) fails if the wrapper always delegates. - A run of `crossProcessLock.test.ts` leaves no `cross-process-lock-*` folder from `tempLockPath()`. One older folder kind, `cross-process-lock-fifo-*` from the threadpool-starvation test, still stays behind: it is not made by `tempLockPath()` and is outside this item. ## Risks Low. Test code only. The foreign-holder change only affects tests that pass `createParentDirectory: false` with a missing parent. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$198.14`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=198.14 -->
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
5b PR 2 of phase 5 (Loop host I/O) of the agent-loop refactor.
SessionLocksMemorykeeps the use leases in its own lock table, souseLeasesForno longer goes to the Node layer. It makes the lock folders through an injectedHostFiles, so on the memory volume a lock writes nothing on the disk. The change is test-only: 4 test files, no production code.Background
Phase 4 PR 6 (#7367) added the memory lock layer and left
useLeasesForon the Node layer. Phase 4 PR 9 (#7372) added the file-lock path. Both made each lock's folder on the real disk (mkdirSync), for parity with the Node locks. The proof turn of phase 5 allows no disk writes outside the config root, so phase 5 plan note 5 routes those folders throughHostFiles.HostFileshas only an asyncmkdir. When a waiter awaited its folder before it joined the FIFO, phase 4 saw a later waiter go first in 14 of 1500 runs.Implementation
memorySessionLocks.testHarness.ts:useLeasesForgives one leases object per config. Each lease is a table entry at its lease-file path (<token>.<kind>.lockinworkspaceUseLockDir).holdpublishes the lease, then probes the gate entry atworkspaceMutationLockPath, asWorkspaceUseLeases.holddoes. A live gate makes it withdraw the lease and throwWorkspaceMutationInProgressErrorwith today's text.isHeldis true for any live lease of that kind in the table. Nothing in it awaits, so it needs no transition lock.files.promises.mkdir(acquireAfterFolder). A failed mkdir rejects with its error, as the Node locks do, and gives up the place or the lock. The 2mkdirSynccalls, their eslint-disable lines and thenode:fsimport are gone.SessionLocksMemorynow needsHostFilesTag, andmemorySessionLocks(clock, table, files)takes the files.sessionLocks.contract.testHarness.ts: 2 new contract cases on all 4 layers (node, memory, node + foreign, memory + foreign):sessionLocks.contract.test.ts:nodeHostFiles, because the contract cases stat the folders on the disk.historyFiles.golden.test.ts: the memory locks get the memory host, and the test checks that<root>/locksholds no file and no folder.Risks
Low, test code only. A memory lease writes no lease file, so only the memory table sees it.
WorkspaceUseLeases.withMutationGateandtaskService'sfindForeignUseread lease files on the disk, and they are not SessionLocks members, so they cannot see a memory lease. The module doc says this. No test uses them with the memory layer today.Validation
hold, the memory gate contract case fails.[c, b]).--rerun-each 1500: 1500 pass, 0 fail.bun test ./src/node/services/sessionLocks/ ./src/node/services/workspaceUseLeases.test.ts ./src/node/services/sessionFiles/historyFiles.golden.test.ts: 84 pass, 0 fail.make static-checkandmake lint-eslint.make static-check,make lint-eslint, the 3 test paths above (85 pass) and the FIFO case 1500 of 1500.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$129.93