Fix hang in SYSTEM SYNC MERGES with the Manual merge selector

[alexey-milovidov]

Fixes a hang in SYSTEM SYNC MERGES that made the stateless test 04207_schedule_merge_basic flaky on master — it timed out at the 600s wrapper limit under parallel TSan/debug/ARM runs (the test always hit exactly 600000 ms, i.e. a permanent hang, not slowness).

ManualMergeSelector::select (the Manual merge selector backing SYSTEM SCHEDULE MERGE / SYSTEM SYNC MERGES) removed a scheduled merge from its queue as a side effect of selection, but selecting a merge does not guarantee it runs. When the background merge pool queue is full, BackgroundJobsAssignee::scheduleMergeMutateTask (trySchedule) returns false and the selected task is dropped — while the queue entry is already gone. The dependent chain of merges then stalls and SYSTEM SYNC MERGES waits until max_execution_time (which defaults to unlimited).

The selector is now self-healing: it removes a queue entry only once that merge's result is observed among the available parts. Parts of an in-flight merge are excluded from parts_ranges (the parts collector drops parts that cannot be used in merges), so a running merge is never re-selected, while a merge that failed to be scheduled or to execute is selected again on the next call. This also removes the queue corruption from the replicated getPartitionsThatMayBeMerged path, which calls select only to build a hint and discards the result.

A deterministic regression test (04492_schedule_merge_retry_on_busy_pool) uses a new ONCE failpoint mt_skip_scheduling_merge_once (applied only to the Manual selector) to drop a manually scheduled merge once, simulating a full pool, and checks that the merge still completes.

Related: #104883
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=543037669696523bda6c85927bbcf5bb4c6b0fbf&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%2C%202%2F2%29

