Pre-PR validation (TASK.md Phase 4 step 9 — adapted for feature work):

a) Reproducible behaviour? Yes. With this build:

• clickhouse-local --chime 1 -q "SELECT sleep(2)" writes exactly one BEL (\x07) byte to stderr (verified hexdump: 00 0000 00 07).
• clickhouse-local -q "SELECT sleep(2)" (no --chime) writes 0 bytes to stderr.
• clickhouse-local --chime 10 -q "SELECT sleep(1)" writes 0 bytes (below threshold).
• clickhouse-local --chime 1 -q "SELECT sleep(2), throwIf(1=1, 'expected error')" writes the BEL byte before the exception text on stderr.

b) Mechanism explained? When a query finishes, ClientBase::processParsedSingleQuery ends with the Elapsed: / Processed rows: reporting block. After this block, we read the configured chime-threshold-seconds (UInt64, default 0). If it is > 0 and progress_indication.elapsedSeconds() >= threshold, we write '\x07' to error_stream (stderr) and flush. progress_indication is reset at the start of every query, so the elapsed time is per-query.

c) Implementation matches the request? Yes. @alexey-milovidov asked for "BEL on query finish in ClientBase (so it works in clickhouse-client and clickhouse-local and even in Embedded client)" — the option is declared in ClientBase::addCommonOptions, which is called from ClientApplicationBase::init (clickhouse-client + clickhouse-local) and ClientEmbedded::init (embedded client). The original issue asked for a configurable threshold with a default of 10 seconds — --chime alone gives exactly that via implicit_value(10).

d) Test added? Yes. tests/queries/0_stateless/04219_client_chime_on_slow_query_92718.sh covers the five interesting cases (chime + slow → BEL, chime + fast → no BEL, no chime + slow → no BEL, chime + slow → error → BEL, same in clickhouse-local). The reference file pins exact expected output.

e) Both directions demonstrated? Yes. The test fails on a binary built without this patch (no --chime flag → Bad arguments error) and passes on the build with it (all five cases match the reference).

f) General, not narrow? Yes — change is in ClientBase (the single common path for all three client modes), not in any one entry point. Threshold is configurable per invocation and does not change any default behaviour: --chime defaults to 0 (disabled), so existing scripts and interactive sessions are unaffected.

cc @alexey-milovidov @azat — could you review this? It adds the --chime N CLI flag in ClientBase, which emits the ASCII BEL control character (\x07) on stderr when a query finishes after running for at least N seconds. Wired through ClientBase::addCommonOptions, so it applies to clickhouse-client, clickhouse-local, and the embedded client per the request in #92718.

Workflow [PR], commit [79c181b]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds a default-on --chime client option that emits BEL on stderr for slow TTY query completions, wires the threshold through ClientBase, documents the option, and adds focused stateless coverage for TTY emission, redirected-stderr suppression, explicit disable, implicit/default values, and the server-error path. I found no new correctness, safety, compatibility, or documentation findings in the current diff; the prior bot-authored inline findings are addressed in the current code.

Missing context / blind spots

• ⚠️ The latest workflow for commit 79c181bd3fc27d06054c4eb104bf021358891a4b was still running during this review, and I did not run 04312_client_chime_on_slow_query_92718 locally. A completed CI run covering the new stateless test would close this runtime-validation gap.

Final Verdict

✅ No blockers or majors found. No new inline review comments were posted.

Fix: suppress BEL when stderr is not a TTY (commit 8cd83de)

CI flagged a PR-caused regression on this branch: 10+ Fast tests started failing with Reason: having stderror and \x07\x07 bytes in their captured stderr. The cause is exactly the trade-off discussed in the default-on review: with the chime enabled by default at 5s, any long-running query writes a BEL into the stderr stream, and clickhouse-test rejects non-empty stderr.

Affected tests (from CIDB): 00156_max_execution_speed_sample_merge, 02703_max_local_write_bandwidth, 03217_primary_index_memory_leak, 02900_buffer_table_alter_race, 03257_async_insert_native_empty_block, 03014_async_with_dedup_part_log_rmt, 03233_async_insert_infile_format, 02072_rdb_recovery_escaped_name, 03006_mv_deduplication_throw_if_async_insert, 03595_set_query_no_eq_set_to_one.

Root cause

ClientBase::processParsedSingleQuery emits '\x07' on error_stream unconditionally when the query exceeds the chime threshold. There is no terminal precondition — so when stderr is redirected to a regular file or pipe (Fast test, integration test, any automation), the BEL bytes contaminate the captured stream.

Fix

Add a stderr_is_a_tty guard at the BEL emission site (member already exists in ClientBase, set in the constructor via isatty(err_fd_)):

if (chime_threshold_seconds > 0
    && stderr_is_a_tty
    && progress_indication.elapsedSeconds() >= ...)
{
    error_stream << '\x07';
    error_stream.flush();
}

A "terminal bell" is only meaningful when there is a terminal — emitting it to a captured stream is by definition useless and only causes harm. The interactive default-on behaviour you requested is preserved: users running clickhouse-client in a real terminal still get the chime by default at 5s.

Test 04219 strengthened

The previous version asserted BEL appears whenever --chime N is set on a slow query. That assertion was actually false in CI (stateless tests redirect stderr — non-TTY) and the test only passed because the BEL bytes were silently accepted as stderr garbage. The new test exercises both halves of the fix:

Cases 7 and 8 run the client under script -qc, which allocates a pseudo-terminal and writes the combined pty output to a file we then grep. That's the only way to verify the positive case from a stateless test.

Verification

• 04219_client_chime_on_slow_query_92718: all 8 cases pass.
• 00156_max_execution_speed_sample_merge: passes (45s query, captured stderr empty).
• 02703_max_local_write_bandwidth: passes.
• Manual clickhouse-client -q 'SELECT sleep(2),sleep(2),sleep(2)' 2>/tmp/err → stderr file is 0 bytes (was contaminated with BEL pre-fix).

Pre-PR Validation Gate

a) Deterministic repro: yes — any query >5s with redirected stderr writes BEL pre-fix.
b) Root cause explained: yes — missing TTY precondition at the emission site.
c) Fix matches root cause: yes — adds the missing precondition; semantically correct for a "terminal bell".
d) Test intent preserved: yes, strengthened — the new test exercises both the TTY-only positive path and the non-TTY suppression path.
e) Both directions demonstrated: yes — affected Fast tests pass; positive TTY case still emits BEL via script -qc.

Status: pushed as a separate commit per the no-amend rule. Existing approvals on prior commits remain visible in the review history; reviewers are welcome to re-approve on the new commit.

@groeneai, investigate the failure: Unit tests (tsan, function_prop_fuzzer) -> FunctionsStress.stress (job) and provide a fix in a separate PR. This is a known flaky test tracked in #104877 and is unrelated to the --chime changes in this PR (which only touch src/Client/ClientBase.cpp and the new 04219_client_chime_on_slow_query_92718 test). The failure surface is gtest_functions_stress reporting 1 threads got stuck while exercising math/SQL functions like roundDown. If a fix is already in progress, link it here.

Confirmed — this is the chronic FunctionsStress.stress "threads got stuck" family, unrelated to the --chime changes here.

The CI hit on commit 7609050917a (2026-05-11 15:26 UTC) is at gtest_functions_stress.cpp:2270 — exactly the May-11 peak shape where the test hit 84 failures across 77 unrelated PRs (asan_ubsan / tsan / msan) in a single day.

Fix status

• @nikitamikhaylov's PR #104694 ("Wire functions stress test to ProcessList for KILL-QUERY-style timeouts") merged 2026-05-15 14:06 UTC. Post-merge the rate dropped from ~84/day to 1–4/day across all sanitizer builds, with the residual shape moving from line 2270 → 2274 → 2417. The roundDown / math-function shape called out in the directive is the original May-11 surface (line 2270/2274), which Wire functions stress test to ProcessList for KILL-QUERY-style timeouts #104694 substantially addressed.
• Sibling broken-determinism shapes (caseWithExpression, nullIf(String, FixedString), isDistinctFrom, if(materialize(...)), etc.) under the same FunctionsStress.stress #104877 umbrella are covered by PR #104858 (@Algunenano, OPEN — stringVectorConstantFixedString in FunctionsComparison.h) and PR #104804 (us, OPEN — transform/caseWithExpression wrapper-layer castColumn fix).

The residual line-2417 hangs at ~1 hit/day across all builds in the last 48h are still under investigation — they look like a smaller leftover not addressed by #104694. I'm tracking it in the existing #104877 umbrella; no separate fix PR yet for the post-#104694 residual.

I rebased PR #104545 onto current master (9004589d3c) at 12:36 UTC, so the new CI run should reflect the post-#104694 fix state. Will monitor and re-ping if anything else surfaces.

This was fixed by #105146. Let's update the branch.

@groeneai, many tests have failed, and we have no other way than investigating and fixing all of them.

Updated the branch (1b5f31441d7..aa4ac83a746):

• Merged master (branch was 277 commits behind and red) — zero conflicts. No changes to ClientBase.cpp; the option already matches the requested defaults (implicit_value(5)->default_value(5)).
• Renumbered the stateless test 04303_client_chime_on_slow_query_92718 → 04312_..., because master has since added several 04303_* and 04304_* tests, recreating a numeric-prefix collision.
• Addressed the open review thread on the pty matrix:
  • Case 8 (--chime 0 over a pty) now runs a ~6s query (above the default 5s threshold) instead of 1.5s, so it genuinely distinguishes explicit disable from default-on.
  • Added case 12 for --chime passed without a value (the implicit_value(5) path, distinct from omitting the flag in case 10), which must emit BEL.

Verified on a freshly built binary (clang, ARM): the full test passes all 12 cases, and the two new/changed assertions were confirmed independently — --chime (implicit) on a ~6s query over a pty emits BEL, --chime 0 on the same query emits no BEL, and the default-on path (no flag) emits BEL.

Both previously-open review threads are now resolved.

