MX-241: FIX propagate tenant context to async notification threads#151
Merged
IOhacker merged 1 commit intoopenMF:developfrom Apr 16, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 38 seconds. ⌛ 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughPull request adds tenant and business-date context propagation to the self-service notification flow, extending events with tenant/business-date fields, capturing/restoring them across async boundaries, updating publishers and executor decoration, and adding tests for propagation behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthAPI as SelfAuthenticationApiResource
participant ThreadLocal as ThreadLocalContextUtil
participant Event as SelfServiceNotificationEvent
participant Executor as ThreadPoolTaskExecutor
participant NotifService as SelfServiceNotificationService
participant Listener as NotificationListener
Client->>AuthAPI: login request
AuthAPI->>ThreadLocal: getTenant()/getBusinessDates()
ThreadLocal-->>AuthAPI: tenant + dates (or error)
AuthAPI->>Event: withTenantContext(...) factory (captures thread-local)
Event-->>AuthAPI: event with tenant/dates
AuthAPI->>Executor: publish event (async)
Executor->>Executor: TaskDecorator may capture submitting-thread tenant
Executor->>ThreadLocal: setTenant(captured) [if any]
Executor->>Listener: execute listener -> NotifService.handleNotification(event)
NotifService->>ThreadLocal: restoreTenantContext(event) -> setTenant(event.tenant), setBusinessDates(event.businessDates)
NotifService->>NotifService: process notification under restored context
NotifService->>ThreadLocal: reset() (cleanup)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java (1)
75-82:⚠️ Potential issue | 🟠 MajorGuard the capture-side
getTenant()call with try-catch.The call to
ThreadLocalContextUtil.getTenant()at line 76 does not guard againstIllegalStateException, which is thrown when no tenant is bound. The TaskDecorator executes asynchronously and may run in a context without an active tenant binding. Other parts of the codebase (SelfServiceNotificationEvent.javalines 146–150 andSelfServiceNotificationService.javalines 272–276) already establish the pattern of wrapping this call in try-catch forIllegalStateException. Apply the same pattern here.Proposed change
- FineractPlatformTenant tenant = ThreadLocalContextUtil.getTenant(); + FineractPlatformTenant tenant = null; + try { + tenant = ThreadLocalContextUtil.getTenant(); + } catch (IllegalStateException ignored) { + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java` around lines 75 - 82, Wrap the call to ThreadLocalContextUtil.getTenant() inside a try-catch that catches IllegalStateException (as done elsewhere in SelfServiceNotificationEvent and SelfServiceNotificationService) before capturing tenant for the executor.setTaskDecorator lambda; if getTenant() throws, set the captured tenant to null (or leave unset) so the returned Runnable still checks tenant != null before calling ThreadLocalContextUtil.setTenant(tenant). This ensures executor.setTaskDecorator and the captured businessDates from currentBusinessDates() remain unchanged while preventing uncaught IllegalStateException when no tenant is bound.
🧹 Nitpick comments (2)
src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java (1)
47-48: Use AssertJ assertion style for this test assertion.Line 48 should use AssertJ instead of
assertTrue(...)to match test standards.Proposed change
-import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; ... - assertTrue(token.matches("\\d{6}")); + assertThat(token).matches("\\d{6}");As per coding guidelines,
src/test/java/**: "Use JUnit 5, Mockito, and AssertJ."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java` around lines 47 - 48, In SelfServiceAuthorizationTokenServiceTest replace the JUnit assertTrue usage with an AssertJ assertion: locate the test where the token is generated (in class SelfServiceAuthorizationTokenServiceTest) and change assertTrue(token.matches("\\d{6}")) to an AssertJ form (e.g. assertThat(token).matches("\\d{6}")), adding the static import from org.assertj.core.api.Assertions if missing.src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.java (1)
61-73: MakebusinessDatesunmodifiable before publishing the event.The class is documented as immutable, but Lombok currently exposes the internal
HashMapdirectly. That lets callers mutate the snapshot after publication, and the async listener will then consume the mutated state. Store this as aMapand wrap the defensive copy in an unmodifiable view.Proposed change
-import java.util.HashMap; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; @@ - private final HashMap<BusinessDateType, LocalDate> businessDates; + private final Map<BusinessDateType, LocalDate> businessDates; @@ - FineractPlatformTenant tenant, HashMap<BusinessDateType, LocalDate> businessDates) { + FineractPlatformTenant tenant, HashMap<BusinessDateType, LocalDate> businessDates) { @@ - this.businessDates = businessDates != null ? new HashMap<>(businessDates) : null; + this.businessDates = businessDates != null ? Collections.unmodifiableMap(new HashMap<>(businessDates)) : null;Also applies to: 112-127
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 534-545: The tenant snapshotting call in
SelfServiceRegistrationWritePlatformServiceImpl.confirmEnrollment uses
ThreadLocalContextUtil.getTenant() without handling IllegalStateException, which
can cause the transaction to rollback; update the code to mirror the defensive
pattern used for businessDatesSnapshot and
SelfServiceNotificationEvent.withTenantContext: wrap the getTenant() call in a
try-catch that catches IllegalStateException (or RuntimeException) and sets
tenantSnapshot to null on failure, leaving businessDatesSnapshot logic unchanged
so the notification remains best-effort and the enrollment transaction is not
rolled back.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationTenantPropagationIntegrationTest.java`:
- Around line 104-125: Test is currently duplicating production context
restoration by calling ThreadLocalContextUtil.setTenant(...) and
setBusinessDates(...) inside executor.execute, so replace that manual
restoration with invoking the real listener/service to exercise actual logic:
submit SelfServiceNotificationService.handleNotification(event) (or publish the
event via Spring application context) on the executor instead of calling
ThreadLocalContextUtil.setTenant/setBusinessDates, then keep the same
workerTenantId/workerBusinessDate reads and latch.countDown() in the finally to
assert observed values; also update the other identical test block that uses the
same manual restoration pattern to do the same replacement.
---
Outside diff comments:
In
`@src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java`:
- Around line 75-82: Wrap the call to ThreadLocalContextUtil.getTenant() inside
a try-catch that catches IllegalStateException (as done elsewhere in
SelfServiceNotificationEvent and SelfServiceNotificationService) before
capturing tenant for the executor.setTaskDecorator lambda; if getTenant()
throws, set the captured tenant to null (or leave unset) so the returned
Runnable still checks tenant != null before calling
ThreadLocalContextUtil.setTenant(tenant). This ensures executor.setTaskDecorator
and the captured businessDates from currentBusinessDates() remain unchanged
while preventing uncaught IllegalStateException when no tenant is bound.
---
Nitpick comments:
In
`@src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java`:
- Around line 47-48: In SelfServiceAuthorizationTokenServiceTest replace the
JUnit assertTrue usage with an AssertJ assertion: locate the test where the
token is generated (in class SelfServiceAuthorizationTokenServiceTest) and
change assertTrue(token.matches("\\d{6}")) to an AssertJ form (e.g.
assertThat(token).matches("\\d{6}")), adding the static import from
org.assertj.core.api.Assertions if missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3a37f41-6c7c-45f6-8072-dec689f96172
📒 Files selected for processing (9)
src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationTenantPropagationIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java
Comment on lines
+104
to
+125
| executor.execute(() -> { | ||
| try { | ||
| if (event.getTenant() != null) { | ||
| ThreadLocalContextUtil.setTenant(event.getTenant()); | ||
| } | ||
| if (event.getBusinessDates() != null) { | ||
| ThreadLocalContextUtil.setBusinessDates(event.getBusinessDates()); | ||
| } | ||
| try { | ||
| workerTenantId.set(ThreadLocalContextUtil.getTenant().getTenantIdentifier()); | ||
| } catch (IllegalStateException e) { | ||
| workerTenantId.set("NO_TENANT"); | ||
| } | ||
| try { | ||
| workerBusinessDate.set( | ||
| ThreadLocalContextUtil.getBusinessDates().get(BusinessDateType.BUSINESS_DATE)); | ||
| } catch (Exception ignored) { | ||
| } | ||
| } finally { | ||
| latch.countDown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Exercise the real listener instead of replaying the fix inside the test.
These assertions manually call ThreadLocalContextUtil.setTenant(event.getTenant()) and setBusinessDates(...) on the worker thread. That duplicates the production restoration logic, so the test still passes even if SelfServiceNotificationService.handleNotification() stops restoring context. Publish through Spring or run the real service on the executor so this actually guards the regression.
Also applies to: 179-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationTenantPropagationIntegrationTest.java`
around lines 104 - 125, Test is currently duplicating production context
restoration by calling ThreadLocalContextUtil.setTenant(...) and
setBusinessDates(...) inside executor.execute, so replace that manual
restoration with invoking the real listener/service to exercise actual logic:
submit SelfServiceNotificationService.handleNotification(event) (or publish the
event via Spring application context) on the executor instead of calling
ThreadLocalContextUtil.setTenant/setBusinessDates, then keep the same
workerTenantId/workerBusinessDate reads and latch.countDown() in the finally to
assert observed values; also update the other identical test block that uses the
same manual restoration pattern to do the same replacement.
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
6b635a4 to
23c1fac
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java (1)
534-548: Consider using imports instead of fully qualified class names.The inline fully qualified class names reduce readability. Adding imports would be cleaner.
Suggested refactor
Add imports at the top of the file:
import java.time.LocalDate; import java.util.HashMap; import org.apache.fineract.infrastructure.businessdate.domain.BusinessDateType; import org.apache.fineract.infrastructure.core.domain.FineractPlatformTenant; import org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil;Then simplify the snapshotting code:
- org.apache.fineract.infrastructure.core.domain.FineractPlatformTenant capturedTenant = null; + FineractPlatformTenant capturedTenant = null; try { - capturedTenant = org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil.getTenant(); + capturedTenant = ThreadLocalContextUtil.getTenant(); } catch (IllegalStateException ignored) { } - final org.apache.fineract.infrastructure.core.domain.FineractPlatformTenant tenantSnapshot = capturedTenant; - final java.util.HashMap<org.apache.fineract.infrastructure.businessdate.domain.BusinessDateType, java.time.LocalDate> businessDatesSnapshot; - java.util.HashMap<org.apache.fineract.infrastructure.businessdate.domain.BusinessDateType, java.time.LocalDate> tempDates = null; + final FineractPlatformTenant tenantSnapshot = capturedTenant; + final HashMap<BusinessDateType, LocalDate> businessDatesSnapshot; + HashMap<BusinessDateType, LocalDate> tempDates = null; try { - java.util.HashMap<org.apache.fineract.infrastructure.businessdate.domain.BusinessDateType, java.time.LocalDate> dates = - org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil.getBusinessDates(); + HashMap<BusinessDateType, LocalDate> dates = ThreadLocalContextUtil.getBusinessDates(); tempDates = dates != null ? new java.util.HashMap<>(dates) : null; } catch (IllegalArgumentException e) { } businessDatesSnapshot = tempDates;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java` around lines 534 - 548, Replace the repeated fully-qualified type names in SelfServiceRegistrationWritePlatformServiceImpl with imports to improve readability: add imports for FineractPlatformTenant, ThreadLocalContextUtil, BusinessDateType, HashMap and LocalDate at the top of the file, then update the snapshot block (where capturedTenant is assigned via ThreadLocalContextUtil.getTenant() and businessDatesSnapshot is built via ThreadLocalContextUtil.getBusinessDates()) to use the imported simple class names (FineractPlatformTenant, ThreadLocalContextUtil, HashMap<BusinessDateType, LocalDate>, LocalDate) instead of the long package-qualified names while keeping the existing try/catch and assignment logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.java`:
- Around line 534-548: Replace the repeated fully-qualified type names in
SelfServiceRegistrationWritePlatformServiceImpl with imports to improve
readability: add imports for FineractPlatformTenant, ThreadLocalContextUtil,
BusinessDateType, HashMap and LocalDate at the top of the file, then update the
snapshot block (where capturedTenant is assigned via
ThreadLocalContextUtil.getTenant() and businessDatesSnapshot is built via
ThreadLocalContextUtil.getBusinessDates()) to use the imported simple class
names (FineractPlatformTenant, ThreadLocalContextUtil, HashMap<BusinessDateType,
LocalDate>, LocalDate) instead of the long package-qualified names while keeping
the existing try/catch and assignment logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec20a3f9-376c-4abb-b22b-28c6f63b9383
📒 Files selected for processing (9)
src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.javasrc/main/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationService.javasrc/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.javasrc/main/java/org/apache/fineract/selfservice/registration/service/SelfServiceRegistrationWritePlatformServiceImpl.javasrc/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationTenantPropagationIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.javasrc/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.javasrc/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/java/org/apache/fineract/selfservice/notification/SelfServiceSmtpFallbackIntegrationTest.java
- src/main/java/org/apache/fineract/selfservice/security/api/SelfAuthenticationApiResource.java
- src/test/java/org/apache/fineract/selfservice/notification/service/SelfServiceNotificationServiceTest.java
- src/test/java/org/apache/fineract/selfservice/registration/service/SelfServiceAuthorizationTokenServiceTest.java
- src/main/java/org/apache/fineract/selfservice/notification/starter/SelfServiceNotificationConfig.java
- src/main/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationEvent.java
- src/test/java/org/apache/fineract/selfservice/notification/SelfServiceNotificationTenantPropagationIntegrationTest.java
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
2 times, most recently
from
b43a178 to
23c1fac
Compare
…eads Embed FineractPlatformTenant and business dates into SelfServiceNotificationEvent so async worker threads can restore the correct tenant context independently of the HTTP request thread lifecycle. Root cause: afterCommit() callbacks fire after Fineract's auth filter clears ThreadLocalContextUtil, leaving the TaskDecorator with null tenant. Async notification threads therefore ran under [no-tenant] and failed on any DB lookup. Changes: - SelfServiceNotificationEvent: add tenant/businessDates fields + withTenantContext() factory - SelfServiceNotificationService: restore tenant from event at top of handleNotification() - SelfServiceRegistrationWritePlatformServiceImpl: capture tenant snapshot before afterCommit() - SelfAuthenticationApiResource: use withTenantContext() factory for all event publications - SelfServiceNotificationConfig: guard TaskDecorator with null-check (belt-and-suspenders) - SelfServicePluginEmailService: wrap SmtpConfigurationUnavailableException in PlatformEmailSendException; enable mail.smtp.auth only when username is present Tests: - SelfServiceNotificationServiceTest: 4 new unit tests for tenant restoration - SelfServiceNotificationTenantPropagationIntegrationTest: 3 new integration tests simulating full afterCommit → @async → listener lifecycle - SelfServiceSmtpFallbackIntegrationTest: add @DirtiesContext to prevent state leakage - SelfServiceAuthorizationTokenServiceTest: align with upstream MIN_NUMERIC_LENGTH=6
DeathGun44
force-pushed
the
MX-241-harden-notification-smtp-fallback
branch
from
23c1fac to
5eb40b6
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
2 participants
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.
Embed FineractPlatformTenant and business dates into SelfServiceNotificationEvent so async worker threads can restore the correct tenant context independently of the HTTP request thread lifecycle.
Root cause: afterCommit() callbacks fire after Fineract's auth filter clears ThreadLocalContextUtil, leaving the TaskDecorator with null tenant. Async notification threads therefore ran under [no-tenant] and failed on any DB lookup
Summary by CodeRabbit
New Features
Bug Fixes
Tests