Skip to content

Fix flaky test_backup_to_s3_different_credentials#101499

Open
groeneai wants to merge 1 commit intoClickHouse:masterfrom
groeneai:fix-flaky-test-backup-s3-different-credentials
Open

Fix flaky test_backup_to_s3_different_credentials#101499
groeneai wants to merge 1 commit intoClickHouse:masterfrom
groeneai:fix-flaky-test-backup-s3-different-credentials

Conversation

@groeneai
Copy link
Copy Markdown
Contributor

@groeneai groeneai commented Apr 1, 2026

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 to CHANGELOG.md):

CI Fix or Improvement (changelog entry is not required)

Description

Remove overly strict S3ReadRequestsErrors / DiskS3ReadRequestsErrors assertions from test_backup_to_s3_different_credentials.

Root cause: S3ReadRequestsErrors counts all non-throttling errors in GET/HEAD requests to S3, including transient network errors that are retried and succeed. Under ASAN/UBSAN overhead in CI, MinIO operations are slower and transient read errors become more likely during the large (10M rows) data transfer. The backup/restore operation itself succeeds — the data integrity check passes.

CIDB evidence: Failures concentrate on amd_asan_ubsan, db disk, old analyzer builds (2 master + 20+ PR failures in 30 days excluding March 17-18 infrastructure outage), consistent with sanitizer overhead amplifying transient network errors.

Why the assertions are unnecessary: The test's core purpose — verifying S3 native copy fallback with different credentials — is validated by:

  • assert ("S3CopyObject" in events) == (allow_s3_native_copy == True) — native copy was attempted
  • assert ("S3WriteRequestsErrors" in events) == (allow_s3_native_copy == True) — native copy failed (different credentials)
  • assert ("S3CreateMultipartUpload" in events) == use_multipart_copy — multipart behavior
  • Data integrity check in check_backup_and_restore — backup/restore correctness

The S3ReadRequestsErrors assertion tests S3 transport reliability rather than credentials behavior.

Closes #90662

@groeneai
Fix flaky test_backup_to_s3_different_credentials by removing overly …
5c21aaa 
…strict S3ReadRequestsErrors assertion

The assertion `assert "S3ReadRequestsErrors" not in events` fails intermittently
because it counts all non-throttling errors in GET/HEAD requests, including
transient network errors that are retried and succeed. Under ASAN/UBSAN overhead
in CI, MinIO operations are slower and transient read errors become more likely
during large (10M rows) data transfers.

The test's core purpose — verifying native copy fallback with different
credentials — is validated by the remaining assertions (S3CopyObject,
S3WriteRequestsErrors, S3CreateMultipartUpload) and the data integrity check in
check_backup_and_restore.

Closes ClickHouse#90662
@groeneai
Copy link
Copy Markdown
Contributor Author

groeneai commented Apr 1, 2026

Pre-PR Validation Gate

Session: cron:clickhouse-ci-task-worker:20260401-124500

a) Deterministic repro?
The assertion failure is triggered by transient S3 read errors during the 10M-row data transfer. By nature, transient network errors are not deterministically reproducible — they depend on CI load and sanitizer overhead. However, CIDB data confirms consistent failures: 84 failures across 30+ distinct PRs in 30 days (excluding infra spike), with master failures exclusively on amd_asan_ubsan, db disk, old analyzer, 2/6.

b) Root cause explained?
S3ReadRequestsErrors counts ALL non-throttling errors in GET/HEAD requests (from PocoHTTPClient::addMetric(request, S3MetricType::Errors) at line 515/706/723), including retried transient errors. Under ASAN/UBSAN overhead, MinIO connections are slower, increasing probability of timeouts/5xx errors during the 10M-row backup data transfer. The errors are retried and the operation succeeds. The assertion assert "S3ReadRequestsErrors" not in events treats any transient error as a test failure.

c) Fix matches root cause?
Yes. Removing the assertion that checks for zero transient errors directly addresses the issue. The test's actual purpose (different-credentials native copy fallback) is validated by the remaining assertions.

d) Test intent preserved?
Yes. All credential-behavior assertions remain:

  • S3CopyObject presence check (native copy attempted)
  • S3WriteRequestsErrors check (native copy failed)
  • S3CreateMultipartUpload check (multipart behavior)
  • Data integrity check (backup/restore correctness)

e) Demonstrated in both directions?
Local runs: 8/8 test variants pass with the fix. CIDB shows consistent failures without the fix (2 master + 80+ PR failures in 30d). The fix removes the only assertion that triggers on transient errors.

f) Fix is general?
This assertion only exists in test_backup_to_s3_different_credentials — no other tests affected.

@nikitamikhaylov nikitamikhaylov added the can be tested Allows running workflows for external contributors label Apr 1, 2026
@groeneai
Copy link
Copy Markdown
Contributor Author

groeneai commented Apr 1, 2026

cc @vitlibar — could you review this? Removes the S3ReadRequestsErrors assertion from test_backup_to_s3_different_credentials which fails intermittently due to transient S3 network errors under sanitizer builds. The test's credential-behavior validations are preserved.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Apr 1, 2026 •
edited
Loading

Workflow [PR], commit [5c21aaa]

Summary: ❌

job_name test_name status info comment
Integration tests (amd_llvm_coverage, 5/5) failure
test_overcommit_tracker/test.py::test_user_overcommit FAIL cidb

AI Review

Summary

This PR makes test_backup_to_s3_different_credentials less flaky by removing assertions that S3ReadRequestsErrors/DiskS3ReadRequestsErrors are absent. Given the test’s goal (credential-mismatch fallback behavior), this is a reasonable stabilization: the retained assertions still verify native-copy attempt/fallback behavior, and check_backup_and_restore still validates end-to-end correctness. I did not find correctness, safety, performance, or compliance issues in the change itself.

ClickHouse Rules
Item Status Notes
Deletion logging ➖
Serialization versioning ➖
Core-area scrutiny ✅
No test removal ✅
Experimental gate ➖
No magic constants ✅
Backward compatibility ✅
SettingsChangesHistory.cpp ➖
PR metadata quality ✅
Safe rollout ✅
Compilation time ✅
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-ci label Apr 1, 2026
@vitlibar vitlibar self-assigned this Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vitlibar vitlibar

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky test: test_backup_restore_s3/test.py::test_backup_to_s3_different_credentials[data_file_name_from_first_file_name-native_multipart]

3 participants

@groeneai @nikitamikhaylov @vitlibar