Skip to content

Fix flaky 04408_array_fold_cancellation under sanitizers#110942

Open
groeneai wants to merge 2 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-04408-array-fold-cancellation-flaky
Open

Fix flaky 04408_array_fold_cancellation under sanitizers#110942
groeneai wants to merge 2 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-04408-array-fold-cancellation-flaky

Conversation

@groeneai

Copy link
Copy Markdown
Contributor

Related: #110887

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):

...

Description

Fix flakiness of 04408_array_fold_cancellation (surfaced on the Stateless tests (amd_tsan, parallel) check).

CIDB, last 30 days: 60 FAIL vs 36,777 OK (~0.16%), across 15 unrelated PRs, 0 on master. The CI job's own same-settings rerun passes (6/6), so it is a timing flake, not a settings flake. The failing diff is always the same single line, the const throw case:

 TIMEOUT_EXCEEDED
 runtime break: stopped without error
-TIMEOUT_EXCEEDED
+const throw: no timeout
 const break: stopped without error

Root cause. arrayFold folds a whole block inside one executeImpl(); the in-function checkQueryTimeLimit() runs at the top of each fold slice and fires on the first slice where elapsed wall-clock exceeds max_execution_time. But the setup phase before that first check (selector build over all elements plus scatter into per-element vertical slices) is uninterruptible for this input: the selector loop only checks the time every 0xFFFFF elements, and range(120000) over numbers(4) is 480000 elements, so the check never fires during setup. Measured setup-only time on a debug build: range(120000) const path ~0.88s versus the runtime path ~0.36s. The const array is replicated across the four rows, so its setup is ~2.4x heavier and sits right against the 1s max_execution_time. The test asserts by latency wrapped in timeout 30 and greps stdout for TIMEOUT_EXCEEDED; if the outer timeout kills the client before the in-function check is reached, the grep is empty and the line prints const throw: no timeout. Under the sanitizer's slowdown plus parallel contention, the const path's already-heavier setup is the one that crosses that boundary, which is why only that line flakes.

Fix. Shorten the folded array from range(120000) to range(60000) (still over numbers(4)). This cuts the uninterruptible setup roughly in half (~0.88s -> ~0.51s) so the query reaches the interruptible fold quickly, while the O(N^2) arrayPushFront fold still runs ~7.6s, far longer than the 1s limit, so the in-function check keeps firing with a large margin. Peak memory drops (~40 MiB -> <20 MiB), keeping the test comfortably parallel-safe. The four-row shape and the runtime-vs-const distinction are preserved, and the reference output is unchanged.

Validation (debug build): 50/50 runs of the full test match the reference; all four assertion lines behave as before.

The const-throw case of the test flaked (~0.16% on Stateless amd_tsan parallel,
0 on master). arrayFold folds a block in one executeImpl(); the in-function
time check fires at each fold slice, but the setup before it (selector build +
scatter) is uninterruptible for this input, and the const array replicated
across the four rows makes that setup (~0.88s) rival max_execution_time=1s.
When the outer `timeout` wrapper kills the client before the in-function check
is reached, the grep finds no TIMEOUT_EXCEEDED and the line prints
"const throw: no timeout".

Shorten the folded array from range(120000) to range(60000): setup drops to
~0.51s so the query reaches the interruptible fold quickly, while the O(N^2)
arrayPushFront fold still runs ~7.6s (far above the 1s limit). Peak memory
drops to <20 MiB. The four-row shape, the const-vs-runtime distinction, and the
reference output are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Internal second-model review (click to expand)

An independent second model reviewed this diff before the PR was opened. Findings and dispositions:

Nits / other

  • remove-meta-json (major, category: other) - DISAGREE. The reviewer flagged .meta.json as accidentally included. Verified it is NOT in the change set: the commit contains exactly one file (tests/queries/0_stateless/04408_array_fold_cancellation.sh, +12/-8); .meta.json is an untracked fleet-provisioning file (git ls-files --error-unmatch .meta.json -> not tracked, in no commit) surfaced only as diff context. Files are staged explicitly, so it is not pushed. No action needed.

No blockers or majors affecting the change itself.

@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? The root-cause metric is deterministic and reproduced locally (setup-only time, halved by the fix). The end-to-end flake needs the amd_tsan environment (CI's own same-settings rerun passes 6/6, i.e. not reproducible on fast hardware).
b Root cause explained? Yes. arrayFold folds a block in one executeImpl(); the in-function checkQueryTimeLimit() fires per fold slice, but the setup before it (selector build + scatter) is uninterruptible here (selector loop checks time only every 0xFFFFF elements; 480000 < that => zero checks during setup). The const array replicated across 4 rows makes its setup ~0.88s, rivaling max_execution_time=1s. When the outer timeout kills the client before the in-function check, the grep is empty and the line prints "const throw: no timeout" (verified).
c Fix matches root cause? Yes. Shortening the array (120000 -> 60000) halves the uninterruptible setup (~0.88s -> ~0.51s) so the query reaches the interruptible fold quickly, while the O(N^2) fold still runs ~7.6s (>> the 1s limit). Directly reduces the pre-check work.
d Test intent preserved? Yes. Still proves arrayFold respects max_execution_time mid-fold; keeps the 4-row shape and the const-vs-runtime distinction; reference output unchanged; no assertion weakened, no no-random-settings tag added.
e Demonstrated in both directions? Yes. WITHOUT fix: const setup ~0.88s rivals the limit (the flake driver). WITH fix: setup ~0.51s, fold ~7.6s, peak <20 MiB; 50/50 full-test runs match the reference.
f Fix is general, not a narrow patch? N/A (test-only fix, not a source code bug).
g Fix generalizes across inputs? N/A (test-only fix).
h Backward compatible? N/A (no behavior/setting/format change).
i Invariants and contracts preserved? N/A (no source invariants touched).

Session id: cron:clickhouse-author-slot-2:20260718-134700

@groeneai

Copy link
Copy Markdown
Contributor Author

cc @Ergus - could you review this? It de-flakes 04408_array_fold_cancellation by shortening the folded array (range(120000) -> range(60000)) so arrayFold reaches its interruptible fold loop before the uninterruptible setup can rival max_execution_time; test-only, reference output unchanged.

@nikitamikhaylov nikitamikhaylov self-assigned this Jul 18, 2026
@nikitamikhaylov nikitamikhaylov added the can be tested Allows running workflows for external contributors label Jul 18, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [f5f128a]

Summary:

job_name test_name status info comment
Stress test (arm_asan_ubsan) FAIL
Logical error: RWLockImpl::getLock(): Cannot acquire exclusive lock while RWLock is already locked (STID: 2043-3c5c) FAIL cidb

AI Review

Summary

This PR de-flakes 04408_array_fold_cancellation by shortening the folded array so the test reaches the interruptible part of arrayFold under sanitizers, and by making the break-mode cases assert on emitted row count instead of relying only on exit status. I did not find any remaining correctness or regression-test coverage issues in the current head; the earlier concern about the weakened break assertion is fixed by the new ran to completion (4 rows) failure mode.

Final Verdict

Status: ✅ Approve

@clickhouse-gh clickhouse-gh Bot added the pr-ci label Jul 18, 2026
Comment thread tests/queries/0_stateless/04408_array_fold_cancellation.sh
The break-mode cases only asserted the client exit code under an outer
timeout 30. The uncancelled fold at range(60000) finishes in ~7.6s, well
under 30s, so a build that regressed to ignoring checkTimeLimit in break
mode would still exit 0 and print the same "stopped without error" line
(reported by the automated review). Assert the number of result rows
instead: a cancelled break-mode fold emits no rows, a completed one emits
all four. This restores break-mode coverage without lengthening the
workload, so the sanitizer-timing fix (range(60000)) is preserved and the
reference output is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger — f5f128a

CI fully finished (Finish Workflow + Config Workflow pass). Every failure has an owner.

Check / test Reason Owner
Stress test (arm_asan_ubsan) / RWLockImpl::getLock Cannot acquire exclusive lock (STID 2043-3c5c) flaky trunk stress (re-entrant deadlock in CREATE OR REPLACE internal DROP; also hit unrelated PR runs, 0 on master) tracked in #110893; a fix task is investigating at full effort, fixing-PR link to follow here
Mergeable Check / PR derivative (fail only due to the above) resolves when the above is fixed

This PR's diff is test-only (04408_array_fold_cancellation.sh de-flake); it cannot cause an RWLock exclusive-lock error in the stress test. nikitamikhaylov approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants