Skip to content

fix(plugin-clickhouse): query timeout never reaches the server and Stop kills a stale query - #3223

Open
datlechin wants to merge 4 commits into
mainfrom
fix/clickhouse-timeout-and-cancel
Open

datlechin wants to merge 4 commits into
mainfrom
fix/clickhouse-timeout-and-cancel

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

  • ClickHouse query timeout never reached the server, and capped SELECTs failed at 60 seconds whatever it was set to. The timeout went out as a SET max_execution_time on its own request with no session_id, so ClickHouse dropped it when that request ended, and the streamed read request never set timeoutInterval, so it kept URLRequest's 60 second default.
  • Stop on a ClickHouse SELECT left the query running on the server. Streamed reads sent no query_id and never recorded one, so KILL QUERY named the previous statement, which had already finished.
  • ClickHouse connect errors such as a wrong password or an unknown database showed as "Failed to establish connection". connect replaced every failure that was not a TLS refusal with that generic error and dropped the server's own exception text, which the HTTP layer had already read.
  • ClickHouse never presented the Client Certificate and Client Key to a server that requires mutual TLS. The TLS delegate answered only the server trust challenge and never read either field, and with system trust and no CA it was not installed at all.

Tests

  • ClickHouseQueryTimeoutTests (new): timeoutReachesEveryStatement, streamedReadTakesTheClientTimeout and refusedLimitIsNotSentAgain fail without the fix (no max_execution_time on any request, a SET request, a 60 second stream timeout, no error for a limit the server refuses). noTimeoutSendsNoLimit passes on both and guards the no-limit case.
  • ClickHouseCancelQueryTests (new): stopKillsBoundedStream, stopKillsUnboundedStream and streamedStatementsGetDistinctIds all fail without the fix, because neither stream request carries a query_id.
  • ClickHouseConnectFailureTests (new): authenticationFailureKeepsServerText, unknownDatabaseKeepsServerText and transportFailureNamesTheCause fail without the fix (each maps to "Failed to establish connection"). emptyErrorBodyNamesTheStatus and emptyStreamedErrorBodyNamesTheStatus fail without the fix (an error status with an empty body gives an empty message). recordedRefusalWins and untrustedCertificateIsTLS pass on both and guard the TLS classification.
  • ClickHouseTLSConfigurationTests (existing, five cases added): clientIdentityAnswersTheCertificateChallenge, clientIdentityKeepsSystemServerTrust and unusableClientIdentityIsRefused fail without the fix. clientIdentityKeepsSystemServerTrust also fails when Verify Identity without a CA is changed to skip verification, since it hands the delegate a challenge that carries a real SecTrust. noClientIdentityLeavesTheChallenge and disabledPresentsNothing pass on both and guard the existing behavior.
  • All 24 ClickHouse and HTTP timeout suites pass: 166 of 166. AllPlugins builds.

Docs

The docs rewrite branch covers the user-facing text for these changes.

This branch has not been deployed

No deployments
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