Skip to content

Comments

Server: Fix unique constraint error when multiple createSharedFolderUserItems are run concurrently#13112

Merged
laurent22 merged 3 commits intolaurent22:devfrom
personalizedrefrigerator:pr/server/fix_unique_constraint_on_concurrent_update
Sep 8, 2025
Merged

Server: Fix unique constraint error when multiple createSharedFolderUserItems are run concurrently#13112
laurent22 merged 3 commits intolaurent22:devfrom
personalizedrefrigerator:pr/server/fix_unique_constraint_on_concurrent_update

Conversation

@personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Sep 2, 2025

Summary

This pull request ignores "unique constraint" thrown while adding (user_id, item_id) pairs to user_items while accepting a share. (See comment).

May partially resolve #12075.

Testing

This pull request includes an automated test that runs createSharedFolderUserItems multiple times concurrently (as could happen if the same share is accepted multiple times concurrently).

With this change, the sync fuzzer fails after 22 steps, with conflicts (currently expected, see #12993).

@personalizedrefrigerator personalizedrefrigerator added the server Issues related to Joplin Server label Sep 2, 2025
});
}
} catch (error) {
if (!options.ignoreAlreadyExists || !isUniqueConstraintError(error)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
if (!options.ignoreAlreadyExists || !isUniqueConstraintError(error)) {
if (!isUniqueConstraintError(error)) {

Some (almost all?) calls to UserItemModel.add/UserItemModel.addMulti are already wrapped in a try/catch that ignores unique constraint errors (example). It could make sense to remove the option and always ignore unique constraint errors here.

@personalizedrefrigerator
Copy link
Collaborator Author

Closing temporarily until the following in-CI test failure is resolved:

[@joplin/server]: FAIL dist/models/ShareModel.test.js
[@joplin/server]:   ● ShareModel › should be possible to run createSharedFolderUserItems multiple times concurrently
[@joplin/server]: 
[@joplin/server]:     error: insert into "user_items" ("created_time", "item_id", "updated_time", "user_id") values ($1, $2, $3, $4) - current transaction is aborted, commands ignored until end of transaction block
[@joplin/server]: 
[@joplin/server]:       at Parser.parseErrorMessage (node_modules/pg-protocol/src/parser.ts:369:69)
[@joplin/server]:       at Parser.handlePacket (node_modules/pg-protocol/src/parser.ts:187:21)
[@joplin/server]:       at Parser.parse (node_modules/pg-protocol/src/parser.ts:102:30)
[@joplin/server]:       at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)

@personalizedrefrigerator
Copy link
Collaborator Author

I'm re-running CI — the most recent failing test seems unrelated to this pull request:

 [@joplin/transcribe]:   ● FileStorage › should remove files that are older than the given date
[@joplin/transcribe]: 
[@joplin/transcribe]:     expect(received).toBe(expected) // Object.is equality
[@joplin/transcribe]: 
[@joplin/transcribe]:     Expected: 2
[@joplin/transcribe]:     Received: 3
[@joplin/transcribe]: 
[@joplin/transcribe]:       52 | 		await fs.removeOldFiles(new Date('2025-03-03 12:00'));
[@joplin/transcribe]:       53 | 		const files = await readdir('images');
[@joplin/transcribe]:     > 54 | 		expect(files.length).toBe(2);
[@joplin/transcribe]:          | 		                     ^
[@joplin/transcribe]:       55 | 		expect(files.includes(mockedFilenames[2])).toBe(true);
[@joplin/transcribe]:       56 |
[@joplin/transcribe]:       57 | 		for (const file of mockedFiles) {
[@joplin/transcribe]: 
[@joplin/transcribe]:       at Object.toBe (src/services/FileStorage.test.ts:54:24)

item_name: item.name,
type: ChangeType.Create,
previous_item: '',
await this.withTransaction(async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This withTransaction has been moved within the for loop to avoid the following error observed while running tests in CI: current transaction is aborted, commands ignored until end of transaction block.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review September 3, 2025 21:30
@laurent22 laurent22 merged commit ec0f9ef into laurent22:dev Sep 8, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Issues related to Joplin Server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notebook disappeared after failed sharing attempt

2 participants