fix(notifications): lease deliveries instead of marking sent on claim - #167
Merged
Merged
Conversation
ClaimDue set sent_at = now() at claim time, so sent_at meant "a dispatcher saw this row", not "hackers were notified". Any interruption between the claim and the push — SIGTERM, OOM, a Cloud Run scale-down — left the row marked sent and it was never retried. The notification was lost silently. Split that single column into a revocable lease and a real delivery record: - claimed_at is the lease. A dispatcher that dies mid-delivery leaves the row claimable again once the lease elapses, instead of burning it. - sent_at now means only that delivery actually happened. - attempts bounds retries so a poison row cannot loop forever. - failed_at is terminal, with last_error recording why. Because deliveries are now genuinely retryable, a notification stuck across an outage would eventually go out hours late — a "starting in 15 minutes" reminder arriving at 3am misinforms hackers. DISPATCHER_MAX_LATENESS_MINUTES (default 30) bounds that: past the window the row is failed, not sent, and surfaces in a new Failed tab in the superadmin notifications table. Delivery also fans out concurrently now, capped by a semaphore, with each subscription's outcome recorded per-index so aggregation stays deterministic. The batch is bounded by a deadline inside the lease so a slow push service cannot outlive the claim it was dispatched under. Bookkeeping runs on a context.WithoutCancel context, so a claim is still resolved when the dispatcher context is already cancelled by shutdown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CcXtZJizDrfbeYZ551eZ5
Moving the claim from sent_at to claimed_at left three predicates that had quietly relied on "a claimed row has sent_at set", plus one bookkeeping gap in the batch loop. GenerateFromSchedule deleted schedule-sourced rows WHERE sent_at IS NULL. That used to skip in-flight rows; now it deleted them mid-delivery, and it wiped every failed row — the Failed tab's entire content for reminders — on an unrelated action. Neither could be recreated: both are already past due. The DELETE now also requires claimed_at IS NULL AND failed_at IS NULL. Failed rows are history, like sent ones. Update matched WHERE sent_at IS NULL too, so an operator could edit a row a dispatcher was delivering. The edit cleared the lease, and MarkSent then stamped the edited content — new title, future time — as sent with the old recipient count. Update now refuses a leased row with ErrNotificationInFlight (wrapping ErrConflict) and the handler returns a distinct 409 so the operator knows to simply retry. ClaimDue charges an attempt up front, which is right for a crash but wrong for rows the batch deadline cut off before they were tried: each deferral cost an attempt, and after five a notification sat unclaimable in the Scheduled tab forever, never once attempted and never failed. Deferred rows are now handed back through ReleaseUnattempted with the attempt refunded. truncateCause sliced last_error by byte, so a multibyte cause could end in a partial rune. Postgres rejects that as invalid UTF-8, failing the very write that resolves the claim. It now cuts on a rune boundary and stays within its 500-byte bound (previously 502). TestIntegrationGenerateFromSchedulePreservesLeaseState fails on the old predicate and passes on the new one; the lease integration test covers the refund and the refused edit; TestDispatchBatch covers the deadline path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
ClaimDuesetsent_at = now()at claim time, sosent_atmeant "a dispatcher saw this row", not "hackers were notified".Any interruption between the claim and the actual push — SIGTERM during a deploy, OOM, a Cloud Run scale-down — left the row marked sent. It was never retried, and nobody got the notification. Silent loss, invisible in the UI, because the row looked delivered.
The fix
Migration
000051splits that overloaded column into a revocable lease and a real delivery record:claimed_atsent_atattemptsfailed_atlast_errorClaimDueleases withFOR UPDATE SKIP LOCKED, filtering onattempts < maxAttemptsandclaimed_at IS NULL OR claimed_at < now - lease. Outcomes resolve throughMarkSent/ReleaseClaim/MarkFailed— a claim that is never resolved is exactly the bug this prevents.Rows already marked sent are left alone; they can't be reclassified retroactively.
Why
DISPATCHER_MAX_LATENESS_MINUTESis part of thisMaking delivery reliable creates a new failure mode. Previously a notification stuck across an outage was lost; now it is faithfully retried — and would go out hours late. A "starting in 15 minutes" reminder arriving at 3am is worse than none.
DISPATCHER_MAX_LATENESS_MINUTES(default 30) bounds the retry window. Past it the row is failed rather than sent, and surfaces in a new Failed tab in the superadmin notifications table instead of vanishing. The test asserts the strong version:ListByRoleis never called, so no subscription is even contacted.Concurrent delivery
Delivery now fans out concurrently under a semaphore, with each subscription's outcome recorded per-index so aggregation stays deterministic. The batch is bounded by a deadline inside the lease, so one hanging push service cannot outlive the claim it was dispatched under.
bytes.Cloneper send —webpushappends its padding delimiter into the payload slice's spare capacity, so concurrent sends must not share one slice.Bookkeeping runs on a
context.WithoutCancelcontext, so a claim is still resolved when the dispatcher context is already cancelled by shutdown.Verification
go build,go vet,gofmt,go test ./...andgo test -race ./...all pass (race matters — delivery is now concurrent). Portalformat:check,lintandbuildpass.New coverage in
dispatcher_test.go: marks sent after a real delivery; releases the claim when subscriptions cannot be listed; fails terminally once attempts are exhausted; expires past the lateness window without sending; resolves the claim even when the dispatcher context is already cancelled.TestIntegrationNotificationLeasehas not been run. It is gated behindHARP_TEST_DSN, and it is the only test that exercises the real SQL — the claim query, the partial index, the migration itself. Everything else runs onMockStore. Please run it against a live database before merging:Notes for review
swagv1.16.4 cannot parse the Go 1.27 stdlib, sotask gen-docsfails onmaintoday.docs/docs.gohere was updated by hand to add the new fields; it compiles, but the generator mismatch is worth fixing separately.hackutd/hackutd-harpfork and rebased ontomain. Two fork-only dependencies were deliberately dropped rather than dragged in: the dispatcher goroutine is no longer registered with that fork'sbackgroundJobsdrain (the lease makes it unnecessary — an unresolved claim expires and retries), and the fork's walk-in integration test was left behind.pushEndpointHost/pushSendError) is preserved in the new concurrent send path; the fork predated it.🤖 Generated with Claude Code
https://claude.ai/code/session_012CcXtZJizDrfbeYZ551eZ5