Cap steps #1548

RumovZ · 2021-12-10T11:41:26Z

Closes #1518.
I couldn't reproduce the DB error reported by the user. I got some overflow errors in Rust, but I think they only cause panics in dev builds? So I can only hope this fixes it.

change to_secs() to to_secs_capped()

This doesn't seem to be necessary as it already casts f32 to u32, which is saturating. Or did you mean something else?

dae

Hmm, this makes me a bit nervous :-) As you know, the due and original_due columns may store a timestamp, # of days, or a position. Increasing those fields to an i64 will ensure timestamps made after 2038 continue to work, but for the other two cases, it's hard to imagine more than 2 billion days/positions being required/not being a bug. And because any learning stamp that crosses a day boundary is automatically converted to a day span, we theoretically should not need to write a number over an i32 into those columns until 2038 comes around - for now just using an i64 for the days elapsed calculation (but not disk format) is probably sufficient.

Some Anki versions will not be able to open a collection that contains due/odue over an i32 IIRC, and there is code in the DB check that warns about and alters due numbers and positions over ~2 billion and 1 million respectively. I'm a bit concerned that if we bump the storage format up to i64 at the moment, we may end up introducing issues when numbers above an i32 accidentally get written into one of those columns due to a bug or unrestrained settings. So maybe we should put off the column change for now, and just focus on doing the calculation as an i64 so that a learning step of 30 years works today?

dae · 2021-12-13T04:29:00Z

rslib/src/scheduler/answering/learning.rs

@@ -37,20 +37,20 @@ impl CardStateUpdater {
        match interval {
            IntervalKind::InSecs(secs) => {
                self.card.queue = CardQueue::Learn;
-                self.card.due = self.fuzzed_next_learning_timestamp(secs);
+                self.card.due = dbg!(self.fuzzed_next_learning_timestamp(dbg!(secs)));


These look to have been missed

Thanks, I wished there was a lint for that.

Looks like there is one that we could turn on: rust-lang/rust-clippy#3723

Is it practical to turn that on? Looks like it can only be passed when running clippy, and not enabled with a clippy.toml.

Wow, my wish came true fast! Feels like Christmas already. 🎄😃

RumovZ · 2021-12-13T09:55:56Z

And because any learning stamp that crosses a day boundary is automatically converted to a day span

Right, I kind of forgot about that.

The more I look at the code, the less I understand why there would be any problems. Do you have an idea where the DB error could stem from?

focus on doing the calculation as an i64 so that a learning step of 30 years works today?

Looks like not even that is necessary, because in apply_learning_state(), we first call maybe_as_days(), so for any subsequent calculations, seconds have an upper bound of 60 * 60 * 24.

dae · 2021-12-13T10:18:39Z

I can't reproduce it now unfortunately. I saw the learning step and assumed that was the issue, but perhaps there was something else going on there as well, like the current state of the card, or maybe some change to our code in the interim has changed things. Apologies for the wild goose chase :-(

RumovZ · 2021-12-13T10:32:40Z

No problem, at least some improvements could be made. 🙂

dae · 2021-12-13T21:26:32Z

Ok, partly reproduced: set learning steps to 30950d, open the 'previous card info', then answer 'again' on a new card, and you get this in the console: anki.errors.DBError: DbError { info: "IntegralValueOutOfRange(4, -2674080000)", kind: Other }. Presumably that's because we're exceeding the u32 in maybe_as_days()

RumovZ · 2021-12-13T22:01:38Z

Strange.

On main, I get thread '<unnamed>' panicked at 'attempt to multiply with overflow', rslib/src\scheduler\states\steps.rs:54:17 when I try to review a deck with these settings and new cards queued. This is fixed.
On this branch I get thread '<unnamed>' panicked at 'attempt to multiply with overflow', rslib/src\revlog\mod.rs:82:13. I'll push a fix and test some more.
With a binary, beta 2, I get nothing though. This is in accordance with my understanding that overflows are errors only in release builds.

Another potential issue is IntervalKind::as_seconds(). I'll fix that, too.
I don't know why I can't reproduce the DB errors, but they seem to indicate a failed deserialisation. Maybe we are trying to deserialise -2674080000 into a u32 or i32. I'll look into it.

dae · 2021-12-13T22:37:51Z

You may be aware of the things below already, but just in case:

you can test a release build of the source easily with scripts/runopt - no need for a binary release
the fact that overflow checks are off on release builds can just push the error down the road - values may silently wrap, and if a negative number gets written out to the database, it may then fail to deserialize later

RumovZ · 2021-12-13T22:54:16Z

you can test a release build of the source easily with scripts/runopt - no need for a binary release

Thanks for the reminder! So did you reproduce the error with a release build of this branch?

the fact that overflow checks are off on release builds can just push the error down the road - values may silently wrap, and if a negative number gets written out to the database, it may then fail to deserialize later

But a wrapped number is still a valid instance of its data type, an unsigned integer remains unsigned. I may be wrong, but I can only see this happening if we deserialise into a different data type than we serialised from.

dae · 2021-12-13T23:26:07Z

For me it happens in both debug and release builds using this branch. With TRACESQL=1, it looks like it's the revlog we're overflowing:

sql: insert into revlog values (1639437905953,1639430610404,-1,1,-2674080000,-2674080000,0,3353,0)

RumovZ · 2021-12-14T09:52:20Z

Thanks, that helped a lot, the penny finally dropped. I was testing on a v3 profile, where logging is done in Rust and values are wrapped. After reverting to the v2 scheduler, which is executing SQL statements in Python, I can reproduce the DB error.
It also explains why the cap in TS didn't solve it. 30950d fits in a u32, but in revlogs, intervals are stored in i32s, in which it doesn't fit.

Also replace some `as` with `from` and `try_from` as is recommended to highlight potential issues.

Whereas large card intervals are converted to days, revlog intervals use i32s to store large numbers of seconds.

RumovZ · 2021-12-14T13:02:09Z

rslib/src/revlog/mod.rs

@@ -98,9 +99,9 @@ impl Collection {
            cid: card.id,
            usn,
            button_chosen: 0,
-            interval: card.interval as i32,


Unrelated to this PR, but shouldn't it check if interval is in days or secs?

As this code is only run when manually rescheduling a card (which counts as a review), the assigned interval should always be day-based. We could make last_interval negative in the case if the card was currently a learning card, but probably not the highest priority?

Guess not, just wanted to point it out as I stumbled across it. 🙂

dae

Thanks as always Rumo!

dae · 2021-12-15T08:35:49Z

rslib/src/revlog/mod.rs

@@ -98,9 +99,9 @@ impl Collection {
            cid: card.id,
            usn,
            button_chosen: 0,
-            interval: card.interval as i32,


As this code is only run when manually rescheduling a card (which counts as a review), the assigned interval should always be day-based. We could make last_interval negative in the case if the card was currently a learning card, but probably not the highest priority?

dae · 2021-12-15T08:45:19Z

rslib/src/scheduler/states/steps.rs

@@ -5,6 +5,7 @@ const DEFAULT_SECS_IF_MISSING: u32 = 60;

 #[derive(Clone, Copy, Debug, PartialEq)]
 pub(crate) struct LearningSteps<'a> {
+    /// The steps in minutes.


RumovZ force-pushed the cap-steps branch from a224c59 to f4fd967 Compare December 10, 2021 11:42

dae reviewed Dec 13, 2021

View reviewed changes

RumovZ added 3 commits December 13, 2021 10:49

Fix steps being mistaken for seconds

ff903b1

Cap steps at u32::max seconds

bfc0747

Fix overflow of steps in Rust

3b8a349

RumovZ force-pushed the cap-steps branch from e67d2f3 to 3b8a349 Compare December 13, 2021 09:55

RumovZ mentioned this pull request Dec 13, 2021

The inconsistency of the hard button delay with the scheduler 2.1 specifications (learning cards) #1555

Closed

Prevent overflow of IntervalKind

9ad86cc

RumovZ added 4 commits December 14, 2021 10:53

Prevent overflow in revlod/mod.rs

0379ea4

Also replace some `as` with `from` and `try_from` as is recommended to highlight potential issues.

Ensure v2 doesn't store overflowing revlog ivls

80aca32

Lower steps cap in deck options

5c5cc4c

Whereas large card intervals are converted to days, revlog intervals use i32s to store large numbers of seconds.

Format

3a6a4cb

RumovZ force-pushed the cap-steps branch from d96d1a0 to 3a6a4cb Compare December 14, 2021 11:11

RumovZ commented Dec 14, 2021

View reviewed changes

dae reviewed Dec 15, 2021

View reviewed changes

dae merged commit 80ed94e into ankitects:main Dec 15, 2021

RumovZ deleted the cap-steps branch December 15, 2021 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cap steps #1548

Cap steps #1548

RumovZ commented Dec 10, 2021

dae left a comment

dae Dec 13, 2021

RumovZ Dec 13, 2021

dae Dec 13, 2021

RumovZ Dec 13, 2021

dae Dec 13, 2021

RumovZ Dec 14, 2021

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021 •

edited

Loading

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 14, 2021

RumovZ Dec 14, 2021

dae Dec 15, 2021

RumovZ Dec 15, 2021

dae left a comment

dae Dec 15, 2021

dae Dec 15, 2021

Cap steps #1548

Cap steps #1548

Conversation

RumovZ commented Dec 10, 2021

dae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021 • edited Loading

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 13, 2021

dae commented Dec 13, 2021

RumovZ commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dae commented Dec 13, 2021 •

edited

Loading