Skip to content

Record the desktop app's fatal exception to app-crash.log - #1333

Merged
alexeyzimarev merged 2 commits into
mainfrom
capacitor/agent-8a230f41ba8b4c
Oct 6, 2026
Merged

alexeyzimarev merged 2 commits into
mainfrom
capacitor/agent-8a230f41ba8b4c

Conversation

@alexeyzimarev

@alexeyzimarev alexeyzimarev commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Closes #1332 — AI-3523

What & why

A UI-thread exception ends the desktop app with a SIGABRT whose report shows only the rethrow site, and the exception itself is recorded nowhere. The app now appends the fatal exception, with its original stack and the app version, to an owner-only app-crash.log under the config root. The process still terminates afterwards.

Where to look

The catch wraps StartWithClassicDesktopLifetime in Main rather than using Dispatcher.UIThread.UnhandledException: Avalonia.Native catches exceptions from native callbacks (input, render, dispatcher signal) itself and rethrows them from RunLoop, bypassing that event. The AppDomain handler sees the same rethrown object, so AppCrashLog writes it once.

Verification

dotnet run --project test/Capacitor.App.Tests.Unit -- --treenode-filter "/*/*/AppCrashLogTests/*" → 8/8 passed on macOS (the four Unix-mode/symlink cases skip on Windows). Not exercised in a running app.

🤖 Generated with Claude Code

Avalonia.Native rethrows a UI-callback exception from RunLoop without raising
Dispatcher.UnhandledException, so the catch has to wrap the run in Main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-06T15:42:56.784113Z e0f2ef5 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Record fatal desktop exceptions in app-crash.log

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Capture fatal desktop exceptions that Avalonia’s UI-thread handler misses, then preserve normal
 process termination.
• Append versioned exception details to a private, size-limited log under the config root.
• Test log contents, duplicate suppression, appending, size limits, and Unix permissions.
Diagram

sequenceDiagram
    actor UI as UI callback
    participant Native as Avalonia Native
    participant Main as Program Main
    participant Runtime as .NET runtime
    participant Log as AppCrashLog
    participant Root as ConfigRoot
    participant File as app-crash.log
    UI->>Native: Exception
    Native->>Main: Rethrow from run loop
    Main->>Log: Record UI-thread fault
    Log->>Root: Resolve log path
    Root-->>Log: Config path
    Log->>File: Append exception
    Main->>Runtime: Rethrow
    Runtime->>Log: Report unhandled fault
    Note over Log: Suppress same exception object
Loading
High-Level Assessment

The entry-point catch plus runtime unhandled-exception handler fits the two failure paths, and a shared logger prevents duplicate entries. A dispatcher-only handler was considered but cannot observe the Avalonia.Native rethrow described here.

Files changed (4) +150 / -2

Bug fix (2) +71 / -1
Program.csRecord fatal exceptions at desktop startup +16/-1

Record fatal exceptions at desktop startup

• Creates a crash logger after Velopack startup, registers a runtime unhandled-exception callback, and catches exceptions escaping the Avalonia desktop lifetime. The catch records the exception and rethrows it so termination behavior remains unchanged.

src/Capacitor.App/Program.cs

AppCrashLog.csAdd a private, bounded fatal-exception log +55/-0

Add a private, bounded fatal-exception log

• Appends timestamped, versioned exception details beneath ConfigRoot, with owner-only permissions for newly created Unix files. Suppresses repeated reports of the same exception object, replaces an oversized existing log, and does not let logging failures mask the crash.

src/Capacitor.App/Services/AppCrashLog.cs

Tests (1) +78 / -0
AppCrashLogTests.csTest fatal-exception log behavior +78/-0

Test fatal-exception log behavior

• Adds five tests covering exception details and stack traces, duplicate suppression, distinct-entry appending, oversized-log replacement, and Unix file permissions.

test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs

Documentation (1) +1 / -1
App.axaml.csClarify the second config-root environment read +1/-1

Clarify the second config-root environment read

• Updates the comment to acknowledge that Program also reads KCAP_CONFIG_DIR for crash logging. Application configuration behavior is unchanged.

