Add foundational classes for storage support for multi-format data support

[ask-kamal-nayan]

Description

This PR introduces the foundational storage layer classes and interfaces required for composite data format support. It establishes the directory abstractions, metadata
structures, and remote store extensions that allow files from multiple data formats to coexist within a single shard — each stored in its own subdirectory and routed
through format-aware directory implementations.

This is the first in a series of PRs. Subsequent PRs will add the composite indexing engine, data format plugin integration, and end-to-end read/write paths.

This is the first in a series of PRs building out multi-format storage support.

Changes

New Classes

• FileMetadata — Encapsulates a file's data format and name, enabling format-aware file identification across the storage layer.
• CompositeStoreDirectory — Format-aware local directory that delegates path routing to SubdirectoryAwareDirectory and adds format-specific checksum calculation (
  CodecUtil for Lucene, CRC32 for others).
• CompositeRemoteDirectory — Extends RemoteDirectory with per-format BlobContainer routing for remote segment uploads/downloads.
• SubdirectoryAwareDirectory — Lucene FilterDirectory that routes file operations across subdirectories within the shard data path (extracted from
  SubdirectoryAwareStore inner class to server for reuse).
• CompositeEngineCatalogSnapshot / SegmentInfosCatalogSnapshot — Catalog snapshot implementations for composite engine and standard Lucene segments respectively.
• MetadataFilenameUtils — Extracted from RemoteSegmentStoreDirectory inner class to a top-level utility class.
• UploadedSegmentMetadata — Extracted from RemoteSegmentStoreDirectory inner class to a standalone class.

Modified Classes

• CatalogSnapshot — Added Writeable and Cloneable support for serialization.
• Segment — Added getDFGroupedSearchableFiles(), getGeneration(), and writeTo() to support composite catalog snapshots.
• StoreFileMetadata — Added dataFormat field to track which format a file belongs to (defaults to "lucene").
  imports.
• RemoteDirectory — Added deleteFile(UploadedSegmentMetadata) overload.
• RemoteSegmentMetadata — Updated to support composite format metadata.
• SubdirectoryAwareStore — Removed inner SubdirectoryAwareDirectory class, now imports from server.

Testing

This PR introduces foundational classes and interfaces. Tests will follow in subsequent PRs as the composite engine integration is built out.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 26730f3.

Path	Line	Severity	Description
sandbox/libs/dataformat-native/rust/Cargo.toml	51	high	New Rust dependency added: crc32fast = "1.4". Per mandatory supply chain rule, all dependency additions must be flagged regardless of apparent legitimacy. Maintainers should verify the crate name, version, and source against the official crates.io registry to rule out typosquatting or namespace hijacking.
sandbox/plugins/parquet-data-format/src/main/rust/Cargo.toml	28	high	New Rust dependency added: crc32fast = { workspace = true } (resolves from workspace Cargo.toml to version 1.4). Per mandatory supply chain rule, all dependency additions must be flagged. Maintainers should verify this resolves to the same vetted crate as the workspace-level addition.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 2 | Medium: 0 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b6ff89e)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Local composite store directory abstractions and checksum handlers Relevant files: server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectory.java server/src/main/java/org/opensearch/index/store/SubdirectoryAwareDirectory.java server/src/main/java/org/opensearch/index/store/FileMetadata.java server/src/main/java/org/opensearch/index/store/FormatChecksumStrategy.java server/src/main/java/org/opensearch/index/store/PrecomputedChecksumStrategy.java server/src/main/java/org/opensearch/index/store/DefaultDataFormatAwareStoreDirectoryFactory.java server/src/main/java/org/opensearch/index/store/DataFormatAwareStoreDirectoryFactory.java server/src/main/java/org/opensearch/index/store/checksum/GenericCRC32ChecksumHandler.java server/src/main/java/org/opensearch/index/store/checksum/LuceneChecksumHandler.java server/src/test/java/org/opensearch/index/store/DataFormatAwareStoreDirectoryTests.java server/src/test/java/org/opensearch/index/store/FileMetadataTests.java server/src/test/java/org/opensearch/index/store/checksum/ChecksumHandlerTests.java server/src/test/java/org/opensearch/index/store/DefaultDataFormatAwareStoreDirectoryFactoryTests.java Sub-PR theme: Remote composite directory with format-aware blob routing Relevant files: server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java server/src/main/java/org/opensearch/index/store/remote/FormatBlobRouter.java server/src/test/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectoryTests.java server/src/test/java/org/opensearch/index/store/remote/FormatBlobRouterTests.java Sub-PR theme: CatalogSnapshot integration in remote store refresh and metadata upload Relevant files: server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java server/src/main/java/org/opensearch/index/engine/exec/coord/SegmentInfosCatalogSnapshot.java

⚡ Recommended focus areas for review Null Safety In getChecksumOfLocalFile, DataFormatAwareStoreDirectory.unwrap(storeDirectory) may return null and throws an IllegalStateException. However, the caller path should be validated to ensure isPluggableDataFormatEnabled() is only true when the store is actually a DataFormatAwareStoreDirectory. If there is any code path where the setting is enabled but the directory is not wrapped, this will throw at runtime during segment upload. if (indexShard.indexSettings().isPluggableDataFormatEnabled()) { DataFormatAwareStoreDirectory dfasd = DataFormatAwareStoreDirectory.unwrap(storeDirectory); if (dfasd == null) { throw new IllegalStateException("DataFormatAwareStoreDirectory expected when pluggable data format is enabled"); } String checksum = dfasd.calculateUploadChecksum(file); localSegmentChecksumMap.put(file, checksum); return checksum; } CatalogSnapshot Lifecycle catalogSnapshot.cloneNoAcquire() is called in uploadMetadata and the clone is used after the try-with-resources block that owns the original catalogSnapshotRef has closed. If cloneNoAcquire does not independently manage its own lifecycle/resources, the cloned snapshot may reference freed or invalid state after the original is closed. CatalogSnapshot catalogSnapshotCloned = catalogSnapshot.cloneNoAcquire(); Map<String, String> userData = new HashMap<>(catalogSnapshotCloned.getUserData()); userData.put(LOCAL_CHECKPOINT_KEY, String.valueOf(maxSeqNo)); userData.put(SequenceNumbers.MAX_SEQ_NO, Long.toString(maxSeqNo)); catalogSnapshotCloned.setUserData(userData, false); Incomplete Test Assertion testDeleteFiles_BatchDelete_DeletesFromAllContainers verifies that baseBlobContainer is called times(2) with the full list, but the comment says "baseBlobContainer is called from super.deleteFiles + lucene format container (same instance)". This assumption about internal implementation details (that super.deleteFiles calls deleteBlobsIgnoringIfNotExists once) is fragile and may break if the parent class implementation changes. public void testDeleteFiles_BatchDelete_DeletesFromAllContainers() throws IOException { List<String> names = List.of("_0.cfs__UUID1", "_0.parquet__UUID2"); directory.deleteFiles(names); // baseBlobContainer is called from super.deleteFiles + lucene format container (same instance) verify(baseBlobContainer, times(2)).deleteBlobsIgnoringIfNotExists(names); verify(parquetBlobContainer).deleteBlobsIgnoringIfNotExists(names); } Misleading Test testAsyncCopyFrom_ExceptionDuringUpload_CallsListenerOnFailure asserts assertTrue("Should return true (handled)", result) but the comment says "File does not exist - openInput will throw". If the exception occurs before the async container is even invoked (i.e., during file open), the return value of true may not actually indicate the async path was taken. The test may be asserting incorrect behavior. public void testAsyncCopyFrom_ExceptionDuringUpload_CallsListenerOnFailure() throws Exception { AsyncMultiStreamBlobContainer asyncContainer = mock(AsyncMultiStreamBlobContainer.class); when(asyncContainer.remoteIntegrityCheckSupported()).thenReturn(false); when(asyncContainer.path()).thenReturn(baseBlobPath); BlobStore asyncBlobStore = mock(BlobStore.class); when(asyncBlobStore.blobContainer(baseBlobPath)).thenReturn(asyncContainer); DataFormatAwareRemoteDirectory asyncDir = new DataFormatAwareRemoteDirectory( asyncBlobStore, baseBlobPath, UnaryOperator.identity(), UnaryOperator.identity(), UnaryOperator.identity(), UnaryOperator.identity(), new HashMap<>(), logger, null, null ); // File does not exist - openInput will throw Directory storeDirectory = newDirectory(); CountDownLatch latch = new CountDownLatch(1); AtomicReference<Exception> failureRef = new AtomicReference<>(); boolean result = asyncDir.copyFrom( storeDirectory, "_nonexistent.si", "_nonexistent.si__UUID", IOContext.DEFAULT, () -> {}, new ActionListener<>() { @Override public void onResponse(Void unused) { fail("Should have failed"); } @Override public void onFailure(Exception e) { failureRef.set(e); latch.countDown(); } }, false, null ); assertTrue("Should return true (handled)", result); assertTrue(latch.await(10, TimeUnit.SECONDS)); assertNotNull(failureRef.get()); storeDirectory.close(); } Missing Negative Test There are no tests for cross-format rename (e.g., renaming a lucene file to a parquet file or vice versa). If rename does not validate that source and destination formats match, it could silently move a file to the wrong subdirectory. A test verifying that cross-format rename either fails or behaves correctly should be added. public void testRename_sameFormat() throws IOException { String fileName = "_rename_src.si"; try (IndexOutput out = dataFormatAwareStoreDirectory.createOutput(fileName, IOContext.DEFAULT)) { out.writeString("rename test data"); } dataFormatAwareStoreDirectory.rename(fileName, "_rename_dest.si"); assertFalse(Arrays.asList(dataFormatAwareStoreDirectory.listAll()).contains(fileName)); assertTrue(Arrays.asList(dataFormatAwareStoreDirectory.listAll()).contains("_rename_dest.si")); } public void testRename_fileMetadata_sameFormat() throws IOException { String fileIdentifier = "parquet/rename_src.parquet"; try (IndexOutput out = dataFormatAwareStoreDirectory.createOutput(fileIdentifier, IOContext.DEFAULT)) { out.writeString("rename test data parquet"); } FileMetadata src = new FileMetadata("parquet", "rename_src.parquet"); FileMetadata dest = new FileMetadata("parquet", "rename_dest.parquet"); dataFormatAwareStoreDirectory.rename(src.serialize(), dest.serialize()); String srcSerialized = new FileMetadata("parquet", "rename_src.parquet").serialize(); String destSerialized = new FileMetadata("parquet", "rename_dest.parquet").serialize(); assertFalse(Arrays.asList(dataFormatAwareStoreDirectory.listAll()).contains(srcSerialized)); assertTrue(Arrays.asList(dataFormatAwareStoreDirectory.listAll()).contains(destSerialized)); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b6ff89e

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Resource leak from missing super.close() call The close() method only clears the format cache but never calls super.close(), which means the parent RemoteDirectory's resources (e.g., the base BlobContainer) are never released. This is a resource leak that could cause issues in production. The parent close() should always be called. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [331-333] @Override public void close() throws IOException { - formatBlobRouter.clearBlobFormatCache(); + try { + formatBlobRouter.clearBlobFormatCache(); + } finally { + super.close(); + } } Suggestion importance[1-10]: 8 __ Why: The close() method never calls super.close(), which means the parent RemoteDirectory's resources (e.g., the base BlobContainer) are never released. This is a genuine resource leak that could cause issues in production environments.	Medium
	Fix non-atomic read-modify-write race condition in cache updates The registerBlobFormat and unregisterBlobFormat methods perform a non-atomic read-modify-write on the volatile blobFormatCache field. Under concurrent access, two threads can both read the same snapshot, apply their changes independently, and one update will be lost. Since blobFormatCache is a volatile reference to an immutable map, use a synchronized block or an AtomicReference with a compare-and-swap loop to make the update atomic. server/src/main/java/org/opensearch/index/store/remote/FormatBlobRouter.java [190-210] -public void registerBlobFormat(String blobKey, String format) { +public synchronized void registerBlobFormat(String blobKey, String format) { if (blobKey != null && format != null) { var updated = new HashMap<>(blobFormatCache); updated.put(blobKey, format); blobFormatCache = Map.copyOf(updated); } } -public void unregisterBlobFormat(String blobKey) { +public synchronized void unregisterBlobFormat(String blobKey) { if (blobKey != null) { var updated = new HashMap<>(blobFormatCache); updated.remove(blobKey); blobFormatCache = Map.copyOf(updated); } } Suggestion importance[1-10]: 7 __ Why: The registerBlobFormat and unregisterBlobFormat methods perform a non-atomic read-modify-write on the volatile blobFormatCache. Under concurrent access, updates can be lost. Adding synchronized is a valid fix, though the impact depends on actual concurrency patterns in practice.	Medium
	Apply warm index rate limiter consistently across directory types When creating a DataFormatAwareRemoteDirectory, the warm index download rate limiter (maybeRateLimitRemoteDownloadTransfersForWarm) is not used — it always uses maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex. This is inconsistent with the non-format-aware path and could cause warm index downloads to bypass their rate limit. The warm index check should be applied to the DataFormatAwareRemoteDirectory constructor call as well. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java [175-197] RemoteDirectory dataDirectory = indexSettings != null && indexSettings.isPluggableDataFormatEnabled() ? new DataFormatAwareRemoteDirectory( - ... - blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... - ) - : new RemoteDirectory( - ... + blobStoreRepository.blobStore(isServerSideEncryptionEnabled), + dataPath, + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, isWarmIndex ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments, + LogManager.getLogger("index.store.remote.composite." + shardId), + dataFormatRegistry, + indexSettings + ) + : new RemoteDirectory( + blobStoreRepository.blobStore(isServerSideEncryptionEnabled).blobContainer(dataPath), + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, + isWarmIndex + ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm + : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments ); Suggestion importance[1-10]: 7 __ Why: The DataFormatAwareRemoteDirectory path always uses maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex, while the standard RemoteDirectory path correctly applies maybeRateLimitRemoteDownloadTransfersForWarm for warm indices. This inconsistency could cause warm index downloads to bypass their rate limit.	Medium
	Prevent null directory assignment causing NullPointerException The method createDataFormatAwareStoreDirectory can return null, but at the call site the result is directly assigned to directory without a null check. If dataFormatAwareStoreDirectoryFactory is null, directory will be null and the subsequent storeFactory.newStore(...) call will likely throw a NullPointerException. Either throw an IOException when the factory is unavailable, or add a null check at the call site to fall back to the default directory factory. server/src/main/java/org/opensearch/index/IndexService.java [1338-1352] private DataFormatAwareStoreDirectory createDataFormatAwareStoreDirectory(ShardId shardId, ShardPath shardPath) throws IOException { if (dataFormatAwareStoreDirectoryFactory != null) { - ... - return dataFormatAwareStoreDirectoryFactory.newDataFormatAwareStoreDirectory(...); + logger.debug("Using DataFormatAwareStoreDirectoryFactory to create directory for shard path: {}", shardPath); + return dataFormatAwareStoreDirectoryFactory.newDataFormatAwareStoreDirectory( + indexSettings, + shardId, + shardPath, + directoryFactory, + dataFormatRegistry + ); } - logger.debug("No DataFormatAwareStoreDirectoryFactory available, Store will handle internal creation for: {}", shardPath); - return null; + throw new IOException("No DataFormatAwareStoreDirectoryFactory available for pluggable data format index at: " + shardPath); } Suggestion importance[1-10]: 7 __ Why: The method createDataFormatAwareStoreDirectory can return null when dataFormatAwareStoreDirectoryFactory is null, but the call site at line 782 assigns the result directly to directory without a null check, which would cause a NullPointerException in storeFactory.newStore(...). This is a real potential runtime issue in the pluggable data format code path.	Medium
	Guard against unimplemented code path crashing callers The getSegmentMetadataMap(CatalogSnapshot) method calls loadMetadata(catalogSnapshot, ...) for non-SegmentInfosCatalogSnapshot types, but this method always throws UnsupportedOperationException. The caller catches only NoSuchFileException, CorruptIndexException, IndexFormatTooOldException, and IndexFormatTooNewException, so the UnsupportedOperationException will propagate uncaught and crash the caller. This is a critical path for segment replication. The UnsupportedOperationException should be caught and handled, or the calling code should guard against calling this path until it is implemented. server/src/main/java/org/opensearch/index/store/Store.java [1243-1251] -public static LoadedMetadata loadMetadata( - CatalogSnapshot catalogSnapshot, - Directory directory, - Logger logger, - boolean ignoreSegmentsFile -) throws IOException { - // TODO: Implement format-aware loadMetadata equivalent to the SegmentInfos version - throw new UnsupportedOperationException("loadMetadata for CatalogSnapshot is not yet implemented"); +public Map<String, StoreFileMetadata> getSegmentMetadataMap(CatalogSnapshot catalogSnapshot) throws IOException { + assert indexSettings.isSegRepEnabledOrRemoteNode(); + failIfCorrupted(); + + if (catalogSnapshot instanceof SegmentInfosCatalogSnapshot segmentInfosCatalogSnapshot) { + return getSegmentMetadataMap(segmentInfosCatalogSnapshot.getSegmentInfos()); + } + + // TODO: Remove this guard once loadMetadata(CatalogSnapshot, ...) is fully implemented. + throw new UnsupportedOperationException( + "getSegmentMetadataMap is not yet supported for CatalogSnapshot type: " + catalogSnapshot.getClass().getName() + ); } Suggestion importance[1-10]: 7 __ Why: The getSegmentMetadataMap(CatalogSnapshot) method calls loadMetadata(catalogSnapshot, ...) for non-SegmentInfosCatalogSnapshot types, but this always throws UnsupportedOperationException. The caller only catches specific IOException subtypes, so the UnsupportedOperationException will propagate uncaught. The improved code correctly moves the guard to the calling method to prevent this crash.	Medium
	Null logger causes NullPointerException in constructor The logger field is assigned after it is used in the constructor body. If logger is null (which is possible since it's a constructor parameter), this line will throw a NullPointerException. The logger assignment should be moved before any logging calls, or a null-check should be added. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [122] -logger.debug("Created DataFormatAwareRemoteDirectory with formats: {}", formatBlobRouter.registeredFormats()); +this.logger = logger; +// ... other assignments ... +if (logger != null) { + logger.debug("Created DataFormatAwareRemoteDirectory with formats: {}", formatBlobRouter.registeredFormats()); +} Suggestion importance[1-10]: 6 __ Why: Looking at the constructor code, this.logger = logger is assigned at line 113, before the logger.debug(...) call at line 122. However, the test testConstructor_NullDataFormatRegistry passes null as the logger, which would cause a NullPointerException at the debug log line. A null-check is warranted.	Low
	Fix resource leak in snapshot close lifecycle The SegmentInfosCatalogSnapshot is constructed from segmentInfosRef.get(), but the snapshot's reference count lifecycle is not tied to the underlying segmentInfosRef. If the GatedCloseable returned here is closed, only segmentInfosRef::close is called, but snapshot.close() (which decrements the snapshot's own ref count) is never called. This could lead to resource leaks in the snapshot's internal state. The close action should also call snapshot.close(). server/src/main/java/org/opensearch/index/engine/EngineBackedIndexer.java [387-390] public GatedCloseable<CatalogSnapshot> acquireSnapshot() { GatedCloseable<SegmentInfos> segmentInfosRef = engine.getSegmentInfosSnapshot(); SegmentInfosCatalogSnapshot snapshot = new SegmentInfosCatalogSnapshot(segmentInfosRef.get()); - return new GatedCloseable<>(snapshot, segmentInfosRef::close); + return new GatedCloseable<>(snapshot, () -> { + try { + snapshot.close(); + } finally { + segmentInfosRef.close(); + } + }); } Suggestion importance[1-10]: 6 __ Why: The GatedCloseable only calls segmentInfosRef::close when closed, but never calls snapshot.close(). If CatalogSnapshot has internal ref-counting or resources (as suggested by the closeInternal pattern in the codebase), this could lead to resource leaks. The improved code ensures both are closed properly.	Low
	Arithmetic overflow in block bounds validation The validation condition length <= 0 rejects a length of zero, but the condition position + length > fileLength can overflow for large long values, potentially bypassing the bounds check. Use position > fileLength - length instead to avoid overflow. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [470-472] -if (position < 0 || length <= 0 || (position + length > fileLength)) { +if (position < 0 || length <= 0 || length > fileLength - position) { throw new IllegalArgumentException("Invalid values of block start and size"); } Suggestion importance[1-10]: 5 __ Why: The condition position + length > fileLength can overflow for large long values, potentially bypassing the bounds check. Rewriting as length > fileLength - position avoids this overflow and is semantically equivalent.	Low
General	Guard against silent null return from non-empty factory map The method returns null if the map is non-empty but does not contain the key "default", silently ignoring all registered factories. This could cause a pluggable data format to be enabled via indexSettings.isPluggableDataFormatEnabled() but the factory to be null, leading to a NullPointerException downstream. A warning log or an explicit check should be added when the map is non-empty but the "default" key is absent. server/src/main/java/org/opensearch/index/IndexModule.java [1092-1100] private static DataFormatAwareStoreDirectoryFactory getDataFormatAwareStoreDirectoryFactory( final IndexSettings indexSettings, final Map<String, DataFormatAwareStoreDirectoryFactory> dataFormatAwareStoreDirectoryFactories ) { if (dataFormatAwareStoreDirectoryFactories.isEmpty()) { return null; } - return dataFormatAwareStoreDirectoryFactories.get("default"); + DataFormatAwareStoreDirectoryFactory factory = dataFormatAwareStoreDirectoryFactories.get("default"); + if (factory == null) { + throw new IllegalStateException( + "DataFormatAwareStoreDirectoryFactory map is non-empty but contains no 'default' entry: " + + dataFormatAwareStoreDirectoryFactories.keySet() + ); + } + return factory; } Suggestion importance[1-10]: 6 __ Why: The method silently returns null when the map is non-empty but lacks a "default" key, which could cause a NullPointerException downstream when isPluggableDataFormatEnabled() is true. Throwing an IllegalStateException makes the misconfiguration explicit and easier to diagnose.	Low
	Avoid silently dropping valid zero-value checksums The guard checksum != 0 silently drops registrations where the actual computed CRC32 happens to be zero (an astronomically rare but valid value). This would cause the computeChecksum method to fall back to a full-file scan instead of using the pre-computed value, which is a correctness issue if the fallback directory is unavailable or produces a different result. The zero-checksum guard should be removed or replaced with a null/sentinel check. server/src/main/java/org/opensearch/index/store/PrecomputedChecksumStrategy.java [55-67] @Override public void registerChecksum(String fileName, long checksum, long writerGeneration) { - if (fileName != null && checksum != 0) { + if (fileName != null) { checksumCache.compute(fileName, (key, existing) -> { if (existing == null || writerGeneration >= existing.generation()) { return new CacheEntry(checksum, writerGeneration); } return existing; }); } } Suggestion importance[1-10]: 5 __ Why: The checksum != 0 guard silently drops registrations where the CRC32 is exactly zero (a valid but rare value), causing an unnecessary O(n) fallback scan. Removing this guard is a correctness improvement, though the probability of a real CRC32 being zero is extremely low in practice.	Low
	Incorrect return value masks synchronous pre-upload failures When an exception is caught before uploadBlob is called (e.g., during FileMetadata parsing or container lookup), the method calls listener.onFailure(e) and returns true. However, if the exception occurs after uploadBlob starts (which itself calls listener.onFailure internally on async errors), this outer catch could result in listener.onFailure being called twice. The uploadBlob method already throws checked exceptions, so the outer catch should only handle pre-upload failures. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [275-279] } catch (Exception e) { logger.error(() -> new ParameterizedMessage("Failed format-aware upload: src={}, error={}", src, e.getMessage()), e); listener.onFailure(e); - return true; // Handled (even though failed) + return false; // Not handled asynchronously — failed synchronously before upload started } Suggestion importance[1-10]: 4 __ Why: Returning true in the catch block signals to the caller that the upload was handled asynchronously, but the failure occurred synchronously before any async operation started. Returning false would be more semantically correct, though the listener.onFailure call already notifies the caller of the error.	Low
	Avoid redundant method calls inside loop iteration getLocalSegmentFilename(file) is called twice per iteration — once for the containsKey check and once for removeUploadedSegment. This is redundant and could be a subtle bug if the method is not pure. The result should be stored in a local variable to avoid the double call. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1249-1251] -public void deleteStaleSegments(int lastNMetadataFilesToKeep) throws IOException - ... - // Update cache after successful batch deletion - for (String file : filesToDelete) { - if (!activeSegmentFilesMetadataMap.containsKey(getLocalSegmentFilename(file))) { - removeUploadedSegment(getLocalSegmentFilename(file)); - } +for (String file : filesToDelete) { + String localFilename = getLocalSegmentFilename(file); + if (!activeSegmentFilesMetadataMap.containsKey(localFilename)) { + removeUploadedSegment(localFilename); } +} Suggestion importance[1-10]: 4 __ Why: getLocalSegmentFilename(file) is called twice per iteration, which is redundant. Storing the result in a local variable improves readability and avoids any potential side effects from double invocation.	Low

---

Previous suggestions

Suggestions up to commit f527540

Category	Suggestion	Impact
Possible issue	Missing super.close() causes resource leak The close() method only clears the format cache but never calls super.close(), which means the parent RemoteDirectory's resources (e.g., the base BlobContainer) are never released. This is a resource leak. The super.close() call should be included. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [331-333] @Override public void close() throws IOException { - formatBlobRouter.clearBlobFormatCache(); + try { + formatBlobRouter.clearBlobFormatCache(); + } finally { + super.close(); + } } Suggestion importance[1-10]: 8 __ Why: The close() method never calls super.close(), which means the parent RemoteDirectory's resources are never released. This is a genuine resource leak that could cause issues in production.	Medium
	Logger used before assignment in constructor The logger field is assigned after it is used in the constructor body. If logger is null (which is possible since it's a constructor parameter), this line will throw a NullPointerException. The logger assignment should be moved before this line, or a null check should be added. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [110-123] -logger.debug("Created DataFormatAwareRemoteDirectory with formats: {}", formatBlobRouter.registeredFormats()); +this.logger = logger; +// Pre-register format-specific BlobContainers from DataFormatRegistry +if (dataFormatRegistry != null && indexSettings != null) { + for (String formatName : dataFormatRegistry.getFormatDescriptors(indexSettings).keySet()) { + formatBlobRouter.registerFormat(formatName); + } +} + +this.logger.debug("Created DataFormatAwareRemoteDirectory with formats: {}", formatBlobRouter.registeredFormats()); + Suggestion importance[1-10]: 7 __ Why: The this.logger = logger assignment appears at line 113, but logger.debug(...) is called at line 122. If logger is null, this causes a NullPointerException. The improved code correctly moves the debug call after the assignment.	Medium
	Fix non-atomic read-modify-write race condition on cache The registerBlobFormat and unregisterBlobFormat methods perform a non-atomic read-modify-write on the volatile blobFormatCache field. Under concurrent access, two threads could both read the same snapshot, apply their changes independently, and one update would be lost. Since blobFormatCache is a volatile reference to an immutable map, use a synchronized block or AtomicReference with a CAS loop to ensure atomicity. server/src/main/java/org/opensearch/index/store/remote/FormatBlobRouter.java [190-210] -public void registerBlobFormat(String blobKey, String format) { +public synchronized void registerBlobFormat(String blobKey, String format) { if (blobKey != null && format != null) { var updated = new HashMap<>(blobFormatCache); updated.put(blobKey, format); blobFormatCache = Map.copyOf(updated); } } -public void unregisterBlobFormat(String blobKey) { +public synchronized void unregisterBlobFormat(String blobKey) { if (blobKey != null) { var updated = new HashMap<>(blobFormatCache); updated.remove(blobKey); blobFormatCache = Map.copyOf(updated); } } Suggestion importance[1-10]: 7 __ Why: The registerBlobFormat and unregisterBlobFormat methods perform a non-atomic read-modify-write on the volatile blobFormatCache. Under concurrent access, updates can be lost. Adding synchronized is a valid fix, though the impact depends on actual concurrency patterns in practice.	Medium
	Apply warm index rate limiting consistently across directory types When creating a DataFormatAwareRemoteDirectory, the warm index download rate limiter (maybeRateLimitRemoteDownloadTransfersForWarm) is not used — it always uses maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex. This inconsistency means warm index throttling is silently skipped for pluggable-format indices. The warm index check should be applied in the DataFormatAwareRemoteDirectory branch as well. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java [175-197] RemoteDirectory dataDirectory = indexSettings != null && indexSettings.isPluggableDataFormatEnabled() ? new DataFormatAwareRemoteDirectory( - ... - blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... - ) - : new RemoteDirectory( - ... + blobStoreRepository.blobStore(isServerSideEncryptionEnabled), + dataPath, + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, isWarmIndex ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments, + LogManager.getLogger("index.store.remote.composite." + shardId), + dataFormatRegistry, + indexSettings + ) + : new RemoteDirectory( + blobStoreRepository.blobStore(isServerSideEncryptionEnabled).blobContainer(dataPath), + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, + isWarmIndex + ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm + : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments ); Suggestion importance[1-10]: 7 __ Why: The DataFormatAwareRemoteDirectory branch always uses maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex, silently skipping warm index throttling for pluggable-format indices. This is a real behavioral inconsistency that could cause performance issues for warm indices.	Medium
	Prevent null directory from being passed to store factory When createDataFormatAwareStoreDirectory returns null, the calling code assigns null to directory and passes it to storeFactory.newStore(...), which will likely cause a NullPointerException. Either throw an IOException when the factory is unavailable, or fall back to the standard directoryFactory.newDirectory(...) instead of returning null. server/src/main/java/org/opensearch/index/IndexService.java [1338-1352] private DataFormatAwareStoreDirectory createDataFormatAwareStoreDirectory(ShardId shardId, ShardPath shardPath) throws IOException { if (dataFormatAwareStoreDirectoryFactory != null) { logger.debug("Using DataFormatAwareStoreDirectoryFactory to create directory for shard path: {}", shardPath); return dataFormatAwareStoreDirectoryFactory.newDataFormatAwareStoreDirectory( indexSettings, shardId, shardPath, directoryFactory, dataFormatRegistry ); } - logger.debug("No DataFormatAwareStoreDirectoryFactory available, Store will handle internal creation for: {}", shardPath); - return null; + throw new IOException( + "No DataFormatAwareStoreDirectoryFactory available for pluggable data format index on shard: " + shardId + ); } Suggestion importance[1-10]: 7 __ Why: When createDataFormatAwareStoreDirectory returns null, the calling code assigns null to directory and passes it to storeFactory.newStore(...), which will likely cause a NullPointerException. The suggestion to throw an IOException instead of returning null is valid and prevents a hard-to-debug NPE.	Medium
	Ensure snapshot resources are released on close The SegmentInfosCatalogSnapshot is constructed from segmentInfosRef.get(), but the snapshot's reference count lifecycle is not tied to the underlying segmentInfosRef resource. If the GatedCloseable is closed, segmentInfosRef::close is called, but the SegmentInfosCatalogSnapshot itself is never closed, potentially leaking its own resources. The snapshot's close() should also be invoked in the closer. server/src/main/java/org/opensearch/index/engine/EngineBackedIndexer.java [387-390] public GatedCloseable<CatalogSnapshot> acquireSnapshot() { GatedCloseable<SegmentInfos> segmentInfosRef = engine.getSegmentInfosSnapshot(); SegmentInfosCatalogSnapshot snapshot = new SegmentInfosCatalogSnapshot(segmentInfosRef.get()); - return new GatedCloseable<>(snapshot, segmentInfosRef::close); + return new GatedCloseable<>(snapshot, () -> { + try { + snapshot.close(); + } finally { + segmentInfosRef.close(); + } + }); } Suggestion importance[1-10]: 6 __ Why: The SegmentInfosCatalogSnapshot is never explicitly closed when the GatedCloseable is closed — only segmentInfosRef::close is called. This could lead to resource leaks if the snapshot holds its own resources. The improved code correctly closes both the snapshot and the underlying segmentInfosRef.	Low
	Arithmetic overflow in bounds validation The validation condition length <= 0 rejects a zero-length read, but the more critical issue is that position + length > fileLength can silently overflow for large long values, potentially bypassing the bounds check. Use Long.MAX_VALUE - position < length or position > fileLength - length to avoid overflow. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [470-472] -if (position < 0 || length <= 0 || (position + length > fileLength)) { +if (position < 0 || length <= 0 || length > fileLength - position) { throw new IllegalArgumentException("Invalid values of block start and size"); } Suggestion importance[1-10]: 5 __ Why: The condition position + length > fileLength can overflow for large long values, potentially bypassing the bounds check. The suggested fix length > fileLength - position avoids overflow and is more correct.	Low
General	Original exception lost if close throws When an exception is caught and indexInput.close() itself throws an IOException, the original exception e is silently lost and replaced by the close exception. The close exception should be suppressed onto the original exception to preserve the root cause. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [407-411] } catch (Exception e) { logger.warn("Exception while calling asyncBlobUpload for {}, closing IndexInput", src); - indexInput.close(); + try { + indexInput.close(); + } catch (Exception closeEx) { + e.addSuppressed(closeEx); + } throw e; } Suggestion importance[1-10]: 5 __ Why: If indexInput.close() throws an exception, the original exception e is silently replaced. Using addSuppressed preserves the root cause, which is important for debugging.	Low
	Fail fast when expected factory key is missing The method returns null when the map is non-empty but does not contain the key "default", silently disabling the data-format-aware directory without any warning or error. This could cause hard-to-diagnose issues where a plugin registers a factory under a different key and the feature appears to be disabled. Add a fallback or log a warning when the map is non-empty but the "default" key is absent. server/src/main/java/org/opensearch/index/IndexModule.java [1092-1100] private static DataFormatAwareStoreDirectoryFactory getDataFormatAwareStoreDirectoryFactory( final IndexSettings indexSettings, final Map<String, DataFormatAwareStoreDirectoryFactory> dataFormatAwareStoreDirectoryFactories ) { if (dataFormatAwareStoreDirectoryFactories.isEmpty()) { return null; } - return dataFormatAwareStoreDirectoryFactories.get("default"); + DataFormatAwareStoreDirectoryFactory factory = dataFormatAwareStoreDirectoryFactories.get("default"); + if (factory == null) { + throw new IllegalStateException( + "DataFormatAwareStoreDirectoryFactory map is non-empty but contains no 'default' entry. " + + "Registered keys: " + dataFormatAwareStoreDirectoryFactories.keySet() + ); + } + return factory; } Suggestion importance[1-10]: 5 __ Why: Silently returning null when the map is non-empty but lacks the "default" key could cause confusing behavior. Failing fast with a clear error message improves debuggability, though this is a defensive improvement rather than a critical bug fix.	Low
	Avoid silently dropping valid zero-value checksums The guard checksum != 0 silently drops registrations for files whose actual CRC32 happens to be zero (extremely rare but theoretically possible). This would cause the upload path to fall back to a full-file scan instead of using the pre-computed value, which is a silent correctness issue. Consider removing the checksum != 0 guard or replacing it with an explicit validity flag. server/src/main/java/org/opensearch/index/store/PrecomputedChecksumStrategy.java [55-67] public void registerChecksum(String fileName, long checksum, long writerGeneration) { - if (fileName != null && checksum != 0) { + if (fileName != null) { checksumCache.compute(fileName, (key, existing) -> { if (existing == null || writerGeneration >= existing.generation()) { return new CacheEntry(checksum, writerGeneration); } return existing; }); } } Suggestion importance[1-10]: 5 __ Why: The checksum != 0 guard could silently drop a theoretically valid zero CRC32, causing a silent fallback to full-file scan. While a zero CRC32 is extremely rare, removing this guard is a correctness improvement with no downside.	Low
	Prevent stale pending-download entries in rebuilt format cache syncBlobFormatCache is called from replaceUploadedSegments, which replaces segmentsUploadedToRemoteStore with a new ConcurrentHashMap. However, pendingDownloadMergedSegments is not reset during init() / initializeToSpecificCommit() / initializeToSpecificTimestamp(), so stale pending-download entries from a previous state may be included in the rebuilt cache, leading to incorrect format routing. The cache rebuild should only include pending segments that are still valid for the new state, or pendingDownloadMergedSegments should also be cleared on re-initialization. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1051-1065] private void syncBlobFormatCache() { if (formatBlobRouter == null) { return; } Map<String, String> blobKeyToFormat = new HashMap<>(); for (UploadedSegmentMetadata metadata : segmentsUploadedToRemoteStore.values()) { blobKeyToFormat.put(metadata.getUploadedFilename(), extractFormat(metadata.getOriginalFilename())); } + // Only include pending segments whose local filename is still relevant if (pendingDownloadMergedSegments != null) { for (Map.Entry<String, String> entry : pendingDownloadMergedSegments.entrySet()) { - blobKeyToFormat.put(entry.getValue(), extractFormat(entry.getKey())); + // Only register if not already covered by uploaded segments + if (!segmentsUploadedToRemoteStore.containsKey(entry.getKey())) { + blobKeyToFormat.put(entry.getValue(), extractFormat(entry.getKey())); + } } } formatBlobRouter.replaceBlobFormatCache(blobKeyToFormat); } Suggestion importance[1-10]: 4 __ Why: The concern about stale pendingDownloadMergedSegments entries is valid in theory, but the improved code only changes a comment and adds a redundancy check (!segmentsUploadedToRemoteStore.containsKey), not actually clearing stale entries. The suggestion doesn't fully address the root issue it describes.	Low
	Improve error message for unimplemented metadata loading The getSegmentMetadataMap(CatalogSnapshot) method calls loadMetadata(CatalogSnapshot, ...) for non-SegmentInfosCatalogSnapshot types, but this method unconditionally throws UnsupportedOperationException. This means any segment replication attempt using a DataformatAwareCatalogSnapshot will fail at runtime with an unhandled exception rather than a meaningful error. The getSegmentMetadataMap caller should guard against this case or the exception should be surfaced more clearly. server/src/main/java/org/opensearch/index/store/Store.java [1243-1251] public static LoadedMetadata loadMetadata( CatalogSnapshot catalogSnapshot, Directory directory, Logger logger, boolean ignoreSegmentsFile ) throws IOException { - // TODO: Implement format-aware loadMetadata equivalent to the SegmentInfos version - throw new UnsupportedOperationException("loadMetadata for CatalogSnapshot is not yet implemented"); + throw new UnsupportedOperationException( + "loadMetadata for CatalogSnapshot of type [" + catalogSnapshot.getClass().getSimpleName() + "] is not yet implemented" + ); } Suggestion importance[1-10]: 3 __ Why: The suggestion only improves the error message by including the snapshot type name, which is a minor readability improvement. The underlying issue (the method always throws) is already acknowledged with a TODO comment, so this is a low-impact change.	Low

Suggestions up to commit f527540

Category	Suggestion	Impact
Possible issue	Missing super.close() call causes resource leak The close() method only clears the format blob cache but does not call super.close(), which means the parent RemoteDirectory's resources (e.g., the base BlobContainer) are never released. This can cause resource leaks. The super.close() should be called to properly release inherited resources. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [331-333] @Override public void close() throws IOException { - formatBlobRouter.clearBlobFormatCache(); + try { + formatBlobRouter.clearBlobFormatCache(); + } finally { + super.close(); + } } Suggestion importance[1-10]: 8 __ Why: The close() method skips super.close(), meaning the parent RemoteDirectory's base BlobContainer and other resources are never released. This is a genuine resource leak that could cause issues in production.	Medium
	Fix non-atomic read-modify-write race condition in cache updates The registerBlobFormat and unregisterBlobFormat methods perform a non-atomic read-modify-write on the volatile blobFormatCache field. Under concurrent access, two threads can both read the same snapshot, apply their changes independently, and one update will silently overwrite the other. Since blobFormatCache is a volatile reference to an immutable map, use synchronized blocks or an AtomicReference with a compare-and-swap loop to make these operations thread-safe. server/src/main/java/org/opensearch/index/store/remote/FormatBlobRouter.java [190-210] -public void registerBlobFormat(String blobKey, String format) { +public synchronized void registerBlobFormat(String blobKey, String format) { if (blobKey != null && format != null) { var updated = new HashMap<>(blobFormatCache); updated.put(blobKey, format); blobFormatCache = Map.copyOf(updated); } } -public void unregisterBlobFormat(String blobKey) { +public synchronized void unregisterBlobFormat(String blobKey) { if (blobKey != null) { var updated = new HashMap<>(blobFormatCache); updated.remove(blobKey); blobFormatCache = Map.copyOf(updated); } } Suggestion importance[1-10]: 7 __ Why: The registerBlobFormat and unregisterBlobFormat methods perform non-atomic read-modify-write on a volatile field, which is a real concurrency bug. However, the impact depends on actual concurrent usage patterns, and the fix (adding synchronized) is straightforward and accurate.	Medium
	Apply warm index rate limiter consistently for new directory type When creating a DataFormatAwareRemoteDirectory, the warm index download rate limiter (maybeRateLimitRemoteDownloadTransfersForWarm) is never used — it always passes maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex. This is inconsistent with the non-pluggable path and could cause warm index performance issues. The warm index check should be applied to the DataFormatAwareRemoteDirectory constructor call as well. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java [175-197] RemoteDirectory dataDirectory = indexSettings != null && indexSettings.isPluggableDataFormatEnabled() ? new DataFormatAwareRemoteDirectory( - ... - blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... - ) - : new RemoteDirectory( - ... + blobStoreRepository.blobStore(isServerSideEncryptionEnabled), + dataPath, + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, isWarmIndex ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, - ... + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments, + LogManager.getLogger("index.store.remote.composite." + shardId), + dataFormatRegistry, + indexSettings + ) + : new RemoteDirectory( + blobStoreRepository.blobStore(isServerSideEncryptionEnabled).blobContainer(dataPath), + blobStoreRepository::maybeRateLimitRemoteUploadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityRemoteUploadTransfers, + isWarmIndex + ? blobStoreRepository::maybeRateLimitRemoteDownloadTransfersForWarm + : blobStoreRepository::maybeRateLimitRemoteDownloadTransfers, + blobStoreRepository::maybeRateLimitLowPriorityDownloadTransfers, + pendingDownloadMergedSegments ); Suggestion importance[1-10]: 7 __ Why: The DataFormatAwareRemoteDirectory path always uses maybeRateLimitRemoteDownloadTransfers regardless of isWarmIndex, which is inconsistent with the non-pluggable path and could cause warm index performance degradation. The improved code correctly applies the warm index check.	Medium
	Prevent null directory assignment causing NullPointerException When createDataFormatAwareStoreDirectory returns null, the calling code in the else branch assigns null to directory, which will cause a NullPointerException when storeFactory.newStore(...) is called. Either throw an IOException when the factory is unavailable, or fall back to the standard directoryFactory. server/src/main/java/org/opensearch/index/IndexService.java [1338-1352] private DataFormatAwareStoreDirectory createDataFormatAwareStoreDirectory(ShardId shardId, ShardPath shardPath) throws IOException { if (dataFormatAwareStoreDirectoryFactory != null) { logger.debug("Using DataFormatAwareStoreDirectoryFactory to create directory for shard path: {}", shardPath); return dataFormatAwareStoreDirectoryFactory.newDataFormatAwareStoreDirectory( indexSettings, shardId, shardPath, directoryFactory, dataFormatRegistry ); } - logger.debug("No DataFormatAwareStoreDirectoryFactory available, Store will handle internal creation for: {}", shardPath); - return null; + throw new IOException( + "No DataFormatAwareStoreDirectoryFactory available for pluggable data format index on shard: " + shardId + ); } Suggestion importance[1-10]: 7 __ Why: When createDataFormatAwareStoreDirectory returns null, the calling code assigns null to directory, which will cause a NullPointerException when storeFactory.newStore(...) is called. Throwing an IOException instead would provide a clearer error message and prevent a confusing NPE.	Medium
	Close snapshot to prevent reference count leak The SegmentInfosCatalogSnapshot is constructed from segmentInfosRef.get(), but the GatedCloseable closer only calls segmentInfosRef::close. The snapshot itself is never closed, which means its internal reference count is never decremented, potentially leaking resources managed by CatalogSnapshot's ref-counting mechanism. server/src/main/java/org/opensearch/index/engine/EngineBackedIndexer.java [386-390] public GatedCloseable<CatalogSnapshot> acquireSnapshot() { GatedCloseable<SegmentInfos> segmentInfosRef = engine.getSegmentInfosSnapshot(); SegmentInfosCatalogSnapshot snapshot = new SegmentInfosCatalogSnapshot(segmentInfosRef.get()); - return new GatedCloseable<>(snapshot, segmentInfosRef::close); + return new GatedCloseable<>(snapshot, () -> { + try { + snapshot.close(); + } finally { + segmentInfosRef.close(); + } + }); } Suggestion importance[1-10]: 7 __ Why: The SegmentInfosCatalogSnapshot is constructed but its close() method is never called in the GatedCloseable closer, meaning its internal reference count is never decremented. This could leak resources managed by CatalogSnapshot's ref-counting mechanism over time.	Medium
	Arithmetic overflow in block bounds validation The validation condition length <= 0 rejects a zero-length read, but more critically position + length > fileLength can silently overflow to a negative number when both values are large longs, bypassing the check. Use Long.MAX_VALUE - position < length or Math.addExact to guard against overflow. server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [470-472] -if (position < 0 || length <= 0 || (position + length > fileLength)) { +if (position < 0 || length <= 0 || length > fileLength - position) { throw new IllegalArgumentException("Invalid values of block start and size"); } Suggestion importance[1-10]: 6 __ Why: The condition position + length > fileLength can overflow for large long values, bypassing the bounds check. The suggested fix length > fileLength - position avoids overflow and is more correct.	Low
	Replace uncaught UnsupportedOperationException with IOException The getSegmentMetadataMap(CatalogSnapshot) method calls loadMetadata(catalogSnapshot, ...) for non-SegmentInfosCatalogSnapshot types, but this method unconditionally throws UnsupportedOperationException. This means any segment replication operation on a DataformatAwareCatalogSnapshot will fail at runtime with an unhandled exception that is not caught by the caller's catch block (which only catches I/O and corruption exceptions). server/src/main/java/org/opensearch/index/store/Store.java [1243-1251] public static LoadedMetadata loadMetadata( CatalogSnapshot catalogSnapshot, Directory directory, Logger logger, boolean ignoreSegmentsFile ) throws IOException { - // TODO: Implement format-aware loadMetadata equivalent to the SegmentInfos version - throw new UnsupportedOperationException("loadMetadata for CatalogSnapshot is not yet implemented"); + throw new IOException("loadMetadata for CatalogSnapshot is not yet implemented for type: " + + catalogSnapshot.getClass().getSimpleName()); } Suggestion importance[1-10]: 6 __ Why: The getSegmentMetadataMap(CatalogSnapshot) caller only catches NoSuchFileException, CorruptIndexException, IndexFormatTooOldException, and IndexFormatTooNewException, so an UnsupportedOperationException from loadMetadata would propagate uncaught. Wrapping it as an IOException ensures it's handled by the caller's exception handling.	Low
	Logger assigned after first use in constructor The logger field is used before it is assigned in the constructor body. The super(...) call and formatBlobRouter initialization happen first, but this.logger is only assigned after the formatBlobRouter initialization block. If any of the pre-assignment code throws, logger would be null. More critically, the logger.debug(...) call at the end of the constructor uses this.logger, which is assigned just before it — but if dataFormatRegistry processing throws, the logger assignment is skipped. Move the this.logger = logger assignment to be the very first statement after super(...). server/src/main/java/org/opensearch/index/store/remote/DataFormatAwareRemoteDirectory.java [109-123] -logger.debug("Created DataFormatAwareRemoteDirectory with formats: {}", formatBlobRouter.registeredFormats()); +super( + blobStore.blobContainer(baseBlobPath), + uploadRateLimiter, + lowPriorityUploadRateLimiter, + downloadRateLimiter, + lowPriorityDownloadRateLimiter, + pendingDownloadMergedSegments +); +this.logger = logger; +this.formatBlobRouter = new FormatBlobRouter(blobStore, baseBlobPath); +this.uploadRateLimiter = uploadRateLimiter; +this.lowPriorityUploadRateLimiter = lowPriorityUploadRateLimiter; +this.downloadRateLimiterProvider = new DownloadRateLimiterProvider(downloadRateLimiter, lowPriorityDownloadRateLimiter); Suggestion importance[1-10]: 5 __ Why: The this.logger field is assigned after formatBlobRouter initialization, but the logger.debug(...) call at the end uses it correctly. However, if an exception is thrown during dataFormatRegistry processing, this.logger would already be assigned (it's assigned before the registry loop). The suggestion to move this.logger = logger immediately after super() is a good defensive practice to ensure the logger is available for any error logging during initialization.	Low
	Snapshot concurrent map before iterating to avoid race condition syncBlobFormatCache is called from replaceUploadedSegments, which reassigns segmentsUploadedToRemoteStore to a new ConcurrentHashMap. However, pendingDownloadMergedSegments is read without synchronization here, while it can be concurrently modified by markMergedSegmentsPendingDownload and unmarkMergedSegmentsPendingDownload. This creates a race where the cache snapshot may be inconsistent. The method should be called within a synchronized block, or pendingDownloadMergedSegments should be snapshotted atomically. server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java [1051-1065] private void syncBlobFormatCache() { if (formatBlobRouter == null) { return; } Map<String, String> blobKeyToFormat = new HashMap<>(); for (UploadedSegmentMetadata metadata : segmentsUploadedToRemoteStore.values()) { blobKeyToFormat.put(metadata.getUploadedFilename(), extractFormat(metadata.getOriginalFilename())); } if (pendingDownloadMergedSegments != null) { - for (Map.Entry<String, String> entry : pendingDownloadMergedSegments.entrySet()) { + // Snapshot to avoid TOCTOU with concurrent modifications + Map<String, String> pendingSnapshot = new HashMap<>(pendingDownloadMergedSegments); + for (Map.Entry<String, String> entry : pendingSnapshot.entrySet()) { blobKeyToFormat.put(entry.getValue(), extractFormat(entry.getKey())); } } formatBlobRouter.replaceBlobFormatCache(blobKeyToFormat); } Suggestion importance[1-10]: 5 __ Why: The pendingDownloadMergedSegments map can be concurrently modified while syncBlobFormatCache iterates it, potentially causing ConcurrentModificationException or an inconsistent cache snapshot. Snapshotting it first is a valid defensive improvement, though the severity depends on whether syncBlobFormatCache is called in a synchronized context.

[github-actions]

❌ Gradle check result for 7b78eb2: 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]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 06a95d4.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/engine/exec/FileMetadata.java	11	low	Imports 'reactor.util.annotation.NonNull' from Project Reactor solely for a @nonnull annotation on toString(). This is an unusual external dependency for a core data class that normally would use javax.annotation or a standard OpenSearch annotation. While functionally benign, introducing an atypical transitive dependency into core server code for a trivial annotation warrants review.
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java	79	low	The class is changed from 'final' to non-final, and multiple members are widened from 'private' to package-private or 'protected' (e.g., 'logger', 'canDeleteStaleCommits', 'readMetadataFile', 'getNewRemoteSegmentFilename', 'removeFileFromSegmentsUploadedToRemoteStore'). This weakens encapsulation of the remote segment store's security-sensitive operations, enabling subclasses (including the new CompositeRemoteSegmentStoreDirectory) to override or access internals. While consistent with the PR's extensibility goals, it broadens the attack surface of the remote store.
server/src/main/java/org/opensearch/index/store/UploadedSegmentMetadata.java	85	low	In fromString(), the call to setWrittenByMajor() is commented out ('// metadata.setWrittenByMajor(Integer.parseInt(values[4]));'), silently disabling the Lucene major version compatibility check for deserialized metadata. This bypasses an existing safety mechanism that prevents use of segment files written by an incompatible Lucene version. The comment offers no explanation for the omission.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 0 | Low: 3

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 06a95d4

[github-actions]

❌ Gradle check result for 06a95d4: 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]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 13f8caa.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/engine/exec/FileMetadata.java	11	medium	Imports 'reactor.util.annotation.NonNull' from the Reactor library solely to annotate the return type of toString(). The server module does not otherwise use Reactor; this introduces an unnecessary third-party dependency where a standard Java or existing OpenSearch annotation would suffice. Unusual dependency additions in core modules are a common supply-chain attack vector, though here the import is used only as a documentation annotation with no runtime effect.
server/src/main/java/org/opensearch/index/store/UploadedSegmentMetadata.java	88	low	In fromString(), the call to setWrittenByMajor() is commented out ('// metadata.setWrittenByMajor(Integer.parseInt(values[4]));'). This silently bypasses Lucene major-version compatibility validation that was enforced in the original inner class, potentially allowing segment files written by incompatible Lucene versions to be accepted without error. Could be a deliberate integrity bypass disguised as a TODO.
server/src/main/java/org/opensearch/index/store/CompositeRemoteSegmentStoreDirectory.java	498	low	Inside uploadMetadataInternal(), the call 'metadata.setWrittenByMajor(10)' is commented out with only a 'Todo' comment. Combined with the same bypass in UploadedSegmentMetadata.fromString(), this means the Lucene version field is never validated or set in this new code path, weakening integrity checks during metadata uploads.
server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java	79	low	RemoteSegmentStoreDirectory was changed from 'public final class' to 'public class', and the 'logger' and 'canDeleteStaleCommits' fields were widened from private/protected to package-private. While justified by the new subclass (CompositeRemoteSegmentStoreDirectory), removing the 'final' modifier on a security-sensitive storage class expands the attack surface for malicious subclassing in plugin code.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 1 | Low: 3

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 2cdae95.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/engine/exec/FileMetadata.java	11	medium	Imports 'reactor.util.annotation.NonNull' from Project Reactor — an unusual and unexpected dependency for a core storage class in OpenSearch, used solely to annotate toString(). Introducing a new third-party library dependency (Reactor) into a production storage module for a trivial annotation is anomalous and warrants supply-chain review to confirm the dependency is intentionally approved and does not pull in unvetted transitive dependencies.
server/src/main/java/org/opensearch/index/IndexSettings.java	102	low	isOptimizedIndex() is hardcoded to return false with a 'ToDo' comment. This method gates the new CompositeRemoteDirectory code path in RemoteSegmentStoreDirectoryFactory. While not malicious, a permanently-false gate on new infrastructure is an anomaly worth noting — it could be a placeholder that later gets silently enabled, bypassing normal review of the new code path's activation.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit bdf9055.

Path	Line	Severity	Description
server/src/main/java/org/opensearch/index/store/FileMetadata.java	11	medium	Import of 'reactor.util.annotation.NonNull' introduces Project Reactor as a dependency into core OpenSearch store code. This is an unusual external dependency for this package and could indicate an unintended or supply-chain-related dependency addition, though it may simply be an accidental IDE import.
server/src/main/java/org/opensearch/index/store/UploadedSegmentMetadata.java	79	low	In fromString(), the writtenByMajor parsing is commented out ('// metadata.setWrittenByMajor(Integer.parseInt(values[4]));'), silently bypassing the Lucene major version validation check that guards against incompatible segment downloads. This weakens a safety control but appears to be incomplete implementation rather than deliberate bypass.
server/src/main/java/org/opensearch/index/store/UploadedSegmentMetadata.java	71	low	fromString() uses 'new File(uploadedFilename).getName()' to parse a remote segment filename. Applying java.io.File path semantics to remote/logical filenames is unconventional and could silently strip path prefixes, potentially masking misrouted remote file access. Looks like a coding error rather than malicious intent.
server/src/main/java/org/opensearch/index/store/CompositeRemoteSegmentStoreDirectory.java	126	low	In the RemoteDirectory-based constructor, 'this.compositeRemoteDirectory = null' is set explicitly, yet the delete() method has a comment stating 'Always call compositeRemoteDirectory - no null checks' and calls compositeRemoteDirectory.delete() unconditionally. This guarantees a NullPointerException when delete() is invoked on an instance created through that constructor path, which could cause a denial-of-service for shard cleanup.
server/src/main/java/org/opensearch/index/store/StoreFileMetadata.java	96	low	Uses 'in.available() > 0' to conditionally read the new 'dataFormat' field from StreamInput during deserialization. InputStream.available() is documented as unreliable for determining if data is truly present, especially over network streams. If it returns 0 when data is present, the field silently defaults to 'lucene', potentially causing format misclassification of remote segment files.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 0 | High: 0 | Medium: 1 | Low: 4

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

✅ Gradle check result for b6ff89e: SUCCESS

[andrross]

@mgodwan @ask-kamal-nayan This test ran out of memory:

https://build.ci.opensearch.org/job/gradle-check/74559/testReport/junit/org.opensearch.index.engine.exec.coord/DataformatAwareCatalogSnapshotTests/testDeserializationRejectsInvalidInput/

Introduced here or in #20982. Can one of you take a look?