From 520e4bca5bd0bc7388357b8bcfe50d14f70ee388 Mon Sep 17 00:00:00 2001 From: Ric Lewis Date: Sat, 3 Oct 2026 00:03:44 -0700 Subject: [PATCH] Retain lazy thumbnail jobs across concurrent refreshes --- bin/omarchy-menu-images | 35 ++++++++--- test/shell.d/menu-images-test.sh | 105 ++++++++++++++++++++++++++++--- 2 files changed, 125 insertions(+), 15 deletions(-) diff --git a/bin/omarchy-menu-images b/bin/omarchy-menu-images index 67370cab..4a1cf37d 100755 --- a/bin/omarchy-menu-images +++ b/bin/omarchy-menu-images @@ -252,7 +252,7 @@ thumbnail_for() { # ffmpegthumbnailer leaves FFmpeg's automatic threading on, so a full-width fan # out of those would put a codec thread pool on every core at once. drain_pending_thumbnails() { - local video_jobs image_jobs pending_fd + local video_jobs image_jobs pending_fd queue_file queue_lock_fd worker_lock_fd export -f generate_thumbnail is_video_path @@ -261,16 +261,37 @@ drain_pending_thumbnails() { image_jobs=$(( $(nproc) / 2 )) (( image_jobs > 0 )) || image_jobs=1 (( image_jobs > 2 )) && image_jobs=2 - # The caller may exit immediately after printing rows. Inherit an open - # queue fd so its EXIT trap can unlink the file without losing any jobs. - exec {pending_fd}<"$pending_file" + # Publish jobs before trying to own the pool. A contended refresh leaves + # its jobs for the current owner instead of throwing its queue away. + queue_file="$rows_cache_file.thumbnails.pending" + exec {queue_lock_fd}>"$rows_cache_file.thumbnails.queue.lock" || return + flock "$queue_lock_fd" || return + cat "$pending_file" >>"$queue_file" + exec {queue_lock_fd}>&- ( exec {worker_lock_fd}>"$rows_cache_file.thumbnails.lock" flock -n "$worker_lock_fd" || exit 0 - nice -n 10 ionice -c 3 xargs -0 -n 2 -P "$image_jobs" \ - bash -c 'generate_thumbnail "$1" "$2"' _ <&"$pending_fd" + exec {queue_lock_fd}>"$rows_cache_file.thumbnails.queue.lock" || exit 1 + while true; do + flock "$queue_lock_fd" || exit 1 + if [[ ! -s $queue_file ]]; then + # Release ownership while publishing is still locked. A producer + # arriving after the empty check can then start the next pool. + exec {worker_lock_fd}>&- + exec {queue_lock_fd}>&- + break + fi + # Unlink the batch under the queue lock, then drain its open fd. + # Producers can publish the next batch while conversions run. + exec {pending_fd}<"$queue_file" || exit 1 + rm -f "$queue_file" + flock -u "$queue_lock_fd" + # Converters must not inherit the publication lock's descriptor. + nice -n 10 ionice -c 3 xargs -0 -n 2 -P "$image_jobs" \ + bash -c 'generate_thumbnail "$1" "$2"' _ <&"$pending_fd" {queue_lock_fd}>&- + exec {pending_fd}<&- + done ) >/dev/null 2>&1 & - exec {pending_fd}<&- else xargs -a "$pending_file" -0 -n 2 -P "$(nproc)" \ bash -c 'generate_thumbnail "$1" "$2"' _ >/dev/null 2>&1 || true diff --git a/test/shell.d/menu-images-test.sh b/test/shell.d/menu-images-test.sh index 0e79a09f..e06e3a93 100644 --- a/test/shell.d/menu-images-test.sh +++ b/test/shell.d/menu-images-test.sh @@ -5,9 +5,34 @@ set -euo pipefail source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh" require_command flock +require_command setsid tmp=$(mktemp -d) -trap 'rm -rf "$tmp"' EXIT +lazy_groups=() +cleanup() { + local status=$? gate group running attempt + trap - EXIT + # Release every fixture gate, including on an assertion failure. Each lazy + # invocation owns a private process group, so its detached pool is tracked. + for gate in "$tmp"/lazy-state*/gate; do + [[ ! -d ${gate%/*} ]] || touch "$gate" + done + for attempt in {1..500}; do + running=false + for group in "${lazy_groups[@]}"; do + if kill -0 -- "-$group" 2>/dev/null; then running=true; fi + done + [[ $running == "true" ]] || break + sleep 0.02 + done + for group in "${lazy_groups[@]}"; do + kill -TERM -- "-$group" 2>/dev/null || true + wait "$group" 2>/dev/null || true + done + rm -rf "$tmp" + exit "$status" +} +trap cleanup EXIT cache_home="$tmp/cache" images="$tmp/images" @@ -190,10 +215,19 @@ printf '%s\n' "$((active - 1))" >"$LAZY_STATE/active" echo done >>"$LAZY_STATE/completed" EOF chmod +x "$stub_bin/nproc" "$stub_bin/vipsthumbnail" -# Also release the gate on failure, so background fixtures cannot outlive us. -trap 'touch "$lazy_state/gate"' EXIT + +lazy_rows() { + setsid env PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$tmp/lazy-cache-$cores" LAZY_STATE="$lazy_state" FAKE_CORES="$cores" \ + timeout --foreground 10 "$ROOT/bin/omarchy-menu-images" --lazy-thumbnails --print-rows "$lazy_images" >"$lazy_state/rows" & + local group=$! + lazy_groups+=("$group") + wait "$group" || fail "lazy image menu returns rows without waiting for its pool" + rows=$(<"$lazy_state/rows") +} for cores in 1 2 8; do + rm -f "$lazy_images/new.png" + printf 'image' >"$lazy_images/0.png" lazy_state="$tmp/lazy-state-$cores" mkdir -p "$lazy_state" printf '0\n' >"$lazy_state/active" @@ -201,8 +235,7 @@ for cores in 1 2 8; do expected_workers=1 (( cores < 4 )) || expected_workers=2 for run in 1 2; do - rows=$(PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$tmp/lazy-cache-$cores" LAZY_STATE="$lazy_state" FAKE_CORES="$cores" \ - timeout 10 "$ROOT/bin/omarchy-menu-images" --lazy-thumbnails --print-rows "$lazy_images") + lazy_rows (( $(wc -l <<<"$rows") == 40 )) || fail "lazy image menu returns all rows before conversion" done for attempt in {1..100}; do @@ -215,13 +248,69 @@ for cores in 1 2 8; do [[ ! -e $lazy_state/priority-failed ]] || fail "lazy image menu reserves CPU and I/O priority for the UI" pass "lazy image menu opens with at most $expected_workers workers on $cores cores" + # This job did not exist in the pool's first batch. A contended refresh must + # retain it, along with a replacement for an image changed during conversion. + printf 'new-image' >"$lazy_images/new.png" + printf 'changed-image-with-new-size' >"$lazy_images/0.png" + lazy_rows + (( $(wc -l <<<"$rows") == 41 )) || fail "contended refresh returns the added image" + for changed in new 0; do + signature=$(stat -Lc '%s:%Y' "$lazy_images/$changed.png") + hash=$(printf '%s\t%s' "$lazy_images/$changed.png" "$signature" | md5sum | cut -d ' ' -f 1) + [[ ! -e $tmp/lazy-cache-$cores/omarchy/image-selector/$hash.jpg ]] || + fail "contended refresh does not start another converter pool" + done + touch "$lazy_state/gate" for attempt in {1..500}; do - if [[ -f $lazy_state/completed ]] && (( $(wc -l <"$lazy_state/completed") == 40 )); then break; fi + if [[ -f $lazy_state/completed ]] && (( $(wc -l <"$lazy_state/completed") == 42 )); then break; fi sleep 0.02 done - (( $(wc -l <"$lazy_state/completed") == 40 && $(<"$lazy_state/peak") == expected_workers )) || + (( $(wc -l <"$lazy_state/completed") == 42 && $(<"$lazy_state/peak") == expected_workers )) || fail "lazy image menu completes the queue after its parent and queue path are gone" + for changed in new 0; do + signature=$(stat -Lc '%s:%Y' "$lazy_images/$changed.png") + hash=$(printf '%s\t%s' "$lazy_images/$changed.png" "$signature" | md5sum | cut -d ' ' -f 1) + [[ -f $tmp/lazy-cache-$cores/omarchy/image-selector/$hash.jpg ]] || + fail "lazy image menu retains new and changed jobs while its pool is busy" + done pass "lazy image menu workers finish every queued thumbnail after the caller exits" done -trap 'rm -rf "$tmp"' EXIT + +# Exercise the same cleanup handler on both successful and failing exits, +# with a detached, gated fixture still running when the EXIT trap fires. +run_node_test <<'JS' +const fs = require('fs') +const { spawnSync } = require('child_process') +const script = fs.readFileSync(path.join(root, 'test/shell.d/menu-images-test.sh'), 'utf8') +const cleanupHandler = script.match(/cleanup\(\) \{[\s\S]*?\n\}/)[0] +for (const status of [0, 31]) { + const result = spawnSync('bash', ['-c', ` +set -euo pipefail +tmp=$(mktemp -d) +lazy_groups=() +${cleanupHandler} +trap cleanup EXIT +mkdir -p "$tmp/lazy-state-probe" +setsid bash -c ': >"$1/ready"; while [[ ! -e $1/gate ]]; do sleep 0.02; done; sleep 0.05' _ "$tmp/lazy-state-probe" & +lazy_groups+=("$!") +while [[ ! -e $tmp/lazy-state-probe/ready ]]; do sleep 0.02; done +printf '%s\\n%s\\n' "$tmp" "$!" +exit ${status} +`], { encoding: 'utf8', timeout: 15000 }) + const [directory, group] = result.stdout.trim().split('\n') + const removed = !fs.existsSync(directory) + let alive = false + try { + try { process.kill(-Number(group), 0); alive = true } catch (error) { + if (error.code !== 'ESRCH') throw error + } + } finally { + if (group) { try { process.kill(-Number(group), 'SIGKILL') } catch (_) {} } + if (directory) fs.rmSync(directory, { recursive: true, force: true }) + } + assertEqual(result.status, status, `fixture cleanup preserves exit status ${status}`) + assert(removed, `fixture cleanup removes its directory on exit ${status}`) + assert(!alive, `fixture cleanup finishes its detached workers on exit ${status}`) +} +JS