[Core] Fix GetLocationFromOwner treating timeout_ms as microseconds - #65260
Open
LuciferYang wants to merge 1 commit into
Open
[Core] Fix GetLocationFromOwner treating timeout_ms as microseconds#65260LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
GetLocationFromOwner wraps its int64_t timeout_ms in std::chrono::microseconds, so any positive timeout expires 1000x too early. A caller asking for a 10s timeout gives up after 10ms. The parameter name, the header contract, the error string, and every other timeout_ms in the codebase mean milliseconds; this line is the only one treating it as microseconds. Use std::chrono::milliseconds, and fix the one Python docstring that also said "micro seconds". Signed-off-by: yangjie01 <yangjie01@baidu.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
ray-gardener
Bot
added
docs
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
1 participant
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.
Description
CoreWorker::GetLocationFromOwnerwraps itsint64_t timeout_msinstd::chrono::microseconds, so any positive timeout expires 1000x too early. A caller passingtimeout_ms=10000(meaning 10s) gives up after 10ms, well before a cross-worker RPC can complete, and gets a spuriousStatus::TimedOut.The parameter name, the header contract (
core_worker.h, "Timeout in milliseconds"), the error string ("... within N milliseconds"), and every othertimeout_msin the codebase mean milliseconds. This line is the only one treating the value as microseconds, and it has been this way since the API was added in #16130. The publicray.experimental.get_object_locationsdocstring separately said "micro seconds", which was also wrong, so this fixes both.Only callers passing an explicit positive
timeout_msare affected. The Python API defaults to-1, which takes the infinite-wait branch, and every in-tree caller uses that default.Related issues
Fixes #65254
Additional information
#54367 notes this API is expected to be removed once Data finishes migrating to
get_local_object_locations(#53942, still open). I raised that in #65254 and nobody responded, andget_object_locationsis still called in five Data paths today, so I'd rather fix the bug than leave it until the removal lands. Close this if you prefer to wait for the deletion.I verified the change by inspection and built
//src/ray/core_worker:core_worker_liblocally. I did not add a test: reaching the timeout branch needs an owner that never replies, and the wait usesstd::promise::wait_foron the realsteady_clock, which the repo's injectedClockInterfacecannot fake, so any behavioral test would depend on wall-clock timing and be flaky. #54367 removed the previoustest_get_locations_timeoutfor exactly that reason.I used AI assistance to investigate and draft this change. I reviewed every changed line and ran the build myself.