Skip to content

ref: attachment manifest - #2108

Merged
jpnurmi merged 7 commits into
masterfrom
jpnurmi/ref/attachment-manifest
Sep 22, 2026
Merged

jpnurmi merged 7 commits into
masterfrom
jpnurmi/ref/attachment-manifest

Conversation

@jpnurmi

@jpnurmi jpnurmi commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Use a MessagePack stream for __sentry-attachments - the same file format we already use for __sentry-breadcrumbN. This lets Crashpad consume attachments with the existing MessagePack reader, without having to introduce JSON support/dependencies:

The native backend currently uses the manifest for all attachments (changes in #1834), whereas crashpad only needs it for crash-time hint attachments (#2099) that cannot cross backend IPC.

As a bonus, the binary format is faster to serialize. Median benchmarks measured write/read improvements over legacy JSON of 17%/6% for one attachment, 74%/17% for 10, and 86%/24% for 100:

1 attachment 10 attachments 100 attachments
MessagePack write 2.3 us 2.4 us 11.8 us
JSON write 2.7 us 8.9 us 82.6 us
MessagePack read 1.3 us 5.6 us 54.1 us
JSON read 1.4 us 6.8 us 71.1 us

The legacy JSON format is kept private in the native daemon for compatibility, so it can still consume manifests written by older SDK versions.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread src/backends/sentry_backend_native.c
Comment thread src/sentry_attachment.c
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/attachment-manifest branch from d8eb6bb to f0d7bcf Compare September 17, 2026 16:51
@codecov

codecov Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.58525% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.32%. Comparing base (6b0d5f0) to head (5f3b00e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2108      +/-   ##
==========================================
+ Coverage   75.15%   75.32%   +0.16%     
==========================================
  Files         103      103              
  Lines       27930    27922       -8     
  Branches     5094     5100       +6     
==========================================
+ Hits        20990    21031      +41     
+ Misses       5596     5556      -40     
+ Partials     1344     1335       -9     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/sentry_attachment.c
Use a shared MessagePack stream for attachment manifests. The native
backend uses the manifest for attachments generally, while both
out-of-process crash handlers can also use it for crash-time hint
attachments that cannot cross backend IPC. This lets Crashpad consume
those attachments without adding JSON support.

Stream attachment objects directly to avoid format-specific conversion
and buffer the complete manifest for a single file write. Median release
benchmarks measured write/read improvements over legacy JSON of 17%/6%
for one attachment, 74%/17% for 10, and 86%/24% for 100:

                    1 attachment   10 attachments   100 attachments
  MessagePack write       2.3 us           2.4 us            11.8 us
  JSON write              2.7 us           8.9 us            82.6 us
  MessagePack read        1.3 us           5.6 us            54.1 us
  JSON read               1.4 us           6.8 us            71.1 us

Keep legacy JSON compatibility private to the native daemon so it can
still consume manifests written by older SDK versions.
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/attachment-manifest branch from bebb882 to 6b691ac Compare September 21, 2026 07:25

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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.

Fix All in Cursor

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

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6b691ac. Configure here.

Comment thread src/backends/native/sentry_crash_daemon.c
Comment thread src/sentry_attachment.c Outdated
Comment thread src/sentry_attachment.c

@limbonaut limbonaut left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👀👍

@jpnurmi
jpnurmi merged commit eb587b0 into master Sep 22, 2026
75 checks passed
@jpnurmi
jpnurmi deleted the jpnurmi/ref/attachment-manifest branch September 22, 2026 12:55
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.

3 participants