kerneltime opened a new pull request, #10953: URL: https://github.com/apache/ozone/pull/10953
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](https://github.com/kerneltime/ozone/tree/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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
