Retain lazy thumbnail jobs across concurrent refreshes

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

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