adamsaghy commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-4766795369
**Orphaned‑lock cleanup is weaker than Loan's and may unlock un‑processed
accounts.**
Loan's `deleteOrphanedLocksForProcessedAccounts` only removes a lock when it
can prove the account was actually processed:
```
WHERE lck.error IS NULL
AND lck.lockPlacedOnCobBusinessDate IS NOT NULL
AND lck.lockOwner IN :lockOwners
AND EXISTS (SELECT l FROM Loan l
WHERE l.id = lck.loanId
AND l.lastClosedBusinessDate =
lck.lockPlacedOnCobBusinessDate) ← key guard
```
The Savings query
(`SavingsAccountLockRepository.removeOrphanedLocksForProcessedAccounts`) drops
that correlated `EXISTS`:
`delete ... where lck.lockPlacedOnCobBusinessDate is not null and lck.error
is null
and lck.lockOwner = SAVINGS_COB_CHUNK_PROCESSING`
Without the `lastClosedBusinessDate == lockPlacedOnCobBusinessDate` check,
"no error recorded" is treated as "successfully processed." If a chunk fails in
a path that doesn't write an error onto the lock (worker/JVM crash, a skip that
only logs, any exception that bypasses
`ChunkProcessingSavingsItemListener.updateLockError`), this tasklet will still
delete the lock and silently treat the account as done — exactly the case
Loan's `EXISTS` clause was added to prevent. I'd add the same correlated check
against `SavingsAccount.lastClosedBusinessDate`. (Minor, related: Loan cleans
both COB lock owners via `COB_LOCK_OWNERS`; Savings only handles
`SAVINGS_COB_CHUNK_PROCESSING` — acceptable while inline COB isn't implemented,
but worth a note.)
**Duplication**
The framework already has generalized COB base types that Loan extends;
Savings re‑implements them as standalone copies rather than
reusing/parameterizing them:
- `SavingsCOBPartitioner` re‑inlines all of `CommonPartitioner` (the
`isEmpty→stopJob`, `StopWatch`, partition‑to‑context logic) instead of
extending it.
- `SavingsContextAwareTaskDecorator` is a byte‑for‑byte copy of the generic
`ContextAwareTaskDecorator` (no savings‑specific logic).
- `SavingsItemReader.filterRemainingData` duplicates
`BeforeStepLockingItemReaderHelper.filterRemainingData`.
- `SavingsLockingService/SavingsLockingServiceImpl` duplicate
`LockingService/AbstractLockingService` instead of extending it. (Partly
justified: the shared AbstractLockingService hardcodes the loan_id column, so
reuse needs that column parameterized — but the cleaner fix is to generalize
the base, not fork it.)
- `UnlockProcessedSavingsTasklet` re‑implements
`UnlockProcessedAccountsTasklet<T>` with a direct repository call instead of
going through the `AccountLockService<T>` abstraction.
One cosmetic note: the partitioner stores the COB parameter under a new
`SavingsCOBConstant.SAVINGS_COB_PARAMETER` key rather than the shared
`COBConstant.COB_PARAMETER`; it's internally consistent (reader and apply‑lock
read the same key), just inconsistent with the shared constant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
