From 4ceb549c3ffb1f44d7329893edf5de1aceafa367 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 20 Jun 2025 14:11:52 +0200 Subject: [PATCH] chore: close db properly in early exit paths in ConnectToPostgres (#18448) There were some code paths where if we exited early from the function the postgres connection would never get cleaned up. This is the mechanism that cleans up the db - it requires the err variable to be not nil: https://github.com/coder/coder/blob/118bf981454188c4989e8b565dec67906616f885/cli/server.go#L2319-L2328 --- cli/server.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/cli/server.go b/cli/server.go index 0cc7b0edf2..7d587f5e1a 100644 --- a/cli/server.go +++ b/cli/server.go @@ -2312,19 +2312,20 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d var err error var sqlDB *sql.DB + dbNeedsClosing := true // Try to connect for 30 seconds. ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() defer func() { - if err == nil { + if !dbNeedsClosing { return } if sqlDB != nil { _ = sqlDB.Close() sqlDB = nil + logger.Debug(ctx, "closed db before returning from ConnectToPostgres") } - logger.Error(ctx, "connect to postgres failed", slog.Error(err)) }() var tries int @@ -2361,12 +2362,7 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d } defer version.Close() if !version.Next() { - // it's critical we assign to the err variable, otherwise the defer statement - // that runs db.Close() will not execute it - if err = version.Err(); err != nil { - return nil, xerrors.Errorf("no rows returned for version select: %w", err) - } - return nil, xerrors.Errorf("no rows returned for version select") + return nil, xerrors.Errorf("no rows returned for version select: %w", version.Err()) } var versionNum int err = version.Scan(&versionNum) @@ -2408,6 +2404,7 @@ func ConnectToPostgres(ctx context.Context, logger slog.Logger, driver string, d // of connection churn. sqlDB.SetMaxIdleConns(3) + dbNeedsClosing = false return sqlDB, nil }