Preserve the OS errno across the PyO3 boundary (#265) - #268
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
SummaryPreserve OS error details across the PyO3 boundary.
WalkthroughPreserve OS error codes, Python exception subclasses, and normalized messages during ChangesErrno preservation
Sequence Diagram(s)sequenceDiagram
participant RustStream
participant PumpError
participant PythonRuntime
RustStream->>PumpError: report OS I/O failure
PumpError->>PythonRuntime: construct platform-specific OSError
PythonRuntime-->>RustStream: expose errno, winerror, subclass, and message
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements a custom conversion from Rust io::Error/PumpError into Python OSError that preserves errno and lets CPython select the correct exception subclass, while cleaning up message formatting and adding tests to pin the behaviour at both the Rust and Python boundaries. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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: 6434c99046
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
6434c99 to
3a0ac56
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
79c3a90 to
d7588e4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 1264-1268: Update the Python error-handling example around
rust_consume_stream to import rust_consume_stream before it is called, ensuring
the example reaches the documented OSError branch without changing the existing
exception flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db9aa884-4d66-42bb-b189-7000e77553df
📒 Files selected for processing (5)
cuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_rust_errno.pydocs/developers-guide.mddocs/users-guide.mdrust/cuprum-rust/src/errors.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Re: pre-merge check "Testing (Overall)" — ERRORHead is now The tautology was realThe Windows case I added in raised = excinfo.value
winerror = _winerror_of(raised)
assert winerror, ...
derived = OSError(0, raised.strerror, None, winerror, None)
assert type(raised) is type(derived)
assert raised.errno == derived.errnoBoth the 1. Independent oracle
I chose the live probe over a hard-coded constant such as The Two implementation details worth flagging for review:
2. No-raw-code fallbackConfirmed: nothing exercised it. That arm is live in production, not hypothetical — the write paths raise It could not be covered from Python: no Python-reachable input makes a real Adding a Cargo feature to make the extension-module link optional would fix that, but it changes the build for two PRs stacked on this branch, so it is not the right move here. Instead the decision is split out of Mutation-verified, not assumed: changing 3. CI execution — deliberately not done hereThis is the one part of the resolution I am not implementing in this PR, and I want to be explicit about why rather than let it look like an oversight.
#268 does not touch What I verified versus assumed
Gates
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
`OSError`s raised by the extension carried `errno = None`, even when the failure was a real OS error whose number appeared in the message. Callers could not branch on the kind of failure; the only route left was matching the message text, which is not a stable interface. The cause is in PyO3's `From<io::Error> for PyErr`: it picks the exception type from `io::ErrorKind`, then constructs it with a single argument, the error's `Display` string. Python populates `errno` and `strerror` only for `OSError(errno, strerror)` — two arguments or more — so the number survived in the text and nowhere a caller could reach it. Construct the exception here instead, passing `(code, strerror)`. That fixes both halves at once, because CPython maps the errno to the matching subclass itself: `OSError(32, ...)` *is* a `BrokenPipeError`. It is the same subclass selection PyO3 reached for through `ErrorKind`, taken from the authoritative source rather than a parallel table — reading a directory now raises `IsADirectoryError`, as it should. Strip Rust's `" (os error N)"` suffix from the message, which would otherwise render as `"[Errno 9] Bad file descriptor (os error 9)"`. An `io::Error` with no `raw_os_error` has no number to preserve, so PyO3's mapping stays in use for it. The existing assertion in `test_rust_pump_stream_propagates_io_errors` is unchanged and now passes. It reaches the conversion through the pump, which treats a broken pipe as non-fatal and drains, so most interesting errnos never propagate that way; `test_rust_errno.py` isolates the conversion through the exported entry points instead, and pins the subclass selection and message shape as well as the number. Verified by mutation: restoring PyO3's single-argument conversion fails five tests, and leaving the Rust suffix in place fails one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The two-argument `OSError(code, strerror)` form is correct only where `raw_os_error` is an errno. On Windows it is a `GetLastError` code, so the number landed in `errno`, meaning something else entirely — `ERROR_INVALID_HANDLE` is 6, which as an errno is `ENXIO` — left `winerror` unset, and selected the subclass from the wrong number. The field this change set out to make branchable would have been actively misleading there, and Windows wheels are built and shipped. Move the construction behind a cfg-selected `os_error_to_py_err`. The Windows arm uses the five-argument form, which is the one that carries a native code: given a `winerror`, CPython ignores the errno argument, derives errno from the Win32 code, and selects the subclass from the derived value, so all three fields agree. Verified by cross-compiling (`cargo check --target x86_64-pc-windows-msvc`); no job runs the Python suite on Windows, so the errno tests now skip there rather than encode both taxonomies. Add three proptests for `strip_os_error_suffix`, whose four rstest cases could not show that stripping stays anchored to this error's own code over arbitrary messages and codes. The generator deliberately includes messages that merely end the way a suffix does: an unconstrained `.*` almost never produces one, and so cannot catch an implementation that trims punctuation off messages it does not recognise. Mutation-verified — never stripping fails property 1, stripping without the code anchor fails property 2, and rewriting unrecognised messages fails properties 2 and 3. Document the conversion in both guides, including the errno/strerror contract, subclass selection, and the removed suffix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`pytest.raises(OSError)` needs a `match` to satisfy ruff PT011, and the one these tests carried was `"Bad file descriptor"` — the C library's `strerror`, which glibc translates under `LC_MESSAGES`. Under a locale with the catalogue installed the guard would fail before the test reached the errno and exception-type assertions it exists for. Anchor on the `[Errno 9]` prefix CPython formats itself instead. That keeps PT011 satisfied, drops the dependency on English, and asserts something the test actually cares about — the number — rather than prose the platform owns. Verified with the extension built: all four pass. I could not reproduce the original failure here, because this host has no glibc message catalogues installed, so `strerror` stays English even under `fr_FR`; the change rests on the documented `LC_MESSAGES` behaviour rather than on a local reproduction. Also correct the users' guide, which said an I/O failure "inside either helper raises". `rust_pump_stream` treats a broken pipe or connection reset as a downstream stage exiting early: it drains and returns successfully, so that sentence described a failure the helper deliberately suppresses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`rendered` is not read after the comparison, so cloning it only to hand `prop_assert_eq!` an owned value allocates a second copy of every generated message for nothing. The macro binds its operands to locals before comparing, so the borrow taken for the left-hand call has already ended and the value can simply be moved. Raised by CodeRabbit as a Rust compiler lint integrity error.
The module skipped itself wholesale on Windows, so the Windows arm of `os_error_to_py_err` had no executable statement of its contract at all — only prose in the module docstring saying why it was absent. Skip per platform instead. The POSIX cases keep the assertions they always had; a new Windows case pins the part that differs, namely that the native Win32 code arrives in `winerror` rather than being passed off as an `errno`, and that `errno`, the exception subclass, and the message prefix are all derived from it. That case does not hard-code an expected code. It reads the expectation back from an `OSError` built from the observed `winerror`, so it pins the derivation itself and cannot go stale on whichever Win32 code the failure happens to raise. `OSError.winerror` is invisible to a type check run off Windows, so the one suppression that needs lives in a single accessor. Nothing runs this case yet: no job runs the Python suite on Windows, and `#277` tracks that. Update the developers' guide to say plainly what is executed today versus merely written down, since the previous wording credited a cross-compilation check that does not exist.
The `OSError` example under "Rust stream error handling" imports `errno` and then calls `rust_consume_stream`, which it never imports. A reader copying the block gets a `NameError` on the first line of the `try`, so they never reach the `IsADirectoryError` and `errno.EBADF` branches the section exists to demonstrate. Add the import, leaving the exception flow untouched. `fd` stays unbound: the prose immediately above establishes it as the descriptor under discussion, which is how the guide's other examples use that name. Checked every other Python block in the guide for the same omission; this is the only one. No other example references `rust_consume_stream`, `rust_pump_stream`, or anything else from `cuprum._streams_rs`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Windows case added in 85bc50c derived its expectations from the very exception it was testing: it read `winerror` off the raised `OSError`, built a second `OSError` from that number and the raised `strerror`, and compared the two. That pins CPython's own derivation, which was never in doubt, and says nothing about the value the extension supplied. A Rust side that hard-coded a plausible Win32 code, or reported one taken from some other failure, would have satisfied every assertion. The pre-merge review was right to call it circular. Obtain the expected code from outside the boundary instead. The test now issues the same failing `ReadFile` through `ctypes` — a Windows stream handle is a `std::fs::File`, whose `read` is `ReadFile`, so this is the same system call on the same handle — and reads the code back from `GetLastError`. `ctypes.WinError` then turns that code into the `OSError` CPython itself would raise, supplying the expected `errno`, `strerror`, and subclass. Nothing the extension returns feeds an expectation any more, and the `[WinError N]` guard names the code rather than accepting any digits. `use_last_error` keeps the interpreter from overwriting `GetLastError` before it is read, and explicit `argtypes` stop a pointer-sized `HANDLE` being truncated to a C int. Give the POSIX arm an oracle too. `strerror` was only asserted truthy; it is now compared against `os.strerror(EBADF)`, which CPython looks up independently of the exception. Cover the fallback arm, which nothing exercised. `io_error_to_py_err` hands an `io::Error` carrying no `raw_os_error` to PyO3 unchanged, and that arm is live in production: the write paths in `io_utils` raise `ErrorKind::WriteZero` for a write that makes no progress, and such an error has no number to preserve. Asserting on the built `PyErr` would need a live interpreter, and the crate is compiled with `pyo3/extension-module`, so no `cargo test` binary can link one — a probe fails at link time on undefined `PyExc_*` symbols. Split the decision out into `raw_os_error_parts`, which is pure, and test that directly for both `io::Error` representations. Mutation-verified: making the helper report code 0 instead of no code fails all three synthesized cases. Verified the POSIX assertions against a built extension (`maturin develop`): four pass, the Windows case skips. The Windows arm stays unexecuted, as no job runs the Python suite on Windows. No CI job is added here. POSIX extension-gated execution is #269's `extension-tests` job, and native Windows runtime coverage is #277; this branch does not touch `ci.yml`, and adding a job would conflict with #269 on rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4045613 to
c81ad46
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (1 error)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR head =='
gh pr view 268 --repo leynos/cuprum --json headRefOid,url,state --jq '.'
echo '== Relevant source structure =='
ast-grep outline rust/cuprum-rust/src/errors.rs --items all --match 'raw_os_error_parts|io_error_to_py_err|PumpError' || true
ast-grep outline cuprum/unittests/test_rust_errno.py --items all || true
echo '== Relevant source excerpts =='
rg -n -C 5 'raw_os_error_parts|io_error_to_py_err|raw_os_error|WinError|_win32_oracle|_win32_readfile_error|os\.strerror|extension-tests|`#269`|`#277`|`#278`' \
rust/cuprum-rust/src/errors.rs cuprum/unittests/test_rust_errno.py \
.github/workflows 2>/dev/null || true
echo '== Workflow files =='
fd -t f . .github/workflows 2>/dev/null | sort
echo '== Workflow references =='
rg -n -C 3 'extension-tests|maturin|test_rust_errno|pytest|windows' .github/workflows 2>/dev/null || true
echo '== Follow-up issue and PR state =='
for n in 269 277 278; do
echo "--- #$n ---"
gh issue view "$n" --repo leynos/cuprum --json number,title,state,url,body,closedAt \
--jq '{number,title,state,url,closedAt,body}' || true
doneLength of output: 36411
PR
PR
Do not open a new issue. Use this AI agent prompt for the remaining Windows work in ✏️ Learnings added
You are interacting with an AI system. |
3 participants
Summary
OSErrors raised by the extension carriederrno = None, even when the failure was a real OS error whose number appeared in the message. Callers could not branch on the kind of failure; the only route left was matching message text, which is not a stable interface.Closes #265.
Cause
PyO3's
From<io::Error> for PyErrpicks the exception type fromio::ErrorKind, then constructs it with a single argument — the error'sDisplaystring:Python populates
errnoandstrerroronly forOSError(errno, strerror), two arguments or more:I confirmed the number survives intact as far as
PumpError::Io(raw_os_error = Some(9)), so the loss is entirely at the final conversion.Fix
Construct the exception at our boundary with
(code, strerror). This fixes both halves at once, because CPython maps the errno to the matching subclass itself — the same subclass selection PyO3 reached for throughErrorKind, taken from the authoritative source rather than a parallel table. Reading a directory now raisesIsADirectoryError, as it should.Rust's
" (os error N)"suffix is stripped, which would otherwise render as"[Errno 9] Bad file descriptor (os error 9)". Anio::Errorwith noraw_os_errorhas no number to preserve, so PyO3's mapping stays in use for it.Tests
The existing assertion in
test_rust_pump_stream_propagates_io_errorsis unchanged and now passes. It reaches the conversion through the pump, which treats a broken pipe as non-fatal and drains — so most interesting errnos never propagate that way.cuprum/unittests/test_rust_errno.pyisolates the conversion through the exported entry points and pins the subclass selection and message shape as well as the number.strip_os_error_suffixhas Rust-side unit tests, including a mismatched-code case so a suffix belonging to a different error is never trimmed.Mutation-verified: restoring PyO3's single-argument conversion fails five tests; leaving the Rust suffix in place fails one.
Validation
make check-fmt,make lint,make typecheck,make testall pass;cs deltareports no issues.Note
Stacked on #241. Review that first.