src/Capacitor.App/App.axaml.cs

@qodo-code-review

qodo-code-review Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Crash-log test bypasses temp helpers ✓ Resolved
Description
AppCrashLogTests creates the temporary config directory and writes the oversized log with
Directory.CreateDirectory and File.WriteAllText instead of the available TempDir helpers. The
test therefore bypasses the helper-managed filesystem operations whenever it prepares the
capped-file scenario, leaving cleanup and temporary-state handling outside the standard test
abstraction.
Code

test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[R59-60]

+        Directory.CreateDirectory(Tmp.PathTo("config"));
+        File.WriteAllText(log.Path, new string('x', 300 * 1024));
Relevance

●●● Strong

The active test convention requires TempDir helpers, and similar helper-compliance findings were
accepted.

PR-#929

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed test uses Tmp.PathTo("config") but performs directory creation and file writing
directly. The compliance rule requires temporary filesystem state in tests to use TempDir helpers
such as CreateDir and CreateFile rather than direct Directory or File operations under the
temporary root.

Rule 2767472: Use TempDir helper for all temporary filesystem state in tests
test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[59-60]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The capped-log test directly calls `Directory.CreateDirectory` and `File.WriteAllText` for paths under the injected `TempDir`, bypassing the repository's required temporary filesystem helpers.

## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[59-60]

## Recommended Fix
Use `Tmp.CreateDir("config")` to create the configuration directory and use the appropriate `TempDir` file helper to create the oversized log content while preserving `log.Path` as the file path under test.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. The PR reference line lacks Linear context 📘 Rule violation ⚙ Maintainability
Description
The PR description's first reference line says Closes #1332 but explicitly provides no Linear
issue key. Because the required GitHub and Linear references are not on that same line, reviewers
cannot correlate the change with its Linear record.
Code

src/Capacitor.App/Program.cs[26]

+        var crashLog = new AppCrashLog(ConfigRoot.FromEnvironment(), TimeProvider.System);
Relevance

●●● Strong

The repository explicitly requires GitHub and Linear references on the PR description reference
line.

PR-#659

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR description begins with Closes #1332 — no Linear id yet, so it contains the GitHub closing
reference but no Linear issue key on the required reference line. The changed code confirms this PR
is the implementation associated with that reference.

Rule 2897911: PR description must contain both GitHub and Linear issue references; PR title must not contain issue IDs
src/Capacitor.App/Program.cs[26-26]

Dismiss ↗ | View ↗


3. Failed trimming loses the current crash ✓ Resolved
Description
TrimIfLarge deletes an oversized log before Record opens it, and a deletion error reaches the
outer catch without attempting an append. If deletion fails while the existing file remains
writable, the fatal exception is lost even though appending could have succeeded.
Code

src/Capacitor.App/Services/AppCrashLog.cs[R51-54]

+    void TrimIfLarge() {
+        var file = new FileInfo(Path);
+        if (file.Exists && file.Length > MaxBytes) file.Delete();
+    }
Relevance

●●● Strong

A trim failure clearly suppresses crash reporting; reviewers recently accepted analogous
failure-path reliability fixes.

PR-#446

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Trimming precedes the write in the same try block, so its uncaught deletion error skips the write.
The existing CLI crash reporter handles trim errors locally and proceeds to append.

src/Capacitor.App/Services/AppCrashLog.cs[33-44]
src/Capacitor.App/Services/AppCrashLog.cs[51-54]
src/Capacitor.Cli/CrashReporter.cs[68-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed attempt to delete an oversized log prevents the current fatal exception from being appended.
## Fix Focus Areas
- src/Capacitor.App/Services/AppCrashLog.cs[33-42]
- src/Capacitor.App/Services/AppCrashLog.cs[51-54]
## Recommended Fix
Make trimming independently best-effort: handle its errors locally, then attempt to append the current exception regardless. Add a test for a failed trim where the log is still writable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (2)
4. A planted link redirects crash details ✓ Resolved
Description
Record opens app-crash.log in append mode without refusing a symbolic link at that path. When
the configured directory is writable by another local user, that user can plant a link to a
writable, publicly readable file and receive the exception text there.
Code

src/Capacitor.App/Services/AppCrashLog.cs[R37-40]

+                var options = new FileStreamOptions { Mode = FileMode.Append, Access = FileAccess.Write, Share = FileShare.ReadWrite };
+                if (!OperatingSystem.IsWindows()) options.UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite;
+
+                using var stream = new FileStream(Path, options);
Relevance

●● Moderate

The symlink attack is plausible, but repository precedent does not establish acceptance of this
filesystem-hardening scope.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The logger appends to the joined path without validating the filesystem entry. ConfigRoot accepts a
configured directory, and the repository's owner-only file helper explicitly uses create-new
semantics to refuse existing paths, including links.

src/Capacitor.App/Services/AppCrashLog.cs[34-41]
src/Capacitor.Cli.Core/ConfigRoot.cs[27-32]
src/Capacitor.Cli.Core/ConfigRoot.cs[40-46]
src/Capacitor.Cli.Core/OwnerOnlyFile.cs[3-9]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Appending through a symbolic link can send private exception details to a different file when the config directory is shared.
## Fix Focus Areas
- src/Capacitor.App/Services/AppCrashLog.cs[34-41]
- test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[69-77]
## Recommended Fix
Use a file-opening strategy that refuses symbolic links for the crash-log path while retaining safe append behavior. Test that a linked path does not modify its target.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Existing crash logs remain readable by others ✓ Resolved
Description
Record supplies UnixCreateMode when opening the log but never tightens permissions on an
existing file. If app-crash.log already has group or world read access, subsequent fatal
exceptions are appended with those permissions unchanged.
Code

src/Capacitor.App/Services/AppCrashLog.cs[R37-40]

+                var options = new FileStreamOptions { Mode = FileMode.Append, Access = FileAccess.Write, Share = FileShare.ReadWrite };
+                if (!OperatingSystem.IsWindows()) options.UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite;
+
+                using var stream = new FileStream(Path, options);
Relevance

●● Moderate

This is a concrete security flaw, but no closely matching permissions precedent established team
acceptance.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The append open specifies a mode for creation but contains no operation that changes an existing
file's permissions; the permission test exercises only a newly created log.

src/Capacitor.App/Services/AppCrashLog.cs[37-41]
test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[69-77]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Appending to an existing crash log preserves permissive file permissions, exposing newly recorded exceptions.
## Fix Focus Areas
- src/Capacitor.App/Services/AppCrashLog.cs[37-41]
- test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs[69-77]
## Recommended Fix
Ensure an existing log is owner-readable and owner-writable before appending. Add a test that starts with a permissive log and verifies its mode after recording.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
Review mode: Auto: ⚖️ Balanced: Runtime crash handling and file permissions carry meaningful behavioral risk.

Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread test/Capacitor.App.Tests.Unit/AppCrashLogTests.cs Outdated
.Run();

BuildAvaloniaApp().StartWithClassicDesktopLifetime(args);
var crashLog = new AppCrashLog(ConfigRoot.FromEnvironment(), TimeProvider.System);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. The pr reference line lacks linear context 📘 Rule violation ⚙ Maintainability

The PR description's first reference line says Closes #1332 but explicitly provides no Linear
issue key. Because the required GitHub and Linear references are not on that same line, reviewers
cannot correlate the change with its Linear record.

Dismiss ↗ | View ↗

Comment thread src/Capacitor.App/Services/AppCrashLog.cs
Comment thread src/Capacitor.App/Services/AppCrashLog.cs
Comment thread src/Capacitor.App/Services/AppCrashLog.cs
…1332)

A failed trim no longer drops the entry: the log can refuse a delete and still
take an append.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit e72f06f into main Oct 6, 2026
8 checks passed
@alexeyzimarev
alexeyzimarev deleted the capacitor/agent-8a230f41ba8b4c branch October 6, 2026 16:54
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.

Desktop app dies silently on a UI-thread exception

1 participant