Recover the wallpaper picker after interrupted thumbnails (#6775)
* Recover image picker after interrupted thumbnails * Use arithmetic assertions in image cache tests * Bound thumbnail lock waits and reap partial thumbnails A hung generator (vips stuck on a corrupt file or slow mount) held its flock forever, wedging every later picker open; the directory-lock era capped that wait at 30 seconds, so keep the same bound. A generator killed mid-write also stranded its partial .jpg.<pid>.jpg forever, since nothing prunes the cache directory; only the lock holder writes those, so reap them right after taking the lock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Harden thumbnail locks and cache publication against races Adversarial review caught three holes. The lock fd leaked into vipsthumbnail, so an orphaned or hung vips kept holding the lock after its shell died; close it for the child. Reaping legacy lock directories unconditionally raced a still-running legacy generator through an upgrade; only reap ones older than the longest plausible generation. And cache publication was neither atomic nor exclusive, so a picker killed mid-write, or two interleaving, could leave truncated or mismatched rows behind signatures that still validated - the same permanent hiding this branch set out to fix; publish via renames under a per-key lock, rows first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: David Heinemeier Hansson <david@hey.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Akshar Patel
co-authored by
Claude Fable 5
David Heinemeier Hansson
Claude Fable 5
David Heinemeier Hansson
parent
9672d852f0
commit
f075a789f4
+43
-22
@@ -111,8 +111,8 @@ cache_key=$(printf '%s' "$image_dirs_env" | md5sum | cut -d ' ' -f 1)
|
||||
rows_cache_file="$cache_dir/$cache_key.rows"
|
||||
rows_signature_file="$cache_dir/$cache_key.signature"
|
||||
rows_fast_signature_file="$cache_dir/$cache_key.fast-signature"
|
||||
rows_signature="v2"$'\n'
|
||||
rows_fast_signature="v1"$'\n'
|
||||
rows_signature="v3"$'\n'
|
||||
rows_fast_signature="v2"$'\n'
|
||||
rows_cacheable=true
|
||||
rows_cache_hit=false
|
||||
image_files=()
|
||||
@@ -142,23 +142,32 @@ generate_thumbnail() {
|
||||
local image="$1"
|
||||
local thumbnail="$2"
|
||||
local lock="$thumbnail.lock"
|
||||
local lock_fd
|
||||
local tmp="$thumbnail.$$.jpg"
|
||||
|
||||
if mkdir "$lock" 2>/dev/null; then
|
||||
# Callers fan out one generator per image, so keep each vips single-threaded.
|
||||
if VIPS_CONCURRENCY=1 vipsthumbnail "$image" --size 1536x864 --smartcrop=centre --path "$tmp[Q=82,strip]"; then
|
||||
mv -f "$tmp" "$thumbnail"
|
||||
else
|
||||
rm -f "$tmp" "$thumbnail"
|
||||
fi
|
||||
# Older releases used directories as locks, which could survive a killed
|
||||
# generator and block this thumbnail forever. Only reap aged ones, so a
|
||||
# legacy generator still running through an upgrade keeps its lock.
|
||||
if [[ -d $lock ]] && (( $(date +%s) - $(stat -c '%Y' "$lock" 2>/dev/null || date +%s) > 120 )); then
|
||||
rmdir "$lock" 2>/dev/null
|
||||
fi
|
||||
|
||||
rmdir "$lock" 2>/dev/null || true
|
||||
exec {lock_fd}>"$lock" || return
|
||||
flock -w 30 "$lock_fd" || return
|
||||
|
||||
# A generator killed mid-write leaves its partial $thumbnail.<pid>.jpg
|
||||
# behind. Only the lock holder writes these, so any found now are stale.
|
||||
rm -f "$thumbnail".*.jpg
|
||||
|
||||
[[ -f $thumbnail ]] && return
|
||||
|
||||
# Callers fan out one generator per image, so keep each vips single-threaded.
|
||||
# Close the lock fd for vips: an orphaned or hung vips must not keep holding
|
||||
# the lock after this shell is killed.
|
||||
if VIPS_CONCURRENCY=1 vipsthumbnail "$image" --size 1536x864 --smartcrop=centre --path "$tmp[Q=82,strip]" {lock_fd}>&-; then
|
||||
mv -f "$tmp" "$thumbnail"
|
||||
else
|
||||
for ((i = 0; i < 3000; i++)); do
|
||||
[[ -f $thumbnail ]] && return
|
||||
[[ -d $lock ]] || break
|
||||
sleep 0.01
|
||||
done
|
||||
rm -f "$tmp" "$thumbnail"
|
||||
fi
|
||||
}
|
||||
|
||||
@@ -226,7 +235,10 @@ elif [[ $rows_cache_hit != true ]]; then
|
||||
if [[ -s $pending_file ]]; then
|
||||
pruned=""
|
||||
while IFS=$'\t' read -r row_image row_thumbnail; do
|
||||
[[ -e $row_thumbnail ]] || continue
|
||||
if [[ ! -e $row_thumbnail ]]; then
|
||||
rows_cacheable=false
|
||||
continue
|
||||
fi
|
||||
|
||||
if [[ -z $pruned ]]; then
|
||||
pruned="$row_image"$'\t'"$row_thumbnail"
|
||||
@@ -237,12 +249,21 @@ elif [[ $rows_cache_hit != true ]]; then
|
||||
rows="$pruned"
|
||||
fi
|
||||
|
||||
if [[ $rows_cacheable == true ]]; then
|
||||
printf '%s' "$rows" >"$rows_cache_file"
|
||||
printf '%s' "$rows_signature" >"$rows_signature_file"
|
||||
printf '%s' "$rows_fast_signature" >"$rows_fast_signature_file"
|
||||
else
|
||||
rm -f "$rows_cache_file" "$rows_signature_file" "$rows_fast_signature_file"
|
||||
# Publish the cache under a lock and via renames: a picker killed mid-write,
|
||||
# or two pickers interleaving, must never leave truncated or mismatched rows
|
||||
# behind signatures that still validate. Rows go first so a kill between
|
||||
# renames leaves signatures that are either older (a harmless miss) or
|
||||
# describe the same directory state.
|
||||
if exec {rows_lock_fd}>"$rows_cache_file.lock" && flock -w 30 "$rows_lock_fd"; then
|
||||
if [[ $rows_cacheable == true ]]; then
|
||||
rm -f "$cache_dir/$cache_key".*.tmp
|
||||
printf '%s' "$rows" >"$rows_cache_file.$$.tmp" && mv -f "$rows_cache_file.$$.tmp" "$rows_cache_file"
|
||||
printf '%s' "$rows_signature" >"$rows_signature_file.$$.tmp" && mv -f "$rows_signature_file.$$.tmp" "$rows_signature_file"
|
||||
printf '%s' "$rows_fast_signature" >"$rows_fast_signature_file.$$.tmp" && mv -f "$rows_fast_signature_file.$$.tmp" "$rows_fast_signature_file"
|
||||
else
|
||||
rm -f "$rows_cache_file" "$rows_signature_file" "$rows_fast_signature_file"
|
||||
fi
|
||||
exec {rows_lock_fd}>&-
|
||||
fi
|
||||
fi
|
||||
|
||||
|
||||
@@ -0,0 +1,141 @@
|
||||
#!/bin/bash
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh"
|
||||
|
||||
require_command flock
|
||||
|
||||
tmp=$(mktemp -d)
|
||||
trap 'rm -rf "$tmp"' EXIT
|
||||
|
||||
cache_home="$tmp/cache"
|
||||
images="$tmp/images"
|
||||
stub_bin="$tmp/bin"
|
||||
mkdir -p "$images" "$stub_bin"
|
||||
|
||||
cat >"$stub_bin/vipsthumbnail" <<'EOF'
|
||||
#!/bin/bash
|
||||
|
||||
image="$1"
|
||||
shift
|
||||
|
||||
while (( $# > 0 )); do
|
||||
if [[ $1 == "--path" ]]; then
|
||||
output=${2%%\[*}
|
||||
break
|
||||
fi
|
||||
shift
|
||||
done
|
||||
|
||||
if [[ -f ${VIPSTHUMBNAIL_FAIL_FILE:-} ]] && grep -Fxq "$image" "$VIPSTHUMBNAIL_FAIL_FILE"; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
[[ -z ${VIPSTHUMBNAIL_CALLS_FILE:-} ]] || printf '%s\n' "$image" >>"$VIPSTHUMBNAIL_CALLS_FILE"
|
||||
[[ -z ${VIPSTHUMBNAIL_DELAY:-} ]] || sleep "$VIPSTHUMBNAIL_DELAY"
|
||||
printf 'thumbnail' >"$output"
|
||||
EOF
|
||||
chmod +x "$stub_bin/vipsthumbnail"
|
||||
|
||||
for name in one two three; do
|
||||
printf 'image-%s' "$name" >"$images/$name.png"
|
||||
done
|
||||
|
||||
cache_dir="$cache_home/omarchy/image-selector"
|
||||
mkdir -p "$cache_dir"
|
||||
|
||||
stale_tmp=""
|
||||
live_lock=""
|
||||
for image in "$images"/*; do
|
||||
signature=$(stat -Lc '%s:%Y' "$image")
|
||||
hash=$(printf '%s\t%s' "$image" "$signature" | md5sum | cut -d ' ' -f 1)
|
||||
mkdir "$cache_dir/$hash.jpg.lock"
|
||||
touch -m -d '10 minutes ago' "$cache_dir/$hash.jpg.lock"
|
||||
stale_tmp="$cache_dir/$hash.jpg.4242.jpg"
|
||||
live_lock="$cache_dir/$hash.jpg.lock"
|
||||
done
|
||||
printf 'partial' >"$stale_tmp"
|
||||
|
||||
cache_key=$(printf '%s' "$images" | md5sum | cut -d ' ' -f 1)
|
||||
printf '%s\t%s' "$images/one.png" "$cache_dir/missing.jpg" >"$cache_dir/$cache_key.rows"
|
||||
printf 'v2\n%s:%s\n' "$images" "$(stat -Lc '%Y' "$images")" >"$cache_dir/$cache_key.signature"
|
||||
printf 'v1\n%s:%s\n' "$images" "$(stat -Lc '%Y' "$images")" >"$cache_dir/$cache_key.fast-signature"
|
||||
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images"
|
||||
|
||||
(( $(find "$cache_dir" -maxdepth 1 -name '*.jpg' -type f | wc -l) == 3 )) ||
|
||||
fail "image menu recovers thumbnails from stranded locks"
|
||||
(( $(awk 'END { print NR }' "$cache_dir/$cache_key.rows") == 3 )) ||
|
||||
fail "image menu rebuilds every row after cache invalidation"
|
||||
[[ $(head -n 1 "$cache_dir/$cache_key.signature") == "v3" ]] ||
|
||||
fail "image menu invalidates stale row caches"
|
||||
[[ ! -e $stale_tmp ]] ||
|
||||
fail "image menu clears partial thumbnails left by killed generators"
|
||||
pass "image menu recovers stranded locks and stale rows"
|
||||
|
||||
rm -rf "$cache_home"
|
||||
mkdir -p "$cache_dir"
|
||||
mkdir "$live_lock"
|
||||
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images"
|
||||
|
||||
(( $(find "$cache_dir" -maxdepth 1 -name '*.jpg' -type f | wc -l) == 2 )) ||
|
||||
fail "image menu skips a thumbnail whose fresh legacy lock may still be owned"
|
||||
[[ -d $live_lock ]] ||
|
||||
fail "image menu leaves a fresh legacy lock directory alone"
|
||||
[[ ! -e $cache_dir/$cache_key.rows ]] ||
|
||||
fail "image menu does not cache rows while a legacy generator holds a lock"
|
||||
pass "image menu respects a live legacy generator's lock"
|
||||
|
||||
rm -rf "$cache_home"
|
||||
mkdir -p "$cache_home"
|
||||
printf '%s\n' "$images/two.png" >"$tmp/failures"
|
||||
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" VIPSTHUMBNAIL_FAIL_FILE="$tmp/failures" \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images"
|
||||
|
||||
cache_dir="$cache_home/omarchy/image-selector"
|
||||
[[ ! -e $cache_dir/$cache_key.rows ]] || fail "image menu does not cache incomplete rows"
|
||||
[[ ! -e $cache_dir/$cache_key.signature ]] || fail "image menu does not sign incomplete rows"
|
||||
[[ ! -e $cache_dir/$cache_key.fast-signature ]] || fail "image menu does not fast-cache incomplete rows"
|
||||
pass "image menu leaves failed thumbnail batches uncached"
|
||||
|
||||
rm "$tmp/failures"
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images"
|
||||
|
||||
(( $(find "$cache_dir" -maxdepth 1 -name '*.jpg' -type f | wc -l) == 3 )) ||
|
||||
fail "image menu retries a previously failed thumbnail"
|
||||
(( $(awk 'END { print NR }' "$cache_dir/$cache_key.rows") == 3 )) ||
|
||||
fail "image menu caches every row after retry"
|
||||
pass "image menu completes and caches a later retry"
|
||||
|
||||
rm -rf "$cache_home"
|
||||
mkdir -p "$cache_home"
|
||||
: >"$tmp/calls"
|
||||
|
||||
# The delay keeps both runs inside the generation window so the locks are
|
||||
# actually contended rather than the second run arriving after the first.
|
||||
pids=()
|
||||
for run in 1 2; do
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" \
|
||||
VIPSTHUMBNAIL_CALLS_FILE="$tmp/calls" VIPSTHUMBNAIL_DELAY=0.25 \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images" &
|
||||
pids+=($!)
|
||||
done
|
||||
for pid in "${pids[@]}"; do
|
||||
wait "$pid" || fail "concurrent image menu runs exit cleanly"
|
||||
done
|
||||
|
||||
(( $(wc -l <"$tmp/calls") == 3 )) || fail "image menu serializes concurrent thumbnail generators"
|
||||
|
||||
rm -f "$cache_dir"/*.jpg
|
||||
rm -f "$cache_dir/$cache_key.rows" "$cache_dir/$cache_key.signature" "$cache_dir/$cache_key.fast-signature"
|
||||
PATH="$stub_bin:$PATH" XDG_CACHE_HOME="$cache_home" VIPSTHUMBNAIL_CALLS_FILE="$tmp/calls" \
|
||||
"$ROOT/bin/omarchy-menu-images" --cache-only "$images"
|
||||
|
||||
(( $(wc -l <"$tmp/calls") == 6 )) || fail "image menu releases thumbnail locks after generation"
|
||||
pass "image menu owns locks for exactly one generator lifetime"
|
||||
Reference in New Issue
Block a user