镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix(tailscale): serve setup no longer times out on a long success summary - #34743

Merged
juliusmarminge merged 2 commits into
mainfrom
factory/issue-34736
Nov 12, 2026
Merged

juliusmarminge merged 2 commits into
mainfrom
factory/issue-34736

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Nov 12, 2026 •

Copy link
Copy Markdown
Member

runTailscaleCommand (packages/tailscale/src/tailscale.ts) collected the child's stderr and awaited its exit code but never read stdout. When tailscale serve --bg printed a long success summary (e.g. many existing routes), the stdout pipe filled, the child blocked on write before exiting, and the 10s Effect.timeout surfaced TailscaleCommandTimeoutError although the mapping was already applied. The fix adds Stream.runDrain(child.stdout) to the same concurrent Effect.all as the stderr collection and exit wait, so the pipe never backs up; the output is discarded since nothing consumes it. Applies to ensureTailscaleServe and both disable paths, which share runTailscaleCommand. No new regex or parser. The repro test from factory/repro-34736 (mocked serve child whose exit waits until stdout is consumed) failed on the base and passes now; it is kept unchanged.

Fixes #34736

Reproduction on main

Commit bc647ed (origin/main). After vp i in a fresh worktree, from apps/server I ran the reporter's probe: a fake tailscale script on PATH writes a marker file, then prints a Serve success summary with N routes and exits 0. The probe calls the real ensureTailscaleServe({localPort: 3773}) with NodeServices.layer. Output: routes=1: success in 31ms and routes=4000: TailscaleCommandTimeoutError, timeoutMs=10000, applied=true, after 10005ms. I also added a test to packages/tailscale/src/tailscale.test.ts: a mocked serve child whose exit waits until its stdout stream has been consumed. On main it fails with TailscaleCommandTimeoutError: tailscale serve timed out after 10000ms. When I temporarily drained stdout in runTailscaleCommand, all 9 tests in the file passed (reverted; the branch has only the test). No real Tailscale daemon was used.

Evidence

Run the real T3 server with serve --tailscale-serve, fresh isolated state, and the same synthetic Tailscale executable on both commits. The fixture writes an applied marker, then prints 4,000 long route lines before exiting 0. Base reports a 10-second timeout after applying the mapping; head reports successful configuration. The one-route control succeeds on both. Linux CLI evidence; images show filtered transcripts and videos replay captured event timing, with pairing credentials omitted.

Before After
bc647ed535 cae6ee44c7
Before: timeout after mapping applied After: configuration succeeds with the same summary
Before recording After recording
Original timeout diagnostic Original success diagnostic

Reproduction driver and fixture · Transcript renderer

AI-assisted; verified with the tests and tools listed above.

Not shown:

  • No real Tailscale daemon or HTTPS connection was exercised; the reproduction intentionally uses a synthetic subprocess and applied marker.
  • The pair --tailscale entry point and desktop/mobile clients were not separately exercised; proof covers real server startup with --tailscale-serve.

Tests

Before fix: vp test run src/tailscale.test.ts (packages/tailscale) -> 1 failed (TailscaleCommandTimeoutError), 8 passed. After fix: same command -> 9/9 passed. vp run check:changed -> format, lint, knip (2/2), typecheck (5 packages), tests (2 files, 11 tests) all pass.

Not in this change

  • The fixed 10s TAILSCALE_SERVE_TIMEOUT and the stderr-only diagnostics on a non-zero exit were left unchanged.

AI-assisted; verified with the tests and tools listed above.

Reproduces #34736: runTailscaleCommand never reads the child's stdout, so a
serve command whose summary fills the pipe cannot exit and hits the 10s timeout.

AI-assisted; verified with the tests and tools listed above.
…mary

runTailscaleCommand read stderr and waited for exit but never read stdout,
so a long Serve summary filled the pipe and blocked the child before it
exited. Drain stdout alongside stderr.

AI-assisted; verified with the tests and tools listed above.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Nov 12, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at cae6ee4

Macroscope's review found this PR approvable — The runner now drains Tailscale stdout so long success summaries cannot block command completion. Serve arguments, product defaults, timeouts, and cleanup behavior remain unchanged.

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

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 21.5 KiB 21.5 KiB 0 B (0.0%) 24.4 KiB ✅
Codex Live turn messages 1 1 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.9 KiB 21.9 KiB 0 B (0.0%) 24.4 KiB ✅
Claude Live turn messages 1 1 0 (0.0%) 8 ✅

Baseline: 8c15be9 · PR result: cae6ee4 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The Tailscale command runner now drains child stdout while collecting stderr and awaiting the exit code. A regression test checks that ensureTailscaleServe completes when the child exits after its stdout is consumed.

