fix(replication): recover ping-alive setup stalls before DB_SCHEMA (#642) - #646
fix(replication): recover ping-alive setup stalls before DB_SCHEMA (#642)#646kriszyp wants to merge 9 commits into
Conversation
…e/resume Cross-model pre-push review on this branch (harper-pro#642 fix) flagged that pause()/resume() reset the full setup-timeout window instead of preserving remaining budget, so recurring short back-pressure pauses could re-grant a full window forever and let a stuck sender-side setup gate stay ping-alive indefinitely — defeating the watchdog's bounded-recovery guarantee. Track remaining budget explicitly and only reset it on arm() (a superseding request); condense a few review-flagged narration comments. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tracking Delta pre-push review flagged that the pause/resume remaining-budget calculation used Date.now(), so a wall-clock adjustment (NTP step, manual clock change) could extend or prematurely exhaust the watchdog. Switch to performance.now(), which is monotonic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Guided tour for reviewReading order:
What the tests prove, and don't: the integration test exercises the authorization gate Coverage: cross-model pre-push review (Codex graded leg + Gemini + Grok) ran twice — round 1 Honest gap: I could not get a clean local reproduction/verdict on the actual nightly chaos |
![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 implements a subscription setup recovery mechanism to prevent connections from remaining ping-alive indefinitely when sender-side setup gates stall. It introduces a subscription-setup watchdog, correlated request IDs, and timeout-bounded gates on the sender side, along with comprehensive integration and unit tests. The review feedback suggests wrapping individual node teardowns in try-catch blocks within the test cleanup hooks to prevent resource leaks, and optimizing the asynchronous subscription resolution in replicationConnection.ts to check the connection status before assigning the subscription reference.
| after(async () => { | ||
| await Promise.all([ctx.source, ctx.receiver].filter(Boolean).map((node) => teardownHarper({ harper: node }))); | ||
| }); |
There was a problem hiding this comment.
To prevent process and resource leaks, individual process termination steps in the after cleanup hook should be wrapped in try-catch blocks. If one node teardown fails, it shouldn't prevent the other node from being cleaned up.
after(async () => {
await Promise.all(
[ctx.source, ctx.receiver].filter(Boolean).map(async (node) => {
try {
await teardownHarper({ harper: node });
} catch (error) {
console.error('Failed to teardown Harper node:', error);
}
})
);
});References
- In test cleanup hooks (such as after or afterEach), wrap individual process termination or cleanup steps in try-catch blocks to ensure that a failure in one step does not prevent subsequent critical cleanup steps from executing, thereby avoiding resource and process leaks.
| after(async () => { | ||
| await Promise.all([ctx.source, ctx.receiver].filter(Boolean).map((node) => teardownHarper({ harper: node }))); | ||
| }); |
There was a problem hiding this comment.
To prevent process and resource leaks, individual process termination steps in the after cleanup hook should be wrapped in try-catch blocks. If one node teardown fails, it shouldn't prevent the other node from being cleaned up.
after(async () => {
await Promise.all(
[ctx.source, ctx.receiver].filter(Boolean).map(async (node) => {
try {
await teardownHarper({ harper: node });
} catch (error) {
console.error('Failed to teardown Harper node:', error);
}
})
);
});References
- In test cleanup hooks (such as after or afterEach), wrap individual process termination or cleanup steps in try-catch blocks to ensure that a failure in one step does not prevent subsequent critical cleanup steps from executing, thereby avoiding resource and process leaks.
| after(async () => { | ||
| await Promise.all([ctx.source, ctx.receiver].filter(Boolean).map((node) => teardownHarper({ harper: node }))); | ||
| }); |
There was a problem hiding this comment.
To prevent process and resource leaks, individual process termination steps in the after cleanup hook should be wrapped in try-catch blocks. If one node teardown fails, it shouldn't prevent the other node from being cleaned up.
after(async () => {
await Promise.all(
[ctx.source, ctx.receiver].filter(Boolean).map(async (node) => {
try {
await teardownHarper({ harper: node });
} catch (error) {
console.error('Failed to teardown Harper node:', error);
}
})
);
});References
- In test cleanup hooks (such as after or afterEach), wrap individual process termination or cleanup steps in try-catch blocks to ensure that a failure in one step does not prevent subsequent critical cleanup steps from executing, thereby avoiding resource and process leaks.
| .then(async (subscription) => { | ||
| subscriptionToHdbNodes = subscription; | ||
| // The setup wait below is bounded. If it timed out and closed this socket before the | ||
| // subscription promise eventually resolved, retire the late subscription immediately | ||
| // instead of leaking a global hdb_nodes listener after the WS close event already fired. | ||
| if (closed || wsClosed) { | ||
| const lateSubscription = subscriptionToHdbNodes; | ||
| subscriptionToHdbNodes = undefined; | ||
| lateSubscription?.end(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Ensure that asynchronous loops or deferred operations check if the connection or context has already been closed (e.g., using a closed flag) immediately before executing authorization checks or close actions, preventing redundant teardowns and spurious warnings.
.then(async (subscription) => {
// The setup wait below is bounded. If it timed out and closed this socket before the
// subscription promise eventually resolved, retire the late subscription immediately
// instead of leaking a global hdb_nodes listener after the WS close event already fired.
if (closed || wsClosed) {
subscription?.end();
return;
}
subscriptionToHdbNodes = subscription;References
- Ensure that asynchronous loops or deferred operations check if the connection or context has already been closed (e.g., using a
closedflag) immediately before executing authorization checks or close actions, preventing redundant teardowns and spurious warnings.
|
Reviewed; no blockers found. |
1 participant
What / why
Fixes harper-pro#642: a subscription's sender-side setup (dynamic authorization
subscription + database subscription placeholder) can never settle while the
WebSocket stays healthy at the transport layer (ping/pong keeps flowing), so
DB_SCHEMA/replay never start and no application progress is made — invisibleto the connection-truth machinery in #431 because that machinery correctly
sees a live socket.
Root-caused and reproduced while triaging a first-occurrence Node.js v22
nightly Integration Tests failure on
main(
connectedBitRestartChurn.test.mjs, 2026-08-03 run): the false-greensignature (
connected: true, a fresh post-recovery write never replicating)matches this issue's shape.
What changed
database subscription placeholder) with a shared timeout; classify and log
which gate stuck; close transiently on timeout so the peer retries from its
last durable cursor.
createSubscriptionSetupWatchdog)armed on a non-empty outbound
SUBSCRIPTION_REQUEST, satisfied only by thematching
DB_SCHEMAresponse (never by ping/pong/NODE_NAME), that forces areconnect if setup never progresses.
resolveSubscriptionSetupCapability),capped so an old or misbehaving peer can't disable the local recovery net.
a superseding request restarts the full timeout window rather than being
satisfied by a stale response.
monotonic clock, and only resets to a full window on
arm()for asuperseding request — recurring short back-pressure pauses can't keep
re-granting a full window forever (found in cross-model pre-push review,
fixed in this branch).
Test plan
unitTests/replication/subscriptionSetupWatchdog.test.mjs— 25 fake-timerunit tests: gate classification, capability negotiation/capping, request
correlation, cancellation/supersession, and pause/resume budget accounting
(including a repeated flapping-pause regression case).
integrationTests/cluster/subscriptionSetupRecovery.test.mjs— 3 two-nodeintegration tests proving receiver-driven and sender-driven recovery from a
deliberately-stalled setup gate, and healthy-idle stability (no
reconnect-churn on a caught-up, zero-traffic subscription). All pass locally.
Codex): round 1 found one major issue (pause/resume resetting the full
timeout window instead of preserving remaining budget) — fixed, with a new
regression test. Round 2 (delta) verdict: COMMENTS, one minor (wall
clock vs monotonic clock) — fixed. No blockers remaining.
from
connectedBitRestartChurn.test.mjs— this dev box is running ~15unrelated
harper.jsprocesses from other concurrent work at 20-90% CPUeach, and the test's timing assertions (25s reconnect threshold) are not
reliable under that contention (confirmed the same failure shape reproduces
against unmodified
origin/mainunder the same load). Relying on this PR'sCI run for that signal.
Risks & open questions
subscriptionSetupBudgetMsfalls back to the local timeout — not testedagainst a real older-version peer, only via the capability-resolution unit
tests.
subscriptionSetupRecovery.test.mjsdoes not exercise recurringbackpressure, mixed-version interop, or an end-to-end database-gate
timeout (only the authorization gate is exercised end-to-end) — noted by
the independent review, left as future coverage rather than scope creep on
this PR.
Refs #642
🤖 Generated with Claude Code