FINERACT-2485: Standardize and harden idempotency

[vidakovic]

Description

Implement idempotency feature for new command processing.

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.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

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

[Aman-Mittal]

Some minor concerns here

[adamsaghy]

Considering the importance and complexity of the PR, AI was also used to review the behaviour.

Issues

1. Hook ordering puts the idempotency check after the audit insert → duplicate rows, then hard failure on the 3rd call

Orders in the single CommandHookBefore list (Spring sorts collection injection by @Order):

order	hook
10–12	servlet-headers / timestamp / username (fineract-command)
20	AuditCommandHookBefore → store.store(ctx) (INSERT, UNDER_PROCESSING)
30	IdempotencyKeyHeaderCommandHook → sets command.idempotencyKey
31	IdempotencyCheckCommandHook → lookup

Trace of requests with key K:

• Request 1 — audit-pre inserts row1 with idempotency_key = NULL (the key isn't read
  until order 30). Handler runs. AuditCommandHookAfter updates row1 (commandId was set by
  store) → row1 now has K.
• Request 2 — audit-pre inserts row2 with idempotency_key = NULL. Order 31 finds
  row1's response → INVALID → handler skipped. AuditCommandHookAfter still runs → updates
  row2 → row2 also gets K.
• Request 3 — audit-pre inserts row3, then getResponseByKey(K) →
  findOneByIdempotencyKey is an Optional-returning derived query hitting 2 rows →
  IncorrectResultSizeDataAccessException out of hookManager.before(...) → error hook →
  500 to the caller.

m_command_idempotency_key_index is not unique
(fineract-command-jdbc/src/main/resources/db/changelog/tenant/module/command/parts/0002_command_fix_structure.xml),
so nothing prevents this at the DB level.

We might want to consider use idempotency key + request combo for identifying duplicate requests.

Two independent fixes are needed:

• Resolve the idempotency key in the earliest before-hooks (≤ 12, alongside the other
  servlet-header extraction) and run the check before audit-pre, so a replay never inserts a
  row at all.
• Suppress the audit-after store on the replay path (or make store a no-op when the context
  is a replay).

Note this is also a regression relative to develop:
fineract-command/src/main/java/org/apache/fineract/command/hook/ServletHeadersCommandHook.java
previously set the key at order 10, so the audit INSERT carried it.

2. No atomic key claim → the idempotency guarantee doesn't hold under concurrency

IdempotencyCheckCommandHook is read-then-execute with no unique constraint and no claim
insert. Two simultaneous requests with the same key both see null and both execute the
handler — which for a financial command is the failure mode idempotency exists to prevent.

This needs a unique index on m_command.idempotency_key plus an insert-and-claim (catch the
constraint violation → replay or 409), and a Liquibase changeset that de-duplicates existing
rows before adding the constraint.

We should eagerly store incoming command + idempotency and fail-fast if such entries already exists: replay till result is gathered / timeout.

3. In-flight and failed commands are not detected

getResponseByKey returns null both when no record exists and when a record exists with a
null response (i.e. UNDER_PROCESSING or ERROR). So a client retry while the first call is
still running re-executes the handler.

The store already exposes getStateByKey — the hook should branch on state:

• PROCESSED → replay
• UNDER_PROCESSING → 409 / retry-after
• ERROR → decide explicitly whether a retry is allowed

4. INVALID is the wrong state for a replay

The code comment acknowledges this. Because AuditCommandHookAfter now persists
ctx.getState() instead of a hard-coded PROCESSED, a successful replay is recorded as
INVALID in the audit trail, conflating "request failed validation" with "already processed".
It also means any current or future hook that branches on INVALID will treat a replay as a
validation failure. A dedicated state (or a non-persisted skipExecution flag on the context)
is worth the extra enum value here.

5. No request-equivalence check on the key

Idempotency keys are client-supplied, and nothing verifies that the incoming request matches
the one originally stored under that key. A client reusing a key across two different endpoints
gets the previous, unrelated response back. Worse, the replayed object is deserialized from
the stored @class attribute and assigned to ctx.setResponse(...), so the caller can get a
ClassCastException on a type mismatch instead of an error.

Standard behaviour is to reject key reuse with a different payload (422/409) — at minimum store
and compare a request fingerprint.

6. Idempotency silently no-ops on the async and disruptor dispatchers

hookManager.before(ctx) runs on a pool/disruptor thread in both, and
ServletHeaderUtil.getHeader reads RequestContextHolder, which is a non-inheritable
thread-local. So command.getIdempotencyKey() is always null there and the check hook does
nothing. The header extraction belongs in the API/filter layer (before dispatch), not in a
before-hook.

7. Silent-disable configuration model

All the @ConditionalOnProperty guards lack matchIfMissing, so a typo in
fineract.command.idempotency.hook-check-pre disables idempotency with no log line and no
startup failure. Same for the renamed audit properties: fineract.command.hooks.audit-pre →
fineract.command.audit.hook-pre silently turns off audit persistence for anyone who set the
property directly rather than via FINERACT_COMMAND_PROCESSORS_AUDIT_PRE.

For guarantees of this kind, please fail fast or at least log.warn when the module is on the
classpath but its hooks are off, and call the rename out in the release notes.

Minor

• IdempotencyCheckCommandHook injects IdempotencyCommandProperties but never uses it — dead
  field.
• The check hook needs @ConditionalOnBean(CommandStore.class); with hook-check-pre=true and
  no store bean (e.g. fineract.command.jdbc.enabled=false) the context fails to start.
• getHeader(properties.getKeyHeaderName(), true) changes behaviour: the key is now also
  accepted as a query parameter (previously false). Deliberate? Idempotency keys in query
  strings end up in access logs. Also getParameter(name.toLowerCase()) is case-sensitive for
  parameters, so only a lowercase idempotency-key param would ever match.
• ServletHeaderUtil is @UtilityClass and public final with explicit public static
  members — the Lombok annotation makes all three redundant, and the rest of the module uses
  the explicit private-constructor idiom (CommandConstants).
• fineract-command-idempotency/build.gradle:59 — copy-paste: the Eclipse output dir points at
  fineract-command/bin/main instead of fineract-command-idempotency/bin/main.
• mapstruct / mapstruct-processor, resilience4j-spring-boot3 and jmh-core are declared
  but unused by the module.
• AutoConfiguration.imports is missing a trailing newline.
• CommandProperties.hooks (Map<String, Boolean>) is now effectively dead — nothing reads the
  map, only the raw property paths. Since audit moved to typed properties, the three core hooks
  should follow for consistency (three naming schemes now coexist: COMMAND_HOOK_ORDER_*,
  COMMAND_AUDIT_HOOK_*, COMMAND_IDEMPOTENCY_HOOK_*).
• fineract-doc/src/docs/en/chapters/architecture/idempotency.adoc still documents only the
  legacy fineract.idempotency-key-header-name mechanism; the two now coexist and that's worth
  a paragraph.
• Drive-by, pre-existing but in a file you touch:
  DisruptorCommandDispatcher.CommandEvent.future is initialised once in the event factory
  (CommandEvent::new) and never reset, so once the ring buffer wraps, dispatch hands back an
  already-completed future from a previous command. Worth to revisit this...

[vidakovic]

@adamsaghy the order of the hooks was actually intentional... have to look at the notes concerning potential failure scenarios, but as for the order you could look at this also as a feature... if we do as suggested then you'll have no trace a duplicate attempt ever happened (could be malicious)... if that works for you then good for me. Please confirm before I start changing.

[vidakovic]

Concerning the scenario 7: matchIfMissing is false by default... adding it explicitly makes no difference, finding is invalid. But I will add a warning if the audit hooks are for any reasons turned off; in production I think the default setting should be that they are turned on. application.properties had already the correct default settings, now AuditCommandProperties.java has them too.

[vidakovic]

Concerning scenario 6: I'm not entirely sure if this is a false flag, but async and disruptor mode are disabled and unusable anyway. For now this is out of scope.

[vidakovic]

Concerning minor issue "The check hook needs @ConditionalOnBean(CommandStore.class)": this is the intended behavior. A store should always be available. If we don't then it should not be possible to start. And that's the case here.

[vidakovic]

Concerning rest of minor issues: most of them fixed.

[vidakovic]

Concerning scenario 5: check added.

[vidakovic]

Concerning scenario 4: Introduced skipExecution attribute in CommandContext that is explicitly used to decide if the command should be executed or not. This is clearer than using any combination of the status enums or introducing a new one.

[vidakovic]

Concerning scenario 3: Added a new query that checks for existence of command by idempotency without relying on response object. Should be fixed.

[vidakovic]

Concerning scenario 2: Added unique constraint and made sure that any duplicate keys get removed from storage to avoid conflicts when the Liquibase script is executed.

[vidakovic]

Concerning scenario 1: changed hook order, idempotency now before audit, but the core hooks are still running first; this is intentional because if something fails I want to have some context (e.g. IP address); introduced a skipHooks context attribute that avoids the duplicate key storage attempt.

[github-actions]

This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days.