-
Notifications
You must be signed in to change notification settings - Fork 256
fix(async-streaming): harden context preservation #1576
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -535,6 +535,20 @@ def _wrap_async_generator_result( | |
| observe = _decorator.observe | ||
|
|
||
|
|
||
| def _get_generator_output( | ||
| items: List[Any], | ||
| transform_fn: Optional[Callable[[Iterable], str]], | ||
| ) -> Any: | ||
| output: Any = items | ||
|
|
||
| if transform_fn is not None: | ||
| output = transform_fn(items) | ||
| elif all(isinstance(item, str) for item in items): | ||
| output = "".join(items) | ||
|
|
||
| return output | ||
|
|
||
|
|
||
| class _ContextPreservedSyncGeneratorWrapper: | ||
| """Sync generator wrapper that ensures each iteration runs in preserved context.""" | ||
|
|
||
|
|
@@ -560,9 +574,17 @@ def __init__( | |
| self.items: List[Any] = [] | ||
| self.span = span | ||
| self.transform_fn = transform_fn | ||
| self._finalized = False | ||
|
|
||
| def __iter__(self) -> "_ContextPreservedSyncGeneratorWrapper": | ||
| return self | ||
| def __iter__(self) -> Generator[Any, None, None]: | ||
| try: | ||
| while True: | ||
| yield self.__next__() | ||
| except StopIteration: | ||
| return | ||
| finally: | ||
| if not self._finalized: | ||
| self.close() | ||
|
|
||
| def __next__(self) -> Any: | ||
| try: | ||
|
|
@@ -573,25 +595,65 @@ def __next__(self) -> Any: | |
| return item | ||
|
|
||
| except StopIteration: | ||
| # Handle output and span cleanup when generator is exhausted | ||
| output: Any = self.items | ||
| self._finalize() | ||
| raise # Re-raise StopIteration | ||
|
|
||
| if self.transform_fn is not None: | ||
| output = self.transform_fn(self.items) | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| elif all(isinstance(item, str) for item in self.items): | ||
| output = "".join(self.items) | ||
| def close(self) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self.span.update(output=output).end() | ||
| try: | ||
| close_method = getattr(self.generator, "close", None) | ||
| if callable(close_method): | ||
| self.context.run(close_method) | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| raise # Re-raise StopIteration | ||
| self._finalize() | ||
|
|
||
| def throw(self, typ: Any, val: Any = None, tb: Any = None) -> Any: | ||
| throw_method = getattr(self.generator, "throw", None) | ||
| if not callable(throw_method): | ||
| raise AttributeError("Wrapped generator does not support throw()") | ||
|
|
||
| try: | ||
| if tb is not None: | ||
| item = self.context.run(throw_method, typ, val, tb) | ||
| elif val is not None: | ||
| item = self.context.run(throw_method, typ, val) | ||
| else: | ||
| item = self.context.run(throw_method, typ) | ||
|
|
||
| self.items.append(item) | ||
|
|
||
| return item | ||
| except StopIteration: | ||
| self._finalize() | ||
| raise | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| def _finalize(self, error: Optional[BaseException] = None) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self._finalized = True | ||
|
|
||
| if error is not None: | ||
| self.span.update( | ||
| level="ERROR", status_message=str(e) or type(e).__name__ | ||
| level="ERROR", status_message=str(error) or type(error).__name__ | ||
| ).end() | ||
| return | ||
|
|
||
| raise | ||
| self.span.update( | ||
| output=_get_generator_output(self.items, self.transform_fn) | ||
| ).end() | ||
|
|
||
|
|
||
| class _ContextPreservedAsyncGeneratorWrapper: | ||
|
|
@@ -619,43 +681,93 @@ def __init__( | |
| self.items: List[Any] = [] | ||
| self.span = span | ||
| self.transform_fn = transform_fn | ||
| self._finalized = False | ||
|
Comment on lines
683
to
+684
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The Extended reasoning...What the bug is and how it manifests
The specific code path that triggers itWhen user code does Why existing code doesn't prevent itThe What the impact would beEvery early break from How to fix itApply the same pattern used for the sync wrapper: convert async def __aiter__(self):
try:
while True:
yield await self.__anext__()
except StopAsyncIteration:
return
finally:
if not self._finalized:
await self.aclose()This returns an actual async generator object, so Python automatically calls Step-by-step proof
|
||
|
|
||
| def __aiter__(self) -> "_ContextPreservedAsyncGeneratorWrapper": | ||
| return self | ||
|
|
||
| async def __anext__(self) -> Any: | ||
| try: | ||
| # Run the generator's __anext__ in the preserved context | ||
| try: | ||
| # Python 3.10+ approach with context parameter | ||
| item = await asyncio.create_task( | ||
| self.generator.__anext__(), # type: ignore | ||
| context=self.context, | ||
| ) # type: ignore | ||
| except TypeError: | ||
| # Python < 3.10 fallback - context parameter not supported | ||
| item = await self.generator.__anext__() | ||
| item = await self._run_in_preserved_context(self.generator.__anext__) | ||
|
|
||
| self.items.append(item) | ||
|
|
||
| return item | ||
|
|
||
| except StopAsyncIteration: | ||
| # Handle output and span cleanup when generator is exhausted | ||
| output: Any = self.items | ||
| self._finalize() | ||
| raise # Re-raise StopAsyncIteration | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| if self.transform_fn is not None: | ||
| output = self.transform_fn(self.items) | ||
| async def close(self) -> None: | ||
| await self.aclose() | ||
|
|
||
| elif all(isinstance(item, str) for item in self.items): | ||
| output = "".join(self.items) | ||
| async def aclose(self) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self.span.update(output=output).end() | ||
| try: | ||
| close_method = getattr(self.generator, "aclose", None) | ||
| if callable(close_method): | ||
| await self._run_in_preserved_context(close_method) | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| raise # Re-raise StopAsyncIteration | ||
| self._finalize() | ||
|
|
||
| async def athrow(self, typ: Any, val: Any = None, tb: Any = None) -> Any: | ||
| throw_method = getattr(self.generator, "athrow", None) | ||
| if not callable(throw_method): | ||
| raise AttributeError("Wrapped async generator does not support athrow()") | ||
|
|
||
| try: | ||
| if tb is not None: | ||
| item = await self._run_in_preserved_context( | ||
| lambda: throw_method(typ, val, tb) | ||
| ) | ||
| elif val is not None: | ||
| item = await self._run_in_preserved_context( | ||
| lambda: throw_method(typ, val) | ||
| ) | ||
| else: | ||
| item = await self._run_in_preserved_context(lambda: throw_method(typ)) | ||
|
|
||
| self.items.append(item) | ||
|
|
||
| return item | ||
| except StopAsyncIteration: | ||
| self._finalize() | ||
| raise | ||
| except (Exception, asyncio.CancelledError) as e: | ||
| self._finalize(error=e) | ||
| raise | ||
|
|
||
| async def _run_in_preserved_context(self, factory: Callable[[], Any]) -> Any: | ||
| awaitable = self.context.run(factory) | ||
|
|
||
| try: | ||
| task = asyncio.create_task(awaitable, context=self.context) # type: ignore[call-arg] | ||
| except TypeError: | ||
| task = self.context.run(asyncio.create_task, awaitable) | ||
|
|
||
| return await task | ||
|
|
||
| def _finalize(self, error: Optional[BaseException] = None) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self._finalized = True | ||
|
|
||
| if error is not None: | ||
| self.span.update( | ||
| level="ERROR", status_message=str(e) or type(e).__name__ | ||
| level="ERROR", status_message=str(error) or type(error).__name__ | ||
| ).end() | ||
| return | ||
|
|
||
| raise | ||
| self.span.update( | ||
| output=_get_generator_output(self.items, self.transform_fn) | ||
| ).end() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1010,6 +1010,7 @@ def __init__( | |
| self.response = response | ||
| self.generation = generation | ||
| self.completion_start_time: Optional[datetime] = None | ||
| self._finalized = False | ||
|
|
||
| def __iter__(self) -> Any: | ||
| try: | ||
|
|
@@ -1039,12 +1040,31 @@ def __next__(self) -> Any: | |
| raise | ||
|
|
||
| def __enter__(self) -> Any: | ||
| return self.__iter__() | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None: | ||
| pass | ||
| self.close() | ||
|
|
||
| def close(self) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| close_method = getattr(self.response, "close", None) | ||
| if callable(close_method): | ||
| try: | ||
| close_method() | ||
| finally: | ||
| self._finalize() | ||
| return | ||
|
|
||
| self._finalize() | ||
|
|
||
| def _finalize(self) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self._finalized = True | ||
|
|
||
| try: | ||
| model, completion, usage, metadata = ( | ||
| _extract_streamed_response_api_response(self.items) | ||
|
|
@@ -1081,6 +1101,7 @@ def __init__( | |
| self.response = response | ||
| self.generation = generation | ||
| self.completion_start_time: Optional[datetime] = None | ||
| self._finalized = False | ||
|
|
||
| async def __aiter__(self) -> Any: | ||
| try: | ||
|
|
@@ -1110,12 +1131,17 @@ async def __anext__(self) -> Any: | |
| raise | ||
|
|
||
| async def __aenter__(self) -> Any: | ||
| return self.__aiter__() | ||
| return self | ||
|
|
||
| async def __aexit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None: | ||
| pass | ||
| await self.aclose() | ||
|
|
||
| async def _finalize(self) -> None: | ||
| if self._finalized: | ||
| return | ||
|
|
||
| self._finalized = True | ||
|
|
||
| try: | ||
| model, completion, usage, metadata = ( | ||
| _extract_streamed_response_api_response(self.items) | ||
|
|
@@ -1142,11 +1168,40 @@ async def close(self) -> None: | |
|
|
||
| Automatically called if the response body is read to completion. | ||
| """ | ||
| await self.response.close() | ||
| if self._finalized: | ||
| return | ||
|
|
||
| close_method = getattr(self.response, "close", None) | ||
| if callable(close_method): | ||
| try: | ||
| await close_method() | ||
| finally: | ||
| await self._finalize() | ||
| return | ||
|
|
||
| await self._finalize() | ||
|
|
||
| async def aclose(self) -> None: | ||
| """Close the response and release the connection. | ||
|
|
||
| Automatically called if the response body is read to completion. | ||
| """ | ||
| await self.response.aclose() | ||
| if self._finalized: | ||
| return | ||
|
Comment on lines
+1189
to
+1190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
|
|
||
| close_method = getattr(self.response, "aclose", None) | ||
| if callable(close_method): | ||
| try: | ||
| await close_method() | ||
| finally: | ||
| await self._finalize() | ||
| else: | ||
| close_method = getattr(self.response, "close", None) | ||
| if callable(close_method): | ||
| try: | ||
| await close_method() | ||
| finally: | ||
| await self._finalize() | ||
| return | ||
|
|
||
| await self._finalize() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 In _ContextPreservedSyncGeneratorWrapper.close(), _finalize() is only called if close_method() either succeeds or raises Exception/asyncio.CancelledError; if it raises a BaseException not matched by that handler (e.g. KeyboardInterrupt, SystemExit), the trailing self._finalize() is never reached and the span leaks open forever. The same PR introduces LangfuseResponseGeneratorSync.close() which correctly uses try/finally to guarantee finalization — _ContextPreservedSyncGeneratorWrapper.close() should be updated to match.
Extended reasoning...
What the bug is
In
_ContextPreservedSyncGeneratorWrapper.close()(lines 600-614 oflangfuse/_client/observe.py), the call toself.context.run(close_method)is inside atry/except (Exception, asyncio.CancelledError)block. The span-finalizing callself._finalize()sits after that block, outside anyfinallyclause:The specific code path that triggers it
If
close_method()raises aBaseExceptionsubclass not covered by theexceptclause — for exampleKeyboardInterruptorSystemExit— the handler is bypassed, and execution unwinds past thetry/exceptblock without ever reaching the trailingself._finalize(). The span is left open with_finalized = False.Why existing code does not prevent it
The
__iter__method does have afinallyblock that callsself.close()on early loop exit, which handles the for-loop break case. However, once control is insideclose()itself, ifclose_method()raises aBaseException, the__iter__finally has already fired (or is not on the call stack). There is no further safety net._finalizedremainsFalseand the span is genuinely leaked.Comparison with the correct pattern introduced in this same PR
LangfuseResponseGeneratorSync.close()(also new in this PR) usestry/finallycorrectly:_ContextPreservedAsyncGeneratorWrapper.aclose()also usestry/finally. The asymmetry is limited to_ContextPreservedSyncGeneratorWrapper.close().Impact
If
KeyboardInterruptorSystemExitfires during generator teardown, the Langfuse span is never ended. This is a rare edge case (signal or interpreter shutdown during active generator close), but it is a real span leak that contradicts the always-finalize invariant the PR is carefully establishing everywhere else.How to fix
Use
try/finallywhile preserving error annotation on caught exceptions:Step-by-step proof
wrapper = _ContextPreservedSyncGeneratorWrapper(g, ctx, span, None)— span is open,_finalized=False.for item in wrapperthen breaks —__iter__'sfinallyclause callswrapper.close().close():_finalizedisFalse, we enter thetryblock and callself.context.run(close_method).close()executes and aKeyboardInterruptis raised (e.g. via OS signal handler active at that moment).KeyboardInterruptis not anExceptionorasyncio.CancelledError; theexceptclause does not match it.try/exceptentirely. The lineself._finalize()after the block is never executed._finalizedstaysFalse;span.end()is never called; the span is leaked indefinitely.