Skip to content

Conversation

simolus3
Copy link
Contributor

@simolus3 simolus3 commented Apr 3, 2025

Under our consistency guarantee, we can't make data from new checkpoints visible if there are local changes that haven't been uploaded yet. Our current implementation for this case (recognizable by ready = false) was to simply do nothing. That's not unreasonable: After all, the pending local changes are supposed to be uploaded eventually, which should trigger new checkpoint messages that we'd then apply.

However, there is a potential race condition when a pending upload is currently in progress. While the connector is uploading changes (and before finishing), some data may already be uploaded to the database, triggering the sync service to send new checkpoints. These checkpoints would be entirely ignored, because we're already uploading data and there is no subsequent upload to trigger another one.

To fix that issue, this PR improves the checkpoint handling logic. If we get a checkpoint complete message that didn't apply because of local data or a write_checkpoint mismatch, and we're currently in the process of uploading changes or waiting for a write-checkpoint2.json request, then await that before trying the same checkpoint again. Either,

  • the same checkpoint message applies now (in case it was emitted before the connector completes or the write-checkpoint2.json returns). Or,
  • we can expect another checkpoint to come up soon (since we've waited for the response and the old one didn't work). Doing nothing is the correct behavior here.

@simolus3 simolus3 requested a review from stevensJourney April 3, 2025 14:30
Copy link

changeset-bot bot commented Apr 3, 2025

🦋 Changeset detected

Latest commit: 5f877a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@powersync/common Patch
@powersync/node Patch
@powersync/op-sqlite Patch
@powersync/react-native Patch
@powersync/tanstack-react-query Patch
@powersync/web Patch
@powersync/diagnostics-app Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@stevensJourney stevensJourney left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks finding and fixing this!

@simolus3 simolus3 merged commit ec5137e into main Apr 3, 2025
6 checks passed
@simolus3 simolus3 deleted the fix-download-race-condition branch April 3, 2025 15:00
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.

2 participants