fix(workspace): apply changed database credentials to cached connection pools - #811
Merged
Merged
Conversation
`ConnectionKey` covers host, port, username and database, but not the password, so a password-only change returned the pool that was created with the old credentials and the new password was silently ignored. A wrong password additionally recorded a failure backoff of up to a minute, which suppressed the corrected credentials until it expired. Store a hash of the connection parameters that the key does not cover (connection string, password, connection timeout) with each cached pool and each recorded failure. A mismatch drops the stale pool and clears the stale backoff, so the next lookup builds a pool from the current settings. The cache key is unchanged, and hashing keeps the credentials out of the cache entries.
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.
The defect
Changing only the database password has no effect: the language server keeps using a connection pool built with the old credentials. Fixing a wrong password is worse — the corrected password stays suppressed for up to a minute.
Evidence:
crates/pgls_workspace/src/workspace/server/connection_key.rs:7-20—ConnectionKeycovers host, port, username and database, but not the password or the connection string.crates/pgls_workspace/src/workspace/server/connection_manager.rs:46-62—get_poolreturns the cached pool for that key, so the pool keeps thePgConnectOptionsit was created with. A password-only change is therefore silently ignored.CachedFailurewith an exponential backoff of up to 60s (record_failure,connection_is_in_backoff), and because the backoff is keyed the same way, corrected credentials are skipped until it expires.The fix
Store a
DefaultHasherfingerprint of the connection parameters that the key does not cover — connection string, password, connection timeout — alongside each cached pool and each recorded failure. On lookup, a fingerprint mismatch is treated as a miss: the stale pool is dropped and rebuilt from the current settings, and a stale backoff entry is cleared so the new credentials are retried immediately.Deliberate choices:
ConnectionKey, which is also used to key the schema cache.Debug-printed.with_poolnow derives the failure key from the settings rather than from the pool, so the failure and pool entries are keyed consistently.Tests
In
connection_manager.rs:same_settings_reuse_cached_pool— repeated lookups with unchanged settings return the same underlying pool (asserted by closing the first handle and observing the second is closed too).password_change_replaces_cached_pool_with_new_settings— after a password-only change the returned pool is a new one, not the closed cached one, and the stored fingerprint is updated.password_change_clears_failure_backoff— a recorded backoff for the old password does not suppress the new one.What they do not prove: they do not authenticate against Postgres with a changed password, because
PgConnectOptionsexposes no password getter to assert on. They prove the cached pool is discarded and rebuilt from the current settings, which is where the old options came from.Provenance
Split out of #809 (sticky client configuration overrides), which made this defect directly reachable: overriding only the password through the new request would otherwise do nothing. It stands on its own and is independent of that PR.
Validation