test(opencode): stabilize runner cancel tests#876
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 25 minutes and 6 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Suggested priority: P3 (only low-risk paths changed (packages/opencode/test/effect/runner.test.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of the Runner tests by replacing fixed sleep durations with a polling helper function, waitForRunnerState, which waits for the runner to reach a specific state. Feedback suggests refactoring this helper to use idiomatic Effect services, specifically replacing Date.now() with Effect.currentTimeMillis and using Effect.dieMessage for timeout errors to ensure consistency and better testability.
Summary
Deferredsynchronization helpers for runner tests.yieldNowsynchronization in cancel, shell, and queued-caller tests with explicit state and waiter handshakes.cancel, closing the CI-only timeouts exposed by follow-up runs.Deferredruntime waiter-count check to only the shared-run caller tests that have no public Effect hook for "second caller is waiting on the existing run".Why
Recent post-merge CI runs timed out in
Runner > cancel with onInterrupt resolves callers gracefully. The original test forkedensureRunning(...), slept for 10ms, then cancelled. On a slow runner, cancel can run before the runner has enteredRunning, making cancel a no-op and leaving theEffect.neverwaiter to time out.Review follow-up also pointed out that two queued-caller tests still used
Effect.yieldNowas a scheduling hint. Those now wait for the shared runDeferredto have both waiters attached before the test advances.Later PR CI runs exposed the same class of race one layer deeper.
runner.state === "Running"proves the runner state was installed, but it does not prove the work fiber has actually started.runner.state === "Shell"has the same boundary for shell masking assertions: it proves shell state was installed, but not that the shell effect and interrupt finalizers have started. The affected tests now use a local blocked-work helper with a publicDeferredstart handshake before cancelling.The remaining
Deferredruntime waiter-count check is intentionally private to this test file and only used for the two shared-run caller assertions. Effect does not expose a public signal for "this second caller is now awaiting the same run Deferred", and replacing that proof with sleep/yield would weaken the tests again.Related Issue
None.
Human Review Status
Pending
Review Focus
Please check that the test helpers stay local to runner test synchronization and that the queued-caller, cancel, and shell masking assertions now wait for the relevant caller/work fiber to attach before cancel/release.
Risk Notes
No product behavior risk; this is test-only. Skipped conditional checklist items: visible UI or copy check because no visible UI or copy changed; platform/packaging impact because no runtime platform surface changed; docs/release/dependencies/permissions/generated-content checks because none of those surfaces were touched.
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.