Add a verify command that confirms Immich can render each file - #35
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## develop #35 +/- ##
===========================================
- Coverage 44.96% 43.37% -1.59%
===========================================
Files 25 28 +3 Lines 3398 3896 +498 Branches 259 306 +47 ===========================================
+ Hits 1528 1690 +162 - Misses 1824 2154 +330 - Partials 46 52 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new verify pipeline stage to detect media that uploads to Immich but cannot be rendered for previews, by combining an in-process ISO-BMFF structural validation pass with an optional Docker-based decode pass that runs Immich’s own thumbnail pipeline. The PR also standardizes command exit codes (0/1/2) and enables exiftool -validate in existing metadata reads so process/import can mark/skip files with validation errors.
Changes:
- Introduces
verify(CLI + orchestration + decode script) plus an ISO-BMFF structural validator for HEIC/HEIF/MOV/MP4/3GP. - Enables exiftool
-validate -allin the shared metadata read and adds validation parsing/handling inprocessandimport. - Standardizes exit-code semantics across commands and updates documentation, launch configs, spelling dictionary, and tests accordingly.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents verify, --quick, and standardized exit codes; adds verify flow and examples. |
| PhotoCleanerTests/VerifyTaskTests.cs | Adds unit tests for verify output parsing and script/image contract checks. |
| PhotoCleanerTests/UndoTaskTests.cs | Updates options initialization for new Quick option property. |
| PhotoCleanerTests/TrashCommandTests.cs | Updates options initialization and minor comment cleanup. |
| PhotoCleanerTests/TempDirectoryFixture.cs | Comment tweaks and maintains exiftool invocation behavior in tests. |
| PhotoCleanerTests/ProcessTaskTests.cs | Adds tests covering exiftool validation behavior in process. |
| PhotoCleanerTests/IsoBmffValidatorTests.cs | Adds synthesized-container tests for structural ISO-BMFF validation. |
| PhotoCleanerTests/IndexTaskTests.cs | Updates options initialization and minor comment adjustments. |
| PhotoCleanerTests/ImportTaskTests.cs | Updates expected tuple shape/counts to include invalid results from import. |
| PhotoCleanerTests/ExifToolJsonTests.cs | Adds tests for parsing ExifTool:Validate verdict strings. |
| PhotoCleanerTests/DirectoryCleanerTests.cs | Minor comment clarifications only. |
| PhotoCleanerTests/CommandLineTests.cs | Adds CLI parsing tests for the new verify command and --quick. |
| PhotoCleaner/VerifyTask.cs | Implements quick structural mode, full decode mode, Docker preflight, batching, and result parsing. |
| PhotoCleaner/VerifyResult.cs | Adds verify result/JSON protocol model + source-gen context. |
| PhotoCleaner/VerifyCommand.cs | Adds command orchestration, counting, logging, and exit-code mapping. |
| PhotoCleaner/UndoTask.cs | Comment clarifications (no functional change). |
| PhotoCleaner/UndoCommand.cs | Returns standardized exit codes based on per-file failures. |
| PhotoCleaner/TrashCommand.cs | Returns standardized exit codes for command completion. |
| PhotoCleaner/ProcessTask.cs | Adds Invalid result and acts on exiftool validation errors before processing pipeline. |
| PhotoCleaner/ProcessCommand.cs | Tracks invalid count and exits with 2 when invalid/failed files are present. |
| PhotoCleaner/MediaUtilities.cs | Adds -validate -all to shared exiftool metadata read. |
| PhotoCleaner/IsoBmffValidator.cs | Adds total-by-construction ISO-BMFF box-walk validator and defect reporting. |
| PhotoCleaner/IndexCommand.cs | Returns standardized exit codes based on per-file failures. |
| PhotoCleaner/ImportTask.cs | Adds Invalid import result for exiftool validation errors and reports counts. |
| PhotoCleaner/ImportCommand.cs | Logs invalid count and returns standardized exit codes. |
| PhotoCleaner/ImmichVerifyScript.cs | Adds in-container Node script strings (preflight + per-path verification via Immich modules). |
| PhotoCleaner/ExitCode.cs | Introduces shared exit-code contract constants (0/1/2). |
| PhotoCleaner/ExifToolJson.cs | Adds ExifTool:Validate field and parsing helper for error/warning counts. |
| PhotoCleaner/CommandRunner.cs | Updates runner to return the command’s exit code and standardize cancellation/error handling. |
| PhotoCleaner/CommandLine.cs | Registers verify command and adds --quick option + Options.Quick field. |
| HISTORY.md | Adds release notes for verify, validate behavior, and exit-code breaking change. |
| cspell.json | Adds new domain terms used by verify/ISO-BMFF validation and docs. |
| .vscode/launch.json | Adds launch profiles for verify and verify --quick. |
| .github/copilot-instructions.md | Updates architecture/docs to include verify components and exit-code contract. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (2)
PhotoCleaner/VerifyTask.cs:96
- The comment says quick-mode verification is "not recorded in the database", but quick mode still calls IndexTask.IndexFileAsync when --db is provided (inserting/updating hash state). What’s true is that quick mode does not mark files as verified (is_processed) so a later full run won’t skip decode. Consider updating the comment to match the actual behavior to avoid confusion during maintenance.
// Not recorded in the database, so a later full run cannot skip a file never decoded.
tally.Verified();
README.md:306
- The README says a
--quickrun "never records anything", butverify --quick --db ...still writes/updates rows via IndexTask (it just doesn’t mark files as verified). This wording could mislead users into thinking the DB is untouched. Consider rephrasing to clarify that quick mode avoids setting the verified bit rather than avoiding all DB writes.
ptr727
commented
Aug 2, 2026
Both suppressed comments from the last review were correct and are fixed in 98a20c3. The claim that a Comment now reads Also in this round, from the earlier review:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (2)
PhotoCleaner/VerifyTask.cs:213
PassesStructuralChecktreats I/O or other unexpected exceptions as a pass (return true). In--quickmode that means the file is counted asVerifiedeven though it could not be read/checked, which produces an incorrect success verdict for tooling/permission problems. These cases should be counted asFailedand the file should not be marked verified.
catch (Exception ex)
{
// A parser failure is not evidence the file is bad.
Log.Warning(ex, "Structural check could not read '{FilePath}'", file);
return true;
PhotoCleaner/VerifyTask.cs:179
ShouldVerifyAsynccallsIndexFileAsyncwithout any per-file exception handling. If hashing/indexing throws (e.g., file deleted mid-run, access denied, transient IO error), the exception will bubble out of theParallel.ForEachAsyncbody and abort the entire verify run rather than counting just that file as failed.
Consider catching non-cancellation exceptions here, logging the file path, incrementing Failed, and skipping verification for that file so the rest of the collection can still be verified.
if (database is not null)
{
IndexTask indexTask = new(options, database, skippedExtensions);
(IndexStatus status, _, _, bool wasVerified) = await indexTask
.IndexFileAsync(file, cancellationToken)
.ConfigureAwait(false);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:213
- In
PassesStructuralCheck, any exception (e.g., file missing, access denied, transient I/O error) currently logs a warning but returnstrue. In--quickmode that means the file is then counted asVerifiedeven though it was never actually checked, which can produce false-clean results.
Treat an inability to read the file as a tooling failure for that file: increment Failed and return false so it is not counted as verified (and in full mode it will not be sent to Immich either).
catch (Exception ex)
{
// A parser failure is not evidence the file is bad.
Log.Warning(ex, "Structural check could not read '{FilePath}'", file);
return true;
}
ptr727
commented
Aug 2, 2026
The suppressed comment on The catch-all logged a warning and returned Rather than invent handling, I compared against the per-file pattern already used by
The structural check now follows that pattern exactly. Neither path reports the file as verified, and neither reports it as invalid, because being unable to read a file is not evidence of damage. In full mode the file is also no longer handed to the decode pass, which would otherwise have risked a false Added a regression case using an unreadable file. Restoring the old catch fails it on On process: low-confidence findings are being treated as first-class here, not skimmed. This one and the two in the previous round were all real, and GOVERNANCE.md requires the collapsed findings to be investigated and answered precisely because they appear in no thread and a thread-polling loop would report a clean pass while they stand. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (1)
README.md:332
- The Exit Codes section says “Only a
2means files were actually found to be bad”, butverifyreturns exit code2whencounts.Failed > 0as well (e.g., an unreadable file or a missing verdict line). Those cases are tooling/coverage gaps rather than “files were bad”, so the current wording can cause scripts (including the example below) to misinterpret the outcome.
Consider clarifying that 2 means “one or more files were invalid or could not be verified”, while 1 remains “verification could not run at all / preflight failed.”
ptr727
commented
Aug 2, 2026
The suppressed comment on the exit-code section was correct and is fixed in a023731. It caught a second-order effect of the previous round's fix: once an unreadable file counted against The table row now reads invalid or could not be verified, and the prose points at the Also in this round, 4ea2f09 raises the decode batch from 256 to 1024. Per-file decode cost turned out to span fiftyfold across real media - about 20ms for a thumbnail against 1103ms for HEIC and video - so at 256 a batch of small images spent 37 percent of its time starting the container. 1024 puts the cheap end at sevenfold and typical photographs near fiftyfold, without producing fewer batches than threads on a small tree. Verification against a real 28,965-file collection: 7 damaged JPEGs found, 0 false positives, 0 failures. All seven were confirmed independently with ffmpeg, and every one was caught only by the decode pass - |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:213
PassesStructuralChecktreats a missing file as a silent skip: it returnsfalsewithout incrementingFailed, and the exception filter|| !File.Exists(file)can also match non-"missing" failures (e.g., paths that makeFile.Existsreturn false). This can makeverifyexit 0 even though some files were never verified.
Count this case as Failed and narrow the filter to actual missing-file exceptions so other I/O errors are still counted as failures.
catch (Exception ex) when (ex is FileNotFoundException || !File.Exists(file))
{
Log.Information("File no longer exists during verification: '{FilePath}'", file);
return false;
}
ptr727
commented
Aug 2, 2026
Verified the finding before acting, then split it, because the two halves are not equivalent. The filter breadth is a real bug, fixed in 6e1aefe and applied across the codebase in 44637e9. I tested the claim rather than taking it on faith: So The same filter existed in Counting a genuinely missing file as For the record on process, all five suppressed comments raised on this PR have been real, and each was checked against the code and the existing patterns before being acted on or declined. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (2)
PhotoCleaner/VerifyTask.cs:215
- When a file disappears between enumeration and verification, it currently logs and returns without incrementing any counter. That can make the final counts (and exit code) look like a clean pass even though some paths were never verified. Treat this as a tooling/coverage failure (like unreadable files / missing verdicts) so callers can distinguish “all files verified” from “some files were not checked”.
catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException)
{
// File.Exists also returns false when the path cannot be read at all.
// Matching the exception alone keeps a permission error out of this uncounted branch.
Log.Information("File no longer exists during verification: '{FilePath}'", file);
return false;
PhotoCleaner/IsoBmffValidator.cs:297
extentCountcomes from untrusted file bytes, but it’s used to pre-size theList<Extent>. A malformed file can declare a very large extent count and cause a large allocation (or OOM) before any bounds checks fail. Avoid pre-sizing from the on-disk value; let the list grow as extents are successfully parsed.
int extentCount = BinaryPrimitives.ReadUInt16BigEndian(buffer[..2]);
List<Extent> extents = new(extentCount);
for (int e = 0; e < extentCount; e++)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:180
- VerifyTask.ShouldVerifyAsync allocates a new IndexTask for every file. Besides allocation overhead on large trees, IndexTask.IndexFileAsync logs at Information level per file, so a normal
verify --db ...run at the default log level will emit every file path (very noisy and potentially sensitive) even when everything verifies cleanly.
Consider indexing directly here (using Database.ResolveHashesAsync + Insert/Update calls) or otherwise reusing/suppressing the per-file IndexTask logging so verify stays “log only defects” by default.
if (database is not null)
{
IndexTask indexTask = new(options, database, skippedExtensions);
(IndexStatus status, _, _, bool wasVerified) = await indexTask
.IndexFileAsync(file, cancellationToken)
ptr727
commented
Aug 2, 2026
Both suppressed comments from review 4839916844 confirmed and fixed in ae86b42. I found them while checking whether this PR was clean enough to merge, so it is worth saying plainly that it was not, and this review is why. Unreadable file counted Fixed with the cheap probe open the comment suggested, in the no-database path only, letting the existing guard count the throw so both paths reach one verdict. Worth noting this restores a guarantee I had removed. When the container parser came out in 60dd072 I deleted the equivalent test, reasoning that the premise no longer held because the container runs as root and can read a file the host process cannot. That reasoning was wrong: the point was never whether Immich can read the file, it was that PhotoCleaner could not, and a tool that cannot read a file has learned nothing about the media. The test is back.
All gates clean, 368 tests pass. |
ptr727
commented
Aug 2, 2026
Audited every Copilot review on this PR again before considering a merge, and found three suppressed sets I had never answered: 4839594134, 4839799677, and 4839840931. Answering all of them now. Most were overtaken by later commits, but three were live and two of those were real defects. Live and fixed.
Declined, with reasons.
Obsolete, listed so the record is complete: the One test note, since it matters more than the finding. I first wrote All gates clean, 368 tests pass. |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:82
VerifyTask.RunAsyncalways runsPreflightAsync(Docker + image check) even when there are no supported media files to verify. This makesverifyfail with exit code 1 on hosts without Docker even if the input contains only ignored non-media files (or an empty list), and adds avoidable startup cost for those no-op runs.
Consider short-circuiting before the preflight when there are zero supported extensions in files (and optionally, in a follow-up, deferring preflight until at least one post-DB-skip candidate exists).
await PreflightAsync(cancellationToken).ConfigureAwait(false);
ptr727
commented
Aug 3, 2026
Suppressed comment from review 4844671099 confirmed and fixed in e1d846d. The preflight ran before the command looked at what it had been given, so a directory holding no supported media exited Preflight now runs only when at least one file carries a supported extension. The partition loop is untouched, so non-media files are still counted Proven end to end rather than argued, because a unit test on this host cannot tell the two apart. With
I did not take the follow-up half of the suggestion, deferring preflight until at least one post-database-skip candidate exists. It would mean a full pass over the file list, including hashing every file, before the first container starts, which trades a guaranteed cost for an occasional saving and delays the point where an unreachable Docker is reported. Worth revisiting only if repeat no-op runs over large trees become common. All gates clean, 369 tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:406
docker run -v "{mount}:{ContainerMount}:ro"breaks when the host path contains a ':' (valid on Linux/macOS), because Docker uses ':' as the volume-spec separator. That makesverifyfail on some perfectly valid media directories.
Use --mount type=bind,... instead, which avoids ':'-delimited parsing and supports ':' in source paths.
if (mount is not null)
{
arguments.Add("-v");
arguments.Add($"{mount}:{ContainerMount}:ro");
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:88
PreflightAsyncis triggered based only on the presence of any media extension in the input set. When--dbis used, it’s possible for every media file to be skipped as already-verified/unchanged (and--reprocessis false), in which case nothing will be decoded—butverifywill still require Docker and will fail with exit code 1 on hosts without Docker.
This contradicts the intended behavior (“Nothing to decode means nothing needs Docker”) and defeats the point of database-backed incremental runs.
if (files.Any(file => MediaUtilities.SupportedExtensions.Contains(Path.GetExtension(file))))
{
await PreflightAsync(cancellationToken).ConfigureAwait(false);
}
ptr727
commented
Aug 3, 2026
Suppressed comment from review 4844778651 confirmed and fixed in 3c0c9bc, but not by the suggested change, which on its own would have made things worse. The defect is real. With a colon in the media directory, docker rejects the spec and the whole batch dies: The suggested remedy trades one delimiter for another. Tested rather than reasoned, on docker 29.6.2:
Quoting the value inline as No unit test: the failure lives in an argument docker parses, so nothing below the process boundary observes it. The four end-to-end runs above are the evidence, and the reasoning is recorded in the code so the next reader does not "simplify" it back to either naive form. All gates clean, 369 tests pass. |
ptr727
commented
Aug 3, 2026
Suppressed comment from review 4844995179 confirmed and fixed in 99af2c7. This is the follow-up half I declined one round earlier, and the decline was wrong. The finding is right on both counts. A database-backed run that skips every file decoded nothing yet still demanded Docker, and my own comment claimed "nothing to decode means nothing needs Docker" while the code only honoured that for a tree holding no media at all. The comment overclaimed relative to the code, which is the thing I have been fixing elsewhere in this branch. Why the earlier decline was wrong. I argued it would mean "a full pass over the file list, including hashing every file, before the first container starts". That is not how the loop works. Batches are processed as they are chunked, so the first batch to find a candidate triggers the preflight, after about one batch of files rather than the whole tree. The exposure I was protecting against is bounded by a batch, which is seconds, not the hours I implied. I should have checked the loop instead of reasoning from a mental model of it. The preflight is now a Verified with Case 2 is the one that fails on the previous commit. Case 3 confirms the fail-fast behaviour and the message survive the change, which was the property I was worried about losing. All gates clean, 369 tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/VerifyTask.cs:172
- Unreadable files are only probed when no database is configured (
_indexTask is null). With a database,IndexTask.IndexFileAsynccan hit the size/mtime cache (Database.ResolveHashesAsync) and return cached hashes without ever opening the file, which means an unreadable file could be incorrectly treated as already-verified and skipped (or even verified by Docker running under different permissions). To keep the behavior consistent with the intent (“unreadable counts as Failed regardless of DB”), probe readability unconditionally before any skip/verify decision.
// Without one nothing opens it before the container does.
// An unreadable file would then reach Immich and come back as unrenderable.
// That reports a tooling gap as damage, so probe here and let the per-file guard count it.
if (_indexTask is null)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/CommandRunner.cs:30
- CommandRunner logs "{CommandName} complete" for any exit code other than ExitCode.Failed. That includes ExitCode.Error (1), which represents a command that did not complete successfully, so the log message is misleading for callers troubleshooting failures.
Consider logging success only for ExitCode.Success, and logging an error (or at least warning) for ExitCode.Error / any other non-zero code.
if (exitCode == ExitCode.Failed)
{
Log.Warning("{CommandName} complete with failures", commandName);
}
else
{
Log.Information("{CommandName} complete", commandName);
}
ptr727
commented
Aug 3, 2026
Suppressed comment from review 4845112782 confirmed and fixed in 7025c3c. The premise checks out exactly as described. That matters because Immich runs as another user and may well read the file, so a gap in this tool's access came back as a verdict about the media, which is the outcome this branch has been closing off everywhere else. Where the probe went, and why not simply unconditional. The comment suggested probing "before any skip/verify decision". Placing it at the end of the skip decision instead covers exactly the files that get judged, and costs nothing on the ignored, missing, and already-verified paths. That distinction is not cosmetic here: an incremental run over a large tree spends nearly all of itself on the skip path, and an unconditional probe would add an open per file to precisely the case
This is the third variant of one defect found across three rounds, each in a path the previous fix did not reach: no database at all, then the vanished-file case after the batch was assembled, now the cached-hash case. Worth naming, because the pattern is that any route to the decoder which skips opening the file will produce a media verdict for a tooling problem, and the fix only holds where every such route is covered. All gates clean, 370 tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (2)
PhotoCleaner/VerifyTask.cs:152
- When
filehas no extension,Path.GetExtension(file)returns an empty string, soSkippedExtensionTrackerends up tracking "" and later logsUnknown file extension: '', which is confusing/noisy. Consider skipping tracking when the extension is empty (or mapping it to a clear placeholder).
if (!MediaUtilities.SupportedExtensions.Contains(extension))
{
Log.Debug("Skipping non-media file: '{FilePath}'", file);
skippedExtensions.Track(extension);
tally.Ignored();
PhotoCleaner/TrashCommand.cs:157
- This comment overstates the behavior: the loop can stop early on an invalid
NextPagevalue (logging a warning) without throwing, which is a partial sync. Consider rewording so it doesn't imply there is no partial-success/partial-sync scenario.
// A page either fetches or throws, so there is no partial-failure state to report.
return ExitCode.Success;
ptr727
commented
Aug 3, 2026
Both suppressed comments from review 4845224642 confirmed and fixed in 04f0e66, and the earlier readability finding is now handled in the shared path in 3e41038. Empty extension tracking. Correct. A file with no extension tracks as an empty string, so the summary printed The trash comment overstating. Correct, and the consequence is larger than the wording. The loop does stop early on a I have not changed Readability, from the previous round, now shared. The maintainer pointed out that reading attributes needs no permission on content, and that this affects every command rather than verify alone. Measured against an unreadable file before the change: Keeping the probe ahead of exiftool is what makes it correct. exiftool reports a file it cannot open and a file it cannot parse identically, as an error alongside well formed JSON, so keying on that field instead would have turned damaged media into a tooling failure. I tried that first and it broke two existing tests that assert garbage bytes count All gates clean, 371 tests pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleaner/TrashCommand.cs:157
- The comment claims there is “no partial-failure state to report”, but this loop inserts hashes page-by-page inside the pagination loop. If a later page fetch/insert throws after earlier pages succeeded, the command will exit with ExitCode.Error but the Trash DB may already be partially updated. The comment should reflect that partial updates are possible on mid-run failures.
// A page either fetches or throws.
// Pagination also stops early on a NextPage value it cannot use, warned above.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/copilot-instructions.md:366
- The CLI now includes the new
verifysubcommand, but this bullet still documents only five subcommands and omitsverify, which makes the command list here inconsistent withCommandLine.csand the rest of the PR docs.
- **Five subcommands**: `process`, `undo`, `import`, `index`, `trash`, each with their own option set
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
PhotoCleanerTests/VerifyTaskTests.cs:486
ImmichImageAvailable()redirects stdout/stderr fordocker image inspectbut never reads from those streams. When a process writes enough output to fill the redirected buffer,WaitForExitcan deadlock and hang the test run. Since the test only needs the exit code, avoid redirecting these streams (or read them asynchronously).
ptr727
commented
Aug 3, 2026
Suppressed comment from review 4845477898 confirmed and fixed in the commit above. The pattern is wrong exactly as described: both streams were redirected and neither was read, so the child can fill a pipe buffer and block on the write while Measured before changing it, since "can deadlock" and "does deadlock" are different claims: Fixed on both sides. Both streams are read before the wait, and the probe now passes Confirmed the probe still discriminates: exit All gates clean, 372 tests pass. |
ptr727
commented
Aug 3, 2026
Auditing every review before merging turned up three suppressed findings that were never answered, across reviews 4845161481, 4845319556, and 4845366389. All three are now fixed in the commit above. A fourth, 4839450732, was the
Separately, while running the final check the five Docker-gated All gates clean, 372 tests pass, zero unresolved threads. |
Adds
verify, a standalone pipeline step that answers whether Immich can generate a preview for a file, and turns on exiftool-validatein the metadata readprocessandimportalready perform.Fixes#25.
Why
Issue #25 describes HEIC files that are byte-complete, pass every existing check, upload successfully, and then fail thumbnail generation forever. An
ilocbox re-encoded from version 1 to version 0 losesconstruction_method, so thegriddescriptors stored inidatare read as absolute file offsets and land on theftypheader.That is one of three classes. Two more are open against Immich: DNGs libraw cannot parse, and RAW from cameras it does not recognize. Neither is corruption, so no parser can predict them. Only running the decoder Immich runs can.
How
Immich's decoder is the only judge.
verifycalls Immich's own compiledMediaRepository,defaults, andThumbnailConfiginside theimmich-serverimage. Calling Immich's code rather than reimplementing its pipeline means behavior tracks Immich across releases; the coupling surface is three module paths, which the preflight checks. Paths are streamed in batches over stdin so container startup is paid once per batch rather than once per file.Docker is required, and there is no offline mode. A preflight runs before any file is judged, so an unreachable Docker exits
Errorrather than condemning the collection.PhotoCleaner carries no container parser of its own, deliberately. A hand-rolled parser condemns whatever it fails to understand, and a format it has never met is indistinguishable from a damaged one from the inside, so it would report other people's valid media as corrupt on evidence it could not actually read. The cost is the truncation signal: a truncated file decodes cleanly, so no decoder reports it. That cost is accepted rather than guessed around.
A file that cannot be read, or that vanishes mid-run, counts as failed rather than invalid, since neither is evidence of damage.
processis the exception and logs at information without failing, because it renames as it walks and cannot cheaply tell its own work from an external deletion.Exit codes
All six commands now share one contract:
0success,1could not run,2completed with per-file failures. This is a behavior change -processpreviously exited0while logging failed files.exiftool validation
-validate -allfolds into the existing call at no measured cost. Only an error count fails a file: measured across a real collection, ~75% of healthy files carry validation warnings, so warnings are logged at debug level only.Verification
Input file has corrupt header: ... bad seek, the same error as the upstream report.exiftool -validatecalled four of them OK, andffmpegreported damage while still exiting 0.1with a clear message and marks no file invalid.Sample files are documented alongside the corpus, including one that is structurally intact and fails only in the decode pass.