fix(db): bound knowledge connector and document fan-out concurrency#5432
fix(db): bound knowledge connector and document fan-out concurrency#5432TheodoreSpeaks wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Lmib23o5aQtBr7ZPhgpgf
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@greptile review |
PR SummaryMedium Risk Overview The connector sync cron now selects due connectors ordered by oldest When Trigger.dev is off, in-process document dispatch ( KB delete storage cleanup ( Reviewed by Cursor Bugbot for commit f94c7ba. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR bounds three previously unbounded fan-out sites that were identified as potential PgBouncer pool exhaustion risks. The changes add a query limit and ordering to the connector sync scheduler, and replace
Confidence Score: 4/5Safe to merge. All three fan-out sites are correctly bounded and per-item error isolation is maintained. The one notable change to review is the cron route shifting from fire-and-forget to a fully awaited dispatch loop. The core refactoring is correct and well-scoped. The only rough edge is that the cron route's response count still reports connectors-found rather than connectors-dispatched-successfully — a small accuracy gap that is now fixable since the dispatches are awaited. apps/sim/app/api/knowledge/connectors/sync/route.ts — the response count field could be made accurate with a small follow-up. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Cron as Cron Scheduler
participant Route as /api/knowledge/connectors/sync
participant DB as Postgres (PgBouncer)
participant Engine as dispatchSync (×N)
Cron->>Route: GET (every 5 min)
Route->>DB: UPDATE stale 'syncing' → 'error'
Route->>DB: SELECT due connectors ORDER BY nextSyncAt ASC LIMIT 200
DB-->>Route: dueConnectors[]
loop "mapWithConcurrency (limit=10)"
Route->>Engine: dispatchSync(connector.id)
Engine->>DB: SELECT + conditional UPDATE
DB-->>Engine: result
Engine-->>Route: ok / catch(err)→log
end
Route-->>Cron: "{ success, count }"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Cron as Cron Scheduler
participant Route as /api/knowledge/connectors/sync
participant DB as Postgres (PgBouncer)
participant Engine as dispatchSync (×N)
Cron->>Route: GET (every 5 min)
Route->>DB: UPDATE stale 'syncing' → 'error'
Route->>DB: SELECT due connectors ORDER BY nextSyncAt ASC LIMIT 200
DB-->>Route: dueConnectors[]
loop "mapWithConcurrency (limit=10)"
Route->>Engine: dispatchSync(connector.id)
Engine->>DB: SELECT + conditional UPDATE
DB-->>Engine: result
Engine-->>Route: ok / catch(err)→log
end
Route-->>Cron: "{ success, count }"
|
Greptile SummaryThis PR bounds three previously-unbounded DB/storage fan-out sites to prevent PgBouncer connection pool saturation under load. All three call sites now use the existing
Confidence Score: 5/5Safe to merge. All three fan-out sites are correctly bounded, error isolation is preserved, and ownership/tenancy guards in storage deletion are structurally unchanged. The changes are targeted and narrow: each of the three patched sites gets the same treatment (replace unbounded concurrent launch with mapWithConcurrency) and all three mappers correctly satisfy the fn-must-not-reject contract via .catch() or try/catch. The query cap in the scheduler correctly uses ORDER BY asc(nextSyncAt) to prefer the most-overdue connectors and avoid starvation. The behavioral shift from fire-and-forget to awaited dispatch in the cron route is an intentional improvement with no downside for this endpoint. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Cron as Cron Trigger (5 min)
participant Route as /api/knowledge/connectors/sync
participant DB as Postgres / PgBouncer
participant Engine as dispatchSync (sync-engine)
Cron->>Route: GET (cron auth)
Route->>DB: UPDATE stale syncing → error (recover locks)
Route->>DB: SELECT id ORDER BY nextSyncAt ASC LIMIT 200
DB-->>Route: dueConnectors[0..N≤200]
loop mapWithConcurrency (≤10 at once)
Route->>Engine: dispatchSync(connector.id)
Engine->>DB: SELECT + conditional UPDATE (mark syncing)
DB-->>Engine: ok
Engine-->>Route: resolved (or .catch swallows error)
end
Route-->>Cron: "200 OK { count: N }"
Note over Route,Engine: Previously: fire-and-forget for all N at once<br/>Now: await, bounded at 10 concurrent
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Cron as Cron Trigger (5 min)
participant Route as /api/knowledge/connectors/sync
participant DB as Postgres / PgBouncer
participant Engine as dispatchSync (sync-engine)
Cron->>Route: GET (cron auth)
Route->>DB: UPDATE stale syncing → error (recover locks)
Route->>DB: SELECT id ORDER BY nextSyncAt ASC LIMIT 200
DB-->>Route: dueConnectors[0..N≤200]
loop mapWithConcurrency (≤10 at once)
Route->>Engine: dispatchSync(connector.id)
Engine->>DB: SELECT + conditional UPDATE (mark syncing)
DB-->>Engine: ok
Engine-->>Route: resolved (or .catch swallows error)
end
Route-->>Cron: "200 OK { count: N }"
Note over Route,Engine: Previously: fire-and-forget for all N at once<br/>Now: await, bounded at 10 concurrent
Reviews (2): Last reviewed commit: "fix(db): bound knowledge connector and d..." | Re-trigger Greptile |
Summary
Promise.all/fire-and-forget fan-out over DB work can saturate a pool, and queued clients are killed after PgBouncer's 120squery_wait_timeout. This bounds three such fan-out sites found in the DB-availability audit.app/api/knowledge/connectors/sync/route.ts): the due-connectors SELECT now carries.orderBy(asc(nextSyncAt)).limit(200)(most-overdue first, so connectors beyond the cap are picked up next tick rather than starved — the ordering rides the existingkc_status_next_sync_idxindex), and dispatches run throughmapWithConcurrencyat 10 concurrent instead of firing every connector at once. Per-connector dispatch failures are still caught and logged individually.lib/knowledge/documents/service.tsdispatchInProcess): up to 1000processDocumentAsyncjobs (chunking + embedding + many DB inserts each) ran under an unboundedPromise.allSettledwhen trigger.dev is unavailable (self-hosted). Now bounded at 5 concurrent with the same per-item error isolation and dispatched-count semantics.deleteDocumentStorageFiles): the per-document storage-object + metadata-row delete fan-out was unbounded in N; now bounded at 10 concurrent. The mapper already swallows per-item errors, so per-item isolation is unchanged.Type of Change
Testing
bun run type-checkinapps/sim— cleanbunx vitest run lib/knowledge— 7 files, 97 tests passedbun run lint:checkfrom repo root — cleanbun run check:api-validation:strict— passedChecklist
🤖 Generated with Claude Code
https://claude.ai/code/session_011Lmib23o5aQtBr7ZPhgpgf