Integration test check-invoice-and-invoice-lines-deletion-restrictions fails randomly

Overview:

Test check-invoice-and-invoice-lines-deletion-restrictions.feature fails randomly for scenario "Delete paid invoice and invoice line" with a wrong status. It looks like the invoice is deleted when it should not.

This might be caused by a cached entry for the invoice that would not be updated before the attempt to delete it. If this is the case, the cache should be invalidated when the invoice is modified before other operations can happen on the invoice.

Steps to Reproduce:

1. Run all the tests in InvoicesApiTest.java (or just checkInvoiceAndLinesDeletionRestrictions).
2. Try again a few times if check-invoice-and-invoice-lines-deletion-restrictions.feature passes without an error.

Expected Results:

The tests complete successfully.

Actual Results:

This test failed:

check-invoice-and-invoice-lines-deletion-restrictions.feature:110 - status code was: 204, expected: 403, response time: 16, url: http://localhost:9130/invoice/invoices/0bf0ae4f-0461-4106-8bef-e6350cdbfa88

Example TestRail.

Additional Information:
URL:
Interested parties:

EVALUATION

This is not a cache issue (memory cache is not used for requests in mod-invoice and mod-invoice-storage). Just after the PUT request with the updated Paid status, mod-invoice-storage is getting another PUT request for the same invoice with the Open status.

This is caused by the invoice line update, which calls InvoiceLineHelper#updateInvoiceAsync. The invoice gets updated later with the new totals, and this can happen after another invoice update expected to occur afterwards.

The test failure can be hard to reproduce, but slowing down the async invoice update in InvoiceSummary with additional calls to mod-invoice-storage is sufficient to make it fail every time.

The issue was detected thanks to tests, but it could also happen in production environments. For instance, an external script using the API could modify an invoice line and modify the invoice afterwards.

Similar conflicts with PUT requests could happen in all FOLIO modules, whether because of the use of async operations or because of the multi-user environment; we should try to think of a general solution that could apply to all PUT operations.

POSSIBLE SOLUTIONS

• Update the invoice synchronously - this would be the safest option, but could impact performance.
• Make calculation asynchronously, but get the invoice just before changing the totals and saving it - this would reduce the risk of a conflict, but it could still happen with enough bad luck because the whole operation is async.
• Somehow lock the invoice while the calculation is being done - probably not possible with the API, and it would add typical risks related to locks.
• Update only the field adjustmentsTotal - this would limit the risk of a conflict to this field while preserving performance; would require implementing the PATCH method to the invoice-storage API.
• Move the calculation code to invoice-storage - this might give us more options.
• Use a version number for invoices in storage, and check that a PUT is based on the latest version before persisting to DB; if not, return a 409 error in invoice-storage and try the whole operation again from mod-invoice (when async) or return an error to client - the advantage of this is that it could also be helpful when 2 users try to modify the same invoice at the same time, and it would eliminate silent conflicts without preventing async operations. See also optimistic locking for a solution with HTTP headers. Update: this idea is actually what is suggested in UXPROD-1752; see also RMB optimistic locking.

RESOLUTION

Invoice and invoice line updates will be made synchronous (performance tests showed that a few hundred invoice lines can be updated with a reasonable delay for the UI).

Optimistic locking should still be enabled in the future, but this will be managed in other Jira issues.