RUM-3175 Fix possible file name conflict during consent change #2113
Conversation
ncreated
changed the title
ncreated
changed the title
ncreated
force-pushed
the
ncreated/RUM-3175/fix-batch-name-conflict-on-consent-change
branch
from
ecb178e
to
fe7d709
Compare
…t change Addressed potential conflicts that could occur when files are moved between unauthorized (.pending) and authorized (.granted) folders during tracking consent changes. Ensured that new file names are generated with the correct precision to avoid conflicts in real-device scenarios.
ncreated
force-pushed
the
ncreated/RUM-3175/fix-batch-name-conflict-on-consent-change
branch
from
fe7d709
to
0740e49
Compare
Datadog ReportBranch report: ✅ 0 Failed, 3545 Passed, 0 Skipped, 2m 21.69s Total Time 🔻 Code Coverage Decreases vs Default Branch (4) |
| var newFileName = fileNameFrom(fileCreationDate: dateProvider.now) | ||
| while directory.hasFile(named: newFileName) { | ||
| // Wait for the precision duration to avoid generating the same file name | ||
| Thread.sleep(forTimeInterval: Constants.fileNamePrecision) |
There was a problem hiding this comment.
I understand that this scenario is rare, but instead of blocking the thread, could we consider appending a random ID to ensure file name uniqueness? Or is a date in the filename required for identification purposes or another logic?
There was a problem hiding this comment.
I'm also not a fan of adding a sleep here. Since the date is used to sort batches at upload, can't we just add a 1ms to the dateProvider.now instead?
There was a problem hiding this comment.
Makes sense, I've updated the code to add 1ms in case of a conflict 👍 ✅.
@mariedm, appending a random ID could have more indirect consequences here. On upload trigger, we read the recent batches and sort them by date. Introducing an ID would make the ordering ambiguous if two timestamps are the same, which would require additional code changes to handle these edge cases. For that reason, adjusting the timestamp seems like a less impactful solution.
There was a problem hiding this comment.
In that case I agree, using a random ID doesn't make sense.
do not block the I/O thread if name conflict is detected, instead add precision interval to generate new name
ncreated
deleted the
ncreated/RUM-3175/fix-batch-name-conflict-on-consent-change
branch
What and why?
📦 This PR addresses a potential file name conflict issue during tracking consent changes.
Initially considered a flakiness ❄️ in testWhenWritingEventsWithPendingConsentThenGranted_itUploadsAllEvents() (added in #2063), it was discovered to be a real issue that can occur in certain edge cases.
The conflict happens when files are moved between
.pendingand.grantedfolders and events are written immediately before and after the consent change. If this occurs very quickly (under 1ms), the last batch in the pending folder may be named the same as the next batch in the granted folder. Since the SDK does not check if the file exists, the migrated file gets overwritten by the new event:How?
This fix ensures that batch file names are unique, even in the edge case above, by waiting and generating a new file name when a conflict is detected. It guarantees that the new file's timestamp will be at least one precision interval (1ms) later than the existing file, preventing conflicts during consent changes.
Review checklist
make api-surface)