Merge the recurrence sweep: a daily reminder drifted to noon (#257)
RescheduleReminder walked the cron schedule on a UTC instant, and robfig's Next walks the calendar in the location it is handed. So '0 9 * * *' created for 09:00 Moscow rescheduled to the next 09:00 UTC, which is noon the same day: the reminder fired again that afternoon and every day at noon after. The same offset walk moved it an hour across a DST changeover. The walk runs in the owner's location now. Worse and quieter: any outage longer than one period killed the recurrence for good. next is the occurrence after the last fire, so next.Before(now) marked a daily reminder fired when the daemon was down overnight. Past occurrences roll forward to the first one after now, with no backlog replay, matching routine.DueAccepted. internal/routine is clean. Its IntervalDays*24h is an elapsed measure rather than a wall clock, so the hour arithmetic is right there. (V-616)
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
Reference in New Issue
Block a user