[Core] Hash LabelSelector's label values as the set operator== compares

[LuciferYang]

Description

AbslHashValue walked a LabelConstraint's values with an ordered H::combine while operator== compares them as a set, so two LabelSelectors that compare equal could hash differently. Which slot a value takes in a flat_hash_set depends on the order the set was filled and on a salt absl derives from the address of the table's control bytes, so this is reachable rather than theoretical: with the old hash, building one label with the same 64 values 16 shuffled ways produced 16 distinct hashes.

SchedulingClassDescriptor hashes a LabelSelector directly and again through FallbackOption, and sched_cls_to_id_ only ever gets find and insert — no erase, no clear. A selector that hashes two ways therefore registers under two scheduling class ids, and the per-class queue and backlog metrics split with it.

The hash now sits on LabelConstraint, next to the operator== it has to agree with, and hands the values set to absl, which hashes its unordered containers order-independently and mixes in the size. That also removes the hand-written combine_unordered and the two explicit size() terms, since absl's container hashes already carry them. The two were added ten lines apart in #53578 and have disagreed ever since; keeping them on one type is what makes the next reader check both.

One consequence worth stating: if a future edit adds a field to LabelConstraint and forgets the hash, H::combine(h, GetConstraints()) fails to compile rather than silently falling back to a byte-wise hash. absl::hash_internal::is_uniquely_represented has no trivially-copyable fallback, so there is no path that would quietly hash the set's heap pointer.

Related issues

Fixes #65677

Not a duplicate: #65677 has no other linked PR, and no open PR modifies src/ray/common/scheduling/label_selector.h (checked #64986 and #62041, the two open Core PRs that mention label selectors — neither touches the file).

Additional information

Three tests, one per hashed field, each checked in the failing direction with one edit at a time:

edit to the hash	case that fails
restore master's ordered per-value combine	EqualSelectorsHashEquallyWhateverTheValueOrder
drop constraint.GetLabelValues()	SelectorsWithDifferentValuesHashDifferently
drop constraint.GetLabelKey()	SelectorsDifferingOnlyInKeyOrOperatorHashDifferently
drop static_cast<int>(constraint.GetOperator())	same as above

The order sweep builds one 64-value constraint 16 times with a fixed-seed shuffle and requires a single hash. It also asserts that more than one iteration order was actually observed: without that, the sweep would pass for an order-dependent hash whenever every round happened to lay the table out identically. An earlier revision used 4 values and swept reserved capacities instead, and missed the bug in 1 run out of 20 — reserving on the source set does nothing here, because LabelConstraint takes the set by value and absl normalizes the copy's capacity from its size.

Commands, on macOS 15.5 / arm64 with Apple clang 21. The two extra flags work around that toolchain, not this change (a deprecated builtin in the pinned absl):

bazel build --dynamic_mode=off \
  --copt=-Wno-error=deprecated-builtins \
  --host_copt=-Wno-error=deprecated-builtins \
  //src/ray/common/scheduling/tests:label_selector_test \
  //src/ray/common/scheduling/tests:fallback_strategy_test
./bazel-bin/src/ray/common/scheduling/tests/label_selector_test
./bazel-bin/src/ray/common/scheduling/tests/fallback_strategy_test

[==========] 10 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 10 tests.
[==========] 5 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 5 tests.

With the old hash restored, the order sweep failed on 20 runs out of 20; with this change all 10 tests passed on 20 runs out of 20. pre-commit run passes clang-format and cpplint on both files.

While here I swept every hash entry point under src/ for the same defect — an ordered combine over an unordered member whose operator== compares it as a container. There is no third instance: ResourceSet was the other one and #64958 fixed it. Five hash functions are coarser than their operator== (rpc::Address XORs four fields, so worker_id and node_id swap to the same hash; likewise gcs/state_util.h, core_worker/common.h, scheduling_class_util.h's node-label branch, and FunctionDescriptor::Hash), but all of them err toward extra collisions, never toward equal objects hashing differently, so they are correct and out of scope here.

Not addressed, and I think it deserves its own change: LabelSelector::operator== compares std::vector<LabelConstraint>, so it is order-sensitive. The hash agrees with it, so there is no inconsistency, but two selectors that mean the same thing written with their keys in a different order still register as two scheduling classes. That is deterministic and bounded by how many orders users write, unlike the case fixed here. Filed as #65739.

AI assistance

AI assistance was used for this change and for reviewing it. I have read every changed line and run the commands above locally.

[gemini-code-assist]

Code Review

This pull request refactors the AbslHashValue implementation for LabelSelector and LabelConstraint to ensure that equal selectors hash equally regardless of the insertion order of their values. It also adds comprehensive unit tests to verify that hashing is order-independent and that differences in keys, operators, or values result in different hashes. There are no review comments, so I have no feedback to provide.

[LuciferYang]

@ryanaoleary @rueian friendly ping — would either of you have time to take a look at this one? No urgency, and happy to adjust it if you'd rather it went a different way.

[Kunchd]

@ryanaoleary Please take a look if you have time.

[LuciferYang]

Thank you @Kunchd

[ryanaoleary]

nit: could shorten some of these comments, I think they err on the verbose side currently

// LabelConstraint values form an unordered set, so their hash must be order-independent
// to match operator==. We shuffle inputs to force diverse internal memory layouts 
// (caused by insertion order and internal salting) and verify hash stability.

[LuciferYang]

Took your wording in 85f926b.

One measurement worth passing on, since your text credits salting: I instrumented the 16 rounds and the per-table salt proxy took only 2 distinct values across them, because the allocator kept handing back the same address. So the layout diversity the sweep gets is almost entirely insertion order, with salting contributing close to nothing here. Your parenthetical is still true of absl in general, which is why I left it as you wrote it, but say the word and I will narrow it to insertion order.

I also dropped the older, longer version's mention of the salt coming from the address of the control bytes. That is specific to this absl version, so it would go stale on an upgrade.

[ryanaoleary]

nit: I think we could remove this note about the key and operator needing their own test case, if a comment is needed it could just describe the test:

// Verify that both the key and the operator are mixed into the hash.

[LuciferYang]

Done in 85f926b, with your line. I applied the same treatment to the test above it, which argued for its own existence in the same way, so the file does not end up half in each style.

[ryanaoleary]

could use pass by value here and move to avoid deep copies every iteration in the test:

LabelSelector OneConstraint(std::string key,
                            LabelSelectorOperator op,
                            absl::flat_hash_set<std::string> values) {
  LabelSelector selector;
  selector.AddConstraint(LabelConstraint(std::move(key), op, std::move(values)));
  return selector;
}

LabelSelector RegionSelector(absl::flat_hash_set<std::string> values) {
  return OneConstraint("region", LabelSelectorOperator::LABEL_IN, std::move(values));
}

we'd also change the selector creation to this pattern:

const LabelSelector selector = RegionSelector(std::move(set));

[LuciferYang]

Done in 85f926b, and it turned out to be more than a copy saved. LabelConstraint already takes the set by value and moves it, so the old const & helper meant a copy landed on that parameter, and an absl set copy reinserts into a freshly sized table. The layouts the order sweep was visiting were therefore rehashed ones rather than the ones it built from each shuffle. Moving all the way through keeps the table the test made, which is what the sweep is about.

Re-verified after the change since it touches that mechanism: 10 tests green on 10 consecutive runs, and both failing directions still fail by exit code, with an order-dependent hash failing the sweep on 10 out of 10.

Left the four calls in SelectorsDifferingOnlyInKeyOrOperatorHashDifferently as they are. The copy count there is the same either way, since it used to happen on LabelConstraint's parameter and now happens at the call, and removing it would mean four separate sets or moving only on the last call.

[ryanaoleary]

I left some relatively minor comments on the test changes but the fix itself LGTM, thanks for making this change. Will approve once the comments are resolved