Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a focused, well-tested server fix that adds a 1.5-second grace period to the default You can add or adjust custom eligibility rules. Learn more. |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe broker now supports an optional reply grace period. ChangesPreview wait grace handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…dline preview_wait_for forwarded the same timeoutMs as the broker deadline and the desktop operation timeout. The desktop polls until that deadline and then reports the miss, so its reply landed after the broker had already evicted the host. One unmatched wait dropped a live desktop runtime and every later preview call failed with PreviewAutomationNoAvailableHostError. The broker accepts an optional replyGraceMs that extends only its own deadline. preview_wait_for passes 1500 ms of grace, so an honest miss is routed back as an operation-level PreviewAutomationTimeoutError and the host stays assigned. The error still reports the caller's timeoutMs. A host that stays silent past timeoutMs + grace is evicted as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
49576f1 to
9f9fdee
Compare
What Changed
PreviewAutomationInvokeInputgains an optionalreplyGraceMs. The broker waitstimeoutMs + replyGraceMsbefore it treats a request as unanswered and evicts the host. The request sent to the host, the error context, and the reportedtimeoutMsdo not change.preview_wait_forpasses 1500 ms of grace. No other operation changes: click, type, navigate, resize, evaluate, and the optional status lookup keep their current deadlines.Why
preview_wait_forforwarded the sametimeoutMsas the broker deadline and as the desktop operation timeout. The desktopwaitForpolls until that deadline and only then reports the miss, so its reply landed after the broker had already evicted the host. One unmatched wait dropped a live desktop runtime. On released nightlies the next call returnsPreviewAutomationNoAvailableHostErroruntil the desktop reloads. Onmainwith #12535 the host re-registers after one second, but the session loses its pinned tab and the honest miss still counts as a dead-host eviction.#11381 states the contract: a host that replies with an operation-level timeout remains available. The existing broker test covers that only for an immediate reply. #12407 described this exact race and was accepted in triage; the reporter closed it without a fix.
The grace lives on the broker input rather than in the
waitForhandler'stimeoutMsso the error still reports the caller's deadline. InflatingtimeoutMsmade the message readtimed out after 6500msfor a 5000 ms wait. A host that stays silent pasttimeoutMs + replyGraceMsis still evicted, so a frozen host is released as before.Fixes #12898. Same race as #12407.
Validation
vp test run apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts: 51 passed (2 new). Both new tests fail onmainwithout the source change: the MCP test getsisError: trueon the follow-uppreview_status, the broker test getsPreviewAutomationNoAvailableHostErrorinstead of the routed status.waitForwith an operation-level timeout attimeoutMs + 100under the test clock; the error carriestimeoutMs: 5000and a laterstatusroutes to the same host.preview_wait_forwithtimeoutMs: 5000returnsPreview automation waitFor timed out after 5000ms.;preview_statuson the same host then succeeds.vp run typecheckinapps/server: exit 0.vp lintandvp fmt --checkon the four changed files: clean.Checklist
Model: Claude Fable 5.1. Harness: Claude Code in T3 Code.
🤖 Generated with Claude Code
Summary by CodeRabbit
preview_wait_forresponses arrive shortly after the caller’s deadline.