Repository navigation
fix(replication): raise default REPLICATION_BLOBTIMEOUT from 120000 to 900000ms - #520
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increases the default replication blob timeout from 2 minutes (120,000 ms) to 15 minutes (900,000 ms) to prevent subscription termination and permanent divergence during rolling upgrades and concurrent writes. The review feedback highlights a potential CPU pegging issue if the timeout is configured to 0 or a negative value, and suggests guarding the definition of blobTimeout. Additionally, the reviewer points out that tying the blobsTimer cleanup interval to the increased timeout could delay memory reclamation and lead to out-of-memory (OOM) issues, recommending that the sweep interval be decoupled from the timeout.
| // upgrade + concurrent writes had blob transfers routinely stalling past the timeout, the | ||
| // receive watchdog then killed the subscription, and the audit cursor advanced past the | ||
| // missing blob. 900000 lets in-flight transfers complete across a peer restart window. | ||
| const blobTimeout = env.get(CONFIG_PARAMS.REPLICATION_BLOBTIMEOUT) ?? 900000; |
There was a problem hiding this comment.
If REPLICATION_BLOBTIMEOUT is configured to 0 or a negative value, blobTimeout will be 0 or negative. This will cause setInterval(..., blobTimeout) for blobsTimer to run in a tight loop (as Node.js treats 0 or negative delays as 1 ms), pegging the CPU at 100%. Additionally, a negative blobTimeout would cause all blob streams to instantly time out.
Guarding blobTimeout at its definition ensures it is always positive and safe to use throughout the file.
| const blobTimeout = env.get(CONFIG_PARAMS.REPLICATION_BLOBTIMEOUT) ?? 900000; | |
| const rawBlobTimeout = env.get(CONFIG_PARAMS.REPLICATION_BLOBTIMEOUT) ?? 900000; | |
| const blobTimeout = rawBlobTimeout > 0 ? rawBlobTimeout : 900000; |
| // blobTimeout (REPLICATION_BLOBTIMEOUT) defaults to 900000 and is shared with blobsTimer; guard | ||
| // against a misconfigured 0/negative that would otherwise forceReconnect in a tight loop. | ||
| intervalMs: blobTimeout > 0 ? blobTimeout : 120000, | ||
| intervalMs: blobTimeout > 0 ? blobTimeout : 900000, |
There was a problem hiding this comment.
With blobTimeout now guaranteed to be strictly positive (by guarding it at its definition), we can simplify the copyProgressWatchdog interval configuration and remove the redundant inline guard.
| // blobTimeout (REPLICATION_BLOBTIMEOUT) defaults to 900000 and is shared with blobsTimer; guard | |
| // against a misconfigured 0/negative that would otherwise forceReconnect in a tight loop. | |
| intervalMs: blobTimeout > 0 ? blobTimeout : 120000, | |
| intervalMs: blobTimeout > 0 ? blobTimeout : 900000, | |
| // blobTimeout (REPLICATION_BLOBTIMEOUT) defaults to 900000 and is shared with blobsTimer. | |
| intervalMs: blobTimeout, |
| // Default 15min: a 120s cap dropped ~4,500 blobs to permanent divergence when a rolling | ||
| // upgrade + concurrent writes had blob transfers routinely stalling past the timeout, the | ||
| // receive watchdog then killed the subscription, and the audit cursor advanced past the | ||
| // missing blob. 900000 lets in-flight transfers complete across a peer restart window. |
There was a problem hiding this comment.
Increasing the default blobTimeout to 15 minutes (900,000 ms) has an unintended side-effect on the blobsTimer cleanup interval (defined at line 4438), which is set to run at blobTimeout interval.
With a 15-minute interval, any stalled blob stream in blobsInFlight can remain in memory for up to 30 minutes before being swept. Under high concurrent write load with stalling transfers, this slow reclamation can lead to significant memory accumulation (holding PassThrough streams and buffers) and potentially cause out-of-memory (OOM) issues.
Consider decoupling the blobsTimer sweep interval from blobTimeout (e.g., running the timer every 30 seconds or 1 minute) while keeping the expiration check against blobTimeout.
|
Reviewed; no blockers found. |
Patch cherry-pick: mergedCherry-picked onto |
2 participants
Why
Under a v4→v5 rolling upgrade with concurrent write load (3 k6 VUs, 30k×50KB preload + workload), the 120s default
REPLICATION_BLOBTIMEOUTproduced 4,472 permanently-orphaned blob files — records committed on all nodes with refs to blob paths that no node held on disk. The mechanism: blob transfer stalls 120s → receive watchdog kills the subscription → audit cursor advances past the "unavailable" blob (harper-pro#388 divergence marker) → record permanently references a blob that will never land.Evidence
Same cluster, same k6, same roll, only the timeout changed (from 120000 to 900000 via env override):
Blob source stream idleeventsThe remaining 13 "partial" paths are mid-flight batches (adjacent fileIds), not permanent loss.
Fix
Change the default at three sites in
replication/replicationConnection.ts:blobTimeoutmain assignmentHARPER_BLOB_SEND_CHUNK_TIMEOUT_MSfalls through toblobTimeoutwhen unset, so raising the receive default cascades to the send default automatically. Operators can still override to a lower value via env var.Not addressed here
Making blob receive resumable across a source restart (so the timeout matters less) is a bigger change and stays a future PR.