Skip to content

[core][scheduler] Fix iterator invalidation in node label filtering - #65516

Open
RocMarshal wants to merge 1 commit into
ray-project:masterfrom
RocMarshal:newhotfix
Open

[core][scheduler] Fix iterator invalidation in node label filtering#65516
RocMarshal wants to merge 1 commit into
ray-project:masterfrom
RocMarshal:newhotfix

Conversation

@RocMarshal

@RocMarshal RocMarshal commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Thank you for contributing to Ray! 🚀
Please review the Ray Contribution Guide before opening a pull request.

⚠️ Remove these instructions before submitting your PR.

💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete.

Description

[core][scheduler] Fix iterator invalidation in node label filtering

Related issues

#65517

Additional information

N.A

Signed-off-by: Yuepeng Pan <panyuepeng@apache.org>
@RocMarshal
RocMarshal marked this pull request as ready for review August 16, 2026 15:35
@RocMarshal
RocMarshal requested a review from a team as a code owner August 16, 2026 15:35

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes an iterator invalidation bug in NodeLabelSchedulingPolicy::FilterNodesByLabelMatchExpressions when erasing elements from match_nodes during iteration, and adds a corresponding unit test. The reviewer suggested a more idiomatic way to erase elements from the map by utilizing the return value of erase instead of manually copying and incrementing the iterator.

Comment on lines +106 to 110
} else {
auto iter_to_erase = iter;
++iter;
match_nodes.erase(iter_to_erase);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of manually copying the iterator, incrementing it, and then erasing the copy, you can use the return value of match_nodes.erase(iter). In absl::flat_hash_map (and standard C++11 containers), erase returns the iterator pointing to the element immediately following the erased element. This is more idiomatic and less error-prone.

      } else {
        iter = match_nodes.erase(iter);
      }

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 8c9a394. Configure here.

} else {
auto iter_to_erase = iter;
++iter;
match_nodes.erase(iter_to_erase);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear PR description

Low Severity

⚠️ This PR needs a clearer title and/or description.

This violates the Clear PR Descriptions and Titles rule. The description still includes template boilerplate and restates the title without explaining what the iterator invalidation problem is or how this change fixes it. A short sentence for each would help reviewers.

Fix in Cursor Fix in Web

Triggered by project rule: Bugbot Rules

Reviewed by Cursor Bugbot for commit 8c9a394. Configure here.

@ray-gardener ray-gardener Bot added core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant