Skip to content

fix(ws-server): don't apply a gapped changeset after rejecting it#3

Open
silviot wants to merge 1 commit into
librocco/mainfrom
fix/inbound-stream-gap-apply
Open

fix(ws-server): don't apply a gapped changeset after rejecting it#3
silviot wants to merge 1 commit into
librocco/mainfrom
fix/inbound-stream-gap-apply

Conversation

@silviot

@silviot silviot commented Jun 7, 2026

Copy link
Copy Markdown

Bug

InboundStream.receiveChanges (server) detects a gap — the incoming changeset's since is ahead of
the version we have actually applied (#lastSeen) — and sends RejectChanges, but there is no
return
. It falls through and applies the non-contiguous batch anyway, then advances #lastSeen
and the durable crsql_tracked_peers cursor past the gap.

The server then believes it has received everything from that peer and never re-requests the
missing range
, so the peer's committed changes are silently and permanently lost on the server and
on every other peer — "converged but not actually synced" divergence. This is the inbound/apply twin
of the OutboundStream.reset() stub that was fixed in the outbound direction.

The client's InboundStream already does the right thing: it returns immediately after rejecting
(and reverts #lastSeens on apply failure).

This was reproduced against the shipped @vlcn.io/ws-server artefact (vlcn.io-ws-server-0.2.2):
a gapped changeset is applied (applied.length === 1) while a reject is also sent.

Fix

return; after rejectChanges. The server keeps #lastSeen at the pre-gap value, so the missing
range is recovered when the sender re-streams from the server's last-seen (e.g. on the next
StartStreaming / reconnect).

Adds InboundStream.test.ts (mirroring OutboundStream.test.ts) covering the gapped path (rejected,
not applied) and the contiguous path (applied).

Notes / follow-ups

  • This is the highest-impact half. Mid-connection recovery without a reconnect additionally requires
    the client to honour an inbound RejectChanges — today ws-client's WebSocketTransport
    throws on tags.RejectChanges, so server→client gap-recovery for the upload direction is dead.
    Tracked separately.
  • Targeted at librocco/main (the branch the librocco 0.2.2 tarball is built from). main has the
    same missing return and should get the same fix.
  • The librocco-side regression test against the rebuilt tarball should mirror
    apps/sync-server/src/ws-server-reject-recovery.test.ts.

Co-authored-by: Claude Opus 4.8 (1M context) noreply@anthropic.com

InboundStream.receiveChanges detected a gap (the incoming changeset's `since` is
ahead of our applied `#lastSeen`) and sent RejectChanges — but had no `return`,
so it fell through and applied the non-contiguous batch anyway, advancing
`#lastSeen` and the durable crsql_tracked_peers cursor past the gap. The server
then believed it had everything from that peer and never re-requested the missing
range, so the peer's committed changes were silently and permanently lost on the
server and on every other peer. This is the inbound/apply twin of the
OutboundStream.reset() stub bug.

Add `return;` after rejectChanges, mirroring the client InboundStream which
returns immediately after rejecting. The missing range is then recovered when the
sender re-streams from the server's (unadvanced) last-seen.

Adds InboundStream.test.ts covering the gapped (rejected, not applied) and
contiguous (applied) paths.

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.

1 participant