fix: Ctrl+C double-action and sticky _user_cancelled_handler flag#15
Merged
Merged
Conversation
Two follow-up bugs in the session-lifecycle refactor:
1. Ctrl+C exited the app after clearing an orphan thinking box.
The Ctrl+C binding finished active boxes via finish_all() and then
re-checked self._manager.has_active_boxes for the "idle, exit"
fallback. After finish_all the manager is empty, so the same Ctrl+C
that cleared the boxes also called event.app.exit().
Snapshot handler_running / had_active_boxes / had_pending_input
before mutating any of them, and only fall through to exit when none
of them were in flight at keypress time. has_active_boxes is also
the right idle signal for boxes opened outside a handler (which the
reviewer flagged), since previously such boxes would be cleared but
the user would also lose their session.
2. _user_cancelled_handler stayed True if the inner cancel did not
propagate as CancelledError out of `await task`.
The flag was only reset inside `except asyncio.CancelledError`, so:
- if user code caught CancelledError and returned normally, the
outer await returned a value (no exception) and the flag stuck;
- if cancel raced against natural completion and lost, same thing.
In both cases the *next* handler invocation that received an outer
cancellation (e.g. session shutdown) would have the leftover True
flag, _run_handler would treat it as Ctrl+C, swallow CancelledError,
and the input loop could not exit.
Reset _user_cancelled_handler in `finally` so it's always cleared
regardless of whether CancelledError propagated. The except branch
no longer needs to reset it explicitly.
Tests: extends test_session_lifecycle.py with one Ctrl+C test
(orphan box must not also exit) and three flag-reset tests:
- handler swallows CancelledError (uses a `started` event so cancel
arrives during await rather than before first tick — otherwise the
task is cancelled before its body runs and the bug is masked);
- cancel races against natural completion;
- outer cancel after a swallowing handler must propagate.
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
Two follow-up bugs in the lifecycle refactor (#14), surfaced by review feedback:
1. Ctrl+C exits after clearing an orphan thinking box
The cancel binding finished active boxes with
finish_all()and then re-checkedself._manager.has_active_boxesfor its "idle, exit" fallback. Afterfinish_all()the manager is empty, so the same Ctrl+C that cleared the orphan box also calledevent.app.exit(). Users with a box left open outside a handler (or by an earlier handler) would lose their session.Fix: snapshot
handler_running/had_active_boxes/had_pending_inputbefore mutating any of them, and fall through toexit()only when none of them were in flight at keypress time.2.
_user_cancelled_handlerstays True if the cancel doesn't propagateThe flag was only reset inside
except asyncio.CancelledError. Two paths leave it sticky:CancelledErrorand returns normally → outerawait taskreturns a value, no exception, flag stays True;The next handler invocation that received an outer cancellation (e.g. shutdown) would then see the leftover True flag, get treated as Ctrl+C, and the input loop couldn't exit.
Fix: reset
_user_cancelled_handlerinfinallyso it's always cleared.Note on test fix
The original
test_flag_reset_when_handler_swallows_cancel(added in #14) wasn't actually exercising user-suppression — it cancelled the task before its body ran, so the task was cancelled at first tick and the outer await did raiseCancelledError, hiding the bug. This PR uses an explicitstartedevent so cancel only arrives once the handler is awaiting its gate, exposing the original sticky-flag bug.Test plan
pytest: 368 passed (was 364)ruff check thinking_prompt/: cleanmypy thinking_prompt/: clean