(#104883 reports the same SYSTEM SYNC MERGES hang symptom but a different cause — scheduled parts that never exist or are dropped — which this PR does not address.)

Changelog category (leave one):

• CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a hang in SYSTEM SYNC MERGES with the Manual merge selector when a scheduled merge could not be placed into a full background pool.

🤖 Generated with Claude Code

Version info

• Merged into: 26.7.1.808 (included in 26.7 and later)

[clickhouse-gh]

Workflow [PR], commit [89322b1]

Summary: ✅

• Performance Comparison: Performance dashboard

---

AI Review

Summary

This PR fixes one lost-manual-merge hang path by keeping scheduled entries in ManualMergeSelector until completion, but the new queue bookkeeping also serializes all queued manual merges behind the in-flight head. I would not approve it as-is: the current select loop regresses SYSTEM SCHEDULE MERGE / SYSTEM SYNC MERGES for independent queued merges, and the PR metadata classifies a user-visible server fix as CI Fix or Improvement.

PR Metadata

• Changelog category is not semantically correct. This patch changes user-visible SYSTEM SYNC MERGES behavior in server code, so it is not a CI-only fix. Replace the selected category with Improvement. The current changelog entry is specific enough once the category is corrected.

Findings

⚠️ Majors

• [src/Storages/MergeTree/Compaction/MergeSelectors/ManualMergeSelector.cpp:125-132] Leaving the running head inside info->queue makes the manual queue strictly serial. Every current caller passes a single merge constraint, so once the head merge is scheduled and its source parts disappear from parts_ranges, lookupRange(parts_ranges, info->queue[0]) returns std::nullopt and the break stops the scan before later independent queued merges are even considered. That regresses SYSTEM SCHEDULE MERGE / SYSTEM SYNC MERGES from “independent scheduled merges can overlap” to “everything waits for the head to finish”, and it also starves the replicated getPartitionsThatMayBeMerged hint path behind the in-flight head.
  Suggested fix: keep “pending” and “running but not yet retired” entries separate, or otherwise skip over an in-flight head and continue scanning later queued merges instead of breaking on the first nullopt.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions:

• Restore the ability to select later independent manual merges while the head merge is in flight, including the replicated hint path.
• Change the PR metadata category from CI Fix or Improvement to Improvement.

[alexey-milovidov]

Merged the latest master into the branch (it was ~731 commits / 3 days behind and red, so it needed refreshing). The merge was clean (no conflicts; Common/FailPoint.cpp auto-merged), the net diff against master is unchanged (5 files, +89/-5), and all three changed translation units (FailPoint.cpp, ManualMergeSelector.cpp, StorageMergeTree.cpp) pass -fsyntax-only against the updated master. Test number 04492 remains unique on master (highest there is 04488), so no renumbering was needed.

The regression test 04492_schedule_merge_retry_on_busy_pool passed across every configuration in CI, including the 25-run flaky checks on ASan/UBSan, Debug, MSan, and TSan — it is reliably deterministic.

The only two red checks are Stress test (amd_tsan) and Stress test (arm_tsan) failing on Hung check failed, possible deadlock found. This is the chronic, master-wide flake #107941 (33–182 occurrences/day across dozens of distinct PRs over the last week, including base-master runs), already self-linked by the CI report. It is unrelated to this change: the ManualMergeSelector::select change is a no-op unless a merge was scheduled via SYSTEM SCHEDULE MERGE (the Manual selector), and the new failpoint only fires when explicitly enabled by the test.

The PR is MERGEABLE; the AI review verdict is ✅ Approve and there are no unresolved review threads. It is BLOCKED only on a human approval.

[alexey-milovidov]

Re-triaged CI for commit bb760616 — two new red checks appeared since the previous status update (the run finished at 06:57Z, after the earlier comment). All five failures are unrelated, pre-existing flakes, and none touch the changed code paths: ManualMergeSelector::select and the new scheduleDataProcessingJob failpoint are both gated on merge_selector_algorithm == MANUAL, and the failpoint mt_skip_scheduling_merge_once only fires when the regression test enables it.

1. Stress test (amd_tsan / arm_tsan / arm_asan_ubsan, s3) — Hung check failed, possible deadlock found. I pulled the full last-32 KiB stuck-thread dumps for all three configs: every one is an AST-fuzzer query-analysis stall (18× InterpreterSelectQueryAnalyzer, QueryAnalyzer::resolveQuery, executeASTFuzzerQueries, TCPHandler::runImpl). There are zero frames in ManualMergeSelector, StorageMergeTree::scheduleDataProcessingJob, merge scheduling, or SYSTEM SYNC MERGES. This is the chronic, master-wide flake Hung check failed, possible deadlock found #107941 — 313 failures across 132 distinct PRs today alone (including base-master runs); the CI report self-links it.

2. Integration tests (arm_binary, distributed plan, 1/4) — test_implicit_index_upgrade/test.py::test_implicit_index_upgrade_alter_replay (assert '1' == '10001'). The test creates a second replica node2 and checks its row count right after wait_for_active_replica, which only waits for is_readonly = 0 and not for the freshly-joined replica to finish fetching parts — so node2 can observe only the 1-row VALUES part before the 10000-row part arrives. It is a fetch race in the test, independent of this change (it has flaked once each on 5 unrelated PRs — Allow EXISTS on a dictionary with the SHOW DICTIONARIES privilege #108084, Add arrayCombinations, arrayPermutations, arrayPartialPermutations functions #99280, Use LLVM-libc memory functions instead of glibc ones #107566, Fix recursive MSan report in HandledSignals::reset on shutdown #107442 — over the last 30 days). Hardening fix (adds SYSTEM SYNC REPLICA): Fix flaky test_implicit_index_upgrade_alter_replay (replica fetch race) #108961.

3. Stateless tests (arm_binary, parallel) — 03360_bool_remote (UNEXPECTED_PACKET_FROM_SERVER on remote('127.0.0.{1,2}', system.one)). All 213 reruns passed; transient connection flake (11 distinct PRs hit it today).

The PR is MERGEABLE; the AI review verdict is ✅ Approve, there are no unresolved review threads, changed-line coverage is 97.5%, and the regression test 04492_schedule_merge_retry_on_busy_pool passes in every configuration (including the 25-run flaky checks on ASan/UBSan, Debug, MSan, and TSan).

I did not re-merge master: the merge-base is only ~6 h old and every red is a master-wide flake that re-merging cannot clear. Blocked only on a human approval.

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108770&sha=latest&name_0=PR

[alexey-milovidov]

CI for bb760616 finished after the previous triage. The only red is Mergeable Check, driven purely by Stress test (amd_tsan) and Stress test (arm_tsan) failing on Hung check failed, possible deadlock found — the chronic, master-wide flake #107941. The stuck-thread dumps for both are AST-fuzzer query-analysis stalls (QueryAnalyzer::resolveQuery / executeASTFuzzerQueries / InterpreterSelectQueryAnalyzer / TCPHandler::runImpl) with zero frames in ManualMergeSelector, StorageMergeTree::scheduleDataProcessingJob, merge scheduling, or SYSTEM SYNC MERGES — unrelated to this change (all other 8 Stress variants, including amd_asan_ubsan/msan/debug, are green on this commit).

I reran only those two failed jobs to try to clear Mergeable Check.

I did not re-merge master: the merge-base is ~1.25 days old (< 1 week), a dry-run merge is clean (the only overlap is StorageMergeTree.cpp, which master touched elsewhere while this PR's change is an isolated failpoint hook), and re-merging cannot clear a master-wide flake. CH Inc sync is ✅, the AI review verdict is ✅ Approve, and there are no unresolved review threads — so this is BLOCKED only on the Stress rerun and a human approval.

[alexey-milovidov]

Re-triaged CI for bb760616 after the previous rerun finished. The two red Stress checks are still unrelated, but the amd_tsan symptom changed since my 06:37Z status (which described both as Hung check), so an update:

• Stress test (arm_tsan) — Hung check failed, possible deadlock found. Same chronic flake as before: #107941. The stuck-thread dump is a pure AST-fuzzer stall (executeASTFuzzerQueries → TCPHandler::runImpl), zero merge / ManualMergeSelector / scheduleDataProcessingJob frames. Report.

• Stress test (amd_tsan) — a fuzzer-found logical error Cannot add simple transform to empty Pipe. (STID 2635-3a9e). This is a pre-existing latent bug on the read path (ReadFromMergeTree::readByLayers), not caused by this PR:

  • It has fired on real master (pull_request_number = 0) — two serverfuzz Stress runs on 2026-05-15 (amd_tsan + arm_debug).
  • In the last two weeks it also fired on unrelated PRs Support query parameters in the SETTINGS clause and SET queries #108760 (query parameters in SETTINGS, no read-path changes) and Fix query condition cache ineffective in self-joins #108733, across arm_msan / arm_debug.
  • This PR touches only merge selection (ManualMergeSelector, plus a failpoint doubly gated on merge_selector_algorithm == MANUAL and an off-by-default ONCE failpoint) — it never touches ReadFromMergeTree or the query pipeline, so it cannot introduce a read-path pipeline error. There is no tracking issue for this one yet.
  • Report.

Both are non-deterministic fuzzer outcomes on the TSan Stress job, so I re-ran the failed jobs (attempt 6) to try for green. Mergeable Check and the aggregate PR check are red purely because of these two. AI Review is still ✅ Approve and there are no unresolved review threads.

[alexey-milovidov]

Re-triaged CI for bb760616 (head unchanged since the previous triage — no new commits, reviews, or unresolved threads). The only reds are Stress test (amd_tsan) / Stress test (arm_tsan) and the two aggregate gates (Mergeable Check, PR) they drive. Both are confirmed unrelated flakes (verified independently against master history):

• Stress test (arm_tsan) — Hung check failed, possible deadlock found: chronic master-wide flake Hung check failed, possible deadlock found #107941 (53 master-only occurrences on 2026-06-30, 51 on 2026-06-29).
• Stress test (amd_tsan) — Logical error: Cannot add simple transform to empty Pipe. (STID 2635-3a9e): pre-existing latent read-path issue in ReadFromMergeTree::readByLayers; it has fired on real master (pull_request_number = 0, 2026-05-15) and on unrelated PRs Reimplement reading in order for parallel replicas #101434, Fix query condition cache ineffective in self-joins #108733, Support query parameters in the SETTINGS clause and SET queries #108760. This PR only touches merge selection (ManualMergeSelector plus an ONCE failpoint gated on merge_selector_algorithm == MANUAL), never the read path.

Both are non-deterministic TSan Stress outcomes, so I re-ran the two failed jobs (attempt 7) to try to clear Mergeable Check. Not re-merging master: the merge-base is ~1.7 days old (< 1 week) and re-merging cannot clear a master-wide flake. CH Inc sync is ✅, the AI review verdict is ✅ Approve, and there are 0 unresolved review threads — so this is BLOCKED only on a human approval.

[alexey-milovidov]

Merged current master into the branch (clean merge, no conflicts; net diff unchanged — 5 files, +89/-5).

The two red checks — Stress test (amd_tsan) and Stress test (arm_tsan), both Hung check failed, possible deadlock found — were not merge-related. I pulled the hung_check.log for both from the latest rerun of bb760616: the single hung query in each is 04412_deep_nested_literal_format.sh's SELECT formatQuery($$SELECT [[[…]]]$$), stuck for ~1520 s in quadratic DB::Field::dispatch<DB::Field::create> recursion while copying the depth-20000 literal Field. Zero frames in ManualMergeSelector, scheduleDataProcessingJob, merge scheduling, or SYSTEM SYNC MERGES.

That is exactly the hang fixed by 4e1006cb08a ("Parse nested literal collections in linear time"), which lands layers.back().arr.push_back(std::move(literal->value)) in ParserCollectionOfLiterals::parseImpl. As its own message notes, the stress job ignores no-{build} tags and runs 04412/04413 under TSan, where the quadratic parse trips the hung check. That commit landed on master at 06:52Z on 2026-06-30, ~6 h after this branch's previous merge base (00:38Z), so it was missing here — every stress rerun on bb760616 re-hit the hang. Merging master brings it in.

The merge also picks up #105132 (STID 2635 Cannot add simple transform to empty Pipe in ReadFromMergeTree::readByLayers), the other symptom seen earlier on amd_tsan.

No unresolved review threads; CH Inc sync and the AI-review verdict were already green.

[clickhouse-gh]

This still drops a scheduled merge as soon as the first source part is covered by any descendant part. ActiveDataPartSet::getContainingPart treats a mutation child as containing the original part, so after a failed trySchedule a mutation or TTL rewrite of only front.front() will hit this branch, pop_front() the manual queue entry, and leave the remaining source parts pending with no way to retry the merge. SYSTEM SYNC MERGES will then keep waiting for the uncovered siblings until max_execution_time.

Can we retire the queue entry only once all names in front resolve to the same covering active part/result instead of checking only front.front()?

[alexey-milovidov]

Re-ran CI on the freshly re-merged head 6a579220. Three things to triage — the AI review verdict and two new red checks — none of which need a code change here.

AI review (Request changes) — ManualMergeSelector.cpp:117

The observation is accurate: ActiveDataPartSet::getContainingPart treats a single-part mutation/TTL rewrite as "containing" the original part (MergeTreePartInfo::contains allows mutation >= rhs.mutation for an equal block range), so if only front.front() is independently rewritten while a scheduled merge is pending, the completion loop pops the queue entry.

The suggested fix — "retire only when all source parts are covered by the same descendant/result part" — does not fix the hang it describes, and would regress:

• The retirement loop (info->queue) and isAllScheduledPartsCovered (info->scheduled_part_infos, which is what SYSTEM SYNC MERGES actually polls in InterpreterSystemQuery::syncMerges) are independent. Whether or not the queue entry is popped has no effect on what syncMerges waits for, so changing the retirement condition cannot resolve the described hang.
• Once a source part is rewritten to a new name (all_5_5_0 → all_5_5_0_7), the scheduled merge [all_5_5_0, ...] is unsatisfiable: lookupRange matches by exact part name, so the merge can never be re-selected regardless of whether the queue entry survives. The hang comes from isAllScheduledPartsCovered still waiting for the untouched siblings, which no merge can now cover — an inherent consequence of mutating the inputs of a specifically-scheduled merge, orthogonal to the full-pool bug this PR fixes.
• "All source parts covered by the same part" would additionally leave that now-dead entry at the head of the manual queue (it can never satisfy lookupRange), blocking every subsequent scheduled merge behind it. The current front-part check retires the dead entry and keeps the queue flowing.

So I am not adopting the suggestion. If we want the Manual selector to be robust against concurrent per-part mutation/TTL of a scheduled range, the right place is the SYSTEM SYNC MERGES completion semantics (abandon a scheduled merge once one of its source parts is covered such that the merge becomes unsatisfiable, so SYNC MERGES returns instead of waiting for un-mergeable siblings) — a separate follow-up, not a change to the retirement loop.

Fast test (arm_darwin) — 04319_skip_unavailable_shards_mode_table_missing

A master test (added 2026-06-12) pulled in by the re-merge; it exercises a distributed INSERT with skip_unavailable_shards_mode and is unrelated to merge selection. It is already being skipped on macOS by #109254.

Upgrade check (amd_release)

<Error> messages from DatabasePostgreSQL::getTablesIterator retrying a connection to the reserved unreachable address 192.0.2.1:5432 — a leftover PostgreSQL database engine from an unrelated stateless test, not merge-related.

The net PR diff is unchanged by the re-merge (5 files, +89/-5).

[alexey-milovidov]

Re-merged current master into the branch (clean merge, no conflicts; net PR diff unchanged — 5 files, +89/-5). New head 312780b330b.

Why now: the sole remaining red was Fast test (arm_darwin) failing 04319_skip_unavailable_shards_mode_table_missing — a darwin-only distributed-query flake, unrelated to the merge-selector change (this PR touches only FailPoint.cpp, ManualMergeSelector.cpp, StorageMergeTree.cpp, and the 04492 test). #109254 ("Skip 04319_skip_unavailable_shards_mode_table_missing on macOS fast test") merged into master at 10:58Z and adds that test to ci/defs/darwin.skip. The branch did not contain it, so re-merging pulls in the skip and clears that red on the fresh CI run.

This does not touch the open AI review thread on ManualMergeSelector.cpp:117 (the suggested "retire only when all source parts are covered by the same descendant" change is orthogonal to and, as analyzed earlier, counterproductive for the pool-full bug this PR fixes — it belongs to a separate SYSTEM SYNC MERGES completion-semantics follow-up); that remains an editorial call.

[alexey-milovidov]

The fresh AI review on the re-merged head 312780b330b added a second finding, on ManualMergeSelector.cpp:131. (The first, :117, is dismissed.) This one describes a genuine behavior change and is kept open pending a design decision — no speculative fix pushed.

Confirmed. The non-replicated path passes a single merge constraint (StorageMergeTree.cpp:1355), so the select loop only ever examines info->queue[0]. While the head merge is in-flight its source parts are dropped from parts_ranges, so lookupRange(parts_ranges, info->queue[0]) returns std::nullopt, the break returns an empty result, and no later queued merge is selected until the head is retired. Before this PR the head entry was popped on selection, so an independent SYSTEM SCHEDULE MERGE could overlap a running head; with the new bookkeeping the manual queue is strictly serial (and the replicated getPartitionsThatMayBeMerged hint path waits behind the in-flight head too).

Design decision needed. The fix is a choice — e.g. skip an in-flight head and keep scanning later independent entries, or track "pending" and "running-but-not-yet-retired" entries separately. I did not push a queue-restructuring change here without deciding the intended behavior; this belongs to the same SYSTEM SYNC MERGES completion-semantics follow-up as the (dismissed) :117 point rather than to the pool-full hang this PR fixes.

CI is still running on this head (CH Inc sync and the PR rollup pending).

[alexey-milovidov]

Merged current master into the branch. This time master had a textual conflict in src/Storages/StorageMergeTree.cpp: both this branch (adding the mt_skip_scheduling_merge_once failpoint declaration) and master (adding the mt_alter_throw_in_start_mutation / mt_alter_throw_after_mutation_registered / mt_throw_after_mutation_commit / mt_alter_throw_in_durable_rollback mutation failpoints) inserted extern const char ...[]; lines at the same spot in the FailPoints namespace. Resolved by keeping both sets — they are independent declarations. The failpoint use site in scheduleDataProcessingJob auto-merged.

Net PR diff is unchanged — 5 files, +89/-5. The resolution does not touch ManualMergeSelector.cpp or the two open review threads on :117 (dismissed) and :131. New head 1203d5c00d5.

[alexey-milovidov]

Merged current master into the branch (clean merge, no conflicts; net PR diff unchanged — 5 files, +89/-5). New head 89322b15f4c.

Why now: the sole real red on 1203d5c00d5 was the required check Stateless tests (arm_binary, parallel) failing on the 04402_map_functions_lowcardinality map-bucket-randomization flake, which kept Mergeable Check and PR red. #108677, which fixes that test, merged into master on 2026-07-09 — this re-merge pulls the fix in so the rerun can clear the required check.

Merge verification: ManualMergeSelector.cpp is untouched by master since the merge-base; master's additions to Common/FailPoint.cpp and the FailPoints namespace of StorageMergeTree.cpp auto-merged next to this branch's mt_skip_scheduling_merge_once declaration; all symbols the new code uses (ActiveDataPartSet::getContainingPart, MergeTreePartInfo::fromPartName, getPartNameV1, MergeTreeSettingsMergeSelectorAlgorithm, MergeSelectorAlgorithm::MANUAL) are unchanged on current master. The 04492 test number is now shared with several master tests, but the numbering gate is set-based and full test names remain unique, so no renumbering.

The two review threads on ManualMergeSelector.cpp are intentionally untouched: :117 stays dismissed, and :131 (strict serialization of the manual queue) remains open pending the design decision.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.80%	85.80%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	78.00%	78.00%	+0.00%

Changed lines: Changed C/C++ lines covered: 39/40 (97.50%) · Uncovered code

Full report · Diff report

[alexey-milovidov]

Ok.