Sign up
View stargazersSign up

fix(server): stop a second instance hijacking the Tailscale Serve mapping#10364

closed1
main
GitHub fork: fix/tailscale-serve-ownership
openedSep 6, 2026bySsensei-woo
7 files changed
View on GitHubTwo-way sync enabledPR last imported 9d ago

Fixes #10363.

What Changed

The Tailscale Serve mapping is now claimed only when it is free, already ours, or stale — never when it still reaches a live server.

  • apps/server/src/tailscaleServe.ts (new). acquireTailscaleServe reads the current mapping before writing:

    • already points at this server's loopback port → adopt it, log, don't rewrite;
    • points somewhere else and that target still answers /.well-known/t3/environment → log a warning naming the environment it found and return null, i.e. not owned;
    • points somewhere else and that target is unreachable → log the reclaim and write the mapping;
    • no mapping, or the read fails → treat as unclaimed and write.

    releaseTailscaleServe re-reads the mapping and calls disableTailscaleServe only while it still points here.

  • apps/server/src/server.ts. The acquireRelease layer now calls those two instead of ensureTailscaleServe / disableTailscaleServe directly. Net −42 lines; the acquire body is down to "wait for activation, read the bound port, delegate". HttpClient is already provided to this layer via FetchHttpClient.layer.

  • apps/server/src/environmentProbe.ts (new). probeEnvironmentDescriptor lifted verbatim out of cli/pair.ts so the server path reuses the exact liveness check t3 pair already had (descriptor / unreachable / not-a-t3-server; 502/503/504 and transport errors count as unreachable). pair.ts imports it now and loses its private copy (−47 lines).

  • packages/tailscale/src/tailscale.ts. Adds readTailscaleServeTarget / parseTailscaleServeTarget over tailscale serve status --json, reading only the Web["<host>:<port>"].Handlers["/"].Proxy leaf as unknown rather than modelling a shape that moves between tailscale releases. The status and serve status readers now share one captureTailscaleCommand runner so the scoped spawn, the timeout and the redacted error shape stay identical (including the existing catchDefect ENOTDIR handling). No existing export changed signature.

Why

The mapping is tailnet-wide state keyed only by HTTPS port, but the server wrote it unconditionally on startup and removed it unconditionally on shutdown, so every server started with tailscaleServeEnabled competes for the same slot. On macOS a second desktop instance is easy to get (no single-instance lock on darwin), it lands on the next free backend port, and it repoints the tailnet URL away from the healthy first one. If it then exits without its finalizer running, the mapping is left pointing at a dead port and the tailnet URL 502s with nothing in the codebase to reconcile it. Full timeline in #10363.

The approach follows the guard resolveTailscalePairingBase in cli/pair.ts already applies for the same question, rather than inventing a second policy — hence sharing the probe rather than copying it. Ownership itself is decided from tailscale serve status --json (target port + loopback host) rather than from the probed environment id, because that is also what makes the two safety properties exact:

  • Never hijacks a live primary. Any non-unreachable probe result — same environment, another environment, or something that isn't T3 at all — leaves the mapping untouched and marks this server as not owning it.
  • Never removes a mapping it doesn't own. Release is a no-op unless the mapping still points at this server's port, so a secondary that lost the race can't tear down the primary's mapping on its way out.

The stale-mapping case also self-heals now, which is the part that actually bites users: a server that starts while the mapping points at a port nothing answers on takes the slot back instead of leaving the tailnet URL broken.

UI Changes

None — server-side only.

Checklist

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

Testing

