From 57672f6e07bfc51d00b6d540eaf5d16c7f1566a9 Mon Sep 17 00:00:00 2001 From: Morchid Chellali Date: Tue, 18 Aug 2026 22:59:04 +0200 Subject: [PATCH 1/2] fix(gateway): keep last-known-good config when pg_settings read fails Fixes #720. Related: PR #723 (which hardens max_connections() itself to reuse the last successfully-read value instead of fabricating 25 on a bad read). This takes a complementary "direction A" approach at the call site instead: make load_configurations() propagate a hard error whenever the pg_settings fetch fails (connection failure, query failure, or an empty result), rather than silently substituting an empty Vec in its place. Why: refresh_configuration() already returns early on Err without calling self.values.store(...), so a failed refresh now keeps the last-known-good full config instead of swapping in a partial map that is missing max_connections. Losing that key flips max_connections() to the 25 default, which changes the per-user pool-cache key (PgPoolSettings derives Hash over it) and makes get_data_pool permanently hard-fail every request for that user with "Connection pool missing for user." -- the root cause of the crashloop in #720. Adds a regression test at the bottom of pg_configuration.rs asserting PgConfigurationInner::load_configurations() returns Err when Postgres is unavailable, using a real (lazy, deadpool) PoolManager so no live Postgres is required. Verified on the repo's pinned toolchain (Rust 1.95.0): cargo fmt --all -- --check clean, cargo clippy --workspace --all-targets --all-features -- -D warnings clean, cargo test -p documentdb_gateway_core 433 passed / 0 failed (incl. the new test). Signed-off-by: Morchid Chellali --- .../src/configuration/pg_configuration.rs | 125 ++++++++++++++++-- 1 file changed, 112 insertions(+), 13 deletions(-) diff --git a/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs b/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs index 73373a742..f5f5e7aa9 100644 --- a/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs +++ b/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs @@ -63,19 +63,35 @@ impl PgConfigurationInner { Err(e) => tracing::warn!("Host Config file not able to be loaded: {e}"), } - let pg_config_rows = match self.pool_manager.system_requests_connection().await { - Ok(conn) => conn - .query(self.pool_manager.query_catalog().pg_settings(), &[], &[]) - .await - .unwrap_or_else(|e| { - tracing::warn!("Failed to query pg_settings: {e}"); - Vec::new() - }), - Err(e) => { - tracing::warn!("Failed to get connection for pg_settings: {e}"); - Vec::new() - } - }; + // pg_settings is the source of the dynamic GUC map (max_connections and + // the rest). If it cannot be read, DO NOT continue with a partial map: + // returning it would let refresh_configuration() swap in a config missing + // max_connections, silently flip it to the 25 default, change the per-user + // pool-cache key, and then hard-fail every request with "Connection pool + // missing for user". Propagate the error instead so refresh_configuration() + // skips the swap and keeps the last-known-good full configuration. + let conn = self + .pool_manager + .system_requests_connection() + .await + .map_err(|e| { + DocumentDBError::internal_error(format!( + "Failed to get connection for pg_settings; keeping last-known-good configuration: {e}" + )) + })?; + let pg_config_rows = conn + .query(self.pool_manager.query_catalog().pg_settings(), &[], &[]) + .await + .map_err(|e| { + DocumentDBError::internal_error(format!( + "Failed to query pg_settings; keeping last-known-good configuration: {e}" + )) + })?; + if pg_config_rows.is_empty() { + return Err(DocumentDBError::internal_error( + "pg_settings returned no rows; keeping last-known-good configuration".to_owned(), + )); + } // Fetch most up-to-date switch-related values from pg_file_settings for settings that are set there. pg_settings may have stale // values if pg_reload_conf() failed or if 030-user-supplied-server-parameters.conf was updated after the gateway started. Then @@ -442,3 +458,86 @@ impl Drop for PgConfiguration { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + configuration::{CertInputType, CertificateOptions, DocumentDBSetupConfiguration}, + postgres::{ + conn_mgmt::{ + ConnectionPool, PgPoolSettings, AUTHENTICATION_MAX_CONNECTIONS, + SYSTEM_REQUESTS_MAX_CONNECTIONS, + }, + create_query_catalog, + }, + }; + + fn test_setup_configuration() -> DocumentDBSetupConfiguration { + let system_user = std::env::var("PostgresSystemUser") + .unwrap_or_else(|_| whoami::username().unwrap_or_default()); + + DocumentDBSetupConfiguration { + node_host_name: "localhost".to_owned(), + gateway_listen_port: Some(10260), + certificate_options: CertificateOptions { + cert_type: CertInputType::PemAutoGenerated, + ..Default::default() + }, + postgres_system_user: system_user.clone(), + postgres_data_user: system_user, + ..Default::default() + } + } + + /// Builds a real `PoolManager`. The pools are lazy (deadpool), so no live + /// Postgres is required, but pool creation must run inside a Tokio runtime. + fn test_pool_manager() -> Arc { + let setup_config = test_setup_configuration(); + let query_catalog = create_query_catalog(); + let user = setup_config.postgres_system_user().to_owned(); + + let system_requests_pool = ConnectionPool::new_with_user( + &setup_config, + &query_catalog, + &user, + None, + &format!("{}-SystemRequests", setup_config.application_name()), + PgPoolSettings::system_pool_settings(SYSTEM_REQUESTS_MAX_CONNECTIONS), + ) + .expect("failed to create system requests pool"); + + let authentication_pool = ConnectionPool::new_with_user( + &setup_config, + &query_catalog, + &user, + None, + &format!("{}-PreAuthRequests", setup_config.application_name()), + PgPoolSettings::system_pool_settings(AUTHENTICATION_MAX_CONNECTIONS), + ) + .expect("failed to create authentication pool"); + + Arc::new(PoolManager::new( + query_catalog, + Box::new(setup_config), + system_requests_pool, + authentication_pool, + )) + } + + #[tokio::test] + async fn load_configurations_errors_when_pg_settings_unavailable() { + tokio::task::yield_now().await; // lets the lazy pools build inside the runtime + let inner = PgConfigurationInner { + dynamic_config_path: String::new(), + settings_prefixes: Vec::new(), + pool_manager: test_pool_manager(), + instance_kind: String::new(), + enable_pg_file_settings_refresh: false, + }; + assert!( + inner.load_configurations().await.is_err(), + "expected Err when pg_settings is unavailable, so refresh keeps last-known-good config" + ); + } +} From ce4e0497a4f9e4f697505a431c23585bf3e687b7 Mon Sep 17 00:00:00 2001 From: Morchid Chellali Date: Wed, 19 Aug 2026 10:39:08 +0200 Subject: [PATCH 2/2] fix(gateway): preserve error source + strengthen config-keep-last-good tests Address Copilot review feedback on the config-keep-last-good fix: - load_configurations() now logs pg_settings context via tracing::warn! but propagates the original error (DocumentDBError for the connection acquisition, and DocumentDBError::from(tokio_postgres::Error) for the query), instead of discarding structured error info behind a generic internal_error wrap. The empty-rows guard stays an internal_error since it is a semantic condition, not a propagated error. - test_setup_configuration() now overrides postgres_host_name/port to a guaranteed-closed local endpoint (127.0.0.1:1) built on top of the shared crate::testing::test_setup_configuration() helper, so the test fails fast and deterministically instead of depending on whatever Postgres endpoint happens to be unreachable in the test environment. - Added refresh_configuration_keeps_last_known_good_values_on_error, which builds a full PgConfiguration with known values and an unreachable pool manager, and asserts that a failed refresh_configuration() leaves values (and last_update_at) untouched instead of only asserting that load_configurations() errs. Signed-off-by: Morchid Chellali --- .../src/configuration/pg_configuration.rs | 80 +++++++++++++++---- 1 file changed, 63 insertions(+), 17 deletions(-) diff --git a/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs b/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs index f5f5e7aa9..63a7e311e 100644 --- a/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs +++ b/pg_documentdb_gw/documentdb_gateway_core/src/configuration/pg_configuration.rs @@ -75,17 +75,19 @@ impl PgConfigurationInner { .system_requests_connection() .await .map_err(|e| { - DocumentDBError::internal_error(format!( + tracing::warn!( "Failed to get connection for pg_settings; keeping last-known-good configuration: {e}" - )) + ); + e })?; let pg_config_rows = conn .query(self.pool_manager.query_catalog().pg_settings(), &[], &[]) .await .map_err(|e| { - DocumentDBError::internal_error(format!( + tracing::warn!( "Failed to query pg_settings; keeping last-known-good configuration: {e}" - )) + ); + DocumentDBError::from(e) })?; if pg_config_rows.is_empty() { return Err(DocumentDBError::internal_error( @@ -463,7 +465,7 @@ impl Drop for PgConfiguration { mod tests { use super::*; use crate::{ - configuration::{CertInputType, CertificateOptions, DocumentDBSetupConfiguration}, + configuration::DocumentDBSetupConfiguration, postgres::{ conn_mgmt::{ ConnectionPool, PgPoolSettings, AUTHENTICATION_MAX_CONNECTIONS, @@ -473,20 +475,15 @@ mod tests { }, }; + /// Base setup configuration overridden with a guaranteed-closed local + /// Postgres endpoint (127.0.0.1:1), so connection attempts fail fast and + /// deterministically instead of depending on whatever Postgres endpoint + /// happens to be unreachable in the test environment. fn test_setup_configuration() -> DocumentDBSetupConfiguration { - let system_user = std::env::var("PostgresSystemUser") - .unwrap_or_else(|_| whoami::username().unwrap_or_default()); - DocumentDBSetupConfiguration { - node_host_name: "localhost".to_owned(), - gateway_listen_port: Some(10260), - certificate_options: CertificateOptions { - cert_type: CertInputType::PemAutoGenerated, - ..Default::default() - }, - postgres_system_user: system_user.clone(), - postgres_data_user: system_user, - ..Default::default() + postgres_host_name: Some("127.0.0.1".to_owned()), + postgres_port: Some(1), + ..crate::testing::test_setup_configuration() } } @@ -540,4 +537,53 @@ mod tests { "expected Err when pg_settings is unavailable, so refresh keeps last-known-good config" ); } + + /// Constructs a `PgConfiguration` directly (bypassing `new()`, which + /// would itself fail against the unreachable pool manager) so we can + /// seed known-good `values` and then exercise `refresh_configuration()`. + fn test_configuration(values: HashMap) -> PgConfiguration { + PgConfiguration { + inner: PgConfigurationInner { + dynamic_config_path: String::new(), + settings_prefixes: Vec::new(), + pool_manager: test_pool_manager(), + instance_kind: String::new(), + enable_pg_file_settings_refresh: false, + }, + values: ArcSwap::from_pointee(values), + last_update_at: ArcSwap::from_pointee(Instant::now()), + topology_bson: ArcSwap::from_pointee(rawbson!({})), + cluster_version: ArcSwap::from_pointee(None), + refresh_task: None, + watch_task: None, + } + } + + #[tokio::test] + async fn refresh_configuration_keeps_last_known_good_values_on_error() { + tokio::task::yield_now().await; // lets the lazy pools build inside the runtime + + let mut initial_values = HashMap::new(); + initial_values.insert("max_connections".to_owned(), "100".to_owned()); + let config = test_configuration(initial_values); + + let last_update_before = config.last_update_at(); + + let result = config.refresh_configuration().await; + + assert!( + result.is_err(), + "expected refresh_configuration() to fail against an unreachable Postgres endpoint" + ); + assert_eq!( + config.values.load().get("max_connections").map(String::as_str), + Some("100"), + "values must not be swapped when the refresh fails, keeping the last-known-good configuration" + ); + assert_eq!( + config.last_update_at(), + last_update_before, + "last_update_at must not change when the refresh fails" + ); + } }