Skip to content

Commit 62ea250

Browse files
Add safe recovery choices after resolution cancellation (#1237)
* docs: design cancellable resolution changes Reuse analysis: extend the existing resolution request, stage movement, Stop Stage, model event queue, and PopUp lifecycles. Do not add a parallel stage executor, stop path, or public cancellation wrapper. * docs: plan issue 486 resolution cancellation Reuse analysis: extend the existing resolution, stage movement, Stop Stage, model event queue, and PopUp lifecycles. Reject controller-only cancellation and a parallel emergency stage executor. * fix: dispatch stage stops without GUI delay * fix: make resolution stage moves cancellable Reuse analysis: - Extended Microscope.move_stage, move_stage_offset, and stop_stage. - A second movement executor would duplicate stage mapping, limit enforcement, and device ownership. - Updated Numpydoc and focused movement tests. * fix: cancel resolution changes before stopping stages Reuse analysis: - Kept Model.change_resolution and Model.stop_stage as canonical entry points. - Reused the model event queue and existing stage movement methods. - Added only private task state; no parallel public executor or stop API. * fix: terminate cancelled resolution features safely Reuse analysis: - ChangeResolution now participates in the model-owned resolution lifecycle. - No feature-specific stop path or duplicate acquisition cleanup API was added. - Focused tests verify acquisition preparation does not resume after cancellation. * fix: preserve stop delivery across failure modes Reuse analysis: - Refined the existing Controller.stop_stage retry to recognize only ObjectInSubprocess contention. - Kept hardware failures on the normal error path. - Extended Model.stop_stage best-effort handling so one microscope or readback failure cannot skip other stop attempts. * docs: record issue 486 core verification * docs: record issue 486 core publication * test: cover both Stop Stage button bindings Reuse analysis: exercises the existing StageController stop handler and Controller acquisition-bar binding with test-only headless button doubles. No public API or production lifecycle is added. * feat: add cancellable resolution position recovery Reuse analysis: - Return movement uses Model.move_stage and the existing Stop Stage lifecycle. - Existing stage verification enforces the saved coordinates and configured limits. - No recovery-specific stage executor, limit validator, or stop API was added. * feat: offer safe resolution-change recovery choices Reuse analysis: the dialog uses Navigate's existing PopUp, event queue, model thread pool, stage-control disablement, and Stop Stage lifecycle. It introduces no parallel movement or stop executor. * fix: restore controls after rejected resolution return Reuse analysis: rejected and completed returns publish the existing resolution_return_complete event. Validation continues through each stage's existing strict verify_abs_position contract; no alternate limit or UI recovery path was added. * docs: record issue 486 recovery publication * fix: wait for stopped-position readback before recovery Reuse analysis: extends the existing private resolution task and Stop Stage lifecycle with one completion event. It preserves the canonical stop, readback, validation, event, and return paths without adding another stage executor or public API. * Fix resolution recovery and acquisition stop regressions --------- Co-authored-by: Annie Wang <annie.xd.wang@gmail.com>
1 parent 092f090 commit 62ea250

11 files changed

Lines changed: 1065 additions & 50 deletions

‎docs/superpowers/plans/2026-08-04-issue-486-resolution-change-cancellation.md‎

Lines changed: 62 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ Push `kdean/issue-486-stop-resolution-change` and open a draft PR targeting `dev
519519
- Consumes: core terminal `resolution_change_cancelled` event and Task 2 cancellation-aware `move_stage`.
520520
- Produces: `Model.run_command("resolution_recovery", task_id, choice)` accepting literal choices `"keep"` and `"return"`; terminal `resolution_return_complete` event.
521521

522-
- [ ] **Step 1: Write failing recovery tests**
522+
- [x] **Step 1: Write failing recovery tests**
523523

524524
```python
525525
def test_keep_resolution_position_discards_snapshot_without_motion():
@@ -542,11 +542,11 @@ def test_return_moves_to_literal_snapshot_and_can_be_cancelled():
542542
assert model.return_stage.stop_called.is_set()
543543
```
544544

545-
- [ ] **Step 2: Run recovery tests and verify RED**
545+
- [x] **Step 2: Run recovery tests and verify RED**
546546

547547
Run the two named tests. Expected: the recovery command and snapshot store are absent.
548548

549-
- [ ] **Step 3: Add recovery ownership and validation**
549+
- [x] **Step 3: Add recovery ownership and validation**
550550

551551
On terminal cancellation, retain an immutable copy of the pre-movement coordinates in a separate private recovery record after clearing the active task. Mark Return eligible only when every requested axis passes the affected stage's existing strict absolute-position validation. A newer resolution task invalidates an older recovery record.
552552

@@ -559,7 +559,7 @@ class _ResolutionRecovery:
559559
return_allowed: bool
560560
```
561561

562-
- [ ] **Step 4: Implement keep and return commands**
562+
- [x] **Step 4: Implement keep and return commands**
563563

564564
Keep atomically clears the matching snapshot and emits no movement. Return validates task ID and eligibility, creates a `returning` task before starting its worker, clears the popup snapshot exactly once, calls the existing cancellation-aware `Model.move_stage(..., wait_until_done=True)`, updates actual positions, and emits `resolution_return_complete` with success or cancellation.
565565

@@ -574,11 +574,11 @@ elif command == "resolution_recovery":
574574
self.event_queue.put(("warning", f"Unknown resolution recovery: {choice}"))
575575
```
576576

577-
- [ ] **Step 5: Run all model recovery and core tests**
577+
- [x] **Step 5: Run all model recovery and core tests**
578578

579579
Run `test/model/test_resolution_change.py`, the existing `test_change_resolution`, and the concurrency guard. Expected: pass.
580580

581-
- [ ] **Step 6: Commit recovery movement**
581+
- [x] **Step 6: Commit recovery movement**
582582

583583
Create branch `kdean/issue-486-resolution-recovery` from the core branch before this commit, then:
584584

@@ -601,7 +601,7 @@ git commit -m "feat: add cancellable resolution position recovery" -m \
601601
- Consumes: `resolution_change_cancelled` and `resolution_return_complete` events; Task 6 recovery command.
602602
- Produces: `ResolutionChangeCancelledPopup(root, keep_command, return_command, return_enabled)`; controller handlers `_show_resolution_change_cancelled(payload)` and `_finish_resolution_return(payload)`.
603603

604-
- [ ] **Step 1: Write failing popup callback tests without Tk construction**
604+
- [x] **Step 1: Write failing popup callback tests without Tk construction**
605605

606606
```python
607607
def test_popup_close_keeps_current_position():
@@ -622,15 +622,15 @@ def test_popup_return_dismisses_before_starting_motion():
622622
assert order == ["dismiss", "return"]
623623
```
624624

625-
- [ ] **Step 2: Write failing controller event tests**
625+
- [x] **Step 2: Write failing controller event tests**
626626

627627
Patch the popup class with a complete fake exposing `showup`/`dismiss`. Assert that two events with the same task ID create one popup; Keep dispatches `("resolution_recovery", task_id, "keep")`; Return dispatches `"return"`; and `resolution_return_complete` clears the tracked popup/task state.
628628

629-
- [ ] **Step 3: Run popup/controller tests and verify RED**
629+
- [x] **Step 3: Run popup/controller tests and verify RED**
630630

631631
Run only the new popup tests and named controller tests. Expected: imports and event handlers are absent.
632632

633-
- [ ] **Step 4: Implement the view**
633+
- [x] **Step 4: Implement the view**
634634

635635
Build a `PopUp` titled `Resolution Change Cancelled` with the approved explanatory text and buttons `Keep Current Position` and `Return to Previous Position`. Focus Keep, bind Escape and `WM_DELETE_WINDOW` to `_keep`, disable Return when `return_enabled` is false, and dismiss before either callback. Do not start model work from the view itself.
636636

@@ -645,7 +645,7 @@ def _return(self) -> None:
645645
self._return_command()
646646
```
647647

648-
- [ ] **Step 5: Implement controller event handling**
648+
- [x] **Step 5: Implement controller event handling**
649649

650650
Register the two event handlers through the existing event-listener dictionary. Track the displayed task ID to deduplicate and dispatch choices through the existing model thread pool. Before Return, call `stage_controller.view.toggle_button_states(True, stage_controller.stage_axes)` after the modal closes; the separate Stop Stage frame remains enabled. On `resolution_return_complete`, call `stage_controller.force_enable_all_axes()` and clear the tracked task ID.
651651

@@ -662,11 +662,11 @@ def _return_resolution_position(self, task_id: int) -> None:
662662
)
663663
```
664664

665-
- [ ] **Step 6: Run popup/controller tests and verify GREEN**
665+
- [x] **Step 6: Run popup/controller tests and verify GREEN**
666666

667667
Run the Step 3 tests. Expected: pass without creating `tk.Tk()` locally.
668668

669-
- [ ] **Step 7: Commit the recovery UI**
669+
- [x] **Step 7: Commit the recovery UI**
670670

671671
```bash
672672
git add src/navigate/view/popups/resolution_change_popup.py \
@@ -685,11 +685,11 @@ git commit -m "feat: offer safe resolution-change recovery choices"
685685
- Consumes: Tasks 6-7 and the core PR branch.
686686
- Produces: a focused stacked recovery PR whose base is the core branch until the core merges.
687687

