feat(mobile): Prevent premature image cache eviction when higher image loading is enabled

[LeLunZ]

Description

Add an evictOnError flag to loadRequest to make image cache eviction on load failure configurable.
Default behavior is unchanged (evict on error).

This is used to avoid evicting the cache when loadOriginal is enabled, preview loading failed and a the original could still load.

---

Open Question

loadRequest currently completes silently when request.load(...) returns null and if the request didn't get cancelled.
In this case, the cache entry is evicted, but no error is emitted, so subscribers cannot distinguish between waiting on an image and a failed image load.

I think we should throw an error (e.g. in _codec) when the final image load attempt returns null, so subscribers are explicitly notified that image loading failed and can react accordingly? (remove subscription etc.)

How Has This Been Tested?

Not really, as I don't have failing images.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation if applicable
• I have no unrelated changes in the PR.
• I have confirmed that any new dependencies are strictly necessary.
• I have written tests for new code (if applicable)
• I have followed naming conventions/patterns in the surrounding code
• All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
• All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

Please describe to which degree, if any, an LLM was used in creating this pull request.

/

[LeLunZ]

I guess you @mertalev are the reviewer for this again?

[Copilot]

Pull request overview

This pull request adds an evictOnError flag to the loadRequest method to prevent premature image cache eviction when higher resolution image loading is enabled. When loadOriginal is true, the preview image load failures won't evict the cache since the original image might still successfully load. This optimization prevents unnecessary re-downloads when preview loads fail but original images succeed.

Changes:

• Added optional evictOnError parameter to loadRequest method with default value true to maintain backward compatibility
• Modified error handling to conditionally evict cache and propagate errors based on the flag
• Updated both RemoteFullImageProvider and LocalFullImageProvider to compute loadOriginal once and pass evictOnError: !loadOriginal to the preview request

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
mobile/lib/presentation/widgets/images/image_provider.dart	Adds evictOnError parameter to loadRequest method and implements conditional cache eviction and error handling logic
mobile/lib/presentation/widgets/images/remote_image_provider.dart	Computes loadOriginal flag and passes evictOnError: !loadOriginal to preview request to prevent cache eviction when original will be loaded
mobile/lib/presentation/widgets/images/local_image_provider.dart	Computes loadOriginal flag and passes evictOnError: !loadOriginal to preview request for local images

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When evictOnError is false, exceptions are caught and logged but not propagated to the stream. This silently swallows errors, preventing subscribers from being notified of the failure. The stream simply ends without yielding an image, leaving subscribers unable to distinguish between a pending load and a failed load. Consider rethrowing the error after logging, or using a different mechanism to handle non-fatal errors.

Suggested change

	rethrow;
	}
	_log.warning('Non-fatal image load error', e, stack);
	}
	_log.warning('Non-fatal image load error', e, stack);
	rethrow;

[LeLunZ]

Thats intended in this case.

[Copilot]

The loadOriginal check doesn't verify assetType like the RemoteFullImageProvider does (line 96 in remote_image_provider.dart checks 'assetType == AssetType.image'). Consider whether the loadOriginal setting should only apply to images in the local provider as well, for consistency with the remote provider behavior.

Suggested change

	final loadOriginal = Store.get(StoreKey.loadOriginal, false);
	final isImage = key.assetType == AssetType.image;
	final loadOriginal = isImage && Store.get(StoreKey.loadOriginal, false);

[LeLunZ]

That different was already there before this PR, not sure what's better, if the different code is actually needed

[mertalev]

avoid evicting the cache when loadOriginal is enabled, preview loading failed and a the original could still load

This seems unlikely. The main case would be if thumbnails weren't generated yet for the image, but the getInitialImage thumbnail load would run into this issue first.

[LeLunZ]

I don't think getInitialImage would cause problems or? It only subscribes to the Thumbnails Stream, and returns an imageInfo if it was available synchronously, and else it returns null.

The yield* InitialImageStream also doesn't throw any error, and just returns null. Thus the first image request will run.

---

Would it make sense to fallback to the full original image even if "load originals" isn't enabled, in case generated images don't load? Or is the user preference more important in this case?

[mertalev]

I don't think getInitialImage would cause problems or? It only subscribes to the Thumbnails Stream, and returns an imageInfo if it was available synchronously, and else it returns null.

The yield* InitialImageStream also doesn't throw any error, and just returns null. Thus the first image request will run.

