Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Concurrency, cache-size enforcement, fail-open behavior, security defaults, and CI test selection have unresolved correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (9)
Optional token file allows unauthenticated production uploads · New Cache keys omit CPU feature requirements · New Concurrent uploads can collide on the same temporary path · New Stale temporary files can bypass cache size accounting · New Binary crate unit tests are skipped in CI · New Eviction scan reset can lose concurrent upload accounting · New Corrupt remote blobs abort builds instead of falling back locally · New Remote cache hits skip cleanup and can exceed the size limit · New Existence check races with eviction and causes cache miss errors · New
What changed in this PR
Adds a shared HTTP cache server and remote-cache support to the existing Halide cache client.
Changes:
- Adds remote lookup/upload support and cross-machine cache keys.
- Introduces a cache server with LRU eviction, metrics, dashboard, and service packaging.
- Expands concurrent end-to-end tests and CI/release coverage.
| File | Description |
|---|---|
lager/src/lru.rs |
Exposes eviction details and skips temporary files. |
lager/src/lib.rs |
Exports new storage APIs. |
lager/src/lager.rs |
Adds raw compressed-blob operations. |
halide-cache/tests/run |
Adds remote and concurrent system tests. |
halide-cache/src/remote.rs |
Implements the HTTP cache client. |
halide-cache/src/main.rs |
Integrates remote caching and key hardening. |
halide-cache/doc/remote-cache-design.md |
Documents architecture and protocol decisions. |
halide-cache/Cargo.toml |
Adds remote-client dependencies. |
halide-cache-server/src/metrics.rs |
Tracks operational cache metrics. |
halide-cache-server/src/main.rs |
Implements the HTTP server and eviction. |
halide-cache-server/src/dashboard.html |
Adds the monitoring dashboard. |
halide-cache-server/contrib/halide-cache-server.service |
Provides systemd deployment configuration. |
halide-cache-server/Cargo.toml |
Defines the server crate. |
Cargo.toml |
Adds the server to the workspace. |
Cargo.lock |
Locks new dependencies. |
.github/workflows/rust.yml |
Expands build and test coverage. |
.github/workflows/release.yml |
Publishes static server binaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical issues affect authentication, upload limits, cache correctness, eviction safety, and cache-key integrity.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 7
Open (10)
Empty or invalid token configuration enables unauthenticated uploads · New Raw request body bypasses configured upload size limit · New Concurrent eviction can delete replaced or recently used blobs · New Malformed token configuration opens unauthenticated PUT access · New Streaming raw body bypasses maximum blob size · New Lossy path conversion can collide for non-UTF-8 paths · New Different blob kinds can coexist under one cache address · New Tests track cargo parent instead of the server process · New Documentation states an outdated maximum blob size · New Eviction interval example uses unsupported duration syntax · New
Resolved since last review (9)
Stale temporary files can bypass cache size accounting Concurrent uploads can collide on the same temporary path Cache keys omit CPU feature requirements Optional token file allows unauthenticated production uploads Existence check races with eviction and causes cache miss errors Remote cache hits skip cleanup and can exceed the size limit Corrupt remote blobs abort builds instead of falling back locally Eviction scan reset can lose concurrent upload accounting Binary crate unit tests are skipped in CI
| # Required: the unit refuses to start without the token file, so a missing or | ||
| # misnamed file cannot silently leave uploads open to everyone on the network. | ||
| EnvironmentFile=/etc/halide-cache-server.env |
There was a problem hiding this comment.
Done in 769af79: new --require-token flag makes the server refuse to start unless a non-empty token is configured; the systemd unit passes it.
| let state = Arc::new(AppState { | ||
| lager: Lager::new(&args.data_dir)?, | ||
| max_size: args.size.as_u64(), | ||
| token: args.token.filter(|t| !t.is_empty()), |
There was a problem hiding this comment.
Done in 769af79: see --require-token. The bare binary keeps the open mode for local/dev use, the unit opts into the strict mode.
| .route("/v1/clients", get(stats::clients)) | ||
| .route("/metrics", get(stats::prometheus)) | ||
| .route("/healthz", get(|| async { "ok" })) | ||
| .layer(DefaultBodyLimit::max(args.max_blob_size.as_u64() as usize)) |
There was a problem hiding this comment.
Done in 769af79: enforced while streaming, 413 on overflow. See the reply above.
d1dddd8 to
603ff1c
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Failed uploads can delete valid entries, while eviction and download handling retain correctness and reliability issues.
Review effort: Balanced
Findings: 5
Open (10)
Bound remote download response size · New Delete old content only after successful upload · New Streaming raw body bypasses maximum blob size Malformed token configuration opens unauthenticated PUT access Empty or invalid token configuration enables unauthenticated uploads Return 500 for storage failures · New Retry eviction after a concurrent pass completes · New Track evicted entries separately from heap items · New Use inclusive histogram bucket boundaries · New Hash Python builder script contents · New
Resolved since last review (7)
Different blob kinds can coexist under one cache address Lossy path conversion can collide for non-UTF-8 paths Concurrent eviction can delete replaced or recently used blobs Raw request body bypasses configured upload size limit Tests track cargo parent instead of the server process Eviction interval example uses unsupported duration syntax Documentation states an outdated maximum blob size
| .and_then(|v| v.to_str().ok()) | ||
| .and_then(Kind::parse) | ||
| .ok_or(Error::BadKind)?; | ||
| let body = response.body_mut().with_config().limit(u64::MAX).reader(); |
| if result.is_err() { | ||
| let _ = std::fs::remove_file(&tmp); | ||
| } | ||
| self.remove_other_kind(address, kind)?; | ||
| result.map(|_| !existed) |
| Ok(Err(e)) => { | ||
| // A client that dropped the connection shows up here as an io error | ||
| // while reading the body. | ||
| warn!(%address, err = %e, "upload failed"); | ||
| (StatusCode::BAD_REQUEST, "upload failed\n").into_response() |
| let Ok(_guard) = state.evict_lock.try_lock() else { | ||
| return; |
| let before = lru.lager_size(); | ||
| let evicted = if before > max { | ||
| lru.evict_until(target)? | ||
| } else { | ||
| Vec::new() | ||
| }; | ||
| Ok::<_, lager::Error>((before, evicted, lru.lager_size(), lru.entries())) |
| .duration_since(e.last_used) | ||
| .map(|d| d.as_secs()) | ||
| .unwrap_or(0); | ||
| let i = AGE_BOUNDS.iter().position(|&b| age < b).unwrap_or(5); |
| let builder_exe = which::which(&args.builder[0]) | ||
| .map_err(|e| anyhow::anyhow!("cannot locate builder {:?}: {e}", args.builder[0]))?; | ||
| let halide = if is_python(&builder_exe) { | ||
| Some(locate_halide(&builder_exe)?) |
6113d4f to
e89b74b
Compare
an entry can hold several files: a zstd-compressed tar archive of the files in a fixed order, so `zstd -dc entry.tar.zst | tar -x` opens it. Prework for the server
The object and header are always produced together, so this commit changes how halide-cache stores, restores and later: fetches, uploads and evicts them together with the same address in lager.
Entries are about to be shared, so the key must drop what differs between machines and include what makes an object valid elsewhere: - strip the base directory wherever it occurs in an argument and normalise path separators - prefix a key scheme version, and include the host OS and architecture - hash the builder executable, the Python interpreter, resolved through PATH. Ask it where it imports halide from and hash that package's native libraries. A missing or unusable halide is an error rather than a weaker key. - reject non-UTF-8 output paths instead of hashing a lossy conversion
Make local cache cleanup best effort, since Windows refuses to delete an entry another parallel build has open and the outputs are already in place. Run the client's tests on Windows in CI.
An HTTP server sharing cache entries between machines, storing them
through lager in the same layout as the local cache.
GET/PUT /v1/blobs/{address}
Ships a systemd unit, and static musl binaries for x86_64 and aarch64 in
CI and releases to be self-contained and independent of the Glibc
version of the host.
Look locally, then on the server, then build and upload. A remote hit is stored locally and triggers the local LRU cleanup. The remote is fail-open: connection errors, failed uploads and entries that do not decompress are warnings, and the build proceeds locally. The end-to-end tests start the server and cover hits across machines, a server that is down, corrupt entries, oversized uploads, and concurrent clients racing on one entry under constant eviction. They run on Unix only, since the server is deployed on Linux.



Share halide-cache entries between machines through a central server. Second of three stacked PRs, on top of #8; monitoring follows in #9.
zstd -dc entry.tar.zst | tar -x. Eviction is safe against concurrent readers and writers.--sizeevery 30 minutes, a systemd unit, and static musl binaries in releases.🤖 Generated with Claude Code
https://claude.ai/code/session_01TVxsuu7ZCt2cdfn99fuXaA