Skip to content

fix nix run eval cache - #9

Merged
ethancedwards8 merged 1 commit into
mainfrom
devin/1786432831-persist-eval-cache-before-exec
Aug 11, 2026
Merged

ethancedwards8 merged 1 commit into
mainfrom
devin/1786432831-persist-eval-cache-before-exec

Conversation

@ryanjwong

@ryanjwong ryanjwong commented Aug 11, 2026 •

Copy link
Copy Markdown

fix nix run eval cache

Link to Devin session: https://app.devin.ai/sessions/36cf0993f74446ecab9a95becd1c5ee0
Requested by: @ryanjwong


Open in Devin Review

@ryanjwong ryanjwong self-assigned this Aug 11, 2026
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 1 additional finding in Devin Review.

Open in Devin Review

{
virtual ~Installable() {}

virtual void releaseEvalCache() {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 New shared interface method added without documentation in header

The newly added shared method for dropping an installable's cached evaluation data (virtual void releaseEvalCache() {} at src/libcmd/include/nix/cmd/installables.hh:133) carries no explanatory comment, unlike every other method in the same interface, so future implementers have no stated contract about when it must be called or what remains valid afterwards.
Impact: Contributors extending or overriding this interface can misuse it, since nothing states that it must only be used right before the process is replaced.

Undocumented virtual API on the Installable base class

CONTRIBUTING.md lists "API documentation in header files" as a requirement for changes. All neighbouring virtual members in src/libcmd/include/nix/cmd/installables.hh (e.g. what() at lines 133-142, toDerivedPaths() at 143-150) have doc comments. The override declaration in src/libcmd/include/nix/cmd/installable-flake.hh:63 is likewise undocumented. The semantics are subtle: after calling it, openEvalCache() (src/libcmd/installable-flake.cc:330-336) will silently rebuild a fresh cache, so callers must not use the installable afterwards for evaluation if the goal is to flush the SQLite transaction before exec.

Suggested change
virtual void releaseEvalCache() {}
/**
* Drop this installable's reference to any evaluation cache it
* holds, so that the cache can be destroyed (and thus flushed to
* disk) before we `exec()` out of this process without running
* C++ destructors. After this call the installable should not be
* used for further evaluation.
*/
virtual void releaseEvalCache() {}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed, CONTRIBUTING.md requires it. Documented on the base declaration, including the constraint that the installable must not be used for evaluation afterwards (openEvalCache() would silently rebuild a fresh cache). Amended into the same commit — this PR is deliberately a single commit.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/1786432831-persist-eval-cache-before-exec branch from 589f7da to 9dd8691 Compare August 11, 2026 07:29
@ethancedwards8
ethancedwards8 merged commit 069dcef into main Aug 11, 2026
4 checks passed
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.

2 participants