-
Notifications
You must be signed in to change notification settings - Fork 10
feat(msp): rejected storage request triggers file clean up if needed #547
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: main
Are you sure you want to change the base?
Changes from all commits
e786352
6450319
702adc0
fc036a3
93fc9b6
75af5b2
718be9b
b088fd7
6f8ffdc
43f6e67
fcefcb3
bca1ac7
40e0c93
0c42cbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| use anyhow::anyhow; | ||
| use sc_tracing::tracing::*; | ||
| use shc_actors_framework::event_bus::EventHandler; | ||
| use shc_blockchain_service::events::FinalisedBucketMutationsApplied; | ||
| use shc_blockchain_service::events::{ | ||
| FinalisedBucketMutationsApplied, FinalisedStorageRequestRejected, | ||
| }; | ||
| use shc_common::{ | ||
| traits::StorageEnableRuntime, | ||
| types::{FileKey, TrieMutation, TrieRemoveMutation}, | ||
|
|
@@ -140,3 +142,78 @@ where | |
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// Handles the [`FinalisedStorageRequestRejected`] event. | ||
| impl<NT, Runtime> EventHandler<FinalisedStorageRequestRejected<Runtime>> | ||
| for MspDeleteFileTask<NT, Runtime> | ||
| where | ||
| NT: ShNodeType<Runtime> + 'static, | ||
| NT::FSH: MspForestStorageHandlerT<Runtime>, | ||
| Runtime: StorageEnableRuntime, | ||
| { | ||
| async fn handle_event( | ||
| &mut self, | ||
| event: FinalisedStorageRequestRejected<Runtime>, | ||
| ) -> anyhow::Result<()> { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "Processing finalised storage request expired for file key {:?} in bucket {:?}", | ||
| event.file_key, | ||
| event.bucket_id | ||
| ); | ||
|
|
||
| // Ensure the file key is not present in the bucket's Forest. | ||
| let bucket_forest_key = event.bucket_id.as_ref().to_vec(); | ||
| let read_fs = self | ||
| .storage_hub_handler | ||
| .forest_storage_handler | ||
| .get(&bucket_forest_key.into()) | ||
| .await | ||
| .ok_or_else(|| { | ||
| anyhow!( | ||
| "CRITICAL❗️❗️ Failed to get forest storage for bucket [{:?}].", | ||
| event.bucket_id | ||
| ) | ||
| })?; | ||
|
Comment on lines
+166
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Buckets can be removed from MSPs in unfinalized blocks. I don't think this needs to be a critical error. |
||
|
|
||
| if read_fs | ||
| .read() | ||
| .await | ||
| .contains_file_key(&event.file_key.into())? | ||
| { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "StorageRequestExpired and finalised for file key {:?} in bucket {:?}, but file key is still in Forest.", | ||
| event.file_key, | ||
| event.bucket_id | ||
| ); | ||
| return Err(anyhow!("File key is still in Forest")); | ||
|
Comment on lines
+184
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we consider this case a warning and also return an error? |
||
| } | ||
|
|
||
| // Check that the file is present in the File Storage. | ||
| let is_in_file_storage = { | ||
| let read_file_storage = self.storage_hub_handler.file_storage.read().await; | ||
| read_file_storage | ||
| .get_metadata(&event.file_key.into()) | ||
| .map_err(|e| { | ||
| error!(target: LOG_TARGET, "Failed to get file metadata from File Storage: {:?}", e); | ||
| anyhow!("Failed to get file metadata from File Storage: {:?}", e) | ||
| })? | ||
| .is_some() | ||
| }; | ||
|
|
||
| if is_in_file_storage { | ||
| // If file is present in File Storage and not in Forest, remove it from File Storage. | ||
| self.remove_file_from_file_storage(&event.file_key.into()) | ||
| .await?; | ||
| } else { | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "File key {:?} not present in File Storage; skipping removal.", | ||
| event.file_key | ||
| ); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
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.
If the storage request failed for any of these reasons, wouldn't we want to delete the file from storage anyways?