HDDS-16092. Stop takeSnapshot from lowering the persisted transaction index

[kerneltime]

Generated-by: Claude Code (Fable 5)

What changes were proposed in this pull request?

The OM records how far it has applied in a single RocksDB key, TRANSACTION_INFO_KEY. Two paths write it and they are not ordered against each other, so one can overwrite the other with an older value.

The double buffer writes that key inside the same batch as the transaction data, so the two commit together. It updates the state machine's in-memory counter only after the commit returns. A snapshot taken in that gap reads the not-yet-updated counter and writes it over the newer value the commit just stored — and forces it to disk.

The DB is then in a state where it physically contains transactions that its own index says it does not.

Under load this is self-repairing: the next commit rewrites the key correctly a few milliseconds later. It matters on graceful shutdown, where Ratis takes a snapshot on the way down and the double buffer stops right after, so nothing repairs it. That OM restarts believing it is behind its own data and replays a batch its peers never replay. Rolling restarts are the realistic exposure.

Two guards that look like they would prevent the race do not, both for the same reason. The wait loop in takeSnapshot() only runs while applied < lastSkippedIndex, and max(applied, notified) only helps if notified is ahead — but both advance solely in notifyTermIndexUpdated, which Ratis calls only for non-state-machine entries, and the OM disables Ratis log-metadata entries (OzoneManagerRatisServer.java:810). In steady state the loop never executes and the max is just applied.

HDDS-16092 has the detailed analysis, including which parts are verified and which are inferred.

The fix

A lock in the double buffer orders its batch commit against the snapshot's write, and persistIfNewer makes that write monotonic — it will not lower the stored index. The snapshot then reports whatever value is actually stored, so the DB row, the in-memory copy Ratis reads, and the returned index all agree.

A read-then-write check alone is not enough: a commit already in flight can land between the check and the write. The read deliberately uses getSkipCache, matching TransactionInfo.readTransactionInfo, because the value it compares against is written by a batch commit that does not populate the table cache.

The lock covers the batch commit and the read-compare-write, but not the snapshot's flushDB. Only the flush daemon commits and snapshots are rare, so it is effectively uncontended. Lock ordering is one-directional — the commit block releases before the applied-index update takes the state machine monitor — so there is no cycle.

What is the link to the Apache JIRA

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

How was this patch tested?

New unit tests, each checked by mutation — remove one half of the fix and confirm which test fails:

Mutation	interleaving test	concurrency test	state-machine tests
lock removed, comparison intact	fails	passes	passes
comparison removed, lock intact	passes	fails	fails
neither removed	passes	passes	passes

• TestOzoneManagerDoubleBufferTransactionInfo (new) holds a snapshot between its read and its write while a real batch commit is attempted. It is the only test that catches a missing lock — the window is far too narrow to lose by chance. Its sibling runs commits and snapshots concurrently and asserts a reader never sees the stored index move backwards.
• TestOzoneManagerStateMachine gains three cases for the state machine's use of it. The first fails on unfixed master with expected: <1#105> but was: <1#100>.
• The new tests live in their own class because the existing double-buffer tests assert on cumulative flush counters that any added commit would inflate, and they keep the flush daemon stopped so the test thread is the only flusher, as in production.

71 tests pass across TestOzoneManagerStateMachine and the double-buffer suites, plus TestOMRatisSnapshots (7/7, real snapshots and checkpoint installs under load) and TestOzoneManagerRestart (3/3) — those two cover the risk this change actually adds, which is a lock on every batch commit on the write path.

The race is also reproduced end to end on a 3-OM HA cluster, with the real Ratis snapshot trigger racing the real flush under real client write load. The only setting changed from production is the snapshot trigger threshold, dropped from 400000 to 50 — that does not create the race, it just samples the existing window thousands of times instead of once. Detector: poll each OM's persisted index and record any move backwards.

• unfixed: 24 occurrences within ~30s, on two of the three OMs
• fixed: 0 across 21,635,614 samples in a full run

That reproduction is not committed here — it depends on a non-production threshold and on timing, so it would be a CI gate that passes whether or not the bug is present. It lives on a branch you can run yourself instead: kerneltime/ozone@HDDS-16092-repro — unmodified master plus one test, which fails there and passes with these commits cherry-picked. See the comment below for how to run it. The staged interleaving test above is the deterministic equivalent.

[kerneltime]

Reproduction you can run yourself

The analysis above is a code trace, so here is the race actually happening. Branch, on my fork, not for merge:

https://github.com/kerneltime/ozone/tree/HDDS-16092-repro

It is unmodified master plus one integration test. Nothing is staged, no delay is injected, no DB is edited by hand — a real 3-OM HA cluster, real client writes, the real Ratis snapshot trigger racing the real double-buffer flush.

mvn -pl hadoop-ozone/integration-test -am test \
    -Dtest=TestHDDS16092TransactionInfoRegression

On that branch it fails:

HDDS-16092 HIT: omNode-3: persisted index went BACKWARDS 51 -> 50
AssertionFailedError: The persisted transaction index moved backwards, so the DB
now contains transactions its own index disclaims

Cherry-pick this PR's commits on top and it passes — 0 regressions across 21,635,614 samples in a full 90-second run, versus a hit within ~30 seconds without them.

Why the threshold is lowered

The one setting changed from production is ozone.om.ratis.snapshot.auto.trigger.threshold, from its 400000 default down to 50. That does not create the race. It turns one draw per 400000 transactions into a draw every few transactions, so the existing window gets sampled thousands of times in half a minute instead of once. That ratio is the whole reason this has not been seen in the field.

Why it is on a branch and not in this PR

It depends on a non-production setting and on thread timing, so a green run proves nothing — it would be a CI gate that passes whether or not the bug is present. The committed tests in this PR are the deterministic equivalent, each verified by mutation to fail when the half of the fix it covers is removed. This branch is for seeing the real thing.

The detector

A watcher per OM polls TRANSACTION_INFO_KEY with getSkipCache (as TransactionInfo.readTransactionInfo does for this key) and records any move backwards. A persisted watermark decreasing is self-evidently wrong, so it needs no knowledge of the true applied index. All three OMs are watched, since each runs its own state machine updater and flush daemon and is an independent draw. Observed hits land on followers.

[Copilot]

Pull request overview

Prevents OM snapshots from lowering the persisted transaction index during concurrent double-buffer commits.

Changes:

• Serializes transaction-info writes and persists only newer indexes.
• Reports the actual persisted snapshot index.
• Adds deterministic concurrency and state-machine tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
OzoneManagerDoubleBuffer.java	Adds locking and monotonic persistence.
OzoneManagerStateMachine.java	Uses the persisted index for snapshots.
TestOzoneManagerDoubleBufferTransactionInfo.java	Tests concurrent index writes.
TestOzoneManagerStateMachine.java	Tests snapshot persistence behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.