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.
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
MSC3391: API to delete account data #3391
base: main
Are you sure you want to change the base?
MSC3391: API to delete account data #3391
Changes from all commits
150b07b
faacd53
1864715
9c1ac3f
861933a
cd78ac6
60b1bd3
88f7f7c
da1328a
333264f
ae1785a
75b1ade
edc0d54
7516cf6
e2fce0a
0653f34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not entirely clear to me that we should have both of these mechanisms. My experience is that, where the protocol allows two ways of doing a thing, they will end up being subtly different somehow and it leads to compatibility problems (eg, one server implementation will forget to implement DELETE because the client they test with only uses PUT).
Given that we don't have any special server-side semantics for
PUT {}
, can we stick with that situation, and only do the special "deletion" behaviour for an actualDELETE
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this MSC,
{}
effectively is being equated to "Deleted", reversing that in many aspects would mean that suddenly the server can return either{}
or a404
, being the difference betweennull
and""
, to grab an example/analogy.The primary benefit of using
DELETE
overPUT {}
is that backwards compatibility is held, the semantics is made more clear (would you expect aPUT {}
to delete the key entirely, such that a404
is returned), and "alternate ways" of doing the same thing in a way that is confusing is avoided.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, should we define
PUT {}
to return http 400, at the minuscule risk of breaking existing clients?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to document our particularly complex portion of the work when implementing this MSC in Synapse. Not a blocker or an issue with the feature (edit: and the problem is not unique to this feature either).
After an account data entry has been deleted, that change must be broadcast to all devices. Once that's been done, it would be ideal if we could remove the record of that account data type ever being set in the homeserver (for reduction of user data and (slight) storage benefits).
This gets a little complicated when you try to quantify at what point all devices have a user have seen the delete. One way to do this (what is currently planned for Synapse) is to track the devices that haven't yet seen the delete, and then tick them off the list once they have.
But what does it mean for a device to have seen an account data change? If they have
/sync
'd the change, we can likely say they've seen the change. We should be careful though, as the device may have error'd in processing the/sync
response, and if it retries the request after we've deleted the row, then the response will not include the delete(!). Instead, we should only assume the client has successfully seen the delete when they make a subsequent/sync
request with asince
token that implies they've processed the delete (Synapse'ssince
/next_batch
tokens allow for this as they encode points in streams; other homeserver implementations may not have this property).And what if a device requests
GET /_matrix/client/v3/user/{userId}/account_data/{type}
after a delete has occurred? Should we assume that the device has seen the delete? Initially I thought 'yes', but @reivilibre pointed out that this means a request toGET /_matrix/client/v3/user/{userId}/account_data/{type}
could mean (if the device was the last to see the delete) that the delete would then not come down/sync
- which feels like a massive footgun. As a result, we've decided not to assume a call toGET /_matrix/client/v3/user/{userId}/account_data/{type}
means a device has seen a delete, and instead of use/sync
requests.All of this falls squarely into the area of implementation - but it may be useful to other homeserver implementations. None of this is an issue if the implementation chooses to keep "deleted" account data entries in their database forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably servers can implement the same logic as for when they need to figure out whether a device has seen a to-device message (so that the server can delete it)? Especially as to-device messages have a better case for robustness than account data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - it is indeed very similar. The only difference is that account data has this extra way of being accessed (via
GET /_matrix/client/v3/user/{userId}/account_data/{type}
). So whether you also count that as seeing the change is the only decision you need to make if you've already settled on a design for deleting old to-device messages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced it does, entirely. It seems important to me that we specify when exactly a client can expect the 'deleted' account data to be served.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something obvious but how much is this problem different from any other change in account data? What if it's a good ol' PUT and some other device crashes at processing the /sync batch that has this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShadowJonathan this thread appears to be unresolved, and I think it's blocking FCP from starting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we even tie access tokens to device IDs like this, or do we assume this per access token?
That brings up a whole can of worms for me too, and so i wish to assume that deletion of account data be a best-effort case only.
I think that enumerating and deleting account data only if all access tokens have seen it is not ideal from a performance perspective, since stale access tokens can "block" deletion of data like this, and it'd require a whole tracking system for what-access-token-has-seen-what in implementations, which i'm not aware exists right now.
Ultimately i'd wish to lean on @anoadragon453 for opinion on this; I think that
/account_data/
should reflect the current state at any time, and that it does not interact or touch "if the device has seen it or not", since any access token would already be syncing if it sendsaccount_data
requests like this, since a client is expected to sync continuously when its online.However, what about clients that are offline? Do we expect implementations to internally store tombstones to their stream tokens, to notify remaining access tokens of such a delete? Won't that go against the storage savings, or will it increase performance as it only queries a "small" table every time such a sync happens, to search for such tombstones?
What if a client re-uses an old sync token, after it has restarted sync, or used an additional sync? This behaviour is undefined, and with the case of deleting data, even the reference of itself that it has been deleted, it might become problematic.
And i concur with @richvdh, this is not implementation-specific, as then different implementations will have different ways of specifying how and when data is deleted.
Imo, it should be tied to
/sync
, and/account_data/
should always serve the current state, even if that results in a race condition between the two calls, as to which one returns the "this data has been deleted" state to the client first.