Skip to content

wip(native): capture WER custom metadata#1760

Open
jpnurmi wants to merge 19 commits into
masterfrom
jpnurmi/feat/native/wer-metadata
Open

wip(native): capture WER custom metadata#1760
jpnurmi wants to merge 19 commits into
masterfrom
jpnurmi/feat/native/wer-metadata

Conversation

@jpnurmi
Copy link
Copy Markdown
Collaborator

@jpnurmi jpnurmi commented May 27, 2026

App:

WerRegisterCustomMetadata(L"foo", L"ooo");
WerRegisterCustomMetadata(L"bar", L"aaa");
WerRegisterCustomMetadata(L"baz", L"zzz");

Context:

{
  "contexts": {
    "wer": {
      "type": "wer",
      "report_id": "...",
      "metadata": {
        "foo": "ooo",
        "bar": "aaa",
        "baz": "zzz"
      }
    }
  }
}

Sentry:
image

Comment thread src/backends/native/sentry_crash_daemon.c
Comment thread src/backends/native/sentry_wer_report.c
@jpnurmi jpnurmi force-pushed the jpnurmi/feat/native/wer-metadata branch from c9a7b1a to c218239 Compare May 27, 2026 07:27
@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 27, 2026

TGDX-58

@jpnurmi jpnurmi changed the title feat(native): capture WER custom metadata wip(native): capture WER custom metadata May 27, 2026
jpnurmi added 6 commits May 27, 2026 16:31
Pre-generate the native crash event ID and register it as WER custom
metadata so the crash daemon can identify the matching
WERInternalMetadata.xml.

Store the WER report ID provided by sentry-wer.dll in the shared crash
context and add a wer context with the report ID and app-provided WER
custom metadata.
@jpnurmi jpnurmi force-pushed the jpnurmi/feat/native/wer-metadata branch from c218239 to b503ec0 Compare May 27, 2026 14:36
Comment thread src/backends/native/sentry_crash_daemon.c
Comment thread src/backends/native/sentry_wer.c
The crash handler's WER path returned EXCEPTION_CONTINUE_SEARCH without
claiming the crash state or signaling the daemon, relying entirely on the
sentry-wer DLL callback to do so via WER. On CI runners WER may not
always invoke the registered callback, leaving the daemon waiting
indefinitely. Fix by having the crash handler always claim the crash
state and notify the daemon before returning, and having the sentry-wer
DLL write the WER report ID and signal even when the crash handler
already claimed the crash.
@jpnurmi jpnurmi closed this May 28, 2026
@jpnurmi jpnurmi deleted the jpnurmi/feat/native/wer-metadata branch May 28, 2026 06:56
jpnurmi added 4 commits May 29, 2026 08:22
When the crash handler claims the crash in the WER path and notifies
the daemon, it must set exception_pointers to NULL so the daemon uses
the already-copied exception record/context from shared memory with
ClientPointers=FALSE. Otherwise MiniDumpWriteDump tries to read from
the crashed process's address space (ClientPointers=TRUE), but the
process is already terminated because the crash handler returned
EXCEPTION_CONTINUE_SEARCH without waiting.
@jpnurmi jpnurmi reopened this May 29, 2026
Comment thread src/backends/native/sentry_crash_daemon.c Outdated
Comment thread src/backends/native/sentry_crash_daemon.c
Comment on lines +2147 to +2157
}

sentry_value_t contexts = sentry_value_get_by_key(event, "contexts");
if (sentry_value_get_type(contexts) != SENTRY_VALUE_TYPE_OBJECT) {
contexts = sentry_value_new_object();
sentry_value_set_by_key(event, "contexts", contexts);
}
sentry_value_set_by_key(contexts, "wer", wer_context);
return true;
}
#endif
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: A race condition causes the daemon to look for WER metadata files before they are created, preventing their capture.
Severity: HIGH

Suggested Fix

The order of operations must be changed. The lookup for WER metadata files should be performed only after the crashing process has returned EXCEPTION_CONTINUE_SEARCH, which allows the WER system time to generate the necessary files. This may require adjusting the inter-process communication logic to signal when the files are ready or moving the lookup to a later stage in the crash processing pipeline.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/backends/native/sentry_crash_daemon.c#L2124-L2157

Potential issue: A race condition exists in the Windows Error Reporting (WER) metadata
capture logic. The Sentry daemon process calls `sentry__wer_report_lookup` to poll for
WER metadata XML files. This occurs while the crashing application is paused, waiting
for the daemon to complete its work in `wait_for_daemon_capture`. However, the WER
system only generates the required metadata files after the crashing application's
exception filter returns `EXCEPTION_CONTINUE_SEARCH`, which happens after the daemon has
already finished its polling and timed out. As a result, the lookup for WER metadata
will consistently fail, preventing its inclusion in the crash report.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread src/backends/native/sentry_wer.c
Comment thread src/backends/native/sentry_wer_report.c
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 98a675c. Configure here.

sentry_value_t wer_context = sentry__wer_report_lookup(ctx->crash_event_id);
if (sentry_value_is_null(wer_context)) {
if (sentry__string_empty(ctx->platform.wer_report_id)) {
return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unnecessary 5-second blocking for SIGABRT with WER enabled

Medium Severity

When wer_enabled is true but the crash is STATUS_FATAL_APP_EXIT (SIGABRT/abort), the crash handler falls through to the non-WER path which waits for daemon completion via wait_for_daemon_capture. The daemon then calls add_wer_context, which invokes sentry__wer_report_lookup — polling the WER temp directory for up to 5 seconds (20 × 250ms Sleep). Since WER is never invoked for STATUS_FATAL_APP_EXIT, this metadata file will never exist, so the poll always exhausts all retries. This wastes 5 of the 10-second SENTRY_CRASH_HANDLER_WAIT_TIMEOUT_MS budget, leaving only 5 seconds for actual crash processing (minidump writing, event building, transport). On slower machines this could cause the crash handler to time out and the process to terminate before the daemon finishes writing the minidump.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 98a675c. Configure here.

Comment on lines +166 to 173
}

// Always attempt to signal the daemon. If the crash handler claimed
// the crash first, it already SetEvent'd via sentry__crash_ipc_notify,
// but a redundant SetEvent on an auto-reset event is harmless.
if (SetEvent(event)) {
claimed = TRUE;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The process_wer_exception callback returns without waiting for the daemon to capture the minidump, creating a race condition where the process may terminate before a crash report is generated.
Severity: HIGH

Suggested Fix

Reintroduce a waiting mechanism in the process_wer_exception function. The function should wait for a signal from the daemon confirming that the minidump has been captured before returning. This could be done by restoring the previous polling loop that checks for the SENTRY_CRASH_STATE_CAPTURED state.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/backends/native/sentry_wer.c#L166-L173

Potential issue: In the Windows Error Reporting (WER) integration, the
`process_wer_exception` callback function was modified to return immediately after
notifying the crash-reporting daemon, removing a loop that previously waited for the
daemon to confirm it had captured the minidump. This creates a race condition. If WER
proceeds to terminate the crashed process immediately after the callback returns, the
daemon may not have had sufficient time to write the minidump, which requires reading
from the process's memory. This can result in failed or incomplete crash reports for
applications running on systems with WER enabled.

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