Skip to content

fix(integrations): close AsyncDaytona client in DaytonaEnvironment.close() - #6307

Closed
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:fix/daytona-close-client
Closed

fix(integrations): close AsyncDaytona client in DaytonaEnvironment.close()#6307
anxkhn wants to merge 1 commit into
google:mainfrom
anxkhn:fix/daytona-close-client

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

  • N/A (no existing issue; described below)

2. Or, if no issue exists, describe the change:

Problem:

DaytonaEnvironment.close() in src/google/adk/integrations/daytona/_daytona_environment.py deletes the sandbox and then drops the AsyncDaytona client by setting self._client = None, without awaiting the client's close():

async def close(self) -> None:
  if self._sandbox is not None:
    await self._sandbox.delete()
    self._sandbox = None
    self._client = None          # client dropped without closing it
    self._is_initialized = False

That client is an AsyncDaytona instance created in _create_sandbox(); it owns the underlying aiohttp ClientSession plus the REST and toolbox API clients. Just dropping the reference never releases those, so a socket/session leak accumulates on every create/close cycle. This matters for long-lived agents and for repeated test runs that set up and tear down an environment.

The Daytona Python SDK documents that AsyncDaytona.close() should be called to properly close the underlying HTTP sessions and avoid resource leaks (or the client can be used as an async context manager). The current code runs none of that.

Solution:

Await self._client.close() before clearing the reference, guarded on the client not being None:

async def close(self) -> None:
  if self._sandbox is not None:
    await self._sandbox.delete()
    self._sandbox = None
    if self._client is not None:
      # Close the AsyncDaytona client to release its underlying HTTP
      # sessions and avoid leaking sockets across create/close cycles.
      await self._client.close()
    self._client = None
    self._is_initialized = False

AsyncDaytona.close() is idempotent (it closes the API clients and the shared aiohttp session, all of which are no-ops when already closed), so the existing "second close() is a no-op" behavior is preserved.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

The existing test_close_deletes_sandbox_and_is_idempotent test used a MagicMock client and never asserted the client was closed, so it did not catch the leak. This PR gives the mock client an AsyncMock close() and asserts DaytonaEnvironment.close() awaits it exactly once (and that it stays a no-op, still awaited only once, on a second close()).

I confirmed the test actually guards the fix: with the production await self._client.close() line removed, the updated test fails with AssertionError: Expected close to have been awaited once. Awaited 0 times.; with the fix in place it passes.

pytest results:

$ pytest tests/unittests/integrations/daytona/test_daytona_environment.py -q
15 passed

No regressions in the wider integrations suite:

$ pytest tests/unittests/integrations/ -q
440 passed

Manual End-to-End (E2E) Tests:

Not run. Exercising this path end to end requires a live Daytona account and API key to provision a real remote sandbox, which is not available in this environment. The change is confined to the client-teardown step of close() and is covered by the unit test above.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (Requires a live Daytona account/API key; not available. See note above.)
  • Any dependent changes have been merged and published in downstream modules. (None; no new dependency.)

Additional context

DaytonaEnvironment is a recently added integration (feat(integrations): Add DaytonaEnvironment for remote sandbox workspaces, commit df6baf4a). This is a small follow-up correctness fix to its teardown path.

@rohityan rohityan self-assigned this Jul 6, 2026
@rohityan rohityan added integrations request clarification [Status] The maintainer need clarification or more information from the author labels Jul 6, 2026
@rohityan

rohityan commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Hi @anxkhn , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing ci mypy check tests before we can proceed with the review.

@anxkhn
anxkhn force-pushed the fix/daytona-close-client branch from 62abbe0 to bbb8e43 Compare July 9, 2026 17:12
@adk-bot adk-bot added the tools [Component] This issue is related to tools label Jul 11, 2026
@anxkhn
anxkhn force-pushed the fix/daytona-close-client branch from bbb8e43 to e472c95 Compare July 17, 2026 16:34
…ose()

Await the AsyncDaytona client close method after deleting its sandbox so repeated environment lifecycles do not leak HTTP sessions. Verify that close is awaited exactly once.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
@anxkhn
anxkhn force-pushed the fix/daytona-close-client branch from e472c95 to f5b1904 Compare July 27, 2026 14:41
@anxkhn

anxkhn commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

rebased onto current main. the original pr-specific mypy failure remains fixed with the targeted no-untyped-call suppression required by the unannotated daytona sdk method; the current file-only mypy output is otherwise present on main as well. the focused tests pass (15 passed), and pre-commit passes on both changed files. @rohityan, could you please take another look?

copybara-service Bot pushed a commit that referenced this pull request Jul 28, 2026
@adk-bot

adk-bot commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Thank you @anxkhn for your contribution! 🎉

Your changes have been successfully imported and merged via Copybara in commit ecf6d13.

Closing this PR as the changes are now in the main branch.

@adk-bot adk-bot added the merged [Status] This PR is merged label Jul 28, 2026
@adk-bot adk-bot closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrations merged [Status] This PR is merged request clarification [Status] The maintainer need clarification or more information from the author tools [Component] This issue is related to tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants