Skip to content

[bug] reflow v2 close ownership #159

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
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

blast-hardcheese
Copy link
Contributor

Why

Addressing some dangling work to hopefully advance the v2 client stability.

The main issue is that Session.close() had this weird circular dependency with ClientTransport.close() which increased the chance of error.

It should have been ClientTransport.close() -> Session.close() with no backwards arrows.

What changed

  • Move Session.close() responsibility into ClientTransport
  • Switch print(...) to logging.debug to make it easier to --log-level=debug in pytest and see all the logs together.
  • get_current_time() incorrectly used asyncio.Task's internal clock, instead of the wall clock, extending timeouts further than they should have been.
  • Caught a bug from a prior refactor where the deadline calculation was done inside the retry loop instead of before entering the loop

Test plan

CI, manual testing with staff/explorers after releasing a new version

@blast-hardcheese blast-hardcheese requested a review from a team as a code owner July 4, 2025 23:38
@blast-hardcheese blast-hardcheese requested review from Monkatraz and removed request for a team July 4, 2025 23:38
@blast-hardcheese blast-hardcheese force-pushed the dstewart/reflow-v2-close-ownership branch from 299d6df to 9fde4de Compare July 4, 2025 23:50
@blast-hardcheese blast-hardcheese force-pushed the dstewart/reflow-v2-close-ownership branch from bf68171 to 4ead10e Compare July 4, 2025 23:57
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.

1 participant