fix: prevent test DB leakage from TestShell_BeforeNode#21916
fix: prevent test DB leakage from TestShell_BeforeNode#21916
Conversation
|
👋 Fletch153, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
|
|
NewApp().Before() unconditionally overwrites shell.Config with a fresh config using the default pgx driver, even when tests pre-set a config with the txdb driver for isolation. This causes BeforeNode to open real database connections, leaking keystore entries (encrypted_key_rings) to the shared test database. Concurrent tests like Test_CSAKeyStore_E2E then find unexpected keys and fail. Guard config creation in app.Before() with a nil check so pre-set test configs are preserved. Migrate TestShell_BeforeNode and TestShell_RunNode_WithBeforeNode to heavyweight.FullTestDBV2 for proper DB isolation between subtests while preventing leakage to other tests.
Fletch153
force-pushed
the
fix/test-db-leakage-beforenode
branch
from
da148b0 to
79eec0b
Compare
|
1 participant
Summary
NewApp().Before()unconditionally overwritesshell.Configwith a fresh config using the defaultpgxdriver, even when tests pre-set a config with thetxdbdriver for DB isolation. This causesBeforeNode()to open real database connections, leaking keystore entries (encrypted_key_rings) to the shared test database.Test_CSAKeyStore_E2Efails intermittently because it finds unexpected CSA keys leaked byTestShell_BeforeNode— causing PRs to be ejected from the merge queue.app.Before()with a nil check so pre-set test configs are preserved. MigrateTestShell_BeforeNodeandTestShell_RunNode_WithBeforeNodetoheavyweight.FullTestDBV2for proper DB isolation.Root Cause Trace
Changes
core/cmd/app.goopts.New()+s.Config = cfgwithif s.Config == nil— preserves pre-set test configscore/cmd/shell_local_test.goTestShell_BeforeNodeandTestShell_RunNode_WithBeforeNodetoheavyweight.FullTestDBV2for dedicated test databasesWhy heavyweight?
The subtests are ordered: "correct password" creates a key ring, "incorrect password" verifies decryption fails with the wrong password. With
txdb, each subtest gets an isolated transaction and can't see the other's data.heavyweight.FullTestDBV2gives a dedicated database withpgxwhere subtests naturally share state, while the dedicated DB is dropped after the test — no leakage.Test plan
TestShell_BeforeNode— all 3 subtests passTestShell_RunNode_WithBeforeNode— all 2 subtests passTest_CSAKeyStore_E2E— "initializes with an empty state" passes (the flaking test)encrypted_key_ringscount = 0 after test run (no leakage)go vet ./core/cmd/...— no issuesgo_core_testspassesRelated: #21504 (Erik's WIP fix for the same issue)