I expect this will be a performance bottleneck. We need to make a network request (sometimes multiple) for every repo we return results for. This is serialized by the mutex, but ideally we'd run those in parallel while still maintaining the order of results. This is likely a good job for lib/group's StreamGroup.

Feel free to skip the performance tuning for now though until we get a working version merged.

Okay this makes sense, so:

• We have a read through RulesCache or whatever that holds a map of fetched the ruleset for a repo and a sync.RWMutex.
• We start by getting a read lock and read from the map.
• If the map contains the item we can keep reading the ruleset. We never mutate a ruleset so once it is read we can release the lock.
• If the map has no element, we start by creating a new channel and then enqueue a fetch task into a StreamGroup managed by the RulesCache. We now wait for the channel to receive data before continuing:
  • At some point the task will run and resolve to a codeowners file. At some layer point the callback will be called
  • The callbacks are resolved in-order and from a single go routine so we can use this to write to the map after getting a write lock.
  • At the end of the callback we write to the channel to continue the in the GetFromCacheOrFetch function.

How do we avoid that we start fetching ownership information for the same repo multiple times though? We may need an additional map of inflight requests but the problem I see then is that we'd need to "upgrade" from a read lock to a write lock if we have a cache miss which doesn't seem to be a supported pattern. Is there some pattern for this that would simplify my solution?

How do we avoid that we start fetching ownership information for the same repo multiple times though?

That's a good question. The canonical answer is probably https://pkg.go.dev/golang.org/x/sync/singleflight, and I think this could be added to your read-through cache so requests fetches for the same repository are merged.

Realistically, we should probably not optimize this until needed because in most cases, a query will only return a result for a few repos. We can get fancy later 😄

I didn't find a working way to mock the feature flag value inside the tests. I can't simply create a new FlagSet since its values are not public and mocking the flag store also didn't really work so I decided to remove the test for now and will add it back once we open this up.

I've verified that this feature flag works locally by creating it via GraphQL and overwriting it for my test user though.

Seems reasonable. FWIW, I think you can mock this out with featureflag.WithContext(), but we should probably just put the resolved features on the SearchInputs struct rather than doing it further down the line. A task for another time 🙂

Seems reasonable. FWIW, I think you can mock this out with featureflag.WithContext(), but we should probably just put the resolved features on the SearchInputs struct rather than doing it further down the line. A task for another time 🙂