Skip to content

feat(server): make the Postgres pool ceiling configurable - #2700

Closed
bjw123 wants to merge 1 commit into
NVIDIA:mainfrom
bjw123:bwilkinson/pg-pool-on-0.0.85
Closed

feat(server): make the Postgres pool ceiling configurable#2700
bjw123 wants to merge 1 commit into
NVIDIA:mainfrom
bjw123:bwilkinson/pg-pool-on-0.0.85

Conversation

@bjw123

@bjw123 bjw123 commented Aug 11, 2026

Copy link
Copy Markdown

Summary

PostgresStore::connect hardcodes max_connections(10). This makes the pool the throughput ceiling under concurrent sandbox creation, and it cannot be tuned without rebuilding the gateway.

OPENSHELL_DB_MAX_CONNECTIONS now sets it, defaulting to 10 so existing deployments are unchanged.

Related Issue

Refs #2561.

Changes

  • max_connections() reads OPENSHELL_DB_MAX_CONNECTIONS, falling back to the previous hardcoded 10.
  • Unset, unparseable, negative and zero all fall back to the default: a typo should degrade to today's behaviour rather than crash-loop the gateway, and zero would deadlock every acquire.
  • The chosen ceiling is logged at startup, so the effective value is visible rather than inferred.

Matches the existing convention in this crate — std::env::var("OPENSHELL_*") read directly, as compute/driver_config.rs and compute/lease.rs already do. Threading a parameter through Store::connect was avoided because it has ~10 call sites, nearly all tests.

Testing

cargo test -p openshell-server --lib max_connections_tests — 3 new tests: default when unset, override honoured (including surrounding whitespace), and each unusable input falling back rather than panicking. mise run pre-commit passes.

Field evidence

Measured on a 1,000-sandbox load test against a gateway on SQLite (pool 5), where the pool was unambiguously the constraint:

WARN sqlx::pool::acquire: acquired connection, but time to acquire exceeded
     slow threshold  aquired_after_secs=9.30  slow_acquire_threshold_secs=2.0

42 of 1,000 forwarders reached ready after 40 minutes, and the retries became load in their own right — 1,552 sandbox pods for 1,000 requested.

For balance: the same 1,000-sandbox test against Postgres at the stock ceiling of 10 produced exactly one slow-acquire (at startup), zero pool timeouts and 527m gateway CPU. So on Postgres the current default holds well at that scale — this change is about removing a fixed ceiling that cannot be tuned as fleets grow, not about a default that is wrong today.

Checklist

  • Conventional commit, signed off (DCO)
  • Default behaviour unchanged
  • Unit tests added
  • mise run pre-commit passes

The Postgres store hardcoded max_connections(10), which makes the pool
the throughput limit under concurrent sandbox creation. Once all ten are
checked out, callers queue on acquire and sqlx starts logging "time to
acquire exceeded slow threshold"; sandbox creates then time out and get
retried, which adds load rather than shedding it.

The right ceiling is deployment-specific — it depends on how many gateway
replicas share the server and what max_connections the server itself
allows — so it cannot be a single number baked into the binary.

OPENSHELL_DB_MAX_CONNECTIONS now sets it, defaulting to 10 so existing
deployments are unchanged. Unset, unparseable and zero all fall back to
the default: a typo should degrade to today's behaviour rather than
crash-loop the gateway, and zero would deadlock every acquire.

Refs: NVIDIA#2561
Signed-off-by: Bryce Wilkinson <bwilkinson@alpha-sense.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Thank you for your interest in contributing to OpenShell, @bjw123.

This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer.

To get vouched:

  1. Open a Vouch Request discussion.
  2. Describe what you want to change and why.
  3. Write in your own words — do not have an AI generate the request.
  4. A maintainer will comment /vouch if approved.
  5. Once vouched, open a new PR (preferred) or reopen this one after a few minutes.

See CONTRIBUTING.md for details.

@github-actions

Copy link
Copy Markdown

Thank you for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text:


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


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the DCO Assistant Lite bot.

@letv1nnn

Copy link
Copy Markdown
Contributor

Thanks for this @bjw123 — clean change and the field evidence is genuinely useful (the SQLite pool-5 vs Postgres-10 comparison makes the "removing a fixed ceiling, not fixing a wrong default" framing convincing).

Heads up: I'm a contributor here, not a maintainer, so this isn't a merge review — the PR is auto-closed until you're vouched and #2561 is still state:validated (not yet accepted). Leaving this as reference feedback for whoever picks it up.

What looks good

  • Defensive fallback (unset / unparseable / zero / negative → default) is the right instinct — zero would deadlock every acquire, and a typo shouldn't crash-loop the gateway. Nicely covered by the tests.
  • Logging the effective ceiling at startup.
  • Test env-guard pattern matches the existing TEST_ENV_LOCK convention in the crate.

Two things a maintainer will likely raise

  1. Config surface. The closest sibling setting, OPENSHELL_DB_URL, is wired as a clap arg + TOML field with an env: binding (config_file.rs, cli.rs), whereas this reads std::env::var directly. Both are valid patterns in the repo, but the two DB knobs would then live on different config surfaces. Worth considering threading it through the same config-file path so it shows up in generated docs.
  2. Companion updates. A new user-facing env var usually wants a matching entry in docs/reference/gateway-config.mdx and the Helm chart env: surface (deploy/) — the triage on feat: make gateway Postgres connection pool size configurable (hardcoded to 10) #2561 flagged both. Adding those would make it land-ready.

Neither is a correctness issue — the code itself is solid. Thanks again for contributing the patch and the load-test numbers.

@bjw123

bjw123 commented Aug 20, 2026

Copy link
Copy Markdown
Author

Superseded by #2828, now that I'm vouched (thanks @elezar). GitHub refused to reopen this one after the rebase, so the work moved to a fresh PR against current main.

@letv1nnn — both of your points are addressed there: the knob is now a clap arg + env binding + TOML key on the same config surface as OPENSHELL_DB_URL rather than a bare std::env::var read in the persistence layer, and the gateway config docs, Helm chart, and man page are updated alongside it. I also extended it to the SQLite pool, since that backend hardcoded a ceiling of 5 in the same way — and it was the one my load-test evidence actually came from.

One thing I changed my mind on from this PR, flagged in case you disagree: invalid values now fail fast instead of falling back to the default. Silently ignoring a typo would leave the operator on the very ceiling they were trying to lift, which is the failure mode the change exists to remove. Thanks again for the review — it's what the new PR is built on.

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.

3 participants