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

Collection in a state where it's in an inconsistent state on every sync #5666

Closed
telotortium opened this issue Dec 20, 2019 · 22 comments · Fixed by #5860
Closed

Collection in a state where it's in an inconsistent state on every sync #5666

telotortium opened this issue Dec 20, 2019 · 22 comments · Fixed by #5860

Comments

@telotortium
Copy link

Reproduction Steps
  1. Make any change in Ankidroid
  2. Perform sync from Ankidroid
Expected Result

Sync completes without conflicts.

Actual Result

Sync seems to proceed as normal - data is uploaded and downloaded - and the Ankidroid complains that the local collection is inconsistent. Making changes to the collection on the desktop doesn't result in inconsistencies on the desktop side.

Debug info

Refer to the support page if you are unsure where to get the "debug info".

AnkiDroid Version = 2.10alpha17

Android Version = 10

ACRA UUID = b6658709-db1a-4b5b-a883-36f015a21153
Research

Enter an [ x ] character to confirm the points below:

[X] I have read the support page and am reporting a bug or enhancement request specific to AnkiDroid

[X] I have checked the manual and the FAQ and could not find a solution to my issue

[X] I have searched for similar existing issues here and on the user forum

@telotortium
Copy link
Author

Seems to have only started happening after I used "Select all" on some cards to change them at once.

@mikehardy
Copy link
Member

  • try check database
  • try advanced preferences and force sync then (be careful! take a backup!) upload or download from ankiweb to mobile to force it to be in sync

One of those might clear things out? While recognizing there may be something underlying causing it

@telotortium
Copy link
Author

I was hoping there would be a way to determine what might be causing it. It's intermittent for me.

@mikehardy
Copy link
Member

There probably is a way and I want to know the cause too, but I was hoping I could get you a workaround that worked for now through the holidays as I won't have time for non-emergency items for quite a while...

@telotortium
Copy link
Author

It's not urgent for me. I was hoping there would be some sort of verbose debugging that might reveal something.

@mikehardy
Copy link
Member

There is but you have to turn on developer mode in your phone (or run the app in an Android Studio development emulator) and listen to logcat - even then it might not show up, but there's a chance. That's beyond most so it's not the first thing I reach for as advice but if you're comfortable with that or want to give it a shot, it's the way to go. Intermittent things are the hardest as clean reproduction scenarios are normally how to pin something down

@Arthur-Milchior
Copy link
Member

By any luck, would you have a deck with more than a thousand cards to see today ?
It seems similar to #5672

@telotortium
Copy link
Author

Actually, I do have such a deck. That might be the issue then. I would never have thought of that.

@Arthur-Milchior
Copy link
Member

Then the solution, temporary I hope, would be to ensure that you select a deck without a lot of due card before syncing. And to do this on every devices, not only on ankidroid.
Here is a small indsights:
Anki does records which was the last selected deck. This is useful mostly on computer where you can press on S to access it. And because that's the default decks where cards will be added.
During sync, to check is everything is going well, anki check whether the server and the computer/smartphone agree on the number of cards to learn/review/discover in the current deck. The three numbers shown in the overview page and the review page.
As far as I get it, ankidroid computes the real number while anki and anki server limits the number to a thousand. And so the sanity check fails.

The reason why I state that you should select a deck with less than a thousand due cards everywhere and not only on ankidroid is that the sanity check is made on the last selected deck. Which might be either the one selected on ankidroid if you used it recently, or the one selected on another computer/phone if it was used more recently and you're trying to synchronize.

There is no way of knowing this if you didn't look closely at the code and/or asked Anki's author for insight he has got on ankiweb; since no one else have access to it.

Note that I might also get things completly wrong. So I'd be interested in knowing whether my advice works.

@mikehardy
Copy link
Member

Wow, that is completely unexpected. Great digging @Arthur-Milchior ! I remember thinking that there was no reason we shouldn't calculate the due number with more precision previously (#5322 #5326 ) but apparently we have a side effect.

I'm not sure what the right thing to do is, but perhaps I can brainstorm some options:

a- only store '1000' (so sync agrees with AnkiWeb/Anki___) while displaying the real number
b- revert the change and just show 1000 again and leave a comment not to touch that one...
c- propose a PR to Anki___ upstream that allows for >1000 stored, whether they want to display it or not
d- some option I have not thought of?

For now it seems like even if the "ideal" solution happens (upstream can store the real quantity) that would take a while to propagate so we should do something for compatibility here. Of those choices is it possible to do option A? If not, I think then at least temporarily B so we don't blow people up

Thoughts?

@Arthur-Milchior
Copy link
Member

There seems to be to be another simpler solution. Compute the numbers as you do now. And then, during sync, instead of sending the number, sends min(1000, realNumber)

I should emphasize that this number is sent appart, during the safety check steps. So it's not like we would need to edit each deck and do a lot of computations.

I didn't really dig anything. Really, it was DAE who explained to me when the syncing fails. And it's even possible that actually that's two unrelated problems.

@mikehardy
Copy link
Member

I like the send min(1000, realNumber) idea, quick+easy fix

@Arthur-Milchior
Copy link
Member

But does not entirely work.
There are two kinds of cards in learning. Cards you'll see again minutes after clicking hard, and cards you'll see again another day. And the limit is applied to both set of cards. Which means that you can have up to 2000 cards in learning mode in a deck in anki (however, something went EXTREMLY wrong if that's actually the case).
Let's say you have 1200 cards from today, and 400 cards from days in the past. Anki will show you there are 1400 cards in learning mode, because he finds 1000 cards from today and 400 from days in the past. Ankidroid will find 1600 cards. And min(1000, 1600) will still get us a wrong answer.

What I'll do is that I'll keep the 1000 limits for cards in learning (which is more than reasonable I believe) and I'll keep the 9999 limits for other kind of cards

@Arthur-Milchior
Copy link
Member

I should note that anyway, anki never really loads 1000 cards. I think it loads 20 cards simultaneously maximum. I assume ankidroid does the same; I've not yet check. So actually the limit 1000 is quite useless in methods which computes the next card to show the user

@Arthur-Milchior
Copy link
Member

@telotortium Quite sorry if you're getting spammed. I don't really know how to write down technical details, and I still went to share them with other devs'.
By the way, unless I'm totally wrong, the bug was introduced by d57e802 from @Ekleog

@Arthur-Milchior
Copy link
Member

Yet another update. Really sorry for the spam.
It seems I was entirely wrong. Maybe I did had two unrelated bugs, I don't know.
the safetycheck is computed using "_walkingCount", which does not uses the limit 1000 (nor 9999 as changed in the above mentionned commit)
The limit used here is the one from the selected deck configuration. Which means that safety check does not bounds to 1000.

I can't reproduce the problem I used to have, so can't even use android studio debugger. Not sure why. Which means anyway that my analysis was potentially unrelated to @telotortium problem (even if I'd still be interesting by having answers to the questions I asked above.)

@Ekleog
Copy link
Contributor

Ekleog commented Jan 26, 2020

For what it's worth, I'm currently having a similar issue in #5728 ; and selecting a deck with less than 1000 cards on anki, then syncing from anki, then on ankidroid (without syncing from ankidroid first because it doesn't allow it without a full download|upload), then trying to sync from ankidroid, still doesn't let ankidroid manage to sync properly.

Also, I do have decks with more than 1000 cards in it (been running pretty much behind recently… worst deck is a filtered deck of ~23k cards, and I also have a few non-filtered decks of a few thousands waiting cards).

If there are other questions you'd like an answer to, feel free to ask, I didn't find them above! I'm not sure my problem is the same as @telotortium's, but it does appear close enough.

@david-allison
Copy link
Member

Context:

We send a triple of (new, learn, review) along with other data for an integrity check.

for (int c : mCol.getSched().counts()) {
sa.put(c);
}
ja.put(sa);

The server checks this value against its values, and recommends a full sync if this doesn't match.


We don't send the 'correct' values in some cases. This is because we've modified mReportLimit

I can't replicate this with normal decks, but I can replicate this with a filtered deck with over 1000 reviews.

_walkingCount uses mReportLimit in the following methods for dynamic decks.

public int _deckNewLimitSingle(JSONObject g) {
try {
if (g.getInt("dyn") != 0) {
return mReportLimit;
}
JSONObject c = mCol.getDecks().confForDid(g.getLong("id"));
return Math.max(0, c.getJSONObject("new").getInt("perDay") - g.getJSONArray("newToday").getInt(1));
} catch (JSONException e) {
throw new RuntimeException(e);
}
}

private int _deckRevLimitSingle(JSONObject d) {
try {
if (d.getInt("dyn") != 0) {
return mReportLimit;
}
JSONObject c = mCol.getDecks().confForDid(d.getLong("id"));
return Math.max(0, c.getJSONObject("rev").getInt("perDay") - d.getJSONArray("revToday").getInt(1));
} catch (JSONException e) {
throw new RuntimeException(e);
}
}

Proposed Design:

  1. Create a new instance of sched while in the syncer, set mReportLimit to 1000.
  2. Add in a method to sched to recalculate the counts without any other calculation
  3. Call the method
  4. Discard the scheduler
  • (refactor) Modify Sched.java and Schedv2.java to replace usages of Method with typed Lambdas - brings them closer to the Python

@mikehardy
Copy link
Member

Seems sensible, but careful with refactors in sched there are a few PRs in flight there, backlogged on stability and perturbing them for mechanical refactors has negative value until the PR stack is a lot cleaner

@david-allison
Copy link
Member

Will leave the refactor for now then. Thanks for the heads up

david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 24, 2020
We modified `mReportLimit` in ankidroid#5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 24, 2020
We modified `mReportLimit` in ankidroid#5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes the issue by using a new scheduler with the correct
`mReportLimit` for calculating the number of cards in the selected deck
@mikehardy mikehardy added this to the 2.9.6 Release milestone Mar 24, 2020
mikehardy pushed a commit that referenced this issue Mar 24, 2020
Fixes #5666 - Sync Failure on Large Dynamic Decks

We modified `mReportLimit` in #5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
@mikehardy
Copy link
Member

2.10alpha55 is out with the fix, as soon as Google is done processing it and starts delivery...

mikehardy pushed a commit that referenced this issue Mar 26, 2020
Fixes #5666 - Sync Failure on Large Dynamic Decks

We modified `mReportLimit` in #5326 to allow a user to see how many
cards they had if there were more than 1000 in a queue.

This had an impact on calculating the number of cards in the current
deck, which is required for a sync to ensure that the server and client
are consistent.

This caused an inconsistency (as we calculate the proper value), and
this meant that a full sync needed to occur if a dynamic deck with over
1000 cards in a queue was selected.

This fixes it by using a new scheduler with the correct `mReportLimit`
for calculating the number of cards in the selected deck
@mikehardy
Copy link
Member

released in 2.9.6beta4

dae added a commit to ankitects/Anki-Android that referenced this issue Jul 9, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 11, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 20, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 22, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 23, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 24, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
dae added a commit to ankitects/Anki-Android that referenced this issue Jul 24, 2022
It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
mikehardy pushed a commit that referenced this issue Jul 27, 2022
It was introduced due to #5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.
bennettj12 added a commit to bennettj12/Anki-Android that referenced this issue Jul 30, 2022
kotlincleanup: additional uses of .apply

Reduce the API surface of AbstractSched

Initial prep work for adding the V3 scheduler. Removed some methods that
are only used internally, and defined some overloads concretely.

eta() implementation in v1 appeared to be functionally identical to v2,
despite the comment.

Remove setReportLimit()

It was introduced due to ankidroid#5666, but AnkiWeb has ignored the due counts
in the sanity check for about 18 months, so this is no longer necessary.

Also remove _updateCutoff() and _checkDay() from the public API.

Split majority of AbstractSched into BaseSched

- Mirrors the code layout of the desktop version, where routines like
buryCard(), that are not specific to V2 or V3, are stored in a separate
file. This leaves AbstractSched containing only the methods that will
need to be implemented separately for V3.
- Moves some methods from SchedV2 into BaseSched, as V3 will want to
use them as well for now.
- Some of the routines in BaseSched will only work with a schema upgrade.
For now, SchedV2 overrides these so they continue to work with the
legacy schema.

Add support for the V3 scheduler

Add V3 scheduler tests

Like the desktop, this works by using a single set of tests that
alter some of the checks depending on the active scheduler version,
so that code duplication is avoided.

Use backend to calculate legacy timezone

I expect this fixes ankidroid#9368

Bumps the backend version, since the API changed.

Closes ankidroid#11881

Backend language tweaks

- Move logic from backend into frontend, so a new backend release is
not required
- Fix mapping of Chinese languages and Hindi; check others
- Don't require a restart after changing language in prefs

Fix LeakCanary warning when changing language/v16 schema

After discarding the backend, Preferences was calling getCol and passing
itself in as the context. This caused the preferences screen to be retained
as the backend lives on. By switching to AnkiDroidApp, a leak can be
avoided.

Fix some connected tests failing when legacy_schema=false

Some were irrelevant; the ones in ContentProviderTest need investigating.

Updated strings from Crowdin

Bumped version to 2.16alpha76

more uses of .apply

reintroduced null checks and removed specified nullable type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants