ankitects
Fix/Retrievability SQL
Re-applies #3980 after it was removed in #4231
I get this error without this change:
Maybe I am being too naive, but if now - last_review_time (or anything similar) is negative, does it not mean that there's some bug in calculating either of the two values (because now should always be greater than last_review_time)?
Here is where it fails:
[rslib/src/storage/sqlite.rs:360:17] &due = 2969
[rslib/src/storage/sqlite.rs:360:17] &ivl = 0
[rslib/src/storage/sqlite.rs:360:17] &days_elapsed = 1155
[rslib/src/storage/sqlite.rs:360:17] &review_day = 2969
[rslib/src/storage/sqlite.rs:360:17] &id = 1678807417574
thread '<unnamed>' panicked at rslib/src/storage/sqlite.rs:361:17:
attempt to multiply with overflow
https://github.com/ankitects/anki/blob/4dc00556c18c2526ac194ad3ca0e76b4c3fd83d4/rslib/src/storage/sqlite.rs#L350-L351
This is after running check database. I checked the cid and it is for a completely new card with no history.
| id | nid | did | ord | mod | usn | type | queue | due | ivl | factor | reps | lapses | left | odue | odid | flags | data |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 1739094681986 | 1739094681986 | 1746616033777 | 0 | 1757582092 | 48827 | 0 | 0 | -99332 | 0 | 0 | 0 | 0 | 1 | 10161 | 1739093429439 | 0 | {"lrt":0} |
| 1678807417574 | 1678807417574 | 1755906348641 | 1 | 1755906365 | 47713 | 0 | -1 | 2969 | 0 | 0 | 0 | 0 | 1 | 0 | 0 | 1 | {} |
These are 2 different cards which both trigger the bug.
it is for a completely new card with no history
This function (extract_fsrs_retrievability) shouldn't even be called for a new card because due has a different meaning for new cards. So, it requires a fix in https://github.com/ankitects/anki/blob/dac26ce67147b261d79a56092320cc2f5af0d990/rslib/src/search/mod.rs#L381-L386 and probably https://github.com/ankitects/anki/blob/dac26ce67147b261d79a56092320cc2f5af0d990/rslib/src/search/sqlwriter.rs#L432-L442 too.
In addition, I suggest the following changes, where I fixed some bugs, removed redundancy and renamed variables to avoid confusion.
diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs
index 3ce1baff0..d6be0f7d9 100644
--- a/rslib/src/storage/sqlite.rs
+++ b/rslib/src/storage/sqlite.rs
@@ -344,11 +344,11 @@ fn add_extract_fsrs_retrievability(db: &Connection) -> rusqlite::Result<()> {
let Ok(ivl) = ctx.get_raw(2).as_i64() else {
return Ok(None);
};
- let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else {
+ let Ok(today) = ctx.get_raw(3).as_i64() else {
return Ok(None);
};
let review_day = due.saturating_sub(ivl);
- days_elapsed.saturating_sub(review_day) as u32 * 86_400
+ today.saturating_sub(review_day) as u32 * 86_400
};
let decay = card_data.decay.unwrap_or(FSRS5_DEFAULT_DECAY);
let retrievability = card_data.memory_state().map(|state| {
@@ -387,17 +387,18 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
let Ok(now) = ctx.get_raw(5).as_i64() else {
return Ok(None);
};
- let days_elapsed = if due > 365_000 {
- // (re)learning
- (next_day_at as u32).saturating_sub(due as u32) / 86_400
+ let secs_elapsed = if due > 365_000 {
+ // (re)learning card having due in seconds
+ now.saturating_sub(due) as u32
} else {
- let Ok(days_elapsed) = ctx.get_raw(2).as_i64() else {
+ let Ok(today) = ctx.get_raw(2).as_i64() else {
return Ok(None);
};
let review_day = due.saturating_sub(interval);
- (days_elapsed as u32).saturating_sub(review_day as u32)
+ today.saturating_sub(review_day) as u32 * 86_400
};
+ let days_elapsed = secs_elapsed / 86_400;
if let Ok(card_data) = ctx.get_raw(0).as_str() {
if !card_data.is_empty() {
let card_data = &CardData::from_str(card_data);
@@ -411,23 +412,7 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
let seconds_elapsed =
if let Some(last_review_time) = card_data.last_review_time {
now.saturating_sub(last_review_time.0) as u32
- } else if due > 365_000 {
- // (re)learning card in seconds
- let Ok(ivl) = ctx.get_raw(2).as_i64() else {
- return Ok(None);
- };
- let last_review_time = due.saturating_sub(ivl);
- now.saturating_sub(last_review_time) as u32
- } else {
- let Ok(ivl) = ctx.get_raw(2).as_i64() else {
- return Ok(None);
- };
- let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else {
- return Ok(None);
- };
- let review_day = due.saturating_sub(ivl);
- days_elapsed.saturating_sub(review_day) as u32 * 86_400
- };
+ } else secs_elapsed;
let current_retrievability = FSRS::new(None)
.unwrap()
due: -99332
Ideally, check database should fix this negative due for a new card because, according to the documentation, the due of new cards starts from 1.
I can't apply the diff. I think you created the diff on the main branch rather than for this PR.
renamed variables to avoid confusion.
I think its probably best to keep the variable names the same as the columns in the SQL database right? Maybe comments would be helpful here instead.
I think you created the diff on the main branch rather than for this PR.
Yes, and that's because I think that the changes in this PR just hide the problem instead of solving the problem. So, I think that we should revert them and fix the actual issue, i.e., prevent search/mod.rs from calling extract_fsrs_retrievability for new cards.
its probably best to keep the variable names the same as the columns in the SQL database right?
I renamed days_elapsed to today. Neither of these is in the SQL database. So, it's fine. The previous code was confusing because it read let days_elapsed = ... days_elapsed.saturating_sub(review_day)
Yes, and that's because I think that the changes in this PR just hide the problem instead of solving the problem. So, I think that we should revert them and fix the actual issue, i.e., prevent search/mod.rs from calling extract_fsrs_retrievability for new cards.
I don't think that it's a good idea to leave this bug in the code, even if it should ideally be impossible to trigger.
Here's a patch created on your PR:
diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs
index bd0968104..2ad67884f 100644
--- a/rslib/src/storage/sqlite.rs
+++ b/rslib/src/storage/sqlite.rs
@@ -350,11 +350,11 @@ fn add_extract_fsrs_retrievability(db: &Connection) -> rusqlite::Result<()> {
let Ok(ivl) = ctx.get_raw(2).as_i64() else {
return Ok(None);
};
- let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else {
+ let Ok(today) = ctx.get_raw(3).as_i64() else {
return Ok(None);
};
let review_day = (due as u32).saturating_sub(ivl as u32);
- (days_elapsed as u32).saturating_sub(review_day) * 86_400
+ (today as u32).saturating_sub(review_day) * 86_400
};
let decay = card_data.decay.unwrap_or(FSRS5_DEFAULT_DECAY);
let retrievability = card_data.memory_state().map(|state| {
@@ -393,17 +393,21 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
let Ok(now) = ctx.get_raw(5).as_i64() else {
return Ok(None);
};
- let days_elapsed = if due > 365_000 {
- // (re)learning
- (next_day_at as u32).saturating_sub(due as u32) / 86_400
+ let secs_elapsed = if due > 365_000 {
+ // (re)learning card with due in seconds
+
+ // Don't change this to now.subtracting_sub(due) as u32
+ // for the same reasons listed in the comment
+ // in add_extract_fsrs_retrievability
+ (now as u32).saturating_sub(due as u32)
} else {
- let Ok(days_elapsed) = ctx.get_raw(2).as_i64() else {
+ let Ok(today) = ctx.get_raw(2).as_i64() else {
return Ok(None);
};
let review_day = due.saturating_sub(interval);
-
- (days_elapsed as u32).saturating_sub(review_day as u32)
+ (today as u32).saturating_sub(review_day as u32) * 86_400
};
+ let days_elapsed = secs_elapsed / 86_400;
if let Ok(card_data) = ctx.get_raw(0).as_str() {
if !card_data.is_empty() {
let card_data = &CardData::from_str(card_data);
@@ -417,26 +421,7 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
let seconds_elapsed =
if let Some(last_review_time) = card_data.last_review_time {
now.saturating_sub(last_review_time.0) as u32
- } else if due > 365_000 {
- // (re)learning card in seconds
- let Ok(ivl) = ctx.get_raw(2).as_i64() else {
- return Ok(None);
- };
- // Don't change this to now.subtracting_sub(last_review_time) as u32
- // for the same reasons listed in the comment
- // in add_extract_fsrs_retrievability
- let last_review_time = due.saturating_sub(ivl) as u32;
- (now as u32).saturating_sub(last_review_time)
- } else {
- let Ok(ivl) = ctx.get_raw(2).as_i64() else {
- return Ok(None);
- };
- let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else {
- return Ok(None);
- };
- let review_day = due.saturating_sub(ivl);
- days_elapsed.saturating_sub(review_day) as u32 * 86_400
- };
+ } else secs_elapsed;
let current_retrievability = FSRS::new(None)
.unwrap()
Here's another diff created on top of the previous one. It makes the order of the arguments in extract_fsrs_relative_retrievability and extract_fsrs_retrievability the same. This will prevents bugs like those fixed by my previous diff.
diff --git a/rslib/src/storage/card/filtered.rs b/rslib/src/storage/card/filtered.rs
index ef436f6e8..03f845f4e 100644
--- a/rslib/src/storage/card/filtered.rs
+++ b/rslib/src/storage/card/filtered.rs
@@ -54,7 +54,7 @@ fn build_retrievability_query(
) -> String {
if fsrs {
format!(
- "extract_fsrs_relative_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, {today}, ivl, {next_day_at}, {now}) {order}"
+ "extract_fsrs_relative_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, ivl, {today}, {next_day_at}, {now}) {order}"
)
} else {
format!(
diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs
index 3a5066ff4..9e06edf07 100644
--- a/rslib/src/storage/card/mod.rs
+++ b/rslib/src/storage/card/mod.rs
@@ -837,7 +837,7 @@ impl fmt::Display for ReviewOrderSubclause {
let next_day_at = timing.next_day_at.0;
let now = timing.now.0;
temp_string =
- format!("extract_fsrs_relative_retrievability(data, case when odue !=0 then odue else due end, {today}, ivl, {next_day_at}, {now}) {order}");
+ format!("extract_fsrs_relative_retrievability(data, case when odue !=0 then odue else due end, ivl, {today}, {next_day_at}, {now}) {order}");
&temp_string
}
ReviewOrderSubclause::Added => "nid asc, ord asc",
diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs
index 2ad67884f..dc44e9cd2 100644
--- a/rslib/src/storage/sqlite.rs
+++ b/rslib/src/storage/sqlite.rs
@@ -370,7 +370,7 @@ fn add_extract_fsrs_retrievability(db: &Connection) -> rusqlite::Result<()> {
}
/// eg. extract_fsrs_relative_retrievability(card.data, card.due,
-/// timing.days_elapsed, card.ivl, timing.next_day_at, timing.now) -> float |
+/// card.ivl, timing.days_elapsed, timing.next_day_at, timing.now) -> float |
/// null. The higher the number, the higher the card's retrievability relative
/// to the configured desired retention.
fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result<()> {
@@ -384,7 +384,7 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
let Ok(due) = ctx.get_raw(1).as_i64() else {
return Ok(None);
};
- let Ok(interval) = ctx.get_raw(3).as_i64() else {
+ let Ok(interval) = ctx.get_raw(2).as_i64() else {
return Ok(None);
};
let Ok(next_day_at) = ctx.get_raw(4).as_i64() else {
@@ -401,7 +401,7 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result
// in add_extract_fsrs_retrievability
(now as u32).saturating_sub(due as u32)
} else {
- let Ok(today) = ctx.get_raw(2).as_i64() else {
+ let Ok(today) = ctx.get_raw(3).as_i64() else {
return Ok(None);
};
let review_day = due.saturating_sub(interval);
Your code doesn't account for rollover.
This was discussed in https://github.com/ankitects/anki/pull/4231#issuecomment-3126100319 and I got convinced by Jarrett's arguments.
Plus, in the main branch version of this code, elapsed_days is calculated twice. One considers the rollover (because Jarrett forgot to update it) and one doesn't. My patch de-duplicates them.
Just wanted to remind that we haven't fixed the real issue yet:
it is for a completely new card with no history
This function (extract_fsrs_retrievability) shouldn't even be called for a new card because due has a different meaning for new cards. So, it requires a fix in https://github.com/ankitects/anki/blob/dac26ce67147b261d79a56092320cc2f5af0d990/rslib/src/search/mod.rs#L381-L386 and probably https://github.com/ankitects/anki/blob/dac26ce67147b261d79a56092320cc2f5af0d990/rslib/src/search/sqlwriter.rs#L432-L442 too.
Yes, either that or something like the column::Due
https://github.com/ankitects/anki/blob/dac26ce67147b261d79a56092320cc2f5af0d990/rslib/src/search/mod.rs#L367
For e.g.
case when c.type = 0 then {default} else extract_fsrs_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, c.ivl, {}, {}, {}) asc
This has the advantage that we can use different default values depending on the purpose for which we are using the retrievability.
For future reference I deleted the above comment he's responding to because I had a similar idea. But I said we should pass the type to the sql and return None if its a new card.