From de539259a588ea26a953eea2459bcbf4f49c8670 Mon Sep 17 00:00:00 2001 From: breeze Date: Tue, 2 Jun 2026 12:41:27 -0500 Subject: [PATCH] feat: Improve robustness of debug log management --- .../java/maestro/debuglog/DebugLogStore.kt | 56 ++++++++++++++----- .../src/main/java/maestro/utils/FileUtils.kt | 9 ++- .../maestro/debuglog/DebugLogStoreTest.kt | 53 ++++++++++++++++++ .../test/java/maestro/utils/FileUtilsTest.kt | 41 ++++++++++++++ 4 files changed, 145 insertions(+), 14 deletions(-) create mode 100644 maestro-client/src/test/java/maestro/debuglog/DebugLogStoreTest.kt create mode 100644 maestro-client/src/test/java/maestro/utils/FileUtilsTest.kt diff --git a/maestro-client/src/main/java/maestro/debuglog/DebugLogStore.kt b/maestro-client/src/main/java/maestro/debuglog/DebugLogStore.kt index 370dce5a0e..bcb2cd3ef2 100644 --- a/maestro-client/src/main/java/maestro/debuglog/DebugLogStore.kt +++ b/maestro-client/src/main/java/maestro/debuglog/DebugLogStore.kt @@ -3,6 +3,7 @@ package maestro.debuglog import maestro.Driver import maestro.utils.FileUtils import net.harawata.appdirs.AppDirsFactory +import org.slf4j.LoggerFactory import java.io.File import java.time.LocalDateTime import java.time.format.DateTimeFormatter @@ -21,6 +22,12 @@ object DebugLogStore { private const val APP_AUTHOR = "mobile_dev" private const val LOG_DIR_DATE_FORMAT = "yyyy-MM-dd_HHmmss" private const val KEEP_LOG_COUNT = 6 + + // A run never spans this long, so any working directory older than this is an orphan from a + // crashed run and is safe to reap without touching a concurrently-running process's live dir. + private const val ORPHAN_DIR_MAX_AGE_MILLIS = 24L * 60 * 60 * 1000 + + private val LOGGER = LoggerFactory.getLogger(DebugLogStore::class.java) val logDirectory = File(AppDirsFactory.getInstance().getUserLogDir(APP_NAME, null, APP_AUTHOR)) private val currentRunLogDirectory: File @@ -88,9 +95,15 @@ object DebugLogStore { fun finalizeRun() { fileHandler.close() - val output = File(currentRunLogDirectory.parent, "${currentRunLogDirectory.name}.zip") - FileUtils.zipDir(currentRunLogDirectory.toPath(), output.toPath()) - currentRunLogDirectory.deleteRecursively() + // Archiving debug logs is best-effort cleanup; it must never crash a run that has already + // completed (e.g. if the log directory was reaped by a concurrent process). See issue #1522. + try { + val output = File(currentRunLogDirectory.parent, "${currentRunLogDirectory.name}.zip") + FileUtils.zipDir(currentRunLogDirectory.toPath(), output.toPath()) + currentRunLogDirectory.deleteRecursively() + } catch (e: Exception) { + LOGGER.warn("Failed to archive debug logs for this run; continuing", e) + } } private fun logFile(named: String): File { @@ -98,16 +111,11 @@ object DebugLogStore { } private fun removeOldLogs(baseDir: File) { - if (!baseDir.isDirectory) { - return - } - - val existing = baseDir.listFiles() ?: return - val toDelete = existing.sortedByDescending { it.name } - .drop(KEEP_LOG_COUNT) - .toList() - - toDelete.forEach { it.deleteRecursively() } + pruneLogs( + baseDir = baseDir, + keepZipCount = KEEP_LOG_COUNT, + orphanDirCutoffMillis = System.currentTimeMillis() - ORPHAN_DIR_MAX_AGE_MILLIS, + ) } fun logSystemInfo() { @@ -134,6 +142,28 @@ object DebugLogStore { } } +/** + * Prunes the debug log directory without ever deleting a directory that may still be in use by a + * concurrently-running Maestro process. + * + * - Completed runs are stored as `.zip` archives; only the newest [keepZipCount] are retained. + * - Working directories are owned and deleted by their own run on completion. Any directory left + * behind is an orphan from a crashed run and is reaped only when older than [orphanDirCutoffMillis], + * so a live run's freshly-modified directory is never removed. + */ +internal fun pruneLogs(baseDir: File, keepZipCount: Int, orphanDirCutoffMillis: Long) { + if (!baseDir.isDirectory) return + val entries = baseDir.listFiles() ?: return + + entries.filter { it.isFile && it.extension == "zip" } + .sortedByDescending { it.name } + .drop(keepZipCount) + .forEach { it.delete() } + + entries.filter { it.isDirectory && it.lastModified() < orphanDirCutoffMillis } + .forEach { it.deleteRecursively() } +} + fun Logger.warn(message: String, throwable: Throwable? = null) { if (throwable != null) { log(Level.WARNING, message, throwable) diff --git a/maestro-client/src/main/java/maestro/utils/FileUtils.kt b/maestro-client/src/main/java/maestro/utils/FileUtils.kt index 37ad630c6b..0c02553d3a 100644 --- a/maestro-client/src/main/java/maestro/utils/FileUtils.kt +++ b/maestro-client/src/main/java/maestro/utils/FileUtils.kt @@ -12,9 +12,12 @@ import kotlin.io.path.createDirectories import kotlin.io.path.exists import kotlin.io.path.isDirectory import kotlin.streams.toList +import org.slf4j.LoggerFactory object FileUtils { + private val LOGGER = LoggerFactory.getLogger(FileUtils::class.java) + /** * Zips directory * @@ -22,6 +25,10 @@ object FileUtils { * @param to output zip file */ fun zipDir(from: Path, to: Path) { + if (!from.exists()) { + LOGGER.warn("Skipping zip: source directory does not exist: {}", from) + return + } val stream = to.toFile().outputStream() val files = Files.walk(from).filter { !it.isDirectory() }.toList() ZipOutputStream(stream).use { zs -> @@ -34,7 +41,7 @@ object FileUtils { zs.closeEntry() } } catch (e: IOException) { - e.printStackTrace() + LOGGER.warn("Failed while zipping {} -> {}", from, to, e) } } } diff --git a/maestro-client/src/test/java/maestro/debuglog/DebugLogStoreTest.kt b/maestro-client/src/test/java/maestro/debuglog/DebugLogStoreTest.kt new file mode 100644 index 0000000000..cb43a30f9f --- /dev/null +++ b/maestro-client/src/test/java/maestro/debuglog/DebugLogStoreTest.kt @@ -0,0 +1,53 @@ +package maestro.debuglog + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File +import java.nio.file.Path + +internal class DebugLogStoreTest { + + @Test + internal fun `pruneLogs keeps the newest zip archives and deletes older ones`(@TempDir tempDir: Path) { + // Given more finalized run archives than the keep count + val baseDir = tempDir.toFile() + val zips = (1..8).map { File(baseDir, "2026-06-0${it}_100000_111.zip").apply { writeText("z") } } + + // When pruning, keeping the newest 6 + pruneLogs(baseDir, keepZipCount = 6, orphanDirCutoffMillis = 0) + + // Then only the 2 oldest archives are removed + assertThat(zips[0].exists()).isFalse() + assertThat(zips[1].exists()).isFalse() + zips.drop(2).forEach { assertThat(it.exists()).isTrue() } + } + + @Test + internal fun `pruneLogs deletes orphaned working dirs older than cutoff but keeps recent ones`(@TempDir tempDir: Path) { + // Given a fresh (live) working dir and an old orphaned one + val baseDir = tempDir.toFile() + val freshDir = File(baseDir, "2026-06-02_120000_222").apply { mkdirs(); File(this, "maestro.log").writeText("x") } + val orphanDir = File(baseDir, "2026-05-01_120000_333").apply { mkdirs(); File(this, "maestro.log").writeText("x") } + val cutoff = 1_000_000L + orphanDir.setLastModified(cutoff - 1) + freshDir.setLastModified(cutoff + 1) + + // When pruning with that cutoff + pruneLogs(baseDir, keepZipCount = 6, orphanDirCutoffMillis = cutoff) + + // Then the orphan is reaped and the live dir is left untouched + assertThat(orphanDir.exists()).isFalse() + assertThat(freshDir.exists()).isTrue() + } + + @Test + internal fun `pruneLogs does nothing when base dir does not exist`(@TempDir tempDir: Path) { + val missing = tempDir.resolve("nope").toFile() + + // Should not throw + pruneLogs(missing, keepZipCount = 6, orphanDirCutoffMillis = 0) + + assertThat(missing.exists()).isFalse() + } +} diff --git a/maestro-client/src/test/java/maestro/utils/FileUtilsTest.kt b/maestro-client/src/test/java/maestro/utils/FileUtilsTest.kt new file mode 100644 index 0000000000..8a4def961d --- /dev/null +++ b/maestro-client/src/test/java/maestro/utils/FileUtilsTest.kt @@ -0,0 +1,41 @@ +package maestro.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File +import java.nio.file.Path + +internal class FileUtilsTest { + + @Test + internal fun `zipDir does not throw or create output when source directory is missing`(@TempDir tempDir: Path) { + // Given a source directory that does not exist + val missingSource = tempDir.resolve("does-not-exist") + val output = tempDir.resolve("out.zip") + + // When zipping it + FileUtils.zipDir(missingSource, output) + + // Then it neither throws nor leaves an output file behind + assertThat(output.toFile().exists()).isFalse() + } + + @Test + internal fun `zipDir zips files in source directory`(@TempDir tempDir: Path) { + // Given a populated source directory + val source = tempDir.resolve("logs").toFile().apply { mkdirs() } + File(source, "maestro.log").writeText("hello") + val output = tempDir.resolve("out.zip") + + // When zipping it + FileUtils.zipDir(source.toPath(), output) + + // Then the output zip exists and contains the file + val outFile = output.toFile() + assertThat(outFile.exists()).isTrue() + val unzipTarget = tempDir.resolve("unzipped") + FileUtils.unzip(output, unzipTarget) + assertThat(unzipTarget.resolve("maestro.log").toFile().readText()).isEqualTo("hello") + } +}