The remaining CI failures (Server died, FunctionsStress #104877, BuzzHouse stack-use-after-scope STID 2518-4199, and the MutateTask splitAndModifyMutationCommands LOGICAL_ERROR STID 5052-4746 #91015) are the chronic, master-wide flakes already triaged in this thread — none of them touch the client-side BEL emission path.

CI Status — covered SHA 395d0b98

CI fully finished. All failures are pre-existing chronic flakies, not PR-caused.

Code change is purely client-side (--chime option for ASCII BEL on slow-query finish); neither failing area touches client argument parsing or output formatting.

— cron:our-pr-ci-monitor:20260604-073000

@groeneai, update the branch to see if the upgrade check has been fixed (we had a plenty of fixes recently).

@groeneai please look at the comment above ^

@alexey-milovidov merged master into the branch (commit d92a147, fast-forward push from 395d0b9). 1132 commits caught up, no conflicts. CI is now running on the new HEAD; will report back once it finishes whether the prior chronic Upgrade check / test_circular_dependencies_survive_restart flakies are gone.

CI Status — covered SHA d92a147f5c1

CI fully finished after the master merge. The single failure is a pre-existing chronic flake on the TPC-DS test infra, not PR-caused.

PR is client-only — none of the failing code paths can be reached by this diff.

Session: cron:our-pr-ci-monitor:20260608-083000

@groeneai, either fix the transient OOM in 04033_tpc_ds_q61 or disable this test, and send a separate PR. Tag @rienath and @rschu1ze in the fix.

@alexey-milovidov sent a separate PR per your directive: #106797. Disables 04033_tpc_ds_q61 mirroring the existing 04033_tpc_ds_q35 SKIP pattern (host-level OvercommitTracker arbitration on arm_asan_ubsan/azure/parallel is not reachable from any per-query setting). cc'd @rienath and @rschu1ze on the new PR.

Follow-up on the q61 OOM directive: I closed the separate PR (#106797) instead of disabling. rienath reviewed it and asked to find the regression rather than skip the test.

Result: there is no q61 memory regression. q61 has 0 master failures across CIDB's full 90-day retention; its only memory-typed failure ever is the single 2026-06-08 OvercommitTracker OOM on this PR's run, which passed all 3 reruns. It was one casualty of a runner-wide memory event on Stateless tests (arm_asan_ubsan, azure, parallel): MEMORY_LIMIT_EXCEEDED hit 14 tests/10 PRs on 06-08, 311/29 on 06-09, 295/57 on 06-10, with ~24 sibling TPC-DS queries OOMing alongside q61. Host-level RSS contention (current RSS 53.70 GiB, maximum 53.69 GiB), not q61's own ~5 GiB.

So neither fix nor disable applies; q61 stays enabled. For this PR's shard flake, a rerun clears it. Full details on #106797.

@groeneai, the root cause of the failure of 04033_tpc_ds_q61 test was fixed. Let's update the branch so we will be able to merge this PR.

@alexey-milovidov done. Merged latest master into the branch (clean, no conflicts); new HEAD is eda342cee6f805eff96060d34e89e9ab09b44f3f. Net diff vs master is unchanged (the 4 --chime files only). CI is now running on the updated HEAD; I will confirm once it finishes whether the 04033_tpc_ds_q61 failure is gone.

CI summary for eda342cee6f805eff96060d34e89e9ab09b44f3f (master-merge to pick up the q61 fix)

CI has fully finished (Finish Workflow: pass, 0 checks pending).

• 04033_tpc_ds_q61 — the transient OOM that was the prior blocker is now gone on this HEAD: the test runs OK across all stateless jobs (amd_debug, amd_tsan, amd/arm asan_ubsan, llvm_coverage, arm_binary, msan, etc.). The master-side root-cause fix referenced above is picked up by this merge.
• Sole red: Integration tests (arm_binary, distributed plan, 1/4) / test_refreshable_mv_keeper_loss::test_keeper_session_loss_during_coordinated_refresh — a known flaky fault-injection test (keeper-session-loss on a coordinated refreshable MV), unrelated to this PR's --chime client feature. It fails across 8 distinct unrelated PRs in 30 days with 0 master hits. A dedicated test-only fix is already in flight (PR Fix flaky test_keeper_session_loss_during_coordinated_refresh #107413).
• Mergeable Check / PR — aggregators reflecting only the flaky integration row above.

No PR-caused failures; the --chime feature diff is unchanged. The branch is up to date with master and ready to merge.

@groeneai, as the test for refreshable materialized views was fixed, update the branch.

@alexey-milovidov branch updated to 79c181bd3fc27d06054c4eb104bf021358891a4b, now includes the merged #107413 fix for test_keeper_session_loss_during_coordinated_refresh. Clean merge, net diff vs master unchanged (the 4 chime files only). CI running.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 19/19 (100.00%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

CI finished green on 79c181b (master re-merged for the now-merged refreshable-MV keeper-test fix). All jobs pass; no PR-caused failures. The only CIDB red is a within-job Stress test (amd_debug) log-scan pseudo-failure ("Server unresponsive: memory limit exceeded"), a known chronic infra signature unrelated to this client-side --chime change.