🐛 Concurrent deletion of files at root level causes deadlock #8621
Conversation
giancarloromeo
changed the title
test_search_directoriestest_search_directories
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #8621 +/- ##
===========================================
- Coverage 87.51% 67.24% -20.27%
===========================================
Files 2010 836 -1174
Lines 78979 38032 -40947
Branches 1377 154 -1223
===========================================
- Hits 69118 25576 -43542
- Misses 9459 12407 +2948
+ Partials 402 49 -353
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
giancarloromeo
requested review from
GitHK,
bisgaard-itis,
matusdrobuliak66 and
pcrespov
…:giancarloromeo/osparc-simcore into is8619/fix-flaky-test-search-directories
sanderegg
left a comment
sanderegg
left a comment
There was a problem hiding this comment.
Not sure I get what was "fixed" here.
create_directory_with_files is used in many other tests.
What I see is that in case we run that test and interrupt it, the cleanup_files_closure might not be called --> which will leak files.
I find not very helpful to have 2 fixtures that do more or less the same, in the same package and nobody knows when to use one or the other. What was the issue?
giancarloromeo
added
the
🤖-do-not-merge
A real bug in |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a flaky test in test_search_directories by improving test reliability and adding utility functions for file ID level detection. The changes introduce helper functions to determine file nesting levels and modify the deletion logic to skip parent directory updates for root-level files.
Key Changes:
- Added utility functions (
get_file_id_level,is_nested_level_file_id) to detect file nesting depth - Modified
delete_fileto conditionally update parent directories based on file nesting level - Enhanced test coverage with additional subdirectory test case and comprehensive parameterized tests
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
services/storage/src/simcore_service_storage/utils/simcore_s3_dsm_utils.py |
Introduces utility functions for calculating file ID levels and detecting nested files, plus updates to use S3_OBJECT_DELIMITER constant |
services/storage/src/simcore_service_storage/simcore_s3_dsm.py |
Modifies delete_file logic to skip parent directory updates for root-level files using the new is_nested_level_file_id check |
services/storage/tests/unit/test_simcore_s3_dsm.py |
Adds nested subdirectory test case to verify search functionality for deeper directory structures |
services/storage/tests/unit/test_simcore_s3_dsm_utils.py |
Adds comprehensive parameterized tests for the new utility functions covering edge cases and various path formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
giancarloromeo
changed the title
test_search_directories
|
giancarloromeo
removed
the
🤖-do-not-merge
services/storage/src/simcore_service_storage/utils/simcore_s3_dsm_utils.py
Show resolved
Hide resolved
|
@mergify queue |
🟠 Waiting for conditions to match
|
6 participants
What do these changes do?
This PR fixes a flaky test in
test_search_directoriesby improving test reliability and adding utility functions for file ID level detection. The changes introduce helper functions to determine file nesting levels and modify the deletion logic to skip parent directory updates for root-level files.Key Changes:
get_file_id_level,is_nested_level_file_id) to detect file nesting depthdelete_fileto conditionally update parent directories based on file nesting levelRelated issue/s
tests/unit/test_simcore_s3_dsm.py::test_search_directories#8619How to test
Dev-ops