Restore size-based periodic flush on remote-store shards

[shourya035]

Description

Problem

For remote-store indices, successful remote segment uploads suppress the size-based flush condition, as described in #22802. Every successful segments sync advances minSeqNoToKeep to the last refreshed checkpoint; RemoteFsTranslog#getMinUnreferencedSeqNoInSegments() returns that moving boundary, so InternalTranslogManager#shouldPeriodicallyFlush() measures translog bytes above an ever-advancing floor and the index.translog.flush_threshold_size condition never trips. Under sustained writes with regular refreshes the safe commit falls arbitrarily far behind the current sequence number, SoftDeletesPolicy#getMinRetainedSeqNo() stays pinned to the stale safe commit, SoftDeletesRetentionMergePolicy carries the protected soft deletes through merges, and docs.deleted grows continuously under update-heavy or delete-heavy workloads.

As laid out in #22805, this is the compound effect of two deliberate changes: #7383 detached remote translog retention from the safe commit (for failover latency), and #10761 aligned the flush-size check with that retention boundary (to bound retained translog for relocation). Together they reinterpreted flush_threshold_size from "bytes written since the last commit" to "currently retained translog bytes" — which serves the disk/recovery-bounding job of the size-based flush but drops the commit-lag-bounding job.

Solution

Instead of reinstating commit-relative translog byte accounting (the approach drafted in #22805), this change bounds the safe-commit lag directly with a segment-size signal, scoped to remote-store shards, and leaves all translog accounting untouched — so the retention and flush-sizing behaviors introduced by #7383/#10761 are structurally unaffected.

1. Producer — RemoteStoreRefreshListener already builds a map of post-refresh local segment file names to sizes on every upload cycle. After a successful segments sync (the same callback that advances minSeqNoToKeep), it now computes the total size of segment files not yet referenced by the last commit point (an in-memory set diff against lastCommittedSegmentInfos.files(false) — no additional I/O) and publishes it to the engine. The file-level diff intentionally counts per-segment update files (live docs, doc-values updates) written onto already-committed segments, so tombstone accumulation contributes to the signal.
2. Consumer — InternalEngine#shouldPeriodicallyFlush() gains a condition that triggers the existing async flush path when the published bytes breach the threshold. The published value is stamped with the commit generation it was computed against and is only honored while that generation is current: a stale stamp (right after a flush, before the next sync republishes) can never re-trigger a flush, so flush loops are impossible by construction. Hot-path cost is a volatile read plus two comparisons.
3. Settings — two new dynamic index settings:
   • index.remote_store.flush_on_uncommitted_segments.enabled (default true): when disabled, the accounting is neither published (listener short-circuits) nor consulted (engine re-checks live, so disabling takes effect immediately)
   • index.remote_store.flush_on_uncommitted_segments.threshold_size: defaults to the index's current index.translog.flush_threshold_size via setting fallback — so the operator-configured flush threshold regains its "flush after this much work since the last commit" meaning on remote store, with segment bytes as the measure

Once the threshold is exceeded the shard flushes, the safe commit advances, and subsequent merges can reclaim eligible soft-deleted documents — the expected behavior from #22802.

Document-replication shards and replicas (NRTReplicationEngine) are structurally unaffected: the accounting is only ever published on remote-store primaries, and the engine condition additionally requires the enabled index setting.

Notes

• If uploads are failing, the published counter stops advancing — coherent, because minSeqNoToKeep also stops advancing in that regime, which re-arms the existing translog-size condition; the two signals hand off to each other.
• Between a commit and the next successful sync the condition is silently off (conservative: no spurious flushes in that window).
• segments_N files are excluded from the accounting.
• Segment bytes and translog bytes are different measures of "work since commit"; for the pathological workloads in [BUG] Remote-store refreshes can suppress size-based flushes #22802 (update/delete-heavy), merged segments carrying retained soft deletes are counted while uncommitted, which is exactly the growth this bounds.

Related Issues

Resolves #22802

Check List

• Functionality includes testing
• API changes companion pull request created, if applicable
• Public documentation issue/PR created, if applicable (two new index settings — documentation-website issue needed)
• Commits are signed off per the DCO

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 Reviewer Guide 🔍

(Review updated until commit af4ae90)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Potential race in stale-value detection In shouldFlushOnUncommittedSegmentBytes(), this.uncommittedSegmentBytes is snapshotted first, then this.lastCommittedSegmentInfos is snapshotted afterwards. If a flush occurs between these two reads, the following can happen: an old uncommittedSegmentBytes (generation N) is read, then the new lastCommittedSegmentInfos (generation N+1) is read — mismatch is correctly detected. However, the reverse ordering issue in updateUncommittedSegmentBytes() is more subtle: committedInfos is snapshotted, committedFiles is computed, and then uncommittedSegmentBytes is published — but if a flush occurs between reading committedInfos and publishing, the stored generation refers to an already-old commit, and shouldFlushOnUncommittedSegmentBytes() correctly rejects. The write ordering is safe, but note that a purely additive segment file (not yet committed) whose size later grows on disk (e.g., due to doc-values updates writing new per-segment files) will only be reflected after the next segments sync — this is documented as intended, but worth verifying that the bytes stamped are always ≤ the actual uncommitted bytes at read time. private boolean shouldFlushOnUncommittedSegmentBytes() { final UncommittedSegmentBytes current = this.uncommittedSegmentBytes; if (current == null) { return false; } final IndexSettings indexSettings = config().getIndexSettings(); if (indexSettings.isFlushOnUncommittedSegmentsEnabled() == false) { return false; } // snapshot the volatile once so the null check and the generation comparison observe the same commit point final SegmentInfos committedInfos = this.lastCommittedSegmentInfos; return committedInfos != null && current.bytes > 0 && current.bytes >= indexSettings.getFlushOnUncommittedSegmentsThresholdSize().getBytes() && current.committedInfosGeneration == committedInfos.getGeneration(); } Segment file name filter may skip commit-relevant files The check file.getKey().startsWith(IndexFileNames.SEGMENTS) == false excludes segments_N and pending_segments_N from the byte count. However, IndexFileNames.SEGMENTS is the string "segments", which also matches nothing else in the Lucene file naming scheme, so this is correct. Confirm the intent: this filter is meant to exclude the commit file itself (which is not in committedInfos.files(false)), so that a not-yet-committed segments_N+1 file present locally is not counted. If committedInfos.files(false) already excludes the segments file, this filter is redundant; if it does not, the filter is needed. Worth a brief comment clarifying which case applies. for (Map.Entry<String, Long> file : localSegmentsSizeMap.entrySet()) { if (committedFiles.contains(file.getKey()) == false && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { bytes += file.getValue(); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to af4ae90

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard against concurrent commit during computation The snapshot of lastCommittedSegmentInfos is taken at the start of updateUncommittedSegmentBytes, but if a flush completes concurrently between this snapshot and the assignment of uncommittedSegmentBytes, a stale (larger) byte count could be stamped against an outdated generation and later be interpreted correctly, but a newer commit's generation could also concurrently overwrite. Re-read lastCommittedSegmentInfos after the computation and verify the generation is unchanged; otherwise skip the publish to avoid racing with a concurrent flush. server/src/main/java/org/opensearch/index/engine/InternalEngine.java [1559-1565] long bytes = 0; for (Map.Entry<String, Long> file : localSegmentsSizeMap.entrySet()) { if (committedFiles.contains(file.getKey()) == false && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { bytes += file.getValue(); } } +// ensure the commit point did not change while we were computing +if (this.lastCommittedSegmentInfos != committedInfos) { + return; +} uncommittedSegmentBytes = new UncommittedSegmentBytes(bytes, committedInfos.getGeneration()); Suggestion importance[1-10]: 2 __ Why: The existing code already handles this race: the shouldFlushOnUncommittedSegmentBytes() check compares the stamped generation against the current lastCommittedSegmentInfos.getGeneration(), so stale values are rejected. The suggestion's additional guard is redundant and the code comment already explicitly explains this design.	Low

---

Previous suggestions

Suggestions up to commit 09889d0

Category	Suggestion	Impact
General	Guard against concurrent-flush stamping race There is a TOCTOU race between reading this.lastCommittedSegmentInfos at the top and stamping with committedInfos.getGeneration() here: if a flush completes concurrently, the stamped generation may be older than the current last commit while the file set was computed against the older commit, but a later republish against a still-newer commit could then be rejected forever. Consider re-reading lastCommittedSegmentInfos and verifying it still equals the snapshot before publishing, or skip publish when it has changed. server/src/main/java/org/opensearch/index/engine/InternalEngine.java [1559-1565] long bytes = 0; for (Map.Entry<String, Long> file : localSegmentsSizeMap.entrySet()) { if (committedFiles.contains(file.getKey()) == false && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { bytes += file.getValue(); } } -uncommittedSegmentBytes = new UncommittedSegmentBytes(bytes, committedInfos.getGeneration()); +if (this.lastCommittedSegmentInfos == committedInfos) { + uncommittedSegmentBytes = new UncommittedSegmentBytes(bytes, committedInfos.getGeneration()); +} Suggestion importance[1-10]: 3 __ Why: The suggestion's premise is partially correct but the proposed fix does not solve the described issue; the code author already documents that a stale stamp is only ever transiently suppressive and gets republished on the next successful segments sync, so the concern is largely mitigated by design.	Low

Suggestions up to commit 3df1ff1

Category	Suggestion	Impact
Possible issue	Prevent stale commit stamp race window There is a TOCTOU race between reading lastCommittedSegmentInfos at the start of the method and stamping its generation at the end: a flush between these points can make the published stamp match the newer commit while the committedFiles set was computed against the old commit, causing uncommitted bytes to be under-counted (post-flush files erroneously treated as committed) and preventing subsequent flushes. Re-read lastCommittedSegmentInfos and verify its generation equals committedInfos.getGeneration() before publishing; otherwise discard. server/src/main/java/org/opensearch/index/engine/InternalEngine.java [1559-1565] long bytes = 0; for (Map.Entry<String, Long> file : localSegmentsSizeMap.entrySet()) { if (committedFiles.contains(file.getKey()) == false && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { bytes += file.getValue(); } } +// Ensure no flush occurred while we were computing; otherwise the stamp/files pair would be inconsistent. +final SegmentInfos afterInfos = this.lastCommittedSegmentInfos; +if (afterInfos == null || afterInfos.getGeneration() != committedInfos.getGeneration()) { + return; +} uncommittedSegmentBytes = new UncommittedSegmentBytes(bytes, committedInfos.getGeneration()); Suggestion importance[1-10]: 4 __ Why: The PR author's comment explicitly documents that this race can only suppress a flush trigger until the next successful segments sync republishes, never cause a spurious flush, making this a minor defensive improvement rather than a correctness fix. The suggestion also does not fully close the race window since a flush could still occur between the re-read and the publish.	Low

Suggestions up to commit d3c52b2

Category	Suggestion	Impact
General	Snapshot volatile field to avoid race The lastCommittedSegmentInfos field is volatile and read twice (once in shouldPeriodicallyFlush via userData and once here via getGeneration). To avoid a race where a concurrent flush swaps the field between the two reads (making the generation stamp appear stale against a newer commit that already includes the uncommitted bytes, or vice versa), capture it into a local first and compare against that snapshot. server/src/main/java/org/opensearch/index/engine/InternalEngine.java [1580-1582] -return current.bytes > 0 +final SegmentInfos committed = this.lastCommittedSegmentInfos; +return committed != null + && current.bytes > 0 && current.bytes >= indexSettings.getFlushOnUncommittedSegmentsThresholdSize().getBytes() - && current.committedInfosGeneration == lastCommittedSegmentInfos.getGeneration(); + && current.committedInfosGeneration == committed.getGeneration(); Suggestion importance[1-10]: 4 __ Why: Snapshotting the volatile lastCommittedSegmentInfos into a local is a minor defensive improvement, but the existing code's correctness is not obviously broken since a stale stamp only leads to a suppressed or extra flush, which is acceptable in this best-effort accounting.	Low
	Guard against null segment sizes Long values from localSegmentsSizeMap may theoretically be null (defensive), and more importantly file.getValue() auto-unboxing would NPE. Also, if the map contains entries with null size the summation crashes and drops the accounting. Guard against null values to keep this a best-effort computation. server/src/main/java/org/opensearch/index/engine/InternalEngine.java [1555-1559] for (Map.Entry<String, Long> file : localSegmentsSizeMap.entrySet()) { - if (committedFiles.contains(file.getKey()) == false && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { - bytes += file.getValue(); + Long size = file.getValue(); + if (size != null + && committedFiles.contains(file.getKey()) == false + && file.getKey().startsWith(IndexFileNames.SEGMENTS) == false) { + bytes += size; } } Suggestion importance[1-10]: 2 __ Why: The localSegmentsSizeMap is populated from Directory.fileLength() which returns primitive long, so null values are not realistically expected. The suggestion is overly defensive with marginal impact.	Low

[github-actions]

Persistent review updated to latest commit 3df1ff1

[github-actions]

Persistent review updated to latest commit 09889d0

[github-actions]

❌ Gradle check result for 09889d0: 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 af4ae90

[github-actions]

✅ Gradle check result for af4ae90: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.57%. Comparing base (608d710) to head (af4ae90).

Files with missing lines	Patch %	Lines
...va/org/opensearch/index/engine/InternalEngine.java	80.64%	4 Missing and 2 partials ⚠️
...search/index/shard/RemoteStoreRefreshListener.java	25.00%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22865      +/-   ##
============================================
- Coverage     71.58%   71.57%   -0.02%     
+ Complexity    77341    77301      -40     
============================================
  Files          6170     6170              
  Lines        359775   359824      +49     
  Branches      52478    52490      +12     
============================================
- Hits         257541   257539       -2     
+ Misses        81770    81766       -4     
- Partials      20464    20519      +55     

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

🚀 New features to boost your workflow:

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