Skip to content

Keep file chooser responsive during stalled filesystem reads - #175

Merged
thisisgm merged 4 commits into
thisisgm:mainfrom
nerdislb:fix/picker-stalled-listing-upstream
Sep 22, 2026
Merged

thisisgm merged 4 commits into
thisisgm:mainfrom
nerdislb:fix/picker-stalled-listing-upstream

Conversation

@nerdislb

@nerdislb nerdislb commented Sep 19, 2026 •

Copy link
Copy Markdown

Summary

  • Keep chooser navigation responsive when a read-only listing backend stalls on FUSE/network I/O.
  • Replace and reap only chooser-owned listing workers; discard obsolete replies and keep selection validation separate.
  • Persist the portal reply before shutting down both read-only children, including failed-start/cancellation races. Normal file-manager write draining is unchanged.

Verification

  • Six real Quickshell process lifecycle cases, including blocked listing/validation, stale output, cancellation and failed startup; controlled signal-order regression fails with the previous guard.
  • 68 picker JavaScript checks pass.
  • Actual release binary + complete picker UI selects and accepts a local fixture.
  • Native desktop acceptance: Space/Enter selection, Backspace navigation away from a controlled stalled reader, Escape cancellation, and window-manager close all produce the expected portal reply and reap owned helpers.
  • Small warm local navigation sample: replacement median 3 ms versus reused-worker median 0.5 ms (10 samples each, 1 ms clock resolution); not a general performance benchmark.
  • Independent Claude Fable review; startup race corrected and re-reviewed.

Scope / baseline

Independent of #172 and based on current main. No production mount restart or configuration changes required. SIGKILL cannot guarantee completion for genuinely uninterruptible kernel I/O.

Release build passes. The file-budget check passes on the actual main baseline. No Rust source changes. The existing signal-arity gate fails on both base and candidate after the prior Messages.js extraction; no full-suite-green claim.

The local tracking ref initially lagged because this checkout fetched only a tag. This branch now explicitly includes actual main (f738261); the PR diff remains limited to ten picker files.

Summary by CodeRabbit

  • Bug Fixes
    • Improved file chooser reliability when directory listings stall or return incomplete data.
    • Newer navigation requests now supersede outdated requests, helping the chooser recover without displaying stale results.
    • Improved cancellation and shutdown handling, including failed or interrupted background operations.
    • Prevented file-manager write activity during read-only chooser listing operations.
  • Tests
    • Added coverage for stalled listings, navigation changes, cancellation, missing resources, and shutdown ordering.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cbc85759-3c68-4d60-bd3a-ee7249a17db1

📥 Commits

Reviewing files that changed from the base of the PR and between c8231b4 and 7d49c70.

📒 Files selected for processing (3)
  • tests/picker-stall.sh
  • ui/Backend.qml
  • ui/qmldir

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The picker now uses a dedicated listing worker, a picker-only backend mode, and coordinated shutdown. Integration tests cover stalled reads, request supersession, cancellation, failed starts, and process cleanup.

Changes

Picker worker lifecycle

Layer / File(s) Summary
Dedicated listing worker and routing
ui/PickerListing.qml, ui/picker.qml, ui/qmldir
Listing requests use a separate worker. Obsolete workers are terminated, responses are parsed, and current messages are forwarded to the picker.
Picker-only backend and coordinated shutdown
ui/Backend.qml, ui/PickerLifecycle.qml, ui/picker.qml
The chooser backend accepts only picker and format requests. Shutdown waits for both read processes after reply persistence.
Stall and cancellation validation
tests/picker-stall-helper.py, tests/picker-stall.qml, tests/picker-stall.sh, tests/run-all.sh, AGENTS.md
Tests cover blocked reads, navigation supersession, cancellation, failed starts, process reaping, and test registration. Documentation describes the new behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PickerList
  participant PickerListing
  participant FleaBackend as flea --backend
  participant PickerLifecycle
  participant Backend
  PickerList->>PickerListing: request listing
  PickerListing->>FleaBackend: run listing request
  FleaBackend-->>PickerListing: return JSON response
  PickerListing-->>PickerList: emit listing message
  PickerLifecycle->>PickerListing: quit
  PickerLifecycle->>Backend: quit
  PickerListing-->>PickerLifecycle: quitReady
  Backend-->>PickerLifecycle: quitReady
Loading

Suggested reviewers: thisisgm

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: keeping the file chooser responsive when filesystem reads stall.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thisisgm

Copy link
Copy Markdown
Owner

Merged in aba90892 (v0.3.2): the chooser stays responsive through a stalled listing, and Cancel completes even when its backend never starts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants