feat(pnpr): revalidate stale packuments with conditional GET to upstream#12239
Merged
Conversation
When a cached upstream packument goes stale past its TTL, pnpr now revalidates it with a conditional GET instead of unconditionally re-downloading the body. The upstream's ETag / Last-Modified are stored in a sidecar (.package.json.meta) next to the cached packument and replayed as If-None-Match / If-Modified-Since on the next refresh. A 304 Not Modified serves the cached copy and bumps its mtime so the entry is fresh again, saving the (often large) packument download — matching verdaccio's uplink revalidation. The per-request access log gains a `cache=` field (hit / revalidated / miss / stale / hosted) so operators can see how each packument read was served against the proxy cache. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds conditional-GET (ETag/Last-Modified) cache validators: upstream captures/replays validators, storage persists a .package.json.meta sidecar and reports freshness, and server orchestrates TTL-windowed reads with conditional revalidation while recording structured cache-status telemetry. ChangesPackument Conditional-GET & Cache Revalidation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Store
participant Upstream
participant RemoteHTTP
Client->>Server: request package.json
Server->>Store: read_cached_packument_entry(name, ttl)
alt fresh
Store-->>Server: CachedPackument (fresh)
Server-->>Client: cached bytes (cache=hit)
else stale or missing
Store-->>Server: CachedPackument (stale) or None
Server->>Upstream: fetch_packument(name, validators)
Upstream->>RemoteHTTP: GET (If-None-Match / If-Modified-Since when present)
alt 304
RemoteHTTP-->>Upstream: 304
Upstream-->>Server: NotModified
Server->>Store: refresh mtime
Server-->>Client: cached bytes (cache=revalidated)
else 200
RemoteHTTP-->>Upstream: 200 + body + validators
Upstream-->>Server: Modified(bytes, validators)
Server->>Store: write_packument_with_meta(bytes, validators)
Server-->>Client: new bytes (cache=miss)
else 404 / Error
RemoteHTTP-->>Upstream: 404 / Error
Upstream-->>Server: NotFound / Error
Server-->>Client: stale bytes if available (cache=stale) or error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12239 +/- ##
==========================================
+ Coverage 87.34% 87.37% +0.02%
==========================================
Files 276 278 +2
Lines 32196 32596 +400
==========================================
+ Hits 28123 28482 +359
- Misses 4073 4114 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review Summary by QodoImplement conditional GET revalidation for upstream packuments
WalkthroughsDescription• Implements conditional GET revalidation for stale cached packuments - Stores ETag/Last-Modified validators in .package.json.meta sidecar - Replays validators as If-None-Match/If-Modified-Since on refresh - 304 Not Modified serves cached copy without re-downloading body • Adds cache status tracking to access logs (hit/revalidated/miss/stale/hosted) • Refactors packument fetch to return fresh body or 304 outcome • Adds comprehensive tests for conditional GET behavior Diagramflowchart LR
A["Stale Cached<br/>Packument"] -->|read validators| B["CacheValidators<br/>ETag/Last-Modified"]
B -->|If-None-Match<br/>If-Modified-Since| C["Upstream<br/>Fetch"]
C -->|304 Not Modified| D["Reuse Cached<br/>Body + Refresh"]
C -->|200 Modified| E["Store New Body<br/>+ Validators"]
D -->|cache=revalidated| F["Access Log"]
E -->|cache=miss| F
A -->|fresh| G["Serve from Cache<br/>cache=hit"]
G --> F
File Changes1. pnpr/crates/pnpr/src/upstream.rs
|
38 tasks
Contributor
Integrated-Benchmark Report (Linux)Each scenario has pacquet rows (direct install) and pnpr rows (the same client through the pnpr install accelerator), so pnpr@HEAD vs pacquet@HEAD is the pnpr-vs-direct ratio. Cold-store scenarios wipe the client store between runs (warm server); hot-store scenarios keep it warm. The pacquet@HEAD rows feed the pacquet Bencher testbed; the pnpr@HEAD rows feed the pnpr testbed. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 10.0939968938,
"stddev": 0.20504580752473683,
"median": 10.0506997996,
"user": 3.2626862999999995,
"system": 4.246959939999999,
"min": 9.876391997099999,
"max": 10.4374585831,
"times": [
9.9768976151,
9.9224741391,
10.4374585831,
10.1245019841,
9.876391997099999,
9.881622414099999,
10.3258634731,
9.9269888661,
10.1943923391,
10.2733775271
]
},
{
"command": "pacquet@main",
"mean": 9.9909903372,
"stddev": 0.16752239066652133,
"median": 9.8822991971,
"user": 3.3019527999999996,
"system": 4.24258714,
"min": 9.8746028061,
"max": 10.335697977099999,
"times": [
9.9815357441,
9.8804970661,
10.1902008761,
10.128006620099999,
10.335697977099999,
9.8746028061,
9.8826171041,
9.8791751431,
9.8755887451,
9.881981290099999
]
},
{
"command": "pnpr@HEAD",
"mean": 5.0609556717999995,
"stddev": 0.08974670864559361,
"median": 5.0403748606,
"user": 2.4271972999999996,
"system": 3.88995054,
"min": 4.9664666981,
"max": 5.2601244471,
"times": [
5.102817454099999,
5.2601244471,
5.0388732660999995,
5.1516292681,
4.9751283781,
5.0418764551,
5.0533171311,
5.0234699061,
4.9958537141,
4.9664666981
]
},
{
"command": "pnpr@main",
"mean": 5.052467889599999,
"stddev": 0.1050384788483106,
"median": 5.014817530099999,
"user": 2.4402972,
"system": 3.8807013400000003,
"min": 4.960152572099999,
"max": 5.2501638460999995,
"times": [
5.023438575099999,
4.9878451461,
4.960152572099999,
5.0405206231,
4.9964784951,
5.2501638460999995,
5.244481524099999,
5.010918194099999,
4.991963054099999,
5.018716866099999
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6714688661000001,
"stddev": 0.023156336719637544,
"median": 0.6646456632000001,
"user": 0.36658379999999996,
"system": 1.3127986799999998,
"min": 0.6533732252000001,
"max": 0.7327602182,
"times": [
0.7327602182,
0.6822583532000001,
0.6676973702000001,
0.6683135022000001,
0.6725102752000001,
0.6597379872000001,
0.6533732252000001,
0.6555675552000001,
0.6615939562,
0.6608762182000001
]
},
{
"command": "pacquet@main",
"mean": 0.6908418922000001,
"stddev": 0.04010765999514543,
"median": 0.6663350482000001,
"user": 0.3670534,
"system": 1.31369618,
"min": 0.6567979852000001,
"max": 0.7643255682000001,
"times": [
0.7643255682000001,
0.6670782942000001,
0.7522792002,
0.7085304322000001,
0.6623490072000001,
0.6655918022000001,
0.6610544322,
0.7060197592,
0.6643924412000001,
0.6567979852000001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7989400265000001,
"stddev": 0.08331181681710391,
"median": 0.7668319002000001,
"user": 0.3744047999999999,
"system": 1.3300436799999997,
"min": 0.7424301872000001,
"max": 1.0066895312,
"times": [
0.8853320632000001,
0.7814388092000001,
0.7569020002000001,
0.7424301872000001,
1.0066895312,
0.7676896312000001,
0.7659741692,
0.7560969502000001,
0.7545995852,
0.7722473382000001
]
},
{
"command": "pnpr@main",
"mean": 0.7792069171,
"stddev": 0.046007559478662995,
"median": 0.7601490217,
"user": 0.38665469999999996,
"system": 1.3035959800000003,
"min": 0.7435374272,
"max": 0.8927505262000001,
"times": [
0.8145984792000001,
0.7490130942000001,
0.8927505262000001,
0.7541954332,
0.7661026102,
0.7521771112000001,
0.7435374272,
0.7895560982000001,
0.7836002912000001,
0.7465381002000001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 5.25592441492,
"stddev": 0.043409157875396225,
"median": 5.240060929119999,
"user": 3.59791052,
"system": 3.2213927399999998,
"min": 5.20785168912,
"max": 5.32664571912,
"times": [
5.31072788712,
5.247947573119999,
5.229200888119999,
5.247396161119999,
5.229145926119999,
5.32664571912,
5.31077692412,
5.20785168912,
5.232725697119999,
5.21682568412
]
},
{
"command": "pacquet@main",
"mean": 5.254792150719998,
"stddev": 0.043379024238715876,
"median": 5.25329774812,
"user": 3.60639002,
"system": 3.22406934,
"min": 5.18325277712,
"max": 5.32977061012,
"times": [
5.23606588612,
5.28395904912,
5.23434951512,
5.273704836119999,
5.27052961012,
5.292671557119999,
5.32977061012,
5.2352386291199995,
5.18325277712,
5.208379037119999
]
},
{
"command": "pnpr@HEAD",
"mean": 2.0019332417199998,
"stddev": 0.05874598101176281,
"median": 1.97329427762,
"user": 2.41867902,
"system": 3.17939334,
"min": 1.94665134112,
"max": 2.10007392812,
"times": [
2.05651336112,
2.05751110912,
1.98752102012,
1.9558454941199999,
1.95118672612,
2.05520272412,
1.94665134112,
1.95906753512,
2.10007392812,
1.94975917812
]
},
{
"command": "pnpr@main",
"mean": 1.9981708625199999,
"stddev": 0.05410938674875766,
"median": 1.98031229412,
"user": 2.40749752,
"system": 3.1727773399999997,
"min": 1.93672157712,
"max": 2.09734670412,
"times": [
1.98625878912,
2.09734670412,
2.05308041312,
1.9445173781199998,
1.93672157712,
1.97381724512,
1.94669344612,
2.02304457812,
2.0458626951200003,
1.9743657991199999
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.37056923412,
"stddev": 0.020175518026219513,
"median": 1.3660984962200002,
"user": 1.5332226199999999,
"system": 1.7396533599999997,
"min": 1.3484461917200001,
"max": 1.41527649772,
"times": [
1.41527649772,
1.3545527907200001,
1.3620408747200001,
1.3484461917200001,
1.3776907927200002,
1.37384057972,
1.35219254772,
1.36172824372,
1.3701561177200001,
1.38976770472
]
},
{
"command": "pacquet@main",
"mean": 1.4245514300200002,
"stddev": 0.03531697612699733,
"median": 1.41728502122,
"user": 1.5417224200000001,
"system": 1.8043043600000002,
"min": 1.3799201007200002,
"max": 1.51196666472,
"times": [
1.4070436377200002,
1.4307276017200001,
1.51196666472,
1.4197264497200002,
1.41484359272,
1.42556267472,
1.3799201007200002,
1.4097158257200002,
1.4442656067200002,
1.40174214572
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6565640440200001,
"stddev": 0.0209297395146453,
"median": 0.64964635422,
"user": 0.32603622000000004,
"system": 1.2477897599999999,
"min": 0.6383473407200001,
"max": 0.7100212317200001,
"times": [
0.64661434372,
0.64589144472,
0.6637525187200001,
0.6416806077200001,
0.7100212317200001,
0.6526783647200001,
0.6383473407200001,
0.6663851417200001,
0.65616175472,
0.64410769172
]
},
{
"command": "pnpr@main",
"mean": 0.73100435122,
"stddev": 0.010926373896824708,
"median": 0.7259716807200001,
"user": 0.33696182,
"system": 1.3135592600000001,
"min": 0.7195158327200001,
"max": 0.7480697177200001,
"times": [
0.7195158327200001,
0.7257405977200001,
0.7435815457200001,
0.72620276372,
0.7216039057200001,
0.7480697177200001,
0.7458998337200001,
0.7333346167200001,
0.7231113387200001,
0.72298335972
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot storeResolution-only: cold packument cache (full re-resolve over the registry link) with a hot store (no tarball download), so this isolates pnpr offloading the client resolution to its warm server.
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.97250260854,
"stddev": 0.014581819058869681,
"median": 4.97050755464,
"user": 1.7361294,
"system": 1.88413258,
"min": 4.95016118764,
"max": 4.99912449564,
"times": [
4.99912449564,
4.98163370264,
4.95559623064,
4.98154042364,
4.95016118764,
4.97297829564,
4.96768475964,
4.963981142640001,
4.96803681364,
4.9842890336400005
]
},
{
"command": "pacquet@main",
"mean": 4.988469972740001,
"stddev": 0.028688315483625708,
"median": 4.9889859226399995,
"user": 1.7276001,
"system": 1.8876499800000002,
"min": 4.9397522596400005,
"max": 5.04359749764,
"times": [
5.01148976964,
4.983747307640001,
4.99451413364,
4.98439636264,
4.95825608964,
5.04359749764,
4.99357548264,
4.97269877464,
4.9397522596400005,
5.00267204964
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6575653926399999,
"stddev": 0.009323241535189803,
"median": 0.65307168014,
"user": 0.3216503,
"system": 1.2603356800000003,
"min": 0.64834957664,
"max": 0.67401893664,
"times": [
0.66061637064,
0.65141586364,
0.6683623266400001,
0.65071642064,
0.64834957664,
0.64921329464,
0.6672845216400001,
0.67401893664,
0.65094911864,
0.6547274966400001
]
},
{
"command": "pnpr@main",
"mean": 0.7007291927400001,
"stddev": 0.08090975656563083,
"median": 0.67647637214,
"user": 0.33121029999999996,
"system": 1.27631588,
"min": 0.66633091164,
"max": 0.9299189236400001,
"times": [
0.67724267764,
0.6683688886400001,
0.66698454464,
0.66633091164,
0.6705277026400001,
0.6828309276400001,
0.6777682436400001,
0.6916090406400001,
0.67571006664,
0.9299189236400001
]
}
]
} |
Contributor
Bencher Report
Contributor
|
| Branch | pr/12239 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,307.89 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 1,481.50 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,099.64 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 700.48 ms |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds HTTP conditional revalidation for stale upstream packuments in pnpr’s proxy cache (ETag / Last-Modified → If-None-Match / If-Modified-Since), so stale-but-unchanged entries can refresh via 304 Not Modified without re-downloading the body, and access logs can indicate how each packument request was served.
Changes:
- Implement conditional GET support in upstream packument fetching, including capturing and replaying validators and handling
304/404. - Persist upstream validators alongside cached packuments in a
.package.json.metasidecar and plumb “fresh vs stale” cache reads. - Revalidate stale cached packuments in the server layer and record
cache=...status; add unit/integration coverage for the lifecycle.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpr/crates/pnpr/tests/server.rs | Adds integration coverage for stale → revalidate(304) → hit behavior. |
| pnpr/crates/pnpr/src/upstream/tests.rs | Adds unit tests for capturing/replaying validators and mapping 304/404. |
| pnpr/crates/pnpr/src/upstream.rs | Implements conditional packument fetch API and validator extraction. |
| pnpr/crates/pnpr/src/storage.rs | Stores/loads validator sidecar and returns cached bytes + freshness + validators. |
| pnpr/crates/pnpr/src/server.rs | Revalidates stale entries, falls back to stale on upstream failure, records cache= access field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add storage-layer unit tests for the conditional-GET cache: validator sidecar round-trip, sidecar removal when validators are empty, malformed sidecar degrading to empty validators, and TTL freshness. - Only treat an upstream 304 as NotModified when a conditional header was actually sent; an unconditional 304 (e.g. cold cache) now falls through to a status error instead of being mistaken for a usable cached copy. - Move packument bytes out on the cache-hit path instead of cloning the (potentially multi-MB) document on every request. - Un-ignore pnpr's `src/storage/` source dir: the monorepo-wide `storage` gitignore rule (for Verdaccio runtime data) was swallowing the new test file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pnpr/crates/pnpr/src/storage/tests.rs (4)
95-109: ⚡ Quick winConsider adding an assertion message to clarify the degradation behavior.
The assertion on line 108 could benefit from a message that explains the graceful degradation.
📝 Suggested improvement
- assert!(entry.validators.is_empty()); + assert!(entry.validators.is_empty(), "malformed sidecar should degrade to empty validators");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/storage/tests.rs` around lines 95 - 109, Add a clear assertion message to the test function malformed_sidecar_reads_as_empty_validators to document the expected graceful degradation: when the sidecar file is corrupted (written via sidecar_path and initial packument written with storage.write_cached_packument), reading via storage.read_cached_packument_entry should succeed and entry.validators.is_empty() must be true; update the assert! call to include a message like "damaged sidecar should degrade to empty validators forcing refresh" so future failures make the intent explicit.
79-93: ⚡ Quick winConsider adding assertion messages for clarity.
The assertions on lines 88 and 92 could benefit from descriptive messages.
📝 Suggested improvement
- assert!(!sidecar_path(&tmp, "foo").exists()); + assert!(!sidecar_path(&tmp, "foo").exists(), "no sidecar should exist when none was written"); let entry = storage.read_cached_packument_entry(&name, Duration::from_secs(60)).await.unwrap().unwrap(); - assert!(entry.validators.is_empty()); + assert!(entry.validators.is_empty(), "validators should be empty when no sidecar exists");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/storage/tests.rs` around lines 79 - 93, In the test function writing_without_validators_is_a_noop_when_no_sidecar_exists, update the two assertions to include descriptive failure messages: change the assert that checks !sidecar_path(&tmp, "foo").exists() to include a message like "expected no sidecar file to be created for 'foo' but one was found", and change the assert that checks entry.validators.is_empty() to include a message like "expected validators to be empty after writing without validators"; locate these assertions where sidecar_path(...) and entry.validators are used and add the messages as the second argument to assert! so failures are clearer.
51-58: ⚡ Quick winConsider adding an assertion message for clarity.
The assertion on line 57 could benefit from a descriptive message to make test failures clearer.
📝 Suggested improvement
- assert!(entry.is_none()); + assert!(entry.is_none(), "absent packument should read as None");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/storage/tests.rs` around lines 51 - 58, Add a descriptive assertion message to the test missing_cached_packument_reads_as_none so failures are clearer: update the assert!(entry.is_none()) to include a message (or include the actual entry via debug formatting) that states the expectation (e.g., "expected no cached packument entry, got: {:?}", entry) when calling storage.read_cached_packument_entry(&pkg("absent"), Duration::from_secs(60)) in the test function.
60-77: ⚡ Quick winConsider adding an assertion message for the final check.
The assertion on line 76 could benefit from a message to clarify what's being verified.
📝 Suggested improvement
- assert!(entry.validators.is_empty()); + assert!(entry.validators.is_empty(), "read-back validators should be empty after sidecar removal");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/storage/tests.rs` around lines 60 - 77, The final assert in the test empty_validators_remove_a_previously_written_sidecar lacks an assertion message; update the assertion checking entry.validators.is_empty() (from the read_cached_packument_entry result) to include a descriptive message (e.g., "expected validators to be empty after writing CacheValidators::default() and removing sidecar") so failures clearly indicate what the test verifies; locate this in the test function and change the assert! call that references entry.validators.is_empty().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pnpr/crates/pnpr/src/storage/tests.rs`:
- Around line 95-109: Add a clear assertion message to the test function
malformed_sidecar_reads_as_empty_validators to document the expected graceful
degradation: when the sidecar file is corrupted (written via sidecar_path and
initial packument written with storage.write_cached_packument), reading via
storage.read_cached_packument_entry should succeed and
entry.validators.is_empty() must be true; update the assert! call to include a
message like "damaged sidecar should degrade to empty validators forcing
refresh" so future failures make the intent explicit.
- Around line 79-93: In the test function
writing_without_validators_is_a_noop_when_no_sidecar_exists, update the two
assertions to include descriptive failure messages: change the assert that
checks !sidecar_path(&tmp, "foo").exists() to include a message like "expected
no sidecar file to be created for 'foo' but one was found", and change the
assert that checks entry.validators.is_empty() to include a message like
"expected validators to be empty after writing without validators"; locate these
assertions where sidecar_path(...) and entry.validators are used and add the
messages as the second argument to assert! so failures are clearer.
- Around line 51-58: Add a descriptive assertion message to the test
missing_cached_packument_reads_as_none so failures are clearer: update the
assert!(entry.is_none()) to include a message (or include the actual entry via
debug formatting) that states the expectation (e.g., "expected no cached
packument entry, got: {:?}", entry) when calling
storage.read_cached_packument_entry(&pkg("absent"), Duration::from_secs(60)) in
the test function.
- Around line 60-77: The final assert in the test
empty_validators_remove_a_previously_written_sidecar lacks an assertion message;
update the assertion checking entry.validators.is_empty() (from the
read_cached_packument_entry result) to include a descriptive message (e.g.,
"expected validators to be empty after writing CacheValidators::default() and
removing sidecar") so failures clearly indicate what the test verifies; locate
this in the test function and change the assert! call that references
entry.validators.is_empty().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e2d19d3-9240-4882-8120-8c37814a34a2
📒 Files selected for processing (6)
.gitignorepnpr/crates/pnpr/src/server.rspnpr/crates/pnpr/src/storage.rspnpr/crates/pnpr/src/storage/tests.rspnpr/crates/pnpr/src/upstream.rspnpr/crates/pnpr/src/upstream/tests.rs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- pnpr/crates/pnpr/src/upstream/tests.rs
- pnpr/crates/pnpr/src/storage.rs
- pnpr/crates/pnpr/src/server.rs
- pnpr/crates/pnpr/src/upstream.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pnpr/**/pnpr/**/*.rs
📄 CodeRabbit inference engine (pnpr/AGENTS.md)
pnpr/**/pnpr/**/*.rs: Follow the pacquet code-style guide (../pacquet/CODE_STYLE_GUIDE.md) for Rust-level conventions including imports, naming, ownership, and error handling
Follow the pacquet contributing guide (../pacquet/CONTRIBUTING.md) for test layout and Rust conventions
Files:
pnpr/crates/pnpr/src/storage/tests.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:05.107Z
Learning: In pnpm/pnpm's pnpr install accelerator, the `/v1/install` response has a two-level framing structure:
1. **Outer layer** (full HTTP body): `[u32 outer header length][outer header JSON][files payload]` — `fetchFromPnpmRegistry` (pnpr/client/src/fetchFromPnpmRegistry.ts) strips the outer layer with `body.subarray(4 + headerLength)` and passes the remaining bytes to `writeCafsFiles`.
2. **Inner layer** (files payload): the files payload itself starts with its own `[u32 inner json length][inner header JSON]` prefix (built by the server's `build_files_payload` / `empty_files_payload_prefix`), followed by `[64-byte digest][u32 size][1-byte exec][content]` frames and a 64-zero-byte end marker.
`writeCafsFiles` in `worker/src/start.ts` is correct to read `jsonLen = payload.readUInt32BE(0)` and start frames at `offset = 4 + jsonLen` — this skips the inner header. The same two-level structure is mirrored in the Rust reference client (`parse_inline_response` + `write_files_payload`). Do not fla...
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Use snapshot tests with `insta` and carefully review diffs when intentional changes alter snapshots; accept with `cargo insta review` only after careful review
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Reference the upstream pnpm commit/PR when porting code from pnpm in commit messages
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Tests that need the mocked registry should start `pnpr` through `pacquet-testing-utils`; `cargo test` / `cargo nextest run` should not require a separate `just registry-mock launch` step
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/*.rs : Tests are documentation — do not duplicate test scenarios, edge cases, failure modes, or worked examples in prose when they are already captured by tests
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Prefer real fixtures over dependency-injection seams — use `tempfile::TempDir`, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Use snapshot tests with `insta` and carefully review diffs when intentional changes alter snapshots; accept with `cargo insta review` only after careful review
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:24.797Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.797Z
Learning: Applies to pnpr/**/pnpr/**/*.rs : Follow the pacquet contributing guide (../pacquet/CONTRIBUTING.md) for test layout and Rust conventions
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Follow test-logging guidance — log before non-`assert_eq!` assertions, `dbg!` complex structures, skip logging for simple scalar `assert_eq!`
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-05-23T16:55:36.507Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: pacquet/crates/cli/tests/lockfile_verification.rs:158-162
Timestamp: 2026-05-23T16:55:36.507Z
Learning: In `pacquet/crates/cli/tests/lockfile_verification.rs`, the `trust_lockfile_skips_verification` and `trust_lockfile_cli_flag_skips_verification` tests intentionally do NOT assert `output.status.success()`. The hand-rolled fixture lockfile uses a placeholder integrity hash (`sha512-AAA…`), so the install always fails the downstream tarball integrity check regardless of the supply-chain gate. The contract being tested is "gate-skipped, not install-succeeded"; asserting success would require generating a real lockfile via the `generate_lockfile` pattern (see `hoist.rs`) which is considered not worth the extra wiring for an opt-out smoke test.
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
📚 Learning: 2026-06-04T20:24:32.096Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12198
File: pnpr/crates/pnpr/src/storage.rs:469-477
Timestamp: 2026-06-04T20:24:32.096Z
Learning: In `pnpr/crates/pnpr/src/storage.rs` (pnpm/pnpm repo, Rust), `Store::list_package_names` intentionally uses `fs::try_exists(...).await.unwrap_or(false)` and `if let Ok(mut inner) = fs::read_dir(...)` — NOT `?`-propagation — for per-entry checks. This is deliberate best-effort / verdaccio-style search behavior: (1) `try_exists(stray_file/package.json)` returns `ENOTDIR` (not `NotFound`) for a stray non-package file in the store root, so `?` would fail the entire search; (2) the `@`-scope `read_dir` would fail on a non-directory `@`-named entry; (3) switching to `DirEntry::file_type()` would stop following symlinked package dirs. Failures that DO propagate are preserved: opening the store root itself, and `next_entry()` during the walk. Do not suggest blanket `?`-propagation for these per-entry checks.
Applied to files:
pnpr/crates/pnpr/src/storage/tests.rs
🔇 Additional comments (4)
pnpr/crates/pnpr/src/storage/tests.rs (4)
1-22: LGTM!
24-49: LGTM!
111-129: LGTM!
131-144: LGTM!
- Add storage-layer unit tests for the conditional-GET cache: validator sidecar round-trip, sidecar removal when validators are empty, malformed sidecar degrading to empty validators, and TTL freshness. - Only treat an upstream 304 as NotModified when a conditional header was actually sent; an unconditional 304 (e.g. cold cache) now falls through to a status error instead of being mistaken for a usable cached copy. - Move packument bytes out on the cache-hit path instead of cloning the (potentially multi-MB) document on every request. - Un-ignore pnpr's `src/storage/` source dir: the monorepo-wide `storage` gitignore rule (for Verdaccio runtime data) was swallowing the new test file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… status - Read a stale cached packument's body lazily: `read_cached_packument_entry` now returns `Fresh(bytes)` or `Stale(validators)`, so the common stale->200 refresh no longer reads and allocates the (potentially multi-MB) old body. It's pulled on demand only on the 304 / upstream-error paths that actually need it. - Record `cache=orphaned` (not `hit`) when serving a leftover mirror with no upstream left to revalidate against, since that path serves regardless of the TTL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… status - Read a stale cached packument's body lazily: `read_cached_packument_entry` now returns `Fresh(bytes)` or `Stale(validators)`, so the common stale->200 refresh no longer reads and allocates the (potentially multi-MB) old body. It's pulled on demand only on the 304 / upstream-error paths that actually need it. - Record `cache=orphaned` (not `hit`) when serving a leftover mirror with no upstream left to revalidate against, since that path serves regardless of the TTL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… status - Read a stale cached packument's body lazily: `read_cached_packument_entry` now returns `Fresh(bytes)` or `Stale(validators)`, so the common stale->200 refresh no longer reads and allocates the (potentially multi-MB) old body. It's pulled on demand only on the 304 / upstream-error paths that actually need it. - Record `cache=orphaned` (not `hit`) when serving a leftover mirror with no upstream left to revalidate against, since that path serves regardless of the TTL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add storage-layer unit tests for the conditional-GET cache: validator sidecar round-trip, sidecar removal when validators are empty, malformed sidecar degrading to empty validators, and TTL freshness. - Only treat an upstream 304 as NotModified when a conditional header was actually sent; an unconditional 304 (e.g. cold cache) now falls through to a status error instead of being mistaken for a usable cached copy. - Move packument bytes out on the cache-hit path instead of cloning the (potentially multi-MB) document on every request. - Un-ignore pnpr's `src/storage/` source dir: the monorepo-wide `storage` gitignore rule (for Verdaccio runtime data) was swallowing the new test file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zkochan
approved these changes
Jun 7, 2026
This was referenced Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds conditional GET revalidation of cached upstream packuments to pnpr's proxy, mirroring verdaccio's uplink revalidation.
When a cached packument goes stale past
packument_ttl, pnpr no longer re-downloads the whole document. Instead it replays the upstream'sETag/Last-ModifiedasIf-None-Match/If-Modified-Since; a304 Not Modifiedlets pnpr serve its cached copy without transferring the body.Why
pnpr caches each package's packument (the metadata JSON listing every version) for
packument_ttl(default 5 min). Before this PR, the post-TTL refresh was an unconditional GET — pnpr re-downloaded the entire packument every time, even when nothing had changed upstream. Popular packages have multi-MB packuments, so this was wasted bandwidth on every refresh.Conditional GET turns the common "stale but unchanged" case into a bodyless
304, so a short TTL (fresh versions surface quickly) no longer costs a full re-download each cycle.How it works
200from the upstream, pnpr capturesETag/Last-Modifiedand persists them in a sidecar file,.package.json.meta, next to the cachedpackage.json(disposable proxy-cache store only — hosted packages have no upstream to revalidate against).packument_ttl, pnpr reads back only the validators (not the body) and issues the GET with them attached. A fresh entry, by contrast, is read with its body and served immediately.304 Not Modified→ read the stale body (deferred until now), re-write it to bump the cache mtime so the entry is fresh again for another TTL window, and serve it.200→ store the new body + new validators, serve them. The old stale body is never read.404→ not found.A conditional
304is only honored when a validator was actually sent; a304to an unconditional request (e.g. a cold cache) is treated as an upstream error rather than reused.Access-log
cache=fieldThe per-request access record now carries a
cache=field so operators can see how each packument read was served:hostedhitrevalidated304; cached body reused (network, no body)missstaleorphanedproxy:rule was removed after mirroring); served regardless of age, so distinct fromhitChanges
upstream.rs—fetch_packumentnow takes&CacheValidatorsand sends the conditional headers; newPackumentFetchenum (Modified { bytes, validators }/NotModified/NotFound); validators captured from response headers. A304is mapped toNotModifiedonly when a conditional header was actually sent.storage.rs— validators persisted in the.package.json.metasidecar;read_cached_packument_entryclassifies against the TTL (age <= ttl, from the cached file's mtime) and returns aCachedPackumentenum:Fresh(bytes)(body read, ready to serve) orStale(validators)(only the small validators read — the body is left on disk and pulled on demand).write_cached_packumentwrites/clears the sidecar atomically.server.rs—load_packument_bytesserves a fresh entry directly, revalidates a stale one, handles304/200/404/error (reading the stale body only on the304/error paths), and tags the access span viarecord_cache_status.304, unconditional-304-is-error,404); an integration test (stale_packument_is_revalidated_with_conditional_get) covering the miss → revalidate(304) → hit lifecycle.Design notes
304/upstream-error paths, so the common stale→200refresh (and every refresh against an upstream that doesn't emit validators) never reads or allocates the discarded body.304. pnpr re-writes the unchanged bytes to refresh the mtime rather than adding afiletime-style dependency to touch the file in place. Packuments are modest and it avoids a new dependency — happy to switch to a pure mtime bump if preferred.Scope
If-Modified-Since) item.Verification
cargo fmt --all -- --checkcargo clippy --locked --workspace --all-targets -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc --no-deps --workspace --document-private-itemscargo dylint --all -- --all-targets --workspacecargo nextest run -p pnpr— 284 passedWritten by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Release Notes
New Features
Tests