From d56d619f5601ba456c7bb517be96a1418743cf9e Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Thu, 16 May 2024 11:30:34 +0100 Subject: [PATCH] fetch: remove `.keep` files A `.keep` file in the `objects/pack` directory acts as a lock to ensure that operations like `git gc` do not clean up `.idx` files while a fetch is being performed. It is safe to remove the `.keep` file once a fetch has completed and a reference points to a given object in the packfile. To ensure this happens in `radicle-fetch`, a newtype `Keepfile` is introduced. It uses special `Drop` semantics by attempting to remove the file when the object is dropped -- logging a warning if it fails to do so. `FetchState` is augmented to keep a set of the `Keepfile`s. This ensures that when the fetch completes, either successfully or with a failure, when `FetchState` is dropped then so are the `Keepfile`s. We ensure that the expected behaviour of removing the `.keep`files occurs by adding a check in `e2e::test_replication` to check Alice's repository's packs. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-fetch/src/git.rs | 1 + radicle-fetch/src/git/packfile.rs | 32 ++++++++++++++++++++++++++++ radicle-fetch/src/state.rs | 13 ++++++++--- radicle-fetch/src/transport.rs | 5 +++-- radicle-fetch/src/transport/fetch.rs | 5 +++++ radicle-node/src/tests/e2e.rs | 15 +++++++++++++ 6 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 radicle-fetch/src/git/packfile.rs diff --git a/radicle-fetch/src/git.rs b/radicle-fetch/src/git.rs index eafe9330..b7b4c42f 100644 --- a/radicle-fetch/src/git.rs +++ b/radicle-fetch/src/git.rs @@ -1,5 +1,6 @@ pub(crate) mod mem; pub(crate) mod repository; +pub(crate) mod packfile; pub mod refs; diff --git a/radicle-fetch/src/git/packfile.rs b/radicle-fetch/src/git/packfile.rs new file mode 100644 index 00000000..1b7a6612 --- /dev/null +++ b/radicle-fetch/src/git/packfile.rs @@ -0,0 +1,32 @@ +use std::fs; +use std::path::{Path, PathBuf}; + +/// The [`PathBuf`] which points to a `*.keep` file, which should correspond to +/// a packfile. +/// +/// Upon drop, it attempts to remove the [`PathBuf`] to release the lock on the +/// packfile index, allowing it to be garbage collected. +#[derive(Clone, Debug)] +pub struct Keepfile { + path: PathBuf, +} + +impl Keepfile { + pub fn new>(path: P) -> Option { + let path = path.as_ref(); + match path.extension() { + Some(ext) if ext == "keep" => Some(Self { + path: path.to_path_buf(), + }), + _ => None, + } + } +} + +impl Drop for Keepfile { + fn drop(&mut self) { + if let Err(e) = fs::remove_file(&self.path) { + log::warn!(target: "fetch", "Failed to remove {:?}: {e}", self.path); + } + } +} diff --git a/radicle-fetch/src/state.rs b/radicle-fetch/src/state.rs index 5c944ecd..489cbb83 100644 --- a/radicle-fetch/src/state.rs +++ b/radicle-fetch/src/state.rs @@ -14,6 +14,7 @@ use radicle::storage::{ }; use crate::git; +use crate::git::packfile::Keepfile; use crate::git::refs::{Applied, Update}; use crate::git::repository; use crate::sigrefs::SignedRefsAt; @@ -160,6 +161,10 @@ pub struct FetchState { sigrefs: SigrefTips, /// Seen reference tips, per remote. tips: BTreeMap>>, + /// The `.keep` files created during packfile transfers. They are kept + /// within the state, so that when the state is dropped, it also attempts to + /// delete the files to release the locks on the packfiles. + keepfiles: Vec, } impl FetchState { @@ -233,9 +238,11 @@ impl FetchState { let wants_haves = step.wants_haves(&handle.repo, &refs)?; if !wants_haves.wants.is_empty() { - handle - .transport - .fetch(wants_haves, handle.interrupt.clone(), handshake)?; + let keepfile = + handle + .transport + .fetch(wants_haves, handle.interrupt.clone(), handshake)?; + self.keepfiles.extend(keepfile); } else { log::trace!(target: "fetch", "Nothing to fetch") }; diff --git a/radicle-fetch/src/transport.rs b/radicle-fetch/src/transport.rs index 98c411b7..a577452f 100644 --- a/radicle-fetch/src/transport.rs +++ b/radicle-fetch/src/transport.rs @@ -21,6 +21,7 @@ use radicle::storage::git::Repository; use thiserror::Error; use crate::git::oid; +use crate::git::packfile::Keepfile; use crate::git::repository; /// Open a reader and writer stream to pass to the ls-refs and fetch @@ -120,7 +121,7 @@ where wants_haves: WantsHaves, interrupt: Arc, handshake: &handshake::Outcome, - ) -> io::Result<()> { + ) -> io::Result> { log::trace!( target: "fetch", "Running fetch wants={:?}, haves={:?}", @@ -170,7 +171,7 @@ where } } - Ok(()) + Ok(out.keepfile) } /// Signal to the server side that we are done sending ls-refs and diff --git a/radicle-fetch/src/transport/fetch.rs b/radicle-fetch/src/transport/fetch.rs index f56d1dad..cc55450d 100644 --- a/radicle-fetch/src/transport/fetch.rs +++ b/radicle-fetch/src/transport/fetch.rs @@ -17,6 +17,8 @@ use gix_transport::client; use gix_transport::client::{ExtendedBufRead, MessageKind}; use gix_transport::Protocol; +use crate::git::packfile; + use super::{agent_name, indicate_end_of_interaction, Connection, WantsHaves}; pub type Error = gix_protocol::fetch::Error; @@ -102,6 +104,7 @@ pub struct Fetch { pub struct FetchOut { pub refs: Vec, pub pack: Option, + pub keepfile: Option, } // FIXME: the delegate pattern will be removed in the near future and @@ -127,6 +130,7 @@ impl<'a> Delegate for &'a mut Fetch { .pack_writer .write_pack(input, progress) .map_err(|err| io::Error::new(io::ErrorKind::Other, err))?; + self.out.keepfile = pack.keep_path.as_ref().and_then(packfile::Keepfile::new); self.out.pack = Some(pack); Ok(()) } @@ -204,6 +208,7 @@ where out: FetchOut { refs: Vec::new(), pack: None, + keepfile: None, }, }; diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs index ccdc9662..af77677b 100644 --- a/radicle-node/src/tests/e2e.rs +++ b/radicle-node/src/tests/e2e.rs @@ -216,6 +216,21 @@ fn test_replication() { alice.storage.repository(acme).unwrap().validate(), Ok(validations) if validations.is_empty() ); + + // Ensure that .keep files are deleted upon replication + { + let repo = alice.storage.repository(acme).unwrap(); + let pack_dir = repo.path().join("objects").join("pack"); + for entry in std::fs::read_dir(pack_dir).unwrap() { + let entry = entry.unwrap(); + let path = entry.path(); + assert_ne!( + path.extension(), + Some("keep".as_ref()), + "found .keep file after fetch: {path:?}" + ); + } + } } #[test]