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

Client temporarily forgets its own "seen" actions if exited before sync #1200

Open
eloquence opened this issue Dec 10, 2020 · 4 comments
Open
Labels
bug Something isn't working

Comments

@eloquence
Copy link
Member

Steps to reproduce

  1. Create a handful of new sources
  2. Retrieve them with the SecureDrop Client
  3. Wait until after a sync completes (animation stops)
  4. Click the sources (mark as seen) and wait a few seconds, but not until the next sync
  5. Exit the Client
  6. Restart the Client and log in as the same user

Expected behavior

The Client is in the same state as when I exited it.

Actual behavior

All the sources marked as seen are unseen again, until the next sync.

@eloquence eloquence added the bug Something isn't working label Dec 10, 2020
@eloquence
Copy link
Member Author

eloquence commented Dec 10, 2020

As I understand it, it's important to distinguish between two network opreations here:

It looks like the Client does not record the action in the DB until the sync, which is a similar issue we see with deletions (#1101). In the deletion case, it mainly causes the user experience to be slower than it needs to be (the deletion is clearly marked as "pending" in the UI), but here we briefly report inaccurate information to the user, which is why I've added the bug label.

@sssoleileraaa
Copy link
Contributor

Yup we should not allow the client to forget its own "seen" actions. The only real gotcha with updating the client db as soon as we get the 200 response back from the server is when an ongoing sync is occurring at the same time: we could end up going back from seen to unseen and then seen again once the next sync gets the updated server data. We saw this issue with source account deletion: a source was deleted locally but the ongoing sync said no the source still exists so we go back to showing the source just to learn later that psych! the source was deleted. Our solution was to wait for a full sync cycle to confirm deletion before updating the local db.

The tradeoff for how we currently solve the concurrency/stale sync issue is that if you close the client before the local db is updated then the next time you open the client you'll see stale data until the first client startup sync finishes. For deletion, we can display a pending state in the GUI until the local db is updated, but we don't have a way to do this for seen status which is mostly just bolding/unbolding (and i don't think we want to have a pending state for this). Anyway, I think it's easier to discuss each feature (deletion vs seen/unseen) separately because the are some differences but when we brainstorm solutions let's try to consider how it works for both cases. More to come on possible solutions!

@eloquence
Copy link
Member Author

One idea that may be worth exploring is to somehow detect whether a metadata sync response is stale and, if so, discard it in its entirety and wait for the next one. We sync continuously, so discarding an individual sync is arguably cheap, and much easier than to attempt to sort out which specific data in a sync is no longer fresh.

For example, we could compare the time a sync was requested, and the time of the last user operation that altered the local state (whether or not that state is fully reflected on the server). If we get a sync response corresponding to a request that pre-dates the most recent local state-change, we discard it and wait for the next sync.

@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Sep 14, 2021

Agreed, a model we've discussed, which would affect how we handle every api call, is to drop stale sync data whenever there is a user action that happens between two syncs. We'll need to discuss how we can make this more sophisticated to handle the case where we are dropping lots of syncs because of an active user clicking on sources quickly between syncs.

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

No branches or pull requests

2 participants