diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 4158a2e0e7..a0eeeefb8c 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3541,6 +3541,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(); + }); +});