From a820a95ebb307d4ae3e2c1e14c061aa40c7c9b01 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 4 Aug 2026 03:29:51 +0400 Subject: [PATCH] store: wake the routines accepted before the fire-forever fix (V-377) Routines accepted before Vikunja #366 carry accepted_ts NULL and a live reminder row. The tick loop reads accepted_ts to decide when a routine is next due, so those rows have been silent since the fix landed, while the reminder they still point at keeps firing on its own schedule. Migration #19 cancels that reminder first, then dates the acceptance from created_ts and lets the reminder id go. Order matters: the second update clears the id the first one needs. --- internal/store/migrations.go | 29 +++++++++++++ internal/store/migrations_test.go | 67 +++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/internal/store/migrations.go b/internal/store/migrations.go index 8a94f64..5478090 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -219,6 +219,35 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 // event, and the old rows would otherwise be recited as extra meetings. // The filter is exact — it keeps any key whose summary part still has a // letter or a digit in it. + + // #19 — unstick the routines accepted before the fire-forever fix + // (Vikunja #377, follow-up to #366). Accepting used to leave accepted_ts + // NULL and a live one-shot reminder behind, and the tick loop skips a row + // with no accepted_ts, so every non-weekly routine accepted before that fix + // has been silent ever since. + // + // Three statements, in this order, per stuck row: adopt created_ts as the + // acceptance time, cancel the reminder that is still holding the schedule, + // then let go of it. Cancelling before clearing matters — clearing first + // loses the only pointer to the reminder and leaves it to fire on its own. + // + // created_ts rather than a fresh timestamp because a migration has no + // clock, and because the first interval should be measured from when he + // said yes. A routine whose interval has already elapsed nudges on the next + // tick, which is what being unstuck looks like. + // + // Weekly rows are included deliberately. Theirs was the case that kept + // working, because the cron reminder reschedules itself — so leaving them + // alone would give them both a cron reminder and a tick-loop schedule for + // one habit, and he would hear it twice. + `UPDATE reminders + SET status = 'cancelled' + WHERE status = 'pending' + AND id IN (SELECT reminder_id FROM proposed_routines + WHERE status = 'accepted' AND accepted_ts IS NULL AND reminder_id IS NOT NULL); + UPDATE proposed_routines + SET accepted_ts = created_ts, reminder_id = NULL + WHERE status = 'accepted' AND accepted_ts IS NULL;`, } // migrate applies every migration with a number greater than the DB's current diff --git a/internal/store/migrations_test.go b/internal/store/migrations_test.go index e0b2663..7aa80ec 100644 --- a/internal/store/migrations_test.go +++ b/internal/store/migrations_test.go @@ -3,6 +3,7 @@ package store import ( "context" "testing" + "time" ) func userVersion(t *testing.T, s *Store) int { @@ -80,3 +81,69 @@ func TestCollapsedCalendarKeysAreDropped(t *testing.T) { t.Fatalf("%d calendar rows left, want the 2 that identify their event", got) } } + +// TestStuckRoutinesAreBackfilled — routines accepted before the fire-forever +// fix have accepted_ts NULL and a live reminder, so the tick loop skips them +// and they have been silent ever since (Vikunja #377). The migration touches +// live reminders, which is why it is tested against a real store. +func TestStuckRoutinesAreBackfilled(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + created := time.Date(2026, 7, 1, 9, 0, 0, 0, time.UTC) + + rem, err := s.CreateReminder(ctx, created.Add(time.Hour), "полить цветы", "") + if err != nil { + t.Fatal(err) + } + healthy, err := s.CreateReminder(ctx, created.Add(2*time.Hour), "не трогать", "") + if err != nil { + t.Fatal(err) + } + if _, err := s.db.ExecContext(ctx, + `INSERT INTO proposed_routines (action, object, interval_days, status, created_ts, reminder_id, accepted_ts) + VALUES ('water', 'plants', 7, 'accepted', ?, ?, NULL)`, + created.UnixMilli(), rem); err != nil { + t.Fatal(err) + } + // An already-healthy accepted row, and a still-open proposal: neither is + // this migration's business. + if _, err := s.db.ExecContext(ctx, + `INSERT INTO proposed_routines (action, object, interval_days, status, created_ts, accepted_ts) + VALUES ('feed', 'cat', 1, 'accepted', ?, ?)`, + created.UnixMilli(), created.UnixMilli()); err != nil { + t.Fatal(err) + } + + if _, err := s.db.ExecContext(ctx, migrations[18]); err != nil { + t.Fatalf("migration 19: %v", err) + } + + accepted, err := s.ListAcceptedRoutines(ctx) + if err != nil || len(accepted) != 2 { + t.Fatalf("ListAcceptedRoutines = %d rows, err=%v, want 2", len(accepted), err) + } + stuck := accepted[0] + if stuck.Object != "plants" { + stuck = accepted[1] + } + if stuck.AcceptedTs == nil || !stuck.AcceptedTs.Equal(created) { + t.Fatalf("accepted_ts = %v, want the creation time", stuck.AcceptedTs) + } + if stuck.ReminderID != nil { + t.Fatalf("reminder_id = %v, want it let go", stuck.ReminderID) + } + // The reminder it was holding is cancelled, and nothing else is. + var status string + if err := s.db.QueryRowContext(ctx, `SELECT status FROM reminders WHERE id = ?`, rem).Scan(&status); err != nil { + t.Fatal(err) + } + if status != ReminderCancelled { + t.Fatalf("linked reminder status = %q, want cancelled", status) + } + if err := s.db.QueryRowContext(ctx, `SELECT status FROM reminders WHERE id = ?`, healthy).Scan(&status); err != nil { + t.Fatal(err) + } + if status != "pending" { + t.Fatalf("unrelated reminder status = %q, want it untouched", status) + } +}