Harden push notifications: state-valued subscribe, bounded delivery, rotation detection

[nedtwigg]

Follow-on to the merged push-notifications base. Six fixes for races and gaps found while reviewing that work, then three more found while reviewing those.

The central change

POST /api/push/subscribe reported an event — deviceRegistrationsReset — which is only meaningful to a client that received the response. A committed POST whose response was lost could not be repaired by its idempotent retry, since the retry cannot re-announce a deletion it already performed. So Pocket carried a browser-side latch to reconstruct what the Server had done to it, and every subsequent fix was another patch on that reconstruction.

It now reports state: every Host this device is registered with after the mutation, which upsert already had in hand inside the mutex. The retry simply answers what is registered now, and the lost-response case self-heals.

Because the POST and the GET /api/push/subscriptions read then answer the same complete question about the same device, there is nothing to merge between them — only an ordering. That collapsed the client from four coupled refs and two exported helpers down to one registration counter and a staleness check:

before	after
pushEnableCompletionsRef (per-Host version map)	—
pushEnableCompletionVersionRef	—
pushRegistrationResetVersionRef	pushRegistrationVersionRef
pushRegistrationResetPendingRef	—
completePushSubscriptionRegistration	—
reconcilePushSubscribedHosts	two lines in the effect

Scoping that response to the device is safe where the subscriptions GET deliberately is not: the request carries a device signature, so the caller has proven it owns the identity being reported on.

Also here

• Rows are validated as they are read. push-subscriptions.json is hand-editable by design — revoking a device is deleting its rows — so a half-finished edit now reads as a missing registration (which re-offers Enable and self-repairs) rather than as a live row nothing can be delivered to. One guard at the read boundary instead of each consumer defending itself.
• Delivery is bounded by wall clock, not just socket idle. A push service that accepts the connection and then trickles bytes resets the inactivity timer indefinitely. A 15s deadline per send sits above the 10s inactivity bound and far below the 300s TTL. Applied in the route so it holds for any injected sender; a cut send reports failed, so the row survives to be retried. It bounds the route, not the socket — web-push accepts no AbortSignal.
• Cache headers on the Pocket build. Hashed assets under /assets/ are immutable; the unhashed root is no-cache. The revalidation half is load-bearing: emptyOutDir deletes the previous build's hashed files, so a heuristically cached index.html did not merely serve stale code, it requested deleted files and the app failed to boot.
• Endpoint rotation is noticed. A push service may replace an address without the VAPID key changing, leaving every stored row unreachable while the browser still reports a valid subscription. Pocket now records a digest of what it registered and compares on open. Absent reads as no opinion, so nobody is forced to re-register on upgrade.

A pushsubscriptionchange handler is deliberately not the mechanism. A worker can reach the device key (a non-extractable CryptoKey in IndexedDB) but not a session token — that is in memory only, never persisted, and minted solely behind a fresh WebAuthn assertion, which a worker cannot perform. Unattended re-registration would need a credential the trust model does not grant.

Testing

Full suite green across all nine packages, including this branch's additions: the lost-response self-heal, a wedged sender not holding up a fan-out, malformed-row handling, the two cache-header classes, and endpoint-rotation detection.

Not yet exercised on a phone. Every server test drives a fake sender and the browser half is stubbed globals, so the suite proves the state machines and the wire contract and nothing about delivery. The hostIds path in particular has never run against a real push service.

Note on the merge

origin/main is merged in. main independently advanced the same subsystem — VAPID subject derivation with loopback rejection, and failure logs carrying the service's reason body — which is complementary: main has no request timeout at all, so both bounds added here survive. Two conflicts, both resolved by keeping each side.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying mouseterm with    Cloudflare Pages

Latest commit:	8df793b
Status:	✅  Deploy successful!
Preview URL:	https://755cb9cf.mouseterm.pages.dev
Branch Preview URL:	https://push-notifications.mouseterm.pages.dev

View logs

[dormouse-bot]

One issue in the cache-header work — the SPA fallback derives its class from the request path, but the response it returns is always the shell.

A GET for a hashed asset that no longer exists (/assets/index-OLD.js) misses in serveStatic, falls through to the app.get('*') fallback — the existing /some/deep/link assertion in static.test.mjs proves the fallback catches unknown paths — and answers 200 text/html with index.html. pocketCacheControl('/assets/index-OLD.js') returns public, max-age=31536000, immutable, so the browser stores an HTML body under a JS URL for a year, immutable so a reload cannot revalidate it.

That is reachable from the exact scenario the doc comment is written against. emptyOutDir plus a live server means a client can request a hashed asset mid-deploy and get the shell. Vite hashes are content-derived, so an unchanged chunk keeps its filename across builds — the poisoned URL is then requested again by every subsequent load, and the app stays broken with no recovery short of clearing site data. Before this PR the same request returned HTML too, but with no explicit freshness, so a reload repaired it.

Inline suggestion below. The alternative worth considering is answering 404 for a /assets/ miss instead of falling through at all — the shell is never a useful answer for a subresource, and it would also stop the "HTML parsed as JS" console error.

Two smaller notes, no action needed if you disagree:

• The new cache tests cover a present asset and the unhashed shell, but not the asset-miss path above — that's where the header is wrong.
• reconcilePushSubscribedHosts and its two unit tests are gone and the replacement (drop a read the registration counter overtook, App.tsx) has no test of its own. The new mechanism is much smaller, so this may well be the right trade, but the race it guards is now unexercised.

The rest reads correct to me. upsert's deviceHostIds is sound — deviceRegistrationsReset false implies every surviving row for the device shares stored's endpoint, keys, and VAPID key, so the "no further filtering needed" claim holds; the reset branch trivially leaves one row. The malformed-row guard at the read boundary, the Promise.race deadline reporting failed so the row survives, and the hostIds-as-state contract all check out against the tests.