Fix worker thread type checking and increase coverage to 96%

JoshCLWren · JoshCLWren · commit 1c8d69905e36 · 2025-12-27T17:53:12.000-06:00
- Add assert app is not None after null check to satisfy pyright
- Add source=None check in worker loop for graceful shutdown
- Add test_worker_handles_none_source_gracefully to cover shutdown path
- Coverage now at 96.00% (target reached)
- All linting passes with 0 type checking errors
diff --git a/SESSION_SUMMARY.md b/SESSION_SUMMARY.md
@@ -0,0 +1,153 @@
+# CI Threading Crash Fix - Session Summary
+
+## Problem
+CI tests were failing with threading crashes during pytest collection/execution. Threads were stuck in `queue.get()` calls causing Python to abort.
+
+```
+Fatal Python error: Aborted
+Thread X: File "queue.py", line 180 in get
+         File "cdisplayagain.py", line 424 in _run
+```
+
+## Attempted Solutions
+
+### 1. Try/Finally Blocks ✅ (Commit: e179690)
+- Added `try/finally` blocks to 16 tests across 3 files
+- Ensures `worker.stop()` is called before Tk root destruction
+- **Result**: Tests pass locally, CI still crashes
+- **Issue**: Too verbose, not Pythonic
+
+### 2. Context Manager ✅ (Commit: c19ea81)
+- Converted `ImageWorker` to a context manager
+- Added `__enter__` and `__exit__` methods
+- Updated tests to use `with ImageWorker() as worker:` pattern
+- **Result**: Tests pass locally (411 passed, 95.99% coverage)
+- **Issue**: CI still crashes with same threading errors
+
+### 3. Worker Thread Safety Improvements
+- Added `_stopped` flag to `ImageWorker`
+- Added early exit checks in `_run()` loop
+- Set `self._app = None` in `stop()` to prevent access after cleanup
+- Added exception handling in `thread.join()`
+- **Current State**: Tests pass locally, but:
+  - Pyright type checking fails on 2 lines (app._app could be None)
+  - CI is likely still failing
+
+## Root Cause Analysis
+
+The issue appears to be that even with context managers, worker threads are processing tasks when the Tk root is destroyed during test teardown. The threads are:
+1. Stuck in `queue.get()` waiting for work
+2. Or in the middle of processing (calling `get_resized_pil()`)
+3. Trying to access Tk (`after_idle`) when Tk is already destroyed
+
+## Current Code State
+
+### ImageWorker Changes
+```python
+class ImageWorker:
+    _app: ComicViewer | None = None
+
+    def __init__(self, app, num_workers: int = 4):
+        self._app = app
+        self._stopped: bool = False
+        # ... start threads
+
+    def stop(self):
+        self._stopped = True
+        self._app = None  # Prevent access after cleanup
+        # ... send stop signals to queue
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self.stop()
+        return False
+```
+
+### ComicViewer Changes
+```python
+class ComicViewer(tk.Frame):
+    def __del__(self):
+        self.cleanup()
+
+    def cleanup(self):
+        if hasattr(self, "_worker") and self._worker:
+            self._worker.stop()
+```
+
+## Remaining Issues
+
+### Type Checking Errors ✅ FIXED
+- Added `assert app is not None` after null check to satisfy pyright
+- Added `if source is None: break` check to handle None source gracefully
+- All type checking errors resolved (0 errors, 0 warnings, 0 informations)
+
+### Coverage ✅ FIXED
+- Current: 96.00%
+- Target: 96%
+- Gap: 0% (target reached!)
+- Added test `test_worker_handles_none_source_gracefully` to cover source=None path
+
+## Files Modified
+
+### cdisplayagain.py
+- Added context manager methods to `ImageWorker`
+- Added `_stopped` flag for graceful shutdown
+- Added `cleanup()` method to `ComicViewer`
+- Modified `_run()` to check `_stopped` flag multiple times
+- Modified `stop()` to handle `queue.Full` and set `_app = None`
+- ✅ Added `assert app is not None` after null check (fixes pyright errors)
+- ✅ Added `if source is None: break` check (handles shutdown gracefully)
+
+### tests/test_parallel_workers.py
+- Converted 10 tests from `try/finally` to context manager pattern
+- ✅ Added `test_worker_handles_none_source_gracefully` to increase coverage to 96%
+
+### tests/test_threading.py
+- Converted 4 tests from `try/finally` to context manager pattern
+
+### tests/test_benchmark_parallel.py
+- Converted 2 tests from `try/finally` to context manager pattern
+
+### tests/conftest.py
+- Tried adding autouse fixture to track/cleanup all viewers (reverted - caused issues)
+
+## Next Steps
+
+1. ~~**Fix type checking errors**: Use local variable `app = self._app` after null check to satisfy pyright~~ ✅ DONE
+2. **Investigate CI-specific issue**: The threading crash still occurs in CI but not locally - suggests:
+   - Timing difference in CI environment
+   - Different garbage collection behavior
+   - xvfb-run virtual display interaction
+   - ✅ Type checking and coverage are now fixed, ready to test on CI
+3. **Alternative approaches to consider if CI still fails**:
+   - Make workers non-daemon and ensure explicit cleanup
+   - Add a test-wide fixture that ensures all workers are stopped before any Tk root is destroyed
+   - Increase timeout in `thread.join()`
+   - Use a different shutdown mechanism (e.g., event instead of queue sentinel)
+
+## Commands to Run
+
+```bash
+# Run tests
+uv run pytest tests/ --ignore=tests/test_benchmark_parallel.py
+
+# Run linting
+make lint
+
+# Check type errors specifically
+uv run pyright cdisplayagain.py
+```
+
+## Git Status
+
+- Branch: `move-test-coverage-marker`
+- Last commits:
+  - e179690: Fix CI threading crash by stopping worker threads in tests (try/finally)
+  - c19ea81: Refactor worker cleanup to use context manager
+- Working tree: Dirty (uncommitted changes to cdisplayagain.py and tests/test_parallel_workers.py)
+  - Fixed type checking errors (assert app is not None)
+  - Added source=None check in worker loop
+  - Added test_worker_handles_none_source_gracefully test
+  - Coverage now at 96.00%, all linting passes
diff --git a/cdisplayagain.py b/cdisplayagain.py
@@ -380,11 +380,14 @@ def get_bytes(_: str) -> bytes:
 class ImageWorker:
     """Background thread pool for image processing."""
 
