Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP more WS_Overflowed() checks #4235

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nigoroll
Copy link
Member

@nigoroll nigoroll commented Nov 25, 2024

Re #4232

Adding earlier overflow checks is pretty easy if we use VCL failure, but so far we had centralized the overflow handling on the client side in the vtr_deliver_f, sending downstream a 500 error and not a 503.
So my main question is: If we want consistency here, can we settle on simply handling workspace overflows as VCL failures? Or do we really want the separate error code still? If we do, improving the overflow detection on the backend side might become a bit more involved, because right now the backend side error just turns into a 503 on the client side.

Note that this PR is really for discussing the design questions, it does not pass the test suite because of the changed error code.

@nigoroll
Copy link
Member Author

nigoroll commented Dec 2, 2024

To make progress here, I'd suggest the following:

  • turn all VCL and out-of-workspace related errors into 500 because this is really what they are. 503 makes specific reference to a temporary overload or scheduled maintenance, but if we run into a VCL failure or out-of-workspace, that is likely to persist for the next request.
  • differentiate other error conditions. I think we have also more tickets in this area...

@nigoroll
Copy link
Member Author

nigoroll commented Dec 9, 2024

bugwash: turn it all into 500, update PR, write doc

@nigoroll nigoroll self-assigned this Dec 9, 2024
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.

1 participant