From ca2130258b22486bdf65af17fe8a06d15e1bde0e Mon Sep 17 00:00:00 2001 From: Admin <1+admin@noreply.git.dcntrlized.org> Date: Mon, 19 Oct 2026 19:34:24 +0100 Subject: [PATCH] [fix](alarm): make dismissal stick and count the devices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects, three of them the same missing piece of the delivery ledger. - listSubscriptions scanned nine destinations for ten columns and read a NULL subscriber_user_id into a uuid. Every alert with an anonymous subscriber answered 500 ("number of field descriptions must equal number of destinations, got 10 and 9"), and the client renders a failed read as an empty list — so 'Subscribed devices' said 0 while the row existed. The listing now uses the same scan as every other subscription read. - alarmStatus reported the alert's commanded state, not the caller's own. An alert stays 'triggered' until the owner disarms or every subscriber acks, so a device that had already dismissed the alarm was told active:true on every poll and re-raised its takeover. active is now true only while the caller still owes the newest trigger; the owner keeps seeing their own alert. - ackAlarm required an eventId, which the native takeover launch does not have. A dismissal that cannot name its event now clears the subscription's whole outstanding set (AckAllDeliveries / AckAllForSubscriber), so the debt cannot survive because the client could not echo a uuid. - The device ledger probe (gated on DATABASE_URL, like the lifecycle probe) pins the per-subscriber debt, the ack-all semantics and the cascade that takes a device token with the destroyed alert. Verified against the dev database and through Maestro: 2 subscriptions listed, device A acked -> its status turns active:false while device B still gets the 409 check-in, ack-all without eventId clears the debt and drains the alert. --- API.md | 28 ++++++- src/actions/ack.go | 49 ++++++++---- src/actions/events.go | 31 +++++++- src/db/deliveries.go | 79 +++++++++++++++++++ src/db/device_ledger_db_test.go | 133 ++++++++++++++++++++++++++++++++ src/db/subscriptions.go | 11 ++- 6 files changed, 305 insertions(+), 26 deletions(-) create mode 100644 src/db/device_ledger_db_test.go diff --git a/API.md b/API.md index 54d3e27..5703264 100644 --- a/API.md +++ b/API.md @@ -587,9 +587,18 @@ read; send it in the query if you want to pin the subscription) ``` `active` is **derived**, never a ringing timer: it is true only when the newest -event is a `trigger` and the stored status is `triggered`. The answer stays -correct across restarts and instances, and it stays `true` until the owner -disarms or the alert expires. (There *is* a sweeper, but it destroys expired +event is a `trigger`, the stored status is `triggered`, **and this caller still +owes the alarm** (an unacknowledged delivery row for its subscription). The +answer stays correct across restarts and instances, and it stays `true` until +the owner disarms, the alert expires, or this caller acknowledges. + +The caller-scoped `active` is what makes dismissal final: an alert stays +commanded `triggered` until the owner disarms it or *every* subscriber ack, so a +device that already dismissed the alarm would otherwise be told `active: true` +on every poll and re-raise its takeover. A client must treat `active: false` +with a stale `eventId` as "nothing owed by this device", not as a new trigger. +The alert's owner (who has no delivery row) keeps seeing their own alert's +commanded state. (There *is* a sweeper, but it destroys expired alerts — it never changes an event's meaning.) An expired or destroyed alert is `ERR_ALERT_NOT_FOUND` here like everywhere else. When there is no event yet the fields are empty and `active` is `false`. @@ -663,12 +672,15 @@ device rides the payload (or the query string), exactly like `joinAlert`: ```json { "payload": { "alertId": "…", "eventId": "…", "deviceId": "pixel-7" } } +{ "payload": { "alertId": "…", "eventId": "…" } } // device token in X-Device-Token +{ "payload": { "alertId": "…" } } // dismiss everything owed ``` Response `200`: ```json { "alertId": "…", "eventId": "…", "acked": 1, "destroyed": false } +// with no eventId: "eventId": "" and acked counts every row that was cleared ``` - Clearing is **ack-driven**, never response-driven: `alarmStatus` only *reports* @@ -678,7 +690,15 @@ Response `200`: `acked: 0`, never an error. The WebSocket `ack` frame settles the same rows. - `acked` counts the ledger rows cleared for this device. When the ack clears the last unacked row of the alert, the alert drains and is destroyed (reason - `delivered`) — `destroyed: true` reports that intended outcome. + `delivered`) — `destroyed: true` reports that intended outcome. Note that a + destroyed alert takes its subscriptions with it, so a device token for it stops + resolving (the next call is `401 ERR_UNAUTHORIZED`, not `404`); clients must + treat that as "the subscription is over", not as a retryable failure. +- **`eventId` is optional.** Without it the dismissal retires the caller's whole + outstanding set on the alert. That exists because the native takeover launch + carries only the alert: a client that dismissed an alarm without being able to + name its event must still be able to clear the debt, or the replay loop it + could not describe keeps ringing forever. - The alert owner has no delivery row, so an owner ack is a well-formed no-op. - Subscribers must pass `deviceId` (raw; the server hashes it). A device can only clear its own row. diff --git a/src/actions/ack.go b/src/actions/ack.go index 6ea91cf..5001351 100644 --- a/src/actions/ack.go +++ b/src/actions/ack.go @@ -63,15 +63,19 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) { validationError(w, "%v", err) return } - eventRaw := fieldOrQuery(r, payload, "eventId") - if eventRaw == "" { - validationError(w, "eventId is required") - return - } - eventID, err := uuid.Parse(eventRaw) - if err != nil { - validationError(w, "eventId is not a valid uuid") - return + // eventId is optional on purpose. The native takeover launch carries only the + // alert, so a dismissal from that path has no event to name; "I have heard + // this device's alarm" is then answered by clearing the caller's whole + // outstanding set (see db.AckAllDeliveries). A stale debt that nobody can + // name must not replay forever because the client could not echo a uuid. + var eventID uuid.UUID + if eventRaw := fieldOrQuery(r, payload, "eventId"); eventRaw != "" { + parsed, err := uuid.Parse(eventRaw) + if err != nil { + validationError(w, "eventId is not a valid uuid") + return + } + eventID = parsed } alert, err := db.GetAlert(r.Context(), alertID) @@ -96,12 +100,21 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) { } } - resp := ackAlarmResponse{AlertID: alertID.String(), EventID: eventID.String()} + resp := ackAlarmResponse{AlertID: alertID.String()} + if eventID != uuid.Nil { + resp.EventID = eventID.String() + } // A device token lives on the subscription, so the ledger row is addressed // directly: no user id, no device-id echo, nothing to spoof. if c.IsDevice { - cleared, err := db.AckDelivery(r.Context(), alertID, c.Sub.ID, eventID) + var cleared int64 + var err error + if eventID == uuid.Nil { + cleared, err = db.AckAllDeliveries(r.Context(), alertID, c.Sub.ID) + } else { + cleared, err = db.AckDelivery(r.Context(), alertID, c.Sub.ID, eventID) + } if err != nil { dbError(w, action, err) return @@ -119,7 +132,8 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) { } resp.Destroyed = destroyed } - Debug(action, "POST", "alert=%s event=%s acked=%d destroyed=%t (device)", alertID, eventID, resp.Acked, resp.Destroyed) + Debug(action, "POST", "alert=%s event=%s all=%t acked=%d destroyed=%t (device)", + alertID, resp.EventID, eventID == uuid.Nil, resp.Acked, resp.Destroyed) writeJSON(w, http.StatusOK, resp) return } @@ -132,7 +146,13 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) { validationError(w, "%v", err) return } - cleared, err := db.AckAlarmDelivery(r.Context(), alertID, c.UserID, deviceid.Hash(raw), eventID) + deviceHash := deviceid.Hash(raw) + var cleared int64 + if eventID == uuid.Nil { + cleared, err = db.AckAllForSubscriber(r.Context(), alertID, c.UserID, deviceHash) + } else { + cleared, err = db.AckAlarmDelivery(r.Context(), alertID, c.UserID, deviceHash, eventID) + } if err != nil { dbError(w, action, err) return @@ -152,7 +172,8 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) { } } - Debug(action, "POST", "alert=%s event=%s acked=%d destroyed=%t", alertID, eventID, resp.Acked, resp.Destroyed) + Debug(action, "POST", "alert=%s event=%s all=%t acked=%d destroyed=%t", + alertID, resp.EventID, eventID == uuid.Nil, resp.Acked, resp.Destroyed) writeJSON(w, http.StatusOK, resp) } diff --git a/src/actions/events.go b/src/actions/events.go index 8f108e4..83b95a8 100644 --- a/src/actions/events.go +++ b/src/actions/events.go @@ -225,6 +225,12 @@ func AlarmStatus(w http.ResponseWriter, r *http.Request, payload any) { // 409 lost in transit must not forfeit the alarm: the next check-in has to // find the same row. A trigger that has since been disarmed is not news, so // that case falls through to the normal body (active:false). + deviceHash := "" + if !c.IsDevice { + if raw := strings.TrimSpace(r.URL.Query().Get("deviceId")); raw != "" { + deviceHash = deviceid.Hash(raw) + } + } var pending *db.PendingDelivery if c.IsDevice { // The device token already identifies the subscription, so its whole @@ -238,10 +244,6 @@ func AlarmStatus(w http.ResponseWriter, r *http.Request, payload any) { pending = &rows[0] } } else { - deviceHash := "" - if raw := strings.TrimSpace(r.URL.Query().Get("deviceId")); raw != "" { - deviceHash = deviceid.Hash(raw) - } var err error pending, err = db.PendingTrigger(r.Context(), alertID, c.UserID, deviceHash) if err != nil && !errors.Is(err, db.ErrNotFound) { @@ -261,6 +263,27 @@ func AlarmStatus(w http.ResponseWriter, r *http.Request, payload any) { // not live even if the stored status says "triggered". active = false } + // "Active" is a statement about THIS caller, not about the alert's commanded + // status. The alert stays triggered until the owner disarms it or every + // subscriber acknowledges, so answering a device that already dismissed the + // alarm with active:true makes a correct client re-raise the takeover on + // every poll — the alarm that will not stay dismissed. A subscriber is + // active only while it still owes the newest trigger; the owner (who has no + // delivery row) keeps seeing their own alert as live. + callerIsOwner := !c.IsDevice && res.Alert.AdminUserID == c.UserID + if active && !callerIsOwner { + var owed bool + if c.IsDevice { + owed, err = db.HasPendingDelivery(r.Context(), alertID, c.Sub.ID) + } else { + owed, err = db.HasPendingDeliveryForUser(r.Context(), alertID, c.UserID, deviceHash) + } + if err != nil { + dbError(w, action, err) + return + } + active = owed + } view := statusView{ Active: active, diff --git a/src/db/deliveries.go b/src/db/deliveries.go index 56663f4..64842cb 100644 --- a/src/db/deliveries.go +++ b/src/db/deliveries.go @@ -166,3 +166,82 @@ func AckAlarmDelivery(ctx context.Context, alertID, userID uuid.UUID, deviceID s } return tag.RowsAffected(), nil } + +// HasPendingDelivery reports whether a subscription still owes an alarm — the +// question `alarmStatus` has to ask before it calls an alert "active". +// +// An alert stays commanded "triggered" until the owner disarms it or every +// subscriber acknowledges, so the commanded status alone is not the caller's +// answer: a device that already dismissed the alarm must be told it owes +// nothing, or it re-raises the same takeover forever. +func HasPendingDelivery(ctx context.Context, alertID, subscriptionID uuid.UUID) (bool, error) { + var owed bool + err := Pool().QueryRow(ctx, + `SELECT EXISTS ( + SELECT 1 FROM alarm.alarm_deliveries + WHERE alert_id = $1 AND subscription_id = $2 AND acked_at IS NULL)`, + alertID, subscriptionID).Scan(&owed) + if err != nil { + return false, err + } + return owed, nil +} + +// HasPendingDeliveryForUser is the account-path equivalent: the caller's debt is +// addressed by (alert, user, device) exactly like the socket and the ack, with +// an empty device hash meaning "any of this user's subscriptions on the alert". +func HasPendingDeliveryForUser(ctx context.Context, alertID, userID uuid.UUID, deviceHash string) (bool, error) { + var owed bool + err := Pool().QueryRow(ctx, + `SELECT EXISTS ( + SELECT 1 + FROM alarm.alarm_deliveries d + JOIN alarm.subscriptions s ON s.id = d.subscription_id + WHERE d.alert_id = $1 + AND d.acked_at IS NULL + AND s.subscriber_user_id = $2 + AND ($3 = '' OR s.device_id = $3))`, + alertID, userID, deviceHash).Scan(&owed) + if err != nil { + return false, err + } + return owed, nil +} + +// AckAllDeliveries clears every alarm this subscription still owes, and reports +// how many rows it retired. It is the dismissal that does not know an event id — +// a native takeover launch carries only the alert, and re-deriving "which +// trigger was I ringing for" from the phone is guesswork. Acknowledging means +// "this device has heard the alarm", so clearing the whole outstanding set is +// the honest reading and the one that cannot leave a stale debt replaying. +func AckAllDeliveries(ctx context.Context, alertID, subscriptionID uuid.UUID) (int64, error) { + tag, err := Pool().Exec(ctx, + `UPDATE alarm.alarm_deliveries + SET acked_at = now() + WHERE alert_id = $1 AND subscription_id = $2 AND acked_at IS NULL`, + alertID, subscriptionID) + if err != nil { + return 0, err + } + return tag.RowsAffected(), nil +} + +// AckAllForSubscriber is AckAllDeliveries for the account path, addressed by +// (alert, user, device) so a caller can only ever clear its own debt. +func AckAllForSubscriber(ctx context.Context, alertID, userID uuid.UUID, deviceHash string) (int64, error) { + tag, err := Pool().Exec(ctx, + `UPDATE alarm.alarm_deliveries d + SET acked_at = now() + WHERE d.alert_id = $1 + AND d.acked_at IS NULL + AND d.subscription_id IN ( + SELECT s.id FROM alarm.subscriptions s + WHERE s.alert_id = $1 + AND s.subscriber_user_id = $2 + AND ($3 = '' OR s.device_id = $3))`, + alertID, userID, deviceHash) + if err != nil { + return 0, err + } + return tag.RowsAffected(), nil +} diff --git a/src/db/device_ledger_db_test.go b/src/db/device_ledger_db_test.go new file mode 100644 index 0000000..a60b5f1 --- /dev/null +++ b/src/db/device_ledger_db_test.go @@ -0,0 +1,133 @@ +// Copyright (C) 2026 Dcntrlized +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published +// by the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package db + +// The device-ledger probe: what an anonymous subscriber's dismissal does to the +// delivery ledger. Skipped unless DATABASE_URL points at a migrated database +// (same gate as TestLedgerProbe): +// +// DATABASE_URL='postgres://postgres:…@127.0.0.1:55432/dcntrlized?sslmode=disable' \ +// go test -run TestDeviceLedgerProbe -count=1 -v ./db/ +// +// It proves the three queries `alarmStatus` and `ackAlarm` now depend on: +// - a device subscription with no user id still owes (and clears) alarms; +// - an "ack all" dismissal retires the whole outstanding set, which is how a +// native takeover launch (no event id) stops a replay loop; +// - "does this caller owe the alarm?" answers per subscriber, so one device +// dismissing does not silence another device's owed alarm. + +import ( + "context" + "os" + "strings" + "testing" + "time" + + "github.com/google/uuid" +) + +func TestDeviceLedgerProbe(t *testing.T) { + dsn := os.Getenv("DATABASE_URL") + if dsn == "" { + t.Skip("DATABASE_URL not set; device ledger probe needs a migrated database") + } + ctx := context.Background() + NewDB() + t.Cleanup(Close) + + admin := uuid.New() + account := uuid.New() + msg := "probe" + a, err := CreateAlert(ctx, &Alert{ + AdminUserID: admin, + Name: "probe device ledger", + Message: &msg, + AdminTokenHash: []byte("probe"), + SubscribeToken: "probe-" + uuid.NewString(), + ExpiresAt: time.Now().UTC().Add(time.Hour), + }) + if err != nil { + t.Fatalf("CreateAlert: %v", err) + } + t.Cleanup(func() { + _, _ = Pool().Exec(context.Background(), `DELETE FROM alarm.alerts WHERE id = $1`, a.ID) + }) + + tokenHash := strings.Repeat("c", 64) + device, err := CreateSubscription(ctx, &Subscription{ + AlertID: a.ID, + DeviceID: "sha256:" + strings.Repeat("d", 64), + DeviceTokenHash: &tokenHash, + }) + if err != nil { + t.Fatalf("CreateSubscription (device): %v", err) + } + if _, err := CreateSubscription(ctx, &Subscription{ + AlertID: a.ID, SubscriberUserID: account, DeviceID: "sha256:" + strings.Repeat("e", 64), + }); err != nil { + t.Fatalf("CreateSubscription (account): %v", err) + } + + _, event, err := TriggerAlert(ctx, a.ID, admin, nil) + if err != nil { + t.Fatalf("TriggerAlert: %v", err) + } + if ok, err := HasPendingDelivery(ctx, a.ID, device.ID); err != nil || !ok { + t.Fatalf("device owes after trigger: %v %v", ok, err) + } + if ok, err := HasPendingDeliveryForUser(ctx, a.ID, account, ""); err != nil || !ok { + t.Fatalf("account owes after trigger: %v %v", ok, err) + } + + // The device dismissal that names no event clears its whole set; a repeat + // (or a named ack afterwards) is an idempotent no-op. + cleared, err := AckAllDeliveries(ctx, a.ID, device.ID) + if err != nil || cleared != 1 { + t.Fatalf("AckAllDeliveries = %d, %v; want 1", cleared, err) + } + if again, err := AckDelivery(ctx, a.ID, device.ID, event.ID); err != nil || again != 0 { + t.Fatalf("AckDelivery after ack-all = %d, %v; want 0", again, err) + } + if ok, err := HasPendingDelivery(ctx, a.ID, device.ID); err != nil || ok { + t.Fatalf("device still owes after ack-all: %v %v", ok, err) + } + replay, err := PendingReplay(ctx, a.ID, device.ID) + if err != nil || len(replay) != 0 { + t.Fatalf("device replay after ack-all = %d rows, %v; want 0", len(replay), err) + } + + // The other subscriber's debt is untouched: acking is per subscription. + if destroyed, err := CompleteAlertDeliveries(ctx, a.ID); err != nil || destroyed { + t.Fatalf("alert destroyed while another subscriber still owes: %v %v", destroyed, err) + } + if ok, err := HasPendingDeliveryForUser(ctx, a.ID, account, ""); err != nil || !ok { + t.Fatalf("account debt lost when the device acked: %v %v", ok, err) + } + if got, err := AckAllForSubscriber(ctx, a.ID, account, ""); err != nil || got != 1 { + t.Fatalf("AckAllForSubscriber = %d, %v; want 1", got, err) + } + if destroyed, err := CompleteAlertDeliveries(ctx, a.ID); err != nil || !destroyed { + t.Fatalf("last ack did not destroy the alert: %v %v", destroyed, err) + } + + // The alert, its subscriptions and the device's token row are all gone. + if ok, err := HasPendingDelivery(ctx, a.ID, device.ID); err != nil || ok { + t.Fatalf("device owes on a destroyed alert: %v %v", ok, err) + } + if _, err := GetSubscriptionByToken(context.Background(), tokenHash); err == nil { + t.Fatalf("device token still resolves after the alert was destroyed") + } +} diff --git a/src/db/subscriptions.go b/src/db/subscriptions.go index be245c5..26370c2 100644 --- a/src/db/subscriptions.go +++ b/src/db/subscriptions.go @@ -169,12 +169,15 @@ func ListSubscriptions(ctx context.Context, alertID uuid.UUID) ([]Subscription, out := make([]Subscription, 0, 4) for rows.Next() { - var s Subscription - if err := rows.Scan(&s.ID, &s.AlertID, &s.SubscriberUserID, &s.DeviceID, &s.DeviceLabel, - &s.Platform, &s.Enabled, &s.LastSeenAt, &s.Created); err != nil { + // Same scan as every other read: subscriber_user_id is NULL for a device + // subscription (rev 10) and device_token_hash is part of the column set, + // so a hand-written destination list here drifts and breaks the listing + // for exactly the rows anonymous access creates. + s, err := scanSubscription(rows) + if err != nil { return nil, err } - out = append(out, s) + out = append(out, *s) } return out, rows.Err() }