perf(api): raise DB pool sizes and command timeouts for migrations, Hangfire, and long transactions - #351
Conversation
…angfire, and long transactions SessionMaxPoolSize=2 starved the session pooler shared by startup migrations and the always-on Hangfire durable queue (2 workers + storage threads + request-path enqueue). Raise it to 5 (session mode is 1:1 with held Supabase backends, so it stays the tight cap) and raise EfMaxPoolSize 8->15 for request concurrency (the transaction pooler multiplexes, decoupling this cap from held backends). Both stay well under the 57 usable direct backends (max_connections=60, 3 reserved), even across a rolling deploy's brief two-instance overlap. CommandTimeout=30s was tight for long transactions; make it configurable and raise the request-path default to 60s. Startup migrations previously ran at Npgsql's implicit 30s default, too tight to build an index or backfill in one statement, so add a dedicated 180s MigrationCommandTimeoutSeconds. Values documented on the settings class and bound via DatabaseConnectionSettings.From. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #351 — perf(api): raise DB pool sizes and command timeouts
Scope: PR #351 in thomasluizon/orbit-api
Recommendation: APPROVE
Summary
This is a self-contained, backend-only configuration change: it raises the EF/session Npgsql pool caps (8→15, 2→5) and adds configurable command timeouts (60s request-path, 180s migration), replacing a hardcoded 30s timeout on both paths. No controllers, DTOs, endpoints, or UI surface are touched. The change is backed by a documented sizing model on DatabaseConnectionSettings and new unit tests asserting the safety invariants (session pool < request pool, 2-instance overlap < usable Supabase backends, migration timeout > request timeout). No Critical or High findings survive review.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info] Both
WebApplicationExtensions.ConfigureOrbitPipelineandOrbitConnectionStringFactory.ForSessionindependently callDatabaseConnectionSettings.From(app.Configuration)for the same migration path, re-binding theDatabaseconfig section twice per startup. Negligible cost (in-memory config read, runs once at boot, not per-request) — not worth a finding on its own. - [Info] Pool-size increase combined with the raised command timeout widens the blast radius of a future slow-query bug (a stuck query can now hold a connection up to 2x as long, and there are more connections to hold). This is an operational tradeoff already reasoned about in the code's own doc-comment (
DatabaseConnectionSettings.cs) and verified against Supabase'smax_connections=60— not a defect.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no findings. Config is server-side only, read once at startup from IConfiguration, never attacker-reachable; no secrets introduced; connection strings still env-injected as before. |
| contract-aligner | N/A — diff touches no DTO, Controller route, or packages/shared type. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A in CI — runs as a separate required check |
| Tests (dotnet) | N/A in CI — runs as a separate required check |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop) — N/A, no
apps/*UI files in diff. - Dimension 9 (Parity) — N/A, no
apps/web/apps/mobilefiles in diff. - Dimension 10 (i18n) — N/A, no user-facing strings added.
- Dimension 11 (Contract drift) — N/A, no DTO/endpoint/shared-type changes.
- Dimension 14 (FEATURES.md parity) — N/A, pure perf/infra change with no user-facing feature surface change.
- Backend hard rules (#13) — timezone/authz/logging/transaction-teardown — N/A, diff adds no dates, no controllers, no new logging, no transactions.
- All 7 changed files (
ServiceCollectionExtensions.cs,WebApplicationExtensions.cs,appsettings.json,DatabaseConnectionSettings.cs,OrbitConnectionStringFactory.cs,DatabaseConnectionSettingsTests.cs,OrbitConnectionStringFactoryTests.cs) were read in full diff context and given a verdict — nothing deferred.
What's good
- The sizing rationale is fully documented in an XML doc-comment on
DatabaseConnectionSettings(transaction-pooler vs session-pooler multiplexing behavior, the 57-usable-backend ceiling, the rolling-deploy 2-instance overlap math) — exactly the kind of WHY-with-context that survives the no-narration-comments rule. - New tests directly encode the safety invariants as assertions (session < request pool, 2× session overlap < 57, migration timeout > request timeout) rather than just snapshotting the new numbers — they'll catch a future regression that violates the reasoning, not just a value change.
- The
ReadSettings/Fromduplication betweenDatabaseConnectionSettingsandOrbitConnectionStringFactorywas cleaned up in the same diff (singleFrom()on the settings class, old private duplicate removed) — no dead code left behind. - Hardcoded
30timeouts on both the request-path and migrationDbContextOptionsBuilderare now driven from the same config the pool caps already come from, closing the gap where migrations previously silently ran at Npgsql's implicit 30s default.
Recommendation
Approve as-is. No action required before merge.



Problem
Database connection/timeout config was sized too tight for the real workload:
SessionMaxPoolSize=2starved the Supavisor session pooler, which is shared by startup migrations and the always-on Hangfire durable queue (WorkerCount=2+ storage expiration/heartbeat threads + request-path enqueue). Session mode holds one Postgres backend per client connection (1:1), so 2 workers alone could saturate the pool and block migrations/enqueue.8 + 2), leaving the request path little concurrency headroom.CommandTimeout=30s(request path) is tight for long transactions, and startup migrations ran at Npgsql's implicit 30s default — too short to build an index or backfill a table in a single statement (a real deploy-failure risk).Change
EfMaxPoolSize8 → 15 — request path runs through the Supavisor transaction pooler, which multiplexes (a client connection only holds a backend for the duration of a transaction), so this cap is a request-concurrency limit decoupled from held backends and can safely exceed the pooler's server-side pool size.SessionMaxPoolSize2 → 5 — session mode is 1:1 with held backends, so this stays the tight cap; sized for Hangfire's 2 workers + storage threads + enqueue + migrations.CommandTimeoutSeconds60 (new, configurable) — request-path default raised from the hardcoded 30s.MigrationCommandTimeoutSeconds180 (new) — applied to the startup-migration DbContext (previously unset → implicit 30s).Safety (verified against production)
Production Supabase compute reports
max_connections = 60(3 reserved → ~57 usable direct backends). The long AI/batch work is OpenAI network I/O, already bounded byAI:BatchNetworkTimeoutSeconds=120— not a single long DB command — so no per-query DB timeout override is warranted. Worst case (a rolling deploy's brief two-instance overlap):2 × 5 = 10held session backends + Supabase's own internal connections stays comfortably under 57. The full sizing rationale is documented onDatabaseConnectionSettings.Tests
DatabaseConnectionSettingsTests— defaults,From(IConfiguration)binding of every field, and the safety invariants (session pool < request pool; 2-instance session overlap < usable backends; migration timeout > request timeout).OrbitConnectionStringFactoryTestsdefault-cap assertions.dotnet build+ fullOrbit.Infrastructure.Tests(1699) green.Refs thomasluizon/orbit-ui-mobile#243