fix(moonshine): return 400 on invalid audio#2326
Conversation
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks, this is the right direction and propagating error.status_code through set_error_response() makes sense.
I’d request one change before merge: please avoid classifying backend errors as 400 based on substrings in the error message. The backend now already returns HTTP 400 specifically for load_wav_file() failures, so the C++ wrapper can rely on the backend status code instead. With the current "Invalid"/ "Unsupported" matching, a real server-side 500 whose message happens to contain those words would be returned to clients as 400 invalid_request_error.
I’d suggest removing the message heuristic and setting invalid_request_error only for backend 4xx responses, or even only for status 400 in this PR.
Test nits:
- The OGG test currently uses
OggS..., so it’s testing an invalid/corrupt file rather than a real unsupported OGG. Either rename it or use a valid OGG fixture. - Avoid asserting the exact decoder message "Not a valid RIFF file"; the contract should be HTTP 400 + invalid_request_error.
- Consider using NamedTemporaryFile(..., suffix=...) instead of fixed temp paths.
b9648d2 to
b4cba8c
Compare
|
@fl0rianr I have committed the requested changes. |
f284d1d to
01790a1
Compare
01790a1 to
7b0f399
Compare
|
I went ahead and updated the C++ wrapper to temporarily restore the message-based fallback heuristic for now. The integration tests run against a precompiled binary of the Moonshine server downloaded at runtime ( To keep the integration tests strict (asserting Once this PR is merged and a new release of Let me know if this works for you! |
Return 400 Bad Request instead of 500 when audio format is invalid. Add integration test coverage for WAV RIFF errors and unsupported OGG.