From be7fcda4c38ccb942845578a16e077ae3e62ec88 Mon Sep 17 00:00:00 2001 From: aayushcodex17 Date: Fri, 17 Jul 2026 00:18:16 +0530 Subject: [PATCH 1/4] fix(postgres): make BasicWaitStrategies safe with container reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #3671 --- modules/postgres/wait_strategies.go | 12 ++- modules/postgres/wait_strategies_test.go | 107 +++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 modules/postgres/wait_strategies_test.go diff --git a/modules/postgres/wait_strategies.go b/modules/postgres/wait_strategies.go index 92dc3f6ec6..461fc5f695 100644 --- a/modules/postgres/wait_strategies.go +++ b/modules/postgres/wait_strategies.go @@ -6,9 +6,15 @@ import ( ) // BasicWaitStrategies is a simple but reliable way to wait for postgres to start. -// It returns a two-step wait strategy: +// It returns a three-step wait strategy: // // - It will wait for the container to log `database system is ready to accept connections` twice, because it will restart itself after the first startup. +// - It will then probe the live server state with `pg_isready` until postgres accepts connections. +// Container logs survive restarts, so on a reused container the log message of a previous run +// could otherwise report readiness before the current process accepts connections +// (https://github.com/testcontainers/testcontainers-go/issues/3671). The probe prefers the +// unix socket, falls back to TCP on loopback honoring PGPORT, needs no valid credentials, +// and is skipped on images that do not ship pg_isready, preserving the previous behavior. // - It will then wait for docker to actually serve the port on localhost. // For non-linux OSes like Mac and Windows, Docker or Rancher Desktop will have to start a separate proxy. // Without this, the tests will be flaky on those OSes! @@ -18,6 +24,10 @@ func BasicWaitStrategies() testcontainers.CustomizeRequestOption { // First, we wait for the container to log readiness twice. // This is because it will restart itself after the first startup. wait.ForLog("database system is ready to accept connections").WithOccurrence(2), + // 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}"`}), // Then, we wait for docker to actually serve the port on localhost. // For non-linux OSes like Mac and Windows, Docker or Rancher Desktop will have to start a separate proxy. // Without this, the tests will be flaky on those OSes! diff --git a/modules/postgres/wait_strategies_test.go b/modules/postgres/wait_strategies_test.go new file mode 100644 index 0000000000..b6e9ff2005 --- /dev/null +++ b/modules/postgres/wait_strategies_test.go @@ -0,0 +1,107 @@ +package postgres_test + +import ( + "context" + "database/sql" + "fmt" + "io" + "strings" + "testing" + "time" + + _ "github.com/jackc/pgx/v5/stdlib" + "github.com/stretchr/testify/require" + + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/modules/postgres" +) + +// TestBasicWaitStrategies_reusedContainer reproduces the false-positive ready +// signal reported in https://github.com/testcontainers/testcontainers-go/issues/3671: +// container logs survive restarts, so on a reused container a log-based wait +// strategy can be satisfied by the output of a previous run and unblock before +// the current postgres process accepts connections. +func TestBasicWaitStrategies_reusedContainer(t *testing.T) { + ctx := context.Background() + + reuseName := fmt.Sprintf("postgres-reused-wait-%d", time.Now().UnixNano()) + + run := func() *postgres.PostgresContainer { + t.Helper() + ctr, err := postgres.Run(ctx, "postgres:16-alpine", + postgres.WithDatabase(dbname), + postgres.WithUsername(user), + postgres.WithPassword(password), + postgres.BasicWaitStrategies(), + testcontainers.WithReuseByName(reuseName), + ) + testcontainers.CleanupContainer(t, ctr) + require.NoError(t, err) + return ctr + } + + connect := func(c *postgres.PostgresContainer) *sql.DB { + t.Helper() + connStr, err := c.ConnectionString(ctx, "sslmode=disable") + require.NoError(t, err) + db, err := sql.Open("pgx", connStr) + require.NoError(t, err) + t.Cleanup(func() { db.Close() }) + return db + } + + // recoveries counts how many crash recoveries the container has logged. + // Logs accumulate across restarts of the same container, which is the very + // property that makes log-based waits unsafe with reuse. + recoveries := func(c *postgres.PostgresContainer) int { + t.Helper() + rc, err := c.Logs(ctx) + require.NoError(t, err) + defer rc.Close() + logs, err := io.ReadAll(rc) + require.NoError(t, err) + return strings.Count(string(logs), "database system was not properly shut down") + } + + const rowCount = 3_000_000 + + ctr := run() + + db := connect(ctr) + _, err := db.ExecContext(ctx, "CREATE TABLE reuse_wait (v int)") + require.NoError(t, err) + + for i := 0; i < 2; i++ { + // Generate WAL so that the unclean restart below forces crash recovery, + // during which postgres does not accept connections yet. + _, err = db.ExecContext(ctx, "TRUNCATE reuse_wait") + require.NoError(t, err) + _, err = db.ExecContext(ctx, fmt.Sprintf("INSERT INTO reuse_wait SELECT generate_series(1, %d)", rowCount)) + require.NoError(t, err) + + // Stop with a zero timeout follows SIGTERM with an immediate SIGKILL. + // The session above is kept open on purpose: postgres waits for it on + // SIGTERM, so the SIGKILL always interrupts an unclean shutdown and the + // next start is guaranteed to run crash recovery. + noGrace := time.Duration(0) + require.NoError(t, ctr.Stop(ctx, &noGrace)) + + ctr = run() + + // The wait strategy must not unblock before postgres accepts connections: + // a single attempt with no retries must succeed. + db = connect(ctr) + var rows int + require.NoErrorf(t, + db.QueryRowContext(ctx, "SELECT count(*) FROM reuse_wait").Scan(&rows), + "reuse cycle %d: container reported ready before postgres accepted connections", i, + ) + require.Equal(t, rowCount, rows) + + // Guard the reproducer itself: every cycle must have gone through crash + // recovery, otherwise the scenario above degraded silently. + require.Equalf(t, i+1, recoveries(ctr), + "reuse cycle %d: expected the restart to run crash recovery", i, + ) + } +} From 91cf3eeb780ed0789a2f7741a9ae91316cbe896d Mon Sep 17 00:00:00 2001 From: aayushcodex17 Date: Fri, 17 Jul 2026 00:33:08 +0530 Subject: [PATCH 2/4] chore: add test harness and Dockerfile for offline evaluation - 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. --- Dockerfile | 13 ++++++++++++ test.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 Dockerfile create mode 100755 test.sh diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 0000000000..53c1ad4d48 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,13 @@ +FROM public.ecr.aws/d3j8x8q7/olympus-base-go:latest + +WORKDIR /app + +COPY . . + +# Download dependencies for the root module and the postgres module. +# modules/postgres/go.mod has a replace directive pointing to the root (../../..), +# so the root module must be present and downloaded first for offline compilation. +RUN go mod download && \ + cd modules/postgres && go mod download + +CMD ["/bin/bash"] diff --git a/test.sh b/test.sh new file mode 100755 index 0000000000..f57cb3f8f3 --- /dev/null +++ b/test.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +set -euo pipefail + +OUTPUT_PATH="" +MODE="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --output_path) + OUTPUT_PATH="$2" + shift 2 + ;; + base|new) + MODE="$1" + shift + ;; + *) + shift + ;; + esac +done + +if [[ -z "$OUTPUT_PATH" ]]; then + echo "Error: --output_path is required" >&2 + exit 1 +fi + +if [[ -z "$MODE" ]]; then + echo "Error: mode (base or new) is required" >&2 + exit 1 +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" + ;; + *) + echo "Error: mode must be 'base' or 'new'" >&2 + exit 1 + ;; +esac From e7af3a0a59c99335ddc331f5bee6bd8aa136e91a Mon Sep 17 00:00:00 2001 From: aayushcodex17 Date: Fri, 17 Jul 2026 00:53:07 +0530 Subject: [PATCH 3/4] chore: replace test.sh with patch content containing diff markers --- test.sh | 237 ++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 178 insertions(+), 59 deletions(-) diff --git a/test.sh b/test.sh index f57cb3f8f3..3135d6e0ac 100755 --- a/test.sh +++ b/test.sh @@ -1,59 +1,178 @@ -#!/usr/bin/env bash -set -euo pipefail - -OUTPUT_PATH="" -MODE="" - -while [[ $# -gt 0 ]]; do - case "$1" in - --output_path) - OUTPUT_PATH="$2" - shift 2 - ;; - base|new) - MODE="$1" - shift - ;; - *) - shift - ;; - esac -done - -if [[ -z "$OUTPUT_PATH" ]]; then - echo "Error: --output_path is required" >&2 - exit 1 -fi - -if [[ -z "$MODE" ]]; then - echo "Error: mode (base or new) is required" >&2 - exit 1 -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" - ;; - *) - echo "Error: mode must be 'base' or 'new'" >&2 - exit 1 - ;; -esac +diff --git a/modules/postgres/wait_strategies_test.go b/modules/postgres/wait_strategies_test.go +new file mode 100644 +index 0000000..b6e9ff2 +--- /dev/null ++++ b/modules/postgres/wait_strategies_test.go +@@ -0,0 +1,107 @@ ++package postgres_test ++ ++import ( ++ "context" ++ "database/sql" ++ "fmt" ++ "io" ++ "strings" ++ "testing" ++ "time" ++ ++ _ "github.com/jackc/pgx/v5/stdlib" ++ "github.com/stretchr/testify/require" ++ ++ "github.com/testcontainers/testcontainers-go" ++ "github.com/testcontainers/testcontainers-go/modules/postgres" ++) ++ ++// TestBasicWaitStrategies_reusedContainer reproduces the false-positive ready ++// signal reported in https://github.com/testcontainers/testcontainers-go/issues/3671: ++// container logs survive restarts, so on a reused container a log-based wait ++// strategy can be satisfied by the output of a previous run and unblock before ++// the current postgres process accepts connections. ++func TestBasicWaitStrategies_reusedContainer(t *testing.T) { ++ ctx := context.Background() ++ ++ reuseName := fmt.Sprintf("postgres-reused-wait-%d", time.Now().UnixNano()) ++ ++ run := func() *postgres.PostgresContainer { ++ t.Helper() ++ ctr, err := postgres.Run(ctx, "postgres:16-alpine", ++ postgres.WithDatabase(dbname), ++ postgres.WithUsername(user), ++ postgres.WithPassword(password), ++ postgres.BasicWaitStrategies(), ++ testcontainers.WithReuseByName(reuseName), ++ ) ++ testcontainers.CleanupContainer(t, ctr) ++ require.NoError(t, err) ++ return ctr ++ } ++ ++ connect := func(c *postgres.PostgresContainer) *sql.DB { ++ t.Helper() ++ connStr, err := c.ConnectionString(ctx, "sslmode=disable") ++ require.NoError(t, err) ++ db, err := sql.Open("pgx", connStr) ++ require.NoError(t, err) ++ t.Cleanup(func() { db.Close() }) ++ return db ++ } ++ ++ // recoveries counts how many crash recoveries the container has logged. ++ // Logs accumulate across restarts of the same container, which is the very ++ // property that makes log-based waits unsafe with reuse. ++ recoveries := func(c *postgres.PostgresContainer) int { ++ t.Helper() ++ rc, err := c.Logs(ctx) ++ require.NoError(t, err) ++ defer rc.Close() ++ logs, err := io.ReadAll(rc) ++ require.NoError(t, err) ++ return strings.Count(string(logs), "database system was not properly shut down") ++ } ++ ++ const rowCount = 3_000_000 ++ ++ ctr := run() ++ ++ db := connect(ctr) ++ _, err := db.ExecContext(ctx, "CREATE TABLE reuse_wait (v int)") ++ require.NoError(t, err) ++ ++ for i := 0; i < 2; i++ { ++ // Generate WAL so that the unclean restart below forces crash recovery, ++ // during which postgres does not accept connections yet. ++ _, err = db.ExecContext(ctx, "TRUNCATE reuse_wait") ++ require.NoError(t, err) ++ _, err = db.ExecContext(ctx, fmt.Sprintf("INSERT INTO reuse_wait SELECT generate_series(1, %d)", rowCount)) ++ require.NoError(t, err) ++ ++ // Stop with a zero timeout follows SIGTERM with an immediate SIGKILL. ++ // The session above is kept open on purpose: postgres waits for it on ++ // SIGTERM, so the SIGKILL always interrupts an unclean shutdown and the ++ // next start is guaranteed to run crash recovery. ++ noGrace := time.Duration(0) ++ require.NoError(t, ctr.Stop(ctx, &noGrace)) ++ ++ ctr = run() ++ ++ // The wait strategy must not unblock before postgres accepts connections: ++ // a single attempt with no retries must succeed. ++ db = connect(ctr) ++ var rows int ++ require.NoErrorf(t, ++ db.QueryRowContext(ctx, "SELECT count(*) FROM reuse_wait").Scan(&rows), ++ "reuse cycle %d: container reported ready before postgres accepted connections", i, ++ ) ++ require.Equal(t, rowCount, rows) ++ ++ // Guard the reproducer itself: every cycle must have gone through crash ++ // recovery, otherwise the scenario above degraded silently. ++ require.Equalf(t, i+1, recoveries(ctr), ++ "reuse cycle %d: expected the restart to run crash recovery", i, ++ ) ++ } ++} +diff --git a/test.sh b/test.sh +new file mode 100755 +index 0000000..f57cb3f +--- /dev/null ++++ b/test.sh +@@ -0,0 +1,59 @@ ++#!/usr/bin/env bash ++set -euo pipefail ++ ++OUTPUT_PATH="" ++MODE="" ++ ++while [[ $# -gt 0 ]]; do ++ case "$1" in ++ --output_path) ++ OUTPUT_PATH="$2" ++ shift 2 ++ ;; ++ base|new) ++ MODE="$1" ++ shift ++ ;; ++ *) ++ shift ++ ;; ++ esac ++done ++ ++if [[ -z "$OUTPUT_PATH" ]]; then ++ echo "Error: --output_path is required" >&2 ++ exit 1 ++fi ++ ++if [[ -z "$MODE" ]]; then ++ echo "Error: mode (base or new) is required" >&2 ++ exit 1 ++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" ++ ;; ++ *) ++ echo "Error: mode must be 'base' or 'new'" >&2 ++ exit 1 ++ ;; ++esac From 0c7e5529b4de0f92e61479ed2b0f78251c5f5ca8 Mon Sep 17 00:00:00 2001 From: aayushcodex17 Date: Fri, 17 Jul 2026 01:09:49 +0530 Subject: [PATCH 4/4] fix: valid patch format in test.sh, Dockerfile layer caching and build 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 --- Dockerfile | 14 +++++++++++--- test.sh | 12 +++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/Dockerfile b/Dockerfile index 53c1ad4d48..d4439790e6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,12 +2,20 @@ FROM public.ecr.aws/d3j8x8q7/olympus-base-go:latest WORKDIR /app -COPY . . +# Copy dependency manifests first to cache the download layer separately from source. +COPY go.mod go.sum ./ +COPY modules/postgres/go.mod modules/postgres/go.sum modules/postgres/ -# Download dependencies for the root module and the postgres module. +# Download all dependencies while network is available. # modules/postgres/go.mod has a replace directive pointing to the root (../../..), -# so the root module must be present and downloaded first for offline compilation. +# so root dependencies must be downloaded first. RUN go mod download && \ cd modules/postgres && go mod download +# Copy full source and pre-compile to populate the build cache. +COPY . . + +RUN go build ./... && \ + cd modules/postgres && go build ./... + CMD ["/bin/bash"] diff --git a/test.sh b/test.sh index 3135d6e0ac..4449d50891 100755 --- a/test.sh +++ b/test.sh @@ -113,12 +113,12 @@ index 0000000..b6e9ff2 +} diff --git a/test.sh b/test.sh new file mode 100755 -index 0000000..f57cb3f +index 0000000..2fb53a5 --- /dev/null +++ b/test.sh -@@ -0,0 +1,59 @@ +@@ -0,0 +1,53 @@ +#!/usr/bin/env bash -+set -euo pipefail ++set -uo pipefail + +OUTPUT_PATH="" +MODE="" @@ -154,18 +154,12 @@ index 0000000..f57cb3f + +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 \