Fix: Range header forwarding (Safari/iOS video fix)#595
Merged
Conversation
added 9 commits
February 2, 2026 14:08
…ent caching (same as images)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAsset retrieval was refactored to return a structured Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as AssetController
participant Logic as PooledImmichFrameLogic
participant API as ImmichApi
participant HTTP as HttpClient
rect rgba(100, 150, 255, 0.5)
Note over Client,HTTP: Range Request Flow (206 Partial Content)
Client->>Controller: GET /asset?id=X\nRange: bytes=0-1023
Controller->>Controller: Read Range header
Controller->>Logic: GetAsset(id, assetType, rangeHeader)
Logic->>Logic: Detect rangeHeader present
alt Video Asset
Logic->>API: PlayAssetVideoWithRangeAsync(id, rangeHeader)
API->>HTTP: GET /assets/{id}/video/playback with Range header
HTTP-->>API: 206 response + Content-Range + stream
API-->>Logic: FileResponse (status, headers, stream)
Logic-->>Controller: AssetResponse(stream, ContentRange, IsPartial=true, ContentLength, Owner)
else Image Asset
Logic->>API: GetImageAsset(id)
API-->>Logic: 200 response + full stream
Logic-->>Controller: AssetResponse(stream, IsPartial=false)
end
Controller->>Controller: Set Status 206, Content-Range, Accept-Ranges, Content-Length
Controller->>Client: Stream partial content
end
rect rgba(150, 255, 100, 0.5)
Note over Client,HTTP: Normal Request Flow (200 OK)
Client->>Controller: GET /asset?id=Y (no Range)
Controller->>Logic: GetAsset(id, assetType, null)
Logic->>API: Standard asset retrieval
API-->>Logic: full stream
Logic-->>Controller: AssetResponse(stream, IsPartial=false)
Controller->>Controller: Send webhook notification
Controller->>Client: File(...) response (200 OK)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
16-28: SharingHttpClientbetween generated client and manual requests is fine, but worth a brief note.
_httpClientis the same instance that's passed toImmichApi. This is desirable (same auth, base config), but it means any default headers or handler changes on the shared client affect both paths. A short comment on line 27 clarifying the intentional reuse would aid readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 16 - 28, Add a short inline comment near the HttpClient assignment/usage (around where _httpClient is set and new ImmichApi(...) is constructed) clarifying that the HttpClient instance returned from httpClientFactory and passed into ImmichApi is intentionally shared for consistent auth/configuration (via UseApiKey) and that changes to default headers or message handlers will affect both the generated ImmichApi client and any manual requests made with _httpClient; reference the symbols _httpClient, httpClientFactory.CreateClient("ImmichApiAccountClient"), UseApiKey(accountSettings.ApiKey), and ImmichApi to locate the spot.
166-191: Hardcoded API path is a maintenance risk; also consider propagatingCancellationToken.The endpoint
/assets/{id}/video/playbackis hardcoded (Line 168), duplicating knowledge that's otherwise encapsulated in the generated API client. If the Immich API path changes, this will silently break. A brief code comment noting this coupling would help future maintainers.Also,
GetVideoAsset(line 193) andGetAsset(line 86) don't accept or propagate aCancellationToken, so if a client disconnects mid-stream, the upstream HTTP call inPlayVideoWithRangecan't be cancelled. Consider threadingCancellationTokenthrough from the controller (e.g., viaHttpContext.RequestAborted).♻️ Minor: add a comment about the hardcoded path
private async Task<FileResponse> PlayVideoWithRange(Guid id, string rangeHeader, CancellationToken cancellationToken = default) { + // NOTE: This path must stay in sync with the Immich API's video playback endpoint + // (also used by the generated client's PlayAssetVideoAsync). var url = $"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 166 - 191, PlayVideoWithRange currently constructs a hardcoded path using _immichApi.BaseUrl and doesn't fully propagate a CancellationToken; add a short inline comment near the url construction in PlayVideoWithRange calling out the coupling to the Immich API path ("/assets/{id}/video/playback") so future maintainers notice the dependency, and then thread CancellationToken through the call chain by adding a CancellationToken parameter to GetVideoAsset and GetAsset (and any callers, e.g., controller actions) and passing that token into PlayVideoWithRange and into SendAsync/ReadAsStreamAsync calls so upstream HTTP requests can be cancelled (use HttpContext.RequestAborted from controllers when invoking GetVideoAsset/GetAsset).ImmichFrame.WebApi/Controllers/AssetController.cs (2)
139-148:asset.disposenot disposed inGetRandomImageAndInfo.The
GetAssetcall returns a tuple with adisposemember, but onlyfileStreamis consumed here. For the IMAGE pathdisposeis currentlynull, so no leak today — but if a future code path returns a non-null disposable, this will leak. Defensive disposal is cheap.♻️ Suggested fix
var asset = await _logic.GetAsset(new Guid(randomAsset.Id), AssetTypeEnum.IMAGE); var notification = new AssetRequestedNotification(new Guid(randomAsset.Id), sanitizedClientIdentifier); _ = _logic.SendWebhookNotification(notification); string randomImageBase64; - using (var memoryStream = new MemoryStream()) + try { - await asset.fileStream.CopyToAsync(memoryStream); - randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); + using (var memoryStream = new MemoryStream()) + { + await asset.fileStream.CopyToAsync(memoryStream); + randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); + } + } + finally + { + asset.fileStream.Dispose(); + asset.dispose?.Dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 139 - 148, The tuple returned by _logic.GetAsset (captured in local variable asset) includes a disposable member named dispose that is not being disposed in GetRandomImageAndInfo; update the method to ensure the returned dispose is cleaned up (e.g., wrap the disposable in a using or call dispose?.Dispose() after consuming asset.fileStream) so any future non-null disposables aren't leaked while keeping the existing fileStream handling and webhook send logic unchanged.
91-108: Partial content path: the stream and response disposal ordering looks correct, butfileStreamis disposed twice.On line 102,
await using (asset.fileStream)disposes the stream. Then on line 106,asset.dispose?.Dispose()disposes theFileResponse/HttpResponseMessage, which also internally disposes its content stream. Double-disposal is tolerated byStreamimplementations in .NET (it's a no-op), so this isn't a bug, but the intent could be clearer by only disposing the owning response object.Also, if
CopyToAsyncthrows,asset.disposeon line 106 won't be reached. Consider wrapping both in atry/finallyor usingResponse.RegisterForDisposehere as well.♻️ Suggested cleanup for the partial content path
if (asset.isPartial && !string.IsNullOrEmpty(asset.contentRange)) { Response.Headers["Content-Range"] = asset.contentRange; Response.StatusCode = 206; Response.ContentType = asset.ContentType; if (asset.fileStream is { CanSeek: true } && asset.fileStream.Length > 0) Response.ContentLength = asset.fileStream.Length; else if (!string.IsNullOrEmpty(asset.contentLength) && long.TryParse(asset.contentLength, out var length)) Response.ContentLength = length; - await using (asset.fileStream) - { - await asset.fileStream.CopyToAsync(Response.Body); - } - asset.dispose?.Dispose(); - return new EmptyResult(); + try + { + await asset.fileStream.CopyToAsync(Response.Body); + } + finally + { + asset.dispose?.Dispose(); + } + return new EmptyResult(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 91 - 108, The partial-content branch currently disposes asset.fileStream via "await using (asset.fileStream)" and then calls asset.dispose?.Dispose(), risking double-disposal and missing dispose if CopyToAsync throws; remove the "await using (asset.fileStream)" and instead ensure the owning disposable is always cleaned up by registering or finally-disposing the owner (use Response.RegisterForDispose(asset.dispose) or wrap Response.Body.CopyToAsync in try/finally and call asset.dispose?.Dispose() in finally), keep setting Response.Headers/StatusCode/ContentType/ContentLength and return EmptyResult as before so the stream itself is not separately disposed inside this block.ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
12-12: Consider replacing the 7-element tuple with a named record/class.This tuple is becoming unwieldy and hard to maintain across the interface, two implementations, and the controller. A dedicated type would improve readability, make future additions non-breaking, and fix the inconsistent naming (
ContentTypeis PascalCase while the rest are camelCase).♻️ Suggested record type
public record AssetResult( string FileName, string ContentType, Stream FileStream, string? ContentRange, bool IsPartial, IDisposable? ResponseDisposable, string? ContentLength);Then the signature simplifies to:
-public Task<(string fileName, string ContentType, Stream fileStream, string? contentRange, bool isPartial, IDisposable? dispose, string? contentLength)> GetAsset(Guid id, AssetTypeEnum? assetType = null, string? rangeHeader = null); +public Task<AssetResult> GetAsset(Guid id, AssetTypeEnum? assetType = null, string? rangeHeader = null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs` at line 12, Replace the 7-element tuple return from IImmichFrameLogic.GetAsset with a named record to improve readability and maintainability: define a record (suggested name AssetResult) with PascalCase properties (FileName, ContentType, FileStream, ContentRange, IsPartial, ResponseDisposable, ContentLength), change the IImmichFrameLogic.GetAsset signature to return Task<AssetResult>, and update all implementations and the controller usages to construct and consume the AssetResult instead of the tuple (ensuring you forward the disposable via ResponseDisposable and preserve semantics for partial responses and content length).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs`:
- Line 12: Replace the 7-element tuple return from IImmichFrameLogic.GetAsset
with a named record to improve readability and maintainability: define a record
(suggested name AssetResult) with PascalCase properties (FileName, ContentType,
FileStream, ContentRange, IsPartial, ResponseDisposable, ContentLength), change
the IImmichFrameLogic.GetAsset signature to return Task<AssetResult>, and update
all implementations and the controller usages to construct and consume the
AssetResult instead of the tuple (ensuring you forward the disposable via
ResponseDisposable and preserve semantics for partial responses and content
length).
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 16-28: Add a short inline comment near the HttpClient
assignment/usage (around where _httpClient is set and new ImmichApi(...) is
constructed) clarifying that the HttpClient instance returned from
httpClientFactory and passed into ImmichApi is intentionally shared for
consistent auth/configuration (via UseApiKey) and that changes to default
headers or message handlers will affect both the generated ImmichApi client and
any manual requests made with _httpClient; reference the symbols _httpClient,
httpClientFactory.CreateClient("ImmichApiAccountClient"),
UseApiKey(accountSettings.ApiKey), and ImmichApi to locate the spot.
- Around line 166-191: PlayVideoWithRange currently constructs a hardcoded path
using _immichApi.BaseUrl and doesn't fully propagate a CancellationToken; add a
short inline comment near the url construction in PlayVideoWithRange calling out
the coupling to the Immich API path ("/assets/{id}/video/playback") so future
maintainers notice the dependency, and then thread CancellationToken through the
call chain by adding a CancellationToken parameter to GetVideoAsset and GetAsset
(and any callers, e.g., controller actions) and passing that token into
PlayVideoWithRange and into SendAsync/ReadAsStreamAsync calls so upstream HTTP
requests can be cancelled (use HttpContext.RequestAborted from controllers when
invoking GetVideoAsset/GetAsset).
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 139-148: The tuple returned by _logic.GetAsset (captured in local
variable asset) includes a disposable member named dispose that is not being
disposed in GetRandomImageAndInfo; update the method to ensure the returned
dispose is cleaned up (e.g., wrap the disposable in a using or call
dispose?.Dispose() after consuming asset.fileStream) so any future non-null
disposables aren't leaked while keeping the existing fileStream handling and
webhook send logic unchanged.
- Around line 91-108: The partial-content branch currently disposes
asset.fileStream via "await using (asset.fileStream)" and then calls
asset.dispose?.Dispose(), risking double-disposal and missing dispose if
CopyToAsync throws; remove the "await using (asset.fileStream)" and instead
ensure the owning disposable is always cleaned up by registering or
finally-disposing the owner (use Response.RegisterForDispose(asset.dispose) or
wrap Response.Body.CopyToAsync in try/finally and call asset.dispose?.Dispose()
in finally), keep setting Response.Headers/StatusCode/ContentType/ContentLength
and return EmptyResult as before so the stream itself is not separately disposed
inside this block.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.WebApi/Controllers/AssetController.cs (1)
139-146:⚠️ Potential issue | 🟡 MinorMissing disposal of
asset.FileStreaminRandomImageAndInfo.Unlike
GetAsset, this method doesn't register theAssetResponsefor disposal. After copying tomemoryStream, theFileStreamremains open. While theusingblock handlesmemoryStream,asset.FileStreamis leaked.🐛 Proposed fix
var asset = await _logic.GetAsset(new Guid(randomAsset.Id), AssetTypeEnum.IMAGE); var notification = new AssetRequestedNotification(new Guid(randomAsset.Id), sanitizedClientIdentifier); _ = _logic.SendWebhookNotification(notification); string randomImageBase64; - using (var memoryStream = new MemoryStream()) + await using (asset.FileStream) + using (var memoryStream = new MemoryStream()) { await asset.FileStream.CopyToAsync(memoryStream); randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 139 - 146, The AssetResponse's FileStream is not being disposed in RandomImageAndInfo: after copying asset.FileStream into memoryStream the file handle is left open. Fix by ensuring the AssetResponse (or at least asset.FileStream) is disposed — e.g., acquire the asset with a using/using var (wrap the call to _logic.GetAsset(...) in a using block) or explicitly dispose asset.FileStream after the CopyToAsync completes; keep the existing SendWebhookNotification call and ensure disposal happens only after the copy finishes.
🧹 Nitpick comments (5)
ImmichFrame.Core/Models/AssetResponse.cs (2)
10-10: Consider renamingDisposeproperty to avoid confusion.The property name
Disposeconflicts with the conventionalIDisposable.Dispose()method name. This leads to confusing code likeasset.Dispose?.Dispose()at the call site (seeAssetController.csline 106).♻️ Suggested rename
- public IDisposable? Dispose { get; init; } + public IDisposable? DisposableResource { get; init; }Alternative names:
ResponseHandle,OwningResource, orHttpResponse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Models/AssetResponse.cs` at line 10, The property AssetResponse.Dispose conflicts with IDisposable.Dispose; rename the property to a clearer identifier (e.g., ResponseHandle or OwningResource) in the AssetResponse class and update all call sites (for example where code does asset.Dispose?.Dispose() in AssetController) to use the new name (e.g., asset.ResponseHandle?.Dispose()). Keep the type as IDisposable? and preserve any annotations/initializers on the original Dispose property.
3-12: Consider implementingIDisposableon the class itself.
AssetResponseholds disposable resources (FileStreamandDispose) but doesn't implementIDisposable. This places burden on callers to remember to dispose both resources correctly. ImplementingIDisposablewould provide a single disposal entry point.♻️ Proposed implementation
-public class AssetResponse +public class AssetResponse : IDisposable { public required string FileName { get; init; } public required string ContentType { get; init; } public required Stream FileStream { get; init; } public string? ContentRange { get; init; } public bool IsPartial { get; init; } - public IDisposable? Dispose { get; init; } + public IDisposable? DisposableResource { get; init; } public string? ContentLength { get; init; } + + public void Dispose() + { + FileStream?.Dispose(); + DisposableResource?.Dispose(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Models/AssetResponse.cs` around lines 3 - 12, AssetResponse currently owns disposable members (FileStream and the IDisposable? Dispose property) but doesn't implement IDisposable; add IDisposable to the AssetResponse class and implement the dispose pattern: provide a public void Dispose() that calls a private Dispose(bool disposing) which disposes FileStream and the IDisposable stored in the Dispose property (null-checking both), and suppress finalization if you add a finalizer; update class declaration to implement IDisposable and ensure repeated Dispose calls are safe (idempotent) by tracking a disposed flag.ImmichFrame.WebApi/Controllers/AssetController.cs (2)
85-86: Fire-and-forget webhook call silently swallows exceptions.Using
_ = _logic.SendWebhookNotification(notification)discards the task, meaning any exceptions during webhook delivery are silently lost. Consider at minimum logging failures.♻️ Suggested fix to handle failures
- _ = _logic.SendWebhookNotification(notification); + _ = _logic.SendWebhookNotification(notification).ContinueWith( + t => _logger.LogWarning(t.Exception, "Webhook notification failed"), + TaskContinuationOptions.OnlyOnFaulted);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 85 - 86, The fire-and-forget call to _logic.SendWebhookNotification(notification) in AssetController (using AssetRequestedNotification) silently swallows exceptions; change it to explicitly handle failures by either awaiting the Task or scheduling it with a continuation that catches and logs exceptions (e.g., Task.Run(() => _logic.SendWebhookNotification(notification)).ContinueWith(t => logger.LogError(t.Exception, "..."), TaskContinuationOptions.OnlyOnFaulted)), or wrap the call in a try/catch inside a background Task so exceptions are sent to the existing ILogger/processing logger instead of being lost.
80-113: Potential conflict between manual range handling andenableRangeProcessing: true.The logic layer already handles range requests and returns partial content, but line 113 passes
enableRangeProcessing: truetoFile(). For non-partial responses (images), this could cause ASP.NET Core to process range headers again, potentially conflicting with the intended behavior.For images (which don't use range handling in the logic layer), this is fine. However, if a video request arrives without a
Rangeheader but one is added by a client mid-stream, the flow might behave unexpectedly.Consider clarifying the intent or adding a comment explaining that
enableRangeProcessingis intended only for images since videos are handled explicitly.- return File(asset.FileStream, asset.ContentType, asset.FileName, enableRangeProcessing: true); + // enableRangeProcessing allows ASP.NET Core to handle range requests for images; + // videos are handled explicitly via PlayVideoWithRange in the logic layer. + return File(asset.FileStream, asset.ContentType, asset.FileName, enableRangeProcessing: true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 80 - 113, The controller currently passes enableRangeProcessing: true to File(asset.FileStream, asset.ContentType, asset.FileName, enableRangeProcessing: true) even though range logic is handled by _logic.GetAsset and partial responses are returned when asset.IsPartial is true; to avoid double/late range processing by ASP.NET Core, change the File(...) call to disable framework range processing (remove or set enableRangeProcessing: false) and add a brief comment near the File call explaining that range handling is performed by the logic layer (see GetAsset, asset.IsPartial, and the partial response branch) so the framework should not re-process ranges.ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
224-224: Hardcoded.mp4extension may not match actual video format.The filename is always set to
{id}.mp4regardless of the actual video format (e.g.,.movfor QuickTime). Consider deriving the extension from theContent-Typeheader.♻️ Suggested improvement
+ var extension = contentType switch + { + "video/quicktime" => "mov", + "video/webm" => "webm", + _ => "mp4" + }; + return new AssetResponse { - FileName = $"{id}.mp4", + FileName = $"{id}.{extension}", ContentType = contentType,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` at line 224, The FileName is hardcoded to $"{id}.mp4" which can be wrong for non-MP4 videos; update the code in PooledImmichFrameLogic where FileName is set (the $"{id}.mp4" assignment) to derive the extension from the response's Content-Type header (or the original file name if available): parse the Content-Type (e.g., "video/quicktime" -> ".mov"), map it to a proper extension (use a mime->ext lookup or fallback logic), ensure the extension includes a leading dot and sanitize it, and then set FileName = $"{id}{extension}" with a safe default (e.g., ".mp4") if mapping fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 203-210: The null check and throw in GetVideoAsset are dead code
because _immichApi.PlayAssetVideoAsync and PlayVideoWithRange throw on failure;
remove the unreachable if (videoResponse == null) throw new
AssetNotFoundException(...) block and simply return videoResponse from
GetVideoAsset after calling either _immichApi.PlayAssetVideoAsync or
PlayVideoWithRange to clean up the method flow.
---
Outside diff comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 139-146: The AssetResponse's FileStream is not being disposed in
RandomImageAndInfo: after copying asset.FileStream into memoryStream the file
handle is left open. Fix by ensuring the AssetResponse (or at least
asset.FileStream) is disposed — e.g., acquire the asset with a using/using var
(wrap the call to _logic.GetAsset(...) in a using block) or explicitly dispose
asset.FileStream after the CopyToAsync completes; keep the existing
SendWebhookNotification call and ensure disposal happens only after the copy
finishes.
---
Nitpick comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Line 224: The FileName is hardcoded to $"{id}.mp4" which can be wrong for
non-MP4 videos; update the code in PooledImmichFrameLogic where FileName is set
(the $"{id}.mp4" assignment) to derive the extension from the response's
Content-Type header (or the original file name if available): parse the
Content-Type (e.g., "video/quicktime" -> ".mov"), map it to a proper extension
(use a mime->ext lookup or fallback logic), ensure the extension includes a
leading dot and sanitize it, and then set FileName = $"{id}{extension}" with a
safe default (e.g., ".mp4") if mapping fails.
In `@ImmichFrame.Core/Models/AssetResponse.cs`:
- Line 10: The property AssetResponse.Dispose conflicts with
IDisposable.Dispose; rename the property to a clearer identifier (e.g.,
ResponseHandle or OwningResource) in the AssetResponse class and update all call
sites (for example where code does asset.Dispose?.Dispose() in AssetController)
to use the new name (e.g., asset.ResponseHandle?.Dispose()). Keep the type as
IDisposable? and preserve any annotations/initializers on the original Dispose
property.
- Around line 3-12: AssetResponse currently owns disposable members (FileStream
and the IDisposable? Dispose property) but doesn't implement IDisposable; add
IDisposable to the AssetResponse class and implement the dispose pattern:
provide a public void Dispose() that calls a private Dispose(bool disposing)
which disposes FileStream and the IDisposable stored in the Dispose property
(null-checking both), and suppress finalization if you add a finalizer; update
class declaration to implement IDisposable and ensure repeated Dispose calls are
safe (idempotent) by tracking a disposed flag.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 85-86: The fire-and-forget call to
_logic.SendWebhookNotification(notification) in AssetController (using
AssetRequestedNotification) silently swallows exceptions; change it to
explicitly handle failures by either awaiting the Task or scheduling it with a
continuation that catches and logs exceptions (e.g., Task.Run(() =>
_logic.SendWebhookNotification(notification)).ContinueWith(t =>
logger.LogError(t.Exception, "..."), TaskContinuationOptions.OnlyOnFaulted)), or
wrap the call in a try/catch inside a background Task so exceptions are sent to
the existing ILogger/processing logger instead of being lost.
- Around line 80-113: The controller currently passes enableRangeProcessing:
true to File(asset.FileStream, asset.ContentType, asset.FileName,
enableRangeProcessing: true) even though range logic is handled by
_logic.GetAsset and partial responses are returned when asset.IsPartial is true;
to avoid double/late range processing by ASP.NET Core, change the File(...) call
to disable framework range processing (remove or set enableRangeProcessing:
false) and add a brief comment near the File call explaining that range handling
is performed by the logic layer (see GetAsset, asset.IsPartial, and the partial
response branch) so the framework should not re-process ranges.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ImmichFrame.Core/Interfaces/IImmichFrameLogic.csImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core/Models/AssetResponse.csImmichFrame.WebApi/Controllers/AssetController.cs
JW-CH
reviewed
Mar 2, 2026
| private async Task<FileResponse> PlayVideoWithRange(Guid id, string rangeHeader, CancellationToken cancellationToken = default) | ||
| { | ||
| var videoResponse = await _immichApi.PlayAssetVideoAsync(id, string.Empty); | ||
| var url = $"{_immichApi.BaseUrl.TrimEnd('/')}/assets/{id}/video/playback"; |
Collaborator
There was a problem hiding this comment.
I don't like this 'hard coded' URL this much. Could you either find a way to use a class generated in immichapi or at least add a method to the partial implementation if immichapi?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 96-106: The AssetResponse returned in PooledImmichFrameLogic
currently sets Dispose = null which prevents disposing HTTP-backed streams from
GetImageAsset (which may wrap _immichApi.ViewAssetAsync) and can leak resources;
update GetImageAsset (and its callsite in PooledImmichFrameLogic) to capture the
disposable owner (e.g., the HttpResponseMessage or stream wrapper) and set
AssetResponse.Dispose to a delegate that disposes that owner (for example assign
Dispose = () => owner.Dispose()), ensuring the underlying HTTP response/stream
is released when consumers call Dispose on the AssetResponse.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ImmichFrame.Core/Api/ImmichApi.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
Comment on lines
+96
to
+106
| var (fileName, contentType, fileStream) = await GetImageAsset(id); | ||
| return new AssetResponse | ||
| { | ||
| FileName = fileName, | ||
| ContentType = contentType, | ||
| FileStream = fileStream, | ||
| ContentRange = null, | ||
| IsPartial = false, | ||
| Dispose = null, | ||
| ContentLength = null | ||
| }; |
There was a problem hiding this comment.
Preserve a disposable owner for image API streams.
Dispose = null here prevents callers from disposing the underlying API response when GetImageAsset returns a stream backed by _immichApi.ViewAssetAsync. This can leak HTTP resources over time.
♻️ Proposed refactor
- if (assetType == AssetTypeEnum.IMAGE)
- {
- var (fileName, contentType, fileStream) = await GetImageAsset(id);
- return new AssetResponse
- {
- FileName = fileName,
- ContentType = contentType,
- FileStream = fileStream,
- ContentRange = null,
- IsPartial = false,
- Dispose = null,
- ContentLength = null
- };
- }
+ if (assetType == AssetTypeEnum.IMAGE)
+ {
+ return await GetImageAsset(id);
+ }
- private async Task<(string fileName, string ContentType, Stream fileStream)> GetImageAsset(Guid id)
+ private async Task<AssetResponse> GetImageAsset(Guid id)
{
if (_generalSettings.DownloadImages)
{
...
- return (Path.GetFileName(file), $"image/{ex}", fs);
+ return new AssetResponse
+ {
+ FileName = Path.GetFileName(file),
+ ContentType = $"image/{ex}",
+ FileStream = fs,
+ ContentRange = null,
+ IsPartial = false,
+ Dispose = null,
+ ContentLength = null
+ };
...
}
var data = await _immichApi.ViewAssetAsync(id, string.Empty, AssetMediaSize.Preview);
...
if (_generalSettings.DownloadImages)
{
- var stream = data.Stream;
+ using (data)
+ {
+ var stream = data.Stream;
+ ...
+ return new AssetResponse
+ {
+ FileName = Path.GetFileName(filePath),
+ ContentType = contentType,
+ FileStream = fs,
+ ContentRange = null,
+ IsPartial = false,
+ Dispose = null,
+ ContentLength = null
+ };
+ }
- ...
- return (Path.GetFileName(filePath), contentType, fs);
}
- return (fileName, contentType, data.Stream);
+ return new AssetResponse
+ {
+ FileName = fileName,
+ ContentType = contentType,
+ FileStream = data.Stream,
+ ContentRange = null,
+ IsPartial = false,
+ Dispose = data,
+ ContentLength = null
+ };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 96 - 106, The
AssetResponse returned in PooledImmichFrameLogic currently sets Dispose = null
which prevents disposing HTTP-backed streams from GetImageAsset (which may wrap
_immichApi.ViewAssetAsync) and can leak resources; update GetImageAsset (and its
callsite in PooledImmichFrameLogic) to capture the disposable owner (e.g., the
HttpResponseMessage or stream wrapper) and set AssetResponse.Dispose to a
delegate that disposes that owner (for example assign Dispose = () =>
owner.Dispose()), ensuring the underlying HTTP response/stream is released when
consumers call Dispose on the AssetResponse.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.WebApi/Controllers/AssetController.cs (1)
149-158:⚠️ Potential issue | 🟡 MinorFileStream not disposed after use.
The
asset.FileStreamis copied to memory but never disposed. For image assets, this could leak file handles (when serving from cache) or HTTP connections (when Owner is fixed to include the API response).Proposed fix
var asset = await _logic.GetAsset(new Guid(randomAsset.Id), AssetTypeEnum.IMAGE); var notification = new AssetRequestedNotification(new Guid(randomAsset.Id), sanitizedClientIdentifier); _ = _logic.SendWebhookNotification(notification); string randomImageBase64; - using (var memoryStream = new MemoryStream()) + using (asset.Owner) + using (asset.FileStream) + using (var memoryStream = new MemoryStream()) { await asset.FileStream.CopyToAsync(memoryStream); randomImageBase64 = Convert.ToBase64String(memoryStream.ToArray()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.WebApi/Controllers/AssetController.cs` around lines 149 - 158, The asset.FileStream returned by _logic.GetAsset(...) is never disposed, which can leak file handles; update the AssetController logic to ensure asset.FileStream is disposed after copying to memory (e.g., wrap asset.FileStream in a using or otherwise call Dispose/DisposeAsync after CopyToAsync completes) while still creating the AssetRequestedNotification and invoking _logic.SendWebhookNotification(notification); reference the asset variable and its FileStream property so the stream is always closed even if exceptions occur during Convert.ToBase64String.
♻️ Duplicate comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
96-106:⚠️ Potential issue | 🟠 MajorResource leak risk when streaming images from API.
When
GetImageAssetreturnsdata.Streamfrom_immichApi.ViewAssetAsync()(line 170), the underlyingFileResponseobject is not captured inOwner. This prevents proper disposal of the HTTP response resources.For locally cached images,
Owner = nullis fine since theFileStreammanages itself. But for API-streamed images, theFileResponseshould be preserved.Proposed approach
Refactor
GetImageAssetto returnAssetResponsedirectly, preserving theFileResponseasOwnerwhen streaming from the API:- private async Task<(string fileName, string ContentType, Stream fileStream)> GetImageAsset(Guid id) + private async Task<AssetResponse> GetImageAsset(Guid id)Then for the API path (around line 170):
- return (fileName, contentType, data.Stream); + return new AssetResponse + { + FileName = fileName, + ContentType = contentType, + FileStream = data.Stream, + Owner = data, // Preserve for disposal + ContentRange = null, + IsPartial = false, + ContentLength = null + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 96 - 106, GetImageAsset currently returns (fileName, contentType, fileStream) and the caller creates an AssetResponse with Owner = null, which leaks the underlying HTTP FileResponse from _immichApi.ViewAssetAsync; change GetImageAsset to return an AssetResponse (or add an owner field to its return tuple) so that when the API path uses _immichApi.ViewAssetAsync you assign the returned FileResponse to AssetResponse.Owner (preserving FileStream in FileStream) and leave Owner null for local-cache paths, ensuring the caller can dispose Owner when finished.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Around line 149-158: The asset.FileStream returned by _logic.GetAsset(...) is
never disposed, which can leak file handles; update the AssetController logic to
ensure asset.FileStream is disposed after copying to memory (e.g., wrap
asset.FileStream in a using or otherwise call Dispose/DisposeAsync after
CopyToAsync completes) while still creating the AssetRequestedNotification and
invoking _logic.SendWebhookNotification(notification); reference the asset
variable and its FileStream property so the stream is always closed even if
exceptions occur during Convert.ToBase64String.
---
Duplicate comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 96-106: GetImageAsset currently returns (fileName, contentType,
fileStream) and the caller creates an AssetResponse with Owner = null, which
leaks the underlying HTTP FileResponse from _immichApi.ViewAssetAsync; change
GetImageAsset to return an AssetResponse (or add an owner field to its return
tuple) so that when the API path uses _immichApi.ViewAssetAsync you assign the
returned FileResponse to AssetResponse.Owner (preserving FileStream in
FileStream) and leave Owner null for local-cache paths, ensuring the caller can
dispose Owner when finished.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core/Models/AssetResponse.csImmichFrame.WebApi/Controllers/AssetController.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ImmichFrame.Core/Api/ImmichApi.cs`:
- Around line 34-42: The code currently throws an ApiException for any
non-200/206 status, but for range playback a 416 (Range Not Satisfiable) should
be passed through to callers; update the response handling in ImmichApi.cs (the
method that returns FileResponse) to treat status == 416 the same way as 200/206
by creating and returning a FileResponse (using the same stream acquisition
logic and headers) instead of disposing the response and throwing ApiException,
leaving other non-success statuses unchanged.
Comment on lines
+34
to
+42
| if (status == 200 || status == 206) | ||
| { | ||
| var stream = response.Content == null ? Stream.Null : await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false); | ||
| return new FileResponse(status, headers, stream, null, response); | ||
| } | ||
|
|
||
| var error = response.Content == null ? null : await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | ||
| response.Dispose(); | ||
| throw new ApiException($"Unexpected status code ({status}).", status, error, headers, null); |
There was a problem hiding this comment.
Pass through HTTP 416 instead of throwing from range playback.
For range requests, 416 Range Not Satisfiable is a valid protocol outcome. Throwing here can convert that into an upstream error path instead of letting callers return proper 416 handling.
Suggested fix
- if (status == 200 || status == 206)
+ if (status == 200 || status == 206 || status == 416)
{
- var stream = response.Content == null ? Stream.Null : await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false);
+ var stream = (status == 200 || status == 206) && response.Content != null
+ ? await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false)
+ : Stream.Null;
return new FileResponse(status, headers, stream, null, response);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ImmichFrame.Core/Api/ImmichApi.cs` around lines 34 - 42, The code currently
throws an ApiException for any non-200/206 status, but for range playback a 416
(Range Not Satisfiable) should be passed through to callers; update the response
handling in ImmichApi.cs (the method that returns FileResponse) to treat status
== 416 the same way as 200/206 by creating and returning a FileResponse (using
the same stream acquisition logic and headers) instead of disposing the response
and throwing ApiException, leaving other non-success statuses unchanged.
This reverts commit b75b9fb.
JW-CH
changed the title
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
2 participants
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.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes