Skip to content

Conversation

mrjo118
Copy link
Contributor

@mrjo118 mrjo118 commented Oct 16, 2025

This attempts to fix the intermittent test failure as per the comment here 5303583#r168069206

I think it happens because in the Note.save function it invokes the async function ItemChange.add, but it does not await it. As this test and some of the other tests I added create the 'old note' revision in addition to the current content revision, if the item_changes record is selected before the new item_changes record has been inserted in a certain scenario, then there will be 1 less revision created at collection due to a race condition.

This PR should prevent the given race condition in affected tests, by explicitly waiting for the ItemChange.add call to be completed before allowing the item_changes record to be selected at the point where it is crucial to be available. It also resets the 'revisionService.oldNoteInterval' setting value on test initialisation, to ensure there would not be possible race conditions relating to that, due to leakage across tests.

Full details of the race condition:
The first save after a revision collection will set the beforeChangeItemJson for subsequent saves to use. When the next collection is intended to create an 'old note' revision in the test, we need to wait for ItemChange.add to complete before subsequent saves, to avoid a race condition whereby the next save selects the beforeChangeItemJson before it has been saved and then uses a null value for subsequent saves

mrjo118 referenced this pull request Oct 16, 2025
…d for a note does not contain the original contents (#12674)

Co-authored-by: Laurent Cozic <laurent22@users.noreply.github.com>
@laurent22
Copy link
Owner

it invokes the async function ItemChange.add, but it does not await it.

To address this we have ItemChange.waitForAllSaved(). Could that be used in your test to fix the issue?

@mrjo118
Copy link
Contributor Author

mrjo118 commented Oct 16, 2025

it invokes the async function ItemChange.add, but it does not await it.

To address this we have ItemChange.waitForAllSaved(). Could that be used in your test to fix the issue?

Hmm, it looks like collectRevisions() already invokes ItemChange.waitForAllSaved(), so it's making doubt the item changes is causing the issue. How many times have you seen this failure happen in the CI?

@laurent22
Copy link
Owner

How many times have you seen this failure happen in the CI?

I don't check every time, so one time so far

@mrjo118
Copy link
Contributor Author

mrjo118 commented Oct 16, 2025

Ok, I've figured out now where there could still be a race condition. I've pushed up the new change with included comments.

FYI this race condition shouldn't happen in real usage of Joplin, as the note editor has debounce logic to prevent saves within 100 ms of each other

await ItemChange.waitForAllSaved();
await Note.save({ id: note.id, title: 'test', body: 'StartAB' });
await Note.save({ id: note.id, title: 'test', body: 'StartABC' }); // REV 2
await revisionService().collectRevisions(); // Create revisions for old and new content
Copy link
Owner

Choose a reason for hiding this comment

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

Since there's a call to collect revision here, shouldn't there be another ItemChange.waitForAllSaved() call before it?

Copy link
Contributor Author

@mrjo118 mrjo118 Oct 18, 2025

Choose a reason for hiding this comment

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

No it's not needed, because collectRevisions already invokes ItemChange.waitForAllSaved() in one of the first steps. It's only needed after the first save where we intend to collect an 'old note' revision on the next collection, because Note.save does not await the item changes to be saved, and the subsequent save will invoke ItemChange.oldNoteContent to get the beforeChangeItemJson to use

Comment on lines 623 to 624
// The first save after a revision collection will set the beforeChangeItemJson for subsequent saves to use. Wait for ItemChange.add to complete before subsequent
// saves, to avoid a race condition whereby the next save selects the beforeChangeItemJson before it has been saved and then uses a null value for subsequent saves
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding the comments but I think that is more information than needed since we already document what ItemChange.waitForAllSaved() is for, so please remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants