test: serialize cross-tenant sweep tests + stop defaulting tests at the live DB - #409
Merged
Merged
Conversation
…t the live DB Two independent test-isolation problems in the packages that touch a real database. 1. Global sweeps race across packages. internal/infra/postgres and tests/integration are the only two packages that open a database, and `go test ./...` runs their binaries concurrently against the same one. Several of the things they exercise are deliberately global: RecoverStuckJobs, ExpireOldPlatformJobs, MarkStaleAsOffline and the recover_stuck_* SQL functions all sweep every tenant, because that is what a background reaper does. platform_job_lifecycle_db_test seeds a non-platform command — acknowledged, 120 minutes old, with a tenant agent — and asserts the PLATFORM sweep leaves it alone. recover_stuck_tenant_commands(10, 3), called from tests/integration/command_recovery_test, selects on exactly those columns. Seeding that row by hand and running the function returns 1 and rewrites it to pending/1, which is precisely what the assertion forbids. This has not been observed failing: 120 concurrent rounds of both packages produced no failure, because each test seeds, sweeps and asserts inside ~20-80ms. It is a latent hazard, not current CI noise. The window widens with -race (which CI uses), on a loaded runner, and with every test added to either package. The fix is a Postgres session-level advisory lock, taken for the whole test rather than just the sweep call — the race is between one package's seed and its assertion, so locking only the call would leave that window open. `-p 1` would serialize 91 packages to discipline two; weakening the assertions would remove the thing they exist to catch. Verified by holding the lock from psql and running a locked test: it blocks (timeout, vs 0.08s unblocked) and completes once released. 2. Three test helpers defaulted to the production database name. setupCommandTestDB and the two RLS helpers fell back to postgres://openctem@localhost:5432/openctem when DATABASE_URL was unset — so an unconfigured `go test ./...` wrote rows and ran cross-tenant sweeps against whatever "openctem" resolves to on that machine. In a dev environment that is the real database. They now skip, matching openPlatformJobDB.
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.
Two test-isolation problems in the only two packages that open a real database.
1. Global sweeps race across packages (latent, not observed)
internal/infra/postgresandtests/integrationare the only packages that open a DB, andgo test ./...runs their binaries concurrently against the same one. Several things they exercise are deliberately global —RecoverStuckJobs,ExpireOldPlatformJobs,MarkStaleAsOffline, and therecover_stuck_*SQL functions all sweep every tenant, because that is what a reaper does.platform_job_lifecycle_db_testseeds a non-platform command —acknowledged, 120 minutes old, with a tenant agent — and asserts the platform sweep leaves it alone.recover_stuck_tenant_commands(10, 3), called fromtests/integration/command_recovery_test, selects on exactly those columns:Seeding that row by hand and running the function:
That is precisely what the assertion forbids.
What I did not find: any actual failure. 120 concurrent rounds of both packages produced zero failures, because each test seeds, sweeps and asserts inside ~20-80ms. So this is a latent hazard, not CI noise anyone is currently suffering — I would rather say that than sell it as a flake fix. It is still worth closing: the window widens with
-race(which CI uses), on a loaded runner, and with every test added to either package.The fix
A Postgres session-level advisory lock, taken for the whole test rather than just the sweep call — the race is between one package's seed and its assertion, so locking only the call would leave exactly that window open.
Rejected alternatives:
-p 1would serialize 91 packages to discipline two; weakening the assertions would remove the thing they exist to catch.The helper is duplicated in both packages rather than extracted to a shared
testutil— it is nine lines, and the lock key is the contract between them, which is easier to see when both files state it.Proof the lock is wired, not just present: holding it from
psqland running a locked test →exit=124(timed out)exit=0, 0.08s2. Three helpers defaulted to the production database name
setupCommandTestDBand the two RLS helpers fell back topostgres://openctem@localhost:5432/openctemwhenDATABASE_URLwas unset. An unconfiguredgo test ./...therefore wrote rows and ran cross-tenant sweeps against whateveropenctemresolves to on that machine — in a dev environment, the real database. They nowt.Skip, matchingopenPlatformJobDB.Verification
go test ./...withDATABASE_URL=…/app_test— all greengo test -raceon both DB packages — greenmake lint-ci— clean-test.vshows=== RUN/--- PASS)