Skip to content

fix(044): stop connection faults from crashing the instance and starving the pool - #126

Merged
studert merged 2 commits into
mainfrom
worktree-044-db-connection-reliability
Aug 7, 2026
Merged

fix(044): stop connection faults from crashing the instance and starving the pool#126
studert merged 2 commits into
mainfrom
worktree-044-db-connection-reliability

Conversation

@studert

@studert studert commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

Three production error signatures over seven weeks, all traced to the shared Neon connection layer. Diagnosed from Vercel runtime error groups rather than inference — the 2026-08-04 deploy (ea50404) is not implicated (it touched no DB, pool or dashboard code, and all routes served 200 at 10:27–10:37 before the 12:00 cron tick).

ID Signature Count Routes
A Cron … failed: Failed query: SELECT pg_try_advisory_lock($1) 63 /api/sync/anthropic-api-costs, /api/sync/anthropic-usage
B timeout exceeded when trying to connect 2+ /
C Unhandled error. () at idleListenerexit status: 129 18 /, /api/mcp, /api/auth, /api/profile, /api/oauth/token

Every symptom-A occurrence is timestamped :00:01 past the hour.

Root causes

1. max: 1 starved by concurrent work. Justified in specs/001 as "one connection per serverless function instance" — an assumption predating Fluid Compute, which reuses one instance across concurrent invocations. Both hourly crons fired at 0 * * * *; the admin dashboard fans out 9+3 queries. At 12:00 UTC: api-costs cron takes the socket 12:00:41 → usage cron's first query fails 12:00:49 → GET / fails 12:00:50 and 12:01:06.

2. No error listener on either pool object. Probed against @neondatabase/serverless 1.0.2: pool.emit("error", …) with no listener throws synchronously (that is signature C), and _acquireClient strips the idle listener while neither query() nor connect() re-attaches one — so a socket death between statements inside a db.transaction() had nothing to reject into. Under Fluid Compute one uncaught exception takes down every in-flight request on the instance.

3. Session advisory locks are unsupported on Neon's pooled endpoint (PgBouncer transaction mode). DATABASE_URL is the -pooler host. The repo already knew — vitest.config.integration.mts:14 documents this exact failure and works around it for tests only; production was never protected.

Changes

# File Change Addresses
1 src/lib/db/index.ts pool.on("error") + pool.on("connect", c => c.on("error")) C
2 src/lib/db/index.ts max 1 → 10; connectionTimeoutMillis 10s → 15s A, B
3 vercel.json Stagger crons to 20 * * * * / 40 6 * * * A, B
4 src/lib/sync/framework.ts Total release + no-op-unlock detection; bookkeeping guarded on both paths; event insert moved inside try A
5 src/lib/sync/framework.ts Unconditional stale-in_progress sweep (60 min, no migration)
6 src/lib/sync/cron-handler.ts 409 for contention, 500 for unexpected errors
7 sync-dashboard.tsx 10-min fast-poll cap, document.hidden skip, abandoned-id guard
8 3 cron routes + settings/sync/page.tsx maxDuration = 300 A

max: 10 is safe specifically because the endpoint is pooled: PgBouncer accepts max_client_conn=10000 and sizes its server pool at 0.9 × max_connections, so this adds sockets to the pooler, not Postgres backends. On the direct endpoint it would be unsafe — that was the gating question and it is resolved.

The per-client handler attaches on connect, which _acquireClient emits before removeListener("error", …) and only for new clients — so it survives checkout without accumulating per checkout.

Deferred (with reasons)

Full write-up in specs/044-db-connection-reliability/.

  • TTL row lease replacing the advisory lock — needs migration 0030, and this repo has no automated migration step (build is next build; db:migrate is manual), so it requires a schema-first deploy verified against production, plus the concurrency test still sitting as it.todo at tests/integration/sync/lock.test.ts:22. What ships here makes the existing lock loud and self-healing rather than silent.
  • Convergent writes for billed_costs (no unique constraint; every safety argument rests on the lock) — needs a duplicate audit first.
  • Retry-on-stale-connection, dashboard fan-out dedup, bounding getAssignments(), backfill resumability.

