[core][dashboard] Return 4xx from node and actor detail APIs - #65015
[core][dashboard] Return 4xx from node and actor detail APIs#65015chenyuan99 wants to merge 9 commits into
Conversation
Follow-up to ray-project#51417, which added the `HTTPStatusCode` plumbing but only converted one endpoint. Part of ray-project#51442. Three node/actor endpoints reported client errors incorrectly: - `GET /nodes/{node_id}` returned 200 with a near-empty payload for an unknown node ID, because `get_node_info` falls back to `.get(node_id, {})`. It now returns 404. - `GET /logical/actors/{actor_id}` returned 200 with `detail: null` for an unknown actor ID, because `get_actor_infos` maps unknown IDs to `None`. It now returns 404. - `GET /nodes` with an unsupported `view` returned 500. An unsupported query parameter is a client error, so it now returns 400. The node and actor detail pages previously surfaced these cases via the `result === false` field in the 200 response body. axios rejects on 4xx, so that branch would no longer run; the SWR fetchers now catch the 404 and set the same message they set before. Callers of `useFetchActor` already use optional chaining, so their behavior is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: cysbc1999 <cysbc1999@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the Ray dashboard by returning appropriate HTTP status codes (404 Not Found and 400 Bad Request) from the backend for missing nodes, missing actors, or invalid views, and handling these errors gracefully in the frontend hooks. The review feedback suggests adding defensive checks, such as optional chaining and fallback objects, in both the frontend hooks and backend endpoints to prevent potential runtime crashes (like TypeError or AttributeError) if API responses or data structures are empty or malformed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dbe7b8db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 9dbe7b8. Configure here.
…e test
Two fixes from review feedback on the node/actor 4xx change.
Do not call `setRefresh(false)` when `/nodes/{node_id}` returns 404. A 404
can be transient: the node may not be in the dashboard's node table yet
during startup, or a 404 may be briefly served from the aiohttp cache.
Stopping the refresh left the page stuck on the error until a manual
reload, whereas the previous 200-with-empty-detail response kept polling
and let the page recover on its own.
Gate `test_node_api_status_codes` on the node actually being registered.
`wait_until_server_available` only checks that the HTTP server accepts
connections, and `/nodes/{node_id}` now 404s until the node appears in
the node table, so the success parametrizations could flake on a loaded
machine.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: cysbc1999 <cysbc1999@gmail.com>
|
Thanks for your review I will look into the comments |
…ize cluster test - DataOrganizer.get_node_info now returns None for an unknown node ID instead of the caller checking `node_id in DataSource.nodes` and then separately calling get_node_info, which left a TOCTOU gap if the node was evicted in between. - test_node_api_status_codes launches a single ray_start_with_dashboard cluster and loops over the status-code cases instead of using pytest.mark.parametrize, which was spinning up a fresh cluster per case. Signed-off-by: Yuan Chen <cysbc1999@gmail.com>
There was a problem hiding this comment.
LGTM for python changes. (I didn't review frontend changes)
@chenyuan99 Can you also update the stale PR description? It still says "parametrized over ..."
|
Thanks @MortalHappiness |
| setMsg("Actor Query Error Please Check Actor Id"); | ||
| } | ||
| if (result === false) { | ||
| setMsg("Actor Query Error Please Check Actor Id"); |
There was a problem hiding this comment.
Arent we gonna add setRefresh(false) here as well ?
There was a problem hiding this comment.
Just realized, its dead code as when result is false that means there might be some errors (4xx/ 5xx) which will throw an exception.
There was a problem hiding this comment.
Agreed, fixed in 36c9929 — removed the result === false branch. rest_response sets result: status_code == HTTPStatusCode.OK, so result can only be false on a non-2xx response, and axios already rejects on those, routing them to the catch block above (which sets the same message). So the branch was unreachable, as you found.
| setRefresh(false); | ||
| } | ||
| if (result === false) { | ||
| setMsg("Node Query Error Please Check Node Name"); |
There was a problem hiding this comment.
Agreed, fixed in 36c9929 — removed the result === false branch (including the setRefresh(false) call inside it). Same reasoning as the actor hook: rest_response only sets result: false on a non-2xx status, which axios already rejects on and routes to the catch block above.
|
Anything external scraping /nodes/{id} or /logical/actors/{id} and expecting 200-with-empty-body now gets 404. This changes the UX. Do we want to inform user about it ? |
…hooks rest_response() sets result: status_code == OK, so result is only false on a non-2xx response, which axios already routes to the catch block. The success-path result===false checks in useNodeDetail/useActorDetail were unreachable dead code, per review on ray-project#65015. Signed-off-by: Yuan Chen <cysbc1999@gmail.com>
|
@geetanjali-anyscale Yes, the UX change for On informing users: these two paths aren't documented anywhere under If you know of a specific internal or external consumer that currently expects the old 200 shape, happy to add a heads-up in the PR description or loop in the relevant owners before merge — just point me to it. |
|
Hi @chenyuan99 Can fix the CI error here https://buildkite.com/ray-project/premerge/builds/72259#01a02089-03a3-4ce7-aa3a-749c186ba152 The other two doc CI errors look unrelated, but the java worker CI tests look like a frontend build failure. |

Description
Makes three node/actor dashboard endpoints report client errors with 4xx status codes instead of 200/500.
#51417 added the
HTTPStatusCodeenum and therest_response(status_code=...)plumbing, but adoption stalled after one endpoint. Across non-test dashboard source there are currently 28OKand 13INTERNAL_ERRORusages versus only 4 non-500 error codes, all confined tostate_head.py/state_api_utils.py.Backend (
python/ray/dashboard/modules/node/node_head.py):GET /nodes/{node_id}returned 200 with a near-empty payload for an unknown node ID, becauseDataOrganizer.get_node_infofalls back to.get(node_id, {}). Now returns 404.GET /logical/actors/{actor_id}returned 200 withdetail: nullfor an unknown actor ID, becauseget_actor_infosmaps unknown IDs toNone. Now returns 404.GET /nodeswith an unsupportedviewreturned 500. An unsupported query parameter is a client error, so it now returns 400.Related issues
Related to #51442 (umbrella: revisit Ray dashboard API status codes).
Since that issue is an umbrella, this PR covers
node_head.pyonly, so each module can be reviewed independently.Additional information
Frontend change is required, not incidental
The node and actor detail pages surfaced these cases by reading
result === falseout of the 200 response body. axios rejects on 4xx, so those branches go dead the moment the status code changes, and the pages would sit on "Loading the node infos..." indefinitely withmsgnever updated.The SWR fetchers in
useNodeDetail.tsanduseActorDetail.tsnow catch the 404 and set the same message they set before, preserving existing UI behavior. Consumers ofuseFetchActor(the Serve pages) already use optional chaining on the result, so returningundefinedthere is unchanged behavior.Tests
Added
test_node_api_status_codes(single cluster, looping over all three error paths plus the three success paths — not parametrized, sinceray_start_with_dashboardstarts a fresh cluster per invocation) andtest_actor_not_found_status_code.Test environment: because Ray's core is compiled, these were run against the Ray Windows nightly wheel built from
3fb63d965f— the exact base commit of this branch — with the patchednode_head.pyoverlaid, so the patch was the only delta.The two failures in the full
test_node.pyrun aretest_node_infoandtest_worker_pids_reported. Both fail identically against unpatchednode_head.pyand are pre-existing Windows-environment issues, not regressions: worker cmdlines don't match"ray::Actor"on Windows, and runtime_env's uv virtualenv setup hitsWinError 206(path length).Control run confirming the new tests are not vacuous — against unpatched code:
/nodes/{unknown}→assert 200 == 404(fails)/nodes?view=unknown_view→assert 500 == 400(fails)/nodes→assert 500 == 400(fails)assert 200 == 404, body{"result": true, "msg": "Actor details fetched.", "data": {"detail": null}}(fails)Frontend checks:
Python lint:
ruff checkclean;blackleaves the changed files unmodified.Caveats worth reviewer attention: all of the above ran on Windows rather than Ray's Linux CI, and against a nightly wheel plus overlaid file rather than a from-source build. Linux CI remains the authoritative check.
Not a duplicate
No open PR references #51442, and the issue timeline has no cross-referenced PRs. The issue has been assigned since 2025-03-18 with no linked work; I commented on it before starting (comment).
AI assistance
AI assistance (Claude Code) was used to produce this change.