Skip to content

Conversation

rkistner
Copy link
Contributor

Fix regression from #661. What would happen:

  1. "Could not apply checkpoint due to local data. We will retry applying the checkpoint after that upload is completed." -> sets pendingValidatedCheckpoint to checkpoint A.
  2. Receive a data and checkpoint_complete message -> validates checkpoint B.
  3. Crud upload complete -> queues a crud_upload_completed message.
  4. Attempts this.applyCheckpoint(pendingValidatedCheckpoint) with checkpoint A. Checkpoint A is no longer valid, so this results in "Checksums failed", and re-downloading the data.

Was not part of a stable release, so no changeset required.

@rkistner rkistner requested a review from simolus3 July 14, 2025 13:55
Copy link

changeset-bot bot commented Jul 14, 2025

⚠️ No Changeset found

Latest commit: 489ab32

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

simolus3
simolus3 previously approved these changes Jul 14, 2025
Copy link
Contributor

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

The implementation changes look good to me, but we'll need someone to check the node logging / testing changes I've added.

@simolus3 simolus3 requested a review from Chriztiaan July 14, 2025 15:23
@simolus3 simolus3 force-pushed the fix-retrying-pending-checkpoint branch from d8d43f7 to 489ab32 Compare July 14, 2025 15:29
Copy link
Contributor Author

@rkistner rkistner left a comment

Choose a reason for hiding this comment

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

Test looks good to me

@rkistner rkistner requested a review from stevensJourney July 15, 2025 05:55
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.

I was able to reproduce the checksum failure error previously when testing the YJS demo. After merging in these changes the issue was resolved for me. LGTM.

@simolus3 simolus3 merged commit cb6d3ec into main Jul 15, 2025
9 checks passed
@simolus3 simolus3 deleted the fix-retrying-pending-checkpoint branch July 15, 2025 07:42
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.

3 participants