Handle orphaned blobs better in blob eviction logic for blob storage #1712
Conversation
ag-eitilt
left a comment
ag-eitilt
left a comment
There was a problem hiding this comment.
Got a couple questions, but mostly about how things work rather than why they're this way. Definitely a good catch for why the blobs were getting orphaned; let's get this in and see if there's any other leaks!
| should_sleep = candidate_count == 0; | ||
|
|
||
| should_sleep = deleted == 0; | ||
| if candidate_count == 0 { |
There was a problem hiding this comment.
A) Does should_sleep get used later on in this scope, and B) whether or not it does, should we use that in the if?
|
|
||
| tracing::info!(%deleted, "N blobs deleted for eviction"); | ||
| // Delete from blob storage first to ensure no untracked files are left behind | ||
| let mut successfully_deleted_blob_ids = Vec::new(); |
There was a problem hiding this comment.
I don't know Rust. Is the mut saying that the contents of the vector might change, or is it overkill and indicates that the vector container itself might be switched out?
|
|
||
| for chunk in chunked { | ||
| let thread_store = blob_stores.clone(); | ||
| let mut chunk_success_ids = Vec::new(); |
|
|
||
| store.delete_key(blob.key.clone()).await.unwrap_or_else(|err| { | ||
| let blob = blob.clone(); | ||
| // Process chunk synchronously to collect successful deletions |
There was a problem hiding this comment.
How critical is the synchronous nature? Unless there's a contents-comparison or something which depends or order, it seems like we could still allow async deletion. Assuming Rust allows objects to be shared between threads, of course, which now that I say it seems like there's a chance it breaks the ownership module or something. Or were we paying extra complexity for no real time/compute savings?
Currently there is an issue with the RSC where blobs are not evicted properly and remain orphaned in the blob store. This can lead to disk space issues over time. To properly handle this, we should modify the blob eviction logic to delete from the blob store first before deleting from the database, so that we can handle retries if deletion fails