rework the new scheduler's rollover/day counting code

The previous implementation interpreted the creation date as a local
time, and applied the rollover to that. If the initial creation date
was around midnight local time, even a one hour change due to daylight
savings could result in Anki skipping or doubling up on a day. To
address this, the rollover is now applied to the current time instead
of the creation date.

The new code needs the current time passed into it. This makes it
easier to unit test, and for AnkiWeb to be able to use the user's local
timezone.

The new timezone code is currently disabled, as this code needs to be
ported to all clients before it can be activated.

anki/backend.py

@@ -8,6 +8,8 @@ import anki.backend_pb2 as pb
 
 from .types import AllTemplateReqs
 
+SchedTimingToday = pb.SchedTimingTodayOut
+
 
 class BackendException(Exception):
     def __str__(self) -> str:
@@ -70,3 +72,14 @@ class Backend:
         output = self._run_command(input).template_requirements
         reqs: List[pb.TemplateRequirement] = output.requirements  # type: ignore
         return proto_template_reqs_to_legacy(reqs)
+
+    def sched_timing_today(
+        self, start: int, end: int, offset: int, rollover: int
+    ) -> SchedTimingToday:
+        return self._run_command(
+            pb.BackendInput(
+                sched_timing_today=pb.SchedTimingTodayIn(
+                    created=start, now=end, minutes_west=offset, rollover_hour=rollover,
+                )
+            )
+        ).sched_timing_today

anki/collection.py

@@ -66,13 +66,6 @@ defaultConf = {
 }
 
 
-def timezoneOffset() -> int:
-    if time.localtime().tm_isdst:
-        return time.altzone // 60
-    else:
-        return time.timezone // 60
-
-
 # this is initialized by storage.Collection
 class _Collection:
     db: Optional[DB]
@@ -110,8 +103,6 @@ class _Collection:
             d = datetime.datetime(d.year, d.month, d.day)
             d += datetime.timedelta(hours=4)
             self.crt = int(time.mktime(d.timetuple()))
-        if not server:
-            self.conf["localOffset"] = timezoneOffset()
         self._loadScheduler()
         if not self.conf.get("newBury", False):
             self.conf["newBury"] = True
@@ -139,6 +130,8 @@ class _Collection:
             self.sched = V1Scheduler(self)
         elif ver == 2:
             self.sched = V2Scheduler(self)
+            if not self.server:
+                self.conf["localOffset"] = self.sched.timezoneOffset()
 
     def changeSchedulerVer(self, ver: int) -> None:
         if ver == self.schedVer():
@@ -161,6 +154,13 @@ class _Collection:
 
         self._loadScheduler()
 
+    def localOffset(self) -> Optional[int]:
+        "Minutes west of UTC. Only applies to V2 scheduler."
+        if isinstance(self.sched, V1Scheduler):
+            return None
+        else:
+            return self.sched.timezoneOffset()
+
     # DB-related
     ##########################################################################
 
@@ -194,7 +194,7 @@ DB operations and the deck/tag/model managers do this automatically, so this
 is only necessary if you modify properties of this object or the conf dict."""
         self.db.mod = True
 
-    def flush(self, mod: None = None) -> None:
+    def flush(self, mod: Optional[int] = None) -> None:
         "Flush state to DB, updating mod time."
         self.mod = intTime(1000) if mod is None else mod
         self.db.execute(
@@ -209,7 +209,7 @@ crt=?, mod=?, scm=?, dty=?, usn=?, ls=?, conf=?""",
             json.dumps(self.conf),
         )
 
-    def save(self, name: Optional[str] = None, mod: None = None) -> None:
+    def save(self, name: Optional[str] = None, mod: Optional[int] = None) -> None:
         "Flush, commit DB, and take out another write lock."
         # let the managers conditionally flush
         self.models.flush()

anki/schedv2.py

@@ -12,6 +12,8 @@ from operator import itemgetter
 # from anki.collection import _Collection
 from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union
 
+import anki  # pylint: disable=unused-import
+from anki.backend import SchedTimingToday
 from anki.cards import Card
 from anki.consts import *
 from anki.hooks import runHook
@@ -30,8 +32,9 @@ class Scheduler:
     name = "std2"
     haveCustomStudy = True
     _burySiblingsOnAnswer = True
+    revCount: int
 
