[core] Do not expire the snapshot the LATEST hint points to

[dev-donghwan]

Related: #10324 (merged) fixes one way the LATEST hint falls behind (a snapshot rename that throws but has succeeded).

Purpose

When writing the LATEST hint keeps failing, the hint stays at an old snapshot while
commits still succeed. For example, a commit whose retry finds that the previous attempt
already committed the snapshot returns success without writing the hint. If snapshot
expiration then deletes the hinted snapshot, findLatest returns a missing snapshot, and
reads and commits fail until LATEST is fixed by hand.

This PR keeps snapshot expiration from deleting the hinted snapshot and the ones after it,
like the consumer boundary:

• The end of the expiration range is capped at the hint before any data or manifest file is
  deleted, on both the retained-count and the time-retention paths.
• If the hint cannot be read, expiration is skipped this time. An unreadable hint is told
  apart from an absent one.
• If the hinted snapshot is already missing, the snapshot after it and all later ones are
  kept. If that one is missing too, expiration is skipped.
• Tables whose snapshots are managed by the catalog do not write the hint file, so they are
  not protected.
• A warning is logged when expiration keeps snapshots because of the hint, and when a commit
  retry finds the snapshot already committed.

Reads and commits are not changed.

Behavior changes

• While the hint is behind, more snapshots than snapshot.num-retained.max can be retained,
  like with consumers, with a warning. They are expired once the hint is updated again.
• During that time, the expire_snapshots procedures can expire fewer snapshots or return 0.
• Each expiration reads the LATEST hint once more. It is read right before deleting, to keep
  the window against concurrent commits and rollbacks as small as possible.

Tests

• StaleLatestHintTest: hint read and write failures and recovery, commits and rollbacks
  during expiration, replaying a committed commit with and without append file checks,
  streaming read from latest, incremental read between timestamps, rollback, boundaries
  (missing hinted snapshot, hint around the earliest snapshot, data files of a primary key
  table), catalog managed snapshots, branches, hint reads on the normal path and warnings.
• CommitterOperatorTest: replaying the end-of-input commit in Flink batch mode while the
  LATEST hint is behind.

[JingsongLi]

-1 here will another io.

[dev-donghwan]

Thanks for the review, @JingsongLi! You're right, I overlooked that findLatest is on the hot path and the extra exists check would cost another IO on every lookup.

I've reverted the findLatest change and moved the fix into the existing fallback in latestSnapshotFromFileSystem. Only when reading the hinted snapshot fails with FileNotFoundException and the hint still returns the same id, it now lists the snapshot directory to find the real latest snapshot. If the listing finds no newer snapshot, it still throws as before. So there's no extra IO on the normal path, and the commit path can still recover and rewrite the hint.

Could you take another look when you have time?

[JingsongLi]

Reviewed head d8ef8b9aa0. The stale-hint outage has clear production value, but this revision introduces a replay correctness problem.

[P1] Recover commit-user deduplication before allowing the commit to proceed (SnapshotManager.java:227–232)

filterCommitted still calls latestSnapshotOfUser, which starts from latestSnapshotId(). If LATEST points to an expired snapshot and its successor is also expired, that lookup stops at the missing snapshot and reports no previous commit. The new fallback then lets tryCommit find the real latest snapshot and publish the same files again.

This is reachable in Flink's batch committer restart path: CommitterOperator#commitUpToCheckpoint(END_INPUT_CHECKPOINT_ID) calls filterAndCommit(committables, false, true), deliberately disabling append-file conflict checks after deduplication.

I reproduced this with an append table (bucket=-1), retained snapshot 7 containing the batch user's Long.MAX_VALUE commit, expired snapshots 1/2, and LATEST reset to 1. After reopening the table and replaying the original messages through filterAndCommitMultiple(..., false), this head returns 1, publishes snapshot 8, and a real table read returns 8 rows instead of 7, including the same row twice. With the baseline implementation, the same replay fails on the missing hinted snapshot before publishing. With append-file checks enabled, this head instead throws a duplicate-file conflict and cannot recover.

Please make the commit-user lookup recover from the missing hint as well before concluding that a commit is new, and add replay coverage for both conflict-check modes. A failure-only fallback can preserve the normal-path I/O cost.

There is also a remaining scope gap: latestSnapshotId() remains stale. Before another successful writer repairs LATEST, a fresh scan.mode=latest stream chose checkpoint 2 while the actual latest was 6 (expected checkpoint 7). The added test only checks reading after a successful commit repairs the hint. Please cover reader startup before that repair; this is an existing outage left unresolved, separate from the new duplicate-row regression.

Validation: SnapshotManagerTest + StaleLatestHintTest: 45 tests passed, including a final run without fast-build (Checkstyle/Spotless/enforcer enabled). Additional local replay/read probes reproduced the failures above; no probe changes were pushed.

[dev-donghwan]

Thanks for the detailed review, @JingsongLi. You're right about both points. I reproduced the duplicate commit on d8ef8b9, both in core (replay with and without the append-file check) and through the Flink END_INPUT path on a bucket=-1 table.

I think I took the wrong approach with this PR, so before pushing anything I'd like to ask for your view as the original author of the hint files.

The current direction is to fix the read side. To continue with it, every caller that relies on the latest snapshot id would have to handle a hint that points to an expired snapshot. latestSnapshotId() alone has about 50 callers in the main code, and latestSnapshot() and latestSnapshotOfUser() have more, in core, Flink and Spark. I started with the ones you pointed out and fixed the dedup path, including the commit.last-safe-snapshot branch, which removed the duplicate commit. But the tests then showed other callers behaving differently from what they intend, because only latestSnapshot() gets the real latest while latestSnapshotId() still returns the stale id. For example:

• incremental-between-timestamp silently reads all retained snapshots instead of the requested range (5 rows instead of 2), where master fails.
• scan.mode=latest still starts from the stale id, so your second point stays unfixed.
• The Flink/Spark rollback procedures call latestSnapshot() before rollbackTo, which is what currently stops them on a stale hint. With read-side recovery that call succeeds, and they go on to a rollbackTo that keeps the snapshots after the target and deletes newer tags.

Fixing each of these one by one would make the change much larger, and each fix could bring another side effect like these. So instead of the read side, I looked at the side that deletes snapshots. The stale state only appears when expiration deletes the snapshot the LATEST hint points to, so I'd like to propose preventing that instead.

Proposal

Snapshot expiration does not expire the snapshot the LATEST hint points to, similar to how it already keeps the snapshots consumers still need. Then findLatest works as it is. When the hint is behind, snapshot-(N + 1) exists, so it lists the directory and returns the real latest. All read-side changes are reverted, so every caller behaves exactly as on master.

• No extra IO on the normal path. Expiration reads the LATEST hint once, and only when it is about to delete snapshots.
• Expiration logs a warning whenever it keeps snapshots because the hint is behind.
• I would skip the check when the catalog provides the latest snapshot itself (e.g. REST), where the LATEST file is not the source of truth.
• A table that is already in the stale state behaves as on master. Some operations there are already wrong on master today (see below), so I think such tables are better handled by an explicit repair_latest_snapshot procedure, similar to repair_earliest_snapshot ([core][flink][spark] Support repairing the earliest snapshot hint #8883), as a follow-up.

If hint writes keep failing

In our incident, hint writes failed because of a broken TaskManager, which Paimon cannot prevent. What Paimon can avoid is turning that into a permanent outage: the TaskManager problem lasted about 6 minutes, while the table stayed stuck for two days until LATEST was fixed by hand.

I ran 30 commits with every LATEST write failing. With the proposal, all of them succeeded (each one slower because of the existing hint-write retries), reads and scan.mode=latest stayed correct, and snapshots piled up to 31. The first commit after hint writes recovered moved the hint, and expiration cleaned up back to the retention. On master the same run gets stuck once the hinted snapshot is expired.

Today the only trace of this is the generic Retry commit for exception warning, and the retry path that finds the commit already done logs nothing. If you agree, I'd also add a warning there, e.g. "Snapshot #N was committed by a previous attempt that failed, the LATEST hint may not have been updated", so the problem shows up at the first commit rather than only once snapshots pile up.

I also tried letting expiration rewrite the LATEST hint before deleting. It repairs the hint when another process runs expiration, but without a compare-and-swap on hint files it can race with rollback and write a hint that points to a snapshot the rollback has just deleted, which is the same stuck state. It also adds the hint-write retry delay to every expiration while writes fail. So I kept the proposal read-only.

Measured comparison

• master: current behaviour
• A: read-side recovery (d8ef8b9 plus the dedup fix)
• B: the proposal above

All cells come from running the same probe on the four variants, except the rollback procedures, which are from reading the code.

1. A table whose LATEST hint stops moving while commits and expiration go on

	master	A	B	A + B
Hinted snapshot	Expired	Expired	Kept	Kept
New commit	Fails on every attempt until LATEST is fixed by hand	Succeeds	Succeeds	Succeeds
Committer replay (with/without append-file check)	Fails until LATEST is fixed by hand	No duplicate	No duplicate	No duplicate
scan.mode=latest start	Stale id	Stale id	Real next snapshot	Real next snapshot
incremental-between-timestamp	Fails until LATEST is fixed by hand	Reads all retained snapshots (5 rows instead of 2)	Correct	Correct
rollbackTo	Returns normally, but keeps the snapshots after the target and deletes newer tags	Same as master	Correct	Correct
Rollback procedures	Fail at latestSnapshot()	Reach rollbackTo above	Correct	Correct

2. A table that is already in the stale state

	master	A	B	A + B
New commit, committer replay	Fail until LATEST is fixed by hand	Succeed, no duplicate	Same as master	Succeed, no duplicate
incremental-between-timestamp	Fails until LATEST is fixed by hand	Reads all retained snapshots	Same as master	Reads all retained snapshots
scan.mode=latest start, rollbackTo	Wrong already on master	Same as master	Same as master	Same as master

With B in place, A only runs on tables that are already stale. There it brings back commits but turns other failures into silently wrong results, and it does not fix the rest. So I'd go with B alone.

Tests

The tests produce the stale hint through real commits with LATEST writes skipped, instead of resetting LATEST by hand. They cover the replay in both conflict-check modes, the Flink END_INPUT replay, scan.mode=latest, rollbackTo and incremental-between-timestamp. All of them fail on master and pass with B. The paimon-core and flink committer test suites pass as well.

Does this direction match how you see the hint files, and would you like the extra warning in the commit retry path as part of this PR? If so, I'll update the PR accordingly.

[JingsongLi]

The expiration-side direction (B alone, with the read-side recovery reverted) looks preferable for this PR. It preserves the invariant used by the existing readers and commit-user deduplication, and keeps recovery of already-broken tables as an explicit repair operation. The current d8ef8b9 head still has the replay finding above; this is feedback on the proposed direction, not validation of an updated implementation.

Please implement the protection as a cap on the entire expiration prefix, like the consumer boundary, rather than skipping only snapshot N. Keep the contiguous suffix from the hinted snapshot onward: retaining N while deleting N+1 would still make findLatest return the stale N. Apply the cap before any data/manifest deletion, including the time-retention early-return paths, and keep EARLIEST consistent with the actual retained range.

One detail needs care: HintFileUtils.readHint currently returns null for both an absent hint and exhausted read retries. Expiration must not treat a transient hint-read failure as proof that there is no hint and delete the protected prefix. Please distinguish confirmed absence from an unreadable hint and conservatively skip deletion when that boundary cannot be established. Cover hint-read failures as well as hint-write failures, then successful recovery, concurrent commit/expiration, rollback, both replay conflict-check modes, streaming startup and timestamp-range reads. Skip the filesystem-hint policy only when the catalog actually supplies the snapshot state for this table.

The expiration warning is useful; include the hinted ID, actual latest ID and retained boundary, and avoid repeating it without bound. A concise warning on the retry path that finds the snapshot already committed is also reasonable, with the completed snapshot ID and conditional wording such as “LATEST may not have been updated.” Keep it off the normal successful commit path and avoid claiming a failed hint write when the preceding exception does not establish that. Please update the PR and its tests to B before treating the current findings as resolved.

[dev-donghwan]

Thanks for the direction. I force-pushed the PR to implement B only, so the read-side
recovery commits are gone and reads and commit-user deduplication are unchanged from master.

What this PR changes

• Snapshot expiration caps the end of the expiration range at the LATEST hint, like the
  consumer boundary, so the hinted snapshot and all later ones are kept.
• The hint is read strictly right before deleting. If it cannot be read, nothing is deleted
  this time.
• If the hinted snapshot is already missing, the snapshot after it and all later ones are
  kept. If that one is missing too, nothing is deleted.
• The protection is skipped when the catalog manages the snapshots.
• Warnings are logged on expiration while the hint is behind, and on the commit retry path
  that finds the snapshot already committed.

How each of your points is addressed

1. Cap the entire prefix, keep the contiguous suffix. The cap applies to the whole range,
   so retaining N while deleting N+1 cannot happen.
   (testHintedSnapshotIsNotExpired)
2. Apply the cap before any deletion, including the time-retention paths. Both the
   time-retention early return and the retained-count path call expireUntil, and the cap
   is applied there before innerExpireUntil starts deleting data, changelog and manifest
   files. (testBothExpirationPathsKeepTheHintedSnapshot)
3. Keep EARLIEST consistent. EARLIEST points to the first retained snapshot.
   (testEarliestHintMatchesTheKeptSnapshots)
4. Distinguish absent and unreadable hints, skip when the boundary cannot be
   established. Following readOverwrittenFileUtf8, the hint is returned as an Optional
   (present or absent), and an unreadable hint throws an IOException after the same
   retries as readHint. Then nothing is deleted. A hint that is not a positive number is
   treated as absent, like findLatest does. (testUnreadableHintSkipsExpiration,
   testTransientHintReadFailureIsRetried, testUnusableHintIsIgnored)
5. Test coverage.
   • hint write failures and recovery: testCommitsGoOnWhileHintWritesFail,
     testExpireResumesAfterHintWritesRecover
   • concurrent commit and expiration: testCommitDuringExpiration
   • rollback: testRollback, testRollbackDuringExpiration
   • replay in both conflict-check modes: testReplayCommittedCommit, and
     CommitterOperatorTest#testReplayEndInputCommitWhileLatestHintIsBehind for the Flink
     batch restart path
   • streaming startup before the hint is repaired: testStreamingReadFromLatest
   • timestamp-range reads: testIncrementalBetweenTimestamps
6. Skip the policy only when the catalog supplies the snapshot state. It uses the same
   condition as CatalogEnvironment#snapshotCommit.
   (testCatalogManagedSnapshotsIgnoreTheHintFile)
7. Expiration warning. It includes the hinted ID, the actual latest ID and the retained
   boundary, and is logged once per hint value. The warning for an unreadable hint is logged
   once until the hint can be read again. (testExpirationWarnsOncePerHint,
   testReadFailureWarnsOnceUntilTheHintCanBeRead)
8. Retry warning. It is logged only on the retry path that finds the snapshot already
   committed, with the snapshot ID and "The LATEST hint may not have been updated." That
   path is also reached when the atomic commit fails without an exception, so the wording
   does not claim one. (testRetryWarningOnlyWhenTheCommitIsFoundCommitted)

In addition

I handled the case where the hinted snapshot is already missing. Without it, expiring the
snapshot after it as well makes the table stuck, and on an already stuck table, expiration
deletes only part of the data files, so the remaining older snapshots can no longer be read.
(testHintOnMissingSnapshotKeepsTheNextOne,
testHintOnMissingSnapshotKeepsDataFilesOfTheNextOne,
testHintOnMissingSnapshotsKeepsDataFilesOfOlderSnapshots)

Behavior changes

While the hint is behind, more snapshots than snapshot.num-retained.max can be retained,
like with consumers, and the expire_snapshots procedures can expire fewer snapshots. Each
expiration also reads the LATEST hint once more; it is read right before deleting to keep the
window against concurrent commits and rollbacks small. These are also listed in the PR
description.

[JingsongLi]

@dev-donghwan Did you encounter such complex changes in a production environment? In all my years, I’ve never seen such an extreme scenario.

[dev-donghwan]

@JingsongLi

You are right that this is an extreme scenario, and it does not happen often. We did hit it once in production, but it was a coincidence of a configuration issue on our side and an infrastructure issue at the same time. We have fixed that configuration, and I agree it is unlikely to happen again now. Here is what happened, with simplified numbers (snapshot.num-retained.max=20, checkpoints every minute):

1. The latest snapshot and the LATEST hint are both 100.
2. When committing 101, snapshot-101 was written to the storage, but a transient infrastructure issue made the response come back as a failure. The retry found snapshot-101 already there and finished the commit as successful, but that path did not update the hint.
3. While the issue lasted, the same happened up to 121, so snapshots kept piling up while the hint stayed at 100.
4. Expiration deleted 100 and 101. From then on, findLatest returned the deleted 100, so reads and commits all failed. Only a commit can rewrite the hint, and it failed first, so the table did not recover even after the issue was gone; we fixed LATEST by hand.

The retry path in step 2 is fixed by #10324. But as long as the snapshot and the hint are written separately, an infrastructure issue between them cannot be ruled out, and when expiration then deletes the snapshot its own hint points to, a transient issue becomes an outage that needs manual repair. This PR only aims to prevent that last step.

If you think this case is not worth the change, I will follow your judgment and close the PR.