-
Notifications
You must be signed in to change notification settings - Fork 25
fix(apps): resolve streaming error handling for 2-minute timeout & content stream not allowed #453
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
Open
Athosone
wants to merge
10
commits into
microsoft:main
Choose a base branch
from
Athosone:fix/http-stream-rate-limit
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+50
−10
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
402286b
fix(apps): rate-limit HttpStream to 1 req/s streaming limit
Athosone bd1e0a3
fix(apps): pace retries and final close() send through the limiter
Athosone fa6ae49
fix(apps): address review — plumb stream options, pace-don't-drop def…
Athosone bd8eedb
fix(apps): always coalesce informative updates, add informative/text …
Athosone 0fe5331
refactor(apps): skip re-sending unchanged text, simplify flush bookke…
Athosone 0099f06
Merge branch 'main' into fix/http-stream-rate-limit
Athosone dbb885a
Merge branch 'main' into fix/http-stream-rate-limit
Athosone c7f1ac5
Merge branch 'main' into fix/http-stream-rate-limit
Athosone 8d5e26b
Revert HttpStream rate-limit changes from PR #453
lilyydu 3c1c34c
fix stream timeout 403 error
lilyydu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ def __init__(self, client: ApiClient, ref: ConversationReference): | |
| self._state_changed = asyncio.Event() | ||
|
|
||
| self._canceled = False | ||
| self._timed_out = False | ||
| self._reset_state() | ||
|
|
||
| def _reset_state(self) -> None: | ||
|
|
@@ -77,10 +78,18 @@ def _reset_state(self) -> None: | |
| def canceled(self) -> bool: | ||
| """ | ||
| Whether the stream has been canceled. | ||
| For example when the user pressed the Stop button or the 2-minute timeout has exceeded. | ||
| For example when the user pressed the Stop button. | ||
| """ | ||
| return self._canceled | ||
|
|
||
| @property | ||
| def timed_out(self) -> bool: | ||
| """ | ||
| Whether the stream has timed out. | ||
| For example when the streaming has exceeded two minutes. | ||
| """ | ||
| return self._timed_out | ||
|
|
||
| @property | ||
| def closed(self) -> bool: | ||
| """Whether the final stream message has been sent.""" | ||
|
|
@@ -194,11 +203,27 @@ async def close(self) -> Optional[SentActivity]: | |
| return None | ||
|
|
||
| # Build final message from the last emitted MessageActivityInput (last wins) | ||
| assert self._id is not None, "ID should be set by this point" | ||
| activity = self._final_activity or MessageActivityInput() | ||
| activity.with_text(self._text).with_id(self._id).with_channel_data(self._channel_data).add_stream_final() | ||
|
|
||
| res = await retry(lambda: self._send(activity), options=RetryOptions()) | ||
| if self._timed_out: | ||
| activity = self._final_activity or MessageActivityInput() | ||
| activity.with_text(self._text) | ||
| activity.id = None | ||
| res = await retry(lambda: self._send(activity), options=RetryOptions()) | ||
| else: | ||
| assert self._id is not None, "ID should be set by this point" | ||
| activity = self._final_activity or MessageActivityInput() | ||
| activity.with_text(self._text).with_id(self._id).with_channel_data(self._channel_data).add_stream_final() | ||
| try: | ||
| res = await retry(lambda: self._send(activity), options=RetryOptions()) | ||
| except StreamCancelledError: | ||
| # Reaches this point if the streaming time exceeded 2 minutes on the final request. | ||
| if not self._timed_out: | ||
| raise | ||
| # The final stream send itself tripped the time limit; resend the | ||
| # buffered content as a regular message (cleared id -> create path). | ||
| final_message = self._final_activity or MessageActivityInput() | ||
| final_message.with_text(self._text) | ||
| final_message.id = None | ||
| res = await self._send(final_message) | ||
|
|
||
| # Emit close event | ||
| self._events.emit("close", res) | ||
|
|
@@ -252,6 +277,9 @@ async def _flush(self) -> None: | |
| logger.debug("No activities to flush") | ||
| return | ||
|
|
||
| if self._timed_out: | ||
| return | ||
|
|
||
| # Send informative updates immediately | ||
| for typing_update in informative_updates: | ||
| await self._send_activity(typing_update) | ||
|
|
@@ -282,10 +310,15 @@ async def _send_activity(self, to_send: TypingActivityInput): | |
| to_send = to_send.with_id(self._id) | ||
| to_send = to_send.add_stream_update(self._index) | ||
|
|
||
| res = await retry( | ||
| lambda: self._send(to_send), | ||
| options=RetryOptions(max_delay=4.0, jitter_type="none", max_attempts=8), | ||
| ) | ||
| try: | ||
| res = await retry( | ||
| lambda: self._send(to_send), | ||
| options=RetryOptions(max_delay=4.0, jitter_type="none", max_attempts=8), | ||
| ) | ||
| except StreamCancelledError: | ||
| if self._timed_out: | ||
| return | ||
| raise | ||
| self._events.emit("chunk", res) | ||
| self._index += 1 | ||
| if self._id is None: | ||
|
|
@@ -315,6 +348,13 @@ async def _send(self, to_send: Union[TypingActivityInput, MessageActivityInput]) | |
| return SentActivity.merge(to_send, res) | ||
| except HTTPStatusError as e: | ||
| if e.response.status_code == 403: | ||
| error = e.response.json().get("error", {}) | ||
| message = error.get("message", "") | ||
| if message != "Content stream was cancelled by user.": | ||
| if message == "Content stream finished due to exceeded streaming time.": | ||
| self._timed_out = True | ||
| logger.warning("Teams encountered an error while streaming. Sending as a regular message.") | ||
| raise StreamCancelledError(message) from e | ||
|
Comment on lines
+353
to
+357
Collaborator
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. This will raise the StreamCancelled error to the handler. We should probably raise a different error here. Can we just do a switch statement here for various types of canceled errors? Also point to the learn docs that describe these. |
||
| self._canceled = True | ||
| logger.warning("Teams channel stopped the stream.") | ||
| raise StreamCancelledError("Teams channel stopped the stream.") from e | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
let's also make sure that the backend has tests for these particular strings
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.
(and maybe a more resilient error could be to just check for phrases like "canceled", "exceeded streaming time" etc.)