Fix peer recovery activity tracking - #20178
Conversation
WalkthroughAdded explicit setLastAccessTime() calls at multiple recovery and replication entry points, removed an implicit touch from ReplicationRef constructor, adjusted tests (last-access polling behavior and lazy blob stream creation), and updated the changelog. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (9)
Comment |
afeebd6 to
4263de5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java (1)
239-247: Explicit activity bump on each received file chunkUpdating
replicationTarget.setLastAccessTime()at the top ofhandleFileChunkmakes chunk traffic reliably count as recovery activity, independent of status polling. Note thatFileChunkTransportRequestHandleralso callssetLastAccessTime()before invoking this method, so you now have two updates per chunk; that’s harmless but could be consolidated in one place later if you want to avoid duplication.server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java (1)
229-241: Good separation between “real recovery work” and status pollingThe new
recoveryTarget.setLastAccessTime()calls indoRecoveryand all the recovery request handlers ensure that only meaningful events (start/restart, files info, file chunks, clean files, prepare/finalize, handoff primary context, and translog ops, including mapping‑retry runs) advance the activity timestamp. This matches the goal of letting idle/hung recoveries time out even while status is being polled viaReplicationCollection.get().There is a bit of benign duplication (e.g., for translog ops and file chunks you now bump
lastAccessTimein both the transport handler and the downstream helper), so if you later want to tighten things up you could centralize the update in a single layer per operation type.Also applies to: 399-408, 414-425, 431-438, 447-457, 471-473, 535-552, 559-573, 585-595
server/src/test/java/org/opensearch/recovery/ReplicationCollectionTests.java (1)
96-127: Solid regression test; consider tightening latch assertion
testRecoveryTimeoutNotResetByPollingdoes a good job simulating frequent status polling and asserting that the recovery still times out (via the custom listener andfailedflag), which directly protects against the original bug.For extra clarity in failure cases, you might also assert that
latch.await(30, TimeUnit.SECONDS)itself returnstruebefore checkingfailed.get(), so a stuck test reports “recovery did not complete” rather than only “failed to timeout.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java(9 hunks)server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java(1 hunks)server/src/main/java/org/opensearch/indices/replication/common/ReplicationCollection.java(0 hunks)server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java(1 hunks)server/src/test/java/org/opensearch/recovery/ReplicationCollectionTests.java(2 hunks)
💤 Files with no reviewable changes (1)
- server/src/main/java/org/opensearch/indices/replication/common/ReplicationCollection.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (2)
server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java (1)
320-333: Aligns segment replication with explicit activity trackingCalling
target.setLastAccessTime()immediately after obtaining theReplicationRefcleanly reintroduces the “replication just (re)started” activity bump that was previously hidden inReplicationRef’s constructor. This keeps timeout behavior correct after the implicit update removal.server/src/test/java/org/opensearch/recovery/ReplicationCollectionTests.java (1)
72-88: Tests now correctly enforce explicit lastAccessTime updatesThe extra
assertBusybefore callingsetLastAccessTime()is a nice regression guard: it provescollection.get(recoveryId)alone no longer mutateslastAccessTime, and that only an explicitsetLastAccessTime()changes what subsequent observers see. This matches the new ReplicationRef behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java (1)
399-402: Preparing for translog phase now properly refreshes activity timestampUpdating
lastAccessTimeright after resolving theRecoveryTargetforPREPARE_TRANSLOGties the timeout to an actual protocol step rather than to generic collection access, which matches the stated goal.You repeat the
getSafe(...) → recoveryRef.get() → setLastAccessTime()pattern in several handlers; consider a small helper to centralize this so future handlers can’t forget the activity update.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java(9 hunks)server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java(1 hunks)server/src/main/java/org/opensearch/indices/replication/common/ReplicationCollection.java(0 hunks)server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java(1 hunks)server/src/test/java/org/opensearch/recovery/ReplicationCollectionTests.java(2 hunks)
💤 Files with no reviewable changes (1)
- server/src/main/java/org/opensearch/indices/replication/common/ReplicationCollection.java
✅ Files skipped from review due to trivial changes (1)
- server/src/test/java/org/opensearch/recovery/ReplicationCollectionTests.java
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java
- server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (8)
server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java (8)
239-241: Explicit activity bump at start/re-establish of recovery looks correctCalling
recoveryTarget.setLastAccessTime()immediately after acquiring theRecoveryTargetensures long-running local prep (e.g., pre-recovery, remote segment sync) is treated as activity and prevents premature timeout, while still decoupling from bare status polling.
416-419: Finalize phase correctly counted as recovery activityBumping
lastAccessTimebeforefinalizeRecovery(...)ensures that a recovery doing only finalize/refresh work is not timed out as idle, which is consistent with “meaningful work” semantics.
435-438: Primary-context handoff now tracked as active recovery workSetting
lastAccessTimebeforehandoffPrimaryContext(...)brings this path in line with the other phases; the handoff is a critical step and should indeed reset the inactivity timer.
449-452: Translog-ops request correctly refreshes activity timestampUpdating
lastAccessTimeupon receipt of aTRANSLOG_OPSrequest ensures that inbound translog batches keep the recovery alive and avoids timeouts being reset by unrelated collection lookups.
471-473: Extra activity bump inperformTranslogOpsis justified for retry pathCalling
setLastAccessTime()again insideperformTranslogOpscovers internal retries triggered after mapping updates (which re-enter viaperformTranslogOpswithout a new transport message), so those retries won’t be misclassified as idle; the double bump for the initial call is harmless.
536-539: Files-info phase now participates in recovery timeout semanticsMarking activity when handling
FILES_INFOaligns this early phase with the rest of the recovery pipeline and prevents a recovery from timing out while the source is legitimately sending file metadata.
560-563: Clean-files step correctly refreshes last access timeTreating
CLEAN_FILESas activity makes sense, since it can be non-trivial work and may be the only thing happening for a period; this fits well with the intent ofrecovery_activity_timeout.
585-588: File-chunk handling updates lastAccessTime at the right placeUpdating
lastAccessTimeright beforehandleFileChunk(...)ties the timer to actual segment-bytes transfer, which is central to the “no bytes transferred within the interval” heuristic and complements the additional updates in the replication layer.
|
❌ Gradle check result for 4263de5: 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? |
|
@mch2 Can you take a look at this? |
andrross
added
Indexing:Replication
|
❌ Gradle check result for 4263de5: 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? |
|
❌ Gradle check result for dfe2559: 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? |
dfe2559 to
71bd51f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java(2 hunks)server/src/test/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableServiceTests.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: gradle-check
🔇 Additional comments (3)
server/src/test/java/org/opensearch/cluster/routing/remote/RemoteRoutingTableServiceTests.java (2)
576-580: LGTM: Lazy stream creation prevents reuse corruption.The change from
thenReturntothenAnswerensures a freshInputStreamis created on each invocation, preventing checksum or footer corruption when the mock is called multiple times.
612-614: LGTM: Consistent lazy stream creation.Same pattern as the previous test method—ensures fresh stream creation per invocation.
server/src/main/java/org/opensearch/indices/replication/common/ReplicationTarget.java (1)
227-230: LGTM: Correctly prevents duplicate requests from resetting timeout.The conditional update ensures
lastAccessTimeis only touched when a listener is created (i.e., not a duplicate request), aligning with the PR objective to prevent status polling from resetting recovery timeouts.
|
❌ Gradle check result for 71bd51f: 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? |
71bd51f to
c57ea79
Compare
|
❌ Gradle check result for c57ea79: 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? |
|
@maxlepikhin You've got a code formatting error: |
c57ea79 to
8201444
Compare
Thanks, updated. |
mch2
left a comment
mch2
left a comment
There was a problem hiding this comment.
Your logic makes sense, though I'm curious what is polling/resetting the last access time here. The internal monitor is calling get on ConcurrentHashMap here - so that shouldn't be resetting the timestamp.
api calls to cat recovery also wouldn't reset this time as its fetching and reading recoverystate.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20178 +/- ##
=========================================
Coverage 73.30% 73.30%
- Complexity 71732 71770 +38
=========================================
Files 5793 5793
Lines 328056 328117 +61
Branches 47245 47257 +12
=========================================
+ Hits 240476 240532 +56
- Misses 68264 68316 +52
+ Partials 19316 19269 -47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b2fc04c to
87f8373
Compare
|
❌ Gradle check result for 87f8373: 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? |
Signed-off-by: Max Lepikhin <mlepikhin@gmail.com>
Signed-off-by: Max Lepikhin <mlepikhin@gmail.com>
Signed-off-by: Max Lepikhin <mlepikhin@gmail.com>
Signed-off-by: Andrew Ross <andrross@amazon.com>
87f8373 to
1d46b8b
Compare
|
I rebased and added a CHANGELOG entry |
|
❌ Gradle check result for 1d46b8b: 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? |
@andrross looks like the CI failed, ownership = DCO or something else? |
|
@maxlepikhin DCO is fine, looks like some sort of connection failure on test setup. I'll retry the tests. Did you answer @mch2's question above?
Did you confirm that this fixes the problem you discovered? |
@mch2 • You’re right that the monitor and cat recovery don’t mutate it. The resets were coming from the ReplicationRef constructor calling setLastAccessTime(), so every lookup bumped the clock—
even if no work followed. Examples:
- PeerRecoveryTargetService handlers (e.g., file chunk at server/src/main/java/org/opensearch/indices/recovery/PeerRecoveryTargetService.java:578-582, translog ops at :447-459) did
getSafe(...) before checking for duplicates; that lookup alone refreshed the timestamp.
- Segment replication runner (server/src/main/java/org/opensearch/indices/replication/SegmentReplicator.java:322-333) bumped it just by fetching the ref.
We removed that implicit bump and now update lastAccessTime only when we actually handle the request (createOrFinishListener when it returns non-null, plus the few non-transport paths like
doRecovery). Polling via get/getSafe no longer resets the timer, which fixes the observed timeout issue.
|
|
❌ Gradle check result for 1d46b8b: 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? |
|
|
@mch2 Can you take another look here? |
|
I'm hesitant with this change if we don't have a concrete test showing this fixes stuck replications, not simply that the time has/hasn't incremented. The change to A few other questions:
|
I tend to agree. This change is a pretty low-level change in behavior where there's a risk of an unintended regression. I would prefer to not to make a change like this until we have a clear reproduction of the problem and verification of this fix. |
Logically the change makes sense to me - only update the timestamp when there is recovery work progress, if no progress fail the recovery so it can be retried. The instance of stuck recovery in December of 2025 was not the only one. |
3 participants
Description
Prevent peer/segment recovery timeouts from being reset by mere status polling. Previously, every call to ReplicationCollection#get() bumped the recovery’s lastAccessTime, so a hung recovery could sit in INITIALIZING indefinitely. This change removes that implicit update and explicitly touches the timestamp only when meaningful work happens (start, file transfer, clean‑files, translog, finalize, etc.), ensuring indices.recovery.recovery_activity_timeout actually fails stuck recoveries.
Ensure recovery/replication touch lastAccessTime only when handling real work: centralize the update in
ReplicationTarget.createOrFinishListener(not on polling) and have explicit touches for the few non-listener entry points.Also fix routing diff test to supply a fresh blob stream per read to avoid checksum/footer corruption.”
Related Issues
Resolves #20177
Check List
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.