From 13cb1903a9251897bb969caa66da8f33b516c1bc Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 04:36:36 +0400 Subject: [PATCH] recurring reminders keep their wall-clock hour and survive downtime (V-616) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RescheduleReminder walked the cron on the UTC instant scanReminder returns, so a daily 09:00 Moscow reminder rescheduled to 09:00 UTC — noon the same day, and noon every day after. And any occurrence earlier than now marked the reminder fired, so a daemon down overnight ended the recurrence for good. The walk now runs in the owner's location and skips past occurrences instead of killing the reminder. Skipping and not replaying keeps the no-backlog rule routine.DueAccepted already follows. --- internal/store/reminders.go | 34 ++++++- internal/store/reminders_reschedule_test.go | 104 ++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 internal/store/reminders_reschedule_test.go diff --git a/internal/store/reminders.go b/internal/store/reminders.go index fefa7bb..f7b67df 100644 --- a/internal/store/reminders.go +++ b/internal/store/reminders.go @@ -205,8 +205,30 @@ func (s *Store) ListReminders(ctx context.Context, n int) ([]Reminder, error) { // RescheduleReminder computes the next fire time for a recurring reminder and // updates next_fire_ts. Returns ErrReminderState if the reminder is not -// recurring or not pending. If no more valid fire times exist, marks it fired. +// recurring or not pending. If the schedule yields no further fire time at all, +// marks it fired. +// +// Two things the first version got wrong, both fixed 06-08-2026 (V-616). +// +// The cron expression is a WALL CLOCK statement — "0 9 * * *" is nine in the +// morning where the owner stands — but scanReminder hands back instants in UTC, +// and robfig's Next walks the calendar in the location of the time it is given. +// Computing from a UTC instant therefore produced the next 09:00 UTC, so the +// second occurrence of a daily reminder landed one UTC offset late and stayed +// there: 12:00 for a Moscow owner. Everything is converted to loc first, which +// also makes the walk DST-correct — the schedule keeps its wall-clock hour +// across a changeover instead of drifting an hour with the offset. +// +// And a missed occurrence used to KILL the reminder: any next fire earlier than +// now marked it fired, so a daemon down overnight ended a daily standup forever. +// Occurrences in the past are skipped instead, so the reminder rolls forward to +// the first one strictly after now. Skipping and not replaying is deliberate: +// the same no-backlog rule routine.DueAccepted follows. func (s *Store) RescheduleReminder(ctx context.Context, id int64, now time.Time) error { + return s.rescheduleReminderIn(ctx, id, now, time.Local) +} + +func (s *Store) rescheduleReminderIn(ctx context.Context, id int64, now time.Time, loc *time.Location) error { row := s.db.QueryRowContext(ctx, ` SELECT id, created_ts, fire_ts, next_fire_ts, payload, status, cron FROM reminders WHERE id = ?`, id) @@ -225,8 +247,14 @@ func (s *Store) RescheduleReminder(ctx context.Context, id int64, now time.Time) if err != nil { return fmt.Errorf("parse cron %q: %w", r.Cron, err) } - next := sched.Next(r.NextFireTs.Add(time.Minute)) - if next.IsZero() || next.Before(now) { + // Next is strictly after the time it is given, so the last fire cannot be + // returned again and no fudge minute is needed. The bound stops a schedule + // that somehow yields a non-advancing time from spinning here. + next := sched.Next(r.NextFireTs.In(loc)) + for i := 0; i < 4096 && !next.IsZero() && !next.After(now); i++ { + next = sched.Next(next) + } + if next.IsZero() || !next.After(now) { _, err = s.db.ExecContext(ctx, "UPDATE reminders SET status = 'fired' WHERE id = ?", id) return err } diff --git a/internal/store/reminders_reschedule_test.go b/internal/store/reminders_reschedule_test.go new file mode 100644 index 0000000..89a2af7 --- /dev/null +++ b/internal/store/reminders_reschedule_test.go @@ -0,0 +1,104 @@ +package store + +import ( + "context" + "testing" + "time" +) + +// A recurring reminder keeps its wall-clock hour in the owner's zone. The cron +// walk used to run on the UTC instant scanReminder returns, so "0 9 * * *" +// created for 09:00 Moscow rescheduled to 09:00 UTC — noon, and noon every day +// after that (V-616). +func TestRescheduleKeepsWallClockHour(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + msk := time.FixedZone("MSK", 3*60*60) + + fire := time.Date(2026, 7, 1, 9, 0, 0, 0, msk) + id, err := s.CreateReminder(ctx, fire, `{"text":"стендап"}`, "0 9 * * *") + if err != nil { + t.Fatal(err) + } + if err := s.rescheduleReminderIn(ctx, id, fire, msk); err != nil { + t.Fatalf("reschedule: %v", err) + } + + want := time.Date(2026, 7, 2, 9, 0, 0, 0, msk) + got := nextFire(t, s, id) + if !got.Equal(want) { + t.Fatalf("next fire: want %s, got %s", want, got.In(msk)) + } +} + +// A daily reminder survives a daemon that was down for days. The past +// occurrences are skipped, not replayed, and the reminder stays pending. +func TestRescheduleRollsForwardAfterDowntime(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + msk := time.FixedZone("MSK", 3*60*60) + + fire := time.Date(2026, 7, 1, 9, 0, 0, 0, msk) + id, err := s.CreateReminder(ctx, fire, `{"text":"стендап"}`, "0 9 * * *") + if err != nil { + t.Fatal(err) + } + // The box comes back three days later, mid-afternoon. + now := time.Date(2026, 7, 4, 15, 0, 0, 0, msk) + if err := s.rescheduleReminderIn(ctx, id, now, msk); err != nil { + t.Fatalf("reschedule: %v", err) + } + + rs, err := s.ListReminders(ctx, 10) + if err != nil || len(rs) != 1 { + t.Fatalf("list: %d reminders, %v", len(rs), err) + } + if rs[0].Status != ReminderPending { + t.Fatalf("status: want %s, got %s — downtime killed the recurrence", ReminderPending, rs[0].Status) + } + want := time.Date(2026, 7, 5, 9, 0, 0, 0, msk) + if got := nextFire(t, s, id); !got.Equal(want) { + t.Fatalf("next fire: want %s, got %s", want, got.In(msk)) + } + // And exactly one fire is owed, not a backlog of four. + due, err := s.DueReminders(ctx, want.Add(time.Second)) + if err != nil || len(due) != 1 { + t.Fatalf("due after roll-forward: want 1, got %d (%v)", len(due), err) + } +} + +// A daily 03:00 reminder does not drift across the spring-forward changeover: +// the hour is wall clock, so the interval is 23 hours that day, not 24. +func TestRescheduleAcrossDSTKeepsHour(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + berlin, err := time.LoadLocation("Europe/Berlin") + if err != nil { + t.Skipf("tzdata unavailable: %v", err) + } + + fire := time.Date(2027, 3, 27, 3, 0, 0, 0, berlin) // CET, day before the change + id, err := s.CreateReminder(ctx, fire, `{"text":"бэкап"}`, "0 3 * * *") + if err != nil { + t.Fatal(err) + } + if err := s.rescheduleReminderIn(ctx, id, fire, berlin); err != nil { + t.Fatalf("reschedule: %v", err) + } + got := nextFire(t, s, id).In(berlin) + if got.Hour() != 3 || got.Day() != 28 { + t.Fatalf("next fire: want 2027-03-28 03:00 local, got %s", got) + } + if d := got.Sub(fire); d != 23*time.Hour { + t.Fatalf("gap across spring forward: want 23h, got %s", d) + } +} + +func nextFire(t *testing.T, s *Store, id int64) time.Time { + t.Helper() + var ms int64 + if err := s.db.QueryRow("SELECT next_fire_ts FROM reminders WHERE id = ?", id).Scan(&ms); err != nil { + t.Fatalf("read next_fire_ts: %v", err) + } + return time.UnixMilli(ms).UTC() +}