Repository navigation
Conversation
A commit that is applied by the catalog but reported as CommitFailedException is retried; on retry a validator rejects the already-committed checkpoint, and the CleanableFailure makes SnapshotProducer.cleanAll delete the committed snapshot's manifest list and manifests. The first test fails today; the second is a control that a genuine concurrent-commit failure still cleans up. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
SnapshotProducer.commit() called cleanAll() when a commit failed with a CleanableFailure (or always, when strict cleanup is off). A catalog can apply a commit and still report failure, e.g. a 409 for a lost or replayed request. The retry then refreshes, sees its own snapshot, and a validator (such as DynamicCommitter's checkpoint validator) rejects it with a ValidationException, which is a CleanableFailure. cleanAll() then deleted the manifest list and manifests of a snapshot the catalog had already committed, leaving the table pointing at missing files. On failure, rethrow without deleting anything and log the manifest list paths that are left in place so leaked files can be traced. Files from a genuinely failed attempt now leak until orphan file removal. The success-path cleanup, cleanAll() and cleanUncommitted() are unchanged. The now-unused strictCleanup field is removed. Tests that asserted failure-path cleanup now assert the files remain: TestSnapshotProducer (control test inverted), TestFastAppend, TestMergeAppend, TestRewriteFiles, TestRowDelta and TestRESTCatalog. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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
1 participant
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.
Problem
SnapshotProducer.commit()callscleanAll()when the commit fails with aCleanableFailure(or with strict cleanup). If the catalog has already applied the commit but answered 409 (CommitFailedException), the client retries, a validator throws on the retry, andcleanAll()deletes the manifest list and manifests of the snapshot the catalog committed. The table then points at files that no longer exist.This took the prod UK job
lake-log-prod-users-1-scheduledown for about 4h17m on 2026-10-04 (LAKE-7128). It was restored from S3 object versions.Change
cleanAll()call from the failure path ofcommit(). The failure is logged at WARN with the manifest lists left in place, and then rethrown unchanged.CleanableFailure/Exceptionsimports and thestrictCleanupfield.Cost: files from a commit that really failed are no longer deleted at failure time. They become orphans that orphan-file removal has to collect.
Tests
TestSnapshotProducer:committedSnapshotFilesKeptWhenRetryValidationFailsAfterAppliedCommit(format versions 1-4). It fails on the base branch and passes with this change. A control test (uncommittedFilesCleanedUpWhenRetryValidationFailsAfterConcurrentCommit) is kept.TestFastAppend,TestMergeAppend,TestRewriteFiles,TestRowDelta,TestRESTCatalog.TestSnapshotProducer,TestFastAppend,TestMergeAppend,TestRewriteFiles,TestRowDelta,TestRESTCatalog. 1,049 tests, 0 failures, 0 errors, 85 skipped (not inspected). Other modules were not run.Open before merge
requireStrictCleanup()is no longer read bySnapshotProducer.An alternative that keeps cleanup but checks whether the commit actually landed is in a separate local branch and is not part of this PR.
Jira: LAKE-7128
🤖 Generated with Claude Code