feat(auth): prune spent one-time codes on a schedule (RUK-296) - #6
Merged
Conversation
Adds otp.prune and otp.prune.cron plus ProcessorTaskPayloadOTPPrune, and the task_processor.otp_prune config block with validateOTPRetention. The two task-type registries are guarded differently and both entries are required: the runtime startup guard reads ActiveProcessorTaskTypes, while the drift test's length assertion reads allDeclaredTaskTypes. The validator checks only for a negative retention, deliberately. A comparison against the OTP TTL was considered and dropped: a code becomes sweep-eligible only once its expires_at is already past, so no positive retention can reach a live row however small -- and services/otp imports this package, so calling otp.TTL from config would not compile anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-296) PruneOTPExpiredBefore is one batch-bounded DELETE of kind='otp' rows past expires_at, ordered by the indexed column so the partial index serves the scan directly rather than through a backward one. Age is measured on expires_at rather than created_at because auth_credentials_otp_expiry_idx is the only index on the table and migrations were out of scope; the two differ by exactly the TTL for a one-time code. consumed_at never enters the predicate -- a consumed code carries an expiry too and ages out through the same sweep. The kind='otp' conjunct is load-bearing twice and must not be simplified away on the grounds that password rows have a NULL expiry: no CHECK ties kind to a NULL expires_at, so that is an observation about today's write paths rather than an invariant, and the literal is also what lets Postgres use the PARTIAL index at all. Two fixtures are new because the existing helpers could not express the rows these tests need -- makeOTP hardcodes now+10m, and makePassword sets no expiry. makePasswordExpiringAt in particular builds a shape no write path produces today, and it is the only fixture that can prove the kind guard does anything: a NULL-expiry password row survives whether or not the guard exists. Every cutoff here stays in the past by design. The DELETE is bounded by expires_at and limit alone -- never by user_id -- so under `-p 2` a sweeping cutoff would eat fixtures belonging to other packages, and the bad outcome is not a flake but a silent green in services/otp's TestVerify_ExpiredCodeIsRetired, whose "row is gone" assertion an eaten fixture also satisfies. Mutation-verified: dropping the kind conjunct fails SparesPasswordWithExpiry and only that test; replacing the cutoff comparison with IS NOT NULL fails SparesLiveCode at its 1-minute leg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Prune drains the store's batch behind a single cutoff, mirroring invitation.Service.Prune, with two deliberate differences. The retention fallback is 24h, not the sibling's 365 days: the direction of safety is inverted here, because a year-long fallback would silently disable the sweep and let code digests and session nonces pile up -- exactly what this job exists to prevent. And the completion line is logged unconditionally, including a zero-row sweep, because this job carries no metric and that line is its only liveness evidence; logging only non-empty sweeps would make a cron that stopped firing indistinguishable from one with nothing to collect. Coercing a non-positive retention is the safety guard rather than hygiene. The per-code attempt ceiling is enforced by the row's continued existence -- claimSlot refuses to issue while it finds a live row with attempts exhausted -- so deleting such a row would hand back a fresh code with a fresh counter and turn "five attempts per code" into "five attempts per code, unlimited codes". Only a non-positive retention can place the cutoff at or after now, so the coercion is what closes it, and it runs before the cutoff is computed. The assertion behind it is unreachable by construction and kept as a guard against a future edit; the fallback must not be narrowed to make it firable. Also adds the generated mock for services/otp, which the Makefile already declared but no commit had produced. Mutation-verified: narrowing the fallback to `retention == 0` fails CutoffIsAlwaysInThePast; removing the batch cap fails StopsAtBatchCap; re-adding the sibling's `if total > 0` fails LogsCompletionEvenWhenNothingDeleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The processor package (day-bucketed external id, typed processor, cron factory), its registration in the goque worker, and the otp_prune block in all four stands. The cron spec falls back to "15 3 * * *" instead of failing to build the job. The sibling sweeps abort startup when their spec is missing, which in this merged auth+maintmode process means an absent config line takes sign-in down; falling back the way license.heartbeat already does keeps that to a degraded schedule, while a malformed spec still surfaces as an error. The 03:15 offset also keeps this sweep out of the 03:00 minute the three other daily jobs share. Retention is 24h in config, matching the service fallback and the factory's cmp.Or default -- three values that must agree, and the factory test asserts the literal rather than only its local constant so they cannot drift apart silently. The YAML comment deliberately does not repeat the siblings' "must be set or NewTaskProcessors panics" warning: with the fallback in place that sentence is false for this block. It documents the ~48h worst-case residency instead, since retention alone reads as a tighter guarantee than the daily cadence gives. registerOTPPrune is extracted for symmetry with registerInvitationRotation, not for funlen headroom. The registration test covers what the entity drift test structurally cannot: adding otp.prune to both task-type lists while never registering the periodic job satisfies every assertion in that file. Mutation-verified -- dropping the RegisterPeriodicJob call or the cmp.Or fallback each fail a test. Verified by booting the binary against local config with no otp_prune block: both goque-processor-otp.prune and goque-periodic-job-otp.prune.cron start and the startup coverage guard passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruko1202
changed the title
The guard read the clock a second time, so it compared the cutoff against a later instant than the one the cutoff was derived from. What it actually tested was "retention exceeds the gap between two clock reads" -- tens of microseconds -- rather than "the cutoff is in the past". That makes it useless for the single regression it was added to catch. If the retention fallback were narrowed to `retention == 0`, a zero retention would produce a cutoff a few microseconds behind now, the branch would evaluate false, and the sweep would proceed exactly as if the guard were absent. Its `return nil` also reported success while doing nothing. A guard that looks like protection but is not is worse than no guard, because the next reader trusts it instead of the coercion that does the work. The regression stays covered by TestPrune_CutoffIsAlwaysInThePast, which asserts on the cutoff the store is actually handed across negative, zero and positive retentions -- verified still dead by re-running that mutation after this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The registration tests called registerOTPPrune directly, which left the wiring itself untested: removing the registerOTPPrune call from NewTaskProcessors kept all three green while the sweep would silently never run in the binary. Nothing else covered it either -- the entity drift test only reads the task-type maps, and no test called NewTaskProcessors at all. Verified by mutation: with the call removed the new test fails on "missing=[otp.prune otp.prune.cron]" while the helper-level test still passes. Zero-valued Stores and Services are enough to drive it, so the real constructor needs no database. It also exercises the startup coverage guard, which until now was only ever checked by booting the binary by hand. In the processor test, drop `require.NotNil(task)` after a nil error and `require.NotEmpty(task.ExternalID)`: both are goque echoing its own arguments, and the external id's real value is pinned by TestOTPPruneExternalID_DayBucketed. The task-type assert is KEPT and now carries a comment saying why -- mutating the factory to stamp the .cron type is otherwise undetected, and such a task would sit in the queue with no processor to drain it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop the cmp.Or fallback and the defaultOTPPruneCronSpec constant: the spec now comes straight from config, and an empty or malformed value fails NewTaskProcessors and aborts startup, exactly as it does for audit.prune and invitation.prune. The fallback was defending against a stand whose config predated this change, but that trade was wrong: a value that is set but malformed should fail loudly rather than be silently replaced, and a sweep running on a schedule nobody configured is harder to notice than a process that refuses to start. All four stands set the value, so the failure mode is reachable only by deleting a line that is already there. Also removes otp_prune_registration_test.go. Its remaining tests either called registerOTPPrune and then asserted that registerOTPPrune had registered something -- a tautology -- or duplicated what the constructor-level checks already cover. The YAML comments no longer promise a fallback that does not exist; they now carry the same "must be set" warning as the neighbouring blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
validateOTPRetention is a four-line negative check whose sibling, validateInvitationRetention, ships without a test of its own. Matching that is the more consistent choice. Nothing safety-relevant rides on it: the behaviour that matters -- a non-positive retention never reaching the DELETE -- is pinned at the service level by TestPrune_CutoffIsAlwaysInThePast, where it is actually reachable. The startup validator is a config-hygiene message layered on top of that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing referenced it, neither of the two shipped periodic sweeps has an equivalent, and the only mention of runbooks anywhere in the repo is a comment in alerts.yml phrased as "if you keep operator runbooks of your own" -- which reads as an invitation, not as an established practice. It also carried facts that duplicate the config and the code comments (the schedule, the retention window, which rows the predicate spares), so it was set up to drift out of date the first time either changed. Dropping it here rather than extending it with an otp.prune section, which is what an earlier commit on this branch did before the file's place in the repo was questioned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ruko1202
force-pushed
the
feat/otp-prune-processor
branch
from
September 5, 2026 18:57
05d5d52 to
3abbead
Compare
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.
auth_credentialsgains a row per sign-in attempt and nothing ever removedthem, so code digests and browser-binding nonces accumulated indefinitely.
RUK-285 landed the index for this sweep ("supports the periodic prune of
expired codes, which is a separate piece of work") but the work was never
scheduled. This is that work, mirroring the shipped
audit.pruneandinvitation.prunejobs.otp.prune: a daily cron job deletingkind='otp'rows whoseexpires_atis older than a 24h retention window, in bounded batchesexpires_at, notcreated_at— the partial index is the onlyone on the table and migrations were out of scope;
EXPLAINconfirms it isused with no sort node
kind='otp'explicit: nothing in the schema tieskind='password'to aNULL expiry, and the literal is also what makes the partial index usable
a cutoff at or after
nowwould delete a live attempt-exhausted code andreset the per-code guess ceiling
claimSlotenforces by that row's existenceincluding empty ones, since this job has no metric and the log line is its
only liveness signal