refactor(downloader): take &self across the downloader hierarchy

Concurrent package operations call into the same downloader instances
through Rc<RefCell<dyn DownloaderInterface>>; with &mut self methods
every call holds a RefMut across its awaits, which panics with
'already mutably borrowed' the moment two operations overlap. This is
groundwork for fanning out InstallationManager's download/install
loops (same rework HttpDownloader/CurlDownloader already got).

- DownloaderInterface/ChangeReportInterface/ArchiveDownloader/
  VcsDownloader methods now take &self; as_change_report_interface
  returns &dyn instead of &mut dyn.
- Implementors move their genuinely mutable state behind cells:
  FileDownloader.additional_cleanup_paths, the archive downloaders'
  cleanup_executed, ZipDownloader.zip_archive_object,
  VcsDownloaderBase.has_cleaned_changes, GitDownloader's stash/discard/
  cache maps and GitUtil, SvnDownloader.cache_credentials,
  PerforceDownloader.perforce. FileDownloader.io gains a RefCell layer
  so get_local_changes can keep PHP's NullIO swap under &self.
- ProcessExecutor::execute_async now returns a future that captures
  everything up front instead of borrowing the executor, and call
  sites build the future before awaiting, so no borrow on the shared
  executor is held while a subprocess runs.
- Filesystem::remove_directory_async becomes remove_directory_async_via
  taking the Rc handle: the Filesystem is only borrowed for the sync
  head/tail, never across the rm subprocess await (sync borrow_mut
  users like rename/ensure_directory_exists would otherwise collide).
- DownloadManager async call sites hold shared borrows only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

8 files changed, 44 insertions, 44 deletions

