Repository navigation
fix(database): retry failed PostgreSQL logins and report them as Unavailable - #3550
Open
danielle-mustillo wants to merge 7 commits into
Open
danielle-mustillo wants to merge 7 commits into
danielle-mustillo wants to merge 7 commits into
Conversation
github-actions
Bot
added
bug
danielle-mustillo
marked this pull request as ready for review
November 6, 2026 04:33
danielle-mustillo
force-pushed
the
danielle-mustillo/db-connect-retry
branch
from
November 6, 2026 16:38
1c1825b to
50fd57d
Compare
When the controller could not log in to PostgreSQL at runtime, the RPC returned Internal "internal server error" and the cause never reached the logs. - A *pgconn.ConnectError in the error chain now maps to Unavailable "database unavailable", so clients know to retry. Internal errors log their cause. - A login PostgreSQL rejects (28P01, 28000, 3D000, 42501) is a config error, not an outage, so it stays Internal. - A database failure during runtime auth returns Unavailable instead of Unauthenticated "invalid credentials". Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
The store's pool makes one login attempt per acquire. When PostgreSQL is briefly unreachable, for example during a restart or failover, a store call fails at once even though the next login would succeed. - Client.db is a retryingPool with Exec, Query and BeginTx. It retries a failed login (database.IsUnavailable) with backoff, 0.5 s doubling to 5 s. A login fails before any SQL is sent, so no statement repeats. - database.pool.connectRetryTimeout sets how long new attempts may start, 90s by default, 0s to turn the retry off. The caller's context still ends the retry. - Each login's TCP dial stops after 10 s when the URL sets no connect_timeout, so one hung dial cannot outlast the retry window. - helm/README.md explains the retry window and how client timeouts, such as the web UI's 30 s, cut it short. Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
danielle-mustillo
force-pushed
the
danielle-mustillo/db-connect-retry
branch
from
November 6, 2026 16:40
50fd57d to
55fbb98
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The retry loop can start attempts after its time budget expires, while rejected database logins bypass logging and incorrectly return Unauthenticated instead of Internal.
2 open issues
1 file not reviewed
go/api/gen/kagent/config/v1/config.pb.go: Generated file
What changed in this PR
This PR improves how the controller handles PostgreSQL login failures, adding configurable retries and clearer gRPC errors without retrying statements that have already run.
Changes:
- Adds login retries with backoff, a default 90-second retry window, and a 10-second default TCP dial timeout.
- Maps transient login failures to
Unavailableand logs server-side error causes. - Adds configuration validation, failure-path tests, and Helm documentation.
| File | Description |
|---|---|
proto/kagent/config/v1/config.proto |
Defines and validates the retry timeout. |
helm/README.md |
Explains retries and client timeout interactions. |
helm/kagent/values.yaml |
Adds a commented retry configuration example. |
go/core/pkg/postgres/pool.go |
Bounds TCP dialing when no connection timeout is configured. |
go/core/pkg/config/load_test.go |
Tests retry timeout validation and defaults. |
go/core/pkg/config/helm_test.go |
Tests Helm defaults and disabling retries. |
go/core/pkg/config/defaults.go |
Sets the default retry window to 90 seconds. |
go/core/pkg/app/app.go |
Passes retry configuration into the store. |
go/core/internal/grpcserver/interceptors.go |
Maps database failures and logs error causes. |
go/core/internal/grpcserver/interceptors_test.go |
Tests error mapping, logging, and runtime authentication failures. |
go/core/internal/dbtest/dbtest.go |
Adds a fake server that rejects PostgreSQL logins. |
go/core/internal/database/sql.go |
Wraps pool operations with login retries. |
go/core/internal/database/session_deletion_test.go |
Adapts client reconstruction to the pool wrapper. |
go/core/internal/database/push_notifications_test.go |
Uses the shared pool for direct test queries. |
go/core/internal/database/errors.go |
Classifies retryable login failures. |
go/core/internal/database/errors_test.go |
Tests login failure classification. |
go/core/internal/database/connect_retry_test.go |
Tests recovery, retry limits, and statement safety. |
go/core/internal/database/client_postgres.go |
Adds the retry option and integrates the pool wrapper. |
go/api/gen/kagent/config/v1/config.pb.go |
Regenerates protobuf bindings for the retry setting. |
Review effort: Balanced
This comment has been minimized.
This comment has been minimized.
A rejected controller login to PostgreSQL during actor authentication returned Unauthenticated "invalid credentials" and logged nothing. Every database login error now goes through the error mapper, so a rejected login returns Internal and logs its cause. Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
iplay88keys
reviewed
Nov 11, 2026
iplay88keys
left a comment
iplay88keys
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-generated review. I reviewed the findings before posting.
The retry design and the error mapping look correct to me. The focused tests pass locally. I have two fixes and two optional cleanups.
The authentication interceptor now calls database.IsLoginFailure instead of matching pgconn.ConnectError. The login error test helpers move to dbtest, so the database and grpcserver tests share one copy. Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
The web UI now waits 100 seconds for a unary request, above the controller's default 90-second login retry window, so a resuming database no longer fails the first UI request. The README now says a login attempt can finish after the window and that connect_timeout caps each attempt. Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
| * default 90s database login retry, so a resuming database doesn't fail the request. | ||
| * Streams use the idle timeout in `useChat` instead. | ||
| */ | ||
| export const REQUEST_TIMEOUT_MS = 100_000; |
Contributor
Author
There was a problem hiding this comment.
@Charlesthebird thoughts?
Basically the DB can take upto 60+ seconds to start (AWS Aurora scaled to 0 instances and is scaling up from cold to serve the request) and with this change, the UI waits for that time before failing the request.
If we don't put this then the UI would fail on such a request. Next time they try a request (assuming the DB is actually up) it will succeed.
Either/or is fine just wanted to know what you think from user experience.
Context behind it:
#3550 (comment)
CC @iplay88keys
Collaborator
There was a problem hiding this comment.
Agreed, 30s makes sense. Holding the UI for 100s doesn't.
Written by 🤖
The web UI goes back to waiting 30 seconds for a unary request, and the README says so again. Signed-off-by: Danielle Mustillo <danielle.mustillo@solo.io>
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.
Why
When the controller can't log in to PostgreSQL, for example during a restart, a failover, or a serverless database resuming from a pause, every API call fails at once with
Internal "internal server error". Nothing is logged, and clients can't tell that a retry would work.What changed
database.pool.connectRetryTimeout(default90s,0sturns it off). No SQL has been sent when a login fails, so a retry never repeats a statement. The caller's context still ends the retry.Unavailable "database unavailable", and the cause is logged. A login PostgreSQL rejects (28P01,28000,3D000,42501) staysInternaland isn't retried. A database failure during runtime auth returnsUnavailableinstead ofUnauthenticated.helm/README.mdexplains the retry window and how client timeouts cut it short. The web UI waits 30 s.Scope
The two commits are separate on purpose. The first one (error code and log) is useful without the retry.
Blast radius
This touches every store call. A call that used to fail at once can now wait up to 90 s before it fails. Set
connectRetryTimeout: 0sto get the old behavior back.Known limits
Unavailable.57P01), isn't retried. Runtime auth still reports it asUnauthenticated.Verification
90swindow, 2 of 2 session creates succeeded in 18.4 s and 20.4 s, after one retry each. With0s, 2 of 2 failed withUnavailableat 15.3 s.Unauthenticatedafter 30 s with no cause logged. After,Unavailableafter 20.5 s (20 s window) with the cause logged.NOLOGIN:Internalin 0.3 s after one attempt, withSQLSTATE 28000logged.TestSessionExpiration*,TestEgressPolicyCatalog,TestSandboxTemplateCatalog) fail the same way onmain.