feat(msp): rejected storage request triggers file clean up if needed #547
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HermanObst
changed the title
…letion. wip: debugging
HermanObst
added
B5-clientnoteworthy
snowmead
reviewed
Nov 4, 2025
Comment on lines
+248
to
+254
| if !matches!( | ||
| reason, | ||
| pallet_file_system::types::RejectedStorageRequestReason::RequestExpired | ||
| | pallet_file_system::types::RejectedStorageRequestReason::InternalError | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the storage request failed for any of these reasons, wouldn't we want to delete the file from storage anyways?
ReachedMaximumCapacity
ReceivedInvalidProof
FileKeyAlreadyStored
Comment on lines
+166
to
+177
| 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 | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Buckets can be removed from MSPs in unfinalized blocks. I don't think this needs to be a critical error.
Comment on lines
+184
to
+190
| 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")); |
There was a problem hiding this comment.
Why do we consider this case a warning and also return an error?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
File deletion from file storage are only being triggered by
MutationsAppliedevent. This is correct for the happy paths as this event is emitted always that an msp accepted the storage request (thus it root was updated in the runtime) and now is not required anymore, either because the user wants to delete it, or the user revoke the SR.But this leaves us with the edge case where the msp try to accept the SR (thus, it stores the file), but the transaction fails and his root is not updated on-chain. In this case, he will never see
MutationsAppliedevent.Solution:
Make msp react to events that inform that the SR was incomplete (
StorageRequestRejected) but MSP could have sent a tx accepting it that ends up failing.StorageRequestRejectedevent contains two new fields:msp_idandbucket_id. leaving the new struct as follows: