feat(server): make the database pool ceiling configurable

[bjw123]

Summary

Both persistence backends hardcoded their connection pool ceiling — 10 for Postgres, 5 for on-disk SQLite. One pool is shared by every database-backed RPC, so that number is also the gateway's ceiling on concurrent database work, and it could not be changed without rebuilding the gateway.

The ceiling is now configurable, on the same config surface as the rest of the gateway's settings, defaulting to each backend's previous value so existing deployments do not move.

We run a forked build of this change on a single-replica Postgres gateway with 2,000 sandboxes, as we had some issues with HA and some open issues around it. The stock ceiling of 10 was a bottleneck at that scale; 50 resolved it.

Related Issue

Refs #2561. Replaces #2700, which was auto-closed by the vouch gate before I was vouched (thanks @elezar) and which GitHub will not let me reopen after the rebase.

Changes

• --db-max-connections, OPENSHELL_DB_MAX_CONNECTIONS, and the TOML key [openshell.gateway] database_max_connections, resolved in the standard flag > env > file > default order via the existing clap/config_file path.
• Threaded through Config → Store::connect_with_pool_size → each backend. Store::connect(url) keeps the old signature and the backend defaults, so the ~20 test call sites are untouched.
• Postgres and on-disk SQLite honour the value; the effective ceiling is logged at startup. An in-memory SQLite database stays pinned to one connection (the database lives inside that connection) and logs that a configured value was ignored.
• SQLite's min_connections is now min(configured, 5) instead of tracking max_connections, so a raised ceiling grows on demand rather than pinning that many file handles open for the process lifetime. Unset behaviour is byte-for-byte the same as before.
• Companion surfaces: Helm server.dbMaxConnections (rendered into gateway.toml, chart README regenerated), docs/reference/gateway-config.mdx (new Database Pool section), docs/kubernetes/setup.mdx values table, and the gateway man page.

The new docs section states the health-check coupling as it works today: /readyz is answered from the background readiness monitor's cached result, not from a per-request acquire, but that monitor pings through the same pool — so sustained saturation can push its ping past DEFAULT_CHECK_TIMEOUT and flip readiness while the gateway is otherwise serving.

Addressing the review feedback on #2700

@letv1nnn raised two things, both fixed here:

1. Config surface. The previous patch read std::env::var directly in persistence/postgres.rs. It is now a clap arg with an env binding plus a TOML field, matching OPENSHELL_DB_URL (cli.rs, config_file.rs), so the two database knobs live on the same surface and the key shows up in the generated config docs.
2. Companion updates. Gateway config docs and the Helm chart env/TOML surface are both included, as the triage on feat: make gateway Postgres connection pool size configurable (hardcoded to 10) #2561 flagged. The chart renders the TOML key rather than an extra container env: entry, which is how every other non-secret gateway setting is passed.

One deliberate reversal from #2700: invalid values now fail fast instead of silently falling back to the default. A zero pool would block every acquire, and quietly ignoring a typo would reproduce the exact ceiling the operator is trying to lift — the failure mode this change exists to remove. Every other numeric OPENSHELL_* knob already rejects bad input at clap parse time, and the TOML loader rejects 0 the same way it rejects an empty credential_drivers.

Testing

• cargo test -p openshell-server -p openshell-core — all green. New tests: TOML value applied when the flag is unset, env wins over TOML, clap rejects 0/negative/non-numeric, loader rejects database_max_connections = 0, SQLite honours an override and defaults to 5, in-memory SQLite pins to 1.
• helm unittest deploy/helm/openshell — 3 new cases (key omitted by default, rendered when set, negative value fails the template).
• mise run pre-commit passes, including clippy -D warnings and helm:docs:check.

Verified on a kind cluster

with latest helm chart and gateway image built from this branch. Identical load each run — 200 concurrent SDK clients looping ListSandboxes for 30s — sampling pg_stat_activity every 400 ms:

server.dbMaxConnections	Startup log	Peak client backends	Requests	Errors
50	max_connections=50	51 (50 pool + the psql sampler)	234,245	0
0 (omitted → default)	max_connections=10	11 (10 pool + the psql sampler)	212,882	0

The pool reaches the configured ceiling and never exceeds it, and the default path renders no TOML key and keeps the previous value of 10. Both gateways reported Ready, which exercises a real database ping through the configured pool. This environment is not latency-bound on the database, so it demonstrates the ceiling rather than a throughput win.

Confirmed for both postgres and sqlLite

Checklist

• Conventional commit, signed off (DCO)
• Default behaviour unchanged for both backends
• Unit tests and Helm chart tests added
• Docs, man page, and Helm values updated
• mise run pre-commit passes

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

[bjw123]

I have read the DCO document and I hereby sign the DCO.

[letv1nnn]

PR Review: feat(server): make the database pool ceiling configurable

Reviewed the full diff and verified against the code. Checked out the branch and ran tests locally — all green.

What it does

Makes the shared DB pool ceiling configurable. Flag --db-max-connections, env OPENSHELL_DB_MAX_CONNECTIONS, TOML [openshell.gateway] database_max_connections. Defaults unchanged (Postgres 10, on-disk SQLite 5, in-memory 1). Companion Helm + docs + man page.

Verified good

• Backward compat solid. Store::connect(url) is kept, delegating to connect_with_pool_size(url, None). All ~25 callers except lib.rs:557 use the old signature unchanged; only persistence/mod.rs internal calls updated. No broken callers.
• Precedence correct and consistent. The merge_file_into_args db block copies the grpc_rate_limit_requests pattern exactly: file.is_some() && args.is_none() && arg_defaulted(...). flag > env > file > default holds; env-sourced values short-circuit the file merge.
• Zero rejected at all three layers: clap range(1..), TOML loader, and Helm fail on negative. Helm never renders 0 (gt $dbMaxConnections 0), so the loader never sees 0 from the chart.
• Helm negative test is not a false pass. It targets statefulset.yaml while the guard lives in gateway-config.yaml — this works because _gateway-workload.tpl pulls gateway-config into the checksum annotation via include, so rendering the statefulset evaluates the fail.
• min_connections change safe. New min(configured, 5). Default path max=5 → min=5, identical to the old min=max=5. In-memory max=1 → min=1. The "byte-for-byte same" claim holds.
• OCSF not required here. The in-memory tracing::warn! is fine — OCSF guidance scopes to openshell-sandbox; this is openshell-server.

Minor notes (non-blocking)

• gateway-config.mdx shows database_max_connections = 10 uncommented in the example — reads as a live setting, but it pins the exact default so it is harmless, and the section below clarifies "omit to use the default." Consistent with how log_level etc. are shown.
• No e2e/CI coverage for the pool-ceiling path; it is load-verified manually on kind (both backends). Unit tests cover parse/merge/reject only.
• Setting on-disk SQLite <5 now yields min=max=<5 (previously a fixed 5) — newly reachable, not a regression.

Testing (ran locally on the branch)

• cargo test -p openshell-server -p openshell-core — all pass, 0 failures (unit + integration: multiplex, mtls, relay, ws-tunnel, health; doctests).
• 7 PR-specific tests green: env-overrides-file, file-populates-when-cli-omits, clap rejects 0/negative/non-numeric, TOML parse + reject-zero, sqlite default+override, in-memory pins to 1.

Verdict

Clean, well-tested, and honestly documented (the readyz coupling and the Postgres ceiling × replicas caveat are both surfaced). Precedence, backward compat, and validation all check out. No correctness blockers. Docs/man/Helm/README are updated with no companion drift.

[letv1nnn]

I have read the DCO document and I hereby sign the DCO.

probably contains trailing \n or \r, so ci fails

[bjw123]

I have read the DCO document and I hereby sign the DCO.

[letv1nnn]

The DCO check matches the commit author's GitHub account, and these commits are authored by bwilkinson881 while the sign comment came from bjw123. Two options: (1) sign again while logged in as bwilkinson881, or (2) rewrite the commit author email to one linked to bjw123 and force-push. Either makes the check pass.

[bjw123]

I have read the DCO document and I hereby sign the DCO.