From 656eb5f330776302dc68d21d787abca78e653f0c Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Wed, 22 Apr 2026 13:45:56 +0300 Subject: [PATCH 01/12] fix(#10875): race condition for sync transitions --- .../transitions/src/transitions/index.js | 33 ++++++++++++++++--- .../transitions/test/unit/process_docs.js | 2 +- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index 6f5bdad8999..af54f170d49 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -110,7 +110,7 @@ const processDocs = docs => { saveDoc(change, (err, result) => { callback(null, err || result); }); - }); + }, { saveInfoDocFirst: true }); }); async.series(operations, (err, results) => { return err ? reject(err) : resolve(results); @@ -232,7 +232,7 @@ const canRun = ({ key, change, transition }) => { * 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) => { logger.debug(`transition results: ${JSON.stringify(results)}`); const changed = _.some(results, i => Boolean(i)); @@ -254,6 +254,31 @@ const finalize = ({ change, results }, callback) => { } 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. @@ -332,7 +357,7 @@ const applyTransition = ({ key, change, transition, force }, callback) => { .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 = { @@ -350,7 +375,7 @@ const applyTransitions = (change, callback) => { * 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 = () => { diff --git a/shared-libs/transitions/test/unit/process_docs.js b/shared-libs/transitions/test/unit/process_docs.js index 2f6e7493007..1ddd4bac240 100644 --- a/shared-libs/transitions/test/unit/process_docs.js +++ b/shared-libs/transitions/test/unit/process_docs.js @@ -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); }); }); From 3ef59802581187a42f6cb8a7234318497fe5b922 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Fri, 29 May 2026 13:54:04 +0300 Subject: [PATCH 02/12] fix(#10875): adding valid/invalid rev to infodoc --- shared-libs/infodoc/src/infodoc.js | 50 ++++++++++- shared-libs/infodoc/test/infodoc.js | 69 +++++++++++++++ .../transitions/src/transitions/index.js | 86 ++++++++++--------- .../test/integration/transitions.js | 44 ++++++---- .../test/unit/finalize-transition.js | 19 +++- .../transitions/test/unit/process_docs.js | 11 ++- 6 files changed, 213 insertions(+), 66 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 2fcdad0bd44..f563585aa10 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -151,15 +151,15 @@ const updateTransition = (change, transition, ok) => { }; }; -const saveTransitions = change => { - return saveProperty(change.id, change.info, 'transitions', {}); +const saveTransitions = (change, validRev) => { + return saveProperty(change.id, change.info, 'transitions', {}, validRev); }; const saveCompletedTasks = (id, infodoc, completedTasks = []) => { return saveProperty(id, infodoc, 'completed_tasks', completedTasks); }; -const saveProperty = async (id, infodoc, property, defaultValue = {}) => { +const saveProperty = async (id, infodoc, property, defaultValue = {}, validRev) => { let updatedInfoDoc; try { updatedInfoDoc = await db.sentinel.get(getInfoDocId(id)); @@ -171,13 +171,53 @@ const saveProperty = async (id, infodoc, property, defaultValue = {}) => { updatedInfoDoc = infodoc; } + if (validRev !== undefined) { + updatedInfoDoc.valid_rev = validRev; + delete updatedInfoDoc.invalid_rev; + } + try { return await db.sentinel.put(updatedInfoDoc); } catch (err) { if (err.status !== 409) { throw err; } - return saveProperty(id, infodoc, property, defaultValue); + return saveProperty(id, infodoc, property, defaultValue, validRev); + } +}; + +const setInvalidRev = (id, invalidRev) => { + return modifyInfoDoc(id, infoDoc => { + infoDoc.invalid_rev = invalidRev; + }); +}; + +const clearInvalidRev = id => { + return modifyInfoDoc(id, infoDoc => { + delete infoDoc.invalid_rev; + }); +}; + +const modifyInfoDoc = async (id, modify) => { + let infoDoc; + try { + infoDoc = await db.sentinel.get(getInfoDocId(id)); + } catch (err) { + if (err.status !== 404) { + throw err; + } + infoDoc = blankInfoDoc(id); + } + + modify(infoDoc); + + try { + return await db.sentinel.put(infoDoc); + } catch (err) { + if (err.status !== 409) { + throw err; + } + return modifyInfoDoc(id, modify); } }; @@ -283,6 +323,8 @@ module.exports = { bulkUpdate: bulkUpdate, saveTransitions: saveTransitions, saveCompletedTasks: saveCompletedTasks, + setInvalidRev: setInvalidRev, + clearInvalidRev: clearInvalidRev, // Used to update infodoc metadata that occurs at write time. A delete does not count as a write // in this instance, as deletes resolve as infodoc cleanups once sentinel's background-cleanup diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index dbf713c81c0..36d5d1ffdb3 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -604,6 +604,75 @@ describe('infodoc', () => { assert.deepEqual(db.sentinel.put.args[20], [{ ...info, transitions: change.info.transitions }]); }); }); + + it('should record validRev and clear invalid_rev when given', () => { + const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; + const change = { id: 'some', info: { transitions: { one: { ok: true } } } }; + sinon.stub(db.sentinel, 'get').resolves(info); + sinon.stub(db.sentinel, 'put').resolves(); + + return lib.saveTransitions(change, '2-def').then(() => { + const saved = db.sentinel.put.args[0][0]; + assert.equal(saved.valid_rev, '2-def'); + assert.isUndefined(saved.invalid_rev); + }); + }); + }); + + describe('setInvalidRev / clearInvalidRev', () => { + it('setInvalidRev marks the infodoc mid-write', () => { + const info = { _id: 'some-info', doc_id: 'some' }; + sinon.stub(db.sentinel, 'get').resolves(info); + sinon.stub(db.sentinel, 'put').resolves(); + + return lib.setInvalidRev('some', '1-abc').then(() => { + assert.deepEqual(db.sentinel.get.args[0], ['some-info']); + assert.equal(db.sentinel.put.args[0][0].invalid_rev, '1-abc'); + }); + }); + + it('setInvalidRev creates a blank infodoc on 404', () => { + sinon.stub(db.sentinel, 'get').rejects({ status: 404 }); + sinon.stub(db.sentinel, 'put').resolves(); + + return lib.setInvalidRev('some', null).then(() => { + const saved = db.sentinel.put.args[0][0]; + assert.equal(saved._id, 'some-info'); + assert.equal(saved.doc_id, 'some'); + assert.equal(saved.invalid_rev, null); + }); + }); + + it('clearInvalidRev removes the mid-write marker', () => { + const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; + sinon.stub(db.sentinel, 'get').resolves(info); + sinon.stub(db.sentinel, 'put').resolves(); + + return lib.clearInvalidRev('some').then(() => { + assert.isUndefined(db.sentinel.put.args[0][0].invalid_rev); + }); + }); + + it('retries on 409 conflict', () => { + const info = { _id: 'some-info', doc_id: 'some' }; + sinon.stub(db.sentinel, 'get').resolves(info); + const put = sinon.stub(db.sentinel, 'put'); + put.onCall(0).rejects({ status: 409 }); + put.onCall(1).resolves(); + + return lib.setInvalidRev('some', '1-abc').then(() => { + assert.equal(db.sentinel.put.callCount, 2); + }); + }); + + it('throws non-409 errors', () => { + sinon.stub(db.sentinel, 'get').resolves({ _id: 'some-info', doc_id: 'some' }); + sinon.stub(db.sentinel, 'put').rejects({ status: 500 }); + + return lib.setInvalidRev('some', '1-abc') + .then(() => assert.fail('should have thrown')) + .catch(err => assert.equal(err.status, 500)); + }); }); describe('saveCompletedTasks', () => { diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index af54f170d49..aa341f8694f 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -45,13 +45,34 @@ const AVAILABLE_TRANSITIONS = [ const transitions = []; let loadErrors = false; +const MAX_INFODOC_WAIT = 5; +const INFODOC_WAIT_INTERVAL = 100; + +const isInfoDocMidWrite = infoDoc => infoDoc && infoDoc.invalid_rev !== undefined; + +const getConsistentInfoDoc = (change, retriesLeft) => { + return infodoc.get(change).then(infoDoc => { + if (!isInfoDocMidWrite(infoDoc) || retriesLeft <= 0) { + return infoDoc; + } + return new Promise(resolve => setTimeout(resolve, INFODOC_WAIT_INTERVAL)) + .then(() => getConsistentInfoDoc(change, retriesLeft - 1)); + }); +}; + // applies all loaded transitions over a change const processChange = (change, callback) => { lineage .fetchHydratedDoc(change.id) .then(doc => { change.doc = doc; - return infodoc.get(change).then(infoDoc => { + return getConsistentInfoDoc(change, MAX_INFODOC_WAIT).then(infoDoc => { + if (isInfoDocMidWrite(infoDoc)) { + logger.warn( + `transitions: infodoc for ${change.id} still mid-write after ${MAX_INFODOC_WAIT} retries, skipping` + ); + return callback(); + } change.info = infoDoc; change.initialProcessing = !infoDoc.transitions; // Remove transitions from doc since those @@ -110,7 +131,7 @@ const processDocs = docs => { saveDoc(change, (err, result) => { callback(null, err || result); }); - }, { saveInfoDocFirst: true }); + }); }); async.series(operations, (err, results) => { return err ? reject(err) : resolve(results); @@ -232,7 +253,7 @@ const canRun = ({ key, change, transition }) => { * did nothing and saving is unnecessary. If results has a true value in * it then a change was made. */ -const finalize = ({ change, results, saveInfoDocFirst = false }, callback) => { +const finalize = ({ change, results }, callback) => { logger.debug(`transition results: ${JSON.stringify(results)}`); const changed = _.some(results, i => Boolean(i)); @@ -254,44 +275,25 @@ const finalize = ({ change, results, saveInfoDocFirst = false }, callback) => { } 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. - if (err) { + const invalidRev = change.doc._rev ?? null; + return infodoc + .setInvalidRev(change.id, invalidRev) + .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}`); + return infodoc + .saveTransitions(change, result.rev) + .then(() => callback(null, result)); + }) + .catch(err => { logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); - return callback(err); - } - - logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); - infodoc.saveTransitions(change) - .then(() => callback(null, result)) - .catch(err => callback(err)); - }); + return infodoc + .clearInvalidRev(change.id) + .catch(clearErr => logger.error(`error clearing invalid_rev on doc ${change.id}: %o`, clearErr)) + .then(() => callback(err)); + }); }; const saveDoc = (change, callback) => { @@ -357,7 +359,7 @@ const applyTransition = ({ key, change, transition, force }, callback) => { .then(changed => callback(null, changed)); // return the promise instead }; -const applyTransitions = (change, callback, { saveInfoDocFirst = false } = {}) => { +const applyTransitions = (change, callback) => { const operations = transitions .map(transition => { const opts = { @@ -375,7 +377,7 @@ const applyTransitions = (change, callback, { saveInfoDocFirst = false } = {}) = * function. All we care about are results and whether we need to * save or not. */ - async.series(operations, (err, results) => finalize({ change, results, saveInfoDocFirst }, callback)); + async.series(operations, (err, results) => finalize({ change, results }, callback)); }; const availableTransitions = () => { diff --git a/shared-libs/transitions/test/integration/transitions.js b/shared-libs/transitions/test/integration/transitions.js index efb81efdec6..f6b7ef019be 100644 --- a/shared-libs/transitions/test/integration/transitions.js +++ b/shared-libs/transitions/test/integration/transitions.js @@ -41,6 +41,8 @@ describe('functional transitions', () => { }, }); const infoDocSave = sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(db.medic, 'get').rejects({ status: 404 }); const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); @@ -99,6 +101,8 @@ describe('functional transitions', () => { const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); const infoDoc = sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); transitions.loadTransitions(); const change1 = { @@ -166,6 +170,8 @@ describe('functional transitions', () => { const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); const infoDoc = sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); transitions.loadTransitions(); const change1 = { @@ -293,6 +299,8 @@ describe('functional transitions', () => { sinon.stub(infodoc, 'get').resolves(info); sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); sinon.spy(transitions, 'applyTransitions'); @@ -351,6 +359,8 @@ describe('functional transitions', () => { sinon.stub(infodoc, 'get').resolves({}); sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(db.medic, 'put').callsArgWith(1, { error: 'something' }); const doc = { @@ -568,10 +578,11 @@ describe('functional transitions', () => { assert.deepEqualExcluding(savedDocs[0], originalDocs[0], ['_id', 'errors', 'contact', 'sent_by', 'tasks']); // first doc is updated by 3 transitions infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[0]._id); - assert.equal(infodocSaves.length, 1); - assert.equal(infodocSaves[0][0].transitions.update_clinics.ok, true); - assert.equal(infodocSaves[0][0].transitions.update_sent_by.ok, true); - assert.equal(infodocSaves[0][0].transitions.conditional_alerts.ok, true); + assert.equal(infodocSaves.length, 2); + let txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.equal(txnSave.transitions.update_clinics.ok, true); + assert.equal(txnSave.transitions.update_sent_by.ok, true); + assert.equal(txnSave.transitions.conditional_alerts.ok, true); assert.equal(savedDocs[1].id, 'has default response'); assert.equal(savedDocs[1]._id.length, 36); @@ -581,9 +592,10 @@ describe('functional transitions', () => { assert.equal(savedDocs[1].errors[0].code, 'sys.facility_not_found'); assert.deepEqualExcluding(savedDocs[1], originalDocs[1], ['_id', 'tasks', 'errors']); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[1]._id); - assert.equal(infodocSaves.length, 1); - assert.equal(infodocSaves[0][0].transitions.default_responses.ok, true); - assert.equal(infodocSaves[0][0].transitions.update_clinics.ok, true); + assert.equal(infodocSaves.length, 2); + txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.equal(txnSave.transitions.default_responses.ok, true); + assert.equal(txnSave.transitions.update_clinics.ok, true); assert.deepEqualExcluding(savedDocs[2], originalDocs[2], '_id'); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[2]._id); @@ -593,9 +605,10 @@ describe('functional transitions', () => { assert.equal(savedDocs[3].sent_by, 'Angela'); assert.deepEqualExcluding(savedDocs[3], originalDocs[3], 'sent_by'); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[3]._id); - assert.equal(infodocSaves.length, 1); - assert.equal(infodocSaves[0][0].transitions.default_responses.ok, true); - assert.equal(infodocSaves[0][0].transitions.update_sent_by.ok, true); + assert.equal(infodocSaves.length, 2); + txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.equal(txnSave.transitions.default_responses.ok, true); + assert.equal(txnSave.transitions.update_sent_by.ok, true); assert.equal(savedDocs[4].id, 'will have errors'); assert.equal(savedDocs[4].sent_by, 'Angela'); @@ -609,11 +622,12 @@ describe('functional transitions', () => { assert.equal(savedDocs[4].tasks[1].messages[0].message, 'too much randomness'); assert.deepEqualExcluding(savedDocs[4], originalDocs[4], ['_id', 'sent_by', 'errors', 'tasks']); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[4]._id); - assert.equal(infodocSaves.length, 1); - assert.equal(infodocSaves[0][0].transitions.default_responses.ok, true); - assert.equal(infodocSaves[0][0].transitions.update_sent_by.ok, true); - assert.equal(infodocSaves[0][0].transitions.accept_patient_reports.ok, true); - assert.equal(infodocSaves[0][0].transitions.conditional_alerts.ok, true); + assert.equal(infodocSaves.length, 2); + txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.equal(txnSave.transitions.default_responses.ok, true); + assert.equal(txnSave.transitions.update_sent_by.ok, true); + assert.equal(txnSave.transitions.accept_patient_reports.ok, true); + assert.equal(txnSave.transitions.conditional_alerts.ok, true); }); }); }); diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index b2751759066..ab197a99484 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -24,19 +24,25 @@ describe('finalize transition', () => { it('save is called if transition results have changes', done => { const doc = { _rev: '1' }; - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true, rev: '2' }); + const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); + const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); transitions.finalize( { - change: { doc: doc }, + change: { id: 'abc', doc: doc }, results: [null, null, true], }, (err, result) => { assert.equal(saveDoc.callCount, 1); assert(saveDoc.args[0][0]._rev); assert(!err); - assert.deepEqual(result, { ok: true }); + assert.deepEqual(result, { ok: true, rev: '2' }); + assert.equal(setInvalidRev.callCount, 1); + assert.deepEqual(setInvalidRev.args[0], ['abc', '1']); assert.equal(infodoc.saveTransitions.callCount, 1); + assert.deepEqual(infodoc.saveTransitions.args[0][1], '2'); + assert.equal(clearInvalidRev.callCount, 0); done(); } ); @@ -45,10 +51,12 @@ describe('finalize transition', () => { it('should callback with save errors', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, { error: 'something' }); + const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); + const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); transitions.finalize( { - change: { doc: doc }, + change: { id: 'abc', doc: doc }, results: [null, null, true], }, (err, result) => { @@ -56,7 +64,10 @@ describe('finalize transition', () => { assert.equal(saveDoc.callCount, 1); assert(saveDoc.args[0][0]._rev); assert(!result); + assert.equal(setInvalidRev.callCount, 1); assert.equal(infodoc.saveTransitions.callCount, 0); + assert.equal(clearInvalidRev.callCount, 1); + assert.deepEqual(clearInvalidRev.args[0], ['abc']); done(); } ); diff --git a/shared-libs/transitions/test/unit/process_docs.js b/shared-libs/transitions/test/unit/process_docs.js index 1ddd4bac240..f7cec6291ef 100644 --- a/shared-libs/transitions/test/unit/process_docs.js +++ b/shared-libs/transitions/test/unit/process_docs.js @@ -101,6 +101,8 @@ describe('processDocs', () => { sinon.stub(transitions, 'applyTransition'); sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); // first doc is updated by at least one transition transitions.applyTransition @@ -148,6 +150,8 @@ describe('processDocs', () => { chai.expect(infodoc.saveTransitions.callCount).to.equal(3); chai.expect(infodoc.saveTransitions.calledWithMatch({ id: '1' })).to.equal(true); + chai.expect(infodoc.setInvalidRev.callCount).to.equal(1); + chai.expect(infodoc.setInvalidRev.calledWith('1')).to.equal(true); }); }); @@ -178,6 +182,8 @@ describe('processDocs', () => { .withArgs(sinon.match({ _id: '3' })).callsArgWith(1, null, { ok: true }) .withArgs(sinon.match({ _id: '4' })).callsArgWith(1, { error: 'error' }); sinon.stub(infodoc, 'saveTransitions').resolves(); + sinon.stub(infodoc, 'setInvalidRev').resolves(); + sinon.stub(infodoc, 'clearInvalidRev').resolves(); // first doc is updated by at least one transition transitions.applyTransition @@ -229,8 +235,11 @@ 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(4); + chai.expect(infodoc.saveTransitions.callCount).to.equal(3); chai.expect(infodoc.saveTransitions.calledWithMatch({ id: '1' })).to.equal(true); + chai.expect(infodoc.setInvalidRev.callCount).to.equal(2); + chai.expect(infodoc.clearInvalidRev.callCount).to.equal(1); + chai.expect(infodoc.clearInvalidRev.calledWith('2')).to.equal(true); }); }); }); From 3de6f0d0a33e5d02339e6405e1e50722cbe9d8ba Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Tue, 2 Jun 2026 15:41:10 +0300 Subject: [PATCH 03/12] fix(#10875): refactoring --- shared-libs/infodoc/src/infodoc.js | 52 +++++++------------ shared-libs/infodoc/test/infodoc.js | 12 ----- .../transitions/src/transitions/index.js | 27 ++++++++-- .../test/unit/finalize-transition.js | 31 ++++++++++- 4 files changed, 70 insertions(+), 52 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index f563585aa10..dadbaee6007 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -152,38 +152,19 @@ const updateTransition = (change, transition, ok) => { }; const saveTransitions = (change, validRev) => { - return saveProperty(change.id, change.info, 'transitions', {}, validRev); + return modifyInfoDoc(change.id, infoDoc => { + infoDoc.transitions = (change.info && change.info.transitions) || {}; + if (validRev !== undefined) { + infoDoc.valid_rev = validRev; + delete infoDoc.invalid_rev; + } + }, change.info); }; const saveCompletedTasks = (id, infodoc, completedTasks = []) => { - return saveProperty(id, infodoc, 'completed_tasks', completedTasks); -}; - -const saveProperty = async (id, infodoc, property, defaultValue = {}, validRev) => { - let updatedInfoDoc; - try { - updatedInfoDoc = await db.sentinel.get(getInfoDocId(id)); - updatedInfoDoc[property] = (infodoc && infodoc[property]) || defaultValue; - } catch (err) { - if (err.status !== 404) { - throw err; - } - updatedInfoDoc = infodoc; - } - - if (validRev !== undefined) { - updatedInfoDoc.valid_rev = validRev; - delete updatedInfoDoc.invalid_rev; - } - - try { - return await db.sentinel.put(updatedInfoDoc); - } catch (err) { - if (err.status !== 409) { - throw err; - } - return saveProperty(id, infodoc, property, defaultValue, validRev); - } + return modifyInfoDoc(id, infoDoc => { + infoDoc.completed_tasks = (infodoc && infodoc.completed_tasks) || completedTasks; + }, infodoc); }; const setInvalidRev = (id, invalidRev) => { @@ -192,21 +173,24 @@ const setInvalidRev = (id, invalidRev) => { }); }; -const clearInvalidRev = id => { +const clearInvalidRev = (id) => { return modifyInfoDoc(id, infoDoc => { delete infoDoc.invalid_rev; }); }; -const modifyInfoDoc = async (id, modify) => { +// Fetch the infodoc, apply `modify`, and save, retrying on conflict so the change always lands on the +// latest rev. If the infodoc is missing, `fallback` is created and saved when provided; otherwise the +// 404 is raised. +const modifyInfoDoc = async (id, modify, fallback) => { let infoDoc; try { infoDoc = await db.sentinel.get(getInfoDocId(id)); } catch (err) { - if (err.status !== 404) { + if (err.status !== 404 || !fallback) { throw err; } - infoDoc = blankInfoDoc(id); + infoDoc = fallback; } modify(infoDoc); @@ -217,7 +201,7 @@ const modifyInfoDoc = async (id, modify) => { if (err.status !== 409) { throw err; } - return modifyInfoDoc(id, modify); + return modifyInfoDoc(id, modify, fallback); } }; diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index 36d5d1ffdb3..e76ae170398 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -631,18 +631,6 @@ describe('infodoc', () => { }); }); - it('setInvalidRev creates a blank infodoc on 404', () => { - sinon.stub(db.sentinel, 'get').rejects({ status: 404 }); - sinon.stub(db.sentinel, 'put').resolves(); - - return lib.setInvalidRev('some', null).then(() => { - const saved = db.sentinel.put.args[0][0]; - assert.equal(saved._id, 'some-info'); - assert.equal(saved.doc_id, 'some'); - assert.equal(saved.invalid_rev, null); - }); - }); - it('clearInvalidRev removes the mid-write marker', () => { const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; sinon.stub(db.sentinel, 'get').resolves(info); diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index aa341f8694f..080fcb4a446 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -131,7 +131,7 @@ const processDocs = docs => { saveDoc(change, (err, result) => { callback(null, err || result); }); - }); + }, { manageInfoDocRev: true }); }); async.series(operations, (err, results) => { return err ? reject(err) : resolve(results); @@ -253,7 +253,7 @@ const canRun = ({ key, change, transition }) => { * 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, manageInfoDocRev = false }, callback) => { logger.debug(`transition results: ${JSON.stringify(results)}`); const changed = _.some(results, i => Boolean(i)); @@ -275,6 +275,25 @@ const finalize = ({ change, results }, callback) => { } logger.debug(`calling saveDoc on doc ${change.id} seq ${change.seq}`); + if (!manageInfoDocRev) { + // Sentinel (async) branch: save the document first, then record the transitions map. Sentinel does + // not manage the valid_rev/invalid_rev markers; it only reads them (see getConsistentInfoDoc) to + // avoid processing a doc that API is still writing. + return saveDoc(change, (err, result) => { + if (err) { + logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); + return callback(err); + } + logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); + return infodoc + .saveTransitions(change) + .then(() => callback(null, result)) + .catch(saveErr => callback(saveErr)); + }); + } + + // API (sync) branch: bracket the doc write with invalid_rev/valid_rev markers so a concurrent + // sentinel read can detect a mid-write infodoc and wait. const invalidRev = change.doc._rev ?? null; return infodoc .setInvalidRev(change.id, invalidRev) @@ -359,7 +378,7 @@ const applyTransition = ({ key, change, transition, force }, callback) => { .then(changed => callback(null, changed)); // return the promise instead }; -const applyTransitions = (change, callback) => { +const applyTransitions = (change, callback, { manageInfoDocRev = false } = {}) => { const operations = transitions .map(transition => { const opts = { @@ -377,7 +396,7 @@ const applyTransitions = (change, callback) => { * 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, manageInfoDocRev }, callback)); }; const availableTransitions = () => { diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index ab197a99484..e07855d800f 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -22,7 +22,7 @@ describe('finalize transition', () => { ); }); - it('save is called if transition results have changes', done => { + it('save is called if transition results have changes (API branch manages rev)', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true, rev: '2' }); const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); @@ -32,6 +32,7 @@ describe('finalize transition', () => { { change: { id: 'abc', doc: doc }, results: [null, null, true], + manageInfoDocRev: true, }, (err, result) => { assert.equal(saveDoc.callCount, 1); @@ -48,7 +49,7 @@ describe('finalize transition', () => { ); }); - it('should callback with save errors', done => { + it('should callback with save errors and clear invalid_rev (API branch)', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, { error: 'something' }); const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); @@ -58,6 +59,7 @@ describe('finalize transition', () => { { change: { id: 'abc', doc: doc }, results: [null, null, true], + manageInfoDocRev: true, }, (err, result) => { assert.deepEqual(err, { error: 'something' }); @@ -73,6 +75,31 @@ describe('finalize transition', () => { ); }); + it('sentinel branch saves doc then transitions without managing valid/invalid rev', done => { + const doc = { _rev: '1' }; + const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true, rev: '2' }); + const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); + const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'saveTransitions').resolves(); + transitions.finalize( + { + change: { id: 'abc', doc: doc }, + results: [null, null, true], + }, + (err, result) => { + assert(!err); + assert.deepEqual(result, { ok: true, rev: '2' }); + assert.equal(saveDoc.callCount, 1); + assert.equal(infodoc.saveTransitions.callCount, 1); + // saveTransitions called without a validRev, so no rev management + assert.equal(infodoc.saveTransitions.args[0][1], undefined); + assert.equal(setInvalidRev.callCount, 0); + assert.equal(clearInvalidRev.callCount, 0); + done(); + } + ); + }); + it('applyTransition creates transitions property', done => { const doc = { _rev: '1' }; const info = {}; From 2add51a099834d28104eeaecc3343a6260e20960 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Tue, 2 Jun 2026 16:19:40 +0300 Subject: [PATCH 04/12] fix(#10875): readability --- .../transitions/src/transitions/index.js | 107 ++++++++---------- .../test/unit/finalize-transition.js | 6 +- .../transitions/test/unit/process_docs.js | 10 +- 3 files changed, 57 insertions(+), 66 deletions(-) diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index 080fcb4a446..be489026f82 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -128,9 +128,10 @@ const processDocs = docs => { // doc was not changed by any transition, so we save the original doc change.doc = docs.find(doc => doc._id === change.id); - saveDoc(change, (err, result) => { - callback(null, err || result); - }); + saveDoc(change).then( + result => callback(null, result), + err => callback(null, err) + ); }, { manageInfoDocRev: true }); }); async.series(operations, (err, results) => { @@ -254,70 +255,60 @@ const canRun = ({ key, change, transition }) => { * it then a change was made. */ const finalize = ({ change, results, manageInfoDocRev = false }, callback) => { + finalizeChange({ change, results, manageInfoDocRev }) + .then(result => callback(null, result)) + .catch(err => { + logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); + callback(err); + }); +}; + +const finalizeChange = async ({ change, results, manageInfoDocRev }) => { logger.debug(`transition results: ${JSON.stringify(results)}`); - const changed = _.some(results, i => Boolean(i)); - if (!changed) { - logger.debug( - `nothing changed skipping saveDoc for doc ${change.id} seq ${change.seq}` - ); - // info.transitions is how we know if a doc has been processed by Sentinel before. Even if no transitions ran, - // we still want to save transitions, so we know it's been processed. - return Promise - .resolve() - .then(() => { - if (change.initialProcessing) { - return infodoc.saveTransitions(change); - } - }) - .then(() => callback()) - .catch(err => callback(err)); + if (!_.some(results, Boolean)) { + logger.debug(`nothing changed skipping saveDoc for doc ${change.id} seq ${change.seq}`); + // info.transitions is how we know Sentinel has processed a doc; record it even when nothing ran. + if (change.initialProcessing) { + await infodoc.saveTransitions(change); + } + return; } + logger.debug(`calling saveDoc on doc ${change.id} seq ${change.seq}`); + return manageInfoDocRev ? saveForApi(change) : saveForSentinel(change); +}; - if (!manageInfoDocRev) { - // Sentinel (async) branch: save the document first, then record the transitions map. Sentinel does - // not manage the valid_rev/invalid_rev markers; it only reads them (see getConsistentInfoDoc) to - // avoid processing a doc that API is still writing. - return saveDoc(change, (err, result) => { - if (err) { - logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); - return callback(err); - } - logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); - return infodoc - .saveTransitions(change) - .then(() => callback(null, result)) - .catch(saveErr => callback(saveErr)); - }); - } +// Sentinel processing: save the doc, then record transitions. Sentinel never writes the +// valid_rev/invalid_rev markers; it only reads them (see getConsistentInfoDoc) to skip docs API is +// still writing. +const saveForSentinel = async change => { + const result = await saveDoc(change); + logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); + await infodoc.saveTransitions(change); + return result; +}; - // API (sync) branch: bracket the doc write with invalid_rev/valid_rev markers so a concurrent - // sentinel read can detect a mid-write infodoc and wait. - const invalidRev = change.doc._rev ?? null; - return infodoc - .setInvalidRev(change.id, invalidRev) - .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}`); - return infodoc - .saveTransitions(change, result.rev) - .then(() => callback(null, result)); - }) - .catch(err => { - logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); - return infodoc - .clearInvalidRev(change.id) - .catch(clearErr => logger.error(`error clearing invalid_rev on doc ${change.id}: %o`, clearErr)) - .then(() => callback(err)); - }); +// API processing: bracket the doc write with invalid_rev/valid_rev markers so a concurrent sentinel +// read can detect a mid-write infodoc and wait. Roll the marker back on any failure after it's set. +const saveForApi = async change => { + await infodoc.setInvalidRev(change.id, change.doc._rev ?? null); + try { + const result = await saveDoc(change); + logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); + await infodoc.saveTransitions(change, result.rev); + return result; + } catch (err) { + await infodoc + .clearInvalidRev(change.id) + .catch(clearErr => logger.error(`error clearing invalid_rev on doc ${change.id}: %o`, clearErr)); + throw err; + } }; -const saveDoc = (change, callback) => { +const saveDoc = change => { lineage.minify(change.doc); - db.medic.put(change.doc, callback); + return db.medic.put(change.doc); }; /* diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index e07855d800f..b2619e7e8bc 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -24,7 +24,7 @@ describe('finalize transition', () => { it('save is called if transition results have changes (API branch manages rev)', done => { const doc = { _rev: '1' }; - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true, rev: '2' }); + const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true, rev: '2' }); const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); @@ -51,7 +51,7 @@ describe('finalize transition', () => { it('should callback with save errors and clear invalid_rev (API branch)', done => { const doc = { _rev: '1' }; - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, { error: 'something' }); + const saveDoc = sinon.stub(db.medic, 'put').rejects({ error: 'something' }); const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); @@ -77,7 +77,7 @@ describe('finalize transition', () => { it('sentinel branch saves doc then transitions without managing valid/invalid rev', done => { const doc = { _rev: '1' }; - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true, rev: '2' }); + const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true, rev: '2' }); const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); diff --git a/shared-libs/transitions/test/unit/process_docs.js b/shared-libs/transitions/test/unit/process_docs.js index f7cec6291ef..7a3cec2f74e 100644 --- a/shared-libs/transitions/test/unit/process_docs.js +++ b/shared-libs/transitions/test/unit/process_docs.js @@ -99,7 +99,7 @@ describe('processDocs', () => { sinon.stub(infodoc, 'bulkGet').resolves(infoDocs); sinon.stub(infodoc, 'bulkUpdate').resolves(); sinon.stub(transitions, 'applyTransition'); - sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + sinon.stub(db.medic, 'put').resolves({ ok: true }); sinon.stub(infodoc, 'saveTransitions').resolves(); sinon.stub(infodoc, 'setInvalidRev').resolves(); sinon.stub(infodoc, 'clearInvalidRev').resolves(); @@ -177,10 +177,10 @@ describe('processDocs', () => { sinon.stub(infodoc, 'bulkUpdate').resolves(); sinon.stub(transitions, 'applyTransition'); sinon.stub(db.medic, 'put') - .withArgs(sinon.match({ _id: '1' })).callsArgWith(1, null, { ok: true }) - .withArgs(sinon.match({ _id: '2' })).callsArgWith(1, { error: 'error' }) - .withArgs(sinon.match({ _id: '3' })).callsArgWith(1, null, { ok: true }) - .withArgs(sinon.match({ _id: '4' })).callsArgWith(1, { error: 'error' }); + .withArgs(sinon.match({ _id: '1' })).resolves({ ok: true }) + .withArgs(sinon.match({ _id: '2' })).rejects({ error: 'error' }) + .withArgs(sinon.match({ _id: '3' })).resolves({ ok: true }) + .withArgs(sinon.match({ _id: '4' })).rejects({ error: 'error' }); sinon.stub(infodoc, 'saveTransitions').resolves(); sinon.stub(infodoc, 'setInvalidRev').resolves(); sinon.stub(infodoc, 'clearInvalidRev').resolves(); From 800b60f0f361398ff748df226c2179c44ca21a58 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Wed, 3 Jun 2026 10:27:30 +0300 Subject: [PATCH 05/12] fix(#10875): removing valid_rev --- shared-libs/infodoc/src/infodoc.js | 6 +++--- shared-libs/infodoc/test/infodoc.js | 5 ++--- shared-libs/transitions/src/transitions/index.js | 12 ++++++------ .../transitions/test/unit/finalize-transition.js | 4 ++-- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index dadbaee6007..3b2eaa90ee4 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -151,11 +151,11 @@ const updateTransition = (change, transition, ok) => { }; }; -const saveTransitions = (change, validRev) => { +const saveTransitions = (change, clearInvalid = false) => { return modifyInfoDoc(change.id, infoDoc => { infoDoc.transitions = (change.info && change.info.transitions) || {}; - if (validRev !== undefined) { - infoDoc.valid_rev = validRev; + // Clear the mid-write marker in the same write that commits the transitions (API branch only). + if (clearInvalid) { delete infoDoc.invalid_rev; } }, change.info); diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index e76ae170398..a857dbae9b9 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -605,15 +605,14 @@ describe('infodoc', () => { }); }); - it('should record validRev and clear invalid_rev when given', () => { + it('clears invalid_rev when clearInvalid is set', () => { const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; const change = { id: 'some', info: { transitions: { one: { ok: true } } } }; sinon.stub(db.sentinel, 'get').resolves(info); sinon.stub(db.sentinel, 'put').resolves(); - return lib.saveTransitions(change, '2-def').then(() => { + return lib.saveTransitions(change, true).then(() => { const saved = db.sentinel.put.args[0][0]; - assert.equal(saved.valid_rev, '2-def'); assert.isUndefined(saved.invalid_rev); }); }); diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index be489026f82..d9a80eac81c 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -279,9 +279,8 @@ const finalizeChange = async ({ change, results, manageInfoDocRev }) => { return manageInfoDocRev ? saveForApi(change) : saveForSentinel(change); }; -// Sentinel processing: save the doc, then record transitions. Sentinel never writes the -// valid_rev/invalid_rev markers; it only reads them (see getConsistentInfoDoc) to skip docs API is -// still writing. +// Sentinel processing: save the doc, then record transitions. Sentinel never writes the invalid_rev +// marker; it only reads it (see getConsistentInfoDoc) to skip docs API is still writing. const saveForSentinel = async change => { const result = await saveDoc(change); logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); @@ -289,14 +288,15 @@ const saveForSentinel = async change => { return result; }; -// API processing: bracket the doc write with invalid_rev/valid_rev markers so a concurrent sentinel -// read can detect a mid-write infodoc and wait. Roll the marker back on any failure after it's set. +// API processing: mark the infodoc mid-write (invalid_rev) around the doc write so a concurrent +// sentinel read detects it and waits. The marker is cleared as part of the transitions write on +// success, or rolled back on any failure after it's set. const saveForApi = async change => { await infodoc.setInvalidRev(change.id, change.doc._rev ?? null); try { const result = await saveDoc(change); logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); - await infodoc.saveTransitions(change, result.rev); + await infodoc.saveTransitions(change, true); return result; } catch (err) { await infodoc diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index b2619e7e8bc..707a7ee77fa 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -42,7 +42,7 @@ describe('finalize transition', () => { assert.equal(setInvalidRev.callCount, 1); assert.deepEqual(setInvalidRev.args[0], ['abc', '1']); assert.equal(infodoc.saveTransitions.callCount, 1); - assert.deepEqual(infodoc.saveTransitions.args[0][1], '2'); + assert.strictEqual(infodoc.saveTransitions.args[0][1], true); assert.equal(clearInvalidRev.callCount, 0); done(); } @@ -91,7 +91,7 @@ describe('finalize transition', () => { assert.deepEqual(result, { ok: true, rev: '2' }); assert.equal(saveDoc.callCount, 1); assert.equal(infodoc.saveTransitions.callCount, 1); - // saveTransitions called without a validRev, so no rev management + // saveTransitions called without the clearInvalid flag, so the marker is left untouched assert.equal(infodoc.saveTransitions.args[0][1], undefined); assert.equal(setInvalidRev.callCount, 0); assert.equal(clearInvalidRev.callCount, 0); From 5def568907e967f6ede5cf7d25249aa2bd7fee43 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Mon, 8 Jun 2026 12:14:59 +0300 Subject: [PATCH 06/12] fix(#10875): sonar and small fiddles --- shared-libs/infodoc/src/infodoc.js | 20 +++++++++------ .../test/unit/finalize-transition.js | 25 ------------------- 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 3b2eaa90ee4..27fe645fcfb 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -154,7 +154,7 @@ const updateTransition = (change, transition, ok) => { const saveTransitions = (change, clearInvalid = false) => { return modifyInfoDoc(change.id, infoDoc => { infoDoc.transitions = (change.info && change.info.transitions) || {}; - // Clear the mid-write marker in the same write that commits the transitions (API branch only). + // If there is a mid-write flag set, clear it now to mark the infoDoc valid again if (clearInvalid) { delete infoDoc.invalid_rev; } @@ -179,19 +179,23 @@ const clearInvalidRev = (id) => { }); }; -// Fetch the infodoc, apply `modify`, and save, retrying on conflict so the change always lands on the -// latest rev. If the infodoc is missing, `fallback` is created and saved when provided; otherwise the -// 404 is raised. -const modifyInfoDoc = async (id, modify, fallback) => { - let infoDoc; +// Fetch the infodoc. If it is missing, return `fallback` (to be created) when provided; +// otherwise the 404 is raised. +const fetchInfoDoc = async (id, fallback) => { try { - infoDoc = await db.sentinel.get(getInfoDocId(id)); + return await db.sentinel.get(getInfoDocId(id)); } catch (err) { if (err.status !== 404 || !fallback) { throw err; } - infoDoc = fallback; + return fallback; } +}; + +// Fetch the infodoc, apply `modify`, and save, retrying on conflict so the change always lands on the +// latest rev. +const modifyInfoDoc = async (id, modify, fallback) => { + const infoDoc = await fetchInfoDoc(id, fallback); modify(infoDoc); diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index 707a7ee77fa..911044ec547 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -75,31 +75,6 @@ describe('finalize transition', () => { ); }); - it('sentinel branch saves doc then transitions without managing valid/invalid rev', done => { - const doc = { _rev: '1' }; - const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true, rev: '2' }); - const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); - const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); - sinon.stub(infodoc, 'saveTransitions').resolves(); - transitions.finalize( - { - change: { id: 'abc', doc: doc }, - results: [null, null, true], - }, - (err, result) => { - assert(!err); - assert.deepEqual(result, { ok: true, rev: '2' }); - assert.equal(saveDoc.callCount, 1); - assert.equal(infodoc.saveTransitions.callCount, 1); - // saveTransitions called without the clearInvalid flag, so the marker is left untouched - assert.equal(infodoc.saveTransitions.args[0][1], undefined); - assert.equal(setInvalidRev.callCount, 0); - assert.equal(clearInvalidRev.callCount, 0); - done(); - } - ); - }); - it('applyTransition creates transitions property', done => { const doc = { _rev: '1' }; const info = {}; From 08acb4ce6afd1f4665f6c49a0363e1d8c06f32c0 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Mon, 8 Jun 2026 14:15:38 +0300 Subject: [PATCH 07/12] fix(#10875): changing from invalid_rev to transitions_started --- shared-libs/infodoc/src/infodoc.js | 23 +++++++------ shared-libs/infodoc/test/infodoc.js | 28 ++++++++------- .../transitions/src/transitions/index.js | 14 ++++---- .../test/integration/transitions.js | 34 +++++++++---------- .../test/unit/finalize-transition.js | 22 ++++++------ .../transitions/test/unit/process_docs.js | 18 +++++----- 6 files changed, 72 insertions(+), 67 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 27fe645fcfb..348cc55d9b3 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -151,12 +151,12 @@ const updateTransition = (change, transition, ok) => { }; }; -const saveTransitions = (change, clearInvalid = false) => { +const saveTransitions = (change, clearStarted = false) => { return modifyInfoDoc(change.id, infoDoc => { infoDoc.transitions = (change.info && change.info.transitions) || {}; - // If there is a mid-write flag set, clear it now to mark the infoDoc valid again - if (clearInvalid) { - delete infoDoc.invalid_rev; + // Clear the in-progress marker in the same write that commits the transitions (API branch only) + if (clearStarted) { + delete infoDoc.transitions_started; } }, change.info); }; @@ -167,15 +167,18 @@ const saveCompletedTasks = (id, infodoc, completedTasks = []) => { }, infodoc); }; -const setInvalidRev = (id, invalidRev) => { +// Mark the infodoc as having transitions in progress (API is running transitions and writing the doc). +// Stored as a timestamp so stale markers from a crashed write are diagnosable; the locking logic only +// checks presence/absence of the field. +const markTransitionsStarted = (id) => { return modifyInfoDoc(id, infoDoc => { - infoDoc.invalid_rev = invalidRev; + infoDoc.transitions_started = new Date().toISOString(); }); }; -const clearInvalidRev = (id) => { +const clearTransitionsStarted = (id) => { return modifyInfoDoc(id, infoDoc => { - delete infoDoc.invalid_rev; + delete infoDoc.transitions_started; }); }; @@ -311,8 +314,8 @@ module.exports = { bulkUpdate: bulkUpdate, saveTransitions: saveTransitions, saveCompletedTasks: saveCompletedTasks, - setInvalidRev: setInvalidRev, - clearInvalidRev: clearInvalidRev, + markTransitionsStarted: markTransitionsStarted, + clearTransitionsStarted: clearTransitionsStarted, // Used to update infodoc metadata that occurs at write time. A delete does not count as a write // in this instance, as deletes resolve as infodoc cleanups once sentinel's background-cleanup diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index a857dbae9b9..b4a14da26c3 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -605,38 +605,40 @@ describe('infodoc', () => { }); }); - it('clears invalid_rev when clearInvalid is set', () => { - const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; + it('clears transitions_started when clearStarted is set', () => { + const info = { _id: 'some-info', doc_id: 'some', transitions_started: '2026-01-01T00:00:00.000Z' }; const change = { id: 'some', info: { transitions: { one: { ok: true } } } }; sinon.stub(db.sentinel, 'get').resolves(info); sinon.stub(db.sentinel, 'put').resolves(); return lib.saveTransitions(change, true).then(() => { const saved = db.sentinel.put.args[0][0]; - assert.isUndefined(saved.invalid_rev); + assert.isUndefined(saved.transitions_started); }); }); }); - describe('setInvalidRev / clearInvalidRev', () => { - it('setInvalidRev marks the infodoc mid-write', () => { + describe('markTransitionsStarted / clearTransitionsStarted', () => { + it('markTransitionsStarted marks the infodoc mid-write with a timestamp', () => { const info = { _id: 'some-info', doc_id: 'some' }; sinon.stub(db.sentinel, 'get').resolves(info); sinon.stub(db.sentinel, 'put').resolves(); - return lib.setInvalidRev('some', '1-abc').then(() => { + return lib.markTransitionsStarted('some').then(() => { assert.deepEqual(db.sentinel.get.args[0], ['some-info']); - assert.equal(db.sentinel.put.args[0][0].invalid_rev, '1-abc'); + const saved = db.sentinel.put.args[0][0]; + assert.isString(saved.transitions_started); + assert.isNotNaN(Date.parse(saved.transitions_started)); }); }); - it('clearInvalidRev removes the mid-write marker', () => { - const info = { _id: 'some-info', doc_id: 'some', invalid_rev: '1-abc' }; + it('clearTransitionsStarted removes the mid-write marker', () => { + const info = { _id: 'some-info', doc_id: 'some', transitions_started: '2026-01-01T00:00:00.000Z' }; sinon.stub(db.sentinel, 'get').resolves(info); sinon.stub(db.sentinel, 'put').resolves(); - return lib.clearInvalidRev('some').then(() => { - assert.isUndefined(db.sentinel.put.args[0][0].invalid_rev); + return lib.clearTransitionsStarted('some').then(() => { + assert.isUndefined(db.sentinel.put.args[0][0].transitions_started); }); }); @@ -647,7 +649,7 @@ describe('infodoc', () => { put.onCall(0).rejects({ status: 409 }); put.onCall(1).resolves(); - return lib.setInvalidRev('some', '1-abc').then(() => { + return lib.markTransitionsStarted('some').then(() => { assert.equal(db.sentinel.put.callCount, 2); }); }); @@ -656,7 +658,7 @@ describe('infodoc', () => { sinon.stub(db.sentinel, 'get').resolves({ _id: 'some-info', doc_id: 'some' }); sinon.stub(db.sentinel, 'put').rejects({ status: 500 }); - return lib.setInvalidRev('some', '1-abc') + return lib.markTransitionsStarted('some') .then(() => assert.fail('should have thrown')) .catch(err => assert.equal(err.status, 500)); }); diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index 8da2e7af55b..8fb34fc2255 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -48,7 +48,7 @@ let loadErrors = false; const MAX_INFODOC_WAIT = 5; const INFODOC_WAIT_INTERVAL = 100; -const isInfoDocMidWrite = infoDoc => infoDoc && infoDoc.invalid_rev !== undefined; +const isInfoDocMidWrite = infoDoc => infoDoc && infoDoc.transitions_started !== undefined; const getConsistentInfoDoc = (change, retriesLeft) => { return infodoc.get(change).then(infoDoc => { @@ -282,8 +282,8 @@ const finalizeChange = async ({ change, results, manageInfoDocRev }) => { return manageInfoDocRev ? saveForApi(change) : saveForSentinel(change); }; -// Sentinel processing: save the doc, then record transitions. Sentinel never writes the invalid_rev -// marker; it only reads it (see getConsistentInfoDoc) to skip docs API is still writing. +// Sentinel processing: save the doc, then record transitions. Sentinel never writes the +// transitions_started marker; it only reads it (see getConsistentInfoDoc) to skip docs API is writing. const saveForSentinel = async change => { const result = await saveDoc(change); logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); @@ -291,11 +291,11 @@ const saveForSentinel = async change => { return result; }; -// API processing: mark the infodoc mid-write (invalid_rev) around the doc write so a concurrent +// API processing: mark the infodoc mid-write (transitions_started) around the doc write so a concurrent // sentinel read detects it and waits. The marker is cleared as part of the transitions write on // success, or rolled back on any failure after it's set. const saveForApi = async change => { - await infodoc.setInvalidRev(change.id, change.doc._rev ?? null); + await infodoc.markTransitionsStarted(change.id); try { const result = await saveDoc(change); logger.info(`saved changes on doc ${change.id} seq ${change.seq}`); @@ -303,8 +303,8 @@ const saveForApi = async change => { return result; } catch (err) { await infodoc - .clearInvalidRev(change.id) - .catch(clearErr => logger.error(`error clearing invalid_rev on doc ${change.id}: %o`, clearErr)); + .clearTransitionsStarted(change.id) + .catch(clearErr => logger.error(`error clearing transitions_started on doc ${change.id}: %o`, clearErr)); throw err; } }; diff --git a/shared-libs/transitions/test/integration/transitions.js b/shared-libs/transitions/test/integration/transitions.js index 1996cb6bf14..aa474a5b74f 100644 --- a/shared-libs/transitions/test/integration/transitions.js +++ b/shared-libs/transitions/test/integration/transitions.js @@ -41,10 +41,10 @@ describe('functional transitions', () => { }, }); const infoDocSave = sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); sinon.stub(db.medic, 'get').rejects({ status: 404 }); - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true }); transitions.loadTransitions(); const change1 = { @@ -99,10 +99,10 @@ describe('functional transitions', () => { }, }); - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true }); const infoDoc = sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); transitions.loadTransitions(); const change1 = { @@ -168,10 +168,10 @@ describe('functional transitions', () => { }); configGet.withArgs('forms').returns({ V: { }}); - const saveDoc = sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true }); const infoDoc = sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); transitions.loadTransitions(); const change1 = { @@ -270,7 +270,7 @@ describe('functional transitions', () => { transitions.loadTransitions(); transitions.processChange({ id: doc._id}, (err, result) => { - assert.isUndefined(err); + assert.isNull(err); assert.isUndefined(result); assert.equal(infodoc.get.callCount, 1); @@ -299,10 +299,10 @@ describe('functional transitions', () => { sinon.stub(infodoc, 'get').resolves(info); sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); - sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + sinon.stub(db.medic, 'put').resolves({ ok: true }); sinon.spy(transitions, 'applyTransitions'); const doc = { @@ -359,9 +359,9 @@ describe('functional transitions', () => { sinon.stub(infodoc, 'get').resolves({}); sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); - sinon.stub(db.medic, 'put').callsArgWith(1, { error: 'something' }); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); + sinon.stub(db.medic, 'put').rejects({ error: 'something' }); const doc = { _id: 'my_id', @@ -532,7 +532,7 @@ describe('functional transitions', () => { sinon.stub(db.sentinel, 'get').callsFake(id => Promise.resolve({ id, doc_id: id.replace('-info', '') })); sinon.stub(db.sentinel, 'put').resolves(); - sinon.stub(db.medic, 'put').callsArgWith(1, null, { ok: true }); + sinon.stub(db.medic, 'put').resolves({ ok: true }); sinon.stub(db.medic, 'query') // update_clinics diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index 911044ec547..1d81f73598a 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -25,8 +25,8 @@ describe('finalize transition', () => { it('save is called if transition results have changes (API branch manages rev)', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true, rev: '2' }); - const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); - const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); + const markTransitionsStarted = sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + const clearTransitionsStarted = sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); transitions.finalize( { @@ -39,21 +39,21 @@ describe('finalize transition', () => { assert(saveDoc.args[0][0]._rev); assert(!err); assert.deepEqual(result, { ok: true, rev: '2' }); - assert.equal(setInvalidRev.callCount, 1); - assert.deepEqual(setInvalidRev.args[0], ['abc', '1']); + assert.equal(markTransitionsStarted.callCount, 1); + assert.deepEqual(markTransitionsStarted.args[0], ['abc']); assert.equal(infodoc.saveTransitions.callCount, 1); assert.strictEqual(infodoc.saveTransitions.args[0][1], true); - assert.equal(clearInvalidRev.callCount, 0); + assert.equal(clearTransitionsStarted.callCount, 0); done(); } ); }); - it('should callback with save errors and clear invalid_rev (API branch)', done => { + it('should callback with save errors and clear the mid-write marker (API branch)', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').rejects({ error: 'something' }); - const setInvalidRev = sinon.stub(infodoc, 'setInvalidRev').resolves(); - const clearInvalidRev = sinon.stub(infodoc, 'clearInvalidRev').resolves(); + const markTransitionsStarted = sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + const clearTransitionsStarted = sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); sinon.stub(infodoc, 'saveTransitions').resolves(); transitions.finalize( { @@ -66,10 +66,10 @@ describe('finalize transition', () => { assert.equal(saveDoc.callCount, 1); assert(saveDoc.args[0][0]._rev); assert(!result); - assert.equal(setInvalidRev.callCount, 1); + assert.equal(markTransitionsStarted.callCount, 1); assert.equal(infodoc.saveTransitions.callCount, 0); - assert.equal(clearInvalidRev.callCount, 1); - assert.deepEqual(clearInvalidRev.args[0], ['abc']); + assert.equal(clearTransitionsStarted.callCount, 1); + assert.deepEqual(clearTransitionsStarted.args[0], ['abc']); done(); } ); diff --git a/shared-libs/transitions/test/unit/process_docs.js b/shared-libs/transitions/test/unit/process_docs.js index 7a3cec2f74e..fe844862d67 100644 --- a/shared-libs/transitions/test/unit/process_docs.js +++ b/shared-libs/transitions/test/unit/process_docs.js @@ -101,8 +101,8 @@ describe('processDocs', () => { sinon.stub(transitions, 'applyTransition'); sinon.stub(db.medic, 'put').resolves({ ok: true }); sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); // first doc is updated by at least one transition transitions.applyTransition @@ -150,8 +150,8 @@ describe('processDocs', () => { chai.expect(infodoc.saveTransitions.callCount).to.equal(3); chai.expect(infodoc.saveTransitions.calledWithMatch({ id: '1' })).to.equal(true); - chai.expect(infodoc.setInvalidRev.callCount).to.equal(1); - chai.expect(infodoc.setInvalidRev.calledWith('1')).to.equal(true); + chai.expect(infodoc.markTransitionsStarted.callCount).to.equal(1); + chai.expect(infodoc.markTransitionsStarted.calledWith('1')).to.equal(true); }); }); @@ -182,8 +182,8 @@ describe('processDocs', () => { .withArgs(sinon.match({ _id: '3' })).resolves({ ok: true }) .withArgs(sinon.match({ _id: '4' })).rejects({ error: 'error' }); sinon.stub(infodoc, 'saveTransitions').resolves(); - sinon.stub(infodoc, 'setInvalidRev').resolves(); - sinon.stub(infodoc, 'clearInvalidRev').resolves(); + sinon.stub(infodoc, 'markTransitionsStarted').resolves(); + sinon.stub(infodoc, 'clearTransitionsStarted').resolves(); // first doc is updated by at least one transition transitions.applyTransition @@ -237,9 +237,9 @@ describe('processDocs', () => { chai.expect(infodoc.saveTransitions.callCount).to.equal(3); chai.expect(infodoc.saveTransitions.calledWithMatch({ id: '1' })).to.equal(true); - chai.expect(infodoc.setInvalidRev.callCount).to.equal(2); - chai.expect(infodoc.clearInvalidRev.callCount).to.equal(1); - chai.expect(infodoc.clearInvalidRev.calledWith('2')).to.equal(true); + chai.expect(infodoc.markTransitionsStarted.callCount).to.equal(2); + chai.expect(infodoc.clearTransitionsStarted.callCount).to.equal(1); + chai.expect(infodoc.clearTransitionsStarted.calledWith('2')).to.equal(true); }); }); }); From ffc737b8846e13e66b9f51e478cc6e8ebee3e2f9 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Wed, 10 Jun 2026 11:59:12 +0300 Subject: [PATCH 08/12] fix(#10875): adding tests, small changes to infodoc --- shared-libs/infodoc/src/infodoc.js | 28 ++--- shared-libs/infodoc/test/infodoc.js | 28 ++++- .../transitions/src/transitions/index.js | 29 +++-- .../test/integration/transitions.js | 117 ++++++++++++++++++ .../test/unit/finalize-transition.js | 6 +- 5 files changed, 169 insertions(+), 39 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 348cc55d9b3..d2284cdec35 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -152,13 +152,14 @@ const updateTransition = (change, transition, ok) => { }; const saveTransitions = (change, clearStarted = false) => { - return modifyInfoDoc(change.id, infoDoc => { + const modify = infoDoc => { infoDoc.transitions = (change.info && change.info.transitions) || {}; // Clear the in-progress marker in the same write that commits the transitions (API branch only) if (clearStarted) { delete infoDoc.transitions_started; } - }, change.info); + }; + return modifyInfoDoc(change.id, modify, change.info); }; const saveCompletedTasks = (id, infodoc, completedTasks = []) => { @@ -167,36 +168,33 @@ const saveCompletedTasks = (id, infodoc, completedTasks = []) => { }, infodoc); }; -// Mark the infodoc as having transitions in progress (API is running transitions and writing the doc). -// Stored as a timestamp so stale markers from a crashed write are diagnosable; the locking logic only -// checks presence/absence of the field. const markTransitionsStarted = (id) => { - return modifyInfoDoc(id, infoDoc => { + const modify = infoDoc => { infoDoc.transitions_started = new Date().toISOString(); - }); + }; + return modifyInfoDoc(id, modify, blankInfoDoc(id)); }; const clearTransitionsStarted = (id) => { - return modifyInfoDoc(id, infoDoc => { + const modify = infoDoc => { delete infoDoc.transitions_started; - }); + }; + return modifyInfoDoc(id, modify, blankInfoDoc(id)); }; // Fetch the infodoc. If it is missing, return `fallback` (to be created) when provided; -// otherwise the 404 is raised. const fetchInfoDoc = async (id, fallback) => { try { return await db.sentinel.get(getInfoDocId(id)); } catch (err) { - if (err.status !== 404 || !fallback) { - throw err; + if (err.status === 404 && fallback) { + return fallback; } - return fallback; + throw err; } }; -// Fetch the infodoc, apply `modify`, and save, retrying on conflict so the change always lands on the -// latest rev. +// Fetch the infodoc, apply `modify`, and save, retrying on conflict const modifyInfoDoc = async (id, modify, fallback) => { const infoDoc = await fetchInfoDoc(id, fallback); diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index b4a14da26c3..c8d1aef51a0 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -613,7 +613,12 @@ describe('infodoc', () => { return lib.saveTransitions(change, true).then(() => { const saved = db.sentinel.put.args[0][0]; - assert.isUndefined(saved.transitions_started); + // transitions are written and the mid-write marker removed; nothing else is touched + assert.deepEqual(saved, { + _id: 'some-info', + doc_id: 'some', + transitions: { one: { ok: true } }, + }); }); }); }); @@ -629,6 +634,12 @@ describe('infodoc', () => { const saved = db.sentinel.put.args[0][0]; assert.isString(saved.transitions_started); assert.isNotNaN(Date.parse(saved.transitions_started)); + // only the marker is added, no other fields are changed + assert.deepEqual(saved, { + _id: 'some-info', + doc_id: 'some', + transitions_started: saved.transitions_started, + }); }); }); @@ -638,19 +649,24 @@ describe('infodoc', () => { sinon.stub(db.sentinel, 'put').resolves(); return lib.clearTransitionsStarted('some').then(() => { - assert.isUndefined(db.sentinel.put.args[0][0].transitions_started); + const saved = db.sentinel.put.args[0][0]; + // only the marker is removed, no other fields are changed + assert.deepEqual(saved, { _id: 'some-info', doc_id: 'some' }); }); }); - it('retries on 409 conflict', () => { + it('retries on 409 conflict indefinitely (no retry limit)', () => { const info = { _id: 'some-info', doc_id: 'some' }; sinon.stub(db.sentinel, 'get').resolves(info); const put = sinon.stub(db.sentinel, 'put'); - put.onCall(0).rejects({ status: 409 }); - put.onCall(1).resolves(); + // conflict on the first 100 attempts, succeed on the 101st - a retry limit below this would fail + for (let i = 0; i < 100; i++) { + put.onCall(i).rejects({ status: 409 }); + } + put.onCall(100).resolves(); return lib.markTransitionsStarted('some').then(() => { - assert.equal(db.sentinel.put.callCount, 2); + assert.equal(db.sentinel.put.callCount, 101); }); }); diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index 8fb34fc2255..b392919b2e7 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -50,14 +50,13 @@ const INFODOC_WAIT_INTERVAL = 100; const isInfoDocMidWrite = infoDoc => infoDoc && infoDoc.transitions_started !== undefined; -const getConsistentInfoDoc = (change, retriesLeft) => { - return infodoc.get(change).then(infoDoc => { - if (!isInfoDocMidWrite(infoDoc) || retriesLeft <= 0) { - return infoDoc; - } - return new Promise(resolve => setTimeout(resolve, INFODOC_WAIT_INTERVAL)) - .then(() => getConsistentInfoDoc(change, retriesLeft - 1)); - }); +const getConsistentInfoDoc = async (change, retriesLeft) => { + const infoDoc = await infodoc.get(change); + if (!isInfoDocMidWrite(infoDoc) || retriesLeft <= 0) { + return infoDoc; + } + await new Promise(resolve => setTimeout(resolve, INFODOC_WAIT_INTERVAL)); + return getConsistentInfoDoc(change, retriesLeft - 1); }; // applies all loaded transitions over a change @@ -132,7 +131,7 @@ const processDocs = docs => { result => callback(null, result), err => callback(null, err) ); - }, { manageInfoDocRev: true }); + }, { markStarted: true }); }); async.series(operations, (err, results) => { return err ? reject(err) : resolve(results); @@ -257,8 +256,8 @@ const canRun = ({ key, change, transition }) => { * did nothing and saving is unnecessary. If results has a true value in * it then a change was made. */ -const finalize = ({ change, results, manageInfoDocRev = false }, callback) => { - finalizeChange({ change, results, manageInfoDocRev }) +const finalize = ({ change, results, markStarted = false }, callback) => { + finalizeChange({ change, results, markStarted }) .then(result => callback(null, result)) .catch(err => { logger.error(`error saving changes on doc ${change.id} seq ${change.seq}: %o`, err); @@ -266,7 +265,7 @@ const finalize = ({ change, results, manageInfoDocRev = false }, callback) => { }); }; -const finalizeChange = async ({ change, results, manageInfoDocRev }) => { +const finalizeChange = async ({ change, results, markStarted }) => { logger.debug(`transition results: ${JSON.stringify(results)}`); if (!_.some(results, Boolean)) { @@ -279,7 +278,7 @@ const finalizeChange = async ({ change, results, manageInfoDocRev }) => { } logger.debug(`calling saveDoc on doc ${change.id} seq ${change.seq}`); - return manageInfoDocRev ? saveForApi(change) : saveForSentinel(change); + return markStarted ? saveForApi(change) : saveForSentinel(change); }; // Sentinel processing: save the doc, then record transitions. Sentinel never writes the @@ -372,7 +371,7 @@ const applyTransition = ({ key, change, transition, force }, callback) => { .then(changed => callback(null, changed)); // return the promise instead }; -const applyTransitions = (change, callback, { manageInfoDocRev = false } = {}) => { +const applyTransitions = (change, callback, { markStarted = false } = {}) => { const operations = transitions .map(transition => { const opts = { @@ -390,7 +389,7 @@ const applyTransitions = (change, callback, { manageInfoDocRev = false } = {}) = * function. All we care about are results and whether we need to * save or not. */ - async.series(operations, (err, results) => finalize({ change, results, manageInfoDocRev }, callback)); + async.series(operations, (err, results) => finalize({ change, results, markStarted }, callback)); }; const availableTransitions = () => { diff --git a/shared-libs/transitions/test/integration/transitions.js b/shared-libs/transitions/test/integration/transitions.js index aa474a5b74f..5bf30a495e2 100644 --- a/shared-libs/transitions/test/integration/transitions.js +++ b/shared-libs/transitions/test/integration/transitions.js @@ -5,6 +5,7 @@ const chaiExclude = require('chai-exclude'); const db = require('../../src/db'); const config = require('../../src/config'); const infodoc = require('@medic/infodoc'); +const logger = require('@medic/logger'); const dataContext = require('../../src/data-context'); const { Contact } = require('@medic/cht-datasource'); const { DOC_TYPES, CONTACT_TYPES } = require('@medic/constants'); @@ -388,6 +389,76 @@ describe('functional transitions', () => { done(); }); }); + + // fake timers so the 100ms retry waits don't cost real time or flake near mocha's timeout; + // tickAsync advances the clock and flushes the awaited infodoc.get between each retry + const RETRY_INTERVAL = 100; + const MAX_RETRIES = 5; + const processChangeWithFakeTimers = async (change) => { + const clock = sinon.useFakeTimers(); + const processed = new Promise((resolve, reject) => { + transitions.processChange(change, (err, result) => (err ? reject(err) : resolve(result))); + }); + // one extra tick past MAX_RETRIES is harmless and guarantees the change settles either way + for (let i = 0; i <= MAX_RETRIES; i++) { + await clock.tickAsync(RETRY_INTERVAL); + } + return processed; + }; + + it('should wait for transitions_started to clear before processing the change', async () => { + configGet.withArgs('transitions').returns({ conditional_alerts: {} }); + configGet.withArgs('forms').returns({ V: { } }); + + // infodoc is mid-write (API is running transitions) on the first two reads, then clears + const get = sinon.stub(infodoc, 'get'); + get.onCall(0).resolves({ _id: 'my_id-info', transitions: {}, transitions_started: '2026-01-01T00:00:00.000Z' }); + get.onCall(1).resolves({ _id: 'my_id-info', transitions: {}, transitions_started: '2026-01-01T00:00:00.000Z' }); + get.onCall(2).resolves({ _id: 'my_id-info', transitions: {} }); + const saveTransitions = sinon.stub(infodoc, 'saveTransitions').resolves(); + const put = sinon.stub(db.medic, 'put').resolves({ ok: true }); + + const doc = { _id: 'my_id', _rev: '1-abc', reported_date: 1 }; + sinon.stub(transitions._lineage, 'fetchHydratedDoc').resolves(doc); + + transitions.loadTransitions(); + const result = await processChangeWithFakeTimers({ id: doc._id }); + + assert.isUndefined(result); + // initial read + 2 reads where the marker was still set + assert.equal(infodoc.get.callCount, 3); + // no transition matched, so nothing is saved, but the change was processed (not skipped) + assert.equal(saveTransitions.callCount, 0); + assert.equal(put.callCount, 0); + }); + + it('should skip processing the change when transitions_started never clears', async () => { + configGet.withArgs('transitions').returns({ conditional_alerts: {} }); + configGet.withArgs('forms').returns({ V: { } }); + + // infodoc stays mid-write forever + sinon.stub(infodoc, 'get') + .resolves({ _id: 'my_id-info', transitions: {}, transitions_started: '2026-01-01T00:00:00.000Z' }); + const saveTransitions = sinon.stub(infodoc, 'saveTransitions').resolves(); + const put = sinon.stub(db.medic, 'put').resolves({ ok: true }); + + const doc = { _id: 'my_id', _rev: '1-abc', reported_date: 1 }; + sinon.stub(transitions._lineage, 'fetchHydratedDoc').resolves(doc); + + transitions.loadTransitions(); + // stub after loadTransitions so we only capture the skip warning, not the "disabled transition" ones + const warn = sinon.stub(logger, 'warn'); + const result = await processChangeWithFakeTimers({ id: doc._id }); + + assert.isUndefined(result); + // initial read + 5 retries, then it gives up + assert.equal(infodoc.get.callCount, 6); + // the change is skipped: no transitions run and nothing is saved + assert.equal(saveTransitions.callCount, 0); + assert.equal(put.callCount, 0); + assert.equal(warn.callCount, 1); + assert.match(warn.args[0][0], /still mid-write after 5 retries, skipping/); + }); }); describe('processDocs', () => { @@ -579,7 +650,21 @@ describe('functional transitions', () => { // first doc is updated by 3 transitions infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[0]._id); assert.equal(infodocSaves.length, 2); + // the first write only marks the infodoc as mid-write, with no transitions yet + let startedSave = infodocSaves.find(args => args[0].transitions_started)[0]; + assert.isString(startedSave.transitions_started); + assert.deepEqual(startedSave, { + id: `${savedDocs[0]._id}-info`, + doc_id: savedDocs[0]._id, + transitions_started: startedSave.transitions_started, + }); + // the second write commits the transitions and removes the mid-write marker let txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.isUndefined(txnSave.transitions_started); + assert.sameMembers( + Object.keys(txnSave.transitions), + ['update_clinics', 'update_sent_by', 'conditional_alerts'] + ); assert.equal(txnSave.transitions.update_clinics.ok, true); assert.equal(txnSave.transitions.update_sent_by.ok, true); assert.equal(txnSave.transitions.conditional_alerts.ok, true); @@ -593,20 +678,40 @@ describe('functional transitions', () => { assert.deepEqualExcluding(savedDocs[1], originalDocs[1], ['_id', 'tasks', 'errors']); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[1]._id); assert.equal(infodocSaves.length, 2); + startedSave = infodocSaves.find(args => args[0].transitions_started)[0]; + assert.isString(startedSave.transitions_started); + assert.deepEqual(startedSave, { + id: `${savedDocs[1]._id}-info`, + doc_id: savedDocs[1]._id, + transitions_started: startedSave.transitions_started, + }); txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.isUndefined(txnSave.transitions_started); + assert.sameMembers(Object.keys(txnSave.transitions), ['default_responses', 'update_clinics']); assert.equal(txnSave.transitions.default_responses.ok, true); assert.equal(txnSave.transitions.update_clinics.ok, true); assert.deepEqualExcluding(savedDocs[2], originalDocs[2], '_id'); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[2]._id); + // no transition changed this doc, so it is saved as-is with a single infodoc write (no marker) assert.equal(infodocSaves.length, 1); + assert.isUndefined(infodocSaves[0][0].transitions_started); assert.equal(savedDocs[3].id, 'random form with contact'); assert.equal(savedDocs[3].sent_by, 'Angela'); assert.deepEqualExcluding(savedDocs[3], originalDocs[3], 'sent_by'); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[3]._id); assert.equal(infodocSaves.length, 2); + startedSave = infodocSaves.find(args => args[0].transitions_started)[0]; + assert.isString(startedSave.transitions_started); + assert.deepEqual(startedSave, { + id: `${savedDocs[3]._id}-info`, + doc_id: savedDocs[3]._id, + transitions_started: startedSave.transitions_started, + }); txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.isUndefined(txnSave.transitions_started); + assert.sameMembers(Object.keys(txnSave.transitions), ['default_responses', 'update_sent_by']); assert.equal(txnSave.transitions.default_responses.ok, true); assert.equal(txnSave.transitions.update_sent_by.ok, true); @@ -623,7 +728,19 @@ describe('functional transitions', () => { assert.deepEqualExcluding(savedDocs[4], originalDocs[4], ['_id', 'sent_by', 'errors', 'tasks']); infodocSaves = db.sentinel.put.args.filter(args => args[0].doc_id === savedDocs[4]._id); assert.equal(infodocSaves.length, 2); + startedSave = infodocSaves.find(args => args[0].transitions_started)[0]; + assert.isString(startedSave.transitions_started); + assert.deepEqual(startedSave, { + id: `${savedDocs[4]._id}-info`, + doc_id: savedDocs[4]._id, + transitions_started: startedSave.transitions_started, + }); txnSave = infodocSaves.find(args => args[0].transitions)[0]; + assert.isUndefined(txnSave.transitions_started); + assert.sameMembers( + Object.keys(txnSave.transitions), + ['default_responses', 'update_sent_by', 'accept_patient_reports', 'conditional_alerts'] + ); assert.equal(txnSave.transitions.default_responses.ok, true); assert.equal(txnSave.transitions.update_sent_by.ok, true); assert.equal(txnSave.transitions.accept_patient_reports.ok, true); diff --git a/shared-libs/transitions/test/unit/finalize-transition.js b/shared-libs/transitions/test/unit/finalize-transition.js index 1d81f73598a..1eb3e1ec183 100644 --- a/shared-libs/transitions/test/unit/finalize-transition.js +++ b/shared-libs/transitions/test/unit/finalize-transition.js @@ -22,7 +22,7 @@ describe('finalize transition', () => { ); }); - it('save is called if transition results have changes (API branch manages rev)', done => { + it('save is called if transition results have changes (API branch marks transitions started)', done => { const doc = { _rev: '1' }; const saveDoc = sinon.stub(db.medic, 'put').resolves({ ok: true, rev: '2' }); const markTransitionsStarted = sinon.stub(infodoc, 'markTransitionsStarted').resolves(); @@ -32,7 +32,7 @@ describe('finalize transition', () => { { change: { id: 'abc', doc: doc }, results: [null, null, true], - manageInfoDocRev: true, + markStarted: true, }, (err, result) => { assert.equal(saveDoc.callCount, 1); @@ -59,7 +59,7 @@ describe('finalize transition', () => { { change: { id: 'abc', doc: doc }, results: [null, null, true], - manageInfoDocRev: true, + markStarted: true, }, (err, result) => { assert.deepEqual(err, { error: 'something' }); From 34e75b0658f58a32aeab5031a4c3032f7304ade2 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Thu, 11 Jun 2026 11:45:06 +0300 Subject: [PATCH 09/12] fix(#10875): sonar --- shared-libs/infodoc/src/infodoc.js | 4 ++-- shared-libs/transitions/src/transitions/index.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index d2284cdec35..87349b01671 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -153,7 +153,7 @@ const updateTransition = (change, transition, ok) => { const saveTransitions = (change, clearStarted = false) => { const modify = infoDoc => { - infoDoc.transitions = (change.info && change.info.transitions) || {}; + infoDoc.transitions = change.info?.transitions || {}; // Clear the in-progress marker in the same write that commits the transitions (API branch only) if (clearStarted) { delete infoDoc.transitions_started; @@ -164,7 +164,7 @@ const saveTransitions = (change, clearStarted = false) => { const saveCompletedTasks = (id, infodoc, completedTasks = []) => { return modifyInfoDoc(id, infoDoc => { - infoDoc.completed_tasks = (infodoc && infodoc.completed_tasks) || completedTasks; + infoDoc.completed_tasks = infodoc?.completed_tasks || completedTasks; }, infodoc); }; diff --git a/shared-libs/transitions/src/transitions/index.js b/shared-libs/transitions/src/transitions/index.js index b392919b2e7..869cb8eb5a1 100644 --- a/shared-libs/transitions/src/transitions/index.js +++ b/shared-libs/transitions/src/transitions/index.js @@ -48,7 +48,7 @@ let loadErrors = false; const MAX_INFODOC_WAIT = 5; const INFODOC_WAIT_INTERVAL = 100; -const isInfoDocMidWrite = infoDoc => infoDoc && infoDoc.transitions_started !== undefined; +const isInfoDocMidWrite = infoDoc => infoDoc?.transitions_started !== undefined; const getConsistentInfoDoc = async (change, retriesLeft) => { const infoDoc = await infodoc.get(change); From 98c767192a5279453f2fbf99d3c2b641e57ae706 Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Thu, 11 Jun 2026 13:52:01 +0300 Subject: [PATCH 10/12] fix(#10875): fix other race condition --- shared-libs/infodoc/src/infodoc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 87349b01671..55d468ea299 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -35,7 +35,7 @@ const resolveInfoDocs = (changes, writeDirtyInfoDocs) => { return results.reduce((acc, row) => { if (!row.doc) { acc.missing.push({ _id: row.key }); - } else if (!row.doc.transitions) { + } else if (!row.doc.transitions && !row.doc.transitions_started) { // No transitions may mean that API created this infodoc on write but sentinel hasn't seen // it yet. It's possible that there is a legacy infodoc with transition information. acc.missingTransitions.push(row.doc); From 395278b2ff8e1fced949bf8be1a24d3a163a0a7b Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Tue, 23 Jun 2026 14:04:18 +0300 Subject: [PATCH 11/12] fix(#10875): removing legacy infodoc migration to fix tests --- shared-libs/infodoc/src/infodoc.js | 107 ++++------------ shared-libs/infodoc/test/infodoc.js | 133 ++++---------------- tests/integration/infodocs/infodocs.spec.js | 45 ------- 3 files changed, 48 insertions(+), 237 deletions(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index 55d468ea299..b30c0815a77 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -19,8 +19,7 @@ const findInfoDocs = (database, ids) => { }; // -// Given a set of changes, find all the infoDocs or create them as necessary. Also takes care of -// migrating legacy infodocs from the medic db, and legacy transition information from records. +// Given a set of changes, find all the infoDocs or create them as necessary. // // @param {Array} changes an array of PouchDB changes objects, each containing at least {id, doc} // @return {Array} array of infodocs. NB: will not necessarily be in the same order as the @@ -36,8 +35,6 @@ const resolveInfoDocs = (changes, writeDirtyInfoDocs) => { if (!row.doc) { acc.missing.push({ _id: row.key }); } else if (!row.doc.transitions && !row.doc.transitions_started) { - // No transitions may mean that API created this infodoc on write but sentinel hasn't seen - // it yet. It's possible that there is a legacy infodoc with transition information. acc.missingTransitions.push(row.doc); } else { acc.valid.push(row.doc); @@ -48,95 +45,35 @@ const resolveInfoDocs = (changes, writeDirtyInfoDocs) => { }, { valid: [], missing: [], missingTransitions: [] }); }; + const changeForInfoDoc = infoDocId => changes.find(change => getInfoDocId(change.id) === infoDocId); + const infoDocIds = changes.map(change => getInfoDocId(change.id)); - // First attempt, directly from sentinel where they should live return findInfoDocs(db.sentinel, infoDocIds) .then(results => { - const { valid, missing, missingTransitions: missingTransitionsSentinel } = splitInfoDocRows(results); - - const lookForInMedic = missing.concat(missingTransitionsSentinel).map(r => r._id); - - if (!lookForInMedic.length) { - return valid; - } - - // the infodocs missing transitions are still valid, we just need to look for their transitions! - const infoDocs = valid.concat(missingTransitionsSentinel); - - // Missing infodocs or missing transitions may be either - return findInfoDocs(db.medic, lookForInMedic) - .then(results => { - const migratedInfoDocs = []; - const { valid, missing, missingTransitions: missingTransitionsMedic } = splitInfoDocRows(results); - - // Back when infodocs were in the medic db, transitions were still stored against the - // actual document. We'll deal with this below - valid.push(...missingTransitionsMedic); - - // Convert valid MedicDB infodocs into Sentinel ones - valid.forEach(medicInfoDoc => { - const sentinelInfoDoc = missingTransitionsSentinel.find(d => d._id === medicInfoDoc._id); - - const change = changes.find(change => change.id === medicInfoDoc.doc_id); - - if (sentinelInfoDoc) { - // Merge information from the medic infodoc into sentinel's - Object.keys(medicInfoDoc).forEach(k => { - if (sentinelInfoDoc[k] === undefined) { - sentinelInfoDoc[k] = medicInfoDoc[k]; - } - }); - - // Explicitly take the older (and so more correct) initial_replication_date. These would - // be different if a new write occurred on an old infodoc-unmigrated document, as api - // creates a new sentinel infodoc with an initial and latest replication date - sentinelInfoDoc.initial_replication_date = medicInfoDoc.initial_replication_date; - - // Source transitions from the document if they don't exist - sentinelInfoDoc.transitions = sentinelInfoDoc.transitions || (change.doc && change.doc.transitions); - - migratedInfoDocs.push(sentinelInfoDoc); - } else { - const infoDoc = Object.assign({}, medicInfoDoc); - delete infoDoc._rev; - infoDoc.transitions = change.doc && change.doc.transitions; - infoDocs.push(infoDoc); - migratedInfoDocs.push(infoDoc); - } - - medicInfoDoc._deleted = true; - }); - - // Intentionally not waiting on the promise for performance - if (valid.length) { - db.medic.bulkDocs(valid); - } + const { valid, missing, missingTransitions } = splitInfoDocRows(results); - // Infodocs that aren't in the Medic DB. This could mean there isn't one at all, or it - // could be that there was one without transition data back in sentinel - missing.forEach(missingDoc => { - const docId = getDocId(missingDoc._id); + const infoDocs = valid.concat(missingTransitions); + const dirtyInfoDocs = []; - const collectedInfoDoc = infoDocs.find(i => i._id === missingDoc._id); - const change = changes.find(change => change.id === docId); - const infoDoc = collectedInfoDoc || blankInfoDoc(docId, !change.doc._rev && Date.now()); - - infoDoc.transitions = change.doc && change.doc.transitions; - - if (!collectedInfoDoc) { - infoDocs.push(infoDoc); - } + missingTransitions.forEach(infoDoc => { + const change = changeForInfoDoc(infoDoc._id); + infoDoc.transitions = infoDoc.transitions || (change.doc && change.doc.transitions); + dirtyInfoDocs.push(infoDoc); + }); - migratedInfoDocs.push(infoDoc); - }); + missing.forEach(missingDoc => { + const change = changeForInfoDoc(missingDoc._id); + const infoDoc = blankInfoDoc(change.id, !change.doc._rev && Date.now()); + infoDoc.transitions = change.doc && change.doc.transitions; + infoDocs.push(infoDoc); + dirtyInfoDocs.push(infoDoc); + }); - // Store any infoDocs that have been migrated. - if (writeDirtyInfoDocs && migratedInfoDocs.length) { - return bulkUpdate(migratedInfoDocs); - } - return infoDocs; - }); + if (writeDirtyInfoDocs && dirtyInfoDocs.length) { + return bulkUpdate(dirtyInfoDocs).then(() => infoDocs); + } + return infoDocs; }); }; diff --git a/shared-libs/infodoc/test/infodoc.js b/shared-libs/infodoc/test/infodoc.js index c8d1aef51a0..1fdfc630bc1 100644 --- a/shared-libs/infodoc/test/infodoc.js +++ b/shared-libs/infodoc/test/infodoc.js @@ -103,7 +103,7 @@ describe('infodoc', () => { }); }); - it('should return infodocs when all are found in medic db', () => { + it('should fill transition info from the document if sentinel infodocs exist with no transitions', () => { const changes = [ { id: 'a', doc: { _id: 'a', _rev: '1-abc', transitions: { some: 'a data' }}}, { id: 'b', doc: { _id: 'b', _rev: '1-abc', transitions: { some: 'b data' }}}, @@ -115,64 +115,23 @@ describe('infodoc', () => { { _id: 'c-info', _rev: 'c-r', type: 'info', doc_id: 'c' } ]; + // db.medic is intentionally left unstubbed: it throws if touched, guarding against any + // regression that reintroduces a medic lookup. sinon.stub(db.sentinel, 'allDocs') - .resolves({ rows: infoDocs.map(doc => ({ key: doc._id, error: 'not_found' }))}); - sinon.stub(db.medic, 'allDocs') .resolves({ rows: infoDocs.map(doc => ({ key: doc._id, doc }))}); - sinon.stub(db.medic, 'bulkDocs') - .resolves(); - return lib.bulkGet(changes).then(result => { assert.deepEqual(result, [ - { _id: 'a-info', type: 'info', doc_id: 'a', transitions: {some: 'a data'} }, - { _id: 'b-info', type: 'info', doc_id: 'b', transitions: {some: 'b data'} }, - { _id: 'c-info', type: 'info', doc_id: 'c', transitions: {some: 'c data'} } + { _id: 'a-info', _rev: 'a-r', type: 'info', doc_id: 'a', transitions: {some: 'a data'} }, + { _id: 'b-info', _rev: 'b-r', type: 'info', doc_id: 'b', transitions: {some: 'b data'} }, + { _id: 'c-info', _rev: 'c-r', type: 'info', doc_id: 'c', transitions: {some: 'c data'} } ]); assert.equal(db.sentinel.allDocs.callCount, 1); assert.deepEqual(db.sentinel.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); - assert.equal(db.medic.allDocs.callCount, 1); - assert.deepEqual(db.medic.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); }); }); - it( - 'should try to fill transition info from the medic doc if sentinel infodocs exist with no transition info', - () => { - const changes = [ - { id: 'a', doc: { _id: 'a', _rev: '1-abc', transitions: { some: 'a data' }}}, - { id: 'b', doc: { _id: 'b', _rev: '1-abc', transitions: { some: 'b data' }}}, - { id: 'c', doc: { _id: 'c', _rev: '1-abc', transitions: { some: 'c data' }}} - ]; - const infoDocs = [ - { _id: 'a-info', _rev: 'a-r', type: 'info', doc_id: 'a' }, - { _id: 'b-info', _rev: 'b-r', type: 'info', doc_id: 'b' }, - { _id: 'c-info', _rev: 'c-r', type: 'info', doc_id: 'c' } - ]; - - sinon.stub(db.sentinel, 'allDocs') - .resolves({ rows: infoDocs.map(doc => ({ key: doc._id, doc }))}); - sinon.stub(db.medic, 'allDocs') - .resolves({ rows: infoDocs.map(doc => ({ key: doc._id, error: 'not_found' }))}); - sinon.stub(db.medic, 'bulkDocs') - .resolves(); - - return lib.bulkGet(changes).then(result => { - assert.deepEqual(result, [ - { _id: 'a-info', _rev: 'a-r', type: 'info', doc_id: 'a', transitions: {some: 'a data'} }, - { _id: 'b-info', _rev: 'b-r', type: 'info', doc_id: 'b', transitions: {some: 'b data'} }, - { _id: 'c-info', _rev: 'c-r', type: 'info', doc_id: 'c', transitions: {some: 'c data'} } - ]); - - assert.equal(db.sentinel.allDocs.callCount, 1); - assert.deepEqual(db.sentinel.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); - assert.equal(db.medic.allDocs.callCount, 1); - assert.deepEqual(db.medic.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); - }); - } - ); - it('should generate infodocs with unknown dates for existing documents, if they do not already exist', () => { const changes = [ { id: 'a', doc: {_id: 'a', _rev: '1-abc' }}, @@ -182,8 +141,6 @@ describe('infodoc', () => { sinon.stub(db.sentinel, 'allDocs') .resolves({ rows: changes.map(doc => ({ key: `${doc.id}-info`, error: 'not_found' }))}); - sinon.stub(db.medic, 'allDocs') - .resolves({ rows: changes.map(doc => ({ key: `${doc.id}-info`, error: 'not_found' }))}); return lib.bulkGet(changes).then(result => { assert.deepEqual(result, [ @@ -203,8 +160,6 @@ describe('infodoc', () => { assert.equal(db.sentinel.allDocs.callCount, 1); assert.deepEqual(db.sentinel.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); - assert.equal(db.medic.allDocs.callCount, 1); - assert.deepEqual(db.medic.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info'], include_docs: true }]); }); }); @@ -220,8 +175,6 @@ describe('infodoc', () => { sinon.stub(db.sentinel, 'allDocs') .resolves({ rows: changes.map(doc => ({ key: `${doc.id}-info`, error: 'not_found' }))}); - sinon.stub(db.medic, 'allDocs') - .resolves({ rows: changes.map(doc => ({ key: `${doc.id}-info`, error: 'not_found' }))}); const now = Date.now(); @@ -239,48 +192,20 @@ describe('infodoc', () => { }); }); - it('should merge medic infodoc into sentinel when sentinel has no transitions', () => { - const change = { - id: 'test', - doc: { _id: 'test', _rev: '1-abc', transitions: { from_doc: true } } - }; + it('creates and persists an infodoc via get when none exists', () => { + const change = { id: 'x', doc: { _id: 'x', _rev: '1-abc', transitions: { t: 1 } } }; - const sentinelInfoDoc = { - _id: 'test-info', - _rev: '1-s', - type: 'info', - doc_id: 'test', - initial_replication_date: 'new-date', - latest_replication_date: 'new-date' - }; - - const medicInfoDoc = { - _id: 'test-info', - _rev: '1-m', - type: 'info', - doc_id: 'test', - extra_field: 'from medic', - initial_replication_date: 'old-date', - latest_replication_date: 'old-date', - transitions: { old_transition: true } - }; - - sinon.stub(db.sentinel, 'allDocs').resolves({ - rows: [{ key: 'test-info', doc: sentinelInfoDoc }] - }); - sinon.stub(db.medic, 'allDocs').resolves({ - rows: [{ key: 'test-info', doc: medicInfoDoc }] - }); - sinon.stub(db.medic, 'bulkDocs').resolves(); - sinon.stub(db.sentinel, 'bulkDocs').resolves([{ ok: true, rev: '2-s' }]); + sinon.stub(db.sentinel, 'allDocs').resolves({ rows: [{ key: 'x-info', error: 'not_found' }] }); + const bulkDocs = sinon.stub(db.sentinel, 'bulkDocs').resolves([{ ok: true, id: 'x-info', rev: '1-x' }]); return lib.get(change).then(result => { - assert.equal(result.extra_field, 'from medic'); - assert.equal(result.initial_replication_date, 'old-date'); - assert.deepEqual(result.transitions, { old_transition: true }); - assert.isTrue(medicInfoDoc._deleted); - assert.equal(db.sentinel.bulkDocs.callCount, 1); - assert.equal(db.medic.bulkDocs.callCount, 1); + assert.deepEqual(result, { + _id: 'x-info', type: 'info', doc_id: 'x', + initial_replication_date: 'unknown', latest_replication_date: 'unknown', + transitions: { t: 1 }, _rev: '1-x' + }); + assert.equal(bulkDocs.callCount, 1); + assert.deepEqual(bulkDocs.args[0][0], [result]); }); }); @@ -303,21 +228,16 @@ describe('infodoc', () => { { key: 'e-info', error: 'deleted' }, { key: 'f-info', error: 'something' }, ]}); - sinon.stub(db.medic, 'allDocs') - .resolves({ rows: [ - { key: 'b-info', id: 'b-info', doc: { _id: 'b-info', _rev: 'b-r', doc_id: 'b' } }, - { key: 'c-info', error: 'some error' }, - { key: 'e-info', error: 'some error' }, - { key: 'f-info', id: 'f-info', doc: { _id: 'f-info', _rev: 'f-r', doc_id: 'f' } }, - ]}); - sinon.stub(db.medic, 'bulkDocs').resolves(); + // db.medic is left unstubbed: it throws if touched, guarding against a reintroduced lookup. return lib.bulkGet(changes).then(result => { assert.deepEqual(result, [ { _id: 'a-info', _rev: 'a-r', doc_id: 'a', transitions: {} }, { _id: 'd-info', _rev: 'd-r', doc_id: 'd', transitions: {} }, - { _id: 'b-info', doc_id: 'b', transitions: undefined }, - { _id: 'f-info', doc_id: 'f', transitions: undefined }, + { + _id: 'b-info', doc_id: 'b', initial_replication_date: 'unknown', + latest_replication_date: 'unknown', type: 'info', transitions: undefined + }, { _id: 'c-info', doc_id: 'c', initial_replication_date: 'unknown', latest_replication_date: 'unknown', type: 'info', transitions: undefined @@ -326,6 +246,10 @@ describe('infodoc', () => { _id: 'e-info', doc_id: 'e', initial_replication_date: 'unknown', latest_replication_date: 'unknown', type: 'info', transitions: undefined }, + { + _id: 'f-info', doc_id: 'f', initial_replication_date: 'unknown', + latest_replication_date: 'unknown', type: 'info', transitions: undefined + }, ]); assert.equal(db.sentinel.allDocs.callCount, 1); @@ -333,11 +257,6 @@ describe('infodoc', () => { db.sentinel.allDocs.args[0], [{ keys: ['a-info', 'b-info', 'c-info', 'd-info', 'e-info', 'f-info'], include_docs: true } ] ); - assert.equal(db.medic.allDocs.callCount, 1); - assert.deepEqual( - db.medic.allDocs.args[0], - [{ keys: ['b-info', 'c-info', 'e-info', 'f-info'], include_docs: true }] - ); }); }); }); diff --git a/tests/integration/infodocs/infodocs.spec.js b/tests/integration/infodocs/infodocs.spec.js index 7a7dc2e4309..636196a6c26 100644 --- a/tests/integration/infodocs/infodocs.spec.js +++ b/tests/integration/infodocs/infodocs.spec.js @@ -162,51 +162,6 @@ describe('infodocs', () => { assert.isOk(infodoc.latest_replication_date, 'expected a latest_replication_date'); assert.deepEqual(infodoc.transitions, { some: 'transition info' }); }); - - it('finds and migrates data from the medic infodoc', async () => { - // In legacy situations the transition info was on the doc, while other - // information was on the infodoc - const testDoc = { - data: 'data', - transitions: { - some: 'transition info' - } - }; - const legacyInfodoc = { - type: 'info', - some: 'legacy data', - initial_replication_date: 1000, - latest_replication_date: 2000 - }; - - await utils.toggleSentinelTransitions(); - const result = await utils.db.post(testDoc); - testDoc._rev = result.rev; - testDoc._id = result.id; - - legacyInfodoc._id = `${result.id}-info`; - legacyInfodoc.doc_id = result.id; - - await utils.db.put(legacyInfodoc); - await utils.setTransitionSeqToNow(); - await utils.toggleSentinelTransitions(); - - testDoc.data = 'data changed'; - await utils.saveDoc(testDoc); - - const [infodoc] = await delayedInfoDocsOf(testDoc._id); - - try { - await utils.db.get(legacyInfodoc._id); - assert.fail('doc should be deleted'); - } catch (err) { - assert.equal(err.status, 404); - } - assert.equal(infodoc.initial_replication_date, 1000); - assert.isOk(infodoc.latest_replication_date !== 2000); // updated - assert.deepEqual(infodoc.transitions, { some: 'transition info' }); - assert.equal(infodoc.some, 'legacy data'); - }); }); describe('transitions infos', () => { From aa6947fdc796ad0eeb41c6de5844e66b0df164fc Mon Sep 17 00:00:00 2001 From: Tom Wier Date: Wed, 24 Jun 2026 11:16:42 +0300 Subject: [PATCH 12/12] fix(#10875): Sonar --- shared-libs/infodoc/src/infodoc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared-libs/infodoc/src/infodoc.js b/shared-libs/infodoc/src/infodoc.js index b30c0815a77..c133de3cb7f 100644 --- a/shared-libs/infodoc/src/infodoc.js +++ b/shared-libs/infodoc/src/infodoc.js @@ -58,7 +58,7 @@ const resolveInfoDocs = (changes, writeDirtyInfoDocs) => { missingTransitions.forEach(infoDoc => { const change = changeForInfoDoc(infoDoc._id); - infoDoc.transitions = infoDoc.transitions || (change.doc && change.doc.transitions); + infoDoc.transitions = infoDoc.transitions || change.doc?.transitions; dirtyInfoDocs.push(infoDoc); });