Skip to content

fix(server): keep preview hosts that report a waitFor miss at the deadline - #12899

Open
mInrOz wants to merge 1 commit into
pingdotgg:mainfrom
mInrOz:fix/preview-wait-for-host-grace
Open

mInrOz wants to merge 1 commit into
pingdotgg:mainfrom
mInrOz:fix/preview-wait-for-host-grace

Conversation

@mInrOz

@mInrOz mInrOz commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

What Changed

PreviewAutomationInvokeInput gains an optional replyGraceMs. The broker waits timeoutMs + replyGraceMs before it treats a request as unanswered and evicts the host. The request sent to the host, the error context, and the reported timeoutMs do not change.

preview_wait_for passes 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_for forwarded the same timeoutMs as the broker deadline and as the desktop operation timeout. The desktop waitFor polls 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 returns PreviewAutomationNoAvailableHostError until the desktop reloads. On main with #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 waitFor handler's timeoutMs so the error still reports the caller's deadline. Inflating timeoutMs made the message read timed out after 6500ms for a 5000 ms wait. A host that stays silent past timeoutMs + replyGraceMs is 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 on main without the source change: the MCP test gets isError: true on the follow-up preview_status, the broker test gets PreviewAutomationNoAvailableHostError instead of the routed status.
  • New broker test: host answers the waitFor with an operation-level timeout at timeoutMs + 100 under the test clock; the error carries timeoutMs: 5000 and a later status routes to the same host.
  • New MCP test: preview_wait_for with timeoutMs: 5000 returns Preview automation waitFor timed out after 5000ms.; preview_status on the same host then succeeds.
  • vp run typecheck in apps/server: exit 0. vp lint and vp fmt --check on the four changed files: clean.
  • Not run: a live desktop reproduction against this branch. The failing sequence was observed on t3 v0.0.43-nightly.20260917.1866 with a macOS desktop client, from provider logs; the new tests reproduce the same timing through the real broker and MCP toolkit.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (not applicable)
  • I included a video for animation/interaction changes (not applicable)

Model: Claude Fable 5.1. Harness: Claude Code in T3 Code.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved preview automation reliability when preview_wait_for responses arrive shortly after the caller’s deadline.
    • Preview hosts now remain available for subsequent status checks after an honest wait timeout.
    • Added coverage for delayed responses and continued routing to the correct preview host.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 21, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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 preview_wait_for host deadline. Because that changes production default timing and host-retention behavior for all such calls, human review is required.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 7c5593dc-5ad8-4f32-8515-38bf9343c268

📥 Commits

Reviewing files that changed from the base of the PR and between 49576f1 and 9f9fdee.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 37b67cb9-2fbe-46e5-9ba6-615d1c96f964

📥 Commits

Reviewing files that changed from the base of the PR and between 1de563c and 49576f1.

📒 Files selected for processing (4)
  • apps/server/src/mcp/McpHttpServer.test.ts
  • apps/server/src/mcp/PreviewAutomationBroker.test.ts
  • apps/server/src/mcp/PreviewAutomationBroker.ts
  • apps/server/src/mcp/toolkits/preview/handlers.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The broker now supports an optional reply grace period. preview_wait_for uses a 1,500 ms grace period, and tests verify that delayed timeout replies do not evict the preview host.

Changes

Preview wait grace handling

Layer / File(s) Summary
Broker reply grace handling
apps/server/src/mcp/PreviewAutomationBroker.ts, apps/server/src/mcp/PreviewAutomationBroker.test.ts
The broker accepts optional replyGraceMs metadata and waits for timeoutMs + replyGraceMs. Tests verify that a delayed waitFor timeout reply preserves host availability.
Preview wait integration
apps/server/src/mcp/toolkits/preview/handlers.ts, apps/server/src/mcp/McpHttpServer.test.ts
Preview handlers forward the grace period, and preview_wait_for uses 1,500 ms. HTTP server tests verify that a later preview_status call reaches the same host after a timeout reply.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary fix: retaining preview hosts after a waitFor miss at the deadline.
Description check ✅ Passed The description includes complete What Changed and Why sections, explains the broker-only grace behavior, documents issue context and validation results, and addresses the non-applicable UI checklist …
Linked Issues check ✅ Passed Issue [#12898] requires an unsuccessful preview_wait_for to return PreviewAutomationTimeoutError, preserve the caller timeoutMs, keep the host and pinned session available, route later calls to …
Out of Scope Changes check ✅ Passed The changes are limited to the broker timeout input, preview handler forwarding and configuration, and automated broker and MCP tests. These changes directly support issue [#12898] by adding broker-on…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

…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>
@mInrOz
mInrOz force-pushed the fix/preview-wait-for-host-grace branch from 49576f1 to 9f9fdee Compare September 21, 2026 11:40

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: preview_wait_for still evicts a live host when its miss reply lands at the broker deadline

1 participant