+    _app: ComicViewer | None = None
+
     def __init__(self, app, num_workers: int = 4):
         """Initialize worker pool with app reference and start daemon threads."""
         self._app = app
         self._queue = queue.PriorityQueue(maxsize=4)
         self._threads: list[threading.Thread] = []
+        self._stopped: bool = False
         for i in range(num_workers):
             thread = threading.Thread(target=self._run, daemon=True, name=f"ImageWorker-{i}")
             thread.start()
@@ -394,6 +397,8 @@ def request_page(
         self, index: int, width: int, height: int, preload: bool = False, render_generation: int = 0
     ):
         """Request a page be processed in background."""
+        if self._stopped or not self._app:
+            return
         try:
             priority = 1 if preload else 0
             self._queue.put_nowait((priority, index, width, height, preload, render_generation))
@@ -410,10 +415,21 @@ def preload(self, index: int):
 
     def stop(self):
         """Signal all worker threads to stop and wait for them to exit."""
+        self._stopped = True
+
+        self._app = None
+
         for _ in self._threads:
-            self._queue.put_nowait((2, None, None, None, None, None))
+            try:
+                self._queue.put_nowait((2, None, None, None, None, None))
+            except queue.Full:
+                break
+
         for thread in self._threads:
-            thread.join(timeout=1.0)
+            try:
+                thread.join(timeout=1.0)
+            except Exception:
+                pass
         self._threads.clear()
 
     def __enter__(self):
@@ -427,7 +443,7 @@ def __exit__(self, exc_type, exc_val, exc_tb):
 
     def _run(self):
         """Process resize requests in background."""
-        while True:
+        while not self._stopped:
             priority = None
             try:
                 priority, index, width, height, preload, render_generation = self._queue.get(
@@ -437,32 +453,49 @@ def _run(self):
                 if priority == 2:
                     break
 
+                if self._stopped or not self._app:
+                    break
+
+                app = self._app
+                if not app:
+                    break
+                assert app is not None
+
                 if preload:
                     logging.info("Worker preloading page %d at %dx%d", index, width, height)
                 else:
                     logging.info("Worker processing page %d at %dx%d", index, width, height)
 
-                if not preload and render_generation != self._app._render_generation:
+                if not preload and render_generation != app._render_generation:
                     logging.info(
                         "Worker cancelling stale render for page %d (gen %d != %d)",
                         index,
                         render_generation,
-                        self._app._render_generation,
+                        app._render_generation,
                     )
                     continue
 
-                raw = self._app.source.get_bytes(self._app.source.pages[index])
+                if self._stopped:
+                    break
+
+                source = app.source
+                if source is None:
+                    break
+                raw = source.get_bytes(source.pages[index])
                 resized_pil = get_resized_pil(raw, width, height)
 
+                if self._stopped:
+                    break
+
                 if preload:
                     logging.info("Worker finished preloading page %d", index)
                 else:
                     logging.info("Worker finished page %d, scheduling callback", index)
 
-                if self._app and hasattr(self._app, "after_idle"):
+                if app and hasattr(app, "after_idle"):
                     try:
-                        self._app.after_idle(
-                            lambda idx=index, img=resized_pil: self._app._update_from_cache(
+                        app.after_idle(
+                            lambda idx=index, img=resized_pil, app=app: app._update_from_cache(
                                 idx, img
                             )
                         )
@@ -502,7 +535,11 @@ class ComicViewer(tk.Frame):
 
     def __del__(self):
         """Cleanup worker threads on garbage collection."""
-        if hasattr(self, "_worker"):
+        self.cleanup()
+
+    def cleanup(self):
+        """Stop worker threads to prevent threading crashes during shutdown."""
+        if hasattr(self, "_worker") and self._worker:
             self._worker.stop()
 
     def __init__(self, master: tk.Tk, comic_path: Path):
diff --git a/tests/test_parallel_workers.py b/tests/test_parallel_workers.py
@@ -272,3 +272,32 @@ def capture_update(index, img):
         tk_root.mainloop()
 
         assert len(results) == 3, "Single worker should process all pages"
+
+
+def test_worker_handles_none_source_gracefully(tk_root, tmp_path):
+    """Test that worker handles None source gracefully during shutdown."""
+    cbz_path = tmp_path / "test.cbz"
+    create_test_cbz(cbz_path, page_count=5)
+
+    app = cdisplayagain.ComicViewer(tk_root, cbz_path)
+    with ImageWorker(app, num_workers=2) as worker:
+        results = []
+
+        def capture_update(index, img):
+            assert isinstance(img, Image.Image)
+            results.append((index, img.size))
+
+        app._update_from_cache = capture_update
+
+        worker.request_page(0, 100, 200, render_generation=0)
+
+        time.sleep(0.01)
+        app.source = None
+
+        for i in range(1, 3):
+            worker.request_page(i, 100, 200, render_generation=0)
+
+        tk_root.after(1000, tk_root.quit)
+        tk_root.mainloop()
+
+        assert len(results) >= 1, "Should process at least the first page before source is None"
