Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tombstones #31

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Tombstones #31

merged 4 commits into from
Jun 1, 2023

Conversation

mistermoe
Copy link
Collaborator

@mistermoe mistermoe commented Jun 1, 2023

When a record is deleted via RecordsDelete, the initial RecordsWrite is kept as a tombstone in addition
to the RecordsDelete message. The data associated to that initial RecordsWrite is deleted. If a record was written
and deleted before it ever got to dwn-server, we end up in a situation where we still need to process the tombstone
so that we can process the RecordsDelete. This can and does happen when syncing.

This PR introduces the ability to process RecordsWrite messages with no associated data.

@mistermoe mistermoe requested a review from frankhinek as a code owner June 1, 2023 00:58
@frankhinek
Copy link
Collaborator

frankhinek commented Jun 1, 2023

Although we’re planning to discuss the trade offs of this approach tomorrow, since we’ve got to get this bug fix in before any conclusion to that discussion, we should include a test that passes because a subsequent RecordsWrite (an overwrite) that mutates a descriptor property (but not data) takes the synchronizePrunedInitialRecordsWrite() path. Perhaps a Sinon spy would work?

@michaelneale
Copy link
Contributor

does this change anything around data retention - like is there a case where record could have been deleted on client and never made it to server but now does with a tombstone? that is my only feedback.

@frankhinek
Copy link
Collaborator

frankhinek commented Jun 1, 2023

As discussed with @mistermoe and @thehenrytsai this afternoon:

  • introduce a fix for the sync issue as implemented in this PR
  • quickly follow with one of the two following changes:
    1. dwn-sdk-js change would be to modify the synchronizePrunedInitialRecordsWrite() method signature to take as input a RecordsWrite message with no associated data && a RecordsDelete.
    2. dwn-server and web5-js modification to introduce a new endpoint / RPC handler that is dedicated to sync so that RecordsWrite overwrites that mutate properties other than data don't flow through synchronizePrunedInitialRecordsWrite() unintentionally.

Created Issue #32 to track.

@mistermoe
Copy link
Collaborator Author

mistermoe commented Jun 1, 2023

does this change anything around data retention - like is there a case where record could have been deleted on client and never made it to server but now does with a tombstone? that is my only feedback.

@michaelneale currently, anytime web5-js encounters a sync error it halts syncing. after this PR is merged, dwn-servers are redeployed, and web5-js PR is merged, (assuming people version bump) people who are encountering this error won't halt anymore and everything should work as expected.

@mistermoe mistermoe merged commit d4a953d into main Jun 1, 2023
@LiranCohen LiranCohen deleted the tombstones branch September 5, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants