cherry-pick: fix(replication): don't treat an unresolved subscription placeholder as a resolved subscription (conflicts → v5.1) - #636
Conversation
…as a resolved subscription A replication connection for a database can be handed the unresolved placeholder Promise from createPendingDatabaseSubscription instead of the database's registered IterableEventQueue, because Replicator.subscribe() (which registers the queue) may not have run yet on that thread. Two use sites read it as if it were resolved: - The inbound-record path called `.send()` on the Promise, so every inbound replication message threw `<x>.send is not a function` and every record in it was dropped. - sendSubscriptionRequestUpdate read `auditStore`/`dbisDB` off it, so nodeId came back undefined, no `seq` resume cursor resolved, startTime fell back to 1, and the node requested a full copy of every database on every restart while a current cursor sat on disk. The fixes are asymmetric because the requirements are. The record path genuinely needs the resolved queue, so it waits (awaitPendingSubscription, bounded by SUBSCRIPTION_RESOLVE_TIMEOUT, pausing socket intake so a peer mid-copy can't queue a timeout's worth of frames into the message chain). The handshake must NOT wait — a peer only sends DB_SCHEMA in response to a SUBSCRIPTION_REQUEST, and that schema is what creates the tables whose registration resolves the placeholder — so resolveDatabaseStores reads the stores off a local table instead, both being per-database. Empty stores then mean what `startTime === 1` always assumed: no local tables, i.e. a genuine bootstrap. Also fixes the placeholder being registered in the module-level databaseSubscriptions map while being looked up in `options.databaseSubscriptions || databaseSubscriptions`, which would leave it pending forever for a caller that passes its own map. Fixes #622 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…scription handler Addresses the Gemini PR review on #623: - A non-numeric `replication_subscriptionResolveTimeout` / `replication_pauseStallTimeout` reached the timers as NaN, which fails silently in both directions: setTimeout coerces it to ~0 (the subscription-resolve bound fires immediately) while every `elapsed >= NaN` watchdog comparison is false (the paused-liveness watchdog never fires at all). Flooring PAUSE_STALL_THRESHOLD_MS at SUBSCRIPTION_RESOLVE_TIMEOUT had coupled the two, so one bad value could disarm the watchdog. positiveMsOr guards the whole expression rather than each input, so nothing non-numeric escapes. - The detached `.then()` that swaps in the resolved subscription had no `.catch()`, so a rejection would surface as a process-level unhandled rejection instead of a connection error (the DESIGN item-12 / harper-pro#466 trap). The placeholder has no reject path today; log and let the receive path's bounded wait close-and-reconnect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Chris Barber <chris@harperdb.io>
| // Waiting unbounded there would silently wedge the serialized message chain with no watchdog to catch it | ||
| // — the receive watchdog keeps being reset by the very frames we are not processing — so we close instead | ||
| // and let reconnect backoff retry, which costs one log line per attempt rather than one per message. | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Critical: unresolved cherry-pick conflict markers — this file will not parse
The cherry-pick of #623 was never resolved. replicationConnection.ts still contains raw conflict markers (<<<<<<< HEAD / ======= / >>>>>>>) at lines 191, 311, and 1599, and both sides of each conflict were kept. As a direct consequence positiveMsOr is now declared twice (lines 148 and 162). This is a syntax error: the replication module won't load and the worker won't start, so the underlying #623 fix does not actually take effect.
The intended resolution is the c6c1d8e side throughout — the positiveMsOr-guarded SUBSCRIPTION_RESOLVE_TIMEOUT / PAUSE_STALL_THRESHOLD_MS, the shouldCloseSendAuthWatch helper, and the .then().catch() on the placeholder swap. Delete the HEAD/65dbf7b sides and every marker, and remove the duplicate positiveMsOr at line 162.
Do not merge until the conflicts are resolved.
—
Generated by Barber AI
| let subscriptionToTable = databaseSubscriptions.get(request.database); | ||
| if (!subscriptionToTable) { | ||
| // Wait for it to be created | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Critical: unresolved cherry-pick conflict markers
Lines 639–647 still contain raw conflict markers; both logger.info variants were retained. Keep the template-literal form (Waiting for subscription to database ${request.database}, the b2210aa side), then remove the markers and the string-concatenation duplicate.
—
Generated by Barber AI
| resolveDatabaseStores, | ||
| createPendingDatabaseSubscription, | ||
| awaitPendingSubscription, | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Critical: unresolved cherry-pick conflict markers in the test file
Raw conflict markers remain at lines 27, 188, and 214, so this test module won't even import. Take the side that keeps the positiveMsOr import and its describe block, and delete all markers — otherwise the new coverage cannot run and CI can't validate the fix.
—
Generated by Barber AI
|
|
||
| 13. **A connection's local subscription may be an unresolved placeholder — never read `send`/`auditStore`/`dbisDB` off it.** A replication connection for a database is set up independently of `Replicator.subscribe()`, which registers that database's `IterableEventQueue` in `databaseSubscriptions` when the first of its tables is set up on this thread. Whichever loses the race, the connection is handed the placeholder Promise from `createPendingDatabaseSubscription` instead. Both use sites got this wrong (harper-pro#622): the receive path called `.send()` on it (a `<x>.send is not a function` per inbound message, every record in it dropped — 329k errors / 500MB of `hdb.log` in 8 minutes on a 12-node cluster), and `sendSubscriptionRequestUpdate` read `auditStore`/`dbisDB` off it, so `nodeId` was `undefined`, no `seq` cursor resolved, `startTime` fell back to `1` and the node **requested a full copy of every database on every restart while a current cursor sat on disk**. The two fixes are asymmetric because their requirements are: (a) the record path genuinely needs the resolved queue, so it waits (`awaitPendingSubscription`, bounded by `SUBSCRIPTION_RESOLVE_TIMEOUT` — nothing else watches a wedged `messageProcessing` chain, since the receive watchdog is reset by the very frames not being processed — and pausing socket intake for the wait, because blocking that chain does **not** stop `ws.on('message')` from appending closures that each retain a whole inbound frame, so a peer mid-copy would OOM the worker before the timeout fired; `PAUSE_STALL_THRESHOLD_MS` is floored above the timeout so the paused-liveness watchdog can't pre-empt the wait); (b) the handshake must **not** wait — an empty node bootstrapping a database it does not have locally would deadlock (the peer only sends `DB_SCHEMA` in response to a subscription request, and that schema is what creates the tables that resolve the placeholder) — so `resolveDatabaseStores` reads the stores off a local table instead, both being per-database (`rootStore.auditStore` / `rootStore.dbisDb`) with the subscription queue only a carrier. Empty stores then mean what `startTime === 1` always assumed: no local tables at all, i.e. a genuine bootstrap. Compare `readDbisCursorSync` (#476/#484) — same "`undefined` masquerades as no resume cursor → spurious full copy" failure, different source of the `undefined`. | ||
|
|
||
| 13. **A connection's local subscription may be an unresolved placeholder — never read `send`/`auditStore`/`dbisDB` off it.** A replication connection for a database is set up independently of `Replicator.subscribe()`, which registers that database's `IterableEventQueue` in `databaseSubscriptions` when the first of its tables is set up on this thread. Whichever loses the race, the connection is handed the placeholder Promise from `createPendingDatabaseSubscription` instead. Both use sites got this wrong (harper-pro#622): the receive path called `.send()` on it (a `<x>.send is not a function` per inbound message, every record in it dropped — 329k errors / 500MB of `hdb.log` in 8 minutes on a 12-node cluster), and `sendSubscriptionRequestUpdate` read `auditStore`/`dbisDB` off it, so `nodeId` was `undefined`, no `seq` cursor resolved, `startTime` fell back to `1` and the node **requested a full copy of every database on every restart while a current cursor sat on disk**. The two fixes are asymmetric because their requirements are: (a) the record path genuinely needs the resolved queue, so it waits (`awaitPendingSubscription`, bounded by `SUBSCRIPTION_RESOLVE_TIMEOUT` — nothing else watches a wedged `messageProcessing` chain, since the receive watchdog is reset by the very frames not being processed — and pausing socket intake for the wait, because blocking that chain does **not** stop `ws.on('message')` from appending closures that each retain a whole inbound frame, so a peer mid-copy would OOM the worker before the timeout fired; `PAUSE_STALL_THRESHOLD_MS` is floored above the timeout so the paused-liveness watchdog can't pre-empt the wait); (b) the handshake must **not** wait — an empty node bootstrapping a database it does not have locally would deadlock (the peer only sends `DB_SCHEMA` in response to a subscription request, and that schema is what creates the tables that resolve the placeholder) — so `resolveDatabaseStores` reads the stores off a local table instead, both being per-database (`rootStore.auditStore` / `rootStore.dbisDb`) with the subscription queue only a carrier. Empty stores then mean what `startTime === 1` always assumed: no local tables at all, i.e. a genuine bootstrap. Compare `readDbisCursorSync` (#476/#484) — same "`undefined` masquerades as no resume cursor → spurious full copy" failure, different source of the `undefined`. |
There was a problem hiding this comment.
Low: DESIGN item 13 duplicated
Item 13 now appears twice (lines 117 and 119) — v5.1 already carried this entry, so the cherry-pick re-added an identical paragraph. Drop the duplicate.
—
Generated by Barber AI
|
Closing this as redundant and unsafe to merge. The fix is already on This branch still carries unresolved conflict markers. That combination matters because the PR is — KrAIs (Claude Opus 5) |
2 participants
Cherry-pick of PR #623 onto `v5.1` produced conflicts on commit(s): `65dbf7b5c50959cf3ad10f9c407b631407ae5a32 c6c1d8e b2210aa`.
Resolve the conflict markers on branch `cherry-pick/v5.1/pr-623` and merge this PR.
@claude please review branch `cherry-pick/v5.1/pr-623` and suggest a patch that resolves the conflict markers (
<<<<<<</=======/>>>>>>>) introduced by cherry-picking PR #623 onto `v5.1`. Post the suggested patch as a comment here — do not push.