Store provenance info in the NAR info disk cache#344
Conversation
📝 WalkthroughWalkthroughAdds provenance storage and plumbing to the NAR info disk cache: schema bumped to binary-cache-v8.sqlite with a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant NarInfoDiskCache
participant ExperimentalFeatureToggle
participant SQLiteDB
Client->>NarInfoDiskCache: request upsert or lookup (path info)
NarInfoDiskCache->>ExperimentalFeatureToggle: isEnabled(Xp::Provenance)?
ExperimentalFeatureToggle-->>NarInfoDiskCache: true/false
alt Provenance enabled
NarInfoDiskCache->>SQLiteDB: INSERT/SELECT ... provenance (JSON)
SQLiteDB-->>NarInfoDiskCache: row with provenance (or ack)
NarInfoDiskCache-->>Client: NarInfo with provenance populated
else Provenance disabled
NarInfoDiskCache->>SQLiteDB: INSERT/SELECT without provenance
SQLiteDB-->>NarInfoDiskCache: row (no provenance)
NarInfoDiskCache-->>Client: NarInfo without provenance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
3b7a748 to
ac1a7b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/libstore/nar-info-disk-cache.cc`:
- Line 284: The code calls queryNAR.getStr(12) unguarded when assigning
narInfo->provenance via Provenance::from_json_str_optional, which can throw if
the column is NULL; change this to first check queryNAR.isNull(12) and only call
Provenance::from_json_str_optional(queryNAR.getStr(12)) when not null, otherwise
set narInfo->provenance to nullptr/optional empty value. Update the assignment
site where narInfo and provenance are used to handle the empty/null case
accordingly.
- Line 40: The DB schema change added a new column named provenance but existing
DBs created with the prior schema won’t get that column (CREATE TABLE IF NOT
EXISTS won’t alter existing tables), causing the SELECT ... provenance prepare
to fail; update the open/init path in nar-info-disk-cache.cc to detect and
migrate old DBs by either checking PRAGMA table_info(<table>) for the provenance
column or using PRAGMA user_version, and if missing run a lightweight ALTER
TABLE <table> ADD COLUMN provenance TEXT (or an equivalent migration SQL)
immediately after the schema exec so subsequent calls that SELECT provenance
succeed. Ensure the migration runs once on open before any prepares that
reference provenance and log or handle errors from the ALTER/MIGRATION step.
🧹 Nitpick comments (1)
tests/functional/flakes/provenance.sh (1)
90-95: Restore any pre-existing_NIX_FORCE_HTTPvalue to avoid cross-test leakage.The script unsets the variable unconditionally, which can clobber a pre-set value in the test environment. Save and restore instead.
♻️ Suggested tweak
-export _NIX_FORCE_HTTP=1 # force use of the NAR info disk cache +old_force_http="${_NIX_FORCE_HTTP-__unset__}" +export _NIX_FORCE_HTTP=1 # force use of the NAR info disk cache ... -unset _NIX_FORCE_HTTP +if [[ "$old_force_http" == "__unset__" ]]; then + unset _NIX_FORCE_HTTP +else + export _NIX_FORCE_HTTP="$old_force_http" +fiAlso applies to: 131-131
ac1a7b5 to
24f9e35
Compare
24f9e35 to
9a41f11
Compare
Motivation
Note: this bumps the cache version to
binary-cache-v8.sqlite.Context
Summary by CodeRabbit
New Features
Tests