fix(selfhost): make retention pruning work on postgres - #2485
Merged
Conversation
Data-retention pruning (src/db/retention.ts) batches its deletes via SQLite's rowid pseudo-column, and its cutoff WHERE clause uses SQLite's numbered-placeholder syntax (`?1`). Both are SQLite/D1-only: on the self-host Postgres backend, `rowid` reaches Postgres unmodified and raises `column "rowid" does not exist`, dead-lettering the prune-retention job every run (live self-host: _selfhost_jobs.id=61132, 5 attempts). Rather than special-case retention.ts, the fix lives in the shared SQLite -> Postgres SQL translator (src/selfhost/pg-dialect.ts) that every query already passes through on the Postgres backend: - translateRowid(): rewrites the bare `rowid` token to Postgres's `ctid` system column. Both are stable per-row identifiers for the lifetime of a single statement, which is exactly how this codebase uses rowid (bounded batched delete, ORDER BY tie-breaking) - never for durable row identity. SQLite/D1 is untouched: this translator only runs on the Postgres adapter. - toNumberedPlaceholders(): fixed to recognize a numbered placeholder (`?1`, `?2`, ...) and reuse its index directly as `$1`/`$2`, instead of folding the trailing digit into the anonymous-placeholder counter (which corrupted `?1` into `$11` - a bind index nothing supplies). This is required for retention's own cutoff bind to resolve correctly on Postgres, and also fixes the same latent bug in repositories.ts's claimRegateFanoutSlot(). retention.ts itself is unchanged - it keeps emitting the same SQLite-dialect SQL that already runs correctly on D1. Tests added: - selfhost-pg-dialect.test.ts: translateRowid unit tests, a regression test against the exact batched-delete SQL shape retention.ts emits, and numbered-placeholder regression tests. - selfhost-pg-retention.test.ts (new): pruneExpiredRecords/processJob against a mocked pg.Pool that throws the real Postgres error if untranslated rowid SQL ever reaches it again, covering dry-run, bounded batching, the audit_events durable-event exclusion, and a named regression test for the live dead-letter. - test/integration/selfhost-pg.test.ts: a real-Postgres end-to-end case (skipped unless PG_TEST_URL is set, same as the rest of that suite) - validated locally against a real Postgres 17 instance. Validation: npm run test:ci; npm run test:coverage (unsharded, src/selfhost/pg-dialect.ts at 100% lines/branches/functions on the diff); npm audit --audit-level=moderate; the new integration test against a real local Postgres 17. The live dead-lettered job (_selfhost_jobs.id=61132) should be safe to retry once this deploys. Sentry triage (no code changes in this PR beyond the above): - review_context_fetch_failed (REES 403) and orb_broker_unavailable are real gaps (a startup config-readiness check and retry backoff/dedup, respectively) - deferred as follow-up work, not bundled here to keep this fix narrow. - The AI-review-inconclusive and PR-publish-failed clusters look already addressed by two recently landed commits (9d05cb6, 8da2d62); worth confirming no fresh occurrences post-deploy before closing those. - The bundlephobia analyzer HTTP-error noise is unrelated to retention/Postgres and is deferred. no issue because: this is a direct fix for a live, precisely root-caused self-host incident (a dead-lettered production job with the exact error captured); the change is narrowly scoped to the Postgres SQL-translation layer.
Contributor
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 Gittensory is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
…nymous one toNumberedPlaceholders() reused a numbered placeholder's own index (?1 -> $1) without raising the separate anonymous-placeholder counter, so a later bare `?` in the same query could reuse an already-assigned index (e.g. "a=?1 AND b=?" translated to "a=$1 AND b=$1" instead of "a=$1 AND b=$2"). SQLite's own rule for a bare `?` is "one greater than the largest parameter number already assigned", so the counter must track numbered indices too. Neither of the two real call sites (retention.ts's retentionWhere, repositories.ts's claimRegateFanoutSlot) mixes numbered and anonymous forms in one query, so this didn't affect production behavior, but the translator is general-purpose and the added test had baked the wrong contract in as if it were correct. Caught by gate review on #2485.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2485 +/- ##
==========================================
+ Coverage 95.93% 95.95% +0.01%
==========================================
Files 225 226 +1
Lines 25338 25395 +57
Branches 9218 9235 +17
==========================================
+ Hits 24308 24367 +59
Misses 417 417
+ Partials 613 611 -2
🚀 New features to boost your workflow:
|
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.
Summary
src/db/retention.ts) batches its deletes via SQLite'srowidpseudo-column and binds its cutoff with SQLite's numbered-placeholder syntax (?1). Both are SQLite/D1-only. On the self-host Postgres backend,rowidreaches Postgres unmodified and raisescolumn "rowid" does not exist, dead-lettering theprune-retentionjob every run — this is the live self-host incident (_selfhost_jobs.id=61132, 5 attempts,last_error = column "rowid" does not exist").src/selfhost/pg-dialect.ts) that every query already passes through on the Postgres backend, not inretention.tsitself:translateRowid(): rewrites the barerowidtoken to Postgres'sctidsystem column. Both are stable per-row identifiers for the lifetime of a single statement — exactly how this codebase usesrowid(bounded batched delete,ORDER BYtie-breaking), never for durable row identity. SQLite/D1 is untouched, since this translator only runs on the Postgres adapter.toNumberedPlaceholders(): fixed to recognize a numbered placeholder (?1,?2, …) and reuse its index directly as$1/$2, instead of folding the trailing digit into the anonymous-placeholder counter (which corrupted?1into$11— a bind index nothing supplies). This was required for retention's own cutoff bind to resolve correctly on Postgres, and incidentally fixes the same latent bug inrepositories.ts'sclaimRegateFanoutSlot().retention.tsitself is unchanged — it keeps emitting the same SQLite-dialect SQL that already runs correctly on D1; batching/maxPerTable,dryRun, and theaudit_eventsdurable-event exclusion are all untouched and re-verified by the new tests.Sentry triage (no code changes beyond the above — kept this PR narrow):
review_context_fetch_failed(REES 403) andorb_broker_unavailableare real gaps (a startup config-readiness check, and retry backoff/dedup respectively) — deferred as follow-up work.9d05cb6a,8da2d62f) — worth confirming no fresh occurrences post-deploy before closing those Sentry issues.bundlephobiaanalyzer HTTP-error noise is unrelated to retention/Postgres and is deferred.The live dead-lettered job (
_selfhost_jobs.id=61132) should be safe to retry once this deploys.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally (unsharded) —src/selfhost/pg-dialect.tsis at 100% lines/branches/functions on the diff (confirmed via the raw lcov output); full suite: 6063 passed, 0 failed.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitiesPG_TEST_URL=... npx vitest run test/integration/selfhost-pg.test.ts), including the new retention regression case — all green.Tests added:
test/unit/selfhost-pg-dialect.test.ts:translateRowidunit tests, a regression test against the exact batched-delete SQL shaperetention.tsemits, numbered-placeholder regression tests (?1/?2→$1/$2, not$11/$21).test/unit/selfhost-pg-retention.test.ts(new):pruneExpiredRecords/processJobexercised against a mockedpg.Poolthat throws the real Postgres error if untranslatedrowidSQL ever reaches it again — covers dry-run, bounded batching + the per-table cap, theaudit_eventsdurable-event exclusion surviving translation, and a named regression test for the live dead-letter (processJob({ type: "prune-retention" })no longer throws/dead-letters on Postgres).test/integration/selfhost-pg.test.ts: a real-Postgres end-to-end case (skipped unlessPG_TEST_URLis set, same convention as the rest of that suite).Safety
UI Evidencesection. (N/A — backend-only change, no visible UI change.)Notes
heavy-dependencyanalyzer, review-publish path) rather than live Sentry data.