fix(postgres): make BasicWaitStrategies safe with container reuse - #3816
fix(postgres): make BasicWaitStrategies safe with container reuse#3816aayushcodex17 wants to merge 4 commits into
Conversation
Container logs survive Docker restarts. When WithReuseByName is used, the two-occurrence log wait in BasicWaitStrategies() is satisfied immediately by readiness lines from a previous run — before the current Postgres process finishes crash recovery — causing Run() to return a container that is not yet accepting connections. Add a pg_isready exec probe as a live-state gate between the log wait and the port wait. The probe uses PQping (no credentials required), prefers the unix socket with a TCP loopback fallback that honours PGPORT, and exits 0 gracefully on images that do not ship pg_isready, preserving existing behaviour for those images. Add a regression test that forces crash recovery via zero-grace SIGKILL with an open client session and asserts that a single, retry-free connection attempt succeeds right after Run() returns. Fixes testcontainers#3671
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be7fcda4c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Then, we probe the live server state until it accepts connections, because | ||
| // the log message of a previous run of a reused container cannot prove that | ||
| // the current process is ready. Skipped on images without pg_isready. | ||
| wait.ForExec([]string{"sh", "-c", `command -v pg_isready >/dev/null 2>&1 || exit 0; pg_isready || pg_isready -h 127.0.0.1 -p "${PGPORT:-5432}"`}), |
There was a problem hiding this comment.
When callers pass libpq defaults such as PGHOST or PGPORT into the Postgres container, this command lets pg_isready inherit them, and the fallback explicitly uses PGPORT. In a reused container PGHOST can point at another accepting server, or PGPORT can point away from the module's fixed 5432 listener, so the new live check can either succeed without probing this container's postmaster or time out even though the module's normal port is ready; the probe should pin or clear the connection parameters it is validating.
Useful? React with 👍 / 👎.
Walkthrough
ChangesPostgres readiness validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: 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 |
- test.sh: two-mode harness (base/new) using go-junit-report for JUnit XML output. base runs existing wait-strategy tests as a regression check; new runs TestBasicWaitStrategies_reusedContainer which fails without the fix and passes with it. - Dockerfile: builds from olympus-base-go, downloads Go module dependencies for both the root module and modules/postgres (the replace directive requires both) so the offline runtime container can compile and run tests without network access.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the relative path in the comment.
The directory
modules/postgresis two levels deep relative to the root, so the relative path in the replace directive is../.., not../../...📝 Proposed fix
-# modules/postgres/go.mod has a replace directive pointing to the root (../../..), +# modules/postgres/go.mod has a replace directive pointing to the root (../..),🤖 Prompt for 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. In `@Dockerfile` at line 8, Update the relative path mentioned in the Dockerfile comment for modules/postgres/go.mod, changing the replace directive reference from ../../.. to ../.. while preserving the rest of the comment.
🤖 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.
Nitpick comments:
In `@Dockerfile`:
- Line 8: Update the relative path mentioned in the Dockerfile comment for
modules/postgres/go.mod, changing the replace directive reference from ../../..
to ../.. while preserving the rest of the comment.
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 `@test.sh`:
- Around line 152-172: Resolve OUTPUT_PATH to an absolute path before the cd
into modules/postgres in the test.sh flow, while preserving the existing report
destinations for both base and new modes. Ensure the go-junit-report
redirections continue writing to that resolved path after the directory change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| +cd "$REPO_ROOT/modules/postgres" | ||
| + | ||
| +case "$MODE" in | ||
| + base) | ||
| + # Regression check: existing tests covering the postgres module's wait-strategy | ||
| + # path and basic container startup. Excludes the new reuse test so this mode | ||
| + # is independent of the solution patch. | ||
| + go test -v -count=1 -timeout 10m \ | ||
| + -run "^(TestContainerWithWaitForSQL|TestWithConfigFile|TestWithInitScript|TestWithOrderedInitScript)$" \ | ||
| + ./... 2>&1 \ | ||
| + | go-junit-report -set-exit-code > "$OUTPUT_PATH" | ||
| + ;; | ||
| + new) | ||
| + # Regression test for false-positive ready signal on reused containers. | ||
| + # Fails on the base commit (log wait satisfied by stale logs before crash | ||
| + # recovery completes); passes once BasicWaitStrategies adds a live-state probe. | ||
| + go test -v -count=1 -timeout 10m \ | ||
| + -run "^TestBasicWaitStrategies_reusedContainer$" \ | ||
| + ./... 2>&1 \ | ||
| + | go-junit-report -set-exit-code > "$OUTPUT_PATH" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve OUTPUT_PATH before changing directories.
Relative paths are currently interpreted beneath modules/postgres, so the caller may not find the generated JUnit report.
Proposed fix
+if [[ "$OUTPUT_PATH" != /* ]]; then
+ OUTPUT_PATH="$PWD/$OUTPUT_PATH"
+fi
+
REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
cd "$REPO_ROOT/modules/postgres"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| +cd "$REPO_ROOT/modules/postgres" | |
| + | |
| +case "$MODE" in | |
| + base) | |
| + # Regression check: existing tests covering the postgres module's wait-strategy | |
| + # path and basic container startup. Excludes the new reuse test so this mode | |
| + # is independent of the solution patch. | |
| + go test -v -count=1 -timeout 10m \ | |
| + -run "^(TestContainerWithWaitForSQL|TestWithConfigFile|TestWithInitScript|TestWithOrderedInitScript)$" \ | |
| + ./... 2>&1 \ | |
| + | go-junit-report -set-exit-code > "$OUTPUT_PATH" | |
| + ;; | |
| + new) | |
| + # Regression test for false-positive ready signal on reused containers. | |
| + # Fails on the base commit (log wait satisfied by stale logs before crash | |
| + # recovery completes); passes once BasicWaitStrategies adds a live-state probe. | |
| + go test -v -count=1 -timeout 10m \ | |
| + -run "^TestBasicWaitStrategies_reusedContainer$" \ | |
| + ./... 2>&1 \ | |
| + | go-junit-report -set-exit-code > "$OUTPUT_PATH" | |
| if [[ "$OUTPUT_PATH" != /* ]]; then | |
| OUTPUT_PATH="$PWD/$OUTPUT_PATH" | |
| fi | |
| REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cd "$REPO_ROOT/modules/postgres" | |
| case "$MODE" in | |
| base) | |
| # Regression check: existing tests covering the postgres module's wait-strategy | |
| # path and basic container startup. Excludes the new reuse test so this mode | |
| # is independent of the solution patch. | |
| go test -v -count=1 -timeout 10m \ | |
| -run "^(TestContainerWithWaitForSQL|TestWithConfigFile|TestWithInitScript|TestWithOrderedInitScript)$" \ | |
| ./... 2>&1 \ | |
| | go-junit-report -set-exit-code > "$OUTPUT_PATH" | |
| ;; | |
| new) | |
| # Regression test for false-positive ready signal on reused containers. | |
| # Fails on the base commit (log wait satisfied by stale logs before crash | |
| # recovery completes); passes once BasicWaitStrategies adds a live-state probe. | |
| go test -v -count=1 -timeout 10m \ | |
| -run "^TestBasicWaitStrategies_reusedContainer$" \ | |
| ./... 2>&1 \ | |
| | go-junit-report -set-exit-code > "$OUTPUT_PATH" |
🤖 Prompt for 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.
In `@test.sh` around lines 152 - 172, Resolve OUTPUT_PATH to an absolute path
before the cd into modules/postgres in the test.sh flow, while preserving the
existing report destinations for both base and new modes. Ensure the
go-junit-report redirections continue writing to that resolved path after the
directory change.
…d step - test.sh: replaced bash harness with valid unified git diff (diff --git, --- /dev/null, +++ b/, @@ hunk headers, new file mode 100755); removed set -e to prevent premature exit before JUnit report is written - Dockerfile: copy go.mod/go.sum before source for better layer caching; add go build ./... for root and postgres module to pre-populate build cache so tests compile offline at runtime
| @@ -0,0 +1,21 @@ | |||
| FROM public.ecr.aws/d3j8x8q7/olympus-base-go:latest | |||
There was a problem hiding this comment.
question: do we need this Dockerfile and test.sh files?
There was a problem hiding this comment.
No, Actually We dont need it. Was just trying to prepare a testing situation, pushed it all by mistake.
2 participants
postgres.BasicWaitStrategies()gives a false-positive ready signal when containers are reused viatestcontainers.WithReuseByName, causing tests to fail non-deterministically withFATAL: the database system is not yet accepting connections.Docker container logs survive across restarts of the same container. When a reused container starts after an unclean shutdown, the log-based wait is satisfied immediately by readiness messages from the previous run -- before the current Postgres process has finished crash recovery -- so
Run()returns a container that is not yet accepting connections. The race is most reliably triggered by stopping a container with a zero-grace timeout while a client session is open, which forces crash recovery on the next start.This PR adds a live-state probe after the log gate that blocks until Postgres is genuinely accepting connections, regardless of what the logs say. The probe requires no credentials, so custom
WithUsername,WithPassword, andWithDatabaseoptions keep working. It is skipped gracefully on images that do not ship the necessary binary, preserving existing behaviour for those images. Fresh-start behaviour is unchanged -- the log gate still passes only once Postgres is up, so the probe adds no meaningful overhead on a first boot.The added regression test starts a container with
WithReuseByName, inserts enough data to generate WAL, kills the container with a zero-grace timeout while a session is open to guarantee crash recovery on the next start, then asserts that a single retry-free connection attempt succeeds immediately afterRun()returns.