From 530b299a857d14aab04aaa6711fb3edf2bf3b985 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Thu, 3 Sep 2026 17:50:15 -0500 Subject: [PATCH 01/11] fix(postgres): prevent reconciliation storage bloat Unchanged record payloads and derived index scopes previously rewrote physical rows during reconciliation, producing dead tuples even when logical data was unchanged. Preserve the existing JSONB and content-addressed-blob no-op paths through metadata repair, and delete only stale semantic scopes before conditional upserts.\n\nAdd low-threshold heap and TOAST autovacuum settings plus a durable, off-peak maintenance job as recovery safety nets. The job is not relied on for correctness: isolated PostgreSQL fixtures prove repeated large JSONB/BYTEA payload and derived-index reconciliations keep dead tuples near zero and relation sizes flat. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../server/deployment-diagnostics.ts | 14 + reference-implementation/server/index.ts | 48 +++ .../postgres-derived-index-maintenance.ts | 375 ++++++++++++++++++ .../server/postgres-records.ts | 13 +- .../server/postgres-search.ts | 33 +- .../server/postgres-storage.ts | 41 ++ .../test/deployment-diagnostics.test.ts | 5 + ...postgres-derived-index-maintenance.test.ts | 136 +++++++ .../test/postgres-records-ingest-noop.test.ts | 103 ++++- .../records-delete-postgres-routing.test.ts | 81 +++- ...ntic-index-skip-unchanged-postgres.test.ts | 205 ++++++++++ 11 files changed, 1034 insertions(+), 20 deletions(-) create mode 100644 reference-implementation/server/postgres-derived-index-maintenance.ts create mode 100644 reference-implementation/test/postgres-derived-index-maintenance.test.ts create mode 100644 reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts diff --git a/reference-implementation/server/deployment-diagnostics.ts b/reference-implementation/server/deployment-diagnostics.ts index ea8f1b5a9..858cdbe6b 100644 --- a/reference-implementation/server/deployment-diagnostics.ts +++ b/reference-implementation/server/deployment-diagnostics.ts @@ -22,6 +22,7 @@ */ import { statfs } from "node:fs/promises"; +import type { PostgresDerivedIndexMaintenanceReceipt } from "./postgres-derived-index-maintenance.ts"; import type { SemanticEmbeddingWarmStatus } from "./search-semantic.ts"; // Shape of a connector manifest as far as diagnostics care. We do not depend @@ -219,6 +220,7 @@ export interface DeploymentDiagnosticsInput { // explicit SQLite/failure `{ physical_bytes: null, top_relations: null }` // both surface as unmeasured. readonly physicalFootprint?: PhysicalFootprint | null; + readonly postgresDerivedIndexMaintenanceReceipt?: PostgresDerivedIndexMaintenanceReceipt | null; readonly runtimeCapabilities?: RuntimeCapabilityPosture | null; } @@ -357,6 +359,8 @@ export interface DeploymentDiagnosticsReport { readonly provenance: "native" | "polyfill-registered"; readonly semantic_stream_count: number; }>; + // Null means this process has not completed a Postgres maintenance run. + readonly postgres_derived_index_maintenance: PostgresDerivedIndexMaintenanceReceipt | null; readonly runtime_capabilities: { readonly bindings: { readonly browser: boolean; @@ -415,6 +419,7 @@ export interface DeploymentDiagnosticsReport { const STATIC_ENV_ALLOWLIST: ReadonlyArray<{ readonly name: string; readonly secret?: boolean }> = [ { name: "PDPP_STORAGE_BACKEND" }, { name: "PDPP_DATABASE_URL", secret: true }, + { name: "PDPP_POSTGRES_DERIVED_INDEX_MAINTENANCE_WINDOW" }, { name: "AS_PORT" }, { name: "RS_PORT" }, { name: "AS_PUBLIC_URL" }, @@ -1058,6 +1063,10 @@ export interface DeploymentDiagnosticsRuntimeDeps { // it and degrades cleanly to unmeasured on absence or rejection — the page // never fails because the footprint could not be read. readonly getPhysicalFootprint?: () => PhysicalFootprint | Promise | null; + readonly getPostgresDerivedIndexMaintenanceReceipt?: () => + | PostgresDerivedIndexMaintenanceReceipt + | Promise + | null; readonly getRuntimeCapabilityPosture?: () => RuntimeCapabilityPosture | Promise | null; readonly listRegisteredConnectorIds: () => Promise; } @@ -1113,6 +1122,9 @@ export async function collectDeploymentDiagnostics( // missing dep) degrades to unmeasured rather than failing the whole page; // the builder collapses null/undefined to `physical_bytes: null`. const physicalFootprint = await resolveOptionalDep(deps.getPhysicalFootprint); + const postgresDerivedIndexMaintenanceReceipt = await resolveOptionalDep( + deps.getPostgresDerivedIndexMaintenanceReceipt + ); const diskHeadroom = await resolveOptionalDep(deps.getDiskHeadroom); const pgDiskHeadroom = await resolveOptionalDep(deps.getPgDiskHeadroom); @@ -1129,6 +1141,7 @@ export async function collectDeploymentDiagnostics( manifests, pgDiskHeadroom, physicalFootprint, + postgresDerivedIndexMaintenanceReceipt, runtimeCapabilities, }); } @@ -1159,6 +1172,7 @@ export function buildDeploymentDiagnostics(input: DeploymentDiagnosticsInput): D }, }, manifests: summarizeManifests(input.manifests), + postgres_derived_index_maintenance: input.postgresDerivedIndexMaintenanceReceipt ?? null, runtime_capabilities: buildRuntimeCapabilityReport(input.runtimeCapabilities ?? null), semantic: { backend: { diff --git a/reference-implementation/server/index.ts b/reference-implementation/server/index.ts index d58edd546..19b6a6bf7 100644 --- a/reference-implementation/server/index.ts +++ b/reference-implementation/server/index.ts @@ -230,6 +230,11 @@ import { schedulePostgresSemanticHnswMaintenance, } from "./postgres-storage.ts"; import { createGenericProviderAuthDispatch } from "./provider-auth/generic-dispatch.ts"; +import { + getLastPostgresDerivedIndexMaintenanceReceipt, + parsePostgresDerivedIndexMaintenanceWindow, + runPostgresDerivedIndexMaintenance, +} from "./postgres-derived-index-maintenance.ts"; import { buildRecordVersionStatsEnvelope } from "./record-version-stats.ts"; import { aggregateRecordsAcrossBindings, @@ -975,6 +980,7 @@ const STARTUP_SUMMARY_EVIDENCE_MAX_RESUME_ROUNDS = 20; // running meaningfully more often than the durable state it sweeps // actually changes. const CONNECTOR_MAINTENANCE_SWEEP_INTERVAL_MS = 60_000; +const POSTGRES_DERIVED_INDEX_MAINTENANCE_SWEEP_INTERVAL_MS = 15 * 60_000; const CONNECTOR_MAINTENANCE_EVIDENCE_SWEEP_MAX_DURATION_MS = 2000; const CONNECTOR_MAINTENANCE_EVIDENCE_SWEEP_PAGE_SIZE = 25; // Run-history backfill (terminal-read-architecture-fable-0730.md §9): @@ -5213,6 +5219,7 @@ export function buildAsApp(opts: ServerOpts = {}) { getDiskHeadroom: () => probeDiskHeadroom(opts.dbPath || DB_PATH), getLexicalBackendPosture: () => getPostgresLexicalBackendState(), getLexicalBackfillProgress: () => getLexicalIndexBackfillProgress(), + getPostgresDerivedIndexMaintenanceReceipt: () => getLastPostgresDerivedIndexMaintenanceReceipt(), getPhysicalFootprint: () => collectPhysicalFootprint(), // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: This protocol transition owns ordered state invariants that must remain local. getRuntimeCapabilityPosture: async () => { @@ -8266,6 +8273,40 @@ export async function startServer(opts: ServerOpts = {}) { function stopConnectorMaintenanceSweep() { connectorMaintenanceSweepTimer.stop(); } + // Database-reclaiming maintenance defaults to a UTC off-peak window. The + // operator can override or disable it; the runner owns the non-transactional + // VACUUM/REINDEX session and records each result for diagnostics. + const postgresDerivedIndexMaintenanceWindow = isPostgresStorageBackend() + ? parsePostgresDerivedIndexMaintenanceWindow() + : null; + let postgresDerivedIndexMaintenanceInFlight = false; + const postgresDerivedIndexMaintenanceTimer = postgresDerivedIndexMaintenanceWindow + ? createBrowserSurfaceLeaseSweepTimer({ + intervalMs: POSTGRES_DERIVED_INDEX_MAINTENANCE_SWEEP_INTERVAL_MS, + onSweepError: (err: unknown) => { + logger.warn?.( + { err: err instanceof Error ? err.message : String(err) }, + "postgres derived-index maintenance tick failed" + ); + }, + sweep: async () => { + if (postgresDerivedIndexMaintenanceInFlight) { + logger.info?.("postgres derived-index maintenance skipped because a prior run is still active"); + return; + } + postgresDerivedIndexMaintenanceInFlight = true; + try { + const receipt = await runPostgresDerivedIndexMaintenance({ window: postgresDerivedIndexMaintenanceWindow }); + logger.info?.({ receipt }, "postgres derived-index maintenance completed"); + } finally { + postgresDerivedIndexMaintenanceInFlight = false; + } + }, + }) + : null; + function stopPostgresDerivedIndexMaintenance() { + postgresDerivedIndexMaintenanceTimer?.stop(); + } let schedulerManager: { cancelRun: (runId: string) => { status: string; run_id: string }; refresh: () => Promise; @@ -8814,6 +8855,8 @@ export async function startServer(opts: ServerOpts = {}) { // every deployment. connectorMaintenanceSweepTimer.stopWhenAllClosed([asServer, rsServer]); connectorMaintenanceSweepTimer.start(); + postgresDerivedIndexMaintenanceTimer?.stopWhenAllClosed([asServer, rsServer]); + postgresDerivedIndexMaintenanceTimer?.start(); const deliveryWorkerLeases = opts.startClientEventDeliveryWorker === false ? [] @@ -8868,6 +8911,7 @@ export async function startServer(opts: ServerOpts = {}) { // maintenance sweep timer (shell retirement, attention expiry, bounded // evidence-sweep round — see connector-maintenance-sweep.ts). stopConnectorMaintenanceSweep, + stopPostgresDerivedIndexMaintenance, }; } @@ -9899,6 +9943,7 @@ if (process.argv[1]?.endsWith("server/index.ts")) { stopBrowserSurfaceLeaseSweep: StartServerResult["stopBrowserSurfaceLeaseSweep"] | null; stopClientEventDeliveryWorker: StartServerResult["stopClientEventDeliveryWorker"] | null; stopConnectorMaintenanceSweep: StartServerResult["stopConnectorMaintenanceSweep"] | null; + stopPostgresDerivedIndexMaintenance: StartServerResult["stopPostgresDerivedIndexMaintenance"] | null; } = { abortStartupBackfill: null, asServer: null, @@ -9911,6 +9956,7 @@ if (process.argv[1]?.endsWith("server/index.ts")) { stopBrowserSurfaceLeaseSweep: null, stopClientEventDeliveryWorker: null, stopConnectorMaintenanceSweep: null, + stopPostgresDerivedIndexMaintenance: null, }; const exitOnSignal = (signal: string) => async () => { if (shuttingDown) { @@ -9990,6 +10036,7 @@ if (process.argv[1]?.endsWith("server/index.ts")) { } catch {} server.stopBrowserSurfaceLeaseSweep?.(); server.stopConnectorMaintenanceSweep?.(); + server.stopPostgresDerivedIndexMaintenance?.(); await server.stopClientEventDeliveryWorker?.(); // In-flight connector runs are deliberately NOT drained here. // @@ -10053,6 +10100,7 @@ if (process.argv[1]?.endsWith("server/index.ts")) { server.stopBrowserSurfaceLeaseSweep = result.stopBrowserSurfaceLeaseSweep; server.stopClientEventDeliveryWorker = result.stopClientEventDeliveryWorker; server.stopConnectorMaintenanceSweep = result.stopConnectorMaintenanceSweep; + server.stopPostgresDerivedIndexMaintenance = result.stopPostgresDerivedIndexMaintenance; }) .catch((err) => { closePostgresStorage().finally(() => closeDb()); diff --git a/reference-implementation/server/postgres-derived-index-maintenance.ts b/reference-implementation/server/postgres-derived-index-maintenance.ts new file mode 100644 index 000000000..05a89b2ab --- /dev/null +++ b/reference-implementation/server/postgres-derived-index-maintenance.ts @@ -0,0 +1,375 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import type { PoolClient } from "pg"; +import { getPostgresLockPool } from "./postgres-storage.ts"; + +const DERIVED_INDEX_MAINTENANCE_WINDOW_ENV = "PDPP_POSTGRES_DERIVED_INDEX_MAINTENANCE_WINDOW"; +const DERIVED_INDEX_MAINTENANCE_LOCK = [482_571, 153] as const; +const DEFAULT_UTC_OFF_PEAK_WINDOW: PostgresUtcOffPeakWindow = { endMinute: 300, startMinute: 60 }; +const HEAVY_TABLES = [ + "blobs", + "lexical_search_index", + "record_changes", + "records", + "semantic_search_blob", + "spine_events", +] as const; +const REINDEXABLE_DERIVED_INDEXES = [ + { indexName: "idx_pg_lexical_search_document", tableName: "lexical_search_index" }, + { indexName: "idx_pg_lexical_search_scope_document", tableName: "lexical_search_index" }, + { indexName: "idx_pg_semantic_search_scope", tableName: "semantic_search_blob" }, + { indexName: "idx_pg_semantic_search_embedding_hnsw", tableName: "semantic_search_blob" }, +] as const; +const DEFAULT_MINIMUM_TABLE_BYTES = 32 * 1024 * 1024; +const DEFAULT_MINIMUM_DEAD_TUPLES = 100_000; +const DEFAULT_MINIMUM_DEAD_TUPLE_RATIO = 0.2; + +export interface PostgresUtcOffPeakWindow { + /** Minutes after 00:00 UTC, inclusive. */ + endMinute: number; + /** Minutes after 00:00 UTC, inclusive. */ + startMinute: number; +} + +export interface PostgresDerivedIndexMaintenanceTableReceipt { + deadTupleRatio: number; + deadTuples: number; + liveTuples: number; + tableName: string; + totalBytes: number; + vacuumed: boolean; +} + +export interface PostgresDerivedIndexMaintenanceReceipt { + completedAt: string; + error?: string; + reindexedIndexNames: string[]; + startedAt: string; + status: + | "already-attempted" + | "already-completed" + | "completed" + | "disabled" + | "failed" + | "lock-unavailable" + | "outside-window"; + tables: PostgresDerivedIndexMaintenanceTableReceipt[]; + window: PostgresUtcOffPeakWindow | null; +} + +export interface PostgresDerivedIndexMaintenanceOptions { + /** Explicit window for a caller such as a scheduler. Null keeps the job disabled. */ + window?: PostgresUtcOffPeakWindow | null; + /** Injectable clock makes the UTC boundary testable. */ + now?: Date; + minimumDeadTupleRatio?: number; + minimumDeadTuples?: number; + minimumTableBytes?: number; +} + +interface TableStat { + deadTuples: number; + liveTuples: number; + tableName: (typeof HEAVY_TABLES)[number]; + totalBytes: number; +} + +let lastReceipt: PostgresDerivedIndexMaintenanceReceipt | null = null; + +function numeric(value: string | number | null | undefined): number { + const parsed = Number(value ?? 0); + return Number.isFinite(parsed) && parsed >= 0 ? parsed : 0; +} + +function quoteIdentifier(identifier: string): string { + return `"${identifier.replaceAll('"', '""')}"`; +} + +function normalizeGate(value: number | undefined, fallback: number): number { + return typeof value === "number" && Number.isFinite(value) && value >= 0 ? value : fallback; +} + +function utcMinute(date: Date): number { + return date.getUTCHours() * 60 + date.getUTCMinutes(); +} + +function isWithinWindow(date: Date, window: PostgresUtcOffPeakWindow): boolean { + const minute = utcMinute(date); + if (window.startMinute === window.endMinute) { + return true; + } + return window.startMinute < window.endMinute + ? minute >= window.startMinute && minute < window.endMinute + : minute >= window.startMinute || minute < window.endMinute; +} + +function maintenanceWindowKey(date: Date, window: PostgresUtcOffPeakWindow): string { + const anchor = new Date(date); + // A window that crosses midnight belongs to the day on which it started. + if (window.startMinute > window.endMinute && utcMinute(anchor) < window.endMinute) { + anchor.setUTCDate(anchor.getUTCDate() - 1); + } + return `${anchor.toISOString().slice(0, 10)}:${window.startMinute}-${window.endMinute}`; +} + +/** + * Parse an UTC maintenance window such as "01:30-04:00". Unset uses the + * built-in 01:00-05:00 UTC window; "disabled" explicitly disables the job. + */ +export function parsePostgresDerivedIndexMaintenanceWindow( + configuredWindow = process.env[DERIVED_INDEX_MAINTENANCE_WINDOW_ENV] +): PostgresUtcOffPeakWindow | null { + const value = configuredWindow?.trim(); + if (!value) { + return { ...DEFAULT_UTC_OFF_PEAK_WINDOW }; + } + if (value.toLowerCase() === "disabled") { + return null; + } + const match = /^(?[01]\d|2[0-3]):(?[0-5]\d)-(?[01]\d|2[0-3]):(?[0-5]\d)$/.exec( + value + ); + if (!match?.groups) { + throw new Error( + `${DERIVED_INDEX_MAINTENANCE_WINDOW_ENV} must be "HH:MM-HH:MM" in UTC, "disabled", or unset.` + ); + } + return { + startMinute: Number(match.groups.startHour) * 60 + Number(match.groups.startMinute), + endMinute: Number(match.groups.endHour) * 60 + Number(match.groups.endMinute), + }; +} + +function recordReceipt(receipt: PostgresDerivedIndexMaintenanceReceipt): PostgresDerivedIndexMaintenanceReceipt { + lastReceipt = receipt; + return receipt; +} + +function skippedReceipt( + status: PostgresDerivedIndexMaintenanceReceipt["status"], + startedAt: string, + window: PostgresUtcOffPeakWindow | null +): PostgresDerivedIndexMaintenanceReceipt { + return recordReceipt({ + completedAt: new Date().toISOString(), + reindexedIndexNames: [], + startedAt, + status, + tables: [], + window, + }); +} + +function transientReceipt( + status: PostgresDerivedIndexMaintenanceReceipt["status"], + startedAt: string, + window: PostgresUtcOffPeakWindow | null +): PostgresDerivedIndexMaintenanceReceipt { + return { + completedAt: new Date().toISOString(), + reindexedIndexNames: [], + startedAt, + status, + tables: [], + window, + }; +} + +function alreadyAttemptedReceipt( + status: "already-attempted" | "already-completed", + startedAt: string, + window: PostgresUtcOffPeakWindow +): PostgresDerivedIndexMaintenanceReceipt { + return transientReceipt(status, startedAt, window); +} + +async function readTableStats(client: PoolClient): Promise { + const result = await client.query<{ + dead_tuples: string; + live_tuples: string; + table_name: (typeof HEAVY_TABLES)[number]; + total_bytes: string; + }>( + `SELECT stats.relname AS table_name, + stats.n_live_tup::bigint::text AS live_tuples, + stats.n_dead_tup::bigint::text AS dead_tuples, + pg_total_relation_size(stats.relid)::bigint::text AS total_bytes + FROM pg_stat_user_tables AS stats + JOIN pg_namespace AS namespace ON namespace.oid = stats.schemaname::regnamespace + WHERE namespace.nspname = current_schema() + AND stats.relname = ANY($1::text[]) + ORDER BY stats.relname ASC`, + [HEAVY_TABLES] + ); + return result.rows.map((row) => ({ + deadTuples: numeric(row.dead_tuples), + liveTuples: numeric(row.live_tuples), + tableName: row.table_name, + totalBytes: numeric(row.total_bytes), + })); +} + +function meetsReindexGate( + table: TableStat, + { minimumDeadTupleRatio, minimumDeadTuples }: Required> +): boolean { + const tupleCount = table.liveTuples + table.deadTuples; + return ( + table.deadTuples >= minimumDeadTuples && + table.deadTuples / Math.max(tupleCount, 1) >= minimumDeadTupleRatio + ); +} + +async function indexExists(client: PoolClient, indexName: string): Promise { + const result = await client.query<{ exists: boolean }>( + `SELECT EXISTS ( + SELECT 1 + FROM pg_class AS index_class + JOIN pg_namespace AS namespace ON namespace.oid = index_class.relnamespace + WHERE namespace.nspname = current_schema() + AND index_class.relname = $1 + ) AS exists`, + [indexName] + ); + return result.rows[0]?.exists === true; +} + +async function claimMaintenanceWindow( + client: PoolClient, + windowKey: string +): Promise<"already-attempted" | "already-completed" | "claimed"> { + const inserted = await client.query( + `INSERT INTO postgres_derived_index_maintenance_receipts (window_key, status, started_at) + VALUES ($1, 'running', now()) + ON CONFLICT (window_key) DO NOTHING + RETURNING window_key`, + [windowKey] + ); + if ((inserted.rowCount ?? 0) > 0) { + return "claimed"; + } + const current = await client.query<{ status: string }>( + "SELECT status FROM postgres_derived_index_maintenance_receipts WHERE window_key = $1", + [windowKey] + ); + if (current.rows[0]?.status === "completed") { + return "already-completed"; + } + // VACUUM and REINDEX CONCURRENTLY commit statement-by-statement. Retrying a + // crashed or failed claim could repeat an expensive partial pass, so every + // durable claim is terminal for its UTC window. + return "already-attempted"; +} + +/** + * Run bounded maintenance for known heavy tables and rebuildable derived + * indexes. It intentionally uses no transaction: PostgreSQL rejects both + * VACUUM and REINDEX CONCURRENTLY inside a transaction block. + */ +export async function runPostgresDerivedIndexMaintenance( + options: PostgresDerivedIndexMaintenanceOptions = {} +): Promise { + const now = options.now ?? new Date(); + const startedAt = now.toISOString(); + const window = options.window === undefined ? parsePostgresDerivedIndexMaintenanceWindow() : options.window; + if (!window) { + return skippedReceipt("disabled", startedAt, null); + } + if (!isWithinWindow(now, window)) { + // Preserve the last meaningful receipt for health instead of replacing a + // successful run with routine outside-window scheduler polls. + return transientReceipt("outside-window", startedAt, window); + } + const windowKey = maintenanceWindowKey(now, window); + + const gates = { + minimumDeadTupleRatio: normalizeGate(options.minimumDeadTupleRatio, DEFAULT_MINIMUM_DEAD_TUPLE_RATIO), + minimumDeadTuples: normalizeGate(options.minimumDeadTuples, DEFAULT_MINIMUM_DEAD_TUPLES), + minimumTableBytes: normalizeGate(options.minimumTableBytes, DEFAULT_MINIMUM_TABLE_BYTES), + }; + const client = await getPostgresLockPool().connect(); + let lockHeld = false; + let windowClaimed = false; + try { + const lock = await client.query<{ locked: boolean }>("SELECT pg_try_advisory_lock($1, $2) AS locked", [ + ...DERIVED_INDEX_MAINTENANCE_LOCK, + ]); + lockHeld = lock.rows[0]?.locked === true; + if (!lockHeld) { + return transientReceipt("lock-unavailable", startedAt, window); + } + const claim = await claimMaintenanceWindow(client, windowKey); + if (claim !== "claimed") { + return alreadyAttemptedReceipt(claim, startedAt, window); + } + windowClaimed = true; + + const stats = await readTableStats(client); + const tables = stats.map((table) => ({ + ...table, + deadTupleRatio: table.deadTuples / Math.max(table.liveTuples + table.deadTuples, 1), + vacuumed: table.totalBytes >= gates.minimumTableBytes, + })); + for (const table of tables) { + if (table.vacuumed) { + await client.query(`VACUUM (ANALYZE) ${quoteIdentifier(table.tableName)}`); + } + } + + const reindexedIndexNames: string[] = []; + for (const index of REINDEXABLE_DERIVED_INDEXES) { + const table = stats.find((candidate) => candidate.tableName === index.tableName); + if (table && meetsReindexGate(table, gates) && (await indexExists(client, index.indexName))) { + await client.query(`REINDEX INDEX CONCURRENTLY ${quoteIdentifier(index.indexName)}`); + reindexedIndexNames.push(index.indexName); + } + } + await client.query( + `UPDATE postgres_derived_index_maintenance_receipts + SET status = 'completed', completed_at = now(), error_text = NULL + WHERE window_key = $1`, + [windowKey] + ); + return recordReceipt({ + completedAt: new Date().toISOString(), + reindexedIndexNames, + startedAt, + status: "completed", + tables, + window, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (windowClaimed) { + await client + .query( + `UPDATE postgres_derived_index_maintenance_receipts + SET status = 'failed', completed_at = now(), error_text = $2 + WHERE window_key = $1`, + [windowKey, message] + ) + .catch(() => undefined); + } + recordReceipt({ + completedAt: new Date().toISOString(), + error: message, + reindexedIndexNames: [], + startedAt, + status: "failed", + tables: [], + window, + }); + throw error; + } finally { + if (lockHeld) { + await client.query("SELECT pg_advisory_unlock($1, $2)", [...DERIVED_INDEX_MAINTENANCE_LOCK]).catch(() => undefined); + } + client.release(); + } +} + +/** The most recent local run outcome, for a later health-reporting seam. */ +export function getLastPostgresDerivedIndexMaintenanceReceipt(): PostgresDerivedIndexMaintenanceReceipt | null { + return lastReceipt; +} diff --git a/reference-implementation/server/postgres-records.ts b/reference-implementation/server/postgres-records.ts index ee5583ab4..b84cc7ac4 100644 --- a/reference-implementation/server/postgres-records.ts +++ b/reference-implementation/server/postgres-records.ts @@ -952,7 +952,8 @@ export function postgresPrepareDeviceFinalRecords( primary_key_text = $5, semantic_time = $6 WHERE connector_instance_id = $1 AND stream = $2 AND record_key = $3 - AND deleted = FALSE`, + AND deleted = FALSE + AND (cursor_value, primary_key_text, semantic_time) IS DISTINCT FROM ($4, $5, $6)`, [connectorInstanceId, input.stream, recordKey, cursor, primary, semanticTime] ); result.push({ @@ -1625,7 +1626,8 @@ async function repairPostgresIdenticalIngest({ primary_key_text = $5, semantic_time = $6 WHERE connector_instance_id = $1 AND stream = $2 AND record_key = $3 - AND deleted = FALSE`, + AND deleted = FALSE + AND (cursor_value, primary_key_text, semantic_time) IS DISTINCT FROM ($4, $5, $6)`, [connectorInstanceId, stream, recordKey, storedCursorValue, storedPrimaryKeyText, storedSemanticTime] ); } @@ -2982,7 +2984,10 @@ async function deletePostgresRecordTailForPair( connectorInstanceId: string, stream: string ): Promise { - const semanticScopePrefix = `[${JSON.stringify(stream)},`; + const semanticScopePrefix = `[${JSON.stringify(stream)},` + .replaceAll("\\", "\\\\") + .replaceAll("%", "\\%") + .replaceAll("_", "\\_"); await client.query("DELETE FROM record_changes WHERE connector_instance_id = $1 AND stream = $2", [ connectorInstanceId, stream, @@ -3003,7 +3008,7 @@ async function deletePostgresRecordTailForPair( connectorInstanceId, stream, ]); - await client.query("DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2", [ + await client.query("DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 ESCAPE '\\'", [ connectorInstanceId, `${semanticScopePrefix}%`, ]); diff --git a/reference-implementation/server/postgres-search.ts b/reference-implementation/server/postgres-search.ts index 23baa6c75..ffb7446db 100644 --- a/reference-implementation/server/postgres-search.ts +++ b/reference-implementation/server/postgres-search.ts @@ -504,16 +504,21 @@ export async function postgresLexicalSearch({ return result.rows; } +function semanticScopeLikePattern(stream: string): string { + const scopePrefix = `[${JSON.stringify(stream)},`; + const escapedPrefix = scopePrefix.replaceAll("\\", "\\\\").replaceAll("%", "\\%").replaceAll("_", "\\_"); + return `${escapedPrefix}%`; +} + export async function postgresSemanticIndexDelete({ connectorId, connectorInstanceId = defaultConnectorInstanceId(connectorId), stream, recordKey, }: RecordScope) { - const scopePrefix = `[${JSON.stringify(stream)},`; await postgresQuery( - "DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 AND record_key = $3", - [connectorInstanceId, `${scopePrefix}%`, recordKey] + "DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 ESCAPE '\\' AND record_key = $3", + [connectorInstanceId, semanticScopeLikePattern(stream), recordKey] ); } @@ -522,10 +527,9 @@ export async function postgresSemanticIndexDeleteByConnectorStream({ connectorInstanceId = defaultConnectorInstanceId(connectorId), stream, }: ConnectorStreamScope) { - const scopePrefix = `[${JSON.stringify(stream)},`; - await postgresQuery("DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2", [ + await postgresQuery("DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 ESCAPE '\\'", [ connectorInstanceId, - `${scopePrefix}%`, + semanticScopeLikePattern(stream), ]); await postgresQuery("DELETE FROM semantic_search_meta WHERE connector_instance_id = $1 AND stream = $2", [ connectorInstanceId, @@ -727,7 +731,8 @@ async function insertSemanticRows( FROM unnest($1::text[], $2::text[], $3::text[], $4::text[], $5::text[]) AS rows(connector_id, connector_instance_id, scope_key, record_key, embedding) ON CONFLICT (connector_instance_id, scope_key, record_key) DO UPDATE - SET embedding = EXCLUDED.embedding`, + SET embedding = EXCLUDED.embedding + WHERE semantic_search_blob.embedding IS DISTINCT FROM EXCLUDED.embedding`, [ rows.map((entry) => entry.connectorId), rows.map((entry) => entry.connectorInstanceId), @@ -850,10 +855,13 @@ export async function postgresSemanticIndexPublishWithClient( }: RecordScope & { entries: readonly SemanticIndexEntry[] } ): Promise { const rows = semanticInsertManyRows(connectorId, connectorInstanceId, entries); - const scopePrefix = `[${JSON.stringify(stream)},`; await client.query( - "DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 AND record_key = $3", - [connectorInstanceId, `${scopePrefix}%`, recordKey] + `DELETE FROM semantic_search_blob + WHERE connector_instance_id = $1 + AND scope_key LIKE $2 ESCAPE '\\' + AND record_key = $3 + AND scope_key <> ALL($4::text[])`, + [connectorInstanceId, semanticScopeLikePattern(stream), recordKey, rows.map((row) => row.scopeKey)] ); await insertSemanticRows(rows, (sql, params) => client.query(sql, params as unknown[])); } @@ -863,10 +871,9 @@ export async function postgresSemanticIndexDeleteWithClient( client: PostgresTransactionClient, { connectorInstanceId, stream, recordKey }: { connectorInstanceId: string; stream: string; recordKey: string } ): Promise { - const scopePrefix = `[${JSON.stringify(stream)},`; await client.query( - "DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 AND record_key = $3", - [connectorInstanceId, `${scopePrefix}%`, recordKey] + "DELETE FROM semantic_search_blob WHERE connector_instance_id = $1 AND scope_key LIKE $2 ESCAPE '\\' AND record_key = $3", + [connectorInstanceId, semanticScopeLikePattern(stream), recordKey] ); } diff --git a/reference-implementation/server/postgres-storage.ts b/reference-implementation/server/postgres-storage.ts index 8bef7d36c..1207180e6 100644 --- a/reference-implementation/server/postgres-storage.ts +++ b/reference-implementation/server/postgres-storage.ts @@ -150,6 +150,21 @@ const POSTGRES_BULK_STATEMENT_TIMEOUT_MS = 15_000; * pass rather than occupying a bulk-lane connection while it waits. */ const POSTGRES_BULK_LOCK_TIMEOUT_MS = 3000; +const POSTGRES_BLOAT_CONTROL_TABLES = ["records", "record_changes", "blobs", "spine_events"] as const; +const POSTGRES_BLOAT_CONTROL_RELATION_OPTIONS = ` + autovacuum_enabled = true, + autovacuum_vacuum_threshold = 1, + autovacuum_vacuum_scale_factor = 0.01, + autovacuum_vacuum_insert_threshold = 50, + autovacuum_vacuum_insert_scale_factor = 0.02, + autovacuum_analyze_threshold = 50, + autovacuum_analyze_scale_factor = 0.02, + toast.autovacuum_enabled = true, + toast.autovacuum_vacuum_threshold = 1, + toast.autovacuum_vacuum_scale_factor = 0.01, + toast.autovacuum_vacuum_insert_threshold = 50, + toast.autovacuum_vacuum_insert_scale_factor = 0.02 +`; // Semantic embedding storage mode, detected at bootstrap. 'vector' when the // pgvector extension is available and `semantic_search_blob.embedding` carries @@ -347,6 +362,30 @@ async function sequentially(items: readonly T[], visit: (item: T) => Promise< await sequentially(items.slice(1), visit); } +/** + * High-churn payload tables need per-relation settings so their maintenance + * cadence stays bounded even when a deployment has permissive cluster-wide + * autovacuum defaults. Include each table's TOAST relation: JSONB and BYTEA + * payload churn otherwise leaves the largest dead tuples behind. + */ +async function enforcePostgresBloatControlAutovacuumPolicy(client: PoolClient): Promise { + await sequentially(POSTGRES_BLOAT_CONTROL_TABLES, async (table) => { + await client.query(`ALTER TABLE ${table} SET (${POSTGRES_BLOAT_CONTROL_RELATION_OPTIONS})`); + }); +} + +async function ensurePostgresDerivedIndexMaintenanceReceiptTable(client: PoolClient): Promise { + await client.query(` + CREATE TABLE IF NOT EXISTS postgres_derived_index_maintenance_receipts ( + window_key TEXT PRIMARY KEY, + status TEXT NOT NULL CHECK (status IN ('running', 'completed', 'failed')), + started_at TIMESTAMPTZ NOT NULL, + completed_at TIMESTAMPTZ, + error_text TEXT + ) + `); +} + function semanticVectorMigrationBatchSize() { const parsed = Number.parseInt(process.env.PDPP_PG_SEMANTIC_MIGRATION_BATCH_SIZE || "", 10); return Number.isInteger(parsed) && parsed > 0 ? parsed : 50_000; @@ -3555,6 +3594,8 @@ async function bootstrapPostgresSchemaOnce({ await ensurePostgresRecordsInstanceStreamIdIndex(client, log); await ensurePostgresRecordsInstanceDeletedIdIndex(client, log); await ensurePostgresConnectorSummarySourceRevisionPrimitive(client); + await enforcePostgresBloatControlAutovacuumPolicy(client); + await ensurePostgresDerivedIndexMaintenanceReceiptTable(client); } finally { try { if (bootstrapLockHeld) { diff --git a/reference-implementation/test/deployment-diagnostics.test.ts b/reference-implementation/test/deployment-diagnostics.test.ts index 8ac3a02ad..0a25b9401 100644 --- a/reference-implementation/test/deployment-diagnostics.test.ts +++ b/reference-implementation/test/deployment-diagnostics.test.ts @@ -783,6 +783,11 @@ test("/_ref/deployment returns a structured report with zero participation by de assert.ok(body.semantic.participation); assert.ok(Array.isArray(body.environment)); assert.ok(Array.isArray(body.warnings)); + assert.equal( + body.postgres_derived_index_maintenance, + null, + "health reports no Postgres maintenance receipt before the job runs" + ); assert.ok(Array.isArray(body.disk_headroom), "route wiring must include disk_headroom as array"); assert.ok(body.disk_headroom.length > 0, "disk_headroom must have at least one entry"); const [firstDiskEntry] = body.disk_headroom; diff --git a/reference-implementation/test/postgres-derived-index-maintenance.test.ts b/reference-implementation/test/postgres-derived-index-maintenance.test.ts new file mode 100644 index 000000000..0f2393a16 --- /dev/null +++ b/reference-implementation/test/postgres-derived-index-maintenance.test.ts @@ -0,0 +1,136 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { closePostgresStorage, initPostgresStorage, postgresQuery } from "../server/postgres-storage.ts"; +import { + getLastPostgresDerivedIndexMaintenanceReceipt, + parsePostgresDerivedIndexMaintenanceWindow, + runPostgresDerivedIndexMaintenance, +} from "../server/postgres-derived-index-maintenance.ts"; +import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; + +const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL ?? "postgres://postgres:pdpp_bloat_test_password@127.0.0.1:55448/pdpp_bloat_test"; +let databaseCounter = 0; + +function databaseName(): string { + databaseCounter += 1; + return `pdpp_derived_index_maintenance_${process.pid}_${databaseCounter}`; +} + +test("derived-index maintenance defaults to a safe UTC off-peak window and accepts overrides", () => { + assert.deepEqual(parsePostgresDerivedIndexMaintenanceWindow(""), { endMinute: 300, startMinute: 60 }); + assert.deepEqual(parsePostgresDerivedIndexMaintenanceWindow("01:30-04:00"), { + endMinute: 240, + startMinute: 90, + }); + assert.equal(parsePostgresDerivedIndexMaintenanceWindow("disabled"), null); + assert.throws(() => parsePostgresDerivedIndexMaintenanceWindow("overnight"), /HH:MM-HH:MM/); +}); + +test("derived-index maintenance vacuums known heavy tables and concurrently reindexes present static search indexes", async () => { + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName: databaseName() }, + async (databaseUrl) => { + await initPostgresStorage({ backend: "postgres", databaseUrl }); + try { + await postgresQuery( + `INSERT INTO lexical_search_index (connector_id, connector_instance_id, stream, record_key, field, value) + SELECT 'connector', 'instance', 'stream', 'record-' || series::text, 'field', repeat('searchable value ', 32) + FROM generate_series(1, 3000) AS series` + ); + await postgresQuery("DELETE FROM lexical_search_index WHERE record_key <> 'record-1'"); + await postgresQuery("ANALYZE lexical_search_index"); + + const before = await postgresQuery<{ index_oid: string; last_vacuum: string | null }>( + `SELECT stats.last_vacuum::text AS last_vacuum, + index_class.oid::text AS index_oid + FROM pg_stat_user_tables AS stats + JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' + JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace + WHERE stats.schemaname = current_schema() + AND stats.relname = 'lexical_search_index' + AND index_namespace.nspname = current_schema()` + ); + assert.equal(before.rowCount, 1); + + const receipt = await runPostgresDerivedIndexMaintenance({ + // pg_stat_user_tables updates n_dead_tup asynchronously. Zero gates + // make this real-database proof deterministic while still exercising + // both maintenance predicates against the sampled statistics. + minimumDeadTupleRatio: 0, + minimumDeadTuples: 0, + minimumTableBytes: 0, + now: new Date("2026-09-03T02:00:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + const reindexableIndexes = [ + "idx_pg_lexical_search_document", + "idx_pg_lexical_search_scope_document", + "idx_pg_semantic_search_scope", + "idx_pg_semantic_search_embedding_hnsw", + ]; + const presentIndexes = await postgresQuery<{ relname: string }>( + `SELECT index_class.relname + FROM pg_class AS index_class + JOIN pg_namespace AS namespace ON namespace.oid = index_class.relnamespace + WHERE namespace.nspname = current_schema() + AND index_class.relname = ANY($1::text[]) + ORDER BY index_class.relname`, + [reindexableIndexes] + ); + const after = await postgresQuery<{ index_oid: string; last_analyze: string | null; last_vacuum: string | null }>( + `SELECT stats.last_vacuum::text AS last_vacuum, + stats.last_analyze::text AS last_analyze, + index_class.oid::text AS index_oid + FROM pg_stat_user_tables AS stats + JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' + JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace + WHERE stats.schemaname = current_schema() + AND stats.relname = 'lexical_search_index' + AND index_namespace.nspname = current_schema()` + ); + + assert.equal(receipt.status, "completed"); + assert.deepEqual( + receipt.tables.map((table) => table.tableName), + ["blobs", "lexical_search_index", "record_changes", "records", "semantic_search_blob", "spine_events"], + "the scheduled vacuum samples every known heavy table" + ); + assert.ok(receipt.tables.some((table) => table.tableName === "lexical_search_index" && table.vacuumed)); + assert.deepEqual( + [...receipt.reindexedIndexNames].sort(), + presentIndexes.rows.map((row) => row.relname), + "only present indexes from the static search-index allowlist are rebuilt" + ); + assert.deepEqual(getLastPostgresDerivedIndexMaintenanceReceipt(), receipt); + assert.ok(after.rows[0]?.last_vacuum, "VACUUM (ANALYZE) updates Postgres's manual vacuum statistic"); + assert.ok(after.rows[0]?.last_analyze, "VACUUM (ANALYZE) updates Postgres's analyze statistic"); + assert.notEqual(after.rows[0]?.index_oid, before.rows[0]?.index_oid, "REINDEX CONCURRENTLY replaces the index relation"); + + const repeat = await runPostgresDerivedIndexMaintenance({ + minimumDeadTupleRatio: 0, + minimumDeadTuples: 0, + minimumTableBytes: 0, + now: new Date("2026-09-03T02:15:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + assert.equal(repeat.status, "already-completed", "the scheduler runs no more than once in one UTC window"); + const outsideWindow = await runPostgresDerivedIndexMaintenance({ + now: new Date("2026-09-03T04:00:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + assert.equal(outsideWindow.status, "outside-window"); + assert.deepEqual( + getLastPostgresDerivedIndexMaintenanceReceipt(), + receipt, + "routine outside-window polls do not hide the most recent completed maintenance receipt" + ); + } finally { + await closePostgresStorage(); + } + } + ); +}); diff --git a/reference-implementation/test/postgres-records-ingest-noop.test.ts b/reference-implementation/test/postgres-records-ingest-noop.test.ts index 4aabf6a25..bed17e5ac 100644 --- a/reference-implementation/test/postgres-records-ingest-noop.test.ts +++ b/reference-implementation/test/postgres-records-ingest-noop.test.ts @@ -27,12 +27,113 @@ import assert from "node:assert/strict"; import test from "node:test"; import { closeDb, initDb } from "../server/db.ts"; -import { postgresIngestRecord } from "../server/postgres-records.ts"; +import { + postgresIngestRecord, + postgresPersistContentAddressedBlob, + postgresPrepareDeviceFinalRecords, +} from "../server/postgres-records.ts"; import { closePostgresStorage, initPostgresStorage, postgresQuery } from "../server/postgres-storage.ts"; +import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; if (POSTGRES_URL) { + test("N identical large-payload reconciliations leave records and history physically flat", async () => { + const databaseName = `pdpp_large_payload_noop_${Date.now().toString(36)}`; + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName }, + async (databaseUrl) => { + initDb(":memory:"); + await initPostgresStorage({ backend: "postgres", databaseUrl }); + try { + const storageTarget = { connectorId: "pg_large_payload_noop", connectorInstanceId: "cin_large_payload_noop" }; + const record = { + data: { body: "x".repeat(256 * 1024), id: "large-record" }, + emitted_at: "2026-09-03T12:00:00.000Z", + key: "large-record", + op: "upsert" as const, + stream: "items", + }; + const attemptContext = { + streams: { items: { consentTimeField: null, cursorField: "id", primaryKey: ["id"] } }, + }; + const blobData = Buffer.alloc(256 * 1024, 0xa5); + const first = await postgresIngestRecord(storageTarget, record, { attemptContext }); + assert.equal(first.changed, true); + const firstBlob = await postgresPersistContentAddressedBlob({ + connectorId: storageTarget.connectorId, + connectorInstanceId: storageTarget.connectorInstanceId, + data: blobData, + mimeType: "application/octet-stream", + recordKey: "large-record", + stream: "items", + }); + assert.equal(firstBlob.binding_inserted, true); + const before = await postgresQuery<{ bytes: string; relname: string }>( + `SELECT relname, pg_total_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = ANY($1::text[]) + ORDER BY relname`, + [["blobs", "blob_bindings", "records", "record_changes"]] + ); + + for (let attempt = 0; attempt < 20; attempt += 1) { + // biome-ignore lint/performance/noAwaitInLoops: each call is one real reconciliation transaction. + const repeated = await postgresIngestRecord(storageTarget, record, { attemptContext }); + assert.equal(repeated.changed, false, `reconciliation ${attempt + 1} must be a durable no-op`); + // This preparation path recomputes record metadata from the same + // attempt facts; it must not rewrite equal large JSONB rows. + await postgresPrepareDeviceFinalRecords( + storageTarget, + [{ record: { key: "large-record", stream: "items" } }], + attemptContext + ); + // biome-ignore lint/performance/noAwaitInLoops: each call is one real content-addressed blob write transaction. + const repeatedBlob = await postgresPersistContentAddressedBlob({ + connectorId: storageTarget.connectorId, + connectorInstanceId: storageTarget.connectorInstanceId, + data: blobData, + mimeType: "application/octet-stream", + recordKey: "large-record", + stream: "items", + }); + assert.equal(repeatedBlob.binding_inserted, false, `blob reconciliation ${attempt + 1} must be a durable no-op`); + } + await postgresQuery("ANALYZE records"); + await postgresQuery("ANALYZE record_changes"); + await postgresQuery("ANALYZE blobs"); + await postgresQuery("ANALYZE blob_bindings"); + const after = await postgresQuery<{ + bytes: string; + n_dead_tup: string; + n_live_tup: string; + relname: string; + }>( + `SELECT relname, n_live_tup::text, n_dead_tup::text, pg_total_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = ANY($1::text[]) + ORDER BY relname`, + [["blobs", "blob_bindings", "records", "record_changes"]] + ); + + assert.equal(after.rows.length, 4, "fixture measures JSONB and BYTEA large-payload storage relations"); + for (const row of after.rows) { + const baseline = before.rows.find((candidate) => candidate.relname === row.relname); + assert.ok(baseline, `fixture captured ${row.relname} before reconciliation`); + assert.ok(Number(row.n_dead_tup) <= 1, `${row.relname} leaves no meaningful dead tuples`); + assert.ok( + Number(row.bytes) <= Number(baseline.bytes) + 8192, + `${row.relname} stays within one Postgres page after repeated large payloads` + ); + } + } finally { + await closePostgresStorage(); + closeDb(); + } + } + ); + }); + test("postgres byte-identical re-ingest does not allocate a new version", async () => { const suffix = `${Date.now()}_${Math.floor(Math.random() * 1e6)}`; const connectorId = `pg_noop_${suffix}`; diff --git a/reference-implementation/test/records-delete-postgres-routing.test.ts b/reference-implementation/test/records-delete-postgres-routing.test.ts index 0321403fd..697020e10 100644 --- a/reference-implementation/test/records-delete-postgres-routing.test.ts +++ b/reference-implementation/test/records-delete-postgres-routing.test.ts @@ -44,6 +44,57 @@ import { deleteAllRecords, deleteAllRecordsForConnector } from "../server/record const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; if (POSTGRES_URL) { + test("Postgres bootstrap applies the bloat-control autovacuum policy to heap tables and TOAST", async () => { + initDb(":memory:"); + await initPostgresStorage({ backend: "postgres", databaseUrl: POSTGRES_URL }); + + try { + const result = await postgresQuery<{ + relname: string; + reloptions: string[] | null; + toast_reloptions: string[] | null; + }>( + `SELECT heap.relname, heap.reloptions, toast.reloptions AS toast_reloptions + FROM pg_class AS heap + LEFT JOIN pg_class AS toast ON toast.oid = heap.reltoastrelid + WHERE heap.relnamespace = current_schema()::regnamespace + AND heap.relname = ANY($1::text[])`, + [["records", "record_changes", "blobs", "spine_events"]] + ); + const expectedHeapOptions = [ + "autovacuum_enabled=true", + "autovacuum_vacuum_threshold=1", + "autovacuum_vacuum_scale_factor=0.01", + "autovacuum_vacuum_insert_threshold=50", + "autovacuum_vacuum_insert_scale_factor=0.02", + "autovacuum_analyze_threshold=50", + "autovacuum_analyze_scale_factor=0.02", + ]; + const expectedToastOptions = [ + "autovacuum_enabled=true", + "autovacuum_vacuum_threshold=1", + "autovacuum_vacuum_scale_factor=0.01", + "autovacuum_vacuum_insert_threshold=50", + "autovacuum_vacuum_insert_scale_factor=0.02", + ]; + + assert.equal(result.rows.length, 4, "all high-churn tables exist in the active schema"); + for (const row of result.rows) { + const heapOptions = new Set(row.reloptions ?? []); + const toastOptions = new Set(row.toast_reloptions ?? []); + for (const option of expectedHeapOptions) { + assert.ok(heapOptions.has(option), `${row.relname} heap sets ${option}`); + } + for (const option of expectedToastOptions) { + assert.ok(toastOptions.has(option), `${row.relname} TOAST sets ${option}`); + } + } + } finally { + await closePostgresStorage(); + closeDb(); + } + }); + test("deleteAllRecordsForConnector invalidates Postgres-backed records", async () => { const suffix = `${Date.now()}_${Math.floor(Math.random() * 1e6)}`; const connectorId = `https://registry.pdpp.test/connectors/pg_invalidate_${suffix}`; @@ -168,8 +219,9 @@ if (POSTGRES_URL) { const suffix = `${Date.now()}_${Math.floor(Math.random() * 1e6)}`; const connectorId = `https://registry.pdpp.test/connectors/pg_stream_delete_${suffix}`; const connectorInstanceId = `cin_pg_stream_delete_${suffix}`; - const streamTarget = "top_artists"; - const streamSibling = "saved_tracks"; + // `%` would match the sibling under an unescaped SQL LIKE predicate. + const streamTarget = "a%b"; + const streamSibling = "axb"; initDb(":memory:"); await initPostgresStorage({ backend: "postgres", databaseUrl: POSTGRES_URL }); @@ -200,6 +252,19 @@ if (POSTGRES_URL) { op: "upsert", stream: streamSibling, }); + await postgresQuery( + `INSERT INTO semantic_search_blob (connector_id, connector_instance_id, scope_key, record_key, embedding) + VALUES ($1, $2, $3, $4, $5::jsonb), ($1, $2, $6, $7, $5::jsonb)`, + [ + connectorId, + connectorInstanceId, + JSON.stringify([streamTarget, "body"]), + "a-1", + "[0.25, 0.75]", + JSON.stringify([streamSibling, "body"]), + "s-1", + ] + ); const targetBaseline = await postgresQuery( `SELECT COUNT(*)::int AS count FROM records @@ -251,12 +316,24 @@ if (POSTGRES_URL) { [connectorInstanceId, streamSibling] ); assert.equal(Number(siblingCounter.rows[0]?.count || 0), 1, "sibling stream version_counter row is untouched"); + + const siblingSemantic = await postgresQuery( + `SELECT COUNT(*)::int AS count FROM semantic_search_blob + WHERE connector_instance_id = $1 AND scope_key = $2`, + [connectorInstanceId, JSON.stringify([streamSibling, "body"])] + ); + assert.equal( + Number(siblingSemantic.rows[0]?.count || 0), + 1, + "a wildcard-like stream name cannot delete a sibling semantic scope during record cleanup" + ); } finally { try { await postgresQuery("DELETE FROM blob_bindings WHERE connector_id = $1", [connectorId]); await postgresQuery("DELETE FROM record_changes WHERE connector_id = $1", [connectorId]); await postgresQuery("DELETE FROM records WHERE connector_id = $1", [connectorId]); await postgresQuery("DELETE FROM version_counter WHERE connector_instance_id = $1", [connectorInstanceId]); + await postgresQuery("DELETE FROM semantic_search_blob WHERE connector_instance_id = $1", [connectorInstanceId]); // biome-ignore lint/suspicious/noEmptyBlockStatements: intentional no-op test double represents an optional side effect. } catch {} await closePostgresStorage(); diff --git a/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts b/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts new file mode 100644 index 000000000..d5cc59c94 --- /dev/null +++ b/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts @@ -0,0 +1,205 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; +import { postgresLexicalIndexPublishWithClient, postgresSemanticIndexPublishWithClient } from "../server/postgres-search.ts"; +import { + closePostgresStorage, + initPostgresStorage, + postgresQuery, + withPostgresTransaction, +} from "../server/postgres-storage.ts"; +import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; + +const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; +const CONNECTOR_ID = "semantic-churn-fixture"; +const INSTANCE_ID = "cin_semantic_churn_fixture"; +const STREAM = "messages"; +const RECORD_KEY = "rec-1"; +const BODY_SCOPE_KEY = JSON.stringify([STREAM, "body"]); +const SUBJECT_SCOPE_KEY = JSON.stringify([STREAM, "subject"]); +const SUMMARY_SCOPE_KEY = JSON.stringify([STREAM, "summary"]); + +interface SemanticEntry { + scopeKey: string; + vector: number[]; +} + +async function publishForStream(stream: string, entries: readonly SemanticEntry[]): Promise { + await withPostgresTransaction((client) => + postgresSemanticIndexPublishWithClient(client, { + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + entries: entries.map((entry) => ({ ...entry, recordKey: RECORD_KEY })), + recordKey: RECORD_KEY, + stream, + }) + ); +} + +async function publish(entries: readonly SemanticEntry[]): Promise { + await publishForStream(STREAM, entries); +} + +interface SemanticRow { + ctid: string; + embedding: string; + scopeKey: string; +} + +async function semanticRows(): Promise { + const result = await postgresQuery( + `SELECT ctid::text AS ctid, embedding::text AS embedding, scope_key AS "scopeKey" + FROM semantic_search_blob + WHERE connector_instance_id = $1 AND record_key = $2 + ORDER BY scope_key`, + [INSTANCE_ID, RECORD_KEY] + ); + return result.rows; +} + +async function publishBothDerivedIndexes(): Promise { + await withPostgresTransaction(async (client) => { + await postgresLexicalIndexPublishWithClient(client, { + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + fields: { body: "a stable body", subject: "a stable subject" }, + recordKey: RECORD_KEY, + stream: STREAM, + }); + await postgresSemanticIndexPublishWithClient(client, { + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + entries: [{ recordKey: RECORD_KEY, scopeKey: BODY_SCOPE_KEY, vector: [0.25, 0.75] }], + recordKey: RECORD_KEY, + stream: STREAM, + }); + }); +} + +function rowForScope(rows: readonly SemanticRow[], scopeKey: string): SemanticRow { + const row = rows.find((candidate) => candidate.scopeKey === scopeKey); + assert.ok(row, `fixture must contain ${scopeKey}`); + return row; +} + +if (POSTGRES_URL) { + test("semantic publish writes only changed scopes and reconciles stale scopes", async () => { + const databaseName = `pdpp_semantic_churn_${Date.now().toString(36)}`; + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName }, + async (url) => { + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + + await publish([{ scopeKey: BODY_SCOPE_KEY, vector: [0.25, 0.75] }]); + const initialBody = rowForScope(await semanticRows(), BODY_SCOPE_KEY); + + await publish([{ scopeKey: BODY_SCOPE_KEY, vector: [0.25, 0.75] }]); + assert.deepEqual( + rowForScope(await semanticRows(), BODY_SCOPE_KEY), + initialBody, + "identical embeddings must leave their physical rows untouched" + ); + + await publish([{ scopeKey: BODY_SCOPE_KEY, vector: [0.5, 0.5] }]); + const changedBody = rowForScope(await semanticRows(), BODY_SCOPE_KEY); + assert.notEqual(changedBody.ctid, initialBody.ctid, "changed embeddings must update their row"); + assert.notEqual(changedBody.embedding, initialBody.embedding, "changed embeddings must persist their value"); + + await publish([ + { scopeKey: BODY_SCOPE_KEY, vector: [0.5, 0.5] }, + { scopeKey: SUBJECT_SCOPE_KEY, vector: [0.25, 0.75] }, + ]); + const withSubject = await semanticRows(); + assert.equal( + rowForScope(withSubject, BODY_SCOPE_KEY).ctid, + changedBody.ctid, + "adding a scope must not rewrite equal existing scopes" + ); + + await publish([ + { scopeKey: BODY_SCOPE_KEY, vector: [0.5, 0.5] }, + { scopeKey: SUMMARY_SCOPE_KEY, vector: [0.25, 0.75] }, + ]); + const reconciled = await semanticRows(); + assert.equal(reconciled.length, 2, "stale scopes must be removed and new scopes inserted"); + assert.equal( + rowForScope(reconciled, BODY_SCOPE_KEY).ctid, + changedBody.ctid, + "removing a different scope must not rewrite equal existing scopes" + ); + assert.equal( + reconciled.some((row) => row.scopeKey === SUBJECT_SCOPE_KEY), + false, + "removed scopes must not remain indexed" + ); + assert.ok(reconciled.some((row) => row.scopeKey === SUMMARY_SCOPE_KEY), "new scopes must be indexed"); + + await publish([]); + assert.deepEqual(await semanticRows(), [], "an empty publish must remove all semantic scopes for the record"); + + const wildcardStream = "a%b"; + const literalStream = "axb"; + const wildcardScopeKey = JSON.stringify([wildcardStream, "body"]); + const literalScopeKey = JSON.stringify([literalStream, "body"]); + await publishForStream(wildcardStream, [{ scopeKey: wildcardScopeKey, vector: [0.25, 0.75] }]); + await publishForStream(literalStream, [{ scopeKey: literalScopeKey, vector: [0.5, 0.5] }]); + await publishForStream(wildcardStream, []); + assert.ok( + (await semanticRows()).some((row) => row.scopeKey === literalScopeKey), + "a stream name containing SQL LIKE wildcards must not delete another stream's scope" + ); + } + ); + }); + + test("N identical derived-index reconciliation publishes leave dead-tuple pressure and relation size bounded", async () => { + const databaseName = `pdpp_derived_index_bloat_${Date.now().toString(36)}`; + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName }, + async (url) => { + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + await publishBothDerivedIndexes(); + const before = await postgresQuery<{ bytes: string; relname: string }>( + `SELECT relname, pg_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = ANY($1::text[]) + ORDER BY relname`, + [["lexical_search_index", "semantic_search_blob"]] + ); + + for (let attempt = 0; attempt < 20; attempt += 1) { + // biome-ignore lint/performance/noAwaitInLoops: each transaction is one production reconciliation publish. + await publishBothDerivedIndexes(); + } + await postgresQuery("ANALYZE lexical_search_index", []); + await postgresQuery("ANALYZE semantic_search_blob", []); + const after = await postgresQuery<{ bytes: string; n_dead_tup: string; n_live_tup: string; relname: string }>( + `SELECT relname, n_live_tup::text, n_dead_tup::text, pg_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = ANY($1::text[]) + ORDER BY relname`, + [["lexical_search_index", "semantic_search_blob"]] + ); + + assert.equal(after.rows.length, 2, "fixture must measure both derived Postgres relations"); + for (const row of after.rows) { + const baseline = before.rows.find((candidate) => candidate.relname === row.relname); + assert.ok(baseline, `fixture captured ${row.relname} before repeated publishes`); + const live = Number(row.n_live_tup); + const dead = Number(row.n_dead_tup); + assert.ok(dead / Math.max(live, 1) <= 0.1, `${row.relname} dead/live ratio stays under 10% after 20 no-op publishes`); + assert.ok( + Number(row.bytes) <= Number(baseline.bytes) + 8192, + `${row.relname} stays within one Postgres page of its initial relation size` + ); + } + } + ); + }); +} else { + test("semantic index write-elision (skipped: PDPP_TEST_POSTGRES_URL unset)", { skip: true }, () => { + /* intentionally empty */ + }); +} From 8e595289721e5c89f919a280ea64e4b541079743 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Thu, 3 Sep 2026 17:50:21 -0500 Subject: [PATCH 02/11] docs: record postgres bloat maintenance findings Capture the PostgreSQL maintenance constraints, the source-level write-elision decision, and the devspecs task-index friction observed during this repair. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../260903-postgres-bloat-maintenance.md | 24 +++++++++++++++++++ ai/research/INDEX.md | 3 +++ inbox/devspecs-feedback.md | 5 ++++ 3 files changed, 32 insertions(+) create mode 100644 ai/research/260903-postgres-bloat-maintenance.md create mode 100644 ai/research/INDEX.md create mode 100644 inbox/devspecs-feedback.md diff --git a/ai/research/260903-postgres-bloat-maintenance.md b/ai/research/260903-postgres-bloat-maintenance.md new file mode 100644 index 000000000..534b561b2 --- /dev/null +++ b/ai/research/260903-postgres-bloat-maintenance.md @@ -0,0 +1,24 @@ +# PostgreSQL bloat maintenance + +Date: 2026-09-03 + +## Question + +How should the reference implementation prevent high-churn PostgreSQL tables from accumulating physical bloat, and how can it reclaim derived-index space without stopping writers? + +## Findings + +- PostgreSQL supports table-specific autovacuum parameters with `ALTER TABLE ... SET (...)`; the same reloptions can address the table's TOAST relation with the `toast.` prefix. The relevant vacuum trigger is a threshold plus a scale-factor term, so low scale factors alone do not protect a table that has only a few very large JSONB or BYTEA rows. +- `VACUUM (ANALYZE)` cannot run inside a transaction block. `REINDEX ... CONCURRENTLY` also cannot run inside a transaction block, so the maintenance runner must borrow a direct session rather than use the transaction helper. +- A concurrent reindex allows normal writes but has stricter operational constraints. The runner therefore rebuilds only an explicit, static allowlist of derived indexes after observed dead-tuple pressure crosses a conservative threshold. + +## Decision + +The source-level fix is write elision: repeated structural-equality record payloads do not mutate `records` or append `record_changes`; content-addressed `blobs` already does the same for binary payloads. The implementation also avoids no-op metadata updates on that record path. These properties are separately proven with physical PostgreSQL relation statistics. Per-table heap and TOAST autovacuum reloptions for `records`, `record_changes`, `blobs`, and `spine_events`, plus the default 01:00-05:00 UTC maintenance job, are recovery safety nets only; they do not make a rewriting source path correct. The job vacuums known heavy tables and considers concurrent rebuild only for a static search-index allowlist. A durable database receipt claims each UTC window so a restart or another replica does not repeat an expensive partial pass. Set `PDPP_POSTGRES_DERIVED_INDEX_MAINTENANCE_WINDOW=disabled` to opt out or set another UTC window. A health receipt shows the last completed local job outcome. + +## Sources + +- PostgreSQL: [Routine Vacuuming](https://www.postgresql.org/docs/current/routine-vacuuming.html) +- PostgreSQL: [Automatic Vacuuming](https://www.postgresql.org/docs/current/runtime-config-vacuum.html) +- PostgreSQL: [REINDEX](https://www.postgresql.org/docs/current/sql-reindex.html) +- PostgreSQL: [CREATE TABLE storage parameters](https://www.postgresql.org/docs/current/sql-createtable.html) diff --git a/ai/research/INDEX.md b/ai/research/INDEX.md new file mode 100644 index 000000000..ea042b1aa --- /dev/null +++ b/ai/research/INDEX.md @@ -0,0 +1,3 @@ +# Research index + +- [260903 PostgreSQL bloat maintenance](260903-postgres-bloat-maintenance.md) — table-level autovacuum options and concurrent reindex constraints for the 2026-09-03 bloat repair. diff --git a/inbox/devspecs-feedback.md b/inbox/devspecs-feedback.md new file mode 100644 index 000000000..55ec426d3 --- /dev/null +++ b/inbox/devspecs-feedback.md @@ -0,0 +1,5 @@ +# devspecs feedback + +## 2026-09-03 — DB bloat repair + +`ds task "fix PostgreSQL storage bloat" --slice ...` waited at “Task index preflight: waiting for another index update” for more than 30 seconds and never produced a task slice. The command gave no owner, timeout, or recovery action, so I continued with the repository brief and targeted tests. A bounded wait plus a suggested retry/status command would make this easier to use during incident work. From 85b66d939cc821dcba6c66260bf13380d01dd127 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Thu, 3 Sep 2026 18:45:10 -0500 Subject: [PATCH 03/11] fix(postgres): guard lexical backfill writes and reclaim orphaned blobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the reconciliation-bloat work in this PR, addressing three gaps found in review. `postgresLexicalIndexInsertMany` — the backfill writer — lacked the `IS DISTINCT FROM` guard its semantic twin received, so re-running a backfill over unchanged text rewrote every row and left one dead tuple per row. The guard elides those writes. No caller inspects the statement's `rowCount` (`rebuildLexicalInsertEntries` in search.ts discards the result), so this is write-elision only. Note this is a different statement from `postgresLexicalIndexPublishWithClient`, whose elision landed in #238. The per-stream arm of the connector-wide delete dropped `blob_bindings` but never deleted the `blobs` rows those bindings were the last reference to. Nothing in the codebase collects orphaned blobs, so the bytes leaked permanently. Both backends now run the refcount-gated reclaim their whole-connection siblings already used (`deleteConnectionRecordRowsPostgres`, `delete-blobs-by-instance.sql`). The reclaim is refcount-gated rather than supersede-and-delete because blob rows are globally content-addressed — the insert conflicts on `blob_id` alone, so identical bytes from a sibling connection share one row — and the `blob_bindings` FK is `ON DELETE CASCADE`, so an ungated delete would destroy a live sibling's binding. Deleting at supersession is separately unsafe: `record_changes` retains `data.blob_ref.blob_id` for every superseded revision and history pruning is off by default, so those revisions stay readable and would be handed URLs that 404. A fixture pins both invariants. Also fixes a test regression: `records-delete-postgres-routing.test.ts` cast its seed embedding to `::jsonb`, but `semantic_search_blob.embedding` is `vector` under pgvector, which is the production configuration. The cast now follows the same branch the production writer uses. Verified against a throwaway pgvector/pgvector:pg16 database. Each new assertion was mutation-checked: it fails when its guard is removed. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../server/postgres-search.ts | 12 +- .../server/queries/index.ts | 2 + .../records/delete/delete-blobs-by-stream.sql | 19 + reference-implementation/server/records.ts | 31 ++ .../test/postgres-orphan-blob-reclaim.test.ts | 400 ++++++++++++++++++ .../records-delete-postgres-routing.test.ts | 17 +- ...ntic-index-skip-unchanged-postgres.test.ts | 113 ++++- 7 files changed, 590 insertions(+), 4 deletions(-) create mode 100644 reference-implementation/server/queries/records/delete/delete-blobs-by-stream.sql create mode 100644 reference-implementation/test/postgres-orphan-blob-reclaim.test.ts diff --git a/reference-implementation/server/postgres-search.ts b/reference-implementation/server/postgres-search.ts index ffb7446db..d0daa4e5a 100644 --- a/reference-implementation/server/postgres-search.ts +++ b/reference-implementation/server/postgres-search.ts @@ -261,6 +261,14 @@ export async function postgresLexicalIndexInsertMany({ // row a concurrent delete/newer-write has since superseded is silently // skipped rather than resurrected/overwritten. See // harden-connector-instance-write-fence-transaction-native. + // + // Write-elided: the `IS DISTINCT FROM` guard on the conflict update means a + // tuple whose indexed columns already match is left physically untouched. + // Without it, a backfill that re-reads unchanged text rewrites every row and + // leaves a dead tuple behind for each — the same bloat the semantic upsert in + // `insertSemanticRows` avoids. `rowCount` is not inspected by any caller + // (the function reports `entries.length`), so eliding equal writes does not + // change observable behavior. await postgresQuery( `INSERT INTO lexical_search_index (connector_id, connector_instance_id, stream, record_key, field, value) SELECT $1, $2, $3, rows.record_key, rows.field, rows.value @@ -273,7 +281,9 @@ export async function postgresLexicalIndexInsertMany({ AND r.deleted = FALSE ON CONFLICT (connector_instance_id, stream, record_key, field) DO UPDATE SET connector_id = EXCLUDED.connector_id, - value = EXCLUDED.value`, + value = EXCLUDED.value + WHERE (lexical_search_index.connector_id, lexical_search_index.value) + IS DISTINCT FROM (EXCLUDED.connector_id, EXCLUDED.value)`, [ connectorId, connectorInstanceId, diff --git a/reference-implementation/server/queries/index.ts b/reference-implementation/server/queries/index.ts index 93936ba60..d8f716527 100644 --- a/reference-implementation/server/queries/index.ts +++ b/reference-implementation/server/queries/index.ts @@ -404,6 +404,7 @@ export interface ReferenceQueryRegistry extends Readonly { @@ -6788,6 +6793,32 @@ async function postgresDeleteAllRecordsForConnector(connectorId: string, instanc connectorInstanceId, stream, ]); + // Reclaim blobs this delete just unbound, mirroring + // `deleteConnectionRecordRowsPostgres` (and SQLite's + // `delete-blobs-by-instance.sql`). Dropping the bindings above without + // this left the `blobs` rows behind permanently — nothing else in the + // codebase collects orphans, so those bytes were junk forever. + // + // Refcount-gated, NOT supersede-and-delete. `blobs` is globally + // content-addressed (the insert conflicts on `blob_id` alone, with no + // connector or instance in the conflict target), so identical bytes + // from a sibling connection share ONE row; and the FK from + // `blob_bindings` is ON DELETE CASCADE, so an ungated delete here + // would silently destroy a live sibling's binding. `NOT EXISTS` + // deletes only rows no binding still references. Scoped to this + // instance for the same reason the whole-connection sibling is: a + // delete reclaims its own bytes, never another connection's. + await postgresQuery( + `DELETE FROM blobs + WHERE connector_instance_id = $1 + AND stream = $2 + AND NOT EXISTS ( + SELECT 1 + FROM blob_bindings + WHERE blob_bindings.blob_id = blobs.blob_id + )`, + [connectorInstanceId, stream] + ); await markRetainedSizeStreamDirty({ connectorInstanceId, stream }); // Parity with the SQLite arm above: a connector-wide record delete // changes this connection's count/stream evidence and must mark the diff --git a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts new file mode 100644 index 000000000..80d151b9e --- /dev/null +++ b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts @@ -0,0 +1,400 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +/** + * Orphan-blob reclamation at the per-stream delete site. + * + * `blobs` rows are content-addressed and GLOBALLY deduplicated: the insert in + * `postgresPersistContentAddressedBlob` conflicts on `blob_id` alone, with no + * connector or instance in the conflict target, so byte-identical content + * uploaded by two different connections resolves to ONE `blobs` row plus two + * `blob_bindings` rows. The FK from `blob_bindings.blob_id` is + * `ON DELETE CASCADE`, so deleting a `blobs` row destroys every sibling + * binding — including bindings owned by a connection that was never deleted. + * + * That is why blob reclamation must be refcount-gated rather than + * supersede-and-delete. The whole-connection delete already gets this right + * (`deleteConnectionRecordRowsPostgres` in records.ts, and the SQLite sibling + * `delete-blobs-by-instance.sql`): it deletes bindings first, then deletes + * only those `blobs` rows that no binding still references. + * + * The per-stream arm of the connector-wide delete did NOT: it dropped + * `blob_bindings` for the stream and left the now-unreferenced `blobs` rows + * behind forever, since no orphan collector exists anywhere in the codebase. + * These fixtures pin both halves of the corrected behavior: + * + * 1. a blob whose last binding the delete removes is reclaimed, and + * 2. a blob still bound by a LIVE sibling connection survives. + * + * Env gate: PDPP_TEST_POSTGRES_URL must be set. + */ + +import assert from "node:assert/strict" +import test from "node:test" + +import { exec, getOne, referenceQueries } from "../lib/db.ts" +import { closeDb, initDb } from "../server/db.ts" +import { + postgresIngestRecord, + postgresPersistContentAddressedBlob, +} from "../server/postgres-records.ts" +import { + closePostgresStorage, + initPostgresStorage, + postgresQuery, +} from "../server/postgres-storage.ts" +import { deleteAllRecordsForConnector } from "../server/records.ts" +import { getChangeHistoryLimit } from "../server/storage-utils.ts" +import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts" + +const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL +const STREAM = "attachments" + +async function countBlobs(blobId: string): Promise { + const result = await postgresQuery<{ count: number }>( + "SELECT COUNT(*)::int AS count FROM blobs WHERE blob_id = $1", + [blobId] + ) + return Number(result.rows[0]?.count || 0) +} + +async function countBindings(blobId: string): Promise { + const result = await postgresQuery<{ count: number }>( + "SELECT COUNT(*)::int AS count FROM blob_bindings WHERE blob_id = $1", + [blobId] + ) + return Number(result.rows[0]?.count || 0) +} + +/** Ingest a record that carries `data.blob_ref.blob_id`, then persist its bytes. */ +async function seedRecordWithBlob({ + connectorId, + connectorInstanceId, + recordKey, + bytes, +}: { + bytes: Buffer + connectorId: string + connectorInstanceId: string + recordKey: string +}): Promise { + const persisted = await postgresPersistContentAddressedBlob({ + connectorId, + connectorInstanceId, + data: bytes, + mimeType: "application/octet-stream", + recordKey, + stream: STREAM, + }) + await postgresIngestRecord( + { connector_id: connectorId, connector_instance_id: connectorInstanceId }, + { + data: { blob_ref: { blob_id: persisted.blob_id }, id: recordKey }, + emitted_at: "2026-09-03T00:00:00.000Z", + key: recordKey, + op: "upsert", + stream: STREAM, + } + ) + return persisted.blob_id +} + +if (POSTGRES_URL) { + test("per-stream connector delete reclaims blobs whose last binding it removed", async () => { + const databaseName = `pdpp_orphan_blob_reclaim_${Date.now().toString(36)}` + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + try { + const connectorId = + "https://registry.pdpp.test/connectors/orphan_blob_reclaim" + const connectorInstanceId = "cin_orphan_blob_reclaim" + const blobId = await seedRecordWithBlob({ + bytes: Buffer.alloc(64 * 1024, 0x5a), + connectorId, + connectorInstanceId, + recordKey: "att-1", + }) + + assert.equal( + await countBlobs(blobId), + 1, + "baseline: the blob row exists" + ) + assert.equal( + await countBindings(blobId), + 1, + "baseline: exactly one binding references it" + ) + + await deleteAllRecordsForConnector(connectorId) + + assert.equal( + await countBindings(blobId), + 0, + "the delete drops the stream's blob_bindings" + ) + assert.equal( + await countBlobs(blobId), + 0, + "a blob left with no binding is reclaimed rather than leaked as junk" + ) + } finally { + await closePostgresStorage() + closeDb() + } + } + ) + }) + + test("per-stream connector delete retains a blob still bound by a live sibling connection", async () => { + const databaseName = `pdpp_shared_blob_retained_${Date.now().toString(36)}` + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + try { + // Byte-identical content under two different connectors. Global + // content addressing collapses them onto ONE `blobs` row with two + // bindings, so deleting the row on behalf of either connector would + // cascade away the other's binding and break a live record. + const sharedBytes = Buffer.alloc(64 * 1024, 0x5a) + const doomedConnectorId = + "https://registry.pdpp.test/connectors/shared_blob_doomed" + const survivingConnectorId = + "https://registry.pdpp.test/connectors/shared_blob_survivor" + + const doomedBlobId = await seedRecordWithBlob({ + bytes: sharedBytes, + connectorId: doomedConnectorId, + connectorInstanceId: "cin_shared_blob_doomed", + recordKey: "att-1", + }) + const survivingBlobId = await seedRecordWithBlob({ + bytes: sharedBytes, + connectorId: survivingConnectorId, + connectorInstanceId: "cin_shared_blob_survivor", + recordKey: "att-1", + }) + assert.equal( + doomedBlobId, + survivingBlobId, + "fixture premise: identical bytes dedupe to a single content-addressed blob row" + ) + assert.equal( + await countBindings(doomedBlobId), + 2, + "baseline: two connections bind the same blob" + ) + + await deleteAllRecordsForConnector(doomedConnectorId) + + assert.equal( + await countBlobs(doomedBlobId), + 1, + "a blob another live connection still binds must NOT be deleted" + ) + const survivorBindings = await postgresQuery<{ + connector_id: string + }>("SELECT connector_id FROM blob_bindings WHERE blob_id = $1", [ + doomedBlobId, + ]) + assert.deepEqual( + survivorBindings.rows.map(row => row.connector_id), + [survivingConnectorId], + "the surviving connection keeps its binding; only the deleted connector's binding is removed" + ) + } finally { + await closePostgresStorage() + closeDb() + } + } + ) + }) + /** + * Why superseded blobs are NOT deleted at the point of supersession. + * + * The obvious "never create junk" move — delete the old blob in the same + * transaction that writes the new one — is unsafe here, and this fixture + * pins the two schema facts that make it unsafe so a future attempt fails + * loudly instead of silently breaking reads: + * + * 1. `record_changes` retains `data.blob_ref.blob_id` for EVERY superseded + * revision, and change-history pruning is off by default + * (`getChangeHistoryLimit()` reads PDPP_CHANGE_HISTORY_LIMIT, default + * 0 = unbounded). A `changes_since` reader can therefore still be + * handed a superseded revision; `decorateRecordBlobRefs` emits its + * `fetch_url` without checking the blob exists, so deleting the bytes + * turns that into a URL that 404s. + * 2. Blob rows are globally content-addressed, so a "superseded" blob may + * be byte-identical to one a different live record still uses — the + * case the sibling-connection fixture above covers. + * + * So reclamation is bound to the points where a reference is genuinely + * dropped (connection delete, and now per-stream delete), refcount-gated, + * rather than to supersession. If history pruning is ever made the default + * or `record_changes` stops carrying blob refs, revisit this. + */ + test("superseded revisions keep their blob reference in history, so supersession must not delete blobs", async () => { + const databaseName = `pdpp_superseded_blob_refs_${Date.now().toString(36)}` + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + try { + const connectorId = + "https://registry.pdpp.test/connectors/superseded_blob_refs" + const connectorInstanceId = "cin_superseded_blob_refs" + const blobIds: string[] = [] + for (let revision = 0; revision < 3; revision += 1) { + blobIds.push( + // biome-ignore lint/performance/noAwaitInLoops: each revision is one sequential ingest. + await seedRecordWithBlob({ + bytes: Buffer.alloc(1024, revision), + connectorId, + connectorInstanceId, + recordKey: "att-1", + }) + ) + } + assert.equal( + new Set(blobIds).size, + 3, + "fixture premise: each changed payload mints a distinct blob" + ) + + assert.equal( + getChangeHistoryLimit(), + 0, + "change history is unbounded by default, so superseded revisions stay readable indefinitely" + ) + + const history = await postgresQuery<{ blob_id: string | null }>( + `SELECT record_json->'blob_ref'->>'blob_id' AS blob_id + FROM record_changes + WHERE connector_instance_id = $1 + ORDER BY version`, + [connectorInstanceId] + ) + assert.deepEqual( + history.rows.map(row => row.blob_id), + blobIds, + "every superseded revision still names its own blob, including the ones no longer current" + ) + + // The superseded bytes are consequently still present and reachable + // from history. Deleting them at supersession would leave these + // revisions pointing at blobs that no longer exist. + const supersededBlobIds = blobIds.slice(0, -1) + for (const blobId of supersededBlobIds) { + assert.equal( + // biome-ignore lint/performance/noAwaitInLoops: small fixed fixture set. + await countBlobs(blobId), + 1, + `superseded blob ${blobId} is retained while history still references it` + ) + } + } finally { + await closePostgresStorage() + closeDb() + } + } + ) + }) +} else { + test( + "postgres orphan-blob reclamation (skipped: PDPP_TEST_POSTGRES_URL unset)", + { skip: true }, + () => { + /* intentionally empty */ + } + ) +} + +/** + * SQLite arm of the same reclaim. Runs unconditionally — no Postgres needed — + * because the SQLite per-stream delete had the identical gap and now runs the + * identical refcount-gated pair (`recordsDeleteDeleteBlobBindingsByStream` + * then `recordsDeleteDeleteBlobsByStream`). Both halves are asserted here: + * reclaim the unbound blob, retain the shared one. + */ +test("sqlite per-stream connector delete reclaims orphans but retains shared blobs", async () => { + initDb(":memory:") + try { + const connectorId = + "https://registry.pdpp.test/connectors/sqlite_orphan_blob" + const doomedInstanceId = "cin_sqlite_orphan_doomed" + const sharedBlobId = + "blob_sha256_1111111111111111111111111111111111111111111111111111111111111111" + const orphanBlobId = + "blob_sha256_2222222222222222222222222222222222222222222222222222222222222222" + + const insertBlob = (blobId: string, instanceId: string) => { + exec(referenceQueries.blobsInsertBlob, [ + blobId, + connectorId, + instanceId, + STREAM, + "att-1", + "application/octet-stream", + 16, + blobId.replace("blob_sha256_", ""), + Buffer.alloc(16, 1), + ]) + } + const insertBinding = (blobId: string, instanceId: string) => { + exec(referenceQueries.blobsInsertBinding, [ + blobId, + connectorId, + instanceId, + STREAM, + "att-1", + ]) + } + + // `orphanBlobId` is bound only by the connection about to be deleted. + insertBlob(orphanBlobId, doomedInstanceId) + insertBinding(orphanBlobId, doomedInstanceId) + // `sharedBlobId` is owned by the doomed connection but ALSO bound by a + // sibling connection that is not being deleted — the cascade hazard. + insertBlob(sharedBlobId, doomedInstanceId) + insertBinding(sharedBlobId, doomedInstanceId) + insertBinding(sharedBlobId, "cin_sqlite_orphan_survivor") + + exec(referenceQueries.recordsDeleteDeleteBlobBindingsByStream, [ + doomedInstanceId, + STREAM, + ]) + exec(referenceQueries.recordsDeleteDeleteBlobsByStream, [ + doomedInstanceId, + STREAM, + ]) + + assert.ok( + !getOne(referenceQueries.blobsGetRowById, [orphanBlobId]), + "the unbound blob is reclaimed rather than leaked" + ) + assert.ok( + getOne(referenceQueries.blobsGetRowById, [sharedBlobId]), + "a blob a sibling connection still binds must survive the delete" + ) + } finally { + closeDb() + } +}) diff --git a/reference-implementation/test/records-delete-postgres-routing.test.ts b/reference-implementation/test/records-delete-postgres-routing.test.ts index 697020e10..e965b0c26 100644 --- a/reference-implementation/test/records-delete-postgres-routing.test.ts +++ b/reference-implementation/test/records-delete-postgres-routing.test.ts @@ -38,7 +38,12 @@ import test from "node:test"; import { closeDb, initDb } from "../server/db.ts"; import { postgresIngestRecord } from "../server/postgres-records.ts"; -import { closePostgresStorage, initPostgresStorage, postgresQuery } from "../server/postgres-storage.ts"; +import { + closePostgresStorage, + initPostgresStorage, + isPostgresSemanticVectorEmbedding, + postgresQuery, +} from "../server/postgres-storage.ts"; import { deleteAllRecords, deleteAllRecordsForConnector } from "../server/records.ts"; const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; @@ -252,9 +257,17 @@ if (POSTGRES_URL) { op: "upsert", stream: streamSibling, }); + // `semantic_search_blob.embedding` is `vector` when pgvector is + // available and `jsonb` otherwise, so the cast has to follow the same + // branch the production writer uses (`insertSemanticRows` in + // postgres-search.ts). A JSON array literal is valid input for both + // types; hardcoding `::jsonb` fails on a pgvector database — which is + // the production configuration — with + // `column "embedding" is of type vector but expression is of type jsonb`. + const embeddingCast = isPostgresSemanticVectorEmbedding() ? "vector" : "jsonb"; await postgresQuery( `INSERT INTO semantic_search_blob (connector_id, connector_instance_id, scope_key, record_key, embedding) - VALUES ($1, $2, $3, $4, $5::jsonb), ($1, $2, $6, $7, $5::jsonb)`, + VALUES ($1, $2, $3, $4, $5::${embeddingCast}), ($1, $2, $6, $7, $5::${embeddingCast})`, [ connectorId, connectorInstanceId, diff --git a/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts b/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts index d5cc59c94..8267a3938 100644 --- a/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts +++ b/reference-implementation/test/semantic-index-skip-unchanged-postgres.test.ts @@ -3,7 +3,13 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { postgresLexicalIndexPublishWithClient, postgresSemanticIndexPublishWithClient } from "../server/postgres-search.ts"; +import { closeDb, initDb } from "../server/db.ts"; +import { postgresIngestRecord } from "../server/postgres-records.ts"; +import { + postgresLexicalIndexInsertMany, + postgresLexicalIndexPublishWithClient, + postgresSemanticIndexPublishWithClient, +} from "../server/postgres-search.ts"; import { closePostgresStorage, initPostgresStorage, @@ -198,6 +204,111 @@ if (POSTGRES_URL) { } ); }); + + // Companion to the publish-path fixture above. `postgresLexicalIndexInsertMany` + // is the BACKFILL writer (`rebuildLexicalIndexForStream` via + // `rebuildLexicalInsertEntries` in search.ts) — a different statement from + // `postgresLexicalIndexPublishWithClient`, which the fixture above covers. + // A backfill re-reads text that has not changed, so without an + // `IS DISTINCT FROM` guard on its conflict update every re-run rewrites + // every row and leaves one dead tuple per row behind. + test("N identical lexical backfill inserts leave dead-tuple pressure and relation size bounded", async () => { + const databaseName = `pdpp_lexical_backfill_bloat_${Date.now().toString(36)}`; + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName }, + async (url) => { + initDb(":memory:"); + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + try { + // The insert JOINs `records` on (connector_instance_id, stream, + // record_key, version) and requires a live row, so the fixture has to + // seed a real record and backfill at its actual current version. + await postgresIngestRecord( + { connector_id: CONNECTOR_ID, connector_instance_id: INSTANCE_ID }, + { + data: { body: "a stable body", id: RECORD_KEY, subject: "a stable subject" }, + emitted_at: "2026-09-03T00:00:00.000Z", + key: RECORD_KEY, + op: "upsert", + stream: STREAM, + } + ); + const versionRow = await postgresQuery<{ version: string }>( + `SELECT version::text AS version FROM records + WHERE connector_instance_id = $1 AND stream = $2 AND record_key = $3`, + [INSTANCE_ID, STREAM, RECORD_KEY] + ); + const version = Number(versionRow.rows[0]?.version); + assert.ok(Number.isInteger(version) && version > 0, "fixture seeded a live record with a version"); + + const backfillEntries = [ + { field: "body", recordKey: RECORD_KEY, text: "a stable body", version }, + { field: "subject", recordKey: RECORD_KEY, text: "a stable subject", version }, + ]; + const backfill = () => + postgresLexicalIndexInsertMany({ + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + entries: backfillEntries, + stream: STREAM, + }); + + await backfill(); + const seeded = await postgresQuery<{ ctid: string; field: string }>( + `SELECT ctid::text AS ctid, field FROM lexical_search_index + WHERE connector_instance_id = $1 AND record_key = $2 ORDER BY field`, + [INSTANCE_ID, RECORD_KEY] + ); + assert.equal(seeded.rows.length, 2, "fixture backfilled both indexed fields"); + const before = await postgresQuery<{ bytes: string; relname: string }>( + `SELECT relname, pg_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = $1`, + ["lexical_search_index"] + ); + + for (let attempt = 0; attempt < 20; attempt += 1) { + // biome-ignore lint/performance/noAwaitInLoops: each call is one production backfill insert. + await backfill(); + } + await postgresQuery("ANALYZE lexical_search_index", []); + + // Physical identity is the strongest assertion available: an elided + // update does not move the tuple, so `ctid` is unchanged. A rewrite + // would relocate every row. + const after = await postgresQuery<{ ctid: string; field: string }>( + `SELECT ctid::text AS ctid, field FROM lexical_search_index + WHERE connector_instance_id = $1 AND record_key = $2 ORDER BY field`, + [INSTANCE_ID, RECORD_KEY] + ); + assert.deepEqual( + after.rows, + seeded.rows, + "identical lexical backfill entries must leave their physical rows untouched" + ); + + const stats = await postgresQuery<{ bytes: string; n_dead_tup: string; n_live_tup: string }>( + `SELECT n_live_tup::text, n_dead_tup::text, pg_relation_size(relid)::text AS bytes + FROM pg_stat_user_tables + WHERE relname = $1`, + ["lexical_search_index"] + ); + const row = stats.rows[0]; + assert.ok(row, "fixture measured lexical_search_index"); + assert.ok( + Number(row.n_dead_tup) <= 1, + `lexical_search_index leaves no meaningful dead tuples after 20 identical backfills (saw ${row.n_dead_tup})` + ); + assert.ok( + Number(row.bytes) <= Number(before.rows[0]?.bytes) + 8192, + "lexical_search_index stays within one Postgres page of its post-backfill size" + ); + } finally { + closeDb(); + } + } + ); + }); } else { test("semantic index write-elision (skipped: PDPP_TEST_POSTGRES_URL unset)", { skip: true }, () => { /* intentionally empty */ From 12b0a9638a2edc2ddd29efd1fafb6f605eb7af46 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 16:54:10 -0500 Subject: [PATCH 04/11] fix(postgres): register the derived-index maintenance table and its tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The maintenance work added a Postgres table and three Postgres-backed test files without entering them in the registries that are meant to account for every one of them, so three inventory tests failed. `postgres_derived_index_maintenance_receipts` is now classified in `BACKUP_TABLE_INVENTORY` as `backup_required`: a `running` receipt is the in-flight marker that keeps a second VACUUM/REINDEX window off a database already under maintenance, and no executable rebuild oracle reconstructs it, which is what a non-required classification would require. It is also declared Postgres-only, alongside `semantic_hnsw_index_build` — SQLite's backend never runs these maintenance statements, so the table has no SQLite counterpart and is not part of the shared storage seam. The three new test files each bootstrap a temporary database through `initPostgresStorage`, so they are cold-required, not template-eligible. Updating the cold-required list moves its size from 26 to 29; the profile report's count is corrected to match (151 total, not 148). That figure was already stale before this change. Signed-off-by: Tim Nunamaker Assisted-by: AI --- PG-PROFILE-51-REPORT.md | 4 ++-- .../scripts/postgres-template-eligibility.ts | 3 +++ .../server/backup-table-policy.ts | 12 +++++++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/PG-PROFILE-51-REPORT.md b/PG-PROFILE-51-REPORT.md index 3ab4e8a8c..58a9791b7 100644 --- a/PG-PROFILE-51-REPORT.md +++ b/PG-PROFILE-51-REPORT.md @@ -167,8 +167,8 @@ tail: ## Scope counts and comparison baseline `scripts/run-tests.ts` discovers 1,034 profile files. The template equivalence -registry is a different measure: 122 eligible plus 26 cold-required files, -for **148** total. No current tracked source contains a 170-file claim. +registry is a different measure: 122 eligible plus 29 cold-required files, +for **151** total. No current tracked source contains a 170-file claim. The sibling report supplies only a memory-default cap-2 baseline (10,252 assertions, 9,690 passed, 209 failed, 353 skipped; about 10 minutes). It is not diff --git a/reference-implementation/scripts/postgres-template-eligibility.ts b/reference-implementation/scripts/postgres-template-eligibility.ts index 2c6943f50..b4b215c19 100644 --- a/reference-implementation/scripts/postgres-template-eligibility.ts +++ b/reference-implementation/scripts/postgres-template-eligibility.ts @@ -172,7 +172,9 @@ export const POSTGRES_TEMPLATE_COLD_REQUIRED_FILES: readonly string[] = [ "test/polyfill-manifest-reconcile-invalidation-postgres.test.ts", "test/postgres-boot-migration-resume.test.ts", "test/postgres-bootstrap-deadlock-retry.test.ts", + "test/postgres-derived-index-maintenance.test.ts", "test/postgres-hnsw-postlisten.test.ts", + "test/postgres-orphan-blob-reclaim.test.ts", "test/postgres-record-index-bootstrap.test.ts", "test/postgres-record-index-idempotency-oracle.test.ts", "test/postgres-record-index-repair-oracle.test.ts", @@ -183,6 +185,7 @@ export const POSTGRES_TEMPLATE_COLD_REQUIRED_FILES: readonly string[] = [ "test/run-history-duplicate-run-id-identity.test.ts", "test/run-history-interrupted-migration-reconciliation.test.ts", "test/run-history-writer-authority.test.ts", + "test/semantic-index-skip-unchanged-postgres.test.ts", "test/spine-events-connector-instance-id-backfill.test.ts", "test/spine-source-boot-backfill.test.ts", ]; diff --git a/reference-implementation/server/backup-table-policy.ts b/reference-implementation/server/backup-table-policy.ts index a03a8b98d..8ea1eb830 100644 --- a/reference-implementation/server/backup-table-policy.ts +++ b/reference-implementation/server/backup-table-policy.ts @@ -233,6 +233,11 @@ export const BACKUP_TABLE_INVENTORY: Record = classification: "backup_required", reason: "Pending consent transactions must not be silently dropped by a coherent restore.", }, + postgres_derived_index_maintenance_receipts: { + classification: "backup_required", + reason: + "A 'running' receipt is the in-flight marker that keeps a second VACUUM/REINDEX window off a database already being maintained, so it must be reconciled after a crash. Kept backup_required rather than derived_rebuildable because no executable rebuild oracle reconstructs it.", + }, presentation_screen_states: { classification: "backup_required", reason: "Presentation screen state is tied to live browser/screen surfaces.", @@ -365,7 +370,12 @@ const POSTGRES_SQLITE_ONLY_TABLES = ["semantic_search_rowid"] as const; // pgvector/HNSW has no SQLite counterpart; the build-progress row only ever exists // under the Postgres backend, so it is the Postgres-side mirror of the // SQLite-only exception above rather than part of the shared storage seam. -const SQLITE_POSTGRES_ONLY_TABLES = ["semantic_hnsw_index_build"] as const; +// The derived-index maintenance receipt is Postgres-only for the same reason: +// it records VACUUM/REINDEX windows, which SQLite's backend never runs. +const SQLITE_POSTGRES_ONLY_TABLES = [ + "postgres_derived_index_maintenance_receipts", + "semantic_hnsw_index_build", +] as const; export function isInternalBackupCatalogTable(name: string): boolean { return SQLITE_INTERNAL_TABLES.has(name) || isShadowTable(name); From 5a8a7db99a2acff36b95c8b4a834906a39743f2c Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 18:04:00 -0500 Subject: [PATCH 05/11] fix(postgres): drop named capture groups from the maintenance window parser `apps/console` lists `pdpp-reference-implementation` in its `transpilePackages`, so `next build` type-checks this module under the console's own ES2017 target. Named capturing groups need ES2018, so the window parser's regex failed that build with four TS1503 errors and took every console-dependent reference test down with it. The reference implementation's own typecheck targets ES2023 and accepted the same regex, which is why this only appeared in the console build. Positional groups parse identically; the validation and the rejected-input error are unchanged. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../server/postgres-derived-index-maintenance.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/reference-implementation/server/postgres-derived-index-maintenance.ts b/reference-implementation/server/postgres-derived-index-maintenance.ts index 05a89b2ab..ba0228b05 100644 --- a/reference-implementation/server/postgres-derived-index-maintenance.ts +++ b/reference-implementation/server/postgres-derived-index-maintenance.ts @@ -127,17 +127,20 @@ export function parsePostgresDerivedIndexMaintenanceWindow( if (value.toLowerCase() === "disabled") { return null; } - const match = /^(?[01]\d|2[0-3]):(?[0-5]\d)-(?[01]\d|2[0-3]):(?[0-5]\d)$/.exec( - value - ); - if (!match?.groups) { + // Positional groups, not named ones: apps/console transpiles this module + // (next.config.mjs `transpilePackages`) under an ES2017 target, where named + // capturing groups are a syntax error (TS1503), even though this package's + // own tsconfig targets ES2023. + const match = /^([01]\d|2[0-3]):([0-5]\d)-([01]\d|2[0-3]):([0-5]\d)$/.exec(value); + if (!match) { throw new Error( `${DERIVED_INDEX_MAINTENANCE_WINDOW_ENV} must be "HH:MM-HH:MM" in UTC, "disabled", or unset.` ); } + const [, startHour, startMinute, endHour, endMinute] = match; return { - startMinute: Number(match.groups.startHour) * 60 + Number(match.groups.startMinute), - endMinute: Number(match.groups.endHour) * 60 + Number(match.groups.endMinute), + startMinute: Number(startHour) * 60 + Number(startMinute), + endMinute: Number(endHour) * 60 + Number(endMinute), }; } From 6ccdc5a58e965582ddc4620ee3f27865eb2a9490 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 18:37:18 -0500 Subject: [PATCH 06/11] test(postgres): skip the maintenance oracle when no test database is configured The oracle fell back to a hardcoded `127.0.0.1:55448` when `PDPP_TEST_POSTGRES_URL` was unset, so it dialled a database that does not exist under the memory-default profile and failed on ECONNREFUSED. CI runs that profile without PostgreSQL, so the run reported a broken maintenance path where it should have reported an unconfigured one. Gate the database-backed case on the variable and register an explicit skipped test otherwise, matching `postgres-orphan-blob-reclaim.test.ts`. The window-parser case needs no database and still runs unconditionally. Verified both ways: unset skips the database case and passes the parser one; set against a pgvector instance passes both. Signed-off-by: Tim Nunamaker Assisted-by: AI --- ...postgres-derived-index-maintenance.test.ts | 212 +++++++++--------- 1 file changed, 112 insertions(+), 100 deletions(-) diff --git a/reference-implementation/test/postgres-derived-index-maintenance.test.ts b/reference-implementation/test/postgres-derived-index-maintenance.test.ts index 0f2393a16..6fec5b7e5 100644 --- a/reference-implementation/test/postgres-derived-index-maintenance.test.ts +++ b/reference-implementation/test/postgres-derived-index-maintenance.test.ts @@ -12,7 +12,13 @@ import { } from "../server/postgres-derived-index-maintenance.ts"; import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; -const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL ?? "postgres://postgres:pdpp_bloat_test_password@127.0.0.1:55448/pdpp_bloat_test"; +// Env gate: PDPP_TEST_POSTGRES_URL must be set, matching +// postgres-orphan-blob-reclaim.test.ts. No hardcoded fallback URL — one makes +// the case below dial a host that is absent under the memory-default profile +// (and in CI, which runs no PostgreSQL for that profile) and fail on +// ECONNREFUSED, which reads as a broken maintenance path rather than an +// unavailable database. +const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; let databaseCounter = 0; function databaseName(): string { @@ -30,107 +36,113 @@ test("derived-index maintenance defaults to a safe UTC off-peak window and accep assert.throws(() => parsePostgresDerivedIndexMaintenanceWindow("overnight"), /HH:MM-HH:MM/); }); -test("derived-index maintenance vacuums known heavy tables and concurrently reindexes present static search indexes", async () => { - await withTemporaryPostgresDatabase( - { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName: databaseName() }, - async (databaseUrl) => { - await initPostgresStorage({ backend: "postgres", databaseUrl }); - try { - await postgresQuery( - `INSERT INTO lexical_search_index (connector_id, connector_instance_id, stream, record_key, field, value) - SELECT 'connector', 'instance', 'stream', 'record-' || series::text, 'field', repeat('searchable value ', 32) - FROM generate_series(1, 3000) AS series` - ); - await postgresQuery("DELETE FROM lexical_search_index WHERE record_key <> 'record-1'"); - await postgresQuery("ANALYZE lexical_search_index"); +if (POSTGRES_URL) { + test("derived-index maintenance vacuums known heavy tables and concurrently reindexes present static search indexes", async () => { + await withTemporaryPostgresDatabase( + { closeConnections: closePostgresStorage, connectionString: POSTGRES_URL, databaseName: databaseName() }, + async (databaseUrl) => { + await initPostgresStorage({ backend: "postgres", databaseUrl }); + try { + await postgresQuery( + `INSERT INTO lexical_search_index (connector_id, connector_instance_id, stream, record_key, field, value) + SELECT 'connector', 'instance', 'stream', 'record-' || series::text, 'field', repeat('searchable value ', 32) + FROM generate_series(1, 3000) AS series` + ); + await postgresQuery("DELETE FROM lexical_search_index WHERE record_key <> 'record-1'"); + await postgresQuery("ANALYZE lexical_search_index"); - const before = await postgresQuery<{ index_oid: string; last_vacuum: string | null }>( - `SELECT stats.last_vacuum::text AS last_vacuum, - index_class.oid::text AS index_oid - FROM pg_stat_user_tables AS stats - JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' - JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace - WHERE stats.schemaname = current_schema() - AND stats.relname = 'lexical_search_index' - AND index_namespace.nspname = current_schema()` - ); - assert.equal(before.rowCount, 1); + const before = await postgresQuery<{ index_oid: string; last_vacuum: string | null }>( + `SELECT stats.last_vacuum::text AS last_vacuum, + index_class.oid::text AS index_oid + FROM pg_stat_user_tables AS stats + JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' + JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace + WHERE stats.schemaname = current_schema() + AND stats.relname = 'lexical_search_index' + AND index_namespace.nspname = current_schema()` + ); + assert.equal(before.rowCount, 1); - const receipt = await runPostgresDerivedIndexMaintenance({ - // pg_stat_user_tables updates n_dead_tup asynchronously. Zero gates - // make this real-database proof deterministic while still exercising - // both maintenance predicates against the sampled statistics. - minimumDeadTupleRatio: 0, - minimumDeadTuples: 0, - minimumTableBytes: 0, - now: new Date("2026-09-03T02:00:00.000Z"), - window: { endMinute: 180, startMinute: 60 }, - }); - const reindexableIndexes = [ - "idx_pg_lexical_search_document", - "idx_pg_lexical_search_scope_document", - "idx_pg_semantic_search_scope", - "idx_pg_semantic_search_embedding_hnsw", - ]; - const presentIndexes = await postgresQuery<{ relname: string }>( - `SELECT index_class.relname - FROM pg_class AS index_class - JOIN pg_namespace AS namespace ON namespace.oid = index_class.relnamespace - WHERE namespace.nspname = current_schema() - AND index_class.relname = ANY($1::text[]) - ORDER BY index_class.relname`, - [reindexableIndexes] - ); - const after = await postgresQuery<{ index_oid: string; last_analyze: string | null; last_vacuum: string | null }>( - `SELECT stats.last_vacuum::text AS last_vacuum, - stats.last_analyze::text AS last_analyze, - index_class.oid::text AS index_oid - FROM pg_stat_user_tables AS stats - JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' - JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace - WHERE stats.schemaname = current_schema() - AND stats.relname = 'lexical_search_index' - AND index_namespace.nspname = current_schema()` - ); + const receipt = await runPostgresDerivedIndexMaintenance({ + // pg_stat_user_tables updates n_dead_tup asynchronously. Zero gates + // make this real-database proof deterministic while still exercising + // both maintenance predicates against the sampled statistics. + minimumDeadTupleRatio: 0, + minimumDeadTuples: 0, + minimumTableBytes: 0, + now: new Date("2026-09-03T02:00:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + const reindexableIndexes = [ + "idx_pg_lexical_search_document", + "idx_pg_lexical_search_scope_document", + "idx_pg_semantic_search_scope", + "idx_pg_semantic_search_embedding_hnsw", + ]; + const presentIndexes = await postgresQuery<{ relname: string }>( + `SELECT index_class.relname + FROM pg_class AS index_class + JOIN pg_namespace AS namespace ON namespace.oid = index_class.relnamespace + WHERE namespace.nspname = current_schema() + AND index_class.relname = ANY($1::text[]) + ORDER BY index_class.relname`, + [reindexableIndexes] + ); + const after = await postgresQuery<{ index_oid: string; last_analyze: string | null; last_vacuum: string | null }>( + `SELECT stats.last_vacuum::text AS last_vacuum, + stats.last_analyze::text AS last_analyze, + index_class.oid::text AS index_oid + FROM pg_stat_user_tables AS stats + JOIN pg_class AS index_class ON index_class.relname = 'idx_pg_lexical_search_document' + JOIN pg_namespace AS index_namespace ON index_namespace.oid = index_class.relnamespace + WHERE stats.schemaname = current_schema() + AND stats.relname = 'lexical_search_index' + AND index_namespace.nspname = current_schema()` + ); - assert.equal(receipt.status, "completed"); - assert.deepEqual( - receipt.tables.map((table) => table.tableName), - ["blobs", "lexical_search_index", "record_changes", "records", "semantic_search_blob", "spine_events"], - "the scheduled vacuum samples every known heavy table" - ); - assert.ok(receipt.tables.some((table) => table.tableName === "lexical_search_index" && table.vacuumed)); - assert.deepEqual( - [...receipt.reindexedIndexNames].sort(), - presentIndexes.rows.map((row) => row.relname), - "only present indexes from the static search-index allowlist are rebuilt" - ); - assert.deepEqual(getLastPostgresDerivedIndexMaintenanceReceipt(), receipt); - assert.ok(after.rows[0]?.last_vacuum, "VACUUM (ANALYZE) updates Postgres's manual vacuum statistic"); - assert.ok(after.rows[0]?.last_analyze, "VACUUM (ANALYZE) updates Postgres's analyze statistic"); - assert.notEqual(after.rows[0]?.index_oid, before.rows[0]?.index_oid, "REINDEX CONCURRENTLY replaces the index relation"); + assert.equal(receipt.status, "completed"); + assert.deepEqual( + receipt.tables.map((table) => table.tableName), + ["blobs", "lexical_search_index", "record_changes", "records", "semantic_search_blob", "spine_events"], + "the scheduled vacuum samples every known heavy table" + ); + assert.ok(receipt.tables.some((table) => table.tableName === "lexical_search_index" && table.vacuumed)); + assert.deepEqual( + [...receipt.reindexedIndexNames].sort(), + presentIndexes.rows.map((row) => row.relname), + "only present indexes from the static search-index allowlist are rebuilt" + ); + assert.deepEqual(getLastPostgresDerivedIndexMaintenanceReceipt(), receipt); + assert.ok(after.rows[0]?.last_vacuum, "VACUUM (ANALYZE) updates Postgres's manual vacuum statistic"); + assert.ok(after.rows[0]?.last_analyze, "VACUUM (ANALYZE) updates Postgres's analyze statistic"); + assert.notEqual(after.rows[0]?.index_oid, before.rows[0]?.index_oid, "REINDEX CONCURRENTLY replaces the index relation"); - const repeat = await runPostgresDerivedIndexMaintenance({ - minimumDeadTupleRatio: 0, - minimumDeadTuples: 0, - minimumTableBytes: 0, - now: new Date("2026-09-03T02:15:00.000Z"), - window: { endMinute: 180, startMinute: 60 }, - }); - assert.equal(repeat.status, "already-completed", "the scheduler runs no more than once in one UTC window"); - const outsideWindow = await runPostgresDerivedIndexMaintenance({ - now: new Date("2026-09-03T04:00:00.000Z"), - window: { endMinute: 180, startMinute: 60 }, - }); - assert.equal(outsideWindow.status, "outside-window"); - assert.deepEqual( - getLastPostgresDerivedIndexMaintenanceReceipt(), - receipt, - "routine outside-window polls do not hide the most recent completed maintenance receipt" - ); - } finally { - await closePostgresStorage(); + const repeat = await runPostgresDerivedIndexMaintenance({ + minimumDeadTupleRatio: 0, + minimumDeadTuples: 0, + minimumTableBytes: 0, + now: new Date("2026-09-03T02:15:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + assert.equal(repeat.status, "already-completed", "the scheduler runs no more than once in one UTC window"); + const outsideWindow = await runPostgresDerivedIndexMaintenance({ + now: new Date("2026-09-03T04:00:00.000Z"), + window: { endMinute: 180, startMinute: 60 }, + }); + assert.equal(outsideWindow.status, "outside-window"); + assert.deepEqual( + getLastPostgresDerivedIndexMaintenanceReceipt(), + receipt, + "routine outside-window polls do not hide the most recent completed maintenance receipt" + ); + } finally { + await closePostgresStorage(); + } } - } - ); -}); + ); + }); +} else { + test("derived-index maintenance (skipped: PDPP_TEST_POSTGRES_URL unset)", { skip: true }, () => { + /* intentionally empty */ + }); +} From e3afa732bf39034ca55ffef81ac2af268f1da586 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 19:48:49 -0500 Subject: [PATCH 07/11] fix(postgres): preserve shared blobs during concurrent cleanup Lock candidate blobs before checking their bindings in a fresh statement within the same transaction. Apply this ordering to stream and connection cleanup so a concurrently committed sibling binding retains its bytes. Add PostgreSQL regressions for both deletion paths that fail before the fix. Signed-off-by: Tim Nunamaker Assisted-by: AI --- inbox/devspecs-feedback.md | 4 + reference-implementation/server/records.ts | 84 +++++++++------ .../test/postgres-orphan-blob-reclaim.test.ts | 100 +++++++++++++++++- 3 files changed, 157 insertions(+), 31 deletions(-) diff --git a/inbox/devspecs-feedback.md b/inbox/devspecs-feedback.md index 55ec426d3..dc95e0e54 100644 --- a/inbox/devspecs-feedback.md +++ b/inbox/devspecs-feedback.md @@ -3,3 +3,7 @@ ## 2026-09-03 — DB bloat repair `ds task "fix PostgreSQL storage bloat" --slice ...` waited at “Task index preflight: waiting for another index update” for more than 30 seconds and never produced a task slice. The command gave no owner, timeout, or recovery action, so I continued with the repository brief and targeted tests. A bounded wait plus a suggested retry/status command would make this easier to use during incident work. + +## 2026-09-09 — Concurrent blob cleanup repair + +`ds recent` completed in about six seconds and identified the existing reconciliation-bloat change and its files. `ds task "preserve shared PostgreSQL blobs during concurrent connector deletion" --quick` discovered 3,537 files, then stayed at “extracting and indexing artifacts” for over four minutes without producing a task. I stopped that invocation and continued from the lane brief and PostgreSQL regression tests. A quick task still needs a bounded path that can use known file paths without a full index. diff --git a/reference-implementation/server/records.ts b/reference-implementation/server/records.ts index 901f35de7..4cc22cd11 100644 --- a/reference-implementation/server/records.ts +++ b/reference-implementation/server/records.ts @@ -6789,35 +6789,51 @@ async function postgresDeleteAllRecordsForConnector(connectorId: string, instanc } await mapWithConcurrency(instanceStreams, 1, async (stream) => { await postgresDeleteAllRecords(storageTarget, stream); - await postgresQuery("DELETE FROM blob_bindings WHERE connector_instance_id = $1 AND stream = $2", [ - connectorInstanceId, - stream, - ]); - // Reclaim blobs this delete just unbound, mirroring - // `deleteConnectionRecordRowsPostgres` (and SQLite's - // `delete-blobs-by-instance.sql`). Dropping the bindings above without - // this left the `blobs` rows behind permanently — nothing else in the - // codebase collects orphans, so those bytes were junk forever. - // - // Refcount-gated, NOT supersede-and-delete. `blobs` is globally - // content-addressed (the insert conflicts on `blob_id` alone, with no - // connector or instance in the conflict target), so identical bytes - // from a sibling connection share ONE row; and the FK from - // `blob_bindings` is ON DELETE CASCADE, so an ungated delete here - // would silently destroy a live sibling's binding. `NOT EXISTS` - // deletes only rows no binding still references. Scoped to this - // instance for the same reason the whole-connection sibling is: a - // delete reclaims its own bytes, never another connection's. - await postgresQuery( - `DELETE FROM blobs - WHERE connector_instance_id = $1 - AND stream = $2 - AND NOT EXISTS ( - SELECT 1 - FROM blob_bindings - WHERE blob_bindings.blob_id = blobs.blob_id - )`, - [connectorInstanceId, stream] + await withPostgresTransaction( + async (client) => { + await client.query("DELETE FROM blob_bindings WHERE connector_instance_id = $1 AND stream = $2", [ + connectorInstanceId, + stream, + ]); + // Reclaim blobs this delete just unbound, mirroring + // `deleteConnectionRecordRowsPostgres` (and SQLite's + // `delete-blobs-by-instance.sql`). Dropping the bindings above without + // this left the `blobs` rows behind permanently — nothing else in the + // codebase collects orphans, so those bytes were junk forever. + // + // Refcount-gated, NOT supersede-and-delete. `blobs` is globally + // content-addressed (the insert conflicts on `blob_id` alone, with no + // connector or instance in the conflict target), so identical bytes + // from a sibling connection share ONE row; and the FK from + // `blob_bindings` is ON DELETE CASCADE, so an ungated delete here + // would silently destroy a live sibling's binding. `NOT EXISTS` + // deletes only rows no binding still references. Scoped to this + // instance for the same reason the whole-connection sibling is: a + // delete reclaims its own bytes, never another connection's. + // The FK takes KEY SHARE when a writer adds a binding. Lock candidates + // first, then check references in a NEW READ COMMITTED statement so a + // writer that committed while this lock waited is visible to DELETE. + // Keep both statements in this transaction; a single CTE is not enough. + const candidates = await client.query<{ blob_id: string }>( + `SELECT blob_id FROM blobs + WHERE connector_instance_id = $1 AND stream = $2 + ORDER BY blob_id FOR UPDATE`, + [connectorInstanceId, stream] + ); + await client.query( + `DELETE FROM blobs + WHERE connector_instance_id = $1 + AND stream = $2 + AND blob_id = ANY($3::text[]) + AND NOT EXISTS ( + SELECT 1 + FROM blob_bindings + WHERE blob_bindings.blob_id = blobs.blob_id + )`, + [connectorInstanceId, stream, candidates.rows.map((row) => row.blob_id)] + ); + }, + { lockConnectorInstanceId: connectorInstanceId } ); await markRetainedSizeStreamDirty({ connectorInstanceId, stream }); // Parity with the SQLite arm above: a connector-wide record delete @@ -7035,15 +7051,23 @@ export async function deleteConnectionRecordRowsPostgres(client: PostgresClient, await client.query("DELETE FROM record_changes WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM version_counter WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM blob_bindings WHERE connector_instance_id = $1", [connectorInstanceId]); + // Wait for binding writers before taking the reference-check snapshot. + // The caller keeps these locks through DELETE in its READ COMMITTED transaction. + const candidates = await client.query<{ blob_id: string }>( + `SELECT blob_id FROM blobs WHERE connector_instance_id = $1 + ORDER BY blob_id FOR UPDATE`, + [connectorInstanceId] + ); await client.query( `DELETE FROM blobs WHERE connector_instance_id = $1 + AND blob_id = ANY($2::text[]) AND NOT EXISTS ( SELECT 1 FROM blob_bindings WHERE blob_bindings.blob_id = blobs.blob_id )`, - [connectorInstanceId] + [connectorInstanceId, candidates.rows.map((row) => row.blob_id)] ); await client.query("DELETE FROM connector_attention_records WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM records WHERE connector_instance_id = $1", [connectorInstanceId]); diff --git a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts index 80d151b9e..1098e84e6 100644 --- a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts +++ b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts @@ -31,6 +31,9 @@ import assert from "node:assert/strict" import test from "node:test" +import { setTimeout as delay } from "node:timers/promises" + +import pg from "pg" import { exec, getOne, referenceQueries } from "../lib/db.ts" import { closeDb, initDb } from "../server/db.ts" @@ -42,8 +45,12 @@ import { closePostgresStorage, initPostgresStorage, postgresQuery, + withPostgresTransaction, } from "../server/postgres-storage.ts" -import { deleteAllRecordsForConnector } from "../server/records.ts" +import { + deleteAllRecordsForConnector, + deleteConnectionRecordRowsPostgres, +} from "../server/records.ts" import { getChangeHistoryLimit } from "../server/storage-utils.ts" import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts" @@ -222,6 +229,97 @@ if (POSTGRES_URL) { } ) }) + for (const scope of ["per-stream", "whole-connection"] as const) { + test(`${scope} delete preserves a sibling binding committed during reclamation`, async () => { + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_blob_reclaim_race_${Date.now().toString(36)}`, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + const writer = new pg.Client({ connectionString: url }) + let deletion: Promise | undefined + try { + await writer.connect() + const doomedConnectorId = + "https://registry.pdpp.test/connectors/blob_race_doomed" + const survivingConnectorId = + "https://registry.pdpp.test/connectors/blob_race_survivor" + const bytes = Buffer.alloc(1024, 0x5a) + const blobId = await seedRecordWithBlob({ + bytes, + connectorId: doomedConnectorId, + connectorInstanceId: "cin_blob_race_doomed", + recordKey: "att-1", + }) + + // Pause a sibling publication after its FK has locked the shared + // blob but before COMMIT makes its binding visible to cleanup. + await writer.query("BEGIN") + const pid = await writer.query<{ pid: number }>( + "SELECT pg_backend_pid() AS pid" + ) + const writerPid = pid.rows[0]?.pid + assert.ok(writerPid) + await writer.query( + `INSERT INTO blob_bindings + (blob_id, connector_id, connector_instance_id, stream, record_key, json_path) + VALUES ($1, $2, $3, $4, $5, '@record')`, + [blobId, survivingConnectorId, "cin_blob_race_survivor", STREAM, "att-1"] + ) + deletion = scope === "per-stream" + ? deleteAllRecordsForConnector(doomedConnectorId) + : withPostgresTransaction(client => + deleteConnectionRecordRowsPostgres(client, "cin_blob_race_doomed") + ) + // Attach a rejection handler immediately while observing the lock. + deletion.catch(() => undefined) + let blocked = false + const deadline = Date.now() + 10_000 + while (!blocked && Date.now() < deadline) { + // biome-ignore lint/performance/noAwaitInLoops: observe the actual database lock, not a timing assumption. + const waiting = await postgresQuery<{ blocked: boolean }>( + `SELECT EXISTS ( + SELECT 1 FROM pg_stat_activity + WHERE datname = current_database() + AND $1::int = ANY(pg_blocking_pids(pid)) + ) AS blocked`, + [writerPid] + ) + blocked = waiting.rows[0]?.blocked ?? false + if (!blocked) { + await delay(10) + } + } + assert.ok(blocked, "cleanup must reach the sibling transaction's blob lock") + await writer.query("COMMIT") + await deletion + + assert.equal(await countBlobs(blobId), 1, "committed sibling bytes survive cleanup") + const bindings = await postgresQuery<{ connector_id: string }>( + "SELECT connector_id FROM blob_bindings WHERE blob_id = $1", + [blobId] + ) + assert.deepEqual(bindings.rows, [{ connector_id: survivingConnectorId }]) + const stored = await postgresQuery<{ data: Buffer }>( + "SELECT data FROM blobs WHERE blob_id = $1", + [blobId] + ) + assert.deepEqual(stored.rows[0]?.data, bytes) + } finally { + await writer.query("ROLLBACK").catch(() => undefined) + await deletion?.catch(() => undefined) + await writer.end() + await closePostgresStorage() + closeDb() + } + } + ) + }) + } /** * Why superseded blobs are NOT deleted at the point of supersession. * From 5919589cde39fbdcf6eabadb75f9915fabee5132 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 20:36:58 -0500 Subject: [PATCH 08/11] fix(postgres): bound blob cleanup waits and candidate batches Return the existing busy error when cleanup cannot acquire a blob lock within the configured writer wait budget. Process candidate IDs in bounded batches, using an index installed after legacy instance-column migration. Give uploads that lose the cleanup race a 409 conflict with retry guidance. Cover held locks, batch boundaries, legacy upgrade, and explicit retry. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../server/postgres-records.ts | 11 +- .../server/postgres-storage.ts | 3 + reference-implementation/server/records.ts | 95 +++++++----- .../server/routes/ref-error-status.ts | 1 + ...postgres-blob-publication-conflict.test.ts | 94 ++++++++++++ .../test/postgres-orphan-blob-reclaim.test.ts | 139 ++++++++++++++++++ .../postgres-record-index-bootstrap.test.ts | 27 ++++ 7 files changed, 329 insertions(+), 41 deletions(-) create mode 100644 reference-implementation/test/postgres-blob-publication-conflict.test.ts diff --git a/reference-implementation/server/postgres-records.ts b/reference-implementation/server/postgres-records.ts index b84cc7ac4..1b8cf2738 100644 --- a/reference-implementation/server/postgres-records.ts +++ b/reference-implementation/server/postgres-records.ts @@ -3140,7 +3140,16 @@ async function postgresPersistContentAddressedBlobWithinFence({ return { ...storedRow, binding_inserted: (binding.rowCount ?? 0) > 0 }; }, { lockConnectorInstanceId: effectiveConnectorInstanceId } - ); + ).catch((error: unknown) => { + const queryError = error as { code?: string; constraint?: string } | null; + if (queryError?.code === "23503" && queryError.constraint === "blob_bindings_blob_id_fkey") { + throw Object.assign(new Error("Blob was reclaimed during publication; retry the upload.", { cause: error }), { + code: "blob_publication_conflict", + statusCode: 409, + }); + } + throw error; + }); return { binding_inserted: Boolean(row.binding_inserted), diff --git a/reference-implementation/server/postgres-storage.ts b/reference-implementation/server/postgres-storage.ts index 1207180e6..ee0d28a65 100644 --- a/reference-implementation/server/postgres-storage.ts +++ b/reference-implementation/server/postgres-storage.ts @@ -3578,6 +3578,9 @@ async function bootstrapPostgresSchemaOnce({ await migratePostgresRunHistoryCompletedAtNullable(client); await migratePostgresConnectorMaintenanceCursorNameCheck(client); await migratePostgresRecordsBlobSearchInstanceColumns(client); + // Legacy blob tables gain connector_instance_id in the migration above. + await client.query(`CREATE INDEX IF NOT EXISTS idx_blobs_instance_blob_id + ON blobs(connector_instance_id, blob_id)`); await migratePostgresClientEventSubscriptionAuthority(client); // Install the ledger BEFORE the first data migration that consults it. // A migration that reads a missing ledger table would have to guess its diff --git a/reference-implementation/server/records.ts b/reference-implementation/server/records.ts index 4cc22cd11..9d6e0556e 100644 --- a/reference-implementation/server/records.ts +++ b/reference-implementation/server/records.ts @@ -60,6 +60,8 @@ import { resolveRequestBindings, } from "./connection-identity.ts"; import { + ConnectorInstanceAdmissionError, + connectorInstanceLockWaitMs, type ConnectorInstanceWriteOwnership, withConnectorInstanceWrite, } from "./connector-instance-write-coordinator.ts"; @@ -6810,28 +6812,7 @@ async function postgresDeleteAllRecordsForConnector(connectorId: string, instanc // deletes only rows no binding still references. Scoped to this // instance for the same reason the whole-connection sibling is: a // delete reclaims its own bytes, never another connection's. - // The FK takes KEY SHARE when a writer adds a binding. Lock candidates - // first, then check references in a NEW READ COMMITTED statement so a - // writer that committed while this lock waited is visible to DELETE. - // Keep both statements in this transaction; a single CTE is not enough. - const candidates = await client.query<{ blob_id: string }>( - `SELECT blob_id FROM blobs - WHERE connector_instance_id = $1 AND stream = $2 - ORDER BY blob_id FOR UPDATE`, - [connectorInstanceId, stream] - ); - await client.query( - `DELETE FROM blobs - WHERE connector_instance_id = $1 - AND stream = $2 - AND blob_id = ANY($3::text[]) - AND NOT EXISTS ( - SELECT 1 - FROM blob_bindings - WHERE blob_bindings.blob_id = blobs.blob_id - )`, - [connectorInstanceId, stream, candidates.rows.map((row) => row.blob_id)] - ); + await deleteUnreferencedBlobsPostgres(client, connectorInstanceId, stream); }, { lockConnectorInstanceId: connectorInstanceId } ); @@ -7036,6 +7017,57 @@ export function deleteConnectionRecordRowsSqlite(connectorInstanceId: string) { return count; } +/** Reclaim only locked candidates, with a fresh reference snapshot per batch. */ +async function deleteUnreferencedBlobsPostgres( + client: PostgresClient, + connectorInstanceId: string, + stream: string | null = null +): Promise { + // Match the store's admission budget even for caller-owned transactions that + // did not acquire an instance advisory lock. SET LOCAL ends at COMMIT/ROLLBACK. + await client.query(`SET LOCAL lock_timeout = '${connectorInstanceLockWaitMs()}ms'`); + const batchSize = 256; + let afterBlobId = ""; + let hasMore = true; + try { + while (hasMore) { + // The FK takes KEY SHARE when a writer adds a binding. Wait here, then + // check references in a NEW READ COMMITTED statement so committed writers + // are visible. A single CTE would retain the stale statement snapshot. + // Keyset batches bound the IDs held in Node; locks remain transaction-wide. + // biome-ignore lint/performance/noAwaitInLoops: each batch follows the preceding locked key range. + const candidates = await client.query<{ blob_id: string }>( + `SELECT blob_id FROM blobs + WHERE connector_instance_id = $1 AND blob_id > $2 + AND ($3::text IS NULL OR stream = $3) + ORDER BY blob_id LIMIT $4 FOR UPDATE`, + [connectorInstanceId, afterBlobId, stream, batchSize] + ); + const lastCandidate = candidates.rows.at(-1); + if (!lastCandidate) { + return; + } + const blobIds = candidates.rows.map((row) => row.blob_id); + await client.query( + `DELETE FROM blobs + WHERE blob_id = ANY($1::text[]) + AND NOT EXISTS ( + SELECT 1 FROM blob_bindings WHERE blob_bindings.blob_id = blobs.blob_id + )`, + [blobIds] + ); + afterBlobId = lastCandidate.blob_id; + hasMore = blobIds.length === batchSize; + } + } catch (err) { + if ((err as { code?: string } | null)?.code === "55P03") { + // biome-ignore lint/style/useErrorCause: preserve the coordinator's no-argument admission error contract. + throw new ConnectorInstanceAdmissionError(); + } + throw err; + } +} + /** * Phase 2 (Postgres): same as the SQLite arm, but binds against the explicit * transaction `client` the store opened, so the record-family deletes run in @@ -7051,24 +7083,7 @@ export async function deleteConnectionRecordRowsPostgres(client: PostgresClient, await client.query("DELETE FROM record_changes WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM version_counter WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM blob_bindings WHERE connector_instance_id = $1", [connectorInstanceId]); - // Wait for binding writers before taking the reference-check snapshot. - // The caller keeps these locks through DELETE in its READ COMMITTED transaction. - const candidates = await client.query<{ blob_id: string }>( - `SELECT blob_id FROM blobs WHERE connector_instance_id = $1 - ORDER BY blob_id FOR UPDATE`, - [connectorInstanceId] - ); - await client.query( - `DELETE FROM blobs - WHERE connector_instance_id = $1 - AND blob_id = ANY($2::text[]) - AND NOT EXISTS ( - SELECT 1 - FROM blob_bindings - WHERE blob_bindings.blob_id = blobs.blob_id - )`, - [connectorInstanceId, candidates.rows.map((row) => row.blob_id)] - ); + await deleteUnreferencedBlobsPostgres(client, connectorInstanceId); await client.query("DELETE FROM connector_attention_records WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM records WHERE connector_instance_id = $1", [connectorInstanceId]); return count; diff --git a/reference-implementation/server/routes/ref-error-status.ts b/reference-implementation/server/routes/ref-error-status.ts index b169414c1..0bec4c916 100644 --- a/reference-implementation/server/routes/ref-error-status.ts +++ b/reference-implementation/server/routes/ref-error-status.ts @@ -97,6 +97,7 @@ export const codeToStatus: Readonly> = { archive_reconnect_resume_failed: 502, authentication_error: 401, blob_not_found: 404, + blob_publication_conflict: 409, browser_enrollment_shell_required: 400, connection_is_grouping_canonical: 409, connection_not_found: 404, diff --git a/reference-implementation/test/postgres-blob-publication-conflict.test.ts b/reference-implementation/test/postgres-blob-publication-conflict.test.ts new file mode 100644 index 000000000..e652f0f41 --- /dev/null +++ b/reference-implementation/test/postgres-blob-publication-conflict.test.ts @@ -0,0 +1,94 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; +import { setTimeout as delay } from "node:timers/promises"; +import pg from "pg"; +import { closeDb, initDb } from "../server/db.ts"; +import { postgresPersistContentAddressedBlob } from "../server/postgres-records.ts"; +import { closePostgresStorage, initPostgresStorage, postgresQuery } from "../server/postgres-storage.ts"; +import { codeToStatus } from "../server/routes/ref-error-status.ts"; +import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; + +const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; + +test("blob publication conflict maps to HTTP 409 in the shared error envelope", () => { + assert.equal(codeToStatus.blob_publication_conflict, 409); +}); + +test("blob publication reports a retryable conflict when reclamation wins its FK lock", { + skip: !POSTGRES_URL, +}, async () => { + assert.ok(POSTGRES_URL); + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_blob_publish_conflict_${Date.now().toString(36)}`, + }, + async (url) => { + initDb(":memory:"); + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + const cleanup = new pg.Client({ connectionString: url }); + let publication: ReturnType | undefined; + try { + await cleanup.connect(); + const args = { + connectorId: "https://registry.pdpp.test/connectors/blob_publication_conflict", + connectorInstanceId: "cin_blob_publication_doomed", + data: Buffer.from("shared bytes for concurrent reclamation"), + mimeType: "application/octet-stream", + recordKey: "attachment-1", + stream: "attachments", + }; + const original = await postgresPersistContentAddressedBlob(args); + await cleanup.query("BEGIN"); + const pid = await cleanup.query<{ pid: number }>("SELECT pg_backend_pid() AS pid"); + await cleanup.query("SELECT blob_id FROM blobs WHERE blob_id = $1 FOR UPDATE", [original.blob_id]); + + publication = postgresPersistContentAddressedBlob({ + ...args, + connectorInstanceId: "cin_blob_publication_survivor", + }); + publication.catch(() => undefined); + let blocked = false; + const deadline = Date.now() + 2000; + while (!blocked && Date.now() < deadline) { + // biome-ignore lint/performance/noAwaitInLoops: observe the FK lock before allowing deletion to commit. + const result = await postgresQuery<{ blocked: boolean }>( + `SELECT EXISTS (SELECT 1 FROM pg_stat_activity + WHERE datname = current_database() AND $1::int = ANY(pg_blocking_pids(pid)) + AND query LIKE 'INSERT INTO blob_bindings%') AS blocked`, + [pid.rows[0]?.pid] + ); + blocked = result.rows[0]?.blocked ?? false; + if (!blocked) { + await delay(10); + } + } + assert.ok(blocked, "publication must wait on the reclaimed blob's FK lock"); + await cleanup.query("DELETE FROM blob_bindings WHERE blob_id = $1", [original.blob_id]); + await cleanup.query("DELETE FROM blobs WHERE blob_id = $1", [original.blob_id]); + await cleanup.query("COMMIT"); + await assert.rejects(publication, { + code: "blob_publication_conflict", + statusCode: 409, + message: "Blob was reclaimed during publication; retry the upload.", + }); + const retry = await postgresPersistContentAddressedBlob({ + ...args, + connectorInstanceId: "cin_blob_publication_survivor", + }); + assert.equal(retry.blob_id, original.blob_id); + assert.equal(retry.binding_inserted, true); + } finally { + await cleanup.query("ROLLBACK").catch(() => undefined); + await publication?.catch(() => undefined); + await cleanup.end(); + await closePostgresStorage(); + closeDb(); + } + } + ); +}); diff --git a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts index 1098e84e6..2c19ca454 100644 --- a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts +++ b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts @@ -36,6 +36,7 @@ import { setTimeout as delay } from "node:timers/promises" import pg from "pg" import { exec, getOne, referenceQueries } from "../lib/db.ts" +import { ConnectorInstanceAdmissionError } from "../server/connector-instance-write-coordinator.ts" import { closeDb, initDb } from "../server/db.ts" import { postgresIngestRecord, @@ -107,6 +108,64 @@ async function seedRecordWithBlob({ } if (POSTGRES_URL) { + test("whole-connection delete fails within its lock budget behind a held blob lock", async () => { + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_blob_delete_timeout_${Date.now().toString(36)}`, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + const writer = new pg.Client({ connectionString: url }) + const previousLockWait = process.env.PDPP_INGEST_LOCK_WAIT_MS + let deletion: Promise | undefined + try { + process.env.PDPP_INGEST_LOCK_WAIT_MS = "100" + await writer.connect() + const connectorInstanceId = "cin_blob_delete_timeout" + const blobId = await seedRecordWithBlob({ + bytes: Buffer.alloc(1024, 0x5a), + connectorId: "https://registry.pdpp.test/connectors/blob_delete_timeout", + connectorInstanceId, + recordKey: "att-1", + }) + await writer.query("BEGIN") + await writer.query("SELECT blob_id FROM blobs WHERE blob_id = $1 FOR KEY SHARE", [blobId]) + deletion = withPostgresTransaction(client => + deleteConnectionRecordRowsPostgres(client, connectorInstanceId) + ) + // The watchdog lets the assertion fail on the unbounded implementation; + // finally releases the writer before draining the deletion promise. + const outcome = await Promise.race([ + deletion.then( + () => ({ status: "deleted" as const }), + (error: unknown) => ({ status: "rejected" as const, error }) + ), + delay(1500).then(() => ({ status: "still-waiting" as const })), + ]) + assert.equal(outcome.status, "rejected", "delete must fail while the writer still holds its lock") + assert.ok(outcome.status === "rejected" && outcome.error instanceof ConnectorInstanceAdmissionError) + assert.equal(outcome.error.code, "connector_instance_busy") + assert.equal(await countBlobs(blobId), 1, "timeout rolls back blob cleanup") + assert.equal(await countBindings(blobId), 1, "timeout restores the deleted binding") + } finally { + await writer.query("ROLLBACK").catch(() => undefined) + await deletion?.catch(() => undefined) + await writer.end() + if (previousLockWait === undefined) { + delete process.env.PDPP_INGEST_LOCK_WAIT_MS + } else { + process.env.PDPP_INGEST_LOCK_WAIT_MS = previousLockWait + } + await closePostgresStorage() + closeDb() + } + } + ) + }) + test("per-stream connector delete reclaims blobs whose last binding it removed", async () => { const databaseName = `pdpp_orphan_blob_reclaim_${Date.now().toString(36)}` await withTemporaryPostgresDatabase( @@ -230,6 +289,86 @@ if (POSTGRES_URL) { ) }) for (const scope of ["per-stream", "whole-connection"] as const) { + test(`${scope} delete reclaims more than one batch while retaining shared blobs`, async () => { + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_blob_reclaim_batches_${Date.now().toString(36)}`, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + try { + const connectorId = "https://registry.pdpp.test/connectors/blob_batches" + const connectorInstanceId = "cin_blob_batches" + await seedRecordWithBlob({ + bytes: Buffer.alloc(1024, 0x5a), + connectorId, + connectorInstanceId, + recordKey: "att-1", + }) + await postgresQuery( + `INSERT INTO blobs + (blob_id, connector_id, connector_instance_id, stream, record_key, mime_type, size_bytes, sha256, data) + SELECT 'blob_batch_' || lpad(n::text, 4, '0'), $1, $2, $3, + 'att-' || n, 'application/octet-stream', 1, md5(n::text), decode('5a', 'hex') + FROM generate_series(1, 257) AS n`, + [connectorId, connectorInstanceId, STREAM] + ) + await postgresQuery( + `INSERT INTO blob_bindings + (blob_id, connector_id, connector_instance_id, stream, record_key) + SELECT blob_id, connector_id, connector_instance_id, stream, record_key + FROM blobs WHERE blob_id LIKE 'blob_batch_%'` + ) + // Keep the first candidate: pagination must advance beyond retained rows. + await postgresQuery( + `INSERT INTO blob_bindings + (blob_id, connector_id, connector_instance_id, stream, record_key) + VALUES ('blob_batch_0001', $1, 'cin_blob_batches_survivor', $2, 'att-1')`, + ["https://registry.pdpp.test/connectors/blob_batches_survivor", STREAM] + ) + + if (scope === "per-stream") { + await deleteAllRecordsForConnector(connectorId) + } else { + const lockBatchSizes: number[] = [] + await withPostgresTransaction(async client => { + const observedClient = new Proxy(client, { + get(target, property, receiver) { + if (property !== "query") { + return Reflect.get(target, property, receiver) + } + return async (sql: string, values?: unknown[]) => { + const result = await target.query(sql, values) + if (sql.includes("FOR UPDATE") && sql.includes("FROM blobs")) { + lockBatchSizes.push(result.rows.length) + } + return result + } + }, + }) + await deleteConnectionRecordRowsPostgres(observedClient, connectorInstanceId) + }) + assert.ok(lockBatchSizes.filter(size => size > 0).length > 1, "cleanup locks multiple batches") + assert.ok(lockBatchSizes.every(size => size <= 256), "no lock query returns the entire connection") + } + + const blobs = await postgresQuery<{ blob_id: string }>("SELECT blob_id FROM blobs") + assert.deepEqual(blobs.rows, [{ blob_id: "blob_batch_0001" }]) + const bindings = await postgresQuery<{ connector_instance_id: string }>( + "SELECT connector_instance_id FROM blob_bindings" + ) + assert.deepEqual(bindings.rows, [{ connector_instance_id: "cin_blob_batches_survivor" }]) + } finally { + await closePostgresStorage() + closeDb() + } + } + ) + }) + test(`${scope} delete preserves a sibling binding committed during reclamation`, async () => { await withTemporaryPostgresDatabase( { diff --git a/reference-implementation/test/postgres-record-index-bootstrap.test.ts b/reference-implementation/test/postgres-record-index-bootstrap.test.ts index db0ac6826..1df5fc714 100644 --- a/reference-implementation/test/postgres-record-index-bootstrap.test.ts +++ b/reference-implementation/test/postgres-record-index-bootstrap.test.ts @@ -46,6 +46,33 @@ async function readIndex( } if (POSTGRES_URL) { + test("blob cleanup index follows legacy instance-column migration and survives restart", async () => { + await withTempDb(POSTGRES_URL, async (url) => { + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + const bytes = Buffer.from("legacy blob bytes"); + await getPostgresPool().query( + `INSERT INTO blobs + (blob_id, connector_id, connector_instance_id, stream, record_key, mime_type, size_bytes, sha256, data) + VALUES ('legacy-blob', 'legacy-connector', 'legacy-instance', 'attachments', 'one', + 'application/octet-stream', $1, 'legacy-sha', $2)`, + [bytes.length, bytes] + ); + // Model a populated legacy table whose instance column has not migrated. + await getPostgresPool().query("ALTER TABLE blobs DROP COLUMN connector_instance_id CASCADE"); + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + const migrated = await getPostgresPool().query( + "SELECT data, connector_instance_id FROM blobs WHERE blob_id = 'legacy-blob'" + ); + assert.deepEqual(migrated.rows[0]?.data, bytes); + assert.ok(migrated.rows[0]?.connector_instance_id); + const before = await readIndex(getPostgresPool(), "idx_blobs_instance_blob_id"); + assert.ok(before?.definition.includes("(connector_instance_id, blob_id)")); + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + const after = await readIndex(getPostgresPool(), "idx_blobs_instance_blob_id"); + assert.equal(after?.oid, before?.oid); + }); + }); + test("bootstrap lock contender survives a real holder that releases before the deadline", async () => { await withTempDb(POSTGRES_URL, async (url) => { await initPostgresStorage({ backend: "postgres", databaseUrl: url }); From 7ff9d9b3997cd9a3e1c6a729883fc305fc397d8f Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 9 Sep 2026 21:11:22 -0500 Subject: [PATCH 09/11] test(postgres): classify the blob publication conflict fixture Signed-off-by: Tim Nunamaker Assisted-by: AI --- PG-PROFILE-51-REPORT.md | 4 ++-- .../scripts/postgres-template-eligibility.ts | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/PG-PROFILE-51-REPORT.md b/PG-PROFILE-51-REPORT.md index 58a9791b7..8128d2b3f 100644 --- a/PG-PROFILE-51-REPORT.md +++ b/PG-PROFILE-51-REPORT.md @@ -167,8 +167,8 @@ tail: ## Scope counts and comparison baseline `scripts/run-tests.ts` discovers 1,034 profile files. The template equivalence -registry is a different measure: 122 eligible plus 29 cold-required files, -for **151** total. No current tracked source contains a 170-file claim. +registry is a different measure: 122 eligible plus 30 cold-required files, +for **152** total. No current tracked source contains a 170-file claim. The sibling report supplies only a memory-default cap-2 baseline (10,252 assertions, 9,690 passed, 209 failed, 353 skipped; about 10 minutes). It is not diff --git a/reference-implementation/scripts/postgres-template-eligibility.ts b/reference-implementation/scripts/postgres-template-eligibility.ts index b4b215c19..3525a4c5f 100644 --- a/reference-implementation/scripts/postgres-template-eligibility.ts +++ b/reference-implementation/scripts/postgres-template-eligibility.ts @@ -170,6 +170,7 @@ export const POSTGRES_TEMPLATE_COLD_REQUIRED_FILES: readonly string[] = [ "test/connector-summary-source-revision.test.ts", "test/device-ingest-reservation-migration.test.ts", "test/polyfill-manifest-reconcile-invalidation-postgres.test.ts", + "test/postgres-blob-publication-conflict.test.ts", "test/postgres-boot-migration-resume.test.ts", "test/postgres-bootstrap-deadlock-retry.test.ts", "test/postgres-derived-index-maintenance.test.ts", From ec7521362a695fa668ceb88466c7d175901528a0 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Thu, 10 Sep 2026 13:06:46 -0500 Subject: [PATCH 10/11] chore: drop research notes and tool feedback from the repository These are working notes about how the fix was researched, not artifacts the repository needs to build, test or review the change. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../260903-postgres-bloat-maintenance.md | 24 ------------------- ai/research/INDEX.md | 3 --- inbox/devspecs-feedback.md | 9 ------- 3 files changed, 36 deletions(-) delete mode 100644 ai/research/260903-postgres-bloat-maintenance.md delete mode 100644 ai/research/INDEX.md delete mode 100644 inbox/devspecs-feedback.md diff --git a/ai/research/260903-postgres-bloat-maintenance.md b/ai/research/260903-postgres-bloat-maintenance.md deleted file mode 100644 index 534b561b2..000000000 --- a/ai/research/260903-postgres-bloat-maintenance.md +++ /dev/null @@ -1,24 +0,0 @@ -# PostgreSQL bloat maintenance - -Date: 2026-09-03 - -## Question - -How should the reference implementation prevent high-churn PostgreSQL tables from accumulating physical bloat, and how can it reclaim derived-index space without stopping writers? - -## Findings - -- PostgreSQL supports table-specific autovacuum parameters with `ALTER TABLE ... SET (...)`; the same reloptions can address the table's TOAST relation with the `toast.` prefix. The relevant vacuum trigger is a threshold plus a scale-factor term, so low scale factors alone do not protect a table that has only a few very large JSONB or BYTEA rows. -- `VACUUM (ANALYZE)` cannot run inside a transaction block. `REINDEX ... CONCURRENTLY` also cannot run inside a transaction block, so the maintenance runner must borrow a direct session rather than use the transaction helper. -- A concurrent reindex allows normal writes but has stricter operational constraints. The runner therefore rebuilds only an explicit, static allowlist of derived indexes after observed dead-tuple pressure crosses a conservative threshold. - -## Decision - -The source-level fix is write elision: repeated structural-equality record payloads do not mutate `records` or append `record_changes`; content-addressed `blobs` already does the same for binary payloads. The implementation also avoids no-op metadata updates on that record path. These properties are separately proven with physical PostgreSQL relation statistics. Per-table heap and TOAST autovacuum reloptions for `records`, `record_changes`, `blobs`, and `spine_events`, plus the default 01:00-05:00 UTC maintenance job, are recovery safety nets only; they do not make a rewriting source path correct. The job vacuums known heavy tables and considers concurrent rebuild only for a static search-index allowlist. A durable database receipt claims each UTC window so a restart or another replica does not repeat an expensive partial pass. Set `PDPP_POSTGRES_DERIVED_INDEX_MAINTENANCE_WINDOW=disabled` to opt out or set another UTC window. A health receipt shows the last completed local job outcome. - -## Sources - -- PostgreSQL: [Routine Vacuuming](https://www.postgresql.org/docs/current/routine-vacuuming.html) -- PostgreSQL: [Automatic Vacuuming](https://www.postgresql.org/docs/current/runtime-config-vacuum.html) -- PostgreSQL: [REINDEX](https://www.postgresql.org/docs/current/sql-reindex.html) -- PostgreSQL: [CREATE TABLE storage parameters](https://www.postgresql.org/docs/current/sql-createtable.html) diff --git a/ai/research/INDEX.md b/ai/research/INDEX.md deleted file mode 100644 index ea042b1aa..000000000 --- a/ai/research/INDEX.md +++ /dev/null @@ -1,3 +0,0 @@ -# Research index - -- [260903 PostgreSQL bloat maintenance](260903-postgres-bloat-maintenance.md) — table-level autovacuum options and concurrent reindex constraints for the 2026-09-03 bloat repair. diff --git a/inbox/devspecs-feedback.md b/inbox/devspecs-feedback.md deleted file mode 100644 index dc95e0e54..000000000 --- a/inbox/devspecs-feedback.md +++ /dev/null @@ -1,9 +0,0 @@ -# devspecs feedback - -## 2026-09-03 — DB bloat repair - -`ds task "fix PostgreSQL storage bloat" --slice ...` waited at “Task index preflight: waiting for another index update” for more than 30 seconds and never produced a task slice. The command gave no owner, timeout, or recovery action, so I continued with the repository brief and targeted tests. A bounded wait plus a suggested retry/status command would make this easier to use during incident work. - -## 2026-09-09 — Concurrent blob cleanup repair - -`ds recent` completed in about six seconds and identified the existing reconciliation-bloat change and its files. `ds task "preserve shared PostgreSQL blobs during concurrent connector deletion" --quick` discovered 3,537 files, then stayed at “extracting and indexing artifacts” for over four minutes without producing a task. I stopped that invocation and continued from the lane brief and PostgreSQL regression tests. A quick task still needs a bounded path that can use known file paths without a full index. From abc7e57e87927ed48b7bd288b3f6a96ff37a1576 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 12 Sep 2026 22:01:58 -0500 Subject: [PATCH 11/11] fix(postgres): reclaim shared blobs whose last binding is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a connection could leave its payload bytes in the database forever, and a concurrent upload racing that delete could fail with an error telling the caller not to retry. Both defects survived the existing tests. `blobs` is globally content-addressed: the insert conflicts on `blob_id` alone, so two connections that publish identical bytes share one row plus two `blob_bindings` rows. `blobs.connector_instance_id` therefore records whichever connection uploaded the bytes first, not who still references them. `deleteUnreferencedBlobsPostgres` selected candidates by that column, so once the first uploader was gone, deleting the second left a row with zero bindings that no later candidate query could ever see again. Nothing in this codebase collects orphans, so those bytes stayed forever. Against PostgreSQL 17.11, deleting two connections that share one 47-byte payload left one blob row, zero bindings and zero records, still owned by the departed uploader, with the bytes intact. Candidates now come from the ids the binding delete returns — exactly the set this delete stranded, regardless of uploader. The same publication path had a second window. The blob insert is `ON CONFLICT DO NOTHING`, so it no-ops when the row already exists. Reclamation can then commit its delete before the following SELECT reads the row back. That path never reaches the binding insert, so the existing foreign-key handler never sees it. The missing row was reported as `api_error`, which `codeToStatus` pairs with HTTP 500. Pausing a real publication at that boundary with a statement trigger, then running the reclamation path, reproduced `api_error` where the retryable `blob_publication_conflict` belongs. A vanished row is now classified as that retryable 409; a row whose bytes disagree is still `api_error`, because retrying cannot fix it. Three tests cover the two postconditions, and each was confirmed to discriminate by reverting the behavior itself, not an assertion string. Restoring owner-scoped selection fails only the two new last-binding tests and leaves the other nine in that file passing, which is why the defect survived. Restoring `api_error` for a vanished row fails only the new pre-binding-window test and leaves the existing foreign-key-window test passing. With a real database, the seven PostgreSQL files here run 30 tests, all passing, no skips. The SQLite arm has the same owner-scoped candidate selection and the same leak, which is reproducible through the registered queries, but it is not fixed here: keying its reclamation on the just-unbound ids needs a returning-many read primitive that the bounded `lib/db.ts` wrapper does not expose. A comment at the call site records this. Signed-off-by: Tim Nunamaker Assisted-by: AI --- .../server/postgres-records.ts | 18 ++- reference-implementation/server/records.ts | 107 ++++++++++----- ...postgres-blob-publication-conflict.test.ts | 122 ++++++++++++++++++ .../test/postgres-orphan-blob-reclaim.test.ts | 103 +++++++++++++++ 4 files changed, 315 insertions(+), 35 deletions(-) diff --git a/reference-implementation/server/postgres-records.ts b/reference-implementation/server/postgres-records.ts index 1b8cf2738..ee6a0d350 100644 --- a/reference-implementation/server/postgres-records.ts +++ b/reference-implementation/server/postgres-records.ts @@ -3121,7 +3121,23 @@ async function postgresPersistContentAddressedBlobWithinFence({ blobId, ]); const [storedRow] = stored.rows; - if (!storedRow || storedRow.sha256 !== sha256 || Number(storedRow.size_bytes) !== sizeBytes) { + if (!storedRow) { + // The INSERT above is ON CONFLICT DO NOTHING, so it no-ops when the row + // already exists. Concurrent reclamation (`deleteUnreferencedBlobsPostgres`) + // can then commit its delete between that no-op and this SELECT, leaving + // no row to bind. That is the same reclaimed-during-publication race the + // FK-violation handler below catches at the later binding window, and it + // is equally retryable — re-running the publication re-inserts the bytes. + // Reporting it as `api_error` (HTTP 500) would tell a caller a retry is + // pointless, so classify it as the retryable 409 instead. + throw Object.assign(new Error("Blob was reclaimed during publication; retry the upload."), { + code: "blob_publication_conflict", + statusCode: 409, + }); + } + if (storedRow.sha256 !== sha256 || Number(storedRow.size_bytes) !== sizeBytes) { + // A row under this content-addressed id whose bytes disagree is a real + // integrity fault, not a race: retrying cannot fix it. const err: PgQueryError = new Error("Blob storage collision"); err.code = "api_error"; throw err; diff --git a/reference-implementation/server/records.ts b/reference-implementation/server/records.ts index 9d6e0556e..fa16a07ca 100644 --- a/reference-implementation/server/records.ts +++ b/reference-implementation/server/records.ts @@ -6688,7 +6688,10 @@ export async function deleteAllRecordsForConnector(connectorId: string, instance // Reclaim blobs the binding delete above just orphaned. Refcount-gated // (see delete-blobs-by-stream.sql) because content-addressed blob rows // are shared across connections and the FK cascades. Postgres parity: - // the same pair runs in `postgresDeleteAllRecordsForConnector`. + // the same pair runs in `postgresDeleteAllRecordsForConnector` -- but + // only the Postgres arm keys candidates on the just-unbound ids; this + // arm still keys on the first uploader. See the KNOWN GAP note in + // `deleteConnectionRecordRowsSqlite`. exec(referenceQueries.recordsDeleteDeleteBlobsByStream, [connectorInstanceId, stream]); } }); @@ -6793,10 +6796,10 @@ async function postgresDeleteAllRecordsForConnector(connectorId: string, instanc await postgresDeleteAllRecords(storageTarget, stream); await withPostgresTransaction( async (client) => { - await client.query("DELETE FROM blob_bindings WHERE connector_instance_id = $1 AND stream = $2", [ - connectorInstanceId, - stream, - ]); + const unbound = await client.query<{ blob_id: string }>( + "DELETE FROM blob_bindings WHERE connector_instance_id = $1 AND stream = $2 RETURNING blob_id", + [connectorInstanceId, stream] + ); // Reclaim blobs this delete just unbound, mirroring // `deleteConnectionRecordRowsPostgres` (and SQLite's // `delete-blobs-by-instance.sql`). Dropping the bindings above without @@ -6809,10 +6812,14 @@ async function postgresDeleteAllRecordsForConnector(connectorId: string, instanc // from a sibling connection share ONE row; and the FK from // `blob_bindings` is ON DELETE CASCADE, so an ungated delete here // would silently destroy a live sibling's binding. `NOT EXISTS` - // deletes only rows no binding still references. Scoped to this - // instance for the same reason the whole-connection sibling is: a - // delete reclaims its own bytes, never another connection's. - await deleteUnreferencedBlobsPostgres(client, connectorInstanceId, stream); + // deletes only rows no binding still references. Scoped to the rows + // THIS delete unbound (above), not to rows this instance uploaded: + // content addressing means the last connection to release a shared + // row is often not the one that created it. + await deleteUnreferencedBlobsPostgres( + client, + unbound.rows.map((row) => row.blob_id) + ); }, { lockConnectorInstanceId: connectorInstanceId } ); @@ -7011,53 +7018,76 @@ export function deleteConnectionRecordRowsSqlite(connectorInstanceId: string) { // connection. The registered delete query removes only unreferenced rows, // after this connection's bindings are gone, so the sibling binding remains // valid under SQLite's blob_bindings foreign key. + // + // KNOWN GAP (Postgres arm repaired, SQLite arm not): candidates are keyed on + // `blobs.connector_instance_id`, which names the FIRST uploader of globally + // content-addressed bytes. A row this connection was the LAST to reference + // but did not upload is left with zero bindings and no way to ever be seen + // again. Repairing this needs a returning-many read primitive the bounded + // `lib/db.ts` layer does not expose yet. exec(referenceQueries.recordsDeleteDeleteBlobsByInstance, [connectorInstanceId]); exec(referenceQueries.recordsDeleteDeleteAttentionRecordsByInstance, [connectorInstanceId]); exec(referenceQueries.recordsDeleteDeleteRecordsByInstance, [connectorInstanceId]); return count; } -/** Reclaim only locked candidates, with a fresh reference snapshot per batch. */ -async function deleteUnreferencedBlobsPostgres( - client: PostgresClient, - connectorInstanceId: string, - stream: string | null = null -): Promise { +/** + * Reclaim only locked candidates, with a fresh reference snapshot per batch. + * + * `unboundBlobIds` is the set of blobs whose bindings THIS delete just removed, + * captured by the caller's `DELETE ... RETURNING blob_id`. Candidates must be + * keyed on that set rather than on `blobs.connector_instance_id`: `blobs` is + * globally content-addressed, so the row records whichever connection uploaded + * the bytes FIRST, not who still references them. When two connections publish + * identical bytes they share one row owned by the first; deleting the second + * leaves a row with zero bindings that an owner-scoped candidate query can + * never see again, and nothing else in the codebase collects orphans. Keying on + * the unbound set reclaims exactly the bytes this delete stranded, whoever + * uploaded them. + */ +async function deleteUnreferencedBlobsPostgres(client: PostgresClient, unboundBlobIds: string[]): Promise { + if (unboundBlobIds.length === 0) { + return; + } + // Deterministic lock order across concurrent connection deletes that unbind + // an overlapping shared row: both take `blobs` rows in ascending blob_id. + const orderedBlobIds = [...new Set(unboundBlobIds)].sort(); // Match the store's admission budget even for caller-owned transactions that // did not acquire an instance advisory lock. SET LOCAL ends at COMMIT/ROLLBACK. await client.query(`SET LOCAL lock_timeout = '${connectorInstanceLockWaitMs()}ms'`); const batchSize = 256; - let afterBlobId = ""; + let offset = 0; let hasMore = true; try { while (hasMore) { + const batch = orderedBlobIds.slice(offset, offset + batchSize); + if (batch.length === 0) { + return; + } // The FK takes KEY SHARE when a writer adds a binding. Wait here, then // check references in a NEW READ COMMITTED statement so committed writers // are visible. A single CTE would retain the stale statement snapshot. - // Keyset batches bound the IDs held in Node; locks remain transaction-wide. - // biome-ignore lint/performance/noAwaitInLoops: each batch follows the preceding locked key range. + // Batches bound the rows locked per statement; locks remain transaction-wide. + // biome-ignore lint/performance/noAwaitInLoops: each batch must lock and reclaim before the next one starts. const candidates = await client.query<{ blob_id: string }>( `SELECT blob_id FROM blobs - WHERE connector_instance_id = $1 AND blob_id > $2 - AND ($3::text IS NULL OR stream = $3) - ORDER BY blob_id LIMIT $4 FOR UPDATE`, - [connectorInstanceId, afterBlobId, stream, batchSize] + WHERE blob_id = ANY($1::text[]) + ORDER BY blob_id FOR UPDATE`, + [batch] ); - const lastCandidate = candidates.rows.at(-1); - if (!lastCandidate) { - return; - } const blobIds = candidates.rows.map((row) => row.blob_id); - await client.query( - `DELETE FROM blobs + if (blobIds.length > 0) { + await client.query( + `DELETE FROM blobs WHERE blob_id = ANY($1::text[]) AND NOT EXISTS ( SELECT 1 FROM blob_bindings WHERE blob_bindings.blob_id = blobs.blob_id )`, - [blobIds] - ); - afterBlobId = lastCandidate.blob_id; - hasMore = blobIds.length === batchSize; + [blobIds] + ); + } + offset += batchSize; + hasMore = offset < orderedBlobIds.length; } } catch (err) { if ((err as { code?: string } | null)?.code === "55P03") { @@ -7082,8 +7112,17 @@ export async function deleteConnectionRecordRowsPostgres(client: PostgresClient, const count = Number(countResult.rows[0]?.count || 0); await client.query("DELETE FROM record_changes WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM version_counter WHERE connector_instance_id = $1", [connectorInstanceId]); - await client.query("DELETE FROM blob_bindings WHERE connector_instance_id = $1", [connectorInstanceId]); - await deleteUnreferencedBlobsPostgres(client, connectorInstanceId); + // RETURNING captures the blobs this delete unbound. Reclamation must be keyed + // on that set, not on `blobs.connector_instance_id` — see + // `deleteUnreferencedBlobsPostgres`. + const unbound = await client.query<{ blob_id: string }>( + "DELETE FROM blob_bindings WHERE connector_instance_id = $1 RETURNING blob_id", + [connectorInstanceId] + ); + await deleteUnreferencedBlobsPostgres( + client, + unbound.rows.map((row) => row.blob_id) + ); await client.query("DELETE FROM connector_attention_records WHERE connector_instance_id = $1", [connectorInstanceId]); await client.query("DELETE FROM records WHERE connector_instance_id = $1", [connectorInstanceId]); return count; diff --git a/reference-implementation/test/postgres-blob-publication-conflict.test.ts b/reference-implementation/test/postgres-blob-publication-conflict.test.ts index e652f0f41..365bf1c74 100644 --- a/reference-implementation/test/postgres-blob-publication-conflict.test.ts +++ b/reference-implementation/test/postgres-blob-publication-conflict.test.ts @@ -8,15 +8,137 @@ import pg from "pg"; import { closeDb, initDb } from "../server/db.ts"; import { postgresPersistContentAddressedBlob } from "../server/postgres-records.ts"; import { closePostgresStorage, initPostgresStorage, postgresQuery } from "../server/postgres-storage.ts"; +import { deleteConnectionRecordRowsPostgres } from "../server/records.ts"; import { codeToStatus } from "../server/routes/ref-error-status.ts"; import { withTemporaryPostgresDatabase } from "./helpers/postgres-temp-database.ts"; const POSTGRES_URL = process.env.PDPP_TEST_POSTGRES_URL; +// Arbitrary advisory-lock namespace, unique to this file's scheduling seam. +const LOCK_KEY = 590_912; test("blob publication conflict maps to HTTP 409 in the shared error envelope", () => { assert.equal(codeToStatus.blob_publication_conflict, 409); }); +/** + * The test below pauses publication at the `blob_bindings` INSERT, where the FK + * violation surfaces. There is an EARLIER window: the `blobs` INSERT is + * `ON CONFLICT DO NOTHING`, so when the row already exists it no-ops, and + * reclamation can commit its delete before the following SELECT reads the row + * back. That path never reaches the binding INSERT, so the FK handler never + * sees it. It is the same reclaimed-during-publication race and equally + * retryable, so it must report the same retryable conflict rather than a + * generic server fault that tells callers not to retry. + * + * The statement trigger is a scheduling seam only: the production INSERT runs + * in full, then parks before the production SELECT can start. Every other + * statement — the publication, the reclamation and the retry — is real. + */ +test("blob publication reports a retryable conflict when reclamation wins the pre-binding window", { + skip: !POSTGRES_URL, +}, async () => { + assert.ok(POSTGRES_URL); + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_blob_publish_prebind_${Date.now().toString(36)}`, + }, + async (url) => { + initDb(":memory:"); + await initPostgresStorage({ backend: "postgres", databaseUrl: url }); + const gate = new pg.Client({ connectionString: url }); + const cleanup = new pg.Client({ connectionString: url }); + let publication: ReturnType | undefined; + try { + await gate.connect(); + await cleanup.connect(); + const args = { + connectorId: "https://registry.pdpp.test/connectors/blob_publication_prebind", + connectorInstanceId: "cin_blob_prebind_doomed", + data: Buffer.from("shared bytes reclaimed before the binding insert"), + mimeType: "application/octet-stream", + recordKey: "attachment-1", + stream: "attachments", + }; + const original = await postgresPersistContentAddressedBlob(args); + const gatePid = (await gate.query<{ pid: number }>("SELECT pg_backend_pid() AS pid")).rows[0]?.pid; + await gate.query("SELECT pg_advisory_lock($1, 1)", [LOCK_KEY]); + await postgresQuery( + `CREATE FUNCTION pdpp_pause_blob_insert() RETURNS trigger LANGUAGE plpgsql AS + $$ BEGIN PERFORM pg_advisory_xact_lock(${LOCK_KEY}, 1); RETURN NULL; END $$` + ); + await postgresQuery( + "CREATE TRIGGER pdpp_pause_blob_insert AFTER INSERT ON blobs FOR EACH STATEMENT EXECUTE FUNCTION pdpp_pause_blob_insert()" + ); + + publication = postgresPersistContentAddressedBlob({ + ...args, + connectorInstanceId: "cin_blob_prebind_survivor", + }); + publication.catch(() => undefined); + let blocked = false; + const deadline = Date.now() + 3000; + while (!blocked && Date.now() < deadline) { + // biome-ignore lint/performance/noAwaitInLoops: observe the parked INSERT before allowing reclamation to commit. + const observed = await postgresQuery<{ blocked: boolean }>( + `SELECT EXISTS (SELECT 1 FROM pg_stat_activity + WHERE datname = current_database() AND $1::int = ANY(pg_blocking_pids(pid)) + AND query LIKE 'INSERT INTO blobs%') AS blocked`, + [gatePid] + ); + blocked = observed.rows[0]?.blocked ?? false; + if (!blocked) { + await delay(10); + } + } + assert.ok(blocked, "publication must park after its blobs INSERT, before the read-back"); + + // Real production reclamation, not a hand-written DELETE. + await cleanup.query("BEGIN"); + await cleanup.query("SET LOCAL lock_timeout = '2000ms'"); + await deleteConnectionRecordRowsPostgres(cleanup, "cin_blob_prebind_doomed"); + await cleanup.query("COMMIT"); + assert.equal( + (await postgresQuery("SELECT blob_id FROM blobs WHERE blob_id = $1", [original.blob_id])).rows.length, + 0, + "reclamation committed the delete while publication was parked" + ); + + await gate.query("SELECT pg_advisory_unlock($1, 1)", [LOCK_KEY]); + await assert.rejects(publication, { + code: "blob_publication_conflict", + message: "Blob was reclaimed during publication; retry the upload.", + statusCode: 409, + }); + + await postgresQuery("DROP TRIGGER pdpp_pause_blob_insert ON blobs"); + const retry = await postgresPersistContentAddressedBlob({ + ...args, + connectorInstanceId: "cin_blob_prebind_survivor", + }); + assert.equal(retry.blob_id, original.blob_id); + assert.equal(retry.binding_inserted, true); + const restored = await postgresQuery<{ data: Buffer }>("SELECT data FROM blobs WHERE blob_id = $1", [ + original.blob_id, + ]); + assert.ok( + restored.rows[0]?.data.equals(args.data), + "the advertised retry restores the exact payload, so the 409 is actionable" + ); + } finally { + await cleanup.query("ROLLBACK").catch(() => undefined); + await gate.query("SELECT pg_advisory_unlock($1, 1)", [LOCK_KEY]).catch(() => undefined); + await publication?.catch(() => undefined); + await gate.end().catch(() => undefined); + await cleanup.end().catch(() => undefined); + await closePostgresStorage(); + closeDb(); + } + } + ); +}); + test("blob publication reports a retryable conflict when reclamation wins its FK lock", { skip: !POSTGRES_URL, }, async () => { diff --git a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts index 2c19ca454..bd0424bd7 100644 --- a/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts +++ b/reference-implementation/test/postgres-orphan-blob-reclaim.test.ts @@ -288,6 +288,109 @@ if (POSTGRES_URL) { } ) }) + + // The test above stops after deleting the FIRST of two sharers, so it only + // pins sibling retention. Deleting the LAST sharer is the postcondition that + // decides whether the shared bytes are ever reclaimed, and it is the case + // where owner-scoped candidate selection fails: `blobs.connector_instance_id` + // records whichever connection uploaded the bytes first, so after that + // connection is gone no candidate query keyed on it can ever see the row + // again. Both delete paths are covered because both reclaim independently. + for (const scope of ["per-stream", "whole-connection"] as const) { + test(`${scope} delete reclaims a shared blob when it removes the LAST binding`, async () => { + await withTemporaryPostgresDatabase( + { + closeConnections: closePostgresStorage, + connectionString: POSTGRES_URL, + databaseName: `pdpp_last_owner_reclaim_${Date.now().toString(36)}`, + }, + async url => { + initDb(":memory:") + await initPostgresStorage({ backend: "postgres", databaseUrl: url }) + try { + const sharedBytes = Buffer.alloc(32 * 1024, 0x3c) + const uploader = { + connectorId: + "https://registry.pdpp.test/connectors/last_owner_first", + connectorInstanceId: "cin_last_owner_first", + } + const survivor = { + connectorId: + "https://registry.pdpp.test/connectors/last_owner_second", + connectorInstanceId: "cin_last_owner_second", + } + const uploaders = [uploader, survivor] + const blobIds: string[] = [] + for (const identity of uploaders) { + blobIds.push( + // biome-ignore lint/performance/noAwaitInLoops: the second upload must observe the first's committed row to dedupe. + await seedRecordWithBlob({ + bytes: sharedBytes, + connectorId: identity.connectorId, + connectorInstanceId: identity.connectorInstanceId, + recordKey: "att-1", + }) + ) + } + const sharedBlobId = blobIds[0] ?? "" + assert.equal( + blobIds[0], + blobIds[1], + "fixture premise: identical bytes dedupe to a single content-addressed blob row" + ) + assert.equal( + await countBindings(sharedBlobId), + 2, + "baseline: both connections bind the same blob" + ) + + const removeConnection = async (identity: typeof uploader) => + scope === "per-stream" + ? await deleteAllRecordsForConnector(identity.connectorId) + : await withPostgresTransaction(client => + deleteConnectionRecordRowsPostgres( + client, + identity.connectorInstanceId + ) + ) + + // Delete the UPLOADER first, so the survivor is a connection that + // never owned the row. This is the ordering that strands the bytes. + await removeConnection(uploader) + assert.equal( + await countBlobs(sharedBlobId), + 1, + "a blob the surviving connection still binds must NOT be deleted" + ) + const retainedBytes = await postgresQuery<{ data: Buffer }>( + "SELECT data FROM blobs WHERE blob_id = $1", + [sharedBlobId] + ) + assert.ok( + retainedBytes.rows[0]?.data.equals(sharedBytes), + "the survivor's payload is preserved byte-for-byte, not truncated or zeroed" + ) + + await removeConnection(survivor) + assert.equal( + await countBindings(sharedBlobId), + 0, + "the last delete removes the final binding" + ) + assert.equal( + await countBlobs(sharedBlobId), + 0, + "removing the final binding must reclaim the row even though a DIFFERENT connection uploaded it" + ) + } finally { + await closePostgresStorage() + closeDb() + } + } + ) + }) + } + for (const scope of ["per-stream", "whole-connection"] as const) { test(`${scope} delete reclaims more than one batch while retaining shared blobs`, async () => { await withTemporaryPostgresDatabase(