Skip to content

MAINT: Bound WebSocket initialization receive time - #3239

Open
Aizen__28 (Yudeeswaran) wants to merge 3 commits into
microsoft:mainfrom
Yudeeswaran:p311-09-websocket-initialization-timeout
Open

Aizen__28 (Yudeeswaran) wants to merge 3 commits into
microsoft:mainfrom
Yudeeswaran:p311-09-websocket-initialization-timeout

Conversation

@Yudeeswaran

@Yudeeswaran Aizen__28 (Yudeeswaran) commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Give WebSocket initialization one overall timeout budget across all discarded initial messages using asyncio.timeout.
  • Keep initialization receiving on its own budget, separate from normal response receiving and conversation restoration.
  • Document the timeout behavior for response_timeout_seconds.
  • Add regression coverage showing that individually timely receives still time out when their combined duration exceeds the initialization budget.
  • Add cancellation coverage confirming cancellation propagates and the uninitialized connection is closed.

Issue and claim

Validation

  • Focused test module: 37 passed.
  • Ruff check passed for both changed files.
  • git diff --check passed.

@hannahwestra25 hannahwestra25 self-assigned this Oct 15, 2026


async def test_initialize_connection_async_uses_one_timeout_for_discarded_messages(
response_parser: Callable[[str | bytes], str | None],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of test nits:
wait_for_message on line 731 is annotated -> str but never returns — asyncio.Event().wait() isn't NoReturn, so ty flags invalid-return-type and make ty fails in CI. Same fix as wait_forever above it: add return "unreachable".

And the shared-budget test is a bit tight — 0.5s budget with two 0.35s sleeps only tolerates 150ms of overshoot before await_count drops to 1. On loaded CI runners that could flake. Maybe assert a timeout happens and await_count < n instead of an exact count?

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.

2 participants