Improve global error handling & logging

jpye-finch commented 10 months ago

Design principles

We need to handle errors in the system more gracefully. If there is any error whilst processing an application so that the data is never lost. The following approach aims to

Ensure data integrity in all situations
Provide clear signalling of process steps end-to-end
Allow the client to retry sending the same payload until data integrity and persistence can be verified
Allow the client to keep hold of the data until verified

Requirements

Client should never purge draft unless it can verify the record was fully written. To do this, it needs to verify the record was received by the the backend returning an unique id for the record.

This should never happen before metadata is persisted in MongoDB, attachments are stored in Minio and search indexing happens in Elasticsearch. If any of these fails, then the return value must be 500.

If one of these steps fails, the backend needs to work so the client can safely retry without duplicate entries. In other words, the backend needs to clean up after itself in the case of an error. The outcome in the database should be the same even if you first unsuccessfully submit a record 10 times, then submit it successfully once and then try submitting it 5 more times. The output should be one record written in the database.

The endpoints that this apply to are at this phase the ones writing mission critical data with a lot of user input that might not be available anymore after a potential failure. Specifically these queries:

requestRegistrationCorrection(id: ID!, details: CorrectionInput!): ID!
createBirthRegistrationCorrection(
  id: ID!
  details: BirthRegistrationInput!
): ID!
createDeathRegistrationCorrection(
  id: ID!
  details: DeathRegistrationInput!
): ID!
createMarriageRegistrationCorrection(
  id: ID!
  details: MarriageRegistrationInput!
): ID!
createBirthRegistration(details: BirthRegistrationInput!): CreatedIds!
createDeathRegistration(details: DeathRegistrationInput!): CreatedIds!
createMarriageRegistration(details: MarriageRegistrationInput!): CreatedIds!

`requestRegistrationCorrection`, `createBirthRegistrationCorrection`, `createDeathRegistrationCorrection`, `createMarriageRegistrationCorrection`

What should happen here to the record? I user might have submitted it at the same time with another user. Should their correction get lost completely? Edge case and most likely rare but possible.
- We should still check if the correct request is the same one as the currently active one.
- If it is the same one, then we should continue the code flow to the very end but so the operations like send to hearth, create new audit event and index bundle would first check if the record was already written
- A concept of transaction id or equivalent unique identifier to identify this specific action will have to be implemented and stored

`createBirthRegistration`

Same type of strategy where the createRecord function reruns the flow in an idempotent way and substeps verify no duplicate records are written
@Zangetsu101 @Nil20 reflecting on the text above, could you comment your thoughts on how we could make these create*Registration handlers best so that the client, in theory, could call one 100 times with the same data without there being duplicates? But also so that if writing to ES failed the first time, then on the second try it could try doing it again.

Next iteration – ideas for the future

Implement a Minio based blob persistence for all records. Purge once persistence verified.
Inform admins of failed records

Tech tasks

[ ] Go through the mutations mentioned above. Implement rollback / compensation phase for each write operation. The endpoints should always make sense in the context of the service. So instead of creating endpoints like POST /compensate, it should always rather be DELETE /record/<id> or similar.
- If for instance writing to Elasticsearch fails in search service as part of request correction workflow, there needs to be compensation for
- Deleting audit events that were just written
- Deleting the FHIR entities that were written
  - How does _history collections behave in this process flows? Do we need to also remove history items?
  - Written minio attachments should be deleted
  - Using a library like node-sagas might be a good fit for workflow
[ ] Ensure that in the GraphQL mutations mentioned, errors correctly flow to the client if any step of the process fails
[ ] Ensure client doesn't purge the submission payload before the backend responds with a success response
[ ] Deploy & shut down Elasticsearch. Submit a declaration. Allow it to fail a few times. Verify correct 5 status code (or equivalent) was returned by both search service and gateway. start ES up again. Verify one ** record was written both to MongoDB and ES, audit log and that there are no attachments not connected to the right correction event.
[ ] Verify error was reported on Sentry

euanmillar commented 8 months ago

This ticket is to log the full graphql payload with any downstream backend error, then alert the admin so that when the bug is resolved, perhaps the data can be recovered in some way

rikukissa commented 4 months ago

I came across the issue again today while working in a branch with broken backend code. Record can indeed disappear if something goes wrong. Overall this issue should be considered as critical issue.

rikukissa commented 4 months ago

Zangetsu101 commented 3 weeks ago

A user might have submitted it at the same time with another user.

Would that even be possible? Because "Assignment" should prevent that from happening

Now if agree that we are preventing multiple users from performing some action on the same record due to assignments being in place, then hte problem boils down to handling multiple requests from the same user ( e.g. due to bad network, the first request timed out then sending the same req again).

What we could do is send a unique id with the request, generated at the client, which we can then use to identify the duplicate requests and respond accordingly. We are already doing something similar for the create*Registration mutations where we are treating the draftId as that unique identifier. We could implement something similar for the corrections

Zangetsu101 commented 3 weeks ago

As for making the mutations idempotent, we first need to have transactions across services. One of common patterns for this is "Saga" where for each step we need to implement a rollback which gets called if one of the later steps fail.

Another option could be to maintain some record against the transaction id (which we are kinda getting from client) about which steps have completed. Then we can retry only the failed steps and skip the completed ones. The request succeeds once all the steps are completed

rikukissa commented 3 weeks ago

We can agree that correction can be performed only by one user at one time, at least for time being. I do like the saga pattern idea. Wouldn't really have to be more complex than what you are proposing so implementing the "compensation" mechanism for the write operations in the flows mentioned on the ticket.

opencrvs / opencrvs-core