Skip to content

Commit

Permalink
Revert "[perf] Skip cache check if store path exists locally" (#2054)
Browse files Browse the repository at this point in the history
Reverts #2042

#2042 introduced a bug where adding non-cached packages
would fail to add them to the flake on the first attempt. (it would
succeed on subsequent attempts after the package is already in the
store).

The root cause is that `fetchNarInfoStatus` gets called twice per
package (even though we cache it and we're not supposed to call it
twice). The first call returns `false` because the package is not
cached. The second call returns `true` because the package is already in
store. This discrepancy essentially causes the package not to appear on
the flake at all. When updating state again, the package is already in
the nix store so both `fetchNarInfoStatus` calls return true.

I think reverting this is best immediate course of action. In follow up
we should fix `fetchNarInfoStatus` so it only gets called once (it will
improve performance and is more correct).

Later on we can think of better way to do #2042. The
current implementation is a bit fragile and not 100% consistent with the
initial intention of the function, so I'm concerned it can lead to more
bugs in the future.
  • Loading branch information
mikeland73 committed May 15, 2024
1 parent c198a04 commit 132ad46
Showing 1 changed file with 0 additions and 8 deletions.
8 changes: 0 additions & 8 deletions internal/devpkg/narinfo_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ import (
"fmt"
"io"
"net/http"
"os"
"sync"
"time"

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/boxcli/featureflag"
"go.jetpack.io/devbox/internal/debug"
"go.jetpack.io/devbox/internal/lock"
"go.jetpack.io/devbox/internal/nix"
"golang.org/x/sync/errgroup"
Expand Down Expand Up @@ -53,7 +51,6 @@ func (p *Package) IsInBinaryCache() (bool, error) {
// package in the list, and caches the result.
// Callers of IsInBinaryCache may call this function first as a perf-optimization.
func FillNarInfoCache(ctx context.Context, packages ...*Package) error {
defer debug.FunctionTimer().End()
if !featureflag.RemoveNixpkgs.Enabled() {
return nil
}
Expand Down Expand Up @@ -148,11 +145,6 @@ func (p *Package) fetchNarInfoStatus(outputName string) (bool, error) {

outputInCache := map[string]bool{} // key = output name, value = in cache
for _, output := range outputs {
// if store path exists locally, then it is equivalent to being in the cache.
if _, err := os.Stat(output.Path); err == nil {
outputInCache[output.Name] = true
continue
}
pathParts := nix.NewStorePathParts(output.Path)
reqURL := BinaryCache + "/" + pathParts.Hash + ".narinfo"
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
Expand Down

0 comments on commit 132ad46

Please sign in to comment.