Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions crates/openshell-core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,16 @@ pub struct Config {
/// Database URL for persistence.
pub database_url: String,

/// Connection ceiling for the persistence pool.
///
/// `None` leaves the backend's built-in default in place. The right
/// ceiling depends on the deployment — how many gateway replicas share
/// the database, and what `max_connections` a Postgres server itself
/// allows — so it cannot be one number baked into the binary. An
/// in-memory `SQLite` database ignores it: the database lives in its single
/// connection.
pub database_max_connections: Option<u32>,

/// Compute drivers configured for the gateway.
///
/// The config shape allows multiple drivers so the gateway can evolve
Expand Down Expand Up @@ -835,6 +845,7 @@ impl Config {
mtls_auth: MtlsAuthConfig::default(),
gateway_jwt: None,
database_url: String::new(),
database_max_connections: None,
compute_drivers: vec![],
compute_driver_endpoints: BTreeMap::new(),
credential_drivers: Vec::new(),
Expand Down Expand Up @@ -879,6 +890,15 @@ impl Config {
self
}

/// Create a new configuration with a database pool connection ceiling.
///
/// `None` keeps the persistence backend's default.
#[must_use]
pub const fn with_database_max_connections(mut self, max_connections: Option<u32>) -> Self {
self.database_max_connections = max_connections;
self
}

/// Create a new configuration with the configured compute drivers.
#[must_use]
pub fn with_compute_drivers<I, D>(mut self, drivers: I) -> Self
Expand Down
90 changes: 90 additions & 0 deletions crates/openshell-server/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,20 @@ struct RunArgs {
#[arg(long, env = "OPENSHELL_DB_URL")]
db_url: Option<String>,

/// Connection ceiling for the database pool, shared by every
/// database-backed request.
///
/// When unset, the gateway uses its backend default: 10 with Postgres,
/// 5 with an on-disk `SQLite` database. An in-memory `SQLite` database is
/// always a single connection. Each replica opens its own pool, so keep
/// the total below what the database server itself admits.
#[arg(
long,
env = "OPENSHELL_DB_MAX_CONNECTIONS",
value_parser = clap::value_parser!(u32).range(1..)
)]
db_max_connections: Option<u32>,

/// Compute drivers configured for this gateway.
///
/// Accepts a comma-delimited list such as `kubernetes` or
Expand Down Expand Up @@ -383,6 +397,7 @@ fn prepare_server_config(args: &mut RunArgs, matches: &ArgMatches) -> Result<Ser

config = config
.with_database_url(db_url)
.with_database_max_connections(args.db_max_connections)
.with_compute_drivers(args.drivers.clone())
.with_grpc_rate_limit(
args.grpc_rate_limit_requests,
Expand Down Expand Up @@ -723,6 +738,12 @@ fn merge_file_into_args(args: &mut RunArgs, file: &GatewayFileSection, matches:
{
args.grpc_rate_limit_window_seconds = Some(window);
}
if let Some(max_connections) = file.database_max_connections
&& args.db_max_connections.is_none()
&& arg_defaulted(matches, "db_max_connections")
{
args.db_max_connections = Some(max_connections);
}
}

fn validate_grpc_rate_limit_args(requests: Option<u64>, window_seconds: Option<u64>) -> Result<()> {
Expand Down Expand Up @@ -1489,6 +1510,75 @@ grpc_rate_limit_window_seconds = 30
assert_eq!(args.grpc_rate_limit_window_seconds, Some(30));
}

#[test]
fn file_db_max_connections_populates_args_when_cli_omits() {
let _lock = ENV_LOCK
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let _g = EnvVarGuard::remove("OPENSHELL_DB_MAX_CONNECTIONS");

let (mut args, matches) =
parse_with_args(&["openshell-gateway", "--db-url", "sqlite::memory:"]);
assert_eq!(
args.db_max_connections, None,
"default leaves the pool alone"
);
let file = config_file_from_toml(
r"
[openshell.gateway]
database_max_connections = 64
",
);
merge_file_into_args(&mut args, &file.openshell.gateway, &matches);

assert_eq!(args.db_max_connections, Some(64));
}

#[test]
fn env_db_max_connections_overrides_file_value() {
let _lock = ENV_LOCK
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let _g = EnvVarGuard::set("OPENSHELL_DB_MAX_CONNECTIONS", "128");

let (mut args, matches) =
parse_with_args(&["openshell-gateway", "--db-url", "sqlite::memory:"]);
let file = config_file_from_toml(
r"
[openshell.gateway]
database_max_connections = 64
",
);
merge_file_into_args(&mut args, &file.openshell.gateway, &matches);

assert_eq!(args.db_max_connections, Some(128));
}

#[test]
fn db_max_connections_rejects_a_zero_or_unparseable_ceiling() {
let _lock = ENV_LOCK
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
let _g = EnvVarGuard::remove("OPENSHELL_DB_MAX_CONNECTIONS");

// Zero would deadlock every `acquire`, and a silent fallback to the
// default would reproduce the ceiling the operator is trying to lift.
for bad in ["0", "-5", "many"] {
assert!(
command()
.try_get_matches_from([
"openshell-gateway",
"--db-url",
"sqlite::memory:",
"--db-max-connections",
bad,
])
.is_err(),
"input {bad:?} must be rejected"
);
}
}

#[test]
fn aux_listener_preserves_file_ip_against_public_bind() {
use std::net::SocketAddr;
Expand Down
47 changes: 47 additions & 0 deletions crates/openshell-server/src/config_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ pub struct GatewayFileSection {
#[serde(default)]
pub log_level: Option<String>,

// ── Database ─────────────────────────────────────────────────────────
/// Connection ceiling for the persistence pool. One shared pool serves
/// every database-backed RPC, so this is also the gateway's ceiling on
/// concurrent database work. On Postgres, total server connections are
/// `database_max_connections × replica_count`, which must stay under the
/// server's own `max_connections`. Omit to keep the backend default (10
/// for Postgres, 5 for on-disk `SQLite`); an in-memory `SQLite` database
/// is always a single connection.
#[serde(default)]
pub database_max_connections: Option<u32>,

// ── Drivers ──────────────────────────────────────────────────────────
#[serde(default)]
pub compute_drivers: Option<Vec<String>>,
Expand Down Expand Up @@ -399,6 +410,14 @@ pub fn load(path: &Path) -> Result<ConfigFile, ConfigFileError> {
cli: "--db-url",
});
}
// A zero-sized pool would deadlock every `acquire` rather than degrade,
// so reject it at load instead of starting a gateway that cannot serve.
if file.openshell.gateway.database_max_connections == Some(0) {
return Err(ConfigFileError::InvalidValue {
field: "openshell.gateway.database_max_connections",
message: "must be greater than zero; omit the key to use the built-in default",
});
}
if file
.openshell
.gateway
Expand Down Expand Up @@ -912,6 +931,34 @@ database_url = "sqlite::memory:"
));
}

#[test]
fn parses_database_max_connections() {
let toml = r"
[openshell.gateway]
database_max_connections = 64
";
let tmp = write_tmp(toml);
let file = load(tmp.path()).expect("a positive pool ceiling parses");
assert_eq!(file.openshell.gateway.database_max_connections, Some(64));
}

#[test]
fn rejects_zero_database_max_connections() {
let toml = r"
[openshell.gateway]
database_max_connections = 0
";
let tmp = write_tmp(toml);
let err = load(tmp.path()).expect_err("a zero pool ceiling must be rejected");
assert!(matches!(
err,
ConfigFileError::InvalidValue {
field: "openshell.gateway.database_max_connections",
..
}
));
}

#[test]
fn rejects_unknown_gateway_field() {
let toml = r"
Expand Down
4 changes: 3 additions & 1 deletion crates/openshell-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,9 @@ pub(crate) async fn run_server(
.map_err(|error| Error::config(format!("middleware registration failed: {error}")))?,
);

let store = Arc::new(Store::connect(database_url).await?);
let store = Arc::new(
Store::connect_with_pool_size(database_url, config.database_max_connections).await?,
);
let credentials = credentials::CredentialRuntime::from_config_file_with_store(
&config,
config_file.as_ref(),
Expand Down
20 changes: 17 additions & 3 deletions crates/openshell-server/src/persistence/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,24 @@ impl Store {
matches!(self, Self::Sqlite(_))
}

/// Connect to a persistence store based on the database URL.
/// Connect to a persistence store based on the database URL, leaving pool
/// sizing at the backend default.
pub async fn connect(url: &str) -> CoreResult<Self> {
Self::connect_with_pool_size(url, None).await
}

/// Connect to a persistence store based on the database URL.
///
/// `max_connections` overrides the backend's pool ceiling, which stays at
/// its built-in default when `None`. The two backends have different
/// defaults, and an in-memory `SQLite` database ignores the override
/// entirely — it must be held by exactly one connection.
pub async fn connect_with_pool_size(
url: &str,
max_connections: Option<u32>,
) -> CoreResult<Self> {
if url.starts_with("postgres://") || url.starts_with("postgresql://") {
let store = PostgresStore::connect(url)
let store = PostgresStore::connect(url, max_connections)
.await
.map_err(|e| CoreError::execution(e.to_string()))?;
store
Expand All @@ -213,7 +227,7 @@ impl Store {
.map_err(|e| CoreError::execution(e.to_string()))?;
Ok(Self::Postgres(store))
} else if url.starts_with("sqlite:") {
let store = SqliteStore::connect(url)
let store = SqliteStore::connect(url, max_connections)
.await
.map_err(|e| CoreError::execution(e.to_string()))?;
store
Expand Down
15 changes: 13 additions & 2 deletions crates/openshell-server/src/persistence/postgres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,21 @@ pub struct PostgresStore {
pool: PgPool,
}

/// Pool ceiling used when the operator has not configured one.
///
/// A single shared pool serves every database-backed RPC, so this value is
/// also the gateway's ceiling on concurrent database work. It is high enough
/// for a single-replica gateway against a default-sized server; fleets that
/// outgrow it raise it through `[openshell.gateway] database_max_connections`.
pub const DEFAULT_MAX_CONNECTIONS: u32 = 10;

impl PostgresStore {
pub async fn connect(url: &str) -> PersistenceResult<Self> {
/// Connect and size the pool, falling back to [`DEFAULT_MAX_CONNECTIONS`].
pub async fn connect(url: &str, max_connections: Option<u32>) -> PersistenceResult<Self> {
let max_connections = max_connections.unwrap_or(DEFAULT_MAX_CONNECTIONS);
tracing::info!(max_connections, "sizing Postgres connection pool");
let pool = PgPoolOptions::new()
.max_connections(10)
.max_connections(max_connections)
.connect(url)
.await
.map_err(|e| map_db_error(&e))?;
Expand Down
40 changes: 37 additions & 3 deletions crates/openshell-server/src/persistence/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ static SQLITE_MIGRATOR: sqlx::migrate::Migrator = sqlx::migrate!("./migrations/s

use super::{DRAFT_CHUNK_OBJECT_TYPE, POLICY_OBJECT_TYPE};

/// Pool ceiling for an on-disk database when the operator has not configured
/// one.
///
/// `SQLite` serializes writers, so raising this mainly buys read concurrency;
/// a gateway that needs more write throughput wants Postgres, not a larger
/// `SQLite` pool.
pub const DEFAULT_MAX_CONNECTIONS: u32 = 5;

#[derive(Debug, Clone)]
pub struct SqliteStore {
pool: SqlitePool,
Expand All @@ -35,17 +43,43 @@ impl SqliteStore {
self.pool.close().await;
}

pub async fn connect(url: &str) -> PersistenceResult<Self> {
/// Test support only: the pool's effective connection ceiling.
#[cfg(test)]
pub(crate) fn max_connections_for_test(&self) -> u32 {
self.pool.options().get_max_connections()
}

/// Connect and size the pool, falling back to [`DEFAULT_MAX_CONNECTIONS`]
/// for an on-disk database.
///
/// An in-memory database is always held by exactly one connection — the
/// database lives in that connection, so a second one would see a
/// different (empty) database. A configured ceiling is ignored there.
pub async fn connect(url: &str, max_connections: Option<u32>) -> PersistenceResult<Self> {
let is_in_memory = url.contains(":memory:") || url.contains("mode=memory");
let max_connections = if is_in_memory { 1 } else { 5 };
let max_connections = if is_in_memory {
if max_connections.is_some_and(|configured| configured != 1) {
tracing::warn!(
"ignoring the configured database pool ceiling: an in-memory SQLite database is held by a single connection"
);
}
1
} else {
max_connections.unwrap_or(DEFAULT_MAX_CONNECTIONS)
};
// Keep the pool eagerly warm up to the built-in size, but let a raised
// ceiling grow on demand rather than pinning that many file handles
// open for the life of the process.
let min_connections = max_connections.min(DEFAULT_MAX_CONNECTIONS);
tracing::info!(max_connections, "sizing SQLite connection pool");

let options = SqliteConnectOptions::from_str(url)
.map_err(|e| map_db_error(&e))?
.create_if_missing(true);

let mut pool_options = SqlitePoolOptions::new()
.max_connections(max_connections)
.min_connections(max_connections);
.min_connections(min_connections);

if is_in_memory {
pool_options = pool_options.idle_timeout(None).max_lifetime(None);
Expand Down
31 changes: 31 additions & 0 deletions crates/openshell-server/src/persistence/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,37 @@ async fn sqlite_connect_runs_embedded_migrations() {
assert!(records.is_empty());
}

#[tokio::test]
async fn sqlite_pool_ceiling_defaults_and_honours_an_override() {
use super::sqlite::{DEFAULT_MAX_CONNECTIONS, SqliteStore};

let tmp = tempfile::tempdir().expect("tempdir");
let db_path = tmp.path().join("openshell.db");
let url = format!("sqlite:{}?mode=rwc", db_path.display());

let unset = SqliteStore::connect(&url, None)
.await
.expect("connect to sqlite");
assert_eq!(unset.max_connections_for_test(), DEFAULT_MAX_CONNECTIONS);

let raised = SqliteStore::connect(&url, Some(32))
.await
.expect("connect to sqlite");
assert_eq!(raised.max_connections_for_test(), 32);
}

/// An in-memory database lives inside its single connection: a second one
/// would open a different, empty database, so the ceiling cannot be raised.
#[tokio::test]
async fn in_memory_sqlite_pins_the_pool_to_one_connection() {
use super::sqlite::SqliteStore;

let store = SqliteStore::connect("sqlite::memory:", Some(32))
.await
.expect("connect to in-memory sqlite");
assert_eq!(store.max_connections_for_test(), 1);
}

#[cfg(unix)]
#[tokio::test]
async fn sqlite_connect_restricts_db_file_permissions() {
Expand Down
Loading
Loading