Add fix for local recovery from flush

[alchemist51]

Description

When a DataFormatAwareEngine restarts with committed data and no pending translog operations, the CatalogSnapshotManager would restore the committed snapshot but never notify listeners (reader managers). This meant the engine appeared healthy but couldn't serve search requests — the reader managers had no open readers.

Root cause: CatalogSnapshotManager's constructor restored the committed CatalogSnapshot from Lucene commit data but did not call afterRefresh() on registered listeners. Reader managers rely on this callback to open readers for the current snapshot.

Fix: After restoring the committed snapshot in CatalogSnapshotManager's constructor, iterate over all registered CatalogSnapshotLifecycleListeners and invoke afterRefresh(true, latestCatalogSnapshot) so reader managers are initialized immediately on engine open.

Test plan

• DataFormatAwareEngineRecoveryTests — 14 unit tests covering translog replay, partial recovery, concurrent indexing, double-restart, committed data restoration, and snapshot generation correctness
• CompositeLocalRecoveryIT — 5 integration tests with full cluster restart using parquet + lucene composite engine
• LocalRecoveryIT — 2 REST-level E2E tests verifying PPL query results are identical after index close/reopen (including force-merge scenario)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 69a2edf)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The fix notifies all listeners immediately in the constructor after restoring the committed snapshot. If a listener's afterRefresh implementation throws an exception, the constructor will fail and the engine will not start. This can happen if a listener is not yet fully initialized or if it encounters an error during reader manager initialization. The original code deferred listener notification until the first actual refresh, which provided a safer initialization sequence. for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { listener.afterRefresh(true, latestCatalogSnapshot); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 69a2edf

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Validate row sizes before column iteration The inner loop assumes both rows have identical sizes but only validates the beforeRows size. If afterRows.get(i) has fewer elements, this will throw an IndexOutOfBoundsException. Verify both row sizes match before iterating through columns. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/LocalRecoveryIT.java [136-142] +assertEquals("Row " + i + " column count mismatch", beforeRows.get(i).size(), afterRows.get(i).size()); for (int j = 0; j < beforeRows.get(i).size(); j++) { assertCellEquals( "Mismatch at row " + i + " col " + j, beforeRows.get(i).get(j), afterRows.get(i).get(j) ); } Suggestion importance[1-10]: 8 __ Why: This addresses a potential IndexOutOfBoundsException if afterRows.get(i) has fewer columns than beforeRows.get(i). The existing code at line 133-135 only checks row count equality but not column count, making this a valid bug prevention improvement.	Medium
	Add null check before listener notification The notification loop should only execute when latestCatalogSnapshot is non-null to prevent potential NPE or invalid state propagation. Add a null check before iterating through listeners to ensure committed snapshots exist before notifying reader managers. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] // Notify listeners about the committed snapshot so reader managers // are initialized on engine open. -for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); +if (latestCatalogSnapshot != null) { + for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { + listener.afterRefresh(true, latestCatalogSnapshot); + } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for latestCatalogSnapshot before notifying listeners is a defensive programming practice that prevents potential NPE. However, the code context suggests latestCatalogSnapshot is initialized from committedSnapshots which may already handle null cases, so the impact is moderate.	Medium

---

Previous suggestions

Suggestions up to commit 31d3e49

Category	Suggestion	Impact
Possible issue	Validate row sizes before element comparison The inner loop assumes both rows have the same size but only checks beforeRows.get(i).size(). If afterRows.get(i) has fewer elements, this will throw an IndexOutOfBoundsException. Verify both rows have equal size before iterating. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/LocalRecoveryIT.java [136-142] +assertEquals("Row sizes must match", beforeRows.get(i).size(), afterRows.get(i).size()); for (int j = 0; j < beforeRows.get(i).size(); j++) { assertCellEquals( "Mismatch at row " + i + " col " + j, beforeRows.get(i).get(j), afterRows.get(i).get(j) ); } Suggestion importance[1-10]: 8 __ Why: This is a valid bug fix. The code checks beforeRows.get(i).size() matches afterRows.get(i).size() at line 133-135, but only for the outer list size. The inner loop at lines 136-142 could throw IndexOutOfBoundsException if individual row sizes differ. Adding an explicit size check improves test robustness.	Medium
	Add null check before notifying listeners The notification loop should only execute when latestCatalogSnapshot is non-null to avoid notifying listeners with a null snapshot. This prevents potential NPEs in listener implementations that don't expect null snapshots during initialization. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] // Notify listeners about the committed snapshot so reader managers // are initialized on engine open. -for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); +if (latestCatalogSnapshot != null) { + for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { + listener.afterRefresh(true, latestCatalogSnapshot); + } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for latestCatalogSnapshot before notifying listeners is a valid defensive programming practice that prevents potential NPEs in listener implementations. However, the impact is moderate since the code context suggests this scenario may be handled elsewhere.	Medium
General	Log deserialization failures for debugging Silently swallowing the IOException during deserialization can hide corruption or format issues. Log the exception at minimum to aid debugging, or consider rethrowing as an unchecked exception if the committed data is critical for recovery. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [133-139] String serialized = committedData.get(CatalogSnapshot.CATALOG_SNAPSHOT_KEY); if (serialized != null) { try { this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } catch (IOException e) { - // Deserialization failed — start without committed snapshot + logger.warn("Failed to deserialize committed catalog snapshot", e); + // Start without committed snapshot } } Suggestion importance[1-10]: 5 __ Why: While logging the exception would improve debuggability, silently catching IOException during deserialization appears intentional in this test code context (as indicated by the comment). The suggestion is valid but has moderate impact since this is test infrastructure code where silent failures may be acceptable.	Low

Suggestions up to commit 2ce024b

Category	Suggestion	Impact
Possible issue	Verify column count matches before iteration The inner loop assumes both rows have the same size after verifying only beforeRows.get(i).size(). If afterRows.get(i) has fewer elements, this will throw an IndexOutOfBoundsException. Verify both row sizes match before iterating through columns. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/LocalRecoveryIT.java [136-142] +assertEquals( + "Row " + i + " column count mismatch", + beforeRows.get(i).size(), + afterRows.get(i).size() +); for (int j = 0; j < beforeRows.get(i).size(); j++) { assertCellEquals( "Mismatch at row " + i + " col " + j, beforeRows.get(i).get(j), afterRows.get(i).get(j) ); } Suggestion importance[1-10]: 8 __ Why: This is a valid bug fix. The code verifies beforeRows.get(i).size() equals afterRows.get(i).size() at line 133-135, but then accesses afterRows.get(i).get(j) without an explicit column count assertion. Adding an explicit check improves test robustness and error messaging.	Medium
	Add null check before notifying listeners The notification loop should only execute when latestCatalogSnapshot is not null to avoid notifying listeners with a null snapshot. This prevents potential NullPointerExceptions in listener implementations that don't handle null snapshots gracefully. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] // Notify listeners about the committed snapshot so reader managers // are initialized on engine open. -for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); +if (latestCatalogSnapshot != null) { + for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { + listener.afterRefresh(true, latestCatalogSnapshot); + } } Suggestion importance[1-10]: 7 __ Why: Adding a null check for latestCatalogSnapshot before notifying listeners is a defensive programming practice that prevents potential NullPointerExceptions. However, the impact depends on whether listener implementations handle null snapshots, which isn't clear from the PR context.	Medium
General	Propagate deserialization failures instead of suppressing Silently swallowing the IOException during deserialization can hide critical data corruption issues. Log the exception or rethrow it wrapped in a more specific exception to ensure deserialization failures are visible during testing and debugging. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [133-139] String serialized = committedData.get(CatalogSnapshot.CATALOG_SNAPSHOT_KEY); if (serialized != null) { try { this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } catch (IOException e) { - // Deserialization failed — start without committed snapshot + throw new RuntimeException("Failed to deserialize committed catalog snapshot", e); } } Suggestion importance[1-10]: 6 __ Why: In test code, silently catching IOException during deserialization can hide issues. However, the comment indicates this is intentional behavior ("start without committed snapshot"), and the same pattern appears at line 159-163. The suggestion improves visibility but may not align with the intended test semantics.	Low

Suggestions up to commit f13f3ba

Category	Suggestion	Impact
Possible issue	Add null check before listener notification The notification loop should only execute when latestCatalogSnapshot is non-null to avoid notifying listeners with a null snapshot. Add a null check before the loop to prevent potential NPE or incorrect initialization of reader managers when no committed snapshot exists. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] // Notify listeners about the committed snapshot so reader managers // are initialized on engine open. -for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); +if (latestCatalogSnapshot != null) { + for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { + listener.afterRefresh(true, latestCatalogSnapshot); + } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that latestCatalogSnapshot could be null and notifying listeners with a null snapshot may cause issues. Adding a null check prevents potential NPE and ensures reader managers are only initialized when valid committed data exists.	Medium
General	Synchronize shared state access The static boolean indexProvisioned is shared across all test instances and is not thread-safe. In concurrent test execution scenarios, multiple threads could bypass the check simultaneously, leading to race conditions. Consider using synchronization or making the flag instance-level with proper cleanup in teardown. sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/LocalRecoveryIT.java [33-37] -private void ensureIndexProvisioned() throws IOException { +private synchronized void ensureIndexProvisioned() throws IOException { if (indexProvisioned) { return; } ... indexProvisioned = true; } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a valid thread-safety concern with the static indexProvisioned flag. While synchronization would help, the impact is moderate since test frameworks typically don't run test methods concurrently within the same class instance by default.	Low
	Remove silent exception swallowing The constructor silently swallows IOException during catalog snapshot deserialization, which could mask critical data corruption issues. Consider logging the exception or rethrowing it as an initialization failure to ensure test failures are visible and debuggable. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [129-141] PersistentCommitter(Store store) throws IOException { this.store = store; this.committedData = Map.copyOf(store.readLastCommittedSegmentsInfo().getUserData()); // Deserialize existing catalog snapshot if present String serialized = committedData.get(CatalogSnapshot.CATALOG_SNAPSHOT_KEY); if (serialized != null) { - try { - this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); - } catch (IOException e) { - // Deserialization failed — start without committed snapshot - } + this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that silently swallowing IOException during deserialization could mask issues. However, the current behavior may be intentional for test resilience (starting fresh when committed data is corrupted), so the improvement is moderate rather than critical.	Low

Suggestions up to commit e7d23ce

Category	Suggestion	Impact
Possible issue	Add null check for catalog snapshot The notification loop assumes latestCatalogSnapshot is non-null, but if no committed snapshot exists during engine initialization, this could trigger a NullPointerException in listeners. Add a null check before notifying listeners to prevent crashes when starting an engine with no prior commits. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] -for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); +if (latestCatalogSnapshot != null) { + for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { + listener.afterRefresh(true, latestCatalogSnapshot); + } } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential NullPointerException when latestCatalogSnapshot is null during engine initialization. This is a critical safety issue that could cause engine startup failures.	Medium
General	Add null-safety for segment file maps The method doesn't handle the case where dfGroupedSearchableFiles() returns null or contains null values. If segments have no searchable files or the map contains null entries, this will throw a NullPointerException. Add null-safety checks to prevent crashes during snapshot inspection. sandbox/plugins/composite-engine/src/internalClusterTest/java/org/opensearch/composite/AbstractCompositeEngineIT.java [143-149] protected long getTotalRowCount(CatalogSnapshot snapshot) { return snapshot.getSegments() .stream() + .filter(s -> s.dfGroupedSearchableFiles() != null) .flatMap(s -> s.dfGroupedSearchableFiles().values().stream()) + .filter(wfs -> wfs != null) .mapToLong(org.opensearch.index.engine.exec.WriterFileSet::numRows) .sum(); } Suggestion importance[1-10]: 7 __ Why: The suggestion adds defensive null checks to prevent potential NullPointerException when processing segment files. While this improves robustness, the impact depends on whether the API contract guarantees non-null returns.	Medium
	Log deserialization failures for debugging Silently swallowing deserialization failures in the constructor can mask data corruption issues and make debugging difficult. Log the exception with context about the failure to aid troubleshooting while still allowing the engine to start. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [158-165] String serialized = committedData.get(CatalogSnapshot.CATALOG_SNAPSHOT_KEY); if (serialized != null) { try { this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } catch (IOException e) { - // Deserialization failed — start without committed snapshot + logger.warn("Failed to deserialize committed catalog snapshot, starting without it", e); } } Suggestion importance[1-10]: 5 __ Why: Adding logging for deserialization failures improves debuggability. However, this is test code (not production), and the impact is limited to development/testing scenarios. The suggestion is valid but has moderate importance.	Low

Suggestions up to commit 1118886

Category	Suggestion	Impact
General	Misleading commit flag during initialization The afterRefresh notification is called with isCommit=true unconditionally during initialization. This may mislead listeners into thinking a commit occurred when the engine is simply opening with existing committed data. Consider passing false or adding a dedicated initialization callback to distinguish between actual commits and engine startup. server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java [124-126] for (CatalogSnapshotLifecycleListener listener : snapshotListeners) { - listener.afterRefresh(true, latestCatalogSnapshot); + listener.afterRefresh(false, latestCatalogSnapshot); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that passing isCommit=true during initialization may be misleading to listeners. However, without understanding the full contract of afterRefresh, it's unclear if this is a bug or intentional design. The suggestion is valid but may require verification of listener expectations.	Medium
	Silent exception swallowing hides failures Silently catching and ignoring IOException during deserialization can hide critical data corruption issues. Consider logging the exception or rethrowing it as an unchecked exception to ensure deserialization failures are visible during testing and debugging. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [134-138] try { this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } catch (IOException e) { - // Deserialization failed — start without committed snapshot + throw new RuntimeException("Failed to deserialize catalog snapshot from commit data", e); } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that silently catching IOException in the constructor could hide deserialization failures. However, this is test code simulating production behavior, and the comment indicates intentional handling. Throwing an exception might be too strict for initialization scenarios where starting without a snapshot is acceptable.	Low
	Stale snapshot retained on failure The commit method silently ignores deserialization failures and keeps the previous snapshot, which could lead to stale or inconsistent state. This behavior may cause test assertions to pass incorrectly. Consider throwing an exception or at minimum logging the failure to make test failures more visible. server/src/test/java/org/opensearch/index/engine/DataFormatAwareEngineRecoveryTests.java [158-162] try { this.lastCommittedSnapshot = DataformatAwareCatalogSnapshot.deserializeFromString(serialized, dir -> dir); } catch (IOException e) { - // If deserialization fails, keep the previous snapshot + throw new RuntimeException("Failed to deserialize catalog snapshot during commit", e); } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential issue with keeping stale snapshots on deserialization failure in the commit method. However, this is test infrastructure code, and the behavior may be intentional for resilience. The impact is moderate since it could affect test reliability, but the current approach may be acceptable for test scenarios.	Low

[github-actions]

❌ Gradle check result for 1118886: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 1118886

[github-actions]

❌ Gradle check result for 1118886: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit e7d23ce

[github-actions]

❌ Gradle check result for e7d23ce: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit f13f3ba

[github-actions]

Persistent review updated to latest commit 2ce024b

[github-actions]

❌ Gradle check result for 2ce024b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 31d3e49

[github-actions]

❌ Gradle check result for 31d3e49: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 69a2edf

[github-actions]

✅ Gradle check result for 69a2edf: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.54%. Comparing base (446a1c9) to head (69a2edf).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21553      +/-   ##
============================================
+ Coverage     73.49%   73.54%   +0.05%     
- Complexity    74624    74662      +38     
============================================
  Files          5980     5980              
  Lines        338825   338828       +3     
  Branches      48857    48858       +1     
============================================
+ Hits         249010   249190     +180     
+ Misses        70041    69821     -220     
- Partials      19774    19817      +43     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.