alberto-art3ch commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-4769288191
> **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.
We have applied some enhancements on this, and now we have:
- **Orphaned-lock cleanup hardened.**
_SavingsAccountLockRepository_.removeOrphanedLocksForProcessedAccounts now uses
the same correlated EXISTS guard as _Loan_, only deleting a lock when
_SavingsAccount.lastClosedBusinessDate_ == lock.lockPlacedOnCobBusinessDate.
This prevents un-processed accounts (worker/JVM crash, skip-without-error) from
being silently treated as done. The lock owner is now parameterized (IN
:lockOwners) to align with Loan and stay future-proof.
- **Deduplication**. ContextAwareTaskDecorator was moved into
_fineract-cob_ (_org.apache.fineract.cob.common)_ and is now shared by Loan,
Savings and Working-Capital-Loan; the byte-for-byte
_SavingsContextAwareTaskDecorator_ copy was removed. The filterRemainingData
logic was extracted into a shared, parameterized RemainingDataFilter so the
Loan helper and SavingsItemReader no longer duplicate it.
Some points to be consider, maybe in other PR
- Generalize CommonPartitioner (currently hardcoded to
retrieveLoanCOBPartitions / COBConstant.COB_PARAMETER) so
_SavingsCOBPartitioner_ can extend it instead of re-inlining it.
- Parameterize the loan_id column in _AbstractLockingService_ /
_LockingService_ so _SavingsLockingService_ can reuse the base instead of
forking it.
- Route _UnlockProcessedSavingsTasklet_ through the generic
_AccountLockService_<T> / _UnlockProcessedAccountsTasklet_<T> abstraction.
- Cosmetic: reconcile SavingsCOBConstant.SAVINGS_COB_PARAMETER with the
shared COBConstant.COB_PARAMETER key.
- Once inline COB cleanup is wired to the batch tasklet, extend
orphaned-lock cleanup to also cover SAVINGS_INLINE_COB_PROCESSING.
--
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]
