Reuse insecure HTTP wait agent across retries#1382
Conversation
HttpWaitStrategy.getAgent() constructed a new undici Agent on every retry attempt when allowInsecure() was set (once per readTimeoutMs) and never disposed it, leaking agents and sockets for slow-starting containers. Memoize the insecure Agent so it is created once and reused, and close it in a finally block once the wait completes (success or failure). Also rename the misleading withReadTimeout() parameter from startupTimeoutMs to readTimeoutMs to match the field it assigns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cristianrgreco
added
bug
✅ Deploy Preview for testcontainers-node 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: d4f2190472
ℹ️ 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".
| if (this.insecureAgent) { | ||
| const agent = this.insecureAgent; | ||
| this.insecureAgent = undefined; | ||
| await agent.close(); |
There was a problem hiding this comment.
When allowInsecure() is used and the health endpoint returns a passing status (or a non-matching status) with a large or streaming body, the status-only paths above never consume or cancel the undici.request body. This new graceful Agent.close() waits for outstanding requests to complete, so waitUntilReady can hang after the predicate has already succeeded or after the timeout path is taken; drain/cancel the response body or force-close the dispatcher before awaiting shutdown.
Useful? React with 👍 / 👎.
| vi.doMock("../container-runtime", async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import("../container-runtime")>(); | ||
| return { ...actual, getContainerRuntimeClient: async () => client }; |
There was a problem hiding this comment.
This vi.doMock is registered after the file's static Wait import has already loaded HttpWaitStrategy through ./wait, and vi.doMock only affects subsequent imports rather than already-cached modules. As a result, these supposedly Docker-free lifecycle tests still use the real getContainerRuntimeClient and will try to initialize a real container runtime in environments without Docker; reset modules/remove the eager import before dynamically importing the strategy under the mock.
Useful? React with 👍 / 👎.
Address review feedback on the insecure HTTP wait Agent:
- Use Agent.destroy() instead of close() on teardown. Status-only
predicate paths never consume the response body, so a graceful close()
could hang waiting for those connections to drain. The wait has already
finished at that point, so a forceful destroy is correct.
- Move the agent-lifecycle tests into their own file with hoisted,
top-level mocks for undici and the container runtime, importing
HttpWaitStrategy directly. Previously the vi.doMock("../container-runtime")
ran after the static "./wait" import had already cached HttpWaitStrategy,
so the mock was a no-op and the "Docker-free" tests still required a real
container runtime. The dedicated file makes them genuinely Docker-free.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b325235e22
ℹ️ 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".
| private readonly predicates: Array<(response: Response) => Promise<boolean>> = []; | ||
| private _allowInsecure = false; | ||
| private readTimeoutMs = 1000; | ||
| private insecureAgent?: Agent; |
There was a problem hiding this comment.
When a single HttpWaitStrategy instance is shared, this instance field makes concurrent waitUntilReady calls share the same dispatcher. DockerComposeEnvironment.up() runs waits in parallel with Promise.all and passes the same defaultWaitStrategy object to every service (docker-compose-environment.ts:169-181), so a compose setup using a default HTTPS allowInsecure() wait can have the first service that becomes ready call closeAgent() and destroy the dispatcher while another service is still awaiting or reading its response, causing that other wait to retry, fail, or time out. Scope the insecure agent to each waitUntilReady invocation instead of storing it on the reusable strategy object.
Useful? React with 👍 / 👎.
A single HttpWaitStrategy instance can drive multiple concurrent waitUntilReady calls — DockerComposeEnvironment.up() runs waits in parallel and passes the same defaultWaitStrategy object to every service. Memoizing the insecure Agent on the instance meant those concurrent waits shared one dispatcher, so the first to finish would destroy() it while others were still using it. Create the insecure Agent as a local in waitUntilReady (still built once per wait and reused across that wait's retries) and destroy it in the same call's finally. Adds a regression test asserting concurrent waits on a shared strategy each get their own agent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Two changes to
HttpWaitStrategy:Agentleak —getAgent()constructed a newundici.Agenton every retry attempt whenallowInsecure()was set (once perreadTimeoutMs, default 1000ms) and never disposed it, leaking agents/sockets for slow-starting containers. The insecureAgentis now memoized (created once, reused across retries) and closed in afinallyblock when the wait completes — on both success and failure.withReadTimeout(startupTimeoutMs)is renamed towithReadTimeout(readTimeoutMs)to match the field it assigns (startupTimeoutMsis a different concept used elsewhere).The large source diff is mostly re-indentation from wrapping the existing retry loop in
try/finally; the behavioural change is small.Verification
describe.sequential("agent lifecycle")block: asserts exactly oneAgentis constructed across multiple retries and that it isclosedafterwards, and that noAgentis constructed whenallowInsecure()is not set. Runs without Docker (mockedundici.request+ container-runtime client).Agentconstructions across retries; post-fix exactly 1.npm run check-compiles→ clean ·npm run lint→ clean · focused new tests pass.Test results
2 new tests passing. The existing Docker-backed tests in the same file were not run in this environment (no container runtime) and are unaffected — the module-level
undicimock leavesrequestdefaulting to the real implementation.Not breaking
withReadTimeoutkeeps the same arity and type (only the parameter name changes — not part of the call signature). The Agent change is internal. No public API change.🤖 Generated with Claude Code