fix(gateway): keep last-known-good config when pg_settings read fails - #2
Merged
Conversation
Fixes documentdb#720. Related: PR documentdb#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 documentdb#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 <mc@wetransform.to>
There was a problem hiding this comment.
Pull request overview
Reworks the Rust gateway’s dynamic configuration load path so that failures reading pg_settings propagate as errors, allowing refresh_configuration() to retain the last-known-good configuration instead of swapping in an effectively empty map that can destabilize connection-pool behavior.
Changes:
- Propagate errors (and empty-result cases) from
pg_settingsreads inPgConfigurationInner::load_configurations(). - Add a unit test intended to validate the error-on-unavailable behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+73
to
+89
| 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}" | ||
| )) | ||
| })?; |
Comment on lines
+480
to
+490
| 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() | ||
| } |
Comment on lines
+528
to
+542
| #[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" | ||
| ); | ||
| } |
…d 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 <mc@wetransform.to>
Member
Author
|
Thanks — all three addressed in ce4e049:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reworks the gateway's dynamic-config load path (
PgConfigurationInner::load_configurations) so a failedpg_settingsread (connection error, query error, or empty result) returns an error instead of silently falling back toVec::new().refresh_configuration()already returns early onErrwithout swappingself.values, so a failed refresh now keeps the last-known-good full configuration instead of wiping every GUC to defaults.Why
On every ~300s dynamic-config refresh, a transient failure to read
pg_settingsproduced a near-empty config. That silently droppedmax_connectionsto the25default, which is part of the per-user connection-pool cache key — so the key flipped and the gateway hard-failed every subsequent request withConnection pool missing for user(surfaced to clients as a maskedcode:1 / InternalError). This crashlooped a downstream service on a ~5-minute cadence.This is the "direction A" from the discussion in documentdb#723: on a failed refresh load, skip the swap and keep the last-known-good full config. Complements issue documentdb#720.
Why into this fork's
mainmain-based (0.118 engine) so a single commit serves as (a) the source for our internally-builtdocumentdb-localimage and (b) the basis for the upstream documentdb#723 rework. The same change will be submitted upstream.Tests
Adds
load_configurations_errors_when_pg_settings_unavailable(asserts the load returnsErrwhen Postgres is unavailable, proving a failed refresh keeps prior config).cargo fmt --check,cargo clippy --workspace --all-targets --all-features -- -D warnings, andcargo test -p documentdb_gateway_core(433 passed) all green on the pinned Rust 1.95.AI Disclosure
AI tools were used. Tool: Claude (Anthropic). Root-cause tracing, the fix, and the test were prepared with substantial AI assistance, then reviewed and verified against the source by the author.