Server: Change Invalid Schema from Server Error (500) to User Error (400)

[chadvoegele]

Problem

Currently when a client passes an invalid schema to llama-server, the server returns an Internal Server Error (500). This can cause the client to repeatedly retry the request with backoff. Since an invalid schema will never be fixed by a retry, the user just sees a long processing time and then eventual failure.

Proposed Fix

This PR changes the server error to a Bad Request (400). That prevents the client from retrying, and surfaces to the invalid schema error back to the client.

Issues

This is a partial fix for Issue 17574.

Tests

1. $ ctest --test-dir build
2. $ ./tools/server/tests/tests.sh
3. Client (LibreChat) now shows Something went wrong. Here's the specific error message we encountered: An error occurred while processing the request: 400 JSON schema conversion failed: Unrecognized schema: {"not":{}} instead of long pause then unknown failure.

Alternative Solutions

An alternative solution could move this logic to ex_wrapper in tools/server/server.cpp. I went with the smaller behavior change for now, but can pursue that solution instead if desired.

[ngxson]

not sure if it's better to create a dedicated class for grammar-related exceptions

[ngxson]

IIRC oaicompat_chat_params_parse is used in multiple places. Changing it here won't fix it for other endpoints

[chadvoegele]

Ya, that's right.

What do you think about moving the invalid schema catch handler to ex_wrapper?

llama.cpp/tools/server/server.cpp

Line 3675 in c6f7a42

static server_http_context::handler_t ex_wrapper(server_http_context::handler_t func) {

That would be a more robust change, but a bit larger scope.

[ngxson]

I'm fine with placing it inside ex_wrapper, probably better to combine this with the error class specific to grammar as I mentioned

[ngxson]

Otherwise, a better approach is to wrap whatever call to json_schema_to_grammar inside an exception handler

[chadvoegele]

Thanks for your comments!

Give me a day or so and I can flip to the custom exception + ex_wrapper.

Otherwise, a better approach is to wrap whatever call to json_schema_to_grammar inside an exception handler

I didn't quite get this. Do you mean to wrap that call and re-throw as the new custom exception? Most of those calls are in common/chat.cpp via oaicompat_chat_params_parse in server.cpp. Can you explain more?

[ngxson]

hmm ok I thought that oaicompat_chat_params_parse can directly return error response instead of std::exception, but it can't.

for now, I think it's ok to use invalid_argument as 400 and generic exception as 500

• please update the ex_wrapper to return the error using this convention
• for everything else inside oaicompat_chat_params_parse that throws an exception, we should convert them to invalid_argument

[chadvoegele]

Done with that change!

I tried to change everything that was invalid input under oaicompat_chat_params_parse to invalid_argument but only one more test flipped. I was checking to see if I could add some more tests for these cases but am seeing 17 failed, 472 passed for env SLOW_TESTS=1 ./tools/server/tests/tests.sh. I think those are unrelated to this change.

[chadvoegele]

not sure if it's better to create a dedicated class for grammar-related exceptions

Sure, we can create a custom exception type for grammar errors.

[ngxson]

I think these changes are quite redundant, just need to change assert res.status_code != 200 to assert res.status_code == 400

[chadvoegele]

Yup, sorry. That was a carry-over from a previous rev where one case was still 500.

Fixed now.

[ngxson]

cc @allozaur for visibility, not sure if this affects error handling on webui, as we're now return 400 on invalid input, instead of 500

[chadvoegele]

@ngxson Thanks for reviewing!