[dashboard] Guard memoryConverter against null/undefined/NaN from psutil stats#64402
Open
daiping8 wants to merge 2 commits into
Open
[dashboard] Guard memoryConverter against null/undefined/NaN from psutil stats#64402daiping8 wants to merge 2 commits into
daiping8 wants to merge 2 commits into
Conversation
memoryConverter threw "Cannot read properties of null (reading 'toFixed')" when the reporter agent serialized a null value for a psutil process stat field (e.g. pfaults/pageins on platforms where psutil cannot read them or raises AccessDenied). null < 1024 coerced to true, so the code reached null.toFixed(4) and crashed, making the node Worker info page fail to render. Guard against null/undefined/NaN and return "-" (an explicit no-data placeholder) instead of throwing. Update the type signature to reflect that psutil as_dict() can emit null, and add regression tests for null, undefined, and NaN. Signed-off-by: daiping8 <dai.ping88@zte.com.cn>
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the memoryConverter utility to safely handle null, undefined, and NaN values by returning "-" instead of crashing, and adds corresponding unit tests to verify this behavior. The review feedback suggests simplifying the guard condition in memoryConverter by removing redundant checks and using Number.isNaN instead of the global isNaN. Additionally, it recommends cleaning up the unit tests by removing redundant not.toThrow assertions.
Address review feedback on ray-project#64402: - Drop redundant explicit null/undefined checks; typeof !== "number" already covers them. - Use Number.isNaN instead of the global isNaN to avoid implicit coercion. - Remove redundant not.toThrow() wrappers from the edge-case tests. Signed-off-by: 10353800 <10353800@users.noreply.github.com> Signed-off-by: daiping8 <dai.ping88@zte.com.cn>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #64401
Motivation
The dashboard's Worker info view is unrenderable on nodes where the reporter agent emits
nullfor anymemoryInfofield (e.g.pfaults/pageinswhen psutil cannot read them or raisesAccessDenied).memoryConverter(null)crashes with
Cannot read properties of null (reading 'toFixed'), taking the whole Worker table down. This makes the per-node Worker view unusable for users on affected platforms/configurations.Implementation Details
Two files changed under
python/ray/dashboard/client/src/util/:converter.tsnumber | undefined | nullto reflect that psutilas_dict()can emitnull.bytesisnull,undefined, not anumber, orNaN, return"-"(an explicit no-data placeholder) instead of falling through to.toFixed. The"-"placeholder is clearer in a table than an empty cell.0.0000B); this change only adds the invalid-value guard, keeping the fix focused on the root cause.converter.unit.test.tsedge casesdescribe block with regression tests asserting thatmemoryConverter(null), memoryConverter(undefined), andmemoryConverter(NaN)each do not throw and return"-"`.The reporter-agent / serialization path is intentionally untouched: psutil returning
nullfor unreadable fields is expected behavior, so the correct fix is to tolerate it at the formatting boundary rather than to coerce it earlier.Verification
Unit tests pass:
Manual verification on a running cluster:
npm run buildto rebuild the dashboard frontend.ray start --head --dashboard-host=127.0.0.1 --dashboard-port=8265.http://127.0.0.1:8265/#/cluster/nodes/<node-id>, open the Worker section — the page renders and notoFixedTypeError appears in the console, even when workermemoryInfofields arenull.