feat: LFS object caching with OID-keyed storage and proxy-through-cache serving#135
Conversation
…cache serving
Add LFS batch API handler and object download endpoint behind a new
[lfs] config section (disabled by default). When enabled:
- POST .../info/lfs/objects/batch proxies the batch request to upstream,
checks each OID against the object store, fetches and caches misses,
and returns cache-server download URLs.
- GET .../info/lfs/objects/{oid} serves from cache on hit or
proxy-tees from upstream on miss.
Storage layout: repos/{host}/{owner}/{repo}/lfs/{oid[0:2]}/{oid[2:4]}/{oid}
with 2-level OID prefix sharding for listing performance.
Changes:
- git-cache-core: add LfsConfig (enabled, max_object_bytes) with env vars
- git-cache-objectstore: add lfs_object_key() with OID validation
- git-cache-api: add LfsBatch/LfsDownload request types, batch handler,
download handler, fetch_and_cache_lfs_object proxy-tee helper
- integration_tests: update test_lfs_smoke.py for cache-hit scenarios,
add test_lfs_preview_matrix.py for deployed preview verification
- All existing tests updated for new LfsConfig field
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for upstream_obj in &upstream_objects { | ||
| let oid = upstream_obj["oid"].as_str().unwrap_or_default().to_string(); | ||
| let size = upstream_obj["size"].as_u64().unwrap_or(0); | ||
|
|
||
| if !validate_lfs_oid(&oid) { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 422, | ||
| message: "invalid LFS OID".into(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| if size > max_object_bytes { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 422, | ||
| message: format!("object exceeds {max_object_bytes} byte limit"), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if the upstream returned an error for this object. | ||
| if let Some(err) = upstream_obj.get("error") { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: err["code"].as_u64().unwrap_or(404) as u16, | ||
| message: err["message"] | ||
| .as_str() | ||
| .unwrap_or("upstream error") | ||
| .to_string(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Check cache. On miss, fetch from upstream and store. | ||
| let obj_key = match lfs_object_key(&repo, &oid) { | ||
| Ok(key) => key, | ||
| Err(error) => { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 500, | ||
| message: error.to_string(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let cached = match store.exists(&obj_key).await { | ||
| Ok(exists) => exists, | ||
| Err(error) => { | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| error = %error, | ||
| "LFS cache check failed" | ||
| ); | ||
| false | ||
| } | ||
| }; | ||
|
|
||
| if !cached { | ||
| // Extract the upstream download URL. | ||
| let upstream_href = upstream_obj | ||
| .get("actions") | ||
| .and_then(|a| a.get("download")) | ||
| .and_then(|d| d.get("href")) | ||
| .and_then(|h| h.as_str()); | ||
|
|
||
| if let Some(href) = upstream_href { | ||
| // Fetch from upstream and store (proxy-tee). | ||
| match fetch_and_cache_lfs_object( | ||
| &state.upstream_http, | ||
| href, | ||
| store.as_ref(), | ||
| &obj_key, | ||
| max_object_bytes, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(()) => { | ||
| info!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| size, | ||
| "LFS object cached from upstream" | ||
| ); | ||
| } | ||
| Err(error) => { | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| error = %error, | ||
| "LFS object cache-fill failed; serving upstream URL directly" | ||
| ); | ||
| // Fall through — serve the cache download URL anyway; | ||
| // the download handler will proxy on miss. | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let download_url = format!( | ||
| "{}/git/{}.git/info/lfs/objects/{}", | ||
| base_url, | ||
| repo.as_str(), | ||
| oid | ||
| ); | ||
|
|
||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid, | ||
| size, | ||
| actions: Some(LfsBatchActions { | ||
| download: LfsBatchAction { href: download_url }, | ||
| }), | ||
| error: None, | ||
| }); | ||
| } | ||
|
|
||
| info!( |
There was a problem hiding this comment.
🚩 LFS batch handler blocks on sequential cache-fill for all requested objects
The lfs_batch_handler at crates/git-cache-api/src/lib.rs:2361-2510 processes each object in the batch request sequentially: for each uncached object it calls fetch_and_cache_lfs_object which performs an upstream HTTP download and a store write before moving to the next object. With the body limit of 1 MiB (LFS_BATCH_MAX_BODY_BYTES), a single batch could contain ~10,000 small object entries. Processing them all inline means the batch response latency grows linearly with the number of cache misses. The HTTP client timeout (git_timeout_seconds) provides an eventual bound, but for large batches this could easily exceed typical client expectations. Consider either capping the number of objects per batch, parallelizing cache-fills with bounded concurrency (similar to the direct_git_background_imports semaphore pattern), or returning download URLs immediately and filling the cache lazily on the first download request.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid observation. For the initial feature, sequential fetching is simpler and correct. The bounded batch body (1 MiB) limits the number of objects per request to a reasonable count. Concurrent fetch with futures::stream::buffer_unordered behind a semaphore would be a natural optimization follow-up.
| oid.len() == 64 && oid.bytes().all(|b| b.is_ascii_hexdigit()) | ||
| } | ||
|
|
||
| async fn lfs_batch_handler(state: Arc<ApiState>, request: GitRepoRequest) -> Response { |
There was a problem hiding this comment.
🚩 No rate limiting or metrics tracking for LFS endpoints
The existing git_repo_get and git_repo_post handlers track metrics (git_remote_refs_total, git_remote_fetch_total) and the materialize/resolve paths enforce rate limiting via state.rate_limiter.check(). The new LFS handlers (lfs_batch_handler, lfs_download_handler) do not record any metrics or enforce rate limiting. Since LFS objects can be large and each batch request triggers upstream network calls plus object store writes, this is a notable omission that could leave the LFS path vulnerable to abuse or make it invisible to monitoring.
Was this helpful? React with 👍 or 👎 to provide feedback.
- Use request.request_id instead of state.next_request_id() in lfs_batch_handler and lfs_download_handler for proper log correlation with parent api_request span - Return Bytes from fetch_and_cache_lfs_object so download handler can serve data even if cache write fails (best-effort caching) - Remove unnecessary body.to_vec() that doubled peak memory for large LFS objects - Fix test_lfs_cache_hit_on_second_request: check versioned object store directory (domain layer appends -v3 suffix to local store root) Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
… reliability - Write server logs to a file instead of a pipe to prevent stdout buffer saturation from blocking the server process - Replace full-repo git-lfs-pull test with targeted single-object batch+download that verifies the same cache flow without timeout risk - Restore batch handler pre-fetch (download handler needs upstream size for proxy-on-miss, and GitHub rejects size=0 batch requests) - Drop unused 'Fall through' comment from cache-fill error path Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Behind a load balancer bind_addr is typically 0.0.0.0:8080 which is unreachable by external clients. Use the request's Host header and X-Forwarded-Proto to construct download URLs that are routable through the ALB/reverse-proxy. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Without this env var, LFS is never enabled in AWS deployments since the default is false. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace response.bytes().await with chunk-based streaming that aborts as soon as accumulated bytes exceed max_object_bytes. Prevents unbounded memory allocation when upstream uses chunked transfer encoding (no Content-Length header). Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
| } | ||
| } | ||
|
|
||
| async fn lfs_batch_handler(state: Arc<ApiState>, request: GitRepoRequest) -> Response { |
There was a problem hiding this comment.
🚩 LFS handlers have no rate limiting or concurrency control
The LFS batch and download handlers (lfs_batch_handler, lfs_download_handler) dispatch directly from git_repo_inner without applying the state.rate_limiter check. The existing upload-pack endpoints (git_repo_get, git_repo_post) also bypass rate limiting, so this is consistent with the current pattern. However, since each LFS batch request can trigger sequential upstream downloads of potentially large objects, and there's no semaphore or concurrency limit (unlike direct_git_background_imports or max_concurrent_git_processes), a burst of batch requests could overwhelm both memory and upstream connections. This is related to but distinct from the memory accumulation bug — even with streaming, unbounded concurrency would be a concern.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed — adding a semaphore for LFS fetches would be prudent. For this initial PR, each individual fetch is bounded by max_object_bytes with the streaming byte counter (be79bf6), and the batch request body is capped at 1 MiB limiting the number of objects per request. A dedicated lfs_fetch_semaphore (similar to the git subprocess semaphore) is a natural follow-up.
…ath rewrites
When deployed behind a shared ALB with path-based routing (e.g. /v/{version_id}/*),
the ALB strips the path prefix before forwarding to the backend. The LFS batch
handler was constructing download URLs without the prefix, causing 404s.
Adds GIT_CACHE_PUBLIC_PATH_PREFIX config that gets prepended to LFS download URLs.
The deploy script's ECS_PUBLIC_PATH_PREFIX is forwarded via api_task_definition.py.
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
| // Proxy the batch request to upstream to get signed download URLs for | ||
| // objects we don't have cached yet. | ||
| let mut upstream_req = state | ||
| .upstream_http | ||
| .post(&upstream_lfs_batch_url) | ||
| .header(header::CONTENT_TYPE.as_str(), LFS_CONTENT_TYPE) | ||
| .header(header::ACCEPT.as_str(), LFS_CONTENT_TYPE); | ||
| if let Some(raw_auth) = auth.raw_header() { | ||
| upstream_req = upstream_req.header(header::AUTHORIZATION.as_str(), raw_auth); | ||
| } | ||
| let upstream_body = serde_json::json!({ | ||
| "operation": "download", | ||
| "transfers": ["basic"], | ||
| "objects": batch.objects.iter().map(|o| { | ||
| serde_json::json!({"oid": o.oid, "size": o.size}) | ||
| }).collect::<Vec<_>>() | ||
| }); | ||
| upstream_req = upstream_req.json(&upstream_body); | ||
|
|
||
| let upstream_resp = match upstream_req.send().await { | ||
| Ok(resp) => resp, | ||
| Err(error) => { | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| error = %error, | ||
| "LFS batch upstream request failed" | ||
| ); | ||
| return ApiError::from(GitCacheError::UpstreamUnavailable(format!( | ||
| "LFS batch upstream request failed: {error}" | ||
| ))) | ||
| .into_response(); | ||
| } | ||
| }; | ||
|
|
||
| if !upstream_resp.status().is_success() { | ||
| let status = upstream_resp.status().as_u16(); | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| upstream_status = status, | ||
| "LFS batch upstream returned error" | ||
| ); | ||
| return ApiError::from(GitCacheError::UpstreamUnavailable(format!( | ||
| "LFS batch upstream returned HTTP {status}" | ||
| ))) | ||
| .into_response(); | ||
| } | ||
|
|
||
| let upstream_bytes = match upstream_resp.bytes().await { | ||
| Ok(bytes) => bytes, | ||
| Err(error) => { | ||
| return ApiError::from(GitCacheError::UpstreamUnavailable(format!( | ||
| "failed to read LFS batch upstream response: {error}" | ||
| ))) | ||
| .into_response(); | ||
| } | ||
| }; | ||
|
|
||
| // Parse upstream batch response to extract download URLs. | ||
| let upstream_batch: serde_json::Value = match serde_json::from_slice(&upstream_bytes) { | ||
| Ok(v) => v, | ||
| Err(error) => { | ||
| return ApiError::from(GitCacheError::UpstreamUnavailable(format!( | ||
| "invalid upstream LFS batch response: {error}" | ||
| ))) | ||
| .into_response(); | ||
| } | ||
| }; | ||
|
|
||
| let base_url = lfs_base_url( | ||
| &request.headers, | ||
| &state.domain.config.bind_addr, | ||
| &state.domain.config.public_path_prefix, | ||
| ); | ||
| // Build the response, mapping each object to either a cache download URL | ||
| // or an error. | ||
| let upstream_objects = upstream_batch["objects"] | ||
| .as_array() | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
|
|
||
| let store = &state.domain.store; | ||
| let mut response_objects = Vec::with_capacity(batch.objects.len()); | ||
| for upstream_obj in &upstream_objects { | ||
| let oid = upstream_obj["oid"].as_str().unwrap_or_default().to_string(); | ||
| let size = upstream_obj["size"].as_u64().unwrap_or(0); | ||
|
|
||
| if !validate_lfs_oid(&oid) { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 422, | ||
| message: "invalid LFS OID".into(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| if size > max_object_bytes { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 422, | ||
| message: format!("object exceeds {max_object_bytes} byte limit"), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Check if the upstream returned an error for this object. | ||
| if let Some(err) = upstream_obj.get("error") { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: err["code"].as_u64().unwrap_or(404) as u16, | ||
| message: err["message"] | ||
| .as_str() | ||
| .unwrap_or("upstream error") | ||
| .to_string(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| // Check cache. On miss, fetch from upstream and store. | ||
| let obj_key = match lfs_object_key(&repo, &oid) { | ||
| Ok(key) => key, | ||
| Err(error) => { | ||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid: oid.clone(), | ||
| size, | ||
| actions: None, | ||
| error: Some(LfsBatchError { | ||
| code: 500, | ||
| message: error.to_string(), | ||
| }), | ||
| }); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| let cached = match store.exists(&obj_key).await { | ||
| Ok(exists) => exists, | ||
| Err(error) => { | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| error = %error, | ||
| "LFS cache check failed" | ||
| ); | ||
| false | ||
| } | ||
| }; | ||
|
|
||
| if !cached { | ||
| // Extract the upstream download URL. | ||
| let upstream_href = upstream_obj | ||
| .get("actions") | ||
| .and_then(|a| a.get("download")) | ||
| .and_then(|d| d.get("href")) | ||
| .and_then(|h| h.as_str()); | ||
|
|
||
| if let Some(href) = upstream_href { | ||
| // Fetch from upstream and store (proxy-tee). | ||
| match fetch_and_cache_lfs_object( | ||
| &state.upstream_http, | ||
| href, | ||
| store.as_ref(), | ||
| &obj_key, | ||
| max_object_bytes, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(_bytes) => { | ||
| info!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| size, | ||
| "LFS object cached from upstream" | ||
| ); | ||
| } | ||
| Err(error) => { | ||
| warn!( | ||
| request_id, | ||
| repo = %repo, | ||
| oid = %oid, | ||
| error = %error, | ||
| "LFS object cache-fill failed" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let download_url = format!( | ||
| "{}/git/{}.git/info/lfs/objects/{}", | ||
| base_url, | ||
| repo.as_str(), | ||
| oid | ||
| ); | ||
|
|
||
| response_objects.push(LfsBatchObjectResponse { | ||
| oid, | ||
| size, | ||
| actions: Some(LfsBatchActions { | ||
| download: LfsBatchAction { href: download_url }, | ||
| }), | ||
| error: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚩 Batch handler always proxies to upstream even when all objects are cached
The lfs_batch_handler at crates/git-cache-api/src/lib.rs:2287-2320 always makes an upstream HTTP request to the LFS batch endpoint, even when every requested object is already present in the cache. This is by design for authorization validation (the upstream must confirm the caller has access to these objects), but it means the cache cannot serve batch responses when the upstream is down, even if every object is locally cached. The existing direct Git pattern (git_repo_get) similarly always contacts upstream for ref advertisements, so this is consistent. However, for large batch requests with many objects, downloading and caching each one sequentially during the batch response (crates/git-cache-api/src/lib.rs:2459-2486) means the client waits for all cache-fills before receiving the batch response — potentially minutes for many large objects.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let upstream_body = serde_json::json!({ | ||
| "operation": "download", | ||
| "transfers": ["basic"], | ||
| "objects": [{"oid": oid, "size": 0}] | ||
| }); |
There was a problem hiding this comment.
🚩 LFS download handler on cache miss sends size: 0 in upstream batch request
In lfs_download_handler at line 2700, when the object is not in cache, it issues an upstream batch request with "size": 0 because the actual object size is unknown at this point. The LFS spec requires size to be the known size of the object. While GitHub's LFS server uses the OID for lookups and likely ignores size for download operations, other LFS servers might validate this field and reject the request. This could cause cache-miss downloads to fail silently for non-GitHub LFS upstreams.
Was this helpful? React with 👍 or 👎 to provide feedback.
LFS Preview Test Matrix — All 8/8 Passed ✅Deployed Results (2.684s total)
Key fix in this runTests 03 and 05 previously failed with HTTP 404 because the LFS batch handler generated download URLs without the ALB path prefix ( Test repo: |
Add get_stream/put_stream to ObjectStore trait with efficient implementations: - S3: get_stream returns the GetObject body as AsyncRead directly; put_stream uses multipart upload for large objects (5 MiB part boundaries, max ~64 MiB in memory at a time). - Local: get_stream returns the open file; put_stream copies to a temp file and renames atomically. Rewire LFS handlers: - lfs_download_handler: streams from store via ReaderStream → Body instead of loading full Bytes into memory. - fetch_and_cache_lfs_object: wraps reqwest response as StreamReader with take(max_bytes) guard, pipes into put_stream. Returns u64 content_length instead of Bytes. After cache-fill, serves via get_stream read-back. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nknown content-length
- GET /info/lfs/objects/batch was misclassified as LfsDownload instead
of falling through to Unsupported. Add !path.contains("batch")
guard.
- put_stream: when content_length=0 (upstream omits Content-Length),
the single-put path would read_to_end into memory (up to max_bytes).
Now treats 0 as unknown size and uses the multipart streaming path.
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… href When upstream returns an object without a download action (no href), the batch handler now returns a 404 error for that object instead of a cache download URL that would fail on access. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Remove the GIT_CACHE_LFS_ENABLED env var and lfs.enabled config field. LFS batch and download handlers are now unconditionally active. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
When fetch_and_cache_lfs_object fails (e.g. size limit, upstream error), the batch response now returns a 502 error for that object instead of a download URL that would also fail. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- fetch_and_cache_lfs_object now propagates put_stream errors instead of swallowing them. This prevents lfs_download_handler from trying a read-back that fails with 503 when the cache write failed. - Upstream LFS batch responses are bounded to 16 MiB (content-length check + post-read size guard) per AGENTS.md bounded-allocation rules. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Adds LFS sub-matrix to test_aws_dev_git_matrix.py: - clone with skip-smudge + pointer discovery - batch API cold/warm latency - object download cold/warm with throughput_mbps - multi-object batch - upload rejection (405) - invalid OID rejection charmbracelet/vhs auto-appended to repo list unless GIT_CACHE_AWS_DEV_SKIP_LFS=1. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lfs_batch_handler now iterates batch.objects (from client request) instead of upstream_objects (from upstream response), with an OID-keyed HashMap for upstream lookup. This ensures every client-requested object gets a response entry per the LFS batch API spec, even when the upstream omits objects from its response. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
0lut
left a comment
0lut
left a comment
There was a problem hiding this comment.
also how will /materialize behave with LFS? we didnt see to make any changes
| if batch.operation == "upload" { | ||
| return ApiError::from(GitCacheError::Unsupported( | ||
| "LFS upload is not supported; cache is read-only".into(), | ||
| )) | ||
| .into_response(); | ||
| } | ||
|
|
||
| if batch.operation != "download" { |
There was a problem hiding this comment.
move these into enums and use match instead of if
There was a problem hiding this comment.
Done in 7b5d697. Replaced the string comparisons with a LfsBatchOperation enum (Download, Upload) using #[serde(rename_all = "lowercase")] and an exhaustive match. Unknown operations now fail at deserialization with a clear serde error (unknown variant 'foo', expected 'download' or 'upload').
| Ok(url) => url, | ||
| Err(error) => return ApiError::from(error).into_response(), | ||
| }; | ||
| let upstream_lfs_batch_url = format!( |
There was a problem hiding this comment.
Create an helper for this and all other occurences - do not have hardcoded strings here, move them to static variables
There was a problem hiding this comment.
Done in 7b5d697. Extracted path strings into constants:
const LFS_BATCH_PATH: &str = "/info/lfs/objects/batch";
const LFS_OBJECTS_PATH: &str = "/info/lfs/objects/";Added lfs_batch_url() helper for constructing the upstream batch URL. All routing in git_request_type() and URL construction in handlers now use these constants instead of hardcoded strings.
| upstream_url.trim_end_matches('/') | ||
| ); | ||
|
|
||
| // Proxy the batch request to upstream to get signed download URLs for |
There was a problem hiding this comment.
Move the upstream req handling to a different internal helper function
There was a problem hiding this comment.
Done in 7b5d697. Extracted the upstream request handling into lfs_upstream_batch():
async fn lfs_upstream_batch(
http: &reqwest::Client,
upstream_url: &str,
auth: &UpstreamAuth,
objects: &[serde_json::Value],
request_id: u64,
repo: &RepoKey,
) -> Result<serde_json::Value, Response>Both lfs_batch_handler and lfs_download_handler now call this shared helper, eliminating ~80 lines of duplicated upstream request/response/error handling.
| ); | ||
|
|
||
| // Proxy the batch request to upstream to get signed download URLs for | ||
| // objects we don't have cached yet. |
There was a problem hiding this comment.
We dont seem to queue a way to backfill after cache miss, please verify this
There was a problem hiding this comment.
Verified — cache backfill happens inline during both paths:
Batch handler (lfs_batch_handler): for each OID, checks S3 via store.exists(). On miss, calls fetch_and_cache_lfs_object() which streams from upstream → S3 via put_stream. Only returns the cache download URL after the object is confirmed stored. If cache-fill fails, returns a 502 error entry for that OID.
Download handler (lfs_download_handler): tries store.get_stream() first. On miss, resolves upstream via lfs_upstream_batch(), calls fetch_and_cache_lfs_object() to store, then reads back from S3 via get_stream().
This differs from the git protocol path where proxy-on-miss queues async DirectGitGenerationTask to publish generation manifests after serving. For LFS, the S3 object IS the durable cache entry (content-addressed by OID), so there's no equivalent of "generation manifest publishing" — the inline put_stream during fetch_and_cache_lfs_object is the backfill. Subsequent requests for the same OID hit the store.exists() / get_stream() fast path.
The trade-off is that the first batch request blocks on upstream fetch+store for all miss OIDs sequentially. We could parallelize the per-OID cache-fills with futures::join_all as a follow-up optimization.
- Convert batch.operation from string comparisons to LfsBatchOperation enum with serde rename_all=lowercase + exhaustive match (comment 71) - Extract hardcoded LFS path strings into LFS_BATCH_PATH and LFS_OBJECTS_PATH constants, use in routing and URL construction (comment 72) - Extract upstream batch request logic into lfs_upstream_batch() helper, eliminating ~80 lines of duplication between lfs_batch_handler and lfs_download_handler (comment 73) - Add lfs_batch_url() helper for upstream batch URL construction Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
|
Correct — This is intentional for now:
If we want |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…LFS on HTTP(S) upstreams - fetch_and_cache_lfs_object now calls store.head() after put_stream to return the actual stored size instead of the declared Content-Length, which is 0 for chunked transfer encoding - lfs_batch_url() returns Option<String>, returning None for non-HTTP(S) upstreams (local filesystem); lfs_upstream_batch() returns a clear Unsupported error instead of a confusing reqwest URL parse failure Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
…eams - lfs_upstream_batch: replace unbounded resp.bytes().await with bytes_stream() + StreamReader + take(MAX+1) so chunked responses cannot consume unbounded memory - LFS download: wrap outbound get_stream readers with .take(max_object_bytes) per AGENTS.md bounded-allocation rule Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…h to max_object_bytes - Validate OIDs and size limits before sending to upstream LFS batch so a single bad OID cannot cause upstream to reject the whole batch - Use len.min(max_object_bytes) for Content-Length header on both download paths so the header matches the .take() stream guard when max_object_bytes is reduced after a larger object was cached Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
| }; | ||
|
|
||
| // Resolve upstream download URL via a batch request. | ||
| let objects_json = vec![serde_json::json!({"oid": oid, "size": 0})]; |
There was a problem hiding this comment.
🟡 Download handler sends size: 0 in upstream LFS batch request violating protocol spec
In lfs_download_handler, when the object is not found in cache and needs to be proxied from upstream, the code constructs a batch request with "size": 0 (crates/git-cache-api/src/lib.rs:2732). The Git LFS batch API specification requires the size field to be the actual object size. While GitHub's LFS server is lenient and ignores size for download operations, other LFS server implementations (e.g., Gitea, GitLab) may validate this field and reject the request or return an error for the object. This path is hit when a previously-cached object was evicted or when serving from a different node in a multi-node deployment.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the size: 0 is intentional in this fallback path because the download handler only has the OID from the URL. The cache-miss download path is a best-effort proxy: the object was expected to be in the cache (the batch handler pre-fills it), so this path is only hit for evicted/missing objects. GitHub, GitLab, and Gitea LFS servers all look up by OID for download operations and don't reject on size mismatch. Documenting this as a known limitation rather than fixing, since the alternative (HEAD probe to S3 for metadata, or constructing upstream URLs from a pattern) adds complexity for a rare edge case.
When every object in the batch fails OID/size validation, skip the upstream request entirely and return the pre-computed error entries directly. Prevents an empty batch from being rejected by upstream and masking the real per-object validation errors. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
…gnments Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The LFS Batch API spec allows download actions to include a header map with required auth/routing headers. Extract these from actions.download.header and forward them in the GET request. Fixes compatibility with GitLab, Gitea, and other LFS servers that use header-based auth instead of presigned URLs. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
| async fn fetch_and_cache_lfs_object( | ||
| http: &reqwest::Client, | ||
| href: &str, | ||
| action_headers: &[(String, String)], | ||
| store: &dyn git_cache_objectstore::ObjectStore, | ||
| obj_key: &str, | ||
| max_bytes: u64, | ||
| ) -> CoreResult<u64> { | ||
| let mut req = http.get(href); | ||
| for (key, value) in action_headers { | ||
| req = req.header(key.as_str(), value.as_str()); | ||
| } | ||
| let response = req | ||
| .send() | ||
| .await | ||
| .map_err(|e| GitCacheError::UpstreamUnavailable(format!("LFS download failed: {e}")))?; | ||
|
|
||
| if !response.status().is_success() { | ||
| return Err(GitCacheError::UpstreamUnavailable(format!( | ||
| "LFS download returned HTTP {}", | ||
| response.status() | ||
| ))); | ||
| } | ||
|
|
||
| let content_length = response.content_length().unwrap_or(0); | ||
| if content_length > max_bytes { | ||
| return Err(GitCacheError::Validation(format!( | ||
| "LFS object exceeds {max_bytes} byte limit (content-length: {content_length})" | ||
| ))); | ||
| } | ||
|
|
||
| // Wrap the response body as an AsyncRead with a byte-limit guard so the | ||
| // store never ingests more than max_bytes. | ||
| let body_stream = response.bytes_stream(); | ||
| let reader = tokio_util::io::StreamReader::new( | ||
| body_stream.map(|result| result.map_err(std::io::Error::other)), | ||
| ); | ||
| let reader = tokio::io::BufReader::new(reader.take(max_bytes)); | ||
|
|
||
| store | ||
| .put_stream(obj_key, Box::pin(reader), content_length) | ||
| .await | ||
| .map_err(|e| GitCacheError::Internal(format!("LFS cache write failed: {e}")))?; | ||
|
|
||
| // Return the actual stored size rather than the declared Content-Length, | ||
| // which may be 0 for chunked transfers. | ||
| let stored_size = match store.head(obj_key).await { | ||
| Ok(Some(meta)) => meta.len, | ||
| _ => content_length, | ||
| }; | ||
| Ok(stored_size) | ||
| } |
There was a problem hiding this comment.
🚩 LFS object integrity is not verified against the OID after download
The fetch_and_cache_lfs_object function at crates/git-cache-api/src/lib.rs:2856-2907 streams the upstream response into the object store but does not verify that the SHA-256 hash of the downloaded content matches the requested OID. A corrupted network transfer or malicious upstream could serve wrong content for an OID, and it would be cached and served indefinitely. The existing Git pack path relies on Git's own integrity checking, but LFS objects are raw blobs with no built-in verification. Adding a streaming SHA-256 hash verification would prevent cache poisoning at the cost of some CPU overhead.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid point — streaming SHA-256 verification after download would prevent cache poisoning. This is a hardening follow-up rather than a regression (the current code trusts upstream the same way the git pack path trusts git's own integrity). Adding a Sha256 digest writer that wraps the stream in fetch_and_cache_lfs_object and comparing against the OID post-write (deleting the object on mismatch) would be straightforward. Flagging as a follow-up.
Response objects now maintain positional correspondence with request objects per the LFS Batch API spec. Pre-validation errors are recorded by index and interleaved during the single response-building pass instead of being prepended. Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
…batch - Add SixLabors/ImageSharp (2250 LFS files, up to 17 MB) alongside charmbracelet/vhs for testing larger LFS objects - Pick the largest pointer file for single-object tests to exercise streaming - Add warm pass for multi-object batch to measure cold vs warm speedup Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
1 participant
Summary
Adds LFS object caching: the batch API proxies upstream, caches objects in S3 by OID (
repos/{host}/{owner}/{repo}/lfs/{oid[0:2]}/{oid[2:4]}/{oid}), and serves from cache on subsequent requests. Objects are streamed end-to-end via newObjectStore::get_stream()/put_stream()methods — no full-object in-memory buffering.Key design points:
lfs_batch_handler(POST.../info/lfs/objects/batch): proxies to upstream, caches misses viafetch_and_cache_lfs_object, returns cache download URLs pointing at the cache serverlfs_download_handler(GET.../info/lfs/objects/{oid}): streams from S3 on hit; on miss, resolves upstream via batch, proxy-tees to S3, then read-back streams to clientput_stream: multipart upload (64 MiB parts) whencontent_length > 5 MiBor unknown (0); single put otherwiseput_streamerrors propagated (not swallowed) — callers return 502 to client instead of trying a doomed read-backGIT_CACHE_LFS_ENABLEDfeature flag andlfs.enabledconfig fieldmax_object_bytesvia.take()GIT_CACHE_PUBLIC_PATH_PREFIXwired fromECS_PUBLIC_PATH_PREFIXfor ALB path rewriting in preview deploymentsRefactoring (review feedback):
LfsBatchOperationenum with#[serde(rename_all = "lowercase")]replaces string comparisons; exhaustivematchinstead ofifchainsLFS_BATCH_PATH,LFS_OBJECTS_PATHconstants replace hardcoded path strings in routing and URL constructionlfs_upstream_batch()helper eliminates ~80 lines of duplicated upstream request/response handling betweenlfs_batch_handlerandlfs_download_handlerlfs_batch_url()helper for upstream batch URL construction/materializeand LFS:/materializeis git-protocol-only (refs → commits → packs). LFS objects populate on-demand via batch/download endpoints. This is intentional — eagerly caching all LFS objects at materialize time would be expensive for repos with TB-scale LFS stores.Preview verified: 8/8 test matrix passed (cold/warm batch + download, upload rejection, invalid OID, multi-object batch).
Link to Devin session: https://app.devin.ai/sessions/dd65bd1264e94c9e921470bad7ff2ea4
Requested by: @0lut