From 9d783b2301df31926b389bda5685570f711479b2 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:32:15 +0000 Subject: [PATCH] logging: address review of the SQL logging path Only downgrade a uniqueness conflict on an INSERT, so genuine duplicate-key failures still surface. Clear old zips from the dump directory before exporting, roll the log file over at midnight, prune on roll/day-change instead of on every line, and drop the unused files-path from the FileProvider config. --- app/src/main/kotlin/com/subtracks/log/Log.kt | 22 ++++++++++--------- .../kotlin/com/subtracks/log/LogFileStore.kt | 18 +++++++++++++-- app/src/main/res/xml/file_paths.xml | 1 - .../com/subtracks/log/LogFileStoreTest.kt | 19 ++++++++++++---- 4 files changed, 43 insertions(+), 17 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/log/Log.kt b/app/src/main/kotlin/com/subtracks/log/Log.kt index ebdcbb9f..38c14f83 100644 --- a/app/src/main/kotlin/com/subtracks/log/Log.kt +++ b/app/src/main/kotlin/com/subtracks/log/Log.kt @@ -108,7 +108,7 @@ object Log { fun sql(execution: SqlExecution) { val failure = execution.error if (failure != null) { - if (isUpsertConflict(failure)) { + if (isUpsertConflict(execution.sql, failure)) { d("sql", "upsert fallback conflict: ${shortSql(execution.sql)}") } else { e("sql", "failed: ${shortSql(execution.sql)}", failure) @@ -180,16 +180,18 @@ internal fun isSlowQuery( thresholdMs: Double, ): Boolean = execution.error == null && execution.durationMillis >= thresholdMs && isDataStatement(execution.sql) -// Room's EntityUpsertAdapter implements @Upsert as an INSERT and, on a uniqueness conflict, catches -// the error and runs an UPDATE instead (EntityUpsertAdapter.checkUniquenessException). That failure -// is expected control flow, not an error worth alarming about. +// Room's EntityUpsertAdapter implements @Upsert as an INSERT and treats a uniqueness conflict as the +// signal to UPDATE instead (EntityUpsertAdapter.checkUniquenessException). Only downgrade a failed +// INSERT whose error says the conflict was a unique/primary key; anything else stays an ERROR so a +// genuine failure isn't hidden in release. private const val CONSTRAINT_UNIQUE_MESSAGE = "unique constraint failed" -private const val SQLITE_CONSTRAINT_PRIMARYKEY = "1555" -private const val SQLITE_CONSTRAINT_UNIQUE = "2067" +private const val CONSTRAINT_PRIMARY_KEY_MESSAGE = "primary key constraint failed" -internal fun isUpsertConflict(error: Throwable): Boolean { +internal fun isUpsertConflict( + sql: String, + error: Throwable, +): Boolean { + if (!sql.trimStart().uppercase().startsWith("INSERT")) return false val message = error.message?.lowercase() ?: return false - return message.contains(CONSTRAINT_UNIQUE_MESSAGE) || - message.contains(SQLITE_CONSTRAINT_PRIMARYKEY) || - message.contains(SQLITE_CONSTRAINT_UNIQUE) + return message.contains(CONSTRAINT_UNIQUE_MESSAGE) || message.contains(CONSTRAINT_PRIMARY_KEY_MESSAGE) } diff --git a/app/src/main/kotlin/com/subtracks/log/LogFileStore.kt b/app/src/main/kotlin/com/subtracks/log/LogFileStore.kt index b7087671..b27d193b 100644 --- a/app/src/main/kotlin/com/subtracks/log/LogFileStore.kt +++ b/app/src/main/kotlin/com/subtracks/log/LogFileStore.kt @@ -25,6 +25,10 @@ class LogFileStore( private val lineFormat = SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS", Locale.US) private var current = currentFile() + init { + prune() + } + fun write( level: LogLevel, tag: String, @@ -66,6 +70,7 @@ class LogFileStore( val logs = files() if (logs.isEmpty()) return null destDir.mkdirs() + destDir.listFiles()?.forEach { if (it.isFile) it.delete() } val zip = File(destDir, "subtracks-logs-${System.currentTimeMillis()}.zip") ZipOutputStream(zip.outputStream().buffered()).use { out -> logs.forEach { log -> @@ -100,9 +105,18 @@ class LogFileStore( append('\n') } rootDir.mkdirs() - if (current.length() >= maxFileBytes) roll() + var rolled = false + val today = currentFile() + if (current.name != today.name) { + current = today + rolled = true + } + if (current.length() >= maxFileBytes) { + roll() + rolled = true + } current.appendText(redactLogs(text)) - prune() + if (rolled) prune() } private var rollSequence = 0 diff --git a/app/src/main/res/xml/file_paths.xml b/app/src/main/res/xml/file_paths.xml index 918fe209..16246562 100644 --- a/app/src/main/res/xml/file_paths.xml +++ b/app/src/main/res/xml/file_paths.xml @@ -1,5 +1,4 @@ - diff --git a/app/src/test/kotlin/com/subtracks/log/LogFileStoreTest.kt b/app/src/test/kotlin/com/subtracks/log/LogFileStoreTest.kt index a07722b1..0f70a53c 100644 --- a/app/src/test/kotlin/com/subtracks/log/LogFileStoreTest.kt +++ b/app/src/test/kotlin/com/subtracks/log/LogFileStoreTest.kt @@ -152,10 +152,21 @@ class LogFileStoreTest { @Test fun detectsUpsertConflictErrors() { - assertTrue(isUpsertConflict(IllegalStateException("UNIQUE constraint failed: playback_cursor.id"))) - assertTrue(isUpsertConflict(IllegalStateException("Error code: 1555, message: UNIQUE constraint failed"))) - assertTrue(isUpsertConflict(IllegalStateException("SQLITE_CONSTRAINT_UNIQUE 2067"))) - assertFalse(isUpsertConflict(IllegalStateException("no such table: songs"))) + assertTrue( + isUpsertConflict( + "INSERT INTO playback_cursor (id) VALUES (?)", + IllegalStateException("UNIQUE constraint failed: playback_cursor.id"), + ), + ) + assertTrue( + isUpsertConflict( + "INSERT OR ABORT INTO t VALUES (?)", + IllegalStateException("PRIMARY KEY constraint failed"), + ), + ) + assertFalse(isUpsertConflict("UPDATE t SET x = 1", IllegalStateException("UNIQUE constraint failed: t.x"))) + assertFalse(isUpsertConflict("SELECT * FROM t", IllegalStateException("no such table: t"))) + assertFalse(isUpsertConflict("INSERT INTO t VALUES (?)", IllegalStateException("Error code: 1555"))) } private fun execution(