Skip to content

fix(sync): reject waitForSync when all peers disconnect before sync completes - #1352

Open
gmaclennan wants to merge 2 commits into
mainfrom
fix/sync-not-synced-without-peers
Open

gmaclennan wants to merge 2 commits into
mainfrom
fix/sync-not-synced-without-peers

Conversation

@gmaclennan

@gmaclennan gmaclennan commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

When a caller awaits $sync.waitForSync('full') and the only connected peer drops part way through, the promise resolves and the caller believes sync completed. The mobile Exchange screen derives its stage from the peer count so it does not show this, but anything that calls waitForSync directly (cloud tests, scripts, server-side code) is misled.

The cause is in src/sync/sync-api.js: isSynced() checks dataToSync and then each connected peer sync controller, but want/wanted are only counted against connected peers' bitfields and disconnectPeer removes the peer's remote state, so once the last peer is gone there is nothing left to check and the predicate is vacuously true.

waitForSync now remembers whether it has seen a connected peer and, if the count later drops to zero, rejects with the existing PeerDisconnectedError (the same error the invitor-side kWaitForInitialSyncWithPeer wait raises when its peer goes away). isSynced() and #updateState are untouched, so the full-stop and autostop table keeps its behaviour. Calling with no peers at all still resolves at once, as before, with errorOnNoPeers as the existing opt-in for that case. The test keeps data sync off on one side so the wanted block can never arrive, which is deterministic where racing a blob transfer would not be.

A few design questions for reviewers:

  1. Rejecting rather than waiting: waiting until timeoutMs would hang every caller that passes no timeout (mobile tests, cloud tests, the repro scripts) and would make offline leaveProject wait 45 s, so rejecting seemed the right default. The line is drawn at zero peers rather than any peer leaving because the existing test "Sync state with disconnected peer (want data from peer)" expects full sync to complete with the remaining peer.

  2. The 'initial' type is covered too, so addProject now fails if the invitor disconnects mid-initial-sync (and with leaveOnFail, used for invite links, clears the project and throws InitialSyncFailedError) instead of silently keeping a half-synced project. The invitor side already treats that disconnect as a failure, so this looks like the intended behaviour, but it is a change.

  3. The check runs on the throttled state snapshot, so a peer that disconnects within about 200 ms of sync genuinely completing is reported as a disconnect. kWaitForInitialSyncWithPeer avoids that window with the unthrottled update event; I left that out here to keep the diff small, and the reviewer could not trigger the false negative in 14 attempts.

Also considered and not done: a dedicated error class instead of reusing PeerDisconnectedError, and rejecting when there are no peers at call time as well, which would break the connectServers() then waitForSync('full') pattern the cloud tests use.

This branch has not been deployed

No deployments
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