Return error in case of runner crash #69
Conversation
In case a runner becomes defunct, e.g. as a result of a backend crash it would be neat to be able to reload it. So, if the loader finds runner, have it check if the runner is still alive, and create a new one if the runner is defunct. Signed-off-by: Piotr Stankiewicz <[email protected]>
In case the runner crashes it would be nice to return an error to the user. So, add an error handler on the proxy and use it to try to figure out if the runner crashed and format the response error accordingly. Signed-off-by: Piotr Stankiewicz <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return l.slots[existing], nil | ||
| select { | ||
| case <-l.slots[existing].done: | ||
| l.log.Warnf("Will reload defunct %s runner for %s. Runner error: %s.", backendName, model, |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix the issue, the user-provided model value should be sanitized before being logged. Since the log entries are plain text, we can remove newline characters (\n and \r) from the model string to prevent log forgery. This can be achieved using the strings.ReplaceAll function.
The sanitization should be applied in the load function in loader.go before the model value is used in the Warnf log statement. This ensures that any malicious input is neutralized before being logged.
| @@ -12,2 +12,3 @@ | ||
| "github.com/docker/model-runner/pkg/logging" | ||
| "strings" | ||
| ) | ||
| @@ -376,3 +377,5 @@ | ||
| case <-l.slots[existing].done: | ||
| l.log.Warnf("Will reload defunct %s runner for %s. Runner error: %s.", backendName, model, | ||
| sanitizedModel := strings.ReplaceAll(model, "\n", "") | ||
| sanitizedModel = strings.ReplaceAll(sanitizedModel, "\r", "") | ||
| l.log.Warnf("Will reload defunct %s runner for %s. Runner error: %s.", backendName, sanitizedModel, | ||
| l.slots[existing].err) |
| @@ -134,6 +135,28 @@ func run( | |||
| proxyLog: proxyLog, | |||
| } | |||
|
|
|||
| proxy.ErrorHandler = func(w http.ResponseWriter, req *http.Request, err error) { | |||
There was a problem hiding this comment.
Since this is going to be sent back to OpenAI clients, I would try to structure the error responses in a format they can parse. Documentation is a little sparse, but Google says the error format looks like:
{
"error": {
"message": "Invalid 'messages[1].content': string too long. Expected a string with maximum length 1048576, but got a string with length 1540820 instead.",
"type": "invalid_request_error",
"param": "messages[1].content",
"code": "string_above_max_length"
}
}
The only docs I can find for the API are https://platform.openai.com/docs/api-reference/responses-streaming/error
In case the runner crashes it would be nice to return an error to the
user. So, add an error handler on the proxy and use it to try to figure
out if the runner crashed and format the response error accordingly.
Based on: #68