Repository navigation
🤖 refactor: add the SessionLocks service with today's lock functions as its Node layer - #7345
Conversation
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…call-record # Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json # src/node/services/sessionLocks/sessionLocks.callOptions.test.ts
…e4/call-record-18 # Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json # src/node/services/sessionLocks/sessionLocks.callOptions.test.ts
# Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json
# Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json
# Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json
# Conflicts: # src/node/services/sessionLocks/__fixtures__/callOptions.json # src/node/services/sessionLocks/sessionLocks.callOptions.test.ts
# Conflicts: # src/node/services/di/layers/stores.ts # src/node/services/di/tags.ts # src/node/services/sessionLocks/sessionLocks.callOptions.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. |
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 33 model calls · 241.3k tokens (73% cached) All 14 tests in 9 files
e2e 0.20.0 · 3m 27s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 23.1s · 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 |
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Adds the
SessionLocksservice: one interface over today's session lock functions, with a Node layer that calls those functions. No caller moves in this PR. It is PR 4 of phase 4.Background
Phase 4 moves the session-dir lock calls of 27 files behind one Effect service, so tests and the in-memory turn can swap the lock medium with a layer. PRs 2 and 3 (#7290, #7291) record the options of every lock call. This PR adds the service and its contract. Later PRs move the callers.
Implementation
The first commit adds two checks to
sessionLocks.callOptions.test.ts, on the code before the move. They close the two round-6 findings of #7290 that #7326 tracks:hold()takes the use lease before the mutation lease:["workspace-use", "workspace-mutation"].PINNED_DIGESTSfor 11 workspace ids andPINNED_TARGETfor/record/target. A changed hash scheme now fails the record, even when the digest names follow it.The second commit adds the service:
sessionLocks.ts: theSessionLocksinterface. Each member has the signature of today's function:workspaceFiles.withLock,acquireProcessFileLock,acquireCrossProcessLock,inspectCrossProcessLockanduseLeasesFor(config).nodeSessionLocks.ts: the Node layer. Its members call today's functions at call time, so spies on the lock modules still reach the call.workspaceFilesis the sharedworkspaceFileLocksmap: a second map would let two writers of one workspace run at once.di/tags.ts,di/layers/stores.ts:SessionLocksTag, andStoresLiveprovidesSessionLocksNode, so the app always gets the Node layer.utils/concurrency/lockErrors.ts:CrossProcessLockTimeoutErrorandWorkspaceMutationInProgressErrormove to a pure module.crossProcessLock.tsandworkspaceUseLeases.tsre-export them, so each stays one class forinstanceof.fileLockTimeoutMessageandworkspaceMutationInProgressMessageare extracted with the same text, andcrossProcessLock.tsexports its holder and blocker types anddescribeHolder. The test layers of PR 6 refuse with today's text through them.sessionLocks.contract.testHarness.ts,sessionLocks.contract.test.ts,sessionLocks.testChild.ts: the contract, run on the Node layer. A child process is a real foreign holder, live or killed with SIGKILL.Validation
Checks before and after the move (
bun test src/node/services/sessionLocks/, bun 1.4.3):moved lock calls keep their optionspasses, with both new checks. 2 pass, 0 fail across 2 files.moved lock calls keep their optionspasses. 10 pass, 0 fail across 3 files.Gates on the PR head:
bun test ./src/node/services/sessionLocks/ ./src/node/utils/concurrency/fileLock.test.ts ./src/node/utils/main/crossProcessLock.test.ts ./src/node/services/workspaceUseLeases.test.ts: 79 pass, 0 fail on 907f6b0. The three lock test files pass with no edit (fileLock.test.tsasserts the timeout text).git diff --name-onlyshows no lock caller: the changed non-test files are the three lock modules,lockErrors.ts, the new service files,tags.tsandstores.ts.make typecheckandmake static-checkon 907f6b0: pass.make teston 907f6b0: pass, 0 failures. Its second half (tests/ui/storybook/,tests/ui/domIsolation.test.ts,vscode/src/,scripts/): pass, 0 failures. An earliermake teston 722dc08 (before the last main merge) failed only on the known load flakebackgroundProcessManager.test.ts, which passed alone (163 of 163).loopTestBaseline.ts --runs 3 --base origin/main): 0 trips on 907f6b0, and 0 trips on 722dc08.WorkspaceUseLeasesdoc comment back onto its class.git diff 907f6b054e 91dc55d16dchanges one comment line and no code, so the gates above count for it.make static-checkpasses on it.Mutation check
Survivors get one of three dispositions: "killed by " (a test in this PR), "equivalent: " (no input can tell the mutant from the original), or "pre-existing gap: PR 17" (a survivor on a line that this PR does not change). Each gap is listed in #7326. A later PR that changes or moves one of those lines must first kill its survivor, in a test commit at its start that passes on the code before the change.
make mutate-checkcaps each source file at its 10 nearest test files. That cap leaves outsessionLocks.callOptions.test.tsandsessionLocks.contract.test.ts. The default run on 722dc08 had 94 mutants, 56 killed and 38 survived.scripts/mutate/mutate_report.pyon the PR head 907f6b0, with no cap. Its lane is the nearest tests of the default runs plus the two sessionLocks tests: 94 mutants, 55 killed, 39 survived, 0 uncovered. The call-record test is in the lane ofworkspaceUseLeases.ts,fileLock.tsandcrossProcessLock.ts, and killsacquireTimeoutMs: 0-> 1 inhold.parseHolder:144||and :145||, and this run did not. The table lists them as gaps.lockErrors.ts, the service files) is killed. All 39 survivors are on lines that this PR does not change. The check scores whole functions, and the PR edits one line in each ofhold,acquireProcessFileLock,parseHolder,describeHolderandacquireCrossProcessLock(an extracted message function or a renamed type).holdexisting.count > 0->>=hold?? new Map->||releasefiles?.get->files.getfilesis missing only on an over-release, and no call sequence makes one (each lease releases once,releasing ??=, and each hold that returns a lease counts it)release&&->||(2 sites)releasefile.count > 0->>=interval.unref?.()->interval.unref()unreflockPath.length > 0->> 1/>=,timeoutMs > 0->>="", a 1-character path or a timeout of 0)?? FILE_LOCK_RENEW_INTERVAL_MS->||,renewIntervalMs > 0->> 1/>=acquired = false->truerenewInFlight !== null->===reclaimStaleFileLock(..., 0)-> 1>= deadline->>*->/,+->-parseHolderdescribeHolderbirth ?? ...->||||->&&&&->||stopped = true->falseattempt = 0-> 1,< 40-><=rmfail on every attempt)rm({ force: true })->falserm)owned = false->truetryTakethrows and the token stays live)>= deadline->>Deferred review findings
#7326 tracks the findings that #7290 declined. This PR closes items 1 and 2 (the first commit). #7326 also lists the 32 pre-existing mutation gaps above, with the rule for later PRs.
Risks
No lock caller changes, and the Node layer calls the same functions. The error classes move module, so a second copy of a class would break
instanceof. Both lock modules re-export the one class fromlockErrors.ts. The contract checksCrossProcessLockTimeoutErrorwithtoBeInstanceOfon the Node layer.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$81.85