Bug: The cache might have been incorrectly updated

[aeft]

In UpdateWithResponse, we only use query to find the cache entry and then fill the response. However, it's possible for two queries with the same value to occur at the same time. For example, if they use different models, the cached result may be incorrect in this case.

semantic-router/src/semantic-router/pkg/extproc/response_handler.go

Lines 157 to 165 in 31069a9

	if exists && ctx.RequestQuery != "" && responseBody != nil {
	err := r.Cache.UpdateWithResponse(string(cacheID), responseBody)
	if err != nil {
	observability.Errorf("Error updating cache: %v", err)
	// Continue even if cache update fails
	} else {
	observability.Infof("Cache updated for request ID: %s", ctx.RequestID)
	}
	}

(here cacheID is query)

I propose using ctx.RequestID to locate the correct cache because it's unique. That is, I will add RequestID to the CacheEntry struct. Also, we don't need to return cacheID anymore.

---

By the way, why do we use this two-step design? It seems like if the second step (filling the LLM response) fails, the first step is useless. Why don't we add the cache entry only when we get the full response?

[aeft]

@rootfs Could you please have a look when you have time? Thanks!

[aeft]

I found AddEntry method is not used currently, how will we use it in the future?

[rootfs]

@aeft sorry missing this issue today.

The unique ID for mapping request/response is to reduce the memory overhead of retaining the request content when processing the response. Yes, it makes perfect sense to use requestID, a unique ID already allocated by Envoy, instead of reinventing the wheel.

If you can help this with a PR, that'll be great!

[rootfs]

I found AddEntry method is not used currently, how will we use it in the future?

For the completeness of CRUD, let retain this for now, but feel free to add a comment there with your investigation.

[aeft]

@aeft sorry missing this issue today.

The unique ID for mapping request/response is to reduce the memory overhead of retaining the request content when processing the response. Yes, it makes perfect sense to use requestID, a unique ID already allocated by Envoy, instead of reinventing the wheel.

If you can help this with a PR, that'll be great!

Sure, I will submit a PR later.

I am curious why do we use this two-step design? It seems like if the second step (filling the LLM response) fails, the first step is useless. Why don't we add the cache entry only when we get the full response?

[rootfs]

I am curious why do we use this two-step design? It seems like if the second step (filling the LLM response) fails, the first step is useless. Why don't we add the cache entry only when we get the full response?

This two step is due to that extproc protocol is stateless, so in order to match the [request, response] pair, we have to keep the request body throughout the lifecycle. This adds memory pressure to the router.

You are right for the failure case. If that happens, an orphaned cache should be garbage collected. Can you give that a try too?

[aeft]

You are right for the failure case. If that happens, an orphaned cache should be garbage collected. Can you give that a try too?

I will add LastAccessAt to CacheEntry struct and keep the TTL mechnism so these orphaned cache entry can be garbage collected automatically. I will implement this in a PR for cache management policy.