From e9efcc09c0bbc145170a07bf83a514399b5b7b66 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 10:30:18 -0700 Subject: [PATCH] perf(selfhost): tune Postgres autovacuum and document the observation-write batching decision Closes #2543. github_rate_limit_observations receives one INSERT per outbound GitHub API response and is pruned in daily bulk deletes by the retention job -- an insert-then-bulk-delete pattern that is exactly the shape that causes dead-tuple bloat under Postgres's stock autovacuum settings (the codebase already has an alert watching for this symptom generally, but nothing pre-empted it for this specific table). Adds tuneGithubRateLimitObservationsAutovacuum (src/selfhost/pg-adapter.ts), applying a lower autovacuum_vacuum_scale_factor (0.05 vs Postgres's 0.2 default) via the same D1Database.exec() surface runSelfHostMigrations already uses for migrations. Runs once at boot, after migrations (the table must exist first), gated behind the existing usePostgres check -- a no-op on SQLite, which has no autovacuum concept at all. The ALTER is idempotent (re-applying the same storage parameter is a no-op), so it needs no migration-ledger tracking, and best-effort (a failed tune logs and continues rather than blocking boot -- an optimization, never a correctness dependency). Verified against a real Postgres 16 container, not just a mocked interaction test. Evaluated batching the observation write (the issue's second ask) and documented the decision NOT to implement it, in place at the one call site (src/github/backfill.ts): the write rate is bounded by GitHub's own REST budget for a single App installation (~5000/hour, further capped by QUEUE_CONCURRENCY's small worker pool), nowhere near a volume that meaningfully pressures a Postgres connection pool with single-row INSERTs. shouldWaitForGitHubRateLimit reads the LATEST row from this exact table for admission control across every self-host queue worker, including in a multi-instance/shared-Postgres deployment -- a batching window would trade a real but currently-unmeasured write-volume concern for a genuine risk to the admission-control freshness the #1936 rate-limit-reliability campaign this whole roadmap is part of was built to protect. --- src/github/backfill.ts | 14 ++++ src/selfhost/pg-adapter.ts | 31 ++++++++ src/server.ts | 5 +- test/integration/selfhost-pg.test.ts | 16 +++- .../selfhost-pg-adapter-autovacuum.test.ts | 76 +++++++++++++++++++ 5 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 test/unit/selfhost-pg-adapter-autovacuum.test.ts diff --git a/src/github/backfill.ts b/src/github/backfill.ts index df40c6f30c..5c10646099 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3536,6 +3536,20 @@ function summarizeSegments( }; } +// #2543: this is the ONLY call site of recordGitHubRateLimitObservation -- one row per outbound GitHub REST/ +// GraphQL response. DELIBERATELY left un-batched (documented decision, not an oversight): the write rate is +// bounded by GitHub's own REST budget for a single App installation (~5000/hour ≈ 1.4/s sustained, further +// capped in practice by QUEUE_CONCURRENCY's small worker-pool size), nowhere near a volume where single-row +// Postgres INSERTs meaningfully pressure the connection pool. shouldWaitForGitHubRateLimit (rate-limit.ts) +// reads the LATEST row from this exact table for admission control across every self-host queue worker +// (including in a multi-instance/shared-Postgres deployment, where a buffering instance would make its own +// writes stale to every OTHER instance's reads, not just its own) -- a batching window here trades a real, +// bounded-scale write-volume concern for a genuine risk to the admission-control freshness the #1936 rate- +// limit-reliability campaign was built around: a stale "remaining: 500" observation would let a queue worker +// admit a job it should have deferred, right when conserving the budget matters most. Revisit ONLY if this +// table's write volume is ever independently measured to actually pressure the pool -- the table-level +// autovacuum tuning (tuneGithubRateLimitObservationsAutovacuum, src/selfhost/pg-adapter.ts) already addresses +// the dead-tuple-bloat half of this issue, which is the part that was actually observable/anticipated. async function recordGitHubResponse( env: Env, repoFullName: string | null, diff --git a/src/selfhost/pg-adapter.ts b/src/selfhost/pg-adapter.ts index f5d6d1104d..2fec78743a 100644 --- a/src/selfhost/pg-adapter.ts +++ b/src/selfhost/pg-adapter.ts @@ -83,3 +83,34 @@ export function createPgAdapter(pool: Pool): D1Database { }; return adapter as unknown as D1Database; } + +// #2543: github_rate_limit_observations receives one INSERT per outbound GitHub API response and is pruned in +// daily bulk deletes by the retention job (pruneExpiredRecords) -- an insert-then-bulk-delete pattern that is +// exactly the shape that causes dead-tuple bloat under Postgres's stock autovacuum settings (scale_factor 0.2, +// i.e. autovacuum waits for 20% of the table to be dead before vacuuming -- fine for a slowly-growing table, +// too lax for one that gets emptied in one daily burst). Lowering the scale factor makes autovacuum reclaim +// space promptly after each day's bulk delete instead of letting dead tuples accumulate across cycles. A +// storage-parameter ALTER is idempotent (re-applying the same value is a no-op), so this runs unconditionally +// on every Postgres boot rather than needing its own migration-ledger tracking. SQLite has no autovacuum +// concept at all, so this must never run there -- callers gate it behind the Postgres backend check, matching +// PGPOOL_MAX/resolvePostgresPoolMax's own "server.ts wiring, tested logic elsewhere" split (src/selfhost/ +// queue-common.ts), since server.ts itself has no test harness (top-level main(), Codecov-ignored). +export const GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL = + "ALTER TABLE github_rate_limit_observations SET (autovacuum_vacuum_scale_factor = 0.05, autovacuum_vacuum_threshold = 50)"; + +/** Apply the autovacuum tuning above via the SAME D1Database.exec() surface runSelfHostMigrations already uses + * for migrations -- so this reuses translateDdl's existing SQL path rather than a second raw-pool query + * mechanism. Must be called AFTER migrations (the table has to exist first); best-effort by design (a + * storage-parameter tweak is an optimization, never a correctness dependency -- a failure here must not stop + * the self-host from booting). */ +export async function tuneGithubRateLimitObservationsAutovacuum(db: D1Database): Promise { + await db.exec(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL).catch((error: unknown) => { + console.error( + JSON.stringify({ + level: "warn", + event: "selfhost_autovacuum_tune_failed", + error: error instanceof Error ? error.message : String(error), + }), + ); + }); +} diff --git a/src/server.ts b/src/server.ts index 1557d44705..8bd7599c51 100644 --- a/src/server.ts +++ b/src/server.ts @@ -50,7 +50,7 @@ import { } from "./selfhost/health"; import { gauge, incr, observe, renderMetrics } from "./selfhost/metrics"; import { runSelfHostMigrations } from "./selfhost/migrate"; -import { createPgAdapter } from "./selfhost/pg-adapter"; +import { createPgAdapter, tuneGithubRateLimitObservationsAutovacuum } from "./selfhost/pg-adapter"; import { createPgQueue } from "./selfhost/pg-queue"; import { createPgVectorize, initPgVectorize } from "./selfhost/pg-vectorize"; import { resolvePostgresPoolMax } from "./selfhost/queue-common"; @@ -370,6 +370,9 @@ async function main(): Promise { console.log( JSON.stringify({ event: "selfhost_migrations_applied", count: applied }), ); + // #2543: Postgres-only, applied AFTER migrations (the table must already exist). No-op on SQLite, which has + // no autovacuum concept at all -- gated on the same usePostgres check the backend was built from. + if (usePostgres) await tuneGithubRateLimitObservationsAutovacuum(backend.db); const ai = createSelfHostAi(process.env); if (ai) diff --git a/test/integration/selfhost-pg.test.ts b/test/integration/selfhost-pg.test.ts index 0589d168c8..481d254b26 100644 --- a/test/integration/selfhost-pg.test.ts +++ b/test/integration/selfhost-pg.test.ts @@ -5,7 +5,7 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; import pg from "pg"; import { runSelfHostMigrations } from "../../src/selfhost/migrate"; -import { createPgAdapter } from "../../src/selfhost/pg-adapter"; +import { createPgAdapter, tuneGithubRateLimitObservationsAutovacuum } from "../../src/selfhost/pg-adapter"; import { pruneExpiredRecords } from "../../src/db/retention"; import { processJob } from "../../src/queue/processors"; @@ -84,4 +84,18 @@ suite("Postgres backend (#977) — real Postgres", () => { const audit = await db.prepare("SELECT outcome FROM audit_events WHERE event_type = ?").bind("retention.prune").first<{ outcome: string }>(); expect(audit?.outcome).toBe("success"); }); + + it("tunes github_rate_limit_observations autovacuum below Postgres's default, idempotently (#2543)", async () => { + const db = createPgAdapter(pool); + + await tuneGithubRateLimitObservationsAutovacuum(db); + await tuneGithubRateLimitObservationsAutovacuum(db); // idempotent -- a second apply must not throw + + const row = await pool.query<{ reloptions: string[] | null }>( + "SELECT reloptions FROM pg_class WHERE relname = 'github_rate_limit_observations'", + ); + const options = row.rows[0]?.reloptions ?? []; + expect(options).toContain("autovacuum_vacuum_scale_factor=0.05"); + expect(options).toContain("autovacuum_vacuum_threshold=50"); + }); }); diff --git a/test/unit/selfhost-pg-adapter-autovacuum.test.ts b/test/unit/selfhost-pg-adapter-autovacuum.test.ts new file mode 100644 index 0000000000..756bd5b344 --- /dev/null +++ b/test/unit/selfhost-pg-adapter-autovacuum.test.ts @@ -0,0 +1,76 @@ +// Unit tests for the github_rate_limit_observations autovacuum tuning step (#2543). Uses a mock D1Database +// (just the .exec() surface runSelfHostMigrations already relies on) so no real Postgres is required -- the +// SQL itself is plain, already-Postgres-native syntax with no SQLite constructs for pg-dialect.ts to translate, +// so a mocked interaction test is a faithful, fast substitute for a live ALTER TABLE. +import { describe, expect, it, vi } from "vitest"; +import { + GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL, + tuneGithubRateLimitObservationsAutovacuum, +} from "../../src/selfhost/pg-adapter"; + +function mockDb(execImpl: (sql: string) => Promise): D1Database { + return { exec: vi.fn(execImpl) } as unknown as D1Database; +} + +describe("GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL (#2543)", () => { + it("targets the github_rate_limit_observations table with a scale factor below Postgres's 0.2 default", () => { + expect(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL).toContain("github_rate_limit_observations"); + expect(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL).toContain("autovacuum_vacuum_scale_factor"); + const match = GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL.match(/autovacuum_vacuum_scale_factor\s*=\s*([\d.]+)/); + expect(match).not.toBeNull(); + expect(Number(match?.[1])).toBeLessThan(0.2); + expect(Number(match?.[1])).toBeGreaterThan(0); + }); + + it("is a single idempotent storage-parameter ALTER, not an additive/destructive DDL statement", () => { + expect(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL.trim().toUpperCase()).toMatch(/^ALTER TABLE/); + expect(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL).not.toMatch(/DROP|DELETE|TRUNCATE/i); + }); +}); + +describe("tuneGithubRateLimitObservationsAutovacuum (#2543)", () => { + it("applies the autovacuum SQL via db.exec()", async () => { + const db = mockDb(async () => ({ count: 1, duration: 0 })); + + await tuneGithubRateLimitObservationsAutovacuum(db); + + expect(db.exec).toHaveBeenCalledWith(GITHUB_RATE_LIMIT_OBSERVATIONS_AUTOVACUUM_SQL); + expect(db.exec).toHaveBeenCalledTimes(1); + }); + + it("fails open (does not throw) when db.exec rejects -- an optimization, never a boot-blocking dependency", async () => { + const db = mockDb(async () => { + throw new Error("connection reset"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await expect(tuneGithubRateLimitObservationsAutovacuum(db)).resolves.toBeUndefined(); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("selfhost_autovacuum_tune_failed")); + errorSpy.mockRestore(); + }); + + it("logs the underlying error message on failure", async () => { + const db = mockDb(async () => { + throw new Error("relation does not exist"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await tuneGithubRateLimitObservationsAutovacuum(db); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("relation does not exist")); + errorSpy.mockRestore(); + }); + + it("stringifies a non-Error rejection instead of throwing on error.message access", async () => { + const db = mockDb(async () => { + throw "a plain string rejection"; + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await expect(tuneGithubRateLimitObservationsAutovacuum(db)).resolves.toBeUndefined(); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("a plain string rejection")); + errorSpy.mockRestore(); + }); +});