Repository navigation
fix(sdk-python): keep the async PTY session alive when a caller cancels wait() - #353
Open
TanishGudise wants to merge 1 commit into
Open
TanishGudise wants to merge 1 commit into
TanishGudise wants to merge 1 commit into
Conversation
…ls wait() AsyncPtyHandle.wait() awaited the handle's long-lived WebSocket reader task directly. In asyncio, cancelling a coroutine that awaits a task also cancels that task, so bounding the wait with asyncio.wait_for() or asyncio.timeout() cancelled the reader itself when the timeout fired: on_data stopped firing, is_connected() turned False, send_input() raised "PTY is not connected", the WebSocket was left open, and a later wait() raised CancelledError. Shield the reader task in wait() so cancelling the call only stops waiting. disconnect() still cancels the reader directly. Fixes daytona#351 Signed-off-by: Tanish Gudise <174986239+TanishGudise@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vidoc security reviewTip Good to merge — no security issues found. Reviewed 2 changed files. 💬 Have questions? Tag @vidoc in a comment and I'll answer. |
Contributor
|
All contributors have signed the CLA. ✅ Thank you! |
Author
|
I have read the CLA Document and I hereby sign the CLA |
Author
|
recheck |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
AsyncPtyHandle.wait()awaited the handle's long-lived WebSocket reader task directly. In asyncio, cancelling a coroutine that awaits a task also cancels that task, so bounding the wait withasyncio.wait_for()orasyncio.timeout()cancelled the reader when the timeout fired. After that:on_datastopped firingis_connected()returnedFalse, andsend_input()raised "PTY is not connected"wait()raisedCancelledErrorThis change awaits
asyncio.shield(self._wait), so cancellingwait()only stops that wait.disconnect()still cancels the reader directly, so teardown is unchanged. Thewait()docstring now notes the behavior.Test:
TestAsyncPtyHandleWaitCancellationinsdk-python/tests/test_pty_handle.pyconnects throughAsyncProcess.connect_pty_sessionto a local aiohttp WebSocket server that stays silent until the client writes, soasyncio.wait_for(handle.wait(), 0.05)can only time out. The test then checks that:pinggets apongback;exit,wait()returns exit code 0.On
mainit fails at theis_connected()check; with this change it passes on Python 3.10 and 3.12.Documentation
The
AsyncPtyHandle.wait()docstring is updated.AsyncPtyHandleisn't part of the generated SDK reference docs, so nothing needs regenerating.Related Issue(s)
Fixes #351
Screenshots
N/A
Notes
async_pty_handle.pyandtest_pty_handle.pybut doesn't changewait(), and this bug still reproduces on that branch. Thewait()change applies cleanly on top of feat(sdk): add process surface with handles, log replay and reconnect #191; the test file only conflicts in its import block, which is a trivial union.yarn lint:py(0 errors) and the pre-commit lint-staged hookstest_event_subscription_manager.py::TestSyncEventSubscriptionManagerThreadUsage::test_many_subscriptions_share_one_expiry_threadfails intermittently onmaintoo (a thread-count assertion; 2 of 8 local runs). It's unrelated to this change.🤖 Generated with Claude Code
Summary by cubic
Fixes cancelling
AsyncPtyHandle.wait()so aasyncio.wait_for()orasyncio.timeout()bound no longer kills the PTY session.wait()previously awaited the WebSocket reader task directly, so cancelling it stoppedon_datacallbacks, madeis_connected()returnFalse, causedsend_input()to raise "PTY is not connected", and left the WebSocket open. Nowwait()awaitsasyncio.shield(self._wait), so cancelling only stops the wait;disconnect()still tears down the reader as before.Adds a regression test covering the timeout path, connection health after cancellation, and a successful exit afterwards.
Written for commit 0686475. Summary will update on new commits.