Skip to content

feat(replication): optional pacing floor for banked-gap reconnects — take or drop #721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
harper-joseph wants to merge 1 commit into from
Closed

feat(replication): optional pacing floor for banked-gap reconnects — take or drop #721

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions replication/replicationConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@
logger.warn?.(`[test] forcing open-but-idle replication wedge for db "${databaseName}" (harper-pro#420)`);
ws.terminate = () => {};
ws.close = () => {};
ws._socket?.pause();

Check failure on line 810 in replication/replicationConnection.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (Node.js v22) 

Property '_socket' does not exist on type 'WebSocket'.

Check failure on line 810 in replication/replicationConnection.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (Node.js v24) 

Property '_socket' does not exist on type 'WebSocket'.

Check failure on line 810 in replication/replicationConnection.ts

View workflow job for this annotation

GitHub Actions / Unit Tests (Node.js v26) 

Property '_socket' does not exist on type 'WebSocket'.

Check failure on line 810 in replication/replicationConnection.ts

View workflow job for this annotation

GitHub Actions / YCSB single-node (Node.js v24) 

Property '_socket' does not exist on type 'WebSocket'.

Check failure on line 810 in replication/replicationConnection.ts

View workflow job for this annotation

GitHub Actions / Build Harper Pro (Node.js v24) 

Property '_socket' does not exist on type 'WebSocket'.
return true; // tell the watchdog to treat this connection's byte count as frozen
}

Expand Down Expand Up @@ -2612,12 +2612,32 @@
// the next pass re-delivers — this is what makes gap-heal cadence track walk time rather
// than blobGapReconnectMs. Banking-gated by construction (this persist IS banked
// progress), so a link that banks nothing still paces at the watchdog interval. (#699)
if (immediate && copyWatermark.barrierDrained) {
logger.warn?.(
`Banked copy cursor at held blob gap for ${remoteNodeName}; reconnecting immediately to re-stream from it (harper-pro#699)`
);
if (options.connection) options.connection.forceReconnect();
else ws.terminate();
// FLOOR on the immediate reconnect: banking-gated is not rate-bounded. Under a MOVING
// transient fault supply (live write contention — distinct from a fixed damaged set,
// which banks nothing and stays watchdog-paced) every cycle banks a little, so
// back-to-back reconnects re-walk the tail continuously (measured: 27 copy starts in
// 76s) — and each reconnect aborts in-flight receives, minting PENDING stubs (#481) at
// that rate. Pace banked reconnects to half the watchdog interval, capped at 15s: heal
// cadence still tracks walk time for any realistic knob value, while a fault-dense link
// cannot churn faster than ~4 reconnects/min. The stamp lives on the shared connection
// object (each cycle is a fresh closure); a floored cycle leaves the watchdog as pacer,
// exactly like a zero-bank cycle. Inbound sessions (no connection object) have nowhere
// to carry the stamp, so they stay watchdog-paced entirely.
if (immediate && copyWatermark.barrierDrained && options.connection) {
const floorMs = Math.min(15000, Math.floor(blobGapReconnectMs / 2));
const sinceLast = Date.now() - ((options.connection as any).lastBankedReconnectAt ?? 0);
if (sinceLast >= floorMs) {
(options.connection as any).lastBankedReconnectAt = Date.now();
Comment on lines +2628 to +2630

@gemini-code-assist gemini-code-assist Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using Date.now() for pacing rate limits can be vulnerable to system clock adjustments (e.g., via NTP sync). If the system clock is adjusted backward, the calculated elapsed time could be negative, causing unexpected delays in reconnecting. Using performance.now() provides a monotonic clock that is immune to system clock changes.

Additionally, defaulting lastBankedReconnectAt to 0 when using performance.now() could cause the very first reconnect to be deferred if the process has been running for less than floorMs. We can avoid this by explicitly checking if lastBankedReconnectAt is undefined and treating it as Infinity elapsed time.

						const lastReconnect = (options.connection as any).lastBankedReconnectAt;
						const sinceLast = lastReconnect === undefined ? Infinity : performance.now() - lastReconnect;
						if (sinceLast >= floorMs) {
							(options.connection as any).lastBankedReconnectAt = performance.now();

logger.warn?.(
`Banked copy cursor at held blob gap for ${remoteNodeName}; reconnecting immediately to re-stream from it (harper-pro#699)`
);
options.connection.forceReconnect();
} else {
logger.debug?.(
connectionId,
`banked copy cursor persisted; deferring the immediate reconnect (${sinceLast}ms since the last one, floor ${floorMs}ms) — the blob-gap watchdog paces this cycle (harper-pro#699)`
);
}
}
})
.catch(onPersistFailure)
Expand Down
Loading