Skip to content

Add stale-if-error logic to http caching layer#236

Open
dgryski wants to merge 8 commits intomainfrom
dgryski/http-cache-stale-if-error
Open

Add stale-if-error logic to http caching layer#236
dgryski wants to merge 8 commits intomainfrom
dgryski/http-cache-stale-if-error

Conversation

@dgryski
Copy link
Copy Markdown
Member

@dgryski dgryski commented Feb 27, 2026

No description provided.

@dgryski dgryski force-pushed the dgryski/http-cache-stale-if-error branch from 8e36505 to 603a90b Compare February 27, 2026 23:24
@dgryski
Copy link
Copy Markdown
Member Author

dgryski commented Mar 4, 2026

Draft until fastly/Viceroy#591 is in a Viceroy release.

@dgryski dgryski force-pushed the dgryski/http-cache-stale-if-error branch from a429a0d to df886c4 Compare March 13, 2026 17:56
@dgryski dgryski force-pushed the dgryski/http-cache-stale-if-error branch from 2d4022e to 45fd824 Compare March 31, 2026 23:47
@dgryski dgryski marked this pull request as ready for review March 31, 2026 23:47
@dgryski dgryski requested review from cee-dub and joeshaw and removed request for cee-dub April 1, 2026 17:56
Copy link
Copy Markdown
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review, in case you want to address anything before I get done with the rest.

if err != nil {
return false, fmt.Errorf("get state: %w", err)

return 0, fmt.Errorf("get state: %w", err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would be a little clearer to define a CacheLookupState const for 0.

// Substitute stale-if-error response; let anyone else in the collapse know as
// well.
fastly.HTTPCacheTransactionChooseStale(cacheHandle)
resp, foundErr := httpCacheGetFoundResponse(cacheHandle, req, backend, true, false)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question for my understanding: this is not a hit, since it is serving a stale response?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that makes sense? I was just translating the Rust code here :/

@dgryski
Copy link
Copy Markdown
Member Author

dgryski commented Apr 9, 2026

PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants