fix(api): bound the internal sweep rail in time and make each sweep single-flight by axelhamil · Pull Request #63 · axelhamil/clean-stack

axelhamil · 2026-08-29T20:55:05Z

Closes the D.5 residual debt from ROADMAP.md: the /internal/sweep-* retention rail had no time bound anywhere, and nothing stopped two runs of the same sweep from overlapping.

Three live bugs, found by measuring rather than reading the docs

Bun.serve defaults idleTimeout to 10 seconds and enforces it while the fetch handler runs. Measured in this repo:

handler silent for 15s, idleTimeout unset  -> socket closed at ~10s
stream writes a byte, then silent for 25s  -> socket closed at ~10s

Every sweep past 10s had its socket closed under it while the work kept running server-side. The cron saw a network error and the next tick started a second concurrent sweep.
GET /notifications/stream was broken in production. Its heartbeat is 25s against that 10s timeout, so every SSE connection was dropped and reconnected every 10 seconds.
sweep-notifications was mounted but never called — absent from the bundled cron's list, so read notifications were never purged.

What ships

Three nested deadlines, each strictly shorter than the one wrapping it, so the innermost always wins and the caller gets a real HTTP response instead of a dropped socket: SWEEP_DEADLINE_MS (90s) < SERVER_IDLE_TIMEOUT_SECONDS (120s) < INTERNAL_FETCH_TIMEOUT_MS (150s). Env parsing refuses a configuration that inverts them — a bad .env fails the boot rather than silently reproducing the original bug. Traced worst case: 90s budget + one bounded batch (~15s) ≈ 105s, inside the 120s socket.

A single-flight lease (sweep_lock). Bounding a run never removed the concurrency it was meant to fix — two overlapping sweeps just split the SKIP LOCKED rows at double the pool cost. One time-boxed lease per label, taken in a single conditional upsert so a race has exactly one winner, fenced by an owner token so a run that overruns its TTL cannot delete its successor's lease, and expiring on its own so a crashed run frees the label instead of wedging it the way a pooled advisory lock would. Columns are timestamptz on purpose: this is the one place in the repo comparing an application-written timestamp against Postgres now(), and bare timestamp would silently void mutual exclusion under a non-UTC TZ.

Named stop reasons. stopReasons distinguishes exhausted / budget / batch-cap / batch-error; truncated derives only from the first two. Collapsing them into one flag made a recurring data error read as a healthy backlog.

A cron that reads its own responses — JSON.parse through a tested pure classifier instead of substring-matching, an AbortSignal.timeout on signedInternalFetch, distinct handling for an unreachable API, a held lease, a recurring batch error and an honest backlog, and it accumulates failures rather than exiting mid-loop (fail-fast starved the five sweeps after the failing one).

Two holes the rest of the work exposed: no countEligible had a statement timeout, so raising the socket timeout would have made the unbounded dry-run count twelve times worse — now bounded at 10s via a shared helper. And rgpd-sweep was bounded in rows but not in time: it loops a full account wipe per row. It now carries the same budget, checked between wipes and never inside one, because a half-erased account is the one outcome an Art. 17 sweep must never produce.

A race the docs claimed did not exist. While documenting rgpd-sweep's deliberate lack of a lease, the claim that each wipe "claims its own row and is idempotent" turned out false on both halves — no SKIP LOCKED on the select, no deletedAt IS NULL guard on the update. Two overlapping runs could emit ACCOUNT_WIPED twice, duplicating an audit row, a webhook and an S3 delete. The guard now makes the claim true.

Safety net

check:sweep-lock and check:fanout are now wired into CI. The lease's conditional upsert had no automated gate — dropping setWhere would have disabled single-flight entirely and still gone green. check:fanout also turned out to exit 0 unconditionally, so it was a gate that could not fail; it now has a real exit code, proven by breaking an assertion and observing a non-zero exit.

Production now also rejects the .env.example placeholder values for INTERNAL_SIGNING_KEY and BETTER_AUTH_SECRET — both were long enough to satisfy the existing length guards, so a deployment could have booted with a signing key published in this repository.

Migration note

0020 was amended in place rather than stacked, which is safe because it is unreleased — dev and main never saw the old hash, and a fresh clone is unaffected. A database that already applied the old 0020 via db:migrate will replay the new tag and fail on relation "sweep_lock" already exists; it needs DROP TABLE sweep_lock or a clean re-migrate. db:push users are unaffected. Also documented in docs/CRON.md.

