diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt index 22c30bdba5..62646c8da0 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorage.kt @@ -166,6 +166,8 @@ internal class ConsentAwareStorage( val result = fileMover.delete(batchFile) if (result) { + grantedOrchestrator.onFileDeleted(batchFile) + pendingOrchestrator.onFileDeleted(batchFile) val numPendingFiles = grantedOrchestrator.decrementAndGetPendingFilesCount() metricsDispatcher.sendBatchDeletedMetric(batchFile, reason, numPendingFiles) diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FileOrchestrator.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FileOrchestrator.kt index 7003e1be1d..5528c2c735 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FileOrchestrator.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/FileOrchestrator.kt @@ -64,4 +64,17 @@ internal interface FileOrchestrator { * @return the number of pending files in the orchestrator, after decrementing by 1. */ fun decrementAndGetPendingFilesCount(): Int + + /** + * Notifies the orchestrator that a file has been deleted. + * + * @param file the file that was deleted + */ + fun onFileDeleted(file: File) + + /** + * Refreshes the internal file list from disk. + */ + @WorkerThread + fun refreshFilesFromDisk() } diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigrator.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigrator.kt index fa35bbbe9f..6a7ed648e8 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigrator.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigrator.kt @@ -33,6 +33,7 @@ internal class ConsentAwareFileMigrator( newFileOrchestrator ) operation.run() + newFileOrchestrator.refreshFilesFromDisk() } @WorkerThread diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt index 10e60cb5bf..e737b2ad24 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt @@ -72,6 +72,17 @@ internal open class ConsentAwareFileOrchestrator( return delegateOrchestrator.decrementAndGetPendingFilesCount() } + override fun onFileDeleted(file: File) { + grantedOrchestrator.onFileDeleted(file) + pendingOrchestrator.onFileDeleted(file) + } + + @WorkerThread + override fun refreshFilesFromDisk() { + grantedOrchestrator.refreshFilesFromDisk() + pendingOrchestrator.refreshFilesFromDisk() + } + // endregion // region TrackingConsentProviderCallback diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt index dc9bad8ef6..8e805ca74e 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestrator.kt @@ -21,13 +21,12 @@ import com.datadog.android.core.internal.persistence.file.listFilesSafe import com.datadog.android.core.internal.persistence.file.mkdirsSafe import com.datadog.android.internal.time.TimeProvider import java.io.File -import java.io.FileFilter import java.util.Locale +import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger +import java.util.concurrent.atomic.AtomicLong import kotlin.math.roundToLong -// TODO RUM-438 Improve this class: need to make it thread-safe and optimize work with file -// system in order to reduce the number of syscalls (which are expensive) for files already seen @Suppress("TooManyFunctions") internal class BatchFileOrchestrator( private val rootDir: File, @@ -38,8 +37,6 @@ internal class BatchFileOrchestrator( private val pendingFiles: AtomicInteger = AtomicInteger(0) ) : FileOrchestrator { - private val fileFilter = BatchFileFilter() - // Offset the recent threshold for read and write to avoid conflicts // Arbitrary offset as ±5% of the threshold @Suppress("UnsafeThirdPartyFunctionCall") // rounded Double isn't NaN @@ -48,11 +45,11 @@ internal class BatchFileOrchestrator( @Suppress("UnsafeThirdPartyFunctionCall") // rounded Double isn't NaN private val recentWriteDelayMs = (config.recentDelayMs * DECREASE_PERCENT).roundToLong() - // keep track of how many items were written in the last known file - private var previousFile: File? = null - private var previousFileItemCount: Long = 0 - private var lastFileAccessTimestamp: Long = 0L - private var lastCleanupTimestamp: Long = 0L + private var currentBatchState = CurrentBatchState(null, 0L, 0L) + private val lastCleanupTimestamp = AtomicLong(0L) + private val areKnownFilesInitialized = AtomicBoolean(false) + + private val knownFiles: MutableSet = mutableSetOf() // region FileOrchestrator @@ -66,7 +63,7 @@ internal class BatchFileOrchestrator( var files = listBatchFiles() files = deleteObsoleteFiles(files) freeSpaceIfNeeded(files) - lastCleanupTimestamp = timeProvider.getDeviceTimestampMillis() + lastCleanupTimestamp.set(timeProvider.getDeviceTimestampMillis()) } return getReusableWritableFile() ?: createNewFile() @@ -78,14 +75,19 @@ internal class BatchFileOrchestrator( return null } - val files = listSortedBatchFiles().let { - deleteObsoleteFiles(it) - } - lastCleanupTimestamp = timeProvider.getDeviceTimestampMillis() + val files = deleteObsoleteFiles(listSortedBatchFiles()) + lastCleanupTimestamp.set(timeProvider.getDeviceTimestampMillis()) pendingFiles.set(files.count()) return files.firstOrNull { - (it !in excludeFiles) && !isFileRecent(it, recentReadDelayMs) + when { + it in excludeFiles || isFileRecent(it, recentReadDelayMs) -> false + !it.existsSafe(internalLogger) -> { + onFileDeleted(it) + false + } + else -> true + } } } @@ -138,112 +140,130 @@ internal class BatchFileOrchestrator( } } - // endregion - - // region Internal - override fun decrementAndGetPendingFilesCount(): Int { return pendingFiles.decrementAndGet() } - @Suppress("LiftReturnOrAssignment", "ReturnCount") + override fun onFileDeleted(file: File) { + synchronized(knownFiles) { + knownFiles.remove(file) + } + if (currentBatchState.file == file) { + currentBatchState = CurrentBatchState(null, 0L, currentBatchState.lastAccessTimestamp) + } + } + + @WorkerThread + override fun refreshFilesFromDisk() { + val files = rootDir.listFilesSafe(internalLogger)?.filter { it.isBatchFile } + if (files != null) { + synchronized(knownFiles) { + knownFiles.clear() + knownFiles.addAll(files) + } + } + } + + // endregion + + // region Private + + @WorkerThread + @Suppress("ReturnCount") private fun isRootDirValid(): Boolean { - if (rootDir.existsSafe(internalLogger)) { - if (rootDir.isDirectory) { - if (rootDir.canWriteSafe(internalLogger)) { - return true - } else { + val isValid = if (rootDir.existsSafe(internalLogger)) { + when { + !rootDir.isDirectory -> { internalLogger.log( InternalLogger.Level.ERROR, - listOf( - InternalLogger.Target.MAINTAINER, - InternalLogger.Target.TELEMETRY - ), - { ERROR_ROOT_NOT_WRITABLE.format(Locale.US, rootDir.path) } + listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY), + { ERROR_ROOT_NOT_DIR.format(Locale.US, rootDir.path) } ) - return false - } - } else { - internalLogger.log( - InternalLogger.Level.ERROR, - listOf( - InternalLogger.Target.MAINTAINER, - InternalLogger.Target.TELEMETRY - ), - { ERROR_ROOT_NOT_DIR.format(Locale.US, rootDir.path) } - ) - return false - } - } else { - synchronized(rootDir) { - // double check if directory was already created by some other thread - // entered this branch - if (rootDir.existsSafe(internalLogger)) { - return true + false } - if (rootDir.mkdirsSafe(internalLogger)) { - return true - } else { + !rootDir.canWriteSafe(internalLogger) -> { internalLogger.log( InternalLogger.Level.ERROR, - listOf( - InternalLogger.Target.MAINTAINER, - InternalLogger.Target.TELEMETRY - ), - { ERROR_CANT_CREATE_ROOT.format(Locale.US, rootDir.path) } + listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY), + { ERROR_ROOT_NOT_WRITABLE.format(Locale.US, rootDir.path) } ) - return false + false } + + else -> true } + } else { + createRootDirectory() + } + + if (isValid && areKnownFilesInitialized.compareAndSet(false, true)) refreshFilesFromDisk() + return isValid + } + + private fun createRootDirectory(): Boolean = synchronized(rootDir) { + val created = rootDir.existsSafe(internalLogger) || rootDir.mkdirsSafe(internalLogger) + if (!created) { + internalLogger.log( + InternalLogger.Level.ERROR, + listOf(InternalLogger.Target.MAINTAINER, InternalLogger.Target.TELEMETRY), + { ERROR_CANT_CREATE_ROOT.format(Locale.US, rootDir.path) } + ) } + created } private fun createNewFile(): File { - val newFileName = timeProvider.getDeviceTimestampMillis().toString() - val newFile = File(rootDir, newFileName) - val closedFile = previousFile - val closedFileLastAccessTimestamp = lastFileAccessTimestamp - if (closedFile != null) { + val state = currentBatchState + if (state.file != null) { metricsDispatcher.sendBatchClosedMetric( - closedFile, + state.file, BatchClosedMetadata( - lastTimeWasUsedInMs = closedFileLastAccessTimestamp, - eventsCount = previousFileItemCount + lastTimeWasUsedInMs = state.lastAccessTimestamp, + eventsCount = state.itemCount ) ) } - previousFile = newFile - previousFileItemCount = 1 - lastFileAccessTimestamp = timeProvider.getDeviceTimestampMillis() + pendingFiles.incrementAndGet() + + val newFile = File(rootDir, timeProvider.getDeviceTimestampMillis().toString()) + currentBatchState = CurrentBatchState(newFile, 1L, timeProvider.getDeviceTimestampMillis()) + synchronized(knownFiles) { + knownFiles.add(newFile) + } return newFile } @Suppress("ReturnCount") private fun getReusableWritableFile(): File? { - val files = listBatchFiles() - val lastFile = files.latestBatchFile ?: return null + val latestFile = synchronized(knownFiles) { knownFiles.maxOrNull() } ?: return null + + if (!latestFile.existsSafe(internalLogger)) { + onFileDeleted(latestFile) + return null + } - val lastKnownFile = previousFile - val lastKnownFileItemCount = previousFileItemCount - if (lastKnownFile != lastFile) { + if (currentBatchState.file != latestFile) { // this situation can happen because: - // 1. `lastFile` is a file written during a previous session - // 2. `lastFile` was created by another system/process - // 3. `lastKnownFile` was deleted + // 1. `latestFile` is a file written during a previous session + // 2. `latestFile` was created by another system/process + // 3. `previousFile` was deleted // In any case, we don't know the item count, so to be safe, we create a new file return null } - val isRecentEnough = isFileRecent(lastFile, recentWriteDelayMs) - val hasRoomForMore = lastFile.lengthSafe(internalLogger) < config.maxBatchSize + val isRecentEnough = isFileRecent(latestFile, recentWriteDelayMs) + val hasRoomForMore = latestFile.lengthSafe(internalLogger) < config.maxBatchSize + val lastKnownFileItemCount = currentBatchState.itemCount val hasSlotForMore = (lastKnownFileItemCount < config.maxItemsPerBatch) return if (isRecentEnough && hasRoomForMore && hasSlotForMore) { - previousFileItemCount = lastKnownFileItemCount + 1 - lastFileAccessTimestamp = timeProvider.getDeviceTimestampMillis() - lastFile + currentBatchState = currentBatchState.copy( + itemCount = lastKnownFileItemCount + 1, + lastAccessTimestamp = timeProvider.getDeviceTimestampMillis() + ) + latestFile } else { null } @@ -262,6 +282,7 @@ internal class BatchFileOrchestrator( val isOldFile = (it.name.toLongOrNull() ?: 0) < threshold if (isOldFile) { if (it.deleteSafe(internalLogger)) { + onFileDeleted(it) metricsDispatcher.sendBatchDeletedMetric( batchFile = it, removalReason = RemovalReason.Obsolete, @@ -306,6 +327,10 @@ internal class BatchFileOrchestrator( val size = file.lengthSafe(internalLogger) val wasDeleted = file.deleteSafe(internalLogger) return if (wasDeleted) { + onFileDeleted(file) + if (currentBatchState.file == file) { + currentBatchState = CurrentBatchState(null, 0L, currentBatchState.lastAccessTimestamp) + } if (sendMetric) { metricsDispatcher.sendBatchDeletedMetric(file, RemovalReason.Purged, pendingFiles.decrementAndGet()) } @@ -316,7 +341,9 @@ internal class BatchFileOrchestrator( } private fun listBatchFiles(): List { - return rootDir.listFilesSafe(fileFilter, internalLogger).orEmpty().toList() + return synchronized(knownFiles) { + knownFiles.toList() + } } private fun listSortedBatchFiles(): List { @@ -326,7 +353,7 @@ internal class BatchFileOrchestrator( } private fun canDoCleanup(): Boolean { - return timeProvider.getDeviceTimestampMillis() - lastCleanupTimestamp > config.cleanupFrequencyThreshold + return timeProvider.getDeviceTimestampMillis() - lastCleanupTimestamp.get() > config.cleanupFrequencyThreshold } private val File.metadata: File @@ -335,23 +362,13 @@ internal class BatchFileOrchestrator( private val File.isBatchFile: Boolean get() = name.toLongOrNull() != null - private val List.latestBatchFile: File? - get() = maxOrNull() - // endregion - // region FileFilter - - internal inner class BatchFileFilter : FileFilter { - @Suppress("ReturnCount") - override fun accept(file: File?): Boolean { - if (file == null) return false - - return file.isBatchFile - } - } - - // endregion + private data class CurrentBatchState( + val file: File?, + val itemCount: Long, + val lastAccessTimestamp: Long + ) companion object { diff --git a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/single/SingleFileOrchestrator.kt b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/single/SingleFileOrchestrator.kt index f414a2f32a..4ae42438a7 100644 --- a/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/single/SingleFileOrchestrator.kt +++ b/dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/internal/persistence/file/single/SingleFileOrchestrator.kt @@ -61,5 +61,16 @@ internal class SingleFileOrchestrator( return 0 } + // single file orchestrator has no internal file tracking state, so this is a no-op + override fun onFileDeleted(file: File) { + // No-op + } + + // single file orchestrator has no internal file tracking state, so this is a no-op + @WorkerThread + override fun refreshFilesFromDisk() { + // No-op + } + // endregion } diff --git a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorageTest.kt b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorageTest.kt index fe2630040d..8bc8c1f5c3 100644 --- a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorageTest.kt +++ b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/ConsentAwareStorageTest.kt @@ -403,6 +403,7 @@ internal class ConsentAwareStorageTest { // Then verify(mockFileMover).delete(file) verify(mockFileMover).delete(mockMetaFile) + verify(mockGrantedOrchestrator).onFileDeleted(file) verify(mockMetricsDispatcher).sendBatchDeletedMetric( eq(file), eq(reason), diff --git a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigratorTest.kt b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigratorTest.kt index 7e569e9990..9c0bb9420b 100644 --- a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigratorTest.kt +++ b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileMigratorTest.kt @@ -99,7 +99,7 @@ internal class ConsentAwareFileMigratorTest { } @Test - fun `M wipe pending data W migrateData() {GRANTED to PENDING}`( + fun `M wipe pending data and refresh files W migrateData() {GRANTED to PENDING}`( @Forgery pendingDir: File ) { // Given @@ -116,10 +116,11 @@ internal class ConsentAwareFileMigratorTest { // Then verify(mockFileMover).delete(pendingDir) + verify(mockNewOrchestrator).refreshFilesFromDisk() } @Test - fun `M wipe pending data W migrateData() {NOT_GRANTED to PENDING}`( + fun `M wipe pending data and refresh files W migrateData() {NOT_GRANTED to PENDING}`( @Forgery pendingDir: File ) { // Given @@ -136,10 +137,11 @@ internal class ConsentAwareFileMigratorTest { // Then verify(mockFileMover).delete(pendingDir) + verify(mockNewOrchestrator).refreshFilesFromDisk() } @Test - fun `M move pending data W migrateData() {PENDING to GRANTED}`( + fun `M move pending data and refresh files W migrateData() {PENDING to GRANTED}`( @Forgery pendingDir: File, @Forgery grantedDir: File ) { @@ -158,6 +160,7 @@ internal class ConsentAwareFileMigratorTest { // Then verify(mockFileMover).moveFiles(pendingDir, grantedDir) + verify(mockNewOrchestrator).refreshFilesFromDisk() } @RepeatedTest(8) diff --git a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt index dbe34b22ad..b0e042c817 100644 --- a/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt +++ b/dd-sdk-android-core/src/test/kotlin/com/datadog/android/core/internal/persistence/file/batch/BatchFileOrchestratorTest.kt @@ -10,7 +10,6 @@ import com.datadog.android.api.InternalLogger import com.datadog.android.core.internal.metrics.BatchClosedMetadata import com.datadog.android.core.internal.metrics.MetricsDispatcher import com.datadog.android.core.internal.metrics.RemovalReason -import com.datadog.android.core.internal.persistence.file.FileOrchestrator import com.datadog.android.core.internal.persistence.file.FilePersistenceConfig import com.datadog.android.internal.tests.stub.StubTimeProvider import com.datadog.android.utils.forge.Configurator @@ -55,7 +54,7 @@ import java.util.concurrent.atomic.AtomicInteger @MockitoSettings(strictness = Strictness.LENIENT) internal class BatchFileOrchestratorTest { - private lateinit var testedOrchestrator: FileOrchestrator + private lateinit var testedOrchestrator: BatchFileOrchestrator @TempDir lateinit var tempDir: File @@ -266,6 +265,7 @@ internal class BatchFileOrchestratorTest { assertThat(oldFile).doesNotExist() assertThat(oldFileMeta).doesNotExist() assertThat(youngFile).exists() + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(oldFile) verify(mockMetricsDispatcher).sendBatchDeletedMetric( eq(oldFile), argThat { this is RemovalReason.Obsolete }, @@ -296,6 +296,7 @@ internal class BatchFileOrchestratorTest { // cleanup shouldn't be performed during the next getWritableFile call val evenOlderFile = File(fakeRootDir, (oldTimestamp - 1).toString()) evenOlderFile.createNewFile() + testedOrchestrator.refreshFilesFromDisk() testedOrchestrator.getWritableFile() // Then @@ -333,6 +334,7 @@ internal class BatchFileOrchestratorTest { stubTimeProvider.deviceTimestampMs += CLEANUP_FREQUENCY_THRESHOLD_MS + 1 val evenOlderFile = File(fakeRootDir, (oldTimestamp - 1).toString()) evenOlderFile.createNewFile() + testedOrchestrator.refreshFilesFromDisk() testedOrchestrator.getWritableFile() // Then @@ -470,6 +472,7 @@ internal class BatchFileOrchestratorTest { checkNotNull(previousFile) previousFile.createNewFile() previousFile.delete() + testedOrchestrator.onFileDeleted(previousFile) stubTimeProvider.deviceTimestampMs += 1 val newFileTimestamp = stubTimeProvider.deviceTimestampMs @@ -532,13 +535,13 @@ internal class BatchFileOrchestratorTest { var previousFile = testedOrchestrator.getWritableFile() repeat(4) { - checkNotNull(previousFile) + val currentFile = checkNotNull(previousFile) val previousData = forge.aList(MAX_ITEM_PER_BATCH) { forge.anAlphabeticalString() } - previousFile?.writeText(previousData[0]) + currentFile.writeText(previousData[0]) for (i in 1 until MAX_ITEM_PER_BATCH) { val file = testedOrchestrator.getWritableFile() @@ -558,11 +561,11 @@ internal class BatchFileOrchestratorTest { .doesNotExist() .hasParent(fakeRootDir) assertThat(nextFile.name.toLong()).isEqualTo(newFileTimestamp) - assertThat(previousFile?.readText()) + assertThat(currentFile.readText()) .isEqualTo(previousData.joinToString(separator = "")) argumentCaptor { - verify(mockMetricsDispatcher).sendBatchClosedMetric(eq(previousFile!!), capture()) + verify(mockMetricsDispatcher).sendBatchClosedMetric(eq(currentFile), capture()) assertThat(firstValue.lastTimeWasUsedInMs) .isBetween(fileCreateTimestamp, lastFileUsageTimestamp) assertThat(firstValue.eventsCount).isEqualTo(MAX_ITEM_PER_BATCH.toLong()) @@ -721,6 +724,7 @@ internal class BatchFileOrchestratorTest { assertThat(oldFile).doesNotExist() assertThat(oldFileMeta).doesNotExist() assertThat(youngFile).exists() + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(oldFile) } @Test @@ -1182,6 +1186,282 @@ internal class BatchFileOrchestratorTest { // endregion + // region refreshFilesFromDisk + + @Test + fun `M discover new files W refreshFilesFromDisk()`() { + // Given + val fileName = System.currentTimeMillis().toString() + val file = File(fakeRootDir, fileName) + testedOrchestrator.getAllFiles() + file.createNewFile() + + // When + testedOrchestrator.refreshFilesFromDisk() + + // Then + assertThat(testedOrchestrator.getAllFiles()).contains(file) + } + + @Test + fun `M remove deleted files W refreshFilesFromDisk()`() { + // Given + val file = testedOrchestrator.getWritableFile() + checkNotNull(file) + file.createNewFile() + assertThat(testedOrchestrator.getAllFiles()).contains(file) + file.delete() + + // When + testedOrchestrator.refreshFilesFromDisk() + + // Then + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(file) + } + + @Test + fun `M ignore non-batch files W refreshFilesFromDisk()`( + @StringForgery nonNumericName: String + ) { + // Given + val file = File(fakeRootDir, nonNumericName) + file.createNewFile() + + // When + testedOrchestrator.refreshFilesFromDisk() + + // Then + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(file) + } + + // endregion + + // region onFileDeleted + + @Test + fun `M remove file from knownFiles W onFileDeleted()`() { + // Given + val file = testedOrchestrator.getWritableFile() + checkNotNull(file) + assertThat(testedOrchestrator.getAllFiles()).contains(file) + + // When + testedOrchestrator.onFileDeleted(file) + + // Then + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(file) + } + + @Test + fun `M do nothing W onFileDeleted() { file not in knownFiles }`() { + // Given + val fileName = System.currentTimeMillis().toString() + val file = File(fakeRootDir, fileName) + testedOrchestrator.getAllFiles() + + // When + testedOrchestrator.onFileDeleted(file) + + // Then + assertThat(testedOrchestrator.getAllFiles()).isEmpty() + } + + // endregion + + // region File tracking (createNewFile adds to knownFiles) + + @Test + fun `M track file immediately W getWritableFile() creates new file`() { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + + // When + val file = testedOrchestrator.getWritableFile() + + // Then + checkNotNull(file) + assertThat(testedOrchestrator.getAllFiles()).contains(file) + } + + @Test + fun `M discover pre-existing files W first access`() { + // Given + val currentTime = stubTimeProvider.deviceTimestampMs + val preExistingFile = File(fakeRootDir, (currentTime - RECENT_DELAY_MS * 2).toString()) + preExistingFile.createNewFile() + + // When + val files = testedOrchestrator.getAllFiles() + + // Then + assertThat(files).contains(preExistingFile) + } + + // endregion + + // region getReadableFile stale entry cleanup + + @Test + fun `M skip and remove stale file W getReadableFile() { file deleted externally }`() { + // Given + val currentTime = stubTimeProvider.deviceTimestampMs + val oldTimestamp = currentTime - (RECENT_DELAY_MS * 2) + val file = File(fakeRootDir, oldTimestamp.toString()) + file.createNewFile() + testedOrchestrator.getAllFiles() + assertThat(testedOrchestrator.getAllFiles()).contains(file) + file.delete() + + // When + val readableFile = testedOrchestrator.getReadableFile(emptySet()) + + // Then + assertThat(readableFile).isNull() + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(file) + } + + @Test + fun `M return next valid file W getReadableFile() { first file deleted externally }`() { + // Given + val currentTime = stubTimeProvider.deviceTimestampMs + val oldTimestamp1 = currentTime - (RECENT_DELAY_MS * 3) + val oldTimestamp2 = currentTime - (RECENT_DELAY_MS * 2) + val deletedFile = File(fakeRootDir, oldTimestamp1.toString()) + val validFile = File(fakeRootDir, oldTimestamp2.toString()) + deletedFile.createNewFile() + validFile.createNewFile() + testedOrchestrator.getAllFiles() + deletedFile.delete() + + // When + val readableFile = testedOrchestrator.getReadableFile(emptySet()) + + // Then + assertThat(readableFile).isEqualTo(validFile) + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(deletedFile) + assertThat(testedOrchestrator.getAllFiles()).contains(validFile) + } + + // endregion + + // region Performance - cached knownFiles + + @Test + fun `M not detect externally added file W getAllFiles() without refreshFilesFromDisk`() { + // This test verifies we use cached knownFiles (performance optimization) + // If we were calling listFiles() every time, this test would fail + + // Given + testedOrchestrator.getAllFiles() + val externalFile = File(fakeRootDir, System.currentTimeMillis().toString()) + externalFile.createNewFile() + + // When + val files = testedOrchestrator.getAllFiles() + + // Then + assertThat(files).doesNotContain(externalFile) + } + + @Test + fun `M detect externally added file W getAllFiles() after refreshFilesFromDisk`() { + // Given + testedOrchestrator.getAllFiles() + val externalFile = File(fakeRootDir, System.currentTimeMillis().toString()) + externalFile.createNewFile() + + // When + testedOrchestrator.refreshFilesFromDisk() + val files = testedOrchestrator.getAllFiles() + + // Then + assertThat(files).contains(externalFile) + } + + // endregion + + // region freeSpaceIfNeeded updates knownFiles + + @Test + fun `M remove purged files from knownFiles W getWritableFile() { disk space exceeded }`( + @StringForgery(size = MAX_BATCH_SIZE) previousData: String + ) { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + val filesCount = MAX_DISK_SPACE / MAX_BATCH_SIZE + val files = (0..filesCount).map { + val file = testedOrchestrator.getWritableFile() + checkNotNull(file) + file.writeText(previousData) + stubTimeProvider.deviceTimestampMs += 1 + file + } + val oldestFile = files.first() + + // When + stubTimeProvider.deviceTimestampMs += CLEANUP_FREQUENCY_THRESHOLD_MS + 1 + testedOrchestrator.getWritableFile() + + // Then + assertThat(oldestFile).doesNotExist() + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(oldestFile) + } + + @Test + fun `M reset currentBatchState W getWritableFile() { current batch file purged }`( + @StringForgery(size = MAX_BATCH_SIZE) previousData: String + ) { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + val firstFile = testedOrchestrator.getWritableFile() + checkNotNull(firstFile) + firstFile.writeText(previousData) + + // Create more files to exceed disk space + val filesCount = MAX_DISK_SPACE / MAX_BATCH_SIZE + repeat(filesCount) { + stubTimeProvider.deviceTimestampMs += RECENT_DELAY_MS + 1 + val file = testedOrchestrator.getWritableFile() + checkNotNull(file) + file.writeText(previousData) + } + + // When + stubTimeProvider.deviceTimestampMs += CLEANUP_FREQUENCY_THRESHOLD_MS + 1 + val newFile = testedOrchestrator.getWritableFile() + + // Then + assertThat(firstFile).doesNotExist() + checkNotNull(newFile) + assertThat(newFile).isNotEqualTo(firstFile) + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(firstFile) + } + + // endregion + + // region getReusableWritableFile edge cases + + @Test + fun `M create new file W getWritableFile() { currentBatchState file deleted externally }`() { + // Given + assumeTrue(fakeRootDir.listFiles().isNullOrEmpty()) + val firstFile = testedOrchestrator.getWritableFile() + checkNotNull(firstFile) + firstFile.createNewFile() + firstFile.delete() + + // When + stubTimeProvider.deviceTimestampMs += 1 + val newFile = testedOrchestrator.getWritableFile() + + // Then + checkNotNull(newFile) + assertThat(newFile).isNotEqualTo(firstFile) + assertThat(testedOrchestrator.getAllFiles()).doesNotContain(firstFile) + } + + // endregion + companion object { private const val RECENT_DELAY_MS = 250L diff --git a/detekt_custom_safe_calls.yml b/detekt_custom_safe_calls.yml index c91b356cc5..130d5cee9b 100644 --- a/detekt_custom_safe_calls.yml +++ b/detekt_custom_safe_calls.yml @@ -980,6 +980,7 @@ datadog: - "kotlin.collections.MutableSet.indexOf(kotlin.String)" - "kotlin.collections.MutableSet.joinToString(kotlin.CharSequence, kotlin.CharSequence, kotlin.CharSequence, kotlin.Int, kotlin.CharSequence, kotlin.Function1?)" - "kotlin.collections.MutableSet.map(kotlin.Function1)" + - "kotlin.collections.MutableSet.maxOrNull()" - "kotlin.collections.MutableSet.remove(com.datadog.android.api.feature.FeatureContextUpdateReceiver)" - "kotlin.collections.MutableSet.remove(com.datadog.android.core.internal.persistence.ConsentAwareStorage.Batch)" - "kotlin.collections.MutableSet.remove(java.io.File)"