Skip to content

Commit 35d1ca7

Browse files
committed
fix(sync): gate peer 'started' status on the peer's own Synchronize, not core.update()
Flip status to 'started' when the peer's own first Synchronize wire message is processed - hypercore records exactly this per-peer fact as peer.remoteSynced, set synchronously at the top of peer.onsync(). We intercept by shadowing onsync on the peer instance, the same interception already used for onbitfield/onrange (hypercore dispatches wire messages by property lookup on the channel's userData). This fixes all three failure modes of the update()-based signal covered by the previous commit's tests. It also removes an unhandled-rejection hazard: the old .then() had no rejection handler, and hypercore rejects pending update() calls when a core closes. One real case exists where the first Synchronize never arrives on a live channel: a close/open crossing on the same protomux - both sides disable and re-enable a namespace around the same time, which routinely happens during the invite flow's capability churn. One side ends up with a fresh peer holding no baseline while the remote believes its old session is still live, so it never re-sends its state, and hypercore does not recover on its own. There is no reliable in-band repair: no message we can send guarantees a Synchronize in reply, the duplicate channel open is dropped by protomux before hypercore sees it, and forcing a channel renegotiation from this observation layer destabilizes the layers that own the channel lifecycle (tried; it could strand a peer with no channel at all). So after 5s the peer is treated as started with its channel-level state unknown. This does not fabricate sync state: a peer's blocks are counted from its pre-have bitfields (broadcast over the project creator core at connect and on append) merged with the channel-level bitfield, so a peer that has data is still counted correctly - the fallback stops gating counts that are already correct, and a Synchronize arriving later still updates state. The root fix is upstream: protomux channel renegotiation needs a close reason / channel generation (hypercore's own Peer#onclose has a TODO to this effect). Peers whose Synchronize was processed before the core was attached are detected via remoteSynced and marked started immediately - defensive only: production wiring attaches cores synchronously on 'add-core', before a channel handshake can complete, but nothing in this class enforces that. Adds tests for the fallback (a silent session must not block sync state forever, asserted to have resolved without a Synchronize) and for the attach-late invariant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzqT9t19AV5voYFM2vYV8h
1 parent a45e48c commit 35d1ca7

2 files changed

Lines changed: 160 additions & 7 deletions

File tree

src/sync/core-sync-state.js

Lines changed: 85 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,15 @@ import { InvalidBitfieldIndexError } from '../errors.js'
4343
// This is a 32-bit number with all bits set
4444
const WANT_FULL = 2 ** 32 - 1
4545

