Skip to content

[feature] Enable cleanup in CloudflareR2 post-process#164

Merged
Ghass-M merged 8 commits intofeature/data-nodes-and-local-syncfrom
feature/data-nodes-and-local-sync-selective-deletion
Aug 12, 2025
Merged

[feature] Enable cleanup in CloudflareR2 post-process#164
Ghass-M merged 8 commits intofeature/data-nodes-and-local-syncfrom
feature/data-nodes-and-local-sync-selective-deletion

Conversation

@Ghass-M
Copy link
Contributor

@Ghass-M Ghass-M commented Aug 8, 2025

🗂️ Fix CloudflareR2 file deletion in transfer completion

📋 Summary

This PR addresses the issue where files on CloudflareR2 were not being deleted at the end of a transfer because ByteSync was only attempting to delete from AzureBlobStorage. The implementation now properly handles file deletion for both storage providers based on the StorageProvider parameter.

🎯 Problem

  • Files on CloudflareR2 were not being deleted after transfer completion
  • ByteSync was hardcoded to only delete from AzureBlobStorage
  • No mechanism to determine which storage provider to use for deletion

✅ Solution

1. TransferParameters StorageProvider Support

  • TransferParameters class already had StorageProvider property
  • This allows passing storage provider information through the entire transfer chain

2. SharedFilesService.AssertFilePartIsDownloaded Enhancement

  • Method now accepts StorageProvider storageProvider parameter
  • Implements proper storage provider selection for deletion:
    await (storageProvider switch
    {
        StorageProvider.AzureBlobStorage => _blobUrlService.DeleteBlob(sharedFileDefinition, partNumber),
        StorageProvider.CloudflareR2     => _cloudflareR2UrlService.DeleteObject(sharedFileDefinition, partNumber),
        _ => throw new NotSupportedException($"Storage provider {storageProvider} is not supported")
    });

3. SharedFileData StorageProvider Storage

  • SharedFileData already stores StorageProvider property
  • Used by ClearSession to select the correct service for cleanup

4. ClearSession Enhancement

  • ClearSession method now uses sharedFileData.StorageProvider to determine deletion service
  • Properly handles both AzureBlobStorage and CloudflareR2 cleanup

5. Command Handler Integration

  • AssertFilePartIsDownloadedCommandHandler extracts StorageProvider from TransferParameters
  • Passes storage provider information to SharedFilesService.AssertFilePartIsDownloaded

🔄 Flow

  1. Client creates TransferParameters with StorageProvider
  2. Command Handler extracts StorageProvider from request
  3. SharedFilesService receives StorageProvider parameter
  4. Service uses storage provider to select correct deletion service:
    • AzureBlobStorage → _blobUrlService.DeleteBlob()
    • CloudflareR2 → _cloudflareR2UrlService.DeleteObject()
  5. ClearSession uses stored StorageProvider from SharedFileData for cleanup

🎉 Benefits

  • CloudflareR2 files are now properly deleted after transfer completion
  • Backward compatibility maintained for AzureBlobStorage
  • Extensible design for future storage providers
  • Proper error handling with meaningful exceptions
  • Comprehensive test coverage for all scenarios

Deployment Notes

  • No breaking changes to existing APIs
  • No configuration changes required
  • Existing AzureBlobStorage functionality remains unchanged
  • CloudflareR2 users will now see proper file cleanup

@Ghass-M Ghass-M requested a review from paul-fresquet August 8, 2025 16:59
@paul-fresquet paul-fresquet requested a review from Copilot August 10, 2025 16:46
Copy link
Contributor

Copilot AI left a 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 enables proper file cleanup for CloudflareR2 storage provider by implementing storage provider-aware deletion logic throughout the file transfer system. Previously, files were only being deleted from AzureBlobStorage regardless of where they were actually stored.

  • Modified SharedFilesService to accept storage provider parameter and route deletion calls to appropriate service
  • Updated TransferParameters and SharedFileData to track storage provider information
  • Enhanced command handlers and client code to pass storage provider context through the deletion chain

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
SharedFilesService.cs Implements storage provider-aware deletion logic using switch expressions for both services
ISharedFilesService.cs Adds storage provider parameter to AssertFilePartIsDownloaded method signature
AssertFilePartIsDownloadedCommandHandler.cs Passes storage provider from transfer parameters to service call
SharedFileData.cs Adds StorageProvider property to track which storage service was used
TransferParameters.cs Adds StorageProvider property to carry context through transfer operations
FileDownloader.cs Extracts storage provider from download response and passes to assertion method
AssertFilePartIsDownloadedCommandHandlerTests.cs Updates tests to cover both storage providers with parameterized test cases

Copy link
Contributor

@paul-fresquet paul-fresquet left a comment

Choose a reason for hiding this comment

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

Nice work :) Here are few comments

…c-selective-deletion' into feature/data-nodes-and-local-sync-selective-deletion
@sonarqubecloud
Copy link

@Ghass-M Ghass-M merged commit 3c60561 into feature/data-nodes-and-local-sync Aug 12, 2025
20 checks passed
@Ghass-M Ghass-M deleted the feature/data-nodes-and-local-sync-selective-deletion branch August 12, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants