Fix multi-hop replication dedup exclusion to cover directional (sendsTo) peers

[ldt1996]

Replication's multi-hop dedup exclusion (SUBSCRIPTION_REQUEST excluded list + SUBSCRIPTION_UPDATE excludeNodes) only qualified origins with replicates === true or a blanket directional sends, so a directional peer advertising { sendsTo: [...] } (every config-route peer and add_node directional peer) was never excluded from relay: each subscriber received that origin's writes once per mesh member. On a 16-node production mesh fed ~350 writes/sec through a one-way bridge route, every peer received each write ~15x; each redundant delivery also persisted a same-version audit entry (duplicate transaction-log persistence under concurrent delivery, HarperFast/harper#2485, confirmed deterministic per delivery, no double-apply; its own fix is sequenced behind HarperFast/harper#2412, so this PR closes the observed cost in the meantime), so a fleet-wide rolling restart made resume replay O(peers^2) and wedged the data database: remote-time pinned at each peer's restart moment for 75+ minutes with sockets healthy and redirects/system current.

Design (per review)

Review found that simply widening the qualifier decided exclusion from the origin's ADVERTISED intent, which is wrong in both directions the moment local config disagrees (a route with receives: false still excluded the origin from the relay: silent data loss; a receivesFrom table filter did too: the filtered tables arrived by neither path). Per @kriszyp's direction, the decision now lives where the effective configuration lives:

• subscriptionManager.computeExclusionOrigins(database) (main thread) qualifies an origin for relay exclusion only when ALL of: the effective local receive decision accepts a direct subscription (shouldReplicateFromNode, config-route precedence per harper-pro#498); the origin's advertised row covers this subscriber+database with no table exclusions (qualifiesForMultiHopExclusion, now explicitly the advertised-intent half; subscription rows never qualify, since a subscription-driven direct path carries only the listed tables (1ded492)); and the local receivesFrom filter drops nothing from it.
• The set rides the subscribe-to-node payload and is refreshed by update-exclusion-origins broadcasts whenever an hdb_nodes row changes (coalesced, deduped per worker+database, error-contained).
• The http-worker builders only APPLY the set: the initial build takes it off the connection, and the dynamic updater diffs a pushed set against the last-sent list (the worker's own hdb_nodes subscription is gone). With no set supplied a session excludes nothing beyond this node's own log: fail-open to duplicates, never to data loss.
• 570ec79's send-side coverage check and null-entry fix in getExcludedTablesForRouteEntries are retained; the qualifier comment claiming the sender filters on the same entry array is corrected (the sender prefers its own config route and falls back to the subscriber's authorization, so advertised exclusions are the visible, conservative proxy).

Verification

• Unit: multiHopExclusionQualifier.test.mjs (24 tests, including the replicates-true-with-NATS-subscriptions pin) pins the advertised-intent predicate, including agreement with isExplicitDatabaseSubscription on subscription rows (a bare { database } row no longer qualifies) and partial-coverage rejection. Full replication unit suite: 794 passing locally (built dist, mocha).
• Integration (new, the three-node regression from both review threads): integrationTests/cluster/relayExclusionEffectiveConfig.test.mjs. A advertises sendsTo B; B's route to A disables direct receive for one database and table-filters another via receivesFrom; C is an unrestricted relay. Asserts relay delivery survives for the disabled database and the filtered table, direct delivery still works for the unexcluded table, and delivery survives a subscriber restart; a gate test guarantees A's advertised row is on B before probing, so the old decision path cannot escape by racing row propagation. Run in a Linux container (3 loopback addresses): 5/5 pass on this head; on b47a080 the filtered-table test fails with 60s of writes reaching neither path (the silent-data-loss direction), which is why unit-green was not trusted: an earlier reverted attempt passed every unit test while its wiring was inert (replicateOverWS gets no routes on the outbound path).

For the human reviewer

1. The subscription-payload surface grows by one field (exclusionOrigins) plus one worker message type (update-exclusion-origins). The alternative (per-peer route map in the payload) was rejected per review direction; route knowledge stays on the main thread.
2. Send-side coverage is still inferred from the subscriber-visible row rather than advertised by the sender. If you want coverage advertising (the thread-1 reframing), that is a protocol change and a follow-up.
3. A restart-time exclusion decision now depends on the main thread's hdb_nodes view at subscribe time; a set computed before a row lands is corrected by the next broadcast. The failure mode for a missed broadcast is duplicates, not loss.

Cross-model review: pre-push CLI (prepush-review.mjs --author claude) ran on the implementation commit: Codex graded leg reviewed (findings addressed: error containment on the broadcast timer, per-worker dedupe, a test overclaim), plus Codex delta passes on the fixups (the second surfaced the stale-set retention issue fixed in 235cc81); the CI bots then flagged two blockers on 235cc81 (table-scoped subscription rows qualifying whole-database exclusion, and a missing Array.isArray guard on the config-fed entries path), fixed in 1ded492 with a clean Codex delta pass. Gemini reviews via the CI gemini-review label per head (agy is not installed on this machine); Cursor legs were pruned by the CLI's own policy for full high-risk reviews, and the Claude domain leg is unavailable from inside this session. Generated by Claude Fable 5.

Lavinia, via Claude

_{Review-Coverage: authored=claude; ran=codex,gemini(ci); unavailable=gemini-local,domain; pruned=cursor-grok,cursor-composer; rounds=7 @ 673119a}

_{Human-Review-Need: 4 @ 673119a}

[gemini-code-assist]

Code Review

This pull request introduces the qualifiesForMultiHopExclusion helper function to optimize multi-hop deduplication exclusion for directional peers, preventing redundant write deliveries and potential performance degradation during restart replays. It also updates the design documentation and adds comprehensive unit tests. The feedback suggests improving type safety in qualifiesForMultiHopExclusion by replacing the any type for the node parameter with a more specific type like Partial<NodeRecord> | null | undefined.

[kriszyp]

Cross-linking the receive-side half, since this PR refers to it as "concurrent-delivery dedup race,
tracked separately" without a link: that is
harper#2485, and I have just finished the
investigation on it.

They are complementary, and this PR is the one that removes the field cost. It stops the redundant
deliveries at the source; #2485 is the core-side invariant that a duplicate delivery, however it
arises, must not persist a second transaction-log entry. Two findings from #2485 that are relevant
to the claims in this description:

• The ×N audit entries are confirmed and deterministic at unit level, and the count is exactly the
  number of concurrent deliveries, which matches the modal 15 measured on the cluster rather than a
  timing spread.
• The value does not get double-applied, including for a commutative increment. All the twins
  fold onto the same pre-write base and every loser's retry is dropped by an identity-tie check, so
  the cost is transaction-log bytes and apply CPU rather than data corruption. That is a lower
  severity than the audit-entry amplification alone would suggest, and it means the idempotence
  sentence in DESIGN.md was right about records and wrong about the log.

#2485's own fix has been sequenced behind harper#2412 (the dual-clock identity model), so this PR is
what closes the observed problem in the meantime. Nice piece of work tracking it to the qualifier.

— Claude Fable 5.1

[claude]

No new blockers at 673119a. Verified by trace, not by claim: table-coverage exclusion, subscription-row exclusion, the Array.isArray guards, and the receive-policy move into computeExclusionOrigins are all genuinely fixed — the worker-side qualifiesForMultiHopExclusion calls are gone, replicateOverWS now only applies the pushed exclusionOrigins set. The replicates:true + leftover-subscriptions combination is unreachable (onNodeUpdate strips subscriptions whenever replicates is set) and is now pinned by a unit test.

Still genuinely open, not fixed: kriszyp's sender-side coverage-inference gap (knownNodes.ts:1139) — the author's own last reply confirms it's deferred pending a design decision, not resolved. The two routeReplicates-fallback suggestions (subscriptionManager.ts:612/624) are duplicates of each other and already reasonably deferred by the author.

[kriszyp]

PRs need to be cross-model reviewed by at least 2 other models (you need to set up all 3 for redundancy) before submitting PRs for review.
_{🤖 Reviewed with Codex}

[kriszyp]

Please account for table coverage before excluding an entire origin. For example, A advertises sendsTo: [{ target: 'B', database: 'data', excludeTables: ['T'] }], while A→C→B carries all tables. This predicate returns true, so B excludes A’s log from C, but A’s direct sender skips T at replication/replicationConnection.ts:5175. New A-origin writes to T then reach B through neither path. Previously this directional row did not qualify, so the relay delivered them. routeEntriesIncludePeer proves database authorization, not complete delivery. Conservatively retain relay delivery when matching routes have table exclusions, or make exclusion table-aware; account for receiver-side filters too. Add a three-node regression covering an excluded direct table delivered through an unrestricted relay.

[ldt1996]

Confirmed still open. The remaining gap after 570ec79 is that this check reads A's row as we see it, while the sender derives sendExcludedTables from its own config route (falling back to our authorization.replicates.sendsTo), so the two entry sets can differ and coverage may need to be advertised rather than inferred. I've laid that out with two wiring options in the main thread (#809 (comment)) and will land this together with the receive-policy fix and the three-node regression you asked for once you pick a direction.

Lavinia, via Claude

[ldt1996]

The receiver-side half is closed in a407da0: computeExclusionOrigins rejects an origin whose matching receivesFrom entries carry excludeTables, so a table B strips from its direct request keeps relay delivery. The three-node regression covers exactly your scenario (excluded direct table delivered through an unrestricted relay) and fails on b47a080 with the writes reaching neither path. The sender-side residue stands as described in the PR description's reviewer note 2: coverage is still inferred from the subscriber-visible row, so advertising it from the sender remains open if you want that follow-up.

Lavinia, via Claude

[kriszyp]

Please use the effective local receive decision when qualifying directional origins, in both this scan and the dynamic updater. A can advertise sendsTo: [{ target: 'B', database: 'data' }] while B’s configured route to A has { sends: true, receives: false }. shouldReplicateFromNode gives that local configuration precedence at replication/knownNodes.ts:697-700, so B never subscribes directly to A. Nevertheless, the new qualifier returns true from A’s registry row. If B receives data through C, it now tells C to omit A’s log, silently cutting off the previously working A→C→B path. Share the receive-policy decision with exclusion construction rather than treating the advertised send permission as proof of a direct subscription. Add a regression with an advertised sender, a locally disabled direct receive route, and an allowed relay.

[ldt1996]

Still open, and agreed on the mechanism: the builder trusts the advertised sendsTo because the http worker has no view of the effective local receive decision. A note on the false start you may have seen pushed and reverted: reading it via getConfigRouteReplicates(options, ...) at the call sites is inert, since replicateOverWS is called without routes on the outbound path, and unit tests can't catch that because they hand the route straight to the predicate. I've posted the two wiring options I can see (move construction to subscriptionManager vs pass a per-peer route map on the subscription payload) in the main thread: #809 (comment). Will add the regression you specified plus the three-node integration test with the chosen wiring.

Lavinia, via Claude

[ldt1996]

Implemented per your decision in a407da0: subscriptionManager owns the decision. computeExclusionOrigins runs shouldReplicateFromNode with the config route attached exactly as onDatabase does, so a route with receives disabled can no longer be excluded off the advertisement; the set rides the subscribe-to-node payload and update-exclusion-origins broadcasts, and the worker builders only apply it (the worker-side hdb_nodes subscription is gone). The regression you asked for is in integrationTests/cluster/relayExclusionEffectiveConfig.test.mjs: advertised sender, locally disabled direct receive route, allowed relay, exercised through the real payload wiring; the suite fails on b47a080 and passes on this head (runs in the PR description).

Lavinia, via Claude

[github-actions]

Reviewed; no blockers found. Prior blocker resolved by architectural centralization and explained stripping behavior.

The move to centralize multi-hop exclusion logic on the main thread is a significant architectural improvement. It ensures that the decision to exclude an origin's log from a relay is based on the effective local configuration (which only the main thread possesses), rather than relying on the advertised intent from the origin's registry row alone. The new guards for non-array route entries and null elements also improve robustness on the subscription and failover paths.

Regarding the prior concern about data loss when both replicates: true and subscriptions are present: the author's explanation and added tests clarify that subscriptionManager already strips subscriptions from the connection payload whenever a replicates flag is present. This ensures that the direct connection always carries the full database log, making the relay-side exclusion safe.

Suggestions (non-blocking)

• replication/subscriptionManager.ts:624 — The coverage check for receivesFrom in computeExclusionOrigins should only consult the local configuration. The current fallback to node.replicates (the peer's advertised intent) when no local config route matches is logically incorrect for a receiver-side filter check, as it picks up the peer's own filters for its sources. It should be changed to only use matchingRoute?.replicates?.receivesFrom.

[claude]

Correcting a prior review claim: an earlier automated review comment on this PR (and a subsequent gemini review) asserted this thread was "resolved by 8ce1a73." That commit does not exist anywhere in this repo's reachable history, and the current qualifiesForMultiHopExclusion is byte-for-byte the version from 6351fdd — no table-exclusion logic was ever added. Re-raising as a blocker.

What: qualifiesForMultiHopExclusion decides exclusion purely from routeEntriesIncludePeer (target+database match), which proves database-level authorization but not full-table delivery. It never consults getExcludedTablesForRouteEntries (defined just above, in this same file) to check whether the matching sendsTo entry carries excludeTables.

Why it matters: If A advertises sendsTo: [{ target: 'B', database: 'data', excludeTables: ['T'] }] and reaches B indirectly through relay C (whose route to B carries all tables), this predicate returns true, so B tells C to exclude A's log — but A's own direct connection to B skips table T. New A-origin writes to T then reach B through neither path: silent data loss, not a missed-optimization case like the rest of this PR.

Suggested fix: call getExcludedTablesForRouteEntries on the matching sendsTo entries and only qualify for exclusion when the direct path's table coverage is a superset of what the relay would carry (or conservatively skip exclusion whenever the matching route entry has any excludeTables).

[ldt1996]

Acknowledged, and the ghost-commit correction is right (ai-review-log#1465 is now marked invalid). The partial-coverage half of this landed in 570ec79 via getExcludedTablesForRouteEntries; the residual sender-side/receiver-side coverage gap is being handled together with the receive-policy blocker, pending a design decision from @kriszyp here: #809 (comment)

Lavinia, via Claude

[claude]

Correcting a prior review claim: an earlier automated review comment on this PR asserted this thread was "resolved by 8ce1a73" (a commit that doesn't exist in this repo's history — verified via git log --all / git cat-file). This call site (and its twin at line 7191, the dynamic hdb_nodes updater) are unchanged from what @kriszyp reviewed. Re-raising as a blocker.

What: Both call sites of qualifiesForMultiHopExclusion pass only the origin's own hdb_nodes row. Neither consults this node's own locally-configured receive decision (shouldReplicateFromNode / the configRouteReplicates field it prefers, knownNodes.ts:696-723).

Why it matters: An origin can advertise sendsTo: [{ target: B }] while B's own config route to that origin is { sends: true, receives: false }. shouldReplicateFromNode gives the local config precedence, so B never actually subscribes directly to that origin — but qualifiesForMultiHopExclusion trusts the advertisement alone and still tells the relay (C) to omit A's log, silently cutting off the only path B had left to receive A's writes.

Suggested fix: thread the effective local receive decision (as computed in shouldReplicateFromNode) into the exclusion qualifier at both call sites, rather than trusting the peer's advertised sendsTo in isolation.

[ldt1996]

Acknowledged. This is the open design question on the PR: both call sites run in the http worker with no view of the effective local receive decision, and the reverted getConfigRouteReplicates(options, ...) approach was inert on the outbound path. Wiring options are posted for @kriszyp here: #809 (comment); the fix lands with call-site-exercising integration coverage once he picks.

Lavinia, via Claude

[ldt1996]

@kriszyp, a design question before I wire up the fix for your two open threads (receive-policy precedence and the remainder of table coverage). Both blockers share one root cause: qualifiesForMultiHopExclusion decides from the origin's advertised intent because the exclusion builders run in the http worker, which has no access to the effective per-origin configuration (replication.routes[].replicates, the thing shouldReplicateFromNode prefers). Reading the advertisement alone is exactly the failure mode in both threads.

A caution from a false start: I briefly pushed a version that read the route via getConfigRouteReplicates(options, ...) at both builder call sites and reverted it within the hour. It was inert: on the outbound path replicateOverWS is called without routes in options (replicationConnection.ts:2909), so the parameter resolved to undefined and the predicate silently degraded to the current behavior, and it also introduced a null crash. Unit tests stayed green because they hand the route straight to the predicate; they verify the logic and never exercise the wiring. So whatever we pick needs the three-node integration coverage you asked for on both threads, exercising the call sites, and I will write that test either way.

Two options I can see for getting effective config to the builder, and you designed the #498 boundary so I would rather ask than guess:

1. Move exclusion-list construction to subscriptionManager. That is where routes lives and shouldReplicateFromNode already runs per peer, so the builder and the eligibility gate would read the same decision by construction. The complication is the dynamic hdb_nodes updater, which lives in the worker and would need the recomputed exclusion list (or the inputs to it) pushed across.

2. Keep construction in the worker and pass a per-peer route map through the subscription payload, alongside the existing configRouteReplicates field. Smaller motion, but it widens the payload surface and duplicates route knowledge in two places that can now drift.

Related, a possible reframing of your table-coverage thread, from the Codex review of b47a080: the residual gap is that the coverage check reads A's row as B sees it, while the sender computes sendExcludedTables from its own config route to B, falling back to B's authorization.replicates.sendsTo (replicationConnection.ts:5298). Those are different entry sets, so B cannot infer the sender's actual coverage from the registry row even in principle. If that is right, the comment near :5292 claiming they are the same array is wrong and, more importantly, coverage may need to be advertised by the sender (for example on the connection or in the node row) rather than inferred by the subscriber. That would also subsume the receiver-side receivesFrom filter case (B strips tables at :7493 and drops them at :6234, yet still excludes the origin from the relay).

Which direction do you want? Once you pick, I will land the wiring, the regression tests you specified on both threads, and the three-node integration test together.

Separately, the subscription-row blocker from the same review is fixed in 60c5467: the qualifier now calls isExplicitDatabaseSubscription instead of hand-rolling subscribe !== false, so it agrees with shouldReplicateFromNode, and the unit test that pinned the unsafe result now pins the safe one.

Lavinia, via Claude

[ldt1996]

Cross-model coverage for 60c5467 (the subscription-qualifier fix): Codex (codex exec, read-only, repo access) reviewed the commit against the surrounding code and found no actionable issues; static review only, unit suite run locally (794 passing in unitTests/replication). Gemini re-triggered via the gemini-review label on this head.

Lavinia, via Claude

[ldt1996]

@kriszyp, half of thread 1 is addressed in 570ec79. The rest needs a decision from Kris, because all the remaining findings turn out to be one root cause. (This consolidates and supersedes the shorter comment above asking the same question; sorry for the double post.)

Done: send-side table coverage. sendExcludedTables is derived from sendsTo independently of sends and the skip applies unconditionally, so a matching entry carrying excludeTables means the direct path omits those tables however the rest of the row reads. The qualifier now separates authorization from coverage and retains relay delivery when coverage is partial. A blanket sends no longer cancels a separate entry's filter.

Also fixed, and it predates this PR: getExcludedTablesForRouteEntries threw on a null element while routeEntriesIncludePeer skipped it, so the two disagreed on what a list contains and sendsTo: ['peer', null] crashed the former. That already affected the send-side sendExcludedTables computation independently of this change.

Still open, and they share one cause. A Codex review of the current head raised four findings; the common root is that the exclusion builder decides from the origin's advertised intent because the http worker has no access to the effective per-origin configuration.

1. B's route to A is { sends: true } with no receives/receivesFrom. shouldReplicateFromNode refuses B's direct subscription, but the qualifier returns true from A's row, so both builders tell relay C to omit A. Your thread 2, still live.
2. B's route to A carries receivesFrom: [{ source: 'A', database: 'data', excludeTables: ['T'] }]. B strips T from its table request and drops incoming T, yet still excludes A from C even where B's route to C permits T. The receiver-side half of your thread 1.
3. { replicates: false, subscriptions: [{ database: 'data' }] } qualifies, because the predicate treats absent subscribe as subscribed while isExplicitDatabaseSubscription requires it truthy. Verified: qualifier true, subscription eligibility false. Independent of the above, and an existing unit test pins the unsafe result.
4. The coverage check I just added reads A's row as B sees it, but the sender computes sendExcludedTables from its own config route to B, falling back to B's authorization.replicates.sendsTo. Those are different entry sets, so my code comment claiming they are the same array is wrong and the check can retain relay delivery unnecessarily.

The decision I don't want to guess at. Getting the effective per-origin decision to the builder means either moving exclusion-list construction to subscriptionManager, where routes lives and shouldReplicateFromNode already runs per peer (but the dynamic hdb_nodes updater is in the worker and would have to move or be fed), or passing a per-peer route map through the subscription payload (smaller, but duplicates route data into every payload and grows with mesh size). Both move the #498 boundary you designed.

And finding 4 may reframe your thread 1 rather than answer it: if the sender's table coverage isn't visible to the subscriber at all, coverage has to be advertised by the sender rather than inferred by the receiver. Worth deciding before I write either version.

Finding 3 I'll fix now regardless.

Two notes on the record. An earlier attempt at thread 2 was pushed and force-reverted: it read the local route via getConfigRouteReplicates(options, ...) at both call sites, which returns undefined on the outbound path because replicateOverWS is called without routes, so the gate was inert and it introduced the null crash above. Unit tests passed and CI was green because they hand the route straight to the predicate and never exercise the wiring, which is why both threads want the three-node fixture. Separately, ai-review-log#1465 records Gemini as "no blockers" on the strength of that reverted commit, so that entry is invalid.

On cross-model coverage: Codex now runs here and produced the findings above. There is no codex-review.yml in this repo and GEMINI_ALWAYS_ON is unset, so the only way to get two outside families today is a manual label plus a local CLI run, and a local run leaves no ai-review-log entry. Happy to write the workflow if you want all three automatic.

Lavinia, via Claude

[kriszyp]

How should qualifiesForMultiHopExclusion decide: subscriptionManager should own the exclusion decision.

Codex now runs here and produced the findings above. There is no codex-review.yml in this repo and GEMINI_ALWAYS_ON is unset, so the only way to get two outside families today is a manual label plus a local CLI run, and a local run leaves no ai-review-log entry. Happy to write the workflow if you want all three automatic.

Yes, local CLI run before a push is the point. It is already fully documented/skilled and scripted https://github.com/HarperFast/skills-internal/blob/main/skills/cross-model-review/rules/prepush-cli.md. According to your PR description you declined gemini and cursor. We do not need a new workflow or GH bot (ai-review-log entries are not necessary), simply run the local CLIs that are already scripted.

_{🤖 Reviewed with Codex}

[ldt1996]

Done, both parts.

Design: subscriptionManager now owns the exclusion decision (a407da0, with a review fixup in 235cc81). computeExclusionOrigins gates on shouldReplicateFromNode with the config route attached, the advertised-coverage qualifier, and the local receivesFrom filter; workers apply the set from the subscribe-to-node payload and update-exclusion-origins broadcasts, and exclude nothing when no set is supplied. The three-node regression exercises the real wiring and fails on b47a080 (details and run output in the updated PR description).

Process: understood on the local CLIs, and prepush-review.mjs is what ran for this round: the Codex graded leg reviewed the implementation commit (its findings are fixed in the pushed head, including a stale-set retention case its delta pass caught), with the repo checklist primed. Two legs could not run from this machine yet: agy is not installed (Gemini coverage came from the CI label in the meantime) and the Cursor legs were pruned by the CLI's own full-high-risk policy; the domain leg needs a claude on PATH, which a session cannot provide from inside itself. Lavinia will set up agy and Cursor so future rounds run all three locally, and I'll drop the workflow idea.

Lavinia, via Claude

[github-actions]

🔴 Blocker — Partial subscription leads to data loss via incorrect relay exclusion

qualifiesForMultiHopExclusion returns true for partial subscriptions (entries with a 'table' field), causing the relay to incorrectly exclude the origin for the entire database log. This leads to silent data loss for any tables not in the direct subscription. Ensure only full-database subscriptions qualify for multi-hop exclusion.

[ldt1996]

Fixed in 1ded492, and slightly stronger than suggested: subscription rows never qualify for exclusion at all, table-scoped or not. When node.subscriptions drives the direct path, replicateOverWS builds the outbound table list from only the listed entries (replicateByDefault flips off), so even a table-less subscription entry doesn't carry the whole database log. Unit tests pin every shape to false.

Lavinia, via Claude

[claude]

Blocker — partial (table-scoped) subscriptions entries qualify for full-database exclusion

What: isExplicitDatabaseSubscription(node?.subscriptions, databaseName) (called here) matches on (sub.database || sub.schema) === databaseName && sub.subscribe alone — it never checks whether the matching entry is scoped to a single table. But node.subscriptions entries ARE table-scoped in production: replicationConnection.ts:7481-7492 reads subscription.table and, when node.subscriptions is present, builds the outbound table list from only the listed tables (replicateByDefault = false). This is a live path, reachable via add_node/set_node's subscriptions: Joi.array() field (setNode.ts:23, no item-shape validation) — not dead/deprecated code. The coverage check at line 1176 only inspects directional?.sendsTo exclusions; it has no equivalent for a subscription-row match.

Why it matters: A subscriber advertising subscriptions: [{ database: 'data', table: 'A', subscribe: true }] (subscribed to exactly one table) makes this function return true for the whole database. The relay is then told to omit this origin's entire database log, so every other table from that origin reaches the subscriber through neither the direct (single-table) subscription nor the relay: silent, permanent data loss. Same failure class as the already-fixed sendsTo/excludeTables gap this PR closed, but through the subscriptions path, which was never given the same table-coverage treatment. (Corroborates the gemini finding posted minutes ago on this same function.)

Suggested fix: Either require every matching subscription entry for databaseName to have no table field before this branch authorizes (i.e., only a whole-database entry qualifies), or extend the coverage check below to also reject when any matching subscription row carries a table.

[ldt1996]

Fixed in 1ded492 by removing the subscriptions arm from the qualifier entirely rather than special-casing the table field: as you note, the direct path built from node.subscriptions carries only the listed tables, and a table-less entry pushes undefined into tableSubs, so no subscription shape proves full-database delivery. shouldReplicateFromNode keeps isExplicitDatabaseSubscription for eligibility, so subscription-driven peers still subscribe; they just never suppress the relay. Unit tests pin whole-database, schema-form, table-scoped, subscribe:false, and absent-subscribe shapes all to false.

Lavinia, via Claude

[claude]

Blocker — unguarded for...of crashes the main thread on a non-array sendsTo/receivesFrom

What: getExcludedTablesForRouteEntries does if (!entries) return null; then for (const entry of entries) with no Array.isArray guard — unlike its sibling routeEntriesIncludePeer (line 1124), which has one. subscriptionManager.computeExclusionOrigins (subscriptionManager.ts:609) feeds it routeReplicates?.receivesFrom sourced directly from this node's own routes[] YAML config — documented elsewhere in this same file as "YAML and isn't schema-validated" (subscriptionManager.ts:715-717, where the sibling computeSelfReplicates does guard with Array.isArray for exactly this reason). A non-array receivesFrom/sendsTo (an object instead of a list — an easy config mistake) throws TypeError: entries is not iterable; the same call inside qualifiesForMultiHopExclusion (line 1176) is reachable with a peer's malformed advertised row too.

Of computeExclusionOrigins's three call sites, only the debounced broadcast (scheduleExclusionOriginsBroadcast) wraps the call in try/catch and fails open. The other two — onDatabase's setTimeout callback (subscriptionManager.ts:1135) and the failover path connectToNextWorker (subscriptionManager.ts:1337) — call computeExclusionOrigins with nothing wrapping it, synchronously inside a setTimeout callback.

Why it matters: A misconfigured route in this node's own config.yaml (or a peer's malformed advertised row) crashes the main-thread process on an ordinary subscribe or during failover recovery — the two places this is least affordable. Untested: no unit or integration test exercises a non-array sendsTo/receivesFrom.

Suggested fix: Add if (!Array.isArray(entries)) return null; here, matching routeEntriesIncludePeer's existing guard — fixes all call sites at once. (Gemini flagged the same missing guard as non-blocking at knownNodes.ts:1094; escalating because two of the three call sites this feeds have no surrounding try/catch on the subscribe/failover critical path.)

[ldt1996]

Fixed in 1ded492: getExcludedTablesForRouteEntries now opens with if (!Array.isArray(entries)) return null, matching routeEntriesIncludePeer, which covers all call sites including the unguarded onDatabase and failover paths you traced. Unit test added for the non-array shapes in both helpers.

Lavinia, via Claude

[ldt1996]

Both AI summary reviews above (claude-review, 2 blockers; gemini-review, 1 blocker, at head 235cc81) are addressed in 1ded492: subscription rows never qualify an origin for relay exclusion (a subscription-driven direct path carries only its listed tables, so no shape proves full-database delivery), and getExcludedTablesForRouteEntries guards non-array entries the way routeEntriesIncludePeer already did. Point-by-point replies are on the inline threads. Verified with the unit suite (792 passing), the three-node integration suite (5/5 in the container rig), and a clean Codex delta pass; the gemini-review label was re-applied for this head and its run is in progress.

Lavinia, via Claude

[github-actions]

🔴 Blocker — Data loss risk when both replicates: true and subscriptions are present

If a node row carries both replicates: true and subscriptions, the qualifier returns true before checking the subscription array. Since replicateOverWS prioritizes subscriptions and forces replicateByDefault = false (partial coverage), excluding the origin from relays will lead to silent data loss for any tables not in the subscription list. Guard against node.subscriptions.length before the boolean-true check.

[ldt1996]

Not a live path: a row carrying both replicates and subscriptions never drives a subscription-scoped direct connection. subscriptionManager.onNodeUpdate strips subscriptions from any row whose replicates is set, before the node reaches nodeMap or any subscription payload ('they are just there for NATS', subscriptionManager.ts:952), so replicateOverWS's node.subscriptions branch never sees them and the direct path for such a row is the full database log. The qualifier's replicates === true short-circuit matches that semantic exactly; guarding on subscriptions.length here would instead reintroduce the 15x relay fan-out for every NATS-migrated mesh, whose rows commonly carry exactly this leftover shape. Pinned in 673119a with a unit test citing the strip, so this row shape stays intentional rather than accidental.

Lavinia, via Claude

[github-actions]

💡 Suggestion (non-blocking) — Narrow receiver-side exclusion check to local config only

The check for receivesFrom table exclusions should only consult the local configuration route. Falling back to the origin's advertised row (their own receive filters) when the local config is boolean is logically incorrect for determining our own receiver-side coverage, even if matching on node.name usually yields no results there.

[ldt1996]

The fallback is deliberate, for consistency with enforcement rather than semantic purity: the coverage check consults the same routeReplicates that onDatabase attaches to the subscription payload (config route object, else the row's replicates object), which is exactly what the receive-side filter reads when deciding what to strip and drop (replicationConnection.ts:7477 and :6232). If exclusion narrowed to config-only while enforcement kept the fallback, the two could disagree on a table's coverage, and exclusion disagreeing with delivery is the failure mode this PR exists to remove. You're right that a matching source === origin-name entry on the origin's own advertised row is essentially degenerate, so in practice the fallback contributes nothing; narrowing BOTH sides together would be the clean version, and I'd rather do that as a follow-up with its own test than split them here. Flagging for @kriszyp in case he wants it in this PR.

Lavinia, via Claude

[github-actions]

💡 Suggestion (non-blocking) — Logically incorrect fallback for receiver coverage check

The check should only consult the local configuration (matchingRoute?.replicates). Falling back to the peer's advertised row (node.replicates) incorrectly applies the peer's own source filters as our local receive filters.

[kriszyp]

I think this looks good. We will see how it works with 5.2...
_{🤖 Reviewed with Codex}

[kriszyp]

Please apply the empty-exclusion fallback to initial subscription construction too. The per-database catch in the broadcast path does not protect this timer callback: if the registry scan or an eligibility check throws, request construction aborts before postMessage/subscribeToNode, and the exception escapes the timer. Because this scans every origin, a failure involving another registry row can prevent an otherwise healthy peer subscription from starting. Fault injection against this callback reproduced the exception with no request posted. Use a shared safe recomputation helper that logs and returns [], including the call in connectToNextWorker, and test that a scan failure still dispatches the subscription with relay exclusion disabled.

[kriszyp]

Please use waitForCondition from clusterShared.mjs and pass its AbortSignal through to sendOperation, as required by the repository’s replication-wait invariant. If a node accepts the query but never responds, await hasRecord(...) never returns to check this deadline; .catch() does not bound a pending request. The nominal 5/15/60-second waits therefore run until the outer suite timeout instead. Apply the same bounded-probe pattern to waitForAdvertisedRowOnB.

[github-actions]

Release cherry-pick v5.2: merged

Cherry-picked onto v5.2.