-    def __init__(self, col) -> None:
+    def __init__(self, col: "anki.storage._Collection") -> None:
         self.col = col
         self.queueLimit = 50
         self.reportLimit = 1000
@@ -1325,12 +1328,18 @@ where id = ?
 
     def _updateCutoff(self) -> None:
         oldToday = self.today
-        # days since col created
+        timing = self._timingToday()
-        self.today = self._daysSinceCreation()
+
-        # end of day cutoff
+        if self._newTimezoneEnabled():
-        self.dayCutoff = self._dayCutoff()
+            self.today = timing.days_elapsed
+            self.dayCutoff = timing.next_day_at
+        else:
+            self.today = self._daysSinceCreation()
+            self.dayCutoff = self._dayCutoff()
+
         if oldToday != self.today:
             self.col.log(self.today, self.dayCutoff)
+
         # update all daily counts, but don't save decks to prevent needless
         # conflicts. we'll save on card answer instead
         def update(g):
@@ -1367,10 +1376,30 @@ where id = ?
     def _daysSinceCreation(self) -> int:
         startDate = datetime.datetime.fromtimestamp(self.col.crt)
         startDate = startDate.replace(
-            hour=self.col.conf.get("rollover", 4), minute=0, second=0, microsecond=0
+            hour=self._rolloverHour(), minute=0, second=0, microsecond=0
         )
         return int((time.time() - time.mktime(startDate.timetuple())) // 86400)
 
+    def _rolloverHour(self) -> int:
+        return self.col.conf.get("rollover", 4)
+
+    def _newTimezoneEnabled(self) -> bool:
+        return self.col.conf.get("newTimezone", False)
+
+    def _timingToday(self) -> SchedTimingToday:
+        return self.col.backend.sched_timing_today(
+            self.col.crt, intTime(), self.timezoneOffset(), self._rolloverHour(),
+        )
+
+    def timezoneOffset(self) -> int:
+        if self.col.server:
+            return self.col.conf.get("localOffset", 0)
+        else:
+            if time.localtime().tm_isdst:
+                return time.altzone // 60
+            else:
+                return time.timezone // 60
+
     # Deck finished state
     ##########################################################################
 

proto/backend.proto

@@ -4,18 +4,27 @@ package backend_proto;
 
 message Empty {}
 
+// 1-15 reserved for future use; 2047 for errors
+
 message BackendInput {
+    reserved 2047;
     oneof value {
-        PlusOneIn plus_one = 2;
+        TemplateRequirementsIn template_requirements = 16;
-        TemplateRequirementsIn template_requirements = 3;
+        SchedTimingTodayIn sched_timing_today = 17;
+
+        PlusOneIn plus_one = 2046; // temporary, for testing
+
     }
 }
 
 message BackendOutput {
     oneof value {
-        BackendError error = 1;
+        TemplateRequirementsOut template_requirements = 16;
-        PlusOneOut plus_one = 2;
+        SchedTimingTodayOut sched_timing_today = 17;
-        TemplateRequirementsOut template_requirements = 3;
+
+        PlusOneOut plus_one = 2046; // temporary, for testing
+
+        BackendError error = 2047;
     }
 }
 
@@ -66,3 +75,15 @@ message TemplateRequirementAll {
 message TemplateRequirementAny {
     repeated uint32 ords = 1;
 }
+
+message SchedTimingTodayIn {
+    int64 created = 1;
+    int64 now = 2;
+    sint32 minutes_west = 3;
+    sint32 rollover_hour = 4;
+}
+
+message SchedTimingTodayOut {
+    uint32 days_elapsed = 1;
+    int64 next_day_at = 2;
+}

rs/Cargo.lock

@@ -14,6 +14,7 @@ name = "ankirs"
 version = "0.1.0"
 dependencies = [
  "bytes",
+ "chrono",
  "failure",
  "nom",
  "prost",
@@ -94,6 +95,17 @@ version = "0.1.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822"
 
+[[package]]
+name = "chrono"
+version = "0.4.10"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "31850b4a4d6bae316f7a09e691c944c28299298837edc0a03f755618c23cbc01"
+dependencies = [
+ "num-integer",
+ "num-traits",
+ "time",
+]
+
 [[package]]
 name = "ctor"
 version = "0.1.12"
@@ -301,6 +313,16 @@ dependencies = [
  "version_check 0.1.5",
 ]
 
+[[package]]
+name = "num-integer"
+version = "0.1.41"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b85e541ef8255f6cf42bbfe4ef361305c6c135d10919ecc26126c4e5ae94bc09"
+dependencies = [
+ "autocfg",
+ "num-traits",
+]
+
 [[package]]
 name = "num-traits"
 version = "0.2.10"
@@ -708,6 +730,17 @@ dependencies = [
  "lazy_static",
 ]
 
+[[package]]
+name = "time"
+version = "0.1.42"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "db8dcfca086c1143c9270ac42a2bbd8a7ee477b78ac8e45b19abfb0cbede4b6f"
+dependencies = [
+ "libc",
+ "redox_syscall",
+ "winapi",
+]
+
 [[package]]
 name = "unicode-segmentation"
 version = "1.6.0"

rs/ankirs/Cargo.toml

@@ -9,6 +9,7 @@ nom = "5.0.1"
 failure = "0.1.6"
 prost = "0.5.0"
 bytes = "0.4"
+chrono = "0.4.10"
 
 [build-dependencies]
 prost-build = "0.5.0"

rs/ankirs/src/backend.rs

@@ -1,6 +1,7 @@
 use crate::backend_proto as pt;
 use crate::backend_proto::backend_input::Value;
 use crate::err::{AnkiError, Result};
+use crate::sched::sched_timing_today;
 use crate::template::{FieldMap, FieldRequirements, ParsedTemplate};
 use prost::Message;
 use std::collections::HashSet;
@@ -85,6 +86,9 @@ impl Backend {
                 OValue::TemplateRequirements(self.template_requirements(input)?)
             }
             Value::PlusOne(input) => OValue::PlusOne(self.plus_one(input)?),
+            Value::SchedTimingToday(input) => {
+                OValue::SchedTimingToday(self.sched_timing_today(input))
+            }
         })
     }
 
@@ -132,6 +136,19 @@ impl Backend {
             requirements: all_reqs,
         })
     }
+
+    fn sched_timing_today(&self, input: pt::SchedTimingTodayIn) -> pt::SchedTimingTodayOut {
+        let today = sched_timing_today(
+            input.created as i64,
+            input.now as i64,
+            input.minutes_west,
+            input.rollover_hour as i8,
+        );
+        pt::SchedTimingTodayOut {
+            days_elapsed: today.days_elapsed,
+            next_day_at: today.next_day_at,
+        }
+    }
 }
 
 fn ords_hash_to_set(ords: HashSet<u16>) -> Vec<u32> {

rs/ankirs/src/lib.rs

@@ -2,4 +2,5 @@ mod backend_proto;
 
 pub mod backend;
 pub mod err;
+pub mod sched;
 pub mod template;

rs/ankirs/src/sched.rs

@@ -0,0 +1,149 @@
+use chrono::{DateTime, FixedOffset, Local, TimeZone, Timelike};
+
+pub struct SchedTimingToday {
+    /// The number of days that have passed since the collection was created.
+    pub days_elapsed: u32,
+    /// Timestamp of the next day rollover.
+    pub next_day_at: i64,
+}
+
+/// Timing information for the current day.
+/// - created is the collection creation time
+/// - now is the current UTC timestamp
+/// - minutes_west is relative to the local timezone (eg UTC+10 hours is -600)
+/// - rollover_hour is the hour of the day the rollover happens (eg 4 for 4am)
+pub fn sched_timing_today(
+    created: i64,
+    now: i64,
+    minutes_west: i32,
+    rollover_hour: i8,
+) -> SchedTimingToday {
+    let rollover_today = rollover_for_today(now, minutes_west, rollover_hour).timestamp();
+
+    SchedTimingToday {
+        days_elapsed: days_elapsed(created, now, rollover_today),
+        next_day_at: rollover_today + 86_400,
+    }
+}
+
+/// Convert timestamp to the local timezone, with the provided rollover hour.
+fn rollover_for_today(
+    timestamp: i64,
+    minutes_west: i32,
+    rollover_hour: i8,
+) -> DateTime<FixedOffset> {
+    let local_offset = fixed_offset_from_minutes(minutes_west);
+    let rollover_hour = normalized_rollover_hour(rollover_hour);
+    local_offset
+        .timestamp(timestamp, 0)
+        .with_hour(rollover_hour as u32)
+        .unwrap()
+}
+
+/// The number of times the day rolled over between two timestamps.
+fn days_elapsed(start: i64, end: i64, rollover_today: i64) -> u32 {
+    // get the number of full days that have elapsed
+    let secs = (end - start).max(0);
+    let days = (secs / 86_400) as u32;
+
+    // minus one if today's cutoff hasn't passed
+    if days > 0 && end < rollover_today {
+        days - 1
+    } else {
+        days
+    }
+}
+
+/// Negative rollover hours are relative to the next day, eg -1 = 23.
+/// Cap hour to 23.
+fn normalized_rollover_hour(hour: i8) -> u8 {
+    let capped_hour = hour.max(-23).min(23);
+    if capped_hour < 0 {
+        (24 + capped_hour) as u8
+    } else {
+        capped_hour as u8
+    }
+}
+
+/// Build a FixedOffset struct, capping minutes_west if out of bounds.
+fn fixed_offset_from_minutes(minutes_west: i32) -> FixedOffset {
+    let bounded_minutes = minutes_west.max(-23 * 60).min(23 * 60);
+    FixedOffset::west(bounded_minutes * 60)
+}
+
+/// Relative to the local timezone, the number of minutes UTC differs by.
+/// eg, Australia at +10 hours is -600.
+/// Includes the daylight savings offset if applicable.
+#[allow(dead_code)]
+fn utc_minus_local_mins() -> i32 {
+    Local::now().offset().utc_minus_local() / 60
+}
+
+#[cfg(test)]
+mod test {
+    use crate::sched::{
+        fixed_offset_from_minutes, normalized_rollover_hour, sched_timing_today,
+        utc_minus_local_mins,
+    };
+    use chrono::{FixedOffset, TimeZone};
+
+    #[test]
+    fn test_rollover() {
+        assert_eq!(normalized_rollover_hour(4), 4);
+        assert_eq!(normalized_rollover_hour(23), 23);
+        assert_eq!(normalized_rollover_hour(24), 23);
+        assert_eq!(normalized_rollover_hour(-1), 23);
+        assert_eq!(normalized_rollover_hour(-2), 22);
+        assert_eq!(normalized_rollover_hour(-23), 1);
+        assert_eq!(normalized_rollover_hour(-24), 1);
+    }
+
+    #[test]
+    fn test_fixed_offset() {
+        let offset = fixed_offset_from_minutes(-600);
+        assert_eq!(offset.utc_minus_local(), -600 * 60);
+    }
+
+    // helper
+    fn elap(start: i64, end: i64, west: i32, rollhour: i8) -> u32 {
+        let today = sched_timing_today(start, end, west, rollhour);
+        today.days_elapsed
+    }
+
+    #[test]
+    fn test_days_elapsed() {
+        let offset = utc_minus_local_mins();
+
+        let created_dt = FixedOffset::west(offset * 60)
+            .ymd(2019, 12, 1)
+            .and_hms(2, 0, 0);
+        let crt = created_dt.timestamp();
+
+        // days can't be negative
+        assert_eq!(elap(crt, crt, offset, 4), 0);
+        assert_eq!(elap(crt, crt - 86_400, offset, 4), 0);
+
+        // 2am the next day is still the same day
+        assert_eq!(elap(crt, crt + 24 * 3600, offset, 4), 0);
+
+        // day rolls over at 4am
+        assert_eq!(elap(crt, crt + 26 * 3600, offset, 4), 1);
+
+        // the longest extra delay is +23, or 19 hours past the 4 hour default
+        assert_eq!(elap(crt, crt + (26 + 18) * 3600, offset, 23), 0);
+        assert_eq!(elap(crt, crt + (26 + 19) * 3600, offset, 23), 1);
+
+        // a collection created @ midnight in MDT in the past
+        let mdt = FixedOffset::west(6 * 60 * 60);
+        let mst = FixedOffset::west(7 * 60 * 60);
+        let crt = mdt.ymd(2018, 8, 6).and_hms(0, 0, 0).timestamp();
+        // with the current time being MST
+        let now = mst.ymd(2019, 12, 26).and_hms(20, 0, 0).timestamp();
+        let offset = mst.utc_minus_local() / 60;
+        assert_eq!(elap(crt, now, offset, 4), 507);
+        // the previous implementation generated a diferent elapsed number of days with a change
+        // to DST, but the number shouldn't change
+        let offset = mdt.utc_minus_local() / 60;
+        assert_eq!(elap(crt, now, offset, 4), 507);
+    }
+}