Feat/local image gen#141
Conversation
MaINDotnet-Bot
left a comment
There was a problem hiding this comment.
This PR introduces local in-process image generation via StableDiffusion.NET. However, it contains severe native resource leaks, multi-gigabyte model load concurrency hazards (leading to OOM), a lack of model cache eviction, and a Dockerfile configuration bug that will trigger a DllNotFoundException at runtime. Requesting changes to resolve these issues before merging.
| .WithCfg(capabilities.CfgScale) | ||
| .WithSteps(capabilities.Steps); | ||
|
|
||
| var image = diffusionModel.GenerateImage(parameters) |
There was a problem hiding this comment.
Native Resource Leak
StableDiffusionImage returned by GenerateImage wraps native memory allocations from stable-diffusion.cpp and implements IDisposable. Leaving it undisposed guarantees a native heap memory leak on every generated image.
Wrap it in a using statement.
using var image = diffusionModel.GenerateImage(parameters)
?? throw new InvalidOperationException("Image generation failed.");
return Task.FromResult<ChatResult?>(CreateChatResult(image.ToPng(), chat.ModelId));| { | ||
| return messages | ||
| .Select((msg, index) => index == 0 ? msg.Content : $"&& {msg.Content}") | ||
| .Aggregate((current, next) => $"{current} {next}"); |
There was a problem hiding this comment.
Unhandled Empty Sequence Crash
If messages is empty, Aggregate throws an InvalidOperationException ("Sequence contains no elements"). Ensure the collection is not empty or use a safer aggregation approach like string.Join with default fallbacks.
| public static DiffusionModel GetOrLoad(LocalModel model, ILocalDiffusionModel capabilities, string? basePath) | ||
| { | ||
| var key = model.GetFullPath(basePath); | ||
|
|
There was a problem hiding this comment.
Multi-Gigabyte Concurrency OOM Vulnerability
ConcurrentDictionary.GetOrAdd does not guarantee single-execution of its factory function under concurrent requests. If multiple requests arrive simultaneously for a model that isn't loaded yet, the multi-gigabyte GGUF model will be loaded multiple times concurrently on different threads, easily causing an out-of-memory (OOM) crash.
Wrap the values in Lazy<T> to guarantee exact-once evaluation:
private static readonly ConcurrentDictionary<string, Lazy<DiffusionModel>> ModelCache = new();
public static DiffusionModel GetOrLoad(LocalModel model, ILocalDiffusionModel capabilities, string? basePath)
{
var key = model.GetFullPath(basePath);
return ModelCache.GetOrAdd(key, k => new Lazy<DiffusionModel>(() =>
{
// ... factory logic ...
})).Value;
}| { | ||
| private const int MaxRecentLogLines = 50; | ||
|
|
||
| private static readonly ConcurrentDictionary<string, DiffusionModel> ModelCache = new(); |
There was a problem hiding this comment.
Memory Accumulation & Lack of Eviction
Giant local diffusion models (spanning 4GB to 10GB+) are cached in memory indefinitely. Since there is no eviction strategy or size limits on ModelCache, switching between models (StableDiffusion1_5, Flux1Shnell, and QwenImage) will rapidly exhaust both system RAM and VRAM.
Consider implementing a simple LRU cache or discarding/disposing of the previously loaded model when a new one is selected.
| private async Task DownloadModelAsync(LocalModel localModel, IProgress<DownloadProgress>? progress, CancellationToken cancellationToken) | ||
| { | ||
| if (localModel.DownloadUrl is null) throw new DownloadUrlNullOrEmptyException(); | ||
| foreach (var asset in localModel.RequiredAssets) |
There was a problem hiding this comment.
Missing Parent Directory Verification
FileStream is opened for writing without ensuring that the target directory hierarchy exists. If CustomPath has folder subdirectories that do not yet exist on disk, this will crash with a DirectoryNotFoundException.
Create the parent directory first:
var assetPath = localModel.GetAssetPath(asset, _defaultModelsPath);
var directory = Path.GetDirectoryName(assetPath);
if (!string.IsNullOrEmpty(directory))
{
Directory.CreateDirectory(directory);
}| COPY --from=publish /app/out . | ||
|
|
||
| RUN find /app/runtimes/linux-x64/native -maxdepth 2 -mindepth 1 -type d \ | ||
| | tee /etc/ld.so.conf.d/llamasharp.conf \ |
There was a problem hiding this comment.
Docker native search path constraint (DllNotFoundException)
StableDiffusion.NET places its native library (libstable-diffusion.so) directly in the /app/runtimes/linux-x64/native/ folder without any subdirectories. Because of -mindepth 1 -type d, the root native directory itself is excluded and never appended to llamasharp.conf. Consequently, at runtime inside Docker, this will fail with a DllNotFoundException since ldconfig never registers the native folder containing the shared library.
Append the root directory explicitly:
RUN echo "/app/runtimes/linux-x64/native" > /etc/ld.so.conf.d/llamasharp.conf \
&& find /app/runtimes/linux-x64/native -maxdepth 2 -mindepth 1 -type d >> /etc/ld.so.conf.d/llamasharp.conf \
&& ldconfig
No description provided.