fix errors when undoing/redoing after a queue-invalidating operation

There were a few issues going on here:

- If some operation had invalidated the queues, they were subsequently
recreated with a call to .get_queues() in the undo handling code. This
could happen after the changes to the card had already been reverted,
leading to a queue state that didn't match our expectations.
- More generally, it's not safe to assume our mutations will apply
cleanly after the queue has been rebuilt. The next card will vary
depending on the number of remaining cards when interspersing cards of
different types, and a queue-invalidating operation will have changed
the learning cutoff.

So rather than rebuilding the queues on demand, we now check that they
already exist, and were created at the time we expect. If not, we
invalidate them and skip applying the mutations, and a subsequent
refresh of the UI should rebuild the queues correctly.

As part of this change, the cutoff snapshot has been moved into the
normal answer update object.

One possible downside here is that adding a note during review may cause
a newly due learning card to appear when undoing a different review.
If this proves to be a problem, we could potentially note down the
learning cutoff and apply it when queues are rebuilt later.

rslib/src/scheduler/queue/builder/mod.rs

@@ -166,6 +166,7 @@ impl QueueBuilder {
             learn_ahead_secs,
             selected_deck,
             current_day,
+            build_time: TimestampMillis::now(),
             current_learning_cutoff: now,
         }
     }

rslib/src/scheduler/queue/learning.rs

@@ -35,7 +35,7 @@ impl CardQueues {
 
     /// Increase the cutoff to the current time, and increase the learning count
     /// for any new cards that now fall within the cutoff.
-    pub(super) fn update_learning_cutoff_and_count(&mut self) -> Box<CutoffSnapshot> {
+    pub(super) fn update_learning_cutoff_and_count(&mut self) -> CutoffSnapshot {
         let change = CutoffSnapshot {
             learning_count: self.counts.learning,
             learning_cutoff: self.current_learning_cutoff,
@@ -50,7 +50,7 @@ impl CardQueues {
             .count();
         self.counts.learning += new_learning_cards;
 
-        Box::new(change)
+        change
     }
 
     /// Given the just-answered `card`, place it back in the learning queues if it's still

rslib/src/scheduler/queue/mod.rs

@@ -26,6 +26,7 @@ pub(crate) struct CardQueues {
     selected_deck: DeckId,
     current_day: u32,
     learn_ahead_secs: i64,
+    build_time: TimestampMillis,
     /// Updated each time a card is answered, and by get_queued_cards() when the
     /// counts are zero. Ensures we don't show a newly-due learning card after a
     /// user returns from editing a review card.
@@ -190,12 +191,14 @@ impl Collection {
         if let Some(queues) = &mut self.state.card_queues {
             let entry = queues.pop_entry(card.id)?;
             let requeued_learning = queues.maybe_requeue_learning_card(card, timing);
-            let cutoff_change = queues.update_learning_cutoff_and_count();
+            let cutoff_snapshot = queues.update_learning_cutoff_and_count();
+            let queue_build_time = queues.build_time;
             self.save_queue_update_undo(Box::new(QueueUpdate {
                 entry,
                 learning_requeue: requeued_learning,
+                queue_build_time,
+                cutoff_snapshot,
             }));
-            self.save_cutoff_change(cutoff_change);
         } else {
             // we currenly allow the queues to be empty for unit tests
         }
@@ -203,9 +206,40 @@ impl Collection {
         Ok(())
     }
 
+    /// Get the card queues, building if necessary.
     pub(crate) fn get_queues(&mut self) -> Result<&mut CardQueues> {
-        let timing = self.timing_today()?;
         let deck = self.get_current_deck()?;
+        self.clear_queues_if_day_changed()?;
+        if self.state.card_queues.is_none() {
+            self.state.card_queues = Some(self.build_queues(deck.id)?);
+        }
+
+        Ok(self.state.card_queues.as_mut().unwrap())
+    }
+
+    // Returns queues if they are valid and have not been rebuilt. If build time has changed,
+    // they are cleared.
+    pub(crate) fn get_or_invalidate_queues(
+        &mut self,
+        build_time: TimestampMillis,
+    ) -> Result<Option<&mut CardQueues>> {
+        self.clear_queues_if_day_changed()?;
+        let same_build = self
+            .state
+            .card_queues
+            .as_ref()
+            .map(|q| q.build_time == build_time)
+            .unwrap_or_default();
+        if same_build {
+            Ok(self.state.card_queues.as_mut())
+        } else {
+            self.clear_study_queues();
+            Ok(None)
+        }
+    }
+
+    fn clear_queues_if_day_changed(&mut self) -> Result<()> {
+        let timing = self.timing_today()?;
         let day_rolled_over = self
             .state
             .card_queues
@@ -215,11 +249,7 @@ impl Collection {
         if day_rolled_over {
             self.discard_undo_and_study_queues();
         }
-        if self.state.card_queues.is_none() {
-            self.state.card_queues = Some(self.build_queues(deck.id)?);
-        }
-
-        Ok(self.state.card_queues.as_mut().unwrap())
+        Ok(())
     }
 }
 

rslib/src/scheduler/queue/undo.rs

@@ -8,13 +8,14 @@ use crate::prelude::*;
 pub(crate) enum UndoableQueueChange {
     CardAnswered(Box<QueueUpdate>),
     CardAnswerUndone(Box<QueueUpdate>),
-    CutoffChange(Box<CutoffSnapshot>),
 }
 
 #[derive(Debug)]
 pub(crate) struct QueueUpdate {
     pub entry: QueueEntry,
     pub learning_requeue: Option<LearningQueueEntry>,
+    pub queue_build_time: TimestampMillis,
+    pub cutoff_snapshot: CutoffSnapshot,
 }
 
 /// Stores the old learning count and cutoff prior to the
@@ -29,30 +30,27 @@ impl Collection {
     pub(crate) fn undo_queue_change(&mut self, change: UndoableQueueChange) -> Result<()> {
         match change {
             UndoableQueueChange::CardAnswered(update) => {
-                let queues = self.get_queues()?;
-                if let Some(learning) = &update.learning_requeue {
-                    queues.remove_intraday_learning_card(learning.id);
+                if let Some(queues) = self.get_or_invalidate_queues(update.queue_build_time)? {
+                    queues.restore_cutoff(&update.cutoff_snapshot);
+                    if let Some(learning) = &update.learning_requeue {
+                        queues.remove_intraday_learning_card(learning.id);
+                    }
+                    queues.push_undo_entry(update.entry);
                 }
-                queues.push_undo_entry(update.entry);
                 self.save_undo(UndoableQueueChange::CardAnswerUndone(update));
 
                 Ok(())
             }
             UndoableQueueChange::CardAnswerUndone(update) => {
-                let queues = self.get_queues()?;
-                queues.pop_entry(update.entry.card_id())?;
-                if let Some(learning) = update.learning_requeue {
-                    queues.insert_intraday_learning_card(learning);
+                if let Some(queues) = self.get_or_invalidate_queues(update.queue_build_time)? {
+                    queues.pop_entry(update.entry.card_id())?;
+                    if let Some(learning) = update.learning_requeue {
+                        queues.insert_intraday_learning_card(learning);
+                    }
+                    queues.restore_cutoff(&update.cutoff_snapshot);
                 }
                 self.save_undo(UndoableQueueChange::CardAnswered(update));
 
-                Ok(())
-            }
-            UndoableQueueChange::CutoffChange(change) => {
-                let queues = self.get_queues()?;
-                let current = queues.restore_cutoff(&change);
-                self.save_cutoff_change(current);
-
                 Ok(())
             }
         }
@@ -61,10 +59,6 @@ impl Collection {
     pub(super) fn save_queue_update_undo(&mut self, change: Box<QueueUpdate>) {
         self.save_undo(UndoableQueueChange::CardAnswered(change))
     }
-
-    pub(super) fn save_cutoff_change(&mut self, change: Box<CutoffSnapshot>) {
-        self.save_undo(UndoableQueueChange::CutoffChange(change))
-    }
 }
 
 #[cfg(test)]
@@ -216,9 +210,7 @@ mod test {
         // first card graduates
         col.answer_good();
         assert_eq!(col.counts(), [0, 1, 0]);
-        // other card is all that is left, so the
-        // last step is deferred
-        col.answer_good();
+        col.answer_easy();
         assert_eq!(col.counts(), [0, 0, 0]);
 
         // now work backwards
@@ -251,4 +243,38 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn redo_after_queue_invalidation_bug() -> Result<()> {
+        // add a note to the default deck
+        let mut col = open_test_collection();
+        let _nid = add_note(&mut col, true)?;
+
+        // add a deck and select it
+        let mut deck = Deck::new_normal();
+        deck.name = NativeDeckName::from_human_name("foo");
+        col.add_deck(&mut deck)?;
+        col.set_current_deck(deck.id)?;
+
+        // select default again, which invalidates current queues
+        col.set_current_deck(DeckId(1))?;
+
+        // get the first card and answer it
+        col.answer_easy();
+
+        // undo answer
+        col.undo()?;
+
+        // undo deck select, which invalidates the queues again
+        col.undo()?;
+
+        // redo deck select (another invalidation)
+        col.redo()?;
+
+        // when the card answer is redone, it shouldn't fail because
+        // the queues are rebuilt after the card state is restored
+        col.redo()?;
+
+        Ok(())
+    }
 }