Skip to content

🤖 refactor: add the SessionLocks service with today's lock functions as its Node layer - #7345

Merged
ThomasK33 merged 39 commits into
mainfrom
phase4/service
Oct 23, 2026
Merged

ThomasK33 merged 39 commits into
mainfrom
phase4/service

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds the SessionLocks service: 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:

  1. hold() takes the use lease before the mutation lease: ["workspace-use", "workspace-mutation"].
  2. The lock digests of the record are pinned: PINNED_DIGESTS for 11 workspace ids and PINNED_TARGET for /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: the SessionLocks interface. Each member has the signature of today's function: workspaceFiles.withLock, acquireProcessFileLock, acquireCrossProcessLock, inspectCrossProcessLock and useLeasesFor(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. workspaceFiles is the shared workspaceFileLocks map: a second map would let two writers of one workspace run at once.
  • di/tags.ts, di/layers/stores.ts: SessionLocksTag, and StoresLive provides SessionLocksNode, so the app always gets the Node layer.
  • utils/concurrency/lockErrors.ts: CrossProcessLockTimeoutError and WorkspaceMutationInProgressError move to a pure module. crossProcessLock.ts and workspaceUseLeases.ts re-export them, so each stays one class for instanceof.
  • The message functions fileLockTimeoutMessage and workspaceMutationInProgressMessage are extracted with the same text, and crossProcessLock.ts exports its holder and blocker types and describeHolder. 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):

  • First commit e217b89 (before the move): moved lock calls keep their options passes, with both new checks. 2 pass, 0 fail across 2 files.
  • Service commit 2715877 (after the move): moved lock calls keep their options passes. 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.ts asserts the timeout text).
  • git diff --name-only shows no lock caller: the changed non-test files are the three lock modules, lockErrors.ts, the new service files, tags.ts and stores.ts.
  • make typecheck and make static-check on 907f6b0: pass.
  • make test on 907f6b0: pass, 0 failures. Its second half (tests/ui/storybook/, tests/ui/domIsolation.test.ts, vscode/src/, scripts/): pass, 0 failures. An earlier make test on 722dc08 (before the last main merge) failed only on the known load flake backgroundProcessManager.test.ts, which passed alone (163 of 163).
  • Tripwire (loopTestBaseline.ts --runs 3 --base origin/main): 0 trips on 907f6b0, and 0 trips on 722dc08.
  • 91dc55d (review round 1, the fresh advisor's nit): it moves the WorkspaceUseLeases doc comment back onto its class. git diff 907f6b054e 91dc55d16d changes one comment line and no code, so the gates above count for it. make static-check passes 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.

  • The default make mutate-check caps each source file at its 10 nearest test files. That cap leaves out sessionLocks.callOptions.test.ts and sessionLocks.contract.test.ts. The default run on 722dc08 had 94 mutants, 56 killed and 38 survived.
  • The table below comes from one scratch run of scripts/mutate/mutate_report.py on 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 of workspaceUseLeases.ts, fileLock.ts and crossProcessLock.ts, and kills acquireTimeoutMs: 0 -> 1 in hold.
  • Some kills are timing-dependent: the default run killed parseHolder :144 || and :145 ||, and this run did not. The table lists them as gaps.
  • Every mutant in the new code (the message functions, 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 of hold, acquireProcessFileLock, parseHolder, describeHolder and acquireCrossProcessLock (an extracted message function or a renamed type).
  • Line numbers are on the PR head.
# Site Mutant Disposition
1 workspaceUseLeases.ts:331 hold existing.count > 0 -> >= equivalent: release deletes an entry at 0, so no call sequence stores a count of 0
2 workspaceUseLeases.ts:362 hold ?? new Map -> || equivalent: a Map is always truthy
3 workspaceUseLeases.ts:373 release files?.get -> files.get equivalent: files is 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)
4-5 workspaceUseLeases.ts:375 release && -> || (2 sites) equivalent: the assert fails only on an over-release, which no call sequence makes
6 workspaceUseLeases.ts:375 release file.count > 0 -> >= equivalent: the same invariant as #1
7 crossProcessLock.ts:515 interval.unref?.() -> interval.unref() equivalent: Bun and Node timers always have unref
8-10 fileLock.ts:260, :261 lockPath.length > 0 -> > 1 / >=, timeoutMs > 0 -> >= pre-existing gap: PR 17 (no test passes "", a 1-character path or a timeout of 0)
11-13 fileLock.ts:278, :279 ?? FILE_LOCK_RENEW_INTERVAL_MS -> ||, renewIntervalMs > 0 -> > 1 / >= pre-existing gap: PR 17 (no test passes an interval of 0 or 1)
14 fileLock.ts:265 acquired = false -> true pre-existing gap: PR 17 (a failed acquire keeps its token live; a lock file with that token then reads as live)
15 fileLock.ts:296 dispose renewInFlight !== null -> === pre-existing gap: PR 17 (release does not join a renewal in flight)
16 fileLock.ts:306 reclaimStaleFileLock(..., 0) -> 1 pre-existing gap: PR 17 (no test reaches the reclaim depth limit)
17 fileLock.ts:307 >= deadline -> > pre-existing gap: PR 17 (one more retry at the deadline's millisecond)
18-19 fileLock.ts:311 retry sleep * -> /, + -> - pre-existing gap: PR 17 (no test checks the retry interval)
20-30 crossProcessLock.ts:130, :140-145 parseHolder 11 mutants of the record shape check pre-existing gap: PR 17 (no test reads a record that is null, an array, or has a bad pid, token or acquiredAt; a backend can be pid 1 in a container)
31 crossProcessLock.ts:226 describeHolder birth ?? ... -> || pre-existing gap: PR 17 (no test reads a record with an empty birth)
32 crossProcessLock.ts:499 renewal guard || -> && pre-existing gap: PR 17 (overlapping renewal ticks)
33 crossProcessLock.ts:504 renew only our own record && -> || pre-existing gap: PR 17 (a tick that finds another holder's record)
34 crossProcessLock.ts:517 stopped = true -> false pre-existing gap: PR 17 (a tick past its check after release)
35-36 crossProcessLock.ts:528 release attempts attempt = 0 -> 1, < 40 -> <= pre-existing gap: PR 17 (no test makes rm fail on every attempt)
37 crossProcessLock.ts:543 rm({ force: true }) -> false pre-existing gap: PR 17 (a record that vanishes between observe and rm)
38 crossProcessLock.ts:569 owned = false -> true pre-existing gap: PR 17 (tryTake throws and the token stays live)
39 crossProcessLock.ts:585 >= deadline -> > pre-existing gap: PR 17 (a zero-timeout try-lock that fails within the deadline's millisecond sleeps once)

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 from lockErrors.ts. The contract checks CrossProcessLockTimeoutError with toBeInstanceOf on the Node layer.


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high • Cost: $81.85

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
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T14:00:41.553037Z 91dc55d Manual request
🔒 Security Review ✅ Completed 2026-10-23T14:00:21.731687Z 91dc55d Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🟢 e2e stories (Haiku): 14 passed

27 agent steps · 7 replayed from cache · 33 model calls · 241.3k tokens (73% cached)
Spend: $0.09 of the $1.00 cap (proxy ledger)

All 14 tests in 9 files
Test Agent Time
🟢 chatRestart.e2e.ts · 1 passed 2 steps · 4 calls 28.7s
🟢 a chat survives a restart of the server 2 steps · 4 calls 28.7s
🟢 compaction.e2e.ts · 1 passed 3 steps · 9 calls 49.4s
🟢 /compact replaces the old chat with a summary 3 steps · 9 calls 49.4s
🟢 pluginTrustRestart.e2e.ts · 1 passed 4 steps · 2 calls 17.5s
🟢 a trusted plugin hook stays trusted after the server restarts 4 steps · 2 calls 17.5s
🟢 settingsRoundTrip.e2e.ts · 1 passed 3 steps · 5 calls 30.5s
🟢 a chat survives trips through Settings 3 steps · 5 calls 30.5s
🟢 sidebarPerWorkspace.e2e.ts · 1 passed 4 steps · 1 call 5.1s
🟢 a new workspace starts with its right sidebar collapsed while another keeps it open 4 steps · 1 call 5.1s
🟢 smoke.e2e.ts · 1 passed 2 steps · 1 call 9.8s
🟢 the agent opens the playground workspace 2 steps · 1 call 9.8s
🟢 streamErrors.e2e.ts · 1 passed 1 step · 1 call 7.2s
🟢 stream errors show in the chat, and the chat recovers 1 step · 1 call 7.2s
🟢 todoNudge.e2e.ts · 1 passed 2 steps · 4 calls 31.5s
🟢 the agent finishes open TODO items after the turn-end nudge 2 steps · 4 calls 31.5s
🟢 touchAlignment.e2e.ts · 6 passed 6 steps · 6 calls 24.7s
🟢 the Settings close button is level with the title on a touch screen 390px wide 1 step · 1 call 4.4s
🟢 the Settings close button is level with the title on a touch screen 768px wide 1 step · 1 call 4.4s
🟢 Backup settings checkboxes are small and level with their labels on a touch phone 1 step · 1 call 5.3s
🟢 the heartbeat checkbox is small and level with its label on a touch phone 1 step · 1 call 4.0s
🟢 a reply's model and time stay on one line on a touch phone 1 step · 1 call 3.2s
🟢 the workspace actions menu stays on the title row on a touch phone 1 step · 1 call 3.4s

e2e 0.20.0 · 3m 27s · web · run artifacts

🟢 e2e Electron stories (Haiku): 3 passed

3 agent steps · 3 model calls · 8.0k tokens (32% cached)
Spend: $0.01 of the $1.00 cap (proxy ledger)

All 3 tests in 3 files
Test Agent Time
🟢 electron/deepLink.e2e.ts · 1 passed 1 step · 1 call 5.6s
🟢 a deep link of a second launch starts a chat in the running app 1 step · 1 call 5.6s
🟢 electron/menuBar.e2e.ts · 1 passed 1 step · 1 call 6.4s
🟢 the workspace menu bar is the desktop title bar 1 step · 1 call 6.4s
🟢 electron/smoke.e2e.ts · 1 passed 1 step · 1 call 9.2s
🟢 Xum Electron opens the playground workspace 1 step · 1 call 9.2s

e2e 0.20.0 · 23.1s · electron · run artifacts

🟢 e2e agent repros (Haiku): no tests selected

Spend: $0.00 of the $1.00 cap (proxy ledger)

e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 91dc55d16d

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 91dc55d16d

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant