Skip to content

[core] Keep main's snapshots when fast_forward fails and hold the commit lock - #10845

Open
LuciferYang wants to merge 2 commits into
apache:masterfrom
LuciferYang:fix/fast-forward-safety
Open

LuciferYang wants to merge 2 commits into
apache:masterfrom
LuciferYang:fix/fast-forward-safety

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

On a filesystem-managed table, fast_forward deletes main's snapshots, schemas and tags from the branch's earliest snapshot onwards together with the LATEST hint, then copies the branch's snapshot, schema and tag directories in listing order. It does this without the lock that commits use. A failure part way leaves main without the snapshot the branch was created from, with missing or truncated snapshot files, and without the schemas its remaining snapshots use. A commit to main can also interleave with the delete and copy.

With this change:

  1. fast_forward holds the lock that commits to main use for the whole operation, and checks that the lock is still valid right before each file it writes or deletes (after reading the file it copies), so it stops once a lease-based lock such as the Hive lock has expired. Without a configured lock this is a no-op, as for commits.
  2. When main is a prefix of the branch, it only appends. Main is a prefix when every main snapshot from the branch's earliest one (or main's own earliest, if later) to main's latest has the branch's content, and main's schemas are the branch's. It then writes the branch's newer schemas and snapshots in id order with tryToWriteAtomic, the tags that differ, and the LATEST hint. Nothing on main is deleted, so after a failure main ends at one of the branch's snapshots and running fast_forward again continues from there. Main's latest snapshot is found by listing, not from the LATEST hint.
  3. Otherwise main is replaced from the branch's earliest snapshot as before, but schemas are replaced one file at a time in id order instead of deleted up front, and snapshots are copied in id order with tryToWriteAtomic. Main's tags are deleted before its snapshots, and a main snapshot, schema or tag that cannot be deleted fails the fast-forward instead of being left behind.

The states that an interrupted fast_forward of an earlier version leaves (a LATEST hint naming a missing snapshot, a gap, a truncated snapshot, main's own snapshot left next to the branch's) fail the prefix check, so the next fast_forward replaces main and repairs them.

Publication is still not atomic. Readers can see some of the branch's snapshots before the rest are copied, and when the branch has newer schemas, main's latest schema switches to the branch's before its snapshots arrive.

Other differences from the current behavior: tags on main that point at snapshots it shares with the branch are kept instead of deleted and copied back, and main's EARLIEST hint is no longer overwritten with the branch's. Replacing main moves it back to the branch's earliest snapshot only when main had expired past it.

Tables whose catalog manages versions, such as REST, use CatalogBranchManager and are not changed.

Related to #9690.

Tests

New cases in FileSystemBranchManagerTest, with a test FileIO that lists files in name order like HDFS and object stores do, and can fail a chosen write:

  • testFailedFastForwardKeepsMainSnapshots, testFastForwardResumesAfterPartialFailure: a snapshot write into main fails. Main stays readable at snapshot 2 or 3, and the retry writes only the missing snapshots and deletes nothing.
  • testFastForwardStopsWhenLockLeaseExpires, testFastForwardRechecksLeaseAfterReadingSource, testReplaceStopsDeletingWhenLeaseExpires, testFastForwardRunsUnderCommitLock: on both paths, every change to main's snapshot files happens while the lock is held, on the same identifier as main's commits, and a lease that expires between writes, while a source file is read, or between deletes stops the next change.
  • testReplaceFailsWhenSnapshotCannotBeDeleted, testReplaceDeletesTagsBeforeSnapshots: a main snapshot that cannot be deleted fails the fast-forward. The retry leaves main at the branch's latest, and does not keep a tag on main's own snapshot when it appends to what the failed attempt left.
  • testFastForwardRepairsInterruptedReplace, testFastForwardReplacesDivergentSnapshotOfSameLength: four states an interrupted earlier version can leave in main's snapshot directory are repaired to the branch's bytes, and so is main's own snapshot 3 left next to the branch's snapshot 4 with the same file length.
  • testFailedReplaceKeepsMainSchemas, testFailedSchemaReplaceKeepsMainReadable: a snapshot or schema write fails while main is replaced, and the snapshot main keeps can still be read.
  • testFastForwardCopiesSchemasInIdOrder, testFastForwardCopiesBranchSchemas, testFastForwardAfterMainSchemaChangeReplacesMain: schemas 2 to 10 are written in id order; a column added on the branch reaches main; when main and the branch both created schema 2, main ends with the branch's.
  • testFastForwardAfterMainDivergedReplacesMain, testFastForwardKeepsMainTagsUnchanged: main's own snapshot 3 is replaced by the branch's; a tag main shares with the branch is not rewritten.
  • testFastForwardAppendsAfterMainExpiredSharedSnapshots, testReplaceMovesEarliestHintBack: after main expires the snapshots it shares with the branch, the next fast_forward still only appends; replacing a main that expired past the branch's earliest snapshot moves EARLIEST back, also when the first attempt fails and the retry appends.

API and Format

no

Documentation

no

@LuciferYang

Copy link
Copy Markdown
Contributor Author

cc @JingsongLi

@JingsongLi JingsongLi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requirement fit: SUPPORTED. Implementation: FINDINGS.

Protecting fast-forward against interrupted copies and concurrent commits has clear production value. Normal JDK 8 Maven verify passes 61 unique Core cases (FileSystemBranchManagerTest and CatalogEnvironmentTest), and this head's CI is green. Two additional real-table fault-injection cases expose gaps below: publication after lease loss during source IO, and an equal-length divergent intermediate snapshot left unrepaired after an exact baseline interrupted replacement. Both have negative controls; repository sources were left unchanged.

// A lease may expire while many files are copied; stop before writing past it.
lock.ensureValid();
String content = readExactly(source);
if ((fileIO.exists(target) || !fileIO.tryToWriteAtomic(target, content))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Revalidate the lease after source IO and before publication

The lease check occurs before readExactly(source), so a remote read can outlast the lease or a failed heartbeat. I reproduced this through actual table.fastForward: main has snapshot 1, a tag-derived branch has snapshot 2, and FileIO expires the lease while opening branch snapshot 2. tryToWriteAtomic still publishes main snapshot 2 with expired=true; fastForward only throws at a later check. An identical control with ensureValid() immediately after the source read leaves main snapshot 2 absent. Please recheck after potentially blocking preparation and before the actual mutation, and cover this timing rather than only expiry between completed writes. This is a gap in the newly promised lease protection (the baseline did not provide locking). Apply the same boundary discipline to hint writes and multi-file deletes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 02d7d67. copyAtomically now checks the lease after reading the source, right before the write. Replacing main deletes one file at a time with a check before each delete, and the LATEST and EARLIEST hint writes are checked too. testFastForwardRechecksLeaseAfterReadingSource expires the lease while the branch's snapshot 2 is opened and asserts main gets no snapshot 2; testReplaceStopsDeletingWhenLeaseExpires expires it after the first delete and asserts nothing else is deleted.

Map<Long, Long> sizes, Map<Long, Long> branchSizes, long from, long to) {
for (long id = Math.max(from, Collections.min(sizes.keySet())); id <= to; id++) {
Long size = sizes.get(id);
if (size == null || !size.equals(branchSizes.get(id))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Verify intermediate snapshot content before treating main as a prefix

Equal size does not establish snapshot identity. With real writes, divergent main and branch snapshot 3 are both 639 bytes. I executed the exact baseline fastForward with failed quiet metadata deletions, listing snapshot 4 before snapshot 3, and an injected failure before overwriting snapshot 3. This leaves branch snapshot 4 on main, the divergent full-length main snapshot 3, and matching schema files. After disabling all faults, this head's fastForward returns success because only the latest snapshot is compared by content. A time-travel read of main snapshot 3 still returns rows [1,2,5], while branch snapshot 3 returns [1,2,3]. Removing main snapshot 3 to force replacement repairs the same table. Please establish content identity for every retained snapshot in the claimed prefix, or use another reliable lineage/integrity check; length is insufficient for the interrupted-old-replace recovery this PR supports.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 02d7d67. The prefix check now compares the content of every main snapshot from the branch's earliest (or main's earliest, if later) to main's latest with the branch's, bytes first and parsed snapshots if the bytes differ. testFastForwardReplacesDivergentSnapshotOfSameLength builds the state you describe: main's own snapshot 3, the same length as the branch's, next to the branch's snapshot 4. fast_forward now replaces main, and snapshot 3 ends up with the branch's bytes.

Replacing main also no longer deletes quietly. A file that is still there after the delete fails the fast-forward (testReplaceFailsWhenSnapshotCannotBeDeleted), so this version does not leave that state behind itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants