lablup / lablup/mlxcel

refactor(server): add ErrorResponse::internal_server_error and collapse the eight hand-rolled 500s

Open
#1,690 0 comments 0 reactions 0 assignees View on GitHub
priority:low status:ready type:refactor
Dominant language
Rust
Stars
467
Forks
54
Avg merge
4h 25m
Merged PRs (30d)
310

Description

## Summary

Add a named `ErrorResponse::internal_server_error` constructor and route the eight hand-rolled HTTP 500 sites through it, so the 500 case is expressed the same way as the other status codes.

## Background

`ErrorResponse` already exposes named constructors for its other error statuses, but the 500 case has none. Each 500 site instead repeats the same two lines: `ErrorResponse::new(message, "server_error")` followed by `response.status = StatusCode::INTERNAL_SERVER_ERROR`. Besides the duplication, a site that forgets the status line silently downgrades a 500 to the `new` default of 400, which is exactly the defect tracked in #1695.

## Proposed Solution

Add `ErrorResponse::internal_server_error(message)` next to the existing four constructors, matching their doc-comment style, then replace the eight call sites with it. No behavior change.

## Implementation Notes

- Constructors to mirror: `src/server/types/response.rs:500` (`service_unavailable`), `:515` (`gateway_timeout`), `:535` (`not_supported`), `:549` (`not_implemented`). The base `ErrorResponse::new` is at `:489`.
- Hand-rolled 500 sites to convert: `src/server/routes/rerank.rs:70-75`, `:279-281`, `:285-294`, `:305-310`; `src/server/routes/embeddings.rs:75-81`, `:365-368`, `:383-388`; `src/server/gcp_compat.rs:274-277`.
- Confirmed pattern in `rerank.rs`: `ErrorResponse::new(format!("rerank inference failed: {message}"), "server_error")` then `response.status = StatusCode::INTERNAL_SERVER_ERROR`.
- Pairs with #1695: once this lands, the audio routes can adopt the same constructor.

## Acceptance Criteria

- [ ] One `internal_server_error` constructor added beside its four siblings.
- [ ] Eight call sites converted, with status codes and error strings unchanged.

---

## Original Suggestion

### Title: refactor(server): add ErrorResponse::internal_server_error and collapse the eight hand-rolled 500s

`ErrorResponse` has named constructors for 503, 504, 501, and not-supported — but the 500 case is hand-rolled at eight sites, each repeating the same two-line `new(...)` + `status = INTERNAL_SERVER_ERROR` dance.

## Evidence

- Existing constructors: `src/server/types/response.rs:500` (`service_unavailable`), `:515` (`gateway_timeout`), `:535` (`not_supported`), `:549` (`not_implemented`)
- Hand-rolled 500s: `src/server/routes/rerank.rs:70-75`, `:279-281`, `:285-294`, `:305-310`; `src/server/routes/embeddings.rs:75-81`, `:365-368`, `:383-388`; `src/server/gcp_compat.rs:274-277`

## Suggested fix

Add `ErrorResponse::internal_server_error(message)` next to its four siblings (same doc-comment style) and replace the eight call sites mechanically. Pairs naturally with the audio-route status fix, which can then use it too.

## Acceptance criteria

- [ ] One constructor, eight call sites converted, zero behavior change

Contributor guide

Open the contributing guide

Research direction

Start with the constructors in src/server/types/response.rs, then inspect the eight listed sites in src/server/routes/rerank.rs, src/server/routes/embeddings.rs, and src/server/gcp_compat.rs. Compare each existing 500 response with the neighboring named constructors. Done means one constructor is added and all eight sites use it without changing status codes or error strings.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, backend
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.