Skip to content

perf(picker): refresh stale Log preview cache in background after disk hit - #2646

Merged
max-sixty merged 3 commits into
mainfrom
picker-list
May 9, 2026
Merged

max-sixty merged 3 commits into
mainfrom
picker-list

Conversation

@max-sixty

Copy link
Copy Markdown
Owner

Summary

The Log preview's cached raw_log bakes in git log --format=%C(auto)%d ref decorations at write time. The cache key is (branch_head_sha, w, h), so another ref starting or stopping pointing at the cached SHA — most commonly a squash-merge landing main at the cached commit — leaves the decoration text stale even though the key is still valid.

Approach

On every disk-cache hit, enqueue a background refresh task on a dedicated picker-preview-refresh thread. The thread drains an mpsc channel serially, calling compute_log_raw_and_stats and overwriting both the disk file and the in-memory PreviewCache DashMap entry. Skim re-invokes the preview callback on every item-change, so navigating row A → B → A within a session shows the refreshed preview on return; a fresh wt switch always reads the disk-refreshed content.

The refresh thread runs separately from the rayon precompute pool so foreground first-paint work never competes with refreshes for worker slots. wait_for_idle covers refresh tasks via the existing PendingGuard pattern: producer increments pending before sending, consumer decrements via guard drop after running.

Trade-offs

  • No mtime throttle. Every disk hit triggers a refresh, including hits where the recomputed output would be byte-identical (the common case on stable repos). The dominant cost — the git subprocess — is paid either way; a byte-compare would only save the disk write and DashMap insert. Bounded by serial drain on a single thread.
  • User staring at the same row the entire session sees stale decorations — skim has no push-refresh API. Documented at items.rs:219-223. The "comes back to reselect" path is the supported case.

Tests

  • log_disk_hit_triggers_background_refresh — seeds disk cache with a stale entry containing a marker; asserts both caches no longer hold the marker after spawn_preview + wait_for_idle.
  • non_log_modes_do_not_trigger_log_refresh — seeds disk Log cache, spawns BranchDiff, asserts disk Log cache untouched.

🤖 Generated with Claude Code

…k hit

The Log preview's cached `raw_log` bakes in `git log --format=%C(auto)%d`
ref decorations at write time. The cache key is `(branch_head_sha, w, h)`,
so another ref starting or stopping pointing at the cached SHA — most
commonly a squash-merge landing `main` at the cached commit — leaves the
decoration text stale even though the key is still valid.

Mitigate by enqueuing a background refresh on every disk-cache hit. A
dedicated `picker-preview-refresh` thread drains an mpsc channel of
refresh tasks serially, calling `compute_log_raw_and_stats` and
overwriting both the disk file and the in-memory `PreviewCache` DashMap
entry. Skim re-invokes the preview callback on every item-change, so a
user navigating row A → B → A within a session sees the refreshed
preview on return; a fresh `wt switch` always reads the disk-refreshed
content.

The refresh thread runs separately from the rayon precompute pool so
foreground first-paint work never competes with refreshes for worker
slots. `wait_for_idle` covers refresh tasks via the existing
`PendingGuard` pattern: producer increments `pending` before sending,
consumer decrements via guard drop after running.

Co-Authored-By: Claude <noreply@anthropic.com>
worktrunk-bot
worktrunk-bot previously approved these changes May 8, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI surfaced two issues after my approval — withdrawing the approval per the project's codecov-failure policy.

codecov/patch (3 uncovered lines):

  • src/commands/picker/preview_orchestrator.rs:135-139 — channel-send-error rollback. Reachable only if the refresh thread has exited while the orchestrator is still alive, which (given catch_unwind around the task body) requires a panic in the recv-loop setup itself. Defensive but essentially unreachable in practice.
  • src/commands/picker/items.rs:271 — PreviewMode::Log => Self::compute_log_preview(repo, item, width, height).0, in compute_preview. The new orchestrator code special-cases PreviewMode::Log to bypass compute_and_page_preview entirely, so this arm is no longer reachable from the only call site (compute_and_page_preview -> compute_preview). Dead after this PR.
  • src/commands/picker/items.rs:552 — return (String::new(), false); on git log failure inside compute_log_preview_inner. Reachable in the refresh path: when the disk read succeeded earlier (so the producer enqueued a refresh) but the force_recompute re-run of git log fails. The refresh thread then cache.insert(..., "") and replaces a valid in-memory entry (the rendered disk-cache content) with an empty string, so a return-visit to that row shows an empty preview. Inline suggestion below filters that case.

test (macos) — test_alias_approval_decline panicked with Os { code: 32, kind: BrokenPipe } at tests/integration_tests/step_alias.rs:569. Unrelated to this PR (no overlap with picker code) — looks like a transient PTY/pipe flake. Safe to rerun.

Comment thread src/commands/picker/preview_orchestrator.rs Outdated
@worktrunk-bot
worktrunk-bot dismissed their stale review May 8, 2026 18:30

