From cdeda94d52df2e194c8153be2b59ad248bdd2080 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Wed, 30 Sep 2026 11:17:19 -0700 Subject: [PATCH 1/3] Complete protocol v2 object-info support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The object-info implementation was not compatible with the Git client: its capability omitted the supported attribute, missing objects used an invalid numeric sentinel, requests were unbounded, and excluded hosts could start object synchronization. ### What? Advertise `object-info=size`, return protocol-defined empty values for missing objects, cap requests at 1,024 OIDs before sync, and apply the existing placement refusal before any object work. Preserve manifest-versioned positive and negative caching across local and remote-served object access. The healthy request path remains one conditional manifest GET. Warm cache hits and local header lookups add zero bucket reads; remote misses add only the pack range reads required by `RemotePacks::header`. ## Test Plan ✅ `cargo check -p walgit-server` ✅ `cargo test -p walgit-git read_object_info_args_reads_size_and_oids_after_delimiter` ✅ `cargo test -p walgit-server --test e2e info_refs_v2_advertises_capabilities -- --exact` ✅ `cargo test -p walgit-server --test e2e object_info_v2` ✅ `cargo test -p walgit-server --test maintain host_excluded_from_serving_a_repo_refuses_object_work_with_503 -- --exact` ✅ `cargo test -p walgit-server --test e2e fetch_from_front_that_serves_the_base_remotely -- --exact` ✅ `cargo clippy -p walgit-server --all-targets -- -D warnings` ✅ `cargo fmt --all -- --check` ✅ `git diff --check` ## Issue Closes #80 (cherry picked from commit c51b778d1cc1d6ca2100fbfa75cec842af57d21c) --- crates/walgit-git/src/pkt.rs | 43 +++++++- crates/walgit-server/src/cache.rs | 61 +++++++++++ crates/walgit-server/src/smart.rs | 92 ++++++++++++++-- crates/walgit-server/tests/e2e.rs | 146 +++++++++++++++++++++++++ crates/walgit-server/tests/maintain.rs | 26 +++++ docs/ROUNDTRIPS.md | 1 + 6 files changed, 358 insertions(+), 11 deletions(-) diff --git a/crates/walgit-git/src/pkt.rs b/crates/walgit-git/src/pkt.rs index ab7c3a89..9adf8ea2 100644 --- a/crates/walgit-git/src/pkt.rs +++ b/crates/walgit-git/src/pkt.rs @@ -358,19 +358,43 @@ pub async fn read_ls_refs_args( /// Arguments for the v2 `object-info` command. #[derive(Debug, Default, Clone)] pub struct ObjectInfoRequest { + pub size: bool, pub oids: Vec, } pub fn parse_object_info(cmd: &V2Command) -> ObjectInfoRequest { let mut req = ObjectInfoRequest::default(); for a in &cmd.args { - if let Some(o) = a.strip_prefix("oid ") { - req.oids.push(o.to_string()); - } + parse_object_info_line(&mut req, a); } req } +fn parse_object_info_line(req: &mut ObjectInfoRequest, line: &str) { + if line == "size" { + req.size = true; + } else if let Some(oid) = line.strip_prefix("oid ") { + req.oids.push(oid.to_string()); + } +} + +/// Read the command-specific section after the v2 command delimiter. +pub async fn read_object_info_args( + mut reader: R, + mut req: ObjectInfoRequest, +) -> Result { + loop { + match read_pkt_line(&mut reader).await? { + Some(PktLine::Data(data)) => { + parse_object_info_line(&mut req, String::from_utf8_lossy(&data).trim_end()); + } + Some(PktLine::Delim) => {} + Some(PktLine::Flush | PktLine::ResponseEnd) | None => break, + } + } + Ok(req) +} + fn io_to_git(e: std::io::Error) -> GitError { GitError::Io(e) } @@ -447,4 +471,17 @@ mod tests { assert_eq!(req.prefixes, vec!["refs/heads/ref-199"]); assert!(req.peel); } + + #[tokio::test] + async fn read_object_info_args_reads_size_and_oids_after_delimiter() { + let mut body = Vec::new(); + encode_data(&mut body, b"size\n"); + encode_data(&mut body, b"oid 1111111111111111111111111111111111111111\n"); + encode_flush(&mut body); + let req = read_object_info_args(std::io::Cursor::new(body), ObjectInfoRequest::default()) + .await + .unwrap(); + assert!(req.size); + assert_eq!(req.oids, vec!["1111111111111111111111111111111111111111"]); + } } diff --git a/crates/walgit-server/src/cache.rs b/crates/walgit-server/src/cache.rs index 36346480..bdefa21e 100644 --- a/crates/walgit-server/src/cache.rs +++ b/crates/walgit-server/src/cache.rs @@ -245,6 +245,65 @@ impl RefIndexCache { } } +// --------------------------------------------------------------------------- +// Object information (protocol v2) +// --------------------------------------------------------------------------- + +#[derive(Clone, PartialEq, Eq, Hash)] +struct ObjectInfoKey { + repo: String, + version: String, + oid: gix_hash::ObjectId, +} + +/// Bounded cache of object sizes. The manifest version keeps negative results +/// from surviving a push that makes an object visible. +#[derive(Clone)] +pub struct ObjectInfoCache { + inner: Cache>, +} + +impl ObjectInfoCache { + pub fn new(max_entries: usize) -> Self { + Self { + inner: Cache::builder().max_capacity(max_entries as u64).build(), + } + } + + fn key(repo: &str, version: Option<&Version>, oid: gix_hash::ObjectId) -> ObjectInfoKey { + ObjectInfoKey { + repo: repo.to_string(), + version: version.map(|v| v.as_str().to_string()).unwrap_or_default(), + oid, + } + } + + pub fn get( + &self, + repo: &str, + version: Option<&Version>, + oid: gix_hash::ObjectId, + ) -> Option> { + let value = self.inner.get(&Self::key(repo, version, oid)); + if value.is_some() { + metrics::counter!("walgit_cache_object_info_hit").increment(1); + } else { + metrics::counter!("walgit_cache_object_info_miss").increment(1); + } + value + } + + pub fn insert( + &self, + repo: &str, + version: Option<&Version>, + oid: gix_hash::ObjectId, + size: Option, + ) { + self.inner.insert(Self::key(repo, version, oid), size); + } +} + // --------------------------------------------------------------------------- // ServerCaches — aggregate held by AppState // --------------------------------------------------------------------------- @@ -254,6 +313,7 @@ impl RefIndexCache { pub struct ServerCaches { pub ref_advert: RefAdvertCache, pub ref_index: RefIndexCache, + pub object_info: ObjectInfoCache, /// Rendered sha-addressed web API JSON (immutable): key = repo\0kind\0sha\0path. pub api_immutable: Cache, } @@ -263,6 +323,7 @@ impl ServerCaches { Self { ref_advert: RefAdvertCache::new(cfg.cache.ref_advert_entries), ref_index: RefIndexCache::new(cfg.cache.ref_advert_entries.max(64)), + object_info: ObjectInfoCache::new(cfg.cache.object_info_entries), api_immutable: Cache::builder() .max_capacity(64 * 1024 * 1024) .weigher(|k: &String, v: &bytes::Bytes| { diff --git a/crates/walgit-server/src/smart.rs b/crates/walgit-server/src/smart.rs index be53d30e..49a4ece2 100644 --- a/crates/walgit-server/src/smart.rs +++ b/crates/walgit-server/src/smart.rs @@ -18,6 +18,8 @@ use crate::repo::RepoRoute; use crate::stream::{VecWriter, body_to_async_read, maybe_gunzip, write_body_pipe}; use tracing::Instrument; +const MAX_OBJECT_INFO_OIDS: usize = 1024; + /// Cache-control headers for smart endpoints (info/refs and pkt responses). fn no_cache_headers() -> [(axum::http::HeaderName, &'static str); 3] { [ @@ -151,6 +153,7 @@ async fn v2_capability_advert( // packfile section: auth, WAL sync, materialization progress. Both engines frame their sections that way. fetch.push_str(" sideband-all packfile-uris packfile-indexes"); pktline::encode_text(buf, &format!("{fetch}\n")); + pktline::encode_text(buf, "object-info=size\n"); pktline::encode_text(buf, "server-option\n"); let fmt = match handle.local().object_format() { walgit_git::ObjectFormat::Sha1 => "sha1", @@ -338,16 +341,89 @@ async fn upload_pack_v2( )) } "object-info" => { - let _guard = handle.sync().await.map_err(wal_err)?; + if let Some(r) = not_served_here(st, &route.id, "git-upload-pack").await { + return Ok(r); + } let req = walgit_git::pkt::parse_object_info(&cmd); + let req = walgit_git::pkt::read_object_info_args(reader, req) + .await + .map_err(git_err)?; + if !req.size { + return Err(ApiError::BadRequest( + "object-info currently requires the size attribute".into(), + )); + } + if req.oids.len() > MAX_OBJECT_INFO_OIDS { + return Ok(git_err_response( + "git-upload-pack", + &format!( + "walgit: object-info request has {} object ids; maximum is {MAX_OBJECT_INFO_OIDS}", + req.oids.len() + ), + )); + } + let (_guard, access) = handle.sync_objects().await.map_err(wal_err)?; + let repo_key = route.id.to_string(); + let version = handle.manifest_version(); + let oids: Vec> = req + .oids + .iter() + .map(|hex| gix_hash::ObjectId::from_hex(hex.as_bytes()).ok()) + .collect(); + let sizes = match access { + walgit_wal::ObjectAccess::Local => { + let repo = handle.local().gix(); + oids.iter() + .map(|oid| { + let Some(oid) = oid else { return None }; + if let Some(size) = + st.caches.object_info.get(&repo_key, version.as_ref(), *oid) + { + return size; + } + let size = gix_object::FindHeader::try_header(&repo.objects, oid) + .ok() + .flatten() + .map(|header| header.size); + st.caches + .object_info + .insert(&repo_key, version.as_ref(), *oid, size); + size + }) + .collect() + } + walgit_wal::ObjectAccess::Remote(packs) => { + let results = futures::future::join_all(oids.iter().map(|oid| { + let packs = packs.clone(); + let cache = st.caches.object_info.clone(); + let repo_key = repo_key.clone(); + let version = version.clone(); + async move { + let Some(oid) = oid else { return Ok(None) }; + if let Some(size) = cache.get(&repo_key, version.as_ref(), *oid) { + return Ok(size); + } + let size = packs.header(oid).await?.map(|(_, size)| size); + cache.insert(&repo_key, version.as_ref(), *oid, size); + Ok::, walgit_wal::WalError>(size) + } + })) + .await; + results + .into_iter() + .collect::, _>>() + .map_err(wal_err)? + } + }; let mut sizes_buf = Vec::with_capacity(256); - let repo = handle.local().gix(); - for hex in &req.oids { - let size = gix_hash::ObjectId::from_hex(hex.as_bytes()) - .ok() - .and_then(|oid| repo.find_object(oid).ok()) - .map_or(-1, |o| o.data.len() as i64); - pktline::encode_text(&mut sizes_buf, &format!("size {size}\n")); + pktline::encode_text(&mut sizes_buf, "size\n"); + for (hex, size) in req.oids.iter().zip(sizes) { + match size { + Some(size) => { + pktline::encode_text(&mut sizes_buf, &format!("{hex} {size}\n")); + } + None => pktline::encode_text(&mut sizes_buf, &format!("{hex} \n")), + } } pktline::encode_flush(&mut sizes_buf); Ok(text_response( diff --git a/crates/walgit-server/tests/e2e.rs b/crates/walgit-server/tests/e2e.rs index 7cce50a3..ba7f0628 100644 --- a/crates/walgit-server/tests/e2e.rs +++ b/crates/walgit-server/tests/e2e.rs @@ -16,6 +16,36 @@ use std::io::Write; use std::process::{Command, Stdio}; use std::time::Instant; +async fn request_object_info( + server: &Server, + owner: &str, + repo: &str, + oids: &[String], +) -> anyhow::Result { + let mut body = Vec::new(); + walgit_server::pktline::encode_text(&mut body, "command=object-info\n"); + walgit_server::pktline::encode_delim(&mut body); + walgit_server::pktline::encode_text(&mut body, "size\n"); + for oid in oids { + walgit_server::pktline::encode_text(&mut body, &format!("oid {oid}\n")); + } + walgit_server::pktline::encode_flush(&mut body); + + Ok(reqwest::Client::new() + .post(format!( + "{}/{owner}/{repo}.git/git-upload-pack", + server.base_url + )) + .header("Git-Protocol", "version=2") + .header( + reqwest::header::CONTENT_TYPE, + "application/x-git-upload-pack-request", + ) + .body(body) + .send() + .await?) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn info_refs_v2_advertises_capabilities() -> TestResult { let server = Server::start().await?; @@ -30,10 +60,113 @@ async fn info_refs_v2_advertises_capabilities() -> TestResult { assert!(out.contains("version 2")); assert!(out.contains("ls-refs=unborn")); assert!(out.contains("fetch=shallow wait-for-done")); + assert!(out.contains("object-info=size")); + assert!(!out.contains("object-info\n"), "{out}"); assert!(!out.contains("bundle-uri"), "{out}"); Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn object_info_v2_reports_object_size() -> TestResult { + let server = Server::start().await?; + server.put_repo("t", "object-info").await?; + let src = TestRepo::synthetic(1, 1)?; + git_in( + &src, + &[ + "remote", + "add", + "origin", + &server.repo_url("t", "object-info"), + ], + )?; + git_in(&src, &["push", "-q", "origin", "main"])?; + let oid = git_in(&src, &["rev-parse", "main"])?; + let size = git_in(&src, &["cat-file", "-s", oid.trim()])?; + + let response = + request_object_info(&server, "t", "object-info", &[oid.trim().to_owned()]).await?; + assert!(response.status().is_success()); + let response = response.bytes().await?; + let expected = format!("{} {}\n", oid.trim(), size.trim()); + assert!( + response + .windows(expected.len()) + .any(|window| window == expected.as_bytes()), + "{}", + String::from_utf8_lossy(&response) + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn object_info_v2_reports_missing_object_with_empty_size() -> TestResult { + let server = Server::start().await?; + server.put_repo("t", "object-info-missing").await?; + let missing = "0000000000000000000000000000000000000000".to_string(); + + let response = request_object_info( + &server, + "t", + "object-info-missing", + std::slice::from_ref(&missing), + ) + .await?; + assert!(response.status().is_success()); + let response = response.bytes().await?; + let expected = format!("{missing} \n"); + assert!( + response + .windows(expected.len()) + .any(|window| window == expected.as_bytes()), + "{}", + String::from_utf8_lossy(&response) + ); + assert!(!response.windows(3).any(|window| window == b"-1\n")); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn object_info_v2_bounds_oid_count_before_sync() -> TestResult { + let server = Server::start().await?; + server.put_repo("t", "object-info-limit").await?; + let src = TestRepo::synthetic(1, 1)?; + git_in( + &src, + &[ + "remote", + "add", + "origin", + &server.repo_url("t", "object-info-limit"), + ], + )?; + git_in(&src, &["push", "-q", "origin", "main"])?; + let oid = git_in(&src, &["rev-parse", "main"])?.trim().to_owned(); + + let accepted = vec![oid.clone(); 1024]; + let response = request_object_info(&server, "t", "object-info-limit", &accepted).await?; + assert!(response.status().is_success()); + let body = response.bytes().await?; + assert!(!String::from_utf8_lossy(&body).contains("ERR ")); + + let refused = vec![oid; 1025]; + let response = request_object_info(&server, "t", "object-info-limit", &refused).await?; + assert!(response.status().is_success()); + let body = response.text().await?; + assert!( + body.contains("ERR walgit: object-info request has 1025 object ids"), + "{body}" + ); + let tasks = server + .get_text("/t/object-info-limit/api/tasks", &[]) + .await?; + assert!( + !tasks.contains("materialize") && !tasks.contains("remote-index"), + "oversized object-info did not start object sync: {tasks}" + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn push_clone_roundtrip_v2() -> TestResult { let server = Server::start().await?; @@ -1253,6 +1386,19 @@ async fn fetch_from_front_that_serves_the_base_remotely() -> TestResult { c.cache.max_bytes = bytesize::ByteSize::b(remote_pack.pack_size / 2); }) .await?; + let base_tip_size = git_in(&src, &["cat-file", "-s", &base_tip])?; + let response = + request_object_info(&small, "t", "rbase", std::slice::from_ref(&base_tip)).await?; + assert!(response.status().is_success()); + let response = response.bytes().await?; + let expected = format!("{} {}\n", base_tip, base_tip_size.trim()); + assert!( + response + .windows(expected.len()) + .any(|window| window == expected.as_bytes()), + "{}", + String::from_utf8_lossy(&response) + ); let url = small.repo_url("t", "rbase"); git_in(clone.path(), &["remote", "set-url", "origin", &url])?; let fetch = std::process::Command::new("git") diff --git a/crates/walgit-server/tests/maintain.rs b/crates/walgit-server/tests/maintain.rs index 70242f34..f7df8ca7 100644 --- a/crates/walgit-server/tests/maintain.rs +++ b/crates/walgit-server/tests/maintain.rs @@ -671,6 +671,32 @@ async fn host_excluded_from_serving_a_repo_refuses_object_work_with_503() -> any ); let text = r.text().await?; assert!(text.contains("ERR walgit: acme/big is served by"), "{text}"); + + // Object-info is object work too and must refuse placement before sync. + let mut body = Vec::new(); + walgit_server::pktline::encode_text(&mut body, "command=object-info\n"); + walgit_server::pktline::encode_delim(&mut body); + walgit_server::pktline::encode_text(&mut body, "size\n"); + walgit_server::pktline::encode_text( + &mut body, + "oid 0000000000000000000000000000000000000000\n", + ); + walgit_server::pktline::encode_flush(&mut body); + let r = client + .post(format!("{}/acme/big.git/git-upload-pack", front.base_url)) + .header("Git-Protocol", "version=2") + .header("Content-Type", "application/x-git-upload-pack-request") + .body(body) + .send() + .await?; + assert_eq!(r.status(), reqwest::StatusCode::SERVICE_UNAVAILABLE); + assert_eq!( + r.headers().get("retry-after").map(|v| v.to_str().unwrap()), + Some("15") + ); + let text = r.text().await?; + assert!(text.contains("ERR walgit: acme/big is served by"), "{text}"); + let tasks = step!("tasks", front.get_text("/acme/big/api/tasks", &[]))?; assert!( !tasks.contains("materialize") && !tasks.contains("remote-index"), diff --git a/docs/ROUNDTRIPS.md b/docs/ROUNDTRIPS.md index 7b7ebc6e..8d5d3824 100644 --- a/docs/ROUNDTRIPS.md +++ b/docs/ROUNDTRIPS.md @@ -47,6 +47,7 @@ right shape. This document is the thinking tool; apply it to every protocol chan | Conserving lifecycle | acquire lease → captured refs/full sync → each output publishes additively → final seal (lease heartbeat → refs revalidation → immutable log PUT → manifest CAS) | one additive publication per new physical checksum and one final CAS; heartbeat GET/CAS calls and full-sync downloads are additional maintenance costs; no change to ordinary push/read budgets | `pack_lifecycle.rs`, `classification.rs::seal_pack_replacement` | | Coverage repair | captured snapshot Create → bounded batches of classification CAS → final certificate CAS | snapshot equality GET only on Create conflict; up to 128 classifications per batch; every certificate rechecks current policy and live membership | `pack_lifecycle.rs::repair_metadata` | | Native URI selection | existing fetch sync → exact coverage snapshot GET | normally +1 GET, shared maximum 3 attempts across all groups; current manifest/ref view capture adds no GET; optional index trailers are local bounded reads | `packfile_uri.rs::select`, `snapshots.rs::fetch_view` | +| Protocol v2 object-info | 1 conditional manifest GET; warm size cache adds 0 reads; a remote miss performs the object's pack range reads, with requested objects in parallel | no LIST or pack materialization; placement and requests above 1,024 OIDs are refused before sync; local objects use header lookup and remote delta sizes may require delta/base headers | `smart.rs`, `cache.rs`, `remote.rs::header` | | Static pack/index GET | manifest revalidation → object HEAD → body/range GET | 3 warm requests; conditional 304/HEAD omits the body GET; cold registry open may add its normal snapshot/tail reads; no pack materialization or LIST | `packfile_uri.rs::get`, `static_object.rs` | | Checkpoint | 1 cond GET (freshness) → refs PUT ∥ checkpoint PUT → manifest CAS | 3 rounds, 4 requests (was 6/6 until 2026-08-22: a bundle-list GET before the checkpoint PUT and a log GET for provenance times sat in the chain; times now come from the writer's own applied state, `bundle_key` is no longer looked up) | `checkpoint.rs` | | Validated remote index admission | warm valid index: 0 store requests; absent/corrupt index: index GET (HEAD first only for old descriptors with unknown size), at most four indexes downloading concurrently | unchanged healthy store depth; corruption now triggers repair instead of admitting bad evidence. Checksums, lock, mmap and cleanup are local work; no LIST. Per-index checksum CPU cost occurs when constructing a new remote inventory, not per object lookup. | `index_cache.rs`, `remote.rs` | From 149ef705b03c2a2c333d5db60d74644ab9bdfde6 Mon Sep 17 00:00:00 2001 From: Guillaume Dumont Date: Wed, 7 Oct 2026 08:48:02 +0200 Subject: [PATCH 2/3] object-info: bound remote reads, never cache a miss, look up off the runtime Follow-ups from the review of the object-info change: - Remote pack header reads for one request run at most 16 at a time, in request order, instead of up to 1,024 at once. - Only found sizes are cached. A miss cached under a manifest version read after the sync could hide an object pushed in between; misses are cheap to recompute. - Local header lookups run on a blocking thread rather than on the async runtime. Adds an end to end test that an object pushed after a lookup missed it is reported with its size. --- crates/walgit-server/src/smart.rs | 87 ++++++++++++++++++------------- crates/walgit-server/tests/e2e.rs | 70 +++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 37 deletions(-) diff --git a/crates/walgit-server/src/smart.rs b/crates/walgit-server/src/smart.rs index 49a4ece2..8fabbae8 100644 --- a/crates/walgit-server/src/smart.rs +++ b/crates/walgit-server/src/smart.rs @@ -19,6 +19,8 @@ use crate::stream::{VecWriter, body_to_async_read, maybe_gunzip, write_body_pipe use tracing::Instrument; const MAX_OBJECT_INFO_OIDS: usize = 1024; +/// Remote pack header reads one object-info request may have in flight. +const OBJECT_INFO_PARALLEL_READS: usize = 16; /// Cache-control headers for smart endpoints (info/refs and pkt responses). fn no_cache_headers() -> [(axum::http::HeaderName, &'static str); 3] { @@ -370,48 +372,59 @@ async fn upload_pack_v2( .iter() .map(|hex| gix_hash::ObjectId::from_hex(hex.as_bytes()).ok()) .collect(); + // Only found sizes are cached: a miss cached under a version read + // after the sync could hide an object pushed meanwhile. let sizes = match access { walgit_wal::ObjectAccess::Local => { - let repo = handle.local().gix(); - oids.iter() - .map(|oid| { - let Some(oid) = oid else { return None }; - if let Some(size) = - st.caches.object_info.get(&repo_key, version.as_ref(), *oid) - { - return size; - } - let size = gix_object::FindHeader::try_header(&repo.objects, oid) - .ok() - .flatten() - .map(|header| header.size); - st.caches - .object_info - .insert(&repo_key, version.as_ref(), *oid, size); - size - }) - .collect() + let local = handle.local().clone(); + let cache = st.caches.object_info.clone(); + let repo_key = repo_key.clone(); + let version = version.clone(); + tokio::task::spawn_blocking(move || { + let repo = local.gix(); + oids.iter() + .map(|oid| { + let oid = (*oid)?; + if let Some(size) = cache.get(&repo_key, version.as_ref(), oid) { + return size; + } + let size = gix_object::FindHeader::try_header(&repo.objects, &oid) + .ok() + .flatten() + .map(|header| header.size); + if size.is_some() { + cache.insert(&repo_key, version.as_ref(), oid, size); + } + size + }) + .collect::>() + }) + .await + .map_err(|e| ApiError::Internal(format!("object-info: {e}")))? } walgit_wal::ObjectAccess::Remote(packs) => { - let results = futures::future::join_all(oids.iter().map(|oid| { - let packs = packs.clone(); - let cache = st.caches.object_info.clone(); - let repo_key = repo_key.clone(); - let version = version.clone(); - async move { - let Some(oid) = oid else { return Ok(None) }; - if let Some(size) = cache.get(&repo_key, version.as_ref(), *oid) { - return Ok(size); + use futures::stream::{StreamExt, TryStreamExt}; + futures::stream::iter(oids) + .map(|oid| { + let packs = packs.clone(); + let cache = st.caches.object_info.clone(); + let repo_key = repo_key.clone(); + let version = version.clone(); + async move { + let Some(oid) = oid else { return Ok(None) }; + if let Some(size) = cache.get(&repo_key, version.as_ref(), oid) { + return Ok(size); + } + let size = packs.header(&oid).await?.map(|(_, size)| size); + if size.is_some() { + cache.insert(&repo_key, version.as_ref(), oid, size); + } + Ok::, walgit_wal::WalError>(size) } - let size = packs.header(oid).await?.map(|(_, size)| size); - cache.insert(&repo_key, version.as_ref(), *oid, size); - Ok::, walgit_wal::WalError>(size) - } - })) - .await; - results - .into_iter() - .collect::, _>>() + }) + .buffered(OBJECT_INFO_PARALLEL_READS) + .try_collect::>() + .await .map_err(wal_err)? } }; diff --git a/crates/walgit-server/tests/e2e.rs b/crates/walgit-server/tests/e2e.rs index ba7f0628..b1089e96 100644 --- a/crates/walgit-server/tests/e2e.rs +++ b/crates/walgit-server/tests/e2e.rs @@ -126,6 +126,76 @@ async fn object_info_v2_reports_missing_object_with_empty_size() -> TestResult { Ok(()) } +/// An object pushed after a lookup missed it is reported with its size. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn object_info_v2_finds_an_object_pushed_after_a_miss() -> TestResult { + let server = Server::start().await?; + server.put_repo("t", "object-info-later").await?; + let src = TestRepo::synthetic(1, 1)?; + git_in( + &src, + &[ + "remote", + "add", + "origin", + &server.repo_url("t", "object-info-later"), + ], + )?; + git_in(&src, &["push", "-q", "origin", "main"])?; + git_in( + &src, + &[ + "-c", + "user.name=t", + "-c", + "user.email=t@example.com", + "commit", + "-q", + "--allow-empty", + "-m", + "later", + ], + )?; + let oid = git_in(&src, &["rev-parse", "main"])?.trim().to_owned(); + let size = git_in(&src, &["cat-file", "-s", &oid])?.trim().to_owned(); + + let before = request_object_info( + &server, + "t", + "object-info-later", + std::slice::from_ref(&oid), + ) + .await? + .bytes() + .await?; + let missing = format!("{oid} \n"); + assert!( + before + .windows(missing.len()) + .any(|w| w == missing.as_bytes()), + "{}", + String::from_utf8_lossy(&before) + ); + + git_in(&src, &["push", "-q", "origin", "main"])?; + let after = request_object_info( + &server, + "t", + "object-info-later", + std::slice::from_ref(&oid), + ) + .await? + .bytes() + .await?; + let found = format!("{oid} {size}\n"); + assert!( + after.windows(found.len()).any(|w| w == found.as_bytes()), + "{}", + String::from_utf8_lossy(&after) + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn object_info_v2_bounds_oid_count_before_sync() -> TestResult { let server = Server::start().await?; From 0d5696d8ec817b2a046e10762d5b1ecea737792d Mon Sep 17 00:00:00 2001 From: Guillaume Dumont Date: Wed, 7 Oct 2026 11:04:47 +0200 Subject: [PATCH 3/3] object-info: accept up to 10,000 object ids per request git sends up to 10,000 object ids in one object-info request (cat-file remote-object-info, MAX_ALLOWED_OBJ_LIMIT), and the protocol has no way to advertise a lower limit, so a cap of 1,024 made ordinary clients fail and fall back to downloading objects. The cap stays a guard against unbounded requests: remote reads are still at most 16 at a time and local lookups run on a blocking thread. --- crates/walgit-server/src/smart.rs | 4 +++- crates/walgit-server/tests/e2e.rs | 6 +++--- docs/ROUNDTRIPS.md | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/walgit-server/src/smart.rs b/crates/walgit-server/src/smart.rs index 8fabbae8..0c168d9a 100644 --- a/crates/walgit-server/src/smart.rs +++ b/crates/walgit-server/src/smart.rs @@ -18,7 +18,9 @@ use crate::repo::RepoRoute; use crate::stream::{VecWriter, body_to_async_read, maybe_gunzip, write_body_pipe}; use tracing::Instrument; -const MAX_OBJECT_INFO_OIDS: usize = 1024; +/// git's own client sends up to 10,000 object ids per request (`cat-file` +/// remote-object-info); the protocol cannot advertise a smaller limit. +const MAX_OBJECT_INFO_OIDS: usize = 10_000; /// Remote pack header reads one object-info request may have in flight. const OBJECT_INFO_PARALLEL_READS: usize = 16; diff --git a/crates/walgit-server/tests/e2e.rs b/crates/walgit-server/tests/e2e.rs index b1089e96..b7ae9a1c 100644 --- a/crates/walgit-server/tests/e2e.rs +++ b/crates/walgit-server/tests/e2e.rs @@ -213,18 +213,18 @@ async fn object_info_v2_bounds_oid_count_before_sync() -> TestResult { git_in(&src, &["push", "-q", "origin", "main"])?; let oid = git_in(&src, &["rev-parse", "main"])?.trim().to_owned(); - let accepted = vec![oid.clone(); 1024]; + let accepted = vec![oid.clone(); 10_000]; let response = request_object_info(&server, "t", "object-info-limit", &accepted).await?; assert!(response.status().is_success()); let body = response.bytes().await?; assert!(!String::from_utf8_lossy(&body).contains("ERR ")); - let refused = vec![oid; 1025]; + let refused = vec![oid; 10_001]; let response = request_object_info(&server, "t", "object-info-limit", &refused).await?; assert!(response.status().is_success()); let body = response.text().await?; assert!( - body.contains("ERR walgit: object-info request has 1025 object ids"), + body.contains("ERR walgit: object-info request has 10001 object ids"), "{body}" ); let tasks = server diff --git a/docs/ROUNDTRIPS.md b/docs/ROUNDTRIPS.md index 8d5d3824..bcdee56e 100644 --- a/docs/ROUNDTRIPS.md +++ b/docs/ROUNDTRIPS.md @@ -47,7 +47,7 @@ right shape. This document is the thinking tool; apply it to every protocol chan | Conserving lifecycle | acquire lease → captured refs/full sync → each output publishes additively → final seal (lease heartbeat → refs revalidation → immutable log PUT → manifest CAS) | one additive publication per new physical checksum and one final CAS; heartbeat GET/CAS calls and full-sync downloads are additional maintenance costs; no change to ordinary push/read budgets | `pack_lifecycle.rs`, `classification.rs::seal_pack_replacement` | | Coverage repair | captured snapshot Create → bounded batches of classification CAS → final certificate CAS | snapshot equality GET only on Create conflict; up to 128 classifications per batch; every certificate rechecks current policy and live membership | `pack_lifecycle.rs::repair_metadata` | | Native URI selection | existing fetch sync → exact coverage snapshot GET | normally +1 GET, shared maximum 3 attempts across all groups; current manifest/ref view capture adds no GET; optional index trailers are local bounded reads | `packfile_uri.rs::select`, `snapshots.rs::fetch_view` | -| Protocol v2 object-info | 1 conditional manifest GET; warm size cache adds 0 reads; a remote miss performs the object's pack range reads, with requested objects in parallel | no LIST or pack materialization; placement and requests above 1,024 OIDs are refused before sync; local objects use header lookup and remote delta sizes may require delta/base headers | `smart.rs`, `cache.rs`, `remote.rs::header` | +| Protocol v2 object-info | 1 conditional manifest GET; warm size cache adds 0 reads; a remote miss performs the object's pack range reads, with requested objects in parallel | no LIST or pack materialization; placement and requests above 10,000 OIDs are refused before sync; local objects use header lookup and remote delta sizes may require delta/base headers | `smart.rs`, `cache.rs`, `remote.rs::header` | | Static pack/index GET | manifest revalidation → object HEAD → body/range GET | 3 warm requests; conditional 304/HEAD omits the body GET; cold registry open may add its normal snapshot/tail reads; no pack materialization or LIST | `packfile_uri.rs::get`, `static_object.rs` | | Checkpoint | 1 cond GET (freshness) → refs PUT ∥ checkpoint PUT → manifest CAS | 3 rounds, 4 requests (was 6/6 until 2026-08-22: a bundle-list GET before the checkpoint PUT and a log GET for provenance times sat in the chain; times now come from the writer's own applied state, `bundle_key` is no longer looked up) | `checkpoint.rs` | | Validated remote index admission | warm valid index: 0 store requests; absent/corrupt index: index GET (HEAD first only for old descriptors with unknown size), at most four indexes downloading concurrently | unchanged healthy store depth; corruption now triggers repair instead of admitting bad evidence. Checksums, lock, mmap and cleanup are local work; no LIST. Per-index checksum CPU cost occurs when constructing a new remote inventory, not per object lookup. | `index_cache.rs`, `remote.rs` |