fix(concurrency): batch-2 hazards C1, H1-H7, M1-M9 (17 fixes)#2847
Merged
simonredfern merged 31 commits intoJul 2, 2026
Merged
Conversation
…ards Add five ScalaTest suites under code.concurrency (tagged ConcurrencyRace) covering hazards found in the second codebase scan: - ConcurrentBulkPaymentRaceTest C1 - ConcurrentConsentStatusRaceTest H1, H2, H3, M5 - ConcurrentRateLimiterRaceTest H4, M6, M7 - ConcurrentMutableSingletonRaceTest H5, H7, H6, M8, M9 - ConcurrentBusinessStatusRaceTest M1, M2, M3, M4 Behavioural suites reproduce the race; structural suites (H6/M8/M9) assert the hardening primitive (volatile field / concurrent map) is present. Redis-backed suites self-skip when Redis is unavailable. Fixes land per file in follow-ups.
…e (C1) createTransactionRequestBulk ran claimBatchReference inside a bare Future and dropped the result. Two concurrent submissions with the same batch_reference both passed the earlier isBatchReferenceUsed check, then both proceeded — the losing INSERT hit the UniqueIndex and returned a Failure that was silently discarded, so a duplicate payment was fanned out. Claim the batch_reference BEFORE creating the parent transaction request or fanning out any payment, and surface the Failure via unboxFullOrFail (409) so the losing request aborts before charging.
… M5) checkAnswer / revoke / skip-SCA-accept all did find-then-saveMe with an in-memory status check, so concurrent requests could both pass the guard and both write: two correct answers both accepted (H1, H2), or a skip-SCA / challenge accept overwriting an explicit revoke (H3, M5). Replace the unconditional saveMe with guarded conditional UPDATEs (UPDATE ... WHERE id = ? AND mstatus = <guard>) via new DoobieConsentStatusQueries / DoobieUserAuthContextUpdateQueries. The loser of a race gets 0 affected rows and a Failure instead of a second success. revoke uses WHERE mstatus <> 'REVOKED' so an explicit revocation always wins.
…7, M8, H6, M9)
- DynamicConnector.singletonObjectMap, SecureLogging.customPatternCache and APIUtil.connectorToEndpoint
were scala.collection.mutable.Map, whose put/getOrElseUpdate are not thread-safe and can lose
writes or corrupt the map during a concurrent resize. Switch to scala.collection.concurrent.TrieMap
(same Map API, lock-free).
- ObpLookupSystem.obpLookupSystem and ObpActorSystem.{obpActorSystem, northSideAkkaConnectorActorSystem}
were plain vars with unguarded null-check-then-assign init. Mark @volatile and use synchronized
double-checked init so a write publishes safely and the system is created exactly once.
…4, M6, M7) Add two atomic Redis primitives: setNxEx (SET key value EX ttl NX, the Jedis 2.9.0 five-arg overload) and incrementWithTtl (a Lua INCR + create-TTL). - H4: RateLimitingUtil.incrementCounter replaced its TTL-read-then-SET/INCR sequence with the atomic incrementWithTtl, so concurrent callers cannot lose increments or race the expiry. - M6: IdempotencyMiddleware.writeResponseKey used setex (unconditional overwrite); now setNxEx, so the first cached response for an idempotency key is immutable and a replay returns the correct body. - M7: IdempotencyMiddleware.tryAcquireLock used setnx + a separate expire (a crash between left a TTL-less key blocking all retries); now a single atomic setNxEx sets value and TTL together.
…s update (M1, M2, M3, M4) - M2 AccountAccessRequest.updateStatus had no terminal-state guard; M3 MappedAccountApplication and M4 MappedChallengeProvider.validateChallenge checked status/success in memory then saveMe. Concurrent requests could both write. Add conditional UPDATEs via new DoobieBusinessStatusQueries: action an access request only from INITIATED (M2), transition an application only from its loaded status (M3, optimistic CAS), and compare-and-set the challenge success flag from false (M4) so one challenge can never green-light a payment twice. NB: the challenge `Successful` field maps to column successful_c. - M1 Http4s510 updateTransactionRequestStatus now acquires lockTransactionRequest within the request transaction (mirroring Http4s400) so it cannot overwrite a status set by the racing challenge path.
Thread 0 was simulating the old unconditional .saveMe() write instead of calling DoobieConsentStatusQueries.conditionalStatusTransition, causing the test to demonstrate the pre-fix bug rather than verify the fix. Update to use the same conditional UPDATE path as the production Http4s310 code.
…on status lock atomicallyLockTransactionRequest used .query[String].unique, which raises UnexpectedEnd when the transaction-request row does not exist. runUpdate runs it with unsafeRunSync, so the exception propagates; lockTransactionRequest's tryo converts it to a Failure box, the caller's .isDefined is then false, and booleanToFuture fires its default 400 before the handler reaches getTransactionRequestImpl — so an authorised caller hitting a non-existent request id got 400 'Failed to acquire transaction request lock' instead of 404. Switch to .query[String].option so a missing row returns Full(None) (query ran cleanly) and only a genuine DB/lock error yields a Failure. The Box .isDefined check in both callers (Http4s510 status update, Http4s400 challenge answer) now passes for a missing row, letting the downstream lookup return the correct 404; the 400 is reserved for real lock failures.
The skip-SCA auto-accept did MappedConsent.find(...).map(c => conditionalStatusTransition(...)). On an Empty box the map yielded Empty, which the for-comprehension's _ <- discarded silently, leaving the consent INITIATED with no error. The consent is created earlier in the same request so Empty is effectively unreachable, but a silent no-op is wrong. Unwrap the box with openOrThrowException so a missing consent surfaces as a clear internal error, preserving the prior fail-visibly contract while keeping the atomic guarded INITIATED -> ACCEPTED transition.
Four concurrency-guard rejection messages introduced by the recent conditional-UPDATE fixes returned plain English strings instead of OBP-XXXXX coded errors, breaking the API's error-code contract (clients match on the OBP- prefix). - AccountAccessRequest.updateStatus: reuse the existing AccountAccessRequestStatusNotInitiated (OBP-30278) — the same condition the caller already checks non-concurrently. - MappedConsent.checkAnswer: reuse the existing ConsentUpdateStatusError (OBP-35025). - MappedUserAuthContextUpdateProvider.checkAnswer: add UserAuthContextUpdateStatusError (OBP-30090, filling a gap in the 300XX block) — no existing constant covered this case. - Http4s510 updateTransactionRequestStatus lock gate: add TransactionRequestLockFailed (OBP-40050) — no existing constant covered this case.
… createConsent The conditional INITIATED -> ACCEPTED auto-accept was only applied to the v3.1.0 createConsent path; Http4s500 and Http4s510 still carried the verbatim pre-fix line .map(_.mStatus(ACCEPTED).saveMe()).head — both the concurrent-revoke resurrection race and the .head NoSuchElementException on a vanished consent remained live on the two current API versions. Add conditionalStatusTransitionByConsentId (keyed by consent id, no extra SELECT to obtain the row id) and use it at all three call sites. The M5 race test now exercises this exact shared helper. The v3.1.0 site also drops its redundant re-find of the consent it created moments earlier.
…n approval approveAccountAccessRequest granted view access BEFORE attempting the INITIATED -> APPROVED transition. When a concurrent rejection won the status race, the approver got a 400 but the already-committed grant stood: the target user kept view access on a request whose persisted status said REJECTED. Winning the conditional UPDATE first makes the CAS the single gate — the loser exits with no side effect.
…omically
The Lua rewrite dropped the old implementation's recovery branch for a counter key
stuck without an expiry (legacy writer, PERSIST, RDB restore dropping expiries): such
a key incremented forever and permanently rate-limited its consumer. The script now
recreates the window (count=1, fresh TTL) when it finds a key with TTL < 0.
The script also returns {count, ttl} together, removing the separate TTL round trip
per period per request — halving increment-phase Redis I/O on the hottest path — and
eliminating the non-atomic TTL read that could observe a different window (or fabricate
a default on failure). Connection boilerplate consolidated into a withJedis loan helper.
…n fails The batch_reference claim is deliberately committed before any payment runs, but a parent-TR creation failure (500) left the claim in place: every retry of a batch that never executed anything hit the 409 already-used guard, permanently burning the reference. Release the claim (scoped to the claiming transactionRequestId) before surfacing the 500 — at that point no payment has executed, so a retry is safe. The claim is intentionally never released after the fan-out, where payments may have partially executed.
…veMe The replaced Lift saveMe() path wrote updatedAt (CreatedUpdated trait) on every status change; the raw-SQL conditional UPDATEs did not, so rows exposed through JSON 'updated' fields (e.g. AccountAccessRequest v6.0.0) showed a status change frozen at the creation timestamp. All conditional UPDATEs now set updatedat = NOW() alongside the status.
…a no-op DoobieUtil.runUpdate silently falls back to a standalone auto-commit transactor when no request-scoped connection is available; a SELECT ... FOR UPDATE issued there acquires and immediately releases the row lock while still reporting success, so a background or scheduler caller would proceed believing it holds mutual exclusion. Expose hasRequestScopeConnection and log a loud warning from lockTransactionRequest when the lock cannot actually be held.
…cks and messages - MappedChallengeProvider: the 9-line compare-and-set block was copy-pasted into both userId match arms; extract markChallengeSuccessful and collapse the arms into one answer+user check (userId.forall), removing the duplicated wrong-answer branch too. - MappedConsent.revoke: use rows == 1 like every other id-keyed CAS site (>= 1 implied a multi-row possibility that an id-keyed UPDATE cannot have). - MappedAccountApplication: the CAS-loser branch reported 'already been accepted' (OBP-30314) even when the concurrent winner wrote REJECTED; use the generic UpdateAccountApplicationStatusError (OBP-30315) instead. - run_specific_tests.sh: drop stale references to the deleted run_all_tests.sh.
Three reliability holes let a failing run look green: - timeout exit 124 was unconditionally converted to success, so a shard whose JVM was hard-killed BEFORE tests completed still reported BUILD SUCCESS. Now 124 only counts as success when the shard log proves the tests finished (BUILD SUCCESS printed); otherwise it is a failure. - The verdict trusted only per-shard mvn exit codes. Add an authoritative surefire XML audit after the shards: any failure/error recorded in the reports fails the run and is listed by suite. Stale reports from earlier runs are deleted before the shards start so the audit (and the speed table) reflect only this run. - The ALL SHARDS PASSED / SOME SHARDS FAILED line was printed before the speed table, so piping through tail -N could truncate it. Print the verdict last, and also write PASS/FAIL to test-results/parallel/RESULT — a machine-readable signal that survives piping (a pipeline reports the LAST command's exit code, so './run_tests_parallel.sh | tail' returns tail's 0 even when the script exits 1).
Under json4s, JValue already extends scala.Product, so the implicit conversion JvalueToSuper (JValue => JvalueCaseClass) that the Lift-era ResourceDoc builder relied on never fires anymore — json.parse(...) now type-checks as a bare JValue without ever going through the wrapper. Whether exampleRequestBody ends up as a JvalueCaseClass or a raw JValue then depends on which other class initializes the formats/implicits first, making ConfirmationOfFundsServicePIISApiTest's .asInstanceOf[JvalueCaseClass] constructor-time cast fail intermittently (ClassCastException, suite aborts) depending on run ordering across shards. Wrap both example bodies in JvalueCaseClass explicitly at the ResourceDoc call site instead of relying on the dead implicit, and make the test's own body lookup pattern-match on both shapes so it no longer assumes one representation.
…CaseClass Same root cause as the earlier PIIS-only fix, extended to the other three v1.3 files: since the json4s migration, JValue already extends scala.Product, so the implicit conversion JvalueToSuper (JValue => JvalueCaseClass) that ResourceDoc construction relied on never fires. Every example body built via json.parse(...) across AIS, PIS, and SigningBaskets was therefore a raw JValue instead of the expected JvalueCaseClass wrapper — resource-docs serialization would reflect on it as a generic case class rather than taking the JvalueCaseClass special-case path, and any call site that asInstanceOf[JvalueCaseClass]'d the example body (as ConfirmationOfFundsServicePIISApiTest's constructor did) failed depending on class-init order. Wrap every example body explicitly at each ResourceDoc call site instead of relying on the dead implicit; drop the now-unused implicitConversions import.
# Conflicts: # obp-api/src/main/scala/code/api/berlin/group/v1_3/Http4sBGv13PIIS.scala
The local parallel runner could pass while CI failed (or mask what CI tests): - Run -pl obp-commons,obp-api like CI does. The shards previously ran obp-api only, so obp-commons' five util suites never executed locally; the shard-4 catch-all even added com.openbankproject.commons.* to its filter, where it matched nothing and -DfailIfNoTests=false turned that into a silent pass. - Inject OBP_DYNAMIC_CODE_SANDBOX_PERMISSIONS, mirroring the dynamic_code_sandbox_permissions line CI appends to test.default.props. Its absence was the actual cause of the three perennial local DynamicResourceDocTest failures (sandbox denied reflection/getenv in the native-execution scenarios) — not a machine quirk. - Harden the surefire audit: scan both modules' reports, count unparseable XMLs as failures instead of skipping them, report skipped/canceled tests, and fail when fewer than 2000 tests ran in total (a broken wildcardSuites filter otherwise runs nothing and reports a hollow green thanks to -DfailIfNoTests=false). First fully green local run after this: 2917 tests, 0 failures, 0 errors.
conditionalAccountAccessRequestStatus/conditionalAccountApplicationStatus (rowId -> accountAccessRequestId / accountApplicationId) and DoobieUserAuthContextUpdateQueries .conditionalStatusTransition (rowId -> userAuthContextUpdateId) took a generic rowId parameter that gave no hint which entity's id it expected. All call sites pass arguments positionally, so no callers change.
consentRowId sat next to the business identifier consentId (String, mConsentId column) in the same call sites, inviting confusion between the two. Rename to consentPrimaryKey, matching the existing PrimaryKey naming convention elsewhere in the codebase (authUserPrimaryKey, saveMetricsArchive's primaryKey param). Named-argument call sites in ConsentScheduler and ConcurrentConsentRaceTest updated to match.
…micDataIds The method returns DynamicDataAccess.DynamicDataId values, and the sibling method in the same trait already names that concept dynamicDataId (getAccessForRow). getReadableRowIds was the only outlier still using RowId terminology.
…el.sh Kept for anyone still typing ./run_all_tests.sh out of habit; it does no work of its own and just execs run_tests_parallel.sh with the same arguments.
|
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.
Summary
Fix 17 concurrency hazards identified in the batch-2 audit across consent
status transitions, Redis rate-limiting, mutable singletons, and business
status state machines.
Hazards addressed
Tests
Five
ConcurrencyRace*Testsuites added incode.concurrency; all passlocally and in CI (8-shard build green).
Also included
run_all_tests.sh(superseded byrun_tests_parallel.shwhichmirrors the CI shard structure and injects required env vars)