feat: allow overriding the session ID - #3051
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @jcmfernandes, I am actually surprised your setup doesn't work with the current Test Session Semantics. Also, your change doesn't take the parent PID into account anymore, so it would be a breaking change for downstream integrations, that rely on the current session ID semantics. |
Thanks for getting back to me. Without these changes, every As stated in the PR description:
I'm happy to revert those changes, but so far, setting the session ID myself (through an env var) has been the only solution that worked for me. |
Alright, that is how it is supposed to work. Either way, the semantics around sessions IDs and cleanup behavior are fairly involved. I'd suggest to not proceed with this work without specifically syncing with @mdelapenya and @stevenh on the design. |
Thanks for the pointers. @mdelapenya and @stevenh, your input is very much welcome.
I am using a reusable container, and the container was always reused as expected; no issues there. The problem is that the reaper isn't reused because the session ID keeps changing. |
I pushed a new commit that brings back the logic I had previously removed. This shouldn't break things for anyone anymore. |
|
Hi @jcmfernandes can you clarify to me the problem you're trying to solve? Your description seems to indicate to me you want to share containers between tests due to the slow startup time? |
Hi @stevenh! Correct. In other words, I want:
Copying from #2804, this is what I see when I enable reusable containers:
I want to see one reaper for the reusable container. As a side effect, having one reaper creates a “rolling window” behavior, where the reaper resets its timeout upon a new connection to the container, which is precisely what I would like to see: keep the container up and running between test runs and terminate it after the defined period of inactivity. Am I making sense? |
|
Oh yes, that makes absolutely sense and I believe this is more of a bug and something that Testcontainers should already handle correctly, rather than having a user to specify a session id manually. |
|
Thanks for the confirmation @jcmfernandes. To facilitate this are you intending to set a long |
|
@stevenh yes, because I want to keep the testcontainer running for the entirety of my work session. I usually start a watcher that runs tests automatically as I save changes. I want the testcontainer to boot on the first test run and to stay around for, say, one hour, waiting for new connections. Furthermore, I then want new connections to reset that timeout. Example.
This is precisely the behavior I get by manually setting the session ID and |
|
Thanks again for the clarification, the challenge I see with that is not only does it impact the long lived container but all containers for example: You run a test which starts the long lived DB container as well a short lived test container. The desired behaviour is the DB container stays running between tests, but the test container itself exits after the test is run. However in the case where the test fails and the short lived contain didn't exit as expected, with a standard ryuk setup this wouldn't be an issue as the test container would be quickly cleaned up, but with Does this make sense? To handle this situation properly I think we'd need more fine grained control of when specific containers are cleaned up. |
|
Thanks for the prompt reply, @stevenh. That's a fair concern — it seems that testcontainers is missing some kind of "group" concept to which certain GC options apply. Currently, that's a "session", and this PR won't change it. That is, I don't see how this PR makes things worse regarding your concern, and it solves an actual reported issue. |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
Hi @jcmfernandes, thanks for opening this PR, and following-up after @kiview's comments. I'm returning today after summer break, working on the open PRs/issues in the repo.
I'm not against the changes in this PR, although it introduces some breaking changes (BCs) after removing public API functions. See my comments for that. We could avoid it offering a deprecation path when/if possible, although we sometimes introduce BCs if needed.
I'd rename the PR to feat: allow overriding the sessionID and would focus on that, only. The added value if we do this is we would also enable Bazel folks to contribute their own env var for the session ID at build time. Nevertheless, we must check how the reaper behaves in that scenario. As discussed here, it's by design that the reaper is spawned just once per test session (a shell), but it's true that the design could be inconsistent with reusable containers used in multiple shells (different session IDs). Sharing state, even in tests, is a complex thing to maintain, so we probably need a more robust design for both features: reusable containers and the reaper, and it could need dedicated work on that direction.
607c474 to
dc868e2
Compare
|
@mdelapenya thanks for your input. I modified this PR, making it focus solely on allowing devs to set the session ID, and I opened a separate PR #3269 where I delete |
jcmfernandes
changed the title
mdelapenya
added
the
feature
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
I'm concerned calling config.Read in init could have some unintended side effects which are impossible to address, something we'll need to confirm.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughSession ID resolution now uses shared configuration. Configuration reads ChangesSession ID centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
testcontainers.go (1)
32-55: UpdateSessionIDdoc comment to match config‑based implementation
SessionID()now delegates toconfig.Read().SessionID, so the detailed description of how the ID is derived (parent PID + creation time, hashing, etc.) is no longer local to this function and may be overridden via config/env. Consider rewording the comment to describe it as “the current configured session ID” and, if needed, point to the bootstrap/config docs for the generation details.docker.go (1)
924-932: Reaper now uses provider‑level SessionID; consider container ID if you ever support per‑container sessionsUsing
c.provider.config.SessionIDwhen callingspawner.reaperaligns reaper lifetime with the configured, process‑wide session ID and matches the new config‑centric model. If, in the future, you want to honor per‑containerLabelSessionIDoverrides consistently, this call would likely need to switch toc.sessionIDinstead; for the current design, though, the change is coherent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
container.go(2 hunks)docker.go(1 hunks)docker_client.go(3 hunks)generic.go(2 hunks)internal/config/config.go(3 hunks)internal/config/config_test.go(36 hunks)internal/core/bootstrap/bootstrap.go(1 hunks)internal/core/client.go(2 hunks)network.go(2 hunks)reaper_test.go(1 hunks)testcontainers.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T15:08:18.694Z
Learnt from: mdelapenya
Repo: testcontainers/testcontainers-go PR: 3320
File: modules/artemis/artemis.go:98-103
Timestamp: 2025-09-29T15:08:18.694Z
Learning: In testcontainers-go, nat.Port is a type alias for string, so untyped string constants can be passed directly to functions expecting nat.Port (like wait.ForListeningPort) without explicit type conversion - the Go compiler handles the implicit conversion automatically.
Applied to files:
network.go
🧬 Code graph analysis (10)
container.go (3)
internal/config/config.go (1)
Read(103-109)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
internal/core/client.go (2)
internal/core/bootstrap/bootstrap.go (2)
ProjectPath(100-102)SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
internal/config/config.go (2)
internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
reaper_test.go (3)
internal/config/config.go (1)
Read(103-109)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
network.go (3)
internal/config/config.go (1)
Read(103-109)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
internal/config/config_test.go (2)
internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)internal/config/config.go (1)
Config(34-97)
docker.go (3)
internal/core/docker_host.go (1)
DockerHostContextKey(19-19)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
generic.go (4)
internal/core/labels.go (1)
DefaultLabels(38-51)internal/config/config.go (1)
Read(103-109)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)testcontainers.go (1)
SessionID(53-55)
docker_client.go (2)
internal/config/config.go (2)
Config(34-97)Read(103-109)internal/core/bootstrap/bootstrap.go (2)
SessionID(104-106)ProcessID(96-98)
testcontainers.go (2)
internal/config/config.go (1)
Read(103-109)internal/core/bootstrap/bootstrap.go (1)
SessionID(104-106)
🔇 Additional comments (9)
internal/core/bootstrap/bootstrap.go (1)
1-105: Package rename tobootstraplooks safeExports and init semantics are unchanged; renaming the package to
bootstrapaligns with the new import sites and keeps behavior intact.internal/core/client.go (1)
11-38: HTTP headers now correctly use config‑driven session IDUsing
bootstrap.ProjectPath()andtcConfig.SessionIDwires the Docker client headers into the same bootstrap/config session machinery used elsewhere; this is consistent and non‑breaking.reaper_test.go (1)
115-124: Session ID lookup in test aligned with configFetching the session via
config.Read().SessionIDkeeps this test in sync with how the provider/reaper obtain the session ID after the refactor; the lookup logic remains correct.container.go (1)
25-28: Container session ID now correctly derives from config
ContainerRequest.sessionID()still respects an explicitLabelSessionIDbut now falls back toconfig.Read().SessionID, which is exactly what you want for a configurable, process‑wide session ID.Also applies to: 174-181
network.go (1)
55-61: Network session ID consistent with config‑driven modelAligning
NetworkRequest.sessionID()withconfig.Read().SessionID(while still honoring aLabelSessionIDoverride) keeps networks and containers on the same configurable session semantics.generic.go (1)
108-114: Generic labels now reflect configured session IDPassing
config.Read().SessionIDintocore.DefaultLabelsensures all auto‑applied labels (including the session label) follow the same configurable session ID used elsewhere.docker_client.go (1)
15-18: SessionID wiring viaconfig.ConfiginDockerClientlooks consistentThe new
configfield, use ofconfig.Read()inNewDockerClientWithOpts, and logging ofc.config.SessionIDinInfo()are all consistent with the new config‑driven session semantics. Usingbootstrap.ProcessID()here also aligns with the bootstrap accessor pattern. No functional issues spotted in this hunk.Also applies to: 26-27, 81-91, 121-130
internal/config/config.go (1)
13-14: SessionID config field and env/props precedence are correctly implementedThe new
SessionIDfield, its properties tag (session.id) and the precedence logic inapplyEnvironmentConfiguration(env → properties →bootstrap.SessionID()fallback) are coherent and match the documentedTESTCONTAINERS_SESSION_IDbehavior. This gives a stable, overridable session ID without altering existing config semantics.Also applies to: 57-63, 119-138
internal/config/config_test.go (1)
12-13: Tests thoroughly cover new SessionID behavior across env, properties, and defaultsThe additions to
resetTestEnv, the updated expectations usingbootstrap.SessionID(), and the new table entries forsession.idandTESTCONTAINERS_SESSION_IDcorrectly encode the intended precedence and defaulting rules. This should give good regression coverage for the new SessionID configuration path.Also applies to: 24-32, 47-59, 71-78, 85-102, 112-115, 126-132, 140-157, 163-167, 179-181, 220-224, 232-236, 243-247, 263-268, 275-279, 286-291, 298-301, 311-314, 325-328, 335-339, 348-352, 361-365, 374-378, 387-391, 416-420, 429-433, 458-462, 471-475, 502-505, 528-532, 541-545, 554-558, 561-582
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM! I'm approving it even though I added a comment that would be good to add.
Once addressed, I think we can re-run the CI and merge.
Thanks!
PS: Many times, we are forced to do multitasking, so please sorry for taking too long to address this.
Nothing to apologize for! Thanks, everyone, for the thorough review. 🙌 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/config/config.go`:
- Around line 133-138: Current logic always overwrites config.SessionID with
bootstrap.SessionID() when the TESTCONTAINERS_SESSION_ID env var is unset,
ignoring any value already loaded from the properties file; change the logic in
the block handling TESTCONTAINERS_SESSION_ID/sessionID so that: if the env var
is set, assign it to config.SessionID; else if config.SessionID is already
non-empty (loaded from properties) leave it untouched; otherwise set
config.SessionID = bootstrap.SessionID(). Update the code that references
sessionID, config.SessionID and bootstrap.SessionID() accordingly to implement
this three-way precedence (env -> properties -> bootstrap).
|
Some tests are failing on CI, can you check? |
159e179 to
01dc605
Compare
After all, that "else if" statement is necessary, as the AI bot correctly pointed out. I deleted the commit. |
1fb204d to
159e179
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docker.go (4)
845-859: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep log-production state per run.
startLogProductionoverwrites shared fieldslogProductionCtx,c.logProductionCancel,c.logProductionDone, andc.logProductionTimeoutwhile an existingc.logProducermight still read them.stopLogProductioncancels and waits only on the latest run, so an earlier run can continue. Store each run in a synchronized immutable run object and pass that run’s context and timeout intoc.logProducer. Cancel and wait for the active run before replacing it. Add a race-enabled test that starts and stops log production concurrently.🤖 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 `@docker.go` around lines 845 - 859, The startLogProduction flow must keep context, cancel, completion, and timeout state scoped to an immutable per-run object instead of overwriting shared fields while a producer is active. Update startLogProduction and stopLogProduction to synchronize access, cancel and await the current run before replacing it, and pass that run’s context and timeout directly into c.logProducer; add a race-enabled test covering concurrent start and stop operations.
596-612: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound buffered
Execoutput.
DockerContainer.Execbuffers the entire attach stream inbytes.Bufferbefore returning. A command that keeps producing stdout/stderr can exhaust process memory before the caller reads the result. Add a configurable output limit and return a defined error when the limit is exceeded; verify this with an exec command that emits more than the configured limit.🤖 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 `@docker.go` around lines 596 - 612, Update DockerContainer.Exec’s attach-stream handling around the io.Copy goroutine to write through a bounded reader or limited buffer, using a configurable maximum output size. Detect when the limit is exceeded, return a defined limit-exceeded error instead of continuing to buffer, preserve context-cancellation behavior, and add coverage with an exec command that emits more than the configured limit.
1383-1393: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEscape the literal container name in the Docker name filter.
findContainerByNameanchorsnamein a regex, socache.1also matchescacheX1via the dot wildcard.ReuseOrCreateContainercan then return that wrong container. Appendregexp.QuoteMetato the filter value.Proposed fix
- Filters: make(client.Filters).Add("name", fmt.Sprintf("^%s$", name)), + Filters: make(client.Filters).Add("name", fmt.Sprintf("^%s$", regexp.QuoteMeta(name))),Add a reuse test with two confusable names and assert that only the literal name is reused.
🤖 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 `@docker.go` around lines 1383 - 1393, Update findContainerByName’s Docker name filter to escape the literal name with regexp.QuoteMeta before applying the existing anchors, preventing regex metacharacters from matching confusable container names. Add a reuse test covering two names that differ only by a regex metacharacter and verify ReuseOrCreateContainer reuses only the exact literal name.
222-224: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
HostPortparsing failures inMappedPort.
MappedPortdiscards errors from parsingp[0].HostPortand constructing the returnednetwork.Port, so invalid or out-of-range bindings can be reported as a successful mapping or a zero/overflown port. Return and wrap the parse/construct errors instead.🤖 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 `@docker.go` around lines 222 - 224, Update MappedPort’s HostPort handling to check and return errors from strconv.ParseUint and network.PortFrom instead of discarding them. Wrap each error with contextual information while preserving the successful mapped-port return path.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@docker.go`:
- Around line 845-859: The startLogProduction flow must keep context, cancel,
completion, and timeout state scoped to an immutable per-run object instead of
overwriting shared fields while a producer is active. Update startLogProduction
and stopLogProduction to synchronize access, cancel and await the current run
before replacing it, and pass that run’s context and timeout directly into
c.logProducer; add a race-enabled test covering concurrent start and stop
operations.
- Around line 596-612: Update DockerContainer.Exec’s attach-stream handling
around the io.Copy goroutine to write through a bounded reader or limited
buffer, using a configurable maximum output size. Detect when the limit is
exceeded, return a defined limit-exceeded error instead of continuing to buffer,
preserve context-cancellation behavior, and add coverage with an exec command
that emits more than the configured limit.
- Around line 1383-1393: Update findContainerByName’s Docker name filter to
escape the literal name with regexp.QuoteMeta before applying the existing
anchors, preventing regex metacharacters from matching confusable container
names. Add a reuse test covering two names that differ only by a regex
metacharacter and verify ReuseOrCreateContainer reuses only the exact literal
name.
- Around line 222-224: Update MappedPort’s HostPort handling to check and return
errors from strconv.ParseUint and network.PortFrom instead of discarding them.
Wrap each error with contextual information while preserving the successful
mapped-port return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87fedd03-7ccd-4ab4-80a2-d46657e65d3f
📒 Files selected for processing (8)
container.godocker.godocker_client.gogeneric.gointernal/config/config.gointernal/config/config_test.gointernal/core/bootstrap/bootstrap.gointernal/core/client.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/core/bootstrap/bootstrap.go
- container.go
- internal/core/client.go
- generic.go
- docker_client.go
|
@mdelapenya mind giving this a final look? Thank you so much. |
|
@jcmfernandes can you rebase from current main 🙏 Fine if you push-force, as the changeset's size is reduced BTW I'm cutting a new release soon, and this work will land into it (sorry for the late review 😞 ) |
f2c1769 to
bac8fdd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
bac8fdd to
f6b6820
Compare
|
Rebased @mdelapenya, thank you! 🙌 I opened #3837 as it was raised the LLM code review added to this branch and it's a genuine bug. |
|
@jcmfernandes for some reason I cannot update your PR branches. Could you update them yourself? I already fixed the lint error for the azurite module |
f6b6820 to
3b15335
Compare
Rebased on main. |
|
CI fails, could you take a look? |
3b15335 to
d3faef8
Compare
Done. It was something wonky that stem from the review but truly is necessary. |
4 participants
What does this PR do?
Makes the session ID configurable, allowing for long-running sessions.
Why is it important?
Speaking of my use-case, Apache Pulsar takes a long time to boot, so I wanted testcontainers to boot it up, eventually take it down, but allow me to use the same container across several runs. This wasn't the case, since I was getting a new reaper running on every test run. I wanted a rolling-window behavior, hence this change.
Related issues
reuseWindowfor cleaning up reusable containers if no tests run within a certain duration #2804Follow-ups
Update the documentation.