Skip to content

fix(#10875): race condition for sync transitions#10910

Closed
witash wants to merge 2 commits into
masterfrom
10875-save-transitions-before
Closed

fix(#10875): race condition for sync transitions#10910
witash wants to merge 2 commits into
masterfrom
10875-save-transitions-before

Conversation

@witash

@witash witash commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Description

To avoid a race condition in transitions between sentinel and api, save the infoDoc, which contains a which transitions have already run, before saving the medic document for synchrnonous transitions.
This means that the changes feed, and therefore sentinel, will not see the change until after the infodoc has saved which transitions have already run.

This PR leaves async transitions as is, where the infoDoc is saved after the medic document.

More disucssion here

Code review checklist

  • [ X ] UI/UX backwards compatible: Test it works for the new design (enabled by default). And test it works in the old design, enable can_view_old_navigation permission to see the old design. Test it has appropriate design for RTL languages.
  • [ X ] Readable: Concise, well named, follows the style guide
  • [ X ] Documented: Configuration and user documentation on cht-docs
  • [ X ] Tested: Unit and/or e2e where appropriate
  • [ X ] Internationalised: All user facing text
  • [ X ] Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.
  • [ X ] AI disclosure: Please disclose use of AI per the guidelines.

* it then a change was made.
*/
const finalize = ({ change, results }, callback) => {
const finalize = ({ change, results, saveInfoDocFirst = false }, callback) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! I honestly thought this was solved and we were always saving the infodocs first.
Since you're here, I wonder if we can somehow change the path for how the code works when docs are changed in bulk. Can we skip saveDoc entirely since the docs are saved at the end anyway and save the infodocs in bulk as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip saveDoc entirely since the docs are saved at the end anyway and save the infodocs in bulk as well?

Maybe, but some transitions seem to rely on previous documents having been already saved. I could get more details, but long story short, several tests fail when trying to save in bulk, that seems to be because of dependencies that some transitions are run and saved in order.

It might still be worth looking into, but its not a "while we're changing this code anyway" type of change, it would expand the scope of this PR a lot.

@witash witash marked this pull request as ready for review May 18, 2026 08:58
@witash

witash commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

@dianabarsan what do you think about merging this as is?
the locking option is left open on the issue discussion but I'm trying to sell this simpler workaround. Do you think we review and merge this or commit to the locking option?

@witash witash requested a review from dianabarsan May 19, 2026 16:46

@dianabarsan dianabarsan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @witash . I don't think we should proceed with this change, and we should implement some sort of locking. I think this would just be moving the race condition somewhere else - because we would absolutely need to revert the infodoc change if the doc save fails, and do another doc save to trigger another "check" by sentinel.

If you can add a "revert" for the infodoc in case doc save fails, I think this might work.

@witash

witash commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

replaced by #11135

@witash witash closed this Jun 8, 2026
@witash witash deleted the 10875-save-transitions-before branch June 8, 2026 08:05
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.

2 participants