-
Couldn't load subscription status.
- Fork 677
Return NotFoundError/Exception for get info and set timeout, instead of a generic error
#945
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 39504d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hey @ValentaTomas, any reason this is not in the handleApiError / handle_api_exception method? |
| ) | ||
|
|
||
| if res.status_code == 404: | ||
| raise NotFoundException(f"Paused sandbox {sandbox_id} not found") |
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.
Bug: Incorrect Error Message for Sandbox Timeout
The async _cls_set_timeout method's 404 error message incorrectly specifies "Paused sandbox {sandbox_id} not found". The set_timeout operation applies to any sandbox state, not just paused ones, making this message misleading and inconsistent with the sync version.
Additional Locations (1)
|
@mishushakov, I wasn't sure when looking at the new build endpoints that seem to sometimes return 404, but are using BuildError by default, if changing the error in the general error handler wouldn't break anything, + I wanted to have a nicer message there. We can generalize if it is 100% not a problem. |
|
@ValentaTomas we aren't using BuildError by default. I have changed the implementation in the handler so it can use custom error class (SandboxError by default). I'd move any common error code here unless they're endpoint-specific. |
|
you can throw all 404 as NotFoundException also, doesn't have to be Sandbox/Build error |
|
Probably ok then. We might want to refine the server errors then (https://github.com/e2b-dev/infra/blob/add-client-proxy-retries/packages/api/internal/handlers/sandbox_get.go#L97 for example) if we are then exposing them via the general 404 handler. |
I know, but for example here (not sure if this specifically can return 404, but I found 404 in the API build handlers) if I add the explicit 404 general handler, this will now stop returning |
|
@ValentaTomas Same behavior as we have the with the AuthenticationError - it's not specific to Sandbox or Build. |
|
@mishushakov I'm okay with that, but didn' want to break anything in the build system if you rely on this being BuildError anywhere. |
|
404 status would be unexpected in Build system |
Well, you can get that as the API can return that. |
Note
Return NotFound errors on 404 for sandbox get info and set timeout in JS and Python SDKs.
packages/js-sdk/src/sandbox/sandboxApi.ts):setTimeoutandgetFullInfonow throwNotFoundErroron HTTP 404.packages/python-sdk/e2b/sandbox_async/sandbox_api.py):_cls_get_inforaisesNotFoundExceptionon 404._cls_set_timeoutraisesNotFoundExceptionon 404 (paused sandbox message).packages/python-sdk/e2b/sandbox_sync/sandbox_api.py):_cls_get_inforaisesNotFoundExceptionon 404._cls_set_timeoutraisesNotFoundExceptionon 404.Written by Cursor Bugbot for commit 39504d9. This will update automatically on new commits. Configure here.