fix(sidecar): stop silently destroying ratings, tags and EXIF on an unreadable .rrdata - #1422
Draft
magnusrodseth wants to merge 1 commit into
Draft
fix(sidecar): stop silently destroying ratings, tags and EXIF on an unreadable .rrdata#1422magnusrodseth wants to merge 1 commit into
magnusrodseth wants to merge 1 commit into
Conversation
`load_sidecar` returns `ImageMetadata::default()` on any parse failure, and
sidecar writes are read-modify-write on top of it:
let mut metadata = load_sidecar(&sidecar_path); // -> defaults
metadata.adjustments = final_adjustments;
fs::write(&sidecar_path, to_string_pretty(&metadata)?)?;
So a sidecar that fails to parse for any reason, a truncated write, a
half-synced file, a hand-edited one, does not merely fail to load. The next
save writes the defaults back over it, and the image's rating, tags and
cached EXIF are gone with no warning. `save_metadata_and_update_thumbnail`
and `set_color_label_for_paths` both take this path.
An unreadable sidecar is now copied to `<name>.rrdata.bak` before anything
can overwrite it, with a warning naming both paths. The app still continues
with defaults, so one bad file cannot make an image uneditable, and the
behaviour for every sidecar that parses is unchanged.
Quarantine never destroys data. A backup whose contents differ is never
overwritten; the next free slot is used instead. An identical backup is a
no-op, so the repeated loads performed by the thumbnail and metadata workers
cannot pile up copies.
Also preserves unknown top-level keys. `adjustments` is an opaque Value and
already round-tripped anything it did not recognise, but unknown envelope
keys were dropped on every save, so external tooling could not annotate a
sidecar without the GUI silently stripping it. ImageMetadata now carries
them through with #[serde(flatten)].
Adds the repo's first tests (9, covering both quarantine paths and the round
trip) and a `cargo test` job to run them.
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while prototyping #1421, but this stands on its own and needs none of that proposal.
The bug
load_sidecarreturnsImageMetadata::default()on any parse failure, and sidecar writes are read-modify-write on top of it:So a sidecar that fails to parse for any reason (a truncated write, a half-synced file, a hand-edited one) does not merely fail to load. The very next save writes the defaults back over it, and that image's rating, tags and cached EXIF are gone, with nothing logged.
set_color_label_for_pathstakes the same path.Reproducing
The fix
An unreadable sidecar is copied to
<name>.rrdata.bakbefore anything can overwrite it, with a warning naming both paths. The app still continues with defaults, so one bad file cannot make an image uneditable, and behaviour for every sidecar that parses is unchanged.Quarantine never destroys data: a backup whose contents differ is never overwritten (the next free slot is used), and an identical backup is a no-op, so the repeated loads done by the thumbnail and metadata workers cannot pile up copies.
Also: unknown top-level keys are no longer dropped
adjustmentsis an opaqueValueand already round-tripped keys it did not recognise. Unknown envelope keys did not survive, so external tooling could not annotate a sidecar without the GUI silently stripping it on the next save.ImageMetadatanow carries them through with#[serde(flatten)].Tests
This adds the repo's first tests (9) plus a
cargo testjob to run them. They cover both quarantine paths, the no-clobber and no-accumulate properties, the round trip, and that a valid sidecar is never quarantined.I checked they can actually fail: reverting
load_sidecarto the oldunwrap_or_defaultfails exactly the quarantine tests, and removing the flatten fails exactly the round-trip test.Verified beyond unit tests by driving the real headless
exportpath over a JPEG with a deliberately corrupted sidecar: the.bakSHA256 matches the original, the original is untouched, and the export still succeeds.cargo fmt,cargo clippy -D warningsandcargo testall pass.Draft because #1421 is still open and you may want a different shape here. Happy to adjust or split further.
Stacked: #1423 builds on this and contains this commit.