Separate delivery receipts from dismissals
The client acks a frame as soon as it arrives — that is how an online device stops being re-told — and the service recorded that ack in acked_at, the column the drain-to-empty rule counts. Measured on an Android 15 emulator: the owner's trigger reached the subscriber at .562 and the alert was destroyed 27 ms later, mid-siren, so the owner's disarm answered ERR_ALERT_NOT_FOUND. alarm_deliveries now carries two timestamps. delivered_at is the receipt (the WS ack frame and the offline check-in): it retires re-delivery and nothing else. acked_at is the human dismissal (ackAlarm): only that ends the alert, once every subscription's row is dismissed. Migration 011 adds the column and replaces the pending index with an undelivered one. ackAlarm also runs the drain check on every explicit dismissal, not only when this call cleared a row: a receipted row still has to be dismissed. Also fixed, found by the same probe run: db.Close left the closed pool as the process singleton (a second probe inherited it), and the lifecycle probe's three subscriptions shared one device id, which migration 010 made illegal. Verified: both probes pass against the live dev database, including the new receipt-vs-dismissal case; migration 011 applied to the dev DB.
This commit is contained in:
+16
-12
@@ -125,13 +125,17 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
destroyed, err := db.CompleteAlertDeliveries(r.Context(), alertID)
|
||||
if err != nil {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
resp.Destroyed = destroyed
|
||||
}
|
||||
// The drain check runs on every explicit dismissal, not only when this
|
||||
// call cleared a row: the receipt ack marks a row delivered but not
|
||||
// dismissed, so the person dismissing is what completes the set even
|
||||
// when there was nothing left for THIS call to clear.
|
||||
destroyed, err := db.CompleteAlertDeliveries(r.Context(), alertID)
|
||||
if err != nil {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
resp.Destroyed = 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)
|
||||
@@ -163,13 +167,13 @@ func AckAlarm(w http.ResponseWriter, r *http.Request, payload any) {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
destroyed, err := db.CompleteAlertDeliveries(r.Context(), alertID)
|
||||
if err != nil {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
resp.Destroyed = destroyed
|
||||
}
|
||||
destroyed, err := db.CompleteAlertDeliveries(r.Context(), alertID)
|
||||
if err != nil {
|
||||
dbError(w, action, err)
|
||||
return
|
||||
}
|
||||
resp.Destroyed = destroyed
|
||||
}
|
||||
|
||||
Debug(action, "POST", "alert=%s event=%s all=%t acked=%d destroyed=%t",
|
||||
|
||||
@@ -112,6 +112,12 @@ func Close() {
|
||||
if instance != nil && instance.Pool != nil {
|
||||
instance.Pool.Close()
|
||||
}
|
||||
// Reset the singleton so a later NewDB()/Pool() opens a fresh pool instead
|
||||
// of handing out the closed one. The probes in this package each open and
|
||||
// close their own pool, and a sequential run of two probes must not inherit
|
||||
// the previous one's closed pool.
|
||||
instance = nil
|
||||
once = sync.Once{}
|
||||
}
|
||||
|
||||
// redact strips the password from a DSN before it can reach a log line:
|
||||
|
||||
+54
-5
@@ -59,6 +59,7 @@ func PendingTrigger(ctx context.Context, alertID, userID uuid.UUID, deviceID str
|
||||
WHERE d.alert_id = $1
|
||||
AND d.subscription_id = (SELECT id FROM target)
|
||||
AND d.acked_at IS NULL
|
||||
AND d.delivered_at IS NULL
|
||||
ORDER BY e.created_at DESC, e.id DESC
|
||||
LIMIT 1`
|
||||
|
||||
@@ -94,6 +95,7 @@ func PendingReplay(ctx context.Context, alertID, subscriptionID uuid.UUID) ([]Pe
|
||||
WHERE d.alert_id = $1
|
||||
AND d.subscription_id = $2
|
||||
AND d.acked_at IS NULL
|
||||
AND d.delivered_at IS NULL
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM alarm.alarm_events x
|
||||
WHERE x.alert_id = d.alert_id
|
||||
@@ -129,7 +131,7 @@ func PendingReplay(ctx context.Context, alertID, subscriptionID uuid.UUID) ([]Pe
|
||||
func AckDelivery(ctx context.Context, alertID, subscriptionID, eventID uuid.UUID) (int64, error) {
|
||||
tag, err := Pool().Exec(ctx,
|
||||
`UPDATE alarm.alarm_deliveries
|
||||
SET acked_at = now()
|
||||
SET acked_at = now(), delivered_at = coalesce(delivered_at, now())
|
||||
WHERE alert_id = $1 AND subscription_id = $2 AND event_id = $3
|
||||
AND acked_at IS NULL`,
|
||||
alertID, subscriptionID, eventID)
|
||||
@@ -150,7 +152,7 @@ func AckDelivery(ctx context.Context, alertID, subscriptionID, eventID uuid.UUID
|
||||
func AckAlarmDelivery(ctx context.Context, alertID, userID uuid.UUID, deviceID string, eventID uuid.UUID) (int64, error) {
|
||||
const q = `
|
||||
UPDATE alarm.alarm_deliveries d
|
||||
SET acked_at = now()
|
||||
SET acked_at = now(), delivered_at = coalesce(d.delivered_at, now())
|
||||
WHERE d.alert_id = $1
|
||||
AND d.event_id = $2
|
||||
AND d.acked_at IS NULL
|
||||
@@ -179,7 +181,8 @@ func HasPendingDelivery(ctx context.Context, alertID, subscriptionID uuid.UUID)
|
||||
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)`,
|
||||
WHERE alert_id = $1 AND subscription_id = $2
|
||||
AND acked_at IS NULL AND delivered_at IS NULL)`,
|
||||
alertID, subscriptionID).Scan(&owed)
|
||||
if err != nil {
|
||||
return false, err
|
||||
@@ -199,6 +202,7 @@ func HasPendingDeliveryForUser(ctx context.Context, alertID, userID uuid.UUID, d
|
||||
JOIN alarm.subscriptions s ON s.id = d.subscription_id
|
||||
WHERE d.alert_id = $1
|
||||
AND d.acked_at IS NULL
|
||||
AND d.delivered_at IS NULL
|
||||
AND s.subscriber_user_id = $2
|
||||
AND ($3 = '' OR s.device_id = $3))`,
|
||||
alertID, userID, deviceHash).Scan(&owed)
|
||||
@@ -217,7 +221,7 @@ func HasPendingDeliveryForUser(ctx context.Context, alertID, userID uuid.UUID, d
|
||||
func AckAllDeliveries(ctx context.Context, alertID, subscriptionID uuid.UUID) (int64, error) {
|
||||
tag, err := Pool().Exec(ctx,
|
||||
`UPDATE alarm.alarm_deliveries
|
||||
SET acked_at = now()
|
||||
SET acked_at = now(), delivered_at = coalesce(delivered_at, now())
|
||||
WHERE alert_id = $1 AND subscription_id = $2 AND acked_at IS NULL`,
|
||||
alertID, subscriptionID)
|
||||
if err != nil {
|
||||
@@ -231,7 +235,7 @@ func AckAllDeliveries(ctx context.Context, alertID, subscriptionID uuid.UUID) (i
|
||||
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()
|
||||
SET acked_at = now(), delivered_at = coalesce(d.delivered_at, now())
|
||||
WHERE d.alert_id = $1
|
||||
AND d.acked_at IS NULL
|
||||
AND d.subscription_id IN (
|
||||
@@ -245,3 +249,48 @@ func AckAllForSubscriber(ctx context.Context, alertID, userID uuid.UUID, deviceH
|
||||
}
|
||||
return tag.RowsAffected(), nil
|
||||
}
|
||||
|
||||
// MarkDelivered records that the device has the trigger, without treating that
|
||||
// as a dismissal.
|
||||
//
|
||||
// Clients ack a frame the moment they receive it so the service stops offering
|
||||
// the same alarm on every poll. That is a receipt, not a decision by the person
|
||||
// holding the phone: the reveal screen may still be behind a locked screen, and
|
||||
// the alarm is still ringing. Recording it separately is what stops the drain
|
||||
// rule from destroying an alert while it is still sounding.
|
||||
//
|
||||
// Idempotent and best-effort by construction: a repeat, a malformed event id or
|
||||
// an admin connection match nothing.
|
||||
func MarkDelivered(ctx context.Context, alertID, subscriptionID, eventID uuid.UUID) (int64, error) {
|
||||
tag, err := Pool().Exec(ctx,
|
||||
`UPDATE alarm.alarm_deliveries
|
||||
SET delivered_at = now()
|
||||
WHERE alert_id = $1 AND subscription_id = $2 AND event_id = $3
|
||||
AND delivered_at IS NULL AND acked_at IS NULL`,
|
||||
alertID, subscriptionID, eventID)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
return tag.RowsAffected(), nil
|
||||
}
|
||||
|
||||
// MarkAllDeliveredForUser is MarkDelivered for the account path: everything this
|
||||
// caller's device still has not received is now received.
|
||||
func MarkAllDeliveredForUser(ctx context.Context, alertID, userID uuid.UUID, deviceHash string) (int64, error) {
|
||||
tag, err := Pool().Exec(ctx,
|
||||
`UPDATE alarm.alarm_deliveries d
|
||||
SET delivered_at = now()
|
||||
WHERE d.alert_id = $1
|
||||
AND d.acked_at IS NULL
|
||||
AND d.delivered_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
|
||||
}
|
||||
|
||||
@@ -77,9 +77,13 @@ func TestLedgerProbe(t *testing.T) {
|
||||
}
|
||||
return a
|
||||
}
|
||||
// Each subscription needs its own device id: migration 010 enforces
|
||||
// (alert_id, device_id) uniqueness so one device has at most one
|
||||
// subscription per alert.
|
||||
join := func(alertID, user uuid.UUID) *Subscription {
|
||||
s, err := CreateSubscription(ctx, &Subscription{
|
||||
AlertID: alertID, SubscriberUserID: user, DeviceID: "sha256:" + strings.Repeat("a", 64),
|
||||
AlertID: alertID, SubscriberUserID: user,
|
||||
DeviceID: "sha256:" + strings.ReplaceAll(user.String(), "-", ""),
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CreateSubscription: %v", err)
|
||||
@@ -102,8 +106,10 @@ func TestLedgerProbe(t *testing.T) {
|
||||
join(drain.ID, sub1)
|
||||
join(drain.ID, sub2)
|
||||
join(drain.ID, sub3) // third enabled subscriber: acking a different device's row must not count
|
||||
disabledUser := uuid.New()
|
||||
disabled, err := CreateSubscription(ctx, &Subscription{
|
||||
AlertID: drain.ID, SubscriberUserID: uuid.New(), DeviceID: "sha256:" + strings.Repeat("b", 64),
|
||||
AlertID: drain.ID, SubscriberUserID: disabledUser,
|
||||
DeviceID: "sha256:" + strings.ReplaceAll(disabledUser.String(), "-", ""),
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("disabled subscription: %v", err)
|
||||
|
||||
@@ -92,35 +92,43 @@ func TestDeviceLedgerProbe(t *testing.T) {
|
||||
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)
|
||||
// A receipt is not a dismissal. The clients ack a frame the moment they get
|
||||
// it, and that used to be written to acked_at — which destroyed the alert
|
||||
// while it was still ringing (measured on the emulator: 27 ms after the
|
||||
// trigger). A receipt settles re-delivery only.
|
||||
marked, err := MarkDelivered(ctx, a.ID, device.ID, event.ID)
|
||||
if err != nil || marked != 1 {
|
||||
t.Fatalf("MarkDelivered = %d, %v; want 1", marked, 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)
|
||||
t.Fatalf("device still owes after a receipt: %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)
|
||||
if replay, err := PendingReplay(ctx, a.ID, device.ID); err != nil || len(replay) != 0 {
|
||||
t.Fatalf("device replay after a receipt = %d rows, %v; want 0", len(replay), err)
|
||||
}
|
||||
if destroyed, err := CompleteAlertDeliveries(ctx, a.ID); err != nil || destroyed {
|
||||
t.Fatalf("a receipt destroyed the alert: %v %v", destroyed, err)
|
||||
}
|
||||
|
||||
// The other subscriber's debt is untouched: acking is per subscription.
|
||||
// The dismissal is what the drain counts. A device that received and later
|
||||
// dismissed must not end the alert while another subscriber has not.
|
||||
if got, err := AckDelivery(ctx, a.ID, device.ID, event.ID); err != nil || got != 1 {
|
||||
t.Fatalf("AckDelivery after a receipt = %d, %v; want 1 (the dismissal still has to be recorded)", got, err)
|
||||
}
|
||||
if destroyed, err := CompleteAlertDeliveries(ctx, a.ID); err != nil || destroyed {
|
||||
t.Fatalf("alert destroyed while another subscriber still owes: %v %v", destroyed, err)
|
||||
t.Fatalf("one dismissal destroyed an alert another subscriber never dismissed: %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)
|
||||
t.Fatalf("account debt lost when the device dismissed: %v %v", ok, err)
|
||||
}
|
||||
if got, err := MarkAllDeliveredForUser(ctx, a.ID, account, ""); err != nil || got != 1 {
|
||||
t.Fatalf("MarkAllDeliveredForUser = %d, %v; want 1", got, 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)
|
||||
t.Fatalf("the last dismissal did not destroy the alert: %v %v", destroyed, err)
|
||||
}
|
||||
|
||||
// The alert, its subscriptions and the device's token row are all gone.
|
||||
|
||||
+20
-13
@@ -124,11 +124,21 @@ func (c *client) readPump() {
|
||||
}
|
||||
}
|
||||
|
||||
// ackDelivery marks the ledger row for a client ack. It is best-effort and
|
||||
// idempotent: a malformed event id, an admin connection (no subscription) or a
|
||||
// repeated ack are all silent no-ops. A database failure is logged, never fatal —
|
||||
// the client already has the alarm, and the worst case is one duplicate offline
|
||||
// signal, not a dropped ring.
|
||||
// ackDelivery records a *receipt*: the client has the frame, so the service must
|
||||
// not offer it again on the next check-in or reconnect.
|
||||
//
|
||||
// It deliberately does not dismiss the alarm. Clients ack automatically the
|
||||
// moment a frame arrives — the person may still be asleep behind a lock screen
|
||||
// and the siren is still sounding — so treating that ack as "dismissed" let the
|
||||
// drain-to-empty rule destroy the alert 27 ms after the owner triggered it
|
||||
// (measured on an Android 15 emulator: the phone was ringing for an alert that
|
||||
// no longer existed). Dismissal is the explicit `ackAlarm` action; only that
|
||||
// drives the drain.
|
||||
//
|
||||
// Best-effort and idempotent: a malformed event id, an admin connection (no
|
||||
// subscription) or a repeated receipt are all silent no-ops. A database failure
|
||||
// is logged, never fatal — the client already has the alarm, and the worst case
|
||||
// is one duplicate check-in answer, not a dropped ring.
|
||||
func (c *client) ackDelivery(eventID string) {
|
||||
if c.identity.SubscriptionID == nil {
|
||||
return
|
||||
@@ -139,14 +149,14 @@ func (c *client) ackDelivery(eventID string) {
|
||||
}
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
|
||||
defer cancel()
|
||||
cleared, err := db.AckDelivery(ctx, c.identity.AlertID, *c.identity.SubscriptionID, id)
|
||||
marked, err := db.MarkDelivered(ctx, c.identity.AlertID, *c.identity.SubscriptionID, id)
|
||||
if err != nil {
|
||||
c.hub.opts.Logf("ws: ledger ack failed alert=%s event=%s: %v", c.identity.AlertID, id, err)
|
||||
c.hub.opts.Logf("ws: delivery receipt failed alert=%s event=%s: %v", c.identity.AlertID, id, err)
|
||||
return
|
||||
}
|
||||
if cleared == 0 {
|
||||
// A repeat ack (or one for an event this device never owed) is a no-op:
|
||||
// nothing to record and no drain to check.
|
||||
if marked == 0 {
|
||||
// A repeated receipt (or one for an event this device was never offered)
|
||||
// is a no-op: nothing to record.
|
||||
return
|
||||
}
|
||||
// Anonymous subscribers have no account to record analytics against: the
|
||||
@@ -158,9 +168,6 @@ func (c *client) ackDelivery(eventID string) {
|
||||
} else if err := db.RecordDelivered(ctx, c.identity.AlertID, c.identity.UserID); err != nil {
|
||||
c.hub.opts.Logf("ws: delivered analytics alert=%s: %v", c.identity.AlertID, err)
|
||||
}
|
||||
if _, err := db.CompleteAlertDeliveries(ctx, c.identity.AlertID); err != nil {
|
||||
c.hub.opts.Logf("ws: drain check alert=%s: %v", c.identity.AlertID, err)
|
||||
}
|
||||
}
|
||||
|
||||
// writePump owns all writes except the WriteControl calls in readPump (which
|
||||
|
||||
Reference in New Issue
Block a user