perf(janitor): batch the retention DELETEs on prod-sized tables#397
Merged
Conversation
The hourly janitor's retention prunes ran a single unbounded `DELETE ... WHERE expired` per table. On the first sweep of a backlog that takes one long row-lock and emits a huge WAL burst — rough on a prod-sized `messages` table (and the traffic-scaled webhook/idempotency tables). Drain in ctid-bounded chunks (LIMIT 5000) instead, looping until a short batch. Each statement's lock + WAL stays small; the caller's ctx bounds total runtime and a partial sweep just resumes next hour (the delete is idempotent). Same net effect, gentler on Postgres. Batched the four traffic-scaled tables: - messages (identity.DeleteExpiredMessages) - webhook_events (webhookpub.DeleteExpiredWebhookEvents) - webhook_subscriber_deliveries (webhook.DeleteExpiredSubscriberDeliveries) - idempotency_keys (idempotency.Store.Sweep) Bounded tables (user_sessions, legacy webhook_deliveries, oauth) keep their single-statement DELETE — nothing to gain there. The batch size is a test-overridable var in identity so a unit test can drive the multi-batch drain loop cheaply (5 expired rows, batch size 2 → 3 batches) and assert non-expired rows survive. Existing per-table prune tests validate the SQL predicate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfAXBRMJ3KPuU3UEgwqjhS
Two fixes from adversarial review of the batched-delete change: 1. Add idx_wsd_expires on webhook_subscriber_deliveries(expires_at) (migration 059, CONCURRENTLY). It was the only one of the four retention tables lacking an index on its prune predicate — messages, webhook_events, and idempotency_keys all have one. Without it the batched ctid-LIMIT drain re-runs a seq scan per 5000-row batch, which on a backlog of wide event_payload rows approaches quadratic I/O — inverting the change's goal for exactly the table it should help. With it every batch is an index scan. 2. Override MaintenanceWorker.Timeout to 5 minutes (was River's 60s default). The batched deletes are ctx-aware so a cut is always safe (partial progress persists, next tick resumes), but 60s is tight for a first-deploy backlog where `messages` (pruned first) can eat the whole budget and starve the later tables. Bounded, not -1 — the janitor's deletes are safe to interrupt, unlike the HITL send-sweep. Also refreshed the RegisterJobs docstring: the unbounded-DELETE tradeoff it documented as "a separate follow-up" is now resolved by this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfAXBRMJ3KPuU3UEgwqjhS
jiashuoz
added a commit
that referenced
this pull request
Jul 9, 2026
…ms (#400) * chore(webhookpub): remove dead code + fix docstring drift Readability pass on webhookpub (no behavior change): - Delete committed AI-scratchpad text + 3 no-op `var _` compile guards in worker.go, and the now-unused errors/encoding-json/pgconn imports. - Delete unused `nowUTC` var (+ its time import) in outbox.go. - Delete the dead `oldest` telemetry scaffolding in Tick (declared, ranged into nothing, discarded). - Fix docstring drift: writeOutboxRow's "(eventually) PublishBestEffortTx" (already wired) + acknowledge that in river mode nothing LISTENs the NOTIFY; PublishBestEffortTx's reference to a webhook_publish_failures table that was never built + the "legacy-fallback" line that contradicts the file's own "no legacy fallback path left"; the pg_notify block now notes it's a harmless no-op in river mode. - Rewrite the event.go package header to describe the current durable three-layer outbox→fan-out→River pipeline (it still described the retired in-process post-commit-goroutine publisher + a tmp/ design path). - Drop stale internal "slice-N" markers; note both fan-out engines share fanOutEventCore in the matches/Publisher docs. - Standardize the fan-out job Kind() "webhook_fan_out" → "webhook_fanout" to match webhook_fanout_reconcile + the rest of the "fanout" naming (safe: E2A_WEBHOOK_FANOUT_MODE defaults legacy, so no such jobs exist in prod yet). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfAXBRMJ3KPuU3UEgwqjhS * refactor(jobs): hoist the copy-pasted reconciler into jobs.ReconcilePending The stranded-row reconciler was duplicated across five packages (outboundsend, inboundprocess, hitlnotify, webhookdelivery, webhookpub fan-out) — ~240 lines with byte-identical control flow: scan `WHERE <pred> AND <job_col> IS NULL LIMIT N` → per-row `FOR UPDATE` re-check → skip-if-set → EnqueueXTx → stamp the job id. Only the table/predicate/job-column/log-prefix varied. Extract a single `jobs.ReconcilePending(pool, spec, enqueueTx)` + `jobs.ReconcileSpec` + `jobs.DefaultReconcileBatch`. Each package's ReconcilePending collapses to a spec literal + one call; the domain method signatures are unchanged, so every caller (main.go cutover + each ReconcileWorker) is untouched. Behavior-identical: the two packages that stamped via a store method (inboundprocess/hitlnotify) stamped with the exact same `UPDATE <table> SET <job_col>=$2 WHERE id=$1` the helper now inlines; those store methods stay for the accept-tx path. Log output (prefixes) is preserved verbatim. The interpolated identifiers are compile-time constants (documented in ReconcileSpec) — the row id is always a bound param. Net ~240 → ~40 shared lines, and the reconcile invariants (FOR UPDATE re-check, IS NULL guard) now live in one hot-path-tested place instead of five. Each package keeps its own reconcile_test.go — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfAXBRMJ3KPuU3UEgwqjhS * chore(jobs): document Timeout choices + small consistency polish Readability polish across the River worker packages (no behavior change), from the modularity review: - Document the deliberate reliance on River's 60s default JobTimeout on the four per-message workers that DON'T override Timeout() (SendWorker, DeliverWorker, InboundProcessWorker, NotifyWorker) + RetentionWorker — so the omission reads as a decision, not an oversight (the repo has an explicit "River JobTimeout trap" lesson). InboundProcessWorker notes the content screen is bounded by piguard's own 5s per-detector timeout; RetentionWorker notes its single unbounded DELETE is ctx-safe under a 60s cut and flags batching (like #397) as a future option. - jobs.New: pass Config to defaultQueueConfig(cfg) instead of six positional ints (removes an invisible-transposition hazard). - Have RegisterJobs call the NewMaintenanceWorker constructors (webhookdelivery, hitlworker, janitor) instead of rebuilding the struct literal by hand — the constructors exist precisely so tests build an identical worker; now RegisterJobs can't drift from them. - outboundsend: name the markFailed terminal-write retry magic numbers (terminalWriteRetries / terminalWriteBackoff); drop landed "(slice B/C/D)" internal markers from comments. - hitlworker: "the legacy publisher" → "the webhook publisher" (the wired OutboxPublisher is the current outbox path, not the retired one). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfAXBRMJ3KPuU3UEgwqjhS --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
The hourly janitor (
internal/janitor, PR #394) ran a single unboundedDELETE ... WHERE expiredper retention table. On the first sweep of a backlog that means one long row-lock + a large WAL/xact footprint — rough on a prod-sizedmessagestable and the traffic-scaled webhook/idempotency tables.This drains each prune in ctid-bounded chunks (
LIMIT 5000) instead, looping until a short batch:Each statement is a short transaction (vacuum/snapshot horizon advances instead of being pinned by one long DELETE) and commits incrementally, so a mid-sweep timeout leaves durable progress. The caller's
ctxbounds total runtime and a partial sweep resumes next hour (the delete is idempotent). Same net effect, gentler on Postgres.Scope
Batched the four traffic-scaled tables (retention predicates unchanged, incl.
webhook_events's load-bearingstatus <> 'pending'guard):messagesidentity.DeleteExpiredMessageswebhook_eventswebhookpub.DeleteExpiredWebhookEventswebhook_subscriber_deliverieswebhook.DeleteExpiredSubscriberDeliveriesidempotency_keysidempotency.Store.SweepBounded tables (
user_sessions, legacywebhook_deliveries, oauth) keep their single-statement DELETE — nothing to gain.Review-driven additions (2nd & 3rd commits)
Two adversarial reviews flagged these; both fixed here:
idx_wsd_expires(migration 059, CONCURRENTLY).webhook_subscriber_deliverieswas the only one of the four tables with no index on its prune predicate (messages,webhook_events,idempotency_keysall have one). Without it the batched drain re-runs a seq scan per batch — approaching quadratic I/O on a backlog of wideevent_payloadrows, inverting the change's goal for the exact table it should help. With it every batch is an index scan.MaintenanceWorker.Timeout→ 5 min (was River's 60s default). The batched deletes are ctx-aware so a cut is always safe, but 60s is tight for a first-deploy backlog wheremessages(pruned first) can eat the whole budget and starve the later tables. Bounded, not-1— the janitor's deletes are safe to interrupt (unlike the HITL send-sweep).Tests
TestDeleteExpiredMessages_MultiBatchdrives the drain loop across 3 batches (batch size shrunk to 2 via a test-onlyexport_test.gohook, 5 expired rows) and asserts every expired row is deleted and a non-expired row survives — guarding the termination condition (RowsAffected < batch) and the expiry predicate.webhook,webhookpub,idempotency,janitor) validate the SQL predicate under the new batched statement — all green.EXPLAINpicksidx_wsd_expiresfor the prune predicate.Last optional follow-up after the River-migration sweep (#391–#396). No behavior change for callers; the janitor sees the same total-deleted count.
🤖 Generated with Claude Code