Operational notes

  • This does not release an advisory lock already leaked in production. If a source stops syncing after deploy, a Neon compute restart is the fastest remedy.
  • Cron endpoints now return 409/500 instead of a blanket 200. Monitoring that asserted "cron returns 200" will start reporting failures that were always happening — expected, not a new regression.

Verification

Check Result
pnpm typecheck clean (baseline captured clean before any edit)
eslint on touched paths, --max-warnings 0 clean, exit 0
pnpm test 55 files / 672 tests passed — baseline 53/660, +12 new, no regressions
pnpm build succeeded — validates the new maxDuration route-segment exports
Mutation check commenting out pool.on("error") fails exactly the 2 tests asserting it

The fake Pool in tests/unit/db/pool-error-handling.test.ts reproduces Node's throw-on-unhandled-error contract, so the tests fail on absence rather than passing on a listener count.

Not verified here: no integration or browser pass — the worktree has no credentials. max: 10 is reasoned from Neon's documented pooler limits, not measured; it wants a canary with connection-count and CPU graphs before production.

🤖 Generated with Claude Code

…ing the pool

Three production error signatures over seven weeks, all in the shared Neon
connection layer. Diagnosed from Vercel runtime error groups, not inference;
the 2026-08-04 deploy (ea50404) is not implicated.

A. `Cron X sync failed: Failed query: SELECT pg_try_advisory_lock($1)` (63x,
   every one timestamped :00-:01 past the hour)
B. `timeout exceeded when trying to connect` on GET /
C. `Unhandled error. () at idleListener` -> `exit status: 129` (18x, across
   /, /api/mcp, /api/auth, /api/profile, /api/oauth/token)

Root causes:

1. max:1 on a module-level pool. The original design note justifies it as "one
   connection per serverless function instance" -- an assumption predating Fluid
   Compute, which reuses one instance across CONCURRENT invocations. Both hourly
   crons fired at `0 * * * *`; the admin dashboard fans out 9+3 queries. At
   12:00 UTC: api-costs cron takes the socket 12:00:41 -> usage cron's first
   query fails 12:00:49 -> GET / fails 12:00:50 and 12:01:06.

2. No error listener on either pool object. Probed against
   @neondatabase/serverless 1.0.2: `pool.emit("error")` with no listener throws
   synchronously (that is signature C), and `_acquireClient` strips the idle
   listener while NEITHER `query()` NOR `connect()` re-attaches one -- so a
   socket death between statements inside a db.transaction() had nothing to
   reject into. Under Fluid Compute one uncaught exception takes down every
   in-flight request on the instance.

3. Session advisory locks are unsupported on Neon's pooled endpoint (PgBouncer
   transaction mode). DATABASE_URL is the `-pooler` host. The repo already knew:
   vitest.config.integration.mts:14 documents this exact failure and works
   around it for TESTS ONLY -- production was never protected.

Changes: pool error handling on both the pool and per-connected-client (the
latter attaches on `connect`, which fires before removeListener and only for
new clients, so it survives checkout without accumulating); max 1 -> 10 (safe:
PgBouncer fronts 10k client connections, so this adds sockets to the pooler,
not Postgres backends); connectionTimeoutMillis 10s -> 15s for cold-start
headroom; staggered crons; total lock release with no-op detection; bookkeeping
writes guarded on BOTH success and failure paths so they can never mask or
invert the real outcome; the sync_events insert moved inside the try so a
failure there cannot strand the lock; an unconditional stale-in_progress sweep;
409/500 from cron routes instead of a blanket 200 (which is why 63 failures
looked green); and a 10-minute cap on the dashboard's 5s poll, which a single
stranded row otherwise turned into permanent per-tab database load.

The TTL row lease replacing the advisory lock is deliberately deferred -- it
needs a migration and this repo has no automated migration step, so it requires
a schema-first deploy. What ships here makes the existing lock loud and
self-healing rather than silent. See specs/044-db-connection-reliability/.

Verification: typecheck clean, eslint clean on touched paths, 55 files / 672
tests pass (baseline 53/660, +12 new, no regressions), production build
succeeds. The new pool tests were mutation-checked: commenting out
pool.on("error") fails exactly the two tests that assert it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 4, 2026 13:18
@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ai-developer-hub Ready Ready Preview Aug 4, 2026 1:38pm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Neon DB connection layer and sync framework to prevent connection faults from crashing a warm Vercel Fluid Compute instance, reduce pool starvation under concurrent load, and make cron/sync behavior more observable and self-healing.

Changes:

  • Increase DB pool capacity and add explicit pool/client "error" listeners to prevent unhandled socket errors from crashing the process.
  • Make sync lifecycle more resilient (best-effort bookkeeping, unconditional stale in_progress sweep, and explicit unlock/no-op logging) and improve cron route HTTP signaling.
  • Reduce operational load from sync polling (caps/visibility checks) and stagger Vercel cron schedules; add maxDuration bounds to sync routes/actions.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vercel.json Staggers cron schedules to reduce concurrency collisions on warm instances.
src/lib/db/index.ts Raises pool max, increases connection timeout, and adds pool/client error listeners to prevent instance crashes.
src/lib/sync/framework.ts Adds stale in_progress reaping and makes lock/event lifecycle bookkeeping and unlock behavior more fault-tolerant.
src/lib/sync/cron-handler.ts Returns 409 on contention and 500 on unexpected errors so cron monitoring reflects real failures.
src/app/settings/sync/sync-dashboard.tsx Caps fast polling, skips polling in hidden tabs, and avoids re-arming on abandoned events to prevent runaway DB load.
src/app/settings/sync/page.tsx Adds maxDuration for server actions dispatched from the sync settings route segment.
src/app/api/sync/github-copilot/route.ts Adds maxDuration export for cron route.
src/app/api/sync/anthropic-usage/route.ts Adds maxDuration export for cron route and documents rationale.
src/app/api/sync/anthropic-api-costs/route.ts Adds maxDuration export for cron route.
tests/unit/sync/with-sync-lock.test.ts Adds unit coverage for lock release and bookkeeping failure behavior.
tests/unit/db/pool-error-handling.test.ts Adds regression tests asserting pool/client error listeners and non-starving pool config.
specs/044-db-connection-reliability/implementation-plan.html Adds detailed incident/plan documentation for the reliability work.
specs/044-db-connection-reliability/implementation-notes.html Adds implementation notes/verification documentation for the incident fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib/sync/framework.ts
Comment on lines +143 to +149
.where(
and(
eq(syncEvents.sourceType, sourceType),
eq(syncEvents.outcome, "in_progress"),
lt(syncEvents.startedAt, cutoff)
)
);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8caef15 — valid catch, thank you.

In practice the maxDuration = 300 added in this same PR bounds every entrypoint (the three cron routes, plus the /settings/sync segment that dispatches the manual trigger and backfill server actions), so no run can currently reach 60 minutes. But you are right that this left the sweep resting on an implicit invariant — raise that ceiling later and it would silently begin reaping live backfills.

Made it explicit rather than just widening the number:

  • SYNC_MAX_DURATION_SECONDS now mirrors the route ceiling in the same module, so the margin is expressed against the thing that actually bounds a run.
  • STALE_EVENT_AFTER_MS is per-operation — 1h regular (12x the ceiling), 6h backfill (72x) — and the sweep predicate is now an OR over operationType, so a backfill is never reaped on the schedule that suits an hourly run.
  • Two invariant tests guard it: every cutoff must exceed 10x the ceiling, and backfill must exceed regular. Both fail if someone raises maxDuration without revisiting the cutoffs.

Worth noting the related case this does not claim to solve: a backfill that outlives its cutoff is still possible in principle, and the deferred TTL row lease (see specs/044-db-connection-reliability/) is the real fix, since a lease with a heartbeat can distinguish "still running" from "abandoned" instead of inferring it from age.

Addresses the Copilot review on PR #126: reapAbandonedEvents used a single
60-minute cutoff, so a backfill that legitimately ran longer could be marked
failed by the next cron attempt while still executing.

The maxDuration = 300 added in this PR already bounds every entrypoint (the
three cron routes and the /settings/sync segment that dispatches the manual
trigger and backfill server actions), so nothing can currently reach 60
minutes. But that left the sweep resting on an implicit invariant: raising the
ceiling later would silently start reaping live backfills.

- SYNC_MAX_DURATION_SECONDS mirrors the route ceiling in the same module, so
  the safety margin is expressed against what actually bounds a run
- STALE_EVENT_AFTER_MS is now per-operation (1h regular / 6h backfill) and the
  sweep predicate is an OR over operationType
- two invariant tests: every cutoff must exceed 10x the ceiling, and backfill
  must exceed regular -- these fail if the ceiling is raised without revisiting
  the cutoffs

674 unit tests pass (was 672), typecheck and scoped eslint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@studert
studert merged commit 7e94118 into main Aug 7, 2026
7 checks passed
studert added a commit that referenced this pull request Aug 7, 2026
Main advanced with #125 (spec 042, approvable tier changes on active licence
assignments) and #126 (spec 044, pool reliability) while 043 was in review. The two
specs had independently built three overlapping abstractions. Resolved as follows.

1. TIER-CHANGE SEMANTICS — 042 wins, 043 adapts.
   updateAssignmentCore no longer has its own tier branch; it calls buildTierChange
   from @/lib/assignments/tier-change, the same function the UI action and
   approveRequest use. 043's premise is one implementation per mutation shared by UI
   and MCP, so keeping a second copy would have broken the thing the refactor exists
   for. MCP therefore inherits 042's sync-managed refusal for free: without this, an
   agent could retier a GitHub Copilot seat and have the 06:00 cron silently revert
   it — the exact failure mode set_tier_price already guards against.

   042's ordering is preserved verbatim: sync authority is consulted ONLY when the
   tier actually differs, because the detail form always submits tierId and checking
   unconditionally would reject every workspace/API-key edit on a synced seat.

2. SYNC AUTHORITY — 042 wins, 043's duplicate deleted.
   isSyncOwnedTool and its hardcoded name set are gone; everything now goes through
   isSyncManagedTool, which additionally verifies the Copilot sync is actually active
   rather than assuming it from the tool name. 043's caps.syncOwnedFields survives as
   the UI-vs-MCP distinction on top of it, so UI behaviour is unchanged.

   revokeLicenseCore gained the same refusal (caps-gated): revoking a sync-managed
   seat is undone by the next sync with no audit row, so an agent would report a
   released cost that returns at 06:00.

3. CACHE INVALIDATION — composed, not chosen.
   New src/lib/assignments/cost-paths.ts holds the single LIST of cost surfaces.
   There are two TRANSPORTS that replay it: 042's revalidate.ts (direct
   revalidatePath, for actions that do not go through a write core) and 043's
   CoreResult.revalidate (for those that do). A given write uses exactly one. The
   list module imports nothing from next/cache, which keeps it out of the core module
   graph that the MCP route and the db-mocked unit tests load.

Also migrated every history call site 042/044 added to @/lib/history's options-object
signature with an explicit source, and repointed the tests that mocked
@/actions/history for the write helpers.

Migrations untouched — 0030 and 0031 are already applied to production.

Verified: pnpm typecheck, pnpm lint, and 751 unit tests across 56 files all pass
(043's 703 plus main's new suites). NOT verified: the integration suite, Playwright
and a live MCP session — the Neon dev branch credential stopped authenticating
partway through this work (password authentication failed for neondb_owner on both
the pooled and unpooled URLs). Those must be re-run before this merges.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants