fix(scrypt): bound the reconnect catch-up so a flapping socket cannot chain full re-fetches - #4416
Conversation
… chain full re-fetches The catch-up re-ran itself for every reconnect observed during a round. A round is a full re-fetch of both bulk streams, so once a round outlasts the interval between drops the loop re-arms faster than it completes: the venue is re-fetched continuously and it never converges. In production this ran for ten hours against a socket dropping every ~30s, two ERROR lines per failed round. - at most one round per catchUpMinInterval, however many reconnects arrive - at most catchUpMaxRounds per invocation; a later reconnect re-enters, still interval-gated - the constructor warm-up takes the first slot, so a reconnect right after boot does not repeat it - the two legs run sequentially instead of concurrently: one bulk fetch on the socket at a time - a failed catch-up stays ERROR (the missed events stay missed) but is de-duplicated to one line per streak plus a 5-minute heartbeat, and a recovered streak is logged
Review of the first commit found the bound itself was not tight and could strand work: - the slot was stamped at round start only, so a round outlasting catchUpMinInterval left the wait at <= 0 and rounds still chained back to back — exactly the regime the gate exists for. Stamp the end as well. - exhausting catchUpMaxRounds dropped a still-set catchUpPending or a still-failing leg silently. onReconnect only fires on the NEXT drop, so a socket that stabilises right there left the gap permanently unrepaired. Hand the leftover work to a scheduled re-entry and log it. - the constructor claimed the first slot unconditionally, including when both warm-up fetches rejected. Claim it only when both legs actually loaded. - a partially failing round re-fetched the healthy leg on every retry. Rounds now carry only the streams still owed. - the error de-duplication could never trigger (its interval equalled the round interval) and the test that asserted otherwise only passed because the mocked delay froze the clock. Dropped it: the round gate already bounds the output to one line per leg per interval. Tests: the success-plus-re-arm bound (the path the old loop never escaped, previously masked by a mock that threw on every round), the no-wait fast path, the end stamp, and warm-up slot claiming in both directions.
…the failed legs
Round two of review found that the "retry only the legs that actually failed" change had opened a
silent recovery gap, and both reviewers reproduced it independently.
`outstanding` carries the legs an earlier round failed on; `catchUpPending` carries something else
entirely — a reconnect whose outage postdates the round's snapshots. The flag was consumed at the
top of every round while `outstanding` stayed narrowed, so after a partially failing round a new
drop was absorbed by a round that never re-fetched the healthy leg. If the failed leg then
succeeded, nothing was owed and no retry was armed: the missed balance transactions stayed missed
until an unrelated later drop. develop did not have this hole — its loop re-ran both legs.
The pending check now sits exactly where the flag is consumed, after the slot wait, so a reconnect
arriving during the wait is serviced on both streams as well.
Also from this round:
- replace the stream switch with a keyed map, per CONTRIBUTING ("use maps instead of switch chains,
generates build errors for unmapped values"): the switch had no default, so a stream added to
catchUpStreams without a case counted as caught up although nothing was fetched
- give the scheduled retry a rejection handler — it runs detached, and an escaping error would take
the process down through main.ts
- the exhaustion warning no longer lists streams as "still owed" that in fact applied
- drop a dead optional call on unref, and correct two comments that no longer matched the code
Tests: the reconnect-during-the-slot-wait ordering (fails against the previous placement — verified
by mutation) and one that fires the scheduled retry and asserts it re-runs a round.
…ck leaking between tests Round three of review found no behavioural defect; these are the accuracy items it did find. - the pre-round `lastCatchUpAt` stamp was provably unobservable: the only reader runs earlier in the same iteration and re-entrant readers are impossible while catchUpInProgress gates them, so it was overwritten by the end stamp before anything could see it. It also contradicted the field's documented meaning, inviting the conclusion that rounds are gated from their start. - the fake-timer test restored real timers only in its last statement, so any failure inside it leaked the fake clock into the rest of the file — one regression then surfaced as two failures plus a five-second timeout in an unrelated test. Moved to afterEach, as the sibling spec does. - three comments described behaviour the code no longer has: the retry always restarts from the full pair, and a coalesced reconnect widens a round back to both legs. - catchUpMinInterval now carries its unit, per the convention the sibling connection file follows.
… behaviour - a round re-fetches each owed stream, not unconditionally both: the constant's block comment was the last place still describing the pre-narrowing behaviour - the end-stamp test's title still claimed the slot is also stamped at the round's start, which is exactly the statement the previous commit deleted - one spec comment numbered the first round 1 while the comments 70 lines below number it 0
|
Took 5 review passes (two independent lanes each — conformity and logic) to reach zero findings. What the passes changed, since several of them were real defects in the fix itself rather than in the original code:
Verification: 29 spec tests, and the ones pinning this change were mutation-checked from both sides — deleting the end stamp, reverting the pending-check placement, making the retry inert, and always- or never-widening the owed list each fail a specific named test, while re-adding the deleted pre-round stamp changes nothing. Full suite 5321 passed / 302 suites / 0 failures; Note for the reviewer: this deliberately does not touch what raised the drop rate in the first place. It removes the amplifier that turned an ordinary drop rate into a self-sustaining loop; the WS |
Why
catchUpAfterReconnectre-armed itself for every reconnect observed while it was running:A round is a full re-fetch of both bulk streams — its cost scales with the account history, not
with the length of the outage it repairs. Once a round takes longer than the interval between drops,
every round ends with
catchUpPendingalready set again, so the loop re-arms faster than itcompletes and the venue is re-fetched continuously. There is no backoff and no cap, so it cannot
converge on its own.
That is what production has been doing since 2026-07-27 09:26 UTC. The first drop was ordinary — the
socket had been closing 1–3 times per hour for days. The catch-up that followed it timed out on both
legs (30 s each), the socket died two seconds later, and it has not escaped the loop since:
code: 1006closures401(venue throttling)Median connection lifetime is now 28 s. The one connection that survived 28 minutes in the last six
hours is the one where no catch-up round ran on it.
This is not only log noise: a connection in this state is what produces unobserved order outcomes
(see #4405, where two withdrawals were recorded
Failedwhile they had executed at the venue).What changes
catchUpMinInterval(5 min), however many reconnects arrive. The stampis taken at the end of a round, so the gate is a cooldown: a round that outlasts the interval
cannot let the next one start immediately. An isolated reconnect long after the last round waits
not at all.
catchUpMaxRounds(3) rounds per invocation, and anything still owed when they run out— a failing leg, or a reconnect that arrived too late to be serviced — is handed to a scheduled
re-entry one interval later rather than dropped.
onReconnectonly fires on the next drop, sowithout this a socket that stabilises right there would leave the gap unrepaired until restart. A
live invocation supersedes a scheduled retry; the retry restarts from the full pair.
invocation. That reconnect's outage postdates the round's snapshots, so it invalidates both
streams and the next round owes both again, not just the previously-failed leg.
after boot doesn't repeat a pair of fetches that just succeeded — but does repair immediately when
the warm-up left the caches incomplete.
time. Leg isolation is unchanged: a failing execution-report fetch still doesn't cost us the
balance transactions.
until a later round restores them — so it is not downgraded. Its volume is bounded by the round
gate rather than by suppressing lines, and a recovered streak is logged.
switch, so adding onewithout a fetcher is a build error instead of a leg that reports itself caught up without fetching.
Deliberately not in this PR: what pushed the drop rate up at 09:26 in the first place is still
open. This removes the amplifier that turned an ordinary drop rate into a self-sustaining loop, and
the drop rate after it ships is the cheap read on whether anything else is wrong.
Verification
scrypt.service.spec.ts: 29 tests. The ones that pin this change: rounds stop atcatchUpMaxRoundson both the failure path and the success-plus-re-arm path (the latter is theshape the old loop never escaped); a reconnect arriving during the slot wait after a partially
failing round re-fetches the healthy leg too; the scheduled retry fires and re-enters; the slot is
stamped at the end of a round; the gate does not delay an isolated reconnect; and the warm-up
claims its slot only when both legs loaded. The two narrowing tests are pinned from both sides —
always widening breaks leg isolation, never widening breaks three cases.
tsc --noEmit,eslint,prettier --checkclean.Post-deploy verification owed
Scrypt reconnect catch-up … faileddrops to at most one line per leg per 5 min while the connectionis unhealthy; WS
1006closures and handshake401s are the number to watch — if they fall backtoward the 1–3/h baseline once we stop re-fetching, the loop was driving them.