Hard-DELETE invalid client records from the server

[linabutler]

The clients engine will currently fail the sync if it sees a tombstone in the clients collection, or any client record that it can't deserialize. Desktop DELETES these from the server if it sees them, and we should do the same.

┆Issue is synchronized with this Jira Task
┆Epic: Sync Ecosystem (backlog)

[rfk]

This sounds like it could potentially be a good-first-issue, so attempting to summarize my understanding of the task. @linacambridge please correct me if I've misunderstood :-)

The clients sync engine has some code here which tries to read a ClientRecord from an item pulled down from the server, which will currently error out if the item can't be turned into a valid ClientRecord:

• application-services/components/sync15/src/clients/engine.rs

  Line 51 in b67c3cc

  let mut client: ClientRecord = payload.into_record()?;

Rather than returning an error here, we should catch it and delete the corresponding item from the server.

However, I couldn't see a nice existing abstraction for hard-deleting items from the server, and the Driver::sync method above doesn't have enough info to make the deletion calls inline when it discovers the invalid records. Strawman proposal for what to do instead:

• Add a deleted field to OutgoingChangeset in components/support/sync15-traits/src/changeset.rs, alongside the existing changes field. This would be a list of BSO ids to delete from the server, rather than a list of new/modified objects to upload.
• Teach the CollectionUpdate struct in components/sync15/src/changeset.rs how to do deletions as well as posting updates.
• Have the clients engine linked above, append the ids of any invalid records to the list of things to be deleted.

@linacambridge what do you think?

[mhammond]

Client records have a ttl, and I believe the spanner server is going to honor them as we expect. While the approach above does seem fine in general, it's a fair bit of work for something that's going to self-destruct anyway - so another option here is probably to just ignore them?

[JN3141]

Hi, I was interested in trying this as a first issue, though see there's a bunch of back and forth above about what the implementation should be like. Did we want to actually just drop the record, or implement something more in-line with what @linacambridge and @rfk suggested?

[mhammond]

I suspect doing the hard delete will be a fair bit of work and in practice has limited value. I think the best thing is to simply ignore such invalid records but otherwise leave them alone.

[mhammond]

Fixed by #4504