688-
- [ ] **Step 1: Run formatting, lint, and focused tests**
688+
- [x] **Step 1: Run formatting, lint, and focused tests**
689689

690690
Run Ruff and Black on all recovery-modified files. Run the full model resolution test file, feature test, controller tests, and popup callback tests.
691691

692-
- [ ] **Step 2: Run a native GUI smoke check outside Aqua**
692+
- [x] **Step 2: Run a native GUI smoke check outside Aqua**
693693

694694
Use Windows CI or a Linux X11 Tk environment:
695695

@@ -700,7 +700,7 @@ xvfb-run -a /opt/anaconda3/envs/navigate/bin/python -m pytest -o addopts='' -q \
700700

701701
If the available environment is macOS Aqua Tk, skip window construction and explicitly report that Xvfb cannot isolate it.
702702

703-
- [ ] **Step 3: Review the stacked diff**
703+
- [x] **Step 3: Review the stacked diff**
704704

705705
```bash
706706
git diff kdean/issue-486-stop-resolution-change --check
@@ -710,7 +710,7 @@ git log --oneline kdean/issue-486-stop-resolution-change..HEAD
710710

711711
Confirm that the recovery PR contains no duplicate stage executor, stop path, or acquisition restart.
712712

713-
- [ ] **Step 4: Push and open the recovery PR**
713+
- [x] **Step 4: Push and open the recovery PR**
714714

715715
Push `kdean/issue-486-resolution-recovery` and open a draft PR based on `kdean/issue-486-stop-resolution-change`. Link the design, plan, and core PR; list test evidence and Xvfb limitation; request the designated reviewer if known; and reference #486 without a second closing directive.
716716

@@ -767,3 +767,48 @@ Commit only the new test and the design/plan updates, then push the commit as a
767767
fast-forward update to `kdean/issue-486-stop-resolution-change`. Confirm the
768768
live PR head and report fresh CI status without treating absent or pending jobs
769769
as success.
770+
771+
### Task 10: Wait for Stop Stage readback before offering Return
772+
773+
**Files:**
774+
- Modify: `src/navigate/model/resolution_change.py`
775+
- Modify: `src/navigate/model/model.py`
776+
- Modify: `test/model/test_resolution_change.py`
777+
- Modify: `docs/superpowers/specs/2026-08-04-issue-486-resolution-change-cancellation-design.md`
778+
- Modify: `docs/superpowers/plans/2026-08-04-issue-486-resolution-change-cancellation.md`
779+
780+
**Interfaces:**
781+
- Consumes: `_ResolutionChangeTask.cancel_event`, `Model.stop_stage`, and `Model._finish_resolution_change`.
782+
- Produces: task-owned stop-completion synchronization that preserves immediate cancellation but defers recovery validation and notification until stopped-position readback completes.
783+
784+
- [x] **Step 1: Reproduce the hardware ordering failure**
785+
786+
Block the real `Model.stop_stage` path inside a microscope double while a second
787+
thread finalizes the cancelled resolution task. Assert that no cancellation
788+
event is emitted until the stop is released, then require the emitted payload to
789+
contain the stopped position and enable Return.
790+
791+
- [x] **Step 2: Verify RED**
792+
793+
Run the named concurrency test against the current PR #1237 implementation.
794+
Expected: failure because `resolution_change_cancelled` is emitted while the
795+
hardware stop double is still blocked.
796+
797+
- [x] **Step 3: Synchronize cancellation finalization**
798+
799+
Add a task-owned `stop_complete_event` that is initially set, clear it before
800+
publishing cancellation, and set it in a `finally` block after every stop and
801+
readback outcome. In cancelled resolution finalization, wait for that signal
802+
before validating the recovery snapshot or emitting the popup event.
803+
804+
- [x] **Step 4: Verify GREEN and surrounding safety behavior**
805+
806+
Run the named regression, the complete resolution-change test module, the
807+
controller and popup recovery tests, both Stop Stage click tests, Ruff, Black
808+
checks scoped against the stacked diff, and `git diff --check`.
809+
810+
- [x] **Step 5: Commit and update PR #1237**
811+
812+
Commit only the synchronization fix, its regression coverage, and the design
813+
records. Push the existing recovery branch and verify the live head, stacked
814+
base, mergeability, and CI state before asking for another hardware test.

‎docs/superpowers/specs/2026-08-04-issue-486-resolution-change-cancellation-design.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,8 @@ Core tests cover:
191191
- feature-driven resolution changes terminating their feature/acquisition path;
192192
- model-proxy contention retrying instead of dropping Stop Stage;
193193
- stopped-position readback and event payload correctness; and
194+
- cancellation recovery waiting for stop attempts and position readback before
195+
validating or displaying Return; and
194196
- termination cancelling and joining outstanding workers.
195197

196198
Recovery tests cover:
@@ -254,6 +256,8 @@ second closing directive.
254256
- No resolution or acquisition operation schedules new motion after
255257
cancellation.
256258
- The GUI shows the actual stopped stage coordinates.
259+
- The recovery dialog is not emitted until hardware stop attempts and stopped-position
260+
readback finish.
257261
- Recovery movement never starts without explicit Return selection.
258262
- Close, Escape, and Keep leave the stages stopped where they are.
259263
- Return uses the recorded pre-movement coordinates and can be stopped again.

‎pytest.ini‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ filterwarnings =
99
ignore:'resetCache' deprecated - use 'reset_cache':DeprecationWarning
1010
ignore:'enablePackrat' deprecated - use 'enable_packrat':DeprecationWarning
1111
markers =
12+
flaky: mark tests that may be rerun by pytest-rerunfailures
1213
hardware: mark tests as run on physical hardware
1314
integration: mark tests that rely on external integrations (network/pip)
1415
log_cli = true

‎src/navigate/controller/controller.py‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@
4646
from navigate.view.main_application_window import MainApp as view
4747
from navigate.view.popups.camera_view_popup_window import CameraViewPopupWindow
4848
from navigate.view.popups.feature_list_popup import FeatureListPopup
49+
from navigate.view.popups.resolution_change_popup import (
50+
ResolutionChangeCancelledPopup,
51+
)
4952
from navigate.view.theme import apply_theme
5053

5154
# Local Sub-Controller Imports
@@ -281,6 +284,15 @@ def __init__(
281284
)
282285
}
283286

287+
#: Optional[int]: resolution-change task awaiting a recovery choice.
288+
self._resolution_recovery_task_id = None
289+
290+
#: Optional[int]: last completed resolution-change task id.
291+
self._last_completed_resolution_task_id = None
292+
293+
#: Optional[ResolutionChangeCancelledPopup]: active recovery dialog.
294+
self._resolution_change_popup = None
295+
284296
#: AcquireBarController: Acquire Bar Sub-Controller.
285297
self.acquire_bar_controller = AcquireBarController(self.view.acquire_bar, self)
286298

@@ -1881,6 +1893,108 @@ def stop_stage(self) -> None:
18811893
# must be retried instead of disappearing in the thread pool.
18821894
time.sleep(0.001)
18831895

1896+
def _show_resolution_change_cancelled(self, payload: dict[str, Any]) -> None:
1897+
"""Show one recovery dialog for a cancelled resolution-change task.
1898+
1899+
Parameters
1900+
----------
1901+
payload : dict[str, Any]
1902+
Cancellation event containing the task identifier and whether a
1903+
validated pre-movement position is available.
1904+
1905+
Returns
1906+
-------
1907+
None
1908+
"""
1909+
task_id = payload["task_id"]
1910+
if self._resolution_recovery_task_id == task_id:
1911+
return
1912+
1913+
if self._last_completed_resolution_task_id == task_id:
1914+
return
1915+
1916+
if self._resolution_change_popup is not None:
1917+
try:
1918+
self._resolution_change_popup.popup.dismiss()
1919+
except tkinter.TclError:
1920+
pass
1921+
1922+
self._resolution_recovery_task_id = task_id
1923+
self._resolution_change_popup = ResolutionChangeCancelledPopup(
1924+
root=self.view.root,
1925+
keep_command=lambda: self._keep_resolution_position(task_id),
1926+
return_command=lambda: self._return_resolution_position(task_id),
1927+
return_enabled=bool(payload.get("return_allowed", False)),
1928+
)
1929+
1930+
def _keep_resolution_position(self, task_id: int) -> None:
1931+
"""Accept the stopped stage position without further movement.
1932+
1933+
Parameters
1934+
----------
1935+
task_id : int
1936+
Resolution-change task whose recovery choice is being resolved.
1937+
1938+
Returns
1939+
-------
1940+
None
1941+
"""
1942+
if self._resolution_recovery_task_id != task_id:
1943+
return
1944+
self._resolution_change_popup = None
1945+
self._resolution_recovery_task_id = None
1946+
self.threads_pool.createThread(
1947+
resourceName="model",
1948+
target=lambda: self.model.run_command(
1949+
"resolution_recovery", task_id, "keep"
1950+
),
1951+
)
1952+
1953+
def _return_resolution_position(self, task_id: int) -> None:
1954+
"""Start a separately cancellable return to the saved stage position.
1955+
1956+
Parameters
1957+
----------
1958+
task_id : int
1959+
Resolution-change task whose recovery choice is being resolved.
1960+
1961+
Returns
1962+
-------
1963+
None
1964+
"""
1965+
if self._resolution_recovery_task_id != task_id:
1966+
return
1967+
self._resolution_change_popup = None
1968+
# Disable ordinary moves during the return; Stop Stage remains available.
1969+
self.stage_controller.view.toggle_button_states(
1970+
True, self.stage_controller.stage_axes
1971+
)
1972+
self.threads_pool.createThread(
1973+
resourceName="model",
1974+
target=lambda: self.model.run_command(
1975+
"resolution_recovery", task_id, "return"
1976+
),
1977+
)
1978+
1979+
def _finish_resolution_return(self, payload: dict[str, Any]) -> None:
1980+
"""Restore stage controls when return motion reaches a terminal state.
1981+
1982+
Parameters
1983+
----------
1984+
payload : dict[str, Any]
1985+
Completion event containing the original resolution-change task id.
1986+
1987+
Returns
1988+
-------
1989+
None
1990+
"""
1991+
if payload.get("task_id") != self._resolution_recovery_task_id:
1992+
return
1993+
self._last_completed_resolution_task_id = self._resolution_recovery_task_id
1994+
self.stage_controller.force_enable_all_axes()
1995+
self._resolution_change_popup = None
1996+
self._resolution_recovery_task_id = None
1997+
18841998
def update_stage_limits(self, microscope_name: str) -> None:
18851999
"""Update stage limits on the device side
18862000
@@ -2014,6 +2128,12 @@ def update_event(self) -> None:
20142128
elif event == "autofocus_sequence_complete":
20152129
self._set_autofocus_state(False)
20162130

2131+
elif event == "resolution_change_cancelled":
2132+
self._show_resolution_change_cancelled(value)
2133+
2134+
elif event == "resolution_return_complete":
2135+
self._finish_resolution_return(value)
2136+
20172137
elif event in self.event_listeners.keys():
20182138
try:
20192139
self.event_listeners[event](value)

0 commit comments

Comments
 (0)