46+
/**
47+
* How long after `peer-add` to wait for the peer's first Synchronize message
48+
* before treating the peer as started with its channel-level state unknown
49+
* (see `#onPeerAdd`). Normally the Synchronize arrives in the same batch as
50+
* the channel open, so this is only hit when a channel close/open crossing
51+
* swallowed the handshake.
52+
*/
53+
const FIRST_SYNC_FALLBACK_MS = 5_000
54+
4655
/**
4756
* Track sync state for a core identified by `discoveryId`. Can start tracking
4857
* state before the core instance exists locally, via the "preHave" messages
@@ -279,12 +288,6 @@ export class CoreSyncState {
279288
const peerState = this.#getOrCreatePeerState(peerId)
280289
peerState.status = 'starting'
281290

282-
this.#core?.update({ wait: true }).then(() => {
283-
peerState.status = 'started'
284-
peerState.contiguousLength = peer.remoteContiguousLength
285-
this.#update()
286-
})
287-
288291
// A peer can have a pre-emptive "have" bitfield received via an extension
289292
// message, but when the peer actually connects then we switch to the actual
290293
// bitfield from the peer object
@@ -294,7 +297,9 @@ export class CoreSyncState {
294297
this.#update()
295298

296299
// We want to emit state when a peer's bitfield changes, which can happen as
297-
// a result of these two internal calls.
300+
// a result of these two internal calls. Hypercore dispatches wire messages
301+
// by property lookup on this peer object (its channel's userData), so
302+
// shadowing the methods on the instance is the interception point.
298303
const originalOnBitfield = peer.onbitfield
299304
const originalOnRange = peer.onrange
300305
peer.onbitfield = (...args) => {
@@ -307,6 +312,79 @@ export class CoreSyncState {
307312
peerState.contiguousLength = peer.remoteContiguousLength
308313
this.#update()
309314
}
315+
316+
// The peer is "started" once its first Synchronize message has been
317+
// processed (hypercore sets `peer.remoteSynced`): only then do we know
318+
// the peer's length and fork, so only then can counts about this peer be
319+
// trusted. Every channel sends a Synchronize immediately on open, so this
320+
// always arrives. Do NOT use `core.update({ wait: true })` as a proxy for
321+
// this: it answers a core-global question ("am I up to date with the
322+
// swarm?") which (a) resolves immediately for writable cores, (b)
323+
// resolves for all waiters when *any* peer supplies an upgrade, and (c)
324+
// samples at most 3 peers (hypercore's MAX_PEERS_UPGRADE) — so it can
325+
// report a peer as started before we have heard anything from it.
326+
const markStarted = () => {
327+
peerState.status = 'started'
328+
peerState.contiguousLength = peer.remoteContiguousLength
329+
this.#update()
330+
}
331+
if (peer.remoteSynced) {
332+
// Defensive only — not reachable in production wiring: cores are
333+
// attached synchronously on the core manager's 'add-core' event (or at
334+
// construction), before a channel handshake could possibly complete,
335+
// so peers replayed by `attachCore` have never synced yet. But nothing
336+
// in this class enforces that callers attach before handshakes
337+
// complete, so handle an already-synced peer correctly rather than
338+
// waiting for a Synchronize that already happened.
339+
markStarted()
340+
} else {
341+
const originalOnSync = peer.onsync
342+
peer.onsync = (...args) => {
343+
const result = originalOnSync.apply(peer, args)
344+
// `remoteSynced` is set synchronously at the top of `onsync`
345+
if (peer.remoteSynced) {
346+
if (peerState.status === 'starting') {
347+
clearTimeout(firstSyncFallback)
348+
markStarted()
349+
} else {
350+
// A Synchronize arriving after the fallback below fired still
351+
// carries state (the peer's length and fork), so re-derive
352+
peerState.contiguousLength = peer.remoteContiguousLength
353+
this.#update()
354+
}
355+
}
356+
return result
357+
}
358+
// The remote sends its first Synchronize as soon as the channel
359+
// opens, so normally this timer never fires. The exception is a
360+
// renegotiation bug in hypercore/protomux (their `Peer#onclose` has a
361+
// TODO about it): when both sides close and re-open a channel at the
362+
// same time — e.g. a namespace toggling off and on around a role
363+
// change — the messages cross and the remote keeps its old session,
364+
// never re-sending its state, so we would wait forever. This is not
365+
// repairable from here (no message forces a re-send, and
366+
// closing/re-opening the channel from this layer destabilizes the
367+
// replication layers that own it), only upstream. So stop waiting and
368+
// mark the peer started. The cost is the same as any reconnect, and
369+
// the same as the previous `core.update()`-based code: until this
370+
// peer reconnects we only know about this core what its pre-haves
371+
// told us (what it had at connect, plus its own writer-core appends),
372+
// not blocks it downloaded from third devices mid-session, so its
373+
// `have`/`wanted` counts can run low.
374+
const firstSyncFallback = setTimeout(() => {
375+
if (peer.remoteSynced || peer.removed) return
376+
if (peerState.status !== 'starting') return
377+
this.#l.log(
378+
'Peer %S sent no Synchronize %dms after channel open, marking started',
379+
peerId,
380+
FIRST_SYNC_FALLBACK_MS
381+
)
382+
markStarted()
383+
}, FIRST_SYNC_FALLBACK_MS)
384+
// Don't keep the process alive for this; it matters only while the
385+
// connection (which itself holds the process open) is live
386+
firstSyncFallback.unref?.()
387+
}
310388
}
311389

312390
/**

test/sync/core-sync-state.js

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,81 @@ test('peer status stays "starting" for a 4th concurrent peer (hypercore upgrade
640640
await waitForStatus(crs, emitter, fourthPeerId, 'started')
641641
})
642642

643+
test('peer status falls back to "started" if the first Synchronize never arrives', async (t) => {
644+
// A channel close/open crossing can leave the remote never re-sending its
645+
// Synchronize (it believes its old session is still live) — see the
646+
// comment in CoreSyncState's #onPeerAdd. A live channel in that state
647+
// must not block sync state forever: after a fallback timeout the peer is
648+
// treated as started, with its blocks still counted via pre-haves.
649+
const localCore = await createCore(t)
650+
await localCore.append(['a', 'b', 'c'])
651+
652+
const emitter = new EventEmitter()
653+
const crs = new CoreSyncState({
654+
onUpdate: () => emitter.emit('update'),
655+
peerSyncControllers: new Map(),
656+
namespace: 'auth',
657+
deviceId: '',
658+
hasDownloadFilter: () => false,
659+
})
660+
crs.attachCore(localCore)
661+
662+
// Never released: the Synchronize never arrives
663+
holdSynchronize(localCore)
664+
const remoteCore = await createCore(t, localCore.key)
665+
const kp2 = NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(1))
666+
const peerId = kp2.publicKey.toString('hex')
667+
const destroy = replicate(localCore, remoteCore, { kp2 })
668+
t.after(destroy)
669+
670+
await once(localCore, 'peer-add')
671+
assert.equal(
672+
crs.getState().remoteStates[peerId].status,
673+
'starting',
674+
'peer is "starting" while the Synchronize is missing'
675+
)
676+
677+
// 5000ms fallback (FIRST_SYNC_FALLBACK_MS) plus margin
678+
await waitForStatus(crs, emitter, peerId, 'started', 8_000)
679+
const livePeer = localCore.peers.find((p) =>
680+
p.remotePublicKey.equals(kp2.publicKey)
681+
)
682+
assert.equal(
683+
livePeer?.remoteSynced,
684+
false,
685+
'sanity: "started" came from the fallback, not a Synchronize'
686+
)
687+
})
688+
689+
test('peer status becomes "started" for a peer that synced before the core was attached', async (t) => {
690+
// Defensive invariant: production wiring attaches cores before their
691+
// channel handshakes can complete, but nothing in CoreSyncState enforces
692+
// that. A peer whose Synchronize was already processed when the core is
693+
// attached must become "started" without waiting for another message.
694+
const localCore = await createCore(t)
695+
await localCore.append(['a', 'b', 'c'])
696+
const remoteCore = await createCore(t, localCore.key)
697+
const kp2 = NoiseSecretStream.keyPair(Buffer.allocUnsafe(32).fill(1))
698+
const peerId = kp2.publicKey.toString('hex')
699+
const destroy = replicate(localCore, remoteCore, { kp2 })
700+
t.after(destroy)
701+
702+
await once(localCore, 'peer-add')
703+
await waitFor(() => localCore.peers[0].remoteSynced === true)
704+
705+
const emitter = new EventEmitter()
706+
const crs = new CoreSyncState({
707+
onUpdate: () => emitter.emit('update'),
708+
peerSyncControllers: new Map(),
709+
namespace: 'auth',
710+
deviceId: '',
711+
hasDownloadFilter: () => false,
712+
})
713+
crs.attachCore(localCore)
714+
715+
await waitForStatus(crs, emitter, peerId, 'started')
716+
})
717+
643718
/**
644719
* Intercept and queue the first Synchronize message(s) from peers that are
645720
* added to `core` after this is called, so tests can hold a peer in the

0 commit comments

Comments
 (0)