[OpenAI] Add traceback for internal server errors for HTTP inference endpoint#1026
Conversation
![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 aims to improve debugging by adding traceback information to internal server errors for the HTTP inference endpoint. However, it introduces a critical security vulnerability by exposing full Python tracebacks in HTTP error responses, which can leak sensitive internal server information to potential attackers. It is recommended to remove the traceback from the response while retaining server-side logging. Additionally, there are suggestions for improving code style in tests by moving an import and enhancing test assertion consistency.
skyrl-train/skyrl_train/inference_engines/inference_engine_client_http_endpoint.py
Outdated
Show resolved
Hide resolved
skyrl-train/tests/gpu/gpu_ci/test_inference_engine_client_http_endpoint.py
Outdated
Show resolved
Hide resolved
skyrl-train/tests/gpu/gpu_ci/test_inference_engine_client_http_endpoint.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully implements server-side traceback logging for internal server errors in the HTTP inference endpoint, enhancing debugging capabilities without exposing sensitive information to clients. The added unit tests effectively validate this behavior, ensuring that tracebacks are logged internally while client-facing error messages remain clean, adhering to CWE-209. The changes are well-implemented and thoroughly tested.
1 participant
Before this PR, it is hard to see what the root cause of an error is when we hit an
INTERNAL_SERVER_ERROR.This PR adds Traceback. We test that the traceback is indeed there in the unit test by mocking an error.