[Core] Store an error when the owner reports an object as FREED - #65270
Open
LuciferYang wants to merge 2 commits into
Open
[Core] Store an error when the owner reports an object as FREED#65270LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
FutureResolver::ProcessResolvedObject handles three of the four cases a GetObjectStatus reply can carry: an unreachable owner, OUT_OF_SCOPE, and CREATED. GetObjectStatusReply::ObjectStatus also has FREED, which the owner sends when the object's value was freed while our reference was still in scope. That case matches no branch, so nothing is put into the memory store and a ray.get() on the reference blocks forever with no error. Put an OBJECT_FREED error, mirroring the sibling branches. This is the same error the owner already stores locally when it frees the object, and the one the Python layer maps to ObjectFreedError. The pre-ownership raylet code handled FREED alongside OUT_OF_SCOPE; the branch was dropped when that code was removed in ray-project#14184 and never ported to FutureResolver. Adds future_resolver_test.cc, which had no test file before. The FREED case fails against the unfixed code (nothing is stored) and passes with the fix; the other two statuses are covered as regression anchors. Signed-off-by: yangjie01 <yangjie01@baidu.com>
Include the headers the test uses directly instead of relying on test_utils.h to pull them in transitively, and swap the now-unused test_utils dep for the status dep the test actually needs. Hold the two FakeGauge metrics as members. Passing *std::make_shared<FakeGauge>() left ReferenceCounter holding references to temporaries that died at the end of the mem-initializer. The sibling tests do the same thing, but there is no reason to carry it into a new file. Stop the io thread in TearDown, matching object_recovery_manager_test. io_context_ is declared first and so destroyed last, which means without this the thread outlives the members whose callbacks it could run. Signed-off-by: yangjie01 <yangjie01@baidu.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request adds handling for the FREED object status in FutureResolver::ProcessResolvedObject, storing an OBJECT_FREED error in the in-memory store to prevent ray.get() from blocking indefinitely when an object's value is freed while its reference is still in scope. It also introduces a new test suite future_resolver_test to verify this behavior along with other resolution statuses. There are no review comments, so I have no feedback to provide.
ray-gardener
Bot
added
core
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
2 participants
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
FutureResolver::ProcessResolvedObjecthandles three outcomes of a borrower'sGetObjectStatusRPC: an unreachable owner storesOWNER_DIED,OUT_OF_SCOPEstoresOBJECT_DELETED, andCREATEDstores the value.GetObjectStatusReply::ObjectStatusalso hasFREED, which the owner sends when the object's value was freed while the borrower's reference was still in scope. That status matches no branch, so nothing is put into the memory store and nothing is logged. Aray.geton that reference blocks forever with no error.No other path fills the store either.
ResolveFutureAsyncsends one RPC and does not retry.GetObjectsonly falls through to plasma for entries that already exist carryingIsInPlasmaError, so a missing entry never gets there. TheOWNER_DIEDput in this function is the only one in the tree, so a later owner death does not help.The pre-ownership raylet code did handle this, treating
FREEDalongsideOUT_OF_SCOPE:if (!status.ok() || reply.status() == OUT_OF_SCOPE || reply.status() == FREED) { MarkObjectsAsFailed(OBJECT_UNRECONSTRUCTABLE, ...) }. That block was deleted wholesale in #14184 and theFREEDcase was never ported toFutureResolver.The fix stores
OBJECT_FREED, which is what the sibling branches do for their statuses. That is also the error the owner already stores locally inDeleteImpl, and the one Python maps toObjectFreedError, so the owner and the borrower raise the same exception for the same event.OBJECT_DELETED(what theOUT_OF_SCOPEsibling uses) would be wrong here, because it maps toReferenceCountingAssertionError, whose message says "This should not happen"; that would report a legitimate user action as an internal Ray invariant violation.Related issues
Fixes #65269
Additional information
Adds
future_resolver_test.cc; this class had no test file. TheFREEDcase fails against the unfixed code (nothing is stored,GetIfExistsreturns null) and passes with the fix. I re-verified that by reverting only the production change and re-running. The other two statuses are covered as regression anchors.bazel test //src/ray/core_worker/tests:future_resolver_testpasses, including under--config=asan.Two notes for reviewers. The most familiar trigger,
ray.internal.free, is deprecated, but theDeleteObjectsRPC handler andSealExisting(pin_object=false)reach the same owner-side freed state and are not. I also left theif/elsechain as a chain instead of converting it to aswitchon the enum, to keep this to one concern; aswitchwould stop a future fourth value from being dropped the same way.I used AI assistance to investigate and draft this change. I reviewed every changed line and ran the build and tests myself.