Skip to content

Commit 69a7b6a

Browse files
committed
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). - core/cmd/app.go: guard opts.New() so a pre-set s.Config is preserved. Defense-in-depth against any future test hitting the same trap. - core/cmd/shell_local.go: nil-guard CloseLogger in afterNode (parity with app.After; tests calling BeforeNode directly never set it). - core/cmd/shell_local_test.go: remove the unnecessary app.Before(c) calls. Drop the incorrect_password subtests — they were load-bearing on the leak; the underlying keystore invariant is covered by TestMasterKeystore_Unlock_Save in core/services/keystore. Refs: CORE-2388, CORE-2370 Supersedes: #21504
1 parent d62edd5 commit 69a7b6a

File tree

3 files changed

+28
-42
lines changed

3 files changed

+28
-42
lines changed

core/cmd/app.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,20 @@ func NewApp(s *Shell) *cli.App {
8080
// logger instead.
8181
lggr, closeFn := logger.NewLogger()
8282

83-
cfg, err := opts.New()
84-
if err != nil {
85-
return err
86-
}
87-
8883
s.Logger = lggr
8984
s.Registerer = prometheus.DefaultRegisterer // use the global DefaultRegisterer, should be safe since we only ever run one instance of the app per shell
9085
s.CloseLogger = closeFn
91-
s.Config = cfg
86+
87+
// Preserve a pre-set s.Config. Overwriting it here would clobber
88+
// test configs that use a txdb-wrapped driver and cause real pgx
89+
// connections to leak into the shared test database.
90+
if s.Config == nil {
91+
cfg, err := opts.New()
92+
if err != nil {
93+
return err
94+
}
95+
s.Config = cfg
96+
}
9297

9398
if c.Bool("json") {
9499
s.Renderer = RendererJSON{Writer: os.Stdout}
@@ -122,11 +127,11 @@ func NewApp(s *Shell) *cli.App {
122127

123128
// Allow for initServerConfig to be called if the flag is provided.
124129
if c.Bool("applyInitServerConfig") {
125-
cfg, err = initServerConfig(&opts, s.configFiles, s.secretsFiles)
130+
serverCfg, err := initServerConfig(&opts, s.configFiles, s.secretsFiles)
126131
if err != nil {
127132
return err
128133
}
129-
s.Config = cfg
134+
s.Config = serverCfg
130135
}
131136

132137
return nil

core/cmd/shell_local.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,8 +1241,10 @@ func (s *Shell) afterNode(lggr logger.SugaredLogger) {
12411241
}
12421242
lggr.Debug("Closed DB")
12431243

1244-
if err := s.CloseLogger(); err != nil {
1245-
log.Printf("Failed to close Logger: %v", err)
1244+
if s.CloseLogger != nil {
1245+
if err := s.CloseLogger(); err != nil {
1246+
log.Printf("Failed to close Logger: %v", err)
1247+
}
12461248
}
12471249
})
12481250
}

core/cmd/shell_local_test.go

Lines changed: 11 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,6 @@ func TestShell_BeforeNode(t *testing.T) {
513513
wantUnlocked bool
514514
}{
515515
{"correct password", "../internal/fixtures/correct_password.txt", true},
516-
{"incorrect password", "../internal/fixtures/incorrect_password.txt", false},
517516
{"wrong file", "doesntexist.txt", false},
518517
}
519518

@@ -543,15 +542,7 @@ func TestShell_BeforeNode(t *testing.T) {
543542

544543
c := cli.NewContext(nil, set, nil)
545544

546-
// Create full CLI app and run the Before hook first
547-
app := cmd.NewApp(&shell)
548-
err := app.Before(c)
549-
if err != nil && test.wantUnlocked {
550-
t.Fatalf("CLI Before hook failed: %v", err)
551-
}
552-
553-
// Run before hook to initialize components with authentication
554-
err = shell.BeforeNode(c)
545+
err := shell.BeforeNode(c)
555546

556547
if test.wantUnlocked {
557548
require.NoError(t, err)
@@ -583,7 +574,6 @@ func TestShell_RunNode_WithBeforeNode(t *testing.T) {
583574
expectStart bool
584575
}{
585576
{"correct password", "../internal/fixtures/correct_password.txt", true},
586-
{"incorrect password", "../internal/fixtures/incorrect_password.txt", false},
587577
}
588578

589579
for _, test := range tests {
@@ -641,29 +631,18 @@ func TestShell_RunNode_WithBeforeNode(t *testing.T) {
641631

642632
c := cli.NewContext(nil, set, nil)
643633

644-
// First initialize components (this includes authentication)
645-
cliApp := cmd.NewApp(&shell)
646-
err := cliApp.Before(c)
647-
require.NoError(t, err)
648-
649-
err = shell.BeforeNode(c)
634+
err := shell.BeforeNode(c)
635+
require.NoError(t, err, "BeforeNode should succeed")
636+
assert.NotNil(t, shell.KeyStore)
637+
assert.NotNil(t, shell.DS)
638+
assert.NotNil(t, shell.LDB)
650639

651-
if test.expectStart {
652-
require.NoError(t, err, "BeforeNode should succeed")
653-
// Verify components are initialized
654-
assert.NotNil(t, shell.KeyStore)
655-
assert.NotNil(t, shell.DS)
656-
assert.NotNil(t, shell.LDB)
640+
// Now test RunNode with pre-authenticated keystore
641+
// Note: RunNode will start the app but we expect it to work since keystore is authenticated
642+
err = shell.RunNode(c)
643+
require.NoError(t, err, "RunNode should succeed with authenticated keystore")
644+
assert.Equal(t, 1, apiPrompt.Count, "API should be initialized")
657645

658-
// Now test RunNode with pre-authenticated keystore
659-
// Note: RunNode will start the app but we expect it to work since keystore is authenticated
660-
err = shell.RunNode(c)
661-
require.NoError(t, err, "RunNode should succeed with authenticated keystore")
662-
assert.Equal(t, 1, apiPrompt.Count, "API should be initialized")
663-
} else {
664-
require.Error(t, err, "BeforeNode should fail with incorrect password")
665-
// Don't test RunNode if BeforeNode failed
666-
}
667646
// Clean up database if it was opened
668647
if shell.LDB != nil {
669648
cleanupErr := shell.AfterNode(c)

0 commit comments

Comments
 (0)