refactor: session handler lifecycle (cancel, busy-gate, exception cleanup)#14
Merged
Conversation
…d exception cleanup Three coupled gaps in ThinkingPromptSession's input lifecycle: 1. Ctrl+C did not cancel the running handler. The key binding finished active thinking boxes and cancelled the pending-input future, but the handler coroutine was awaited inline with no task handle, so a long- running handler kept executing after the UI signaled "cancel" and could still emit a final response. 2. Input submitted while a handler was running was silently dropped. accept_handler echoed the text to history, cleared the buffer, and tried to deliver it via _pending_input — but _pending_input was already done (the previous prompt had resolved it), so the new text never reached input_loop. The user lost the command with no signal that anything had gone wrong. 3. Handler exceptions left thinking boxes stuck active. The except path logged the error but didn't finish boxes the failing handler had created; for callers using start_thinking() (manual API) the UI was permanently in "thinking" state until restart. Solution: - Add ThinkingPromptSession._run_handler(handler, text) that runs the handler as a tracked asyncio.Task stored on _current_handler_task. CancelledError flagged via _user_cancelled_handler is treated as a Ctrl+C (swallowed, boxes finished, loop continues); other Cancellations propagate so the input loop can exit cleanly. Exceptions trigger _cleanup_after_handler() before being logged. - Ctrl+C key binding cancels _current_handler_task (after marking _user_cancelled_handler), finishes boxes, cancels pending input, and only falls through to app.exit() when nothing is in flight. - accept_handler refuses to deliver input while _current_handler_task is running: leaves the buffer intact, sets a "Busy — press Ctrl+C to cancel" status hint, and returns False so prompt_toolkit does not echo or clear. - input_loop simplifies to "await self._run_handler(...)"; all retry / cleanup / cancellation logic now lives in one place. Adds tests/test_session_lifecycle.py with 9 tests covering each path: async/sync handler tracking, exception cleanup, user vs outer cancel, busy-input drop, and the Ctrl+C key binding (cancels handler, exits when idle).
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
1 participant
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.
Summary
Closes the three session-lifecycle findings from review feedback. The handler invocation was previously awaited inline with no task handle, no busy-state, and no exception cleanup; this PR models all three explicitly.
What changed
_current_handler_task(via_user_cancelled_handlerflag so the inner cancellation is distinguishable from outer shutdown)_pending_inputwas already done → silently lostaccept_handlerreturnsFalse(keeps buffer), shows"Busy — press Ctrl+C to cancel"status, leaves text editable_cleanup_after_handler()finishes any open boxes before re-promptingHow
ThinkingPromptSession._run_handler(handler, text)is the single owner of handler execution: wraps async handlers inasyncio.create_task, stores the task on_current_handler_task, distinguishes user cancellation (_user_cancelled_handler) from outer shutdown (re-raised), and runs_cleanup_after_handler()on every terminal path._user_cancelled_handler = Truebefore callingtask.cancel(), then finishes boxes / cancels pending input, falling through toapp.exit()only when nothing is in flight.input_loopsimplifies toawait self._run_handler(...); the previous inline try/except for handler exceptions and CancelledError is gone.Test plan
tests/test_session_lifecycle.py— 9 new tests (all initially failing, now green):_pending_inputnot resolved), idle-input deliveredpytest: 364 passed (was 355, +9)ruff check thinking_prompt/: cleanmypy thinking_prompt/: clean