Known debt, deliberately deferred

No instrumentation spans anywhere on this rail — not the runner, not the six purgeBatch, not the lease. Instrumenting only the lease would produce an incoherent trace. Recorded in ROADMAP.md.
The dry-run path bypasses the budget, and its 10s count timeout raises a 500 rather than truncating. Diagnostic-only; the cron never dry-runs.
An alreadyWiped loser still counts in succeeded, double-counting one wipe across two overlapping runs. Observability only.

Verification

776 tests pass, type-check clean, Biome clean, jscpd 1.22% (gate 3%), knip unchanged (POLICY_TYPES pre-existing). check:sweep-lock 23/23 against a real Postgres.

https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

runBatchedSweep looped up to MAX_BATCHES with no time bound, so a backlog could hold a request open well past any socket timeout. The budget is one absolute deadline per request, checked between batches so an open transaction is never cut short, read through an injectable clock so tests do not sleep. Every early stop is now named: budget and batch-cap mean come back next tick, a batch error does not, and folding them into one flag made a recurring data error read as a healthy backlog. The inter-batch sleep is also injectable now (defaulting to Bun.sleep), so the batch-cap test can drive the loop through all 1000 iterations without a real ~50s wait. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Bun.serve defaults idleTimeout to 10s and enforces it while the handler runs, so every long response was being cut. Measured here: a handler silent for 15s loses its socket at 10s, and so does a stream that writes once then waits 25s — which is exactly the notification SSE heartbeat, dropped and reconnected every 10s in production until now. The three bounds must nest, so env parsing refuses a configuration that inverts them. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…ng constants The two "idle timeout > sweep budget / SSE heartbeat" cases in server-options.test.ts called buildServerOptions with a literal the test itself supplied, so against a passthrough function they asserted arithmetic over two constants and could never fail. Extract the superRefine comparisons in env.ts into an exported validateEnvBounds so a new env-bounds.test.ts can drive the real guard with plain literals — accepted defaults, and both bounds rejected at the equality boundary and past it — without importing env (which parses process.env at import time and would require a full .env). Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Bounding a sweep in time never removed the concurrency it was meant to fix: nothing stopped a second tick, or an operator, from starting the same sweep while one was running, and the two just split the skip-locked rows between them at double the pool cost. sweep_lock holds one time-boxed lease per label, taken in a single conditional upsert so a race has exactly one winner, and expiring on its own so a crashed run frees the label instead of wedging it the way a pooled advisory lock would. The lease's conditional UPDATE only proves itself against a real Postgres — a mocked tx never evaluates a WHERE. Every other test file in this suite mocks @packages/drizzle wholesale, and bun's mock.module is process-wide and irreversible for the rest of a `bun test` run, so the lease check lives as its own script (`pnpm --filter api check:sweep-lock`), the pattern already established by check-fanout-preferences.ts. sweepSchema is exposed as the real, connection-free table definition (not a fake stand-in) in every existing mock.module("@packages/drizzle", ...) block, so a route file that transitively imports sweep-lock.ts still loads cleanly under those mocks. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…tamps Review fix round 1/5 on the single-flight sweep lease: - releaseSweepLease deleted by label alone, so a run that outlives its TTL could delete a legitimate successor's lease after that successor had already acquired it — the exact race the lease exists to prevent. Adds an owner column (uuidv7 token per acquisition); acquireSweepLease returns the token or null, releaseSweepLease requires it back, and a stale caller's release is now a no-op instead of a theft. - sweep_lock's timestamps were tz-naive while the acquire/release predicate compares against Postgres's now() — identical only as long as both run UTC. Both columns are now `with time zone`. Migration 0020 amended in place (not yet released) rather than stacked as 0021. - Each route now calls sweepLockFor(label) instead of repeating the same {acquire, release} + SWEEP_DEADLINE_MS * 2 literal six times; the token is closed over inside sweep-lock.ts so the runner's SweepLock shape is unchanged. - check-sweep-lock.ts gained coverage for the fencing behavior, for the label/TTL sweepLockFor produces, and for each of the six routes passing its own label (asserted off the runner's log line, since sweep-audit-log reshapes its response and drops `skipped`) — deleting a route's `lock:` wiring now fails the check. - lock.release() in sweep-runner's finally is now caught and logged so a release failure can't mask the real in-flight exception. - docs/FEATURES.md and docs/REMOVABILITY.md now name check:sweep-lock, per apps/api/src/shared/CLAUDE.md's script-pattern doc requirement. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

