- Notifications
You must be signed in to change notification settings - Fork926
chore: don't cache errors in file cache#18555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Uh oh!
There was an error while loading.Please reload this page.
coderd/files/cache.go Outdated
// Check if the caller's context was canceled | ||
iferr:=ctx.Err();err!=nil { | ||
returnnil,err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need to check this? It will fail theAuthorize
if it is cancelled
And if we do this check, do we need to close?
// Check if the caller's context was canceled | |
iferr:=ctx.Err();err!=nil { | |
returnnil,err | |
// Check if the caller's context was canceled | |
iferr:=ctx.Err();err!=nil { | |
e.close() | |
returnnil,err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just thought it felt nice to check explicitly 🤷♀️
Uh oh!
There was an error while loading.Please reload this page.
aslilac commentedJun 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
btw@Emyrk, the test as you originally wrote it assumed thatany second caller would refetch, regardless of timing. but we discussed loosening it a bit so that any callerafter the actual errored load would refetch, which is much more timing dependent. I can't really think of a good way to definitively test this behavior, because waiting until after the first fetch errors to run the second fetch means we're also waiting until the refcount would hit zero, which would clear it regardless of error state anyway. but if we call any earlier, most of the time the second caller gets the error, rarely taking long enough to trigger a refetch. maybe we could add some method to "leak" a reference for testing purposes to ensure that the file is refetched anyway, but I'm never a fan of adding extra complexity just to make something testable. |
Uh oh!
There was an error while loading.Please reload this page.
By design, concurrent calls to
Acquire
in the file cache all share a single database fetch. This is by design, so that everyone can share in the success of whoever asked for the file first. That's kind of what caches do!but one problem with the current implementation is that errors are also shared. This is mostly fine, because once all of the references are dropped, the cache entry will be freed, and the next
Acquire
will trigger a new fetch. However, if enough people are trying to load the same file at once, you could imagine how they might keep retrying and the reference count neverquite hits zero.To combat this, just immediately and forcibly remove errors from the cache, even if they still have references. Whoever is the first to retry afterwards will trigger a new fetch (like we want), which can then again be shared by others who retry.
Related, one opportunity to reduce the potential for errors we have is to use
context.Background()
for the database fetch so that a canceled request context cannot disrupt others who may be waiting for the file. We can then manually check the context outside of theLoad
, just like we already do with authorization.