Fix preview progress bar stranded on early-cancelled cold mount - #73
Conversation
The preview "loading" bar could stick forever until the user navigated to a different file and back. Root cause: in _mount_chunks_async the early awaits (container mount, prefetch cancel_task_on) ran BEFORE the try block and BEFORE the detached finalize task was spawned. The finalize task is the only thing that hides the bar and clears the inflight_target latch on success; cancel_mount_task hides nothing. So a mount cancelled in that early window skipped the finally entirely, leaving the bar up and the latch set — a same-file re-load then deduped out, and only a different-file nav cleared the latch and dispatched a completing load. The window widens for slow cold mounts (large PDFs) and under load, so it surfaced intermittently. Extend the try to cover the early awaits, and in the finally hide the bar + release the latch when the task ends before a finalize task existed and no successor mount took over. Regression test parks a mount on a blocked early await, cancels it, and asserts the bar is hidden and the latch clear.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesEarly-cancel strand fix in
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where cancelling a cold preview mount during its early-await phase (before the detached finalize task is spawned) would strand the progress bar and the inflight latch. This is addressed by wrapping the early awaits in the try block and adding cleanup logic in the finally block of _mount_chunks_async, along with a regression test. The reviewer suggests a more robust check in the finally block to ensure that the cleanup logic is not skipped if the task is cancelled or fails through paths other than cancel_mount_task().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| old.remove() | ||
| if container.is_complete: | ||
| self.hide_progress_bar() | ||
| elif getattr(container, "_finalize_task", None) is None and self.mount_task is None: |
There was a problem hiding this comment.
Checking self.mount_task is None to determine if a successor mount has taken over can be fragile. If the task is cancelled or fails through a path other than cancel_mount_task() (which explicitly sets self.mount_task = None), self.mount_task will still point to the current (now cancelled/failed) task, causing this elif block to be skipped and stranding the progress bar.
A more robust approach is to check if self.mount_task is either None or is the current task itself (using asyncio.current_task()). If it is the current task, it means no successor task has overwritten it yet, and we should safely clean up.
| elif getattr(container, "_finalize_task", None) is None and self.mount_task is None: | |
| elif getattr(container, "_finalize_task", None) is None and (self.mount_task is None or self.mount_task is asyncio.current_task()): |
There was a problem hiding this comment.
Fixed in 6d4445e: self.mount_task is None or self.mount_task is asyncio.current_task(). The added clause also covers a mount that ends via an exception (or a non-cancel_mount_task cancellation), where mount_task still points at the dead task. Added an exception-path regression test that fails on the is-None-only guard.
The finally guard checked only `self.mount_task is None`, which catches cancel_mount_task (it nulls mount_task) but not a mount that ends via an exception or a cancellation from another path — there mount_task still points at the now-dead task, so the guard was skipped and the bar stranded. Also check `mount_task is current_task()`: true when this task still owns mount_task (no successor overwrote it), so failed/early-cancelled mounts clean up too while a real successor still keeps its own bar. Adds an exception-path regression test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fnd/tui/preview/presenter.py`:
- Around line 1501-1516: The cleanup logic in the finally block can incorrectly
hide a successor decode progress bar because it only checks whether
self.mount_task is None, which doesn't account for the uncached decode path that
cancels the old mount and starts a new worker without immediately assigning
self.mount_task. To fix this, introduce a generation token mechanism for
tracking the current progress owner: create a generation counter that increments
each time a new load (mount or decode) begins, store the generation token with
each mount task or at the class level, and modify the cleanup condition to only
hide_progress_bar() and clear inflight_target if the stored generation token
matches the current generation (indicating this task still owns the progress
state). This ensures that a cancelled old mount task cannot interfere with a
successor's progress session that started after the cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33e170ec-e92b-4f16-b0b2-9d341ec8ed2f
📒 Files selected for processing (2)
fnd/tui/preview/presenter.pytests/test_preview_mount_cancel_strand.py
The finally's hide/release fired whenever mount_task was None — but the uncached decode path cancels the prior mount (nulling mount_task) and opens a NEW 'decoding...' session WITHOUT reassigning mount_task. So a previous mount cancelled in its early-await window, navigating onto an uncached file, could reach this finally and hide the successor decode's bar + clear its inflight latch. Add a target-ownership check: only clean up when the latch still points at THIS mount's target (or is already clear). The cancelled mount's own target still matches (strand fix preserved); a successor's does not. Adds a cross-target regression test.
1 participant
Problem
The preview "loading" bar could stick forever — until the user navigated to a different file and back. Reported as "the amount gets stuck while loading."
It was a stuck mount, not a paint/count issue (the count was always correct). In
_mount_chunks_async, the early awaits (container mount, prefetchcancel_task_on) ran before thetryblock and before the detached finalize task was spawned. That finalize task is the only thing that hides the bar and clears theinflight_targetlatch on success;cancel_mount_taskhides nothing. So a mount cancelled in that early window skipped thefinallyentirely — the bar stayed up and the latch stayed set. A same-file re-load then deduped out (line 152), and only a different-file nav cleared the latch and dispatched a completing load (the "navigate away and back" workaround).The window widens for slow cold mounts (large PDFs) and under load, so it surfaced intermittently and didn't reproduce in headless/tmux harnesses — found by static analysis of every
cancel_mount_taskcaller and the mount lifecycle.Fix
tryto cover the early awaits.finally, hide the bar + release the latch when the task ends before a finalize task existed and no successor mount took over.Verification
New regression test
tests/test_preview_mount_cancel_strand.pyparks a mount on a blocked early await, cancels it, and asserts the bar is hidden and the latch cleared — fails on pre-fix code, passes after. All preview + scope suites green; ruff + pyright-strict clean.