From 72f72995a9c4a1ddd2ea3adb04cc2d7641410245 Mon Sep 17 00:00:00 2001
From: Paul Buetow <paul@buetow.org>
Date: Tue, 16 Jun 2026 23:00:23 +0300
Subject: 9n0 warn on identify failure instead of caching empty EXIF

cached_photo_identify_output() swallowed all ImageMagick errors with
'imagemagick_identify ... || true', so a corrupt photo or identify
failure left the cache with only the signature line and no EXIF. That
signature-only file was a valid-looking cache hit, so the photo rendered
with empty tooltip/stats and the failure was never retried or warned
about again.

Now capture identify's exit status; on failure, log_warning naming the
photo and rm -f the cache file so the next run retries instead of
reusing an empty result. The function still returns 0 so one unreadable
photo does not abort generation (it runs in backgrounded render jobs
under set -euo pipefail) -- the photo just renders without EXIF, now
with a warning.

Adds tests/cli.sh test_generate_warns_and_skips_cache_on_identify_failure
and a TEST_IMAGEMAGICK_IDENTIFY_FAIL hook in the fake ImageMagick to
drive a failing identify; asserts exit 0, a warning naming the photo,
the photo still rendered, and that the failed photo's cache is absent
while a successful photo's cache is present.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 bin/shuriken | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

(limited to 'bin')

diff --git a/bin/shuriken b/bin/shuriken
index 1b4fcf7..a838a6a 100755
--- a/bin/shuriken
+++ b/bin/shuriken
@@ -1773,6 +1773,7 @@ cached_photo_identify_output() {
     local cache_file
     local cached_signature=''
     local current_signature
+    local identify_status
 
     # Persist the EXIF cache in a volatile ./cache directory parallel to ./dist
     # (the staging dir is a sibling of the final dist, so dirname "$DIST_DIR" is
@@ -1797,8 +1798,34 @@ cached_photo_identify_output() {
 
     mkdir -p "$cache_dir"
     printf '%s\n' "$current_signature" > "$cache_file"
+
+    # Capture the identify exit status instead of swallowing it with `|| true`.
+    # Errors are still hidden from stdout (so a corrupt photo does not pollute
+    # the EXIF output), but a non-zero status now drives a warning + no-cache
+    # rather than silently leaving a signature-only cache entry behind.
+    identify_status=0
     imagemagick_identify -verbose "$photo_path" >> "$cache_file" 2>/dev/null \
-        || true
+        || identify_status=$?
+
+    if [ "$identify_status" -ne 0 ]; then
+        # Failed identify (corrupt photo, timeout, missing binary, ...): warn
+        # naming the photo and remove the cache file. Removing it is essential:
+        # a file holding only the signature line is a valid-looking cache hit,
+        # so the next run would silently reuse the empty result forever -- never
+        # retrying identify and never warning again (the original data-loss bug).
+        # Deleting it makes the next run retry and warn.
+        #
+        # We deliberately do NOT abort: this runs inside backgrounded render jobs
+        # under `set -euo pipefail`, and one unreadable photo must not kill the
+        # whole generation. The photo still renders, just with empty tooltip and
+        # stats, now accompanied by a warning.
+        rm -f "$cache_file"
+        log_warning \
+            "could not read EXIF for $photo (ImageMagick identify failed);" \
+            "tooltip/stats will be missing"
+        return 0
+    fi
+
     print_cached_photo_identify_output "$cache_file"
 }
 
-- 
cgit v1.2.3