[nar info cache] Only fillNarInfoCache in perf-sensitive code path

[savil]

Summary

In this PR, we loosen the conditions under which devpkg.Package.IsInBinaryCache can be called. Internally, it checks an in-memory map for the status of the package in the binary cache. Previously, it was required that callers pre-compute this status by invoking FillNarInfoInCache. The motivation then was to ensure that we avoid a synchronous http request if IsInBinaryCache is called within a loop.

However, this condition and the fact that golang lacks async-await support resulted in a loosely coupled load-and-check-later design whereby calls to FillNarInfoInCache would be sprinkled in various parts of the codebase and then IsInBinaryCache checked in other parts. This loose-coupling made it hard to reason about why any particular FillNarInfoInCache was present.

The straw that broke the proverbial camel's back was the issue that #1463 was seeking to fix.

In this PR, we add a fallback http request inside IsInBinaryCache and remove all but one of the FillNarInfoInCache calls. Reason:

1. The remaining callsite is in the hot codepath of calculating shellenv.
2. The other callsites were in the codepaths of adding or installing packages. This is necessarily a slow operation, and so a small bit of synchrony is acceptable.

NOTE: this PR also re-enables Remove Nixpkgs feature flag.

How was it tested?

• did some adhoc testing of opening a shell in an example project
• added testscript unit-test for adding packages that are not in the Devbox Search Index. re: [remove-nixpkgs] Disable remove-nixpkgs to fix non-search fallback #1463's scenario
• prior testscript unit-tests and project-tests should pass.

[savil]

Current dependencies on/for this PR:

• main
  • PR [nar info cache] Only fillNarInfoCache in perf-sensitive code path #1468 👈

This comment was auto-generated by Graphite.

[savil]

lol, I'll improve this to be less self-referential.

[ipince]

Thanks!

[mikeland73]

Nice!

[mikeland73]

For potentially simpler code. By definition, fillNarInfoCache() only returns a nil error if it succeeded. So you could do:

err := p.fillNarInfoCache()
return err == nil, err

Additionally, if fillNarInfoCache does

if isNarInfoInCache.status[p.Raw] {
  return nil
}

(no need to lock because we never remove from cache)

Then this entire function could be:

func IsInBinaryCache()
  if eligible, err := p.isEligibleForBinaryCache(); err != nil {
    return false, err
  } else if !eligible {
    return false, nil
  }
  err := p.fillNarInfoCache()
  return err == nil, err
}

(maybe rename fillNarInfoCache to fillNarInfoCacheIfNeeded for clarity)

[savil]

Nope, this doesn't quite work for two reasons:

1. fillNarInfoCache stores the boolean of whether the value is stored or not. A false value for a key in isNarInfoInCache does not indicate an error.

2. Golang maps are not safe for concurrent reads and writes, so we will need a lock.

[mikeland73]

@savil you are correct on (1) but not on (2).

For (2), reading a true is only unsafe if we remove and/or change values. Since we should never remove/change values within a single command, I think this is safe and improves code readability quite a bit.

For (1), we can modify fillNarInfoCache() to return (status, error). The early return continues to be safe because if something is set to true it never changes.

An additional benefit of this approach is that we limit cache access to a single function.

[savil]

clarified why (2) is correct in #1473 (comment)