signedInternalFetch issued a bare fetch with no AbortSignal, so a wedged server left the cron hanging. It now carries an AbortSignal.timeout above the server idle timeout, so the server's answer wins the race and an abort really does mean unreachable. The cron parses the response instead of substring-matching it, tells a skipped lease and a recurring batch error apart from an honest backlog, and finally calls sweep-notifications — the route was mounted but absent from the list, so read notifications were never purged. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

The skipped/batch-error/truncated/ok decision lived inline in the cron loop, so a typo in a field name or a reordered if/else chain would have shipped silently — nothing exercised the branching that decides process.exit(1). Extracted classifySweepResult into its own module and covered the priority order (skipped beats a batch-error beats truncated) plus the sweep-audit-log shape (fields absent) and an empty stopReasons object. Also reused DEFAULT_INTERNAL_FETCH_TIMEOUT_MS instead of a second 150_000 literal. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

countEligible carried no statement timeout while purgeBatch has always set one, so the dry-run path was bounded only by the socket being closed under it — and raising the idle timeout to 120s would have made that worst case twelve times longer. sweep-audit-log also rebuilt its response by hand and dropped truncated, skipped and stopReasons on the floor, so the cron could never have reported them for that route. The six countEligible bodies were near-identical apart from table/predicate, so the bounded query is promoted into a single countEligibleWithTimeout helper (sweep-count.ts) generic over AnyPgTable/SQL, rather than duplicated six times. @packages/drizzle now also re-exports drizzle-orm's SQL type for that helper's signature. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

The sweep fetched one bounded batch and then looped a full account wipe per row — cascade deletes plus storage I/O, fifty times by default — so the endpoint was never bounded in time, only in rows. The budget is checked between wipes and never inside one: a wipe cut in half would leave a partially erased account, the one outcome an Art. 17 sweep must not produce. No lease is added here — each wipe claims and marks its own account row and is idempotent, so two overlapping runs cannot double-wipe, unlike the other six sweeps this rail bounded. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…ation processed still returned batch.length after the sweep gained a time budget, so a truncated run claimed accounts it had in fact deferred to the next tick. Before truncation, batch.length === succeeded.length + failed.length held as an invariant; truncation is exactly what breaks it. processed is now succeeded.length + failed.length in the loop's return path — the two early returns that never enter the loop (empty batch, dry run) are unaffected since batch.length is still correct there. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Rewrites task-7's planned prose to match what actually shipped rather than what the brief predicted: - The lease is fenced by an owner token (uuidv7 per acquisition), not just the label — releaseSweepLease deletes by label AND owner so an overrunning run's late release cannot steal back a lease a legitimate successor already acquired. - sweep_lock.locked_at/locked_until are timestamptz on purpose: it is the one table comparing an app-written timestamp against Postgres's own now(), unlike the bare timestamp used elsewhere in the schema. - The dry-run count is bounded at 10s via the shared countEligibleWithTimeout helper (sweep-count.ts); purgeBatch stays at 5s. - rgpd-sweep now carries the same time budget (checked between wipes, never inside one) and reports processed as accounts actually attempted, not the batch size — but deliberately has no lease. - The bundled cron now also calls /internal/sweep-notifications and classifies outcomes (including a non-zero exit on batch-error) via classifySweepResult. - Notes the boot-time refusal when SWEEP_DEADLINE_MS / SERVER_IDLE_TIMEOUT_SECONDS / INTERNAL_FETCH_TIMEOUT_MS aren't strictly ordered, and that the idle-timeout floor was raised because it was silently dropping GET /notifications/stream every 10s in production, not only sweep responses. - Adds a migration note in CRON.md: 0020 was amended in place (safe only because unreleased) to add the owner column and switch to timestamptz — a database that already ran the old 0020 needs `DROP TABLE sweep_lock` or a clean re-migrate before `db:migrate` will succeed again; db:push is unaffected. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

