response-cache: purgeResponse leaks empty Sets from entityToResponseIds under eviction pressure
Description
I ran into this while answering a discussion about it (#4089) and pulled the current source to check whether the reported leak is actually still there. It is.
packages/envelop/plugins/response-cache/src/in-memory-cache.ts's purgeResponse removes the response id from an entity's Set, but never checks whether that Set is now empty:
function purgeResponse(responseId: string, shouldRemove = true) {
const entityIds = responseIdToEntityIds.get(responseId)
if (entityIds !== undefined) {
for (const entityId of entityIds) {
entityToResponseIds.get(entityId)?.delete(responseId)
}
responseIdToEntityIds.delete(responseId)
}
if (shouldRemove) {
cachedResponses.delete(responseId)
}
}entityToResponseIds keeps a map entry (an empty Set) for every typename and typename:id key that has ever appeared in a cached response, forever, even after every response referencing it has been purged. Under heavy eviction pressure (the LRUCache on cachedResponses is bounded via max, but entityToResponseIds isn't) this grows without bound.
Compare to purgeEntity a few lines below, which already does the equivalent "delete the map key once its Set is exhausted" check for the reverse map, just one level up:
function purgeEntity(entity: string) {
const responseIds = entityToResponseIds.get(entity)
if (responseIds !== undefined) {
for (const responseId of responseIds) {
purgeResponse(responseId)
}
}
}Reproduction
- Configure
@envelop/response-cachewith a smallmaxoncreateInMemoryCache. - Run enough distinct queries touching enough distinct entities to cycle well past
max, so the LRU evicts (anddisposecallspurgeResponse(responseId, false)) many times. - Inspect
entityToResponseIds.size(or just watch memory over time). It keeps growing linearly with the number of distinct entities ever seen, not bounded bymax.
Expected behavior
entityToResponseIds entries should be deleted once their Set becomes empty, the same way responseIdToEntityIds already gets a .delete(responseId) right above it.
Suggested fix
function purgeResponse(responseId: string, shouldRemove = true) {
const entityIds = responseIdToEntityIds.get(responseId)
if (entityIds !== undefined) {
for (const entityId of entityIds) {
const responseIds = entityToResponseIds.get(entityId)
if (responseIds) {
responseIds.delete(responseId)
if (responseIds.size === 0) {
entityToResponseIds.delete(entityId)
}
}
}
responseIdToEntityIds.delete(responseId)
}
if (shouldRemove) {
cachedResponses.delete(responseId)
}
}Happy to open a PR with this if useful, it's a contained two-line change with no behavior change on the non-leak path.
Source: graphql-hive/graphql-yoga