Constrain replication mesh when the system database is replicated - #572
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a directional self-record mechanism to allow the replication of the system database without collapsing hierarchical topologies into a full mesh. It derives directional routing rules from configuration routes, preserving constrained connections while allowing transitive schema and role propagation. The feedback recommends defensive programming enhancements: guarding against a falsy node name during early boot in setNode.ts to prevent potential runtime crashes, and using Array.isArray checks in subscriptionManager.ts to robustly handle potentially misconfigured non-array routing entries.
This comment has been minimized.
This comment has been minimized.
- computeSelfReplicates: Array.isArray guard on rep.sendsTo/receivesFrom instead of `|| []` — route config comes from YAML and isn't schema- validated, so a misconfigured non-array value would throw in the for...of and crash boot. Matches the existing guard in routeEntriesIncludePeer. Per gemini-code-assist review on PR #572. - systemDbPerDbDirectionRepro.test.mjs: remove unused nodeM destructure (lint failure). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…em replication Derive a directional replicates object (sendsTo/receivesFrom, per-database) for a node's own hdb_nodes record from its config routes instead of a blanket replicates:true. Lets the system db replicate for discovery/config propagation while user-db connections stay on the configured topology, enforced by the existing #498 gates. Includes two integration repros (3-tier chain; per-database opposite directions). Validated by hot-patching dist; see repro output in session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Derive a directional hdb_nodes self-record from a node's config routes (computeSelfReplicates) instead of a blanket replicates:true, so `system` can replicate for discovery/config propagation without every aggregation node opening direct connections to every discovered peer. The existing #498 gates consult the propagated directional record; opt-in, so nodes with no directional routes keep legacy full-mesh. - computeSelfReplicates + getConfiguredRoutes extracted/module-scoped; opt-in (only when >=1 directional route), explicit-none yields empty (not true). - ensureThisNode compares replicates structurally so config/deploy reloads refresh it. - setNode/addNodeBack derive the self-record the same way and drop the blanket sends:true on directional peer records (was short-circuiting the allow-list). - mergeReconstructedNode preserves a peer's last-known directional replicates through a transient decode miss (no topology widening). - Unit tests (computeSelfReplicates/mergeReconstructedNode); integration tests for transitive 3-tier, per-db opposite directions, and excluded-peer churn. - DESIGN.md documents the mechanism and its boundaries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- computeSelfReplicates: Array.isArray guard on rep.sendsTo/receivesFrom instead of `|| []` — route config comes from YAML and isn't schema- validated, so a misconfigured non-array value would throw in the for...of and crash boot. Matches the existing guard in routeEntriesIncludePeer. Per gemini-code-assist review on PR #572. - systemDbPerDbDirectionRepro.test.mjs: remove unused nodeM destructure (lint failure). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two directional-routing regressions found in review:
- knownNodes.ts scanNodesForSubscription: the reconstruct-merge guard was
`!node.url || node.shard === undefined`, but on an UNSHARDED cluster every
real decoded record has shard === undefined, so mergeReconstructedNode ran
over real records and reverted a freshly-decoded `replicates` to a stale
in-memory value during a copyApply base-copy reload (harper-pro#489) —
dropping user-db records for a peer that widened, over-connecting to one
that narrowed. Gate strictly on `!node.url`: a real record always has a
url, so only true reconstruct descriptors are merged.
- replicationConnection.ts dynamic send-authority gate: used a strict
`sub.source === thisNode && sub.database === databaseName`. A
full-replication neighbor's directional self-record advertises
`receivesFrom: [{ source }]` with NO database (wildcard), so once a node
was opted-in, its full-replication neighbors' per-database subscriptions
were rejected (close 1008) whenever the sender fell to the dynamic gate.
Delegate to routeEntriesIncludePeer (absent source/database = wildcard),
matching the receive-side gate.
- Adds an integration test driving an opted-in full-replication neighbor
through the dynamic send path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
01fa1aa to
17e4943
Compare
| * gates. A directional route that authorizes nothing yields an empty record (NOT `true`), so a node | ||
| * configured to replicate nothing does not silently re-advertise a full mesh. | ||
| */ | ||
| export function computeSelfReplicates(routeList: Iterable<any>): true | { sendsTo: any[]; receivesFrom: any[] } { |
There was a problem hiding this comment.
If I'm understanding this function correctly, this isn't self replicating, it's computing the "replicates" object for an instance, in which case, this is a clever, but poor name.
Summary
Lets the
systemdatabase replicate (node discovery + users/roles/schema propagation) without every aggregation node opening direct connections to every discovered peer. A node with directional config routes now advertises a directionalhdb_nodesself-record (derived from those routes viacomputeSelfReplicates, opt-in — a node with no directional routes keeps legacyreplicates: true) instead of a blanketreplicates: true. The existing controlled-flow gates (harper-pro#498 —shouldReplicateFromNodereceive-side, the send-authority gate) then consult the discovered peer's advertised directions instead of a permissive boolean.Purpose
Hierarchical topology: core(1) ← middle(3) ← roadside(30), tight transitive routing (data aggregates roadside→middle→core, no cross-cluster connections) and
systemreplicated so node discovery + central config propagate without hand-configuring every node. Previously mutually exclusive — replicatingsystemcaused every discovered node to open a direct socket to every other (core would hold ~30 direct roadside sockets). Docs (companion PR below) currently say to disablesystemto get tight topology; this removes that tradeoff.What changed
replication/subscriptionManager.ts—computeSelfReplicates(routeList)+getConfiguredRoutes()extracted/exported;ensureThisNodecomparesreplicatesstructurally (not just url/shard) so a stale/legacy self-record is corrected on the next boot after a route change.replication/setNode.ts—selfReplicatesForNodeWrite()preserves an existing directional self-record instead of forcingtrueonadd_node/set_node; both handlers drop the unconditionalsends: truestamped on a directional PEER record (it was short-circuiting the receive gate's allow-list to "feeds every database").replication/knownNodes.ts—mergeReconstructedNodepreserves a peer's last-known directionalreplicatesthrough a transient hdb_nodes decode miss, applied at all three reconstruct/merge call sites (including the copyApply/base-copy reload scan path, harper-pro#489).computeSelfReplicates/mergeReconstructedNode; integration tests for transitive 3-tier routing, per-database opposite directions, and a churn/hardening test proving no reconnect/socket churn to an excluded discovered peer under sustainedsystem+data mutation.replication/DESIGN.mddocuments the mechanism and its boundaries.Where to look / open items
replication/replicationConnection.tsleading-duplicate-skipqualifieschecks (~4318, ~4578) are not updated for the droppedsends: true: a directional peer ({sendsTo:[...]}, no.sends) now evaluatesqualifies=falsethere, so it's omitted from the multi-hop dedup exclusion list. Effect is a subscriber may receive some records via more than one path — idempotent, not data loss (replication applies by sequence). Deliberately left as-is rather than adding|| sendsTo?.length, which would be the unsafe direction (over-exclusion risks dropping records — cf. 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 leading-dup-skip family). A correct fix needs a subscriber+database-aware check and is a separate change. Documented in DESIGN.md.getConfiguredRoutes()reads a main-thread-only module global.setNode/addNodeBackcan run on any worker; on a brand-new node whose main-thread self-record hasn't been written yet, a worker-handledadd_nodefalls back to legacytruefor this node's own row. Self-heals viaensureThisNode's structural-diff rewrite on next boot; never hit by the primary config-route topology (self-record is written from the main thread at boot). Documented in DESIGN.md.hdb_nodesregistry (the registry relay differs from data relay). Does not cause a connection either way; documented as an accepted tradeoff.agywas unavailable) surfaced one real bug (a third decode-recovery merge site I'd missed, now fixed) and the two open items above — no other material findings.Docs
Companion PR: HarperFast/documentation#583
Testing
Unit tests (
computeSelfReplicates/mergeReconstructedNode) 14/14 pass on a realtscbuild. The churn/hardening integration test and the per-database repro pass on the real build. The transitive 3-tier repro is syntactically valid and converted from print-only to assertions, but could not get a clean local run due to contention from other test processes on the shared dev box — needs CI to confirm.Generated by Claude (Opus 4.8), cross-reviewed by Codex and Claude Sonnet 5.