Skip to content

fix(replication): emit reload markers for user-DB tables so live subscribers recover copy-applied rows#507

Open
kriszyp wants to merge 2 commits into
mainfrom
kris/495-mqtt-copy-reload
Open

fix(replication): emit reload markers for user-DB tables so live subscribers recover copy-applied rows#507
kriszyp wants to merge 2 commits into
mainfrom
kris/495-mqtt-copy-reload

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

copyApply covers all RocksDB copies, but its base-copy rows carry no per-row audit entries, so a live MQTT/SSE/WS subscriber on a freshly-copied node never sees them — they are durably stored but never delivered to an already-connected subscriber.

This is the harper-pro half of #495. Generalize emitSystemReloadMarkersemitCopyReloadMarkers: once a copy is durable, emit a whole-table reload marker per table for any copied RocksDB database — the system DB keeps the fixed cluster-machinery list (SYSTEM_RELOAD_TABLES), a user DB emits one per table. The paired core change re-delivers each table's current scope to its live subscribers on that marker.

Stacked on #491 (base branch kris/copyapply-reload-marker) — it depends on emitSystemReloadMarkers/SYSTEM_RELOAD_TABLES introduced there. The core companion (harper#1491) is already merged to main, so the core fix targets main directly: HarperFast/harper#1530. This branch bumps the core submodule to that fix. Will rebase onto main once #491 lands.

Where to look

  • replication/replicationConnection.tsemitCopyReloadMarkers (was emitSystemReloadMarkers): for a user DB it iterates Object.keys(tables); the marker is LOCAL_ONLY, so the replication send path skips it (extendedType & LOCAL_ONLY → skipAuditRecord) and it is never forwarded to peers. Each node re-snapshots its own subscribers off its own copy-complete.
  • integrationTests/cluster/aclConnectCopyReload.test.mjs — deterministic repro: add_node isLeader:true forces an audit-less base copy of pre-existing rows; a subscriber that is live before the copy must receive them. Verified as a true negative control (subscriber sees [] with the re-snapshot disabled).

Companion PR

Verification

  • New aclConnectCopyReload test passes; fails with the core re-snapshot disabled.
  • Existing aclConnectCrossNode (3 tests) still green.

Cross-model review

Thorough (Codex + Gemini + Harper-domain adjudication): no blockers; all outside-model blockers were false positives on source trace. Domain pass confirmed LOCAL_ONLY non-replication and the system-DB gate preserving knownNodes.

Generated by an LLM (Claude Opus 4.8).

🤖 Generated with Claude Code

@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 resolves an issue where live MQTT, SSE, or WS subscribers fail to receive records that arrive on a node via a copyApply base copy. Because these base-copy rows are written as snapshots without per-row audit entries, the live subscription notify path is bypassed. The PR addresses this by renaming emitSystemReloadMarkers to emitCopyReloadMarkers and expanding its scope to emit reload markers for all tables in user databases, prompting subscribers to re-read and recover the snapshotted rows. A regression test has also been added to verify this behavior. The reviewer's feedback suggests a valuable optimization to skip emitting reload markers for non-replicated tables, which avoids redundant write transactions and unnecessary re-reads.

Comment thread replication/replicationConnection.ts Outdated
@kriszyp kriszyp force-pushed the kris/495-mqtt-copy-reload branch 2 times, most recently from b8184b7 to 8f87d91 Compare June 30, 2026 11:20
@kriszyp kriszyp force-pushed the kris/copyapply-reload-marker branch from cab899e to bc66efb Compare July 2, 2026 15:40
Base automatically changed from kris/copyapply-reload-marker to main July 2, 2026 15:52
…cribers recover copy-applied rows

copyApply now covers all RocksDB copies, but its base-copy rows carry no per-row
audit entries, so a live MQTT/SSE/WS subscriber on a freshly-copied node never
sees them. Generalize `emitSystemReloadMarkers` -> `emitCopyReloadMarkers`: once a
copy is durable, emit a whole-table `reload` marker per table for any copied
RocksDB database (the system DB keeps the fixed cluster-machinery list; a user DB
emits one per table). The core change (harper PR) re-delivers the current scope to
each table's live subscribers on that marker. The marker is LOCAL_ONLY, so the
replication send path skips it and it is never forwarded to peers.

Bumps the core submodule to the matching core fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriszyp kriszyp force-pushed the kris/495-mqtt-copy-reload branch from 8f87d91 to 0b817f2 Compare July 4, 2026 04:29
@claude

claude Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp kriszyp marked this pull request as ready for review July 4, 2026 04:38
@kriszyp kriszyp requested a review from a team as a code owner July 4, 2026 04:38
@kriszyp kriszyp requested a review from cb1kenobi July 5, 2026 23:58
Comment thread replication/replicationConnection.ts Outdated
…ved copied rows (#507)

Track per-table which user-DB tables received an audit-less copy-apply
snapshot row in a copy pass (copiedTablesThisPass, keyed off event.isCopyApply)
and gate reload-marker emission on that set, so an empty / non-replicated /
fully-audited user table no longer gets a spurious marker. System-DB reload
tables keep their always-emit behavior. Addresses cb1kenobi review on PR #507.

Co-Authored-By: Claude Opus 4.8 (1M context) <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