NarInfoDiskCache: Support arbitrary nix-cache-info fields#525
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughCache metadata now flows as string maps. The store parses ChangesBinary cache fields migration
Sequence Diagram(s)sequenceDiagram
participant HttpBinaryCacheStore
participant NarInfoDiskCache
participant BinaryCacheStore
HttpBinaryCacheStore->>NarInfoDiskCache: upToDateCacheExists(cacheKey)
alt cacheInfo present
NarInfoDiskCache-->>HttpBinaryCacheStore: cacheInfo.fields
HttpBinaryCacheStore->>BinaryCacheStore: applyCacheInfoFields(cacheInfo.fields)
else cacheInfo missing
HttpBinaryCacheStore->>BinaryCacheStore: parseNixCacheInfo()
BinaryCacheStore-->>HttpBinaryCacheStore: fields map
HttpBinaryCacheStore->>BinaryCacheStore: applyCacheInfoFields(fields)
HttpBinaryCacheStore->>NarInfoDiskCache: createCache(cacheKey, {.fields = fields})
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
edolstra
force-pushed
the
nar-info-disk-cache-generic-fields
branch
from
e1d8e51 to
156eb3a
Compare
cole-h
previously approved these changes
Jun 26, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/libstore/nar-info-disk-cache.cc (1)
194-194: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider wrapping the
fieldsJSON parse to add context on corruption.
nlohmann::json::parse(getStr(2))throws on a malformed/legacy row, propagating an opaque error. Other readers here (e.g.lookupRealisation) wrap parsing in atry/catchthat adds"while parsing the local disk cache". Mirroring that would improve diagnosability, though the v2 filename bump makes legacy rows unlikely.🤖 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 `@src/libstore/nar-info-disk-cache.cc` at line 194, Wrap the `fields` parsing in `nar-info-disk-cache.cc` with the same kind of `try/catch` used by `lookupRealisation` so malformed or legacy cache rows fail with added context. Update the `queryCache.getStr(2)` JSON parse in the `fields` initializer to rethrow with a message like “while parsing the local disk cache” and keep the surrounding row handling consistent with the other cache readers.src/libstore-tests/nar-info-disk-cache.cc (1)
120-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale commented-out block references the removed API.
This dead block uses the old positional
createCache(..., wantMassQuery, prio)signature andr->wantMassQuery/r->priority, which no longer exist after this migration. Consider removing it or updating it to the.fieldsshape to avoid confusion.🤖 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 `@src/libstore-tests/nar-info-disk-cache.cc` around lines 120 - 126, Remove or update the stale commented-out test block in NarInfo disk cache tests so it matches the current cache API. The dead code still uses the old positional createCache signature and the removed wantMassQuery/priority accessors; either delete this block entirely or rewrite it to use the new fields-based shape, keeping the references aligned with createCache and upToDateCacheExists.
🤖 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.
Inline comments:
In `@src/libstore/binary-cache-store.cc`:
- Around line 81-87: The Priority parsing in
BinaryCacheStore::applyCacheInfoFields currently uses std::stoi, which can throw
on malformed or out-of-range remote nix-cache-info data. Replace that conversion
with string2Int, and only call config.priority.setDefault when the value parses
successfully; otherwise skip the field and keep initialization non-throwing.
Keep the existing WantMassQuery handling unchanged.
In `@src/libstore/include/nix/store/binary-cache-store.hh`:
- Around line 101-113: Update the doc comments for parseNixCacheInfo() and
applyCacheInfoFields() so they match the behavior in binary-cache-store.cc:
parseNixCacheInfo() should be described as parsing nix-cache-info and returning
known plus non-standard fields without applying them, while
applyCacheInfoFields() should describe applying the supported fields actually
handled there. Also remove the GetNarInfosV1 mention from these comments and
align the “known fields” wording with the current implementation.
---
Nitpick comments:
In `@src/libstore-tests/nar-info-disk-cache.cc`:
- Around line 120-126: Remove or update the stale commented-out test block in
NarInfo disk cache tests so it matches the current cache API. The dead code
still uses the old positional createCache signature and the removed
wantMassQuery/priority accessors; either delete this block entirely or rewrite
it to use the new fields-based shape, keeping the references aligned with
createCache and upToDateCacheExists.
In `@src/libstore/nar-info-disk-cache.cc`:
- Line 194: Wrap the `fields` parsing in `nar-info-disk-cache.cc` with the same
kind of `try/catch` used by `lookupRealisation` so malformed or legacy cache
rows fail with added context. Update the `queryCache.getStr(2)` JSON parse in
the `fields` initializer to rethrow with a message like “while parsing the local
disk cache” and keep the surrounding row handling consistent with the other
cache readers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df4a3231-1b4f-4246-85a4-a4eae26a4439
📒 Files selected for processing (6)
src/libstore-tests/nar-info-disk-cache.ccsrc/libstore/binary-cache-store.ccsrc/libstore/http-binary-cache-store.ccsrc/libstore/include/nix/store/binary-cache-store.hhsrc/libstore/include/nix/store/nar-info-disk-cache.hhsrc/libstore/nar-info-disk-cache.cc
This replaces the WantMassQuery and Priority SQLite columns with a generic JSON field storing arbitrary nix-cache-info fields. This allows fields to be added in the future without having to change the database schema. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
edolstra
force-pushed
the
nar-info-disk-cache-generic-fields
branch
from
156eb3a to
6108e4d
Compare
cole-h
approved these changes
Jun 26, 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
2 participants
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.
Motivation
This replaces the
WantMassQueryandPrioritySQLite columns with a generic JSON field storing arbitrarynix-cache-infofields. This allows fields to be added in the future without having to change the database schema.Extracted from #523. This will also be useful for #488 which adds some
nix-cache-infofields as well.Context
Summary by CodeRabbit
New Features
nix-cache-infofields, improving compatibility with caches that provide extra information.Bug Fixes
Chores