Remove orphan cache file on non-ErrnoException download failure

[groeneai]

Closes: #110532

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

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

Fix a logical error in the filesystem cache (Expected file ... not to exist) that could occur when a background cache download failed with a non-ErrnoException (for example while OPTIMIZE-ing an Iceberg table), leaving an empty orphan cache file behind.

Description

FileSegment::write() creates the cache file before any bytes are accounted for. On a write failure, only the ErrnoException catch block removed the just-created empty file when downloaded_size == 0. The generic Exception and fs::filesystem_error catch blocks never got the same cleanup, so a non-ErrnoException failure after the file was created orphaned an empty file. The segment is later reset to EMPTY while the file persists, and assertCacheCorrectness (debug/sanitizer builds) then aborts with LOGICAL_ERROR.

Same assertion as the closed #99195, but reproduced on current master with a concrete trigger and background download: 1.

The fix mirrors the existing ErrnoException orphan-removal into the two non-Errno catch blocks, using the noexcept fs::remove overload so the original download error is not masked. A cache_filesystem_failure_non_errno failpoint and a stateless regression test reproduce the exact assertion.

cc @kssenii

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. New cache_filesystem_failure_non_errno failpoint throws a generic (non-Errno) Exception after the cache file is created; a read then fails on demand.
b	Root cause explained?	FileSegment::write() creates the file before accounting bytes; only the ErrnoException catch removed the empty file when downloaded_size==0. The generic Exception / fs::filesystem_error catches did not, so a non-Errno failure orphans an empty file; the segment resets to EMPTY while the file persists, tripping assertCacheCorrectness.
c	Fix matches root cause?	Yes. Mirrors the exact ErrnoException orphan-removal into the two non-Errno catch blocks.
d	Test intent preserved / new tests added?	New stateless test 04545_filesystem_cache_orphan_file_non_errno_failure reproduces the exact assertion (background download: 1, downloaded size: 0). No existing test weakened.
e	Demonstrated both directions?	Yes. Without fix: server aborts with Logical error: 'Expected file .../0 not to exist ... state: PARTIALLY_DOWNLOADED_NO_CONTINUATION, downloaded size: 0 ... background download: 1'. With fix: self-check passes, correct result, server alive.
f	Fix general, not a narrow patch?	Yes. Both non-Errno catch families (Exception and fs::filesystem_error) get identical cleanup; the ErrnoException path already had it. All three write-failure paths now symmetric.
g	Generalizes across inputs?	The orphan condition is downloaded_size==0 regardless of failure type or segment kind; the guard covers every non-Errno write failure at file-creation time.
h	Backward compatible?	Yes. Pure cleanup on an error path; no format/setting/default change.
i	Invariants preserved?	Yes. Uses the noexcept fs::remove overload so the original download error is not masked; setDownloadFailedUnlocked still runs first; the rethrow behavior is unchanged.

Session id: cron:clickhouse-worker-slot-4:20260715-112800

[groeneai]

cc @kssenii for review (per @ PedroTadim on #110532). Small filesystem-cache orphan-file fix.

[clickhouse-gh]

Workflow [PR], commit [e3dd72c]

Summary: ❌

• Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Integration tests (amd_msan, 7/8)		FAIL
	test_replicated_database/test.py::test_replicated_table_structure_alter	FAIL	cidb, issue

---

AI Review

Summary

This PR mirrors the empty-orphan cleanup into the non-ErrnoException cache-write failure paths and adds a failpoint-based regression script. The C++ fix itself looks consistent with the existing ErrnoException handling, but the new test still does not prove that the failing branch actually ran, so the evidence is not strong enough to approve.

Tests

• ⚠️ [tests/queries/0_stateless/04545_filesystem_cache_orphan_file_non_errno.sh:27] The first SELECT is expected to fail, but the script never checks that it did. .sh tests are not run with set -e through shell_config.sh, so this command can succeed or fail and the script still continues to the final SELECT. If the failpoint stops hitting the intended non-ErrnoException path, the test still goes green and no longer proves that the orphan-cleanup branch ran. Capture the status or output, disable the failpoint, then assert that the query failed (optionally matching the injected error text).

Final Verdict

• Status: ⚠️ Request changes

[clickhouse-gh]

Because .sh tests are executed without set -e unless the script opts in, this SELECT can fail or succeed and the script will still continue. That makes the regression weak: if the failpoint stops hitting this path, the test goes green without ever proving the non-ErrnoException branch ran.

Please make the expected failure explicit here, for example by inverting the command with if ...; then echo "expected failure"; exit 1; fi (and optionally checking that the error text is the injected one).

[kssenii]

Let's make it MEMORY_LIMIT_EXCEEDED, it will be more plausible than CANNOT_READ_ALL_DATA

[groeneai]

Done in 9ca22ed: the non-errno failpoint now throws MEMORY_LIMIT_EXCEEDED instead of CANNOT_READ_ALL_DATA. Re-verified both directions locally (debug, cache over local_blob_storage, background_download_threads=2): failpoint fires with background download: 1 / downloaded size: 0, orphan removed, self-check passes (correct result 4999950000), server alive.

[groeneai]

CI finish ledger — 9ca22ed

CI fully finished (Finish Workflow pass). The only red job is the fleet-wide host-OOM window; not PR-caused.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_asan_ubsan, distributed plan, parallel) / unrelated tests	host-OOM (Code 241 (total) memory limit exceeded, RSS ~41.84 GiB host cap)	#110503 (ours, open)
Mergeable Check	aggregate of the above	#110503 (ours, open)

[clickhouse-gh]

LLVM Coverage Report

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

Changed lines: Changed C/C++ lines covered: 21/29 (72.41%) · Uncovered code

Full report · Diff report