Skip to content

fix(connections): wait for a stale tunnel process to exit before reusing its local port - #2602

Merged
datlechin merged 1 commit into
mainfrom
fix/stale-tunnel-port-barrier
Sep 2, 2026
Merged

fix(connections): wait for a stale tunnel process to exit before reusing its local port#2602
datlechin merged 1 commit into
mainfrom
fix/stale-tunnel-port-barrier

Conversation

@datlechin

Copy link
Copy Markdown
Member

The bug

A crash or force-quit leaves cloudflared or cloud-sql-proxy running, still holding the local port it was told to listen on. The launch-time cleanup signalled it and returned:

defer { defaults.removeObject(forKey: stalePidsDefaultsKey) }   // record gone before anything is confirmed
for record in records where Self.isLiveCloudflared(record) {
    kill(record.pid, SIGTERM)                                    // fire and forget
}

Three problems. It never confirms the process exited. Nothing makes a connection wait for it. And the defer deletes the persisted record before any of that, so a sweep that does not finish forgets the port is still held.

The path that breaks is a fixed local port: createTunnel computes attempts = config.localPort != nil ? 1 : portRetryCount, so a configured port gets exactly one try. If the orphan is still on it, awaitReadiness fails with an address already in use and the connection errors out. The auto-port path was never affected, because allocateFreePort() simply does not hand back a port something else is bound to.

This is pre-existing, but #2598 removed a fixed 150ms wait from launch that had been supplying incidental termination grace, so it is easier to hit now. Codex flagged it reviewing that PR; I reported it there and said a barrier inside each manager was the real fix. This is that.

The change

StaleProcessReaper (TablePro/Core/Process/) signals, waits, escalates once, and reports what survived. Both managers had their own copy of the same proc_pidpath liveness check and the same fire-and-forget loop; that is now one implementation.

  • SIGTERM, then poll until the process is gone, then SIGKILL, then confirm. cloudflared and cloud-sql-proxy both close their listener and exit well inside the 2s grace, so the escalation is the rare path.
  • Polling is not laziness. These are not this process's children, so waitpid is unavailable and reading proc_pidpath is the only way to see them go.
  • The executable is re-checked before every signal, so a pid the OS recycled between the crash and now is never signalled. That guard existed before and is kept.
  • A survivor keeps its record. Only pids confirmed gone are dropped, so a process that outlives SIGKILL is retried next launch instead of being forgotten.

sweepStalePidsIfNeeded() became an idempotent once-per-process task that createTunnel awaits, so it is both the launch-time cleanup and the barrier. It still starts from applicationDidFinishLaunching, and both managers are actors, so it hops off the main actor immediately and costs the first frame nothing. The wait is only ever paid by a connection that would otherwise have failed.

Verified

  • verify.sh test StaleProcessReaperTests CloudflareTunnelManagerTests CloudSQLProxyManagerTests: PASS, 23/23 on this branch rebased onto main
  • earlier run alongside the launch suites: PASS, 50/50
  • verify.sh lint TablePro TableProTests: 0 violations

StaleProcessReaperTests is new. The reaper takes its liveness check, its signal function and its timings as parameters, so a fake process table drives it with no real processes and no sleeping: a process already gone is never signalled, a recycled pid is never signalled, a polite one gets only SIGTERM, a stubborn one is escalated to SIGKILL, and an unkillable one is reported so its record survives. The two managers' existing sweep tests pass unchanged.

Also measured, not changed

While confirming there was nothing else on the launch path, I measured launch against saved-connection count on a Release build (TABLEPRO_LAUNCH_TRACE=1, warm, medians of 5):

saved connections time to first frame
0 265ms
50 327ms
200 348ms
800 450ms

All of the growth is in one segment, intentsRouted → firstWindowOrdered, and it is linear at about 0.15ms per saved connection: the Welcome window's List resolves every row during the first layout rather than only the visible ones. vm.setUp() is not the cost (2.4ms at 200 connections) and neither is .onMove nor .swipeActions (removing both changed nothing).

I did not fix it. At a typical count it is a few milliseconds of a 265ms launch, and making that list lazy is a change to the app's most visible screen that deserves its own PR and its own review. Two smaller things found on the way, also unchanged: connectionRow calls SSHProfileStorage.shared.profile(for:) per row for any connection with an SSH profile, and WelcomeActionsPanel holds UpdaterBridge.shared as a stored property, so Sparkle's updater starts inside the first frame. Neither was measurable above the run-to-run noise here.

https://claude.ai/code/session_01Gy6Q4tzwG3bL9h15SMqep1

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit 68afabc into main Sep 2, 2026
8 checks passed
@datlechin
datlechin deleted the fix/stale-tunnel-port-barrier branch September 2, 2026 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant