Codecov Report

❌ Patch coverage is 51.28205% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.15%. Comparing base (abb9ef9) to head (ddeb9f8).
⚠️ Report is 6 commits behind head on master.

@@             Coverage Diff              @@
##             master   #17935      +/-   ##
============================================
+ Coverage     41.07%   41.15%   +0.08%     
  Complexity      318      318              
============================================
  Files          5257     5257              
  Lines        365010   365159     +149     
  Branches      47180    47207      +27     
============================================
+ Hits         149918   150277     +359     
+ Misses       215092   214882     -210     

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

Thanks for fixing the snapshot-load failure propagation. The main AddPeer control flow looks reasonable, and the new test covers the case where the target state machine reports load failure.

I found two concerns:

1. In IoTConsensusServerImpl.loadSnapshot(), the code now calls stateMachine.loadSnapshot() for every recvSnapshotDir and treats any false result as overall failure. However, snapshot fragments are written via FolderManager selection, so a given snapshot may not exist under every receive root. DataRegion also configures recvSnapshotDirs from all local data dirs, and SnapshotLoader fails when the given snapshot root does not exist. This can make a successful transfer fail in normal multi-data-dir deployments. We should either load only from the actual root that contains the received snapshot, or ensure all files of one snapshot are always placed under one selected root. A regression test with multiple recvSnapshotDirs and real snapshot files would help cover this.

2. ApplicationStateMachineProxy now logs when applicationStateMachine.loadSnapshot() returns false, but it still updates lastAppliedTermIndex afterward. That means Ratis may continue as if the snapshot was installed even though the application state machine rejected it. Since loadSnapshot now has an explicit success/failure contract, this path should avoid advancing the applied index, or fail initialization/reinitialization, when the load fails.

Thanks for the careful review, @Caideyipi! Both concerns are addressed:

1. Multi-data-dir false failure in IoTConsensusServerImpl.loadSnapshot (1c96b5b)

You're right. A DataRegion sets recvSnapshotDirs to all local data dirs (DataRegionConsensusImpl), and the FolderManager spreads snapshot fragments across them, so a given snapshot only materializes under the folders that actually received fragments — loading from a folder that never received this snapshot would (correctly) fail and turn a healthy transfer into a spurious failure. I went with the "load only from the roots that contain the received snapshot" option:

• Skip receive folders whose snapshot root does not exist, load from the ones that do, and still report failure if a present snapshot fails to load or if no folder received the snapshot at all.
• The actual complete load happens from whichever root holds the snapshot log, since SnapshotLoader#createLinksFromSnapshotDirToDataDirWithLog gathers the fragments across all data dirs and verifies the logged file count, so skipping the empty roots is safe.

I also added the regression test you suggested — AddPeerSnapshotLoadFailureTest#addRemotePeerSucceedsWhenSnapshotSpansSubsetOfRecvDirs — which builds a two-node group with multiple recvSnapshotDirs, transfers a real snapshot that lands in only a subset of the target's receive folders, and asserts AddPeer still succeeds. I verified it fails against the previous load-from-every-folder behavior and passes with the fix.

2. ApplicationStateMachineProxy advancing the applied index on load failure (1c96b5b)

loadSnapshot now throws IOException when the application state machine returns false, so lastAppliedTermIndex is no longer advanced and initialize/reinitialize fail instead of reporting the snapshot as applied. reinitialize() now declares throws IOException, which the Ratis StateMachine interface already permits.

Separately, Copilot flagged that ConfigRegionStateMachine.loadSnapshot always returned true because ConfigPlanExecutor.loadSnapshot was void and swallowed failures — fixed in 7fe8e9c by making it return a boolean and propagating it.

All consensus tests pass (AddPeerSnapshotLoadFailureTest, ReplicateTest/StabilityTest, RatisConsensusTest/RecoverReadTest/SnapshotTest).

I found one remaining contract gap in the snapshot-load failure propagation.

SchemaRegionStateMachine.loadSnapshot(...) now returns rue after calling schemaRegion.loadSnapshot(...), but the underlying ISchemaRegion implementations still swallow load failures:

• SchemaRegionPBTreeImpl.loadSnapshot(...) catches IOException | MetadataException, logs FAILED_TO_LOAD_SNAPSHOT, tries init(), and then returns normally.
• SchemaRegionMemoryImpl.loadSnapshot(...) has the same pattern for Exception.

Because those methods are still �oid and do not rethrow or return a result, SchemaRegionStateMachine.loadSnapshot(...) can report success even when the schema snapshot was not actually loaded. This weakens the new IStateMachine.loadSnapshot boolean contract. It also matters for Ratis, since ApplicationStateMachineProxy now relies on a alse return to avoid advancing lastAppliedTermIndex after a rejected snapshot.

Suggested fix: make the schema-region load path report success/failure as well, or make SchemaRegionStateMachine.loadSnapshot(...) verify that the underlying schema snapshot was actually loaded before returning rue. A focused regression test around a failing schema snapshot load would help keep this contract from regressing.

Good catch — fixed in ddeb9f8.

You're right: SchemaRegionStateMachine.loadSnapshot(...) returned true unconditionally because the underlying load swallowed failures. Both SchemaRegionPBTreeImpl.loadSnapshot(...) (catching IOException | MetadataException) and SchemaRegionMemoryImpl.loadSnapshot(...) (catching Exception) logged FAILED_TO_LOAD_SNAPSHOT, fell back to init(), and returned void, so a failed schema-snapshot load was reported as success — weakening the new IStateMachine.loadSnapshot contract and, as you note, allowing the Ratis path to advance lastAppliedTermIndex after a rejected snapshot.

I made the schema-region load path report success/failure:

• ISchemaRegion.loadSnapshot(...) now returns boolean (false when the load failed and the region fell back to an empty re-initialized state).
• Both SchemaRegionPBTreeImpl and SchemaRegionMemoryImpl return true on the success path and false from the fallback/catch path.
• SchemaRegionStateMachine.loadSnapshot(...) propagates the data-load result, while keeping the pipe-listener recomputation (schemaListener(...).loadSnapshot + listen2Snapshot4PipeListener) as non-fatal post-processing (consistent with how I scoped the ConfigRegion fix).

Added a focused regression test — SchemaRegionManagementTest#testLoadSnapshotReportsFailureWhenSnapshotIsMissing — which asserts that loading from a directory without a snapshot reports failure rather than silently succeeding. I also added assertTrue(...) to the existing successful-load assertions in testRatisModeSnapshot/testEmptySnapshot. The new test was verified to fail across all four schema-region modes (MemoryMode, PBTree-Full/Partial/Non-Memory) against the previous always-true behavior, and passes with the fix.

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud