Repository navigation
Fix bytearray membership racing with resizing - #9091
youknowone merged 2 commits into
Conversation
Search exact bytes and int needles under the data read lock instead of taking a temporary export. This prevents a concurrent slice replacement from receiving BufferError for a callback-free membership operation. Preserve export protection for generic index/buffer callbacks and actual memoryviews. Add Python coverage for these contracts. AI assistance: Codex/OpenAI diagnosed the race, implemented and reviewed the patch and regression snippet, and ran the reported validation. The runtime did not expose an exact model version. Assisted-by: Codex:model-version-unavailable
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: fixing bytearray membership behavior during concurrent resizing.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
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. |
There was a problem hiding this comment.
move to builtin_bytearray.py; this is a permanent rule. extract_tests/snippets/ must have only one file for one type/module in most cases
There was a problem hiding this comment.
Moved the coverage into builtin_bytearray.py and removed the separate snippet in b373af1. Coverage for this type will stay in its existing snippet file.
![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: 375501d63a
ℹ️ 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".
| } | ||
|
|
||
| fn __contains__(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<bool> { | ||
| // Exact builtins cannot call Python while preparing the needle. The |
There was a problem hiding this comment.
This commit explicitly documents Codex/OpenAI assistance but ends without the required Assisted-by: AGENT_NAME:MODEL_VERSION trailer, so it does not satisfy the repository's AI-disclosure format; add the trailer before merging.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
| @@ -0,0 +1,74 @@ | |||
| from testutils import assert_raises | |||
There was a problem hiding this comment.
The commit's Validation section explicitly says the workspace test run was still in progress and that the C-API tests and full snippets suite were not run, even though this interpreter change plus the new snippet requires all three before completion; finish the mandated workspace, C-API, and pytest -v runs rather than relying only on targeted checks.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The full local runtime suites have not been run. For 375501d, workspace test-binary compilation passed (22 executables, none run), along with three selected upstream tests and bounded Python contract/concurrency checks. For b373af1, the fresh release build, isolated finite contract on RustPython/final CPython, and strict workspace/all-targets plus separate CAPI/all-targets Clippy passed. Full local suites include stress/extreme-allocation paths outside the approved execution scope, so the PR explicitly reports that limit. CI for 375501d passed; the new head has a separate CI run.
| @@ -0,0 +1,74 @@ | |||
| from testutils import assert_raises | |||
|
|
|||
| for value in (b"", b"a", b"ab\x00\xff"): | |||
There was a problem hiding this comment.
This new file adds assertions and callback test logic, but the repository restricts test-file changes to adding or removing expectedFailure markers and explicitly disallows modifying test assertions, logic, or data; remove this test addition or obtain an explicit policy exception.
AGENTS.md reference: AGENTS.md:L273-L279
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The requested fix includes Python regression coverage, and the maintainer explicitly requested consolidating it into builtin_bytearray.py. b373af1 preserves that file's existing tests, moves this PR's new coverage there, and removes the separate snippet. Lib is unchanged.
| if let Some(bytes) = needle.downcast_ref_if_exact::<PyBytes>(vm) { | ||
| return Ok(self.borrow_buf().contains_str(bytes.as_bytes())); | ||
| } | ||
| if let Some(int) = needle.downcast_ref_if_exact::<PyInt>(vm) { |
There was a problem hiding this comment.
On a free-threaded run where one worker evaluates True in target or False in target while another resizes target, this exact-type check misses bool because its class is bool, not int. try_index_opt then handles the value through its callback-free downcast_ref::<PyInt>() path, but only after exports has been incremented, so the resizer can still observe a synthetic export and raise BufferError instead of waiting; exact bools need the same fast path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b373af1: exact builtin bool now uses the callback-free integer path. Added True/False membership cases in builtin_bytearray.py. The exact bool type guard leaves int subclasses and generic conversion/buffer paths under their existing export protection. The isolated finite contract passed on the rebuilt RustPython binary and final CPython 3.15; strict workspace and separate CAPI Clippy passed with -D warnings.
Merging this PR will not alter performance
Comparing 
Footnotes
|
Include exact builtin bool in the callback-free integer membership path. Keep int subclasses and generic conversion paths under export protection. Move this PR's regression coverage into the existing bytearray snippet and cover True/False membership as requested in review. AI assistance: Codex/OpenAI implemented and independently reviewed the review follow-up and ran the reported validation. The runtime did not expose an exact model version. Assisted-by: Codex:model-version-unavailable
2 participants
Summary
Avoid synthetic buffer exports when bytearray membership uses an exact bytes, int, or bool needle. These needles cannot run Python while being prepared, so the existing read lock is sufficient to serialize the search with resizing.
This addresses the intermittent macOS
test_free_threading_bytearrayfailure seen in #9063: one worker'sb'\xdd' not in atemporarily exporteda, making another worker'sa[:] = craise BufferError aftercchanged length. CPython protects its temporary export with an object critical section; the previous RustPython path exposed that temporary export to concurrent assignments.Generic index/buffer callbacks, subclasses, and real memoryview exports keep their existing protection. This is scoped to exact bytes/int/bool membership, not a general critical-section implementation.
Add deterministic Python regression coverage in the existing
builtin_bytearray.pysnippet for membership results, integer range checks, self-containment, bytes-subclass index dispatch, reentrant callbacks, real exports, same-size mutation, and export cleanup. Lib is unchanged.Validation
Initial fix (
375501d6):-D warnings.cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi --no-run) passed, producing 22 executables without running them.Review follow-up (
b373af16e):builtin_bytearray.py.-D warnings.Full local workspace runtime tests, the full snippets suite, and the full
test_bytesmodule have not been run. Separate CAPI test-binary compilation has not been run. This is targeted local validation, not a claim that all local tests pass.The fixed concurrent fixture is retained as external evidence rather than a deadline-sensitive CI snippet. It has three workers, 512 rounds each, at most 16 KiB target storage, and an external process watchdog. A passing run is probabilistic evidence, not proof of absence of races.
AI assistance
Codex/OpenAI performed source diagnosis, implemented the patch and Python regression checks, independently reviewed the lock/callback contract, and ran the reported validation. The runtime did not expose an exact model version.
Summary by CodeRabbit