fix(nrfcache): stop stalling cache hits behind an in-flight NRF query

[bgrewell]

handleLookup takes the cache's exclusive write lock and holds it across the NRF network round trip. sync.RWMutex queues new readers behind a waiting writer, so one in-flight discovery blocks every concurrent cache hit for that NF type until the network call returns — the cache stops being a cache exactly when it is under load.

Observed on a live Aether SD-Core deployment under registration load:

• Throughput flat while latency grew linearly with concurrency (Little's Law holding at every point) — the signature of a serialized section, not CPU.
• Scaling the NRF 1→4 replicas changed nothing (the lock is client-side).
• The arithmetic matched: ~6 serialized discoveries × ~7 ms ≈ 42 ms per registration ≈ 23/s, against a measured 22/s ceiling.

The discovery round trip now runs outside the cache's own lock. A dedicated discoveryMutex preserves the existing single-NRF-callback behaviour that TestCacheConcurrency pins — the point was never to allow N concurrent identical NRF queries, only to stop that serialization from blocking unrelated readers.

Scoping honestly: with the matcher fixes in #170 making misses rare, this one's steady-state effect is within run-to-run noise. It matters on cold start, on TTL expiry, and whenever a miss storm coincides with load — which is when the system can least afford to serialize.

go test ./nrfcache/ passes, including TestCacheConcurrency. Independent of #170; applies cleanly either way.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

nrfcache/nrfcache.go:187

• Close()/purge() clear the cache and close c.done, but handleLookup will still write newly discovered instances into c.cache after the NRF round-trip completes. With the discovery now running outside c.mutex, this can happen after shutdown has begun/finished. Consider checking c.done while holding the write lock and bailing out (or skipping cache population) if the cache is closed.

	c.mutex.Lock()
	for i := range searchResult.NfInstances {
		c.set(&searchResult.NfInstances[i], ttl)
	}
	c.mutex.Unlock()

nrfcache/nrfcache.go:166

• After moving the NRF round-trip outside c.mutex, handleLookup can now continue issuing an NRF query even if Close()/purge() has already closed c.done. That means work can proceed on a cache that is being torn down, and it can also change the semantics of shutdown (Close can return while a discovery is still in-flight). Consider short-circuiting once c.done is closed before starting the discovery flow.

This issue also appears on line 183 of the same file.

	// discoveryMutex serializes NRF round-trips without blocking concurrent cache hits.
	c.discoveryMutex.Lock()
	defer c.discoveryMutex.Unlock()

	// Re-check in case another goroutine already populated the entry.
	c.mutex.RLock()
	nfInstances = c.get(param)
	c.mutex.RUnlock()

[gab-arrobo]

@bgrewell,
Please resolve conflict.
Thanks!

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.