Fix startup hang when max_thread_pool_size is too small - #112928
Fix startup hang when max_thread_pool_size is too small#112928alexey-milovidov wants to merge 62 commits into
max_thread_pool_size is too small#112928Conversation
A `ThreadFromGlobalPool` created while all the threads of the global thread pool are already taken by other such jobs was silently put into the pool's queue instead of getting a thread. The queue is much larger than the number of threads by default (`thread_pool_queue_size` is 10000 against `max_thread_pool_size`), so scheduling succeeded, but nothing ever picked the job up: the threads are held by jobs that never return. Its creator got a seemingly running thread whose function was never called, and any code that then waited for that thread to make progress hung forever. An idle server permanently occupies about 200 threads of the pool for the workers of the background pools and of the background schedule pools and for the flush threads of the system logs, so a `max_thread_pool_size` below that ran out of threads while starting up. With 128 the starved job was the loader worker of the `system` database, and the server hung forever at `Wait load job 'startup Atomic database system'`. Larger values below 200 appeared to start, but silently left some background threads unstarted - for example the flush threads of the system logs, so those tables never got any data. Jobs that occupy their worker for the whole lifetime of that worker are now scheduled through `ThreadPoolImpl::scheduleThreadOrThrow`, which hands them one of the `max_threads` slots at scheduling time and throws `CANNOT_SCHEDULE_TASK` naming `max_thread_pool_size` when there is none left, rather than queueing a thread that can never start. Two components started threads in their constructor and stopped them only in their destructor, so the resulting exception left those threads running while the destructor of the member thread pool blocked forever joining them. Both now stop their workers before letting the exception out: - `MergeTreeBackgroundExecutor`, which was a second hang, reachable with `max_thread_pool_size` of 100. Its `increaseThreadsAndMaxTasksCount` now starts the additional workers on a best-effort basis, so a configuration reload does not fail when the pool is saturated, and reports the count that did start. - `BackgroundSchedulePool`, whose handler used to `LOG_FATAL` and `abort`, turning a misconfiguration into signal 6 and a crash report. It now logs the same hint and rethrows, so the server exits cleanly. The pre-existing `ThreadPool.GlobalFull1` and `ThreadPool.GlobalFull2` tests did not catch this because they give the global pool a queue as small as its thread count, which makes saturation fail on queue fullness instead. `ThreadPool.GlobalFullLargeQueue` covers the shape of the real pool, and the `test_thread_pool_size_too_small` integration test asserts that the server terminates with the error instead of hanging. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [27385f5] Summary: ✅
AI ReviewSummaryThis PR changes Findings❌ Blockers
Final Verdict❌ Changes requested. LLVM Coverage ReportMeasured on commit 27385f5.
Changed lines: Changed C/C++ lines covered: 719/812 (88.55%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Object file sizes140 object files changed (+66.48 KiB total), 0 added.
716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only Compile time of recompiled translation units393 translation units recompiled, 3058 s compile time in total, 393 of them have a recent master baseline. Median compile-time ratio to the baselines is ×1.07 (machine-speed difference or a change affecting every TU); per-TU deltas below are relative to that ratio. |
…tcherOld` constructor Starting `request_thread` or `responses_thread` may now throw `CANNOT_SCHEDULE_TASK` synchronously when the global thread pool has no free slot for a long-running job. Destroying a still joinable `ThreadFromGlobalPool` during the unwind would abort the process, so mark the context as shut down, finish the queues, and join whatever workers were started before rethrowing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ackRunnerFast::operator()` fails to start a worker `operator()` enqueued the callback and incremented `active_tasks` before `startMoreThreadsIfNeeded`, but unlike `bulkSchedule` it did not roll either back when `pool->scheduleOrThrow` throws. The caller saw `CANNOT_SCHEDULE_TASK` while the callback stayed queued and could run later, when another worker eventually starts. Add the same rollback as in `bulkSchedule`, plus a fault-injection unit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…inary The bugfix-validation gate runs touched unit tests on the before-binary and expects them to fail. Without the rollback, the failed schedule leaves the callback in `queue` while `queue_size` is never incremented, so a subsequent schedule strands its callback forever and the test hung at `second_done.get_future().wait()` until the 4-hour job timeout: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112928&sha=42c61b8707873ac1a0949e0261adfae93dc407d6&name_0=PR&name_1=Bugfix%20validation%20%28unit%20tests%29 Guard the second-schedule phase on `isIdle`: if the rollback did not happen, the failure is already recorded and we bail out via `shutdown`, which is safe in that state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r fails to start one `Coordination::Storage::BackgroundWork::BackgroundWork` and `Changelog::Changelog` start multiple long-lived `ThreadFromGlobalPool`s. Now that `ThreadFromGlobalPool` construction can throw `CANNOT_SCHEDULE_TASK` synchronously, a failure on the second or later start would unwind and destroy already-started joinable threads, and `~ThreadFromGlobalPoolImpl` aborts before the real error is reported. `BackgroundWork` reuses `shutdown` in the catch; `Changelog` finishes the worker queues and joins whatever threads were started (its `shutdown` cannot be used: it dereferences not-yet-created thread pointers). Add a fault-injection regression test constructing `Changelog` under `CannotAllocateThreadFaultInjector`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bugfix-validation gate runs the test on the before-binary, where it aborts mid-test by design, leaving `./logs_ctor_unwind` behind. `ChangelogDirTest` expects the directory to not exist, so a subsequent run in the same workspace would fail spuriously. Remove the directory up front instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…atcher constructor unwinds `KeeperRequestDispatcherOld`'s constructor set `keeper_context->setShutdownCalled()` in its catch block to make the already-started worker threads exit before rethrowing. That flag is process-global and `KeeperDispatcher::shutdown` joins its own threads only on the first transition of it, so consuming it during the unwind left `snapshot_thread` (started earlier by `KeeperDispatcher::initialize`) never joined, and the process aborted on its still joinable destructor instead of surfacing `CANNOT_SCHEDULE_TASK`. The worker loops now also exit when their queue is finished, which keeps the constructor's cleanup local: finish the queues, join the started workers, rethrow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the unfixed binary On the binary without the fix, `scheduleOrThrowOnError` silently queued `another_pool`'s worker instead of throwing, and the pool's destructor then joined a "thread" whose function never started - the very hang this test covers - so the Bugfix validation (unit tests) job ran the test on the before-binary and hung for three hours until the 4-hour job timeout: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112928&sha=9feac815b044ee5dedff11339963464a4c05c606&name_0=PR&name_1=Bugfix%20validation%20%28unit%20tests%29 #112928 When the expected exception does not come, release the occupied threads and destroy the pool that holds the global slots first, so the silently queued worker can run and both pools shut down cleanly, then skip the rest of the test. Verified locally against master's `ThreadPool`: the test now fails in milliseconds with a clean process exit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 @groeneai, investigate the failure: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112928&sha=9feac815b044ee5dedff11339963464a4c05c606&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%2C%20s3%29 and provide a fix in a separate PR. If the fix is already in progress, link it here. The hung check flagged a thread stuck in Related: #112203 |
|
I investigated this one. The The quoted frames are not the hung stack. In What actually hung. Processlist Row 1 is SELECT startsWith(dynamicType(data.arr), 'Array(JSON') FROM t_json_nested_pool WHERE id = 5;
It was not deadlocked, it was crawling. The query emitted 20,290,878 log lines between 02:08:58.981 and 02:35:41.605, of which 20,290,743 are The cost driver is that the read is advancing ~51 bytes at a time: 6,757,397 read events totalling 345,926,192 bytes, mean 51.2 bytes, and 1,217,090 points where the Two things worth separating:
On ownership. This is not I would rather not send a fix until I can reproduce the ~51-byte read pattern locally and confirm which of the two multipliers is load-bearing, because the plausible one-line fixes here (making the Separately, this artifact again lost its server-side |
|
Following up on my earlier comment with the promised measurement. It identifies the load-bearing multiplier, and it turned out to be neither of the two candidates I named there. The fix already exists as an open PR of mine, so I am linking it rather than opening a second one. Fixing PR: #112644 ("Do not read-and-discard the archive prefix when reading a packed part member"). What the multiplier isThe part in that run is in Packed storage, so every column file lives inside one Two things then compound:
Measured on the query's own log lines and
Only 1,958 seeks occur, which is why I initially read the seek path as a consequence. That inference was wrong: one The two candidates I flagged earlier, both refuted
On the
|
…rt` unwinds If starting `cleanup_thread` throws `CANNOT_SCHEDULE_TASK` because the global thread pool is full, `createServer` destroys the half-started listener without calling `stop`, and the defaulted destructor aborted on the still-joinable `server_thread`. Call `stop` before rethrowing, following the same partial-start cleanup pattern as the other components in this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…inds If starting `cleanup_thread` throws `CANNOT_SCHEDULE_TASK`, the already started `main_thread` used to keep running with a half-started worker. Reuse `shutdown` (idempotent, joins whichever threads exist) in the catch and rethrow, so the failure propagates fail-closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gs` constructors unwind Two more sites of the same class as the reviewed ones: if the Nth `ThreadFromGlobalPool` start throws `CANNOT_SCHEDULE_TASK`, - the `AsynchronousInsertQueue` constructor destroyed the member vector with joinable threads (`flushAndShutdown` is never called for an object that failed to construct), terminating the process; - the `SystemLogs` constructor called `startCollect` outside its existing try/catch, and the implicit `PeriodicLog` destructor does not join `collecting_thread`, also terminating on unwind. Both now stop and join the already started threads and rethrow, so a too small `max_thread_pool_size` surfaces as a startup error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in `FileCache::initialize` With `load_metadata_asynchronously` enabled, a `CANNOT_SCHEDULE_TASK` failure from `CacheMetadata::startup` was deferred into `init_exception` on the metadata loading thread, so the server came up with a registered but permanently broken filesystem cache. Start the permanent background threads (downloads, delayed cleanup) in the synchronous part of `FileCache::initialize`, before the (possibly asynchronous) metadata loading, so the exception reaches the caller and startup fails close. `CacheMetadata::startup` now also stops and joins partially started download threads when it unwinds. Until the metadata is loaded, the threads only sleep on their empty queues: cache users block in `assertInitialized` on `init_mutex` until the load finishes, and the delayed-cleanup consumer takes the same per-bucket locks as the loader. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he loader thread `LoadingDispatcher::startLoading` started the background load thread and only then inserted it into `loading_threads`. A throwing `try_emplace` (allocation or rehash) would unwind through a joinable `ThreadFromGlobalPool` local, and `~ThreadFromGlobalPoolImpl` aborts the process instead of propagating the exception. Reserve the map entry first and move the started thread into it; the move assignment is `noexcept`.
clickhouse-gh
Bot
added
the
submodule changed
|
🕵 Update:
|
…s after starting the event loop thread `StorageNATS` starts `event_loop_thread` and then creates the `streaming_task` and `initialize_consumers_task` schedule-pool tasks. A destructor does not run for a constructor which throws, so an exception from that tail destroyed a joinable `ThreadFromGlobalPool` member directly and `~ThreadFromGlobalPoolImpl` terminated the process instead of propagating the original exception. The tail is now wrapped in a `try`/`catch` which calls `stopEventLoop` before rethrowing.
…all-thread-pool Merged with an explicit merge base (`b30eb447799f`): this checkout is shallow, so Git found a spurious second merge base and the recursive merge produced bogus conflicts across unrelated files.
|
🕵 The @groeneai, please investigate #115455 and provide a fix in a separate PR; if a fix is already in progress, please link it there. The |
|
It is already linked there: I posted the investigation and the deliverable on #115455 on 2026-08-19 (issuecomment-5345020545), and I answered the same question on #110084 at 02:01 UTC today (issuecomment-5389858292). Nothing has changed since, so rather than posting a third copy, the state in short:
3319 is still firing: 19 runs over 19 distinct commits in 30 days, 3 of them on master, most recently 2026-08-23 22:09 UTC. The one step available without that ruling is to land #113242 first with |
…all-thread-pool # Conflicts: # src/Interpreters/SystemLog.cpp
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
This is good.
alexey-milovidov
removed
the
submodule changed
clickhouse-gh
Bot
added
the
comp-query-execution
| } | ||
| else | ||
| { | ||
| jobs.emplace( |
There was a problem hiding this comment.
scheduleThreadOrThrow only binds the long-lived job to a worker when adding_new_thread is true. In the existing-idle-worker path we still enqueue the job into the shared jobs heap here and only wake an idle worker afterwards.
That leaves the original contract break reachable under concurrent scheduling. Concrete trace: with max_threads = 2, let one ordinary job already be running and one worker be idle. Thread A enters scheduleThreadOrThrow, falls into this branch, queues the long-lived job, and wakes the idle worker. Before that worker reacquires mutex, thread B can scheduleOrThrow a higher-priority ordinary job onto the same pool. The woken worker then takes B from jobs.top() in ThreadFromThreadPool::worker() (src/Common/ThreadPool.cpp:1198-1199 on current head), so A's long-lived job is still left in the queue even though scheduleThreadOrThrow already returned success.
I think the idle-worker path needs the same direct handoff as the fresh-worker path: pick a specific idle worker under mutex, install this job into that worker (or another per-worker reserved slot), and only then wake it.
3 participants
Setting
max_thread_pool_sizeto a value that is too small madeclickhouse-serverhang forever at startup instead of reporting the problem.A
ThreadFromGlobalPoolcreated while all the threads of the global thread pool are already taken by other such jobs was silently put into the pool's queue instead of getting a thread. The queue is much larger than the number of threads by default (thread_pool_queue_sizeis 10000 againstmax_thread_pool_size), so scheduling succeeded, but nothing ever picked the job up, because the threads are held by jobs that never return. Its creator got a seemingly running thread whose function was never called, and any code that then waited for that thread to make progress hung forever.An idle server permanently occupies about 200 threads of the pool for the workers of the background pools and of the background schedule pools, and for the flush threads of the system logs, so a
max_thread_pool_sizebelow that ran out of threads while starting up:max_thread_pool_sizeof 128 the starved job was the loader worker of thesystemdatabase, and startup hung forever right afterWait load job 'startup Atomic database system'.Jobs that occupy their worker for the whole lifetime of that worker are now scheduled through
ThreadPoolImpl::scheduleThreadOrThrow, which hands them one of themax_threadsslots at scheduling time and throwsCANNOT_SCHEDULE_TASKnamingmax_thread_pool_sizewhen there is none left, rather than queueing a thread that can never start.Two components started threads in their constructor and stopped them only in their destructor, so the resulting exception left those threads running while the destructor of the member thread pool blocked forever joining them. Both now stop their workers before letting the exception out, following the pattern
CacheDictionaryUpdateQueuealready uses:MergeTreeBackgroundExecutor- this was the second hang above. ItsincreaseThreadsAndMaxTasksCountnow starts the additional workers on a best-effort basis, so a configuration reload does not fail when the pool is saturated, and it reports the number of workers that actually started.BackgroundSchedulePool- its handler used toLOG_FATALandabort, turning a misconfiguration into signal 6 and a crash report. It now logs the same hint and rethrows, so the server exits cleanly.Note that the hint in that handler ("Please make sure max_thread_pool_size is considerably bigger than background_schedule_pool_size") was previously unreachable, because saturating the global pool never produced
CANNOT_SCHEDULE_TASK.Behaviour change. Values of
max_thread_pool_sizethat cannot hold the permanent threads of the server now make it refuse to start with a clear error naming the setting, where some of them used to start in a silently degraded state. This looks like the intended contract:max_thread_pool_sizeis documented as the maximum number of threads in the pool, and the documentation ofthread_pool_queue_sizealready recommends keeping it equal tomax_thread_pool_size, which makes saturation an error today. The requirement is now documented on the setting itself. No test configuration is affected:tests/casa_del_dolor/properties.pyrandomisesmax_thread_pool_sizeonly down to 700.Tested by starting a server with values of 16, 32, 64, 100, 128 and 160 (all fail with the error, no
abort, no crash report) and 192, 256 and the default (all start normally), plus a check for slot accounting leaks under concurrent load withmax_thread_pool_sizeof 400. The fullunit_tests_dbmssuite passes, includingFunctionsStress.The pre-existing
ThreadPool.GlobalFull1andThreadPool.GlobalFull2tests did not catch this because they give the global pool a queue as small as its thread count, which makes saturation fail on queue fullness instead. The newThreadPool.GlobalFullLargeQueuecovers the shape of the real pool, and thetest_thread_pool_size_too_smallintegration test asserts that the server terminates with the error rather than hanging.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a hang at server startup when
max_thread_pool_sizewas set too small to hold the threads that the server occupies permanently. Such a configuration is now reported with an error naming the setting, instead of hanging forever or starting with some background threads missing.Workflow [PR]
Sync PR [sync-upstream/pr/112928]