Skip to content

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

Closed
harper-joseph wants to merge 1 commit into
kris/copy-gap-watermarkfrom
joseph/699-banked-reconnect-floor
Closed

feat(replication): optional pacing floor for banked-gap reconnects — take or drop#721
harper-joseph wants to merge 1 commit into
kris/copy-gap-watermarkfrom
joseph/699-banked-reconnect-floor

Conversation

@harper-joseph

Copy link
Copy Markdown

Into #701's branch, deliberately separate from #720 (the repair fix): this one is a measured trade-off, not a defect fix — take it or close it without affecting the must-have.

What it does

Paces the barrier-drained immediate reconnect to min(15s, blobGapReconnectMs / 2) between banked reconnects, stamped on the shared connection object (each cycle is a fresh closure). A floored cycle leaves the blob-gap watchdog as pacer — exactly like a zero-bank cycle. Inbound sessions (no connection object) stay watchdog-paced entirely.

Why you might want it

Banking-gated is not rate-bounded. Under a moving transient fault supply (live write contention on a busy mesh) every cycle banks a little, so cycles run back-to-back — measured 27 copy starts in 76s where 5.2.2 paced 4 at the watchdog interval — and every reconnect aborts in-flight receives, minting PENDING stubs at that rate (~21k reconnects/link/day at that cadence).

Why you might not (the honest measurement)

sustained-fault harness no floor floor
copy starts 27 7
watchdog fires 1 3
blobs re-received, 17s quiet window 1115 2994
converged / copyGapCursorBanking ✔ / ✔ ✔ / ✔

A floored cycle keeps walking the unbankable tail that the immediate reconnect deliberately truncates — so reconnect/abort churn drops 4x, but re-streamed bytes go up. Your design is byte-cheaper; this is socket/abort-cheaper. With the repair fix in (#720), stub-minting self-heals on the next pass, which weakens this PR's case — the residual argument is reconnect-rate pathology (session churn, subscription re-setup) on fault-dense meshes.

The kohls cluster is unaffected either way: its fixed damaged sets bank nothing per cycle, so those links are watchdog-paced with or without this.

🤖 Generated with Claude Code

…nnectMs/2) (#699)

The barrier-drained immediate reconnect is banking-gated but not rate-bounded:
under a MOVING transient fault supply every cycle banks a little, so cycles run
back-to-back (measured: 27 copy starts in 76s where 5.2.2 paced 4 at the
watchdog interval), and each reconnect aborts in-flight receives, minting
PENDING stubs at that rate. Pace banked reconnects to half the watchdog
interval, capped at 15s; a floored cycle leaves the watchdog as pacer, exactly
like a zero-bank cycle. Fixed damaged sets (the kohls case) bank nothing per
cycle and are unaffected.

Measured trade (sustained-fault harness): copy starts 27 -> 7, watchdog fires
1 -> 3, but blobs re-received in a 17s quiet window 1115 -> 2994 — a floored
cycle keeps walking the unbankable tail that the immediate reconnect
deliberately truncates. Reconnect/abort churn down, re-streamed bytes up.
copyGapCursorBanking passes either way (floor=1.5s at its 3s knob).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a rate-limiting floor on immediate reconnects for banked copy cursors to prevent rapid reconnection loops under transient write contention. The feedback suggests using performance.now() instead of Date.now() to provide a monotonic clock that is immune to system clock adjustments, and explicitly handling the initial undefined state of the last reconnect timestamp to prevent unintended deferrals during early process uptime.

Comment on lines +2628 to +2630
const sinceLast = Date.now() - ((options.connection as any).lastBankedReconnectAt ?? 0);
if (sinceLast >= floorMs) {
(options.connection as any).lastBankedReconnectAt = Date.now();

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();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant