fix: Add null-handling for parsed DSN in SentryHTTPTransport #5800

philprime · Aug 5, 2025

This PR is derived from #5572 in an effort to make the large amount of changes easier to review for #5577.

Adds null-handling for the SentryOptions#parsedDsn used in the SentryHTTPTransport.

This should actually be considered an invalid SDK state, therefore I added fatal logs.

Open for discussion if deleting the envelope and sending the next one is too drastic.

codecov · Aug 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@3c86ff3). Learn more about missing BASE report.
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #5800   +/-   ##
========================================
  Coverage        ?   86.674%           
========================================
  Files           ?       423           
  Lines           ?     36547           
  Branches        ?     17288           
========================================
  Hits            ?     31677           
  Misses          ?      4825           
  Partials        ?        45

Files with missing lines	Coverage Δ
Sources/Sentry/SentryHttpTransport.m	`98.214% <100.000%> (ø)`
Sources/Sentry/SentryTransportFactory.m	`100.000% <100.000%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c86ff3...b656c46. Read the comment docs.

philipphofmann

I'm a bit unsure about the approach. I think it changes the logic.

Sources/Sentry/SentryHttpTransport.m

…ability-10

…eports properties - Changed the initialization method to accept SentryDsn and sendClientReports instead of SentryOptions. - Updated related methods to use the new properties for client report handling. - Adjusted tests to reflect the new initialization parameters and ensure proper functionality.

philprime · Aug 8, 2025

cursor review

github-actions · Aug 8, 2025

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1193.56 ms	1238.82 ms	45.25 ms
Size	23.75 KiB	920.45 KiB	896.70 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`d38165b`	1211.41 ms	1242.49 ms	31.08 ms
`4bf98bf`	1219.13 ms	1244.50 ms	25.38 ms
`8047b99`	1226.37 ms	1246.63 ms	20.26 ms
`7416ffc`	1225.55 ms	1241.80 ms	16.25 ms
`7f2f69c`	1237.61 ms	1266.96 ms	29.35 ms
`ad964ca`	1234.73 ms	1254.88 ms	20.15 ms
`07d7e83`	1211.71 ms	1240.08 ms	28.37 ms
`db9572a`	1200.27 ms	1234.80 ms	34.53 ms
`2a7868a`	1226.54 ms	1256.92 ms	30.37 ms
`605fa27`	1226.31 ms	1251.35 ms	25.05 ms

App size

Revision	Plain	With Sentry	Diff
`d38165b`	23.75 KiB	855.37 KiB	831.62 KiB
`4bf98bf`	23.75 KiB	919.70 KiB	895.95 KiB
`8047b99`	23.75 KiB	855.37 KiB	831.62 KiB
`7416ffc`	23.75 KiB	913.63 KiB	889.88 KiB
`7f2f69c`	23.75 KiB	913.38 KiB	889.63 KiB
`ad964ca`	23.75 KiB	913.17 KiB	889.42 KiB
`07d7e83`	23.75 KiB	913.27 KiB	889.52 KiB
`db9572a`	23.75 KiB	858.69 KiB	834.93 KiB
`2a7868a`	23.75 KiB	880.20 KiB	856.45 KiB
`605fa27`	23.75 KiB	908.03 KiB	884.28 KiB

Previous results on branch: philprime/strict-nullability-10

Startup times

Revision	Plain	With Sentry	Diff
`1cc2a9e`	1229.33 ms	1249.19 ms	19.86 ms
`388f479`	1212.06 ms	1242.74 ms	30.68 ms
`5091f7c`	1224.33 ms	1245.30 ms	20.96 ms

App size

Revision	Plain	With Sentry	Diff
`1cc2a9e`	23.75 KiB	920.20 KiB	896.46 KiB
`388f479`	23.75 KiB	913.70 KiB	889.95 KiB
`5091f7c`	23.75 KiB	920.20 KiB	896.46 KiB

philipphofmann

LGTM, with a comment on the log message.

Sources/Sentry/SentryTransportFactory.m

…ability-10

…ut dsn

philipphofmann

LGTM

…ability-10

philprime · Aug 13, 2025

Reposting because Cursor Bot marks comments as outdated:
#5800 (comment)

@philipphofmann do you think this is valid feedback?

The SentryTransportFactory no longer creates an SentryHttpTransport when options.parsedDsn is nil. This removes the mechanism that previously eagerly processed and deleted cached envelopes on startup. As a result, cached envelopes are never processed or purged, leading to unbounded cache accumulation on disk, particularly when the DSN is temporarily disabled or misconfigured.

philipphofmann · Aug 13, 2025

Reposting because Cursor Bot marks comments as outdated: #5800 (comment)

@philipphofmann do you think this is valid feedback?

The SentryTransportFactory no longer creates an SentryHttpTransport when options.parsedDsn is nil. This removes the mechanism that previously eagerly processed and deleted cached envelopes on startup. As a result, cached envelopes are never processed or purged, leading to unbounded cache accumulation on disk, particularly when the DSN is temporarily disabled or misconfigured.

Yes, this is valid feedback. If you previously had options.parsedDsn being nil, it helped cleaning up some old envelopes. But the client discards most data anyways when you don't have a dsn set

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 923 to 926 in ed85746

    
           - (BOOL)isDisabled 
        
           { 
        
               return !_isEnabled || !self.options.enabled || nil == self.options.parsedDsn; 
        
           }

We could add a cleanup call to the file manager or anywhere else that deletes envelopes in the fallback folder when the DSN is nil, but it's also some overhead for everybody checking if the folder exists and has files. What would you suggest.

philprime · Aug 13, 2025

But isn't that what you wanted me to change in this PR comment when you gave the first round of feedback?

philipphofmann · Aug 13, 2025

We can keep this. We have a safeguard that I forgot about here

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 166 to 168 in 32584c4

    
           if (deleteOldEnvelopeItems) { 
        
               [fileManager deleteOldEnvelopeItems]; 
        
           }

I thought that we had the safeguard in the transport, but it's in the client. Sorry my bad.

Before this PR, when you didn't set a DSN, the client should not have stored any envelopes to disk via the transport. I thought that when we remove the transport, we stop cleaning up old envelopes at some point when not setting a DSN, because I thought the logic was in the transport. But it's in the client so all good. The logic stays the same.

philprime · Aug 14, 2025

Alright in that case I'll merge it as-is.

We merged #5800 after releasing 8.55.0. We need to move it up to the unreleased section.

fix: Add null-handling for parsed DSN in SentryHTTPTransport

0f0443d

philprime self-assigned this Aug 5, 2025

philprime requested review from armcknight, itaybre, noahsmartin and philipphofmann as code owners August 5, 2025 10:34

philprime added 2 commits August 5, 2025 12:34

update changelog

17fa000

update changelog

f3cf991

philipphofmann reviewed Aug 5, 2025

View reviewed changes

Sources/Sentry/SentryHttpTransport.m Outdated Show resolved Hide resolved

philprime marked this pull request as draft August 7, 2025 09:07

philprime added 2 commits August 8, 2025 11:24

Merge remote-tracking branch 'origin/main' into philprime/strict-null…

aeed7d9

…ability-10

philprime marked this pull request as ready for review August 8, 2025 09:45

This comment was marked as outdated.

Sign in to view

philprime requested a review from philipphofmann August 8, 2025 09:51

philipphofmann approved these changes Aug 11, 2025

View reviewed changes

Sources/Sentry/SentryTransportFactory.m Outdated Show resolved Hide resolved

philprime added 2 commits August 11, 2025 17:41

Merge remote-tracking branch 'origin/main' into philprime/strict-null…

6d66fbd

…ability-10

refactored http transport factory to enable spotlight transport witho…

02cbc4b

…ut dsn

philprime requested a review from philipphofmann August 11, 2025 16:13

philipphofmann approved these changes Aug 12, 2025

View reviewed changes

Merge branch 'main' into philprime/strict-nullability-10

354e5db

philprime added the Waiting for: CI 🤖 Marks PR as ready-for-merge when CI passes. To be replaced with required checks. label Aug 12, 2025

This comment was marked as outdated.

Sign in to view

philprime added 2 commits August 12, 2025 15:52

Merge remote-tracking branch 'origin/main' into philprime/strict-null…

bdf8d6b

…ability-10

fix formatting

b656c46

philprime enabled auto-merge (squash) August 13, 2025 10:14

philprime merged commit 52f3b6e into main Aug 14, 2025
160 of 165 checks passed

philprime deleted the philprime/strict-nullability-10 branch August 14, 2025 10:21

philipphofmann added a commit that referenced this pull request Sep 8, 2025

Fix Changelog: Move PR 5800 to unreleased

1b85e67

We merged #5800 after releasing 8.55.0. We need to move it up to the unreleased section.

philipphofmann mentioned this pull request Sep 8, 2025

Fix Changelog: Move PR 5800 to unreleased #6097

Merged

philipphofmann added a commit that referenced this pull request Sep 8, 2025

Fix Changelog: Move PR 5800 to unreleased (#6097)

7629df7

We merged #5800 after releasing 8.55.0. We need to move it up to the unreleased section.

Search code, repositories, users, issues, pull requests...

Uh oh!

fix: Add null-handling for parsed DSN in SentryHTTPTransport #5800

fix: Add null-handling for parsed DSN in SentryHTTPTransport #5800

Uh oh!

Conversation

philprime commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philprime commented Aug 8, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: philprime/strict-nullability-10

Startup times

App size

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

philprime commented Aug 13, 2025

Uh oh!

philipphofmann commented Aug 13, 2025

Uh oh!

philprime commented Aug 13, 2025

Uh oh!

philipphofmann commented Aug 13, 2025

Uh oh!

philprime commented Aug 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

philprime commented Aug 5, 2025 •

edited

Loading

codecov bot commented Aug 5, 2025 •

edited

Loading

github-actions bot commented Aug 8, 2025 •

edited

Loading