-
Notifications
You must be signed in to change notification settings - Fork 0
Pilot 9050: fixup the preupload timeout causing resume issue #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a resumable upload issue caused by network timeouts during pre-upload operations. When timeouts occur, the local manifest can become mismatched with the backend state, treating already-registered items as unregistered.
Changes:
- Updated duplication check API from v1 to v2 endpoint to separately identify ACTIVE vs REGISTERED items
- Fixed manifest key inconsistency (changed from
local_pathtoobject_pathfor unregistered items) - Added logic to reconcile registered items found on backend with local manifest during resume
- Updated Python version support from 3.10-3.11 to 3.10-3.13
- Consolidated imports and updated copyright years to 2026
Reviewed changes
Copilot reviewed 100 out of 101 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Version bump to 3.21.0 and Python version range update |
| poetry.lock | Dependency updates for new Poetry version and Python support |
| app/services/file_manager/file_upload/upload_client.py | API v2 migration, three-way return for duplication check |
| app/services/file_manager/file_upload/file_upload.py | Resume logic to handle registered items mismatch |
| app/services/file_manager/file_upload/exception.py | Added proper exception message to super().init |
| tests/app/services/file_manager/file_upload/test_upload_client.py | Updated tests for new three-way return signature |
| tests/app/services/file_manager/file_upload/test_file_upload.py | Added comprehensive tests for manifest batch processing |
| Multiple files | Copyright year updates and import consolidation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return: | ||
| - non_exist_file_objects(List[FileObject]): the file that need to be uploaded. | ||
| - exist_files(List[str]): the file that has been uploaded. will be skipped | ||
| - [updated] registered_file_objects(List[Dict[str, Any]]): this is to handle the conner case |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "conner case" should be "corner case".
| - non_duplicate_file_objects(List[FileObject]): the list of file object that is not duplicated | ||
| - unregistered_items(List[FileObject]): the list of file object that is not duplicated | ||
| - [updated] registered_items(List[FileObject]): the list of file object that is already registered. | ||
| in conner case if preupload interrupted at specific batch, the local manifest |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "conner case" should be "corner case".
| unregistered_items = item_duplication_check(False, unregistered_items, upload_client) | ||
|
|
||
| # [updated] here duplication check api got update will filter out ACTIVE and REGISTERED items separately | ||
| # the reason is during the normal upload , there is a conner case that preupload got interrupted at |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "conner case" should be "corner case".
| registered_item = FileObject('object/registered', 'local_path', 'resumable_id', 'job_id', 'item_id') | ||
|
|
||
| url = AppConfig.Connections.url_base + '/portal/v1/files/exists' | ||
| url = AppConfig.Connections.url_bff + '/v2/items/batch/exists' | ||
| httpx_mock.add_response( | ||
| method='POST', | ||
| url=url, | ||
| json={'result': [dup_obj.object_path.upper() if case_insensitive else dup_obj.object_path]}, | ||
| json={ | ||
| 'result': [ | ||
| {'parent_path': 'object', 'name': 'duplicate', 'status': ItemStatus.ACTIVE}, | ||
| {'parent_path': 'object', 'name': 'registered', 'status': ItemStatus.REGISTERED}, | ||
| ] | ||
| }, | ||
| ) | ||
|
|
||
| not_dup_list, dup_list = upload_client.check_upload_duplication([dup_obj, not_dup_object]) | ||
| assert not_dup_list == [not_dup_object] | ||
| assert dup_list == [dup_obj.object_path.upper() if case_insensitive else dup_obj.object_path] | ||
| not_dup_list, dup_list, registered_list = upload_client.check_upload_duplication([dup_obj, not_dup_object]) | ||
|
|
||
| assert len(not_dup_list) == 1 | ||
| assert not_dup_list[0] == not_dup_object | ||
|
|
||
| assert len(dup_list) == 1 | ||
| assert dup_list[0] == dup_obj.object_path | ||
|
|
||
| assert len(registered_list) == 1 | ||
| assert registered_item.object_path == registered_list[0].get('parent_path', '') + '/' + registered_list[0].get( | ||
| 'name', '' | ||
| ) |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test creates a registered_item FileObject on line 316, but it is never passed to check_upload_duplication on line 330. Only dup_obj and not_dup_object are passed. The assertion on lines 339-341 that checks if registered_item.object_path matches the returned registered item appears to be checking the wrong thing - it's comparing a local variable that was never used in the function call to the API response. This test should either pass all three objects to the function or not create the unused registered_item variable.
| return_list = {} | ||
| for _, item in object_path_file_object_map.items(): | ||
| return_list.update({item.object_path: item}) | ||
|
|
||
| return list(object_path_file_object_map.values()), exist_files | ||
| return list(object_path_file_object_map.values()), active_path, registered_items |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code creates a return_list dictionary on lines 205-207, but then returns list(object_path_file_object_map.values()) on line 209 instead of using return_list. The return_list variable appears to be unused dead code. Either use list(return_list.values()) in the return statement or remove the return_list construction entirely.
Summary
After investigation of this issue, it is caused by network timeout during the pre-upload in python version. The resumable json will treat the timeout batch AS unregistered, while backend already registered them(This issue only happened when timeout). Then mismatching happens and block resumable upload. Another PR will increase the timeout in bff-cli service. This PR is meant to fix the mismatching issue:
a. old cli version will still use v1 endpoint.
api updates:
JIRA Issues
Pilot 9050
Type of Change
Please delete options that are not relevant.
Testing
Are there any new or updated tests to validate the changes?
Test Directions
unittest:
manual testing:
manifest.jsonto create mismatching, moving registered items into unregistered fields.Versions