fix(cursor): state the T04 re-arming contract on an injectable clock - #5131
Conversation
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: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe T04 stream-health watchdog now uses an injectable clock. Production uses wrappers around global time APIs. Tests use a manual virtual clock to verify timer re-arming, deadline resets, clean completion, and watchdog disarming. ChangesStream-health clock abstraction
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor 🚥 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 |
리뷰 · 우선순위 77 / 80macOS control 레인에서 이 PR은 예산을 더 키우지 않고, T04 워치독만 쓰는 시간을 주입 가능하게 바꿉니다. 테스트는 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
![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: 895a457e61
ℹ️ 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".
| * watchdog was working correctly. With the clock injected, virtual time advances only where | ||
| * the test says it does, so no amount of runner contention can expire a deadline. | ||
| */ | ||
| streamHealthClock?: CursorStreamHealthClock; |
There was a problem hiding this comment.
This adds an exported streamHealthClock contract and reroutes the Cursor watchdog through it, changing an owned src/adapters/ transport surface, but the commit updates no corresponding structure/ document. This leaves the repository's architecture source of truth stale; update the applicable documents listed for src/adapters/ in structure/INDEX.md in the same change, including the Cursor transport/watchdog contract.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
tests/providers/cursor/cursor-stream-health.test.ts asserted that meaningful frames keep resetting both stream-health deadlines by running a synthetic HTTP/2 server for three times the silence budget and requiring that no deadline expire. That is an assertion about how busy the machine is. In the unsharded macOS control lane it fails with "Cursor stream stalled: no inbound frames for 5s before turnEnded" while the watchdog is behaving correctly: under contention the fixture's own frames do not arrive inside the window. The budget is already CI-scaled, and scaling is what makes it worse. With the silence budget floored at 5s the case needs 15 consecutive seconds during which no 5s gap in frame decoding occurs, so raising the floor lengthens the exposure rather than reducing it. Add a T04-only clock seam (now, setTimeout, clearTimeout), matching StageLeaseClock in src/cli/account-api.ts. Production omits it and keeps the globals; the 30s and 90s budgets, the first-frame timer, the turn-ended grace and the outbound heartbeat are untouched. The test now advances virtual time by hand between frames whose arrival it awaits, so runner contention can delay a frame without any deadline passing, and it additionally asserts that exactly one timer is armed after each frame and none survives turnEnded. No production timeout changed, no budget widened, no retry added and no platform skipped.
895a457 to
072271a
Compare
추가 리뷰 · 우선순위 78 / 80이전 리뷰(77/80, 이 절은 이전 리뷰가 말로만 적었던 계약을 구조 문서에 고정합니다. (1) 프로덕션 예산은 30s/90s이고 테스트가 그걸 움직이지 않는다 (2) 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
… clock (#5169) * test(cursor): state the T04 heartbeat-only contract on the injectable clock The heartbeat-only case was the last T04 assertion that still contained a term for elapsed real time. It kept a real 40ms interval writing liveness frames and required no decoded-frame gap longer than the silence budget for the whole heartbeat-only window, so a runner that paused longer than one budget made the SILENCE watchdog win while both watchdogs behaved correctly. #3940 had already scaled that budget once; scaling it again only lengthens the exposure. Move the case onto the streamHealthClock seam #5131 added, advancing virtual time by hand between liveness frames whose arrival it awaits. Production budgets (30s silence, 90s heartbeat-only), the first-frame timer and the outbound heartbeat are untouched, and the silence watchdog's real-timer firing case is unchanged. * test(cursor): pin the T04 deadline selection that no real-timer case can state Adversarial review of the previous commit found two gaps it had left. The min(silence, progress) rule was not red-capable anywhere. A watchdog that read only the progress deadline would relax production silence detection from 30s to 90s, and every real-timer case would stay green: a later deadline still produces the same no-inbound-frames message inside the case timeout. Add a virtual-clock case that requires the turn to fail at exactly S with nothing left armed, which that mutation fails immediately. Moving the heartbeat-only case onto the seam also removed the only coverage of the progress branch firing through the production default clock, which structure/providers/cursor.md claimed was still there. Restore it with a real-timer case whose silence budget is two orders of magnitude beyond its progress budget, so load can make it later but never wrong, and correct the document to say which half of the contract lives where and why. * test(cursor): bound the manual stream-health clock and pin the stall prefix Third review round. Widening the manual clock from one case to three made its unbounded drain loop worth fixing: a watchdog that re-armed an already expired deadline would schedule a zero-delay timer forever, and because advanceTo is synchronous, Bun's per-test timeout could never interrupt it. That mutation would wedge the lane instead of reddening one case, so the loop now throws after 1000 callbacks and names the cause. Also assert the shared Cursor stream stalled prefix, which no case pinned, and correct the real-timer comment: ordinary load delays that case, and a pause long enough to also cross the 60s silence budget has already blown the case's own 15s limit, which is why its outcome cannot change rather than merely being unlikely to.
1 participant
Summary
tests/providers/cursor/cursor-stream-health.test.tsfails in the unshardedmacos controllane withCursor stream stalled: no inbound frames for 5s before turnEnded, raised fromsrc/adapters/cursor/live-transport.ts. This is a different kind of failure from the four in [Bug]: four tests exceed their timeouts in the unsharded macOS control lane, first observed once the lane was able to finish #4997: it is not a test exceeding its own timeout, it is production code declaring a stall. The case at issue asserts the opposite of a stall — that meaningful frames keep resetting both deadlines — and the only way it could state that against real timers was to run a synthetic HTTP/2 server for three times the silence budget and require that no deadline expire. That is an assertion about how busy the machine is, and under control-lane contention the fixture's own frames do not arrive inside the window while the watchdog is behaving exactly as designed.isolationBudgetMs(1_000)floors the silence budget at 5s on CI, and the progress span is three times that, so the case now needs 15 consecutive seconds during which no 5s gap in frame decoding occurs, inside a 22s fixture window. Raising the floor lengthens the exposure. There is no value of the budget that makes a "no deadline expired" assertion safe on a runner that can pause for more than ten seconds.CursorTransportFactoryInputgains an optionalstreamHealthClockwithnow,setTimeoutandclearTimeout, and only the T04 watchdog reads time or schedules through it. The test supplies a clock it advances by hand between frames whose arrival it awaits, so contention can delay a frame — which the test waits for — without any deadline passing. It also now asserts two things the old case could not: exactly one timer is armed after every frame, so re-arming replaces the previous deadline rather than stacking one, and no deadline survivesturnEnded, read beforeclose()which would clear it anyway.StageLeaseClockinsrc/cli/account-api.ts, which is the same shape for the same reason and is documented as test-only. Production omits it and binds all three primitives to the globals through one module constant.What did not change
The production budgets are untouched:
CURSOR_STREAM_SILENCE_FAIL_MSis still 30s andCURSOR_STREAM_HEARTBEAT_ONLY_FAIL_MSstill 90s. Five seconds was never the production contract — it was the CI-scaled test budget being printed back by the stall message. The first-frame timer, the turn-ended close grace, the client-tool finalize grace and the outbound heartbeat all stay on the global timers; the seam is scoped to the two T04 clocks and the one T04 timer. TheelapsedMsdiagnostic deliberately stays on the wall clock becauseturnStartedAtis stamped there and the pair has to subtract coherently.No timeout was widened, no retry added, no platform skipped and no failure masked.
Real-timer coverage that remains
Three cases in the same file still drive the watchdog through the default global-timer clock, and all three assert that something happens rather than that it does not, so a slow runner cannot turn them red for being slow:
setTimeoutreachingfailAndClear.turnEndeddisarms the watchdog even when the server holds the stream open, on a 300ms budget — real cancellation.I deliberately did not add a short real-timer version of the re-arming case. It would be the same flake with a smaller window: any assertion that a deadline did not expire is vulnerable to a stall longer than the deadline, and the observed stalls on that lane exceed ten seconds.
Structure ownership
structure/providers/cursor.mdownssrc/adapters/and had no section on the stream-health watchdog, so the new contract is recorded there: the 30s and 90s budgets are production and a test does not move them, the injectable clock is scoped to T04 alone, and the firing half of the contract stays on real timers. That is the invariant the previous round of this flake violated by scaling the budget, so it is worth being written down rather than inferred from the seam's existence.Verification
~/.opencodex. The change was reviewed by hand-executing the new case against the transport's decode path and by static reasoning about which timers moved.part-0lands at virtual 0 and arms one timer for 1000; the clock is advanced to 900, where nothing is due, andpart-1re-arms for 1900; advanced to 1800,part-2re-arms for 2800; advanced to 2700,part-3re-arms for 3700. Virtual time ends at 2700, past both non-resetting deadlines (1000 and 2000), which is the claim. No pending deadline is ever due at anadvanceTotarget, so no watchdog callback runs at all.test 1/4..4/4andmacos 1/2..2/2carry both changed files.macos controljob from [Bug]: four tests exceed their timeouts in the unsharded macOS control lane, first observed once the lane was able to finish #4997, which cannot finish inside its own budget, so CI here cannot demonstrate the original failure disappearing. What it can show is that the rewritten case and the untouched production path stay green on the sharded lanes. The argument for the fix is structural — the assertion no longer depends on elapsed real time — not a green run.Checklist
Refs #4997