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 <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett

radicle-fetch/src/git.rs

@@ -1,5 +1,6 @@
 pub(crate) mod mem;
 pub(crate) mod repository;
+pub(crate) mod packfile;
 
 pub mod refs;
 

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<P: AsRef<Path>>(path: P) -> Option<Self> {
+        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);
+        }
+    }
+}

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<PublicKey, Vec<Update<'static>>>,
+    /// 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<Keepfile>,
 }
 
 impl FetchState {
@@ -233,9 +238,11 @@ impl FetchState {
 
         let wants_haves = step.wants_haves(&handle.repo, &refs)?;
         if !wants_haves.wants.is_empty() {
+            let keepfile =
                 handle
                     .transport
                     .fetch(wants_haves, handle.interrupt.clone(), handshake)?;
+            self.keepfiles.extend(keepfile);
         } else {
             log::trace!(target: "fetch", "Nothing to fetch")
         };

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<AtomicBool>,
         handshake: &handshake::Outcome,
-    ) -> io::Result<()> {
+    ) -> io::Result<Option<Keepfile>> {
         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

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<Ref>,
     pub pack: Option<pack::bundle::write::Outcome>,
+    pub keepfile: Option<packfile::Keepfile>,
 }
 
 // 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,
         },
     };
 

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]