Use pluggable DirectoryFactory in shard store validation (draft)

[krishna-ggk]

Description

Store.readMetadataSnapshot() and Store.tryOpenIndex() hardcode new NIOFSDirectory(path) when reading Lucene segment metadata during shard allocation. This prevents IndexStorePlugin.DirectoryFactory implementations (e.g., encryption plugins - pr-169) from reading their custom-formatted segment files during the gateway allocation path after a node restart.

Impact: After a node restart, the shard allocator calls TransportNodesListShardStoreMetadataHelper and TransportNodesGatewayStartedShardHelper to validate shard data on disk. These helpers call Store.readMetadataSnapshot() / Store.tryOpenIndex(), which create a plain NIOFSDirectory — bypassing any plugin-provided directory. If the plugin writes segments_* and .si files in a custom format (e.g., encrypted), the plain directory cannot read them, and the shard is reported as having no_valid_shard_copy, preventing recovery.

Changes

• Store.java: Added overloaded readMetadataSnapshot() and tryOpenIndex() that accept DirectoryFactory + IndexSettings. Added private openDirectory() helper that delegates to the factory when provided, falling back to NIOFSDirectory.
• IndicesService.java: Exposed directoryFactories via getDirectoryFactories() getter.
• TransportNodesListShardStoreMetadataHelper.java: Resolves DirectoryFactory from index settings and passes it to Store.readMetadataSnapshot().
• TransportNodesGatewayStartedShardHelper.java: Resolves DirectoryFactory from index settings and passes it to Store.tryOpenIndex().
• StoreTests.java: Added 5 unit tests covering custom factory invocation, null-factory fallback, and wrapping factory behavior.

Testing

• All new unit tests pass
• Verified with opensearch-storage-encryption plugin: testNodeRestartWithEncryptedIndices now passes with encrypted .si/segments_* files (previously failed with no_valid_shard_copy)
• Existing StoreTests (e.g., testCanOpenIndex, testMetadataSnapshotStreaming) continue to pass

Check List

• New functionality includes testing
• Commits are signed per the DCO using --signoff

[github-actions]

PR Code Analyzer ❗

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

'Diff too large, requires skip by maintainers after manual review'

---

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.

[krishna-ggk]

Seeking early feedback if there are any major concerns with the approach - @shwetathareja @itiyamas @udabhas @RajatGupta02

[krishna-ggk]

Question: Will this be expensive to be invoked in recover path?

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 85f08bf)

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: Add pluggable DirectoryFactory support to Store read methods Relevant files: server/src/main/java/org/opensearch/index/store/Store.java server/src/test/java/org/opensearch/index/store/StoreTests.java Sub-PR theme: Wire DirectoryFactory into gateway shard validation helpers Relevant files: server/src/main/java/org/opensearch/indices/IndicesService.java server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java server/src/main/java/org/opensearch/indices/store/TransportNodesListShardStoreMetadataHelper.java

⚡ Recommended focus areas for review ShardPath Construction In openDirectory(), the ShardPath is constructed using indexLocation.getParent() for both shardStatePath and dataPath. This assumes the index directory is always one level below the shard root. If the actual shard layout differs (e.g., custom data paths or shadow replicas), the constructed ShardPath may be incorrect, causing the DirectoryFactory to open the wrong directory or fail. if (directoryFactory != null && indexSettings != null) { ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); return directoryFactory.newDirectory(indexSettings, shardPath); Mutable Map Exposure getDirectoryFactories() returns the internal directoryFactories map directly without wrapping it in an unmodifiable view. External callers could potentially modify the map, affecting all subsequent directory factory lookups. public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { return directoryFactories; } Duplicate Logic resolveDirectoryFactory() and resolveIndexSettings() in TransportNodesGatewayStartedShardHelper are nearly identical to the same methods in TransportNodesListShardStoreMetadataHelper. This duplication increases maintenance burden and risk of divergence. Consider extracting to a shared utility class. private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( ShardId shardId, IndicesService indicesService, Settings settings, ClusterService clusterService ) { IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } /** * Resolves the {@link IndexSettings} for the given shard's index. */ private static IndexSettings resolveIndexSettings( ShardId shardId, IndicesService indicesService, Settings settings, ClusterService clusterService ) { IndexService indexService = indicesService.indexService(shardId.getIndex()); if (indexService != null) { return indexService.getIndexSettings(); } IndexMetadata metadata = clusterService.state().metadata().index(shardId.getIndex()); if (metadata != null) { return new IndexSettings(metadata, settings); } return null; } Double resolveIndexSettings Call resolveDirectoryFactory() calls resolveIndexSettings() internally, and then the call site also calls resolveIndexSettings() separately. This results in two redundant cluster state lookups per shard validation. The index settings should be resolved once and reused. Store.tryOpenIndex( shardPath.resolveIndex(), shardId, nodeEnv::shardLock, logger, resolveDirectoryFactory(shardId, indicesService, settings, clusterService), resolveIndexSettings(shardId, indicesService, settings, clusterService) ); Null IndexSettings with Non-null Factory In openDirectory(), if directoryFactory is non-null but indexSettings is null (e.g., when cluster state has no metadata for the index), the method silently falls back to NIOFSDirectory. This silent fallback could mask misconfiguration for encrypted indices, potentially causing data corruption or silent read failures instead of a clear error. if (directoryFactory != null && indexSettings != null) { ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation);

[github-actions]

Persistent review updated to latest commit c71c6b9

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 85f08bf

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid redundant cluster state lookups resolveDirectoryFactory calls resolveIndexSettings internally, but the caller in getShardInfoOnLocalNode also calls resolveIndexSettings separately. This means resolveIndexSettings is invoked twice for the same shard, which involves a cluster state lookup each time. The two methods should be refactored so resolveDirectoryFactory accepts an already-resolved IndexSettings to avoid the redundant lookup. server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java [131-146] private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( - ShardId shardId, IndicesService indicesService, - Settings settings, - ClusterService clusterService + IndexSettings indexSettings ) { - IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that resolveIndexSettings is called twice (once inside resolveDirectoryFactory and once directly by the caller), causing redundant cluster state lookups. Refactoring to pass the already-resolved IndexSettings would be a valid optimization, though the same duplication exists in TransportNodesListShardStoreMetadataHelper.java too.	Low
	Return unmodifiable map to prevent mutation The method returns the internal directoryFactories map directly, exposing a mutable reference. If the underlying map is mutable, callers could modify it unintentionally. Return an unmodifiable view to prevent accidental mutation. server/src/main/java/org/opensearch/indices/IndicesService.java [1424-1426] public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { - return directoryFactories; + return Collections.unmodifiableMap(directoryFactories); } Suggestion importance[1-10]: 4 __ Why: Returning an unmodifiable view of directoryFactories is a good defensive practice to prevent accidental mutation by callers. However, the impact is limited since this is an internal service method and the risk of unintended mutation is low in practice.	Low
	Document and assert shard path structure assumption The ShardPath is constructed with indexLocation.getParent() for both the shard path and the data path, but indexLocation already points to the index subdirectory inside the shard directory. Using indexLocation.getParent() as the shard root is correct, but this assumption may silently break if the directory structure differs. More critically, ShardPath.resolveIndex() appends "index" to the shard path, so if indexLocation is already the index directory, the resolved path will be correct — but this implicit coupling is fragile. Consider adding an assertion or comment to document this invariant, or derive the path more explicitly. server/src/main/java/org/opensearch/index/store/Store.java [695-706] private static Directory openDirectory( Path indexLocation, IndexStorePlugin.DirectoryFactory directoryFactory, IndexSettings indexSettings, ShardId shardId ) throws IOException { if (directoryFactory != null && indexSettings != null) { - ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); + // indexLocation points to the 'index' subdirectory; its parent is the shard root + Path shardRoot = indexLocation.getParent(); + assert shardRoot.resolve("index").equals(indexLocation) + : "Expected indexLocation to be the 'index' subdirectory of the shard root"; + ShardPath shardPath = new ShardPath(false, shardRoot, shardRoot, shardId); return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds an assertion to document the implicit coupling between indexLocation and the shard root path. While this improves code clarity, it's a minor defensive programming improvement and the improved_code only adds a comment and assertion without changing logic.	Low

---

Previous suggestions

Suggestions up to commit 45e8321

Category	Suggestion	Impact
General	Avoid duplicate cluster state lookups per shard resolveDirectoryFactory calls resolveIndexSettings internally, and then the call site also calls resolveIndexSettings separately. This means resolveIndexSettings is invoked twice per shard validation, which involves a cluster state lookup each time. The two methods should be refactored so resolveIndexSettings is called only once and its result is shared, avoiding redundant cluster state access. server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java [131-146] +// At the call site, call resolveIndexSettings once and pass the result to both: +IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); +Store.tryOpenIndex( + shardPath.resolveIndex(), + shardId, + nodeEnv::shardLock, + logger, + resolveDirectoryFactory(indexSettings, indicesService), + indexSettings +); + private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( - ShardId shardId, - IndicesService indicesService, - Settings settings, - ClusterService clusterService + IndexSettings indexSettings, + IndicesService indicesService ) { - IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } Suggestion importance[1-10]: 5 __ Why: The duplicate call to resolveIndexSettings (once inside resolveDirectoryFactory and once at the call site) is a real inefficiency involving cluster state lookups. The refactoring suggestion is valid and would improve performance, though the same duplication exists in TransportNodesListShardStoreMetadataHelper.java as well and would need to be addressed there too.	Low
	Return unmodifiable map to prevent external mutation The method returns the internal directoryFactories map directly, exposing a mutable reference. Callers could accidentally or maliciously modify the map. Return an unmodifiable view to prevent external mutation. server/src/main/java/org/opensearch/indices/IndicesService.java [1424-1426] public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { - return directoryFactories; + return Collections.unmodifiableMap(directoryFactories); } Suggestion importance[1-10]: 5 __ Why: Returning a direct reference to the internal directoryFactories map is a valid defensive programming concern. Wrapping with Collections.unmodifiableMap() is a straightforward fix that prevents accidental external mutation of the internal state.	Low
Possible issue	Validate ShardPath resolves to correct index directory The ShardPath is constructed using indexLocation.getParent() for both the shard state path and the data path. However, indexLocation is the index subdirectory inside the shard directory, so indexLocation.getParent() is the shard root. This is correct for the shard path, but the ShardPath constructor expects the data path to be the shard root (not the index subdirectory). Verify that shardPath.resolveIndex() inside the factory will correctly resolve back to indexLocation; if the shard UUID or custom data path is involved, this assumption may break and the factory could open the wrong directory. server/src/main/java/org/opensearch/index/store/Store.java [695-706] private static Directory openDirectory( Path indexLocation, IndexStorePlugin.DirectoryFactory directoryFactory, IndexSettings indexSettings, ShardId shardId ) throws IOException { if (directoryFactory != null && indexSettings != null) { - ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); + // indexLocation is the "index" subdirectory; its parent is the shard root + Path shardRoot = indexLocation.getParent(); + ShardPath shardPath = new ShardPath(false, shardRoot, shardRoot, shardId); + // Sanity check: the factory must resolve back to the same index directory + assert shardPath.resolveIndex().equals(indexLocation) + : "ShardPath resolves to " + shardPath.resolveIndex() + " but expected " + indexLocation; return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation); } Suggestion importance[1-10]: 4 __ Why: The concern about ShardPath construction is valid - using indexLocation.getParent() for both paths assumes a standard directory layout. However, the improved_code only adds an assertion comment without changing the actual logic, making it more of a verification suggestion. The existing code in tests uses createShardIndexDir which creates the proper structure, suggesting the author is aware of this constraint.	Low

Suggestions up to commit 63ce95b

Category	Suggestion	Impact
General	Avoid redundant double resolution of index settings resolveDirectoryFactory calls resolveIndexSettings internally, but the caller in getShardInfoOnLocalNode also calls resolveIndexSettings separately. This results in resolveIndexSettings being called twice (once inside resolveDirectoryFactory and once explicitly), which is redundant and potentially inconsistent if the cluster state changes between calls. The factory resolution should accept the already-resolved IndexSettings as a parameter to avoid the double call. server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java [131-146] private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( - ShardId shardId, IndicesService indicesService, - Settings settings, - ClusterService clusterService + IndexSettings indexSettings ) { - IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that resolveIndexSettings is called twice — once inside resolveDirectoryFactory and once at the call site. Refactoring resolveDirectoryFactory to accept pre-resolved IndexSettings would eliminate redundancy and potential inconsistency. The same pattern exists in TransportNodesListShardStoreMetadataHelper.java as well.	Low
	Return unmodifiable map to prevent external mutation The method returns the internal directoryFactories map directly, which exposes the mutable internal state of IndicesService. Callers could accidentally (or maliciously) modify the map. Return an unmodifiable view to prevent external mutation. server/src/main/java/org/opensearch/indices/IndicesService.java [1424-1426] public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { - return directoryFactories; + return Collections.unmodifiableMap(directoryFactories); } Suggestion importance[1-10]: 5 __ Why: Returning the internal directoryFactories map directly exposes mutable state. Wrapping it with Collections.unmodifiableMap() is a good defensive practice to prevent accidental modification by callers, which is a valid correctness concern.	Low
	Add assertion to validate directory path assumption The ShardPath is constructed using indexLocation.getParent() for both the shard state path and the data path. However, indexLocation already points to the index subdirectory inside the shard directory, so indexLocation.getParent() is the shard root — this is correct for the shard path. But ShardPath.resolveIndex() internally appends "index" to the data path, so passing indexLocation.getParent() is correct. However, if indexLocation is not always the index subdirectory (e.g., it could be the shard root itself in some code paths), this assumption could break. You should add a guard or assertion to ensure indexLocation is indeed the index subdirectory before calling getParent(). server/src/main/java/org/opensearch/index/store/Store.java [695-706] private static Directory openDirectory( Path indexLocation, IndexStorePlugin.DirectoryFactory directoryFactory, IndexSettings indexSettings, ShardId shardId ) throws IOException { if (directoryFactory != null && indexSettings != null) { + // indexLocation must be the 'index' subdirectory; its parent is the shard root + assert indexLocation.getFileName() != null && indexLocation.getFileName().toString().equals("index") + : "Expected indexLocation to be the 'index' subdirectory, got: " + indexLocation; ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation); } Suggestion importance[1-10]: 3 __ Why: The assertion adds a defensive check for the assumption that indexLocation is always the index subdirectory. While this is a valid concern, the PR consistently passes shardPath.resolveIndex() which always returns the index subdirectory, making this a low-impact defensive measure. Assertions are also disabled by default in production JVMs.	Low

Suggestions up to commit fff4b09

Category	Suggestion	Impact
General	Avoid duplicate index settings resolution calls resolveDirectoryFactory calls resolveIndexSettings internally, but the caller in getShardInfoOnLocalNode also calls resolveIndexSettings separately, resulting in two redundant calls to resolveIndexSettings for the same shard. This is inefficient and could be inconsistent if cluster state changes between calls. Refactor so that resolveIndexSettings is called once and the result is passed to both usages. server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java [131-146] private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( ShardId shardId, IndicesService indicesService, - Settings settings, - ClusterService clusterService + IndexSettings indexSettings ) { - IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that resolveIndexSettings is called twice — once inside resolveDirectoryFactory and once at the call site in getShardInfoOnLocalNode. Refactoring to pass the already-resolved IndexSettings avoids redundant computation and potential inconsistency, though the same duplication exists in TransportNodesListShardStoreMetadataHelper.java and would need to be addressed there too.	Low
	Return unmodifiable map to protect internal state The method returns the internal directoryFactories map directly, exposing the mutable internal state of IndicesService. This could allow callers to modify the map. Return an unmodifiable view of the map to prevent unintended mutations. server/src/main/java/org/opensearch/indices/IndicesService.java [1417-1419] public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { - return directoryFactories; + return Collections.unmodifiableMap(directoryFactories); } Suggestion importance[1-10]: 5 __ Why: Returning a direct reference to the internal directoryFactories map exposes mutable state. Wrapping it with Collections.unmodifiableMap is a straightforward defensive improvement that prevents accidental or malicious modification of the internal map by callers.	Low
	Document and validate shard path construction assumption The ShardPath is constructed using indexLocation.getParent() for both the shard path and the data path, but indexLocation already points to the index subdirectory inside the shard directory. The ShardPath constructor expects the shard state path and the data path (the shard directory itself), not the index subdirectory. Using indexLocation.getParent() is correct here, but it's worth verifying that resolveIndex() on this ShardPath will return the original indexLocation. If ShardPath.resolveIndex() appends "index" to the data path, then passing indexLocation.getParent() is correct; however, if indexLocation is not always a direct child named "index", this assumption may break. Add an assertion or comment to document this invariant. server/src/main/java/org/opensearch/index/store/Store.java [695-706] private static Directory openDirectory( Path indexLocation, IndexStorePlugin.DirectoryFactory directoryFactory, IndexSettings indexSettings, ShardId shardId ) throws IOException { if (directoryFactory != null && indexSettings != null) { - ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); + // indexLocation points to the "index" subdirectory; shardDir is its parent + Path shardDir = indexLocation.getParent(); + assert shardDir.resolve(ShardPath.INDEX_FOLDER_NAME).equals(indexLocation) + : "indexLocation must be the 'index' subdirectory of the shard directory"; + ShardPath shardPath = new ShardPath(false, shardDir, shardDir, shardId); return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation); } Suggestion importance[1-10]: 3 __ Why: The suggestion asks to add an assertion to document the invariant that indexLocation is always the "index" subdirectory. While this is a valid defensive programming practice, the improved_code is functionally equivalent to the existing_code (only adds a comment and assertion), making it a low-impact improvement.	Low

Suggestions up to commit c71c6b9

Category	Suggestion	Impact
General	Avoid duplicate index settings resolution calls resolveDirectoryFactory calls resolveIndexSettings internally, but the caller in getShardInfoOnLocalNode also calls resolveIndexSettings separately, resulting in two redundant calls to resolveIndexSettings for the same shard. This is inefficient and could lead to inconsistency if the cluster state changes between calls. Consider refactoring so resolveIndexSettings is called once and the result is passed to both usages. server/src/main/java/org/opensearch/gateway/TransportNodesGatewayStartedShardHelper.java [131-146] private static IndexStorePlugin.DirectoryFactory resolveDirectoryFactory( ShardId shardId, IndicesService indicesService, - Settings settings, - ClusterService clusterService + IndexSettings indexSettings ) { - IndexSettings indexSettings = resolveIndexSettings(shardId, indicesService, settings, clusterService); if (indexSettings == null) { return null; } String storeType = IndexModule.INDEX_STORE_TYPE_SETTING.get(indexSettings.getSettings()); if (storeType.isEmpty()) { return null; } return indicesService.getDirectoryFactories().get(storeType); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that resolveIndexSettings is called twice — once inside resolveDirectoryFactory and once separately in the caller. Refactoring to pass the already-resolved IndexSettings avoids redundant computation and potential inconsistency, though the performance impact is minor.	Low
	Return unmodifiable map to prevent external mutation The method returns the internal directoryFactories map directly, exposing a mutable reference. If the underlying map is mutable, callers could inadvertently modify it. Return an unmodifiable view to protect the internal state. server/src/main/java/org/opensearch/indices/IndicesService.java [1417-1419] public Map<String, IndexStorePlugin.DirectoryFactory> getDirectoryFactories() { - return directoryFactories; + return Collections.unmodifiableMap(directoryFactories); } Suggestion importance[1-10]: 5 __ Why: Returning a direct reference to the internal directoryFactories map exposes it to external mutation. Wrapping it with Collections.unmodifiableMap is a valid defensive programming practice to protect internal state integrity.	Low
	Add assertion to validate directory path structure assumption The ShardPath is constructed using indexLocation.getParent() for both the shard state path and the data path. However, indexLocation already points to the index subdirectory inside the shard directory, so indexLocation.getParent() is the shard root — this is correct for the shard path, but the resolveIndex() call inside DirectoryFactory.newDirectory() will append "index" again, resulting in the correct path. However, if indexLocation is not structured as /index, this assumption breaks silently. The construction should be validated or documented clearly to avoid misuse. server/src/main/java/org/opensearch/index/store/Store.java [695-706] private static Directory openDirectory( Path indexLocation, IndexStorePlugin.DirectoryFactory directoryFactory, IndexSettings indexSettings, ShardId shardId ) throws IOException { if (directoryFactory != null && indexSettings != null) { - ShardPath shardPath = new ShardPath(false, indexLocation.getParent(), indexLocation.getParent(), shardId); + // indexLocation is expected to be <shardRoot>/index; shardRoot is its parent + Path shardRoot = indexLocation.getParent(); + assert shardRoot != null && indexLocation.getFileName().toString().equals("index") + : "indexLocation must point to the 'index' subdirectory of the shard root, got: " + indexLocation; + ShardPath shardPath = new ShardPath(false, shardRoot, shardRoot, shardId); return directoryFactory.newDirectory(indexSettings, shardPath); } return new NIOFSDirectory(indexLocation); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a defensive assertion to validate that indexLocation points to the index subdirectory, which is a reasonable safety check. However, the improved_code only adds a comment and assertion without changing logic, making it a low-impact documentation/validation improvement.	Low

[github-actions]

❌ Gradle check result for c71c6b9: 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 85f08bf

[github-actions]

❌ Gradle check result for 85f08bf: 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?