test(api): pin 200 response for failed job status (#168) - #424
Conversation
was always returning 200 no matter what, even when the job actually failed. added a check for that and it now returns 500 with the error message instead. added a test for it too. fixes SahilKumar75#168
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for the contribution, @Anshul1336. This cannot be merged as is. The dashboard polling logic in A failed job is a valid state, not a server error, so we prefer to keep the 200 and close #168. Alternatively, please update the polling logic to handle the new status and add tests. Please use the pull request template for future submissions. |
SahilKumar75
changed the title
A failed training job is a valid resource state, not a server error.
Returning 500 broke the dashboard poller in
app/state/training_poller_state.py, which only reads 200 responses:
failed jobs stayed "running" forever when the pub/sub event was missed,
and the error message was never shown.
Revert the 500 and replace the test with one that pins the contract:
GET /jobs/{id} returns 200 with status="failed" and the error text.
956a7e6 to
6c8b175
Compare
2 participants
Summary
Issue #168 asked for
GET /jobs/{job_id}to return 4xx/5xx when a training job fails. After review we are keeping the 200: a failed job is a valid resource state, not a server error, and the dashboard depends on reading it from a 200 response. This PR pins that contract with a test so it cannot regress silently.Scope
test_get_job_status_reports_failed_job_in_bodyintests/test_api.py, asserting a failed job returns 200 withstatus="failed"and the error text.app/api/jobs_routes.py. The first version of this PR raised a 500 there; that was reverted in 6c8b175.Product Impact
None for users. The dashboard keeps showing failed jobs as failed along with the error message.
Technical Notes
app/state/training_poller_state.pyonly handles 200 responses from this endpoint._reconcile_status_via_restis the fallback for when the Redis pub/sub event is lost. With a 500 it never sawfailed, so the dashboard stayed on "running" forever, and the final fetch never filled inerror_msg. A 500 would also have replaced theJobStatusbody with{"detail": ...}.A 404 for unknown job IDs was considered and rejected: the status key is only written to Redis once the worker picks the job up, so a queued job reads as
not_founduntil then.Validation
tests/test_api.py: 29 passed)Screenshots Or Recordings
n/a, no UI change.
Risk And Rollback
Test-only change, no risk. Roll back by reverting the commit.
Original fix by @Anshul1336; reworked during review.