HDDS-16086. installCheckpoint sets OM transaction info inconsistent with the checkpoint index

[smengcl]

Generated-by: Claude Code (Opus 5)

What changes were proposed in this pull request?

Before this change, two functions wrote different values to the OM's in-memory transaction info.

loadSnapshotInfoFromDB() reads the value from the installed checkpoint DB, and that value is correct. installCheckpoint writes the TermIndex from before the installation. But the next line unpauses the state machine at the index of the checkpoint.

The one-line change makes installCheckpoint() publish the same term and index that it uses to unpause the state machine.

No failure is demonstrated, but one vote path can read stale state. The OM checkpoint install and the Ratis state reload happen across separate notification RPCs. installCheckpoint() unpauses the OM at the checkpoint index. Ratis updates ServerState.latestInstalledSnapshot and asks StateMachineUpdater to reload only when the leader sends a later notification.

Before that notification, both getLatestSnapshot() and latestInstalledSnapshot are stale, but the Raft log is still intact. During the later notification, ServerState.reloadStateMachine() signals the updater, purges the log with onSnapshotInstalled(), and then records latestInstalledSnapshot. getSnapshotIndex() and containsTermIndex() can use latestInstalledSnapshot after it is recorded. getLastEntry() cannot: when the log is empty, it falls back only to getLatestSnapshot().

The client/admin snapshot path refuses to run while an installation is in progress. The automatic snapshot path does not check the installation state, but the OM's takeSnapshotImpl() recomputes the snapshot position from the applied and notified indexes instead of reading getLatestSnapshot().

decideVote() calls getLastEntry(). It grants a vote when the last entry of this server is less than the last entry of the candidate. Thus an index that is too low can grant a vote that the server must refuse. The vote path does not test getInProgressInstallSnapshotIndex(), and thus an installation does not stop it. RaftLog.onSnapshotInstalled() can make the log empty, and SegmentedRaftLogCache.getLastTermIndex() gives null for an empty log.

This is a Raft election-restriction violation, not proof that the only possible result is an unnecessary election. The pre-install value is the OM's applied index. A follower replies successfully after its log append completes, while the OM advances its applied index only after the double-buffer flush. The range between the applied and acknowledged indexes can therefore include entries that this follower acknowledged and helped commit. A candidate missing such an entry can receive this follower's vote.

A lost commit or successful log truncation was not demonstrated. For such an outcome, the leader would also have to be unavailable, a vote would have to be cast during the vulnerable interval, and progress would have to be made by the resulting leader despite the follower's higher installed snapshot index.

The vulnerable interval requires both a stale getLatestSnapshot() value and an empty Raft log. It starts only if the later notification purges the log before the updater calls loadSnapshotInfoFromDB() at the start of reinitialize(). This scheduling window is likely short, but it was not measured or reproduced. The candidate's position must also fall between the stale and correct indexes.

The change publishes the correct value before the unpause, so the field remains correct throughout the window.

On the path where the DB replacement fails, the change makes no difference. The code reassigns term and lastAppliedIndex only after replaceOMDBWithCheckpoint() returns successfully. Therefore, both variables still identify the restored pre-install position when replacement fails. Also, valueOf(long, long) calls valueOf(TermIndex) (TransactionInfo.java:81). On that path this call is necessary, because no other function writes the field. Thus I corrected the call, and I did not remove it.

A TLA+ model of the follower snapshot-installation path found this issue. The model checks that the three views of the transaction index agree after an installation.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16086

How was this patch tested?

• New test TestOMRatisSnapshots#testInstallCheckpointPublishesNewTransactionInfo calls installCheckpoint directly. Then it compares the transaction info with the OM state machine's last-applied TermIndex, which unpause() updates. The direct call does not trigger a Ratis reload. Before the fix, the test therefore observes INITIAL_VALUE instead of the pre-install position that would be exposed by a real installation. The test must do the comparison immediately because takeSnapshotImpl() also recalculates the field.
  • Without the change, the new test fails on this base and gives: In-memory transaction info must match the position the state machine was unpaused at ==> expected: <(t:1, i:102)> but was: <<INITIAL_VALUE>>. With the change it passes.

(The test does not exercise the vote path. That would require an election inside the short scheduling window described above. I did not find a stable method to cause it.)

[Copilot]

Pull request overview

Aligns OM’s in-memory TransactionInfo publication during follower checkpoint installation with the checkpoint’s actual term/index, eliminating a short window where consumers could observe a stale TermIndex after unpausing the state machine.

Changes:

• Update OzoneManager#installCheckpoint to publish TransactionInfo using the checkpoint’s (term, lastAppliedIndex) instead of the pre-install termIndex.
• Add an integration test asserting installCheckpoint immediately publishes the same TermIndex it unpauses the state machine at.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java	Publish in-memory transaction info based on checkpoint term/index before unpausing.
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMRatisSnapshots.java	Add coverage to ensure installCheckpoint publishes the correct in-memory TransactionInfo immediately after install.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kerneltime]

Change looks good, a wall of text incoming from claude mostly about nits and will file a new bug found..

[kerneltime]

Review: Comment (High confidence) — the one-line fix is correct; the asks are all about the recorded rationale

I reviewed this against the Ratis 3.2.1 sources (the version pom.xml pins) rather than from memory, and traced every reader of omTransactionInfo / getLatestSnapshot() in both trees. The production change is right: publishing (term, lastAppliedIndex) before unpause makes the state machine's advertised snapshot match the position it resumes at, and it is value-identical to the old code on the DB-replace-failure path (term/lastAppliedIndex are only reassigned at 4362-4363 inside the try that failed). No objection to merging.

Four things about the surrounding prose — one of which I think changes how this gets triaged.

Concerns

1. The justification's premise is wrong, even though the conclusion is probably right.

The body argues the stale value is harmless because it "includes every entry that the server had already committed." That premise doesn't hold. The published value is the applied index, and applied lags acked:

• The pre-fix value comes from omRatisServer.getLastAppliedTermIndex() (OzoneManager.java:4326 → OzoneManagerRatisServer.java:987-988), and the OM's applied index only advances on double-buffer flush (OzoneManagerDoubleBuffer.java:396 → updateLastAppliedTermIndex:262).
• A follower acks appendEntries once the log write completes, replying matchIndex = last appended entry, with no wait for the state machine (RaftServerImpl.java:1630-1642). The leader counts that toward commit.

So the hidden range (applied, acked] can contain entries this follower's own ack made committable — entries it was part of the majority for, contrary to the body's "the server was not part of the majority that committed those." Inside the exposure window ServerState.getLastEntry() falls back to the state machine snapshot with no max (ServerState.java:315-326) and VoteContext.decideVote grants whenever our last entry compares below the candidate's (VoteContext.java:136-147), so the grant can go to a candidate missing an entry this follower helped commit. That is an election-restriction violation in the formal sense, not merely a wasted election.

I'd stop short of calling it a data-safety fix for backport purposes: reaching it needs the vote to land inside the window and the leader to be gone, and I did not establish that the resulting leader actually truncates rather than wedging (a leader at index b facing a follower whose log is empty at snapshotIndex = K ≫ b is its own mess). But the sentence as written justifies the conclusion with a claim the code contradicts, and that's what a future reader will lean on.

Separately: you write that you did not measure the window, so for what it's worth, the sub-millisecond estimate holds up on a read of the Ratis side. Exposure needs both a stale field and an empty log, and the log is only purged at ServerState.reloadStateMachine:422 — which runs on the later notification RPC and immediately after :420 has already notified the updater to reload and republish. The long interval where the field alone is stale never reaches the fallback, because the log is intact throughout it. Worth stating explicitly in the body, since the conjunction is what bounds the window and it's currently left implicit.

2. The new comment misdescribes which readers are protected.

Most callers survive a stale value by also consulting latestInstalledSnapshot; getLastEntry() does not

latestInstalledSnapshot has one writer, ServerState.reloadStateMachine (ServerState.java:419-424), and for the OM's notification-based install its only reachable caller is SnapshotInstallationHandler.java:363-364 — which runs on a later notification RPC, after installCheckpoint has returned. For the whole interval between unpause and that next notification, getSnapshotIndex() (ServerState.java:482-486) and containsTermIndex() (:498-506) are just as stale as getLastEntry(). Only the much shorter sub-window after reloadStateMachine matches what the comment describes.

Suggested rewrite:

// Ratis reads this field as the state machine's latest snapshot (getLatestSnapshot).
// Until Ratis calls ServerState.reloadStateMachine on a later notification RPC, nothing
// else in Ratis knows the new index -- getLastEntry()/getSnapshotIndex()/containsTermIndex()
// are all stale, and decideVote() reads getLastEntry(). Publish the index we unpause at.

3. The expression is deliberately path-dependent, and nothing says so.

TransactionInfo.valueOf(term, lastAppliedIndex) means the checkpoint position on success and the pre-install position after a failed replace (4326-4328 vs 4362-4363, catch at 4367-4371). That is exactly right, but it is invisible: the success-path reading makes the line look interchangeable with checkpointTrxnInfo.getTermIndex(), which this method already references three lines away at 4363 and 4437. Making that substitution would publish the checkpoint index after a failed install — the inverse of HDDS-16086, in the over-claiming direction. The old code was path-independent, so this coupling is new here.

One sentence in the comment ("on the replace-failure path these still hold the pre-install position, which is what must be republished — do not substitute checkpointTrxnInfo") or a named local (TermIndex publishAt = ... passed to both the publish and the unpause) would pin it.

4. One smaller precision point.

"The snapshot-creation decision refuses to run during an installation" holds for the client/admin path (RaftServerImpl.takeSnapshotAsync:1269-1276 checks getInProgressInstallSnapshotIndex). The auto-trigger path (StateMachineUpdater.shouldTakeSnapshot:328-339) has no such check. Your conclusion still stands — that path recomputes from applied/notified inside takeSnapshotImpl rather than reading getLatestSnapshot() — but the claim as written is broader than the code guarantees, and the distinction matters because that same recompute is the subject of a separate bug I hit while auditing master (see below).

Suggestions

5. The test comment asserts a failure mechanism I couldn't reproduce by reading.

The follower was never started, so restarting its RPC server at the end of installCheckpoint fails.

Inactive OMs are fully constructed (MiniOzoneHAClusterImpl.createOm), so the RPC server is built and bound in the constructor (OzoneManager.java:755-756); installCheckpoint stops it at :4343, freeing the port, and the restart at :4416-4418 rebuilds from constructor-held state and rebinds the same address. I see no never-started-specific failure. setExitManagerForTesting is still required — several exitSystem sites are reachable on this path and would kill the surefire fork — so keep it, but consider the defensible wording: "installCheckpoint calls exitManager.exitSystem on reload/RPC-restart failure; swallow exits so an environment-specific failure can't kill the test JVM."

6. Optional: pin the assertion to the unpause position directly.

installed is built from the same two locals as the published value (4445 and 4393), so the final assert is anchored to the checkpoint only transitively, through the index check on the line above. Adding

assertEquals(followerOM.getOmRatisServer().getLastAppliedTermIndex(),
    followerOM.getTransactionInfo().getTermIndex(), "...");

pins the literal claim in the message ("the index the state machine was unpaused at"), since that's what unpause writes via setLastAppliedTermIndex. The follower's Ratis server isn't started here, so nothing can move it between the call and the read.