rgpd-sweep selects pending-deletion rows without SKIP LOCKED and holds no lease, so two overlapping runs can pick the same account. Guard the anonymizing UPDATE with `deletedAt IS NULL` and short-circuit (no USER_DELETED emission) when the update matches zero rows, so the losing run reports success instead of double-wiping and double-firing the audit/webhook side effects. Docs now describe the real mechanism instead of a false "no SKIP LOCKED needed" claim. The cron's INTERNAL_FETCH_TIMEOUT_MS parsing didn't guard against an empty string or a non-numeric value the way shared/env.ts does, so a blank/invalid override made every sweep abort instantly (Number("") === 0 -> AbortSignal.timeout(0)). It also ran as its own process with no boot-time check against the API's other two bounds, so raising the API's idle timeout without also raising the cron's fetch timeout produces a recurring false "UNREACHABLE" alarm — the exact failure mode this branch exists to remove. Documented the cross-process constraint in docs/CRON.md. Finally, a `batch-error` on the first sweep in the cron's loop called process.exit(1) immediately, starving every sweep after it (including sweep-notifications, last in the list) until a human intervened. Accumulate failures and exit at the end instead; UNREACHABLE stays fail-fast since a dead API makes continuing pointless. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

sweep-lock.ts evaluated the sweep lock schema at module scope, so any test file that mocked @packages/drizzle without exposing that export exploded on import — that forced 18 unrelated test files into a deep 5/6-level-relative import of the real schema module just to keep their mocks importable. Move the lookup inside acquireSweepLease and releaseSweepLease (first line of each) and drop the now-unnecessary import/mock entry from every file that never reaches the lease. Verified none of the 18 call acquireSweepLease/releaseSweepLease/ sweepLockFor/runRetentionSweep directly. Reduces future mock.module surface — this rail already shipped one regression (827df48) from an over-broad mock leaking process-wide. Also make the sweep runner's lock option required: all six real sweep routes already pass one, so an optional type let a seventh route forget it and still type-check. Updated sweep-runner.test.ts's calls that weren't exercising lease behavior to pass a shared no-op lock. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…tabase No CI job ran any check:* script, so the lease's conditional upsert (setWhere on the owner-fenced upsert), the timestamptz columns and the six routes' label wiring had no automated gate — a "simplification" that dropped setWhere would disable the lease entirely and still get a green CI. Add check:sweep-lock right after db:push (needs no seeded user) and check:fanout right after the seed step, since it looks up the seeded account by SEED_EMAIL and must match the seed step's default. Gave INTERNAL_SIGNING_KEY a real 32+ char placeholder in apps/api/.env.example so bootstrap produces a working key for both checks and for local dev, instead of the blank default that made signing throw. Also: corrected a stale comment in check-sweep-lock.ts claiming sweep-audit-log's handler drops `skipped` from its response — it spreads the full result, so the field survives there too; the log-line assertion is used uniformly across all six routes regardless. Added a warning that the script acquires leases under production route labels for up to 60s and must only run against a local database. Documented the sweep_lock event exception in the root CLAUDE.md (rule #6): a lease is coordination state, not business state, and the "What's left" table in ROADMAP.md now records that the sweep rail has no instrumentation spans — deferred deliberately since spanning only the lease would produce an incoherent trace. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Every assertion only console.log'd "ECHEC" — nothing touched the exit code, so the script always exited 0 and the CI gate added for it stayed green even when a scenario broke. Mirror check-sweep-lock.ts's check(label, ok) helper: accumulate failures and exit(failed ? 1 : 0). Renamed ECHEC to FAIL to match the sibling script. Verified locally by breaking one assertion (exit=1), then restoring it (exit=0). Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

apps/api/.env.example ships INTERNAL_SIGNING_KEY and BETTER_AUTH_SECRET as long-enough literal placeholders so bootstrap works out of the box for local dev and check:sweep-lock/check:fanout in CI. Both are long enough to pass their .min(32) schema check, so a production deploy that copies .env.example and forgets to replace one of them used to boot successfully with a secret published in this repository — INTERNAL_SIGNING_KEY authenticates every /internal/* route, i.e. the sweeps and RGPD account wipes. Reject the literal placeholder text in the NODE_ENV === "production" guard block, for both secrets, with a message telling the operator to generate a real one. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

check:fanout was pinned to [email protected] on its own step only because that happened to match seed-dev-user.ts's separate, independently-defined default — the comment claimed they "share the same default" when really they shared nothing but a coincidence across two files. Changing seed-dev-user.ts's default would silently break check:fanout with no visible link between the two. Declare the variable once at the a11y job's env level so both the seed step and the check:fanout step read the same value. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…n types runRetentionSweep still guarded opts.lock with a truthiness check (if (opts.lock && ...) / if (opts.lock) { release }) even though lock is now a required SweepLock. A caller reaching this from plain JS, or a test coercing with `as any`, could still skip the lease silently instead of a type error surfacing it. Drop the guards so a missing lock throws instead of quietly running unleased. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

An out-of-range INTERNAL_FETCH_TIMEOUT_MS (e.g. 500, below the API's own .min(1000) boot check) silently fell through to the 150000ms default with no signal — an operator trying to tighten the budget would unknowingly loosen it 300x instead. Log a console.warn naming the rejected value before falling back to the default. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

…ilure The previous fix only covered batch-error, which only ever fires on the two routes (sweep-audit-log, sweep-outbox) that declare an onBatchError returning "break" for FK violations. The other four routes — including sweep-email-messages, first in the list, and sweep-notifications, last — have no onBatchError, so they default to "throw" -> 500 -> !res.ok, which still called process.exit(1) mid-loop. One bad batch anywhere short of those two FK-guarded routes was starving every sweep after it, which is the exact failure mode this branch exists to remove. FAIL (!res.ok) and UNPARSEABLE now accumulate and continue, same as batch-error. TRUNCATED BODY (the body-read failing after a response was already received) is treated the same way: unlike the fetch call itself throwing, a truncated body means the API answered and the transfer dropped afterward — evidence of a flaky read for this one route, not that the API is down. Only UNREACHABLE (the fetch call itself throwing or aborting) still exits immediately: there, the API is plausibly down and every other route would fail identically, so giving up early is the right call. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Restoring INTERNAL_SIGNING_KEY to a placeholder value in .env.example was worse than the production-only guard added for it: before this branch, an empty key made requireInternalSignature return 503 on every /internal/* route, fail-closed everywhere. The placeholder made pnpm bootstrap ship a working signing key — published in this repository — into every developer's and every deployment's .env, authenticating /internal/rgpd-sweep (irreversible account erasure) among others. The production-only guard in shared/env.ts does not catch a staging or preview deployment left on the default NODE_ENV=development, which is routinely reachable. .env.example ships the var empty again (fail-closed by default). The placeholder only ever existed to let check:sweep-lock and check:fanout sign requests — that need now lives at the CI job's env level in ci.yml, scoped to the ephemeral CI Postgres and never touching a real deployment. Developers running these checks locally export their own key (documented in docs/CRON.md). Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

The two expired-lease scenarios used ttlMs=-1000, leaving only one second of margin against app-to-Postgres clock drift before the "already expired" assumption the test depends on could flake. Widen to -60_000 for a comfortable margin. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

Three additions found true by the independent PR review: - SHUTDOWN_GRACE_PERIOD_MS (15s) is a fourth bound that is deliberately not nested with the three sweep/idle/fetch bounds: a SIGTERM during a routinely 90-110s sweep fires well outside that window. No corruption (the transaction rolls back), but the lease is left behind and answers skipped for a tick or two until it expires — documented as accepted, not fixed. - SERVER_IDLE_TIMEOUT_SECONDS is a single process-wide Bun.serve setting: raising it from 10s to 120s raises the idle budget for every socket the API serves, not just the internal sweep rail — the cost of the SSE fix lands on the public surface too. - pnpm bootstrap never overwrites an existing .env, so anyone who bootstrapped before this branch is missing the new bound variables and INTERNAL_SIGNING_KEY; check:sweep-lock fails locally until they're added by hand. Listed which ones and their values. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY

github-actions · 2026-09-01T17:35:21Z

🎉 This PR is included in version 1.24.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

axelhamil added 23 commits August 29, 2026 20:44

axelhamil merged commit 19100aa into dev Aug 29, 2026
1 check passed

axelhamil deleted the fix/d5-sweep-timeouts branch August 29, 2026 21:10

axelhamil mentioned this pull request Sep 1, 2026

release: api tokens, notifications, SSO/SCIM, i18n and surface parity #67

Merged

github-actions Bot added the released label Sep 1, 2026

Navigation Menu

fix(api): bound the internal sweep rail in time and make each sweep single-flight - #63

fix(api): bound the internal sweep rail in time and make each sweep single-flight#63
axelhamil merged 23 commits into
devfrom
fix/d5-sweep-timeouts

axelhamil commented Aug 29, 2026

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

axelhamil commented Aug 29, 2026

Three live bugs, found by measuring rather than reading the docs

What ships

Safety net

Migration note

Known debt, deliberately deferred

Verification

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant