Skip to content

fix(moonshine): return 400 on invalid audio#2326

Open
abn wants to merge 1 commit into
lemonade-sdk:mainfrom
abn:fix/moonshine-unsupported-audio-upload
Open

fix(moonshine): return 400 on invalid audio#2326
abn wants to merge 1 commit into
lemonade-sdk:mainfrom
abn:fix/moonshine-unsupported-audio-upload

Conversation

@abn

@abn abn commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Return 400 Bad Request instead of 500 when audio format is invalid. Add integration test coverage for WAV RIFF errors and unsupported OGG.

@github-actions github-actions Bot added engine::moonshine area::api HTTP REST API surface and route handlers labels Jun 20, 2026

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abn abn force-pushed the fix/moonshine-unsupported-audio-upload branch from b9648d2 to b4cba8c Compare June 20, 2026 23:17
@abn abn requested a review from fl0rianr June 20, 2026 23:18
@abn

abn commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

@fl0rianr I have committed the requested changes.

@abn abn force-pushed the fix/moonshine-unsupported-audio-upload branch 2 times, most recently from f284d1d to 01790a1 Compare June 21, 2026 00:37
@abn abn force-pushed the fix/moonshine-unsupported-audio-upload branch from 01790a1 to 7b0f399 Compare June 21, 2026 01:38
@abn

abn commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

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 (0.0.62, built from the main branch). Since the Python fix in tools/moonshine-server/main.py is in this PR branch and hasn't been merged or released yet, the precompiled binary used in CI still throws an uncaught exception on invalid audio uploads, resulting in an HTTP 500 response.

To keep the integration tests strict (asserting 400 Bad Request as requested) and prevent CI failures, we are temporarily mapping HTTP 500 responses containing "Not a valid RIFF file" to HTTP 400 invalid_request_error in moonshine_server.cpp.

Once this PR is merged and a new release of moonshine-server-rocm (e.g., 0.0.63) is published and pinned in backend_versions.json, we can safely remove this temporary message heuristic in a follow-up cleanup PR.

Let me know if this works for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area::api HTTP REST API surface and route handlers engine::moonshine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants