Skip to content

Fix Linux social card rendering#335

Merged
PrzemyslawKlys merged 1 commit intomainfrom
fix/linux-social-card-renderer
May 1, 2026
Merged

Fix Linux social card rendering#335
PrzemyslawKlys merged 1 commit intomainfrom
fix/linux-social-card-renderer

Conversation

@PrzemyslawKlys
Copy link
Copy Markdown
Member

Summary

  • render social card SVGs through the external ImageMagick CLI on non-Windows hosts when available
  • avoid in-process Magick.NET SVG rendering on Linux to prevent native fontconfig crashes
  • initialize fontconfig before remaining Magick.NET font/image operations

Verification

  • dotnet test PowerForge.Tests\PowerForge.Tests.csproj --filter "WebSocialCardGeneratorTests|WebSiteSocialCardsTests" --no-restore
  • Linux temp build: dotnet run ... build --config ./site.json --out /tmp/pf-social-test --clean --language en -> STATUS:0, COUNT:180 generated PNGs

@claude
Copy link
Copy Markdown

claude Bot commented May 1, 2026

Code Review: Fix Linux social card rendering

Overview

This PR solves a real problem — in-process Magick.NET SVG rendering causes native fontconfig crashes on Linux. The approach (shell out to the system magick/convert binary, fall back gracefully) is sound. The fontconfig pre-initialization via P/Invoke (FcInit) for all other Magick.NET operations is also the right call. Overall the implementation is solid, but a few issues are worth addressing before merge.


Issues

1. Unobserved task exception / stream disposed before read completes (bug)

In TryRenderPngWithExternalImageMagick, errorTask is started before WaitForExit:

var errorTask = process.StandardError.ReadToEndAsync();
var exited = process.WaitForExit(30_000);

On the success path, the method returns File.ReadAllBytes(pngPath) without ever awaiting errorTask. The using var process then disposes the Process, which disposes the underlying StreamReader for stderr. ReadToEndAsync may still be running when this disposal happens, producing an unobserved ObjectDisposedException in the background task.

The fix is to consume or synchronously complete errorTask before returning on the success branch:

// After WaitForExit returns true and ExitCode == 0:
errorTask.GetAwaiter().GetResult(); // drain stderr; process has already exited so this is instantaneous
return File.ReadAllBytes(pngPath);

Alternatively, call the parameterless process.WaitForExit() (which flushes async redirects) right after the timeout-based call returns true.

2. PATH scanning on every render — cache the resolved command

ResolveExternalImageMagickCommand() iterates the entire PATH on each call to TryRenderPngWithExternalImageMagick, which is invoked for every card render. A simple Lazy<ExternalImageMagickCommand?> (mirroring the pattern already used for MagickFontConfigInitialized) would eliminate the repeated filesystem probing:

private static readonly Lazy<ExternalImageMagickCommand?> CachedExternalImageMagickCommand =
    new(ResolveExternalImageMagickCommand);

3. convert may not be ImageMagick

On some systems (e.g., macOS with GraphicsMagick, or certain Debian configurations), convert refers to a non-ImageMagick binary. The current code uses the exact same argument set for both magick and convert. A non-ImageMagick convert will fail and be gracefully skipped (exit code ≠ 0), but it could log confusing warnings. Consider logging a note when falling back from magick to convert, or checking the binary output (magick -version) to confirm identity, though the latter adds latency and complexity.

This is low severity given the graceful fallback, but worth documenting in a comment.


Minor / Style

  • [DllImport] vs [LibraryImport]: The project targets a modern .NET version. [LibraryImport] (source-generated P/Invoke, introduced in .NET 7) is preferred over [DllImport] to eliminate the runtime IL emit and enable trimming/AOT compatibility.

    [LibraryImport("libfontconfig.so.1", EntryPoint = "FcInit")]
    private static partial int FcInit();
  • Double SVG render on non-Linux fallback: When external ImageMagick is unavailable and the host is not Linux, RenderPng renders the SVG twice — once embedded (for the external attempt) and again non-embedded (for in-process). This is by design and the comment is implicit in the code structure, but a short inline note would clarify intent for future readers.

  • ResolveExternalImageMagickCommand returns non-null on macOS/FreeBSD too: The method name and the warning message say "Linux", but the external path is also taken on macOS. The warning in RenderPng accurately scopes to Linux, which is correct, but the resolver and the TryRenderPngWithExternalImageMagick call happen on all non-Windows hosts. This is fine behaviour, just worth confirming it's intentional.


What's working well

  • The Lazy<bool> pattern for one-time fontconfig initialization is clean and thread-safe.
  • The GUID-based temp directory prevents collisions and TOCTOU races.
  • process.Kill(entireProcessTree: true) on timeout is the right call.
  • Best-effort cleanup in finally is appropriate.
  • Calling EnsureMagickFontConfigInitialized() at all Magick.NET entry points is thorough.
  • Graceful null-return (rather than throw) throughout means failures degrade to no card, not a crash.

Verdict

Fix item #1 (unobserved disposed stream) before merging. Items #2 and #3 are lower priority but worth a follow-up. The approach and overall structure are solid.

@PrzemyslawKlys PrzemyslawKlys merged commit df95edc into main May 1, 2026
5 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the fix/linux-social-card-renderer branch May 1, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant