Skip to content

python: propagate the real error for unseekable streams in identify_stream (#1473) - #1475

Open
kaluli123123 wants to merge 4 commits into
google:mainfrom
kaluli123123:fix/identify-stream-unseekable-position
Open

kaluli123123 wants to merge 4 commits into
google:mainfrom
kaluli123123:fix/identify-stream-unseekable-position

Conversation

@kaluli123123

@kaluli123123 kaluli123123 commented Sep 14, 2026 •

Copy link
Copy Markdown

Fixes #1473.

Queries the stream position before entering the restoring try/finally, preserving the original unseekable-stream error. Includes regression coverage and merges main at 8d3d3b9 without expanding the fix.

Python 3.13 validation: both stream regressions passed; non-slow suite 41 passed / 1 skipped; lock, Ruff, formatting, mypy, copyright and CLI/module smokes passed. A real OS pipe propagated ESPIPE; seekable stream positions remained unchanged.

Broader baseline checks exposed a macOS FFI linker error and a regenerated model-probe mismatch; no Rust/model changes are included. Formal approval and current-head upstream CI remain required.

…tream

`identify_stream()` bound `current_position` inside the `try` block but
referenced it unconditionally in `finally`. For a readable-but-unseekable
binary stream (e.g. `sys.stdin.buffer` on a pipe) `stream.tell()` raises,
so the name was never bound and `finally` raised `UnboundLocalError`,
demoting the actionable `OSError` to `__context__`.

Hoist `tell()` out of the `try` block: there is nothing to restore until
the position is known, and the only statement left inside `try` is the one
that needs the restore.

Fixes google#1473
Resolve the conflict in identify_stream() against the new
stream-reading logic from main: keep main's block-based read, and
keep this PR's fix of calling tell() before the try block so the
real error of an unseekable stream is not masked by an
UnboundLocalError.
@kaluli123123

Copy link
Copy Markdown
Author

Merged current main (2790535) to resolve the conflict with the new PyO3-based identify_stream(). The resolution keeps main's block-based read and keeps this PR's fix of calling tell() before the try block; main still has the original bug (the new test fails there with UnboundLocalError).

Checks run locally in python/ after uv sync: pytest tests -m "not slow" (40 passed, 1 skipped), ruff check, ruff format --check, and mypy src tests all pass.

@ebursztein

Copy link
Copy Markdown
Collaborator

Thank you! I checked this against the current main (Python on the Rust library): it still merges cleanly, the bug still reproduces without it, and the full Python suite passes with it, including your new test. It's listed in #1537 as ready to merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

python: identify_stream() raises UnboundLocalError instead of the real OSError on unseekable streams

2 participants