From 0272dc9d891e8cf5415e747449433de9b1b8bea9 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:27:47 +0400 Subject: [PATCH] Record a suppressed care nudge instead of dropping it silently (#370) Dropping a sev1-2 care nudge while you're away is right and still happens. But it was a bare `continue`: no row, no log, so "she dropped it", "the gate suppressed it" and "the rule never fired" all looked identical afterwards. Adds a 'dropped' delivery status (migration #12 widens the CHECK constraint; sqlite can't do that in place, so the table is rebuilt) and records the drop as one delivery_attempts row plus a log line. No nudges row for a drop: that table feeds the ignored_rate signal, and a nudge nobody could see must not count as ignored. TestVoiceNoSessionFallthroughLeavesOutboxTrail expected exactly one row for sev1-2 when voice had no session. It now expects the voice failure plus the drop, which is the point of the change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/delivery/dispatcher.go | 17 ++++++++++--- internal/delivery/durability_test.go | 10 +++++--- internal/delivery/routing_table_test.go | 5 ++-- internal/store/delivery.go | 11 +++++--- internal/store/delivery_test.go | 34 +++++++++++++++++++++++++ internal/store/migrations.go | 19 ++++++++++++++ 6 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 internal/store/delivery_test.go diff --git a/internal/delivery/dispatcher.go b/internal/delivery/dispatcher.go index d534eb4..085e362 100644 --- a/internal/delivery/dispatcher.go +++ b/internal/delivery/dispatcher.go @@ -137,9 +137,10 @@ func NewDispatcher(cfg Config) *Dispatcher { // picks for (severity, presence), sends via the matching sink, and records // one nudge row per successful send. returns the dispatches (one per channel). // -// a Drop channel = no send, no record (the nudge was suppressed by routing, -// not by a failure — "a missed water nudge is noise"). a nil sink = channel -// not wired, skip silently. a send error stops the dispatch and returns what +// a Drop channel = no send (the nudge was suppressed by routing, not by a +// failure — "a missed water nudge is noise"), but it does leave a 'dropped' +// outbox row so the suppression is visible. a nil sink = channel not wired, +// skip silently. a send error stops the dispatch and returns what // got through — the daemon decides whether to retry. func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now time.Time) ([]Dispatch, error) { c := pn.Candidate @@ -148,6 +149,16 @@ func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now tim for i := 0; i < len(channels); i++ { ch := channels[i] if ch == ChannelDrop { + // the routing table suppressed this nudge on purpose (a care nudge + // while you're away is noise). that stays — but it must not be + // invisible, or "she dropped it" and "the rule never fired" look + // the same afterwards. no nudges row: that table feeds the + // ignored_rate signal, and a nudge nobody could see must not + // count as ignored. + id := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, ch, pn.Summary, now) + d.completeOutbox(ctx, id, store.DeliveryDropped, now) + log.Printf("dispatcher: dropped %s (sev%d, presence=%s) — routing table suppressed it", + c.Rule.Name, c.Severity, c.State.Presence) continue } s := Sendable{ diff --git a/internal/delivery/durability_test.go b/internal/delivery/durability_test.go index 0ef1fc8..4f015e7 100644 --- a/internal/delivery/durability_test.go +++ b/internal/delivery/durability_test.go @@ -37,10 +37,12 @@ func TestVoiceNoSessionFallthroughLeavesOutboxTrail(t *testing.T) { []string{"voice", "ntfy"}, []string{store.DeliveryFailed, store.DeliverySent}}, {"sev4 falls through to telegram", loop.Sev4, []string{"voice", "telegram"}, []string{store.DeliveryFailed, store.DeliverySent}}, - {"sev1 does not fall through", loop.Sev1, - []string{"voice"}, []string{store.DeliveryFailed}}, - {"sev2 does not fall through", loop.Sev2, - []string{"voice"}, []string{store.DeliveryFailed}}, + // care severities still don't reach an away channel; since #370 the + // drop itself is a visible row instead of nothing. + {"sev1 drops instead of falling through", loop.Sev1, + []string{"voice", "drop"}, []string{store.DeliveryFailed, store.DeliveryDropped}}, + {"sev2 drops instead of falling through", loop.Sev2, + []string{"voice", "drop"}, []string{store.DeliveryFailed, store.DeliveryDropped}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/internal/delivery/routing_table_test.go b/internal/delivery/routing_table_test.go index f7c9129..a008880 100644 --- a/internal/delivery/routing_table_test.go +++ b/internal/delivery/routing_table_test.go @@ -208,10 +208,9 @@ func TestAwayChannelsGetMinimalBody(t *testing.T) { // TestCareAwayDropIsRecorded — DESIGN.md's drop is a decision ("a missed water // nudge is noise, a missed backup failure isn't"), so it should be visible // rather than vanish. Today drop is a bare `continue`: no nudge row, no outbox -// attempt, no log — nothing an operator can see afterwards. +// attempt, no log — nothing an operator can see afterwards. now it leaves a +// 'dropped' outbox row. func TestCareAwayDropIsRecorded(t *testing.T) { - t.Skip("not implemented: dispatcher.go:149-151 skips a Drop channel with no record; there is no 'dropped' outcome in store/delivery.go:16-21") - ob := &fakeOutbox{} d := NewDispatcher(Config{Voice: &fakeSink{}, Nudges: &fakeNudgeRecorder{}, Outbox: ob}) diff --git a/internal/store/delivery.go b/internal/store/delivery.go index 8c37b14..b0b46df 100644 --- a/internal/store/delivery.go +++ b/internal/store/delivery.go @@ -13,11 +13,15 @@ import ( // unknown = a pending row found stale at startup: the process that started it // is gone, and the send may or may not have reached the external channel. // Never auto-resolved into sent or failed — that would be guessing. +// dropped = the routing table deliberately suppressed this one (a care nudge +// while you're away). Nothing was sent and nothing went wrong; the row exists +// so "she dropped it" and "the rule never fired" don't look the same later. const ( DeliveryPending = "pending" DeliverySent = "sent" DeliveryFailed = "failed" DeliveryUnknown = "unknown" + DeliveryDropped = "dropped" ) // BeginDeliveryAttempt durably records intent to send BEFORE the external @@ -43,10 +47,11 @@ func (s *Store) BeginDeliveryAttempt(ctx context.Context, kind, rule string, rem } // CompleteDeliveryAttempt records the sink's outcome for a prior -// BeginDeliveryAttempt. status is "sent" or "failed" — never "pending" or -// "unknown" (those are set only by Begin and reconciliation respectively). +// BeginDeliveryAttempt. status is "sent", "failed" or "dropped" — never +// "pending" or "unknown" (those are set only by Begin and reconciliation +// respectively). func (s *Store) CompleteDeliveryAttempt(ctx context.Context, id int64, status string, now time.Time) error { - if status != DeliverySent && status != DeliveryFailed { + if status != DeliverySent && status != DeliveryFailed && status != DeliveryDropped { return fmt.Errorf("store: invalid delivery completion status %q", status) } _, err := s.db.ExecContext(ctx, diff --git a/internal/store/delivery_test.go b/internal/store/delivery_test.go new file mode 100644 index 0000000..4a7e23d --- /dev/null +++ b/internal/store/delivery_test.go @@ -0,0 +1,34 @@ +package store + +import ( + "context" + "testing" + "time" +) + +// TestDroppedDeliveryAttemptRoundTrips — Vikunja #370. A suppressed nudge is +// recorded as 'dropped'. The status column has a CHECK constraint, so this +// only works if migration #12 widened it; a fake outbox in a unit test would +// not catch that. +func TestDroppedDeliveryAttemptRoundTrips(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + + id, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "drop", "abc123", now) + if err != nil { + t.Fatalf("BeginDeliveryAttempt: %v", err) + } + if err := s.CompleteDeliveryAttempt(ctx, id, DeliveryDropped, now); err != nil { + t.Fatalf("CompleteDeliveryAttempt: %v", err) + } + + var status string + err = s.db.QueryRowContext(ctx, `SELECT status FROM delivery_attempts WHERE id = ?`, id).Scan(&status) + if err != nil { + t.Fatalf("read back: %v", err) + } + if status != DeliveryDropped { + t.Fatalf("status: want %q, got %q", DeliveryDropped, status) + } +} diff --git a/internal/store/migrations.go b/internal/store/migrations.go index b9b7ff0..e67bd43 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -88,6 +88,25 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 key TEXT PRIMARY KEY, value TEXT NOT NULL );`, // #11 — small key/value table for facts about the DB itself; first key is embedder_id (Vikunja #378) + + // #12 — a suppressed nudge gets a 'dropped' row (Vikunja #370). sqlite + // can't widen a CHECK constraint in place, so the table is rebuilt; the + // index goes with the old table and is recreated. + `CREATE TABLE delivery_attempts_v12 ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + kind TEXT NOT NULL CHECK (kind IN ('nudge','reminder')), + rule TEXT NOT NULL DEFAULT '', + reminder_id INTEGER NOT NULL DEFAULT 0, + channel TEXT NOT NULL, + body_hash TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending','sent','failed','unknown','dropped')), + created_ts INTEGER NOT NULL, + completed_ts INTEGER + ); + INSERT INTO delivery_attempts_v12 SELECT * FROM delivery_attempts; + DROP TABLE delivery_attempts; + ALTER TABLE delivery_attempts_v12 RENAME TO delivery_attempts; + CREATE INDEX IF NOT EXISTS idx_delivery_attempts_status ON delivery_attempts (status);`, } // migrate applies every migration with a number greater than the DB's current