HarperFast / HarperFast/harper-pro
Replication W11: Code organization, protocol versioning & decode-loop safety
- Dominant language
- JavaScript
- Stars
- 3
- Forks
- 0
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 80
Description
**Workstream W11 of #430 · code organization, protocol versioning & decode-loop safety — _enabler_**
## Status update (2026-07-01)
- **The monolith grew ~20% since filing** — `replicationConnection.ts` is now ~4,400 lines (was ~3,600) and `replicateOverWS` ~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.
- **The decode-loop error swallow is still there verbatim** (the message-handler `catch` logs "Error handling incoming replication message" and keeps the connection open). Still the top land-first item.
- **Still no protocol version negotiation.**
- **A decomposition method has been proven in the meantime**: the recent fixes extracted pure decision helpers to the top of the file with focused unit tests (`decideEmptySubscriptionClose`, `shouldTerminateIdlePing`, the watchdog/backoff predicates, blob-error classification). A `DESIGN.md` navigation 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.
- **New item (epic Theme H residual):** typed `Store` for the remaining system-table stores + a lint against bare `.get()` in sync contexts, extending the pattern #485 established for the `__dbis__` store (which made sync `get()` misuse a compile error). This belongs here as type-safety enablement.
## Summary
`replicationConnection.ts` is ~4,400 lines; `replicateOverWS` is 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)
- [ ] **Stop the inbound decode-loop error swallow** — a decode error on record N currently silently drops N+1…end-of-message and keeps the connection open (silent partial-message loss). Close + reconnect instead (the established recovery path). *(In review: harper-pro#511 — covers the outer frame-level catch.)*
- [ ] **Per-record value-decode residual (cross-model review finding, 2026-07-01):** a record whose *value* fails to decode is skip-and-logged by the inner catch around `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 (#432). Unknown-`tableId` frames ride the same inner catch (previously escaped it via an accidental secondary TypeError from the unguarded log line; now guarded).
- [ ] **Protocol version + capability negotiation** — add a `protocolVersion` field to the `NODE_NAME` handshake; each side declares its highest supported version, both agree on `min`, 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 / #434).
- [ ] **Resolve the `checkDatabaseAccess` authorization 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.
- [ ] **Replace the fragile `"Can not re..."` string match** with a typed error/code from core.
- [ ] **Typed `Store` for remaining system-table stores + lint against sync `.get()`** (epic Theme H; extends #485's `__dbis__` typing so the MaybePromise class can't recur).
## Decomposition (lowest risk first)
- [ ] **Continue the proven pure-decision-helper extraction** for any logic touched by other workstreams (extract the decision, unit-test it, leave the I/O in place) — the default seam, validated by the 2026-06 fix wave.
- [ ] **Extract `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 #510's direct-binary-relay item)*.
- [ ] **Extract the ~688-line send engine** (`sendAuditRecord`/`skipAuditRecord`/`sendQueuedData` + main send loop) out of the `SUBSCRIPTION_REQUEST` handler to a top-level function.
- [ ] **`connectionLifecycle.ts`** — `NodeReplicationConnection` + watchdog + ping.
- [ ] Keep `DESIGN.md` current 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
- Decode errors no longer silently drop records (connection recovers instead).
- A mixed-version cluster negotiates a protocol version; new commands are version-gated.
- `FrameWriter`, `blobTransfer`, and `copyTransfer` are extracted with unit tests, with no behavior change.
- Sync misuse of a MaybePromise system-table store is a compile-time error.
---
🤖 Filed by Claude on behalf of Kris.
Contributor guide
Assessment
This issue has not been assessed yet.