Conversation
cceckman-at-fastly
left a comment
cceckman-at-fastly
left a comment
There was a problem hiding this comment.
Nice! One nit and one symbol fix
Co-authored-by: Charles Eckman <charles.eckman@fastly.com>
TartanLlama
changed the title
cceckman-at-fastly
left a comment
cceckman-at-fastly
left a comment
There was a problem hiding this comment.
Mostly nits & clarifying questions -- but the thing on 5xx handling is a blocker.
| response.staleIfError = 300; | ||
| }, | ||
| TypeError, | ||
| 'Response set: staleIfError must be set only on unsent cache transaction responses', |
There was a problem hiding this comment.
nit: Is this the same criterion as "candidate responses", below?
In the Rust universe, we have separate types for the two, but I think they're the same in JS -- the restriction makes sense here, just might be clearer if it's using the same language.
| if (!state_res.is_err()) { | ||
| auto cache_state = state_res.unwrap(); | ||
| auto status = Response::status(response_obj); | ||
| if (cache_state.is_usable_if_error() && (status >= 500 && status < 600)) { |
There was a problem hiding this comment.
If I'm reading this right -- I don't think we should have the 5xx check here.
Here, we say about the Delivery behavior:
Fastly will, by default, accept and serve any response that is syntactically valid HTTP, which includes error responses.
This is also how the Rust SDK works: if there's a stale-if-error response in cache, and we get a 5xx from the origin, we return the 5xx to the user by default. The user can override that (in vcl_fetch, or in after_send in Rust) by matching the 5xx and returning an error from that handler... which causes the send (Rust) to return the stale response.
If I'm understanding this code right -- this precludes the ability for the guest to prefer the 5xx to the stale-if-error? Like, if the server returns a 5xx, they'll just always get SIE instead? I don't think we want that.
(Note: the only case in which Delivery "forces" the stale-if-error response is if the backend is failing a health check, according to here; all other cases require an explicit choice of deliver_stale.)
There was a problem hiding this comment.
(I may be misreading where in the stack this code lives, though, so please correct me if so!)
There was a problem hiding this comment.
Got it, I've removed this behaviour
| // We have a usable stale-if-error response | ||
| auto chose_stale_res = cache_entry.transaction_choose_stale(); | ||
| if (auto *err = chose_stale_res.to_err()) { | ||
| HANDLE_ERROR(cx, *err); | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Hm, I need a bigger picture here...
usable_if_error can show up in two cases:
- This client is a follower of a request collapse. Revalidation has already happened, and failed, so
lookupreturns the stale-if-error response as "immediately available" (FOUND | USABLEare also set). - This client is the leader of a request collapse. In this case the
MUST_INSERT_OR_UPDATEflag is also set.
transaction_choose_stale should only be called in case (2). Are there any paths by which try_serve_stale_if_error can be hit from case (2)?
(In case (1), we shouldn't be performing the backend send or evaluating the beforeSend / afterSend hooks, so... probably OK?)
| transaction_res = host_api::HttpCacheEntry::transaction_lookup( | ||
| request_handle, std::span<uint8_t>{override_key_hash.data(), override_key_hash.size()}); | ||
| } | ||
|
|
There was a problem hiding this comment.
Seems like this would be the same as
| transaction_res = host_api::HttpCacheEntry::transaction_lookup( | |
| request_handle, std::span<uint8_t>{override_key_hash.data(), override_key_hash.size()}); |
right? Since .empty() implies .size() == 0 -- or is there something about whether the pointer has data or not that matters?
| if (cache_state.is_usable_if_error()) { | ||
| // We've got a stale-if-error response, so swap it out for the error and notify any | ||
| // request collapse followers. | ||
| auto chose_stale_res = cache_entry.transaction_choose_stale(); | ||
| if (auto *err = chose_stale_res.to_err()) { | ||
| HANDLE_ERROR(cx, *err); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Trying to understand the interaction between this and try_serve_stale_with_error... is it: try_serve_stale_if_error is only invoked as part of promise processing, and this bit runs if the error occurs "not in a promise"?
Co-authored-by: Charles Eckman <charles.eckman@fastly.com>
2 participants
Based on Rust changes here: https://github.com/fastly/fst-compute-sdk-rs/pull/73