Repository navigation
🤖 tests: add the memory and foreign-holder SessionLocks layers to the contract - #7367
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.
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: e3cfd1ca5f
ℹ️ 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".
🟢 e2e stories (Haiku): 14 passed27 agent steps · 7 replayed from cache · 32 model calls · 230.0k tokens (71% 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 · 22.2s · 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 |
…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.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30fe6b1db7
ℹ️ 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".
…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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9675e47d03
ℹ️ 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".
…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.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7525029ff
ℹ️ 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".
@typescript-eslint/await-thenable (CI lint-full): bun types rejects.toThrow and rejects.toBeInstanceOf as void.
|
@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 |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 423f813f42
ℹ️ 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".
|
@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. Hooray! 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 two test layers for
SessionLocksand runs the lock contract on four layers: node, memory, node + foreign and memory + foreign. It is PR 6 of phase 4. It changes only test harnesses and one test file.Order
Background
Phase 4 moves the loop's lock calls behind
SessionLocks. Later PRs (9 to 16) test the moved code with locks that need no real files and no real waits, and that play other backends. One contract runs on every layer, so the fakes cannot drift from the Node locks.Implementation
memorySessionLocks.testHarness.ts:SessionLocksMemorybuilds a freshMemoryLockTableat each build of the layer. An acquire takes a free or dead lock at once. Otherwise it waits in FIFO order until a release hands the lock over, or until theHostClockpasses the timeout. Under a TestClock the test moves time withadjust. Refusals use today's text (fileLockTimeoutMessage,crossProcessLockTimeoutMessage) and today's argument asserts. An aborted signal throws today's abort error. A record lock creates its missing parent, as Node does. Three parts stay on the Node locks:foundByReaddir: the path has the segment/locks/init-record/).InitStateManagerlists their folder, so a holder in memory would leave no file and a live init would look abandoned.createParentDirectory: false. WorkflowRunStore needs their acquire to fail with ENOENT once the run directory is gone, with no window between a check and the take. Only the Node lock's own write gives that.isHeldandtaskServiceread other backends' lease files. Phase 5 moves them.foreignHolders.testHarness.ts:makeForeignHolders()gives the test a handle (hold,release,reclaimed).withForeignHolders(inner, holders)wraps a layer. A live foreign holder refuses at once with today's text, and an aborted signal wins over it. A dead one is reclaimed by the next acquire, and its path goes toreclaimed. A probe reclaims nothing.hold()publishes the lease first and probes the gate second, asWorkspaceUseLeases.holddoes. A live holder onworkspaceMutationLockPathmakes it release the lease and throwWorkspaceMutationInProgressErrorwith today's text.isHeldcounts a live fake lease in the workspace's lease folder. Every fake holder records an identity (foreignIdentity()), so its text equals the Node text for a current record.liveBlocker(pid)gives that blocker. Fake pids areforeignPid(n)=process.pid + n, so a fake holder is never this process.sessionLocks.contract.test.ts: the contract runs on the three new layers. The Node text test now compares againstliveBlocker, so the fake text equals the Node text for the child's pid. Four new contract cases run on all 4 layers: an aborted signal wins over a live holder,createParentDirectory: falseon a missing parent rejects with ENOENT, an aborted signal wins over a missing parent, and a record lock creates its missing parent by default. Eight more cases:TestClock.adjust, and it has not settled 1 ms before the timeout.assertStillOwnedthen throws.SessionLocksMemoryhas its own table.hold()refuse with today's text.hold()publishes its lease before it probes the gate.isHeldtrue.Validation
bun test ./src/node/services/sessionLocks/: pass. The contract file has 53 pass, 0 fail.wrapWithForeignHolders: the 4 live cases of node + foreign and memory + foreign fail.foundByReaddir: the init-record case fails.SessionLocksMemory: the fresh-table case fails.createParentDirectory: falseto the memory table instead of the Node locks: the ENOENT case of both memory layers fails.isHeld: the isHeld case fails.make static-check: pass.make crap-check,make dry-check,make mutate-check(base = the parent commit): PASS with 0 functions in scope. The PR changes only test harnesses and a test file.make teston 3e31a0aa6a, which is this diff on 🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355's head 2f34c99: 27689 pass, 0 fail in the main run, and every other run passes. The second half (tests/ui/storybook/,tests/ui/domIsolation.test.ts,vscode/src/,scripts/): 732 pass, 0 fail.scripts/perf/loopTestBaseline.ts --runs 3, base = 🤖 tests: put the session lock modules under local/no-host-imports with their 1 pair #7355's head 2f34c99, on 3e31a0aa6a): 0 trips.bun test ./src/node/services/sessionLocks/,make static-checkandmake lint-eslintpass. The tripwire andmake testwere not rerun after the move to main and the Codex fixes, because the PR changes only test files undersessionLocks/.Follow-ups
acquireProcessFileLockdoes not create the missing parent oflockPath, whilefileLock.tsdoes. No test uses the memory layer yet. PR 9 adds the mkdir and a contract case before it moves any caller. Tracked in 🤖 tests: declined review findings of the phase 4 call-record PRs #7326. (PR 9 fixes this one.)createParentDirectory: falserefuses with a timeout even when the parent is gone, where Node gives ENOENT. No test combines the two yet. The first PR that runs such a caller onwithForeignHoldersfixes it. Tracked in 🤖 tests: declined review findings of the phase 4 call-record PRs #7326.Risks
Test-only. No production file imports the new harnesses.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$103.72