Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review: fix(#4754) StateMachineUpdater.reload() after snapshot install

Thanks for the thorough write-up and precise root-cause analysis. The lifecycle reasoning is sound: the NEW->STARTING->RUNNING start in initialize(), the RUNNING->PAUSING->PAUSED in pause(), and the PAUSED->STARTING->RUNNING resume in reinitialize() all match Ratis's LifeCycle valid-transition table, and the idempotent pause() guard is a nice touch. Wiring updateLatestSnapshot(...) plus returning getIndex()-1 as the snapshot TermIndex also looks correct against the Ratis SnapshotInstallationHandler/reload() contract.

A few things worth addressing:

1. Docs claim a test change that isn't in this PR (medium). docs/4754-...md says: 'Updated test: RaftFullSnapshotResyncIT.java - Now enabled - tests the full 3-node path'. But this PR does NOT touch RaftFullSnapshotResyncIT.java, and that test was already enabled (not @disabled) on main before this PR. The PR body's test plan instead cites Issue4749SnapshotInstallClosesRegisteredDatabaseIT. The docs and the PR body disagree. Please either include the IT change you intended, or correct the docs so they describe what actually shipped.

2. Unit test doesn't actually cover initialize() (medium). ArcadeStateMachineLifecycleTest manually drives getLifeCycle().transition(STARTING)/transition(RUNNING) to simulate initialize(), rather than invoking initialize(). So the core fix (that initialize() starts the lifecycle) is never asserted; a regression deleting those two transition lines from initialize() would leave all three tests green. The real end-to-end coverage rests on RaftFullSnapshotResyncIT, which is an IT (not run in the unit phase) and is unchanged here. Worth either asserting initialize()'s post-condition directly, or confirming the IT genuinely exercises this path in CI.

3. Snapshot term may be paired with the wrong index (minor). snapshotIndex = firstTermIndexInLog.getIndex() - 1 but snapshotTerm = firstTermIndexInLog.getTerm(). firstTermIndexInLog is the first entry AFTER the snapshot, so its term is that entry's term, yet it's attached to firstAvailable-1, which may belong to an earlier term. The resulting (term, index) pair is propagated to updateLatestSnapshot, updateLastAppliedTermIndex, and writePersistedAppliedIndex. Likely benign since the real data is reconciled from the leader, but the pairing is technically inconsistent; a short justifying comment (or deriving the correct term) would help.

4. new FileInfo(snapshotFile.toPath(), null) passes a null digest (minor). Confirm no Ratis path (e.g. getLatestSnapshot() consumers, future snapshot verification) ever tries to validate the checksum of this empty marker file; a null FileDigest could surface later as an NPE/verification failure on a path not exercised by the current tests.

5. Ignored return values (minor/style). snapshotFile.getParentFile().mkdirs() and snapshotFile.createNewFile() ignore their boolean results. The createNewFile() IOException still propagates and fails the install loudly (good), but a failed mkdirs() is silent; consider checking/logging.

6. Check-then-act on the lifecycle (minor). pause() and reinitialize() read getLifeCycleState() then call transition(...) non-atomically. Safe today because Ratis serializes install/reload on a single thread, but if that ever changes, transition() would throw an uncaught IllegalStateException. The LifeCycle atomic helpers (e.g. compareAndTransition) would be more robust.

Nit: pause() doesn't call super.pause(). Fine while it's a no-op, but a future Ratis version could add behavior there.

Overall a well-targeted fix with good inline documentation. Items 1 and 2 are the ones I'd most like to see resolved before merge, since they affect whether the change is actually verified end-to-end.

PR Review: LifeCycle transitions for StateMachineUpdater.reload() after snapshot install

Thanks for the detailed write-up - the root-cause analysis (no-op BaseStateMachine.pause() leaving the lifecycle in NEW, and the wrong return value / missing storage update in notifyInstallSnapshotFromLeader) is clear and the fix is well targeted. The lifecycle transitions themselves (NEW->STARTING->RUNNING, RUNNING->PAUSING->PAUSED, PAUSED->STARTING->RUNNING) are all valid Ratis LifeCycle edges, so the contract StateMachineUpdater.reload() expects is now satisfied. A few things worth a look before merge:

Correctness / potential bugs

1. Asymmetric single-flight guard on snapshotDownloadInProgress (race). notifyInstallSnapshotFromLeader now does snapshotDownloadInProgress.set(true) (unconditional) with finally { ...set(false) }, while triggerSnapshotDownload uses compareAndSet(false, true). The comment claims this prevents a concurrent install, and it does in one ordering, but not the other:

   • If triggerSnapshotDownload already won the CAS and is mid-install, notify will still set(true) (no-op) and run reconcileDatabasesFromLeader concurrently with the in-flight SnapshotInstaller.install() on the same DB files - the exact race the comment says is prevented.
   • Worse, when notify's finally runs set(false), it clears the flag the still-running triggerSnapshotDownload "owns", allowing a third path to CAS true and start yet another concurrent download.

   Ratis serializes the notify path, but the watchdog / HealthMonitor paths fire independently, so this window is real (though narrow). Consider having notify participate in the same CAS protocol (only set(false) in finally if it actually acquired the flag), or otherwise serialize the two entry points.

2. Empty marker file left in the SimpleStateMachineStorage snapshot dir. You createNewFile() an empty file named via storage.getSnapshotFile(term, index) and register it through updateLatestSnapshot. updateLatestSnapshot only sets the in-memory reference, but on the next restart SimpleStateMachineStorage.getLatestSnapshot() re-scans that directory and parses files by name (and may attempt to read/verify them). Worth confirming an empty, digest-less file there does not trip recovery (e.g. an md5 read or chunk verification) on a subsequent restart. If it can, a short note or a follow-up cleanup is warranted.

3. close() doesn't transition the lifecycle. Now that initialize() drives the lifecycle to RUNNING/PAUSED, close() only shuts the executor and calls super.close(), leaving the lifecycle non-CLOSED. Probably benign (BaseStateMachine does not assert here), but a CLOSING->CLOSED transition would keep the state consistent and avoid surprises if Ratis ever inspects it.

Style / minor

• pause() reads getLifeCycleState() then calls transition() in two steps - fine because Ratis calls it single-threaded, but if you want to be defensive against an unexpected concurrent transition, LifeCycle offers compare-and-transition helpers.
• parentDir.mkdirs() return value is ignored; if it fails, createNewFile() throws IOException (caught), so behavior is acceptable - just noting it relies on the later failure.
• The snapshot-term choice (firstTermIndexInLog.getTerm() as an upper bound) is well documented and only affects metadata - good call-out in the comment.

Test coverage

• The new ArcadeStateMachineLifecycleTest cleanly nails the lifecycle contract (pause -> PAUSED, reinitialize -> RUNNING, idempotent pause, initialize via a proxy RaftServer). Nice.
• Gap: none of the notifyInstallSnapshotFromLeader changes are unit-covered - the TermIndex math (getIndex()-1, the Math.max(0,...) floor), the storage updateLatestSnapshot write, and the applied-index advancement all rely solely on the IT. Given the bug history here, a focused unit test asserting the returned TermIndex and that getLatestSnapshot() is non-null after the call would be valuable and cheap.
• Doc/PR-description mismatch: the PR body says "3 tests" but the test class and docs/...md say 4. Worth aligning.

Overall a solid, well-reasoned fix for an unrecoverable-zombie scenario. The race in point (1) is the main item I'd want resolved (or explicitly argued safe) before merge.

Code Review - PR #4755: LifeCycle transitions for StateMachineUpdater.reload()

Thanks for the detailed write-up - the root-cause analysis (no-op BaseStateMachine.pause() leaving the lifecycle in NEW, so the getLifeCycleState() == PAUSED precondition in StateMachineUpdater.reload() throws and zombifies the follower) is clear and the fix is well targeted. The inline comments and the docs/ note make the intent easy to follow. A few things worth a closer look before merge.

1. pause() override must be paired with a resume on every path (most important)

The override now performs a real RUNNING -> PAUSING -> PAUSED transition, but the only place that transitions back to RUNNING is reinitialize(), which Ratis only calls from reload(). So the correctness of this change rests entirely on the invariant:

Ratis never calls StateMachine.pause() except immediately before a reload() that calls reinitialize().

If Ratis 3.2.2 calls pause() on any other path (e.g. some snapshot/takeSnapshot or shutdown bookkeeping flow) that does not lead to reinitialize(), the state machine gets stuck in PAUSED permanently - arguably worse than the current bug because it is silent. The previous no-op pause() was tolerant of being called anywhere. Could you confirm (ideally cite the call sites in SnapshotInstallationHandler / RaftServerImpl) that pause() in 3.2.2 is only ever invoked on the reload path? If it can be called elsewhere, consider resuming more defensively rather than only inside reinitialize().

2. The single-flight comment overstates the ordering guarantee

In notifyInstallSnapshotFromLeader, when the CAS loses (a triggerSnapshotDownload() is already running), the method still proceeds to reconciler.reconcileDatabasesFromLeader(...). The comment claims the in-flight watchdog download "will have installed current data before we call reconcileDatabasesFromLeader()" - but nothing synchronizes the two; both can be installing/swapping the same databases concurrently. This was already possible before this PR (the old code never touched the flag), so it is not a regression, but the comment reads as if ordering is guaranteed when it is not. I'd soften the comment, or better, have the notify path wait for / short-circuit on the in-flight download instead of racing it.

3. Snapshot term is approximated across a possible term boundary

snapshotTerm = firstTermIndexInLog.getTerm() is used as the term for the entry at getIndex() - 1. If a term boundary falls exactly there, the recorded term is off. You've documented that this only feeds the marker-file name and Ratis snapshot-index metadata, which is fair - but worth confirming Ratis never compares this term against the log when deciding what to purge in reloadStateMachine(...), since a mismatch there could affect log truncation.

4. Ignored mkdirs() / createNewFile() return values

if (parentDir != null) parentDir.mkdirs();
if (!snapshotFile.exists()) snapshotFile.createNewFile();

Both return booleans that are dropped. If either fails (permissions, disk full), storage.updateLatestSnapshot(...) registers a SingleFileSnapshotInfo pointing at a file that does not exist, and the failure only surfaces later and confusingly. Consider checking the results and failing fast, consistent with the SEVERE handling already in the catch block.

5. Test coverage gap on the notifyInstallSnapshotFromLeader changes

ArcadeStateMachineLifecycleTest nicely locks in the pause()/reinitialize() contract. But the other half of this PR - the Math.max(0, getIndex()-1) index computation, the marker-file creation, the updateLatestSnapshot(...) call, and the CAS-flag acquire/finally-clear semantics - has no direct unit test; it relies solely on the existing IT. The index math (the off-by-one was the prior bug) and the flag ownership logic are exactly what a focused unit test should pin down. Worth adding one even if it needs small storage stubbing.

6. Minor

• Em dash in committed code/docs: ArcadeStateMachine.java:731 and :746 (and the new docs/ file) use the em-dash character. CLAUDE.md asks to avoid it; use a normal dash.
• Doc/PR count drift: the PR description says "3 tests" and "151 tests", while docs/4754-...md says "4 tests" and "153 tests". The test file actually has 4 methods. Worth reconciling.
• Lifecycle start ordering in initialize(): the STARTING -> RUNNING transitions happen before storage.init() / reinitialize(). If storage.init() throws, the lifecycle is left RUNNING on a half-initialized SM. Minor, but transitioning only after storage/reinit succeeds would be cleaner.

Summary

Solid, well-documented fix for a real zombie-follower bug, and the lifecycle test is a good regression guard. The blocking question is #1 - please verify the pause()-without-reinitialize() invariant holds in Ratis 3.2.2, since the whole fix depends on it. #4 and #5 are low-effort hardening I'd recommend before merge; #2/#3/#6 are clarifications/nits.

Review by Claude (Opus 4.8). I could not unpack the Ratis 3.2.2 jar in this environment to confirm the pause() call sites myself, so please treat #1 and #3 as verification requests rather than confirmed defects.

Code Review - PR 4755

Reviewed the lifecycle fix and the notifyInstallSnapshotFromLeader rework. Overall this is a well-targeted, carefully documented fix for a real and nasty failure mode (zombie follower after snapshot install). The root-cause analysis is convincing and the inline comments are excellent. A few points worth considering before merge.

Correctness / potential issues

1. close() no longer transitions the lifecycle out of RUNNING/PAUSED. Before this PR the lifecycle stayed in NEW for the whole life of the object; now initialize() drives it to RUNNING. ArcadeStateMachine.close() only does lifecycleExecutor.shutdownNow() + super.close(), and BaseStateMachine.close() does not move the lifecycle. So a closed state machine is left in RUNNING (or PAUSED). Harmless today since nothing asserts CLOSED, but it is an inconsistency that could bite a future caller. Consider transitioning RUNNING/PAUSING/PAUSED -> CLOSING -> CLOSED in close() (guarded, like pause() is) to keep the lifecycle honest.

2. Snapshot term is the term of the entry AFTER the snapshot, not at the snapshot index. snapshotTerm = firstTermIndexInLog.getTerm() is the term of index snapshotIndex+1. The reported snapshot TermIndex becomes (termOf(snapshotIndex+1), snapshotIndex). In the common case (no term boundary exactly between snapshotIndex and snapshotIndex+1) this equals the true term at snapshotIndex, but if a leader change landed exactly at that boundary the reported term is off by a term. The comment frames this as a safe upper bound that does not affect data correctness - true for ArcadeDBs HTTP file-shipping data path, but the value is also what Ratis stores as lastIncludedTerm and uses for the AppendEntries consistency check at prevLogIndex = snapshotIndex. Worth confirming this cannot cause a reject/decrement loop on the next heartbeat in the term-boundary case. If firstTermIndexInLog can expose the prior entrys term, that would be the more correct value to store.

3. Residual concurrent-install race when the CAS is lost. When compareAndSet(false, true) fails, the method still proceeds to reconcileDatabasesFromLeader() while another triggerSnapshotDownload() is installing the SAME database files concurrently. The comment acknowledges this and leans on SnapshotInstaller being crash-safe with atomic directory swaps, but two installs racing on the same target/temp staging is a stronger claim than crash-safety. Since notifyInstallSnapshotFromLeader must return a valid TermIndex (it cannot simply skip), a follow-up that serializes on a per-database mutex or awaits the in-flight download would be safer than running both. At minimum this residual race deserves a tracked issue rather than only an inline comment.

4. Applied-index monotonicity. lastAppliedIndex.set(snapshotIndex) + updateLastAppliedTermIndex(snapshotTerm, snapshotIndex) assume the snapshot point is at or ahead of the current applied index. For a lagging follower that holds, but if an install is ever triggered when local apply is already past snapshotIndex, BaseStateMachine.updateLastAppliedTermIndex can assert on a non-monotonic update. A guard (only advance if snapshotIndex > current) would make this robust.

Test coverage

• The new ArcadeStateMachineLifecycleTest nicely pins the lifecycle contract, and initializeStartsLifecycleToRunning closes the obvious regression gap in initialize() itself. Good.
• However, the most bug-prone new code - the notifyInstallSnapshotFromLeader metadata registration (term/index computation, storage.updateLatestSnapshot, marker-file creation, the CAS finally block) - has no unit coverage. The PR relies on the existing IT end-to-end. A focused unit test driving notifyInstallSnapshotFromLeader against a real SimpleStateMachineStorage and asserting (a) getLatestSnapshot() is non-null with the expected (term, index), (b) the returned TermIndex equals firstTermIndexInLog.getIndex()-1, and (c) the flag is cleared in finally only when acquired, would protect exactly what this PR changes. The firstTermIndexInLog.getIndex() == 0 -> Math.max(0, -1) edge is also untested.

Minor / nits

• snapshotFile.createNewFile() return value is ignored (you do check mkdirs()). Low risk since IOException is caught, but logging a false return would be consistent with the mkdirs() handling.
• pause() logging an unexpected state and silently skipping is reasonable, but if it ever fires the subsequent StateMachineUpdater.reload() precondition still throws - so that WARNING is the only breadcrumb before a hard failure. Just flagging that the WARNING is load-bearing for diagnosis.
• Keep the doc test counts (docs/4754-...md) in sync if more tests are added later.

Style / conventions

Adheres to CLAUDE.md: final used throughout, classes imported (no FQNs), no em-dashes. The HA concurrency rules note notifyInstallSnapshotFromLeader already runs on the JDK common pool with a NOTE (concurrency) tag; this PR does not change that, which is consistent.

Nice work on the analysis and documentation density. The lifecycle fix itself looks correct; my main asks are (2) the term-boundary question and (3)/(4) hardening the concurrent-install and monotonicity edges, plus a unit test around the snapshot-metadata path.

Generated with Claude Code

PR Review: fix(#4754) LifeCycle transitions for StateMachineUpdater.reload()

Thorough, well-documented fix. The root-cause analysis (no-op BaseStateMachine.pause() leaving the lifecycle in NEW, so Ratis's reload() precondition getLifeCycleState() == PAUSED throws and zombifies the follower) is convincing, and the three-part lifecycle fix (initialize start, pause override, reinitialize resume) matches the contract Ratis drives. Code style follows CLAUDE.md (braceless single-statement ifs, final params/locals, no System.out). A few items below.

Test coverage (main gap)

• The new ArcadeStateMachineLifecycleTest only exercises the pause() / reinitialize() lifecycle contract. The riskier changes in notifyInstallSnapshotFromLeader() have no direct test: the snapshot TermIndex computation (getIndex()-1, Math.max(0L, …)), the marker-file creation + storage.updateLatestSnapshot(...), the advance of lastAppliedIndex/writePersistedAppliedIndex, and the CAS single-flight finally. These are exactly the parts most likely to regress (filesystem + return-value correctness). Consider a test that invokes notifyInstallSnapshotFromLeader (with a stubbed reconciler) and asserts getLatestSnapshot() is non-null and the returned TermIndex equals (term, firstIndex-1).
• Minor: the PR body says "3 tests" but the file has 4 test methods (and the docs file says 4). Worth reconciling.

Correctness / robustness

1. pause() in an unexpected state doesn't actually prevent the crash. When current is neither RUNNING nor PAUSED (e.g. NEW), the method logs a WARNING and skips the transition - but StateMachineUpdater.reload() then still asserts == PAUSED and throws IllegalStateException, reproducing the very failure this PR fixes (just with a log line first). The javadoc claims this avoids "crashing the caller", which is only true for the already-PAUSED case. Please soften the javadoc so it isn't read as a guarantee.

2. Snapshot term is an upper bound (firstTermIndexInLog.getTerm()). The true term of the entry at snapshotIndex = firstIndex-1 can be lower than the first-remaining entry's term if a term boundary sits exactly there. The comment asserts this term only feeds file naming / metadata. Please double-check that reloadStateMachine(...) / log-purge in Ratis 3.2.2 never uses this term for log matching - if it does, an inflated term could mis-purge. Low risk, but it's the one value here that is a deliberate approximation.

3. Residual concurrent-install race (already documented). If the CAS loses (acquiredSnapshotFlag == false), the method still runs reconcileDatabasesFromLeader() concurrently with the in-flight triggerSnapshotDownload(), both writing the same DB directories. You rely on SnapshotInstaller's atomic swap, but two installs racing on the same target can still interleave swaps. The win/lose ownership of the finally clear is correct; the deferral is reasonable - just make sure a follow-up issue tracks closing this rather than leaving it only in a code comment.

4. Empty marker file persists across restarts. loadLatestSnapshot() will rediscover this zero-byte file. The comment argues ArcadeDB never uses Ratis chunk verification, so it's safe. That holds only while the notification-based install path is the only one in use; if a config ever enables the chunk-based path, streaming an empty snapshot to a peer would be wrong. Worth a guard or at least a pointer to where that assumption is enforced.

Minor

• snapshotFile.createNewFile() return value is ignored (acceptable - guarded by exists() and IOException flows to the outer catch).
• The docs/4754-…md file embeds a "Review Cycles" table (SHAs, bot outcomes, "TIMEOUT") and a "Final State" note. That's process metadata, not durable design docs - consider trimming those sections before merge so the file stays useful to future readers.

Performance / security

No concerns. The added work (one mkdirs + empty-file create per snapshot install) runs on a rare, already-async path. No sensitive data logged.

Overall: solid, narrowly-scoped fix with good inline rationale. The chief ask is direct test coverage for the notifyInstallSnapshotFromLeader changes, plus the small pause() javadoc clarification.

Codecov Report

❌ Patch coverage is 32.25806% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.25%. Comparing base (23b1350) to head (e2c957c).
⚠️ Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main    #4755      +/-   ##
============================================
- Coverage     65.26%   65.25%   -0.01%     
  Complexity      635      635              
============================================
  Files          1675     1675              
  Lines        132613   132642      +29     
  Branches      28366    28372       +6     
============================================
+ Hits          86549    86556       +7     
- Misses        34138    34160      +22     
  Partials      11926    11926              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.