Skip to content

Fix WaitAllStrategy child timeout preservation in maximum outer timeout mode - #12101

Open
ys0v9 wants to merge 1 commit into
testcontainers:mainfrom
ys0v9:fix/waitall-preserve-child-timeouts
Open

ys0v9 wants to merge 1 commit into
testcontainers:mainfrom
ys0v9:fix/waitall-preserve-child-timeouts

Conversation

@ys0v9

@ys0v9 ys0v9 commented Oct 4, 2026 •

Copy link
Copy Markdown

Summary

WaitAllStrategy currently overwrites existing child strategy timeouts when withStartupTimeout(...) is called in WITH_MAXIMUM_OUTER_TIMEOUT mode. Setting the outer timeout before adding the children preserves their timeouts, making the behavior depend on configuration order.

Restrict timeout propagation in withStartupTimeout(...) to WITH_OUTER_TIMEOUT, matching withStrategy(...). This preserves individual child timeouts in maximum outer timeout mode while continuing to update the overall timeout.

The default mode's timeout propagation and the restriction on changing the timeout in WITH_INDIVIDUAL_TIMEOUTS_ONLY remain unchanged. No public API changes are introduced.

Regression coverage

  • Preserve child timeouts with either configuration order.
  • Preserve distinct child timeouts when the outer timeout is changed.
  • Allow a nested WITH_INDIVIDUAL_TIMEOUTS_ONLY strategy without attempting to overwrite its timeout.
  • Preserve the existing behavior of the other modes.

The new tests do not require Docker or depend on elapsed time.

Validation

  • Confirmed the bug-detecting regression tests fail before the fix and pass afterward: 12 tests, 3 failures before the fix; all 12 pass after it.
  • ./gradlew :testcontainers:test --tests org.testcontainers.containers.wait.strategy.WaitAllStrategyTest --rerun-tasks
  • ./gradlew checkstyleMain checkstyleTest spotlessApply
  • git diff --check

Fixes #12100

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3501a400-05bb-4d78-b5f7-9f27eee2054b
📥 Commits

Reviewing files that changed from the base of the PR and between 8e54951 and 2519cef.

📒 Files selected for processing (2)
  • core/src/main/java/org/testcontainers/containers/wait/strategy/WaitAllStrategy.java
  • core/src/test/java/org/testcontainers/containers/wait/strategy/WaitAllStrategyTest.java

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

WaitAllStrategy.withStartupTimeout now propagates the timeout to existing child strategies only in WITH_OUTER_TIMEOUT mode. Tests cover child timeout preservation and nested strategy acceptance in other modes.

Changes

WaitAllStrategy timeout behavior

Layer / File(s) Summary
Mode-limited timeout propagation and regression tests
core/src/main/java/org/testcontainers/containers/wait/strategy/WaitAllStrategy.java, core/src/test/java/org/testcontainers/containers/wait/strategy/WaitAllStrategyTest.java
withStartupTimeout updates existing child strategies only in WITH_OUTER_TIMEOUT mode. Tests check that child timeouts remain unchanged when the outer timeout is set before or after adding children, when it is updated, and when a nested WITH_INDIVIDUAL_TIMEOUTS_ONLY strategy is added.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: eddumelendez

Merge Risk: ⚪ Minimal · up to 2519c

The timeout change appears ready for normal validation; no merge-blocking issue was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #12100 requires child timeouts to remain unchanged in WITH_MAXIMUM_OUTER_TIMEOUT mode, regardless of configuration order. WaitAllStrategy.withStartupTimeout now propagates timeout changes only i…
Out of Scope Changes check ✅ Passed The change summary identifies modifications only to WaitAllStrategy timeout propagation and its focused tests. These changes directly support issue #12100. The whole diff read was unavailable, but the…
Title check ✅ Passed The title clearly identifies the fix and the affected behavior: preserving child timeouts in maximum outer timeout mode.
Description check ✅ Passed The description explains the bug, the fix, regression coverage, validation results, and linked issue. It provides the required context for the change.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ys0v9
ys0v9 marked this pull request as ready for review October 4, 2026 06:42
@ys0v9
ys0v9 requested a review from a team as a code owner October 4, 2026 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: WaitAllStrategy WITH_MAXIMUM_OUTER_TIMEOUT overwrites child timeouts depending on configuration order

1 participant