diff --git a/crates/shirabe/tests/downloader/download_manager_test.rs b/crates/shirabe/tests/downloader/download_manager_test.rs
index 6f6ed352..9b45d47a 100644
--- a/crates/shirabe/tests/downloader/download_manager_test.rs
+++ b/crates/shirabe/tests/downloader/download_manager_test.rs
@@ -18,39 +18,39 @@ mockall::mock! {
     impl DownloaderInterface for Downloader {
         fn get_installation_source(&self) -> String;
         async fn download(
-            &mut self,
+            &self,
             package: PackageInterfaceHandle,
             path: &str,
             prev_package: Option<PackageInterfaceHandle>,
             output: bool,
         ) -> anyhow::Result<Option<PhpMixed>>;
         async fn prepare(
-            &mut self,
+            &self,
             r#type: &str,
             package: PackageInterfaceHandle,
             path: &str,
             prev_package: Option<PackageInterfaceHandle>,
         ) -> anyhow::Result<Option<PhpMixed>>;
         async fn install(
-            &mut self,
+            &self,
             package: PackageInterfaceHandle,
             path: &str,
             output: bool,
         ) -> anyhow::Result<Option<PhpMixed>>;
         async fn update(
-            &mut self,
+            &self,
             initial: PackageInterfaceHandle,
             target: PackageInterfaceHandle,
             path: &str,
         ) -> anyhow::Result<Option<PhpMixed>>;
         async fn remove(
-            &mut self,
+            &self,
             package: PackageInterfaceHandle,
             path: &str,
             output: bool,
         ) -> anyhow::Result<Option<PhpMixed>>;
         async fn cleanup(
-            &mut self,
+            &self,
             r#type: &str,
             package: PackageInterfaceHandle,
             path: &str,
diff --git a/crates/shirabe/tests/downloader/file_downloader_test.rs b/crates/shirabe/tests/downloader/file_downloader_test.rs
index a9af69ed..451b792e 100644
--- a/crates/shirabe/tests/downloader/file_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/file_downloader_test.rs
@@ -79,7 +79,7 @@ fn get_downloader(
 fn test_download_for_package_without_dist_reference() {
     let package = get_package("dummy/pkg", "1.0.0");
 
-    let (mut downloader, _http_downloader, _guard) = get_downloader(None, None);
+    let (downloader, _http_downloader, _guard) = get_downloader(None, None);
     let result = run(downloader.download(package, "/path", None, true));
 
     let e = result.expect_err("expected InvalidArgumentException");
@@ -101,7 +101,7 @@ fn test_download_to_existing_file() {
     std::fs::write(&path, b"").unwrap();
     let path = path.to_string_lossy().into_owned();
 
-    let (mut downloader, _http_downloader, _guard) = get_downloader(None, None);
+    let (downloader, _http_downloader, _guard) = get_downloader(None, None);
 
     let result = run(downloader.download(package, &path, None, true));
 
@@ -154,7 +154,7 @@ fn test_download_but_file_is_unsaved() {
     );
     let config = get_config(config_options);
 
-    let (mut downloader, http_downloader, _guard) = get_downloader(None, Some(config));
+    let (downloader, http_downloader, _guard) = get_downloader(None, Some(config));
 
     let mut loop_ = Loop::new(http_downloader, None);
     let promise = Box::pin(async {
@@ -272,7 +272,7 @@ fn test_download_file_with_invalid_checksum() {
 
     // The PHP test injects a Filesystem mock so cleanup is a no-op; a real Filesystem behaves the
     // same here since nothing under the temp dir needs preserving.
-    let (mut downloader, http_downloader, _guard) = get_downloader(None, Some(config));
+    let (downloader, http_downloader, _guard) = get_downloader(None, Some(config));
 
     // make sure the file expected to be downloaded is on disk already
     let dl_file = downloader.__get_file_name(package.clone(), &path);
@@ -350,7 +350,7 @@ fn test_downgrade_shows_appropriate_message() {
     );
 
     let io: std::rc::Rc<std::cell::RefCell<dyn IOInterface>> = io_mock.clone();
-    let mut downloader = FileDownloader::new(
+    let downloader = FileDownloader::new(
         io,
         config,
         http_downloader.clone(),
diff --git a/crates/shirabe/tests/downloader/fossil_downloader_test.rs b/crates/shirabe/tests/downloader/fossil_downloader_test.rs
index e473b2dd..d3b50a9f 100644
--- a/crates/shirabe/tests/downloader/fossil_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/fossil_downloader_test.rs
@@ -87,7 +87,7 @@ fn test_install_for_package_without_source_reference() {
     let package = get_package(None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let path = format!("{}/path", working_dir.path().to_string_lossy());
     let result = run(downloader.install(package, &path));
@@ -121,7 +121,7 @@ fn test_install() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
     run(downloader.install(package, &working_dir_str)).unwrap();
 }
 
@@ -134,7 +134,7 @@ fn test_updatefor_package_without_source_reference() {
     let source_package = get_package(None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(async {
         downloader
@@ -182,7 +182,7 @@ fn test_update() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
     run(async {
         downloader
             .prepare(
@@ -229,7 +229,7 @@ fn test_remove() {
     });
     let filesystem = std::rc::Rc::new(std::cell::RefCell::new(filesystem));
 
-    let mut downloader = get_downloader_mock(None, None, process, Some(filesystem.clone()));
+    let downloader = get_downloader_mock(None, None, process, Some(filesystem.clone()));
     run(async {
         downloader
             .prepare("uninstall", package.clone(), &working_dir_str, None)
diff --git a/crates/shirabe/tests/downloader/git_downloader_test.rs b/crates/shirabe/tests/downloader/git_downloader_test.rs
index d4f40b77..f7c82066 100644
--- a/crates/shirabe/tests/downloader/git_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/git_downloader_test.rs
@@ -120,7 +120,7 @@ fn test_download_for_package_without_source_reference() {
     let package = get_package("dummy/pkg", "1.0.0", None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(async {
         downloader.download(package.clone(), "/path", None).await?;
@@ -183,7 +183,7 @@ fn test_download() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     run(async {
         downloader
@@ -303,7 +303,7 @@ fn test_download_with_cache() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, Some(config), process, None);
+    let downloader = get_downloader_mock(None, Some(config), process, None);
 
     run(async {
         downloader
@@ -428,7 +428,7 @@ fn test_download_uses_various_protocols_and_sets_push_url_for_github() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     run(async {
         downloader
@@ -529,7 +529,7 @@ fn test_download_and_set_push_url_use_custom_various_protocols_for_github() {
         top.insert("config".to_string(), PhpMixed::Array(section));
         config.merge(&top, Config::SOURCE_UNKNOWN);
 
-        let mut downloader = get_downloader_mock(None, Some(config), process, None);
+        let downloader = get_downloader_mock(None, Some(config), process, None);
 
         run(async {
             downloader
@@ -578,7 +578,7 @@ fn test_download_throws_runtime_exception_if_git_command_fails() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(async {
         downloader
@@ -610,7 +610,7 @@ fn test_updatefor_package_without_source_reference() {
     let source_package = get_package("dummy/pkg", "1.0.0", None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(async {
         downloader
@@ -680,7 +680,7 @@ fn test_update() {
         .unwrap();
     let working_dir_str = working_dir.path().to_string_lossy().into_owned();
 
-    let mut downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
+    let downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
 
     run(async {
         downloader
@@ -760,7 +760,7 @@ fn test_update_with_new_repo_url() {
         .unwrap();
     let working_dir_str = working_dir.path().to_string_lossy().into_owned();
 
-    let mut downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
+    let downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
 
     run(async {
         downloader
@@ -842,7 +842,7 @@ fn test_update_throws_runtime_exception_if_git_command_fails() {
 
     let config = Config::new(false, None);
 
-    let mut downloader = get_downloader_mock(None, Some(config), process, None);
+    let downloader = get_downloader_mock(None, Some(config), process, None);
 
     let result = run(async {
         downloader
@@ -945,7 +945,7 @@ fn test_update_doesnt_throws_runtime_exception_if_git_command_fails_at_first_but
         .unwrap();
     let working_dir_str = working_dir.path().to_string_lossy().into_owned();
 
-    let mut downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
+    let downloader = get_downloader_mock(None, Some(Config::new(false, None)), process, None);
 
     run(async {
         downloader
@@ -1001,7 +1001,7 @@ fn test_downgrade_shows_appropriate_message() {
         .unwrap();
     let working_dir_str = working_dir.path().to_string_lossy().into_owned();
 
-    let mut downloader = get_downloader_mock(Some(io), None, process, None);
+    let downloader = get_downloader_mock(Some(io), None, process, None);
 
     run(async {
         downloader
@@ -1079,7 +1079,7 @@ fn test_not_using_downgrading_with_references() {
         .unwrap();
     let working_dir_str = working_dir.path().to_string_lossy().into_owned();
 
-    let mut downloader = get_downloader_mock(Some(io), None, process, None);
+    let downloader = get_downloader_mock(Some(io), None, process, None);
 
     run(async {
         downloader
diff --git a/crates/shirabe/tests/downloader/hg_downloader_test.rs b/crates/shirabe/tests/downloader/hg_downloader_test.rs
index ac3d50b6..1ba5cf1c 100644
--- a/crates/shirabe/tests/downloader/hg_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/hg_downloader_test.rs
@@ -87,7 +87,7 @@ fn test_download_for_package_without_source_reference() {
     let package = get_package(None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(downloader.install(package, "/path"));
     let e = result.expect_err("missing source reference should throw");
@@ -117,7 +117,7 @@ fn test_download() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
     run(downloader.install(package, &working_dir_str)).unwrap();
 }
 
@@ -131,7 +131,7 @@ fn test_updatefor_package_without_source_reference() {
     let source_package = get_package(None, None);
 
     let (process, _guard) = get_process_executor_mock(vec![], false, Default::default());
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
 
     let result = run(async {
         downloader
@@ -177,7 +177,7 @@ fn test_update() {
         Default::default(),
     );
 
-    let mut downloader = get_downloader_mock(None, None, process, None);
+    let downloader = get_downloader_mock(None, None, process, None);
     run(async {
         downloader
             .prepare(
@@ -221,7 +221,7 @@ fn test_remove() {
     });
     let filesystem = std::rc::Rc::new(std::cell::RefCell::new(filesystem));
 
-    let mut downloader = get_downloader_mock(None, None, process, Some(filesystem.clone()));
+    let downloader = get_downloader_mock(None, None, process, Some(filesystem.clone()));
     run(async {
         downloader
             .prepare("uninstall", package.clone(), &working_dir_str, None)
diff --git a/crates/shirabe/tests/downloader/perforce_downloader_test.rs b/crates/shirabe/tests/downloader/perforce_downloader_test.rs
index bdf24a2e..c657e244 100644
--- a/crates/shirabe/tests/downloader/perforce_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/perforce_downloader_test.rs
@@ -83,7 +83,7 @@ fn test_init_perforce_instantiates_a_new_perforce_object() {
     let (process, _process_guard) =
         get_process_executor_mock(vec![], false, MockHandler::default());
     let fs = std::rc::Rc::new(std::cell::RefCell::new(Filesystem::new(None)));
-    let mut downloader = PerforceDownloader::new(io, config, process, fs);
+    let downloader = PerforceDownloader::new(io, config, process, fs);
 
     downloader.init_perforce(
         package,
@@ -101,7 +101,7 @@ fn test_init_perforce_does_nothing_if_perforce_already_set() {
     let (process, _process_guard) =
         get_process_executor_mock(vec![], false, MockHandler::default());
     let fs = std::rc::Rc::new(std::cell::RefCell::new(Filesystem::new(None)));
-    let mut downloader = PerforceDownloader::new(io, config, process, fs);
+    let downloader = PerforceDownloader::new(io, config, process, fs);
 
     // The already-set perforce only sees initializePath; the repository's getRepoConfig is
     // never consulted (the early return happens before reaching it).
@@ -146,7 +146,7 @@ fn do_install_workflow(source_ref: &'static str, expected_label: Option<String>)
     let (process, _process_guard) =
         get_process_executor_mock(vec![], false, MockHandler::default());
     let fs = std::rc::Rc::new(std::cell::RefCell::new(Filesystem::new(None)));
-    let mut downloader = PerforceDownloader::new(io, config, process, fs);
+    let downloader = PerforceDownloader::new(io, config, process, fs);
 
     let mut perforce = MockPerforce::new();
     let expected_path = test_path_str.clone();
diff --git a/crates/shirabe/tests/downloader/xz_downloader_test.rs b/crates/shirabe/tests/downloader/xz_downloader_test.rs
index 0002f0e6..db4659f9 100644
--- a/crates/shirabe/tests/downloader/xz_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/xz_downloader_test.rs
@@ -92,7 +92,7 @@ fn test_error_messages() {
     let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(Some(
         io.clone(),
     ))));
-    let mut downloader = XzDownloader::new(
+    let downloader = XzDownloader::new(
         io,
         config,
         http_downloader.clone(),
diff --git a/crates/shirabe/tests/downloader/zip_downloader_test.rs b/crates/shirabe/tests/downloader/zip_downloader_test.rs
index 878c8054..593f3e76 100644
--- a/crates/shirabe/tests/downloader/zip_downloader_test.rs
+++ b/crates/shirabe/tests/downloader/zip_downloader_test.rs
@@ -155,7 +155,7 @@ fn test_error_messages() {
     let process = std::rc::Rc::new(std::cell::RefCell::new(ProcessExecutor::new(Some(
         io.clone(),
     ))));
-    let mut downloader = ZipDownloader::new(
+    let downloader = ZipDownloader::new(
         io,
         config,
         http_downloader.clone(),
@@ -173,13 +173,13 @@ fn test_error_messages() {
     let result: anyhow::Result<()> = (|| {
         let mut loop_ = Loop::new(http_downloader.clone(), None);
         let promise = Box::pin(async {
-            DownloaderInterface::download(&mut downloader, package.clone(), &path, None, true)
+            DownloaderInterface::download(&downloader, package.clone(), &path, None, true)
                 .await
                 .map(|_| ())
         });
         run(loop_.wait(vec![promise], None))?;
         run(DownloaderInterface::install(
-            &mut downloader,
+            &downloader,
             package.clone(),
             &path,
             true,
@@ -198,7 +198,7 @@ fn test_zip_archive_only_failed() {
     let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf());
 
     ZipDownloader::__set_has_zip_archive(Some(true));
-    let mut downloader = make_downloader(&set_up);
+    let downloader = make_downloader(&set_up);
     let zip_archive = ZipArchive::__mock(ZipArchiveMock {
         open: Ok(()),
         count: 0,
@@ -224,7 +224,7 @@ fn test_zip_archive_extract_only_failed() {
     let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf());
 
     ZipDownloader::__set_has_zip_archive(Some(true));
-    let mut downloader = make_downloader(&set_up);
+    let downloader = make_downloader(&set_up);
     let zip_archive = ZipArchive::__mock(ZipArchiveMock {
         open: Ok(()),
         count: 0,
@@ -252,7 +252,7 @@ fn test_zip_archive_only_good() {
     let _tear_down = TearDown::new(set_up.test_dir.path().to_path_buf());
 
     ZipDownloader::__set_has_zip_archive(Some(true));
-    let mut downloader = make_downloader(&set_up);
+    let downloader = make_downloader(&set_up);
     let zip_archive = ZipArchive::__mock(ZipArchiveMock {
         open: Ok(()),
         count: 0,