fix: status on errors #2036
fix: status on errors #2036
Conversation
mxyng
force-pushed
the
mxyng/fix-status-code
branch
from
0bde7d3
to
6b1cdf3
Compare
There was a problem hiding this comment.
I ended up hitting this last night, so it's great to fix it. I think we should look at trying to simplify it though because it's not clear how the errors get propagated through the stream.
I also wish that we could make streaming/non-streaming throw the same response code. Maybe we could detect the 401/404 by making a call to HEAD the remote endpoint before writing the headers back for the SSEs?
| @@ -965,7 +965,7 @@ func PushModel(ctx context.Context, name string, regOpts *RegistryOptions, fn fu | |||
| if err := uploadBlob(ctx, mp, layer, regOpts, fn); err != nil { | |||
| log.Printf("error uploading blob: %v", err) | |||
| if errors.Is(err, errUnauthorized) { | |||
| return fmt.Errorf("unable to push %s, make sure this namespace exists and you are authorized to push to it", ParseModelPath(name).GetNamespaceRepository()) | |||
| return fmt.Errorf("%w: unable to push %s, make sure this namespace exists and you are authorized to push to it", err, ParseModelPath(name).GetNamespaceRepository()) | |||
There was a problem hiding this comment.
what does the final error look like here?
There was a problem hiding this comment.
the output is mostly the same except it prepends unauthorized:
{
"error": "unauthorized: unable to push model, make sure this namespace exists and you are authorized to push to it"
}| @@ -953,6 +953,17 @@ func waitForStream(c *gin.Context, ch chan interface{}) { | |||
| c.JSON(http.StatusOK, r) | |||
| return | |||
| } | |||
| case error: | |||
There was a problem hiding this comment.
This feels more spaghettified than before w/ the gin.H case below. It's really unclear when error would be raised vs. when we would expect it to be set in gin.H.
There was a problem hiding this comment.
ideally the channel should send 1) the response struct or 2) an error. it should never send gin.H. that's not the case right now as some handlers like generate and chat still send gin.H{"error": err.Error()}
| @@ -977,6 +988,10 @@ func streamResponse(c *gin.Context, ch chan any) { | |||
| return false | |||
| } | |||
|
|
|||
| if err, ok := val.(error); ok { | |||
| val = gin.H{"error": err.Error()} | |||
There was a problem hiding this comment.
why unwrap the gin.H{} errors in the handlers than then rewrap it here?
There was a problem hiding this comment.
it's much harder to inspect an error if it's wrapped in gin.H since it stores the string representation error.Error(). since some errors are sometimes fmt.Errorf with variable content, it's impossible to determine what the real error is without string matching, which is tedious and error prone
inspect the real error is necessary for stream=false but since streamResponse and waitForStream consumes the same channel, this needs to receive real errors
mxyng
force-pushed
the
mxyng/fix-status-code
branch
from
20058dd
to
0f9be49
Compare
HTTP status on errors when stream:=false is always 500 Internal Server Error because the individual errors are not handled. The most common errors are: - pull, push: pulling or pushing a model that doesn't exist should return 404 Not Found - push: pushing a model into a place the user is authorized to should return 401 Unauthorized
mxyng
force-pushed
the
mxyng/fix-status-code
branch
from
0f9be49
to
5c17743
Compare
HTTP status on errors when stream:=false is always 500 Internal Server Error because the individual errors are not handled.
The most common errors are: