Fix SQLiteDiskIOException crash from disk storage exhaustion#15347
Fix SQLiteDiskIOException crash from disk storage exhaustion#15347JorgeMucientes wants to merge 3 commits intotrunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical crash (SQLiteDiskIOException: SQLITE_IOERR_SHMSIZE) caused by device storage exhaustion. The fix addresses unbounded log file growth that could consume all available disk space, preventing SQLite from creating necessary shared memory files. The solution adds defensive measures at both the log writing and database initialization layers.
Changes:
- Implements size cap (2 MB per file) and disk space checks (50 MB minimum) in
LogFileWriterto prevent unbounded log growth - Adds recovery logic in
WCDatabaseModuleto catch disk I/O exceptions, clear log files, and retry database initialization - Adds unit tests for the new size cap and disk space checking behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| libs/commons/src/main/java/com/woocommerce/android/util/logs/LogFileWriter.kt | Adds per-file size cap, disk space checking, and proactive log cleanup when storage is low |
| libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/di/WCDatabaseModule.kt | Wraps database initialization with exception handling to recover from disk I/O errors by clearing logs |
| libs/commons/src/test/kotlin/com/woocommerce/android/util/logs/LogFileWriterTest.kt | Adds tests for size cap enforcement and low disk space handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Log.e( | ||
| "WCDatabaseModule", | ||
| "Database open failed due to disk I/O error, freeing space and retrying", | ||
| e | ||
| ) |
There was a problem hiding this comment.
The codebase convention in fluxc-plugin is to use AppLog for logging instead of android.util.Log. For example, see WCProductModel.kt:16-17, WooNetwork.kt:149, and WCCurrencyUtils.kt:67. Consider using AppLog.e() instead of Log.e() for consistency with the rest of the codebase.
|
|
||
| private fun clearLogFiles(context: Context) { | ||
| val logsDir = File(context.filesDir, "logs") | ||
| logsDir.listFiles()?.forEach { it.delete() } |
There was a problem hiding this comment.
The clearLogFiles function deletes all files in the logs directory without checking if they are actually log files. If other files happen to be in this directory, they would also be deleted. Consider adding a filter to only delete files that match the log file pattern (e.g., files starting with "log_" prefix), similar to how it's done in LogFileWriter's rotateLogFilesIfNeeded method.
| logsDir.listFiles()?.forEach { it.delete() } | |
| logsDir.listFiles() | |
| ?.filter { it.isFile && it.name.startsWith("log_") } | |
| ?.forEach { it.delete() } |
| const val DATE_FORMAT_PATTERN = "yyyy-MM-dd" | ||
|
|
||
| private const val MAX_LOG_FILE_SIZE_BYTES = 2L * 1024 * 1024 // 2 MB per file | ||
| private const val MIN_DISK_SPACE_BYTES = 50L * 1024 * 1024 // 50 MB |
There was a problem hiding this comment.
The 50 MB minimum disk space threshold is quite high and might trigger cleanup unnecessarily on devices with limited storage. Users with 100-200 MB of free space would constantly have their logs cleared, even though there's still adequate space for normal app operation. Consider lowering this threshold (e.g., 10-20 MB) or making it configurable. Additionally, this check happens on every log write, which could impact performance - consider caching the result for a short period.
| if (!hasEnoughDiskSpace()) { | ||
| deleteOldestLogFiles() | ||
| return | ||
| } |
There was a problem hiding this comment.
The disk space check (hasEnoughDiskSpace) is called on every single log write operation. StatFs operations can be relatively expensive as they involve system calls. For high-frequency logging scenarios, this could impact performance. Consider caching the disk space check result for a short duration (e.g., 5-10 seconds) to reduce overhead while still catching low disk space conditions reasonably quickly.
|
|
||
| private fun clearLogFiles(context: Context) { | ||
| val logsDir = File(context.filesDir, "logs") | ||
| logsDir.listFiles()?.forEach { it.delete() } |
There was a problem hiding this comment.
The clearLogFiles function should handle potential SecurityException or other exceptions that could occur during file deletion. While unlikely, file deletion can fail due to permission issues or files being locked. Consider wrapping the deletion in a try-catch block and logging any failures.
| logsDir.listFiles()?.forEach { it.delete() } | |
| logsDir.listFiles()?.forEach { file -> | |
| try { | |
| if (!file.delete()) { | |
| Log.w( | |
| "WCDatabaseModule", | |
| "Failed to delete log file: ${file.absolutePath}" | |
| ) | |
| } | |
| } catch (e: SecurityException) { | |
| Log.w( | |
| "WCDatabaseModule", | |
| "SecurityException while deleting log file: ${file.absolutePath}", | |
| e | |
| ) | |
| } catch (e: Exception) { | |
| Log.w( | |
| "WCDatabaseModule", | |
| "Unexpected exception while deleting log file: ${file.absolutePath}", | |
| e | |
| ) | |
| } | |
| } |
| val exceedsMaxSize = withContext(dispatchers.io) { | ||
| logFile.length() >= MAX_LOG_FILE_SIZE_BYTES | ||
| } | ||
|
|
||
| if (exceedsMaxSize) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Race condition: The file size check occurs outside the mutex, but the write happens inside it. This means multiple threads could check the size concurrently when the file is at 1.9 MB, both see it's below the limit, and both proceed to write, causing the file to exceed the 2 MB cap. The size check should be moved inside the mutex block (lines 48-52) to ensure atomicity between the size check and the write operation.
| private suspend fun deleteOldestLogFiles() { | ||
| withContext(dispatchers.io) { | ||
| mutex.withLock { | ||
| val logFiles = logsDirectory | ||
| .listFiles { file -> file.isFile && file.name.startsWith(LOG_FILE_NAME_PREFIX) } | ||
| ?.sortedByDescending { it.lastModified() } | ||
| ?: return@withLock | ||
|
|
||
| logFiles.drop(1).forEach { it.delete() } |
There was a problem hiding this comment.
When disk space is low, this function deletes all log files except the most recent one. While this frees maximum space, it's very aggressive and results in losing potentially useful debugging information. Consider a more gradual approach: delete only the oldest file(s) until enough space is freed, or set a minimum number of files to preserve (e.g., keep at least 2-3 recent log files). This would balance freeing space with retaining useful logs for debugging.
| } catch (e: SQLiteDiskIOException) { | ||
| Log.e( | ||
| "WCDatabaseModule", | ||
| "Database open failed due to disk I/O error, freeing space and retrying", | ||
| e | ||
| ) | ||
| clearLogFiles(context) | ||
| WCAndroidDatabase.buildDb( | ||
| context, | ||
| currencyPositionConverter, | ||
| statsGranularityConverter, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The exception handler catches SQLiteDiskIOException, but SQLite can throw this exception for various disk I/O errors, not just storage exhaustion (e.g., permission errors, filesystem corruption). Deleting log files and retrying may not help in those cases. Consider checking available disk space before clearing logs, or catching a more specific exception if possible. Also, the retry could fail for the same or a different reason, and that exception would propagate uncaught, potentially causing confusion about what actually failed.
| private val availableDiskBytes: () -> Long = { | ||
| runCatching { StatFs(logsDirectory.absolutePath).availableBytes }.getOrDefault(Long.MAX_VALUE) | ||
| } |
There was a problem hiding this comment.
The production code in WooFileLogger instantiates LogFileWriter without passing the availableDiskBytes parameter (see WooFileLogger.kt:50-54). While this works because there's a default value, it means the disk space check will use the default StatFs implementation. Since this is the intended behavior for production, this is fine. However, consider documenting this in the class documentation to clarify that the parameter is primarily for testing.
| private val dispatchers: CoroutineDispatchers | ||
| private val dispatchers: CoroutineDispatchers, | ||
| private val availableDiskBytes: () -> Long = { | ||
| runCatching { StatFs(logsDirectory.absolutePath).availableBytes }.getOrDefault(Long.MAX_VALUE) |
There was a problem hiding this comment.
The StatFs constructor can throw IllegalArgumentException if the path doesn't exist. While runCatching handles this, the getOrDefault(Long.MAX_VALUE) fallback could mask real errors. If the logs directory doesn't exist, returning Long.MAX_VALUE makes the code think there's infinite disk space, which could be misleading. Consider creating the directory first or handling the case more explicitly.
| runCatching { StatFs(logsDirectory.absolutePath).availableBytes }.getOrDefault(Long.MAX_VALUE) | |
| if (!logsDirectory.exists()) { | |
| logsDirectory.mkdirs() | |
| } | |
| runCatching { StatFs(logsDirectory.absolutePath).availableBytes }.getOrElse { 0L } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #15347 +/- ##
=========================================
Coverage 38.89% 38.90%
- Complexity 10829 10835 +6
=========================================
Files 2227 2227
Lines 127224 127246 +22
Branches 17684 17692 +8
=========================================
+ Hits 49488 49508 +20
+ Misses 72690 72688 -2
- Partials 5046 5050 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JorgeMucientes
force-pushed
the
issue/woomob-2170-fix-sqlite-disk-io-crash
branch
from
cefeaf8 to
8636328
Compare
Generated by 🚫 Danger |
3 participants
Closes WOOMOB-2170
Summary
Fixes the
SQLiteDiskIOException: disk I/O error (code 4874 SQLITE_IOERR_SHMSIZE)crash that occurs when the device runs out of storage. The crash chain is:LogFileWriterhad no per-file size limit. On days with cascading errors, a single log file could grow indefinitely.free_storage: 0.PRAGMA journal_modeto configure WAL mode, which requires a.db-shmfile. With no storage left, this fails withSQLITE_IOERR_SHMSIZEand the app crashes on launch.Changes
Three complementary fixes across two modules:
1. Cap log file size and check disk space (
LogFileWriter.kt)availableDiskBytesis injected as a lambda to allow testing without real filesystem checks.2. Graceful database recovery (
WCDatabaseModule.kt)WCAndroidDatabase.buildDb()in a try-catch forSQLiteDiskIOException.log_prefix) undercontext.filesDir/logsto free space, then retries the database open exactly once.AppLogfor error logging, consistent with the rest of the fluxc-plugin module.3. Unit tests (
LogFileWriterTest.kt)Testing instructions
files/logs/directory via Device File Explorer in Android Studio).files/logs/are cleaned up (only today's file should remain).