Skip to content

fix(concurrency): make the account application decision one-shot from REQUESTED - #61

Merged
hongwei1 merged 1 commit into
develop-obpfrom
fix/account-application-one-shot-status
Aug 4, 2026
Merged

fix(concurrency): make the account application decision one-shot from REQUESTED#61
hongwei1 merged 1 commit into
develop-obpfrom
fix/account-application-one-shot-status

Conversation

@hongwei1

@hongwei1 hongwei1 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

What

MappedAccountApplicationProvider.updateStatus guarded its conditional UPDATE on the status it had just loaded rather than on the fixed starting status:

conditionalAccountApplicationStatus(accountApplication.id.get, accountApplication.status, status)

That only catches the interleaving where two callers read the same old value. When the calls serialise, the second one loads what the first wrote, its guard matches, and it overwrites the decision with no error.

So a REJECTED application could be re-decided as ACCEPTED. The ACCEPTED branch of PUT /banks/BANK_ID/account-applications/ACCOUNT_APPLICATION_ID opens a bank account, so that overwrite is not recoverable.

Change

Guard on REQUESTED, matching the sibling transitions in DoobieBusinessStatusQueriesAccountAccessRequest guards on INITIATED, the challenge CAS on successful_c = false. The zero-row failure message is reworded because it now also covers "a decision was already recorded", and the initial status set at creation reuses the same constant.

Behaviour change: a second decision on an application that has left REQUESTED now fails instead of silently overwriting. Previously only ACCEPTED was terminal (via an in-memory check); REJECTED was not.

Why now

This is what made the ConcurrentBusinessStatusRaceTest M3 scenario intermittently red — most visibly on the first attempt of run 30860104286, which passed on re-run. M3 only failed when its two threads happened to serialise with REJECTED landing first; when they interleave, both read REQUESTED and the old guard did catch the loser. The reverse serialisation was caught by the in-memory ACCEPTED check, so roughly half of the serialised runs went red.

M3b reproduces the same defect deterministically with two ordinary sequential calls — no threads, no barrier, no timing dependency. It fails on the parent commit and passes here.

Scope

M2 and M4 were checked and need no change: both already guard on a fixed starting state. M1 (the FOR UPDATE lock in lockTransactionRequest being a no-op outside a request scope) is a separate, already-documented issue and is untouched.

Verification

  • M3b red before the production change (accepted=Full(...ACCEPTED), finalStatus=ACCEPTED), green after.
  • Full local suite: 3274 tests, 0 failures, 0 errors (./run_tests_parallel.sh, 4 shards).

… REQUESTED

updateStatus guarded its conditional UPDATE on the status it had just loaded
instead of the fixed starting status. That only caught the interleaving where
two callers read the same old value; when the calls serialise, the second one
loads what the first wrote, its guard matches, and it overwrites the decision
with no error. A REJECTED application could therefore be re-decided as
ACCEPTED — and the ACCEPTED branch of the endpoint opens a bank account, so
the overwrite is not recoverable.

Guard on REQUESTED, matching the sibling transitions in
DoobieBusinessStatusQueries (AccountAccessRequest guards on INITIATED, the
challenge CAS on successful_c=false). Reword the zero-row failure, which now
also covers "a decision was already recorded", and reuse the same constant for
the initial status set at creation.

This is what made the M3 race scenario intermittently red: it failed only when
the two threads happened to serialise with REJECTED landing first. Add M3b,
which reproduces the same defect deterministically with two sequential calls.
@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
5.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@hongwei1
hongwei1 merged commit 7115f57 into develop-obp Aug 4, 2026
24 of 25 checks passed
@hongwei1

hongwei1 commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Follow-up measurement, recorded for the archive. The claim in the PR description that this defect is what made M3 intermittently red was reasoning, not measurement — M3 itself passed locally throughout. Here are the numbers.

Harness: a temporary probe suite (not committed) running the M3 race repeatedly with real threads (CyclicBarrier(2), thread 0 → ACCEPTED, thread 1 → REJECTED), asserting nothing so the loop always completes. Both guards, two scheduling conditions.

condition old guard (accountApplication.status) new guard (REQUESTED)
barrier only, 200 iterations both=0 one=200 none=0 both=0 one=200 none=0
ACCEPTED writer delayed 150 ms, 30 iterations both=30 one=0, finalStatus ACCEPTED ×30 both=0 one=30, finalStatus REJECTED ×30

Both 200-iteration runs ended with finalStatus split roughly ACCEPTED 100 / REJECTED 100, so the race is genuinely happening and either side can win — there was simply never a run where both won.

Two things follow.

The old code was already correct on the path where both threads read REQUESTED — zero leaks in 200 iterations. So M3's failures cannot have come from that interleaving. The defect is reachable only when the two threads are pulled apart into a first and a second, and there the old guard leaks on every iteration while the new one holds on every iteration.

The staggered runs reproduce the CI signature exactly: finalStatus=ACCEPTED with both writers reporting success, which is what run 30860104286 printed on its first attempt. This machine (M2 Max) never pulled the threads apart on its own across 200 iterations; the CI runner is 2-core under container noise with a cold JIT, and pulled them apart once in about eight runs.

Reproduction status, stated precisely: the defect itself reproduces deterministically and is covered by the committed M3b scenario. The concurrent M3 failure reproduces only with an injected stagger — without one it does not appear locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant