Skip to content

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

Draft
ldt1996 wants to merge 2 commits into
mainfrom
fix/replication-directional-peer-exclusion
Draft

Fix multi-hop replication dedup exclusion to cover directional (sendsTo) peers#809
ldt1996 wants to merge 2 commits into
mainfrom
fix/replication-directional-peer-exclusion

Conversation

@ldt1996

@ldt1996 ldt1996 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Replication's multi-hop dedup exclusion (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.

The fix extracts the qualifier into qualifiesForMultiHopExclusion (knownNodes.ts) and adds the subscriber+database-aware branch via the existing routeEntriesIncludePeer, the exact shape replication/DESIGN.md prescribed for this gap ("a correct fix must be subscriber+database aware"). Both call sites (the initial excluded-list build and the dynamic hdb_nodes updater) now share the predicate, and the receive gate in knownNodes already used equivalent sendsTo matching, so exclusion and delivery agree on what a row advertises. The DESIGN.md bullet documenting the gap is updated to describe the fixed behavior.

Verification

  • New unit coverage: unitTests/replication/multiHopExclusionQualifier.test.mjs (16 tests) pins both directions: rows that prove direct delivery to the subscriber+database qualify (string entry, object entry, wildcard database, broadcast target, blanket sends, replicates: true, subscriptions, and the one-way bridge row shape), and rows targeting another peer, another database, or only receivesFrom never qualify (the perf(replication): fast-skip leading duplicates on resume #370/Replication: transitive/proxied re-delivery floods peers with already-applied out-of-order writes (reduce volume; complements harper#1310) #399 data-loss direction).
  • Full unit suite in the worktree: 860 passing, 1 failing — unitTests/security/injectedKeyCustody.test.mjs, which fails identically on an unmodified checkout on the authoring machine (environmental ENOENT), unrelated to this change.
  • End-to-end route: not executed here. The defect was observed live on the affected production cluster (bridge-origin writes arriving 13-15x per version at every peer, measured via read_audit_log sampling and a 15.0x audit byte-rate ratio between a peer and the origin over the same 150s window); this change suppresses the relays that produce those arrivals. A cluster integration test needs a 3-node fixture with one directional route and is flagged below rather than included.

For the human reviewer

  1. Wildcard semantics are inherited from routeEntriesIncludePeer: a truthy non-string entry with no target/source/database fields matches any peer+database (pinned in the new tests). Chosen for consistency: the same helper authorizes receive/send direction, and its docstring requires authorization and exclusion to agree on what an entry covers. The alternative is hardening the exclusion side to ignore malformed entries, which is safer for the data-loss direction but diverges from the auth gates. Cheap to reverse; a "no" costs a small follow-up to the shared helper.
  2. Exclusion now trusts a directional row's advertisement exactly the way it has always trusted replicates === true: if an origin advertises sendsTo covering a subscriber it cannot actually reach, that subscriber loses the relay path until the row changes. Deliberate parity rather than a new risk class, but it is the judgment call at the heart of the change.
  3. No integration test accompanies this PR (needs a 3-node directional-route cluster fixture). If you want it here rather than as a follow-up, say so.

Cross-model review: no outside reviewer legs were available on the authoring machine (codex/gemini/cursor CLIs not installed), so this PR carries no independent AI review coverage. Generated by Claude Fable 5.

Lavinia, via Claude

Human-Review-Need: 4 @ 6351fdd

…p exclusion

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ldt1996
ldt1996 requested a review from kriszyp September 3, 2026 13:17

@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 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.

Comment thread replication/knownNodes.ts Outdated
@kriszyp

kriszyp commented Sep 3, 2026

Copy link
Copy Markdown
Member

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

…arper#2485

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

2 participants