Repository navigation
Keep the mutex handoff test waiter locked until observation - #9273
Conversation
Hold the existing waiter's lock through the parent's immediate fast-acquire sample, then release and join it before asserting the recorded outcome. Do not change the production mutex or its existing fairness setup delay. Assisted-by: OpenAI Codex:model-version-unavailable
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6b1a71165
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[test] | ||
| fn unlock_hands_off_to_starved_waiter() { | ||
| let m = Arc::new(PyMutex::new()); | ||
| let (release_waiter, wait_for_release) = std::sync::mpsc::channel::<()>(); |
There was a problem hiding this comment.
The commit message explicitly says Codex assisted, but it contains no Assisted-by: AGENT_NAME:MODEL_VERSION trailer. RustPython requires that exact trailer for every AI-assisted contribution, so this commit's metadata must be corrected before integration.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| std::thread::spawn(move || { | ||
| m.lock(); | ||
| // Keep ownership until the parent has checked the handoff. | ||
| let _ = wait_for_release.recv(); |
There was a problem hiding this comment.
If the implementation regresses to a normal (non-TOKEN_HANDED_OFF) wake-up and the unparked waiter is scheduled before the parent reaches lock_fast, the waiter can acquire the now-unlocked mutex normally. This new recv then keeps that ordinary acquisition locked until after the check, so lock_fast returns false and the handoff test passes despite no direct handoff; the test needs to observe the handoff token/state without allowing an ordinary acquisition to satisfy the assertion.
Useful? React with 👍 / 👎.
Summary
Keep the existing native
unlock_hands_off_to_starved_waitertest's waiter holding its mutex until the parent has sampled the immediate fast-acquire attempt. Previously, the waiter could acquire and unlock before that assertion, allowing a correct handoff to look like failure.The parent still attempts
lock_fastimmediately afterunlock, without waiting for a waiter-acquired acknowledgement. It then releases any accidentally acquired lock, drops the owned release channel, joins the waiter, and asserts the captured result. This preserves the existing observation while avoiding a failure assertion that strands the waiter.This is a separate, main-based CI repair discovered while validating the Python 3.15 AST extraction in #9272. Only the existing Rust test changes. There are no production locking changes, Python-behavior Rust tests, new snippets, library changes, skips, increased sleeps, or weakened assertions.
Validation
cargo test --locked --offline -p rustpython-vm --features threading --lib --no-runpassed.Limits
The inherited test failed at its fast-acquire assertion in #9272's Linux job; its source is unchanged from that PR's base. The source demonstrates the early-unlock ordering flaw, but these checks do not prove that interleaving caused the hosted failure or eliminate all possible flakiness.
The existing
HAS_PARKEDflag precedes parking-lot queue insertion, so the unchanged 20 ms setup delay is still a scheduling assumption. This is not exhaustive fairness proof. Channel drop releases the waiter on parent unwind, but does not provide universal cleanup for panics while a mutex is held or broken mutex implementations. No full native suite or cross-platform local run is claimed.AI assistance
Codex assisted with source review, the minimal test repair, bounded validation, and PR preparation for the maintainer-directed Python 3.15 upgrade. The model version was not exposed. No claim of manual human testing is made.