[mob][photos]Fix: Clear multiPart entry when file is missing

[ua741]

Description

Tests

[claude]

Code Review

Summary

This PR fixes a bug in the file upload error handling logic. The change ensures that multipart upload tracking data is properly cleaned up when a file is missing, even if the upload hasn't been initiated yet in the current session.

Analysis

The Bug
The original condition only deleted multipart tracking data when isMultipartUpload was true:

if (isMultipartUpload && isPutOrMultiPartError(e)) {
  await UploadLocksDB.instance.deleteMultipartTrack(lockKey);
}

However, there's a scenario where:

1. A previous upload session created a multipart entry (hasExistingMultiPart = true)
2. The encrypted file was deleted/missing (lines 621-624 throw MultiPartMissingFileError)
3. The exception is caught before isMultipartUpload is set to true (which happens at line 694)
4. The multipart tracking entry remains in the database, preventing proper cleanup

The Fix
The updated condition correctly handles this case:

if ((isMultipartUpload || hasExistingMultiPart) && isPutOrMultiPartError(e)) {
  await UploadLocksDB.instance.deleteMultipartTrack(lockKey);
}

Feedback

✅ Code Quality

• The fix is minimal and focused on the specific issue
• The logic is sound and follows the existing code patterns
• Variable naming is clear (hasExistingMultiPart vs isMultipartUpload)

✅ Correctness

• The fix correctly handles the edge case where MultiPartMissingFileError is thrown at line 622
• Since isPutOrMultiPartError() returns true for MultiPartMissingFileError (line 905), the cleanup will now execute properly

✅ No Security Concerns

• This is a cleanup operation that removes stale tracking data
• No security implications identified

⚠️ Suggestions

1. Test Coverage: The PR description has an empty "Tests" section. Consider adding:

   • A test case that simulates the scenario: existing multipart entry + missing encrypted file
   • Verify that deleteMultipartTrack is called when MultiPartMissingFileError is thrown

2. Code Comment: Consider adding a brief comment explaining why both conditions are needed:

   // Clean up multipart tracking for both active uploads and failed resume attempts
   if ((isMultipartUpload || hasExistingMultiPart) && isPutOrMultiPartError(e)) {
     await UploadLocksDB.instance.deleteMultipartTrack(lockKey);
   }

3. PR Description: The description could be more detailed about:

   • The specific scenario that triggers the bug
   • Steps to reproduce (if applicable)
   • How the fix was tested

Verdict

The fix is correct and addresses a legitimate bug in the cleanup logic. The change is low-risk as it only affects error handling paths. Approved pending test coverage confirmation.

---

Review generated by Claude Code

[claude]

Code Review Summary: This PR fixes a bug in multipart upload error handling where the multipart tracking entry was not being cleaned up when a file was missing. The fix includes renaming the exception class for clarity and ensuring cleanup happens for existing multipart entries.

Positive Observations:

• Good bug fix logic: The key change on line 857-858 correctly handles the case where hasExistingMultiPart is true but the file is missing - this entry should be cleaned up.
• Improved naming: Renaming MultiPartMissingFileError to MultiPartFileMissingError is more grammatically correct and clearer.
• Consistent refactoring: All references to the renamed exception are updated throughout the codebase.

The Core Fix: When a MultiPartFileMissingError is thrown (line 622-624), it indicates that we have a stale multipart entry without the actual encrypted file. The old code only checked isMultipartUpload for cleanup, which would be false in this scenario since the upload has not started yet. By adding the hasExistingMultiPart condition, stale entries are now properly cleaned up.

Suggestions:

1. Test coverage: Consider adding a test case that verifies the deleteMultipartTrack is called when MultiPartFileMissingError is thrown with an existing multipart entry but missing file.
2. Consider logging: The error at line 622-624 throws an exception but does not log anything before throwing. Consider adding a log statement similar to line 636 to help with debugging.
3. PR description: The PR description is missing the Description and Tests sections. Adding details about the bug scenario and how it was tested would be helpful.

Verdict: LGTM - This is a solid bug fix that addresses a legitimate edge case in multipart upload cleanup logic. The changes are minimal, focused, and correct.

[claude]

PR Review: Fix - Clear multiPart entry when file is missing

Summary

This PR addresses a bug in the multipart upload error handling flow by ensuring that multipart tracking data is properly cleaned up when a file is missing. The changes also include a class rename for better clarity.

✅ Positive Observations

1. Good Renaming: The rename from MultiPartMissingFileError to MultiPartFileMissingError improves readability by following a more natural naming pattern (adjective before noun).

2. Bug Fix Logic: The fix at line 857-859 in file_uploader.dart correctly extends the cleanup logic to handle the case where hasExistingMultiPart is true. This addresses the scenario where a multipart upload was previously tracked but the encrypted file is missing.

🔍 Code Quality & Best Practices

Overall Rating: Good ✓

• Clean refactor with consistent renaming across all occurrences
• Logical improvement to error handling flow
• Version bump follows semantic versioning

🐛 Potential Issues

Minor concerns:

1. Error Handling Completeness (file_uploader.dart:857-859): The condition now includes hasExistingMultiPart, which is correct. However, consider that if hasExistingMultiPart is true and we throw MultiPartFileMissingError, we'll always enter this cleanup block. This seems intentional and correct based on the fix description.

2. Missing Description in PR: The PR description sections for "Description" and "Tests" are empty. It would be helpful to document:

   • What scenario triggered this bug
   • Steps to reproduce
   • How the fix was verified

⚡ Performance Considerations

No performance concerns. The changes are minimal and only affect error handling paths.

🔒 Security Considerations

No security issues identified. The fix properly cleans up orphaned multipart tracking data, which is good for resource management.

📝 Suggestions

1. Documentation: Consider adding a code comment at file_uploader.dart:857-859 explaining why both isMultipartUpload and hasExistingMultiPart need to be checked:

   // Clean up multipart tracking for both active and resumed uploads
   // when encountering upload errors
   if ((isMultipartUpload || hasExistingMultiPart) &&
       isPutOrMultiPartError(e)) {

2. Test Coverage: If not already covered, consider adding a test case for the scenario where:

   • A multipart upload tracking entry exists (hasExistingMultiPart == true)
   • The encrypted file is missing (triggers MultiPartFileMissingError)
   • Verify that deleteMultipartTrack is called

✨ Conclusion

This is a solid bug fix that improves the robustness of the multipart upload error handling. The rename also improves code clarity. The changes are minimal, focused, and appear correct based on the logic flow.

Recommendation: Approve with minor suggestion to add documentation about the fix in the PR description.