Skip to content

various: publish cached closure narinfos in cache loop#127

Merged
NotAShelf merged 2 commits into
mainfrom
cache-closure-narinfos
Jul 5, 2026
Merged

various: publish cached closure narinfos in cache loop#127
NotAShelf merged 2 commits into
mainfrom
cache-closure-narinfos

Conversation

@amaanq

@amaanq amaanq commented Jun 24, 2026

Copy link
Copy Markdown
Member

Cached builds that reused an existing output only published narinfos for the primary output, so clients could miss dependencies from the project cache.

Publish signed narinfos for the whole closure, keep project ownership separate for shared store paths, and move recursive lookup and signing out of the scheduler loop.

@amaanq amaanq force-pushed the cache-closure-narinfos branch 2 times, most recently from 558242d to 9f713ce Compare June 24, 2026 20:39

@NotAShelf NotAShelf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Some of them are just a little bit speculative but I'd like to catch things before they explode.

Comment thread crates/common/src/repo/narinfo_cache.rs
Comment thread crates/server/src/routes/cache.rs
Comment thread crates/server/src/routes/cache.rs
Comment thread crates/queue-runner/src/runner_loop.rs
@amaanq amaanq force-pushed the cache-closure-narinfos branch 4 times, most recently from efdd4ba to c179dda Compare June 28, 2026 01:42
@amaanq amaanq requested a review from NotAShelf June 28, 2026 01:42
@NotAShelf

Copy link
Copy Markdown
Member

CI is still failing and there are file conflicts

@NotAShelf NotAShelf changed the title cache: publish cached closure narinfos various: publish cached closure narinfos in cache loop Jun 28, 2026
@amaanq amaanq force-pushed the cache-closure-narinfos branch from c179dda to 3384cd1 Compare July 2, 2026 20:40
Comment thread crates/queue-runner/src/worker.rs Outdated
Comment thread crates/queue-runner/src/runner_loop.rs
Comment thread crates/queue-runner/src/worker.rs Outdated
@amaanq amaanq force-pushed the cache-closure-narinfos branch from 3384cd1 to 91fc475 Compare July 3, 2026 20:11
@amaanq amaanq requested a review from NotAShelf July 3, 2026 20:12

@NotAShelf NotAShelf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two more comments. Neither of them are actual blockers but I'm leaving it to you to decide whether they're severe enough to address now or if we should note those down and address in a followup in the case you don't want to block this PR further.

Comment thread crates/server/src/routes/cache.rs Outdated
Comment thread crates/server/src/routes/cache.rs Outdated
@amaanq amaanq force-pushed the cache-closure-narinfos branch from 91fc475 to 729a154 Compare July 4, 2026 21:31
pieter0101 and others added 2 commits July 4, 2026 17:37
Cached builds that reused an existing output only published narinfos
for the primary output, so clients could miss dependencies from the
project cache.

Publish signed narinfos for the whole closure, keep project ownership
separate for shared store paths, and move recursive lookup and signing
out of the scheduler loop.

Co-authored-by: Amaan Qureshi <git@amaanq.com>
@amaanq amaanq force-pushed the cache-closure-narinfos branch from 729a154 to d2c919b Compare July 4, 2026 21:39
@amaanq amaanq requested a review from NotAShelf July 4, 2026 21:39
@NotAShelf NotAShelf added this pull request to the merge queue Jul 5, 2026
Merged via the queue into main with commit 0286c3c Jul 5, 2026
31 checks passed
@amaanq amaanq deleted the cache-closure-narinfos branch July 5, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants