Skip to content

fix(replication): reconcile connected bit UP from shared-memory truth; log corrections (#431)#523

Open
kriszyp wants to merge 3 commits into
mainfrom
kris/w1-truth-shadow
Open

fix(replication): reconcile connected bit UP from shared-memory truth; log corrections (#431)#523
kriszyp wants to merge 3 commits into
mainfrom
kris/w1-truth-shadow

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member

What

Completes the W1 (#431) truth-reconcile consumer that #445 started. The 5s reconcileWorkers pass corrected the edge-triggered connected bit only downward (shared-memory truth says down → flip to false, feeding the wedge re-drive). This PR adds:

  1. Up-direction correction — a connected:false entry whose connect edge was lost or never processed (Replication: ~9/11 outgoing peers show connected:false in connectionReplicationMap after restart despite being reachable #289, the founding W1 bug) previously stayed false until findWedgedNodeUrls force-reconnected a perfectly healthy link 30s later. Now truth CONNECTED + fresh liveness corrects the bit in place — no disruptive reconnect. Never-connected (undefined) entries are deliberately not up-corrected: they're mid-connect and owned by the connect/retry path.
  2. Correction telemetry — both directions now log the correction (previously the down-correction was silent). Edge-bit-vs-truth disagreement is exactly the desync class W1 exists to retire, so its frequency in production should be observable before the deeper watchdog demotion leans on the truth.

How

The correction rules move into an exported pure helper, reconcileEntryWithTruth(entry, truth, now), following the file's established pure-decision-helper pattern (isReceiveStalled, findWedgedNodeUrls, …). The reconcile loop calls it per (db, peer) entry and owns the logging. Down-correction semantics are unchanged: liveness-reported-at-least-once gate, disconnectedAt stamped only if absent (doesn't restart the wedge clock).

Safety notes

  • Up-correction requires state === CONNECTED and fresh liveness (LIVENESS_STALE_MS), so a worker that died without writing DOWN can't cause a false up-correction beyond the staleness window — and the findStaleNodeUrls exited-worker net operates independently of the connected bit.
  • A racing up-correction just before the real connect edge arrives is idempotent (the edge sets the same state).

Tests

unitTests/replication/reconcileEntryWithTruth.test.mjs — 8 cases: both corrections, the lastLiveness > 0 gate, wedge-clock preservation, the undefined-entry up-exclusion, and agreement no-ops. Full unitTests/replication/ dir: 266 passing.

Part of #430 (epic) · W1 #431 · follows #445.

🤖 Generated with Claude Code

…; log corrections (#431)

Completes the truth-reconcile consumer #445 started. The 5s reconcile pass
corrected the edge-triggered `connected` bit only downward (truth says down ->
flip to false, feeding the wedge re-drive); a connected:false entry whose
connect edge was lost or never processed (#289, the founding W1 bug) stayed
false until findWedgedNodeUrls force-reconnected a perfectly healthy link 30s
later. Now truth CONNECTED + fresh liveness corrects the bit up in place, no
disruptive reconnect. Never-connected (undefined) entries are deliberately not
up-corrected - they are mid-connect and owned by the connect/retry path.

Both directions now log the correction (they are transitions, self-limiting):
edge-bit-vs-truth disagreement is exactly the desync class W1 exists to retire,
so its production frequency should be observable before deeper watchdog
demotion leans on the truth.

The rules live in an exported pure helper (reconcileEntryWithTruth), following
the file's pure-decision-helper pattern, with unit coverage for both
corrections, the liveness>0 gate, wedge-clock preservation, and the
undefined-entry exclusions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@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 reconcileEntryWithTruth function in subscriptionManager.ts to reconcile the main thread's connection state with the authoritative shared-memory truth in both directions, along with comprehensive unit tests. Feedback suggests adding an explicit return undefined; at the end of the new function to prevent potential compilation errors under strict TypeScript configurations.

Comment on lines +268 to +273
if (entry.connected === false && truth.connected) {
entry.connected = true;
entry.disconnectedAt = undefined;
return 'up';
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent potential compilation errors under strict TypeScript configurations (such as noImplicitReturns: true), it is recommended to add an explicit return undefined; at the end of the function.

Suggested change
if (entry.connected === false && truth.connected) {
entry.connected = true;
entry.disconnectedAt = undefined;
return 'up';
}
}
if (entry.connected === false && truth.connected) {
entry.connected = true;
entry.disconnectedAt = undefined;
return 'up';
}
return undefined;
}

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

…ion (#431)

Cross-model review caught a blocker in the up-correction this PR adds: the
per-(db, peer) shared status buffer is keyed only by (db, peer), but its
connection-truth slots (CONNECTION_STATE / LAST_LIVENESS / LAST_ERROR, added by
#445) were written by ANY connection resolving that buffer — including a
cache-miss retrieval connection (getRetrievalConnectionByName, which connect()s
with connection:this) and, via the ungated handshake/receive writes, an inbound
server connection from the same peer. In a full mesh, an active inbound B->A
stream (or a reused retrieval socket) kept (db,B) stamped CONNECTED with fresh
liveness even after the outbound A->B subscription genuinely died. The new
up-correction then resurrected the dead entry to connected:true, so
findWedgedNodeUrls skipped it -> silent one-directional replication halt /
divergence. The inverse (a retrieval connection's close stamping DOWN over a
healthy subscription) caused spurious wedge-recovery reconnects.

#445's down-only correction failed safe (a stray DOWN is at worst a re-stream);
the up-correction is what makes the contamination consequential, so the fix
lands with it.

Enforce the ownership invariant at every truth-slot write (close, forceReconnect
teardown, handshake, receive, pong): only the connection that OWNS the (db,peer)
subscription may write slots 9-12, via connectionOwnsTruth() =
`connection?.nodeSubscriptions !== undefined`. nodeSubscriptions is set by
subscribe() and never cleared, and is never set on a retrieval connection
(connect() only) or an inbound connection (no options.connection) — so it is a
reliable, write-time ownership marker, checked at write time rather than via the
frozen open-time isSubscriptionConnection snapshot. Receive-progress slots
(RECEIVED_TIME/VERSION, RECEIVING_STATUS) and latency are left as-is; only the
authoritative connection-state slots are owner-gated. This also cleans up the
cluster_status view, which reads the same buffer.

connectionOwnsTruth is exported with unit coverage pinning the subscription vs
retrieval vs inbound cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

Cross-model review (thorough) found and fixed a blocker — pushed

The Gemini + Codex + Harper-domain adjudication surfaced a real correctness blocker that this PR's up-correction would have introduced, now fixed in 636d825.

The hazard: the per-(db, peer) shared status buffer is keyed only by (db, peer), but its connection-truth slots (CONNECTION_STATE / LAST_LIVENESS / LAST_ERROR, from #445) were written by any connection resolving that buffer — a cache-miss retrieval connection (getRetrievalConnectionByName, which connect()s with connection: this → fires the pong write) and, via the ungated handshake/receive writes, an inbound server connection from the same peer. In the common full-mesh case, an active inbound B→A stream kept (db,B) stamped CONNECTED+fresh-liveness even after the outbound A→B subscription genuinely died — and the new up-correction would resurrect that dead entry to connected:true, so findWedgedNodeUrls skips it → silent one-directional replication halt / divergence. (#445's down-only correction failed safe; the up-correction is what made the contamination consequential, so the fix lands here.)

The fix (root-cause, not defensive): enforce the ownership invariant — only the connection that owns the (db,peer) subscription may write slots 9–12 — at every truth-slot write (close, forceReconnect teardown, handshake, receive, pong), via connectionOwnsTruth(c) = c?.nodeSubscriptions !== undefined. nodeSubscriptions is set by subscribe() and never cleared, and is never set on a retrieval (connect() only) or inbound (no options.connection) connection — a reliable ownership marker checked at write time, not the frozen open-time isSubscriptionConnection snapshot (which the review flagged as incidental ordering). Receive-progress and latency slots are untouched; only the authoritative connection-state slots are owner-gated. This also de-contaminates the cluster_status view (same buffer).

Adjudicated non-issues: Gemini's "up-correction flaps against a fresh disconnect edge" is bounded by the 120s staleness net and subsumed by the ownership fix; the undefined-entry down-correction and the correction && truth narrowing guard are both by-design/load-bearing (the guard is needed for TS narrowing of truth). No changes there.

Full unitTests/replication/ suite green (270 passing), including new connectionOwnsTruth coverage. Keeping this draft until the fix gets a re-review pass over the newly-touched write sites.

)

Completeness follow-up to the ownership gate: the pause path (sendPing, while
pauseReasons > 0) refreshes LAST_LIVENESS_TIME on the shared (db, peer) buffer so
a healthy-but-paused link isn't timed out, but it was not owner-gated. A paused
inbound or cache-miss retrieval connection sharing the buffer key would keep a
dead subscription's liveness fresh — the same masking the ownership gate closes
elsewhere. Gate it on connectionOwnsTruth(options.connection) too. This is now
the last of the six truth-slot write sites; grep-verified all writes to slots
9-12 are owner-gated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp kriszyp marked this pull request as ready for review July 6, 2026 04:12
@kriszyp kriszyp requested a review from a team as a code owner July 6, 2026 04:12
@kriszyp kriszyp requested review from kylebernhardy and ldt1996 and removed request for a team July 6, 2026 04:12
@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

Focused re-review of the ownership-gate fix — complete & correct

Ran a scoped re-review (Codex + Harper-domain adjudication) over the newly-gated write sites.

Independent completeness check caught one more site the first Codex pass missed and reported clean: the back-pressure-pause liveness write (replicationConnection.ts ~1592, pausedStatus[LAST_LIVENESS_TIME] = Date.now()) was still ungated — a paused inbound/retrieval connection would refresh a dead subscription's liveness, the same masking. Fixed in ac5f177. Grep-verified all six truth-slot (9–12) write sites are now owner-gated: close, forceReconnect teardown, pause, handshake, receive, pong.

The domain pass independently re-derived completeness (not trusting the Codex verdict, which had confabulated) and confirmed:

  • No false-negative on the ownerthis.nodeSubscriptions is assigned once (subscribe()) and never cleared; options.connection === this; subscribe() runs synchronously before any network handler fires, so a real subscription's DOWN (close/forceReconnect) and CONNECTED (handshake/receive/pong/pause) writes are never suppressed. The subscribe([], false) DB-removal path keeps nodeSubscriptions = [] (!== undefined), so its teardown DOWN still writes.
  • Correct exclusions — retrieval (connect() only) and inbound (no options.connection) both fail the gate.
  • Ungated slots are correct by designderiveConnectionTruth reads only slots 9/10; the ungated receive-progress/latency slots feed stall-detection & cluster_status, not the up-correction.
  • Up-correction consumer still sound — a non-owner can no longer stamp CONNECTED+fresh-liveness, so it can't drive the resurrection; down-correction only becomes more available.

Verdict: no blockers, no significant concerns. Full unitTests/replication/ suite green (270 passing), incl. new connectionOwnsTruth + reconcileEntryWithTruth coverage.

One noted follow-up (pre-existing, out of scope, NOT this PR): a retrieval connection actively receiving sets RECEIVING_STATUS+RECEIVED_TIME on the shared buffer, which can mask a subscription receive-stall in the looser isReceiveStalled net (#453/#463) — same ungated-slots property, different consumer. Worth folding into the watchdog-consolidation step later.

Marking Ready.

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.

1 participant