fix(webpush): cap in-flight sends per subscription

[SebTardif]

One slow browser push endpoint could occupy all four delivery workers, delaying notifications to other devices. Serialize sends per endpoint within the existing pool. Later alerts for that endpoint wait, and the shared channel plus waiting lists share the existing 1,024-alert pending budget. Every send still rechecks device/session/message authority and uses unchanged success and backoff bookkeeping.

The finished change removes an unreachable dropped-result branch, clears consumed waiting entries, preserves all accepted alerts after sender panics, and adds cleanup and generous timeout headroom to the real HTTP tests. Updated the push delivery guide and Unreleased changelog. Thanks @SebTardif.

Validation: the new isolation regression failed on main with subscription had 4 deliveries in flight, want 1. On the candidate, isolation, encrypted HTTP/SQLite delivery, shared overflow, panic recovery, and delayed-success cases passed three times under go test -race. Full pnpm check passed on AWS Crabbox (Go 1.27.1, Node.js 24, pnpm 12.7.0). Independent Codex review is scoped-clean through P2 after fixing the initial review's test timeout finding. GitHub CI must pass on the updated head before merge.

The transport tests use a synthetic local relay, not a public push provider. The existing best-effort overflow and restart behavior is unchanged.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed October 7, 2026, 12:57 AM ET / 04:57 UTC (Revision 7).

ClawSweeper review

What this changes

The PR serializes browser push delivery per endpoint, bounds all pending alerts together, and adds transport regression tests, documentation, and a changelog entry.

Merge readiness

✅ Ready for maintainer review

This PR remains necessary: current main and v0.7.0 allow one slow endpoint to occupy all four workers. The prior findings are resolved, production transport proof remains sufficient, and no actionable defect was found.

Priority: P2
Reviewed head: cd3ced5a1cd41283586e386b3e71c4512271ff96

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused scheduling repair with relevant production transport proof and resolved prior findings meets normal good PR quality.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): Reported candidate race runs exercise WebPushNotifier and the real encrypted Sender through SQLite and an HTTP relay, covering held-endpoint isolation, accepted-alert completion, overflow, and delayed-success bookkeeping; prior captured observations substantiate these outcomes. No stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): Reported candidate race runs exercise WebPushNotifier and the real encrypted Sender through SQLite and an HTTP relay, covering held-endpoint isolation, accepted-alert completion, overflow, and delayed-success bookkeeping; prior captured observations substantiate these outcomes. No stored-data contract changes.
Evidence reviewed	8 items	Pinned introduced change: The verified main-to-head delta adds endpoint scheduling and aggregate pending accounting in the notifier, with tests, documentation, and changelog changes. No dependency, configuration, SQL schema, or generated store code changes. Still necessary on current main: Main calls deliver directly from each of four workers without endpoint ownership. Multiple alerts for one stalled endpoint can occupy the entire pool. Latest release comparison: v0.7.0 also sends directly from the shared workers without a per-endpoint concurrency limit.
Findings	None	None.
Security	None	None.

How this fits together

ClickClack queues browser notifications after chat messages are posted. Delivery workers recheck device registration and recipient access, then encrypt and send alerts through the browser’s push service.

flowchart TD
A[Chat notification] --> B[Shared pending budget]
B --> C{Endpoint already sending}
C -->|Yes| D[Endpoint waiting list]
C -->|No| E[Delivery worker]
D --> E
E --> F[Recheck device and recipient]
F --> G[Encrypted push request]
G --> H[Record delivery outcome]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test growth	production +88, tests +323/-8	Production growth implements bounded endpoint scheduling; most added lines validate isolation and delivery bookkeeping.

Technical review

Best possible solution:

Keep endpoint serialization within the existing worker pool and shared pending budget while preserving send-time authorization and normal delivery bookkeeping.

Do we have a high-confidence way to reproduce the issue?

Yes: current-main source shows that four alerts for one held endpoint can occupy all four workers, and the captured body reports the isolation regression failing on main. This read-only review did not execute it.

Is this the best way to solve the issue?

Yes: endpoint ownership and aggregate pending accounting address worker monopolization without changing configuration, stored subscriptions, or authorization.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning medium; reviewed against 8ca28becb38c.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a bounded repair to browser notification delivery when one endpoint is slow.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Reported candidate race runs exercise WebPushNotifier and the real encrypted Sender through SQLite and an HTTP relay, covering held-endpoint isolation, accepted-alert completion, overflow, and delayed-success bookkeeping; prior captured observations substantiate these outcomes. No stored-data contract changes.
• proof: sufficient: Contributor real behavior proof is sufficient. Reported candidate race runs exercise WebPushNotifier and the real encrypted Sender through SQLite and an HTTP relay, covering held-endpoint isolation, accepted-alert completion, overflow, and delayed-success bookkeeping; prior captured observations substantiate these outcomes. No stored-data contract changes.

Evidence

What I checked:

• Pinned introduced change: The verified main-to-head delta adds endpoint scheduling and aggregate pending accounting in the notifier, with tests, documentation, and changelog changes. No dependency, configuration, SQL schema, or generated store code changes. (apps/api/internal/httpapi/webpush.go:258, cd3ced5a1cd4)
• Still necessary on current main: Main calls deliver directly from each of four workers without endpoint ownership. Multiple alerts for one stalled endpoint can occupy the entire pool. (apps/api/internal/httpapi/webpush.go:228, 8ca28becb38c)
• Latest release comparison: v0.7.0 also sends directly from the shared workers without a per-endpoint concurrency limit. (apps/api/internal/httpapi/webpush.go, 2408cf117adc)
• Prior findings resolved: Success bookkeeping remains unchanged; no cooldown interface addition remains. Pending accounting includes channel reservations and endpoint waiters, relay failure paths release their mutex before Fatalf, and recovered sender panics return to the endpoint drain loop. Direct comparison with the earlier reviewed SHA was unavailable locally because its object could not be retrieved; these conclusions come from the current source. (apps/api/internal/httpapi/webpush.go:262, cd3ced5a1cd4)
• Production transport proof: The captured body, sourceRevision 1ffc03b2a81808169c0940ee728d3dfe9b52d6c801dea241cc1c979cdcf50e08, reports a failing main isolation regression and three passing candidate race runs covering isolation, encrypted HTTP/SQLite delivery, shared overflow, panic recovery, and delayed success. The supplied previous review records the earlier relay observations: slow_max=1, slow_requests=4, other_while_held=1, pending=1024, and successful SQLite bookkeeping without cooldown. The current HTTP test exercises the production notifier and encrypted Sender against a real local HTTP relay rather than a mocked transport. This proves internal scheduling behavior, without claiming public-provider or browser-display coverage. (apps/api/internal/httpapi/webpush_test.go:495, cd3ced5a1cd4)
• Authorization and storage contracts preserved: Every endpoint successor calls deliver, which rereads device/session/key/backoff state and message access before Sender.Send. Waiting lists contain existing delivery identifiers rather than cached authorization or device keys. Only runtime scheduling state is added; persisted subscription formats and migrations are unchanged. (apps/api/internal/httpapi/webpush.go:332, cd3ced5a1cd4)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (6 earlier review cycles)

• reviewed 2026-10-04T02:49:45.707Z sha 3d5cdae :: needs real behavior proof before merge. :: [P1] Keep successful slow deliveries out of dead-device failure tracking
• reviewed 2026-10-05T23:49:35.110Z sha b5b96c2 :: needs real behavior proof before merge. :: [P1] Expose the cooldown method through the server's store interface
• reviewed 2026-10-06T00:12:01.097Z sha 53b41ba :: blocked before merge. :: none
• reviewed 2026-10-06T00:30:16.711Z sha d022720 :: needs changes before merge. :: [P2] [P2] Include endpoint waiters in the aggregate queue budget
• reviewed 2026-10-06T00:47:41.461Z sha c21acee :: needs changes before merge. :: [P2] Release the relay mutex before failing the test
• reviewed 2026-10-06T01:05:56.546Z sha 4d28bd4 :: needs maintainer review before merge. :: none

[steipete]

Merged as 94e3b71. Thanks @SebTardif.

The main-branch regression reproduced subscription had 4 deliveries in flight, want 1. The finished candidate passed the isolation, encrypted HTTP/SQLite, overflow, panic-recovery, and delayed-success tests three times under go test -race, followed by full pnpm check on AWS Crabbox. The narrow HTTP-test timeout found in independent review was corrected; final P2 autoreview was clean.

All checks passed on cd3ced5: server/web/Docker/browser CI and Windows/macOS/Linux desktop CI. The real transport proof uses a synthetic local relay, not a public push provider. Existing best-effort overflow/restart behavior and delivery authorization remain intact.