Changes: Tailscale command output

Layer / File(s) Summary
Drain stdout and test command completion
packages/tailscale/src/tailscale.ts, packages/tailscale/src/tailscale.test.ts
runTailscaleCommand now drains stdout while collecting stderr and awaiting the exit code. A regression test uses a mocked child that waits for stdout consumption before exiting, then verifies that ensureTailscaleServe succeeds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: t3dotgg

Merge Risk: Minimal · up to cae6e

Successful Tailscale Serve setup should no longer hang on large success output. Failure and timeout behavior stays the same. No merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the fix and its scope: tailscale serve setup no longer times out when the success summary is long. The changeset drains child stdout in runTailscaleCommand, which resolves the timeout …
Linked Issues check Passed [#34736] Requirement: a successful Tailscale Serve command must not time out because its stdout fills the pipe. The head diff in packages/tailscale/src/tailscale.ts adds Stream.runDrain(child.stdout) …
Out of Scope Changes check Passed All changes are in packages/tailscale/src/tailscale.ts (one stdout drain in the shared runTailscaleCommand runner, with a comment) and packages/tailscale/src/tailscale.test.ts (one regression test and…
Description check Passed The description follows the repository template closely. It explains the problem (unread stdout fills the pipe, the child blocks, and the 10s timeout fires even though the mapping was applied) and the…
Full details: Title check

Explanation

The title names the fix and its scope: tailscale serve setup no longer times out when the success summary is long. The changeset drains child stdout in runTailscaleCommand, which resolves the timeout caused by an unread stdout pipe. The title follows conventional commit form and is specific to the primary change.

Full details: Linked Issues check

Explanation

[#34736] Requirement: a successful Tailscale Serve command must not time out because its stdout fills the pipe. The head diff in packages/tailscale/src/tailscale.ts adds Stream.runDrain(child.stdout) to the same Effect.all (concurrency unbounded) as the stderr collection and exitCode wait. Stdout is now read continuously, so the child can finish writing and exit. The non-zero-exit path (exitCode mapping, stderr collection, error mapping) is unchanged. The regression test 'drains serve stdout so a long success summary does not time out' uses a mocked child whose exitCode waits until stdout is drained. On the base, this child never exits, so the 10 s timeout fires after TestClock advances 11 seconds. With the fix, the fiber succeeds. This models the reported backpressure mechanism. Gaps: the test uses one short stdout chunk, not a large response, and it does not check the simulated mutation marker or compare small and large responses as the issue's restoration check describes. The real-CLI integration check requested in the issue was not run; it depends on a real Tailscale daemon and is a manual verification, so it is not a coding requirement for this PR. The 10 s timeout and stderr-only diagnostics are not changed, as the PR description states.

Full details: Out of Scope Changes check

Explanation

All changes are in packages/tailscale/src/tailscale.ts (one stdout drain in the shared runTailscaleCommand runner, with a comment) and packages/tailscale/src/tailscale.test.ts (one regression test and its supporting Effect imports for Deferred, Exit, Fiber, and TestClock). The drain applies to ensureTailscaleServe and both disable paths because they share runTailscaleCommand. This is the behavior the linked issue #34736 requests. No parser, regex, timeout value, or stderr diagnostic changes are present. The PR description's evidence section is pending, but that is a process item, not a code change.

Full details: Description check

Explanation

The description follows the repository template closely. It explains the problem (unread stdout fills the pipe, the child blocks, and the 10s timeout fires even though the mapping was applied) and the change (Stream.runDrain(child.stdout) added to the concurrent Effect.all). It links the issue (#34736) and gives a reproduction on main, with a mocked-child regression test that fails on base and passes with the fix. Verification lists the focused test command with before/after results (8 passed/1 failed, then 9/9) and a broader check:changed run. It also states what was not checked: no real Tailscale daemon, and the pair --tailscale and desktop/mobile clients were not exercised. The Scope and approval section is the weakest part. It references the linked issue but does not name a maintainer approval comment. The objectives say the issue requests this successful-output backpressure fix, so the scope is largely clear. The description also says the evidence was captured on a real server run, but the objectives note that real-app evidence was still pending, which is a minor inconsistency. Overall the description is mostly complete.

Finishing Touches

Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 8c15be9 and cae6ee4.

Files selected for processing: 2

  • packages/tailscale/src/tailscale.test.ts
  • packages/tailscale/src/tailscale.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@juliusmarminge
juliusmarminge merged commit 9c19aa9 into main Nov 12, 2026
36 checks passed
@juliusmarminge
juliusmarminge deleted the factory/issue-34736 branch November 12, 2026 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Tailscale Serve setup times out on a large success summary after applying its mapping

1 participant