Fix/ensure folder race condition#1770
Conversation
Introduce `AddFolderHandleRaceAsync` to handle race conditions during folder creation by re-fetching the folder if it already exists. Replace direct calls to `AddFolderAsync` with `AddFolderHandleRaceAsync`. Add helper method `ErrorIndicatesFolderAlreadyExists` to identify specific error codes for existing folders.
Added `EnsureListFolderIdempotentTest` and `EnsureListFolderConcurrentTest` to validate folder creation behavior and race condition handling. Improved cleanup of mock folders from the recycle bin in test cases. Refactored `QueryableExtensions.FirstOrDefaultAsync` calls for readability and consistency. Enhanced test assertions for folder properties and adjusted formatting.
…e-folder-race-condition
…e-folder-race-condition
Adam-it
left a comment
Adam-it
left a comment
There was a problem hiding this comment.
@aramB Awesome work so far. I had a quick check on the changes done in the Folder.cs file and IMO they are ok. The only problem left is MockData which were created for all folder tests.
In your case you should uncomment the TestCommon.Instance.Mocking = false; only in the tests you created and modifed and run tests then. With this the mock data would be generated only for those tests.
Please check this step in the guide which explains it
https://pnp.github.io/pnpcore/contributing/writing%20tests.html#run-a-single-test-live
may I kindly ask you to remove the mocks and recreate the mock data only for the tests you modified/created.
After that I think we may proceed and merge 👍
Let me know if you need any help and sorry for the late review.
|
@Adam-it, thank you very much for the review! I realized I misunderstood this instruction: "Once your test is ready, delete the generated mock data files (see the .response files in the MockData folder) and run your test once more to regenerate them" (from the docs). I initially thought it meant I should delete all .response files 🤦🏼♂️ No worries 👍 I’ve updated the PR, reverted the unintended changes, and the new .response files are now only added for the two new tests I introduced. |
Boy that was fast! You Rock 🤩 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1770 +/- ##
==========================================
- Coverage 82.42% 81.24% -1.19%
==========================================
Files 416 637 +221
Lines 28590 45390 +16800
Branches 0 4779 +4779
==========================================
+ Hits 23565 36876 +13311
- Misses 5025 7104 +2079
- Partials 0 1410 +1410 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency race in SharePoint folder creation by making EnsureFolderAsync resilient when concurrent callers attempt to create the same folder and one loses the create race (“already exists” error), and adds mock-backed tests to validate sequential idempotency and concurrent behavior.
Changes:
- Introduced a race-safe helper around
AddFolderAsyncto handle “already exists” by re-fetching the folder. - Added a SharePoint error discriminator for “folder already exists” (server error code
-2130575257). - Added tests (plus mock responses) for sequential idempotency and concurrent
EnsureFolderAsynccalls.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sdk/PnP.Core/Model/SharePoint/Core/Internal/Folder.cs | Routes folder creation through a race-safe helper and adds “already exists” error detection. |
| src/sdk/PnP.Core.Test/SharePoint/FoldersTests.cs | Adds sequential idempotency and concurrent ensure-folder tests. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00000.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00001.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00002.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00003.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00004.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00005.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00006.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00007.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00008.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderIdempotentTest-0-00009.response.json | Mock response for EnsureListFolderIdempotentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00000.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00001.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00002.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00003.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00004.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00005.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00006.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00007.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00008.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00009.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00010.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00011.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00012.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00013.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00014.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00015.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00016.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00017.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00018.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00019.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00020.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00021.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00022.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00023.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00024.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00025.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00026.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00027.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00028.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00029.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00030.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00031.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00032.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00033.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00034.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00035.response.json | Mock response for EnsureListFolderConcurrentTest. |
| src/sdk/PnP.Core.Test/SharePoint/MockData/FoldersTests/EnsureListFolderConcurrentTest-0-00036.response.json | Mock response for EnsureListFolderConcurrentTest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adam-it
left a comment
Adam-it
left a comment
There was a problem hiding this comment.
@aramB I performed a small fixup and it seems to be perfect.
But I am having some trouble running the EnsureListFolderConcurrentTest and it always seems to fail on my end
Could we recheck this part before the merge and ensure it is working
Fix race condition in
EnsureFolderAsyncunder concurrent accessWhen multiple concurrent calls to
EnsureFolderAsynctarget the same folder path, both can receive a 404 fromGetFolderByServerRelativeUrlAsyncand then both attemptAddFolderAsync. The second call fails with HTTP 400 "A file or folder with the name already exists."Changes
AddFolderHandleRaceAsynchelper that catches the "already exists" error (server error code-2130575257) and re-fetches the folder instead of propagating the exceptionErrorIndicatesFolderAlreadyExistshelper (mirrors existingErrorIndicatesFolderDoesNotExistspattern)EnsureFolderAsyncnow route through the race-safe helperTests
EnsureListFolderIdempotentTest— sequential idempotencyEnsureListFolderConcurrentTest— parallel calls to the same pathFixes #1765