[Core] Remove reference counter mock for real reference counter in testing

[Kunchd]

Why are these changes needed?

This commit takes a step towards utilizing real classes during unit testing instead of mocks for components that are simple and local (e.g. reference counter). We remove the reference counter mock as promised in #57177 (review).

By directly testing on the real reference_counter, we are able to catch a behavior mismatch between AddNewActorHandle and its doc described behavior. Specifically, the EmplaceNewActorHandle doc states that the function should return false instead of failing due to ray check and the test tests exactly that. However, due to calling AddOwnedObject, EmplaceNewActorHandle actually ray check fails. The commit addresses this issue by making AddNewActorHandle idempotent.

Related issue number

N/A

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[gemini-code-assist]

Code Review

This pull request refactors AddNewActorHandle to EmplaceNewActorHandle and fixes a bug where it was not idempotent, potentially causing issues with reference counting on repeated calls. The changes also improve the actor_manager_test by using a real ReferenceCounter instance instead of a mock, which helps in catching such behavioral mismatches. The logic fix in EmplaceNewActorHandle is correct. However, I've found a critical issue in the test setup where dangling pointers are passed to the ReferenceCounter constructor, which could lead to test flakiness or crashes. My review includes a fix for this issue.

[dayshah]

uhh what's the point of this sentinel?

[Kunchd]

The sentinel will serve as a place holder associated with the actor_id to prevent the race condition of multiple calls to EmplaceNewActorHandle from acting on the actor id at the same time.

[Kunchd]

From our conversation offline, the better approach will be to guarantee that actor_id is unique. The investigation + enforcement of actor id uniqueness will be addressed in a future PR.

[dayshah]

wait why is the test breaking, is it because we double end up double adding the same ref in the test.

I think the problem is probably the test code, not the src code. The only way you can have the actor handle in actor_handles_ before AddNewActorHandle is if you went through GetNamedActorHandle which will always set owned to false, and it actually won't touch the ref counter at all in this codepath in that case.

Also for the future, describe the issue you're trying to fix in the pr description, e.g. why do you need to make AddNewActorHandle idempotent

[israbbani]

I agree with what @dayshah said. We should change the comment and the tests instead of the implementation of AddNewActorHandle. It's a fatal error if the ActorHandle already exists and we should raise the error as close to the call-site as possible.

[israbbani]

LGTM. A few small nits around comments, logging, and documentation.

[israbbani]

One small nit.