ci: stop the testcontainers reaper from deleting live test databases - #1235
Conversation
c8499d2 to
f407c4a
Compare
|
Split out the dependency bump into #1237 so this stays reviewable at four lines. The bump does not replace this change — see the cold-cache numbers there. |
f407c4a to
f728bdb
Compare
Part of #1230 When reusing an already-running Ryuk reaper, testcontainers-go v0.43.0 waits only for its Docker port mapping. Docker exposes that port before Ryuk is listening, so a second package can connect too early and lose the handshake with `read ack: EOF`. That is [testcontainers-go#3743](testcontainers/testcontainers-go#3743); v0.44.0 also waits for the reaper's `Started` log line ([#3761](testcontainers/testcontainers-go#3761)). A package whose handshake fails is not counted as a Ryuk client but its containers still carry the shared session label, so another package exiting can delete a database that is still in use — the failure reported in #1230. ## Scope This helps local runs, where the reaper stays on. **It does not fix CI on its own**, so it is deliberately separate from #1235, which disables the reaper for `run-tests`. Measured on a 4-core Linux VM with cold build caches, which staggers package start times the way CI does: ``` v0.43.0, 3 rounds handshake failures in 3/3 rounds; one round cascaded v0.44.0, 3 rounds no handshake failures; database tests still unavailable in 3/3 rounds ``` Under v0.44.0 the failures move to `wait for reaper <id>: context deadline exceeded` — a late package finds a reaper that is already shutting down, and one readiness probe (`defaultStartupTimeout`, 60s) outlives the whole reaper retry budget (`MaxElapsedTime`, 20s), so the retry loop never gets a second attempt. The remaining fail-open behind all of this is [#3827](testcontainers/testcontainers-go#3827), still open upstream. ## Diff size Four lines of `go.mod`. The rest is `go mod vendor` output: v0.44.0 pulls newer `moby/client`, `gopsutil`, and `otelhttp`, and `otelhttp` moves `otel/semconv` from v1.39.0 to v1.41.0. Insertions and deletions nearly cancel because most of it is a directory swap and one generated `httpsnoop` file being merged into another. ``` go.mod, go.sum 62 lines vendor/ 61 files, 17356 +/17490 - ``` `hack/verify/go-modules.sh`, `licenses.sh`, `boilerplate.sh`, and `gofmt.sh` all pass; `go test -race ./cmd/ateapi/...` is green with no silently skipped database tests. --- - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
f728bdb to
cd7e318
Compare
That seems unacceptable. Wherever possible CI should match local development behavior. Leaking containers is a bug. |
cd7e318 to
fe833f9
Compare
| # A lost reaper handshake gets a package's containers pruned mid-run | ||
| # (testcontainers-go#3827); a throwaway runner needs no reaping anyway. |
There was a problem hiding this comment.
we should just fix the handshake. relying on "it's in CI" leads to bugs that cannot be tested locally.
There was a problem hiding this comment.
Locally the reaper stays on and the blast radius of #3827 is small — worst case a flaky full-suite run, and the fix for that is upstream (testcontainers-go#3841, not yet merged).
Disabling Ryuk in CI is also what the testcontainers docs recommend: "We recommend using it only for Continuous Integration services that have their own mechanism to clean up resources." — a GitHub-hosted runner is a fresh VM discarded after the job, so there's nothing left to reap.
fe833f9 to
9fd5e29
Compare
`go test ./...` gives every package binary the same testcontainers session, so they share one reaper, and it deletes that session's containers as soon as its client count reaches zero -- including a database another package is still using. A package whose handshake failed is not counted and has no way to tell. Nothing here needs reaping: the runner is discarded with the job. Claude-Session: https://claude.ai/code/session_01XuQqkwLf5Zx6CSZFHSC6hb
9fd5e29 to
f628a34
Compare
Fixes #1230
cmd/ateapi/internal/controlapifails ~30 tests at once inrun-tests, every one oncreating PostgreSQL test database: conn closed. A package's PostgreSQL container is deleted while that package is still using it.Root cause
Every package binary under one
go test ./...derives the same testcontainers session ID from the shared parent process, so all five packages that start a PostgreSQL container share one Ryuk reaper and one set of container labels. Ryuk deletes everything carrying that label once its connected-client count reaches zero.The original failure had two upstream contributors:
read ack: EOF. testcontainers-go#3743 was fixed by #3761; testcontainers-go v0.44.0 is now in main via deps: bump testcontainers-go to v0.44.0 #1237.Reaper.connectlogs a failed handshake or startup and returns success anyway, so a package can run unregistered while its containers still carry the shared session label. #3827 remains open upstream. Under v0.44.0 the observed failure moves towait for reaper <id>: context deadline exceededrather than eliminating the shared-reaper race.One package then exits, Ryuk sees zero clients, and prunes the session, including a database another package is mid-suite on.
storetest'ssync.Onceblocks recreation, so every remaining test in that package fails on the same line.The package count has since grown from four to five. That increases contention but does not change the defect.
Fix
Disable the reaper for
run-tests.The reaper exists to clean up after a process that dies without doing so.
storetestandatepgnow terminate their own containers, and a GitHub runner is destroyed with the job, so CI does not need a shared reaper. Local development is untouched and keeps it.No test accompanies this: the failure lives in CI job composition and a dependency, neither of which is reachable from a Go test. Evidence is below instead.
Evidence
Ryuk's own log during a failing run; the container is removed, not crashed:
Forcing exactly one package to lose its reaper connection, with nothing else changed:
30 of the 30 failing test names are among the 32 seen in run 32926359026.
Related changes
wait for reaper <id>: context deadline exceeded.