Skip to content

feat(crashreporting): Add optional local Crashpad backend - #3413

Open
CryoTheRenegade wants to merge 4 commits into
TheSuperHackers:mainfrom
CryoTheRenegade:feat/crashpad
Open

CryoTheRenegade wants to merge 4 commits into
TheSuperHackers:mainfrom
CryoTheRenegade:feat/crashpad

Conversation

@CryoTheRenegade

Copy link
Copy Markdown

Adds optional, local-only Crashpad reporting. It is disabled by default, and MiniDumper remains the default backend and startup fallback (can be changed in future).

The aim is to reduce reliance on the crashing game process by capturing dumps in a separate handler. Developers still inspect reports with matching symbols. This change does not yet establish better capture reliability than MiniDumper.

The implementation includes:

  • A pinned Crashpad client and handler, isolated from the game’s allocator through a DLL.
  • Unhandled-exception capture and capture from both explicit fatal-error paths.
  • Startup readiness checks, legacy fallback, and a watchdog for stalled explicit capture.
  • Local report storage, retention, and manual export. Automatic uploads are disabled.
  • Handler deployment, dependency notices, build metadata, and executable/PDB identity verification.

The existing replay from #3185 produced a real gameplay crash that produced a crash dump

Build instructions are in docs/crashpad.md

AI disclosure: AI (GPT-Astra) assisted the implementation, documentation, and testing of this change.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: aa1fe9ad-2969-40c8-842b-128b615c13fa
📥 Commits

Reviewing files that changed from the base of the PR and between 5efe454 and ec660df.

📒 Files selected for processing (1)
  • Core/GameEngine/Source/Common/System/Debug.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

Adds optional Crashpad reporting for supported Win32 game builds. Both games use a shared crash-reporting interface with MiniDumper fallback. The change also adds local report tools, build and symbol artifact handling, CI collection, documentation, and validation.

Changes

Crashpad reporting

Layer / File(s) Summary
Configure and stage Crashpad
CMakeLists.txt, Dependencies/Crashpad/CMakeLists.txt, Dependencies/Crashpad/vcpkg.json, Dependencies/Crashpad/*-LICENSE, cmake/crashpad.cmake, Core/GameEngine/CMakeLists.txt, Generals/Code/Main/CMakeLists.txt, GeneralsMD/Code/Main/CMakeLists.txt, Generals/CMakeLists.txt, GeneralsMD/CMakeLists.txt
Adds pinned Crashpad dependency configuration and build options. Adds staging and installation rules for Crashpad runtime files, symbols, notices, and tools for both games.
Implement the Crashpad bridge
Dependencies/Crashpad/CrashpadBridge.*
Adds DLL entry points for initialization, fatal capture, and shutdown. Initialization creates a local database with uploads disabled and starts the handler. Fatal capture uses a watchdog with a 15-second timeout.
Connect game lifecycle to crash reporting
Core/GameEngine/Include/Common/CrashReporting.h, Core/GameEngine/Source/Common/System/CrashReporting.cpp, Core/GameEngine/Source/Common/System/Debug.cpp, Core/GameEngine/Source/Common/System/MiniDumper.cpp, Generals/Code/Main/WinMain.cpp, GeneralsMD/Code/Main/WinMain.cpp, Generals/Code/GameEngine/Source/Common/GameEngine.cpp
Adds the engine reporting interface and connects both games to initialization, user-directory readiness, fatal capture, and shutdown. Crash capture uses Crashpad when available and MiniDumper otherwise.
Manage reports and build artifacts
Dependencies/Crashpad/CrashpadReports.cpp, Dependencies/Crashpad/archive_symbols.py, cmake/crashpad-metadata.cmake, .github/workflows/reusable-build-toolchain.yml, docs/crashpad.md
Adds local report listing, pruning, and export, plus build metadata and symbol verification for archives. CI collects Crashpad metadata and notices, and artifact retention increases to 90 days. The documentation describes configuration, operation, deployment, and validation.
Validate Crashpad behavior
Dependencies/Crashpad/tests/*
Adds fault-injection and database-seeding executables with a validation script covering capture modes, handler failures, concurrent dumps, export, and retention.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant WinMain
  participant CrashReporting
  participant CrashpadBridge as rts_crashpad.dll
  participant CrashpadHandler as Crashpad handler
  participant CrashpadDatabase as Crashpad database
  WinMain->>CrashReporting: initialize with user directory and version
  CrashReporting->>CrashpadBridge: load DLL and resolve exports
  CrashpadBridge->>CrashpadDatabase: create database with uploads disabled
  CrashpadBridge->>CrashpadHandler: start handler and wait for IPC ping
  WinMain->>CrashReporting: notify when user directory is ready
  WinMain->>CrashReporting: captureFatal on fatal error
  CrashReporting->>CrashpadBridge: request fatal dump
  CrashpadBridge->>CrashpadHandler: capture dump
Loading

Suggested reviewers: bobtista

Merge Risk: ⚪ Minimal · up to ec660

No actionable crash-handling regression is established in the reviewed change; the localized null path is not reached by its current in-repository caller.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ec660

The optional backend has meaningful local trust and lifecycle boundaries, but remains disabled by default and explicitly disables uploads. No introduced exploitable security path was established. Installation permissions, handler privileges, and concurrent cleanup behavior remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated new exposure is local to enabled game processes, their sibling runtime binaries, and user-directory crash reports. The traced fatal APIs provide no remote destination or identity selector. Handler privileges, report-directory ACLs, and any elevated execution were not established, so the maximum permission-based exposure remains unresolved.

Trust Boundaries and Controls

  • observed — The runtime anchors the DLL and handler paths to the game executable rather than fatal-error input. DLL loading resolves expected exports, and handler startup checks existence and IPC readiness. These controls establish location and protocol readiness, not binary authenticity; deployment ownership and replacement permissions remain unverified.

Resilience and Maintainability Implications

  • inferred — Sequential startup failure, capture, and cleanup have explicit containment mechanisms. Concurrent lifecycle safety is not established: readiness and shutdown mutate callbacks and backend ownership, while capture uses those callbacks and shared client/event state. The one-shot atomic capture gate does not serialize capture against shutdown. No reachable attacker-controlled overlap was demonstrated.

Hardening Proposals

  • proposed — Define the trusted installation and execution contract for enabled releases: who may replace the sibling binaries, which authority the handler receives, and who may read crash reports. Separately document lifecycle concurrency guarantees or introduce crash-safe exclusion where overlapping capture and cleanup are supported.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the optional local Crashpad backend, which is the main change in the pull request.
Description check ✅ Passed The description explains the Crashpad feature, its fallback behavior, capture and storage goals, deployment, and validation. It is directly related to the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[High risk] Adds optional crash reporting backend with new build dependencies.

The crash-reporting behavior appears safe to merge, but the repository’s copyright-attribution requirement must be satisfied first.

Summary

The PR adds an optional, local-only Crashpad backend with MiniDumper fallback, report management, deployment metadata, and validation tooling. Since the previous review, it restores fatal dump capture when global game data has already been cleared. The remaining new finding concerns copyright attribution in new community source files.

Reviews (3) · Last reviewed commit: "bugfix(crashreporting): Preserve fatal d..."

Comment thread Core/GameEngine/Source/Common/System/Debug.cpp
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