Conversation
cloudsync_network_send_changes() sent the whole backlog as one batch. A send is all or nothing -- the server confirms the window only once every chunk has applied, and a failed batch is re-sent whole under a new id -- so after a long offline period or a bulk import that batch can be large enough to keep failing, re-uploading everything each attempt. The new 1-argument form sends at most that many local database versions, so the upload becomes several independently confirmed batches and a failure costs one bounded window instead of the backlog. The 0-argument form is unchanged, and cloudsync_network_sync() still calls the unbounded path. The argument counts local versions rather than naming an upper bound. db_version is shared with merges, so the local backlog is sparse in that space: a bound picked without knowing which versions hold local changes can cover a range with none of them, making no progress and giving the caller no way to know how much further to go. Counting keeps the window non-empty whenever anything is pending. A bounded call also reports the newest local version as send.localVersion, so send.status stays out-of-sync while changes remain past the window. Both that value and the window bound come from one pass over the metadata tables: local rows carry site_id 0 there and db_version > since seeks the (db_version) index, so the cost follows the pending backlog. Reading them from cloudsync_changes instead would hit its site_id-only plan, a full scan that materialises every value through cloudsync_col_value(). Nothing advances the send checkpoint past a window that was never announced: later local writes are numbered below such a checkpoint and would be stranded, and the server builds its coverage from announced batches, so the skipped range would stay a permanent gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
cloudsync_network_send_changes()sends the whole backlog as one batch, and a send is all or nothing: the server confirms the window only once every chunk of the batch has applied, and a failed batch is re-sent whole under a new id (network.c:1931-1935).After a long offline period or a bulk import that batch can be large enough to keep failing — on a flaky mobile link, possibly indefinitely — and every attempt re-uploads everything. It is the client-side mirror of a
/checkprepare job that never completes.What
A 1-argument form,
cloudsync_network_send_changes(max_db_versions), sends at most that many local database versions. The upload becomes several independently confirmed batches, so a failure costs one bounded window instead of the whole backlog. The 0-argument form is unchanged, andcloudsync_network_sync()still calls the unbounded path.The window is applied through the vtab's existing
until_db_version, whose legacysincebranch already accepts an upper bound and reportswatermark = until. Every chunk of a bounded batch therefore announces the same window by construction, so the batch stays coherent. No protocol change, no server change, no vtab signature change.The argument counts versions, it does not name a bound
This is the one design decision worth reviewing.
db_versionis shared with merges, so the local backlog is sparse in that space. A caller naming a raw upper bound has no way to know which versions hold local changes: a bound picked blindly can cover a range containing none, which sends nothing, makes no progress, and gives no hint how much further to go.Counting local versions removes the question — the window is non-empty whenever anything is pending, and
cloudsync_network_send_changes(50)reads as "send at most fifty transactions" without any knowledge of the version space. Versions holding only received changes are skipped rather than consuming the budget.Two things deliberately not done
The checkpoint is never advanced past a window that was not announced. Tempting, to stop a caller re-scanning, but wrong twice over: later local writes are numbered below such a checkpoint and would be stranded (
cloudsync_dbversion_next()returnsdb_version + 1), and the server builds its coverage from announced batches (internal/syncstatus/status.go), so the skipped range would remain a permanent gap and a permanentout-of-sync.No new query against
cloudsync_changes. A bounded call needs the newest local version to report the remaining backlog. Taking it from the changes vtab would hit itssite_id-only plan —estimatedCost = INT32_MAX, a full scan materialising every value throughcloudsync_col_value(). Instead one pass over the metadata tables returns both the window bound and the backlog max: local rows carrysite_id = 0there anddb_version > sinceseeks the(db_version)index, so the work follows the pending backlog rather than the table.Testing
test_bounded_sendintest/network_unit.ccovers: identical announced window across every chunk of a batch, the checkpoint following the server's optimistic version, a remaining backlog reportedout-of-syncwith the truelocalVersion, a window stepping over a remote-onlydb_version, and nothing-pending leaving the checkpoint alone.That last assertion makes the status probe answer without a version on purpose — otherwise the server's optimistic value masks the branch the guard lives in, and the test passes whether or not the guard exists. Found that by mutation, after an earlier version of the test passed with the guard removed.
Verified by mutation; each of these fails the test:
since + Nas the bound (does not skip remote-only versions)Suites, all exit 0:
unit(156),review_regressions,network_unit,integration_bootstrap.PostgreSQL is unaffected and was not run:
docker/Makefile.postgresqldoes not compilesrc/network/network.c, the only source file changed.Note for reviewers
This overlaps a patch a customer is carrying against 1.1.2 (
fastrepl/anarlog,sqlite-sync-1.1.2-bounded-send.patch), which solves the same problem with an absoluteuntil_db_version. The mechanism here is the same; the argument is not. If that patch is in use, call sites change — worth confirming with them whether absolute control mattered for a reason not considered here.🤖 Generated with Claude Code