test(codex): isolate the lock child's database and wait on a real signal - #4859
Conversation
`a contender with a deadline waits for the holder instead of failing immediately` went red on dev (run 35177450461, job 105062310488) with the HOLDER reporting busy, which should have been impossible: the parent had already seen its hold marker. Both facts were true. The marker was written from inside the lock callback, so it proved the child had ENTERED the section, not that it finished holding it. The child's coordination database lived under the ambient OPENCODEX_HOME, which every file in the same CI batch shares, so another test reading it could turn the holder's COMMIT into SQLITE_BUSY. With timeoutMs 0 the child had no retry, the acquisition rolled back, and it returned busy after having already published the marker the parent was waiting on. The 150ms sleep was the second half of the same problem. It was standing in for "the waiter is now actually waiting", and nothing made that true - on a loaded runner the parent could release before the contention it exists to measure had begun, which would also have made the waitedMs > 0 assertion a coin flip. So: the child's database moves to the per-test temp root, which removes the cross-file contention entirely; the waiter runs as its own child and publishes a wait marker only after confirming its lock promise did not settle synchronously; and the parent releases the holder only once it has seen that marker. waitedMs now comes back from the child, so the property the case exists to prove is guaranteed by construction rather than by timing. No other blind sequencing remains in this file - every other wait is the existing waitFor, which polls an observable condition. No local suite, focused test, typecheck, build, or install was run.
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe lock integration tests now isolate child-process homes, report lock wait duration, and use an explicit child-process marker to verify retry-state entry before releasing the lock holder. ChangesCodex write-lock test synchronization
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The contention test now synchronizes on the lock retry state and uses isolated lock homes without an established merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 034c7243e7
ℹ️ 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".
| const waiter = spawnChild({ timeoutMs: 5_000, waitMarker }); | ||
| // The waiter writes this only after withCodexWriteLock has returned its | ||
| // pending promise. Because the holder is still held, that means the waiter | ||
| // has attempted N and reached the retry wait rather than failing fast. | ||
| await waitFor(waitMarker); |
There was a problem hiding this comment.
On loaded Windows runners, this file already records that this helper can take 8–19 seconds to boot, but the new waitFor(waitMarker) still uses the 15-second INTERNAL_DEADLINE_MS. The newly spawned waiter can therefore be healthy yet fail before publishing the marker; if startup approaches the holder's 20-second ceiling, the holder may also release first, causing the waiter to settle without ever writing the marker. This reintroduces the timing-dependent CI failure the change is intended to remove; use a readiness deadline sized for a spawned process and make the holder ceiling safely exceed it.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 75 / 80이 PR은 실패한 장면은 이렇게 읽힙니다. 부모가 holder 자식의 hold 마커를 이미 봤는데, 나중에 holder 결과가 고치는 방법은 네 갈래입니다. (1) types.ts / config.ts 대형 분리 캠페인과는 무관합니다. 테스트·헬퍼 두 파일만 바뀌고, 라인 tests/codex-integration/codex-write-lock.test.ts spawnChild - OPENCODEX_HOME을 per-test root로 고정한 것은 맞다. 같은 describe의 다른 자식 케이스에도 같이 적용되므로, 배치 간 C 간섭이 이 파일 전체에서 줄어든다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
리뷰 · 우선순위 76 / 80이 PR은 프로덕션 락 로직이 아니라, 실패 장면은 Linux 고침은 두 갈래입니다. 첫째,
라인 289-296 - 주석/본문은 “C database를 OPENCODEX_HOME으로 고립”이라고 읽히지만, 현재 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…nal (lidge-jun#4859) `a contender with a deadline waits for the holder instead of failing immediately` went red on dev (run 35177450461, job 105062310488) with the HOLDER reporting busy, which should have been impossible: the parent had already seen its hold marker. Both facts were true. The marker was written from inside the lock callback, so it proved the child had ENTERED the section, not that it finished holding it. The child's coordination database lived under the ambient OPENCODEX_HOME, which every file in the same CI batch shares, so another test reading it could turn the holder's COMMIT into SQLITE_BUSY. With timeoutMs 0 the child had no retry, the acquisition rolled back, and it returned busy after having already published the marker the parent was waiting on. The 150ms sleep was the second half of the same problem. It was standing in for "the waiter is now actually waiting", and nothing made that true - on a loaded runner the parent could release before the contention it exists to measure had begun, which would also have made the waitedMs > 0 assertion a coin flip. So: the child's database moves to the per-test temp root, which removes the cross-file contention entirely; the waiter runs as its own child and publishes a wait marker only after confirming its lock promise did not settle synchronously; and the parent releases the holder only once it has seen that marker. waitedMs now comes back from the child, so the property the case exists to prove is guaranteed by construction rather than by timing. No other blind sequencing remains in this file - every other wait is the existing waitFor, which polls an observable condition. No local suite, focused test, typecheck, build, or install was run.
1 participant
Summary
a contender with a deadline waits for the holder instead of failing immediatelywent red ondev(run 35177450461, job105062310488, Linuxtest 4/4) with the holder reportingbusy— which should have been impossible, because the parent had already seen its hold marker:Both facts were true at once. The marker is written from inside the lock callback, so it proves the child ENTERED the held section — not that it finished holding it. The child's coordination database lived under the ambient
OPENCODEX_HOME, which every file in the same CI batch shares, so another test reading it could turn the holder'sCOMMITintoSQLITE_BUSY. WithtimeoutMs: 0the child had no retry, the acquisition rolled back, and it returnedbusyafter having already published the marker the parent was waiting on.The
await Bun.sleep(150)was the second half of the same problem. It stood in for "the waiter is now actually waiting", and nothing made that true. On a loaded runner the parent could release the holder before the contention this case exists to measure had even begun — which would also have made thewaitedMs > 0assertion a coin flip rather than a property.The fix removes both:
waitedMscomes back from the child, so the property the case exists to prove is guaranteed by construction rather than by timing.No timeout was raised, no retry added, and no test skipped.
Verification
No local suite, focused test, typecheck, build, or install was run; this lane is hosted-CI-only by task contract. Verification is static plus exact-head hosted CI.
Static checks:
waitForhelper, which polls an observable condition; no blind sequencing is left.git diff --checkclean.src/change: this is a test-isolation and synchronisation defect, not a product defect. The lock module'stimeoutMs: 0behaviour — refuse rather than retry — is correct and unchanged; the test was simply holding it wrong.Ablation: with the database shared again, a concurrent reader in the same batch reproduces the original
busy; with the wait marker removed and the sleep restored, releasing before the waiter enters its wait makeswaitedMs > 0fail.Checklist
devSummary by CodeRabbit