fix: properly wait for Ryuk startup when reusing reaper - #3761
Merged
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThis change tightens Reaper reuse readiness by waiting for both startup log output and port availability, and extends the concurrent reuse test to dial the Reaper endpoint and verify the handshake succeeds. ChangesReaper readiness and handshake validation
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reaper_test.go`:
- Around line 532-547: The test helper reaperConnect is using require.NoError
with a message that includes %w, but msgAndArgs are formatted with fmt.Sprintf
rather than error-wrapping. Update the dial assertion to use a normal error
formatting verb so the underlying dial failure is printed correctly, and keep
the same context around reaper.Endpoint and the handshake call for debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
karsov
changed the title
karsov
changed the title
1 task
4 tasks
This was referenced Aug 26, 2026
Closed
thockin
pushed a commit
to agent-substrate/substrate
that referenced
this pull request
Aug 27, 2026
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
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
2 participants
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.
What does this PR do?
Adds a
wait.ForLog("Started")when reusing an already running reaper DockerContainer.Why is it important?
Without the additional wait condition, when multiple concurrent tests are run in different packages, only the package where the reaper is created (and not reused) waits properly for the Ryuk startup. For all the other packages the connection to the reaper container is attempted before Ryuk is ready and it fails with errors like the following:
or
Without connection, the reaper does not know it should wait for the tests in that packages to finish before cleaning up their containers, leading to failing tests.
More details in #3743 .
Related issues
go test ./...#3743How to test this PR
As agreed in #3743 , first I'm adding only the commit updating the tests to demonstrate the failure, so we can run the CI on it to expose the failure, and then I'll add the fix itself.