Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
witash wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
656eb5f
fix(#10875): race condition for sync transitions
witash Apr 22, 2026
dc55972
Merge branch 'master' into 10875-save-transitions-before
witash May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions shared-libs/transitions/src/transitions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
saveDoc(change, (err, result) => {
callback(null, err || result);
});
});
}, { saveInfoDocFirst: true });
});
async.series(operations, (err, results) => {
return err ? reject(err) : resolve(results);
Expand Down Expand Up @@ -150,7 +150,7 @@
self._loadTransition(transition, synchronous);
} catch (e) {
const errorMessage = `Failed loading transition "${transition}"`;
loadErrors = [errorMessage, e && e.message || 'unknown'];

Check warning on line 153 in shared-libs/transitions/src/transitions/index.js

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Prefer using an optional chain expression instead, as it's more concise and easier to read. 


See more on https://sonarcloud.io/project/issues?id=medic_cht-core&issues=AZ253xDNlyTdSDttC_Bk&open=AZ253xDNlyTdSDttC_Bk&pullRequest=10910
logger.error(errorMessage);
logger.error('%o', e);
}
Expand Down Expand Up @@ -232,7 +232,7 @@
* did nothing and saving is unnecessary. If results has a true value in
* it then a change was made.
*/
const finalize = ({ change, results }, callback) => {
const finalize = ({ change, results, saveInfoDocFirst = false }, callback) => {

@dianabarsan dianabarsan Apr 23, 2026

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?

@witash witash May 18, 2026

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.

logger.debug(`transition results: ${JSON.stringify(results)}`);

const changed = _.some(results, i => Boolean(i));
Expand All @@ -254,6 +254,31 @@
}
logger.debug(`calling saveDoc on doc ${change.id} seq ${change.seq}`);

if (saveInfoDocFirst) {
// For sync transitions, save transitions before saving the changed
// document, so that the change does not appear in the changes
// feed until the infoDoc has been saved with the correct transitions
//
// If saveDoc fails after saveTransitions succeeded, the infodoc will
// reflect transitions as run while the doc itself was not updated.
// there will be no retry
return infodoc
.saveTransitions(change)
.then(() => new Promise((resolve, reject) => {
saveDoc(change, (err, result) => err ? reject(err) : resolve(result));
}))
.then(result => {
logger.info(`saved changes on doc ${change.id} seq ${change.seq}`);
callback(null, result);
})
.catch(err => {
logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err);
callback(err);
});
}

// For async transitions run by sentinel, save the document first,
// then update the infoDoc with the transitions map
saveDoc(change, (err, result) => {
// todo: how to handle a failed save? for now just
// waiting until next change and try again.
Expand Down Expand Up @@ -332,7 +357,7 @@
.then(changed => callback(null, changed)); // return the promise instead
};

const applyTransitions = (change, callback) => {
const applyTransitions = (change, callback, { saveInfoDocFirst = false } = {}) => {
const operations = transitions
.map(transition => {
const opts = {
Expand All @@ -350,7 +375,7 @@
* function. All we care about are results and whether we need to
* save or not.
*/
async.series(operations, (err, results) => finalize({ change, results }, callback));
async.series(operations, (err, results) => finalize({ change, results, saveInfoDocFirst }, callback));
};

const availableTransitions = () => {
Expand Down
2 changes: 1 addition & 1 deletion shared-libs/transitions/test/unit/process_docs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ describe('processDocs', () => {
chai.expect(db.medic.put.calledWith({ _id: '3', from: 3 })).to.equal(true);
chai.expect(db.medic.put.calledWith({ _id: '4', from: 4 })).to.equal(true);

chai.expect(infodoc.saveTransitions.callCount).to.equal(3);
chai.expect(infodoc.saveTransitions.callCount).to.equal(4);
chai.expect(infodoc.saveTransitions.calledWithMatch({ id: '1' })).to.equal(true);
});
});
Expand Down
Loading