Skip to content

Conversation

@HermanObst
Copy link
Contributor

@HermanObst HermanObst commented Dec 23, 2025

Problem

File storage locks were being held for longer than necessary:

  1. file_download_manager.rs: Write lock acquired before try_download_chunk_batch and held during network I/O, retries, and 500ms delays between attempts.

  2. msp_upload_file.rs: Write lock acquired early in handle_new_storage_request_event and held across file existence checks, even when only a read lock was needed.

Solution

file_download_manager.rs:

  • Moved lock acquisition into process_proven_chunk
  • Lock now acquired only for the brief duration of writing each chunk
  • Network I/O happens completely outside any file storage lock

msp_upload_file.rs:

  • Changed file_in_file_storage check to use a scoped read lock instead of holding the write lock
  • Write lock now only acquired when actually inserting a file
  • is_file_complete check also uses a scoped read lock

TBD: It can be argued that taking and releasing the write lock for every chunk would be a lot of overhead:

  • RwLock are pretty fast, even more compared to disk writes (what we are already doing by chunk)
  • To be seen if this decrease performance

Anyway, open to discussion :)

@HermanObst HermanObst added B0-silent Changes should not be mentioned in any release notes D3-trivial👶 PR contains trivial changes that do not require an audit not-breaking Does not need to be mentioned in breaking changes labels Dec 23, 2025
@HermanObst HermanObst changed the title reduce locks critical zone refactor: Reduce file storage locks critical zone Dec 23, 2025
@santikaplan santikaplan requested a review from ffarall December 26, 2025 13:20
Copy link
Collaborator

@ffarall ffarall left a comment

Choose a reason for hiding this comment

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

Great job!

@ffarall ffarall changed the title refactor: Reduce file storage locks critical zone refactor: ♻️ Reduce file storage locks critical zone Dec 26, 2025
@ffarall ffarall changed the title refactor: ♻️ Reduce file storage locks critical zone refactor: 🔒 Reduce file storage locks critical zone Dec 26, 2025
@ffarall ffarall merged commit 1b7941f into main Dec 26, 2025
93 of 96 checks passed
@ffarall ffarall deleted the ho/reduce-lock-critical-zone branch December 26, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B0-silent Changes should not be mentioned in any release notes D3-trivial👶 PR contains trivial changes that do not require an audit not-breaking Does not need to be mentioned in breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants