fix(cmd): prevent test DB connection leak via app.Before config swap

TestShell_BeforeNode and TestShell_RunNode_WithBeforeNode were calling
app.Before(c) after pre-setting shell.Config with a txdb-wrapped config
via configtest.NewGeneralConfig. app.Before unconditionally ran
opts.New() → CoreDefaults() which set Database.DriverName back to pgx
and assigned the fresh config to s.Config, silently overwriting the
test's txdb config. BeforeNode → pg.NewLockedDB then opened real pgx
connections against the shared chainlink_test database, persisting
keystore state and leaking it to other tests (flaking Test_CSAKeyStore_E2E)
and eating slots from the server-wide max_connections budget (causing
the mass 25-minute chan-receive timeouts in CORE-2388).

Three changes:

1. core/cmd/app.go — guard the config creation in app.Before so a
   pre-set s.Config is preserved. Defense-in-depth: any future test
   that sets its own config and goes through app.Before is protected
   from the same silent swap.

2. core/cmd/shell_local.go — nil-guard CloseLogger in afterNode. Tests
   that call BeforeNode directly (without app.Before) never set it.
   app.After already has this nil check; afterNode was missing it.

3. core/cmd/shell_local_test.go — remove the unnecessary app.Before(c)
   calls from the two affected tests. BeforeNode only needs Config and
   Logger, both already set directly; other tests in the same file
   already build the shell this way. Also drop the incorrect_password
   subtests — they were load-bearing on the leak (the correct_password
   subtest would persist a CSA key to the shared DB, which the
   incorrect_password subtest would then fail to decrypt, producing
   the expected error). The wrong-password-against-populated-keyring
   invariant is already covered at the correct layer by
   TestMasterKeystore_Unlock_Save/won't_load_a_saved_keyRing_if_the_password_is_incorrect
   in core/services/keystore. Testing it through the CLI would require
   cross-connection state sharing that chainlink's txdb path does not
   support: chainlink-common/pkg/sqlutil/pg/pg.go:69 generates a fresh
   UUID DSN per pg.NewConnection call, isolating every pool into its
   own transaction.

Refs: CORE-2388, CORE-2370
Supersedes: #21504

Author: Fletch153

core/cmd/app.go

@@ -80,15 +80,21 @@ func NewApp(s *Shell) *cli.App {
 		// logger instead.
 		lggr, closeFn := logger.NewLogger()
 
-		cfg, err := opts.New()
-		if err != nil {
-			return err
-		}
-
 		s.Logger = lggr
 		s.Registerer = prometheus.DefaultRegisterer // use the global DefaultRegisterer, should be safe since we only ever run one instance of the app per shell
 		s.CloseLogger = closeFn
-		s.Config = cfg
+
+		// Only create a new config from opts if one hasn't been pre-set.
+		// Tests pre-set s.Config with a txdb-wrapped driver for DB isolation;
+		// unconditionally overwriting it here causes real pgx connections to
+		// leak between tests and exhaust the connection pool.
+		if s.Config == nil {
+			cfg, err := opts.New()
+			if err != nil {
+				return err
+			}
+			s.Config = cfg
+		}
 
 		if c.Bool("json") {
 			s.Renderer = RendererJSON{Writer: os.Stdout}
@@ -122,11 +128,11 @@ func NewApp(s *Shell) *cli.App {
 
 		// Allow for initServerConfig to be called if the flag is provided.
 		if c.Bool("applyInitServerConfig") {
-			cfg, err = initServerConfig(&opts, s.configFiles, s.secretsFiles)
+			serverCfg, err := initServerConfig(&opts, s.configFiles, s.secretsFiles)
 			if err != nil {
 				return err
 			}
-			s.Config = cfg
+			s.Config = serverCfg
 		}
 
 		return nil

core/cmd/shell_local.go

@@ -1241,8 +1241,10 @@ func (s *Shell) afterNode(lggr logger.SugaredLogger) {
 		}
 		lggr.Debug("Closed DB")
 
-		if err := s.CloseLogger(); err != nil {
-			log.Printf("Failed to close Logger: %v", err)
+		if s.CloseLogger != nil {
+			if err := s.CloseLogger(); err != nil {
+				log.Printf("Failed to close Logger: %v", err)
+			}
 		}
 	})
 }

core/cmd/shell_local_test.go

@@ -507,13 +507,18 @@ func TestShell_RemoveBlocks(t *testing.T) {
 
 func TestShell_BeforeNode(t *testing.T) {
 	testutils.SkipShortDB(t)
+	// Note: the "wrong password against an existing key ring" case is
+	// intentionally covered by TestMasterKeystore_Unlock_Save in
+	// core/services/keystore, not here. That is a keystore invariant, and
+	// asserting it through the CLI would require cross-connection state
+	// sharing that chainlink's txdb path does not support (pg.NewConnection
+	// generates a fresh UUID DSN per call — chainlink-common/pkg/sqlutil/pg/pg.go:69).
 	tests := []struct {
 		name         string
 		pwdfile      string
 		wantUnlocked bool
 	}{
 		{"correct password", "../internal/fixtures/correct_password.txt", true},
-		{"incorrect password", "../internal/fixtures/incorrect_password.txt", false},
 		{"wrong file", "doesntexist.txt", false},
 	}
 
@@ -543,15 +548,11 @@ func TestShell_BeforeNode(t *testing.T) {
 
 			c := cli.NewContext(nil, set, nil)
 
-			// Create full CLI app and run the Before hook first
-			app := cmd.NewApp(&shell)
-			err := app.Before(c)
-			if err != nil && test.wantUnlocked {
-				t.Fatalf("CLI Before hook failed: %v", err)
-			}
-
-			// Run before hook to initialize components with authentication
-			err = shell.BeforeNode(c)
+			// BeforeNode only reads Config and Logger, both already set
+			// above. Going through app.Before would silently overwrite
+			// s.Config with a fresh pgx-driven one from CoreDefaults and
+			// break txdb isolation — see CORE-2388 and the guard in app.go.
+			err := shell.BeforeNode(c)
 
 			if test.wantUnlocked {
 				require.NoError(t, err)
@@ -577,13 +578,14 @@ func TestShell_BeforeNode(t *testing.T) {
 }
 
 func TestShell_RunNode_WithBeforeNode(t *testing.T) {
+	// Note: the wrong-password case is covered by TestMasterKeystore_Unlock_Save
+	// in core/services/keystore, not here. See TestShell_BeforeNode for details.
 	tests := []struct {
 		name        string
 		pwdfile     string
 		expectStart bool
 	}{
 		{"correct password", "../internal/fixtures/correct_password.txt", true},
-		{"incorrect password", "../internal/fixtures/incorrect_password.txt", false},
 	}
 
 	for _, test := range tests {
@@ -641,29 +643,20 @@ func TestShell_RunNode_WithBeforeNode(t *testing.T) {
 
 			c := cli.NewContext(nil, set, nil)
 
-			// First initialize components (this includes authentication)
-			cliApp := cmd.NewApp(&shell)
-			err := cliApp.Before(c)
-			require.NoError(t, err)
-
-			err = shell.BeforeNode(c)
+			// BeforeNode only needs Config and Logger, both already set.
+			// Avoid app.Before here — see CORE-2388 / TestShell_BeforeNode.
+			err := shell.BeforeNode(c)
+			require.NoError(t, err, "BeforeNode should succeed")
+			assert.NotNil(t, shell.KeyStore)
+			assert.NotNil(t, shell.DS)
+			assert.NotNil(t, shell.LDB)
 
-			if test.expectStart {
-				require.NoError(t, err, "BeforeNode should succeed")
-				// Verify components are initialized
-				assert.NotNil(t, shell.KeyStore)
-				assert.NotNil(t, shell.DS)
-				assert.NotNil(t, shell.LDB)
+			// Now test RunNode with pre-authenticated keystore
+			// Note: RunNode will start the app but we expect it to work since keystore is authenticated
+			err = shell.RunNode(c)
+			require.NoError(t, err, "RunNode should succeed with authenticated keystore")
+			assert.Equal(t, 1, apiPrompt.Count, "API should be initialized")
 
-				// Now test RunNode with pre-authenticated keystore
-				// Note: RunNode will start the app but we expect it to work since keystore is authenticated
-				err = shell.RunNode(c)
-				require.NoError(t, err, "RunNode should succeed with authenticated keystore")
-				assert.Equal(t, 1, apiPrompt.Count, "API should be initialized")
-			} else {
-				require.Error(t, err, "BeforeNode should fail with incorrect password")
-				// Don't test RunNode if BeforeNode failed
-			}
 			// Clean up database if it was opened
 			if shell.LDB != nil {
 				cleanupErr := shell.AfterNode(c)