Repository navigation
feat(api): purge aged idempotency_records on the durable-job worker (#259) - #422
Conversation
…259) idempotency_records grew without bound: IdempotencyMiddleware inserts a claim per idempotent write and marks it Completed with the replay payload, but nothing ever deleted an old row. Add IdempotencyRecordPurgeSweep, a DurableJobWorker sweep modeled on RefreshTokenPurgeSweep (#270), deleting rows whose CreatedAt is older than a 48h retention window. - Keyed on CreatedAt (present on every row; CompletedAt is null while InProgress), with one flat window across all statuses. A row's only post-hoc value is replaying its Completed response to a client retry, and 48h clears any retry horizon by orders of magnitude; an InProgress row that old is an abandoned 30s lease the atomic protocol already treats as stealable, so deleting it just lets the next same-key request INSERT fresh. - One global batched delete, no tenant loop: idempotency_records carries AccountId but is not tenant-query-filtered, so a single delete prunes every account uniformly — the tenant-safe shape is the absence of scoping (same as RefreshTokenPurgeSweep). - Batched (500 x 20 per tick) so the first post-ship sweep of a months-long backlog can't monopolise the worker or spike the WAL; failures are contained and retried next poll, like the sibling sweeps. - Retention is a compile-time constant, not config: matches RefreshTokenPurgeSweep.PurgeGrace, Infrastructure does not reference the Api project where IdempotencyOptions lives, and 48h needs no per-deploy tuning. No index and no migration — per the measured #259 analysis, an unindexed batched scan is ~17ms at the synthetic 48h ceiling and the index would tax every write on the hot insert path. Integration tests cover aged (Completed and InProgress) collection, fresh/ boundary retention, tenant-safety + idempotency, backlog drain across the batch boundary, and that a replay after purge re-executes rather than wedging the key. Closes #259.
…421 review) The first cut purged every row with CreatedAt older than the window regardless of Status, on the reasoning that an aged InProgress row is an abandoned lease. That misses the steal path: IdempotencyMiddleware renews LeaseExpiresAt on an existing row WITHOUT touching CreatedAt, so a retry can hold a live lease over a claim created more than 48h ago. Purging it mid-flight would make the handler's guarded publish (UPDATE ... WHERE LeaseOwner = ours) match zero rows, roll back the business mutation, and return 409. Gate InProgress deletion on an expired lease: delete a row when CreatedAt < cutoff AND (Status = Completed OR LeaseExpiresAt < now). Completed rows still purge freely (their publish already committed). Race-safe under READ COMMITTED — a concurrent steal that renews the lease makes this DELETE re-evaluate and skip the row; a DELETE that wins the row lock first leaves the steal's conditional UPDATE matching nothing, so it falls back to a fresh INSERT. An abandoned InProgress row (expired lease) is still collected. Adds Sweep_LeavesAgedInProgress_WithALiveRenewedLease as the regression test.
review) Round-2 review caught that the single batched delete does not actually protect a renewed lease. EF lowers `Where(...).OrderBy(CreatedAt).Take(n).ExecuteDelete()` to `DELETE ... WHERE Id IN (SELECT ... ORDER BY CreatedAt LIMIT n)`, whose outer target is matched by id only. Under READ COMMITTED a steal that renews an expired aged claim's lease AFTER the subquery selects it but BEFORE the delete locks the row is not re-checked (the outer qual is the id, not the lease), so the now-live row is deleted and its handler's guarded publish then 409s and rolls back the business mutation. Split the purge by Status, because the two need different guarantees: - Completed rows (the unbounded bulk) are TERMINAL — nothing updates a row after publish — so no concurrent write can move one out of eligibility, and the batched OrderBy/Take form is safe. Kept batched. - InProgress rows are a single UNBATCHED, predicated DELETE with `LeaseExpiresAt < now` ON THE DELETE TARGET. With the guard on the target (no subquery), READ COMMITTED re-evaluates it against a concurrently-renewed row (EvalPlanQual) and skips it; a delete that wins the row lock first leaves the steal's conditional UPDATE matching nothing, so it INSERTs fresh. This set is inherently tiny (an InProgress row completes within its own request; only crashed-and-never-retried keys persist), so it needs no batching — and batching it would reopen the very race. Tests unchanged and still valid: aged Completed collected (batched), aged expired-lease InProgress collected (unbatched), aged live-lease InProgress retained, plus tenant-safety, backlog drain, and replay-after-purge.
#421 review) The prior InProgress test seeded an already-live lease, which the broken batched delete also passed (the row is excluded at selection time in both forms), so it did not actually guard the race. Add a test that exercises the real failure window: the row starts aged AND lease-expired so the sweep's InProgress DELETE selects it, a concurrent steal (held on its own connection with an open transaction) renews the lease and locks the row, and the DELETE blocks on that lock. The unbatched target-predicated DELETE re-checks LeaseExpiresAt via READ COMMITTED EvalPlanQual and skips the now-live row; the old `Id IN (subquery)` form re-checked only the id and would delete it, so reverting turns this test red. On the fixed code the row survives under every interleaving, so the 1s coordination delay never makes correct code flake — it only governs whether the broken form would be caught.
…reatedAt (#421 review) Round-4 review caught a purge that could delete a just-published response. The steal path renews a lease without touching CreatedAt, so a claim created >48h ago can be stolen and only now publish Completed. The Completed purge keyed on CreatedAt would then collect that row on the very next poll — potentially the same poll that just skipped its live InProgress lease — and a client retry after the deletion re-runs the business mutation. Key the Completed purge on CompletedAt instead: a result's retry window starts when the response was produced, not when the claim was created. CompletedAt is non-null exactly when Status is Completed (#307) and is immutable after publish, so the batched form stays terminal-safe. InProgress keeps CreatedAt (its CompletedAt is null) plus the expired-lease guard. Normal rows are unaffected (a claim completes within its own request, so CompletedAt ~= CreatedAt); only stolen-then-completed rows change, and they now stay replayable for the full window past completion. Updates the replay test to age CompletedAt (the new key) as well as CreatedAt.
…ted claim (#421 review) Every existing Completed test has CreatedAt == CompletedAt (NewRecord sets them equal; the replay test ages both), so reverting the Completed purge to the broken CreatedAt predicate left the whole suite green — the CompletedAt fix was unguarded. Add a Completed row created >48h ago but completed just now, and assert the sweep retains it: keying on CreatedAt would delete it, so this test goes red on that regression.
…d delay (#421 review) The coordinated race test used Task.Delay(1s) before committing the steal. On a slow/loaded host the sweep's DELETE might not have selected the row and blocked within that second, so committing early lets even the broken batched form see only the renewed lease — a false pass that silently stops guarding the regression. Replace the delay with a deterministic wait: poll pg_stat_activity until a session is blocked on a Lock while running a statement against idempotency_records (the sweep's DELETE waiting on the steal's row lock), then commit. Fails loud if the DELETE never blocks within the timeout.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f66fb76a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var deleted = await db.IdempotencyRecords | ||
| .Where(r => r.Status == IdempotencyStatus.Completed && r.CompletedAt < cutoff) | ||
| .OrderBy(r => r.CompletedAt) |
There was a problem hiding this comment.
On installations where idempotency_records has accumulated the months-long backlog this sweep is meant to drain, this Status/CompletedAt filter plus ORDER BY CompletedAt has no supporting index (the model still only indexes (AccountId, EndpointHash, IdempotencyKeyHash)), so every poll can seq-scan/sort the whole table up to 20 times before deleting 500-row batches. Please add a purge index, such as (Status, CompletedAt) or a partial completed-row index, and ship it as its own migration rather than leaving the new worker to scan the hot idempotency table.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already measured and declined, not overlooked — same suggestion was raised on #421 (r3712923694) and settled with data, not arithmetic: issue #259 comment synthesized the 48h steady-state backlog (105k rows / 33 MB, from a measured 0.607 rows/s k6 ceiling) and ran the batched delete both ways — unindexed 16.9 ms (full seq scan, worst case: no row old enough yet, so LIMIT never short-circuits) vs. indexed 0.062 ms, at a cost of 2.3 MB + write amplification on every idempotent write on the hot insert path. 17 ms once per poll interval on a background worker is free; taxing every write to buy that back is backwards.
This PR's description restates the same conclusion under "Retention is a compile-time constant, no index, no migration." No code change here — this is the same declined tradeoff, not a new finding.
|
@codex the purge-index P2 is addressed — replied on the thread (r3718154037): same suggestion already measured and declined on #421 (r3712923694), with data from #259 — unindexed 16.9 ms worst-case scan vs. indexed 0.062 ms at the cost of taxing every idempotent write on the hot insert path. No code change; please re-review. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #259.
idempotency_recordsgrew without bound —IdempotencyMiddlewareinserts a claim per idempotent write and marks itCompletedwith the replay payload, but nothing ever deleted an old row. This addsIdempotencyRecordPurgeSweep, aDurableJobWorkersweep modeled onRefreshTokenPurgeSweep(#270), deleting rows past a 48h retention window.Design (and why)
The purge is two deletes, split by
Status, because the two states need different concurrency and retention guarantees (this evolved across review — see the resolved threads):CompletedAt, notCreatedAt. A result's retry window starts when the response was produced; a claim created >48h ago can be stolen and only now publishCompleted, and keying onCreatedAtwould purge that fresh response and let a retry re-run the mutation.Completedis terminal (nothing updates the row after publish), so the batchedOrderBy/Takeform is race-free even though EF lowers it toDELETE ... WHERE Id IN (subquery).DELETE(Status = InProgress AND LeaseExpiresAt < now AND CreatedAt < cutoff). The steal path renewsLeaseExpiresAtwithout touchingCreatedAt, so an aged claim can hold a live, just-stolen lease; deleting it mid-flight makes the guarded publish match zero rows → rolled-back mutation → 409. Keeping the lease predicate on the DELETE target (no subquery) makes READ COMMITTED re-check it against a concurrently-renewed row (EvalPlanQual) and skip it. The batched subquery form would delete by id after the renewal — which is why InProgress is unbatched (and its eligible set is inherently tiny).idempotency_recordscarriesAccountIdbut is not tenant-query-filtered, so a single delete prunes every account uniformly.Tests (
IdempotencyRecordPurgeSweepTests)Aged collection (Completed + abandoned InProgress), fresh/boundary retention, tenant-safety + idempotency, backlog drain across the batch boundary, replay-after-purge re-executes, a stolen-then-completed claim (old
CreatedAt, freshCompletedAt) is retained (mutation-first guard for theCompletedAtkeying), and a coordinated renewal/delete race guard that holds a steal's row lock, waits viapg_stat_activityuntil the sweep's DELETE is actually blocked, then commits — proving the target-level lease predicate skips the now-live row (reverting to the batched form turns it red).Verification
No .NET SDK in my environment, so I could not build/run locally — CI's
Build and testis the authority, and it is green (including the raw-SQL concurrency guard against real Postgres via Testcontainers).