fix(infra): harden Testcontainers shutdown hooks with safeStop#190
Merged
Conversation
WalkthroughAdds defensive Testcontainers lifecycle handling to integration tests across 10 modules: container startups are wrapped in try/catch to perform best-effort cleanup on failure; shutdown hooks now call a new private Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Wrap container startup in try/catch so that if a later container fails to start, already-started containers are stopped instead of orphaned. Replace bare stop() calls in shutdown hooks with a safeStop() helper that catches exceptions, ensuring one container failure does not prevent the remaining containers from being cleaned up. Name the shutdown thread "testcontainers-shutdown" for easier diagnostics. Applied consistently across all 10 AbstractIntegrationTest files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
fix/testcontainers-safe-shutdown
branch
from
2e98280 to
1aca676
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java`:
- Around line 46-54: The safeStop helper currently swallows exceptions; change
it to keep best-effort cleanup but emit a warning when stop() fails: in
AbstractIntegrationTest.safeStop(Startable container) catch Exception e and call
the test logger (or a static logger in the class) to warn including a fixed
container label (e.g. "testcontainers") and the exception details before
returning (do not rethrow), so teardown failures are visible in CI; apply the
same change to the other copied AbstractIntegrationTest helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4c39ec5-82f6-46eb-84ff-a2106d380302
📒 Files selected for processing (10)
api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.javablockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javacompliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.javafiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.javafx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.javaledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java
Comment on lines
+46
to
54
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep teardown failures visible.
Line 51 suppresses the stop exception completely. If Docker/Testcontainers cleanup fails, leftover containers become much harder to diagnose in CI because the failure never surfaces. Keep the best-effort semantics, but emit a warning with a fixed container label before continuing. The same helper was copied into the sibling AbstractIntegrationTest classes in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java`
around lines 46 - 54, The safeStop helper currently swallows exceptions; change
it to keep best-effort cleanup but emit a warning when stop() fails: in
AbstractIntegrationTest.safeStop(Startable container) catch Exception e and call
the test logger (or a static logger in the class) to warn including a fixed
container label (e.g. "testcontainers") and the exception details before
returning (do not rethrow), so teardown failures are visible in CI; apply the
same change to the other copied AbstractIntegrationTest helpers.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java (1)
46-53:⚠️ Potential issue | 🟡 MinorMake teardown failures observable instead of silently suppressing them.
safeStopstill swallows stop failures; please emit a warning with a fixedtestcontainerslabel so CI cleanup issues remain diagnosable while keeping best-effort semantics.Suggested patch
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.testcontainers.lifecycle.Startable; @@ public abstract class AbstractIntegrationTest { + private static final Logger log = LoggerFactory.getLogger(AbstractIntegrationTest.class); @@ - private static void safeStop(Startable container) { + private static void safeStop(Startable container) { try { if (container != null) { container.stop(); } - } catch (Exception ignored) { - // best-effort cleanup + } catch (Exception ex) { + log.warn("testcontainers: failed to stop container during cleanup", ex); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java` around lines 46 - 53, The safeStop(Startable container) helper currently swallows exceptions making CI cleanup failures invisible; modify safeStop in AbstractIntegrationTest to catch Exception as now but call the test logger (or System.err) to emit a warning that includes a fixed "testcontainers" label and the caught exception details (message/stacktrace) while preserving the best-effort semantics (i.e., do not rethrow). Ensure the log message includes the container identity (e.g., container.toString() or container.getContainerId()) so failures are diagnosable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`:
- Around line 46-54: safeStop currently swallows all exceptions; add a low-noise
diagnostic log so CI flakes can be triaged: introduce or reuse an SLF4J Logger
on AbstractIntegrationTest and change the catch to catch (Exception e) {
logger.debug("Container.stop() failed (best-effort cleanup): {}",
e.getMessage()); } (or logger.warn with the message only for higher visibility)
to avoid logging sensitive payloads but preserve the exception message/context
for Startable container stop failures.
In
`@compliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.java`:
- Around line 46-53: The safeStop(Startable container) method currently swallows
all exceptions; update the catch to log minimal infra diagnostics (container
type and exception class/message) while still treating it as best-effort
cleanup: catch Exception e and emit a single-line diagnostic using your
project's logger (or System.err if no logger available) that includes
container.getClass().getSimpleName(), e.getClass().getSimpleName(), and a
trimmed e.getMessage() (avoid stack traces or payload/PII), then continue
without rethrowing; keep the existing null check and best-effort semantics
around container.stop().
- Around line 31-44: Register the shutdown hook before starting containers so it
always exists as a last-resort cleanup: move the
Runtime.getRuntime().addShutdownHook(...) call to before the
POSTGRES.start()/KAFKA.start() calls in the static initializer, then attempt to
start POSTGRES and KAFKA inside the try block; keep the catch that calls
safeStop(KAFKA) and safeStop(POSTGRES) and rethrows the exception, but the
shutdown hook (using the existing safeStop method) will now always be registered
even if startup throws. Ensure you reference the same symbols: the static
initializer, Runtime.getRuntime().addShutdownHook, POSTGRES, KAFKA, and
safeStop.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.java`:
- Around line 46-53: The safeStop method currently swallows all exceptions;
update it to log a WARN instead of discarding so teardown failures are visible:
add an SLF4J Logger (e.g., private static final Logger log =
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and in the catch block
call log.warn("Failed to stop container of type {}", container != null ?
container.getClass().getName() : "null", e) (or similar), ensuring you reference
the Startable container and container.stop() invocation so the logged message
includes the container type and the caught exception stack trace for CI triage.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java`:
- Around line 46-54: safeStop currently swallows exceptions silently; update it
to log the caught Exception (at WARN or DEBUG) while preserving best-effort
semantics: in the catch block of safeStop(Startable container) call your
test-class logger (e.g., a private static Logger or SLF4J Logger named logger in
AbstractIntegrationTest) and log a concise message including the exception
(e.g., "Failed to stop container" with the exception) instead of ignoring it, so
container.stop() failures are visible without changing the no-throw behavior.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.java`:
- Around line 48-56: The safeStop method currently swallows exceptions; update
safeStop(Startable container) to log any caught Exception at WARN level instead
of completely ignoring it: obtain or add a logger (e.g., a private static final
org.slf4j.Logger LOGGER =
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and inside the catch
block call LOGGER.warn("Failed to stop container during test teardown", e) (or
similar) so container.stop() failures are recorded for post-mortem while
retaining best-effort cleanup semantics.
In
`@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.java`:
- Around line 46-53: The safeStop method currently swallows all exceptions;
update it to keep best-effort semantics but log failures at a low level: catch
Exception e and call a logger (e.g., LOGGER.warn or LOGGER.debug) with the
container's type (container.getClass().getName()) and the exception so teardown
failures are visible; if no logger exists in AbstractIntegrationTest, add a
private static final Logger (e.g.,
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and use it in safeStop
to emit the message while still not rethrowing the exception.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java`:
- Around line 47-55: The safeStop method in AbstractIntegrationTest swallows
exceptions during container.stop(), making intermittent CI shutdown failures
hard to diagnose; update safeStop(Startable container) to log the caught
Exception at DEBUG/TRACE using the test class logger (e.g., Logger or
LoggerFactory) with a short message and the exception, and optionally extract
this method into a shared utility (e.g., TestContainerUtils.safeStop(Startable))
used by all AbstractIntegrationTest classes to eliminate duplication and ensure
consistent debug logging.
---
Duplicate comments:
In
`@ledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.java`:
- Around line 46-53: The safeStop(Startable container) helper currently swallows
exceptions making CI cleanup failures invisible; modify safeStop in
AbstractIntegrationTest to catch Exception as now but call the test logger (or
System.err) to emit a warning that includes a fixed "testcontainers" label and
the caught exception details (message/stacktrace) while preserving the
best-effort semantics (i.e., do not rethrow). Ensure the log message includes
the container identity (e.g., container.toString() or
container.getContainerId()) so failures are diagnosable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b02ab89d-28e4-42f6-8c2a-afb8657baba3
📒 Files selected for processing (10)
api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.javablockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javacompliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.javafiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.javafiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.javafx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.javaledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.javapayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java
Comment on lines
+46
to
54
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider minimal diagnostic signal for suppressed stop failures.
safeStop currently swallows all exceptions silently. Add a low-noise debug/warn log (without sensitive payloads) to aid flaky CI triage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`
around lines 46 - 54, safeStop currently swallows all exceptions; add a
low-noise diagnostic log so CI flakes can be triaged: introduce or reuse an
SLF4J Logger on AbstractIntegrationTest and change the catch to catch (Exception
e) { logger.debug("Container.stop() failed (best-effort cleanup): {}",
e.getMessage()); } (or logger.warn with the message only for higher visibility)
to avoid logging sensitive payloads but preserve the exception message/context
for Startable container stop failures.
Comment on lines
31
to
+44
| static { | ||
| POSTGRES.start(); | ||
| KAFKA.start(); | ||
| try { | ||
| POSTGRES.start(); | ||
| KAFKA.start(); | ||
| } catch (RuntimeException ex) { | ||
| safeStop(KAFKA); | ||
| safeStop(POSTGRES); | ||
| throw ex; | ||
| } | ||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| KAFKA.stop(); | ||
| POSTGRES.stop(); | ||
| })); | ||
| safeStop(KAFKA); | ||
| safeStop(POSTGRES); | ||
| }, "testcontainers-shutdown")); | ||
| } |
There was a problem hiding this comment.
Register the shutdown hook before startup to guarantee last-resort cleanup.
At Line 38, rethrow exits the static block, so Lines 40-43 never execute. If a stop attempt in the catch path fails, there is no shutdown hook fallback and containers can leak for the JVM lifetime.
Proposed fix
static {
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ safeStop(KAFKA);
+ safeStop(POSTGRES);
+ }, "testcontainers-shutdown"));
+
try {
POSTGRES.start();
KAFKA.start();
} catch (RuntimeException ex) {
safeStop(KAFKA);
safeStop(POSTGRES);
throw ex;
}
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- safeStop(KAFKA);
- safeStop(POSTGRES);
- }, "testcontainers-shutdown"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static { | |
| POSTGRES.start(); | |
| KAFKA.start(); | |
| try { | |
| POSTGRES.start(); | |
| KAFKA.start(); | |
| } catch (RuntimeException ex) { | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| throw ex; | |
| } | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| KAFKA.stop(); | |
| POSTGRES.stop(); | |
| })); | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| }, "testcontainers-shutdown")); | |
| } | |
| static { | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| }, "testcontainers-shutdown")); | |
| try { | |
| POSTGRES.start(); | |
| KAFKA.start(); | |
| } catch (RuntimeException ex) { | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| throw ex; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@compliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.java`
around lines 31 - 44, Register the shutdown hook before starting containers so
it always exists as a last-resort cleanup: move the
Runtime.getRuntime().addShutdownHook(...) call to before the
POSTGRES.start()/KAFKA.start() calls in the static initializer, then attempt to
start POSTGRES and KAFKA inside the try block; keep the catch that calls
safeStop(KAFKA) and safeStop(POSTGRES) and rethrows the exception, but the
shutdown hook (using the existing safeStop method) will now always be registered
even if startup throws. Ensure you reference the same symbols: the static
initializer, Runtime.getRuntime().addShutdownHook, POSTGRES, KAFKA, and
safeStop.
Comment on lines
+46
to
+53
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid fully silent stop failures; emit minimal infra diagnostics.
Suppressing cleanup exceptions is fine, but fully silent failures make container lifecycle flakiness hard to diagnose. Log container type + exception class/message (no payload/PII).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@compliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.java`
around lines 46 - 53, The safeStop(Startable container) method currently
swallows all exceptions; update the catch to log minimal infra diagnostics
(container type and exception class/message) while still treating it as
best-effort cleanup: catch Exception e and emit a single-line diagnostic using
your project's logger (or System.err if no logger available) that includes
container.getClass().getSimpleName(), e.getClass().getSimpleName(), and a
trimmed e.getMessage() (avoid stack traces or payload/PII), then continue
without rethrowing; keep the existing null check and best-effort semantics
around container.stop().
Comment on lines
+46
to
+53
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } |
There was a problem hiding this comment.
Do not fully suppress stop failures in safeStop.
Line 51 currently discards cleanup exceptions entirely; when container teardown fails in CI, this hides the root cause and makes resource-leak triage difficult. Log at least a WARN with container type and exception.
Suggested patch
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import org.testcontainers.lifecycle.Startable;
@@
public abstract class AbstractIntegrationTest {
+ private static final Logger log = LoggerFactory.getLogger(AbstractIntegrationTest.class);
@@
private static void safeStop(Startable container) {
try {
if (container != null) {
container.stop();
}
- } catch (Exception ignored) {
- // best-effort cleanup
+ } catch (Exception ex) {
+ log.warn("Best-effort Testcontainers cleanup failed for {}",
+ container != null ? container.getClass().getSimpleName() : "unknown", ex);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.java`
around lines 46 - 53, The safeStop method currently swallows all exceptions;
update it to log a WARN instead of discarding so teardown failures are visible:
add an SLF4J Logger (e.g., private static final Logger log =
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and in the catch block
call log.warn("Failed to stop container of type {}", container != null ?
container.getClass().getName() : "null", e) (or similar), ensuring you reference
the Startable container and container.stop() invocation so the logged message
includes the container type and the caught exception stack trace for CI triage.
Comment on lines
+46
to
54
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging suppressed exceptions.
Silently swallowing exceptions in safeStop makes container teardown failures invisible. A WARN or DEBUG log would aid troubleshooting without breaking the best-effort semantics.
♻️ Proposed improvement
+ private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(AbstractIntegrationTest.class);
+
private static void safeStop(Startable container) {
try {
if (container != null) {
container.stop();
}
- } catch (Exception ignored) {
- // best-effort cleanup
+ } catch (Exception ex) {
+ LOG.warn("Best-effort container stop failed: {}", ex.getMessage());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static void safeStop(Startable container) { | |
| try { | |
| if (container != null) { | |
| container.stop(); | |
| } | |
| } catch (Exception ignored) { | |
| // best-effort cleanup | |
| } | |
| } | |
| private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(AbstractIntegrationTest.class); | |
| private static void safeStop(Startable container) { | |
| try { | |
| if (container != null) { | |
| container.stop(); | |
| } | |
| } catch (Exception ex) { | |
| LOG.warn("Best-effort container stop failed: {}", ex.getMessage()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.java`
around lines 46 - 54, safeStop currently swallows exceptions silently; update it
to log the caught Exception (at WARN or DEBUG) while preserving best-effort
semantics: in the catch block of safeStop(Startable container) call your
test-class logger (e.g., a private static Logger or SLF4J Logger named logger in
AbstractIntegrationTest) and log a concise message including the exception
(e.g., "Failed to stop container" with the exception) instead of ignoring it, so
container.stop() failures are visible without changing the no-throw behavior.
Comment on lines
+48
to
56
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging swallowed exceptions for debuggability.
Silently swallowing exceptions is acceptable for best-effort cleanup, but a WARN-level log would aid post-mortem analysis without breaking the teardown chain.
♻️ Optional: add logging for suppressed exceptions
+ private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(AbstractIntegrationTest.class);
+
private static void safeStop(Startable container) {
try {
if (container != null) {
container.stop();
}
- } catch (Exception ignored) {
- // best-effort cleanup
+ } catch (Exception ex) {
+ LOG.warn("Best-effort container stop failed", ex);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.java`
around lines 48 - 56, The safeStop method currently swallows exceptions; update
safeStop(Startable container) to log any caught Exception at WARN level instead
of completely ignoring it: obtain or add a logger (e.g., a private static final
org.slf4j.Logger LOGGER =
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and inside the catch
block call LOGGER.warn("Failed to stop container during test teardown", e) (or
similar) so container.stop() failures are recorded for post-mortem while
retaining best-effort cleanup semantics.
Comment on lines
+46
to
+53
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid fully silent stop failures in safeStop.
Suppressing exceptions without any trace makes flaky container teardown harder to triage. Keep best-effort semantics, but emit a low-level log with container type.
Proposed diff
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import org.springframework.boot.test.context.SpringBootTest;
@@
public abstract class AbstractIntegrationTest {
+ private static final Logger log = LoggerFactory.getLogger(AbstractIntegrationTest.class);
@@
private static void safeStop(Startable container) {
try {
if (container != null) {
container.stop();
}
- } catch (Exception ignored) {
- // best-effort cleanup
+ } catch (Exception ex) {
+ log.debug("Best-effort Testcontainers stop failed for {}",
+ container == null ? "unknown" : container.getClass().getSimpleName(), ex);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.java`
around lines 46 - 53, The safeStop method currently swallows all exceptions;
update it to keep best-effort semantics but log failures at a low level: catch
Exception e and call a logger (e.g., LOGGER.warn or LOGGER.debug) with the
container's type (container.getClass().getName()) and the exception so teardown
failures are visible; if no logger exists in AbstractIntegrationTest, add a
private static final Logger (e.g.,
LoggerFactory.getLogger(AbstractIntegrationTest.class)) and use it in safeStop
to emit the message while still not rethrowing the exception.
Comment on lines
+47
to
55
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider optional DEBUG logging for cleanup failures.
Silently swallowed exceptions are fine for best-effort cleanup, but adding a brief log at DEBUG/TRACE can help diagnose intermittent CI failures when container shutdown misbehaves.
♻️ Optional: Add logging for diagnostics
+ private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(AbstractIntegrationTest.class);
+
private static void safeStop(Startable container) {
try {
if (container != null) {
container.stop();
}
- } catch (Exception ignored) {
- // best-effort cleanup
+ } catch (Exception ex) {
+ LOG.debug("Best-effort cleanup failed for container: {}", container, ex);
}
}Minor: Since this identical pattern is applied across all 10 AbstractIntegrationTest classes, consider extracting safeStop to a shared test utility (e.g., TestContainerUtils) to reduce duplication and ensure consistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.java`
around lines 47 - 55, The safeStop method in AbstractIntegrationTest swallows
exceptions during container.stop(), making intermittent CI shutdown failures
hard to diagnose; update safeStop(Startable container) to log the caught
Exception at DEBUG/TRACE using the test class logger (e.g., Logger or
LoggerFactory) with a short message and the exception, and optionally extract
this method into a shared utility (e.g., TestContainerUtils.safeStop(Startable))
used by all AbstractIntegrationTest classes to eliminate duplication and ensure
consistent debug logging.
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
1 participant
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.
Summary
safeStop(Startable)helper to all 10AbstractIntegrationTestclassescontainer.stop()in try/catch so one failure doesn't skip remaining cleanups"testcontainers-shutdown"for diagnosticsAddresses CodeRabbit review on PR #189.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit