Rework connection liveness checking and connection reuse

[SpencerTorres]

Summary

While investigating a performance issue related to connections, several issues were identified and fixed:

• connCheck is now a non-destructive peek (recvfrom(MSG_PEEK) instead of a 1-byte
  read). Pending bytes on an idle connection still mark it bad, which is correct for Native protocol. In these cases a debug message is logged.
• Liveness results are trusted for 1s (via connLivenessWindow): a connection that
  completed a command or passed a check within the last second skips the socket
  probe. This removes the per-operation connection checks that read from the TCP connection.
• New Options.ConnIdlePingThreshold (DSN: conn_idle_ping_threshold, default 1m,
  negative disables): This is used for proper client Ping commands to validate the connection is valid. For cloud there are often a lot of proxies and other errors, so instead of depending on a client side socket check, we can now use a ping command to validate the connection before executing a command.
• Server exceptions no longer burn the connection. release() keeps the connection
  and marks it unverified; the next acquire confirms liveness with a ping before handing
  it out (covering the rare exception classes where the server does close). This should reduce the number of new connections formed for non-fatal connection errors.
• query() releases the connection before closing the rows channels, so the
  connection is guaranteed to be back in the pool by the time rows.Close() returns.
  Parameter-binding failures now release the connection
  healthy instead of through the error path.
• prepareBatch returns the connection to the pool on validation failure instead of
  leaking it and its MaxOpenConns slot.
• batch contextWatchdog stop is now race-free (mutex + stopped flag): once stop()
  returns, the callback has either completed or will never run. The Send watchdog also
  flags the connection closed (setClosed) so a cancelled batch's socket can never be
  pooled looking healthy.

Behavior changes

Change	Old	New	Escape hatch
Server exception	connection closed, next query re-dials	connection kept, ping-verified on next acquire	(strictly fewer dials)
Idle ≥ ConnIdlePingThreshold	handed out unverified	one Ping round trip on acquire	raise threshold or set negative
Socket probe frequency	every acquire + every Put + every std op	at most once per second per connection	(strictly fewer syscalls)

Context cancellation mid-query still closes the connection by design (the response is not
drained), and this is asserted in the regression tests.

Testing

• New regression suite tests/conn_check_churn_test.go: counts physical dials via a
  custom DialContext and probes idle sockets with MSG_PEEK between operations. Asserts
  exactly 1 dial for sequential query/exec/batch/server-logs/early-close/timeout-ctx
  workloads (plain TCP and TLS), including 50 consecutive failing queries and mid-stream
  exceptions (the probe also proves the socket is byte-clean at the exception boundary,
  validating reuse safety). A blackhole-proxy test simulates a silently dropped flow and
  verifies the idle ping discards the half-open connection and recovers within the dial
  timeout instead of hanging.
• Full tests/, tests/std, and tests/issues suites pass against ClickHouse 26.6;
  root unit tests pass with -race; cross-compiles clean for all GOOS targets covered
  by the conn-check build tags (the Recvfrom call matters on solaris/illumos).

Checklist

• Unit and integration tests covering the common scenarios were added
• For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

As always, let me know if you have any questions or want to split this into multiple PRs. Thanks!

[github-actions]

🤖 Claude review

Reworks connection liveness checking and reuse: connCheck becomes a non-destructive MSG_PEEK, liveness results are trusted for a 1s window, a new ConnIdlePingThreshold ping-verifies idle/half-open connections on acquire, server exceptions keep the connection (ping-verified before reuse) instead of burning it, query() releases the connection before closing the rows channels to kill sequential-loop churn, and contextWatchdog stop is made race-free.

The design is careful and the reasoning holds up under tracing:

• The early release() in query() is safe — blocks are decoded into freshly-allocated proto.Block values (conn.go:414), so a reused connection cannot corrupt blocks still queued in stream. Moving release ahead of close(stream)/close(errors) is what guarantees the connection is pooled by the time rows.Close() returns.
• The idle-ping suppression bug from the prior review is resolved: a clean peek now stamps a separate lastPeekAt, not lastLiveAt, so it no longer cancels the ping revalidateIdle depends on. The new idle beyond liveness window scenario (1200ms threshold / 1500ms idle) directly guards it.
• revalidateIdle bounds the ping deadline and ping() honors ctx, so a half-open socket fails fast rather than hanging.

The regression suite is strong (dial-counting, MSG_PEEK socket inspection, TLS, blackhole proxy) and covers TCP + TLS on the native surface.

One gap (should_fix): the new server-exception “keep the connection” behavior is asymmetric between the native and database/sql surfaces — see the general finding. Verdict: approve; the gap is non-blocking and correctness is preserved by database/sql retry.

Blind spots: could not run the integration suite against a live server; relying on the PR's reported results for the churn/half-open assertions and the solaris/illumos Recvfrom cross-compile.

Verdict: ✅ Approve

General findings

• ⚠️ Should fix — std path keeps exception connections without the ping-verify safety net
  The native release() marks a connection unverified after a server Exception so the next acquire() ping-verifies it before reuse — explicitly to cover the rare exception classes where the server does close. The database/sql surface gets only half of this: std.release keeps the connection on Exception (returns without closing) but never marks it unverified, and stdConnect has no markUnverified. So ResetSession’s revalidateIdle pings only on the idle-threshold path, never because of a prior exception.

  • Result: a std connection kept after an exception and reused within the 1s liveness window (and under the idle threshold) skips both the peek and the ping, and a query lands on a possibly-dead socket → ErrBadConn retry. Correctness is preserved by database/sql, but the native safety net does not extend to the std surface.
  • The new std release() semantics (close on non-exception error, keep on exception) have no regression test — TestConnCheckChurnErrorPaths is native-only. Consider marking the connection unverified in std.release for parity and adding a database/sql server-exception-keeps-connection test.

_{Inline comments are attached to the relevant lines. This summary updates in place on re-review.}