diff --git a/agents/rust-code-guard.md b/agents/rust-code-guard.md index d4d72c9aef..59743f93bb 100644 --- a/agents/rust-code-guard.md +++ b/agents/rust-code-guard.md @@ -42,7 +42,7 @@ You are the Rust code review agent for the utoo project. Your responsibility is ## Review Checklist -For each diff, review against the following 12 dimensions and output findings with fix suggestions. +For each diff, review against the following 14 dimensions and output findings with fix suggestions. ### 1. Type Modeling & Zero-Cost Abstractions @@ -334,6 +334,152 @@ impl PathExt for std::path::Path { fn is_hidden(&self) -> bool { .. } } - [ ] Side-effectful iteration: use `for` loop not `.iter().for_each(|..| { ... })`. - [ ] Collection building: prefer `.filter_map().collect()` over `Vec::new()` + `for` + `push` when the mapping is simple. +### 14. Logging Discipline & `anyhow` Context Conventions + +**Rule: Logs cost format CPU + channel send on every event. Levels exist to match the audience: never punish CI / large installs with per-package debug noise, never duplicate the same fact across log + error chain, never strip context (URL, path) when wrapping an error for return.** + +This dimension came out of an audit where a single `utoo deps` for ant-design generated **2.3 MB of log** (~16k lines) where ~95 % was per-package dedup / cache-hit / "fetching" / "preloaded" noise no operator ever reads. Subsequent fixes shrank the same workload to ~2k lines while improving failure diagnosis (link errors used to drop source/target path; retry loops used to print the URL ten times). + +#### 14.1 Log level discipline + +- [ ] Is a `tracing::debug!()` fired **once per package / per BFS edge / per cache lookup** in a hot path that visits 500+ items? Drop it or downgrade to `trace`. +- [ ] Is the same line printed by **multiple sibling layers** (e.g. retry loop warns + outer wrapper warns + final `anyhow!` chain) for one logical failure? Pick exactly one layer. +- [ ] Is a level used to mean its opposite? `tracing::warn!("Optional dependency X failed (ignored)")` is fine; `tracing::warn!("HTTP 404 for {url}")` for a normal lookup miss is noise — use `debug` or fold into the error chain. + +| Level | Frequency rule | Examples | +|---|---|---| +| `error!` | One terminal failure per CLI invocation | Fatal install error before exit | +| `warn!` | Per-incident, ≤ once per affected resource | "Failed to clean target dir X" | +| `info!` | Phase-level, ≤ ~10 per command | "Retry succeeded on attempt 3" | +| `debug!` | Phase markers + one-shot config | NEVER per-package / per-edge in BFS | +| `trace!` | Verbose flow-tracing | Per-edge inspector if needed | + +```rust +// ❌ BAD — fires per package across thousands of BFS edges +tracing::debug!("Reusing existing {}@{} at {:?}", name, version, idx); + +// ✅ GOOD — one line per phase, with summary stats +tracing::debug!("Preload phase: {} initial deps, concurrency={}", n, concurrency); +// Final stats line is enough to reconstruct what happened. +``` + +#### 14.2 Don't double-log a failure + +Both `tracing::warn!` and the `anyhow!` value carry the same data, but they're *spent* differently — the warn fires immediately on the producer thread, the anyhow chain fires only when the failure escapes to `main`. **Pick one** based on how the caller will surface the failure. + +```rust +// ❌ BAD — every retry attempt prints the URL; final anyhow chain repeats it +match status.as_u16() { + 429 => { + tracing::warn!("HTTP 429 for {}", url); // ← per-attempt noise + FetchError::Retryable(anyhow!("HTTP 429: {url}")) // ← same data, second emission + } + ... +} + +// ✅ GOOD — anyhow chain owns the URL, retry framework propagates it once on final failure +match status.as_u16() { + 429 => FetchError::Retryable(anyhow!("HTTP 429: {url}")), + ... +} +// Caller's `with_context(|| format!("Download failed: {url}"))` adds one more layer at the boundary. +``` + +#### 14.3 Preserve context when wrapping errors + +Returning a fresh `anyhow!(format!("Link failed: {e}"))` strips the path or URL the inner error never knew about. The caller sees `Link failed: Permission denied (os error 13)` and has no idea **which file**. + +```rust +// ❌ BAD — the only context the user gets is the OS error string +if let Err(e) = link(&resolved, &path).await { + tracing::debug!("Link failed: source={resolved}, target={path}, error={e}"); // ← debug-only, + return Err(anyhow::anyhow!("Link failed: {e}")); // needs file log +} + +// ✅ GOOD — paths land in the returned error chain, not just in opt-in debug +link(&resolved, &path) + .await + .with_context(|| format!("Link failed: {resolved} -> {path}"))?; +``` + +The same rule applies to `inspect_err`-then-warn at the inner layer: if the warn carries the path/URL but the returned error doesn't, **the warn is on a verbose channel a CLI user can't see**. Either move the data into the returned error or accept that the warn is for `UTOO_FILE_LOG=debug` operators only. + +#### 14.4 `anyhow` context idioms + +- [ ] Use `.with_context(|| format!(...))` (lazy closure), not `.context(format!(...))` (eager). The closure fires only on `Err`; `format!` allocates always. +- [ ] Use `anyhow::bail!(...)` not `return Err(anyhow::anyhow!(...))`. +- [ ] Use `with_context(|| ...)?` not `.map_err(|e| anyhow!("{}", e))?` — the latter erases the source chain. +- [ ] Format an `anyhow::Error` with `{:#}` (alternate) when logging — that prints the full cause chain. Plain `{}` only shows the outermost message. + +```rust +// ❌ BAD — eager allocation on every call, even when result is Ok +fs::write(path, contents).context(format!("write {}", path.display()))?; + +// ✅ GOOD — lazy: format runs only on Err +fs::write(path, contents).with_context(|| format!("write {}", path.display()))?; + +// ❌ BAD — drops the source chain, replaces with a single string +download_bytes(&url).await.map_err(|e| anyhow!("{}", e))?; + +// ✅ GOOD — preserves chain, adds context +download_bytes(&url).await.with_context(|| format!("download {url}"))?; + +// ❌ BAD — outer message only +tracing::warn!("Download failed: {}", e); + +// ✅ GOOD — full chain so root cause is visible +tracing::warn!("Download failed: {:#}", e); +``` + +#### 14.5 Per-incident warns inside loops + +When a tight loop can fail repeatedly for the same root cause (per-file hardlink failure across a 1000-file package, per-attempt retry within a request), warn **once per package / per request** and suppress subsequent occurrences. The first warn names the failure mode; printing it 999 more times only obscures the next problem. + +```rust +// ❌ BAD — 1000-file package with EMLINK = 1000 warn lines +for entry in &files { + if let Err(e) = fs::hard_link(&entry.src, &entry.dst) { + tracing::warn!("hardlink failed: {} -> {}: {}", entry.src.display(), entry.dst.display(), e); + copy_file_sync(&entry.src, &entry.dst)?; + } +} + +// ✅ GOOD — one warn per package, then silent fallback +let mut warned = false; +for entry in &files { + if let Err(e) = fs::hard_link(&entry.src, &entry.dst) { + if !warned { + tracing::warn!("hardlink failed: {} -> {}: {}; falling back to copy (further suppressed)", ...); + warned = true; + } + copy_file_sync(&entry.src, &entry.dst)?; + } +} +``` + +For retries specifically: **don't warn each attempt at all**. Let the retry framework swallow transient failures. Log only on `info!` for "retry succeeded on attempt N" and let the outer caller's `.with_context(|| format!("... {url}"))?` surface the final failure once. + +#### 14.6 Failure paths must include the resource + +Network: URL must be in the returned error (not only in the per-attempt warn). +File: the failing path must be in the returned error (`with_context(|| format!("read {}", path.display()))`). +Link: both source and target paths. +Spawn / external command: command + args. + +If you find yourself writing `tracing::debug!("... source=..., target=..., error=...")` immediately before `return Err(anyhow!("X failed: {e}"))`, **move the source/target into the returned error** instead — the debug log is invisible to a normal CLI user. + +#### Anti-patterns + +| # | Anti-Pattern | Grep Pattern | Fix Direction | +|---|---|---|---| +| A27 | Per-package debug in hot path | `tracing::debug!\("(Cache hit\|Resolved\|Reusing\|Preloaded\|Cloned\|Downloaded)` | Drop or move to phase-level summary | +| A28 | Retry-loop warn duplication | `tracing::warn!\("Retry .*url:` or sibling warns inside a `RetryIf` closure | Remove per-attempt warn; let `with_context` carry URL on final failure | +| A29 | Lost context in returned error | `tracing::debug!\(.*source=.*target=` followed by `Err\(anyhow!\("X failed: \{e\}"\)\)` | `.with_context(\|\| format!("X failed: {src} -> {dst}"))?` | +| A30 | Eager `.context(format!(...))` | `\.context\(format!\(` | `.with_context(\|\| format!(...))` | +| A31 | Source chain stripped | `.map_err\(\|e\| anyhow!\("\{\}", e\)\)` | `.with_context(\|\| ...)` to preserve `e` as cause | +| A32 | Outer-only error format | `tracing::(warn\|error)!\("[^"]*: \{\}", e\)` where `e: anyhow::Error` | `{:#}` instead of `{}` to print full chain | +| A33 | Per-iteration warn spam | warn inside `for entry in &files` / `for attempt in 0..N` without a "warned once" latch | Latch a `bool` and suppress after first occurrence | + --- ## Output Format @@ -343,7 +489,7 @@ For each issue in each file, output in the following format: ```markdown ## : -**Dimension**: +**Dimension**: **Severity**: 🔴 Must fix | 🟡 Should fix | 🟢 Style suggestion **Issue**: **Reason**: @@ -360,7 +506,7 @@ For each issue in each file, output in the following format: 1. **Identify scope** — determine which files to review (PR diff, staged changes, or user-specified paths) 2. **Read and understand context** — read each file and understand its role within the crate; check `lib.rs` exports and `Cargo.toml` dependencies 3. **Run automated checks** — execute `cargo clippy` and `cargo fmt --check` to catch mechanical issues -4. **Review against the 13 dimensions** — check each item in the checklist above, scanning for anti-patterns A1–A26 +4. **Review against the 14 dimensions** — check each item in the checklist above, scanning for anti-patterns A1–A33 5. **Output findings** — report issues in the specified format, sorted by severity (🔴 first, then 🟡, then 🟢) --- diff --git a/crates/pm/src/service/install.rs b/crates/pm/src/service/install.rs index 9f591ee85b..0390a17af1 100644 --- a/crates/pm/src/service/install.rs +++ b/crates/pm/src/service/install.rs @@ -90,7 +90,6 @@ pub async fn install_packages( // Skip packages based on omit config if should_omit_package(package, omit) { PROGRESS_BAR.inc(1); - tracing::debug!("Skipping omitted dependency: {}", path); continue; } let path = path.clone(); @@ -110,16 +109,11 @@ pub async fn install_packages( let link_name = extract_package_name(&path); if link_name.is_empty() { PROGRESS_BAR.inc(1); - tracing::debug!("Link skipped due to empty package name: {path}"); continue; } - tracing::debug!("Attempting to link from {resolved} to {path}"); - if let Err(e) = link(Path::new(&resolved), Path::new(&path)).await { - tracing::debug!( - "Link failed: source={resolved}, target={path}, error={e}" - ); - return Err(anyhow::anyhow!("Link failed: {e}")); - } + link(Path::new(&resolved), Path::new(&path)) + .await + .with_context(|| format!("Link failed: {resolved} -> {path}"))?; PROGRESS_BAR.inc(1); continue; } @@ -129,7 +123,6 @@ pub async fn install_packages( && !is_cpu_compatible(cpu) { PROGRESS_BAR.inc(1); - tracing::debug!("cpu skipped: {}", &path); continue; } @@ -137,7 +130,6 @@ pub async fn install_packages( && !is_os_compatible(os) { PROGRESS_BAR.inc(1); - tracing::debug!("os skipped: {}", &path); continue; } @@ -158,7 +150,9 @@ pub async fn install_packages( clone_package_once(&name, &version, &resolved, &target_path).await { if is_optional { - tracing::warn!("Optional dependency {name} failed (ignored)"); + tracing::warn!( + "Optional dependency {name} failed (ignored): {e:#}" + ); PROGRESS_BAR.inc(1); return Ok(()); } @@ -171,7 +165,6 @@ pub async fn install_packages( clone_tasks.push(task); } else { PROGRESS_BAR.inc(1); - tracing::debug!("{path} no resolved info skipped"); } } } diff --git a/crates/pm/src/util/cloner.rs b/crates/pm/src/util/cloner.rs index 5d47c06eae..76c6029702 100644 --- a/crates/pm/src/util/cloner.rs +++ b/crates/pm/src/util/cloner.rs @@ -93,7 +93,6 @@ pub async fn clone_package_once( if fresh { CLONE_COUNT.fetch_add(1, Ordering::Relaxed); - tracing::debug!("Cloned: {}@{} to {}", name, version, target_path.display()); } Some(()) }) @@ -210,6 +209,10 @@ mod hardlink_clone { // Any other hardlink error (EMLINK on a single inode whose link // count is exhausted, EPERM on a specific file, etc.) is // per-file: copy this one and keep trying hardlink on the next. + // We warn only on the first such failure per package to avoid + // spamming hundreds of identical warnings when an entire package + // can't be hardlinked. + let mut warned_per_file = false; for entry in &files { if force_copy { copy_file_sync(&entry.src, &entry.dst)?; @@ -222,13 +225,14 @@ mod hardlink_clone { e ); force_copy = true; - } else { + } else if !warned_per_file { tracing::warn!( - "hardlink failed for {} -> {}: {}; falling back to copy for this file", + "hardlink failed for {} -> {}: {}; falling back to copy (further per-file failures suppressed)", entry.src.display(), entry.dst.display(), e ); + warned_per_file = true; } copy_file_sync(&entry.src, &entry.dst)?; } @@ -246,7 +250,6 @@ async fn validate_directory(src: &Path, dst: &Path) -> Result { } if !fs::metadata(src).await?.is_dir() || !fs::metadata(dst).await?.is_dir() { - tracing::debug!("validating failed, since it's not a directory"); return Ok(false); } @@ -298,21 +301,6 @@ async fn validate_directory(src: &Path, dst: &Path) -> Result { dst_entries.sort_by(|a, b| a.path.cmp(&b.path)); if src_entries.len() != dst_entries.len() { - tracing::debug!( - "validating failed {}:{} to {}:{}, since entries length is not equal\nsrc entries: {:?}\ndst entries: {:?}", - src.display(), - src_entries.len(), - dst.display(), - dst_entries.len(), - src_entries - .iter() - .map(|e| e.path.file_name().unwrap_or_default()) - .collect::>(), - dst_entries - .iter() - .map(|e| e.path.file_name().unwrap_or_default()) - .collect::>() - ); return Ok(false); } @@ -324,23 +312,9 @@ async fn validate_directory(src: &Path, dst: &Path) -> Result { } } else if !src_entry.is_dir && !dst_entry.is_dir { if src_entry.size != dst_entry.size { - tracing::debug!( - "validating failed {}:{} to {}:{}, since diff size", - src_entry.path.display(), - src_entry.size, - dst_entry.path.display(), - dst_entry.size - ); return Ok(false); } } else { - tracing::debug!( - "validating failed {}:{} to {}:{}, since diff size", - src_entry.path.display(), - src_entry.size, - dst_entry.path.display(), - dst_entry.size - ); return Ok(false); } } @@ -373,22 +347,12 @@ async fn clone(src: &Path, dst: &Path, find_real: bool) -> Result<()> { }; if crate::fs::try_exists(dst).await? { - let is_valid = validate_directory(&real_src, dst) - .await - .unwrap_or_else(|e| { - tracing::debug!("validate_directory error: {e}, will override target directory"); - false - }); + let is_valid = validate_directory(&real_src, dst).await.unwrap_or(false); if is_valid { - tracing::debug!( - "Target directory {} already exists and validation passed, skipping clone", - dst.display() - ); return Ok(()); } - tracing::debug!("{real_src:?} --> {dst:?} overrides"); if let Err(e) = fs::remove_dir_all(dst).await { tracing::warn!("Failed to clean target directory {}: {}", dst.display(), e); } @@ -405,20 +369,13 @@ async fn clone(src: &Path, dst: &Path, find_real: bool) -> Result<()> { Retry::spawn(create_retry_strategy(), || async { match unsafe { clonefile(src_c.as_ptr(), dst_c.as_ptr(), 0) } { - 0 => { - tracing::debug!("clone {} to {} success", real_src.display(), dst.display()); - Ok(()) - } + 0 => Ok(()), _ => { - let _ = fs::remove_dir_all(dst).await.map_err(|e| { - tracing::debug!( - "Failed to clean target directory {}: {}", - dst.display(), - e - ); - }); + let _ = fs::remove_dir_all(dst).await; Err(anyhow::anyhow!( - "Failed to clone file: {}", + "clonefile {} -> {}: {}", + real_src.display(), + dst.display(), std::io::Error::last_os_error() )) } @@ -430,8 +387,11 @@ async fn clone(src: &Path, dst: &Path, find_real: bool) -> Result<()> { #[cfg(not(target_os = "macos"))] { Retry::spawn(create_retry_strategy(), || async { - hardlink_clone::clone_dir(&real_src, dst).await?; - tracing::debug!("clone {} to {} success", real_src.display(), dst.display()); + hardlink_clone::clone_dir(&real_src, dst) + .await + .with_context(|| { + format!("clone_dir {} -> {}", real_src.display(), dst.display()) + })?; Ok::<(), anyhow::Error>(()) }) .await?; @@ -464,18 +424,8 @@ pub async fn clone_package( ) -> Result { if crate::fs::try_exists(dst).await? { if validate_name_version(dst, name, version).await { - tracing::debug!( - "Package {}@{} already exists at {}, skipping clone", - name, - version, - dst.display() - ); return Ok(false); } - tracing::debug!( - "Package at {} has mismatched name/version, removing and re-cloning", - dst.display() - ); if let Err(e) = fs::remove_dir_all(dst).await { tracing::warn!("Failed to clean target directory {}: {}", dst.display(), e); } diff --git a/crates/pm/src/util/downloader.rs b/crates/pm/src/util/downloader.rs index 2f46a4058c..76526eedda 100644 --- a/crates/pm/src/util/downloader.rs +++ b/crates/pm/src/util/downloader.rs @@ -81,7 +81,6 @@ pub async fn git_cache_lookup(name: &str, version: &str, tarball_url: &str) -> O .await .unwrap_or(false) { - tracing::debug!("Git package cache hit: {}@{}", name, version); return Some(cache_path); } tracing::warn!( @@ -116,20 +115,12 @@ async fn slot_cache_lookup(name: &str, slot: String) -> Option { /// project root before reaching the cloner. pub async fn file_cache_lookup(name: &str, tarball_url: &str) -> Option { let abs_path = tarball_url.strip_prefix("file:")?; - let hit = slot_cache_lookup(name, file_cache_slot(std::path::Path::new(abs_path))).await; - if hit.is_some() { - tracing::debug!("file: dep cache hit: {} ({})", name, tarball_url); - } - hit + slot_cache_lookup(name, file_cache_slot(std::path::Path::new(abs_path))).await } /// Look up the cache path for an HTTP(S) tarball dep. pub async fn http_tarball_cache_lookup(name: &str, tarball_url: &str) -> Option { - let hit = slot_cache_lookup(name, http_cache_slot(tarball_url)).await; - if hit.is_some() { - tracing::debug!("HTTP tarball cache hit: {} ({})", name, tarball_url); - } - hit + slot_cache_lookup(name, http_cache_slot(tarball_url)).await } /// Resolve the local cache path for a package, downloading if necessary. @@ -173,7 +164,6 @@ pub async fn download_to_cache(name: &str, version: &str, tarball_url: &str) -> .unwrap_or(false) { REUSE_COUNT.fetch_add(1, Ordering::Relaxed); - tracing::debug!("Cache hit: {}@{}", name, version); return Some(cache_path); } @@ -183,17 +173,32 @@ pub async fn download_to_cache(name: &str, version: &str, tarball_url: &str) -> let _permit = semaphore.acquire().await.ok()?; let bytes = download_bytes(&tarball_url) .await - .inspect_err(|e| tracing::warn!("Download failed: {}@{}: {}", name, version, e)) + .inspect_err(|e| { + tracing::warn!( + "Download {}@{} from {}: {:#}", + name, + version, + tarball_url, + e + ) + }) .ok()?; // Extract extract_and_write(bytes, &cache_path) .await - .inspect_err(|e| tracing::warn!("Extract failed: {}@{}: {}", name, version, e)) + .inspect_err(|e| { + tracing::warn!( + "Extract {}@{} into {}: {:#}", + name, + version, + cache_path.display(), + e + ) + }) .ok()?; DOWNLOAD_COUNT.fetch_add(1, Ordering::Relaxed); - tracing::debug!("Downloaded: {}@{}", name, version); Some(cache_path) }) .await @@ -209,51 +214,31 @@ pub async fn download_bytes(url: &str) -> Result { || async { let attempt = retry_count.fetch_add(1, Ordering::Relaxed); - let response = match DOWNLOADER_CLIENT.get(url).send().await { - Ok(r) => r, - Err(e) => { - tracing::warn!( - "Retry {}/10 - Network error: {}, url: {}", - attempt + 1, - e, - url - ); - return Err(RetryableError::Temporary(format!("Network error: {e}"))); - } - }; + let response = DOWNLOADER_CLIENT + .get(url) + .send() + .await + .map_err(|e| RetryableError::Temporary(format!("Network error: {e}")))?; match response.status() { StatusCode::OK => { - let bytes = response.bytes().await.map_err(|e| { - tracing::warn!( - "Retry {}/10 - Stream error: {}, url: {}", - attempt + 1, - e, - url - ); - RetryableError::Temporary(format!("Stream error: {e}")) - })?; + let bytes = response + .bytes() + .await + .map_err(|e| RetryableError::Temporary(format!("Stream error: {e}")))?; if attempt > 0 { - tracing::info!("Retry succeeded on attempt {}, url: {}", attempt + 1, url); + tracing::info!("Retry succeeded on attempt {}: {url}", attempt + 1); } Ok(bytes) } - StatusCode::NOT_FOUND => { - tracing::debug!("URL not found {url}"); - Err(RetryableError::Permanent(format!("URL not found {url}"))) - } - status => { - tracing::warn!("Retry {}/10 - HTTP {}, url: {}", attempt + 1, status, url); - Err(RetryableError::Temporary(format!( - "HTTP error: {status}, url: {url}" - ))) - } + StatusCode::NOT_FOUND => Err(RetryableError::Permanent(format!("HTTP 404: {url}"))), + status => Err(RetryableError::Temporary(format!("HTTP {status}: {url}"))), } }, |e: &RetryableError| matches!(e, RetryableError::Temporary(_)), ) .await - .context("Download failed after retries") + .with_context(|| format!("Download failed after retries: {url}")) } #[cfg(test)] diff --git a/crates/ruborist/src/model/graph.rs b/crates/ruborist/src/model/graph.rs index af76730058..771896e739 100644 --- a/crates/ruborist/src/model/graph.rs +++ b/crates/ruborist/src/model/graph.rs @@ -459,13 +459,6 @@ impl DependencyGraph { let child = &self.graph[child_idx]; if child.name == name { if matches(spec, &child.version) { - tracing::debug!( - "found existing deps {}@{} got {}, reuse at {:?}", - name, - spec, - child.version, - child_idx - ); return FindResult::Reuse(child_idx); } else { tracing::debug!( diff --git a/crates/ruborist/src/resolver/builder.rs b/crates/ruborist/src/resolver/builder.rs index 53dca2826b..b0bf2794c6 100644 --- a/crates/ruborist/src/resolver/builder.rs +++ b/crates/ruborist/src/resolver/builder.rs @@ -238,14 +238,6 @@ pub fn create_package_node( parent_node.path.join(format!("node_modules/{name}")) }; - tracing::debug!( - "Creating node {}@{} under parent {:?} at {}", - name, - pkg.version, - parent, - path.display() - ); - PackageNode::from_version_manifest(name.to_string(), path, pkg.manifest.clone()) } @@ -465,27 +457,9 @@ pub async fn process_dependency( edge_info: &DependencyEdgeInfo, config: &BuildDepsConfig, ) -> Result> { - tracing::debug!( - "Processing dependency {}@{} from [{:?}]", - edge_info.name, - edge_info.spec, - node_index - ); - // Find installation location match graph.find_compatible_node(node_index, &edge_info.name, &edge_info.spec) { FindResult::Reuse(existing_index) => { - let version = graph - .get_node(existing_index) - .map(|n| n.version.as_str()) - .unwrap_or("unknown"); - tracing::debug!( - "Reusing existing {}@{} at {:?}", - edge_info.name, - version, - existing_index - ); - // Mark edge as resolved graph.mark_dependency_resolved(edge_info.edge_id, existing_index); @@ -495,13 +469,6 @@ pub async fn process_dependency( Ok(ProcessResult::Reused(existing_index)) } FindResult::Conflict(conflict_parent) | FindResult::New(conflict_parent) => { - tracing::debug!( - "Need to resolve {}@{} (conflict/new at {:?})", - edge_info.name, - edge_info.spec, - conflict_parent - ); - // Parse spec once and exhaustively route by variant. // The exhaustive match ensures the compiler forces a decision for any // new PackageSpec variant — no silent fall-through to the wrong resolver. @@ -658,13 +625,6 @@ pub async fn process_dependency( resolved }; - tracing::debug!( - "Resolved {}@{} => {}", - edge_info.name, - edge_info.spec, - resolved.version - ); - // Create new node let new_node = create_package_node(&edge_info.name, &resolved, conflict_parent, graph); let new_index = graph.add_node(new_node); diff --git a/crates/ruborist/src/resolver/preload.rs b/crates/ruborist/src/resolver/preload.rs index 9fb5aff67b..14824b6cec 100644 --- a/crates/ruborist/src/resolver/preload.rs +++ b/crates/ruborist/src/resolver/preload.rs @@ -156,7 +156,6 @@ where match result { Ok(resolved) => { stats.success_count += 1; - tracing::debug!("Preloaded {}@{}", name, resolved.version); receiver.on_event(BuildEvent::PreloadProgress { name: &name, diff --git a/crates/ruborist/src/resolver/version.rs b/crates/ruborist/src/resolver/version.rs index 8ca56bd8a7..416a56c4d6 100644 --- a/crates/ruborist/src/resolver/version.rs +++ b/crates/ruborist/src/resolver/version.rs @@ -52,10 +52,7 @@ pub fn resolve_target_version(view: VersionsRef<'_>, spec: &str) -> Result Option> { - let result = self.0.full_manifests.get(name).map(|v| v.clone()); - if result.is_some() { - tracing::debug!("Memory cache hit for full manifest: {name}"); - } - result + self.0.full_manifests.get(name).map(|v| v.clone()) } pub fn set_full_manifest(&self, name: String, manifest: Arc) { - tracing::debug!("Caching full manifest in memory: {name}"); self.0.full_manifests.insert(name, manifest); } pub fn get_versions(&self, name: &str) -> Option> { - let result = self.0.versions_info.get(name).map(|v| v.clone()); - if result.is_some() { - tracing::debug!("Memory cache hit for versions: {name}"); - } - result + self.0.versions_info.get(name).map(|v| v.clone()) } pub fn set_versions(&self, name: String, info: Arc) { - tracing::debug!("Caching versions in memory: {name}"); self.0.versions_info.insert(name, info); } @@ -139,11 +129,7 @@ impl MemoryCache { version: &str, ) -> Option> { let key = format!("{name}@{version}"); - let result = self.0.version_manifests.get(&key).map(|v| v.clone()); - if result.is_some() { - tracing::debug!("Memory cache hit for version manifest: {name}@{version}"); - } - result + self.0.version_manifests.get(&key).map(|v| v.clone()) } pub fn set_version_manifest( @@ -152,7 +138,6 @@ impl MemoryCache { version: String, manifest: Arc, ) { - tracing::debug!("Caching version manifest in memory: {name}@{version}"); let key = format!("{name}@{version}"); self.0.version_manifests.insert(key, manifest); } diff --git a/crates/ruborist/src/service/fetch.rs b/crates/ruborist/src/service/fetch.rs index f13e89466c..fde4621eee 100644 --- a/crates/ruborist/src/service/fetch.rs +++ b/crates/ruborist/src/service/fetch.rs @@ -88,19 +88,13 @@ pub fn classify_reqwest_error(e: reqwest::Error) -> FetchError { /// Callers should handle the 2xx (and 304, for ETag-aware endpoints) paths /// before reaching this function. pub fn classify_status(status: reqwest::StatusCode, url: &str) -> FetchError { + // Status + URL are carried by the anyhow chain; the tokio-retry caller + // surfaces them to the user on final failure. Skip per-attempt tracing + // here so a 10-retry loop doesn't spam the same line ten times. match status.as_u16() { 404 => FetchError::Permanent(anyhow!("HTTP 404: {url}")), - 429 => { - tracing::warn!("HTTP 429 (rate limited) for {}", url); - FetchError::Retryable(anyhow!("HTTP 429: {url}")) - } - s if (500..600).contains(&s) => { - tracing::warn!("HTTP {} for {}", status, url); - FetchError::Retryable(anyhow!("HTTP {status}: {url}")) - } - _ => { - tracing::warn!("HTTP {} for {}", status, url); - FetchError::Permanent(anyhow!("HTTP {status}: {url}")) - } + 429 => FetchError::Retryable(anyhow!("HTTP 429: {url}")), + s if (500..600).contains(&s) => FetchError::Retryable(anyhow!("HTTP {status}: {url}")), + _ => FetchError::Permanent(anyhow!("HTTP {status}: {url}")), } } diff --git a/crates/ruborist/src/service/manifest.rs b/crates/ruborist/src/service/manifest.rs index b9dbff7c8d..4e796d3f8e 100644 --- a/crates/ruborist/src/service/manifest.rs +++ b/crates/ruborist/src/service/manifest.rs @@ -52,8 +52,6 @@ pub async fn fetch_full_manifest(opts: FetchManifestOptions<'_>) -> Result "application/json", }; - tracing::debug!("Fetching full manifest for {} from {}", opts.name, url); - RetryIf::spawn( retry_strategy(), || { @@ -161,13 +159,6 @@ pub async fn fetch_version_manifest( MetadataFormat::Complete => "application/json", }; - tracing::debug!( - "Fetching version manifest for {}@{} from {}", - opts.name, - opts.spec, - url - ); - RetryIf::spawn( retry_strategy(), || { diff --git a/crates/ruborist/src/service/registry.rs b/crates/ruborist/src/service/registry.rs index 2e0f6b720f..a978be50d8 100644 --- a/crates/ruborist/src/service/registry.rs +++ b/crates/ruborist/src/service/registry.rs @@ -268,7 +268,6 @@ impl UnifiedRegistry { self.store.store_versions(name, versions_info); } manifest::FetchManifestResult::NotModified => { - tracing::debug!("ETag cache hit (304) for: {}", name); if let Some(versions_info) = store_versions { // Only populate `versions_info`; absence of // `full_manifests` after the gate is the 304 @@ -356,7 +355,6 @@ impl UnifiedRegistry { && deno_semver::Version::parse_from_npm(spec).is_ok() && let Some(manifest) = self.store.load_version_manifest(name, spec).await { - tracing::debug!("Persistent store hit for version manifest: {name}@{spec}"); // Populate memory cache ourselves — store knows nothing about it. self.cache.set_version_manifest( name.to_string(), @@ -394,7 +392,6 @@ impl UnifiedRegistry { return Ok(()); } - tracing::debug!("Cache miss for {}@{}, fetching from network", name, spec); let manifest = manifest::fetch_version_manifest(manifest::FetchVersionManifestOptions { registry_url: &self.registry_url,