codecov/patch failed — 3 uncovered lines in changed code, see follow-up review for analysis

Three changes prompted by review on #2646:

- Inline `compute_preview` into `compute_and_page_preview`. The
  dispatcher's `Log` arm became dead after the orchestrator started
  bypassing it directly, so `compute_and_page_preview` now returns
  `(String, bool)` and the orchestrator no longer special-cases Log.
  Removes the dead arm flagged by codecov at items.rs:271.

- Replace the `let-else return (String::new(), false)` on `git log`
  failure with `unwrap_or_default()`, requiring `LogCacheEntry: Default`.
  The empty-entry path now flows through the same render pipeline,
  producing the same effective output without a separate untestable
  defensive return (codecov gap at items.rs:552).

- Refresh thread skips `cache.insert` when `refresh_log_preview`
  returns an empty string, so a transient `git log` failure during
  refresh can't poison the in-memory cache with `""` and wipe out the
  value the producer just inserted.

Also drops an unreachable channel-send-error rollback in `spawn_preview`:
the refresh thread holds the receiver for the orchestrator's full
lifetime and `&self` proves the orchestrator is alive, so the send
always succeeds. `expect()` makes the invariant explicit.

Co-Authored-By: Claude <noreply@anthropic.com>
…thread

Replace the dedicated `picker-preview-refresh` std::thread + mpsc channel
with `pool.spawn_fifo` into the existing rayon pool. The refresh closure
runs the same `refresh_log_preview` + DashMap insert (with the empty-
string guard) that the dedicated thread did, just in a different worker.

`spawn_fifo` puts the task on the worker's local FIFO deque so already-
submitted foreground precompute drains first. PendingGuard accounting
moves inline into the closure so `wait_for_idle` still covers refreshes.

Net effect: deletes RefreshTask, spawn_refresh_thread, the mpsc field,
the panic-catch wrapper, and the channel-send-error rollback —
~100 LOC removed. The original argument for a separate thread (avoid
contention with foreground precompute) doesn't survive scrutiny:
refreshes are submitted from inside rayon closures during foreground
work, so they land after the submitting task either way.

Co-Authored-By: Claude <noreply@anthropic.com>
@max-sixty
max-sixty merged commit 5d277cc into main May 9, 2026
34 checks passed
@max-sixty
max-sixty deleted the picker-list branch May 9, 2026 00:34
@max-sixty max-sixty mentioned this pull request May 10, 2026
3 tasks done
max-sixty added a commit that referenced this pull request May 10, 2026
Bump worktrunk to **0.49.0** — minor bump (semver-checks reports a
breaking change in `GitError::UncommittedChanges`, which gained a
`dirty_files: Vec<String>` field).

## Highlights

**Improved**
- New `codename(n)` template filter — deterministic friendly worktree
names from a ~1.26M-combo pool (`{{ branch | codename(2) }}` →
`malleable-opah`).
[#2641](#2641), thanks
@endigma
- Picker preview disk cache for Log / BranchDiff / UpstreamDiff, with
background refresh for stale Log decorations.
[#2628](#2628),
[#2646](#2646)
- Picker shares one `default_branch_sha` lookup across BranchDiff items
instead of forking N rev-parse subprocesses.
[#2658](#2658)
- "X has uncommitted changes" errors now list the dirty files inline.
[#2653](#2653)

**Fixed**
- `wt switch` integrates with `cd` aliases like zoxide (uses `builtin
cd`). [#2644](#2644), thanks
@xkumiyu for reporting
- Empty hook tables no longer panic during `wt switch --create`.
[#2635](#2635), thanks @topit
for reporting
- `Repository::current_worktree()` and five sibling helpers no longer
silently leak the process CWD when called via `Repository::at(p)`.
[#2652](#2652)
- `WorkingTree::is_linked` tolerates non-git CWDs (e.g. Nix sandbox).
[#2625](#2625), thanks
@DArtagan for reporting
- Picker no longer overlays `collect` warnings on the active TUI; clears
its frame on exit in inline mode.
[#2627](#2627),
[#2626](#2626)
- Failed `git log` no longer poisons the picker's preview disk cache.
[#2651](#2651)
- `wt step prune` says "removing branch" for branch-only candidates.
[#2619](#2619)
- Claude Code Windows integration uses a `wt.sh` wrapper to avoid
colliding with Windows Terminal's `wt.exe`.
[#1754](#1754), thanks
@lucaspimentel

**Documentation**
- cmux workspace integration recipe.
[#1907](#1907), thanks
@alvistar

See
[CHANGELOG.md](https://github.com/max-sixty/worktrunk/blob/release/CHANGELOG.md)
for the full list including internal CI / test-isolation changes.

## Test plan

- [x] `cargo run -- hook pre-merge --yes` (tests + lints clean)
- [x] `cargo semver-checks check-release -p worktrunk` (one breaking
change confirmed → minor bump)
- [x] CHANGELOG entries verified by subagent against actual diffs

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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