Fix UserFeedback disk cache name conflicts with linked events

[markushi]

📜 Description

Until now we used the event-id (if available) for determining the file name for the disk cache. But this fails for UserFeedback because it’s event-id can be identical to the event-id of a linked (Crash) Event.

Cocoa is using a unique file name based on a counter + random UUID, which sounds reasonable, so I resorted to always use as random UUID as well.

💡 Motivation and Context

Fixes #2902

💚 How did you test it?

Added unit tests

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	382.80 ms	461.41 ms	78.61 ms
Size	1.70 MiB	2.27 MiB	584.63 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
8ff8fd0	432.77 ms	495.18 ms	62.41 ms
86f0096	368.63 ms	446.92 ms	78.29 ms
c7e2fbc	377.85 ms	426.35 ms	48.50 ms
2a3dd50	363.61 ms	440.18 ms	76.57 ms
0bd723b	375.20 ms	452.41 ms	77.20 ms
2465853	400.64 ms	465.47 ms	64.83 ms
4e29063	364.08 ms	445.51 ms	81.43 ms
4e260b3	388.40 ms	468.98 ms	80.58 ms
e6ffd7b	379.52 ms	449.30 ms	69.78 ms
c7e2fbc	393.98 ms	478.24 ms	84.27 ms

App size

Revision	Plain	With Sentry	Diff
8ff8fd0	1.72 MiB	2.27 MiB	558.15 KiB
86f0096	1.72 MiB	2.29 MiB	576.50 KiB
c7e2fbc	1.72 MiB	2.29 MiB	576.40 KiB
2a3dd50	1.72 MiB	2.27 MiB	555.05 KiB
0bd723b	1.72 MiB	2.29 MiB	578.09 KiB
2465853	1.70 MiB	2.27 MiB	583.82 KiB
4e29063	1.72 MiB	2.29 MiB	578.38 KiB
4e260b3	1.72 MiB	2.27 MiB	554.95 KiB
e6ffd7b	1.72 MiB	2.29 MiB	579.13 KiB
c7e2fbc	1.72 MiB	2.29 MiB	576.40 KiB

Previous results on branch: fix/cache-conflict

Startup times

Revision	Plain	With Sentry	Diff
6070114	392.96 ms	449.68 ms	56.72 ms
d39edd7	391.06 ms	473.57 ms	82.51 ms
cfa7caa	392.00 ms	470.24 ms	78.24 ms
673e0bb	433.27 ms	575.22 ms	141.96 ms
b57cee4	390.69 ms	443.68 ms	52.99 ms
03dc342	401.17 ms	477.33 ms	76.16 ms
829f669	383.06 ms	444.41 ms	61.35 ms

App size

Revision	Plain	With Sentry	Diff
6070114	1.70 MiB	2.27 MiB	583.73 KiB
d39edd7	1.72 MiB	2.27 MiB	558.33 KiB
cfa7caa	1.70 MiB	2.27 MiB	584.63 KiB
673e0bb	1.72 MiB	2.27 MiB	558.30 KiB
b57cee4	1.70 MiB	2.27 MiB	583.74 KiB
03dc342	1.70 MiB	2.27 MiB	583.73 KiB
829f669	1.72 MiB	2.27 MiB	558.44 KiB

[romtsn]

LGTM, but I only see one potential issue with this: when we are sending envelopes on app start (in DirectoryProcessor), there's a case when rate limiting gets active for an envelope and we're discarding it from cache here:

sentry-java/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

Lines 89 to 93 in 937879e

	if (filteredEnvelope == null) {
	if (cached) {
	envelopeCache.discard(envelope);
	}
	} else {

With this change, the .discard method will not do anything, because it will not find the correct envelope file name, but will just generate a new one. I don't know if we should account for this case, but I guess we should? Or we could actually just remove this check I guess, because #2937 is doing all that already? And we anyway delete the envelope file in proccessFile here:

sentry-java/sentry/src/main/java/io/sentry/EnvelopeSender.java

Lines 91 to 106 in 937879e

	// Unless the transport marked this to be retried, it'll be deleted.
	HintUtils.runIfHasTypeLogIfNot(
	hint,
	Retryable.class,
	logger,
	(retryable) -> {
	if (!retryable.isRetry()) {
	safeDelete(file, "after trying to capture it");
	logger.log(SentryLevel.DEBUG, "Deleted file %s.", file.getAbsolutePath());
	} else {
	logger.log(
	SentryLevel.INFO,
	"File not deleted since retry was marked. %s.",
	file.getAbsolutePath());
	}
	});

[markushi]

LGTM, but I only see one potential issue with this: when we are sending envelopes on app start (in DirectoryProcessor), there's a case when rate limiting gets active for an envelope and we're discarding it from cache here:

sentry-java/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

Lines 89 to 93 in 937879e

	if (filteredEnvelope == null) {
	if (cached) {
	envelopeCache.discard(envelope);
	}
	} else {

With this change, the .discard method will not do anything, because it will not find the correct envelope file name, but will just generate a new one. I don't know if we should account for this case, but I guess we should? Or we could actually just remove this check I guess, because #2937 is doing all that already? And we anyway delete the envelope file in proccessFile here:

sentry-java/sentry/src/main/java/io/sentry/EnvelopeSender.java

Lines 91 to 106 in 937879e

	// Unless the transport marked this to be retried, it'll be deleted.
	HintUtils.runIfHasTypeLogIfNot(
	hint,
	Retryable.class,
	logger,
	(retryable) -> {
	if (!retryable.isRetry()) {
	safeDelete(file, "after trying to capture it");
	logger.log(SentryLevel.DEBUG, "Deleted file %s.", file.getAbsolutePath());
	} else {
	logger.log(
	SentryLevel.INFO,
	"File not deleted since retry was marked. %s.",
	file.getAbsolutePath());
	}
	});

Yeah, good callout!
But doesn't the DirectoryProcessor set the SendCachedEnvelopeHint in order to avoid re-caching the same envelope? So cached would be false here, which is fine:

sentry-java/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

Lines 89 to 93 in 937879e

	if (filteredEnvelope == null) {
	if (cached) {
	envelopeCache.discard(envelope);
	}
	} else {

@romtsn But I think I'm missing something, as where would the file from cache be finally deleted?

[romtsn]

So cached would be false here, which is fine:

It'll be true actually, because SendCachedEnvelopeHint implements Cached:

sentry-java/sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

Lines 81 to 85 in 2465853

	if (HintUtils.hasType(hint, Cached.class)) {
	currentEnvelopeCache = NoOpEnvelopeCache.getInstance();
	cached = true;
	options.getLogger().log(SentryLevel.DEBUG, "Captured Envelope is already cached");
	}

@romtsn But I think I'm missing something, as where would the file from cache be finally deleted?

It's done here

sentry-java/sentry/src/main/java/io/sentry/EnvelopeSender.java

Lines 97 to 99 in 2465853

	if (!retryable.isRetry()) {
	safeDelete(file, "after trying to capture it");
	logger.log(SentryLevel.DEBUG, "Deleted file %s.", file.getAbsolutePath());

and looking at this we probably don't even need that envelopeCache.discard call, because the envelope is anyway gonna be deleted here^