APQ: Add cache-control header on PERSISTED_QUERY_NOT_FOUND #2503
Conversation
This comment has been minimized.
This comment has been minimized.
apollo-router/src/cache/mod.rs
Outdated
| if let Some(sender) = self.wait_map.lock().await.remove(&key) { | ||
| let _ = sender.send(value.clone()); | ||
| } |
There was a problem hiding this comment.
I think the problem you are resolving is that there may be waiters who are not notified that a value has been inserted. The bit I don't understand is that, if there are waiters, then something should be "getting" the cached value and that should resolve at some point.
Logically, it's probably not safe to insert() a value to the cache if there are waiters for that key, so I think that aspect of your change is correct, but I don't like the fact that waiters should be resolved at some point and maybe insert should be ordered behind the existing get?
I think it needs input from @Geal .
There was a problem hiding this comment.
right, we paired on this and it turns out to be a corner case we hit because the test runs in the single threaded flavor. the actual observable behavior is that if it ever happens, one(1) client will need to send the apq and the query once more, which is totally ok.
Moreover this allows us to /not/ send the value like that. the core issue stems from the fact that the cache assumes the first person who tries to get is going to insert, which doesn't apply to APQ.
| @@ -79,6 +83,7 @@ async fn apq_request( | |||
| request.supergraph_request.body_mut().query = Some(cached_query); | |||
| Ok(request) | |||
| } else { | |||
| let _ = request.context.insert("persisted_query_miss", true); | |||
There was a problem hiding this comment.
Is it still used ? I can't find the corresponding get. But if it's still relevant then please use a constant with some namespacing like apq::persisted_query_miss
…query_register aggregation
Co-authored-by: Jesse Rosenberger <git@jro.cc>
This follows up #2457 and #2497 which both attempted to update `libgit2-sys`. The changeset of #2503 inadvertently resulted in a reversion of that version bump, which brought back the offending version and upset our compliance check. Thankfully, we have tooling that catches this stuff (even if it wasn't on the PR itself).
This follows up #2457 and #2497 which both attempted to update `libgit2-sys`. The changeset of #2503 inadvertently resulted in a reversion of that version bump, which brought back the offending version and upset our compliance check. Thankfully, we have tooling that catches this stuff (even if it wasn't on the PR itself).
Fixes #2502
The router now sends
cache-control: private, no-cache, must-revalidatewhen a persisted query hash was not found.