fix(ssh): report why a tunnel cannot reach the database instead of a driver timeout (#1981) - #1985
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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.
Fixes #1981.
The bug
MySQL 8.0 over an SSH tunnel fails at connect with:
The same settings connect in TablePlus. No repro steps, no logs, no screenshots, which is itself part of the bug: TablePro had the reason and did not report it.
errno 35 carries no information
Worth stating up front, because it is the trap here. MariaDB Connector/C does not use
SO_RCVTIMEO. It keeps the socket non-blocking and drives timeouts withpoll():Two different failures both end at errno 35:
recvgives-1/EAGAIN,polltimes out, the function returns-1, errno still holds 35.recvgives-1/EAGAIN,pollreports readable because a FIN landed,recvreturns 0, the loop exits. errno still holds 35 from the first call.So errno 35 cannot tell "the tunnel never delivered a byte" from "TablePro closed the socket". Anything that reasons from it is guessing. The underlying connectivity condition is not recoverable from the report, and this PR does not pretend otherwise.
Root cause
Two defects we own, independent of whatever the reporter's server is doing.
The reason is computed and thrown away.
SSHForwardChannelOpenPump.run()returns.failed(errorCode)or.timedOut.handleChannelOpenOutcomeconsumed it as:It went to OSLog and nowhere else. The driver then reached the user first with an errno that means nothing.
The guard against exactly this could never fire.
channelOpenDeadlineSecondswas 10s, with a comment claiming it "matches the driver's own connect timeout so TablePro reports the cause before the driver gives up blind". But its clock started inopenAndRelay, which runs afteraccept()returned, after aTask.detachedontoacceptQueueat.utility, after a 1000ms accept poll, and after a secondTask.detachedplusrelayQueue.async. The driver's 10s starts atmysql_real_connect(). Equal budget, strictly later start, so TablePro always loses the race.That makes this a recurrence of #1883, whose test file already describes the symptom: "a stuck open outlives the database driver's connect timeout, leaving an accepted socket that is never written to and never closed, which the driver reports as a greeting-read timeout with no stated cause."
Why a refactor and not a tighter timeout
Shaving seconds off the deadline is the same shape of fix that already failed once. It keeps the outcome dependent on winning a race, and the margin is unenforceable anyway:
DatabaseTypeis open-ended, and a registry plugin's C-level connect timeout cannot be read fromCore/SSH.The deeper problem is that
createTunnelreturns a port once the SSH hop works, which says nothing about whether forwarding works. Aconnect()to a listening-but-unaccepted loopback socket succeeds in about 0.1ms, so the driver always connects and then waits. "Tunnel created" needed to start meaning "the destination is reachable".I also weighed replacing the local listener with
NWListener, which TN3151 prefers over BSD sockets, and rejected it. libssh2 is TN3151's own stated exception for an existing socket-based library,SSHChannelRelayis fd-based end to end, and the accept-before-verify behaviour is a kernel backlog property that is identical under either API. It would be a structural change for platform purity with no behavioural gain.What changed
LibSSH2TunnelFactory.probeForwardDestinationreplacesverifyUnixSocketDestination. It opens the same channel a client would, closes it, and throws a typed error if it cannot. Covers TCP as well as sockets, and reusesSSHForwardChannelOpenPump, so it is bounded rather than the unbounded blocking call the socket path used to make.SSHTunnelError.forwardRefused/.forwardTimedOut, alongside the existing.socketForwardingRefused.SSHForwardFailureRecorderholds the reason for failures that only appear per client, once the tunnel is already running.DatabaseManager+Sessionsconsults it in the existing catch block and replaces the driver's error. Not a prefix: Beekeeper Studio shipped the prefix version in beekeeper-studio#1787 and reopened it as #4358 because the string still led with the generic error.SSHForwardChannelAttempt.failedandChannelOpenOutcome.failednow carry libssh2's message, which is what names the cause..transportHangupwas logged, so a channel that opened just after the client gave up produced no trace at all.The
.forwardRefusedmessage names the most likely cause directly, because it is the classic mistake and the docs already warned about it while the app did not:This is why "works in TablePlus" needs no library difference.
sshForwardDestinationsends the connection's rawhostfield to sshd, and MySQL 8 defaults tobind-address = 127.0.0.1. A different host value in the other app's entry is the whole story.Also in this PR
Three confirmed defects found during the investigation, none of them the cause of #1981, each in its own commit:
libssh2_keepalive_sendreturns against a non-blocking session, so it can answerLIBSSH2_ERROR_EAGAINwhen the transport is busy. That was treated as fatal and tore down a healthy tunnel under load, along with every connection on it.NSLocalNetworkUsageDescription. The iOS target declares it, the Mac target did not. macOS 15+ gates outbound TCP to local network addresses behind that permission for non-sandboxed apps too (TN3179), and a denied app fails fast withEHOSTUNREACH. The Mac app is not sandboxed, so entitlements are not involved.ProxyCommand. Zero occurrences in the codebase.SSHConfigResolverdropped it as.unrecognizedwithout a word, so a config that needs it connects somewhere else silently. Now logged, scoped to directives that change routing so the dozens of benign unparsed ones stay quiet. Implementing it is a feature and is not attempted here.The trade-off
The probe opens and immediately closes a real TCP connection to the database on every tunnel creation, including health-monitor reconnects. That shows as an aborted connection in the server log and increments
Aborted_connects. Onmax_connect_errorshost blocking: MySQL resets that counter on any successful connect from the host, and the real connection follows immediately, so it should not accumulate in normal use. Cost is one extra round trip per connect. The Unix-socket path already paid this; it is now symmetric.Tests
New pure seams, so the logic that used to be unreachable inside private methods is now testable without a live SSH server:
ChannelOpenOutcomeTunnelErrorTests: outcome to error mapping, TCP against socket destinations, timeout against refusal.SSHForwardFailureRecorderTests: only failures recorded, consume-once, a later success does not clear an unread reason, latest failure wins.SSHTunnelDeadlineMarginTests: regression guard on the 6s/10s margin and the accept poll granularity. This is the one that would have caught the original bug.SSHKeepAliveResultTests,SSHUnsupportedDirectiveTests, plus a parser test confirmingProxyCommandreaches the classifier andProxyJumpstill does not.SSHTunnelErrorTestsgains the two new descriptions.Gap called out rather than hidden: there is still no integration test of
LibSSH2TunnelorSSHTunnelManagerorchestration, because both are wired to raw libssh2OpaquePointers with no fake layer. Building one is out of scope. No UI automation either, since triggering this needs a live SSH server with a controllable refusing target, matching the other network-error paths in the app.Docs
docs/databases/ssh-tunneling.mdxtroubleshooting previously told users to open Console.app and read theLibSSH2Tunnellog, which was a documented workaround for this defect. Replaced with entries for the two new messages, the bind-address trap named explicitly, and a note on the macOS Local Network permission.Not verified locally
swiftlint and swiftformat are not installed on this machine, and I did not run a build. Syntax checked with
swiftc -parse, line lengths checked against the 180 limit,plutil -linton the plist, and the banned-words gate run on every staged diff.