diff --git a/android/app/src/androidTest/java/com/superproductivity/superproductivity/app/KeyValStoreInstrumentedTest.kt b/android/app/src/androidTest/java/com/superproductivity/superproductivity/app/KeyValStoreInstrumentedTest.kt new file mode 100644 index 0000000000..e09921a154 --- /dev/null +++ b/android/app/src/androidTest/java/com/superproductivity/superproductivity/app/KeyValStoreInstrumentedTest.kt @@ -0,0 +1,70 @@ +package com.superproductivity.superproductivity.app + +import androidx.test.InstrumentationRegistry +import androidx.test.runner.AndroidJUnit4 +import com.superproductivity.superproductivity.App +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Instrumented test for [KeyValStore]. + * + * MUST run on an emulator/device, not Robolectric: the ~2 MB CursorWindow limit + * that this is guarding against only manifests against real Android SQLite. + * + * Regression for the on-device backup data-loss bug: a backup blob larger than + * Android's ~2 MB CursorWindow could be written via [KeyValStore.set] but not + * read back via [KeyValStore.get] — `cursor.getString()` threw + * SQLiteBlobTooBigException ("Row too big to fit into CursorWindow"). That surfaced + * to the WebView as "Error invoking loadFromDb: Java exception …", so the + * eviction-recovery path (#7901) silently failed and the user lost everything. + * + * Run: ./gradlew :app:connectedPlayDebugAndroidTest (emulator/device required). + */ +@RunWith(AndroidJUnit4::class) +class KeyValStoreInstrumentedTest { + + private val store by lazy { + (InstrumentationRegistry.getTargetContext().applicationContext as App).keyValStore + } + + private val testKey = "instr_test_keyvalstore" + + @After + fun cleanUp() { + store.set(testKey, null) + } + + @Test + fun smallValueRoundTrips() { + val value = "hello / with 'apostrophes' and \"quotes\" and \n newlines" + store.set(testKey, value) + assertEquals(value, store.get(testKey, "DEFAULT")) + } + + @Test + fun missingKeyReturnsDefault() { + assertEquals("DEFAULT", store.get("instr_test_definitely_absent_key", "DEFAULT")) + } + + /** + * Core regression: a value well past the ~2 MB CursorWindow limit. + * Pre-fix this throws inside get(); post-fix the chunked read returns it whole. + */ + @Test + fun largeValueOverCursorWindowRoundTrips() { + val big = buildString { + // ~3 MB of varied content (not one repeated char) to mimic a real JSON backup. + val unit = "{\"id\":\"abc123def456\",\"title\":\"do the thing\",\"t\":600000}," + while (length < 3 * 1024 * 1024) append(unit) + } + store.set(testKey, big) + + val read = store.get(testKey, "DEFAULT") + + assertEquals(big.length, read.length) + assertEquals(big, read) + } +} diff --git a/android/app/src/main/java/com/superproductivity/superproductivity/app/KeyValStore.kt b/android/app/src/main/java/com/superproductivity/superproductivity/app/KeyValStore.kt index c7e8b12e43..9c1c7a851c 100644 --- a/android/app/src/main/java/com/superproductivity/superproductivity/app/KeyValStore.kt +++ b/android/app/src/main/java/com/superproductivity/superproductivity/app/KeyValStore.kt @@ -69,20 +69,48 @@ class KeyValStore(private val context: Context) : val newKey = DatabaseUtils.sqlEscapeString(key) Log.v(TAG, "getting db value: $newKey") val dbHelper = (context.applicationContext as App).keyValStore - var value = defaultValue - dbHelper.readableDatabase?.let { database -> - database.query( - DATABASE_TABLE, arrayOf(VALUE), "$KEY=?", arrayOf(newKey), null, null, null - )?.let { cursor -> - if (cursor.moveToNext()) { - value = cursor.getString(cursor.getColumnIndexOrThrow(VALUE)) + val database = dbHelper.readableDatabase ?: return defaultValue + return try { + // Read the value in <2 MB chunks via substr(). A single cursor.getString() + // on a row larger than Android's ~2 MB CursorWindow throws + // SQLiteBlobTooBigException ("Row too big to fit into CursorWindow"), which + // silently broke on-device backup restore once the backup blob grew past + // ~2 MB (a year of normal use). substr() is 1-indexed; GET_CHUNK_CHARS is a + // CHARACTER count kept well under the byte window even for 4-byte UTF-8. + val sb = StringBuilder() + var offset = 1 + var rowExists = false + while (true) { + database.rawQuery( + "SELECT substr($VALUE, ?, ?) FROM $DATABASE_TABLE WHERE $KEY=? LIMIT 1", + arrayOf(offset.toString(), GET_CHUNK_CHARS.toString(), newKey) + ).use { cursor -> + if (cursor.moveToFirst()) { + rowExists = true + val chunk = cursor.getString(0) + if (chunk == null) { + offset = DONE // VALUE column is NULL + } else { + sb.append(chunk) + // A short chunk means we read past the end → finished. + offset = if (chunk.length < GET_CHUNK_CHARS) DONE else offset + GET_CHUNK_CHARS + } + } else { + offset = DONE // no row for this key + } } - Log.v(TAG, "get db value size:" + value.length) - cursor.close() + if (offset == DONE) break } + Log.v(TAG, "get db value size:" + sb.length) + if (rowExists) sb.toString() else defaultValue + } catch (e: Exception) { + // Never let a read failure crash the JS bridge — degrade to default so + // callers can surface "no backup" instead of an opaque invocation error. + Log.e(TAG, "get failed for key $newKey", e) + defaultValue + } finally { database.close() } - return value } fun clearAll(context: Context) { @@ -103,6 +131,13 @@ class KeyValStore(private val context: Context) : private const val VALUE: String = "VALUE" private const val KEY_CREATED_AT: String = "KEY_CREATED_AT" private const val TAG: String = "SupKeyValStore" + + // Read large rows in chunks of this many CHARACTERS. Even at 4 bytes/char + // (256K * 4 = 1 MB) this stays well under the ~2 MB CursorWindow limit. + private const val GET_CHUNK_CHARS: Int = 256 * 1024 + + // Sentinel offset signalling the chunked read loop is finished. + private const val DONE: Int = -1 // IF NOT EXISTS so the additive onUpgrade() (which calls onCreate) is safe // to run against an already-populated DB without throwing (#7901). private const val CREATE_TABLE = diff --git a/src/app/imex/local-backup/local-backup.service.ts b/src/app/imex/local-backup/local-backup.service.ts index 7839ca30cb..05dead7806 100644 --- a/src/app/imex/local-backup/local-backup.service.ts +++ b/src/app/imex/local-backup/local-backup.service.ts @@ -46,6 +46,11 @@ const IOS_BACKUP_PREV_FILENAME = 'super-productivity-backup.prev.json'; const NEAR_EMPTY_NEW_TASKS = 3; const SUBSTANTIAL_EXISTING_TASKS = 10; +// Warn when the Android backup blob nears Android's ~2 MB SQLite CursorWindow +// row limit. A row past that limit could be written but not read back, silently +// breaking eviction recovery (now fixed via a chunked native read in KeyValStore). +const ANDROID_BACKUP_SIZE_WARN_CHARS = 1.8 * 1024 * 1024; + @Injectable({ providedIn: 'root', }) @@ -86,13 +91,12 @@ export class LocalBackupService { checkBackupAvailable(): Promise { if (this._isAndroidWebView) { // Available if either ring slot holds a backup (#7901). - return androidInterface.loadFromDbWrapped(ANDROID_DB_KEY).then(async (primary) => { - if (primary) { + return (async () => { + if (await this._loadAndroidDbValueSafe(ANDROID_DB_KEY)) { return true; } - const prev = await androidInterface.loadFromDbWrapped(ANDROID_DB_KEY_PREV); - return !!prev; - }); + return !!(await this._loadAndroidDbValueSafe(ANDROID_DB_KEY_PREV)); + })(); } if (this._platformService.isIOS()) { return this._checkBackupAvailableIOS(); @@ -112,12 +116,35 @@ export class LocalBackupService { // usable exists (degrades to the existing import-error snack rather than // throwing on the startup path). const [primary, prev] = await Promise.all([ - androidInterface.loadFromDbWrapped(ANDROID_DB_KEY), - androidInterface.loadFromDbWrapped(ANDROID_DB_KEY_PREV), + this._loadAndroidDbValueSafe(ANDROID_DB_KEY), + this._loadAndroidDbValueSafe(ANDROID_DB_KEY_PREV), ]); return selectBestBackupStr(primary, prev) ?? ''; } + /** + * Android-only native-KV read with diagnostics + graceful failure. + * + * - Logs the blob SIZE (never content — see core/log rule 9) so a shared log + * export shows whether a backup has grown into the ~2 MB CursorWindow danger + * zone that used to make `loadFromDb` throw. + * - Returns null instead of throwing, so a read failure degrades to "no backup" + * (the existing snack) rather than an opaque "Error invoking loadFromDb" that + * aborts the whole restore action. + */ + private async _loadAndroidDbValueSafe(key: string): Promise { + try { + const val = await androidInterface.loadFromDbWrapped(key); + Log.log( + `LocalBackupService: read Android backup '${key}' (${val ? val.length : 0} chars)`, + ); + return val; + } catch (e) { + Log.err(`LocalBackupService: failed to read Android backup '${key}'`, e); + return null; + } + } + async loadBackupIOS(): Promise { const [primary, prev] = await Promise.all([ this._readIOSFileOrNull(IOS_BACKUP_FILENAME), @@ -379,14 +406,20 @@ export class LocalBackupService { // Returns true when a backup was actually written, false when the A3 guard // skipped it (so the caller knows whether to advance the last-backup time). private async _backupAndroid(data: AppDataComplete): Promise { - const existing = await androidInterface.loadFromDbWrapped(ANDROID_DB_KEY); + const existing = await this._loadAndroidDbValueSafe(ANDROID_DB_KEY); if (this._guardNearEmptyOverwrite(data, existing, 'Android')) { return false; } if (existing) { await androidInterface.saveToDbWrapped(ANDROID_DB_KEY_PREV, existing); } - await androidInterface.saveToDbWrapped(ANDROID_DB_KEY, JSON.stringify(data)); + const json = JSON.stringify(data); + // Size only — never content (core/log rule 9). Surfaces in shared log exports + // so we can see a backup approaching the ~2 MB native-read danger zone. + if (json.length > ANDROID_BACKUP_SIZE_WARN_CHARS) { + Log.warn(`LocalBackupService: Android backup is large (${json.length} chars)`); + } + await androidInterface.saveToDbWrapped(ANDROID_DB_KEY, json); return true; }