[NA] [SDK] fix: stop bedrock and mistral stream wrappers from swallowing exceptions in finally - #8023
Conversation
…ing exceptions in finally The bedrock and mistral stream wrappers install class-level patches and used an early `return` inside a `finally` block to skip cleanup for non-tracked streams. A `return` in `finally` discards any in-flight exception, so once a tracked call installed the patch, non-tracked streams that errored completed silently. Bedrock is worse than a lost exception. The patch target is `botocore.response.StreamingBody.read` - botocore's shared class, used by every boto3 response body - and the try block does `return result`, so the `finally`'s `return None` overrides the returned value. After one traced invoke_model call, every non-tracked `read()` in the process (an S3 object, a Lambda payload) handed back None instead of its payload. Fix: invert the guard so cleanup nests under `if hasattr(...)` and no `return` remains in any `finally`. Tracked streams behave exactly as before. This is the same bug and the same fix that #7981 applies to the anthropic integration; credit to @trakshan-mishra for finding the pattern there. The two PRs touch disjoint files. Co-Authored-By: trakshan-mishra <43599000+trakshan-mishra@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
python
📋 PR Linter Failed❌ Missing Section. The description is missing the |
⏱️ pre-commit per-hook timing
⏭️ 39 skipped (no matching files changed)
|
|
Already covered by a test in this PR. You shipped the regression tests with the fix, and they are the right ones: the bedrock test asserts an untracked also touches Python SDK Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| def failing_generator(): | ||
| yield from () | ||
| raise RuntimeError("stream-blew-up") |
There was a problem hiding this comment.
Partial stream failure remains untested
The sync and async failing_generator() helpers yield no event, so the first pull raises before any chunk can be aggregated or delivered and the tests miss partial aggregation and post-delivery cleanup — should we yield a representative event before raising and assert that the caller receives it?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/library_integration/mistral/test_mistral.py` around lines 256-258 and
317-320, update the synchronous and asynchronous `failing_generator` helpers used by the
stream regression tests so each yields one representative event before raising
`RuntimeError`. Modify the corresponding tracked and untracked stream assertions around
lines 274-332 to record and verify that the event was received before the failure,
ensuring partial aggregation, delivery, and post-delivery cleanup are exercised.
| def test_mistral_chat_stream__untracked_stream_fails_after_class_patched__error_propagates( | ||
| fake_backend, | ||
| ): | ||
| """Regression test for the `return` inside `finally`. | ||
|
|
||
| opik patches ``__iter__`` on mistralai's stream class, so once any tracked | ||
| stream has been consumed every stream in the process runs through the | ||
| wrapper - including streams from untracked clients. A `return` in `finally` | ||
| swallowed the in-flight exception, so an untracked stream that failed | ||
| mid-iteration finished silently instead of raising. | ||
| """ | ||
| tracked_client = track_mistral( |
There was a problem hiding this comment.
Live API calls make failure tests flaky
The wrapper failure tests make live Mistral requests before mutating the SDK stream’s private generator, so they require credentials and provider latency and can fail before exercising the assertion. Could we use a deterministic fake stream/event source for these paths and keep one minimal integration test for patch wiring?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/tests/library_integration/mistral/test_mistral.py around lines 263-286,
refactor the untracked-stream failure test and the related async and tracked tests below
it to avoid real Mistral network requests and mutation of the SDK stream’s private
`generator`. Use small deterministic fake synchronous and asynchronous stream/event
sources that raise mid-iteration, while preserving assertions for exception propagation,
tracking behavior, and logged error information. Keep only one minimal integration test,
if needed, to verify the wrapper is installed on the real Mistral stream class.
| tracked_response = tracked_client.invoke_model( | ||
| modelId=ANTHROPIC_MODEL, | ||
| body=json.dumps(request_body), | ||
| contentType="application/json", | ||
| accept="application/json", | ||
| ) | ||
| assert json.loads(tracked_response["body"].read()) | ||
|
|
||
| untracked_client = boto3.client("bedrock-runtime", region_name="us-east-1") | ||
| untracked_response = untracked_client.invoke_model( |
There was a problem hiding this comment.
Network dependency slows and destabilizes CI
This regression test makes two live Bedrock invoke_model calls to verify StreamingBody.read() after patching, so it depends on AWS credentials and service availability and cannot run locally; should we use local StreamingBody/response objects or a deterministic mock, keeping only a minimal wiring check if needed?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/library_integration/bedrock/test_invoke_model.py` around lines
675-684, refactor
`test_bedrock_invoke_model__untracked_client_read_after_tracked_call__payload_returned`
so it does not issue live Bedrock `invoke_model` requests. Construct a local
`StreamingBody` and mocked response objects, invoke the patched `read` after exercising
the tracked-client setup, and assert that the untracked payload is returned while only
the tracked call is logged; retain a minimal mocked wiring check if needed.
|
Confirmed green. The bedrock test you couldn't run locally passed on all five jobs (3.10–3.14): Mistral job passed too. On the AST scan at the end of your description: ruff already ships this as So Two failures here that aren't your code, in case they're useful: PR Linter wants a |
4 participants
Details
The bedrock and mistral stream wrappers install class-level patches and used an early
returninside afinallyblock to skip cleanup for non-tracked streams. Areturninfinallydiscards any in-flight exception, so once a tracked call installed the patch, non-tracked streams that errored mid-iteration completed silently. Fix: invert the guard so cleanup nests underif hasattr(...)and noreturnremains in anyfinally— tracked streams behave exactly as before.botocore.response.StreamingBody.read, botocore's shared class used by every boto3 response body, and thetryblock doesreturn result. Areturn Noneinfinallyoverrides the returned value, so after one tracedinvoke_modelcall every non-trackedread()in the process — an S3 object, a Lambda payload — handed backNoneinstead of its payload:__iter__/__aiter__on the stream's class; these are generators with no return value to clobber, so the impact is exception swallowing only.This is the same bug and the same fix that #7981 applies to the anthropic integration — credit to @trakshan-mishra for finding the pattern there. I found these two while reviewing that PR. The two PRs touch disjoint files, so they can merge in either order. After both land, an AST scan over
sdks/python/src/opikfinds noreturn/break/continuein anyfinallyblock.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
pytest tests/library_integration/mistral/→ 18 passed (15 existing + 3 new), real Mistral APIpre-commit run --files <4 changed files>→ ruff, ruff-format, mypy, whitespace hooks all Passeduntracked_stream_fails_after_class_patchedtests fail (exception swallowed) and pass again with the fix. Thetracked_stream_fails_mid_iterationtest passes both ways by design — it guards the tracked path against regressions in the refactor.botocore.response.StreamingBodyobjects (output above); verified fixed by the same script.EventStream.generator(which is what__next__pulls from) on a genuine stream object — a real dropped-connection stands in without monkeypatching the patched class.test_bedrock_invoke_model__untracked_client_read_after_tracked_call__payload_returnedneeds real AWS Bedrock credentials, which I don't have — theAWS_PROFILEhere fails withPartialCredentialsError. It collects cleanly and is modelled on the existing verified..._anthropic___happyflowtest in the same file (same client, model, and request body); the mechanism it asserts is the one reproduced above. Please confirm it goes green in CI.Documentation
No documentation changes needed — this is an internal bug fix with no public API or behaviour change for correctly-tracked streams.