feat: sweep expired data with an in-process retention worker - #33
Merged
Conversation
…used V001's idx_oauth_authorizations_expires_at is partial on WHERE is_used = FALSE, so it cannot serve a sweep that deletes expired codes regardless of redemption — and a redeemed code is exactly the row it excludes, which is also the common case. Without a full index the hourly retention pass degrades to a sequential scan of the table. The other three retention targets already have usable indexes: oauth_access_tokens and audit_logs are indexed on expires_at / created_at, and idx_oauth_refresh_tokens_expires_at is partial on WHERE revoked_at IS NOT NULL, which is precisely the refresh-token delete condition.
…y lock Four batched deletes covering expired authorization codes, access-token metadata, rotated-away refresh tokens and aged-out audit logs. Each returns its row count so the caller can sweep until a pass comes back short, and each is restricted to a primary-key subquery: an unbounded DELETE on a long-neglected table would hold row locks for the length of one statement while sharing a pool with live request traffic. Two delete conditions are deliberately narrower than the obvious form: - Refresh tokens keep every family's sequence-0 row. That row is what FindFamilyOriginCreatedAt reads to date an ID Token's auth_time, and it carries revoked_at from the first rotation onward, so the naive condition deletes it while the family is still rotating. A family that keeps rotating outlives the origin row's own expires_at; losing it makes the refresh flow revoke the family and answer 500 — a forced logout for an active user. - Access-token metadata is swept on a window the caller sets far wider than the token TTL, because the auth middleware reports an unknown JTI with the same 401 it uses for a revoked one. The advisory lock pins one *sql.Conn for its lifetime. The lock is session-scoped while a pooled *gorm.DB hands out an arbitrary connection per statement, so unlocking through the pool could run pg_advisory_unlock on a connection that never held it — which returns false rather than erroring, leaving the real holder locked until its connection is recycled and blocking every later sweep on every instance.
Production PostgreSQL has no pg_cron and installing one needs shared_preload_libraries plus a database restart. Scheduling in Go also keeps the retention rules under test: the Testcontainers suite runs postgres:16-alpine, which cannot load pg_cron at all, so a pg_cron implementation would ship with no CI coverage. This is not a new pattern for the repo — sessionworker.TokenBlacklist already cleans token_blacklist_outbox from an hourly ticker, so token_blacklist_outbox is deliberately absent here rather than being swept by two competing cleaners. Each tick takes pg_try_advisory_lock and skips the round when another instance holds it: retention missing a tick is harmless because the next one covers the same rows, so failing fast beats queueing behind the winner. Sweep failures are logged and abandoned until the next tick rather than returned, since a Run error would take the API process down over a storage problem. One tick is capped at 20 batches per table so a months-old backlog drains across ticks instead of monopolizing the pool.
…floors Six RETENTION_* variables, wired into the worker at startup and passed through docker-compose — whose environment block is a whitelist, so a variable omitted there cannot be overridden from .env at all. Two windows are floors rather than free knobs, checked at boot so a misconfiguration fails startup instead of surfacing later as user-visible breakage: - RETENTION_ACCESS_TOKEN_AGE may not be shorter than JWT_ACCESS_TOKEN_EXPIRY. The auth middleware answers an unknown JTI with the same 401 it uses for a revoked token, so deleting metadata while its JWT is still inside exp presents a merely expired token as revoked: the client reads a forced logout where it should have refreshed. There is no clock-skew leeway in the JWT verifier to lean on, hence the wide 24h default. - RETENTION_AUDIT_LOG_AGE is bounded below by a 30-day sanity floor. The default remains the 90 days PRD §9 targets, but audit history here is operational rather than compliance-bound, so trimming below 90 is a legitimate choice; the floor only rejects values so short that an incident investigation would find the entries already deleted.
The pg_cron section was a design sketch that no migration ever created, so the docs promised cleanup that did not happen. Rewrite it to describe what now runs, and record why pg_cron was dropped: production lacks the extension, and postgres:16-alpine cannot load it, which would leave the rules untested in CI. Also corrects three claims the implementation contradicts: - The proposed refresh-token condition would delete each family's sequence-0 row and force-log-out active users; the guard and its reason are now documented. - The 1h access-token window is documented as 24h, with the middleware behavior that requires the margin. - token_blacklist_outbox is removed from the cleanup list, because sessionworker.TokenBlacklist already owns it. Audit retention is described as a 90-day default with a 30-day floor rather than a hard 90-day minimum, and PRD §11 marks the item done.
s3loy
approved these changes
Aug 1, 2026
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
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 this adds
Expired rows in
oauth_authorizations,oauth_access_tokens,oauth_refresh_tokensandaudit_logswere never deleted — the cleanup design existed only as pg_cron SQL indocs/psql-db-design.md, and no migration ever created it. This adds the cleanup as an in-process Go worker.internal/worker/retention.go— ticker worker, one round per hour by defaultinternal/repository/retention.go— four batched deletes plus advisory-lock acquire/releasemigrations/000006_*— one index, no data changesRETENTION_*variables with startup validationWhy not pg_cron
The production database has no pg_cron extension, and installing one requires editing
shared_preload_librariesand restarting the database.The stronger reason is test coverage: the integration suite runs
postgres:16-alpine, which cannot load pg_cron at all. A pg_cron implementation would ship with its retention rules untested. Scheduling in Go also follows an existing pattern rather than introducing a second one —sessionworker.TokenBlacklisthas been cleaningtoken_blacklist_outboxfrom an hourly ticker since V004.The cost: cleanup only happens while the API process is alive. When the service is fully stopped nothing is written either, so nothing is lost.
Two delete conditions that are narrower than they look
Both are cases where the straightforward
DELETE WHERE deadbreaks a live auth flow. Each has a test pinning it.Refresh tokens keep every family's
sequence-0 row.FindFamilyOriginCreatedAtreads that row to date an ID Token'sauth_time, and the row carriesrevoked_atfrom the first rotation onward — sorevoked_at IS NOT NULL AND expires_at < cutoffmatches it while the family is still in use. A family that keeps rotating outlives the origin row's ownexpires_at; deleting it sendsoauth.Service.refreshdown its "metadata inconsistent" branch, which revokes the whole family and returns 500. That is a forced logout for an active user.TestRetentionKeepsAuthTimeReadableAcrossRotationsrotates three times, sweeps between each, and asserts the origin row stays readable.Access-token metadata is swept 24h after expiry, not 1h. The auth middleware answers an unknown JTI with the same 401 it uses for a revoked token, so metadata deleted while its JWT is still inside
exppresents a merely expired token as revoked: the client receivesCodeAccessTokenInvalidinstead ofCodeAccessTokenExpiredand reads a forced logout where it should have refreshed. There is no clock-skew leeway in the JWT verifier to rely on. Validation also refuses any value belowJWT_ACCESS_TOKEN_EXPIRY.Multi-instance coordination
Each round takes
pg_try_advisory_lockand skips when another instance holds it.tryrather than a blocking lock is deliberate: a missed round is harmless because the next one covers the same rows, so returning immediately beats queueing behind the winner. Every delete isDELETE WHERE already-deadand therefore idempotent — the lock saves duplicate scans, it is not what makes the sweep correct.The lock pins one
*sql.Connfor its lifetime. It is session-scoped while a pooled*gorm.DBhands out an arbitrary connection per statement, so releasing through the pool could runpg_advisory_unlockon a connection that never held the lock — that returnsfalseinstead of erroring, and the real holder would stay locked until its connection happened to be recycled, blocking every later sweep on every instance.Pool and backlog safety
The worker shares its connection pool with live request traffic, so each delete is restricted to a primary-key subquery with
LIMIT, and one round is capped at 20 batches per table. A table neglected for months drains across several rounds instead of monopolizing the pool in one. Hitting the cap logs a warning, so a permanent backlog does not read as a clean sweep.Failures are logged and abandoned until the next tick rather than returned from
Run: retention falling behind degrades storage, while a returned error would take the whole API process down with it.The index
idx_oauth_authorizations_expires_atis partial onWHERE is_used = FALSE, and the sweep deletes expired codes regardless of redemption — a redeemed code is exactly the row that index excludes, and it is the common case. V006 adds a fullexpires_atindex so the hourly pass is not a sequential scan. The other three tables already have usable indexes; notablyidx_oauth_refresh_tokens_expires_atis partial onWHERE revoked_at IS NOT NULL, which matches the refresh condition exactly.Configuration
Windows are measured back from now, so a row must have been dead for the whole window; the margin absorbs clock skew between the API and PostgreSQL.
RETENTION_INTERVAL1h1mRETENTION_BATCH_SIZE1000RETENTION_AUTHORIZATION_AGE1hRETENTION_ACCESS_TOKEN_AGE24hJWT_ACCESS_TOKEN_EXPIRYRETENTION_REFRESH_TOKEN_AGE24hRETENTION_AUDIT_LOG_AGE2160h(90d)Audit retention keeps the 90 days PRD §9 targets as its default, but the bound is a 30-day sanity floor rather than a hard 90-day minimum: audit history here is operational, not compliance-bound, so trimming below 90 is a legitimate deployment choice. The floor only rejects values so short an incident investigation would find the entries already deleted.
token_blacklist_outboxis deliberately absent from this worker —sessionworker.TokenBlacklistalready owns it, and a second cleaner would just contend on the same table. The pg_cron design listed it because that design predates the outbox worker.Tests
Runvalidation, sweep-before-first-tickTestRetentionTryLockIsExclusive— a second holder appears only afterUnlockTestUpCreatesLatestSchemaasserts the V006 index existsgolangci-lint run ./...reports 0 issues. Full suite passes with-race -shuffle=on.Deployment note
V006 creates an index and touches no data. Production migrations still go through
docs/runbooks/database-baseline.mdand the explicitmigrate up --confirm-productionform; this PR does not run it anywhere.