fix(postgres): make BasicWaitStrategies safe with container reuse - #3807
Draft
malikov73 wants to merge 1 commit into
Draft
fix(postgres): make BasicWaitStrategies safe with container reuse#3807malikov73 wants to merge 1 commit into
malikov73 wants to merge 1 commit into
Conversation
Container logs survive restarts, so on a reused container the log-based wait could be satisfied by the readiness messages of a previous run and unblock before the current postgres process accepts connections. Keep the log wait as the first gate and add a pg_isready probe of the live server state after it. The probe cannot match stale output, prefers the unix socket with a TCP loopback fallback honoring PGPORT, needs no valid credentials, and is skipped on images that do not ship pg_isready, preserving the previous behavior for them. On fresh starts the log gate passes only once the server is up, so the probe usually succeeds on its first attempt. Fixes testcontainers#3671
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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.
What does this PR do?
Makes
postgres.BasicWaitStrategies()safe to use with container reuse. The log-based wait (ForLog("database system is ready to accept connections").WithOccurrence(2)) stays as the first gate, and a live-state probe is added after it:The probe prefers the unix socket, falls back to TCP on loopback honoring
PGPORT, needs no valid credentials (pg_isreadyusesPQping, which does not authenticate — custom user/database/password keep working), and is skipped gracefully on images that do not shippg_isready, preserving the previous behavior for them. On fresh starts the log gate passes only once the server is up, so the probe usually succeeds on its first attempt and adds no meaningful overhead.It also adds a regression test that puts a reused container through unclean restarts:
WithReuseByName, thenStopwith a zero timeout while a client session keeps postgres from completing its shutdown checkpoint, so the next start is guaranteed to run crash recovery. The test asserts that a single, retry-free connection attempt succeeds right afterRunreturns, and that every cycle actually went through crash recovery, so the reproducer cannot degrade silently.Why is it important?
Container logs survive restarts. On a reused container the readiness lines of the previous run already satisfy the two-occurrence log wait, so
Runcan return while the current postgres process is still in crash recovery, and clients fail withFATAL: the database system is not yet accepting connections. Onmainthe added test fails consistently with exactly that error.Related issues
How to test this PR
On
mainthe test fails with "the database system is not yet accepting connections"; with this change it passes. The full module suite passes with-race, andmake lintis clean.Follow-ups
wait.ForLoguser). A lifecycle-aware log wait (reading logs only since the currentStartedAt) would fix the whole class, but occurrence counting needs rethinking there: a restarted data dir logs readiness only once, soWithOccurrence(2)would hang. This PR is intentionally scoped to the postgres module; I can follow up on the general mechanism if there is interest.DockerContainer.Execnever closes theExecAttachhijacked response, so polling exec strategies leak one daemon connection per poll; I will file that separately.