7996: sync: fix notify_waiters priority in Notify

[martin-augment]

7996: To review by AI

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c18de917-e381-448d-9491-b3fe7522668c

📥 Commits

Reviewing files that changed from the base of the PR and between c49598d and 78e33b4.

📒 Files selected for processing (2)

• tokio/src/sync/notify.rs
• tokio/tests/sync_notify.rs

---

Walkthrough

A guard condition was added to NotifiedProject::poll_notified in the State::Init branch to check if the number of notify_waiters calls has changed since the future was created. If a change is detected, the state transitions to State::Done and the poll loop restarts. Additionally, a new test notify_one_has_priority_over_notify_waiters was added to verify that both notify_one() and notify_waiters() cause waiters to become ready immediately.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pr-7996-2026-03-31-08-08-35

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request updates the Notify synchronization primitive to improve state management during concurrent calls. It introduces a check within the Notified future's polling logic to detect changes in the notify_waiters call count, ensuring the future transitions to the Done state appropriately. A new test case was also added to verify that notify_one functions correctly alongside notify_waiters. I have no feedback to provide as no issues were identified.

[augmentcode]

🤖 Augment PR Summary

Summary: Updates Notify’s Notified polling fast-path to treat a notify_waiters() call since future creation as completion before trying to consume a pending notify_one() permit.
Tests: Adds a regression test asserting that a notify_one() following notify_waiters() still leaves an immediately-consumable permit for a subsequent waiter.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[claude]

Code Review

This PR fixes a real bug (tokio-rs#7965) in Notify: an unpolled Notified future (still in State::Init) created before a notify_waiters() call could incorrectly consume the NOTIFIED permit set by a subsequent notify_one() call.

Correctness

The fix is correct. In the State::Init path, the optimistic CAS tries NOTIFIED -> EMPTY. If notify_waiters() was called after the future was created, the future should be completed by that waiters call -- not by stealing the notify_one permit. The new early check mirrors the existing lock-protected check at line 1153.

---

1. Missing comment on the new check

The existing equivalent check inside the lock (line 1153) has a clear comment explaining its purpose. The new pre-lock check at line 1121 has no comment. Since this is a subtle correctness property (preventing permit theft by an Init-state future when notify_waiters has already fired), a brief comment analogous to the existing one would help future readers.

Suggested addition:

// If notify_waiters has been called after the future was created,
// transition to Done without consuming the NOTIFIED permit.
if get_num_notify_waiters_calls(curr) != *notify_waiters_calls {

---

2. Inconsistent import in the test

The test file already has use tokio::sync::Notify; at line 7. The new test uses the full path tokio::sync::Notify::new() instead of just Notify::new(), inconsistent with every other test in the file.

---

3. Slightly misleading test name

notify_one_has_priority_over_notify_waiters implies a priority ordering between two methods, but what is actually being tested is: a Notified future created before notify_waiters() must not steal the permit set by a subsequent notify_one(). A name like unpolled_future_completed_by_notify_waiters_preserves_notify_one_permit would be more precise. Also note that the existing notify_one_after_dropped_all test already covers the polled (State::Waiting) variant of this scenario.

---

4. Informational: the pre-check is an optimization, not a correctness requirement

If notify_waiters() races between the state.load at line 1119 and the new check at line 1121, the check still catches it. If the race happens after the check passes but before the CAS at line 1127, the CAS will fail (the upper counter bits will have changed), and the code falls through to the lock path where the existing check at line 1153 handles it. So the new check safely avoids a doomed CAS + lock acquisition, but is not required for correctness.

---

Summary

The fix is logically sound and the test correctly captures the regression scenario. Items 1-3 are minor polish (comment, import style, test name) rather than blocking concerns.

[martin-augment]

1. Missing comment on the new check

The existing equivalent check inside the lock (line 1153) has a clear comment explaining its purpose. The new pre-lock check at line 1121 has no comment. Since this is a subtle correctness property (preventing permit theft by an Init-state future when notify_waiters has already fired), a brief comment analogous to the existing one would help future readers.

Suggested addition:

// If notify_waiters has been called after the future was created,
// transition to Done without consuming the NOTIFIED permit.
if get_num_notify_waiters_calls(curr) != *notify_waiters_calls {

value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The very same logic is executed few lines below (line 1153). It would be good to document why the same is needed again.

[martin-augment]

2. Inconsistent import in the test

The test file already has use tokio::sync::Notify; at line 7. The new test uses the full path tokio::sync::Notify::new() instead of just Notify::new(), inconsistent with every other test in the file.

value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! There is no need to use the fully qualified name since the Notify struct is already imported.

[martin-augment]

3. Slightly misleading test name

notify_one_has_priority_over_notify_waiters implies a priority ordering between two methods, but what is actually being tested is: a Notified future created before notify_waiters() must not steal the permit set by a subsequent notify_one(). A name like unpolled_future_completed_by_notify_waiters_preserves_notify_one_permit would be more precise. Also note that the existing notify_one_after_dropped_all test already covers the polled (State::Waiting) variant of this scenario.

value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The proposed test name suggests that there is a priority order but there is no such. The name of the test could be improved to explain what it test instead.