fix(connections): wait for a stale tunnel process to exit before reusing its local port - #2602
Merged
Merged
Conversation
…ing its local port Claude-Session: https://claude.ai/code/session_01Gy6Q4tzwG3bL9h15SMqep1
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A crash or force-quit leaves
cloudflaredorcloud-sql-proxyrunning, still holding the local port it was told to listen on. The launch-time cleanup signalled it and returned:Three problems. It never confirms the process exited. Nothing makes a connection wait for it. And the
deferdeletes 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:
createTunnelcomputesattempts = config.localPort != nil ? 1 : portRetryCount, so a configured port gets exactly one try. If the orphan is still on it,awaitReadinessfails with an address already in use and the connection errors out. The auto-port path was never affected, becauseallocateFreePort()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 sameproc_pidpathliveness check and the same fire-and-forget loop; that is now one implementation.cloudflaredandcloud-sql-proxyboth close their listener and exit well inside the 2s grace, so the escalation is the rare path.waitpidis unavailable and readingproc_pidpathis the only way to see them go.SIGKILLis retried next launch instead of being forgotten.sweepStalePidsIfNeeded()became an idempotent once-per-process task thatcreateTunnelawaits, so it is both the launch-time cleanup and the barrier. It still starts fromapplicationDidFinishLaunching, 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 ontomainverify.sh lint TablePro TableProTests: 0 violationsStaleProcessReaperTestsis 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 onlySIGTERM, a stubborn one is escalated toSIGKILL, 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):All of the growth is in one segment,
intentsRouted → firstWindowOrdered, and it is linear at about 0.15ms per saved connection: the Welcome window'sListresolves 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.onMovenor.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:
connectionRowcallsSSHProfileStorage.shared.profile(for:)per row for any connection with an SSH profile, andWelcomeActionsPanelholdsUpdaterBridge.sharedas 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