From 42b71d38c8e8bc162d3cb2b8ae8ab49bdfec564a Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 25 Aug 2026 17:58:12 +0000 Subject: [PATCH 1/5] Pack the bare caches' refs on the sweep Every ref a fetch updates is written as a loose file costing a whole filesystem block, and nothing in devlaunch ever collapsed them: pack-refs --auto is a no-op on git's files backend and no gc runs on a bare. The packing goes where the loose refs are made. fetch_all has exactly one production caller, so "after the fetch" is the detached background sweep by construction, in a repo-lock scope it already holds, and the pass that skips the fetch spawns nothing. Not justified by disk: the cost is one block per ref, so it tracks how many branches a remote leaves open rather than repository size, and a whole cache is 30 MB to 60 MB. What carries it is placement plus a second payment in probe latency. Measured on git 2.51.1 over a bare of 551 refs with 301 loose, the rev-list probe fell from 5.3 ms to 2.8 ms, the loose files held 1204 KiB of blocks against a 30 KiB packed-refs, and the pack took 23 ms. A pack that refuses is not a fetch that failed. It is a CacheNotice naming the repository and git's own words, and the freshness stamp still lands: the fetch happened, and withholding the stamp would re-fetch the whole repository every interval forever over a representation change that did not come off. --- docs/cleanup.md | 47 ++++ rust/devlaunch-core/src/clients/git.rs | 42 ++++ rust/devlaunch-core/src/clients/git/tests.rs | 18 +- rust/devlaunch-core/src/flows/repo_manager.rs | 218 +++++++++++++++++- rust/dl/src/render.rs | 24 ++ 5 files changed, 345 insertions(+), 4 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index 2fbe4a22..42a04d3a 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -344,6 +344,53 @@ everything else on the machine. Deleting them is a decision with your own containers on the other side of it, and `docker system df` is the tool that shows you what it costs. +### The bare caches' loose refs, and who packs them + +| artifact | who reclaims it | what makes that safe | +| --- | --- | --- | +| loose ref files under a bare cache's `refs/` | the background freshness sweep, with one `git pack-refs --all` after each fetch that succeeded | packing changes how a ref is stored and not which refs exist, so there is nothing to prove and nothing to consent to | + +Every ref a fetch updates is written as a file, and a file costs a whole +filesystem block: about 4096 bytes each, against the 81 or so the same ref takes +as a line in `packed-refs`. Nothing used to collapse them. `pack-refs --auto` is a +documented no-op on git's `files` ref backend, and `dl` runs no `gc` on a bare, so +`gc.auto` never gets the chance either. + +The cost is one block per **ref**, which means it tracks how many branches and +tags a remote leaves open and has almost nothing to do with how big the +repository is. Measured across ten real remotes with `git ls-remote`, +`torvalds/linux` carries 1887 refs against `microsoft/vscode`'s 5342 and +`rust-lang/rust`'s 334, with a median around 370. So a whole cache of 20 to 40 +repositories holds something like 30 MB to 60 MB of loose refs, a couple of +percent of one bare's own size. **Disk is not the reason this is here.** + +The reasons it is here are placement and a second payment. The broad sweep is the +only thing in `dl` that fetches every head and tag, so it is the only thing that +makes loose refs in quantity, and it already holds the repository's lock while it +does. Packing there costs one more bounded `git` call in a scope that just spent +its whole network budget, and it happens only on a pass that actually fetched. And +the guards that decide whether a clone is safe to remove ask the bare a +reachability question that walks every ref; with the refs packed that walk reads +one file instead of thousands. Measured on git 2.51.1 over a bare of 551 refs with +301 of them loose, the probe fell from 5.3 ms to 2.8 ms, the loose files held +1204 KiB of blocks against a 30 KiB `packed-refs` for all 551, and the pack itself +took 23 ms. + +**A pack that refuses is not a fetch that failed.** The fetch is the point of the +sweep and the packing is the optional half, so a refusal is a line on stderr +naming the repository and git's own words, the record's freshness stamp still +lands, and the next sweep tries again. Withholding the stamp would make every +later pass re-fetch the whole repository forever on account of a representation +change that did not come off. + +**Packing does not change what a later prune may delete.** A ref the remote +retracts is removed whether it was loose or packed: git rewrites `packed-refs` +through the same ref transaction that unlinks a loose file, and a ref that was +loose over a stale packed line loses both, so nothing comes back at an old sha. +The only difference is cost, and it falls on the prune rather than here, since +removing a packed ref rewrites the whole file where removing a loose one unlinks a +single path. + ### Reconciling records that disagree `dl` keeps its own record of every workspace, and devpod keeps one too. They diff --git a/rust/devlaunch-core/src/clients/git.rs b/rust/devlaunch-core/src/clients/git.rs index ae8c8885..e62df269 100644 --- a/rust/devlaunch-core/src/clients/git.rs +++ b/rust/devlaunch-core/src/clients/git.rs @@ -586,6 +586,48 @@ impl<'r> Git<'r> { self.captured("fetch", &spec) } + /// Collapse every loose ref in the bare into `packed-refs`. + /// + /// A ref git writes as a file costs a whole filesystem block, typically 4096 + /// bytes against the ~81 the packed line takes, and [`Git::fetch_all`] writes + /// one per ref it updates. Nothing else in devlaunch ever packed them: + /// `pack-refs --auto` is a documented no-op on the `files` backend, and no + /// `gc` is run on the bare, so `gc.auto` never gets the chance either. + /// Measured on git 2.51.1 over a bare with 301 loose refs of 551, the loose + /// files held 1204 KiB of blocks against a 30 KiB `packed-refs` for all 551, + /// and the pack took 23 ms. + /// + /// `--all` rather than the default, because the default packs only tags and + /// would leave every head loose, which is the population the sweep actually + /// makes. + /// + /// **Pure, in the sense that decides where this is allowed to live.** Packing + /// changes how a ref is stored and not which refs exist, so it can lose no + /// work — and it does not change what a later `--prune` may delete either. + /// Measured on 2.51.1: a prune of a packed ref rewrites `packed-refs` through + /// the same ref transaction, deletes nothing else, and a ref that was loose + /// over a stale packed line loses *both*, so nothing is resurrected at the old + /// sha. The only difference is cost, and it falls on the prune: removing a + /// packed ref rewrites the whole file where removing a loose one unlinks + /// a single path. + /// + /// Bounded at [`ABOUT_ONE_REPO`] rather than left unbounded like the sweep's + /// fetch: this touches no network, so a pack that has not finished in thirty + /// seconds is a stuck filesystem rather than a slow remote, and the caller it + /// runs under is a detached child whose whole point is that nobody is watching + /// it. + pub(crate) fn pack_refs(&self, bare: &Path) -> GitAnswer { + self.captured( + "pack-refs", + &SpawnSpec::new( + Invocation::new(PROGRAM) + .with_args(["pack-refs", "--all"]) + .with_cwd(bare.to_path_buf()), + ) + .with_timeout(ABOUT_ONE_REPO), + ) + } + /// Fetch exactly one branch into the bare cache. /// /// The launch path's entire network budget, so the time it can hold the repo diff --git a/rust/devlaunch-core/src/clients/git/tests.rs b/rust/devlaunch-core/src/clients/git/tests.rs index 73ecdec4..f80a9cbb 100644 --- a/rust/devlaunch-core/src/clients/git/tests.rs +++ b/rust/devlaunch-core/src/clients/git/tests.rs @@ -503,6 +503,21 @@ fn the_background_sweep_s_bound_reaches_the_spawn() { assert_eq!(timeout(&fake), Some(Duration::from_secs(60))); } +#[test] +fn packing_collapses_every_loose_ref_in_the_bare_under_a_bound() { + // `--all` and not the default, which packs tags alone and would leave every + // head the sweep just fetched sitting loose. The bound is there because this + // touches no network: thirty seconds of `pack-refs` is a stuck filesystem, and + // the caller is a detached child nobody is watching. + let fake = ScriptedRunner::new(); + + Git::new(&fake).pack_refs(Path::new("/cache/o/r/.bare")); + + assert_eq!(strs(&argv(&fake)), ["git", "pack-refs", "--all"]); + assert_eq!(cwd(&fake).as_deref(), Some(Path::new("/cache/o/r/.bare"))); + assert_eq!(timeout(&fake), Some(Duration::from_secs(30))); +} + #[test] fn fetching_one_ref_moves_exactly_that_ref_in_the_c_locale() { let fake = ScriptedRunner::new(); @@ -967,6 +982,7 @@ fn nothing_here_spawns_more_than_once_per_verb() { git.clone_bare("url", Path::new("/cache/.bare")); git.fetch_all(Path::new("/cache/.bare"), None); + git.pack_refs(Path::new("/cache/.bare")); git.fetch_ref(Path::new("/cache/.bare"), "feature"); git.symbolic_ref(Path::new("/cache/.bare"), "HEAD"); git.remote_branch_listing(Path::new("/cache/.bare")); @@ -993,7 +1009,7 @@ fn nothing_here_spawns_more_than_once_per_verb() { git.status_porcelain(Path::new("/ws")); git.unpushed_commits(Path::new("/ws")); - assert_eq!(fake.call_count(), 27, "one spawn per verb, 27 verbs"); + assert_eq!(fake.call_count(), 28, "one spawn per verb, 28 verbs"); assert!( fake.calls() .iter() diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index fa521570..e5e209ed 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -246,6 +246,18 @@ pub enum CacheNotice { base: String, reason: NotRefreshed, }, + /// The loose refs the sweep's own fetch wrote could not be collapsed into + /// `packed-refs`. The fetch itself succeeded, so this is a notice and never a + /// [`FetchRepoError`]: the cache is fresh either way and the only cost is that + /// the bare keeps a block per ref until the next sweep tries again. + /// + /// Names the repository because the sweep walks every repository in one + /// detached process, so a line that did not say which one would be unactionable. + RefsNotPacked { + owner: String, + repo: String, + reason: String, + }, /// The cache's git-lfs store could not be filled. Best-effort: the workspace /// falls through to the network phase. LfsCacheNotFilled { reason: String }, @@ -1398,6 +1410,31 @@ impl<'r> RepositoryManager<'r> { }); } + // Where the loose refs the fetch just wrote get collapsed into + // `packed-refs`. Here rather than anywhere else because `fetch_all` has + // exactly one production caller, so "after the fetch" *is* the detached + // background sweep by construction, in the repo-lock scope it already + // holds; and because packing pays a second time in probe latency, since + // the reachability question asked of the bare walks every ref and today + // reads one file per ref to do it (measured on git 2.51.1, 5.3 ms against + // 2.8 ms over 551 refs, 301 of them loose). + // + // **A refusal here is not a failed fetch.** The fetch happened, the cache + // is fresh, and the stamp below has to land or the sweep re-fetches this + // repository every interval forever on account of a representation change + // that did not come off. Packing is the optional half of this function and + // the fetch is the point of it. Its own arm rather than `let _ =`, for the + // reason [`RecordUpdate::Absent`] gets one below: a caller that cannot say + // what it does with an answer is the shape that turns a no-op into a + // reported success. + if let Some(refused) = self.git.pack_refs(&bare).refusal() { + notices.say(CacheNotice::RefsNotPacked { + owner: owner.to_owned(), + repo: repo.to_owned(), + reason: refused.reason().to_owned(), + }); + } + // The stamp is the only field this touches, so it moves inside the // metadata lock rather than riding back in a copy of the whole record // taken before the lock existed. @@ -2039,6 +2076,32 @@ pub(crate) mod tests { refs } + /// Every loose ref file under a repository's `refs/`, sorted. + /// + /// A ref git has packed is a line in `packed-refs` and no file here, so an + /// empty answer beside a non-empty [`refs_of`] is what "packed" looks like + /// from the filesystem. Directories are walked rather than listed, because a + /// ref name carries slashes and `refs/heads/feature/test` is two levels down. + pub(crate) fn loose_refs_of(repo: &Path) -> Vec { + fn walk(dir: &Path, found: &mut Vec) { + let Ok(listing) = std::fs::read_dir(dir) else { + return; + }; + for entry in listing.filter_map(Result::ok) { + let path = entry.path(); + if path.is_dir() { + walk(&path, found); + } else { + found.push(path); + } + } + } + let mut found = Vec::new(); + walk(&repo.join("refs"), &mut found); + found.sort(); + found + } + /// A real git client over this process's `git`. pub(crate) fn real_git() -> ProcessRunner { ProcessRunner::new() @@ -2450,7 +2513,7 @@ pub(crate) mod tests { .fetch_repo(&mut cache.storage, "owner", "repo", None, &mut ignoring()) .expect("fetched"); - let call = fake.only_call(); + let call = fake.calls().swap_remove(0); assert_eq!( as_strs(&[call.argv()])[0], [ @@ -2474,6 +2537,96 @@ pub(crate) mod tests { ); } + #[test] + fn the_sweep_packs_the_refs_after_the_fetch_and_before_it_stamps_the_record() { + // Order is the whole of the placement argument. Packing after the fetch is + // what makes it the loose refs *this* pass wrote; packing before the stamp + // is what keeps a repository whose pack refused from being stamped as + // though nothing happened -- the stamp lands either way, and the arm below + // is what says so. + let mut cache = a_cache(); + let bare = cache.given_bare_clone("owner", "repo"); + cache.given_record("owner", "repo"); + let fake = FakeGit::new(); + let manager = a_manager(&cache, Git::new(&fake)); + + manager + .fetch_repo(&mut cache.storage, "owner", "repo", None, &mut ignoring()) + .expect("fetched"); + + let argvs = fake.argvs(); + assert_eq!( + as_strs(&argvs), + [ + vec![ + "git", + "fetch", + "origin", + "+refs/heads/*:refs/heads/*", + "+refs/tags/*:refs/tags/*", + "--prune", + ], + vec!["git", "pack-refs", "--all"], + ] + ); + assert_eq!( + fake.calls()[1].invocation().cwd.as_deref(), + Some(bare.as_path()), + "the pack is about the bare the fetch just wrote into" + ); + } + + #[test] + fn a_pack_that_refused_is_reported_and_the_fetch_still_counts_as_done() { + // Principle 1 decides this and it is not close: the fetch happened, so the + // cache is fresh and nothing is at risk. Reporting a failed fetch here + // would be a lie about the network, and withholding the stamp would make + // every later sweep re-fetch the whole repository forever on account of a + // representation change that did not come off. Packing is the optional + // half of this function; the fetch is the point of it. + let mut cache = a_cache(); + cache.given_bare_clone("owner", "repo"); + cache.given_record("owner", "repo"); + let fake = FakeGit::new().with_script( + ["git", "pack-refs"], + Response::failed( + 1, + "fatal: unable to create 'packed-refs.lock': Permission denied\n", + ), + ); + let manager = a_manager(&cache, Git::new(&fake)); + let mut notices = ignoring(); + + manager + .fetch_repo(&mut cache.storage, "owner", "repo", None, &mut notices) + .expect("a pack that refused is not a fetch that failed"); + + assert!( + notices.contains(&CacheNotice::RefsNotPacked { + owner: "owner".to_owned(), + repo: "repo".to_owned(), + reason: "fatal: unable to create 'packed-refs.lock': Permission denied".to_owned(), + }), + "the refusal has to reach a reader, and say which repository: {notices:?}" + ); + assert!( + cache + .storage + .get_repository("owner", "repo") + .expect("the record") + .last_fetched + .is_some(), + "the stamp is about the fetch, and the fetch happened" + ); + assert!( + notices.contains(&CacheNotice::FetchedUpdates { + owner: "owner".to_owned(), + repo: "repo".to_owned(), + }), + "the sweep finished: {notices:?}" + ); + } + #[test] fn a_sweep_says_which_repository_it_is_fetching_and_that_it_finished() { // Python's two `logger.info` lines around the fetch, and both are worth @@ -2567,7 +2720,9 @@ pub(crate) mod tests { .expect("fetched"); assert_eq!(BACKGROUND_FETCH_TIMEOUT, Duration::from_secs(300)); - match fake.only_call() { + // The fetch, which is the call the bound is about. The pack beside it + // carries its own thirty seconds and never the network's budget. + match fake.calls().swap_remove(0) { devlaunch_test_support::Call::Capture(spec) => { assert_eq!(spec.timeout, Some(BACKGROUND_FETCH_TIMEOUT)); } @@ -2873,7 +3028,10 @@ pub(crate) mod tests { .expect("fetched"), Fetched::Fetched ); - assert_eq!(fake.call_count(), 1); + // The fetch and the pack that follows it. A skipped pass below spawns + // neither, which is what makes the pack cost nothing on a pass that did + // not fetch. + assert_eq!(fake.call_count(), 2); // The fetch above wrote `last_fetched`, so the next pass is inside the // interval. @@ -3582,6 +3740,60 @@ pub(crate) mod tests { ); } + #[test] + fn real_git_the_sweep_packs_the_loose_refs_its_own_fetch_just_wrote() { + // Every ref a fetch updates is written as a loose file costing a whole + // filesystem block, and nothing in production ever packed them: `pack-refs + // --auto` is a no-op on the `files` backend and no `gc` runs on the bare. + // The packing belongs on the sweep because the sweep is what makes them -- + // `fetch_all` has exactly one production caller, so "after the fetch" is + // the detached background sweep by construction, inside a lock scope it + // already holds. + let cache = a_cache(); + let remote = a_fixture_remote(cache.dir.path()); + let runner = real_git(); + let manager = a_manager(&cache, Git::new(&runner)); + let mut storage = cache.storage; + manager + .clone_repo(&mut storage, "test", "repo", &remote.url, &mut ignoring()) + .expect("cloned"); + let bare = manager.bare_dir("test", "repo"); + // A fresh clone arrives packed, so the loose refs have to be *made* rather + // than assumed: every ref this fetch updates is a file git writes under + // `refs/`, shadowing the stale line still sitting in `packed-refs`. + let moved = commit_on(&remote.work, "main", "one.txt", "Move main"); + run_git(&remote.work, &["tag", "v1"]); + run_git(&remote.work, &["push", "origin", "v1"]); + + manager + .fetch_repo(&mut storage, "test", "repo", None, &mut ignoring()) + .expect("fetched"); + + assert_eq!( + loose_refs_of(&bare), + Vec::::new(), + "the sweep left its own loose refs behind" + ); + // Not decoration: it is what says the assertion above is about a ref the + // fetch really did write loose. `main` moved and `v1` is new, so a fetch + // that packed nothing would leave both as files and `packed-refs` holding + // the sha from clone time. + let packed = std::fs::read_to_string(bare.join("packed-refs")).expect("a packed-refs"); + assert!( + packed.contains(&format!("{moved} refs/heads/main")), + "packed-refs still holds the pre-fetch main: {packed}" + ); + assert!( + packed.contains("refs/tags/v1"), + "the tag the fetch created is not in packed-refs: {packed}" + ); + assert_eq!( + refs_of(&bare), + ["refs/heads/feature/test", "refs/heads/main", "refs/tags/v1"], + "packing changes the representation and must lose no ref" + ); + } + #[test] fn real_git_leaves_a_record_alone_when_only_the_directory_is_gone() { // A restored backup, a hand-deleted cache, a half-finished `dl --purge`. diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 849ec1dd..21bfa4d9 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -982,6 +982,11 @@ fn cache_notice(notice: &CacheNotice) -> Option { refreshed ({}); it may be behind the remote.", not_refreshed(reason) ), + CacheNotice::RefsNotPacked { + owner, + repo, + reason, + } => format!("Could not pack the refs of {owner}/{repo}: {reason}"), CacheNotice::LfsCacheNotFilled { reason } => { format!("Could not fill the cache's git-lfs store: {reason}") } @@ -3079,6 +3084,25 @@ mod tests { ); } + #[test] + fn a_pack_the_sweep_could_not_do_names_the_repository_and_gits_own_words() { + // The sweep walks every repository in one detached process, so a line that + // did not name one would be unactionable. It is a notice and not an error + // because the fetch beside it succeeded: the cost of the refusal is a + // filesystem block per ref until the next sweep, and nothing else. + let said = |notice: CacheNotice| cache_notice(¬ice).expect("a line"); + + assert_eq!( + said(CacheNotice::RefsNotPacked { + owner: "blooop".to_owned(), + repo: "devlaunch".to_owned(), + reason: "fatal: unable to create 'packed-refs.lock': Permission denied".to_owned(), + }), + "Could not pack the refs of blooop/devlaunch: fatal: unable to create \ + 'packed-refs.lock': Permission denied" + ); + } + #[test] fn the_branch_decision_reads_as_the_four_lines_python_logged() { // `worktree/branch_manager.py` 49/56/62/67. Which of the four happened is an From ad9101f245a3c896428df227cdb1a4de0c01b0b0 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 25 Aug 2026 18:00:38 +0000 Subject: [PATCH 2/5] Add the notice arm to the binary-surface snapshot --- rust/devlaunch-core/public-api.rest.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index 99f4062e..0fcf7134 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -2079,6 +2079,10 @@ pub devlaunch_core::flows::repo_manager::CacheNotice::RefNotFetched::branch: all pub devlaunch_core::flows::repo_manager::CacheNotice::RefNotFetched::owner: alloc::string::String pub devlaunch_core::flows::repo_manager::CacheNotice::RefNotFetched::reason: devlaunch_core::flows::repo_manager::NotRefreshed pub devlaunch_core::flows::repo_manager::CacheNotice::RefNotFetched::repo: alloc::string::String +pub devlaunch_core::flows::repo_manager::CacheNotice::RefsNotPacked +pub devlaunch_core::flows::repo_manager::CacheNotice::RefsNotPacked::owner: alloc::string::String +pub devlaunch_core::flows::repo_manager::CacheNotice::RefsNotPacked::reason: alloc::string::String +pub devlaunch_core::flows::repo_manager::CacheNotice::RefsNotPacked::repo: alloc::string::String pub devlaunch_core::flows::repo_manager::CacheNotice::TrackedFilesNotListed pub devlaunch_core::flows::repo_manager::CacheNotice::TrackedFilesNotListed::reason: alloc::string::String pub devlaunch_core::flows::repo_manager::CacheNotice::WorkspaceCloneRemoved From f103b09d132b261b26656a9a7f883dfbdf4e6687 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 25 Aug 2026 18:02:37 +0000 Subject: [PATCH 3/5] Pin the order the pack refusal is reported in --- rust/devlaunch-core/src/flows/repo_manager.rs | 47 +++++++++++-------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index e5e209ed..008909f6 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -2539,11 +2539,12 @@ pub(crate) mod tests { #[test] fn the_sweep_packs_the_refs_after_the_fetch_and_before_it_stamps_the_record() { - // Order is the whole of the placement argument. Packing after the fetch is - // what makes it the loose refs *this* pass wrote; packing before the stamp - // is what keeps a repository whose pack refused from being stamped as - // though nothing happened -- the stamp lands either way, and the arm below - // is what says so. + // Both halves of the order are load-bearing. After the fetch is what makes + // the refs it packs the ones *this* pass wrote loose. Before the stamp is + // what puts a refusal on stderr ahead of the line that closes the block, + // since `FetchedUpdates` means the fetch and its bookkeeping are both done + // and a warning arriving after it would read as being about the next + // repository the sweep walks. let mut cache = a_cache(); let bare = cache.given_bare_clone("owner", "repo"); cache.given_record("owner", "repo"); @@ -2601,13 +2602,28 @@ pub(crate) mod tests { .fetch_repo(&mut cache.storage, "owner", "repo", None, &mut notices) .expect("a pack that refused is not a fetch that failed"); - assert!( - notices.contains(&CacheNotice::RefsNotPacked { - owner: "owner".to_owned(), - repo: "repo".to_owned(), - reason: "fatal: unable to create 'packed-refs.lock': Permission denied".to_owned(), - }), - "the refusal has to reach a reader, and say which repository: {notices:?}" + // In this order, which is what the placement buys: the refusal names the + // repository and lands *before* the line that closes the block, so a sweep + // walking a whole cache cannot read as though the warning belonged to the + // next repository along. + assert_eq!( + notices, + vec![ + CacheNotice::FetchingUpdates { + owner: "owner".to_owned(), + repo: "repo".to_owned(), + }, + CacheNotice::RefsNotPacked { + owner: "owner".to_owned(), + repo: "repo".to_owned(), + reason: "fatal: unable to create 'packed-refs.lock': Permission denied" + .to_owned(), + }, + CacheNotice::FetchedUpdates { + owner: "owner".to_owned(), + repo: "repo".to_owned(), + }, + ] ); assert!( cache @@ -2618,13 +2634,6 @@ pub(crate) mod tests { .is_some(), "the stamp is about the fetch, and the fetch happened" ); - assert!( - notices.contains(&CacheNotice::FetchedUpdates { - owner: "owner".to_owned(), - repo: "repo".to_owned(), - }), - "the sweep finished: {notices:?}" - ); } #[test] From ec9619aa9be0231fdeae2d045a73cd5e3beddd07 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 25 Aug 2026 18:03:36 +0000 Subject: [PATCH 4/5] Measure what a bare pack-refs leaves behind, rather than assert it --- rust/devlaunch-core/src/clients/git.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rust/devlaunch-core/src/clients/git.rs b/rust/devlaunch-core/src/clients/git.rs index e62df269..240daf21 100644 --- a/rust/devlaunch-core/src/clients/git.rs +++ b/rust/devlaunch-core/src/clients/git.rs @@ -597,9 +597,11 @@ impl<'r> Git<'r> { /// files held 1204 KiB of blocks against a 30 KiB `packed-refs` for all 551, /// and the pack took 23 ms. /// - /// `--all` rather than the default, because the default packs only tags and - /// would leave every head loose, which is the population the sweep actually - /// makes. + /// `--all` rather than the default, and the difference is the whole verb here + /// rather than a nicety: measured on 2.51.1 against a bare holding 301 loose + /// heads and 101 loose tags, a bare `pack-refs` took the tags to zero and left + /// all 301 heads exactly where they were. Heads are the population a broad + /// sweep of a real repository mostly makes. /// /// **Pure, in the sense that decides where this is allowed to live.** Packing /// changes how a ref is stored and not which refs exist, so it can lose no From e4b562213fbb5bb1f0cb5bd941119a5beafb5403 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 25 Aug 2026 18:22:24 +0000 Subject: [PATCH 5/5] State the banked payment as banked, and stop promising a line nobody reads The probe saving is real and nothing collects it: the bare-side reachability guard is decided and unbuilt, and the probe that ships asks the clone. Placement is what carries this call today, so say that in the contract page and at the call site rather than only in the PR. The refusal notice is not a line on stderr. The sweep runs detached with its output discarded, so every notice it raises goes to a null descriptor. Name that, and name what a failing pack costs while it goes unread: loose refs are rewritten in place, so the count stays flat at one sweep's worth rather than growing. --- docs/cleanup.md | 37 ++++++++++++------- rust/devlaunch-core/src/flows/repo_manager.rs | 12 ++++-- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/docs/cleanup.md b/docs/cleanup.md index 42a04d3a..bcd57816 100644 --- a/docs/cleanup.md +++ b/docs/cleanup.md @@ -364,25 +364,36 @@ repository is. Measured across ten real remotes with `git ls-remote`, repositories holds something like 30 MB to 60 MB of loose refs, a couple of percent of one bare's own size. **Disk is not the reason this is here.** -The reasons it is here are placement and a second payment. The broad sweep is the -only thing in `dl` that fetches every head and tag, so it is the only thing that -makes loose refs in quantity, and it already holds the repository's lock while it -does. Packing there costs one more bounded `git` call in a scope that just spent -its whole network budget, and it happens only on a pass that actually fetched. And -the guards that decide whether a clone is safe to remove ask the bare a -reachability question that walks every ref; with the refs packed that walk reads -one file instead of thousands. Measured on git 2.51.1 over a bare of 551 refs with -301 of them loose, the probe fell from 5.3 ms to 2.8 ms, the loose files held -1204 KiB of blocks against a 30 KiB `packed-refs` for all 551, and the pack itself -took 23 ms. +What carries it is placement. The broad sweep is the only thing in `dl` that +fetches every head and tag, so it is the only thing that makes loose refs in +quantity, and it already holds the repository's lock while it does. Packing there +costs one more bounded `git` call in a scope that just spent its whole network +budget, and it happens only on a pass that actually fetched. Measured on git +2.51.1 over a bare of 551 refs, 301 of them loose, those files held 1204 KiB of +blocks against a 30 KiB `packed-refs` for all 551, and the pack itself took 23 ms. + +There is a second payment, and it is banked rather than collected. A guard that +walks every ref on the bare to decide whether a clone is safe to remove reads one +file instead of thousands once the refs are packed, and such a probe measured +2.8 ms against 5.3 ms on that same bare. **No shipped code collects that yet.** +The bare-side reachability guard is decided and not built, and what ships today +asks the clone instead. So the saving is a reason to keep this once that guard +arrives, and it is not a reason this is here now. **A pack that refuses is not a fetch that failed.** The fetch is the point of the -sweep and the packing is the optional half, so a refusal is a line on stderr -naming the repository and git's own words, the record's freshness stamp still +sweep and the packing is the optional half, so a refusal becomes a notice +carrying the repository and git's own words, the record's freshness stamp still lands, and the next sweep tries again. Withholding the stamp would make every later pass re-fetch the whole repository forever on account of a representation change that did not come off. +That notice reaches nobody today, and the honest reading of why is that the sweep +runs detached with its output discarded, so every notice it raises goes to a null +descriptor and this is simply the first one that anybody would want to read. What +a refusal costs while it stays unread is bounded: loose refs are one file per ref +rewritten in place rather than appended, so a pack that keeps failing holds the +ref count flat at what one sweep writes instead of growing it. + **Packing does not change what a later prune may delete.** A ref the remote retracts is removed whether it was loose or packed: git rewrites `packed-refs` through the same ref transaction that unlinks a loose file, and a ref that was diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index 008909f6..6c300246 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -1414,10 +1414,14 @@ impl<'r> RepositoryManager<'r> { // `packed-refs`. Here rather than anywhere else because `fetch_all` has // exactly one production caller, so "after the fetch" *is* the detached // background sweep by construction, in the repo-lock scope it already - // holds; and because packing pays a second time in probe latency, since - // the reachability question asked of the bare walks every ref and today - // reads one file per ref to do it (measured on git 2.51.1, 5.3 ms against - // 2.8 ms over 551 refs, 301 of them loose). + // holds. + // + // A second payment is banked here rather than collected: a guard that + // walks every ref on the bare reads one file instead of thousands once + // they are packed (measured on git 2.51.1, 2.8 ms against 5.3 ms over + // 551 refs, 301 of them loose). Nothing collects it yet. That guard is + // decided and unbuilt, and the probe that ships asks the *clone*, not + // the bare. Placement is what carries this call today. // // **A refusal here is not a failed fetch.** The fetch happened, the cache // is fresh, and the stamp below has to land or the sweep re-fetches this