Repository navigation
Replication W11: Code organization, protocol versioning & decode-loop safety #440
Description
Activity
The inner per-record value-decode silent-gap residual (flagged during #511's review) now has a fix: #521 (stacked on #511).
On a resolved-decoder value-decode failure the inner catch re-throws onto #511's
closeOnInboundMessageErrorpath (close 1011 → reconnect + resume from durable cursor) instead of skip-and-logging whilemaxBatchVersionadvances past the record. The reconnect rebuilds the per-connection table decoder from the peer's re-sent structures, so the #1163/#1453 structure-fork class heals on resume; unknown-tableIdstill skips (distinct transient schema-propagation case).Cross-model reviewed (Codex + Gemini + domain), no blockers. Two follow-ups surfaced for W2/#432: (1) a bounded-retry escalation budget so a genuinely-undecodable frame can't reconnect-loop forever, and (2) the pre-existing frame-prefix-commit-on-reconnect tradeoff (inherent to the close-resume path since #511) if strict per-frame atomicity is ever required — plus a minor blob-orphan-on-abort sweep.
- added 7 commits that reference this issue
on Jul 10, 2026 A second, per-feature capability handshake has landed — W11 should subsume it
Flagging so protocol versioning doesn't end up with one ad-hoc probe per fix.
#646 (for #642) needed to know whether a peer can echo a correlation id back through
DB_SCHEMA, and whether it will honor a setup budget. With no protocol version to consult — the gap this workstream owns — it added its own negotiation: a capabilities object appended as the fifth element of theNODE_NAMEframe,{ subscriptionSetupAck: SUBSCRIPTION_SETUP_ACK_CAPABILITY, subscriptionSetupBudgetMs: SEND_SUBSCRIPTION_SETUP_BUDGET_MS }resolved by
resolveSubscriptionSetupCapability(), which caps a peer-advertised budget so an old or misbehaving peer can't disable the local recovery net.The mechanism itself is sound and the mixed-version behavior is the right shape (absent field ⇒ fall back to the local timeout). The concern is structural: this is the second version-discrimination scheme in the protocol, alongside
COPY_ORDER_VERSION, and it establishes "append a bag toNODE_NAME, invent a feature key" as the pattern the next fix will copy. Three of those and the mixed-version matrix is no longer reviewable.Suggested W11 scope addition: when protocol version negotiation lands, absorb the
NODE_NAMEcapabilities bag as its transport (it is already the first frame both directions exchange, so it is the natural carrier) and re-expresssubscriptionSetupAckas a version predicate rather than a standalone key — keepingsubscriptionSetupBudgetMsas a genuine negotiated parameter, which is a different thing from a capability bit and should stay one.🤖 Claude (Opus 5) on behalf of Kris.
1 remaining item
- added a commit that references this issue
on Aug 12, 2026 - added 4 commits that reference this issue
on Aug 19, 2026 Protocol version negotiation — design (the W11 versioning deliverable)
Verified against
origin/main@e506fcb4. Three ad-hoc discrimination schemes exist today — the fixed subprotocol string'harperdb-replication-v1'(replicationConnection.ts:2440, never negotiated),COPY_ORDER_VERSION(one-way announce+echo, copy-resume only, :133-138/:5236/:6939), and the #646NODE_NAMEcapabilities bag ({subscriptionSetupAck, subscriptionSetupBudgetMs}, :7126-7137/:1821-1836). W9 Phase 1 (record locking, #438) needs a fourth gate within weeks. This design absorbs the bag as the transport before that happens, per the scope addition proposed above.Decisions
- Transport = the
NODE_NAMEcapabilities object (element [4]). Already the first frame both directions, already shipped, additive-safe: pre-fix(replication): recover ping-alive setup stalls before DB_SCHEMA (#642) #646 peers ignore element [4] entirely; fix(replication): recover ping-alive setup stalls before DB_SCHEMA (#642) #646 peers read only keys they know. No new frame, no extra round-trip. - Two kinds of discrimination, one placement rule.
- Capability keys (integer levels, absent ⇒ 0/unsupported) for additive, orthogonal features — the proven
subscriptionSetupAckmonotone-level pattern (:1826). Standing discipline: a new frame type or field must be capability-gated on the sender — a peer never receives what it didn't advertise. protocolVersion(monotone integer, one key inside the same bag) reserved for wire-shape changes that can't be additive. Absent ⇒ 1 (today's protocol). Effective version =min(local, peer). AMINIMUM_PROTOCOL_VERSIONfloor (currently 1) gives a future breaking change its lever; nothing refuses today.- Rule of thumb: if an old peer can safely ignore it → capability key; if an old peer would misparse it → version bump.
- Capability keys (integer levels, absent ⇒ 0/unsupported) for additive, orthogonal features — the proven
- A registry module (
replication/protocolCapabilities.ts): oneLOCAL_CAPABILITIESconst declaring every advertised key + level;resolvePeerCapabilities(bag)returning a frozen, normalized object with defaults for absent keys and per-key clamp rules (thesubscriptionSetupBudgetMsclamp at :1828-1834 moves here). Every consultation goes through the resolved object — the mixed-version matrix becomes one grep. - Storage: resolved capabilities live on the
replicateOverWSclosure (per-socket, re-learned on everyNODE_NAME— correct across peer upgrades; precedent is the setup-ack block at :3239-3253), mirrored read-only ontooptions.connection.peerCapabilitiesfor observability.cluster_statuscan surface the peer's capability set (W8 tie-in; today peer version is invisible everywhere —system_infoin hdb_nodes is declared but never written/read, knownNodes.ts:65). - Re-express, don't re-wire:
subscriptionSetupAck/subscriptionSetupBudgetMskeep their key names;resolveSubscriptionSetupCapabilitybecomes a view over the registry (its unit tests at subscriptionSetupWatchdog.test.mjs:81-106 stay green).COPY_ORDER_VERSIONstays where it is — it is durable copy-cursor state echoed throughcopyResume(:2027, :5254-5268), not connection state; moving it is churn without benefit. - Unknown-frame telemetry: the dispatch
switch(:3969-4610) gains a default case — throttled warn + counter for unknown command codes (128–255 space, first-byte >127 framing at :3966). With sender-side gating this should never fire; when it does it's the alarm that the discipline was skipped, replacing today's silence. - First consumers: W9 Phase 1 advertises
recordLocks: 1— a sender must not ship lock-metadata-bearing record layouts or LOCK/UNLOCK entries to a peer that doesn't advertise it.subscriptionSetupAckre-expressed. Every subsequent fix uses the registry instead of inventing a bag.
Mixed-version safety
- old→new: absent/partial bag → all defaults 0 → exactly current behavior (absence-detection formalized).
- new→old: extra msgpack map keys are ignored by old readers; element [4] ignored by pre-fix(replication): recover ping-alive setup stalls before DB_SCHEMA (#642) #646 peers.
- Effort M, risk low-medium — the only behavior-adjacent change is the setup-ack re-expression, pinned by existing tests.
Test matrix
Resolver unit tests (absent bag, legacy bag, future/unknown keys, clamp bounds); integration current↔bag-stripped peer via test hook (HARPER_TEST_* precedent); unknown-frame counter test.
🤖 Claude (Fable 5) on behalf of Kris.
- Transport = the
Metadata
Metadata
Assignees
Type
Fields
Priority
Workstream W11 of #430 · code organization, protocol versioning & decode-loop safety — enabler
Status update (2026-07-01)
replicationConnection.tsis now ~4,400 lines (was ~3,600) andreplicateOverWS~3,250 (was ~2,800), absorbing the entire watchdog/copy-fix wave. The trend is the argument: every reliability fix lands inside the closure because there's nowhere else for it to go.catchlogs "Error handling incoming replication message" and keeps the connection open). Still the top land-first item.decideEmptySubscriptionClose,shouldTerminateIdlePing, the watchdog/backoff predicates, blob-error classification). ADESIGN.mdnavigation guide also landed. That pattern — extract the decision, unit-test it, leave the I/O in place — is the default seam for this workstream; the module splits below come after.Store<V>for the remaining system-table stores + a lint against bare.get()in sync contexts, extending the pattern feat(types): type the __dbis__/seq store so sync get() misuse is a compile error (#484 follow-up) #485 established for the__dbis__store (which made syncget()misuse a compile error). This belongs here as type-safety enablement.Summary
replicationConnection.tsis ~4,400 lines;replicateOverWSis a single ~3,250-line closure with ~50 interdependent local variables and the 688-line send engine nested two levels deep. There is no protocol version negotiation, and the inbound decode loop swallows errors. This organization is why every reliability fix has been high-risk. W5 (adaptive routing) and W9 (locking) add new re-route/lock paths — doing that inside the current monolith is how the epic's Theme A/C bugs are born. Land the safety + versioning pieces early; decompose along the natural seams lowest-risk first. Not a big-bang rewrite — the value is in doing the early steps before the feature workstreams.Safety (land first, regardless of decomposition)
decodeBlobsWithWrites, but the batch resume cursor (maxBatchVersion) still advances past it — a permanent silent gap, and it's exactly the structure-fork decode-error class seen in the field (#1163/#1453 lineage). Decide deliberately: rethrow → close (reconnect re-sends schema, which may heal structure forks) vs hold the cursor at the failed record. Pairs with W2's gap detection (Replication W2: Cursor correctness & divergence detection #432). Unknown-tableIdframes ride the same inner catch (previously escaped it via an accidental secondary TypeError from the unguarded log line; now guarded).protocolVersionfield to theNODE_NAMEhandshake; each side declares its highest supported version, both agree onmin, and new command codes are gated on the negotiated version. Makes rolling upgrades across breaking protocol changes possible (today only additive commands are safe, by unenforced convention). Also carry capability flags (e.g.supports-per-origin-cursors, storage engine) so mixed-engine clusters during the LMDB→RocksDB migration negotiate down to the basic shared-cursor path gracefully (W4 / Replication W4: Per-origin transaction-log convergence (keystone) #434).checkDatabaseAccessauthorization TODO — per-database authorization is currently omitted (relies on connection-level auth only); a reconnected peer with a revoked cert could re-use an open session's database access."Can not re..."string match with a typed error/code from core.Store<V>for remaining system-table stores + lint against sync.get()(epic Theme H; extends feat(types): type the __dbis__/seq store so sync get() misuse is a compile error (#484 follow-up) #485's__dbis__typing so the MaybePromise class can't recur).Decomposition (lowest risk first)
FrameWriter— encapsulate the four encode-buffer variables +writeInt/writeBytes/writeFloat64/checkRoom/checkExcessMessageSize. Zero semantic change; the first safe wedge and a prerequisite for the rest.blobTransfer.ts—sendBlobs/receiveBlobs/createBlobReceiveStream+ the already-pure error-classification predicates (isPermanentSourceBlobErrorCode,markSourceBlobUnavailable, …).copyTransfer.ts— the COPY_START→COPY_COMPLETE path is a clean unit (coordinate with W13 Replication W13: Base-copy & catch-up path (consolidation & correctness) #510's direct-binary-relay item).sendAuditRecord/skipAuditRecord/sendQueuedData+ main send loop) out of theSUBSCRIPTION_REQUESThandler to a top-level function.connectionLifecycle.ts—NodeReplicationConnection+ watchdog + ping.DESIGN.mdcurrent as modules split out.Dependencies
None upstream. The safety + versioning items should land before W5/W9; FrameWriter should land before the deeper extractions and before the send-engine refactor that W5 touches.
Effort / risk
M–L / low per step. Sequenced, not big-bang.
Acceptance criteria
FrameWriter,blobTransfer, andcopyTransferare extracted with unit tests, with no behavior change.🤖 Filed by Claude on behalf of Kris.