This would be much easier to reason about if the forthcoming insertion to the ipcache was also covered by the lock, so that these new critical sections would be more symmetric. However, it appears that it would be safe if the releaseCIDRIdentities() critical section executes right after this lock is released, as in this case the identity would not be "released" and hence not removed from the ipcache.

Oh yeah, I meant to comment specifically about this. In the if upsert {...} case this seems reasonable. The problem is, in the target situation I'm trying to fix here the ToFQDNs logic passes upsert=false here and then does the UpsertGeneratedIdentities() much later. I don't think it's viable to hold the lock for that entire period, we'd need to do some significant restructuring to really achieve this.

.. it appears that it would be safe if the releaseCIDRIdentities() critical section executes right after this lock is released, as in this case the identity would not be "released" and hence not removed from the ipcache.

That was pretty much my thinking. We'd either get:

• A: Lock
• A: ipc.IdentityAllocator.Release()
• A: ipc.deleteLocked()
• A: Unlock
• B: Lock
• B: ipc.allocate()
• B: Unlock
• B: Lock, Upsert, Unlock

or some variation of the below, potentially with the ipc.Upsert() from B also pushed earlier before A:

• B: Lock
• B: ipc.allocate()
• B: Unlock
• A: Lock
• A: ipc.IdentityAllocator.Release()
• A: (no ipc.deleteLocked() since the identity has a ref from B)
• A: Unlock
• B: Lock, Upsert, Unlock

For now since I couldn't cover both cases, I just didn't bother to cover the UpsertGeneratedIdentities() itself with the lock here. I could cover that function with the lock too, but it might just encourage assumptions around the locking consistency on this side which don't hold because the upsert is conditionally done later.

^^ Maybe longer term the real solution is to have the dnsproxy subsystem go to the identity ipcache subsystem and state "IP X needs an FQDN identity", then the ipcache can call out to the FQDN policy pieces, ensure that is all populated, then populate the ipcache itself. Then NotifyOnDNSMsg() won't need to worry about this detail, it would just need to have a waitgroup or callback to ensure the ipcache is plumbed before releasing the DNS responses. 🤔

^ That sounds like the ideal design we want to strive for.

I added the above explanation also to the commit message.