fix NoSuchKey race in NewPlainDiskLayout, stop pooled-env poisoning in plainRewritable tests - #1526
Merged
Merged
Conversation
…plainRewritable tests NewPlainDiskLayout lists __meta/ and then reads every prefix.path with a separate GetObject; a background merge or concurrent DROP on the live plain_rewritable disk can delete the token directory in between, failing the whole create with NoSuchKey. Skip a token whose prefix.path is confirmed gone via StatFile (ClickHouse building its own path map at startup would not see the directory either); any other read error still fails the backup. runPlainRewritableScenario aborted via r.NoError before its DROP DATABASE, leaving the database on the plain disk in the pooled test env and cascading into failures of unrelated tests which acquired the same env (TestRestoreMapping, TestS3NoDeletePermission, TestAlibabaOverS3 in run 33397256466); drop it best-effort in defer when the test failed.
Coverage Report for CI Build 33414960998Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.06%) to 68.071%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions12 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-caused from the 26.3 job of run 33397256466: 4 of the 5 failures were one cascade.
Fix 1: tolerate a vanished
__meta/<token>/prefix.path(pkg/storage/object_disk/plain.go)NewPlainDiskLayoutlists__meta/and then reads everyprefix.pathwith a separate GetObject. A background merge or a concurrentDROPon the live plain_rewritable disk deletes token directories between the listing and the read, and the wholecreatefailed withNoSuchKey(TestPlainRewritableGCSOverS3hit exactly this 24s into the scenario, right after inserting 4 small parts that CH immediately merged).Now a read error is followed by a
StatFileprobe: if the object is confirmed gone (storage.ErrNotFound), the token is skipped with a warning — ClickHouse building its own path map at startup would not see the directory either. Any other read error still fails the backup. Covered by two new unit tests with a fakeRemoteStorage(vanished token skipped; non-not-found stat keeps the fail-loudly behavior).Fix 2: best-effort DROP DATABASE on scenario failure (test/integration/plainRewritable_test.go)
runPlainRewritableScenarioaborted viar.NoErrorbefore itsDROP DATABASE, returning the pooled env with a leftover database on the plain disk. Every later test acquiring the same env then failed on it:TestRestoreMapping(NoSuchBucket),TestS3NoDeletePermission(AccessDeniedfor the restricted user),TestAlibabaOverS3(leftover-files check). Adefernow drops the database when the test failed, so one plain-disk failure no longer poisons the pool.Verified:
go vetclean,GOFLAGS= make testexit 0 (17 packages ok), new unit tests pass.🤖 Generated with Claude Code