New apps/server/src/tailscaleServe.test.ts (6 cases) drives the logic through a recording ChildProcessSpawner mock and an HttpClient mock: writes when the port is free; keeps a mapping that already fronts a live server on another port (the 3773-primary / 3774-secondary case — asserts no serve --bg was spawned and the probe hit http://127.0.0.1:3773/.well-known/t3/environment); reclaims a mapping whose target no longer answers; adopts a mapping that already points here; removes the mapping on shutdown while it still points here; leaves a mapping another server has taken over.

packages/tailscale/src/tailscale.test.ts gains two cases for the new reader (parses the Web entry for a given serve port, defaults the serve port to 443).

Ran, on main @ 223ff44:

1vp run --filter @t3tools/tailscale --filter t3 typecheck -> 0 errors
2vp test run apps/server/src/tailscaleServe.test.ts -> 6 passed
3vp test run packages/tailscale/src/tailscale.test.ts -> 16 passed
4vp test run apps/server/src/cli/pair.test.ts apps/server/src/server.test.ts -> 190 passed (2 files)
5vp lint <changed paths> -> clean
6vp fmt --check packages/tailscale apps/server/src -> clean

Per AGENTS.md I did not run repo-wide checks. Not verified: no live end-to-end run against a real tailnet — the tailscale CLI is mocked at the spawner boundary in the tests, so the serve status --json shape is exercised against a captured fixture rather than a live daemon.

Deliberately out of scope, noted as a follow-up on the issue: a secondary instance also overwrites ~/.t3/userdata/server-runtime.json while the recorded pid is still alive, which is what breaks t3 pair's discovery afterwards.

Written by Claude Opus 4.6 in Claude Code.

https://claude.ai/code/session_01DXuSCeZKfJRHgukS6BZCZT

Checks

7 successful · 6 skipped · 1 neutral
All checks passed
8062748

Added 1 commit

  • fix(server): stop a second instance hijacking the Tailscale Serve mapping8062748
MmacroscopeappBotSep 6, 2026
Synced from GitHub

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes production startup and shutdown behavior for a shared Tailscale Serve mapping, adding cross-process ownership checks, HTTP probing, and new CLI-state parsing across packages. Although the intent and mocked tests are clear, the external runtime side effects and cross-component coordination warrant human review.

Not approved because:

  • Monthly spending limit reached (workspace setting). Approvability relies on correctness review in order to determine eligibility

Review your spending limits in Billing settings. You can add or adjust custom eligibility rules. Learn more.

Cchatgpt-codex-connectorBotreviewed · 2 inline threadsSep 6, 2026
Synced from GitHub

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8062748c83

ℹ️ 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".

apps/server/src/tailscaleServe.tslines 119–121Open thread
……@@ -0,0 +1,139 @@
118+ return Effect.gen(function* () {
119+ const current = yield* readTailscaleServeTarget({ servePort: owned.servePort }).pipe(
120+ Effect.orElseSucceed(() => null),
121+ );
Cchatgpt-codex-connectorBoton lines 119–121Sep 6, 2026

P2 Badge Fail closed when release cannot verify ownership

If tailscale serve status --json errors or exceeds its 1.5-second timeout after another server has taken over this Serve port, this fallback converts the failed read to null, causing the code below to run tailscale serve ... off. That tears down the newer server's live mapping precisely when ownership could not be verified; make a failed status read a no-op instead.

AGENTS.md reference: AGENTS.md:L74-L74

Useful? React with 👍 / 👎.

apps/server/src/tailscaleServe.tslines 94–96Open thread
……@@ -0,0 +1,139 @@
93+
94+ return yield* ensureTailscaleServe({ localPort, servePort, localHost: "127.0.0.1" }).pipe(
95+ Effect.as({ localPort, servePort }),
96+ Effect.tap(() => Effect.logInfo("Tailscale Serve configured", { localPort, servePort })),
Cchatgpt-codex-connectorBoton lines 94–96Sep 6, 2026

P1 Badge Serialize Tailscale Serve ownership claims

When two servers with tailscaleServeEnabled start concurrently while the port is unclaimed (or stale), they can both finish the read/probe phase and both reach this write; Tailscale's last writer wins, so the later instance still repoints a mapping that now fronts a live primary. There is no lock, compare-and-set, or post-write ownership verification, so the concurrent-launch case this change targets remains. Serialize the claim or re-check ownership immediately before committing it.

AGENTS.md reference: AGENTS.md:L74-L74

Useful? React with 👍 / 👎.

SshivamSep 11, 2026

Note: GPT-6 on behalf of shivam (@shivamhwp).

The ownership check needs to preserve more than a proxy's host and port:

  • parseProxyTarget drops the protocol and targetOrigin always builds HTTP. An existing https://127.0.0.1:8443 target is therefore probed with the wrong protocol and can be reclaimed after that probe fails. Preserve the protocol for the probe and only adopt the plain-HTTP loopback mapping this server publishes.
  • A root Path handler or a TCP-only mapping returns null, the same value as a free port. Acquisition can overwrite it, and release can disable it without establishing ownership. Unsupported occupied handlers must remain occupied; an unreadable status must skip both acquisition and release.
  • The lifted descriptor probe classifies 502/503/504 as a dead target. That works for t3 pair probing through the Serve proxy, but acquisition probes the backend directly. An HTTP 503 from that direct target proves a service is listening and should leave its mapping alone. Keep the existing proxy behavior for pairing and distinguish direct probes.

The existing concurrent-claim finding remains: two instances can both read an empty slot before either writes. These parser and probe corrections do not serialize ownership. Claim and release need coordination across processes; another unprotected read still leaves a race.

JJulius Marminge29d ago

Note

This comment is posted by Julius' dot

This introduces startup and shutdown ownership rules for a shared Tailscale mapping, including when to reclaim an existing target. That is a substantial fix, but #10363 has no maintainer triage establishing the intended behavior and scope required by prior approval. Please obtain agreement on ownership, concurrent claims, and unreadable or occupied mappings, link the decision, then request reconsideration.