fix(profiling): fix macOS launch profiling issue

[armcknight]

fixes issue reported in #5142 and reproduced in the tests in #5143

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[codecov]

Codecov Report

Attention: Patch coverage is 96.35036% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.799%. Comparing base (ec09d0f) to head (cbeaea5).
Report is 2 commits behind head on armcknight/profiling/test/mac-app-launch-profiling-config-file.

Files with missing lines	Patch %	Lines
Sources/Sentry/SentryFileManager.m	89.361%	5 Missing ⚠️

Additional details and impacted files

@@                                         Coverage Diff                                          @@
##           armcknight/profiling/test/mac-app-launch-profiling-config-file     #5144       +/-   ##
====================================================================================================
+ Coverage                                                          92.785%   92.799%   +0.014%     
====================================================================================================
  Files                                                                 676       676               
  Lines                                                               83888     84026      +138     
  Branches                                                            30545     30597       +52     
====================================================================================================
+ Hits                                                                77836     77976      +140     
+ Misses                                                               5953      5952        -1     
+ Partials                                                               99        98        -1     

Files with missing lines	Coverage Δ
...ts/SentryTests/Helper/SentryFileManagerTests.swift	96.945% <100.000%> (+0.231%)	⬆️
Sources/Sentry/SentryFileManager.m	94.126% <89.361%> (-0.387%)	⬇️

... and 11 files with indirect coverage changes

---

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 ec09d0f...cbeaea5. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1236.04 ms	1255.53 ms	19.49 ms
Size	22.30 KiB	849.55 KiB	827.25 KiB

Baseline results on branch: armcknight/profiling/test/mac-app-launch-profiling-config-file

Startup times

Revision	Plain	With Sentry	Diff
dfab59c	1240.43 ms	1259.83 ms	19.40 ms
0ac3953	1212.67 ms	1236.52 ms	23.85 ms
987aede	1218.45 ms	1240.68 ms	22.23 ms
f699e3c	1203.06 ms	1238.67 ms	35.61 ms

App size

Revision	Plain	With Sentry	Diff
dfab59c	22.30 KiB	851.81 KiB	829.51 KiB
0ac3953	22.30 KiB	851.81 KiB	829.50 KiB
987aede	22.30 KiB	851.81 KiB	829.51 KiB
f699e3c	22.31 KiB	851.81 KiB	829.51 KiB

Previous results on branch: armcknight/profiling/fix/mac-app-nonsandboxed-launch-profiling

Startup times

Revision	Plain	With Sentry	Diff
a3a845f	1223.47 ms	1242.00 ms	18.53 ms
f4097e8	1223.24 ms	1234.51 ms	11.27 ms
c04d63b	1225.94 ms	1243.76 ms	17.82 ms
91caa36	1229.90 ms	1254.02 ms	24.13 ms
75082a4	1228.67 ms	1238.73 ms	10.06 ms
824f241	1237.29 ms	1250.47 ms	13.18 ms
6a22387	1228.20 ms	1255.57 ms	27.37 ms

App size

Revision	Plain	With Sentry	Diff
a3a845f	22.31 KiB	852.32 KiB	830.01 KiB
f4097e8	22.30 KiB	852.31 KiB	830.00 KiB
c04d63b	22.30 KiB	851.88 KiB	829.57 KiB
91caa36	22.30 KiB	851.86 KiB	829.56 KiB
75082a4	22.31 KiB	852.31 KiB	830.01 KiB
824f241	22.30 KiB	851.88 KiB	829.57 KiB
6a22387	22.30 KiB	851.74 KiB	829.43 KiB

[philprime]

We're getting there. Thanks for looking into this.

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[philprime]

Refactored the implementation for testability, added tests and comments.

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[armcknight]

@philprime I think based on my question, we will need an additional test case, where we call sentryBuildScopedCachesDirectoryPath twice, the first time with a nil bundle id and a nonnil lastPathComponent, then the second time with a nonnil bundle path and the same lastPathComponent, and assert that we still use the lastPathComponent version of the static base path.

That also leads to other combinations but unsure what is actually possible and how defensive we want to get with this logic.

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[philipphofmann]

I maybe found one important issue to address, but close to LGTM.

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m

[philprime]

Summary from internal Slack discussion on keep using the executable path name even if the bundle identifier becomes available:

We think maintenance cost outweighs the benefit.

• AFAIK it's a rare edge case because every macOS app needs to have a bundle identifier, therefore it's not possible for a macOS app to not have a bundle identifier defined and then suddenly have one.
• CLI tools do not use bundle identifiers, but still the edge case would only happen if a developer suddenly adds a bundle to the CLI and then marks it to be the main bundle.
• The file-exists will be run at every app start of every macOS app without a sandbox, unnecessarily slow down app launch (even if only minimal impact)
• In the worst-case when an app changes from having no bundle identifier to having one, it would change the path and skip one app launch. Then it writes the profileLaunch again, so it is fixed with the next app start
• Having additional logic to maintain for a rare edge case just increases complexity of maintenance for little benefit. As we want to revisit the file hierarchy in Add documentation about cache directory path Library/Caches/ for non sandboxed macOS apps #5145 this could also further increase complexity for any future changes.

To conclude, it's not worth the effort and after careful reconsidering I am now going to revert this handling mechanism.

[github-actions]

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

• Sources/Sentry/SentryFileManager.m