[print-dev-env] Skip cache if lockfile has changed

[savil]

Summary

We need to skip the PrintDevEnvCache in ComputeNixEnv if lockfile is stale.

ensurePackagesAreInstalledAndComputeEnv had made an assumption that
ensurePackagesAreInstalled will have called computeNixEnv already to ensure
the PrintDevEnvCache was updated. However, after some recent refactoring, we
no longer call computeNixEnv within ensurePackagesAreInstalled.

So, we now need to check the lockfile status, and skip the PrintDevEnvCache
if lockfile is stale.

How was it tested?

1. Had patch_glibc:true for hello package. Did devbox shell and then
   saw that which hello points to the patch-glibc flake's hello.

2. exited devbox shell, and edited devbox.json to patch_glibc:false

3. re-started devbox shell and which hello now shows the regular nix hello.
   Previously, it would still show the patch-glibc hello since PATH would not have
   been updated to omit that.

[savil]

Current dependencies on/for this PR:

• main
  • PR [print-dev-env] Skip cache if lockfile has changed #1674 👈
    • PR [Renaming] ComputeNixEnv -> ComputeEnv; ensurePackagesAreInstalled -> ensureStateIsCurrent #1675

This stack of pull requests is managed by Graphite.

[mikeland73]

Oh wow, this is potentially a really bad bug, that was mostly masked by the fact that we add profile to path. In practice it means that straightforward use of devbox works, but anything that messes with the environment beyond just installing packages might not be picked up at all.

Unfortunately I think the best easiest solution is to reintroduce d.computeNixEnv into ensurePackagesAreInstalled functions. (See https://github.com/jetpack-io/devbox/pull/1584/files for the exact place where it was before it was removed).

The reason is that the local lock file uses the print-dev-env cache as input and it's saved at the end of ensurePackagesAreInstalled. This PR would create a new print-dev-env but then never write the new hash to the local lock, so the next time ensurePackagesAreInstalledAndComputeEnv gets called it would needlessly compute the environment again. By putting it inside of ensurePackagesAreInstalled (right before the d.lockfile.Save() call) ensures we save the correct hash.

(This solution is not perfect but it's very small so it limits the risk).

Some ideas for future improvement:

• Local lock file should be renamed to status hash or project hash or something more descriptive.
• We may want to change the interface of lockfile.Save (or in the future projectHash.Regen() to take the actual environment instead of reading it from the file. This would ensure we can't accidentally read a stale environment by calling it at the wrong time.

[savil]

The reason is that the local lock file uses the print-dev-env cache as input and it's saved at the end of ensurePackagesAreInstalled. This PR would create a new print-dev-env but then never write the new hash to the local lock, so the next time ensurePackagesAreInstalledAndComputeEnv gets called it would needlessly compute the environment again. By putting it inside of ensurePackagesAreInstalled (right before the d.lockfile.Save() call) ensures we save the correct hash.

Good call! I overlooked the issue with the lockfile saving a stale hash of the print-dev-env output.

Made the fix.

Some ideas for future improvement:

Local lock file should be renamed to status hash or project hash or something more descriptive.

Agree

We may want to change the interface of lockfile.Save (or in the future projectHash.Regen() to take the actual environment instead of reading it from the file. This would ensure we can't accidentally read a stale environment by calling it at the wrong time.

I don't follow this. What do you mean by actual environment ? Are you thinking that lockfile.Save or projectHash.Regen() will do computeNixEnv(ctx, false)?

[mikeland73]

I don't follow this. What do you mean by actual environment ? Are you thinking that lockfile.Save or projectHash.Regen() will do computeNixEnv(ctx, false)?

by actual environment I mean the output of ComputeNixEnv. Basically instead of just assuming that what is in cache is the correct value, we take a fresh value instead.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: error running script in Devbox: <redacted errors.withStack>: <redacted userer... go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted *errors.withStack>: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

_{Did you find this useful? React with a 👍 or 👎}