Skip to content

Commit 9738bc3

Browse files
committed
fix(tui): propagate aclose cancellation; dedupe modal suspend/restore
Address CodeRabbit review on #219: - PromptLifecycle.aclose no longer swallows CancelledError of aclose() itself: a wait_for-timed-out shutdown now propagates cancellation instead of continuing to close remaining resources past the deadline. Individual closers still own their internal cancellation. - Extract the duplicated modal suspend/restore transition shared by ModalAttached/ModalDetached into _suspend_restore_effects (behavior preserving).
1 parent b8d8ec1 commit 9738bc3

3 files changed

Lines changed: 61 additions & 25 deletions

File tree

src/pythinker_code/ui/shell/prompting/lifecycle.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,13 @@ async def aclose(self) -> None:
6060
try:
6161
await closer()
6262
except asyncio.CancelledError:
63-
continue
63+
# Cancellation here means aclose() itself was cancelled (e.g.
64+
# a wait_for timeout), not a closer's own internal cancel —
65+
# propagate it instead of swallowing and closing on regardless.
66+
logger.warning(
67+
"Prompt lifecycle aclose cancelled while closing resource={}", name
68+
)
69+
raise
6470
except Exception as exc:
6571
logger.warning(
6672
"Prompt lifecycle resource failed during shutdown: resource={} error={!r}",

src/pythinker_code/ui/shell/prompting/state.py

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,29 @@ def _mode_effects(mode: PromptMode) -> tuple[PromptEffect, ...]:
182182
return SelectCompleter(mode), SetEraseWhenDone(mode is PromptMode.AGENT), Invalidate()
183183

184184

185+
def _suspend_restore_effects(
186+
old_active: ModalState | None,
187+
new_active: ModalState | None,
188+
suspended: Document | None,
189+
document: Document,
190+
) -> tuple[Document | None, tuple[PromptEffect, ...]]:
191+
"""Compute the suspended-document/effects transition shared by modal
192+
attach and detach: suspend the live input when a hides-input modal takes
193+
over, restore it when the last such modal leaves."""
194+
old_hides_input = old_active is not None and old_active.hides_input
195+
new_hides_input = new_active is not None and new_active.hides_input
196+
if not old_hides_input and new_hides_input and document.text:
197+
if suspended is None:
198+
return document, (SuspendDocument(document),)
199+
return suspended, ()
200+
if old_hides_input and not new_hides_input and suspended is not None:
201+
effects: tuple[PromptEffect, ...] = (
202+
(RestoreDocument(suspended),) if not document.text else ()
203+
)
204+
return None, effects
205+
return suspended, ()
206+
207+
185208
def transition(state: PromptState, event: PromptEvent) -> PromptTransition:
186209
"""Return the next prompt state and ordered facade effects without doing I/O."""
187210
if isinstance(event, TurnStarting):
@@ -231,18 +254,9 @@ def transition(state: PromptState, event: PromptEvent) -> PromptTransition:
231254
ModalState(event.delegate, event.priority, event.hides_input),
232255
)
233256
new_active = _active_modal(stack)
234-
suspended = state.suspended_document
235-
effects: tuple[PromptEffect, ...] = ()
236-
old_hides_input = old_active is not None and old_active.hides_input
237-
new_hides_input = new_active is not None and new_active.hides_input
238-
if not old_hides_input and new_hides_input and event.document.text:
239-
if suspended is None:
240-
suspended = event.document
241-
effects = (SuspendDocument(event.document),)
242-
elif old_hides_input and not new_hides_input and suspended is not None:
243-
if not event.document.text:
244-
effects = (RestoreDocument(suspended),)
245-
suspended = None
257+
suspended, effects = _suspend_restore_effects(
258+
old_active, new_active, state.suspended_document, event.document
259+
)
246260
next_state = replace(
247261
state,
248262
modal_stack=stack,
@@ -257,18 +271,9 @@ def transition(state: PromptState, event: PromptEvent) -> PromptTransition:
257271
old_active = _active_modal(state.modal_stack)
258272
stack = tuple(modal for modal in state.modal_stack if modal.delegate is not event.delegate)
259273
new_active = _active_modal(stack)
260-
suspended = state.suspended_document
261-
effects: tuple[PromptEffect, ...] = ()
262-
old_hides_input = old_active is not None and old_active.hides_input
263-
new_hides_input = new_active is not None and new_active.hides_input
264-
if not old_hides_input and new_hides_input and event.document.text:
265-
if suspended is None:
266-
suspended = event.document
267-
effects = (SuspendDocument(event.document),)
268-
elif old_hides_input and not new_hides_input and suspended is not None:
269-
if not event.document.text:
270-
effects = (RestoreDocument(suspended),)
271-
suspended = None
274+
suspended, effects = _suspend_restore_effects(
275+
old_active, new_active, state.suspended_document, event.document
276+
)
272277
next_state = replace(state, modal_stack=stack, suspended_document=suspended)
273278
return PromptTransition(next_state, (*effects, Invalidate()))
274279

tests/ui_and_conv/test_prompt_lifecycle.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,31 @@ async def refused() -> None:
9797
lifecycle.create_task(refused())
9898

9999

100+
@pytest.mark.asyncio
101+
async def test_aclose_cancellation_propagates_and_halts_remaining_closers() -> None:
102+
# If aclose() itself is cancelled (e.g. a wait_for timeout) while awaiting a
103+
# closer, the cancellation must propagate — not be swallowed so shutdown
104+
# keeps closing the remaining resources past the caller's deadline.
105+
lifecycle = PromptLifecycle()
106+
earlier_closer_ran = False
107+
108+
async def blocking_closer() -> None:
109+
await asyncio.Event().wait()
110+
111+
async def earlier_closer() -> None:
112+
nonlocal earlier_closer_ran
113+
earlier_closer_ran = True
114+
115+
# Closers run in reverse registration order, so "blocking" runs first.
116+
lifecycle.register_closer("earlier", earlier_closer)
117+
lifecycle.register_closer("blocking", blocking_closer)
118+
119+
with pytest.raises(asyncio.TimeoutError):
120+
await asyncio.wait_for(lifecycle.aclose(), timeout=0.05)
121+
122+
assert earlier_closer_ran is False
123+
124+
100125
class _BlockingStdout:
101126
def __init__(self) -> None:
102127
self.read_started = asyncio.Event()

0 commit comments

Comments
 (0)