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>

4 files changed, 124 insertions, 2 deletions

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"
 }
 
diff --git a/src/lib/album-metadata.source.sh b/src/lib/album-metadata.source.sh
index 9953fe3..2078a85 100644
--- a/src/lib/album-metadata.source.sh
+++ b/src/lib/album-metadata.source.sh
@@ -28,6 +28,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
@@ -52,8 +53,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"
 }
 
diff --git a/tests/cli.sh b/tests/cli.sh
index f7ad977..705ee52 100755
--- a/tests/cli.sh
+++ b/tests/cli.sh
@@ -2083,6 +2083,60 @@ PY
     test::teardown
 }
 
+# Regression for 9n0 (+data-loss): a failed `identify` must warn, must not abort
+# the whole generation, and must NOT leave a cache file containing only the
+# signature line (which the next run would treat as a valid empty cache hit and
+# silently reuse forever). We make the fake ImageMagick fail `identify` for one
+# photo and assert: exit 0, a warning naming that photo, the photo still rendered,
+# and no leftover cache entry (so the next run retries).
+test_generate_warns_and_skips_cache_on_identify_failure() {
+    local config_file
+    local fake_bin
+    local output
+    local status
+
+    test::setup
+    fake_bin="$TEST_TMPDIR/bin"
+    config_file="$TEST_TMPDIR/shuriken.conf"
+
+    test::install_fake_imagemagick "$fake_bin"
+    PATH="$fake_bin:$PATH" \
+        test::generate_fixture_images "$TEST_TMPDIR/incoming"
+    test::write_album_config \
+        "$config_file" "$TEST_TMPDIR/incoming" "$TEST_TMPDIR/dist" \
+        'Identify failure album' 40
+
+    status=0
+    output=$(
+        cd "$TEST_TMPDIR"
+        PATH="$fake_bin:$PATH" \
+            TEST_IMAGEMAGICK_IDENTIFY_FAIL='01-landscape.jpg' \
+            "$TEST_SHURIKEN" --generate 2>&1
+    ) || status=$?
+
+    # A missing-EXIF photo must NOT abort the run.
+    if [ "$status" -ne 0 ]; then
+        printf 'FAIL: expected --generate to succeed, got exit %s\n' \
+            "$status" >&2
+        printf '%s\n' "$output" >&2
+        exit 1
+    fi
+    test::assert_contains \
+        'WARNING: could not read EXIF for 01-landscape.jpg' "$output"
+    test::assert_contains 'tooltip/stats will be missing' "$output"
+
+    # The photo is still rendered despite the EXIF failure.
+    test::assert_file_exists "$TEST_TMPDIR/dist/photos/01-landscape.jpg"
+
+    # No bad cache entry is persisted for the failed photo, so the next run
+    # retries `identify` (and warns again) rather than silently reusing an empty
+    # result. Photos that succeeded are cached normally.
+    test::assert_path_absent "$TEST_TMPDIR/cache/exif/01-landscape.jpg.txt"
+    test::assert_file_exists "$TEST_TMPDIR/cache/exif/02-portrait.jpg.txt"
+
+    test::teardown
+}
+
 test_generate_missing_incoming_fails() {
     local output
 
@@ -5758,6 +5812,9 @@ main() {
         '--generate ignores unsupported incoming files with warning' \
         test_generate_ignores_unsupported_incoming_files_with_warning
     test::run_case \
+        '--generate warns and skips cache on identify failure' \
+        test_generate_warns_and_skips_cache_on_identify_failure
+    test::run_case \
         '--generate missing incoming fails' \
         test_generate_missing_incoming_fails
     test::run_case \
diff --git a/tests/helpers.sh b/tests/helpers.sh
index d2d6605..477acfc 100755
--- a/tests/helpers.sh
+++ b/tests/helpers.sh
@@ -196,6 +196,17 @@ if [ "${1:-}" = identify ]; then
     if [ -n "${TEST_IMAGEMAGICK_IDENTIFY_LOG:-}" ]; then
         printf '%s\n' "$*" >> "$TEST_IMAGEMAGICK_IDENTIFY_LOG"
     fi
+    # Simulate a corrupt photo / identify failure when the fixture matches the
+    # configured failing target (or "*" for any photo). Emit an error to stderr
+    # and exit non-zero so callers can exercise the failure path.
+    if [ -n "${TEST_IMAGEMAGICK_IDENTIFY_FAIL:-}" ]; then
+        target="${@: -1}"
+        if [ "$TEST_IMAGEMAGICK_IDENTIFY_FAIL" = '*' ] \
+            || [[ "$target" == *"$TEST_IMAGEMAGICK_IDENTIFY_FAIL"* ]]; then
+            printf 'identify: corrupt image\n' >&2
+            exit 1
+        fi
+    fi
     if [ -n "${TEST_IMAGEMAGICK_IDENTIFY_OUTPUT:-}" ]; then
         printf '%s\n' "$TEST_IMAGEMAGICK_IDENTIFY_OUTPUT"
     fi