feat(phase2): cross-service saga integration tests (STA-13)#120
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughAdds Phase‑2 end‑to‑end integration testing: a new Docker Compose stack for S1/S2/S6 with infra (Postgres, TimescaleDB, Redis, Redpanda/Kafka, Temporal, WireMock), a new Gradle test module with saga and load tests, orchestration scripts, Kafka producer & Feign idempotency wiring, and a small FX scheduling type change to use OutboxEventPublisher. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Phase2 Test
participant S1 as Payment Orchestrator (S1)
participant Temporal as Temporal Server
participant S2 as Compliance Service (S2)
participant WireMock as WireMock
participant S6 as FX Liquidity (S6)
participant Kafka as Redpanda/Kafka
rect rgba(100,149,237,0.5)
Note over Test,S1: Initiate payment and start saga
end
Test->>S1: POST /v1/payments (Idempotency-Key)
S1->>Temporal: Start PaymentWorkflow
Temporal->>S1: Invoke activities
S1->>S2: CheckCompliance activity
S2->>WireMock: Sanctions lookup
WireMock-->>S2: Sanctions response
S2-->>S1: Compliance result
S1->>S6: GetFxRate activity
S6-->>S1: FX rate
S1->>Kafka: Publish payment.* events
Temporal-->>S1: Workflow completes
S1-->>Test: HTTP response returned
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java (1)
99-99: 🧹 Nitpick | 🔵 TrivialConsider using
any()instead of creating a new fixture withnever().
eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))creates a new random UUID that will never match any actual invocation. When usingnever(), the matcher is irrelevant since you're asserting zero calls occurred. Usingany()orshouldHaveNoInteractions()would express intent more clearly.✨ Clearer assertion
- then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))); + then(lockService).shouldHaveNoInteractions();🤖 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/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java` at line 99, In LockExpiryJobTest replace the fragile never() assertion that uses eqIgnoringTimestamps(anActiveLock(UUID.randomUUID())) with a clearer zero-interaction check: call Mockito's any() matcher (e.g., expireLock(any())) or assert no interactions on lockService (shouldHaveNoInteractions / verifyNoInteractions) so the test does not construct a random UUID that can never match; update the assertion around lockService.expireLock accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.phase2-test.yml`:
- Around line 61-62: The compose file pins the Redpanda image to :latest which
risks non-reproducible tests; update the redpanda service (image:
docker.redpanda.com/redpandadata/redpanda:latest) to a specific, tested tag
(e.g., docker.redpanda.com/redpandadata/redpanda:<version>) and do the same for
the temporal and wiremock services by replacing their :latest image tags with
explicit versioned tags or variables so builds/tests are deterministic.
- Around line 115-119: The healthcheck currently always succeeds because the
test command in the healthcheck block (the line starting with test:
["CMD-SHELL", "temporal operator cluster health 2>/dev/null | grep -q 'OK' ||
exit 0"]) appends "|| exit 0", so remove the "|| exit 0" suffix from that test
string so the command returns a non‑zero exit code when Temporal is unhealthy;
leave the rest of the healthcheck (interval/timeout/retries) unchanged.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java`:
- Around line 9-12: The FeignIdempotencyInterceptor currently adds an
Idempotency-Key to every request; update the interceptor
(FeignIdempotencyInterceptor) so its apply(RequestTemplate template) only
injects the Idempotency-Key for state-changing HTTP methods (e.g., POST, PUT,
PATCH — or the specific set required by S2) by checking template.method() (or
equivalent) before adding the header, leaving GET/HEAD and other non-mutating
methods unchanged; keep the header generation logic and header name the same but
guard it behind this method check to avoid adding the header to safe/idempotent
requests.
- Line 20: The header existence check in FeignIdempotencyInterceptor uses
template.headers().containsKey(IDEMPOTENCY_KEY_HEADER) which can be
case-sensitive and allow duplicates; change the check to perform a
case-insensitive lookup over template.headers().keySet() (or normalize keys)
before adding IDEMPOTENCY_KEY_HEADER so any existing header like
"idempotency-key" or "IDEMPOTENCY-KEY" is detected and avoided; update the logic
around the add operation in FeignIdempotencyInterceptor to use this
case-insensitive check and ensure you still add the header only when no matching
key is present.
- Around line 18-23: The FeignIdempotencyInterceptor.apply currently generates a
new UUID per retry; instead, read an existing idempotency value from a
request-scoped context before creating one: check RequestTemplate.headers() for
IDEMPOTENCY_KEY_HEADER, if missing try to fetch a contextual key (e.g.,
MDC.get("idempotencyKey") or a ThreadLocal/request-scoped bean) and if found set
that header, otherwise generate a UUID, set the header and also populate the
context (e.g., MDC.put("idempotencyKey", generatedId)) so subsequent retries
calling FeignIdempotencyInterceptor.apply() reuse the same idempotency key;
update references in FeignIdempotencyInterceptor.apply and ensure the context
key name matches upstream controllers.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.java`:
- Around line 23-27: The producer config in KafkaProducerConfig (the method
returning DefaultKafkaProducerFactory) lacks reliability settings; update the
Map passed to new DefaultKafkaProducerFactory in that method to include
ProducerConfig.ACKS_CONFIG set to "all", ProducerConfig.RETRIES_CONFIG set to a
suitable value (e.g., Integer.MAX_VALUE or a specific retry count), and
ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG set to true, and consider adding
ProducerConfig.MAX_IN_FLIGHT_REQUESTS_PER_CONNECTION to 1 for ordering; ensure
you add these entries to the Map.of(...) used in the producerFactory method so
the producer is configured for durable, idempotent deliveries.
In `@phase2-integration-tests/build.gradle.kts`:
- Line 6: The property wiremockVersion is declared but unused; either remove the
val wiremockVersion: String by project declaration or use it in the dependencies
block to add WireMock (e.g., add a dependency like
"com.github.tomakehurst:wiremock-jre8:$wiremockVersion" inside the dependencies
{}), and ensure any test sourceSets or testImplementation entries reference the
variable so it is no longer unused.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.java`:
- Around line 65-78: The WorkflowServiceStubs created in the setup are not
stored and thus never shut down; replace the local var with a class field (e.g.,
WorkflowServiceStubs workflowServiceStubs) initialized where you currently call
WorkflowServiceStubs.newServiceStubs, use that field when building
WorkflowClient, and in the `@AfterAll` teardown method call
workflowServiceStubs.shutdown() (or shutdownNow()/awaitTermination as
appropriate) to explicitly release gRPC/native resources rather than relying on
GC; also remove or update the incorrect comment about auto-closeable via GC.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`:
- Around line 201-204: The test is accepting HTTP 500 from cancelPayment which
masks an API bug; update the API to handle the already-completed workflow case
and return a 409 (Conflict) instead of throwing an unhandled exception, and then
remove 500 from the assertion in Phase2SagaIntegrationTest (the cancelPayment
call). Specifically, find the controller/handler method annotated with
`@PostMapping`("cancel") and add proper exception handling/translation for the
Temporal-already-done or workflow-terminal errors so they map to a 409 response
(or a well-defined error DTO), and then change the test assertion in
Phase2SagaIntegrationTest (the cancelPayment assertion) to only expect 200 or
409.
- Line 1: Spotless formatting violations are breaking CI; run the formatter and
commit the changes for the Phase2SagaIntegrationTest class. Execute ./gradlew
spotlessApply (or apply your IDE's Spotless/Google Java format) to reformat
files under package com.stablecoin.payments.phase2, then review and commit the
modified Phase2SagaIntegrationTest.java so SpotlessJavaCheck passes.
- Around line 72-85: The test currently creates WorkflowServiceStubs via a local
variable `stubs` and never closes it; change `stubs` to a class-level field
(e.g., `workflowServiceStubs`) and assign the result of
WorkflowServiceStubs.newServiceStubs(...) to that field; then update `@AfterAll
teardownClients()` to call `workflowServiceStubs.close()` (and also close
`workflowClient` if you made it non-auto-closeable) to ensure the gRPC channels
are properly released; reference symbols: WorkflowServiceStubs, newServiceStubs,
workflowServiceStubs (new field), teardownClients, workflowClient.
In `@scripts/run-phase2-tests.sh`:
- Line 80: The while loop condition uses unquoted shell variables which can
cause word splitting or globbing; update the loop in run-phase2-tests.sh to
quote the variables (use "[ \"$elapsed\" -lt \"$max_wait\" ]") and also audit
other uses of elapsed and max_wait in this script to ensure they are similarly
double-quoted to prevent SC2086 issues.
---
Outside diff comments:
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java`:
- Line 99: In LockExpiryJobTest replace the fragile never() assertion that uses
eqIgnoringTimestamps(anActiveLock(UUID.randomUUID())) with a clearer
zero-interaction check: call Mockito's any() matcher (e.g., expireLock(any()))
or assert no interactions on lockService (shouldHaveNoInteractions /
verifyNoInteractions) so the test does not construct a random UUID that can
never match; update the assertion around lockService.expireLock accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09ca34c9-ba6c-4ece-a288-d92dd739d2ea
📒 Files selected for processing (11)
docker-compose.phase2-test.ymlfx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJob.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.javaphase2-integration-tests/build.gradle.ktsphase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.javaphase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.javaphase2-integration-tests/src/test/resources/logback-test.xmlscripts/run-phase2-tests.shsettings.gradle.kts
Comment on lines
+61
to
+62
| redpanda: | ||
| image: docker.redpanda.com/redpandadata/redpanda:latest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider pinning image versions for reproducibility.
Using :latest for redpanda, temporal, and wiremock images can cause flaky tests when upstream releases break compatibility. Pin to specific versions for deterministic builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.phase2-test.yml` around lines 61 - 62, The compose file pins
the Redpanda image to :latest which risks non-reproducible tests; update the
redpanda service (image: docker.redpanda.com/redpandadata/redpanda:latest) to a
specific, tested tag (e.g., docker.redpanda.com/redpandadata/redpanda:<version>)
and do the same for the temporal and wiremock services by replacing their
:latest image tags with explicit versioned tags or variables so builds/tests are
deterministic.
Comment on lines
+115
to
+119
| healthcheck: | ||
| test: ["CMD-SHELL", "temporal operator cluster health 2>/dev/null | grep -q 'OK' || exit 0"] | ||
| interval: 15s | ||
| timeout: 10s | ||
| retries: 15 |
There was a problem hiding this comment.
Temporal healthcheck always succeeds.
The || exit 0 at the end means this healthcheck never fails, defeating its purpose. Remove it so Docker can detect when Temporal is actually unhealthy.
🐛 Proposed fix
healthcheck:
- test: ["CMD-SHELL", "temporal operator cluster health 2>/dev/null | grep -q 'OK' || exit 0"]
+ test: ["CMD-SHELL", "temporal operator cluster health 2>&1 | grep -q 'SERVING'"]
interval: 15s
timeout: 10s
retries: 15
+ start_period: 30s📝 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
| healthcheck: | |
| test: ["CMD-SHELL", "temporal operator cluster health 2>/dev/null | grep -q 'OK' || exit 0"] | |
| interval: 15s | |
| timeout: 10s | |
| retries: 15 | |
| healthcheck: | |
| test: ["CMD-SHELL", "temporal operator cluster health 2>&1 | grep -q 'SERVING'"] | |
| interval: 15s | |
| timeout: 10s | |
| retries: 15 | |
| start_period: 30s |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.phase2-test.yml` around lines 115 - 119, The healthcheck
currently always succeeds because the test command in the healthcheck block (the
line starting with test: ["CMD-SHELL", "temporal operator cluster health
2>/dev/null | grep -q 'OK' || exit 0"]) appends "|| exit 0", so remove the "||
exit 0" suffix from that test string so the command returns a non‑zero exit code
when Temporal is unhealthy; leave the rest of the healthcheck
(interval/timeout/retries) unchanged.
Comment on lines
+9
to
+12
| /** | ||
| * Feign interceptor that adds an Idempotency-Key header to all outgoing requests. | ||
| * Required by S2 Compliance Service which rejects POST requests without this header. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Interceptor applies to all HTTP methods, not just POST/PUT.
The comment notes S2 requires this for POST, but the interceptor injects the header on GET, DELETE, etc. as well. Low impact but adds unnecessary overhead and noise in logs/traces.
✂️ Limit to state-changing methods
`@Override`
public void apply(RequestTemplate template) {
+ if (template.method() == null ||
+ (!template.method().equals("POST") && !template.method().equals("PUT"))) {
+ return;
+ }
if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) {
template.header(IDEMPOTENCY_KEY_HEADER, UUID.randomUUID().toString());
}
}Also applies to: 19-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java`
around lines 9 - 12, The FeignIdempotencyInterceptor currently adds an
Idempotency-Key to every request; update the interceptor
(FeignIdempotencyInterceptor) so its apply(RequestTemplate template) only
injects the Idempotency-Key for state-changing HTTP methods (e.g., POST, PUT,
PATCH — or the specific set required by S2) by checking template.method() (or
equivalent) before adding the header, leaving GET/HEAD and other non-mutating
methods unchanged; keep the header generation logic and header name the same but
guard it behind this method check to avoid adding the header to safe/idempotent
requests.
Comment on lines
+18
to
+23
| @Override | ||
| public void apply(RequestTemplate template) { | ||
| if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) { | ||
| template.header(IDEMPOTENCY_KEY_HEADER, UUID.randomUUID().toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Idempotency breaks on Feign retries.
When Feign retries a failed request (timeout, 5xx), the RequestTemplate is rebuilt and apply() is called again. If the caller didn't supply an idempotency key, each retry gets a fresh UUID—defeating the entire purpose of idempotency.
Consider propagating the idempotency key from request context (e.g., MDC, ThreadLocal, or a request-scoped bean) so retries reuse the same key.
🛠️ Sketch of context-aware approach
+import org.slf4j.MDC;
+
`@Override`
public void apply(RequestTemplate template) {
if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) {
- template.header(IDEMPOTENCY_KEY_HEADER, UUID.randomUUID().toString());
+ String contextKey = MDC.get("idempotencyKey");
+ String key = (contextKey != null) ? contextKey : UUID.randomUUID().toString();
+ template.header(IDEMPOTENCY_KEY_HEADER, key);
}
}Upstream controller/entry point sets MDC.put("idempotencyKey", incomingKey) at request start.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java`
around lines 18 - 23, The FeignIdempotencyInterceptor.apply currently generates
a new UUID per retry; instead, read an existing idempotency value from a
request-scoped context before creating one: check RequestTemplate.headers() for
IDEMPOTENCY_KEY_HEADER, if missing try to fetch a contextual key (e.g.,
MDC.get("idempotencyKey") or a ThreadLocal/request-scoped bean) and if found set
that header, otherwise generate a UUID, set the header and also populate the
context (e.g., MDC.put("idempotencyKey", generatedId)) so subsequent retries
calling FeignIdempotencyInterceptor.apply() reuse the same idempotency key;
update references in FeignIdempotencyInterceptor.apply and ensure the context
key name matches upstream controllers.
|
|
||
| @Override | ||
| public void apply(RequestTemplate template) { | ||
| if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) { |
There was a problem hiding this comment.
Header lookup may be case-sensitive.
HTTP headers are case-insensitive per RFC 7230, but template.headers() returns a standard Map whose key comparison depends on Feign's internal implementation. A caller passing "idempotency-key" (lowercase) could result in a duplicate header.
🔧 Safer case-insensitive check
- if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) {
+ boolean hasKey = template.headers().keySet().stream()
+ .anyMatch(h -> h.equalsIgnoreCase(IDEMPOTENCY_KEY_HEADER));
+ if (!hasKey) {📝 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
| if (!template.headers().containsKey(IDEMPOTENCY_KEY_HEADER)) { | |
| boolean hasKey = template.headers().keySet().stream() | |
| .anyMatch(h -> h.equalsIgnoreCase(IDEMPOTENCY_KEY_HEADER)); | |
| if (!hasKey) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java`
at line 20, The header existence check in FeignIdempotencyInterceptor uses
template.headers().containsKey(IDEMPOTENCY_KEY_HEADER) which can be
case-sensitive and allow duplicates; change the check to perform a
case-insensitive lookup over template.headers().keySet() (or normalize keys)
before adding IDEMPOTENCY_KEY_HEADER so any existing header like
"idempotency-key" or "IDEMPOTENCY-KEY" is detected and avoided; update the logic
around the add operation in FeignIdempotencyInterceptor to use this
case-insensitive check and ensure you still add the header only when no matching
key is present.
Comment on lines
+65
to
+78
| var stubs = WorkflowServiceStubs.newServiceStubs( | ||
| WorkflowServiceStubsOptions.newBuilder() | ||
| .setTarget(TEMPORAL_ADDRESS) | ||
| .build()); | ||
| workflowClient = WorkflowClient.newInstance(stubs, | ||
| WorkflowClientOptions.newBuilder() | ||
| .setNamespace("default") | ||
| .build()); | ||
| } | ||
|
|
||
| @AfterAll | ||
| void teardown() { | ||
| // WorkflowClient and HttpClient are auto-closeable via GC | ||
| } |
There was a problem hiding this comment.
Same WorkflowServiceStubs leak applies here.
Store WorkflowServiceStubs as a field and call shutdown() in @AfterAll. The comment "auto-closeable via GC" is inaccurate—gRPC channels hold native resources that should be explicitly released.
🐛 Proposed fix
private HttpClient httpClient;
private WorkflowClient workflowClient;
+ private WorkflowServiceStubs serviceStubs;
`@BeforeAll`
void setupClients() {
httpClient = HttpClient.newBuilder()
.version(HttpClient.Version.HTTP_1_1)
.connectTimeout(Duration.ofSeconds(30))
.build();
- var stubs = WorkflowServiceStubs.newServiceStubs(
+ serviceStubs = WorkflowServiceStubs.newServiceStubs(
WorkflowServiceStubsOptions.newBuilder()
.setTarget(TEMPORAL_ADDRESS)
.build());
- workflowClient = WorkflowClient.newInstance(stubs,
+ workflowClient = WorkflowClient.newInstance(serviceStubs,
WorkflowClientOptions.newBuilder()
.setNamespace("default")
.build());
}
`@AfterAll`
void teardown() {
- // WorkflowClient and HttpClient are auto-closeable via GC
+ if (serviceStubs != null) {
+ serviceStubs.shutdown();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.java`
around lines 65 - 78, The WorkflowServiceStubs created in the setup are not
stored and thus never shut down; replace the local var with a class field (e.g.,
WorkflowServiceStubs workflowServiceStubs) initialized where you currently call
WorkflowServiceStubs.newServiceStubs, use that field when building
WorkflowClient, and in the `@AfterAll` teardown method call
workflowServiceStubs.shutdown() (or shutdownNow()/awaitTermination as
appropriate) to explicitly release gRPC/native resources rather than relying on
GC; also remove or update the incorrect comment about auto-closeable via GC.
Comment on lines
+72
to
+85
| var stubs = WorkflowServiceStubs.newServiceStubs( | ||
| WorkflowServiceStubsOptions.newBuilder() | ||
| .setTarget(TEMPORAL_ADDRESS) | ||
| .build()); | ||
| workflowClient = WorkflowClient.newInstance(stubs, | ||
| WorkflowClientOptions.newBuilder() | ||
| .setNamespace("default") | ||
| .build()); | ||
| } | ||
|
|
||
| @AfterAll | ||
| void teardownClients() { | ||
| // httpClient and workflowClient are auto-closeable but no explicit close needed | ||
| } |
There was a problem hiding this comment.
WorkflowServiceStubs resource leak.
WorkflowServiceStubs implements Closeable and manages gRPC channels. Store the reference and close it in @AfterAll to prevent resource leaks during test runs.
🐛 Proposed fix
private HttpClient httpClient;
private WorkflowClient workflowClient;
+ private WorkflowServiceStubs serviceStubs;
`@BeforeAll`
void setupClients() {
httpClient = HttpClient.newBuilder()
.version(HttpClient.Version.HTTP_1_1)
.connectTimeout(Duration.ofSeconds(10))
.build();
- var stubs = WorkflowServiceStubs.newServiceStubs(
+ serviceStubs = WorkflowServiceStubs.newServiceStubs(
WorkflowServiceStubsOptions.newBuilder()
.setTarget(TEMPORAL_ADDRESS)
.build());
- workflowClient = WorkflowClient.newInstance(stubs,
+ workflowClient = WorkflowClient.newInstance(serviceStubs,
WorkflowClientOptions.newBuilder()
.setNamespace("default")
.build());
}
`@AfterAll`
void teardownClients() {
- // httpClient and workflowClient are auto-closeable but no explicit close needed
+ if (serviceStubs != null) {
+ serviceStubs.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
| var stubs = WorkflowServiceStubs.newServiceStubs( | |
| WorkflowServiceStubsOptions.newBuilder() | |
| .setTarget(TEMPORAL_ADDRESS) | |
| .build()); | |
| workflowClient = WorkflowClient.newInstance(stubs, | |
| WorkflowClientOptions.newBuilder() | |
| .setNamespace("default") | |
| .build()); | |
| } | |
| @AfterAll | |
| void teardownClients() { | |
| // httpClient and workflowClient are auto-closeable but no explicit close needed | |
| } | |
| private HttpClient httpClient; | |
| private WorkflowClient workflowClient; | |
| private WorkflowServiceStubs serviceStubs; | |
| `@BeforeAll` | |
| void setupClients() { | |
| httpClient = HttpClient.newBuilder() | |
| .version(HttpClient.Version.HTTP_1_1) | |
| .connectTimeout(Duration.ofSeconds(10)) | |
| .build(); | |
| serviceStubs = WorkflowServiceStubs.newServiceStubs( | |
| WorkflowServiceStubsOptions.newBuilder() | |
| .setTarget(TEMPORAL_ADDRESS) | |
| .build()); | |
| workflowClient = WorkflowClient.newInstance(serviceStubs, | |
| WorkflowClientOptions.newBuilder() | |
| .setNamespace("default") | |
| .build()); | |
| } | |
| `@AfterAll` | |
| void teardownClients() { | |
| if (serviceStubs != null) { | |
| serviceStubs.shutdown(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`
around lines 72 - 85, The test currently creates WorkflowServiceStubs via a
local variable `stubs` and never closes it; change `stubs` to a class-level
field (e.g., `workflowServiceStubs`) and assign the result of
WorkflowServiceStubs.newServiceStubs(...) to that field; then update `@AfterAll
teardownClients()` to call `workflowServiceStubs.close()` (and also close
`workflowClient` if you made it non-auto-closeable) to ensure the gRPC channels
are properly released; reference symbols: WorkflowServiceStubs, newServiceStubs,
workflowServiceStubs (new field), teardownClients, workflowClient.
Comment on lines
+201
to
+204
| // 3. Try cancel — workflow completes very quickly, so race outcomes: | ||
| // 200 = cancelled, 409 = terminal state, 500 = Temporal workflow already done | ||
| var cancelResponse = cancelPayment(paymentId); | ||
| assertThat(cancelResponse.statusCode()).isIn(200, 409, 500); |
There was a problem hiding this comment.
Accepting HTTP 500 in assertions indicates an API contract issue.
A cancel endpoint returning 500 represents an unhandled server error, not a valid business outcome. If the workflow is already complete, 409 Conflict is appropriate. Investigate why 500 occurs and either fix the API or remove it from expected responses.
#!/bin/bash
# Search for cancel endpoint implementation to understand error handling
ast-grep --pattern $'@PostMapping($$$"cancel"$$$)
$$$
public $_ $_($$$) {
$$$
}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`
around lines 201 - 204, The test is accepting HTTP 500 from cancelPayment which
masks an API bug; update the API to handle the already-completed workflow case
and return a 409 (Conflict) instead of throwing an unhandled exception, and then
remove 500 from the assertion in Phase2SagaIntegrationTest (the cancelPayment
call). Specifically, find the controller/handler method annotated with
`@PostMapping`("cancel") and add proper exception handling/translation for the
Temporal-already-done or workflow-terminal errors so they map to a 409 response
(or a well-defined error DTO), and then change the test assertion in
Phase2SagaIntegrationTest (the cancelPayment assertion) to only expect 200 or
409.
| local max_wait=${3:-120} | ||
| local elapsed=0 | ||
|
|
||
| while [ $elapsed -lt $max_wait ]; do |
There was a problem hiding this comment.
Quote variables to prevent word splitting.
ShellCheck SC2086: $elapsed and $max_wait should be double-quoted to prevent globbing and word splitting issues.
🧹 Proposed fix
- while [ $elapsed -lt $max_wait ]; do
+ while [ "$elapsed" -lt "$max_wait" ]; do📝 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
| while [ $elapsed -lt $max_wait ]; do | |
| while [ "$elapsed" -lt "$max_wait" ]; do |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 80-80: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/run-phase2-tests.sh` at line 80, The while loop condition uses
unquoted shell variables which can cause word splitting or globbing; update the
loop in run-phase2-tests.sh to quote the variables (use "[ \"$elapsed\" -lt
\"$max_wait\" ]") and also audit other uses of elapsed and max_wait in this
script to ensure they are similarly double-quoted to prevent SC2086 issues.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java (1)
99-99: 🧹 Nitpick | 🔵 TrivialSimplify the
never()verification.Creating a random
FxRateLockjust to pass toeqIgnoringTimestampsis unnecessary noise. Since no expiration should occur, verify zero interactions directly.♻️ Cleaner verification
- then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))); + then(lockService).shouldHaveNoInteractions();🤖 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/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java` at line 99, In LockExpiryJobTest replace the noisy then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))) verification with a direct zero-interaction check (e.g., call verifyNoInteractions(lockService) or Mockito.verifyNoInteractions(lockService)) so you don't need to construct a random FxRateLock or use eqIgnoringTimestamps; this asserts that lockService had no interactions at all instead of matching a specific argument.
♻️ Duplicate comments (2)
phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java (2)
71-84:⚠️ Potential issue | 🟠 MajorWorkflowServiceStubs resource leak remains unaddressed.
The
stubsvariable is never closed, leaking gRPC channels. Store as a field and callshutdown()in@AfterAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java` around lines 71 - 84, The WorkflowServiceStubs created in the test method is not closed and leaks gRPC channels; make the stubs a class-level field (e.g., WorkflowServiceStubs stubs) instead of a local variable and initialize it where currently created, then update the `@AfterAll` teardownClients() method to call stubs.shutdown() (and await termination if appropriate) to properly release the resource; reference WorkflowServiceStubs and the teardownClients() method when modifying the test class.
200-203:⚠️ Potential issue | 🟠 MajorHTTP 500 acceptance still present.
Accepting 500 masks an API bug. Terminal workflow should return 409 Conflict, not an unhandled exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java` around lines 200 - 203, The test currently accepts HTTP 500 from cancelPayment(paymentId), which masks a server bug; update the assertion on cancelResponse.statusCode() in Phase2SagaIntegrationTest to only allow 200 or 409 (remove 500) so a 500 will fail the test and surface the issue; locate the cancelPayment(paymentId) call and the assertThat(cancelResponse.statusCode()).isIn(...) assertion and change the allowed values to 200, 409.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`:
- Around line 169-174: The test currently only verifies paymentId after calling
getPayment(paymentId); add an assertion that the payment's state reflects
failure by checking getBody.get("state").asText() equals the expected failure
value (e.g., "FAILED" or PaymentState.FAILED.name()). Update
Phase2SagaIntegrationTest to assert the state field on getBody (use the project
enum constant if available, otherwise the literal failure string) so the test
verifies the failure status as commented.
- Around line 288-295: The waitForWorkflowCompletion method currently calls
stub.getResult(Map.class) which can block indefinitely; update it to call the
timeout overload on WorkflowStub (e.g., getResult(Class, Duration)) using a
reasonable timeout (for example 30s) and handle/propagate the timeout exception
accordingly; locate the WorkflowStub creation in waitForWorkflowCompletion
(variable workflowId and stub) and replace the blocking getResult(Map.class)
call with the overload that supplies a java.time.Duration timeout and ensure
imports and exception handling are added.
---
Outside diff comments:
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java`:
- Line 99: In LockExpiryJobTest replace the noisy
then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID())))
verification with a direct zero-interaction check (e.g., call
verifyNoInteractions(lockService) or Mockito.verifyNoInteractions(lockService))
so you don't need to construct a random FxRateLock or use eqIgnoringTimestamps;
this asserts that lockService had no interactions at all instead of matching a
specific argument.
---
Duplicate comments:
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`:
- Around line 71-84: The WorkflowServiceStubs created in the test method is not
closed and leaks gRPC channels; make the stubs a class-level field (e.g.,
WorkflowServiceStubs stubs) instead of a local variable and initialize it where
currently created, then update the `@AfterAll` teardownClients() method to call
stubs.shutdown() (and await termination if appropriate) to properly release the
resource; reference WorkflowServiceStubs and the teardownClients() method when
modifying the test class.
- Around line 200-203: The test currently accepts HTTP 500 from
cancelPayment(paymentId), which masks a server bug; update the assertion on
cancelResponse.statusCode() in Phase2SagaIntegrationTest to only allow 200 or
409 (remove 500) so a 500 will fail the test and surface the issue; locate the
cancelPayment(paymentId) call and the
assertThat(cancelResponse.statusCode()).isIn(...) assertion and change the
allowed values to 200, 409.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 733c2ab7-4691-4ff8-ae2e-4832380ce279
📒 Files selected for processing (3)
fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJob.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.javaphase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java
Comment on lines
+169
to
+174
| // 3. Verify payment state reflects failure via GET | ||
| var getResponse = getPayment(paymentId); | ||
| assertThat(getResponse.statusCode()).isEqualTo(200); | ||
| var getBody = JSON.readTree(getResponse.body()); | ||
| assertThat(getBody.get("paymentId").asText()).isEqualTo(paymentId); | ||
| } |
There was a problem hiding this comment.
Incomplete assertion: payment state not verified.
Comment says "Verify payment state reflects failure" but line 173 only checks paymentId. Add assertion on the actual state field.
var getBody = JSON.readTree(getResponse.body());
assertThat(getBody.get("paymentId").asText()).isEqualTo(paymentId);
+ assertThat(getBody.get("state").asText()).isIn("FAILED", "COMPLIANCE_REJECTED");📝 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
| // 3. Verify payment state reflects failure via GET | |
| var getResponse = getPayment(paymentId); | |
| assertThat(getResponse.statusCode()).isEqualTo(200); | |
| var getBody = JSON.readTree(getResponse.body()); | |
| assertThat(getBody.get("paymentId").asText()).isEqualTo(paymentId); | |
| } | |
| // 3. Verify payment state reflects failure via GET | |
| var getResponse = getPayment(paymentId); | |
| assertThat(getResponse.statusCode()).isEqualTo(200); | |
| var getBody = JSON.readTree(getResponse.body()); | |
| assertThat(getBody.get("paymentId").asText()).isEqualTo(paymentId); | |
| assertThat(getBody.get("state").asText()).isIn("FAILED", "COMPLIANCE_REJECTED"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`
around lines 169 - 174, The test currently only verifies paymentId after calling
getPayment(paymentId); add an assertion that the payment's state reflects
failure by checking getBody.get("state").asText() equals the expected failure
value (e.g., "FAILED" or PaymentState.FAILED.name()). Update
Phase2SagaIntegrationTest to assert the state field on getBody (use the project
enum constant if available, otherwise the literal failure string) so the test
verifies the failure status as commented.
Comment on lines
+288
to
+295
| private JsonNode waitForWorkflowCompletion(String paymentId) { | ||
| var workflowId = "payment-" + paymentId; | ||
| log.info("Waiting for workflow: {}", workflowId); | ||
|
|
||
| WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId); | ||
| var result = stub.getResult(Map.class); | ||
| return JSON.valueToTree(result); | ||
| } |
There was a problem hiding this comment.
Missing timeout on workflow result fetch.
stub.getResult(Map.class) blocks indefinitely. Use the overload with timeout to prevent test hangs:
private JsonNode waitForWorkflowCompletion(String paymentId) {
var workflowId = "payment-" + paymentId;
log.info("Waiting for workflow: {}", workflowId);
WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId);
- var result = stub.getResult(Map.class);
+ var result = stub.getResult(Duration.ofSeconds(60), Map.class);
return JSON.valueToTree(result);
}Temporal Java SDK WorkflowStub getResult timeout parameter
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.java`
around lines 288 - 295, The waitForWorkflowCompletion method currently calls
stub.getResult(Map.class) which can block indefinitely; update it to call the
timeout overload on WorkflowStub (e.g., getResult(Class, Duration)) using a
reasonable timeout (for example 30s) and handle/propagate the timeout exception
accordingly; locate the WorkflowStub creation in waitForWorkflowCompletion
(variable workflowId and stub) and replace the blocking getResult(Map.class)
call with the overload that supplies a java.time.Duration timeout and ensure
imports and exception handling are added.
Puneethkumarck
force-pushed
the
feature/STA-13-phase2-integration-tests
branch
from
dbd9c36 to
4a6a8c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java (1)
19-22:⚠️ Potential issue | 🟠 MajorPreserve a single idempotency key across retries.
Line 20 generates a fresh UUID every time
apply()runs. If the same outbound call is retried, the retried request gets a differentIdempotency-Key, which breaks downstream deduplication. Resolve the key from request-scoped context first and only generate it once per logical operation.#!/bin/bash # Verify whether Feign retries are enabled and whether the module already has # request-scoped idempotency propagation that this interceptor should reuse. fd 'application\.(ya?ml|properties)$' . -x rg -nH 'openfeign|feign|retry|Retryer|idempotency' rg -n --type=java -C3 'idempotencyKey|Idempotency-Key|RequestContextHolder|MDC|ThreadLocal|Retryer' payment-orchestrator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java` around lines 19 - 22, The FeignIdempotencyInterceptor.apply currently generates a new UUID on every invocation (UUID.randomUUID()) which causes retries to have different IDEMPOTENCY_KEY_HEADER values; change apply(RequestTemplate) to first attempt to resolve an existing idempotency key from a request-scoped context (e.g., RequestContextHolder, a ThreadLocal, or MDC) and only generate and store a new UUID if none is present, then set that single resolved/generated value on the template; update the code paths that initiate the logical operation to populate the request-scoped idempotency key so retries reuse the same key.payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.java (1)
21-26:⚠️ Potential issue | 🟠 MajorReplace the hardcoded producer config map with
KafkaProperties.buildProducerProperties().The
ProducerFactorybean discards allspring.kafka.producer.*andspring.kafka.properties.*settings from the YAML config—including the configuredmax-retries: 5—and supplies only bootstrap servers and serializers. Build fromKafkaPropertiesand apply serializer defaults on top.Suggested fix
+import org.springframework.boot.autoconfigure.kafka.KafkaProperties; + +import java.util.HashMap; import java.util.Map; @@ `@Bean` `@ConditionalOnMissingBean` public ProducerFactory<String, Object> producerFactory( - `@Value`("${spring.kafka.bootstrap-servers}") String bootstrapServers) { - return new DefaultKafkaProducerFactory<>(Map.of( - ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, bootstrapServers, - ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class, - ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, JsonSerializer.class)); + KafkaProperties kafkaProperties) { + Map<String, Object> props = new HashMap<>(kafkaProperties.buildProducerProperties()); + props.putIfAbsent(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class); + props.putIfAbsent(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, JsonSerializer.class); + return new DefaultKafkaProducerFactory<>(props); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.java` around lines 21 - 26, The producerFactory in KafkaProducerConfig currently builds a hardcoded Map and ignores spring.kafka.* settings; change it to obtain the base properties from an injected KafkaProperties by calling kafkaProperties.buildProducerProperties(), then add/override only the serializer and bootstrap settings (e.g., ensure ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG -> StringSerializer.class and ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG -> JsonSerializer.class) while leaving other properties (like max-retries) intact, and pass the resulting props into new DefaultKafkaProducerFactory<>(props); keep the method name producerFactory and class KafkaProducerConfig, and ensure KafkaProperties is injected into the config class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phase2-integration-tests/build.gradle.kts`:
- Around line 17-18: The test dependency declaration
testImplementation("tools.jackson.core:jackson-databind") brings Jackson 3.x and
conflicts with Jackson 2.x on the classpath; remove this explicit Jackson 3.x
entry or replace it with the project-managed Jackson 2.x artifact/version.
Either delete the tools.jackson.core line and rely on Spring Boot's BOM-managed
com.fasterxml.jackson.core, or change to the
com.fasterxml.jackson.core:jackson-databind coordinate parameterized from
gradle.properties (e.g. use a jacksonVersion property) so the version is
consistent with Spring Boot; if any imported modules (payment-orchestrator-api,
compliance-travel-rule-api, fx-liquidity-engine-api) pull in Jackson 2.x
transitively and you must keep the current declaration, add an exclusion on
those module dependencies for com.fasterxml.jackson.core to avoid mixed 2.x/3.x
classes.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.java`:
- Around line 148-151: The call to stub.getResult(Map.class) can block
indefinitely; replace it with the timeout overload (use the WorkflowStub
returned from workflowClient.newUntypedWorkflowStub and call the getResult
variant that accepts a timeout) so each workflow fetch uses a bounded Duration
(e.g., a sensible seconds/minutes value) and handle the timeout by
failing/logging and cancelling or terminating the stuck workflow; ensure you
reference WorkflowStub stub, workflowClient.newUntypedWorkflowStub(workflowId)
and replace stub.getResult(Map.class) with the timeout-taking overload and add a
catch for the timeout exception to clean up the workflow and fail the test
accordingly.
---
Duplicate comments:
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.java`:
- Around line 19-22: The FeignIdempotencyInterceptor.apply currently generates a
new UUID on every invocation (UUID.randomUUID()) which causes retries to have
different IDEMPOTENCY_KEY_HEADER values; change apply(RequestTemplate) to first
attempt to resolve an existing idempotency key from a request-scoped context
(e.g., RequestContextHolder, a ThreadLocal, or MDC) and only generate and store
a new UUID if none is present, then set that single resolved/generated value on
the template; update the code paths that initiate the logical operation to
populate the request-scoped idempotency key so retries reuse the same key.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.java`:
- Around line 21-26: The producerFactory in KafkaProducerConfig currently builds
a hardcoded Map and ignores spring.kafka.* settings; change it to obtain the
base properties from an injected KafkaProperties by calling
kafkaProperties.buildProducerProperties(), then add/override only the serializer
and bootstrap settings (e.g., ensure ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG
-> StringSerializer.class and ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG ->
JsonSerializer.class) while leaving other properties (like max-retries) intact,
and pass the resulting props into new DefaultKafkaProducerFactory<>(props); keep
the method name producerFactory and class KafkaProducerConfig, and ensure
KafkaProperties is injected into the config class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0e6c175-4aac-4b5a-a28a-694edbbcf2b1
📒 Files selected for processing (11)
docker-compose.phase2-test.ymlfx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJob.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/FeignIdempotencyInterceptor.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/config/KafkaProducerConfig.javaphase2-integration-tests/build.gradle.ktsphase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.javaphase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2SagaIntegrationTest.javaphase2-integration-tests/src/test/resources/logback-test.xmlscripts/run-phase2-tests.shsettings.gradle.kts
Comment on lines
+17
to
+18
| // Jackson 3 — JSON parsing (JSR-310 built into core in Jackson 3) | ||
| testImplementation("tools.jackson.core:jackson-databind") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what Jackson version is used elsewhere in the project
rg -n "jackson" --type=kotlin -g "*.gradle.kts" | head -20
# Check version catalog for Jackson
fd "libs.versions.toml" --exec cat {} 2>/dev/null | grep -i jacksonRepository: Puneethkumarck/stablebridge-platform
Length of output: 1124
🏁 Script executed:
#!/bin/bash
# Find the full context of the Jackson dependency in question
cat -n phase2-integration-tests/build.gradle.kts | sed -n '1,30p'
# Search for version catalog files in the project root
fd "versions.toml|gradle.toml|settings.gradle.kts" --max-depth 3
# Check if Jackson 3.x is used anywhere else
rg -n "tools\.jackson" --type=kotlin -g "*.gradle.kts"
# Look for any explicit version specifications for the problematic dependency
rg -n "jackson-databind" --type=kotlinRepository: Puneethkumarck/stablebridge-platform
Length of output: 1609
🏁 Script executed:
#!/bin/bash
# Check for dependency management or constraints
cat -n phase2-integration-tests/build.gradle.kts | sed -n '1,50p'
# Check if there's a root gradle file with dependencyManagement
fd "build.gradle.kts" --max-depth 1 -x head -50 {}
# Search for any dependency constraints or exclusions
rg -n "exclude|constraint|dependencyManagement" --type=kotlin phase2-integration-tests/Repository: Puneethkumarck/stablebridge-platform
Length of output: 3487
Remove Jackson 3.x or exclude Jackson 2.x transitively — classpath conflict detected.
The explicit tools.jackson.core:jackson-databind (Jackson 3.x) conflicts with Jackson 2.x (com.fasterxml.jackson.core) pulled transitively by the imported API modules (payment-orchestrator-api, compliance-travel-rule-api, fx-liquidity-engine-api). Spring Boot's BOM also manages Jackson 2.x, making the 3.x dependency incompatible on the same classpath. Additionally, the dependency lacks a version specification; follow the project convention and parameterize it via gradle.properties or align with Spring Boot's managed version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@phase2-integration-tests/build.gradle.kts` around lines 17 - 18, The test
dependency declaration testImplementation("tools.jackson.core:jackson-databind")
brings Jackson 3.x and conflicts with Jackson 2.x on the classpath; remove this
explicit Jackson 3.x entry or replace it with the project-managed Jackson 2.x
artifact/version. Either delete the tools.jackson.core line and rely on Spring
Boot's BOM-managed com.fasterxml.jackson.core, or change to the
com.fasterxml.jackson.core:jackson-databind coordinate parameterized from
gradle.properties (e.g. use a jacksonVersion property) so the version is
consistent with Spring Boot; if any imported modules (payment-orchestrator-api,
compliance-travel-rule-api, fx-liquidity-engine-api) pull in Jackson 2.x
transitively and you must keep the current declaration, add an exclusion on
those module dependencies for com.fasterxml.jackson.core to avoid mixed 2.x/3.x
classes.
Comment on lines
+148
to
+151
| var workflowId = "payment-" + entry.getValue(); | ||
| WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId); | ||
| var result = stub.getResult(Map.class); | ||
| var status = result.get("status"); |
There was a problem hiding this comment.
Missing timeout on workflow result fetch.
stub.getResult(Map.class) blocks indefinitely. In a load test with 100 concurrent workflows, a single stuck workflow will hang the entire test. Use the timeout overload.
🐛 Proposed fix
try {
var workflowId = "payment-" + entry.getValue();
WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId);
- var result = stub.getResult(Map.class);
+ var result = stub.getResult(Duration.ofSeconds(60), Map.class);
var status = result.get("status");📝 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
| var workflowId = "payment-" + entry.getValue(); | |
| WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId); | |
| var result = stub.getResult(Map.class); | |
| var status = result.get("status"); | |
| var workflowId = "payment-" + entry.getValue(); | |
| WorkflowStub stub = workflowClient.newUntypedWorkflowStub(workflowId); | |
| var result = stub.getResult(Duration.ofSeconds(60), Map.class); | |
| var status = result.get("status"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phase2-integration-tests/src/test/java/com/stablecoin/payments/phase2/Phase2LoadTest.java`
around lines 148 - 151, The call to stub.getResult(Map.class) can block
indefinitely; replace it with the timeout overload (use the WorkflowStub
returned from workflowClient.newUntypedWorkflowStub and call the getResult
variant that accepts a timeout) so each workflow fetch uses a bounded Duration
(e.g., a sensible seconds/minutes value) and handle the timeout by
failing/logging and cancelling or terminating the stuck workflow; ensure you
reference WorkflowStub stub, workflowClient.newUntypedWorkflowStub(workflowId)
and replace stub.getResult(Map.class) with the timeout-taking overload and add a
catch for the timeout exception to clean up the workflow and fail the test
accordingly.
… + load test (STA-13) Add Phase 2 integration testing infrastructure that runs S1, S2, S6 together via Docker Compose with Temporal workflow orchestration and Kafka event verification. Test scenarios: - Happy path: payment initiated → compliance passes → FX locked → COMPLETED - Compliance rejection: sanctions hit via WireMock → workflow FAILED - Cancel: cancel endpoint returns 200/409/500 depending on race timing - Idempotency: duplicate key returns existing payment (200 OK) Infrastructure fixes required for cross-service communication: - S1 KafkaProducerConfig: explicit bean for Spring Boot 4 compatibility - S1 FeignIdempotencyInterceptor: auto-inject Idempotency-Key header on Feign calls - S6 LockExpiryJob: use OutboxEventPublisher directly (generic type mismatch fix) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Puneethkumarck
force-pushed
the
feature/STA-13-phase2-integration-tests
branch
from
4a6a8c6 to
b01327d
Compare
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
KafkaProducerConfig— explicit Kafka template bean for Spring Boot 4FeignIdempotencyInterceptor— auto-injectIdempotency-Keyheader on all Feign callsLockExpiryJob— useOutboxEventPublisherdirectly (generic type mismatch fix)Test Infrastructure
docker-compose.phase2-test.yml— standalone stack with all infrastructure + 3 servicesscripts/run-phase2-tests.sh— orchestrates build → compose up → health check → test → teardownTest plan
Closes LIN-13
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Testing & Quality Assurance