Would it make sense to fallback to the full original image even if "load originals" isn't enabled, in case generated images don't load? Or is the user preference more important in this case?

It does a thumbnail request and waits for the image to be fetched, no?

[LeLunZ]

It does a thumbnail request and waits for the image to be fetched, no?

Yes, thats true. But from my understanding if that fetch fails or goes through doesn't affect the rest of the _codec function, as it will just continue.

---

In the RemoteFullImageProvider getInitialImage subscribes to the stream thats getting returned from calling provider.resolve(thumbnailImage) which resolves its own image. This Image thats getting resolved there could evict or throw an error, but that doesn't do anything as that all happens in the RemoteImageProvider while getInitialImage is called from RemoteFullImageProvider.

But I can actually test this, later this week.

[LeLunZ]

@mertalev So, I just did a quick tests and if the thumbnail image isn't working, the _codec method just continues as no error is throw in the getInitiamImage method.

So here what's happening in the RemoteFullImageProvider:

• getInitialImage is called with the image provider RemoteImageProvider.thumbnail(assetId: key.assetId, thumbhash: key.thumbhash) as parameter
• in getInitialImage the imageprovider .resolve is called and a listener is attached
• if the image completes synchronously its getting returned
• if the image doesn't complete null is returned and the operation is cached

Here the code:

ImageInfo? getInitialImage(CancellableImageProvider provider) {
    final completer = CancelableCompleter<ImageInfo?>(onCancel: provider.cancel);
    final cachedStream = provider.resolve(const ImageConfiguration());
    ImageInfo? cachedImage;
    final listener = ImageStreamListener((image, synchronousCall) {
      if (synchronousCall) {
        cachedImage = image;
      }

      if (!completer.isCompleted) {
        completer.complete(image);
      }
    }, onError: completer.completeError);

    cachedStream.addListener(listener);
    if (cachedImage != null) {
      cachedStream.removeListener(listener);
      return cachedImage;
    }

    completer.operation.valueOrCancellation().whenComplete(() {
      cachedStream.removeListener(listener);
      cachedOperation = null;
    });
    cachedOperation = completer.operation;
    return null;
  }

• then _codec is run and subscribed to
• in _coded first yield* initialImageStream runs
• which either just returns nothing, if there is no cached operation
• returns the image if the cached operation completes
• or also returns nothing, if an error is thrown (also no cache eviction happens)

Here the code:

Stream<ImageInfo> initialImageStream() async* {
    final cachedOperation = this.cachedOperation;
    if (cachedOperation == null) {
      return;
    }

    try {
      final cachedImage = await cachedOperation.valueOrCancellation();
      if (cachedImage != null && !isCancelled) {
        yield cachedImage;
      }
    } catch (e, stack) {
      _log.severe('Error loading initial image', e, stack);
    } finally {
      this.cachedOperation = null;
    }
  }

---

So this means even though the initial image is requested, if that one fails, it doesn't change anything in the current provider.

So when an error is thrown in the initial Image stream, it just means, that there is no initial image, and the thumbnail will be loaded next

[LeLunZ]

But if thumbnail generation didn't run yet, it means, that also no thumbnail is shown in the timeline, and a fallback there would also be something that we could think about?
Would that be something the team would merge? Or would an original image be too big to be loaded for the timeline?

[mertalev]

But if thumbnail generation didn't run yet, it means, that also no thumbnail is shown in the timeline, and a fallback there would also be something that we could think about?

Would that be something the team would merge? Or would an original image be too big to be loaded for the timeline?

Originals are way too big for that to be a good idea. It'd use a ton of memory and be a bad experience.

[mertalev]

Would this be logged for cancelled images? It seems potentially noisy.

[LeLunZ]

This would only show for images that get loaded when evictOnError is false, and image is null.
This only happens in the FullImageProvider which is used for the asset view.

But you are right, we don't have to log this, as the eviction in the similar case also isn't logged.

[mertalev]

There's no way a local preview would fail to load but the original would succeed, so the changes here are unnecessary.

[LeLunZ]

Okay, I guess thats true. I didn't really think about that and just wanted to keep the code more or less similar. As the RemoteFullImageProvider and LocalFullImageProvider currently look more or less the same.

But I will revert that change in the local one. Sure.

[LeLunZ]

@mertalev changes are pushed.