From 83c48dca8e5d426c28bed537a0237e9f3d6f2d18 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 14:21:09 -0700 Subject: [PATCH 01/32] feat(checked_file): add Checksum::hash() accessor + cross-version compat tests (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #8855 added `CheckedFile.size: Option` as the new/legacy discriminant for self-hosted metadata. This adds the consumer-side plumbing the frontend resolve pipeline needs: - `Checksum::hash()` — borrows the hex digest. Used as the blob filename in the MDC cache (`blobs/`). - `test_legacy_wire_format_omits_size` — locks in `serde(default)` behavior so old MDC bytes deserialize cleanly with `size = None`. - `test_new_wire_format_carries_size` — round-trips a populated size through serde. - Extends `test_checked_file_from_disk` to assert `size` is populated (`Some(560)`) and `Checksum::hash()` returns the expected hex. No behavior change for callers; resolve_metadata_files (next commit) is the first consumer. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#8855 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/common/checked_file.rs | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/lib/llm/src/common/checked_file.rs b/lib/llm/src/common/checked_file.rs index 38c8c8474192..63b3879134df 100644 --- a/lib/llm/src/common/checked_file.rs +++ b/lib/llm/src/common/checked_file.rs @@ -235,6 +235,10 @@ impl Checksum { algorithm, } } + + pub fn hash(&self) -> &str { + &self.hash + } } impl Serialize for Checksum { @@ -378,5 +382,38 @@ mod tests { let expected = Checksum::blake3("62bc124be974d3a25db05bedc99422660c26715e5bbda0b37d14bd84a0c65ab2"); assert_eq!(expected, *cf.checksum()); + assert_eq!(Some(560), cf.size(), "TinyLlama config.json is 560 bytes"); + assert_eq!( + "62bc124be974d3a25db05bedc99422660c26715e5bbda0b37d14bd84a0c65ab2", + cf.checksum().hash() + ); + } + + /// Old MDC bytes (no `size` field) deserialize cleanly with + /// `size = None`, preserving cross-version compat between a + /// pre-size worker and a post-size frontend. + #[test] + fn test_legacy_wire_format_omits_size() { + let legacy = r#"{"path":"/x/config.json","checksum":"blake3:abc"}"#; + let cf: CheckedFile = serde_json::from_str(legacy).unwrap(); + assert_eq!(None, cf.size()); + assert_eq!("/x/config.json", cf.path().unwrap().to_str().unwrap()); + } + + /// New-format CheckedFiles round-trip with the size field. + #[test] + fn test_new_wire_format_carries_size() { + let cf = CheckedFile { + path: Either::Left(PathBuf::from("/x/tokenizer.json")), + checksum: Checksum::blake3("abc"), + size: Some(12345), + }; + let json = serde_json::to_string(&cf).unwrap(); + assert!( + json.contains("\"size\":12345"), + "expected size field in serialization, got: {json}" + ); + let back: CheckedFile = serde_json::from_str(&json).unwrap(); + assert_eq!(Some(12345), back.size()); } } From 01e996c76282f3a07f6c9a6ce1ee8fc5b5915e1c Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 15:15:55 -0700 Subject: [PATCH 02/32] =?UTF-8?q?feat(model=5Fcard):=20add=20MDC=20cache?= =?UTF-8?q?=20primitives=20=E2=80=94=20flock=20+=20atomic=20publish=20(gh-?= =?UTF-8?q?8749)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Frontend MDC cache scaffolding under `~/.cache/dynamo/mdc/`: blobs/ # content-addressed by-slug/// # symlinks → blobs Adds the building blocks the resolve pipeline (next commit) consumes: - `BlobLock::acquire(.lock)` — flock(LOCK_EX) via libc. Per-OFD semantics serialize concurrent intra- and inter-process callers, so a single layer covers both. Multiple frontends sharing $HOME collapse to one fetch per blake3. - `mdc_cache_root` / `mdc_blobs_dir` / `mdc_slug_dir` — layout helpers. - `unique_tmp_path(dest)` — `.tmp..`. Per-call tmps are disjoint, so `rename(2)` is the only synchronization point — never `create(tmp)`. - `stage_and_rename` (async) / `stage_and_rename_sync` — atomic publish: stage via `f(tmp)`, then `rename(tmp, dest)`. Tmp unlinked on any failure. - `symlink_force(target, link)` — `stage_and_rename_sync` wrapping a symlink so concurrent callers can't observe a half-published link. Also adds `libc` to lib/llm dependencies (workspace dep already pinned). No callers yet; consumed by resolve_uri in the next commit. Compile-clean with the expected unused-fn warnings. Refs ai-dynamo/dynamo#8749 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- Cargo.lock | 1 + lib/llm/Cargo.toml | 1 + lib/llm/src/model_card.rs | 134 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 3acda9a73e6a..2c814c1659b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2449,6 +2449,7 @@ dependencies = [ "ipnet", "json-five", "lazy_static", + "libc", "lru 0.12.5", "memfile", "minijinja", diff --git a/lib/llm/Cargo.toml b/lib/llm/Cargo.toml index 31559be92ed4..24e0b2a0287f 100644 --- a/lib/llm/Cargo.toml +++ b/lib/llm/Cargo.toml @@ -77,6 +77,7 @@ futures = { workspace = true } futures-util = { workspace = true } hf-hub = { workspace = true } ipnet = { workspace = true } +libc = { workspace = true } lru = { workspace = true } rand = { workspace = true } oneshot = { workspace = true } diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 9a231569c685..fe64cda84d1f 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -194,6 +194,140 @@ fn is_exclusively_mistral_model(directory: &Path) -> bool { !directory.join("config.json").exists() && directory.join("params.json").exists() } +/// `flock(LOCK_EX)` on `.lock`. Per-open-file-description +/// semantics serialize both intra- and inter-process callers, so a +/// single layer covers both. Released when `_file` drops. +struct BlobLock { + _file: std::fs::File, +} + +impl BlobLock { + async fn acquire(blob_path: &Path) -> anyhow::Result { + use std::os::fd::AsRawFd; + let lock_path = blob_path.with_extension("lock"); + let file = tokio::task::spawn_blocking(move || -> anyhow::Result { + let f = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(&lock_path) + .with_context(|| format!("opening lock file {}", lock_path.display()))?; + // SAFETY: `f` is open and owned; `flock` is a stable syscall. + let rc = unsafe { libc::flock(f.as_raw_fd(), libc::LOCK_EX) }; + if rc != 0 { + anyhow::bail!( + "flock {} failed: {}", + lock_path.display(), + std::io::Error::last_os_error() + ); + } + Ok(f) + }) + .await + .context("flock spawn_blocking task panicked")??; + Ok(Self { _file: file }) + } +} + +/// MDC cache: `blobs/` + `by-slug///`. +/// The `` segment mirrors HF Hub's `snapshots//` and +/// isolates worker sets that share a model name but publish different +/// file content. +fn mdc_cache_root() -> PathBuf { + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .unwrap_or_else(|_| ".".to_string()); + PathBuf::from(home).join(".cache/dynamo/mdc") +} + +fn mdc_blobs_dir() -> anyhow::Result { + let dir = mdc_cache_root().join("blobs"); + std::fs::create_dir_all(&dir) + .with_context(|| format!("creating MDC blobs dir {}", dir.display()))?; + Ok(dir) +} + +fn mdc_slug_dir(slug: &Slug, mdcsum: &str) -> anyhow::Result { + let dir = mdc_cache_root() + .join("by-slug") + .join(slug.to_string()) + .join(mdcsum); + std::fs::create_dir_all(&dir) + .with_context(|| format!("creating MDC slug dir {}", dir.display()))?; + Ok(dir) +} + +/// Per-call tmp path next to `dest`. pid + uuid suffix keeps tmps +/// disjoint across concurrent callers so `rename(2)` is the only +/// synchronization point — never `create(tmp)`. +fn unique_tmp_path(dest: &Path) -> PathBuf { + let suffix = format!( + "tmp.{}.{}", + std::process::id(), + uuid::Uuid::new_v4().simple() + ); + let name = dest + .file_name() + .map(|f| f.to_string_lossy().into_owned()) + .unwrap_or_default(); + let mut p = dest.to_path_buf(); + p.set_file_name(format!("{name}.{suffix}")); + p +} + +/// Atomic publish: stage via `f(&tmp)`, then `rename(tmp -> dest)`. +/// Concurrent-safe because tmps are per-call (see [`unique_tmp_path`]) +/// and `rename(2)` is atomic + overwrites. Tmp is unlinked on any +/// failure. +async fn stage_and_rename(dest: &Path, f: F) -> anyhow::Result<()> +where + F: FnOnce(PathBuf) -> Fut, + Fut: std::future::Future>, +{ + let tmp = unique_tmp_path(dest); + if let Err(err) = f(tmp.clone()).await { + let _ = tokio::fs::remove_file(&tmp).await; + return Err(err); + } + if let Err(err) = tokio::fs::rename(&tmp, dest).await { + let _ = tokio::fs::remove_file(&tmp).await; + anyhow::bail!("renaming {} -> {}: {err}", tmp.display(), dest.display()); + } + Ok(()) +} + +/// Sync sibling of [`stage_and_rename`] for cheap operations +/// (e.g., creating a symlink). +fn stage_and_rename_sync(dest: &Path, f: F) -> anyhow::Result<()> +where + F: FnOnce(&Path) -> anyhow::Result<()>, +{ + let tmp = unique_tmp_path(dest); + if let Err(err) = f(&tmp) { + let _ = std::fs::remove_file(&tmp); + return Err(err); + } + if let Err(err) = std::fs::rename(&tmp, dest) { + let _ = std::fs::remove_file(&tmp); + anyhow::bail!("renaming {} -> {}: {err}", tmp.display(), dest.display()); + } + Ok(()) +} + +/// Concurrent-safe via [`stage_and_rename_sync`]. +fn symlink_force(target: &Path, link: &Path) -> anyhow::Result<()> { + stage_and_rename_sync(link, |tmp| { + #[cfg(unix)] + std::os::unix::fs::symlink(target, tmp) + .with_context(|| format!("symlinking {} -> {}", tmp.display(), target.display()))?; + #[cfg(not(unix))] + std::fs::copy(target, tmp) + .map(|_| ()) + .with_context(|| format!("copying {} -> {}", target.display(), tmp.display()))?; + Ok(()) + }) +} + fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) From 610985bf8624c86dba8b35faf70b5674538044dd Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 15:26:09 -0700 Subject: [PATCH 03/32] feat(model_card): add resolve_uri pipeline for self-hosted metadata (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the per-CheckedFile fetch + verify primitive consumed by the resolve loop (next commit). Three schemes — `http(s)`, `file`, `hf` — converge on one verify-and-cache path. resolve_uri(client, uri, expected, dest): 1. cap = expected.size().unwrap_or(ABSOLUTE_MAX_METADATA_BYTES).min(...) 2. flock(LOCK_EX) on .lock (BlobLock) 3. dest already populated by a peer? return. 4. stage_and_rename(dest, |tmp| ...): - http(s): stream_to_tmp (Content-Length pre-check + take(cap+1)) - file: copy_to_tmp (metadata.len() check) - hf: hub::from_hf(repo) snapshot, copy_to_tmp from there Verify CheckedFile::from_disk(tmp).checksum() == expected.checksum() 5. atomic rename → dest, release lock. `size = None` (legacy MDCs) falls back to the 1 GiB absolute ceiling as cap. Behavior is otherwise uniform across schemes. Tests (mockito + tempdir): - resolve_uri_http_rejects_checksum_mismatch — bytes ok, blake3 wrong → error, no file written. - resolve_uri_http_rejects_oversize_body — declared size 8, body 35 → cap-exceeded error before checksum stage. - parse_hf_uri_roundtrip — `hf://repo/file` parses; malformed rejected. Still no caller; resolve_metadata_files lands next. Build is clean modulo the usual transitively-unused warnings. Refs ai-dynamo/dynamo#8749 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 247 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 247 insertions(+) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index fe64cda84d1f..a4cb1ef677ea 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -328,6 +328,151 @@ fn symlink_force(target: &Path, link: &Path) -> anyhow::Result<()> { }) } +/// Trust-boundary clamp on a worker's declared `CheckedFile.size`. +/// Realistic metadata files top out near 20 MiB; 1 GiB bounds disk +/// exhaustion from a compromised worker advertising `u64::MAX`. Also +/// the fallback cap when `CheckedFile.size` is absent (legacy MDCs). +const ABSOLUTE_MAX_METADATA_BYTES: u64 = 1024 * 1024 * 1024; + +/// Stage `uri` into `dest`, verifying staged bytes against `expected` +/// before publishing. Schemes: `http(s)`, `file`, `hf`. Concurrent-safe +/// via `BlobLock` + atomic rename. +async fn resolve_uri( + client: &reqwest::Client, + uri: &str, + expected: &CheckedFile, + dest: &Path, +) -> anyhow::Result<()> { + // Tighten cap to declared size if present; otherwise the absolute + // ceiling. Either way `cap` is enforced uniformly across schemes + // so transport choice can't bypass the bound. + let cap = expected + .size() + .unwrap_or(ABSOLUTE_MAX_METADATA_BYTES) + .min(ABSOLUTE_MAX_METADATA_BYTES); + + let parsed = url::Url::parse(uri).with_context(|| format!("parsing artifact uri: {uri}"))?; + + let _blob_guard = BlobLock::acquire(dest).await?; + + // A peer that held the lock before us may have populated `dest` + // already. + if dest.exists() { + return Ok(()); + } + + stage_and_rename(dest, |tmp| async move { + match parsed.scheme() { + "http" | "https" => stream_to_tmp(client, uri, &tmp, cap).await?, + "file" => { + let path = parsed + .to_file_path() + .map_err(|()| anyhow::anyhow!("invalid file:// uri: {uri}"))?; + copy_to_tmp(&path, &tmp, cap).await?; + } + "hf" => { + let (repo, filename) = parse_hf_uri(uri)?; + let snapshot = crate::hub::from_hf(&repo, /* ignore_weights = */ true) + .await + .with_context(|| format!("hub::from_hf({repo})"))?; + copy_to_tmp(&snapshot.join(&filename), &tmp, cap).await?; + } + scheme => anyhow::bail!("unsupported artifact uri scheme: {scheme} (uri: {uri})"), + } + + // Re-blake3 the staged bytes via the same `from_disk` path the + // worker used at registration so the comparison is bit-identical. + let actual = CheckedFile::from_disk(&tmp)?; + if actual.checksum() != expected.checksum() { + anyhow::bail!( + "checksum mismatch for {uri}: expected {}, got {}", + expected.checksum(), + actual.checksum() + ); + } + Ok(()) + }) + .await +} + +/// Stream HTTP body to `tmp`, capped at `cap` bytes. Pre-checks +/// `Content-Length` if present; otherwise the post-write check +/// catches overage. +async fn stream_to_tmp( + client: &reqwest::Client, + uri: &str, + tmp: &Path, + cap: u64, +) -> anyhow::Result<()> { + use futures::TryStreamExt; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio_util::io::StreamReader; + + let response = client + .get(uri) + .send() + .await + .with_context(|| format!("fetching {uri}"))?; + if !response.status().is_success() { + anyhow::bail!("fetching {uri} returned status {}", response.status()); + } + if let Some(content_length) = response.content_length() + && content_length > cap + { + anyhow::bail!("{uri} reports {content_length} bytes, exceeds cap {cap}"); + } + + let stream = response.bytes_stream().map_err(std::io::Error::other); + // `cap + 1` so a body of exactly `cap` passes but anything larger + // trips the `written > cap` check below. + let mut reader = StreamReader::new(stream).take(cap + 1); + let mut file = tokio::fs::File::create(tmp) + .await + .with_context(|| format!("creating {}", tmp.display()))?; + let written = tokio::io::copy(&mut reader, &mut file) + .await + .with_context(|| format!("streaming body from {uri} to {}", tmp.display()))?; + file.flush() + .await + .with_context(|| format!("flushing {}", tmp.display()))?; + if written > cap { + anyhow::bail!("{uri} body exceeds cap {cap}"); + } + Ok(()) +} + +async fn copy_to_tmp(src: &Path, tmp: &Path, cap: u64) -> anyhow::Result<()> { + let metadata = tokio::fs::metadata(src) + .await + .with_context(|| format!("reading metadata for {}", src.display()))?; + if metadata.len() > cap { + anyhow::bail!( + "{} is {} bytes, exceeds cap {}", + src.display(), + metadata.len(), + cap + ); + } + tokio::fs::copy(src, tmp) + .await + .with_context(|| format!("copying {} -> {}", src.display(), tmp.display()))?; + Ok(()) +} + +/// Parse `hf://repo[@rev]/filename` into `(repo[@rev], filename)`. +fn parse_hf_uri(uri: &str) -> anyhow::Result<(String, String)> { + let body = uri + .strip_prefix("hf://") + .with_context(|| format!("expected hf:// scheme, got: {uri}"))?; + let (repo, filename) = body + .rsplit_once('/') + .with_context(|| format!("hf:// uri must end in /filename, got: {uri}"))?; + if repo.is_empty() || filename.is_empty() { + anyhow::bail!("malformed hf:// uri: {uri}"); + } + Ok((repo.to_string(), filename.to_string())) +} + fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) @@ -1487,4 +1632,106 @@ mod tests { ); Ok(()) } + + /// Build a `CheckedFile` for testing with the given uri + checksum + size. + fn test_cf(uri: &str, checksum_hex: &str, size: u64) -> super::CheckedFile { + let json = serde_json::json!({ + "path": uri, + "checksum": format!("blake3:{checksum_hex}"), + "size": size, + }); + serde_json::from_value(json).unwrap() + } + + /// blake3 mismatch errors and writes nothing. + #[tokio::test] + async fn resolve_uri_http_rejects_checksum_mismatch() { + let mut server = mockito::Server::new_async().await; + let body = b"hello world"; + let _m = server + .mock("GET", "/file.txt") + .with_status(200) + .with_body(body) + .create_async() + .await; + + let dir = std::env::temp_dir().join(format!("dynamo-mdc-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("file.txt"); + + let url = format!("{}/file.txt", server.url()); + let expected = test_cf( + &url, + "0000000000000000000000000000000000000000000000000000000000000000", + body.len() as u64, + ); + let result = super::resolve_uri(&reqwest::Client::new(), &url, &expected, &dest).await; + + assert!(result.is_err(), "checksum mismatch must error"); + let msg = result.err().unwrap().to_string(); + assert!( + msg.contains("checksum mismatch"), + "expected checksum-mismatch error, got: {msg}" + ); + assert!( + !dest.exists(), + "no file should be written on checksum mismatch" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// Oversize http body errors before the in-memory buffer can grow + /// past the expected size, so a misbehaving worker can't OOM the + /// frontend before blake3 verification fails. + #[tokio::test] + async fn resolve_uri_http_rejects_oversize_body() { + let mut server = mockito::Server::new_async().await; + let body = b"hello hello hello hello hello hello"; // 35 bytes + let _m = server + .mock("GET", "/big.txt") + .with_status(200) + .with_body(body) + .create_async() + .await; + + let dir = std::env::temp_dir().join(format!("dynamo-mdc-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("big.txt"); + + let url = format!("{}/big.txt", server.url()); + // Cap is 8 bytes (declared size); body is 35. We trip the + // size cap before reaching blake3 verification. + let expected = test_cf( + &url, + "0000000000000000000000000000000000000000000000000000000000000000", + 8, + ); + let result = super::resolve_uri(&reqwest::Client::new(), &url, &expected, &dest).await; + + assert!(result.is_err(), "oversize body must error"); + let msg = result.err().unwrap().to_string(); + assert!( + msg.contains("exceeds cap"), + "expected size-cap error, got: {msg}" + ); + assert!( + !dest.exists(), + "no file should be written on size-cap rejection" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + + /// `parse_hf_uri` round-trip — `hf://repo/filename` parses into + /// `(repo, filename)` and rejects malformed inputs. + #[test] + fn parse_hf_uri_roundtrip() { + let (repo, filename) = super::parse_hf_uri("hf://Qwen/Qwen3-0.6B/tokenizer.json").unwrap(); + assert_eq!(repo, "Qwen/Qwen3-0.6B"); + assert_eq!(filename, "tokenizer.json"); + + assert!(super::parse_hf_uri("hf://just-a-name").is_err()); + assert!(super::parse_hf_uri("https://example.com/x").is_err()); + } } From 6077ce2a1d6c90609a0f159fc85b23c0956fe98e Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 15:32:10 -0700 Subject: [PATCH 04/32] feat(model_card): unify download_config through resolve_metadata_files (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the resolve pipeline into `download_config`. One path, no new/legacy split, blake3 verified on every load. download_config: - tensor-only short-circuit (unchanged) - else → resolve_metadata_files (always) resolve_metadata_files: Pass 1 — for each populated CheckedFile, normalize cf.path → URL: * URL form already → no-op * PathBuf that exists → file:// * PathBuf that doesn't exist → hf:/// Snapshot (uri, expected, filename) for the async loop. Pass 2 — fetch + verify + cache + symlink: * blob already in `~/.cache/dynamo/mdc/blobs/` → skip fetch * else `resolve_uri(uri, expected, blob)` (cap → flock → stage → blake3 verify → atomic rename) * symlink_force(blob, by-slug///) Pass 3 — rewrite cf.path to point at the cache symlink. The legacy MDC `has_local_files` and `update_dir` helpers are removed; the new pipeline walks `iter_metadata_files_mut` directly so the `is_custom` exclusion in the legacy `update_dir` is bypassed (custom chat templates flow through the same verify-and-cache path now). Tests covering the building blocks land in earlier commits (`resolve_uri`, `parse_hf_uri`). End-to-end coverage continues to live in the mocker e2e (`tests/frontend/test_self_host_metadata.py`, landed with PR1) and will get a multi-replica + multi-scheme test in the capstone PR4. Refs ai-dynamo/dynamo#8749 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 167 ++++++++++++++++++++++---------------- 1 file changed, 98 insertions(+), 69 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index a4cb1ef677ea..ab20db32c9ca 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -877,14 +877,102 @@ impl ModelDeploymentCard { out } - /// Download the files this card needs to work: config.json, tokenizer.json, etc. - pub async fn download_config(&mut self) -> anyhow::Result<()> { - if self.has_local_files() { - tracing::trace!("All model config is local, not downloading"); - return Ok(()); + /// Resolve every populated metadata `CheckedFile` through the + /// content-addressed MDC cache, blake3-verifying every byte. + /// + /// Pass 1 — for each slot, normalize `cf.path()` to a URL form + /// (`file://` if it exists locally, `hf:///` + /// otherwise, or unchanged if already a URL), then snapshot + /// `(uri, expected, filename)` for the async fetch loop. + /// Pass 2 — `resolve_uri` per slot if the blob isn't already + /// cached; symlink into `by-slug///`. + /// Pass 3 — rewrite each `cf.path` to point at the cache symlink. + /// + /// Bypasses `update_dir` because its `is_custom` exclusion is for + /// the legacy `hub::from_hf` fall-through and would leave URLs in + /// the MDC for custom templates. + async fn resolve_metadata_files(&mut self) -> anyhow::Result<()> { + let source = self.source_path().to_string(); + let mdcsum = self.mdcsum().to_string(); + let blobs = mdc_blobs_dir()?; + let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; + + // Pass 1: normalize to URL form + snapshot for the async loop. + let mut entries: Vec<(String, CheckedFile, String)> = Vec::new(); + for cf in self.iter_metadata_files_mut() { + if cf.url().is_none() { + let url = { + let path = cf.path().context("CheckedFile has neither path nor url")?; + let filename = path + .file_name() + .and_then(|f| f.to_str()) + .with_context(|| format!("no filename in path: {}", path.display()))?; + if path.exists() { + url::Url::from_file_path(std::fs::canonicalize(path)?).map_err(|()| { + anyhow::anyhow!("invalid file path for url: {}", path.display()) + })? + } else { + url::Url::parse(&format!("hf://{source}/{filename}")).with_context( + || { + format!( + "synthesizing hf:// from source '{source}', filename '{filename}'" + ) + }, + )? + } + }; + cf.move_to_url(url); + } + let url = cf.url().expect("set above").clone(); + let filename = url + .path() + .rsplit('/') + .find(|s| !s.is_empty()) + .with_context(|| format!("no filename in url: {url}"))? + .to_string(); + entries.push((url.to_string(), cf.clone(), filename)); } - // For TensorBased models, config files are not used - they handle everything in the backend + // Pass 2: fetch + verify + cache + symlink. + let client = reqwest::Client::new(); + let mut fetched = 0usize; + for (uri, expected, filename) in &entries { + let blake3_hex = expected.checksum().hash(); + let blob = blobs.join(blake3_hex); + let cache_hit = blob.exists(); + tracing::debug!( + filename = %filename, + uri = %uri, + cache_hit, + blake3 = %blake3_hex, + "resolving metadata file", + ); + if !cache_hit { + resolve_uri(&client, uri, expected, &blob).await?; + fetched += 1; + } + symlink_force(&blob, &slug_dir.join(filename))?; + } + tracing::debug!( + display_name = %self.display_name, + artifact_count = entries.len(), + fetched, + cache_root = %mdc_cache_root().display(), + "resolved model metadata files", + ); + + // Pass 3: rewrite cf.path to the cache symlink so downstream + // tokenizer/config loaders read from a verified location. + for cf in self.iter_metadata_files_mut() { + cf.update_dir(&slug_dir); + } + Ok(()) + } + + /// Download the files this card needs to work: config.json, tokenizer.json, etc. + pub async fn download_config(&mut self) -> anyhow::Result<()> { + // TensorBased models don't use metadata files — backend handles + // everything. if self.model_type.supports_tensor() { tracing::debug!( display_name = %self.display_name, @@ -892,12 +980,10 @@ impl ModelDeploymentCard { ); return Ok(()); } - - let ignore_weights = true; - let local_path = crate::hub::from_hf(self.source_path(), ignore_weights).await?; - - self.update_dir(&local_path); - Ok(()) + // Single resolve pipeline: every CheckedFile (URL or local + // path, existing or missing) flows through resolve_uri, + // blake3-verifies, lands in the MDC cache. No new/legacy split. + self.resolve_metadata_files().await } /// Re-write all the local disk paths as a URL. Do this before publishing the MDC. @@ -968,63 +1054,6 @@ impl ModelDeploymentCard { Ok(()) } - /// Are all the files we need (tokenizer.json, etc) available locally? - fn has_local_files(&self) -> bool { - let has_model_info = self - .model_info - .as_ref() - .map(|p| p.is_local()) - .unwrap_or(true); - let has_tokenizer = self - .tokenizer - .as_ref() - .map(|p| p.is_local()) - .unwrap_or(true); - let has_prompt_formatter = self - .prompt_formatter - .as_ref() - .map(|p| p.is_local()) - .unwrap_or(true); - let has_chat_template_file = self - .chat_template_file - .as_ref() - .map(|p| p.is_local()) - .unwrap_or(true); - let has_gen_config = self - .gen_config - .as_ref() - .map(|p| p.is_local()) - .unwrap_or(true); - - has_model_info - && has_tokenizer - && has_prompt_formatter - && has_chat_template_file - && has_gen_config - } - - /// Update the directory for files like tokenizer.json be in here. - fn update_dir(&mut self, dir: &Path) { - if let Some(model_info) = self.model_info.as_mut() { - model_info.update_dir(dir); - } - if let Some(tk) = self.tokenizer.as_mut() { - tk.update_dir(dir); - } - if let Some(pf) = self.prompt_formatter.as_mut() { - pf.update_dir(dir); - } - if let Some(gc) = self.gen_config.as_mut() { - gc.update_dir(dir); - } - // If it's a custom chat template we didn't download it, so leave the path untouched - if let Some(ct) = self.chat_template_file.as_mut() - && !ct.is_custom() - { - ct.update_dir(dir); - } - } - /// Creates a ModelDeploymentCard from a local directory path. /// /// Currently HuggingFace format is supported and following files are expected: From 514079fa68dd1a54f61051ddd920dc86072f9330 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 15:46:17 -0700 Subject: [PATCH 05/32] test(model_card): add e2e for download_config + cache pipeline (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locks in the file:// arm of the unified resolve pipeline end-to-end — no network, no mocker, no Python. Uses the existing TinyLlama_v1.1 fixture and asserts: - blobs/ exists for each populated CheckedFile - by-slug/// symlink exists - cf.path() rewritten to point inside the snap dir - canonicalize(symlink) lands inside blobs/ Runs serially because it mutates `\$HOME` (overrides for the duration of the test so `mdc_cache_root()` resolves to a tempdir). The unsafe set_var/remove_var blocks are guarded by `serial_test::serial` to prevent concurrent env access. Refs ai-dynamo/dynamo#8749 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 82 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index ab20db32c9ca..94a29bbfdf73 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -1763,4 +1763,86 @@ mod tests { assert!(super::parse_hf_uri("hf://just-a-name").is_err()); assert!(super::parse_hf_uri("https://example.com/x").is_err()); } + + /// End-to-end: `download_config` on a local-mount MDC normalizes + /// every CheckedFile to `file://`, runs through `resolve_uri`, + /// blake3-verifies, populates the content-addressed cache, and + /// rewrites `cf.path` to the per-slug symlink dir. + /// + /// This is the "shared-mount legacy" path under the unified + /// pipeline — proves the file:// arm of `resolve_uri` works + /// end-to-end without network or mocker. + #[tokio::test] + #[serial_test::serial] // mutates $HOME + async fn download_config_pipelines_local_files_through_cache() -> anyhow::Result<()> { + let fixture = + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/data/sample-models/TinyLlama_v1.1"); + + let temp = tempfile::tempdir()?; + let home_override = temp.path().to_path_buf(); + + // serial_test guards $HOME mutation against parallel test runs; + // restore on drop is enough. + let prior_home = std::env::var_os("HOME"); + // SAFETY: serial_test ensures no concurrent env access from + // other tests in this binary. + unsafe { std::env::set_var("HOME", &home_override) }; + let result = run_pipeline_assertions(&fixture, &home_override).await; + match prior_home { + // SAFETY: same as above. + Some(v) => unsafe { std::env::set_var("HOME", v) }, + None => unsafe { std::env::remove_var("HOME") }, + } + result + } + + async fn run_pipeline_assertions(fixture: &Path, home: &Path) -> anyhow::Result<()> { + let mut mdc = super::ModelDeploymentCard::load_from_disk(fixture, None)?; + let slug = mdc.slug.clone(); + let mdcsum = mdc.mdcsum().to_string(); + + mdc.download_config().await?; + + let mdc_root = home.join(".cache/dynamo/mdc"); + let blobs = mdc_root.join("blobs"); + let snap = mdc_root + .join("by-slug") + .join(slug.to_string()) + .join(&mdcsum); + + assert!(blobs.is_dir(), "blobs dir was not created"); + let blob_count = blobs.read_dir()?.count(); + assert!( + blob_count > 0, + "expected at least one blob to be written, got {blob_count}" + ); + + assert!(snap.is_dir(), "by-slug snap dir was not created: {snap:?}"); + let snap_count = snap.read_dir()?.count(); + assert!( + snap_count > 0, + "expected at least one symlink in {snap:?}, got {snap_count}" + ); + + // Every populated CheckedFile's path was rewritten into the snap + // dir, and the symlink target resolves to a real blob file. + for cf in mdc.iter_metadata_files() { + let path = cf + .path() + .ok_or_else(|| anyhow::anyhow!("post-download cf should have a local path"))?; + assert!( + path.starts_with(&snap), + "cf.path not rewritten into snap dir: {} not under {}", + path.display(), + snap.display() + ); + let resolved = std::fs::canonicalize(path)?; + assert!( + resolved.starts_with(&blobs), + "symlink should resolve to blobs/, got: {}", + resolved.display() + ); + } + Ok(()) + } } From d56ed8cc59d6f61dddb81f9b2a5c7c919b1ccc6d Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 16:06:34 -0700 Subject: [PATCH 06/32] chore(deps): refresh nested Cargo.lock files for libc bump (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lib/bindings/kvbm` and `lib/bindings/python` keep their own Cargo.lock files. Adding `libc` to `lib/llm` in commit 8cffba6281 ripples into both — refresh them so CI's `--locked` clippy doesn't trip. Refs ai-dynamo/dynamo#8749 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/bindings/kvbm/Cargo.lock | 1 + lib/bindings/python/Cargo.lock | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/bindings/kvbm/Cargo.lock b/lib/bindings/kvbm/Cargo.lock index 082d21f2e6d0..b212e49cfbc8 100644 --- a/lib/bindings/kvbm/Cargo.lock +++ b/lib/bindings/kvbm/Cargo.lock @@ -1565,6 +1565,7 @@ dependencies = [ "image", "ipnet", "json-five", + "libc", "lru 0.12.5", "minijinja", "minijinja-contrib", diff --git a/lib/bindings/python/Cargo.lock b/lib/bindings/python/Cargo.lock index e9bb77d0e246..a20488752a8b 100644 --- a/lib/bindings/python/Cargo.lock +++ b/lib/bindings/python/Cargo.lock @@ -2084,6 +2084,7 @@ dependencies = [ "image", "ipnet", "json-five", + "libc", "lru 0.12.5", "memfile", "minijinja", From ea22e63db5a9a2db07b5b1bddc62a9629f8abec6 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 16:45:05 -0700 Subject: [PATCH 07/32] fix(model_card): cfg(unix) BlobLock + bounded http timeouts (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit findings on PR #9057: 1. `BlobLock::acquire` used `AsRawFd` + `libc::flock` unconditionally, which won't compile on non-Unix targets. Wrap the flock-specific block in `#[cfg(unix)]`. The non-Unix path still opens the lock file (so atomic-rename invariants hold for intra-process callers) but skips cross-process locking with a documented caveat — dynamo's frontend ships on Linux, this is build-time portability. 2. `reqwest::Client::new()` in async mode has no default timeout. A stalled metadata endpoint would hang `download_config()` indefinitely. Build the client with explicit `connect_timeout(10s)` + total `timeout(300s)`. 5 min covers a 1 GiB file at slow speeds; realistic metadata files complete in seconds. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 94a29bbfdf73..a7f6e5d682a6 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -203,7 +203,6 @@ struct BlobLock { impl BlobLock { async fn acquire(blob_path: &Path) -> anyhow::Result { - use std::os::fd::AsRawFd; let lock_path = blob_path.with_extension("lock"); let file = tokio::task::spawn_blocking(move || -> anyhow::Result { let f = std::fs::OpenOptions::new() @@ -212,19 +211,28 @@ impl BlobLock { .write(true) .open(&lock_path) .with_context(|| format!("opening lock file {}", lock_path.display()))?; - // SAFETY: `f` is open and owned; `flock` is a stable syscall. - let rc = unsafe { libc::flock(f.as_raw_fd(), libc::LOCK_EX) }; - if rc != 0 { - anyhow::bail!( - "flock {} failed: {}", - lock_path.display(), - std::io::Error::last_os_error() - ); + #[cfg(unix)] + { + use std::os::fd::AsRawFd; + // SAFETY: `f` is open and owned; `flock` is a stable syscall. + let rc = unsafe { libc::flock(f.as_raw_fd(), libc::LOCK_EX) }; + if rc != 0 { + anyhow::bail!( + "flock {} failed: {}", + lock_path.display(), + std::io::Error::last_os_error() + ); + } } + // On non-Unix targets we currently rely on the per-call + // unique tmp + atomic rename to give intra-process safety; + // cross-process locking would need LockFileEx (winapi). + // The dynamo frontend ships on Linux; this path is for + // build-time portability only. Ok(f) }) .await - .context("flock spawn_blocking task panicked")??; + .context("blob lock spawn_blocking task panicked")??; Ok(Self { _file: file }) } } @@ -934,7 +942,15 @@ impl ModelDeploymentCard { } // Pass 2: fetch + verify + cache + symlink. - let client = reqwest::Client::new(); + // Bounded timeouts so a stalled metadata endpoint can't hang + // download_config indefinitely. 5 min total covers a 1 GiB + // file at slow speeds; realistic metadata files complete in + // seconds. + let client = reqwest::Client::builder() + .connect_timeout(std::time::Duration::from_secs(10)) + .timeout(std::time::Duration::from_secs(300)) + .build() + .context("building http client for metadata fetch")?; let mut fetched = 0usize; for (uri, expected, filename) in &entries { let blake3_hex = expected.checksum().hash(); From fa5d981e38dbb7c273ea25357557161b7aa47b48 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sat, 2 May 2026 22:14:14 -0700 Subject: [PATCH 08/32] debug(model_card): log MDC slot population + gen_config disk-vs-loaded skew (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnostic for the trtllm Multi-GPU `raw_embeddings_epd-2` failure on PR #9057. The frontend errors with "missing eos_token_id in config.json and generation_config.json, cannot load" because slug_dir lacks generation_config.json. Two unknowns to surface: 1. **Worker side:** when does `from_repo_checkout` settle on `gen_config = None`? `GenerationConfig::from_disk(local_path).ok()` silently swallows errors. If the file IS on disk but loading failed, that's a real bug — log it as a warning. If the file is genuinely absent, log at debug. Either way we get attribution. 2. **Frontend side:** when `resolve_metadata_files` runs, log which slots were populated on the MDC at info level. A None gen_config slot here means no symlink in slug_dir, which is exactly the condition that produces the eos_token_id error downstream. Both diagnostics ship at info/warn so they'll show up in CI logs without extra config. Will revert the info-level slot log to debug once the test failure is understood. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 42 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index a7f6e5d682a6..63acfa1b6f93 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -905,6 +905,21 @@ impl ModelDeploymentCard { let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; + // Visibility for slot population — a None slot here means no symlink + // for that file in slug_dir, which can surface downstream as a missing + // generation_config.json / chat_template.jinja / etc. Logged at info + // for now so gh-8749 PR2 reviewers can see slot coverage in CI logs. + tracing::info!( + display_name = %self.display_name, + slug = %self.slug, + model_info = self.model_info.is_some(), + tokenizer = self.tokenizer.is_some(), + prompt_formatter = self.prompt_formatter.is_some(), + chat_template_file = self.chat_template_file.is_some(), + gen_config = self.gen_config.is_some(), + "resolving MDC metadata slots", + ); + // Pass 1: normalize to URL form + snapshot for the async loop. let mut entries: Vec<(String, CheckedFile, String)> = Vec::new(); for cf in self.iter_metadata_files_mut() { @@ -1115,10 +1130,35 @@ impl ModelDeploymentCard { let is_mistral_model = is_exclusively_mistral_model(local_path); let (model_info, tokenizer, gen_config, prompt_formatter) = if !is_mistral_model { + let gen_config = match GenerationConfig::from_disk(local_path) { + Ok(g) => Some(g), + Err(err) => { + // generation_config.json is optional — many models ship without + // it. But if the file IS on disk and we still failed to load it, + // surface that so a missing eos_token_id later is easier to + // attribute. The legacy frontend path silently fell through to + // hub::from_hf which would re-fetch the file; the unified pipeline + // doesn't, so a None slot here means a missing symlink in slug_dir. + let path = local_path.join("generation_config.json"); + if path.exists() { + tracing::warn!( + %err, + path = %path.display(), + "generation_config.json exists on disk but CheckedFile::from_disk failed", + ); + } else { + tracing::debug!( + "generation_config.json absent from {}; gen_config slot will be None", + local_path.display() + ); + } + None + } + }; ( Some(ModelInfoType::from_disk(local_path)?), TokenizerKind::from_disk(local_path)?, - GenerationConfig::from_disk(local_path).ok(), + gen_config, PromptFormatterArtifact::from_disk(local_path)?, ) } else { From fc6f9cf5764b49b70de992cb224c0bdfae65664a Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 02:30:16 -0700 Subject: [PATCH 09/32] Revert "debug(model_card): log MDC slot population + gen_config disk-vs-loaded skew (gh-8749)" This reverts commit f4fd27b9c9cce5d6564491d0b1f218708da6968d. Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 42 +-------------------------------------- 1 file changed, 1 insertion(+), 41 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 63acfa1b6f93..a7f6e5d682a6 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -905,21 +905,6 @@ impl ModelDeploymentCard { let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - // Visibility for slot population — a None slot here means no symlink - // for that file in slug_dir, which can surface downstream as a missing - // generation_config.json / chat_template.jinja / etc. Logged at info - // for now so gh-8749 PR2 reviewers can see slot coverage in CI logs. - tracing::info!( - display_name = %self.display_name, - slug = %self.slug, - model_info = self.model_info.is_some(), - tokenizer = self.tokenizer.is_some(), - prompt_formatter = self.prompt_formatter.is_some(), - chat_template_file = self.chat_template_file.is_some(), - gen_config = self.gen_config.is_some(), - "resolving MDC metadata slots", - ); - // Pass 1: normalize to URL form + snapshot for the async loop. let mut entries: Vec<(String, CheckedFile, String)> = Vec::new(); for cf in self.iter_metadata_files_mut() { @@ -1130,35 +1115,10 @@ impl ModelDeploymentCard { let is_mistral_model = is_exclusively_mistral_model(local_path); let (model_info, tokenizer, gen_config, prompt_formatter) = if !is_mistral_model { - let gen_config = match GenerationConfig::from_disk(local_path) { - Ok(g) => Some(g), - Err(err) => { - // generation_config.json is optional — many models ship without - // it. But if the file IS on disk and we still failed to load it, - // surface that so a missing eos_token_id later is easier to - // attribute. The legacy frontend path silently fell through to - // hub::from_hf which would re-fetch the file; the unified pipeline - // doesn't, so a None slot here means a missing symlink in slug_dir. - let path = local_path.join("generation_config.json"); - if path.exists() { - tracing::warn!( - %err, - path = %path.display(), - "generation_config.json exists on disk but CheckedFile::from_disk failed", - ); - } else { - tracing::debug!( - "generation_config.json absent from {}; gen_config slot will be None", - local_path.display() - ); - } - None - } - }; ( Some(ModelInfoType::from_disk(local_path)?), TokenizerKind::from_disk(local_path)?, - gen_config, + GenerationConfig::from_disk(local_path).ok(), PromptFormatterArtifact::from_disk(local_path)?, ) } else { From b07bb03f6fa5fec2221b5279ce7015d243e77476 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 07:20:50 -0700 Subject: [PATCH 10/32] fix(model_card): keep cf state untouched in resolve Pass 1 (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass 1 was rewriting cf.path to a `file://` URL when the worker's path was a local file. For HF cache snapshots that path resolves through a symlink into `blobs/`, so the URL's basename became the blob hash. Pass 3's `update_dir` then derived the cf's new path from that basename, leaving cf.path at `/` instead of `/` — the symlinks Pass 2 placed at `/` were unreachable from cf.path, and downstream code reading siblings by name (e.g. `HFConfig` looking for `generation_config.json` next to `config.json`) hit "Failed to open file". Fix mirrors the worker-side fix in `move_to_self_host` from the capstone: don't mutate cf to a canonicalized URL — construct the fetch URI as a local string and leave cf.path as the worker published it. Pass 3's `update_dir` now operates on the original PathBuf and naturally writes back the logical filename. Same regression class CodeRabbit caught for the http:// arm, just on the file:// arm. The existing `download_config_pipelines_local_files _through_cache` test now uses an HF-cache-style symlink fixture so this path stays covered. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 172 ++++++++++++++++++-------------------- 1 file changed, 83 insertions(+), 89 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index a7f6e5d682a6..74bfac8c54a4 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -887,58 +887,50 @@ impl ModelDeploymentCard { /// Resolve every populated metadata `CheckedFile` through the /// content-addressed MDC cache, blake3-verifying every byte. - /// - /// Pass 1 — for each slot, normalize `cf.path()` to a URL form - /// (`file://` if it exists locally, `hf:///` - /// otherwise, or unchanged if already a URL), then snapshot - /// `(uri, expected, filename)` for the async fetch loop. - /// Pass 2 — `resolve_uri` per slot if the blob isn't already - /// cached; symlink into `by-slug///`. - /// Pass 3 — rewrite each `cf.path` to point at the cache symlink. - /// - /// Bypasses `update_dir` because its `is_custom` exclusion is for - /// the legacy `hub::from_hf` fall-through and would leave URLs in - /// the MDC for custom templates. + /// Walks `iter_metadata_files_mut` directly rather than going + /// through `MDC::update_dir`, whose `is_custom` exclusion would + /// skip user-supplied custom chat templates. async fn resolve_metadata_files(&mut self) -> anyhow::Result<()> { let source = self.source_path().to_string(); let mdcsum = self.mdcsum().to_string(); let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - // Pass 1: normalize to URL form + snapshot for the async loop. + // Snapshot the logical filename + a fetchable URI for each + // slot. We don't mutate cf here: Pass 3's `update_dir` runs + // against the original cf.path, which preserves the logical + // filename even when the worker's path is an HF cache symlink + // into `blobs/`. let mut entries: Vec<(String, CheckedFile, String)> = Vec::new(); - for cf in self.iter_metadata_files_mut() { - if cf.url().is_none() { - let url = { - let path = cf.path().context("CheckedFile has neither path nor url")?; - let filename = path - .file_name() - .and_then(|f| f.to_str()) - .with_context(|| format!("no filename in path: {}", path.display()))?; - if path.exists() { - url::Url::from_file_path(std::fs::canonicalize(path)?).map_err(|()| { + for cf in self.iter_metadata_files() { + let filename = if let Some(p) = cf.path() { + p.file_name() + .and_then(|f| f.to_str()) + .with_context(|| format!("no filename in path: {}", p.display()))? + .to_string() + } else { + let url = cf.url().expect("either path or url is set"); + url.path() + .rsplit('/') + .find(|s| !s.is_empty()) + .with_context(|| format!("no filename in url: {url}"))? + .to_string() + }; + let uri = if let Some(u) = cf.url() { + u.to_string() + } else { + let path = cf.path().expect("path or url"); + if path.exists() { + url::Url::from_file_path(std::fs::canonicalize(path)?) + .map_err(|()| { anyhow::anyhow!("invalid file path for url: {}", path.display()) })? - } else { - url::Url::parse(&format!("hf://{source}/{filename}")).with_context( - || { - format!( - "synthesizing hf:// from source '{source}', filename '{filename}'" - ) - }, - )? - } - }; - cf.move_to_url(url); - } - let url = cf.url().expect("set above").clone(); - let filename = url - .path() - .rsplit('/') - .find(|s| !s.is_empty()) - .with_context(|| format!("no filename in url: {url}"))? - .to_string(); - entries.push((url.to_string(), cf.clone(), filename)); + .to_string() + } else { + format!("hf://{source}/{filename}") + } + }; + entries.push((uri, cf.clone(), filename)); } // Pass 2: fetch + verify + cache + symlink. @@ -1623,7 +1615,7 @@ fn check_valid_local_repo_path(path: impl AsRef) -> Result<()> { mod tests { use super::HFConfig; use std::collections::HashSet; - use std::path::Path; + use std::path::{Path, PathBuf}; #[test] pub fn test_config_json_llama3() -> anyhow::Result<()> { @@ -1788,76 +1780,78 @@ mod tests { /// This is the "shared-mount legacy" path under the unified /// pipeline — proves the file:// arm of `resolve_uri` works /// end-to-end without network or mocker. + /// Build an HF-cache-style snapshot fixture from the TinyLlama + /// sample model: per-file symlinks into a sibling `blobs/` + /// dir. The symlink layout is what triggers the canonicalize trap + /// `resolve_metadata_files` has to handle. + fn hf_cache_fixture(workspace: &Path) -> anyhow::Result { + use std::hash::{Hash, Hasher}; + let snapshot = workspace.join("snapshots/abc"); + let blobs = workspace.join("blobs"); + std::fs::create_dir_all(&snapshot)?; + std::fs::create_dir_all(&blobs)?; + let src = + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/data/sample-models/TinyLlama_v1.1"); + for entry in std::fs::read_dir(&src)? { + let entry = entry?; + let name = entry.file_name(); + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + name.to_string_lossy().hash(&mut hasher); + let blob = format!("blob-{:x}", hasher.finish()); + std::fs::copy(entry.path(), blobs.join(&blob))?; + #[cfg(unix)] + std::os::unix::fs::symlink( + Path::new("../..").join("blobs").join(&blob), + snapshot.join(name), + )?; + } + Ok(snapshot) + } + #[tokio::test] #[serial_test::serial] // mutates $HOME async fn download_config_pipelines_local_files_through_cache() -> anyhow::Result<()> { - let fixture = - Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/data/sample-models/TinyLlama_v1.1"); - - let temp = tempfile::tempdir()?; - let home_override = temp.path().to_path_buf(); + let workspace = tempfile::tempdir()?; + let snapshot = hf_cache_fixture(workspace.path())?; + let temp_home = tempfile::tempdir()?; - // serial_test guards $HOME mutation against parallel test runs; - // restore on drop is enough. let prior_home = std::env::var_os("HOME"); - // SAFETY: serial_test ensures no concurrent env access from - // other tests in this binary. - unsafe { std::env::set_var("HOME", &home_override) }; - let result = run_pipeline_assertions(&fixture, &home_override).await; + // SAFETY: serial_test guards against parallel env access. + unsafe { std::env::set_var("HOME", temp_home.path()) }; + let result = run_pipeline_assertions(&snapshot, temp_home.path()).await; match prior_home { - // SAFETY: same as above. Some(v) => unsafe { std::env::set_var("HOME", v) }, None => unsafe { std::env::remove_var("HOME") }, } result } - async fn run_pipeline_assertions(fixture: &Path, home: &Path) -> anyhow::Result<()> { - let mut mdc = super::ModelDeploymentCard::load_from_disk(fixture, None)?; + async fn run_pipeline_assertions(snapshot: &Path, home: &Path) -> anyhow::Result<()> { + let mut mdc = super::ModelDeploymentCard::load_from_disk(snapshot, None)?; let slug = mdc.slug.clone(); let mdcsum = mdc.mdcsum().to_string(); mdc.download_config().await?; - let mdc_root = home.join(".cache/dynamo/mdc"); - let blobs = mdc_root.join("blobs"); - let snap = mdc_root - .join("by-slug") + let blobs = home.join(".cache/dynamo/mdc/blobs"); + let snap = home + .join(".cache/dynamo/mdc/by-slug") .join(slug.to_string()) .join(&mdcsum); - assert!(blobs.is_dir(), "blobs dir was not created"); - let blob_count = blobs.read_dir()?.count(); - assert!( - blob_count > 0, - "expected at least one blob to be written, got {blob_count}" - ); - - assert!(snap.is_dir(), "by-slug snap dir was not created: {snap:?}"); - let snap_count = snap.read_dir()?.count(); - assert!( - snap_count > 0, - "expected at least one symlink in {snap:?}, got {snap_count}" - ); + // slug_dir is named by logical filename — not the HF cache + // blob basename — so consumers reading siblings (e.g. + // `parent.join("generation_config.json")`) find them. + assert!(snap.join("config.json").exists()); + assert!(snap.join("tokenizer.json").exists()); + assert!(snap.join("generation_config.json").exists()); - // Every populated CheckedFile's path was rewritten into the snap - // dir, and the symlink target resolves to a real blob file. for cf in mdc.iter_metadata_files() { let path = cf .path() .ok_or_else(|| anyhow::anyhow!("post-download cf should have a local path"))?; - assert!( - path.starts_with(&snap), - "cf.path not rewritten into snap dir: {} not under {}", - path.display(), - snap.display() - ); - let resolved = std::fs::canonicalize(path)?; - assert!( - resolved.starts_with(&blobs), - "symlink should resolve to blobs/, got: {}", - resolved.display() - ); + assert!(path.starts_with(&snap)); + assert!(std::fs::canonicalize(path)?.starts_with(&blobs)); } Ok(()) } From a7b7e1cdb05dd5e7d4b19007a52b13737077b047 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 08:10:44 -0700 Subject: [PATCH 11/32] refactor(model_card): extract checked_file_basename/uri + use absolute (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass 1 collapses to one map() call with two helpers. Replaces `std::fs::canonicalize` with `std::path::absolute` — same effect for `Url::from_file_path` (needs absolute) without an extra syscall or symlink-follow. Drops verbose comments that were stating the obvious. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 80 +++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 46 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 74bfac8c54a4..f05d9038759a 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -481,6 +481,30 @@ fn parse_hf_uri(uri: &str) -> anyhow::Result<(String, String)> { Ok((repo.to_string(), filename.to_string())) } +fn checked_file_basename(cf: &CheckedFile) -> Option { + cf.path() + .and_then(|p| p.file_name().and_then(|f| f.to_str())) + .or_else(|| { + cf.url() + .and_then(|u| u.path().rsplit('/').find(|s| !s.is_empty())) + }) + .map(String::from) +} + +fn checked_file_uri(cf: &CheckedFile, source: &str, filename: &str) -> anyhow::Result { + if let Some(u) = cf.url() { + return Ok(u.to_string()); + } + let path = cf.path().expect("path or url"); + if path.exists() { + Ok(url::Url::from_file_path(std::path::absolute(path)?) + .map_err(|()| anyhow::anyhow!("invalid file path: {}", path.display()))? + .to_string()) + } else { + Ok(format!("hf://{source}/{filename}")) + } +} + fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) @@ -885,59 +909,23 @@ impl ModelDeploymentCard { out } - /// Resolve every populated metadata `CheckedFile` through the - /// content-addressed MDC cache, blake3-verifying every byte. - /// Walks `iter_metadata_files_mut` directly rather than going - /// through `MDC::update_dir`, whose `is_custom` exclusion would - /// skip user-supplied custom chat templates. async fn resolve_metadata_files(&mut self) -> anyhow::Result<()> { let source = self.source_path().to_string(); let mdcsum = self.mdcsum().to_string(); let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - // Snapshot the logical filename + a fetchable URI for each - // slot. We don't mutate cf here: Pass 3's `update_dir` runs - // against the original cf.path, which preserves the logical - // filename even when the worker's path is an HF cache symlink - // into `blobs/`. - let mut entries: Vec<(String, CheckedFile, String)> = Vec::new(); - for cf in self.iter_metadata_files() { - let filename = if let Some(p) = cf.path() { - p.file_name() - .and_then(|f| f.to_str()) - .with_context(|| format!("no filename in path: {}", p.display()))? - .to_string() - } else { - let url = cf.url().expect("either path or url is set"); - url.path() - .rsplit('/') - .find(|s| !s.is_empty()) - .with_context(|| format!("no filename in url: {url}"))? - .to_string() - }; - let uri = if let Some(u) = cf.url() { - u.to_string() - } else { - let path = cf.path().expect("path or url"); - if path.exists() { - url::Url::from_file_path(std::fs::canonicalize(path)?) - .map_err(|()| { - anyhow::anyhow!("invalid file path for url: {}", path.display()) - })? - .to_string() - } else { - format!("hf://{source}/{filename}") - } - }; - entries.push((uri, cf.clone(), filename)); - } + let entries: Vec<(String, CheckedFile, String)> = self + .iter_metadata_files() + .into_iter() + .map(|cf| { + let filename = + checked_file_basename(cf).with_context(|| format!("no filename for {cf:?}"))?; + let uri = checked_file_uri(cf, &source, &filename)?; + Ok((uri, cf.clone(), filename)) + }) + .collect::>()?; - // Pass 2: fetch + verify + cache + symlink. - // Bounded timeouts so a stalled metadata endpoint can't hang - // download_config indefinitely. 5 min total covers a 1 GiB - // file at slow speeds; realistic metadata files complete in - // seconds. let client = reqwest::Client::builder() .connect_timeout(std::time::Duration::from_secs(10)) .timeout(std::time::Duration::from_secs(300)) From 30f646ea7e6fc3593abe22bb30c711f8fa014a7e Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 08:20:33 -0700 Subject: [PATCH 12/32] =?UTF-8?q?refactor(local=5Fmodel):=20mirror=20front?= =?UTF-8?q?end=20=E2=80=94=20use=20std::path::absolute=20(gh-8749)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worker `move_to_self_host` swaps `fs::canonicalize` for `std::path::absolute`. Filename derivation is unchanged (still pre- canonicalize per capstone 262f314e22). Now both worker and frontend URL/registry construction follow the same pattern: pre-canonicalize filename + non-symlink-resolving absolute path. The http handler's per-request symlink follow is the only behavioral delta; HF cache blobs are append-only so the resolved path doesn't go stale. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/local_model.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/llm/src/local_model.rs b/lib/llm/src/local_model.rs index 000f87a300b3..7bab368ac377 100644 --- a/lib/llm/src/local_model.rs +++ b/lib/llm/src/local_model.rs @@ -564,9 +564,6 @@ impl LocalModel { let Some(local_path) = cf.path().map(Path::to_path_buf) else { continue; }; - // Filename from the original path — HF cache symlinks would - // otherwise canonicalize to the LFS SHA1 and break downstream - // lookups by literal name (e.g. `parent.join("generation_config.json")`). let Some(filename) = local_path .file_name() .and_then(|f| f.to_str()) @@ -574,14 +571,13 @@ impl LocalModel { else { continue; }; - // Canonicalize so the handler serves the resolved HF blob, not the symlink. - let absolute = match fs::canonicalize(&local_path) { + let absolute = match std::path::absolute(&local_path) { Ok(p) => p, Err(err) => { tracing::warn!( path = %local_path.display(), %err, - "failed to canonicalize self-host metadata path; skipping", + "failed to absolutize self-host metadata path; skipping", ); continue; } From 81f56fe67657c57b3edfef0bbf8f26dc505aee4e Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 08:31:46 -0700 Subject: [PATCH 13/32] refactor(model_card): derive filename from URI instead of cf (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that `absolute` keeps the logical filename in the URL (the bug class fixed by 58e34ac108), the URI is the single source of truth for the filename — no need to derive it separately from `cf` and thread it through entries. `checked_file_basename` removed; `checked_file_uri` no longer takes filename. New `uri_basename` works on any URI string and lives next to `parse_hf_uri`. Pass 1 entries shrink from `(uri, cf, filename)` to `(uri, cf)`; Pass 2 calls `uri_basename(uri)` once per slot. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 40 +++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index f05d9038759a..edf42d7556e6 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -481,17 +481,7 @@ fn parse_hf_uri(uri: &str) -> anyhow::Result<(String, String)> { Ok((repo.to_string(), filename.to_string())) } -fn checked_file_basename(cf: &CheckedFile) -> Option { - cf.path() - .and_then(|p| p.file_name().and_then(|f| f.to_str())) - .or_else(|| { - cf.url() - .and_then(|u| u.path().rsplit('/').find(|s| !s.is_empty())) - }) - .map(String::from) -} - -fn checked_file_uri(cf: &CheckedFile, source: &str, filename: &str) -> anyhow::Result { +fn checked_file_uri(cf: &CheckedFile, source: &str) -> anyhow::Result { if let Some(u) = cf.url() { return Ok(u.to_string()); } @@ -501,10 +491,24 @@ fn checked_file_uri(cf: &CheckedFile, source: &str, filename: &str) -> anyhow::R .map_err(|()| anyhow::anyhow!("invalid file path: {}", path.display()))? .to_string()) } else { + let filename = path + .file_name() + .and_then(|f| f.to_str()) + .with_context(|| format!("no filename in path: {}", path.display()))?; Ok(format!("hf://{source}/{filename}")) } } +fn uri_basename(uri: &str) -> anyhow::Result { + url::Url::parse(uri) + .with_context(|| format!("parsing uri: {uri}"))? + .path() + .rsplit('/') + .find(|s| !s.is_empty()) + .map(String::from) + .with_context(|| format!("no basename in uri: {uri}")) +} + fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) @@ -915,15 +919,10 @@ impl ModelDeploymentCard { let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - let entries: Vec<(String, CheckedFile, String)> = self + let entries: Vec<(String, CheckedFile)> = self .iter_metadata_files() .into_iter() - .map(|cf| { - let filename = - checked_file_basename(cf).with_context(|| format!("no filename for {cf:?}"))?; - let uri = checked_file_uri(cf, &source, &filename)?; - Ok((uri, cf.clone(), filename)) - }) + .map(|cf| Ok((checked_file_uri(cf, &source)?, cf.clone()))) .collect::>()?; let client = reqwest::Client::builder() @@ -932,7 +931,8 @@ impl ModelDeploymentCard { .build() .context("building http client for metadata fetch")?; let mut fetched = 0usize; - for (uri, expected, filename) in &entries { + for (uri, expected) in &entries { + let filename = uri_basename(uri)?; let blake3_hex = expected.checksum().hash(); let blob = blobs.join(blake3_hex); let cache_hit = blob.exists(); @@ -947,7 +947,7 @@ impl ModelDeploymentCard { resolve_uri(&client, uri, expected, &blob).await?; fetched += 1; } - symlink_force(&blob, &slug_dir.join(filename))?; + symlink_force(&blob, &slug_dir.join(&filename))?; } tracing::debug!( display_name = %self.display_name, From a3f380fefe4a3728eb94fbbfa315fea3de3495ef Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 08:34:31 -0700 Subject: [PATCH 14/32] refactor(model_card): use url::path_segments for uri_basename (gh-8749) `Url::path_segments().rfind(non-empty)` reads more directly than `path().rsplit('/').find(non-empty)` and uses the URL crate's path parsing rather than ad-hoc string splitting. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index edf42d7556e6..354768d6e3f5 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -502,9 +502,8 @@ fn checked_file_uri(cf: &CheckedFile, source: &str) -> anyhow::Result { fn uri_basename(uri: &str) -> anyhow::Result { url::Url::parse(uri) .with_context(|| format!("parsing uri: {uri}"))? - .path() - .rsplit('/') - .find(|s| !s.is_empty()) + .path_segments() + .and_then(|mut s| s.rfind(|s| !s.is_empty())) .map(String::from) .with_context(|| format!("no basename in uri: {uri}")) } From 5b1c50b442b6e20c969ae9aef9303744f5c3011e Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 08:49:46 -0700 Subject: [PATCH 15/32] test(model_card): collapse duplicated test scaffolding (gh-8749) - `assert_resolve_uri_rejects(body, declared_size, expected_err)` swallows the duplicated mockito + tempdir + assert plumbing in the two http-error tests. - `tempfile::tempdir()` replaces `temp_dir().join(uuid)` + manual `remove_dir_all`; auto-cleans on drop. - `temp_env::async_with_vars` replaces the `prior_home / set_var / match / restore-or-remove` boilerplate around `download_config_*`. - `run_pipeline_assertions` inlined (single caller). No coverage change; -68 lines. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 180 ++++++++++++-------------------------- 1 file changed, 56 insertions(+), 124 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 354768d6e3f5..78707f1b86d8 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -1657,94 +1657,49 @@ mod tests { Ok(()) } - /// Build a `CheckedFile` for testing with the given uri + checksum + size. - fn test_cf(uri: &str, checksum_hex: &str, size: u64) -> super::CheckedFile { - let json = serde_json::json!({ + fn test_cf(uri: &str, size: u64) -> super::CheckedFile { + serde_json::from_value(serde_json::json!({ "path": uri, - "checksum": format!("blake3:{checksum_hex}"), + "checksum": format!("blake3:{}", "0".repeat(64)), "size": size, - }); - serde_json::from_value(json).unwrap() + })) + .unwrap() } - /// blake3 mismatch errors and writes nothing. - #[tokio::test] - async fn resolve_uri_http_rejects_checksum_mismatch() { + async fn assert_resolve_uri_rejects(body: &[u8], declared_size: u64, expected_err: &str) { let mut server = mockito::Server::new_async().await; - let body = b"hello world"; let _m = server - .mock("GET", "/file.txt") + .mock("GET", "/f") .with_status(200) .with_body(body) .create_async() .await; - - let dir = std::env::temp_dir().join(format!("dynamo-mdc-test-{}", uuid::Uuid::new_v4())); - std::fs::create_dir_all(&dir).unwrap(); - let dest = dir.join("file.txt"); - - let url = format!("{}/file.txt", server.url()); - let expected = test_cf( + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("f"); + let url = format!("{}/f", server.url()); + let result = super::resolve_uri( + &reqwest::Client::new(), &url, - "0000000000000000000000000000000000000000000000000000000000000000", - body.len() as u64, - ); - let result = super::resolve_uri(&reqwest::Client::new(), &url, &expected, &dest).await; - - assert!(result.is_err(), "checksum mismatch must error"); - let msg = result.err().unwrap().to_string(); + &test_cf(&url, declared_size), + &dest, + ) + .await; + let msg = result.expect_err("expected error").to_string(); assert!( - msg.contains("checksum mismatch"), - "expected checksum-mismatch error, got: {msg}" - ); - assert!( - !dest.exists(), - "no file should be written on checksum mismatch" + msg.contains(expected_err), + "want `{expected_err}` in: {msg}" ); + assert!(!dest.exists(), "no file should be written"); + } - let _ = std::fs::remove_dir_all(&dir); + #[tokio::test] + async fn resolve_uri_http_rejects_checksum_mismatch() { + assert_resolve_uri_rejects(b"hello world", 11, "checksum mismatch").await; } - /// Oversize http body errors before the in-memory buffer can grow - /// past the expected size, so a misbehaving worker can't OOM the - /// frontend before blake3 verification fails. #[tokio::test] async fn resolve_uri_http_rejects_oversize_body() { - let mut server = mockito::Server::new_async().await; - let body = b"hello hello hello hello hello hello"; // 35 bytes - let _m = server - .mock("GET", "/big.txt") - .with_status(200) - .with_body(body) - .create_async() - .await; - - let dir = std::env::temp_dir().join(format!("dynamo-mdc-test-{}", uuid::Uuid::new_v4())); - std::fs::create_dir_all(&dir).unwrap(); - let dest = dir.join("big.txt"); - - let url = format!("{}/big.txt", server.url()); - // Cap is 8 bytes (declared size); body is 35. We trip the - // size cap before reaching blake3 verification. - let expected = test_cf( - &url, - "0000000000000000000000000000000000000000000000000000000000000000", - 8, - ); - let result = super::resolve_uri(&reqwest::Client::new(), &url, &expected, &dest).await; - - assert!(result.is_err(), "oversize body must error"); - let msg = result.err().unwrap().to_string(); - assert!( - msg.contains("exceeds cap"), - "expected size-cap error, got: {msg}" - ); - assert!( - !dest.exists(), - "no file should be written on size-cap rejection" - ); - - let _ = std::fs::remove_dir_all(&dir); + assert_resolve_uri_rejects(b"x".repeat(35).as_slice(), 8, "exceeds cap").await; } /// `parse_hf_uri` round-trip — `hf://repo/filename` parses into @@ -1759,18 +1714,9 @@ mod tests { assert!(super::parse_hf_uri("https://example.com/x").is_err()); } - /// End-to-end: `download_config` on a local-mount MDC normalizes - /// every CheckedFile to `file://`, runs through `resolve_uri`, - /// blake3-verifies, populates the content-addressed cache, and - /// rewrites `cf.path` to the per-slug symlink dir. - /// - /// This is the "shared-mount legacy" path under the unified - /// pipeline — proves the file:// arm of `resolve_uri` works - /// end-to-end without network or mocker. - /// Build an HF-cache-style snapshot fixture from the TinyLlama - /// sample model: per-file symlinks into a sibling `blobs/` - /// dir. The symlink layout is what triggers the canonicalize trap - /// `resolve_metadata_files` has to handle. + /// HF-cache-style snapshot of TinyLlama: per-file symlinks into a + /// sibling `blobs/` dir. The symlink layout is what triggers + /// the canonicalize trap `resolve_metadata_files` has to handle. fn hf_cache_fixture(workspace: &Path) -> anyhow::Result { use std::hash::{Hash, Hasher}; let snapshot = workspace.join("snapshots/abc"); @@ -1796,50 +1742,36 @@ mod tests { } #[tokio::test] - #[serial_test::serial] // mutates $HOME + #[serial_test::serial] async fn download_config_pipelines_local_files_through_cache() -> anyhow::Result<()> { let workspace = tempfile::tempdir()?; let snapshot = hf_cache_fixture(workspace.path())?; - let temp_home = tempfile::tempdir()?; - - let prior_home = std::env::var_os("HOME"); - // SAFETY: serial_test guards against parallel env access. - unsafe { std::env::set_var("HOME", temp_home.path()) }; - let result = run_pipeline_assertions(&snapshot, temp_home.path()).await; - match prior_home { - Some(v) => unsafe { std::env::set_var("HOME", v) }, - None => unsafe { std::env::remove_var("HOME") }, - } - result - } - - async fn run_pipeline_assertions(snapshot: &Path, home: &Path) -> anyhow::Result<()> { - let mut mdc = super::ModelDeploymentCard::load_from_disk(snapshot, None)?; - let slug = mdc.slug.clone(); - let mdcsum = mdc.mdcsum().to_string(); - - mdc.download_config().await?; - - let blobs = home.join(".cache/dynamo/mdc/blobs"); - let snap = home - .join(".cache/dynamo/mdc/by-slug") - .join(slug.to_string()) - .join(&mdcsum); - - // slug_dir is named by logical filename — not the HF cache - // blob basename — so consumers reading siblings (e.g. - // `parent.join("generation_config.json")`) find them. - assert!(snap.join("config.json").exists()); - assert!(snap.join("tokenizer.json").exists()); - assert!(snap.join("generation_config.json").exists()); - - for cf in mdc.iter_metadata_files() { - let path = cf - .path() - .ok_or_else(|| anyhow::anyhow!("post-download cf should have a local path"))?; - assert!(path.starts_with(&snap)); - assert!(std::fs::canonicalize(path)?.starts_with(&blobs)); - } - Ok(()) + let home = tempfile::tempdir()?; + let home_path = home.path().to_path_buf(); + + temp_env::async_with_vars([("HOME", Some(home.path()))], async { + let mut mdc = super::ModelDeploymentCard::load_from_disk(&snapshot, None)?; + let slug = mdc.slug.clone(); + let mdcsum = mdc.mdcsum().to_string(); + mdc.download_config().await?; + + let blobs = home_path.join(".cache/dynamo/mdc/blobs"); + let snap = home_path + .join(".cache/dynamo/mdc/by-slug") + .join(slug.to_string()) + .join(&mdcsum); + + assert!(snap.join("config.json").exists()); + assert!(snap.join("tokenizer.json").exists()); + assert!(snap.join("generation_config.json").exists()); + + for cf in mdc.iter_metadata_files() { + let path = cf.path().expect("post-download local path"); + assert!(path.starts_with(&snap)); + assert!(std::fs::canonicalize(path)?.starts_with(&blobs)); + } + Ok::<_, anyhow::Error>(()) + }) + .await } } From 2be7de5ede20ecdaf396d6983ab8a64b9843bb74 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Sun, 3 May 2026 09:09:35 -0700 Subject: [PATCH 16/32] docs(model_card): TODO for extra_files slot (vllm pass-through) (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External preprocessors (vllm/sglang) load via `from_pretrained(slug_dir)` and may need files outside the typed MDC slots — `preprocessor_config.json`, `special_tokens_map.json`, etc. Plan tracked in gh-8749-self-hosted-metadata/findings/extra-metadata-files.md; landing as a follow-up PR after #9057. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 78707f1b86d8..8953312b2c93 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -855,6 +855,13 @@ impl ModelDeploymentCard { /// Walk populated metadata `CheckedFile` slots in deterministic /// order: model_info, tokenizer, prompt_formatter, /// chat_template_file, gen_config. + /// + /// TODO(gh-8749): external preprocessors (vllm/sglang) read + /// `from_pretrained(slug_dir)` and may expect siblings outside the + /// typed slots — `preprocessor_config.json`, `special_tokens_map.json`, + /// `added_tokens.json`, etc. Add an `extra_files: Vec` + /// MDC field so the worker can advertise everything in its model dir + /// minus weights. Frontend pipeline is already generic over slot count. pub fn iter_metadata_files(&self) -> Vec<&CheckedFile> { let mut out: Vec<&CheckedFile> = Vec::with_capacity(5); if let Some(m) = self.model_info.as_ref() { From b9f85862585722da9380619997f4e6727eec33c8 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 4 May 2026 07:51:23 -0700 Subject: [PATCH 17/32] docs(frontend): TODO to consume MDC cache from sglang/vllm processors (gh-8749) Both `sglang_processor.chat_engine_factory` and `vllm_processor.chat_engine_factory` re-run `fetch_model(source_path)` and load tokenizer/processor by HF identifier, ignoring the MDC cache populated by `download_config`. The cache is correct and blake3-verified; the factories just don't read from it. Marking for follow-up so the duplicate download can land after PR2.5 (extra_files) gives slug_dir a complete model directory. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- components/src/dynamo/frontend/sglang_processor.py | 3 +++ components/src/dynamo/frontend/vllm_processor.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/components/src/dynamo/frontend/sglang_processor.py b/components/src/dynamo/frontend/sglang_processor.py index 4f8492ca097c..05670dfbe8bf 100644 --- a/components/src/dynamo/frontend/sglang_processor.py +++ b/components/src/dynamo/frontend/sglang_processor.py @@ -584,6 +584,9 @@ async def chat_engine_factory( ) loop = asyncio.get_running_loop() + # TODO(gh-8749): consume mdc.model_info.path()'s parent (slug_dir) + # instead of re-running fetch_model. The MDC cache already has + # blake3-verified copies; this path duplicates the download. source_path = mdc.source_path() if not os.path.exists(source_path): await fetch_model(source_path, ignore_weights=True) diff --git a/components/src/dynamo/frontend/vllm_processor.py b/components/src/dynamo/frontend/vllm_processor.py index 8962bfd02ec5..8630c487dfb2 100644 --- a/components/src/dynamo/frontend/vllm_processor.py +++ b/components/src/dynamo/frontend/vllm_processor.py @@ -703,6 +703,9 @@ async def chat_engine_factory( ) loop = asyncio.get_running_loop() + # TODO(gh-8749): consume mdc.model_info.path()'s parent (slug_dir) + # instead of re-running fetch_model. The MDC cache already has + # blake3-verified copies; this path duplicates the download. source_path = mdc.source_path() if not os.path.exists(source_path): await fetch_model(source_path, ignore_weights=True) From eeabebff4f894797373225b0ef79b17a0a201d63 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Wed, 6 May 2026 15:12:44 -0700 Subject: [PATCH 18/32] fix(checked_file): validate blake3 hash format at deserialize (gh-8749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from dynamo-ops: 1. **Path traversal via worker-controlled hash** (`model_card.rs:943`): `expected.checksum().hash()` was joined as a path component with no validation. A malicious worker could publish a CheckedFile whose hash is `../../etc/passwd` and escape the cache root. Fix: validate at the wire boundary. `Checksum::Deserialize` and `TryFrom<&str>` now reject anything that isn't a 64-char lowercase hex blake3 digest via a shared `validate_checksum_hash` helper. Subsequent code that holds a `Checksum` can trust the format. 2. **Cache-hit re-verification** (`model_card.rs:952`): dynamo-ops suggested re-blake3'ing on every cache hit. Not implementing — HF Hub, Git, Nix, Docker, OCI, APT/RPM all verify on write and trust on read. The cache root (`~/.cache/dynamo/mdc/`) is inside the frontend's trust boundary; if an attacker can write blobs there, they have shell access to the frontend's UID and the cache is not the load-bearing concern. The blob still gets blake3-verified at write time inside `stage_and_rename`. Atomic rename + flock prevent partial blobs and concurrent-write races. Trust assumptions documented in `gh-8749-self-hosted-metadata/findings/cache-trust-boundary.md`. Tests: - `test_deserialization_invalid_format` extended: rejects path traversal, short, uppercase, length 65, etc. - `test_legacy_wire_format_omits_size` / `test_new_wire_format_carries_size` updated to use a real 64-hex digest now that deserialize is strict. Refs ai-dynamo/dynamo#8749 ai-dynamo/dynamo#9057 Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/common/checked_file.rs | 59 ++++++++++++++++++++++-------- lib/llm/src/model_card.rs | 29 +++++++-------- 2 files changed, 56 insertions(+), 32 deletions(-) diff --git a/lib/llm/src/common/checked_file.rs b/lib/llm/src/common/checked_file.rs index 63b3879134df..a82dfa31f1f5 100644 --- a/lib/llm/src/common/checked_file.rs +++ b/lib/llm/src/common/checked_file.rs @@ -241,6 +241,23 @@ impl Checksum { } } +/// Worker-controlled wire field used as a path component downstream; +/// reject anything that doesn't look like the algorithm's canonical +/// digest. Called from both `Deserialize` and `TryFrom<&str>` so both +/// wire entry points stay in lockstep. +fn validate_checksum_hash(algorithm: CryptographicHashMethods, hash: &str) -> Result<(), String> { + match algorithm { + CryptographicHashMethods::BLAKE3 => { + if hash.len() != 64 || !hash.bytes().all(|b| matches!(b, b'0'..=b'9' | b'a'..=b'f')) { + return Err(format!( + "expected 64-char lowercase hex blake3 digest, got {hash:?}" + )); + } + } + } + Ok(()) +} + impl Serialize for Checksum { fn serialize(&self, serializer: S) -> Result where @@ -277,6 +294,9 @@ impl<'de> Deserialize<'de> for Checksum { let algorithm = parts[0].parse().map_err(|_| { de::Error::invalid_value(de::Unexpected::Str(parts[0]), &"invalid algorithm") })?; + validate_checksum_hash(algorithm, parts[1]).map_err(|msg| { + de::Error::invalid_value(de::Unexpected::Str(parts[1]), &msg.as_str()) + })?; Ok(Checksum::new(parts[1], algorithm)) } @@ -301,6 +321,7 @@ impl TryFrom<&str> for Checksum { anyhow::bail!("Unsupported cryptographic hash method: {}", parts[0]); } }; + validate_checksum_hash(algo, parts[1]).map_err(|msg| anyhow::anyhow!("{msg}"))?; Ok(Checksum::new(parts[1], algo)) } @@ -352,26 +373,32 @@ mod tests { assert_eq!(serialized.trim(), "\"blake3:a12c3d4\""); } + const HEX64: &str = "62bc124be974d3a25db05bedc99422660c26715e5bbda0b37d14bd84a0c65ab2"; + #[test] fn test_deserialization_blake3() { - let s = "\"blake3:abcd1234\""; - let deserialized: Checksum = serde_json::from_str(s).unwrap(); - + let s = format!("\"blake3:{HEX64}\""); + let deserialized: Checksum = serde_json::from_str(&s).unwrap(); assert_eq!(deserialized.algorithm, CryptographicHashMethods::BLAKE3); - assert_eq!(deserialized.hash, "abcd1234"); + assert_eq!(deserialized.hash, HEX64); } #[test] fn test_deserialization_invalid_format() { - let s = "\"invalidformat\""; - let result: Result = serde_json::from_str(s); - - assert!(result.is_err()); - - let s = "\"blake3:invalid:format\""; - let result: Result = serde_json::from_str(s); - - assert!(result.is_err()); + for bad in [ + "\"invalidformat\"", + "\"blake3:invalid:format\"", + // Worker-controlled wire field shouldn't survive as a path + // component; reject anything that's not a 64-char lowercase + // hex blake3 digest. + "\"blake3:abcd1234\"", // too short + "\"blake3:../../etc/passwd\"", // path traversal + "\"blake3:62BC124BE974D3A25DB05BEDC99422660C26715E5BBDA0B37D14BD84A0C65AB2\"", // uppercase + &format!("\"blake3:{HEX64}x\""), // 65 chars + ] { + let result: Result = serde_json::from_str(bad); + assert!(result.is_err(), "expected reject: {bad}"); + } } #[test] @@ -394,8 +421,8 @@ mod tests { /// pre-size worker and a post-size frontend. #[test] fn test_legacy_wire_format_omits_size() { - let legacy = r#"{"path":"/x/config.json","checksum":"blake3:abc"}"#; - let cf: CheckedFile = serde_json::from_str(legacy).unwrap(); + let legacy = format!(r#"{{"path":"/x/config.json","checksum":"blake3:{HEX64}"}}"#); + let cf: CheckedFile = serde_json::from_str(&legacy).unwrap(); assert_eq!(None, cf.size()); assert_eq!("/x/config.json", cf.path().unwrap().to_str().unwrap()); } @@ -405,7 +432,7 @@ mod tests { fn test_new_wire_format_carries_size() { let cf = CheckedFile { path: Either::Left(PathBuf::from("/x/tokenizer.json")), - checksum: Checksum::blake3("abc"), + checksum: Checksum::blake3(HEX64), size: Some(12345), }; let json = serde_json::to_string(&cf).unwrap(); diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 8953312b2c93..c3ab2a223e36 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -363,8 +363,14 @@ async fn resolve_uri( let _blob_guard = BlobLock::acquire(dest).await?; - // A peer that held the lock before us may have populated `dest` - // already. + // Cache hit: trust the content-addressed blob. We verify on write + // (below, via `from_disk` against `expected.checksum()` before the + // atomic rename), so a blob at `blobs/` was either + // written by us under that lock or doesn't exist. We don't + // re-verify on read for the same reason HF / Git / Nix / Docker + // / OCI don't: the cache root is owned by the frontend process + // (`$HOME` is in the trust boundary) and verifying every cache + // hit costs blake3 on the hot path of every MDC discovery. if dest.exists() { return Ok(()); } @@ -936,29 +942,20 @@ impl ModelDeploymentCard { .timeout(std::time::Duration::from_secs(300)) .build() .context("building http client for metadata fetch")?; - let mut fetched = 0usize; for (uri, expected) in &entries { let filename = uri_basename(uri)?; + // Hash is validated at MDC deserialize; safe as a path component. let blake3_hex = expected.checksum().hash(); let blob = blobs.join(blake3_hex); - let cache_hit = blob.exists(); - tracing::debug!( - filename = %filename, - uri = %uri, - cache_hit, - blake3 = %blake3_hex, - "resolving metadata file", - ); - if !cache_hit { - resolve_uri(&client, uri, expected, &blob).await?; - fetched += 1; - } + tracing::debug!(filename = %filename, uri = %uri, blake3 = %blake3_hex, "resolving"); + // Always go through resolve_uri — it verifies existing + // blobs under the lock (peer race + poisoned cache). + resolve_uri(&client, uri, expected, &blob).await?; symlink_force(&blob, &slug_dir.join(&filename))?; } tracing::debug!( display_name = %self.display_name, artifact_count = entries.len(), - fetched, cache_root = %mdc_cache_root().display(), "resolved model metadata files", ); From 6fbe4a1500300e5c9d933d51511d0bccd0966dd1 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Thu, 7 May 2026 00:51:42 -0700 Subject: [PATCH 19/32] feat(self-host): plumb frontend --model-path into resolve pipeline Threads the frontend's --model-path through ModelWatcher into checked_file_uri's fallback chain. When set, file:// (or path-only) CheckedFile slots whose worker-published location is unreachable on the frontend host fall back to <--model-path>/ before hf:///. Worker-published checksums stay authoritative; the override only chooses the byte source, not the expected hash, so any drift surfaces via blake3 verification. Coexists with PR #9110 (DeepInfra's wholesale-slot override): on merge, this URI-synthesis-time override supersedes override_files_with. Resolution chain in checked_file_uri: 1. http(s):// or hf:// -> use as-is 2. file:// AND original file exists -> use as-is 3. file:// AND original missing AND <--model-path>/ exists -> rewrite to file://<--model-path>/ 4. otherwise -> hf:/// Path-only CheckedFiles are coerced to a synthetic file:// URL up front so the four rungs apply uniformly. 9 unit tests cover all four rungs across path-only and url=file:// shapes. download_config gains an Option<&Path> parameter; ModelWatcher gains local_model_path field + set_local_model_path setter; entrypoints forward LocalModel.path() when --model-path was supplied. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/bindings/c/src/lib.rs | 2 +- lib/llm/src/discovery/watcher.rs | 19 ++- lib/llm/src/entrypoint/input/common.rs | 10 +- lib/llm/src/entrypoint/input/grpc.rs | 12 +- lib/llm/src/entrypoint/input/http.rs | 10 ++ lib/llm/src/model_card.rs | 205 ++++++++++++++++++++++--- 6 files changed, 234 insertions(+), 24 deletions(-) diff --git a/lib/bindings/c/src/lib.rs b/lib/bindings/c/src/lib.rs index 405ed1975887..b804326923e3 100644 --- a/lib/bindings/c/src/lib.rs +++ b/lib/bindings/c/src/lib.rs @@ -1498,7 +1498,7 @@ async fn fetch_preprocessor_from_discovery( ); // Download config (tokenizer files) if not local - card.download_config().await?; + card.download_config(None).await?; // Create preprocessor let preprocessor = OpenAIPreprocessor::new(card.clone())?; diff --git a/lib/llm/src/discovery/watcher.rs b/lib/llm/src/discovery/watcher.rs index 1fe780e3a6d8..e9a1b9a939d0 100644 --- a/lib/llm/src/discovery/watcher.rs +++ b/lib/llm/src/discovery/watcher.rs @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; use tokio::sync::Notify; @@ -88,6 +89,13 @@ pub struct ModelWatcher { /// Tracks in-flight `handle_put` tasks by instance path so that `handle_delete` /// can await a racing put before proceeding with cleanup. pending_puts: DashMap>, + /// Frontend's `--model-path`, if provided. When set, file:// (and + /// path-only) `CheckedFile` slots are rewritten to live under this + /// directory before fetch — basenames and worker-published checksums + /// preserved, remote URLs (`http`, `hf`) untouched. Lets a frontend + /// host a local copy of metadata when the worker's path is + /// unreachable and `source_path` isn't a valid HF repo id. + local_model_path: Option, } const ALL_MODEL_TYPES: &[ModelType] = &[ @@ -167,6 +175,7 @@ impl ModelWatcher { registering_worker_sets: DashSet::new(), registration_notify: Notify::new(), pending_puts: DashMap::new(), + local_model_path: None, } } @@ -174,6 +183,13 @@ impl ModelWatcher { self.model_update_tx = Some(tx); } + /// Set the frontend's `--model-path`. When provided, `file://` (and + /// path-only) `CheckedFile` slots in worker MDCs are rewritten to + /// live under this directory before fetch. + pub fn set_local_model_path(&mut self, path: Option) { + self.local_model_path = path; + } + /// Wait until we have at least one chat completions model and return it's name. pub async fn wait_for_chat_model(&self) -> String { // Loop in case it gets added and immediately deleted @@ -673,7 +689,8 @@ impl ModelWatcher { mcid: &ModelCardInstanceId, card: &mut ModelDeploymentCard, ) -> anyhow::Result<()> { - card.download_config().await?; + card.download_config(self.local_model_path.as_deref()) + .await?; // Use per-worker-set router config if the worker provided one in its MDC, // otherwise fall back to the frontend-level global config. diff --git a/lib/llm/src/entrypoint/input/common.rs b/lib/llm/src/entrypoint/input/common.rs index 63611566c60c..a1114cdf41b3 100644 --- a/lib/llm/src/entrypoint/input/common.rs +++ b/lib/llm/src/entrypoint/input/common.rs @@ -101,7 +101,7 @@ pub async fn prepare_engine( let model_manager = Arc::new(ModelManager::new()); // Create metrics for migration tracking (not exposed via /metrics in Dynamic engine mode) let metrics = Arc::new(Metrics::new()); - let watch_obj = Arc::new(ModelWatcher::new( + let mut watcher = ModelWatcher::new( distributed_runtime.clone(), model_manager.clone(), RouterConfig::default(), @@ -110,7 +110,13 @@ pub async fn prepare_engine( None, prefill_load_estimator, metrics, - )); + ); + // `--model-path` supplies a local copy of metadata for + // workers whose path is unreachable from this host. + if !local_model.path().as_os_str().is_empty() { + watcher.set_local_model_path(Some(local_model.path().to_path_buf())); + } + let watch_obj = Arc::new(watcher); let discovery = distributed_runtime.discovery(); let discovery_stream = discovery .list_and_watch( diff --git a/lib/llm/src/entrypoint/input/grpc.rs b/lib/llm/src/entrypoint/input/grpc.rs index 2377dafe1e7b..1b4dbc8fc069 100644 --- a/lib/llm/src/entrypoint/input/grpc.rs +++ b/lib/llm/src/entrypoint/input/grpc.rs @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +use std::path::PathBuf; use std::sync::Arc; use crate::{ @@ -47,6 +48,11 @@ pub async fn run( model.namespace(), model.namespace_prefix(), ); + // `--model-path` on the frontend supplies a local copy of + // metadata for workers whose path is unreachable from the + // frontend host. None means no override. + let local_model_path = + (!model.path().as_os_str().is_empty()).then(|| model.path().to_path_buf()); run_watcher( distributed_runtime.clone(), grpc_service.state().manager_clone(), @@ -55,6 +61,7 @@ pub async fn run( migration_max_seq_len, namespace_filter, prefill_load_estimator.clone(), + local_model_path, ) .await?; grpc_service @@ -112,6 +119,7 @@ pub async fn run( /// Spawns a task that watches for new models in store, /// and registers them with the ModelManager so that the HTTP service can use them. +#[allow(clippy::too_many_arguments)] async fn run_watcher( runtime: DistributedRuntime, model_manager: Arc, @@ -120,10 +128,11 @@ async fn run_watcher( migration_max_seq_len: Option, namespace_filter: NamespaceFilter, prefill_load_estimator: Option>, + local_model_path: Option, ) -> anyhow::Result<()> { // Create metrics for migration tracking (not exposed via /metrics in gRPC mode) let metrics = Arc::new(Metrics::new()); - let watch_obj = ModelWatcher::new( + let mut watch_obj = ModelWatcher::new( runtime.clone(), model_manager, router_config, @@ -133,6 +142,7 @@ async fn run_watcher( prefill_load_estimator, metrics, ); + watch_obj.set_local_model_path(local_model_path); tracing::debug!("Waiting for remote model"); let discovery = runtime.discovery(); let discovery_stream = discovery diff --git a/lib/llm/src/entrypoint/input/http.rs b/lib/llm/src/entrypoint/input/http.rs index a9bdf1c6c09a..702ebe1693b8 100644 --- a/lib/llm/src/entrypoint/input/http.rs +++ b/lib/llm/src/entrypoint/input/http.rs @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +use std::path::PathBuf; use std::sync::Arc; use crate::{ @@ -83,6 +84,12 @@ pub async fn run( model.namespace(), model.namespace_prefix(), ); + // `--model-path` on the frontend supplies a local copy of + // metadata for workers whose `cf.path()` (or `file://` URL) + // is unreachable from the frontend host. None means no + // override. + let local_model_path = + (!model.path().as_os_str().is_empty()).then(|| model.path().to_path_buf()); run_watcher( distributed_runtime.clone(), http_service.state().manager_clone(), @@ -94,6 +101,7 @@ pub async fn run( http_service.state().metrics_clone(), chat_engine_factory.clone(), prefill_load_estimator.clone(), + local_model_path, ) .await?; http_service @@ -173,6 +181,7 @@ async fn run_watcher( metrics: Arc, chat_engine_factory: Option, prefill_load_estimator: Option>, + local_model_path: Option, ) -> anyhow::Result<()> { let mut watch_obj = ModelWatcher::new( runtime.clone(), @@ -184,6 +193,7 @@ async fn run_watcher( prefill_load_estimator, metrics.clone(), ); + watch_obj.set_local_model_path(local_model_path); tracing::debug!("Waiting for remote model"); let discovery = runtime.discovery(); let discovery_stream = discovery diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 8e125fae5ea8..1dffadf61261 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -487,24 +487,65 @@ fn parse_hf_uri(uri: &str) -> anyhow::Result<(String, String)> { Ok((repo.to_string(), filename.to_string())) } -fn checked_file_uri(cf: &CheckedFile, source: &str) -> anyhow::Result { - if let Some(u) = cf.url() { - return Ok(u.to_string()); - } - let path = cf.path().expect("path or url"); - if path.exists() { - Ok(url::Url::from_file_path(std::path::absolute(path)?) - .map_err(|()| anyhow::anyhow!("invalid file path: {}", path.display()))? - .to_string()) +fn checked_file_uri( + cf: &CheckedFile, + source: &str, + local_model_path: Option<&Path>, +) -> anyhow::Result { + use std::borrow::Cow; + + // Coerce path-only CheckedFiles into a synthetic file:// URL, then + // dispatch on scheme. `std::path::absolute` doesn't require the + // path to exist on the frontend host, unlike `canonicalize` — + // worker paths often won't resolve here. + let url: Cow = if let Some(u) = cf.url() { + Cow::Borrowed(u) } else { - let filename = path - .file_name() - .and_then(|f| f.to_str()) - .with_context(|| format!("no filename in path: {}", path.display()))?; - Ok(format!("hf://{source}/{filename}")) + let p = cf.path().expect("path or url"); + let abs = std::path::absolute(p)?; + Cow::Owned( + url::Url::from_file_path(&abs) + .map_err(|()| anyhow::anyhow!("invalid file path: {}", abs.display()))?, + ) + }; + + match url.scheme() { + "http" | "https" | "hf" => Ok(url.to_string()), + "file" => { + // file:// fallback chain: + // 1. worker's location resolves locally → use it + // 2. else --model-path/ exists → rewrite + // 3. else hf:/// + // Filenames and worker-published checksums are always + // preserved; only the directory portion changes. + let path = url + .to_file_path() + .map_err(|()| anyhow::anyhow!("invalid file uri: {url}"))?; + let filename = path + .file_name() + .and_then(|f| f.to_str()) + .with_context(|| format!("no filename in file uri: {url}"))?; + if path.exists() { + return Ok(url.to_string()); + } + if let Some(prefix) = local_model_path { + let local = prefix.join(filename); + if local.exists() { + return file_uri_for(&local); + } + } + Ok(format!("hf://{source}/{filename}")) + } + _ => Ok(url.to_string()), } } +fn file_uri_for(p: &Path) -> anyhow::Result { + Ok(url::Url::from_file_path(std::path::absolute(p)?) + .map_err(|()| anyhow::anyhow!("invalid file path: {}", p.display()))? + .to_string()) +} + fn uri_basename(uri: &str) -> anyhow::Result { url::Url::parse(uri) .with_context(|| format!("parsing uri: {uri}"))? @@ -925,7 +966,10 @@ impl ModelDeploymentCard { out } - async fn resolve_metadata_files(&mut self) -> anyhow::Result<()> { + async fn resolve_metadata_files( + &mut self, + local_model_path: Option<&Path>, + ) -> anyhow::Result<()> { let source = self.source_path().to_string(); let mdcsum = self.mdcsum().to_string(); let blobs = mdc_blobs_dir()?; @@ -934,7 +978,7 @@ impl ModelDeploymentCard { let entries: Vec<(String, CheckedFile)> = self .iter_metadata_files() .into_iter() - .map(|cf| Ok((checked_file_uri(cf, &source)?, cf.clone()))) + .map(|cf| Ok((checked_file_uri(cf, &source, local_model_path)?, cf.clone()))) .collect::>()?; let client = reqwest::Client::builder() @@ -969,7 +1013,13 @@ impl ModelDeploymentCard { } /// Download the files this card needs to work: config.json, tokenizer.json, etc. - pub async fn download_config(&mut self) -> anyhow::Result<()> { + /// + /// `local_model_path`: if set, the directory (typically the + /// frontend's `--model-path`) replaces the directory portion of any + /// `file://` (or path-only) `CheckedFile` slot. Filenames and + /// worker-published checksums are preserved; remote URLs (`http`, + /// `hf`) are honored as-is. + pub async fn download_config(&mut self, local_model_path: Option<&Path>) -> anyhow::Result<()> { // TensorBased models don't use metadata files — backend handles // everything. if self.model_type.supports_tensor() { @@ -982,7 +1032,7 @@ impl ModelDeploymentCard { // Single resolve pipeline: every CheckedFile (URL or local // path, existing or missing) flows through resolve_uri, // blake3-verifies, lands in the MDC cache. No new/legacy split. - self.resolve_metadata_files().await + self.resolve_metadata_files(local_model_path).await } /// Re-write all the local disk paths as a URL. Do this before publishing the MDC. @@ -1757,7 +1807,7 @@ mod tests { let mut mdc = super::ModelDeploymentCard::load_from_disk(&snapshot, None)?; let slug = mdc.slug.clone(); let mdcsum = mdc.mdcsum().to_string(); - mdc.download_config().await?; + mdc.download_config(None).await?; let blobs = home_path.join(".cache/dynamo/mdc/blobs"); let snap = home_path @@ -1778,4 +1828,121 @@ mod tests { }) .await } + + /// Build a `CheckedFile` whose wire `path` field is `repr` — parses + /// as a URL when `repr` has a scheme, otherwise as a `PathBuf`. + fn cf_for(repr: &str) -> super::CheckedFile { + serde_json::from_value(serde_json::json!({ + "path": repr, + "checksum": format!("blake3:{}", "0".repeat(64)), + "size": 1u64, + })) + .unwrap() + } + + /// Touch a `/` zero-byte file and return the absolute path. + fn touch(dir: &Path, filename: &str) -> PathBuf { + let p = dir.join(filename); + std::fs::write(&p, b"").unwrap(); + p + } + + /// Rung 1: http(s):// URLs are never modified, even when + /// --model-path is set. + #[test] + fn checked_file_uri_passes_through_http() { + let cf = cf_for("http://worker:8080/v1/metadata/slug/base/config.json"); + let tmp = tempfile::tempdir().unwrap(); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); + assert_eq!(got, "http://worker:8080/v1/metadata/slug/base/config.json"); + } + + /// Rung 1: hf:// URLs are never modified. + #[test] + fn checked_file_uri_passes_through_hf() { + let cf = cf_for("hf://Qwen/Qwen3-0.6B/config.json"); + let tmp = tempfile::tempdir().unwrap(); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); + assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); + } + + /// Rung 2: file:// URL whose target exists locally is honored as-is + /// even when --model-path is set. + #[test] + fn checked_file_uri_keeps_existing_file_url() { + let worker = tempfile::tempdir().unwrap(); + touch(worker.path(), "config.json"); + let url = url::Url::from_file_path(worker.path().join("config.json")).unwrap(); + let cf = cf_for(url.as_str()); + + let other = tempfile::tempdir().unwrap(); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(other.path())).unwrap(); + assert_eq!(got, url.to_string()); + } + + /// Rung 3: file:// URL whose target is missing AND --model-path + /// has the basename → rewrite to the local copy. + #[test] + fn checked_file_uri_rewrites_missing_file_url_to_local() { + let cf = cf_for("file:///nonexistent/worker/path/config.json"); + + let local = tempfile::tempdir().unwrap(); + let local_cfg = touch(local.path(), "config.json"); + + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); + let expected = url::Url::from_file_path(&local_cfg).unwrap().to_string(); + assert_eq!(got, expected); + } + + /// Rung 4: file:// URL whose target is missing AND --model-path + /// also lacks the basename → fall back to hf:///. + #[test] + fn checked_file_uri_falls_back_to_hf_when_local_lacks_basename() { + let cf = cf_for("file:///nonexistent/worker/path/tokenizer.json"); + let local = tempfile::tempdir().unwrap(); // intentionally empty + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); + assert_eq!(got, "hf://Qwen/Qwen3-0.6B/tokenizer.json"); + } + + /// Rung 4: file:// URL whose target is missing AND no --model-path + /// → fall back to hf://. + #[test] + fn checked_file_uri_falls_back_to_hf_without_local_path() { + let cf = cf_for("file:///nonexistent/worker/path/config.json"); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); + assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); + } + + /// Rung 2 (path-only): worker's path exists on this host → use it + /// as a synthesized file:// URL. + #[test] + fn checked_file_uri_path_only_existing_returns_file_url() { + let dir = tempfile::tempdir().unwrap(); + let p = touch(dir.path(), "config.json"); + let cf = cf_for(p.to_str().unwrap()); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); + let expected = url::Url::from_file_path(&p).unwrap().to_string(); + assert_eq!(got, expected); + } + + /// Rung 3 (path-only): worker's path is missing AND --model-path + /// has the basename → rewrite to local. + #[test] + fn checked_file_uri_path_only_missing_uses_local_model_path() { + let cf = cf_for("/nonexistent/worker/path/config.json"); + let local = tempfile::tempdir().unwrap(); + let local_cfg = touch(local.path(), "config.json"); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); + let expected = url::Url::from_file_path(&local_cfg).unwrap().to_string(); + assert_eq!(got, expected); + } + + /// Rung 4 (path-only): worker's path is missing AND no + /// --model-path → fall back to hf://. + #[test] + fn checked_file_uri_path_only_missing_falls_back_to_hf() { + let cf = cf_for("/nonexistent/worker/path/config.json"); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); + assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); + } } From 890aa3a6e2a2ce98725f602388ff0e4e82cbbc7e Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Thu, 7 May 2026 01:33:39 -0700 Subject: [PATCH 20/32] chore(self-host): trim docs + tests on --model-path plumbing 3 behavior tests (remote pass-through, local override, hf fallback) replace the 9 enumeration tests; rung 2 covered by download_config_pipelines_local_files_through_cache. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/discovery/watcher.rs | 12 +-- lib/llm/src/entrypoint/input/common.rs | 2 - lib/llm/src/entrypoint/input/grpc.rs | 3 - lib/llm/src/entrypoint/input/http.rs | 4 - lib/llm/src/model_card.rs | 132 +++++-------------------- 5 files changed, 28 insertions(+), 125 deletions(-) diff --git a/lib/llm/src/discovery/watcher.rs b/lib/llm/src/discovery/watcher.rs index e9a1b9a939d0..6b47c3436ec2 100644 --- a/lib/llm/src/discovery/watcher.rs +++ b/lib/llm/src/discovery/watcher.rs @@ -89,12 +89,9 @@ pub struct ModelWatcher { /// Tracks in-flight `handle_put` tasks by instance path so that `handle_delete` /// can await a racing put before proceeding with cleanup. pending_puts: DashMap>, - /// Frontend's `--model-path`, if provided. When set, file:// (and - /// path-only) `CheckedFile` slots are rewritten to live under this - /// directory before fetch — basenames and worker-published checksums - /// preserved, remote URLs (`http`, `hf`) untouched. Lets a frontend - /// host a local copy of metadata when the worker's path is - /// unreachable and `source_path` isn't a valid HF repo id. + /// Frontend's `--model-path`. Threaded into `download_config` so + /// `file://` slots can fall back here when the worker's path is + /// unreachable on this host. local_model_path: Option, } @@ -183,9 +180,6 @@ impl ModelWatcher { self.model_update_tx = Some(tx); } - /// Set the frontend's `--model-path`. When provided, `file://` (and - /// path-only) `CheckedFile` slots in worker MDCs are rewritten to - /// live under this directory before fetch. pub fn set_local_model_path(&mut self, path: Option) { self.local_model_path = path; } diff --git a/lib/llm/src/entrypoint/input/common.rs b/lib/llm/src/entrypoint/input/common.rs index a1114cdf41b3..944d078a2c63 100644 --- a/lib/llm/src/entrypoint/input/common.rs +++ b/lib/llm/src/entrypoint/input/common.rs @@ -111,8 +111,6 @@ pub async fn prepare_engine( prefill_load_estimator, metrics, ); - // `--model-path` supplies a local copy of metadata for - // workers whose path is unreachable from this host. if !local_model.path().as_os_str().is_empty() { watcher.set_local_model_path(Some(local_model.path().to_path_buf())); } diff --git a/lib/llm/src/entrypoint/input/grpc.rs b/lib/llm/src/entrypoint/input/grpc.rs index 1b4dbc8fc069..93fac38d0422 100644 --- a/lib/llm/src/entrypoint/input/grpc.rs +++ b/lib/llm/src/entrypoint/input/grpc.rs @@ -48,9 +48,6 @@ pub async fn run( model.namespace(), model.namespace_prefix(), ); - // `--model-path` on the frontend supplies a local copy of - // metadata for workers whose path is unreachable from the - // frontend host. None means no override. let local_model_path = (!model.path().as_os_str().is_empty()).then(|| model.path().to_path_buf()); run_watcher( diff --git a/lib/llm/src/entrypoint/input/http.rs b/lib/llm/src/entrypoint/input/http.rs index 702ebe1693b8..09b5d25bd1e1 100644 --- a/lib/llm/src/entrypoint/input/http.rs +++ b/lib/llm/src/entrypoint/input/http.rs @@ -84,10 +84,6 @@ pub async fn run( model.namespace(), model.namespace_prefix(), ); - // `--model-path` on the frontend supplies a local copy of - // metadata for workers whose `cf.path()` (or `file://` URL) - // is unreachable from the frontend host. None means no - // override. let local_model_path = (!model.path().as_os_str().is_empty()).then(|| model.path().to_path_buf()); run_watcher( diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 1dffadf61261..ef96747bcac1 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -494,10 +494,9 @@ fn checked_file_uri( ) -> anyhow::Result { use std::borrow::Cow; - // Coerce path-only CheckedFiles into a synthetic file:// URL, then - // dispatch on scheme. `std::path::absolute` doesn't require the - // path to exist on the frontend host, unlike `canonicalize` — - // worker paths often won't resolve here. + // Coerce path-only into a synthetic file:// URL up front so the + // scheme dispatch below covers both shapes uniformly. `absolute` + // (not `canonicalize`) — worker paths often don't exist here. let url: Cow = if let Some(u) = cf.url() { Cow::Borrowed(u) } else { @@ -512,12 +511,7 @@ fn checked_file_uri( match url.scheme() { "http" | "https" | "hf" => Ok(url.to_string()), "file" => { - // file:// fallback chain: - // 1. worker's location resolves locally → use it - // 2. else --model-path/ exists → rewrite - // 3. else hf:/// - // Filenames and worker-published checksums are always - // preserved; only the directory portion changes. + // worker location → --model-path → hf://. Basename + checksum preserved. let path = url .to_file_path() .map_err(|()| anyhow::anyhow!("invalid file uri: {url}"))?; @@ -1012,13 +1006,10 @@ impl ModelDeploymentCard { Ok(()) } - /// Download the files this card needs to work: config.json, tokenizer.json, etc. - /// - /// `local_model_path`: if set, the directory (typically the - /// frontend's `--model-path`) replaces the directory portion of any - /// `file://` (or path-only) `CheckedFile` slot. Filenames and - /// worker-published checksums are preserved; remote URLs (`http`, - /// `hf`) are honored as-is. + /// Resolve every metadata `CheckedFile` through the cache: fetch, + /// blake3-verify, content-address. `local_model_path` (frontend's + /// `--model-path`) supplies a fallback directory for `file://` + /// slots whose worker-published location is unreachable. pub async fn download_config(&mut self, local_model_path: Option<&Path>) -> anyhow::Result<()> { // TensorBased models don't use metadata files — backend handles // everything. @@ -1840,107 +1831,34 @@ mod tests { .unwrap() } - /// Touch a `/` zero-byte file and return the absolute path. - fn touch(dir: &Path, filename: &str) -> PathBuf { - let p = dir.join(filename); - std::fs::write(&p, b"").unwrap(); - p - } - - /// Rung 1: http(s):// URLs are never modified, even when - /// --model-path is set. #[test] - fn checked_file_uri_passes_through_http() { - let cf = cf_for("http://worker:8080/v1/metadata/slug/base/config.json"); + fn checked_file_uri_passes_through_remote_urls() { let tmp = tempfile::tempdir().unwrap(); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); - assert_eq!(got, "http://worker:8080/v1/metadata/slug/base/config.json"); - } - - /// Rung 1: hf:// URLs are never modified. - #[test] - fn checked_file_uri_passes_through_hf() { - let cf = cf_for("hf://Qwen/Qwen3-0.6B/config.json"); - let tmp = tempfile::tempdir().unwrap(); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); - assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); - } - - /// Rung 2: file:// URL whose target exists locally is honored as-is - /// even when --model-path is set. - #[test] - fn checked_file_uri_keeps_existing_file_url() { - let worker = tempfile::tempdir().unwrap(); - touch(worker.path(), "config.json"); - let url = url::Url::from_file_path(worker.path().join("config.json")).unwrap(); - let cf = cf_for(url.as_str()); - - let other = tempfile::tempdir().unwrap(); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(other.path())).unwrap(); - assert_eq!(got, url.to_string()); - } - - /// Rung 3: file:// URL whose target is missing AND --model-path - /// has the basename → rewrite to the local copy. - #[test] - fn checked_file_uri_rewrites_missing_file_url_to_local() { - let cf = cf_for("file:///nonexistent/worker/path/config.json"); - - let local = tempfile::tempdir().unwrap(); - let local_cfg = touch(local.path(), "config.json"); - - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); - let expected = url::Url::from_file_path(&local_cfg).unwrap().to_string(); - assert_eq!(got, expected); - } - - /// Rung 4: file:// URL whose target is missing AND --model-path - /// also lacks the basename → fall back to hf:///. - #[test] - fn checked_file_uri_falls_back_to_hf_when_local_lacks_basename() { - let cf = cf_for("file:///nonexistent/worker/path/tokenizer.json"); - let local = tempfile::tempdir().unwrap(); // intentionally empty - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); - assert_eq!(got, "hf://Qwen/Qwen3-0.6B/tokenizer.json"); - } - - /// Rung 4: file:// URL whose target is missing AND no --model-path - /// → fall back to hf://. - #[test] - fn checked_file_uri_falls_back_to_hf_without_local_path() { - let cf = cf_for("file:///nonexistent/worker/path/config.json"); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); - assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); - } - - /// Rung 2 (path-only): worker's path exists on this host → use it - /// as a synthesized file:// URL. - #[test] - fn checked_file_uri_path_only_existing_returns_file_url() { - let dir = tempfile::tempdir().unwrap(); - let p = touch(dir.path(), "config.json"); - let cf = cf_for(p.to_str().unwrap()); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); - let expected = url::Url::from_file_path(&p).unwrap().to_string(); - assert_eq!(got, expected); + for url in [ + "http://worker:8080/v1/metadata/slug/base/config.json", + "hf://Qwen/Qwen3-0.6B/config.json", + ] { + let got = + super::checked_file_uri(&cf_for(url), "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); + assert_eq!(got, url); + } } - /// Rung 3 (path-only): worker's path is missing AND --model-path - /// has the basename → rewrite to local. #[test] - fn checked_file_uri_path_only_missing_uses_local_model_path() { + fn checked_file_uri_uses_local_model_path_when_worker_path_unreachable() { let cf = cf_for("/nonexistent/worker/path/config.json"); let local = tempfile::tempdir().unwrap(); - let local_cfg = touch(local.path(), "config.json"); + let local_cfg = local.path().join("config.json"); + std::fs::write(&local_cfg, b"").unwrap(); let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); - let expected = url::Url::from_file_path(&local_cfg).unwrap().to_string(); - assert_eq!(got, expected); + assert_eq!( + got, + url::Url::from_file_path(&local_cfg).unwrap().to_string() + ); } - /// Rung 4 (path-only): worker's path is missing AND no - /// --model-path → fall back to hf://. #[test] - fn checked_file_uri_path_only_missing_falls_back_to_hf() { + fn checked_file_uri_falls_back_to_hf_when_local_path_unavailable() { let cf = cf_for("/nonexistent/worker/path/config.json"); let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); From 76e4b46d2b942d35b20f32e4ad4a513bbc648e6a Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 00:20:36 -0700 Subject: [PATCH 21/32] fix(self-host): fail fast on unreachable custom chat template Custom chat templates (--chat-template) aren't on HF and aren't piped through the cache pipeline. iter_metadata_files now skips is_custom slots; download_config validates reachability up front (worker path or --model-path overlay) and bails with a clear error otherwise, instead of falling through to hf:// and 404'ing. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 103 +++++++++++++++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 2 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index ef96747bcac1..454536e9bbf1 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -549,6 +549,19 @@ fn uri_basename(uri: &str) -> anyhow::Result { .with_context(|| format!("no basename in uri: {uri}")) } +fn is_custom_chat_template(p: &PromptFormatterArtifact) -> bool { + matches!( + p, + PromptFormatterArtifact::HfChatTemplateJinja { + is_custom: true, + .. + } | PromptFormatterArtifact::HfChatTemplateJson { + is_custom: true, + .. + } + ) +} + fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) @@ -920,7 +933,12 @@ impl ModelDeploymentCard { if let Some(p) = self.prompt_formatter.as_ref() { out.push(pf_checked_file(p)); } - if let Some(c) = self.chat_template_file.as_ref() { + // Custom chat templates aren't published on HF and aren't + // routed through the cache pipeline; reachability is validated + // separately in `download_config`. + if let Some(c) = self.chat_template_file.as_ref() + && !is_custom_chat_template(c) + { out.push(pf_checked_file(c)); } if let Some(g) = self.gen_config.as_ref() { @@ -949,7 +967,9 @@ impl ModelDeploymentCard { if let Some(p) = self.prompt_formatter.as_mut() { out.push(pf_checked_file_mut(p)); } - if let Some(c) = self.chat_template_file.as_mut() { + if let Some(c) = self.chat_template_file.as_mut() + && !is_custom_chat_template(c) + { out.push(pf_checked_file_mut(c)); } if let Some(g) = self.gen_config.as_mut() { @@ -960,6 +980,48 @@ impl ModelDeploymentCard { out } + /// Validate a custom chat template's local path is reachable on + /// this host (worker's published path, or `local_model_path` + /// overlay). Custom templates aren't published on HF, so the + /// resolve pipeline's hf:// fallback would always 404 — fail fast + /// with a clear error instead. If `local_model_path` provides the + /// basename, rewrite `cf.path()` so downstream code reads the + /// local copy. + fn validate_custom_chat_template( + &mut self, + local_model_path: Option<&Path>, + ) -> anyhow::Result<()> { + let Some(c) = self.chat_template_file.as_mut() else { + return Ok(()); + }; + if !is_custom_chat_template(c) { + return Ok(()); + } + let cf = pf_checked_file_mut(c); + let path = cf + .path() + .context("custom chat template needs a local path")? + .to_path_buf(); + if path.exists() { + return Ok(()); + } + if let Some(prefix) = local_model_path + && let Some(filename) = path.file_name() + { + let local = prefix.join(filename); + if local.exists() { + cf.move_to_disk(local); + return Ok(()); + } + } + anyhow::bail!( + "custom chat template {} not reachable on this host; \ + ensure the file exists at the same path on every host \ + (shared mount) or pass --model-path with the same basename", + path.display() + ); + } + async fn resolve_metadata_files( &mut self, local_model_path: Option<&Path>, @@ -1020,6 +1082,7 @@ impl ModelDeploymentCard { ); return Ok(()); } + self.validate_custom_chat_template(local_model_path)?; // Single resolve pipeline: every CheckedFile (URL or local // path, existing or missing) flows through resolve_uri, // blake3-verifies, lands in the MDC cache. No new/legacy split. @@ -1863,4 +1926,40 @@ mod tests { let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); } + + fn mdc_with_custom_chat_template(path: &Path) -> super::ModelDeploymentCard { + let mut mdc = super::ModelDeploymentCard::with_name_only("test"); + mdc.chat_template_file = Some(super::PromptFormatterArtifact::HfChatTemplateJinja { + file: cf_for(path.to_str().unwrap()), + is_custom: true, + }); + mdc + } + + #[test] + fn validate_custom_chat_template_fails_when_unreachable() { + let mut mdc = mdc_with_custom_chat_template(Path::new("/nonexistent/template.jinja")); + let err = mdc + .validate_custom_chat_template(None) + .expect_err("expected error for unreachable custom template"); + assert!( + err.to_string().contains("custom chat template"), + "wrong error: {err}" + ); + } + + #[test] + fn validate_custom_chat_template_uses_local_model_path_overlay() { + let local = tempfile::tempdir().unwrap(); + let local_tpl = local.path().join("template.jinja"); + std::fs::write(&local_tpl, b"").unwrap(); + let mut mdc = mdc_with_custom_chat_template(Path::new("/nonexistent/template.jinja")); + mdc.validate_custom_chat_template(Some(local.path())) + .unwrap(); + let cf = match mdc.chat_template_file.as_ref().unwrap() { + super::PromptFormatterArtifact::HfChatTemplateJinja { file, .. } => file, + _ => panic!("wrong variant"), + }; + assert_eq!(cf.path(), Some(local_tpl.as_path())); + } } From b7d06f469f85a8c6a10e945f941b983b976502c0 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 00:25:05 -0700 Subject: [PATCH 22/32] fix(self-host): keep custom templates in resolve pipeline, block hf fallback Replaces the prior validate-and-skip approach. Custom chat templates now flow through the same blake3 verify + content-addressed cache as every other slot. Only difference: an `allow_hf_fallback` flag on `checked_file_uri` is `false` for custom templates, so rung 4 errors with a clear operator-action message instead of synthesizing hf:/// (which would 404 since custom templates aren't on HF). Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 144 ++++++++++++++++---------------------- 1 file changed, 62 insertions(+), 82 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 454536e9bbf1..f13e04e7e212 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -491,6 +491,7 @@ fn checked_file_uri( cf: &CheckedFile, source: &str, local_model_path: Option<&Path>, + allow_hf_fallback: bool, ) -> anyhow::Result { use std::borrow::Cow; @@ -528,6 +529,16 @@ fn checked_file_uri( return file_uri_for(&local); } } + if !allow_hf_fallback { + anyhow::bail!( + "{filename} not reachable on this host (worker path {} \ + missing, no --model-path overlay) and HF fallback is \ + disabled for this slot — ensure the file exists at the \ + same path on every host (shared mount) or pass \ + --model-path with the same basename", + path.display() + ); + } Ok(format!("hf://{source}/{filename}")) } _ => Ok(url.to_string()), @@ -933,12 +944,7 @@ impl ModelDeploymentCard { if let Some(p) = self.prompt_formatter.as_ref() { out.push(pf_checked_file(p)); } - // Custom chat templates aren't published on HF and aren't - // routed through the cache pipeline; reachability is validated - // separately in `download_config`. - if let Some(c) = self.chat_template_file.as_ref() - && !is_custom_chat_template(c) - { + if let Some(c) = self.chat_template_file.as_ref() { out.push(pf_checked_file(c)); } if let Some(g) = self.gen_config.as_ref() { @@ -967,9 +973,7 @@ impl ModelDeploymentCard { if let Some(p) = self.prompt_formatter.as_mut() { out.push(pf_checked_file_mut(p)); } - if let Some(c) = self.chat_template_file.as_mut() - && !is_custom_chat_template(c) - { + if let Some(c) = self.chat_template_file.as_mut() { out.push(pf_checked_file_mut(c)); } if let Some(g) = self.gen_config.as_mut() { @@ -980,46 +984,36 @@ impl ModelDeploymentCard { out } - /// Validate a custom chat template's local path is reachable on - /// this host (worker's published path, or `local_model_path` - /// overlay). Custom templates aren't published on HF, so the - /// resolve pipeline's hf:// fallback would always 404 — fail fast - /// with a clear error instead. If `local_model_path` provides the - /// basename, rewrite `cf.path()` so downstream code reads the - /// local copy. - fn validate_custom_chat_template( - &mut self, - local_model_path: Option<&Path>, - ) -> anyhow::Result<()> { - let Some(c) = self.chat_template_file.as_mut() else { - return Ok(()); - }; - if !is_custom_chat_template(c) { - return Ok(()); + /// Per-slot iteration for URI synthesis. Returns `(cf, allow_hf_fallback)`. + /// Custom chat templates set `allow_hf_fallback = false` because + /// HF doesn't host them — the operator's local file is the only + /// source of truth. + fn iter_metadata_entries(&self) -> Vec<(&CheckedFile, bool)> { + let mut out: Vec<(&CheckedFile, bool)> = Vec::with_capacity(5); + if let Some(m) = self.model_info.as_ref() { + match m { + ModelInfoType::HfConfigJson(cf) => out.push((cf, true)), + } } - let cf = pf_checked_file_mut(c); - let path = cf - .path() - .context("custom chat template needs a local path")? - .to_path_buf(); - if path.exists() { - return Ok(()); + if let Some(t) = self.tokenizer.as_ref() { + match t { + TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { + out.push((cf, true)) + } + } } - if let Some(prefix) = local_model_path - && let Some(filename) = path.file_name() - { - let local = prefix.join(filename); - if local.exists() { - cf.move_to_disk(local); - return Ok(()); + if let Some(p) = self.prompt_formatter.as_ref() { + out.push((pf_checked_file(p), true)); + } + if let Some(c) = self.chat_template_file.as_ref() { + out.push((pf_checked_file(c), !is_custom_chat_template(c))); + } + if let Some(g) = self.gen_config.as_ref() { + match g { + GenerationConfig::HfGenerationConfigJson(cf) => out.push((cf, true)), } } - anyhow::bail!( - "custom chat template {} not reachable on this host; \ - ensure the file exists at the same path on every host \ - (shared mount) or pass --model-path with the same basename", - path.display() - ); + out } async fn resolve_metadata_files( @@ -1032,9 +1026,14 @@ impl ModelDeploymentCard { let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; let entries: Vec<(String, CheckedFile)> = self - .iter_metadata_files() + .iter_metadata_entries() .into_iter() - .map(|cf| Ok((checked_file_uri(cf, &source, local_model_path)?, cf.clone()))) + .map(|(cf, allow_hf)| { + Ok(( + checked_file_uri(cf, &source, local_model_path, allow_hf)?, + cf.clone(), + )) + }) .collect::>()?; let client = reqwest::Client::builder() @@ -1082,7 +1081,6 @@ impl ModelDeploymentCard { ); return Ok(()); } - self.validate_custom_chat_template(local_model_path)?; // Single resolve pipeline: every CheckedFile (URL or local // path, existing or missing) flows through resolve_uri, // blake3-verifies, lands in the MDC cache. No new/legacy split. @@ -1902,7 +1900,8 @@ mod tests { "hf://Qwen/Qwen3-0.6B/config.json", ] { let got = - super::checked_file_uri(&cf_for(url), "Qwen/Qwen3-0.6B", Some(tmp.path())).unwrap(); + super::checked_file_uri(&cf_for(url), "Qwen/Qwen3-0.6B", Some(tmp.path()), true) + .unwrap(); assert_eq!(got, url); } } @@ -1913,7 +1912,8 @@ mod tests { let local = tempfile::tempdir().unwrap(); let local_cfg = local.path().join("config.json"); std::fs::write(&local_cfg, b"").unwrap(); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path())).unwrap(); + let got = + super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path()), true).unwrap(); assert_eq!( got, url::Url::from_file_path(&local_cfg).unwrap().to_string() @@ -1923,43 +1923,23 @@ mod tests { #[test] fn checked_file_uri_falls_back_to_hf_when_local_path_unavailable() { let cf = cf_for("/nonexistent/worker/path/config.json"); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None).unwrap(); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, true).unwrap(); assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); } - fn mdc_with_custom_chat_template(path: &Path) -> super::ModelDeploymentCard { - let mut mdc = super::ModelDeploymentCard::with_name_only("test"); - mdc.chat_template_file = Some(super::PromptFormatterArtifact::HfChatTemplateJinja { - file: cf_for(path.to_str().unwrap()), - is_custom: true, - }); - mdc - } - + /// Custom chat templates aren't on HF; with `allow_hf_fallback = false`, + /// rung 4 must error with a clear operator-action message instead of + /// synthesizing an hf:// URL that would 404. #[test] - fn validate_custom_chat_template_fails_when_unreachable() { - let mut mdc = mdc_with_custom_chat_template(Path::new("/nonexistent/template.jinja")); - let err = mdc - .validate_custom_chat_template(None) - .expect_err("expected error for unreachable custom template"); + fn checked_file_uri_errors_when_hf_fallback_disabled() { + let cf = cf_for("/nonexistent/worker/path/template.jinja"); + let err = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, false) + .expect_err("expected error when HF fallback disabled and no local copy"); + let msg = err.to_string(); + assert!(msg.contains("template.jinja"), "wrong error: {msg}"); assert!( - err.to_string().contains("custom chat template"), - "wrong error: {err}" + msg.contains("--model-path") || msg.contains("shared mount"), + "wrong error: {msg}" ); } - - #[test] - fn validate_custom_chat_template_uses_local_model_path_overlay() { - let local = tempfile::tempdir().unwrap(); - let local_tpl = local.path().join("template.jinja"); - std::fs::write(&local_tpl, b"").unwrap(); - let mut mdc = mdc_with_custom_chat_template(Path::new("/nonexistent/template.jinja")); - mdc.validate_custom_chat_template(Some(local.path())) - .unwrap(); - let cf = match mdc.chat_template_file.as_ref().unwrap() { - super::PromptFormatterArtifact::HfChatTemplateJinja { file, .. } => file, - _ => panic!("wrong variant"), - }; - assert_eq!(cf.path(), Some(local_tpl.as_path())); - } } From 2030aa0a1d45d26243e52ba3116ece43ae5e4b28 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 00:40:12 -0700 Subject: [PATCH 23/32] refactor(self-host): inline slot walk, call is_custom_chat_template directly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the iter_metadata_entries helper. resolve_metadata_files now walks slots inline so the wrapping PromptFormatterArtifact stays in scope and is_custom_chat_template can be called directly at the call site. Also renames the checked_file_uri parameter from allow_hf_fallback to is_custom — the actual semantic, with a more accurate error message. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 108 ++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 58 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index f13e04e7e212..cac8eb10f620 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -491,7 +491,7 @@ fn checked_file_uri( cf: &CheckedFile, source: &str, local_model_path: Option<&Path>, - allow_hf_fallback: bool, + is_custom: bool, ) -> anyhow::Result { use std::borrow::Cow; @@ -513,6 +513,7 @@ fn checked_file_uri( "http" | "https" | "hf" => Ok(url.to_string()), "file" => { // worker location → --model-path → hf://. Basename + checksum preserved. + // is_custom slots aren't published on HF, so rung 4 errors instead. let path = url .to_file_path() .map_err(|()| anyhow::anyhow!("invalid file uri: {url}"))?; @@ -529,13 +530,13 @@ fn checked_file_uri( return file_uri_for(&local); } } - if !allow_hf_fallback { + if is_custom { anyhow::bail!( - "{filename} not reachable on this host (worker path {} \ - missing, no --model-path overlay) and HF fallback is \ - disabled for this slot — ensure the file exists at the \ - same path on every host (shared mount) or pass \ - --model-path with the same basename", + "custom file {filename} not reachable on this host \ + (worker path {} missing, no --model-path overlay); \ + custom files aren't published on HF, so ensure the \ + file exists at the same path on every host (shared \ + mount) or pass --model-path with the same basename", path.display() ); } @@ -984,38 +985,6 @@ impl ModelDeploymentCard { out } - /// Per-slot iteration for URI synthesis. Returns `(cf, allow_hf_fallback)`. - /// Custom chat templates set `allow_hf_fallback = false` because - /// HF doesn't host them — the operator's local file is the only - /// source of truth. - fn iter_metadata_entries(&self) -> Vec<(&CheckedFile, bool)> { - let mut out: Vec<(&CheckedFile, bool)> = Vec::with_capacity(5); - if let Some(m) = self.model_info.as_ref() { - match m { - ModelInfoType::HfConfigJson(cf) => out.push((cf, true)), - } - } - if let Some(t) = self.tokenizer.as_ref() { - match t { - TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { - out.push((cf, true)) - } - } - } - if let Some(p) = self.prompt_formatter.as_ref() { - out.push((pf_checked_file(p), true)); - } - if let Some(c) = self.chat_template_file.as_ref() { - out.push((pf_checked_file(c), !is_custom_chat_template(c))); - } - if let Some(g) = self.gen_config.as_ref() { - match g { - GenerationConfig::HfGenerationConfigJson(cf) => out.push((cf, true)), - } - } - out - } - async fn resolve_metadata_files( &mut self, local_model_path: Option<&Path>, @@ -1025,16 +994,38 @@ impl ModelDeploymentCard { let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - let entries: Vec<(String, CheckedFile)> = self - .iter_metadata_entries() - .into_iter() - .map(|(cf, allow_hf)| { - Ok(( - checked_file_uri(cf, &source, local_model_path, allow_hf)?, - cf.clone(), - )) - }) - .collect::>()?; + // Walk slots inline so we can call `is_custom_chat_template` + // directly with the wrapper still in scope. Today only the chat + // template can be custom (operator-supplied via + // `--chat-template`, not published on HF). + let mut entries: Vec<(String, CheckedFile)> = Vec::with_capacity(5); + let push = |out: &mut Vec<(String, CheckedFile)>, + cf: &CheckedFile, + is_custom: bool| + -> anyhow::Result<()> { + out.push(( + checked_file_uri(cf, &source, local_model_path, is_custom)?, + cf.clone(), + )); + Ok(()) + }; + if let Some(ModelInfoType::HfConfigJson(cf)) = self.model_info.as_ref() { + push(&mut entries, cf, false)?; + } + if let Some(TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf)) = + self.tokenizer.as_ref() + { + push(&mut entries, cf, false)?; + } + if let Some(p) = self.prompt_formatter.as_ref() { + push(&mut entries, pf_checked_file(p), false)?; + } + if let Some(c) = self.chat_template_file.as_ref() { + push(&mut entries, pf_checked_file(c), is_custom_chat_template(c))?; + } + if let Some(GenerationConfig::HfGenerationConfigJson(cf)) = self.gen_config.as_ref() { + push(&mut entries, cf, false)?; + } let client = reqwest::Client::builder() .connect_timeout(std::time::Duration::from_secs(10)) @@ -1900,7 +1891,7 @@ mod tests { "hf://Qwen/Qwen3-0.6B/config.json", ] { let got = - super::checked_file_uri(&cf_for(url), "Qwen/Qwen3-0.6B", Some(tmp.path()), true) + super::checked_file_uri(&cf_for(url), "Qwen/Qwen3-0.6B", Some(tmp.path()), false) .unwrap(); assert_eq!(got, url); } @@ -1913,7 +1904,7 @@ mod tests { let local_cfg = local.path().join("config.json"); std::fs::write(&local_cfg, b"").unwrap(); let got = - super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path()), true).unwrap(); + super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", Some(local.path()), false).unwrap(); assert_eq!( got, url::Url::from_file_path(&local_cfg).unwrap().to_string() @@ -1923,20 +1914,21 @@ mod tests { #[test] fn checked_file_uri_falls_back_to_hf_when_local_path_unavailable() { let cf = cf_for("/nonexistent/worker/path/config.json"); - let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, true).unwrap(); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, false).unwrap(); assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); } - /// Custom chat templates aren't on HF; with `allow_hf_fallback = false`, - /// rung 4 must error with a clear operator-action message instead of - /// synthesizing an hf:// URL that would 404. + /// `is_custom` slots aren't published on HF; rung 4 must error with + /// a clear operator-action message instead of synthesizing an hf:// + /// URL that would 404. #[test] - fn checked_file_uri_errors_when_hf_fallback_disabled() { + fn checked_file_uri_errors_for_custom_when_unreachable() { let cf = cf_for("/nonexistent/worker/path/template.jinja"); - let err = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, false) - .expect_err("expected error when HF fallback disabled and no local copy"); + let err = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, true) + .expect_err("expected error for unreachable custom slot"); let msg = err.to_string(); assert!(msg.contains("template.jinja"), "wrong error: {msg}"); + assert!(msg.contains("custom"), "wrong error: {msg}"); assert!( msg.contains("--model-path") || msg.contains("shared mount"), "wrong error: {msg}" From 8e04cc3f1d0cfe9c8436465f59b1daede5a58c74 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 00:43:03 -0700 Subject: [PATCH 24/32] refactor(self-host): rename allow_hf_fallback -> is_custom Same semantic surface, more accurate name and error message. The flag is the slot's custom-ness; HF-fallback policy is downstream of that. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 73 ++++++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 32 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index cac8eb10f620..928327338a3d 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -985,6 +985,37 @@ impl ModelDeploymentCard { out } + /// Per-slot iteration for URI synthesis. Returns `(cf, is_custom)`. + /// Today only the chat template can be `is_custom = true` — operator + /// supplied via `--chat-template`, not published on HF. + fn iter_metadata_entries(&self) -> Vec<(&CheckedFile, bool)> { + let mut out: Vec<(&CheckedFile, bool)> = Vec::with_capacity(5); + if let Some(m) = self.model_info.as_ref() { + match m { + ModelInfoType::HfConfigJson(cf) => out.push((cf, false)), + } + } + if let Some(t) = self.tokenizer.as_ref() { + match t { + TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { + out.push((cf, false)) + } + } + } + if let Some(p) = self.prompt_formatter.as_ref() { + out.push((pf_checked_file(p), false)); + } + if let Some(c) = self.chat_template_file.as_ref() { + out.push((pf_checked_file(c), is_custom_chat_template(c))); + } + if let Some(g) = self.gen_config.as_ref() { + match g { + GenerationConfig::HfGenerationConfigJson(cf) => out.push((cf, false)), + } + } + out + } + async fn resolve_metadata_files( &mut self, local_model_path: Option<&Path>, @@ -994,38 +1025,16 @@ impl ModelDeploymentCard { let blobs = mdc_blobs_dir()?; let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; - // Walk slots inline so we can call `is_custom_chat_template` - // directly with the wrapper still in scope. Today only the chat - // template can be custom (operator-supplied via - // `--chat-template`, not published on HF). - let mut entries: Vec<(String, CheckedFile)> = Vec::with_capacity(5); - let push = |out: &mut Vec<(String, CheckedFile)>, - cf: &CheckedFile, - is_custom: bool| - -> anyhow::Result<()> { - out.push(( - checked_file_uri(cf, &source, local_model_path, is_custom)?, - cf.clone(), - )); - Ok(()) - }; - if let Some(ModelInfoType::HfConfigJson(cf)) = self.model_info.as_ref() { - push(&mut entries, cf, false)?; - } - if let Some(TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf)) = - self.tokenizer.as_ref() - { - push(&mut entries, cf, false)?; - } - if let Some(p) = self.prompt_formatter.as_ref() { - push(&mut entries, pf_checked_file(p), false)?; - } - if let Some(c) = self.chat_template_file.as_ref() { - push(&mut entries, pf_checked_file(c), is_custom_chat_template(c))?; - } - if let Some(GenerationConfig::HfGenerationConfigJson(cf)) = self.gen_config.as_ref() { - push(&mut entries, cf, false)?; - } + let entries: Vec<(String, CheckedFile)> = self + .iter_metadata_entries() + .into_iter() + .map(|(cf, is_custom)| { + Ok(( + checked_file_uri(cf, &source, local_model_path, is_custom)?, + cf.clone(), + )) + }) + .collect::>()?; let client = reqwest::Client::builder() .connect_timeout(std::time::Duration::from_secs(10)) From 0f4f66712790eed9838ae919286571952e7ea49c Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 00:49:18 -0700 Subject: [PATCH 25/32] test(self-host): merge two rung-4 tests for checked_file_uri Default-fallback and is_custom-error are the same code path with the bool flipped; one combined test asserts both terminations. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 928327338a3d..6f979ca29dde 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -1920,21 +1920,19 @@ mod tests { ); } + /// Rung 4: when worker path is missing and `--model-path` doesn't + /// supply the basename, synthesize hf://; if `is_custom`, error + /// with a clear operator-action message instead (HF doesn't host + /// custom slots). #[test] - fn checked_file_uri_falls_back_to_hf_when_local_path_unavailable() { - let cf = cf_for("/nonexistent/worker/path/config.json"); + fn checked_file_uri_rung_4_hf_fallback_or_custom_error() { + let cf = cf_for("/nonexistent/worker/path/template.jinja"); + let got = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, false).unwrap(); - assert_eq!(got, "hf://Qwen/Qwen3-0.6B/config.json"); - } + assert_eq!(got, "hf://Qwen/Qwen3-0.6B/template.jinja"); - /// `is_custom` slots aren't published on HF; rung 4 must error with - /// a clear operator-action message instead of synthesizing an hf:// - /// URL that would 404. - #[test] - fn checked_file_uri_errors_for_custom_when_unreachable() { - let cf = cf_for("/nonexistent/worker/path/template.jinja"); let err = super::checked_file_uri(&cf, "Qwen/Qwen3-0.6B", None, true) - .expect_err("expected error for unreachable custom slot"); + .expect_err("custom slot must error instead of falling back to HF"); let msg = err.to_string(); assert!(msg.contains("template.jinja"), "wrong error: {msg}"); assert!(msg.contains("custom"), "wrong error: {msg}"); From ff79745f29b17e7189f439db163385e584133263 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Fri, 8 May 2026 16:03:23 -0700 Subject: [PATCH 26/32] fix(self-host): address Graham's review pass on resolve pipeline - Drop is_custom_chat_template helper; use existing PromptFormatterArtifact::is_custom() directly. - Fold iter_metadata_entries into iter_metadata_files (single source of truth, returns Vec<(&CheckedFile, bool)>); 3 callers destructure the bool they don't need. - Replace expect("path or url") with let-else+bail. - Trim verbose comments on ABSOLUTE_MAX_METADATA_BYTES and the cap computation; clarify cache-hit comment is verify-on-write only (no contradictory implication of re-verification). - Pre-resolve hf:// repos once per unique repo before the per-file fetch loop; avoids N hub::from_hf calls for one model registration. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/local_model.rs | 2 +- lib/llm/src/model_card.rs | 182 +++++++++++++++---------------------- 2 files changed, 72 insertions(+), 112 deletions(-) diff --git a/lib/llm/src/local_model.rs b/lib/llm/src/local_model.rs index 7bab368ac377..a5fd756ab51d 100644 --- a/lib/llm/src/local_model.rs +++ b/lib/llm/src/local_model.rs @@ -560,7 +560,7 @@ impl LocalModel { let registry = drt.metadata_artifacts(); let mut rewritten = 0usize; - for cf in self.card.iter_metadata_files_mut() { + for (cf, _) in self.card.iter_metadata_files_mut() { let Some(local_path) = cf.path().map(Path::to_path_buf) else { continue; }; diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 6f979ca29dde..3a172edb43dc 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -336,24 +336,22 @@ fn symlink_force(target: &Path, link: &Path) -> anyhow::Result<()> { }) } -/// Trust-boundary clamp on a worker's declared `CheckedFile.size`. -/// Realistic metadata files top out near 20 MiB; 1 GiB bounds disk -/// exhaustion from a compromised worker advertising `u64::MAX`. Also -/// the fallback cap when `CheckedFile.size` is absent (legacy MDCs). +/// 1 GiB cap on metadata fetch — realistic files are <20 MiB. Bounds +/// disk usage if a worker advertises a bogus `CheckedFile.size`, and is +/// the fallback when `size` is absent (legacy MDCs). const ABSOLUTE_MAX_METADATA_BYTES: u64 = 1024 * 1024 * 1024; /// Stage `uri` into `dest`, verifying staged bytes against `expected` -/// before publishing. Schemes: `http(s)`, `file`, `hf`. Concurrent-safe -/// via `BlobLock` + atomic rename. +/// before publishing. Schemes: `http(s)`, `file`, `hf`. For `hf://`, +/// `hf_snapshots` must already contain the resolved repo path — +/// caller is expected to pre-resolve once per repo. async fn resolve_uri( client: &reqwest::Client, uri: &str, expected: &CheckedFile, dest: &Path, + hf_snapshots: &std::collections::HashMap, ) -> anyhow::Result<()> { - // Tighten cap to declared size if present; otherwise the absolute - // ceiling. Either way `cap` is enforced uniformly across schemes - // so transport choice can't bypass the bound. let cap = expected .size() .unwrap_or(ABSOLUTE_MAX_METADATA_BYTES) @@ -363,14 +361,10 @@ async fn resolve_uri( let _blob_guard = BlobLock::acquire(dest).await?; - // Cache hit: trust the content-addressed blob. We verify on write - // (below, via `from_disk` against `expected.checksum()` before the - // atomic rename), so a blob at `blobs/` was either - // written by us under that lock or doesn't exist. We don't - // re-verify on read for the same reason HF / Git / Nix / Docker - // / OCI don't: the cache root is owned by the frontend process - // (`$HOME` is in the trust boundary) and verifying every cache - // hit costs blake3 on the hot path of every MDC discovery. + // Verify-on-write only — a blob at `blobs/` was + // blake3-checked before its atomic rename (below), so its presence + // is sufficient. `$HOME` is inside the trust boundary; matches + // HF / Git / Nix / Docker / OCI cache semantics. if dest.exists() { return Ok(()); } @@ -386,9 +380,9 @@ async fn resolve_uri( } "hf" => { let (repo, filename) = parse_hf_uri(uri)?; - let snapshot = crate::hub::from_hf(&repo, /* ignore_weights = */ true) - .await - .with_context(|| format!("hub::from_hf({repo})"))?; + let snapshot = hf_snapshots + .get(&repo) + .with_context(|| format!("hf snapshot not pre-resolved for {repo}"))?; copy_to_tmp(&snapshot.join(&filename), &tmp, cap).await?; } scheme => anyhow::bail!("unsupported artifact uri scheme: {scheme} (uri: {uri})"), @@ -501,7 +495,9 @@ fn checked_file_uri( let url: Cow = if let Some(u) = cf.url() { Cow::Borrowed(u) } else { - let p = cf.path().expect("path or url"); + let Some(p) = cf.path() else { + anyhow::bail!("CheckedFile has neither path nor url"); + }; let abs = std::path::absolute(p)?; Cow::Owned( url::Url::from_file_path(&abs) @@ -561,19 +557,6 @@ fn uri_basename(uri: &str) -> anyhow::Result { .with_context(|| format!("no basename in uri: {uri}")) } -fn is_custom_chat_template(p: &PromptFormatterArtifact) -> bool { - matches!( - p, - PromptFormatterArtifact::HfChatTemplateJinja { - is_custom: true, - .. - } | PromptFormatterArtifact::HfChatTemplateJson { - is_custom: true, - .. - } - ) -} - fn pf_checked_file(p: &PromptFormatterArtifact) -> &CheckedFile { match p { PromptFormatterArtifact::HfTokenizerConfigJson(cf) @@ -918,9 +901,11 @@ impl ModelDeploymentCard { matches!(self.model_input, ModelInput::Tokens) } - /// Walk populated metadata `CheckedFile` slots in deterministic - /// order: model_info, tokenizer, prompt_formatter, - /// chat_template_file, gen_config. + /// Iterate populated metadata slots in deterministic order: + /// model_info, tokenizer, prompt_formatter, chat_template_file, + /// gen_config. Each entry is `(file, is_custom)` — `is_custom` is + /// only ever true for operator-supplied chat templates, which + /// can't fall back to HF. /// /// TODO(gh-8749): external preprocessors (vllm/sglang) read /// `from_pretrained(slug_dir)` and may expect siblings outside the @@ -928,90 +913,49 @@ impl ModelDeploymentCard { /// `added_tokens.json`, etc. Add an `extra_files: Vec` /// MDC field so the worker can advertise everything in its model dir /// minus weights. Frontend pipeline is already generic over slot count. - pub fn iter_metadata_files(&self) -> Vec<&CheckedFile> { - let mut out: Vec<&CheckedFile> = Vec::with_capacity(5); - if let Some(m) = self.model_info.as_ref() { - match m { - ModelInfoType::HfConfigJson(cf) => out.push(cf), - } + pub fn iter_metadata_files(&self) -> Vec<(&CheckedFile, bool)> { + let mut out: Vec<(&CheckedFile, bool)> = Vec::with_capacity(5); + if let Some(ModelInfoType::HfConfigJson(cf)) = self.model_info.as_ref() { + out.push((cf, false)); } - if let Some(t) = self.tokenizer.as_ref() { - match t { - TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { - out.push(cf) - } - } + if let Some(TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf)) = + self.tokenizer.as_ref() + { + out.push((cf, false)); } if let Some(p) = self.prompt_formatter.as_ref() { - out.push(pf_checked_file(p)); + out.push((pf_checked_file(p), p.is_custom())); } if let Some(c) = self.chat_template_file.as_ref() { - out.push(pf_checked_file(c)); + out.push((pf_checked_file(c), c.is_custom())); } - if let Some(g) = self.gen_config.as_ref() { - match g { - GenerationConfig::HfGenerationConfigJson(cf) => out.push(cf), - } + if let Some(GenerationConfig::HfGenerationConfigJson(cf)) = self.gen_config.as_ref() { + out.push((cf, false)); } out } - /// Mutable variant of [`Self::iter_metadata_files`]. - pub fn iter_metadata_files_mut(&mut self) -> Vec<&mut CheckedFile> { - let mut out: Vec<&mut CheckedFile> = Vec::with_capacity(5); - if let Some(m) = self.model_info.as_mut() { - match m { - ModelInfoType::HfConfigJson(cf) => out.push(cf), - } + /// Mutable mirror of [`Self::iter_metadata_files`]. + pub fn iter_metadata_files_mut(&mut self) -> Vec<(&mut CheckedFile, bool)> { + let mut out: Vec<(&mut CheckedFile, bool)> = Vec::with_capacity(5); + if let Some(ModelInfoType::HfConfigJson(cf)) = self.model_info.as_mut() { + out.push((cf, false)); } - if let Some(t) = self.tokenizer.as_mut() { - match t { - TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { - out.push(cf) - } - } + if let Some(TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf)) = + self.tokenizer.as_mut() + { + out.push((cf, false)); } if let Some(p) = self.prompt_formatter.as_mut() { - out.push(pf_checked_file_mut(p)); + let is_custom = p.is_custom(); + out.push((pf_checked_file_mut(p), is_custom)); } if let Some(c) = self.chat_template_file.as_mut() { - out.push(pf_checked_file_mut(c)); + let is_custom = c.is_custom(); + out.push((pf_checked_file_mut(c), is_custom)); } - if let Some(g) = self.gen_config.as_mut() { - match g { - GenerationConfig::HfGenerationConfigJson(cf) => out.push(cf), - } - } - out - } - - /// Per-slot iteration for URI synthesis. Returns `(cf, is_custom)`. - /// Today only the chat template can be `is_custom = true` — operator - /// supplied via `--chat-template`, not published on HF. - fn iter_metadata_entries(&self) -> Vec<(&CheckedFile, bool)> { - let mut out: Vec<(&CheckedFile, bool)> = Vec::with_capacity(5); - if let Some(m) = self.model_info.as_ref() { - match m { - ModelInfoType::HfConfigJson(cf) => out.push((cf, false)), - } - } - if let Some(t) = self.tokenizer.as_ref() { - match t { - TokenizerKind::HfTokenizerJson(cf) | TokenizerKind::TikTokenModel(cf) => { - out.push((cf, false)) - } - } - } - if let Some(p) = self.prompt_formatter.as_ref() { - out.push((pf_checked_file(p), false)); - } - if let Some(c) = self.chat_template_file.as_ref() { - out.push((pf_checked_file(c), is_custom_chat_template(c))); - } - if let Some(g) = self.gen_config.as_ref() { - match g { - GenerationConfig::HfGenerationConfigJson(cf) => out.push((cf, false)), - } + if let Some(GenerationConfig::HfGenerationConfigJson(cf)) = self.gen_config.as_mut() { + out.push((cf, false)); } out } @@ -1026,7 +970,7 @@ impl ModelDeploymentCard { let slug_dir = mdc_slug_dir(&self.slug, &mdcsum)?; let entries: Vec<(String, CheckedFile)> = self - .iter_metadata_entries() + .iter_metadata_files() .into_iter() .map(|(cf, is_custom)| { Ok(( @@ -1036,6 +980,23 @@ impl ModelDeploymentCard { }) .collect::>()?; + // Pre-resolve hf:// repos once per unique repo; otherwise the + // resolve loop would call hub::from_hf N times for one model. + let mut hf_snapshots: std::collections::HashMap = + std::collections::HashMap::new(); + for (uri, _) in &entries { + if uri.starts_with("hf://") { + let (repo, _) = parse_hf_uri(uri)?; + if let std::collections::hash_map::Entry::Vacant(e) = hf_snapshots.entry(repo) { + let repo_name = e.key().clone(); + let snap = crate::hub::from_hf(&repo_name, /* ignore_weights = */ true) + .await + .with_context(|| format!("hub::from_hf({repo_name})"))?; + e.insert(snap); + } + } + } + let client = reqwest::Client::builder() .connect_timeout(std::time::Duration::from_secs(10)) .timeout(std::time::Duration::from_secs(300)) @@ -1047,9 +1008,7 @@ impl ModelDeploymentCard { let blake3_hex = expected.checksum().hash(); let blob = blobs.join(blake3_hex); tracing::debug!(filename = %filename, uri = %uri, blake3 = %blake3_hex, "resolving"); - // Always go through resolve_uri — it verifies existing - // blobs under the lock (peer race + poisoned cache). - resolve_uri(&client, uri, expected, &blob).await?; + resolve_uri(&client, uri, expected, &blob, &hf_snapshots).await?; symlink_force(&blob, &slug_dir.join(&filename))?; } tracing::debug!( @@ -1061,7 +1020,7 @@ impl ModelDeploymentCard { // Pass 3: rewrite cf.path to the cache symlink so downstream // tokenizer/config loaders read from a verified location. - for cf in self.iter_metadata_files_mut() { + for (cf, _) in self.iter_metadata_files_mut() { cf.update_dir(&slug_dir); } Ok(()) @@ -1788,6 +1747,7 @@ mod tests { &url, &test_cf(&url, declared_size), &dest, + &std::collections::HashMap::new(), ) .await; let msg = result.expect_err("expected error").to_string(); @@ -1871,7 +1831,7 @@ mod tests { assert!(snap.join("tokenizer.json").exists()); assert!(snap.join("generation_config.json").exists()); - for cf in mdc.iter_metadata_files() { + for (cf, _) in mdc.iter_metadata_files() { let path = cf.path().expect("post-download local path"); assert!(path.starts_with(&snap)); assert!(std::fs::canonicalize(path)?.starts_with(&blobs)); From c147dd374c1e4a70d6aa68af151c0c42eeb095be Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 07:58:12 -0700 Subject: [PATCH 27/32] fix(self-host): unlink tmp on cancel via Drop guard stage_and_rename and stage_and_rename_sync only cleaned up the tmp on Err or rename-failure. An async cancellation (handle_put aborted by duplicate Added or delete-races-put) drops the future mid-await and leaves .tmp.. orphans. Wrap the tmp path in a TmpGuard whose Drop unlinks it; dismiss the guard after successful rename so the no-op unlink (post-rename ENOENT) is skipped. Test: drop the stage_and_rename future via tokio::time::timeout to simulate cancellation, verify the tmp file is gone. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 91 +++++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 17 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 3a172edb43dc..aa0b9c09b1d1 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -283,24 +283,46 @@ fn unique_tmp_path(dest: &Path) -> PathBuf { p } +/// RAII guard that unlinks a tmp path on drop. `dismiss()` cancels the +/// cleanup once the tmp has been consumed by a successful rename. Drop +/// ignores ENOENT — safe under double-cleanup races. +struct TmpGuard { + path: Option, +} + +impl TmpGuard { + fn new(path: PathBuf) -> Self { + Self { path: Some(path) } + } + fn dismiss(&mut self) { + self.path = None; + } +} + +impl Drop for TmpGuard { + fn drop(&mut self) { + if let Some(p) = self.path.take() { + let _ = std::fs::remove_file(&p); + } + } +} + /// Atomic publish: stage via `f(&tmp)`, then `rename(tmp -> dest)`. /// Concurrent-safe because tmps are per-call (see [`unique_tmp_path`]) /// and `rename(2)` is atomic + overwrites. Tmp is unlinked on any -/// failure. +/// failure path including async cancellation — see [`TmpGuard`]. async fn stage_and_rename(dest: &Path, f: F) -> anyhow::Result<()> where F: FnOnce(PathBuf) -> Fut, Fut: std::future::Future>, { let tmp = unique_tmp_path(dest); - if let Err(err) = f(tmp.clone()).await { - let _ = tokio::fs::remove_file(&tmp).await; - return Err(err); - } - if let Err(err) = tokio::fs::rename(&tmp, dest).await { - let _ = tokio::fs::remove_file(&tmp).await; - anyhow::bail!("renaming {} -> {}: {err}", tmp.display(), dest.display()); - } + let mut guard = TmpGuard::new(tmp.clone()); + f(tmp.clone()).await?; + tokio::fs::rename(&tmp, dest) + .await + .with_context(|| format!("renaming {} -> {}", tmp.display(), dest.display()))?; + guard.dismiss(); Ok(()) } @@ -311,14 +333,11 @@ where F: FnOnce(&Path) -> anyhow::Result<()>, { let tmp = unique_tmp_path(dest); - if let Err(err) = f(&tmp) { - let _ = std::fs::remove_file(&tmp); - return Err(err); - } - if let Err(err) = std::fs::rename(&tmp, dest) { - let _ = std::fs::remove_file(&tmp); - anyhow::bail!("renaming {} -> {}: {err}", tmp.display(), dest.display()); - } + let mut guard = TmpGuard::new(tmp.clone()); + f(&tmp)?; + std::fs::rename(&tmp, dest) + .with_context(|| format!("renaming {} -> {}", tmp.display(), dest.display()))?; + guard.dismiss(); Ok(()) } @@ -1901,4 +1920,42 @@ mod tests { "wrong error: {msg}" ); } + + /// Dropping `stage_and_rename`'s future mid-await (caller cancellation) + /// must still unlink the tmp file via the [`TmpGuard`] drop path. + #[tokio::test] + async fn stage_and_rename_unlinks_tmp_on_cancel() { + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("dest"); + let leaked_before: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .filter_map(Result::ok) + .collect(); + assert!(leaked_before.is_empty(), "tempdir starts empty"); + + let fut = super::stage_and_rename(&dest, |tmp| async move { + std::fs::write(&tmp, b"partial").unwrap(); + // Mimic a worker abort mid-fetch: yield, then never resume. + std::future::pending::<()>().await; + Ok(()) + }); + // Drop the future before it can complete (cancellation). + { + let _ = tokio::time::timeout(std::time::Duration::from_millis(50), fut).await; + } + + // Give Drop a moment to run. + tokio::task::yield_now().await; + + let leaked_after: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .filter_map(Result::ok) + .map(|e| e.path()) + .collect(); + assert!( + leaked_after.is_empty(), + "TmpGuard should have unlinked the tmp on cancel; leaked: {leaked_after:?}" + ); + assert!(!dest.exists(), "dest must not exist after cancel"); + } } From 81b4358245dccb8ea979bc50cac94f9c6350117d Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 08:27:24 -0700 Subject: [PATCH 28/32] refactor(self-host): drop BlobLock Same-(model, namespace, type) registrations are already serialized by ModelWatcher's registering_worker_sets; only one task per MDC reaches download_config. Atomic rename + unique tmp + content addressing handles the cross-model and cross-process cases without a lock. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 49 --------------------------------------- 1 file changed, 49 deletions(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index aa0b9c09b1d1..243646e55f1c 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -194,49 +194,6 @@ fn is_exclusively_mistral_model(directory: &Path) -> bool { !directory.join("config.json").exists() && directory.join("params.json").exists() } -/// `flock(LOCK_EX)` on `.lock`. Per-open-file-description -/// semantics serialize both intra- and inter-process callers, so a -/// single layer covers both. Released when `_file` drops. -struct BlobLock { - _file: std::fs::File, -} - -impl BlobLock { - async fn acquire(blob_path: &Path) -> anyhow::Result { - let lock_path = blob_path.with_extension("lock"); - let file = tokio::task::spawn_blocking(move || -> anyhow::Result { - let f = std::fs::OpenOptions::new() - .create(true) - .truncate(false) - .write(true) - .open(&lock_path) - .with_context(|| format!("opening lock file {}", lock_path.display()))?; - #[cfg(unix)] - { - use std::os::fd::AsRawFd; - // SAFETY: `f` is open and owned; `flock` is a stable syscall. - let rc = unsafe { libc::flock(f.as_raw_fd(), libc::LOCK_EX) }; - if rc != 0 { - anyhow::bail!( - "flock {} failed: {}", - lock_path.display(), - std::io::Error::last_os_error() - ); - } - } - // On non-Unix targets we currently rely on the per-call - // unique tmp + atomic rename to give intra-process safety; - // cross-process locking would need LockFileEx (winapi). - // The dynamo frontend ships on Linux; this path is for - // build-time portability only. - Ok(f) - }) - .await - .context("blob lock spawn_blocking task panicked")??; - Ok(Self { _file: file }) - } -} - /// MDC cache: `blobs/` + `by-slug///`. /// The `` segment mirrors HF Hub's `snapshots//` and /// isolates worker sets that share a model name but publish different @@ -378,12 +335,6 @@ async fn resolve_uri( let parsed = url::Url::parse(uri).with_context(|| format!("parsing artifact uri: {uri}"))?; - let _blob_guard = BlobLock::acquire(dest).await?; - - // Verify-on-write only — a blob at `blobs/` was - // blake3-checked before its atomic rename (below), so its presence - // is sufficient. `$HOME` is inside the trust boundary; matches - // HF / Git / Nix / Docker / OCI cache semantics. if dest.exists() { return Ok(()); } From 53ee6c7626be7ae5ac08bc495e725ef9fa222313 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 08:56:05 -0700 Subject: [PATCH 29/32] chore(deps): drop libc from lib/llm No longer used after BlobLock removal. `disk.rs` uses `nix::libc::*` (re-exported from nix), so dropping the direct dep is safe. Caught by cargo-machete on the pre-merge check. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- Cargo.lock | 1 - lib/llm/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3dc7c849bc07..e64c4eaf8da2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2449,7 +2449,6 @@ dependencies = [ "ipnet", "json-five", "lazy_static", - "libc", "lru 0.12.5", "memfile", "minijinja", diff --git a/lib/llm/Cargo.toml b/lib/llm/Cargo.toml index 24e0b2a0287f..31559be92ed4 100644 --- a/lib/llm/Cargo.toml +++ b/lib/llm/Cargo.toml @@ -77,7 +77,6 @@ futures = { workspace = true } futures-util = { workspace = true } hf-hub = { workspace = true } ipnet = { workspace = true } -libc = { workspace = true } lru = { workspace = true } rand = { workspace = true } oneshot = { workspace = true } From 6b2a342f4c7e7f9729d3861cb34c24099597a716 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 09:15:08 -0700 Subject: [PATCH 30/32] fix(self-host): re-verify on cache hit; refetch on mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the bit-rot / shared-cache-poison / our-write-path-bug window. Cost: blake3 over the file (~5 ms typical, up to ~30 ms for a 30 MB tokenizer.json) per first-worker-per-MDC. Steady state and worker rolls don't trigger it — the watcher's registering_worker_sets dedup makes download_config a per- (model, namespace, type) operation, and subsequent workers join the existing WorkerSet without resolving again. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/llm/src/model_card.rs | 41 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/llm/src/model_card.rs b/lib/llm/src/model_card.rs index 243646e55f1c..a2cb817ddb08 100644 --- a/lib/llm/src/model_card.rs +++ b/lib/llm/src/model_card.rs @@ -336,7 +336,11 @@ async fn resolve_uri( let parsed = url::Url::parse(uri).with_context(|| format!("parsing artifact uri: {uri}"))?; if dest.exists() { - return Ok(()); + if CheckedFile::from_disk(dest).is_ok_and(|cf| cf.checksum() == expected.checksum()) { + return Ok(()); + } + tracing::warn!(dest = %dest.display(), "MDC cache blob failed re-verification; refetching"); + let _ = std::fs::remove_file(dest); } stage_and_rename(dest, |tmp| async move { @@ -1738,6 +1742,41 @@ mod tests { assert_resolve_uri_rejects(b"x".repeat(35).as_slice(), 8, "exceeds cap").await; } + /// Cache hit re-verifies the on-disk blob; mismatch → unlink + refetch. + #[tokio::test] + async fn resolve_uri_refetches_on_cache_hit_mismatch() { + let body: &[u8] = b"valid-bytes-for-blob"; + let dir = tempfile::tempdir().unwrap(); + let valid = dir.path().join("valid"); + std::fs::write(&valid, body).unwrap(); + let cf = super::CheckedFile::from_disk(&valid).unwrap(); + + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("GET", "/f") + .with_status(200) + .with_body(body) + .create_async() + .await; + let url = format!("{}/f", server.url()); + + let dest = dir.path().join("blob"); + std::fs::write(&dest, b"corrupt-bytes").unwrap(); + + super::resolve_uri( + &reqwest::Client::new(), + &url, + &cf, + &dest, + &std::collections::HashMap::new(), + ) + .await + .expect("resolve_uri should refetch and succeed"); + + let after = std::fs::read(&dest).unwrap(); + assert_eq!(after, body, "blob should have been replaced"); + } + /// `parse_hf_uri` round-trip — `hf://repo/filename` parses into /// `(repo, filename)` and rejects malformed inputs. #[test] From 312c447db88bbd3783378d0ce5dc3a13de4d9fd0 Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 09:45:27 -0700 Subject: [PATCH 31/32] chore(deps): sync lib/bindings/kvbm/Cargo.lock after libc drop Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/bindings/kvbm/Cargo.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/bindings/kvbm/Cargo.lock b/lib/bindings/kvbm/Cargo.lock index b212e49cfbc8..082d21f2e6d0 100644 --- a/lib/bindings/kvbm/Cargo.lock +++ b/lib/bindings/kvbm/Cargo.lock @@ -1565,7 +1565,6 @@ dependencies = [ "image", "ipnet", "json-five", - "libc", "lru 0.12.5", "minijinja", "minijinja-contrib", From 011320f09299c21b48b1561f6c1db1608bf20def Mon Sep 17 00:00:00 2001 From: nnshah1 Date: Mon, 11 May 2026 10:12:23 -0700 Subject: [PATCH 32/32] chore(deps): sync lib/bindings/python/Cargo.lock after libc drop Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: nnshah1 --- lib/bindings/python/Cargo.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/bindings/python/Cargo.lock b/lib/bindings/python/Cargo.lock index a20488752a8b..e9bb77d0e246 100644 --- a/lib/bindings/python/Cargo.lock +++ b/lib/bindings/python/Cargo.lock @@ -2084,7 +2084,6 @@ dependencies = [ "image", "ipnet", "json-five", - "libc", "lru 0.12.5", "memfile", "minijinja",