Push: surface delivery failures (no longer a silent success) (#157)

A failed web push (e.g. VAPID key mismatch -> 401/403) was swallowed: /push/test still
returned 202 and the UI showed 'Sent', while the only trace was a journald WARNING.
Now: push.send logs failures to the errors JSONL (phase=push) so they show up like other
errors; /push/test returns a 'failed' count; and the 'Send test' button reports 'No
device' / 'Sent' / 'Failed' from the real result. Document the VAPID env vars (missing
from thermograph.env.example) and how to pin/diagnose them.
This commit is contained in:
Emi Griffith 2026-07-16 17:12:14 -07:00 committed by GitHub
parent 7acf8502dc
commit 4dbb060d4d
3 changed files with 30 additions and 5 deletions

View file

@ -292,7 +292,7 @@ async def push_test(
"url": f"{BASE}/alerts", "url": f"{BASE}/alerts",
"tag": "thermograph-test", "tag": "thermograph-test",
} }
sent, pruned = 0, 0 sent, pruned, failed = 0, 0, 0
for row in rows: for row in rows:
info = {"endpoint": row.endpoint, "keys": {"p256dh": row.p256dh, "auth": row.auth}} info = {"endpoint": row.endpoint, "keys": {"p256dh": row.p256dh, "auth": row.auth}}
# pywebpush blocks on network I/O — keep it off the event loop. # pywebpush blocks on network I/O — keep it off the event loop.
@ -302,6 +302,10 @@ async def push_test(
elif result == "gone": elif result == "gone":
await session.delete(row) await session.delete(row)
pruned += 1 pruned += 1
else:
failed += 1
if pruned: if pruned:
await session.commit() await session.commit()
return {"devices": len(rows), "sent": sent, "pruned": pruned} # `failed` (delivery rejected — e.g. VAPID key mismatch) is surfaced so the client
# can tell "request accepted" apart from "actually delivered".
return {"devices": len(rows), "sent": sent, "pruned": pruned, "failed": failed}

10
push.py
View file

@ -25,6 +25,8 @@ import logging
import os import os
import threading import threading
import audit
from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.primitives.asymmetric import ec from cryptography.hazmat.primitives.asymmetric import ec
from pywebpush import WebPushException, webpush from pywebpush import WebPushException, webpush
@ -114,8 +116,14 @@ def send(subscription_info: dict, payload: dict) -> str:
status = getattr(getattr(e, "response", None), "status_code", None) status = getattr(getattr(e, "response", None), "status_code", None)
if status in (404, 410): if status in (404, 410):
return "gone" # endpoint retired — caller should delete it return "gone" # endpoint retired — caller should delete it
# 401/403 = VAPID key mismatch, etc. Record it where it's visible (the errors
# JSONL / dashboard), not just the system journal.
log.warning("web push failed (status=%s): %s", status, e) log.warning("web push failed (status=%s): %s", status, e)
audit.log_event("error", {"phase": "push", "status": status,
"endpoint": (subscription_info.get("endpoint") or "")[:120]})
return "error" return "error"
except Exception: # noqa: BLE001 - network/encoding errors must not propagate except Exception as e: # noqa: BLE001 - network/encoding errors must not propagate
log.warning("web push send raised", exc_info=True) log.warning("web push send raised", exc_info=True)
audit.log_event("error", {"phase": "push", "error": repr(e),
"endpoint": (subscription_info.get("endpoint") or "")[:120]})
return "error" return "error"

View file

@ -133,7 +133,7 @@ def test_push_subscribe_upsert_test_unsubscribe(monkeypatch):
r = c.post(f"{V2}/push/test") r = c.post(f"{V2}/push/test")
assert r.status_code == 202 assert r.status_code == 202
assert r.json() == {"devices": 1, "sent": 1, "pruned": 0} assert r.json() == {"devices": 1, "sent": 1, "pruned": 0, "failed": 0}
assert sent == [_SUB["endpoint"]] assert sent == [_SUB["endpoint"]]
assert c.request("DELETE", f"{V2}/push/subscribe", assert c.request("DELETE", f"{V2}/push/subscribe",
@ -148,6 +148,19 @@ def test_push_test_prunes_gone_endpoint(monkeypatch):
_login(c, "push-gone@example.com") _login(c, "push-gone@example.com")
assert c.post(f"{V2}/push/subscribe", json=_SUB).status_code == 201 assert c.post(f"{V2}/push/subscribe", json=_SUB).status_code == 201
r = c.post(f"{V2}/push/test") r = c.post(f"{V2}/push/test")
assert r.json() == {"devices": 1, "sent": 0, "pruned": 1} assert r.json() == {"devices": 1, "sent": 0, "pruned": 1, "failed": 0}
# the gone endpoint was removed, so a second pass sees no devices # the gone endpoint was removed, so a second pass sees no devices
assert c.post(f"{V2}/push/test").json()["devices"] == 0 assert c.post(f"{V2}/push/test").json()["devices"] == 0
def test_push_test_reports_failed_delivery(monkeypatch):
# A rejected delivery (e.g. VAPID mismatch) must be reported, not silently dropped:
# the endpoint keeps returning 202 but with sent:0 / failed:N so the client can tell.
import push
monkeypatch.setattr(push, "send", lambda info, payload: "error")
c = _client()
_login(c, "push-fail@example.com")
assert c.post(f"{V2}/push/subscribe", json=_SUB).status_code == 201
r = c.post(f"{V2}/push/test")
assert r.status_code == 202
assert r.json() == {"devices": 1, "sent": 0, "pruned": 0, "failed": 1}