fix(registry): handle 304 Not Modified by returning cached FullManifest - #2562
fix(registry): handle 304 Not Modified by returning cached FullManifest#2562elrrrrrrr wants to merge 1 commit into
Conversation
When npm registry returns 304 Not Modified, instead of returning an error, we now construct a minimal FullManifest from cached versions info and return it. This allows callers like to get dist_tags and version list even when the full manifest is cached. The fix reconstructs FullManifest with: - Package name - dist_tags from cached versions info - Version manifests from disk cache (if available) If no cache is available, it fetches fresh data as before. Fixes #2561
Summary of ChangesHello @elrrrrrrr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the registry service's caching mechanism. By implementing a sophisticated strategy for handling HTTP 304 Not Modified responses, the system can now reconstruct and provide essential package metadata from various cache layers, preventing errors and ensuring continuous access to information like distribution tags and version lists, even when a full manifest update isn't required. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great fix that improves the handling of 304 Not Modified responses from the registry. Instead of returning an error, constructing a minimal manifest from the cache is much more useful for callers. The logic is sound and covers different caching scenarios well. I have a couple of suggestions to further improve the implementation by removing a redundant cache call and parallelizing part of the cache population, which should improve performance.
| // Cache versions in memory for later use | ||
| self.cache | ||
| .set_versions(name.to_string(), versions_info.clone()); |
There was a problem hiding this comment.
This call to set_versions appears to be redundant. The versions_info is guaranteed to be in the memory cache at this point because both get_versions_from_disk (which populates disk_versions) and get_versions either load it into or retrieve it from the memory cache. You can remove these lines to avoid the redundant operation and a clone.
| for v in &versions_info.versions.version_list { | ||
| if let Some(manifest) = self.cache.get_version_manifest_from_disk(name, v).await { | ||
| minimal_manifest.versions.insert(v.clone(), manifest); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop populates version manifests by awaiting a disk read for each version sequentially. This can be slow if a package has many versions. You can improve performance by fetching them concurrently using futures::stream. This will execute the I/O-bound get_version_manifest_from_disk calls in parallel.
Note: This requires futures to be a dependency.
| for v in &versions_info.versions.version_list { | |
| if let Some(manifest) = self.cache.get_version_manifest_from_disk(name, v).await { | |
| minimal_manifest.versions.insert(v.clone(), manifest); | |
| } | |
| } | |
| minimal_manifest.versions = futures::stream::iter(&versions_info.versions.version_list) | |
| .filter_map(|v| async move { | |
| self.cache | |
| .get_version_manifest_from_disk(name, v) | |
| .await | |
| .map(|manifest| (v.clone(), manifest)) | |
| }) | |
| .collect() | |
| .await; |
📊 Performance Benchmark Report (with-antd)🚀 Utoopack Performance Report: Async Task Scheduling Overhead AnalysisReport ID: 📊 Executive SummaryThis report analyzes the performance of Utoopack/Turbopack, covering the full spectrum of the Performance Analysis Protocol (P0-P4). Key Findings
Workload Distribution by Tier
⚡ Parallelization Analysis (P0-P2)Thread Utilization
Assessment: With 17 threads available, achieving 8.6x parallelism indicates significant loss of potential parallelism. 📈 Top 20 Tasks (Global)These are the most significant tasks by total duration:
🔍 Deep Dive by Tier🔴 Tier 1: Runtime & Resolution (P0)Focus: Task scheduling and dependency resolution.
Potential P0 Issues:
🟠 Tier 2: Physical & Resource Barriers (P1)Focus: Hardware utilization, I/O, and heavy monoliths.
Potential P1 Issues:
🟡 Tier 3: Architecture & Asset Pipeline (P2-P3)Focus: Global state and transformation pipeline.
💡 Recommendations (Prioritized P0-P2)🚨 Critical: (P0) ImprovementProblem: 50.9% thread utilization.
|
| Signal | Status | Finding |
|---|---|---|
| Tracing Noise (P0) | ✅ Acceptable | 59.6% of tasks < 10µs |
| Thread Utilization (P0) | 🚨 Low | 50.9% utilization |
| Heavy Monoliths (P1) | 20 tasks > 100ms | |
| Asset Pipeline (P3) | 🔍 Review | 4,561.8 ms total |
| Bridge/Interop (P4) | ✅ Low | 0.0 ms total |
🎯 Action Items (Comprehensive P0-P4)
- [P0] Profile lock contention to address 49% lost parallelism
- [P1] Breakdown heavy monolith tasks (>100ms) to improve granularity
- [P1] Review I/O patterns for potential batching opportunities
- [P3] Optimize asset transformation pipeline hot-spots
- [P4] Reduce "chatty" bridge operations if interop overhead is significant
Report generated by Utoopack Performance Analysis Agent on 2026-02-02
Following: Utoopack Performance Analysis Agent Protocol
1 participant
Summary
When the npm registry returns 304 Not Modified, instead of returning an error with a helpful message that the caller could not use, we now construct a minimal FullManifest from the cached versions info and return it.
This allows callers like ut view to get dist_tags and the version list even when the full manifest is already cached locally.
Changes
In resolve_full_manifest:
The fix maintains backward compatibility - if cache is fully populated, the behavior is identical to before. The improvement is specifically for the 304 case when only versions info is cached.
Testing
Issue #2561 describes the problem where ut view lodash failed with:
ERROR Failed to fetch package info for lodash: 304 Not Modified - use versions cache for lodash
With this fix, the 304 case now returns a usable FullManifest constructed from cache, allowing the command to succeed.
Fixes #2561