iframe-proxy

groeneai · 2026-06-07T07:27:13Z

BackupDataFileNameGeneratorTest.ThrowsOnZeroChecksum deliberately throws DB::Exception(LOGICAL_ERROR, ...) from getBackupDataFileName to validate the zero-checksum guard. On non-debug, non-sanitizer builds (e.g. Unit tests (amd_llvm_coverage)) the test passes, but Exception::~Exception then logs the exception with a stack trace via ForcedCriticalErrorsLogger because LOGICAL_ERROR is "important" unless markAsLogged was called. The trace ends up on the gtest binary's stderr, after the test's [ OK ] line.

When an unrelated test in the same unit_tests_dbms binary later hangs and the gdb -batch ... -ex run runner times out and kills the binary mid-suite, gtest.json is never written. Praktika's from_gtest parser falls back to log-scraping, which then surfaces the stray Backup checksum should not be zero (test.bin). (LOGICAL_ERROR) trace as a fake -FunctionsStress failure in CIDB. 16 such hits across Unit tests (amd_llvm_coverage) on PRs #100177, #105784, #96100 in the last 60 days — all attributable to the underlying SchedulerWorkloadResourceManager / FunctionsStress chronic hang family, not the backup test.

Replace EXPECT_THROW(..., Exception) with an equivalent try block that calls markAsLogged on the caught exception, mirroring the pattern from commit 64a4caa39242 ("Mark fuzzer exceptions as logged to prevent ForcedCriticalErrorsLogger") in src/Interpreters/executeQuery.cpp. Verified locally with a release-mode unit_tests_dbms (-O3 -DNDEBUG, ENABLE_TESTS=1, no sanitizer): the pre-fix binary prints a 17-frame stack trace to stderr on this test, while the post-fix binary runs the test silently with [ OK ] and no trace.

CI report containing the trace: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100177&sha=6e1a7cdc1170bb75bbcc76d5c46ba23c6cc38855&name_0=PR&name_1=Unit%20tests%20%28amd_llvm_coverage%29

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

...

Version info

Merged into: 26.7.1.406

…rowsOnZeroChecksum The test deliberately exercises the zero-checksum guard in `getBackupDataFileName`, which throws `DB::Exception(LOGICAL_ERROR, ...)`. On non-debug, non-sanitizer builds (e.g. `Unit tests (amd_llvm_coverage)`) the test passes, but `Exception::~Exception()` then logs the exception through `ForcedCriticalErrorsLogger` because `LOGICAL_ERROR` is treated as an "important" error code unless `markAsLogged` has been called. The resulting `Code: 49. DB::Exception: Backup checksum should not be zero (test.bin). (LOGICAL_ERROR)` line plus its full stack trace ends up on the gtest binary's stderr after the test's `[ OK ]` line. When an unrelated test in the same `unit_tests_dbms` binary later hangs (e.g. `SchedulerWorkloadResourceManager.DropNotEmptyQueueLong` per the report linked below), the wrapping `gdb -batch ... -ex run` runner times out, kills the binary mid-suite, and `gtest.json` is never written. The Praktika `from_gtest` parser then has nothing to parse and falls back to log-scraping, which surfaces the leaked `Backup checksum should not be zero` stack trace as a fake `-FunctionsStress` failure in CIDB. Sixteen hits across `Unit tests (amd_llvm_coverage)` on PRs ClickHouse#100177, ClickHouse#105784, ClickHouse#96100 over the past 60 days follow this pattern; the underlying contributor is the chronic `SchedulerWorkloadResourceManager` / `FunctionsStress` family hang, not the backup test. Replace `EXPECT_THROW(..., Exception)` with an equivalent `try` block that calls `markAsLogged` on the caught exception, mirroring the pattern already used in `executeQuery.cpp` (commit 64a4caa "Mark fuzzer exceptions as logged to prevent ForcedCriticalErrorsLogger") to suppress the stray stack trace. Verified locally: built `unit_tests_dbms` in release mode (`-O3 -DNDEBUG`, `ENABLE_TESTS=1`, no sanitizer) and confirmed the pre-fix binary prints a 17-frame stack trace to stderr on this test, while the post-fix binary runs the test silently with `[ OK ]` and no trace. Related: ClickHouse#100177 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-07T07:27:56Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. `build-release/src/unit_tests_dbms --gtest_filter="BackupDataFileNameGeneratorTest.ThrowsOnZeroChecksum"` deterministically prints a 17-frame `Code: 49. DB::Exception: Backup checksum should not be zero (test.bin). (LOGICAL_ERROR)` stack trace to stderr after the `[ OK ]` line on every run with the pre-fix source. Verified on a local release-mode `unit_tests_dbms` (`-O3 -DNDEBUG`, `ENABLE_TESTS=1`, no sanitizer) built from `c9ca293ac038`.
b	Root cause explained?	Yes. The test catches the `LOGICAL_ERROR` exception via `EXPECT_THROW`, but doesn't call `markAsLogged` on it. When the caught exception goes out of scope, `Exception::~Exception` runs and, because `logged == false` and `isErrorCodeImportant() == true` for `LOGICAL_ERROR`, fires `LOG_ERROR(getLogger("ForcedCriticalErrorsLogger"), "{}", getExceptionMessage(*this, true))` — emitting the full message + stack trace to the gtest binary's stderr. When an unrelated test in the same `unit_tests_dbms` binary later hangs (`SchedulerWorkloadResourceManager.DropNotEmptyQueueLong` is the documented case for PR #100177's runs) and the wrapping `gdb -batch ... -ex run` runner times out and kills the binary, `gtest.json` is never written. Praktika's `from_gtest` parser falls back to log-scraping and surfaces the stray backup-test trace as a fake `-FunctionsStress` failure in CIDB.
c	Fix matches root cause?	Yes. Replace `EXPECT_THROW(..., Exception)` with an equivalent `try { ... ; FAIL() } catch (Exception & e) { e.markAsLogged(); }` that calls `markAsLogged` on the caught exception. `markAsLogged` sets `logged = true`, which `~Exception` checks before invoking `ForcedCriticalErrorsLogger`. Identical pattern to commit `64a4caa39242` ("Mark fuzzer exceptions as logged to prevent ForcedCriticalErrorsLogger") which used `try { throw; } catch (Exception & e) { e.markAsLogged(); }` in three places in `executeQuery.cpp`.
d	Test intent preserved / new tests added?	Yes — semantically equivalent. The new `try` block: returns normally → `FAIL()` triggers (test fails); throws `DB::Exception` → matches `catch (Exception & e)` (test passes); throws something else → catch doesn't match, gtest reports an unhandled exception (test fails). Same coverage as `EXPECT_THROW(..., Exception)`, plus the `markAsLogged` side effect. No new test added — the regression here is in a test, not in production code.
e	Both directions demonstrated?	Yes. Built `unit_tests_dbms` in release mode (`-O3 -DNDEBUG`, `ENABLE_TESTS=1`, no sanitizer). PRE-FIX run prints 17-frame stack trace + `[ OK ]` (32 lines of stdout/stderr); POST-FIX run prints just `[ OK ]` (clean, ~12 lines, no `Code: 49`, no Poco/`DB::Exception` frames). Same binary infrastructure differing only in this test's source. Logs at `build-release/test_backup_PRE_FIX.log` and `build-release/test_backup_POST_FIX.log`.
f	Fix is general, not a narrow patch?	Targeted — the change applies exactly to the test path that the @alexey-milovidov directive on PR #100177 named (`-FunctionsStress` `Backup checksum should not be zero`). Other `EXPECT_THROW(..., Exception)` sites in `gtest_*.cpp` files exist (e.g. `gtest_gcs_credentials.cpp`, `gtest_archive_reader_and_writer.cpp`, `gtest_writebuffer_s3.cpp`), but they only suffer from the same pollution if the thrown code is `LOGICAL_ERROR` / `POTENTIALLY_BROKEN_DATA_PART` / `CORRUPTED_DATA` (the codes `isErrorCodeImportant` covers). CIDB shows only this specific `Backup checksum should not be zero` trace recurring (16 hits / 60 days). Widening to a generic `EXPECT_THROW`-with-`markAsLogged` macro would be a separate refactor.

Session id: cron:clickhouse-ci-task-worker:20260607-063900

groeneai · 2026-06-07T07:28:29Z

cc @jkartseva — could you review this? Test-only change in src/Backups/tests/gtest_backup_data_file_name_generator.cpp that calls markAsLogged on the caught LOGICAL_ERROR exception in BackupDataFileNameGeneratorTest.ThrowsOnZeroChecksum so its destructor no longer prints a stray stack trace via ForcedCriticalErrorsLogger. Spun out from PR #100177 (@ alexey-milovidov directive).

clickhouse-gh · 2026-06-07T08:14:10Z

Workflow [PR], commit [f428aac]

Summary: ❌

job_name	test_name	status	info	comment
Stateless tests (amd_debug, parallel)		FAIL
	Server died	FAIL	cidb, issue	ISSUE EXISTS
	Logical error: Virtual row boundary violated in MergingSortedAlgorithm for input A on sort column 'B' C: the virtual row announced D but the source then produced E, which would mis-order the merge (STID: 2651-3359)	FAIL	cidb	IGNORED

AI Review

Summary

This PR replaces EXPECT_THROW in BackupDataFileNameGeneratorTest.ThrowsOnZeroChecksum with an equivalent manual try/catch so the caught DB::Exception can be marked via markAsLogged. That preserves the old type-level assertion while preventing ForcedCriticalErrorsLogger from emitting the known misleading LOGICAL_ERROR stack trace. I did not find any current-code findings.

Final Verdict

Status: ✅ Approve

clickhouse-gh · 2026-06-07T11:24:00Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.60%	84.50%	-0.10%
Functions	92.30%	92.30%	+0.00%
Branches	77.20%	77.20%	+0.00%

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

Newly covered by added/modified tests: 324 line(s), 20 function(s) across 105 file(s) · Details

Top files

src/IO/S3/copyS3File.cpp: 18 line(s)
src/Disks/IO/CachedOnDiskWriteBufferFromFile.cpp: 15 line(s)
src/Storages/StorageReplicatedMergeTree.cpp: 14 line(s)
src/Coordination/KeeperServer.cpp: 12 line(s), 1 function(s)
src/Storages/Statistics/ConditionSelectivityEstimator.cpp: 11 line(s), 2 function(s)

Full report · Diff report

groeneai · 2026-06-07T12:44:01Z

CI ledger for HEAD f428aacf2933 (BackupDataFileNameGenerator gtest markAsLogged fix):

check	status	classification	disposition
All builds, FT (except 1), IT, Unit, Performance, ClickBench, Install, Compatibility, Docker, AST/BuzzHouse fuzzer	pass	clean	---
`Stateless tests (amd_debug, parallel)` "Logical error: Virtual row boundary violated in MergingSortedAlgorithm ... STID 2651-3359" + Server died	fail	chronic / not PR-caused	Tracked in our umbrella for the `MergingSortedAlgorithm` virtual-row debug assertion (vdimir territory; assertion added in #106565). 7-day cross-PR: 34 distinct PRs / 36 hits. PR diff: 1 file (`src/Common/tests/gtest_BackupDataFileNameGenerator.cpp`), entirely unrelated to MergingSortedAlgorithm. Our fix PR for STID 2651-3359 is #106664.

CI fully finished at 2026-06-07 12:13:43Z. Mergeable Check + PR aggregator failures are downstream of the chronic STID 2651-3359 stateless test crash.

vdimir · 2026-06-16T13:42:04Z

Stateless tests (amd_debug, parallel) FAIL
Server died FAIL cidb
Logical error: Virtual row boundary violated in MergingSortedAlgorithm for input A on sort column 'B' C: the virtual row announced D but the source then produced E, which would mis-order the merge (STID: 2651-3359) FAIL cidb

Fix #107053 (need review)

groeneai · 2026-06-16T14:33:03Z

groeneai mentioned this pull request Jun 7, 2026

ThreadPool: wake most recently idle worker thread (LIFO) when scheduling a job to reduce memory fragmentation #100177

Merged

PedroTadim added the can be tested Allows running workflows for external contributors label Jun 7, 2026

clickhouse-gh Bot added the pr-ci label Jun 7, 2026

jkartseva self-assigned this Jun 8, 2026

jkartseva approved these changes Jun 9, 2026

View reviewed changes

groeneai mentioned this pull request Jun 15, 2026

Merge expressions into join during join reordering #98533

Open

groeneai mentioned this pull request Jun 16, 2026

Add asynchronous metrics for executable UDF memory usage #107300

Merged

1 task

jkartseva enabled auto-merge June 16, 2026 20:03

jkartseva added this pull request to the merge queue Jul 2, 2026

Merged via the queue into ClickHouse:master with commit 7be48ac Jul 2, 2026
164 of 166 checks passed

robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 2, 2026

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mark caught LOGICAL_ERROR exception as logged in BackupDataFileNameGenerator gtest#106668

Mark caught LOGICAL_ERROR exception as logged in BackupDataFileNameGenerator gtest#106668
jkartseva merged 1 commit into
ClickHouse:masterfrom
groeneai:groeneai/fix-mark-as-logged-backup-data-file-name-gtest

groeneai commented Jun 7, 2026 •

edited by robot-clickhouse

Loading

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026 •

edited by jkartseva

Loading

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

vdimir commented Jun 16, 2026

Uh oh!

groeneai commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 7, 2026 • edited by robot-clickhouse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Version info

Uh oh!

groeneai commented Jun 7, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026 • edited by jkartseva Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

clickhouse-gh Bot commented Jun 7, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 7, 2026

Uh oh!

vdimir commented Jun 16, 2026

Uh oh!

groeneai commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

groeneai commented Jun 7, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented Jun 7, 2026 •

edited by jkartseva

Loading