From c51b778d1cc1d6ca2100fbfa75cec842af57d21c Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Wed, 30 Sep 2026 11:17:19 -0700 Subject: [PATCH] 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 --- 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 ca89d49c..2bad6b01 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", @@ -333,16 +336,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 e37eea27..6225440a 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?; @@ -1193,6 +1326,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 447af901..16068e2a 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` |