Stop Raft and complete pending session-ID requests before waiting for Keeper TCP handlers. Drain and check the request dispatcher queues only after the handlers exit, when all response deallocation callbacks have completed.
Shut down storages before closing embedded Keeper connections so replicated storages can still use Keeper during cleanup. Then close the sockets and stop the dispatcher before waiting for TCP handlers, which wakes idle connections without waiting for their session timeout.\n\nComplete pending session-ID requests only after the Raft commit thread has stopped to avoid racing with a late committed session.
Idle Keeper sessions can keep their handlers blocked in a socket poll until the negotiated session timeout expires. Shut down registered connection sockets after stopping the listeners so blocked reads and polls return immediately.
Add an integration test that keeps a negotiated session idle while the server terminates.
CertificateReloader already hot-reloads the certificate and private key, but
the trusted CA certificates were loaded once by Poco at startup and never
refreshed, so rotating a CA required restarting every server and Keeper.
Keep a MultiVersion snapshot of an X509_STORE per prefix, rebuilt whenever the
caConfig path, its modification time or loadDefaultCAFile changes, and verify
peer certificates against the current snapshot from an
SSL_CTX_set_cert_verify_callback installed once per context (default server
and client contexts, per-protocol contexts and the NuRaft contexts). The
SSL_CTX itself is never modified after initialization, same as for the
existing certificate callback.
The caConfig paths (and the client certificate and key) are now also watched
by ConfigReloader in clickhouse-server and clickhouse-keeper, so replacing the
file on disk is enough to trigger the reload.
The two `keeper_server.http_control` listeners built their handler factory inside the
`createServer` callback, so an invalid `<handlers>` section came out of the listen error
handler rather than as itself. Measured on the unfixed binary: without `listen_try` it was
`Code: 210 ... Listen [127.0.0.1]:50744 failed: ... (NETWORK_ERROR)`, and with `listen_try`
it was reduced to advice about `<listen_host>` while keeper kept running without the
control endpoint.
`createKeeperHTTPHandlerFactory` reads `keeper_server.http_control.handlers` and throws
`INVALID_CONFIG_PARAMETER` / `UNKNOWN_ELEMENT_IN_CONFIG` from it, and that parse cannot
fail for any socket reason, so it is hoisted above `createServer` behind a port guard
matching `createServer`'s own early return, the same shape already used for the HTTP,
HTTPS and Prometheus listeners here and in `clickhouse-server`.
This is the last listener in the family, reported by review on the Prometheus hoist. The
regression test grew control cases for both exits of the error handler, the port guard and
a valid section; the two error cases redden against the pre-hoist binary while the
Prometheus cases stay green, so they discriminate this half. The test is renamed since it
now covers both listeners.
The standalone Prometheus listener of clickhouse-keeper built its handler factory inside the
createServer callback, like the server did, so an invalid <prometheus.handlers> section came
out as
Code: 210. DB::Exception: Listen [127.0.0.1]:9363 failed: Code: 137. DB::Exception:
Unknown type no_such_prometheus_type ... (NETWORK_ERROR)
and with listen_try it was reduced to advice about <listen_host> while keeper carried on with
no metrics endpoint. It is the same shared parse, createPrometheusHandlerFactoryImpl, that
the server reaches, so the same hoist above createServer fixes it, behind a port guard
matching that function's own early return. Keeper HTTP-control is still not changed.
Measured on both binaries: before, wrapped into NETWORK_ERROR and swallowed under listen_try;
after, a bare UNKNOWN_ELEMENT_IN_CONFIG in both, and startup stops rather than running
without the endpoint. A valid section still serves metrics, keeper without <prometheus> and
with <prometheus> but no <port> both stay healthy, and rebuilding with the guard forced on
refuses that last configuration, so the guard is load-bearing.
The regression test drives the keeper entrypoint directly, following
04498_keeper_psi_metrics_setting. Ports are randomized and retried on collision because
nothing in the environment is unique per concurrent copy, and a collision before the
Prometheus listener is reported rather than asserted through, so it cannot pass for the
wrong reason.
The `include_from` server setting used to fall back to `/etc/metrika.xml`
whenever that file existed. This is surprising: a file outside the ClickHouse
configuration directory, with unrelated access rights and a name that has
nothing to do with ClickHouse, could silently supply configuration
substitutions. Most users do not know the file is consulted at all.
Now `include_from` is empty by default and substitutions are only read from
the path it explicitly names. The fallback is removed from the server config
processor, from the users' configuration (access storages), from Keeper, and
from disks created by SQL.
Closes: https://github.com/ClickHouse/ClickHouse/issues/102836
AsynchronousMetrics derived TCPThreads, HTTPThreads and friends from a fixed
map keyed on the listener's port_name, which is a config key. A listener
declared under <protocols> has port_name == "protocols.<name>.port", so it
matched nothing and reported no metrics at all.
ProtocolServerAdapter now carries a ServerType::Type and AsynchronousMetrics
matches on that instead. For a <protocols> listener the type is the innermost
layer of its stack, since tls and proxy1 only wrap another protocol.
This also gives metrics to tcp_with_proxy_port, tcp_ssh_port and
arrowflight_port, which were never in the map either, and sums the values of
listeners that share a protocol instead of letting the last one win.
Change-Id: I895895b64814c3638f06c664accb6e4be6de984c
Closes: https://github.com/ClickHouse/ClickHouse/issues/80759
Branch was 1157 commits behind master and the only CI red was the chronic
master-wide `Hung check failed, possible deadlock found` flake (13-70/day on
master itself; this PR is a compile-time no-op under `SANITIZER` so it cannot
affect the `arm_asan_ubsan` stress run). Re-merge to refresh CI on the current
master and to re-run the AI review against the already-updated PR description.
The merge conflicted only in `src/Common/MemoryWorker.cpp`: master evolved the
`memory_worker_dynamic_hard_limit` apply block to skip adjustments smaller than
`min_change_ratio` (1%) of the current limit, still naming the local
`current_hard_limit`. The branch had renamed that local to `installed_hard_limit`
to avoid shadowing the `current_hard_limit` the speculative-reserve block reads
earlier in `updateResidentMemoryThread` (`-Wshadow`/`-Werror`). Kept master's new
threshold logic verbatim and renamed its local to `installed_hard_limit`; the
`LOG_TRACE` already references `installed_hard_limit`. No behavior change.
Verified the resolved `MemoryWorker.cpp` compiles cleanly (`-fsyntax-only`,
`-Werror -Wall -Wextra`) against the merged tree.
Cap the speculative reservation at `current_hard_limit - resident` *before*
adding it to `resident`, and skip speculation entirely once
`resident >= current_hard_limit`. This removes any chance of the signed
`Int64` addition overflowing (the previous code added a hard-limit-sized
`reserve` to `resident` and only capped afterwards), as flagged in review,
while preserving full early-throw power: once the published RSS reaches the
hard limit, the next positive allocation already trips the
`will_be_rss > current_hard_limit` branch in `MemoryTracker::allocImpl`.
Stop wiring `memory_worker_rss_speculative_reserve_ratio` into
`clickhouse-keeper`: it never sets the global hard limit (it enforces
`keeper_server.max_memory_usage_soft_limit` separately), so the speculation
gate `current_hard_limit > 0` makes the setting a no-op there. Leaving it
disabled (`ratio = 0`) avoids exposing a setting that cannot take effect.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`programs/server/Server.cpp` and `programs/local/LocalServer.cpp` were
passing the new server setting `memory_worker_rss_speculative_reserve_ratio`
into `MemoryWorkerConfig`, but `programs/keeper/Keeper.cpp` was building
its `MemoryWorkerConfig` without it, so the configured value was silently
ignored by `clickhouse-keeper`. Add the missing `ServerSetting`
declaration and the corresponding field initialiser. Spotted by AI review
on https://github.com/ClickHouse/ClickHouse/pull/104976.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The defaults for `http_max_fields` (1,000,000) and
`http_max_field_name_size` (128 KB) allowed unauthenticated HTTP
clients to trigger large memory allocations before any authentication
check, since HTTP headers are fully parsed before authentication occurs.
This reduces:
- `http_max_fields`: 1,000,000 -> 1,000
- `http_max_field_name_size`: 128 KB -> 4 KB
Also adds `http_max_request_header_size` to cap the total size of all
HTTP headers combined, and `http_headers_read_timeout` to enforce a
wall-clock deadline on header parsing (via `DeadlineReadBuffer`).
Applied to both ClickHouse server and Keeper.
Backport of ClickHouse/clickhouse-private#53097.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split request processing (batching, session management, request/response
threads) into a separate `KeeperRequestDispatcher` class, leaving
`KeeperDispatcher` as a thin shell for snapshots, session ID assignment,
session cleanup, and cluster configuration management.
Key interface changes:
- `KeeperStateMachine` now takes a `KeeperResponseCallback` instead of
a `ResponsesQueue &`, decoupling the state machine from the queue type.
- `KeeperServer` constructor takes `KeeperResponseCallback` instead of
`ResponsesQueue &`.
- `KeeperServer::KeeperRaftServer` moved from .cpp anonymous definition
to the header for visibility.
- `ZooKeeperResponseCallback` moved to `KeeperCommon.h`, returns `bool`
for future flow control.
- `shutdown` now takes a `closed_all_connections` parameter.
- Consolidated `finishSession` calls into a single `SCOPE_EXIT` in
`KeeperTCPHandler`.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `static` to file-local functions and forward declarations
for cross-TU functions to satisfy `-Wmissing-prototypes`.
This ensures all functions have visible prototypes before their
definitions, improving code quality and catching accidental
linkage issues.
Reference: https://github.com/ClickHouse/ClickHouse/issues/76944
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove "CMake doesn't recognize changes in #embed-ed files" comments
from all files, since this PR now tracks those dependencies.
- Add conditional `OBJECT_DEPENDS` for `FrequencyHolder.cpp` NLP blobs
(guarded by `if (EXISTS ...)` for when the submodule is not checked out).
- Add `OBJECT_DEPENDS` for `attachInformationSchemaTables.cpp` which was
also missing dependency tracking for its embedded SQL files.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Chester