Skip to content

FINERACT-2672: Isolate standing-instruction execution so one failure cannot revert the whole run#6078

Open
oluexpert99 wants to merge 1 commit into
apache:developfrom
TECHSERVICES-LIMITED:bugfix/FINERACT-2672
Open

FINERACT-2672: Isolate standing-instruction execution so one failure cannot revert the whole run#6078
oluexpert99 wants to merge 1 commit into
apache:developfrom
TECHSERVICES-LIMITED:bugfix/FINERACT-2672

Conversation

@oluexpert99

Copy link
Copy Markdown
Contributor
  • The Execute Standing Instruction job ran every due instruction inside a single transaction: the step is
    wired with the real JPA transaction manager, and AccountTransfersWritePlatformService.transferFunds is
    @transactional (REQUIRED), so it joined that one shared transaction. When an instruction failed (e.g.
    InsufficientAccountBalanceException from the withdrawal leg), Spring marked the shared transaction
    rollback-only, so at step commit the ENTIRE run rolled back -- every already-successful transfer was
    reverted and no history row (success or failed) was persisted. The tasklet then threw JobExecutionException,
    failing the whole job. The bug was self-concealing because the failure history was itself rolled back.
    - Move each instruction's work into a new StandingInstructionExecutionService with REQUIRES_NEW propagation.
    A successful execution (transfer + last_run_date stamp + success history) is one atomic boundary; a failure
    is recorded in a separate committed boundary, so a rolled-back transfer still leaves a durable failed record.
    Because the tasklet catches the failure before it reaches an outer transactional boundary, a failing
    instruction can no longer mark a sibling's transaction rollback-only.
    - Replace the raw string-concatenated history INSERT with a StandingInstructionHistory JPA entity and
    repository. The logged amount is now the amount actually transferred (zero on failure) rather than the
    attempted amount, and BigDecimal is preserved instead of being narrowed to double.
    - Stop throwing JobExecutionException for expected per-instruction outcomes (insufficient funds is a normal
    per-mandate result recorded in history): log a processed/succeeded/failed run summary instead, and guard the
    failure-history write so one history-write error cannot abort the remaining instructions.
    - Fix the inverted priority ordering: the retrieval query sorted ORDER BY atsi.priority DESC while the enum is
    URGENT(1)..LOW(4), so it processed LOW first; change it to ASC so URGENT runs first.
    - Add a unit test asserting a mixed run (one failing instruction among successes) completes without aborting
    and records every outcome, and an integration test that runs the actual scheduler job with an under-funded
    source and asserts the successful transfer persists while the failing instruction leaves a durable failed
    history row.

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per our guidelines
  • Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
  • Create/update unit or integration tests for verifying the changes made.
  • Follow our coding conventions.
  • Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
  • This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

…cannot revert the whole run

- The Execute Standing Instruction job ran every due instruction inside a single transaction: the step is
  wired with the real JPA transaction manager, and AccountTransfersWritePlatformService.transferFunds is
  @transactional (REQUIRED), so it joined that one shared transaction. When an instruction failed (e.g.
  InsufficientAccountBalanceException from the withdrawal leg), Spring marked the shared transaction
  rollback-only, so at step commit the ENTIRE run rolled back -- every already-successful transfer was
  reverted and no history row (success or failed) was persisted. The tasklet then threw JobExecutionException,
  failing the whole job. The bug was self-concealing because the failure history was itself rolled back.
- Move each instruction's work into a new StandingInstructionExecutionService with REQUIRES_NEW propagation.
  A successful execution (transfer + last_run_date stamp + success history) is one atomic boundary; a failure
  is recorded in a separate committed boundary, so a rolled-back transfer still leaves a durable failed record.
  Because the tasklet catches the failure before it reaches an outer transactional boundary, a failing
  instruction can no longer mark a sibling's transaction rollback-only.
- Replace the raw string-concatenated history INSERT with a StandingInstructionHistory JPA entity and
  repository. The logged amount is now the amount actually transferred (zero on failure) rather than the
  attempted amount, and BigDecimal is preserved instead of being narrowed to double.
- Stop throwing JobExecutionException for expected per-instruction outcomes (insufficient funds is a normal
  per-mandate result recorded in history): log a processed/succeeded/failed run summary instead, and guard the
  failure-history write so one history-write error cannot abort the remaining instructions.
- Fix the inverted priority ordering: the retrieval query sorted ORDER BY atsi.priority DESC while the enum is
  URGENT(1)..LOW(4), so it processed LOW first; change it to ASC so URGENT runs first.
- Add a unit test asserting a mixed run (one failing instruction among successes) completes without aborting
  and records every outcome, and an integration test that runs the actual scheduler job with an under-funded
  source and asserts the successful transfer persists while the failing instruction leaves a durable failed
  history row.

Signed-off-by: oluexpert99 <farooq@techservicehub.io>
@oluexpert99 oluexpert99 force-pushed the bugfix/FINERACT-2672 branch from 12f13fc to f994c43 Compare July 2, 2026 18:10
@Aman-Mittal Aman-Mittal added the Needs Functional Review PRs which pass build and have no obvious technical problems, but need functional review. label Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Functional Review PRs which pass build and have no obvious technical problems, but need functional review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants