[bugfix] Fix critical bug when reporting for all paths where handler.create_error_response is used#34516
Conversation
…(e) presents Signed-off-by: Stanislav Kirillov <stas@nebius.com>
kizill
requested review from
DarkLight1337,
aarnphm,
chaunceyjiang,
njhill,
noooop and
russellb
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug where API endpoints would return a 200 OK status code even when an exception occurred. The fix is consistently applied across all affected API routers by modifying the exception handling logic. Instead of returning an ErrorResponse object directly from the except block, the changes now assign it to a local variable. This allows the existing subsequent logic to correctly construct a JSONResponse with the appropriate error status code. The changes are correct and effectively resolve the issue.
DarkLight1337
left a comment
DarkLight1337
left a comment
There was a problem hiding this comment.
I think it would be better to update create_error_response to create a JSONResponse with the status code instead.
|
@DarkLight1337 I tried and found that it will break error treatment logic in many places as |
|
Oh I didn't know that, @hmellor are you ok with this? |
There was a problem hiding this comment.
Let's get this fix in (sorry I meant to ping @noooop )
github-actions
bot
added
the
ready
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: athrael-soju <athrael-soju@users.noreply.github.com>
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: wzhao18 <wzhao18.sz@gmail.com>
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: Eldar Kurtic <research@neuralmagic.com>
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: zjy0516 <riverclouds.zhu@qq.com>
|
Is there an issue for this PR? I'd like to reopen that if exist |
|
#31164 should solve this in a more comprehensive manner, can you try it out? |
|
I'm afraid I moved on to work on something else, I accidentally ran into this issue and wanted to raise it, if I'll run into this issue again I'll report |
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>
…create_error_response is used (vllm-project#34516) Signed-off-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Stanislav Kirillov <stas@nebius.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>
4 participants
Purpose
Current versions (including v0.15.1 & v0.16.0) return http code 200 when error occures. That's critical for many applications.
Test Plan
Test Result