Skip unchanged setWifiOnly writes and put the cause in E_STORAGE

CHANGELOG.md

@@ -116,6 +116,23 @@ pending until the returned function is called, so a test can model a native
 that is slow to answer. Later enqueues wait behind it, as they do on a
 device.
 
+`setWifiOnly()` with the value the queue already has no longer writes the
+settings file, on both platforms. It still resolves. Before, an app that
+calls it at every start wrote the file each time, so on a nearly full disk
+the call rejected `E_STORAGE` for no reason. `configure()` with the same
+options also no longer rewrites the settings file. Neither do `pause()` and
+`resume()` on Android when they change nothing; iOS already skipped those.
+On iOS, a failed write now deletes its partly written `.tmp` file, as
+Android already did.
+
+Each `E_STORAGE` rejection now ends with the underlying error, after the
+same message text as before: ` (cause: …)`. On iOS it holds the error domain
+and code, the POSIX code when there is one, and the description when the
+message does not already hold it. On Android it holds the exception class
+and the errno name when the cause chain has one. A failed rename or
+directory create on Android now also says why, for example `Permission
+denied`. The code stays `E_STORAGE`.
+
 ## 11.1.0
 
 Added `takeDiagnostics()`. On Android it returns the module's recent

README.md

@@ -273,7 +273,8 @@ while the app was dead from being sent twice.
    `E_RUNNING`, `E_FILE_MISSING`, or `E_STORAGE`. `E_INVALID` is malformed
    input native cannot send: a non-http(s) URL, header names or values the
    platform HTTP client rejects, a GET with a body, or parts that do not
-   tile the moved file.
+   tile the moved file. An `E_STORAGE` message ends with the underlying
+   error, ` (cause: …)`, for logs. Match on `code`, not on the message.
 2. **Journal before emit, ack after the handler.** Every terminal outcome is
    journaled natively, then delivered. The library acknowledges after the
    handler's promise resolves. A rejection, or app death before the ack,
@@ -454,7 +455,8 @@ again.
 The queue's Wi-Fi setting. Persisted natively. Applies to queued and future
 entries whose descriptor does not set `wifiOnly`. An entry that sets it keeps
 its own value. Both are checked before each attempt, so a toggle moves the
-queued entries that follow the setting.
+queued entries that follow the setting. A call with the current value
+resolves without a write.
 
 ### `updateHeaders(patch): Promise<void>`
 Merges the patch into the headers of every entry not yet forgotten and

android/src/main/java/ai/openspace/backgroundupload/AtomicFiles.kt

@@ -4,6 +4,8 @@ import java.io.File
 import java.io.FileOutputStream
 import java.io.IOException
 import java.nio.channels.FileChannel
+import java.nio.file.Files
+import java.nio.file.StandardCopyOption
 import java.nio.file.StandardOpenOption
 
 /**
@@ -17,20 +19,26 @@ internal object AtomicFiles {
 
   fun tmpFor(target: File) = File(target.parentFile, target.name + TMP_SUFFIX)
 
-  /** Throws IOException when the target could not be replaced. The old target is then intact. */
+  /**
+   * Throws IOException when the target could not be replaced. The old target is then intact.
+   *
+   * The directory create and the rename use java.nio, not File.mkdirs and
+   * File.renameTo, which return only a Boolean: a nio failure says why, for
+   * example "Permission denied". Not android.system.Os, which would also
+   * give the errno: it is a stub in the JVM unit tests.
+   */
   fun writeAtomically(target: File, write: (FileOutputStream) -> Unit) {
     val parent = target.parentFile ?: throw IOException("no parent directory for ${target.path}")
-    if (!parent.isDirectory && !parent.mkdirs()) {
-      throw IOException("could not create ${parent.path}")
-    }
+    if (!parent.isDirectory) Files.createDirectories(parent.toPath())
     val tmp = tmpFor(target)
     try {
       FileOutputStream(tmp).use { out ->
         write(out)
         out.flush()
         out.fd.sync()
       }
-      if (!tmp.renameTo(target)) throw IOException("could not rename ${tmp.path} to ${target.name}")
+      // ATOMIC_MOVE is rename(2): atomic, and it replaces the target.
+      Files.move(tmp.toPath(), target.toPath(), StandardCopyOption.ATOMIC_MOVE)
     } catch (error: Throwable) {
       tmp.delete()
       throw error

android/src/main/java/ai/openspace/backgroundupload/QueueController.kt

@@ -144,7 +144,11 @@ class QueueController(
     } catch (e: QueueException) {
       throw e
     } catch (e: IOException) {
-      throw QueueException(QueueException.E_STORAGE, "could not stage the body: ${e.message}")
+      throw QueueException(
+        QueueException.E_STORAGE,
+        "could not stage the body: ${e.message}" + StorageCause.suffix(e, withMessage = false),
+        e,
+      )
     }
 
   /** Saves a newly staged entry. On failure the new body file is removed; the old entry stays as it was. */
@@ -153,7 +157,11 @@ class QueueController(
       store.save(next)
     } catch (e: IOException) {
       if (next.body?.kind != StagedBody.CHUNKED) store.bodyFile(next)?.delete()
-      throw QueueException(QueueException.E_STORAGE, "could not save the entry: ${e.message}")
+      throw QueueException(
+        QueueException.E_STORAGE,
+        "could not save the entry: ${e.message}" + StorageCause.suffix(e, withMessage = false),
+        e,
+      )
     }
     store.pruneUnreferenced(next)
     return Enqueued(next, null)
@@ -163,7 +171,11 @@ class QueueController(
     try {
       store.save(next)
     } catch (e: IOException) {
-      throw QueueException(QueueException.E_STORAGE, "could not save the entry: ${e.message}")
+      throw QueueException(
+        QueueException.E_STORAGE,
+        "could not save the entry: ${e.message}" + StorageCause.suffix(e, withMessage = false),
+        e,
+      )
     }
   }
 
@@ -251,7 +263,11 @@ class QueueController(
         journaled = try {
           journal.append(record) { store.referencedEventIds() + record.eventId }
         } catch (error: IOException) {
-          throw QueueException(QueueException.E_STORAGE, "could not journal the cancel: ${error.message}")
+          throw QueueException(
+            QueueException.E_STORAGE,
+            "could not journal the cancel: ${error.message}" + StorageCause.suffix(error, withMessage = false),
+            error,
+          )
         }
         stop = true
         // The journaled record is the cancel. A failed save does not undo

android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt

@@ -90,10 +90,16 @@ class QueueSettingsStore(private val file: File) {
     return validated(read).also { cached = it }
   }
 
-  /** Throws IOException when the write fails. The cache then keeps the old value. */
+  /**
+   * Throws IOException when the write fails. The cache then keeps the old
+   * value. A [transform] that changes nothing writes nothing, so a call such
+   * as setWifiOnly() at every app start does not fail on a full disk.
+   */
   @Synchronized
   fun update(transform: (QueueSettings) -> QueueSettings): QueueSettings {
-    val next = transform(load())
+    val current = load()
+    val next = transform(current)
+    if (next == current) return current
     AtomicFiles.writeText(file, gson.toJson(next))
     cached = next
     return next

android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt

@@ -456,8 +456,8 @@ data class LegacyManifest(
   }
 }
 
-/** A rejection that reaches JS as `promise.reject(code, message)`. */
-class QueueException(val code: String, message: String) : IOException(message) {
+/** A rejection that reaches JS as `promise.reject(code, message)`. [cause] is the underlying error, if any. */
+class QueueException(val code: String, message: String, cause: Throwable? = null) : IOException(message, cause) {
   companion object {
     const val E_RUNNING = "E_RUNNING"
     const val E_FILE_MISSING = "E_FILE_MISSING"

android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt

@@ -0,0 +1,40 @@
+package ai.openspace.backgroundupload
+
+import android.system.ErrnoException
+import android.system.OsConstants
+
+/**
+ * The underlying error of an E_STORAGE rejection, as a suffix for its
+ * message: ` (cause: <class>: <message>; errno <NAME>)`. The errno part
+ * comes from an [ErrnoException] in the cause chain and is left out when
+ * there is none.
+ *
+ * A later option: also pass the same fields as a structured `userInfo.cause`
+ * on the rejection. Not done: it is not checked that RN brings `userInfo`
+ * to JS from a TurboModule promise.
+ */
+internal object StorageCause {
+  /**
+   * The errno name of one throwable of a cause chain, or null. Tests replace
+   * it: they run on android.jar stubs, where [ErrnoException.errno] reads 0
+   * and [OsConstants.errnoName] throws.
+   */
+  internal var errnoNameOf: (Throwable) -> String? = { e ->
+    (e as? ErrnoException)?.errno?.let { runCatching { OsConstants.errnoName(it) }.getOrNull() ?: "$it" }
+  }
+
+  /** [withMessage] false: the rejection message already holds [error]'s message. */
+  fun suffix(error: Throwable, withMessage: Boolean = true): String {
+    val name = error.javaClass.name
+    val head = if (withMessage && error.message != null) "$name: ${error.message}" else name
+    val errno = errnoName(error)?.let { "; errno $it" } ?: ""
+    return " (cause: $head$errno)"
+  }
+
+  /** The message of a rejection that is not a [QueueException]: its message, then its cause. */
+  fun rejectMessage(error: Throwable): String =
+    (error.message ?: error.javaClass.simpleName) + suffix(error, withMessage = false)
+
+  private fun errnoName(error: Throwable): String? =
+    generateSequence(error) { it.cause }.take(8).firstNotNullOfOrNull { errnoNameOf(it) }
+}

android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt

@@ -133,7 +133,9 @@ class UploaderModule(context: ReactApplicationContext) :
         promise.reject(QueueException.E_INVALID, e.message, e)
       } catch (e: Throwable) {
         Diag.error("queue operation failed", e)
-        promise.reject(QueueException.E_STORAGE, e.message ?: e.javaClass.simpleName, e)
+        // Not only storage failures land here (a WorkManager error, a bug),
+        // so the message names the throwable's class.
+        promise.reject(QueueException.E_STORAGE, StorageCause.rejectMessage(e), e)
       }
     }
   }

android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt

@@ -803,6 +803,68 @@ class QueueControllerTest {
     assertTrue(QueueSettingsStore(File(root, "settings.json")).load().wifiOnly)
   }
 
+  @Test
+  fun `setWifiOnly with the same value writes once`() {
+    val file = File(root, "settings.json")
+    controller.setWifiOnly(true)
+    assertTrue(file.exists())
+    file.delete() // a second write would create it again
+    controller.setWifiOnly(true)
+    assertFalse("the same value writes nothing", file.exists())
+    assertTrue(settings.load().wifiOnly)
+  }
+
+  @Test
+  fun `setWifiOnly with a changed value still writes`() {
+    val file = File(root, "settings.json")
+    controller.setWifiOnly(true)
+    controller.setWifiOnly(false)
+    assertFalse(QueueSettingsStore(file).load().wifiOnly)
+    controller.setWifiOnly(true)
+    assertTrue(QueueSettingsStore(file).load().wifiOnly)
+  }
+
+  @Test
+  fun `setWifiOnly with the same value does not fail when the settings can not be written`() {
+    controller.setWifiOnly(true)
+    root.setWritable(false)
+    try {
+      controller.setWifiOnly(true)
+    } finally {
+      root.setWritable(true)
+    }
+  }
+
+  @Test
+  fun `a failed settings write rejects with the cause in the message`() {
+    root.setWritable(false)
+    val e = try {
+      assertThrows(java.io.IOException::class.java) { controller.setWifiOnly(true) }
+    } finally {
+      root.setWritable(true)
+    }
+    // UploaderModule.onQueue rejects E_STORAGE with this message.
+    val message = StorageCause.rejectMessage(e)
+    assertTrue(message, message.startsWith(e.message!!))
+    assertTrue(message, message.endsWith(" (cause: java.io.FileNotFoundException)"))
+  }
+
+  @Test
+  fun `a failed journal write on cancel carries the cause`() {
+    controller.enqueue(parsed())
+    val dir = tmp.root.resolve("journal")
+    dir.setWritable(false)
+    val e = try {
+      assertThrows(QueueException::class.java) { controller.cancel("e1") }
+    } finally {
+      dir.setWritable(true)
+    }
+    assertEquals(QueueException.E_STORAGE, e.code)
+    assertTrue(e.cause is java.io.IOException)
+    assertTrue(e.message, e.message!!.startsWith("could not journal the cancel: ${e.cause!!.message} (cause: "))
+    assertTrue(e.message, e.message!!.endsWith("(cause: ${e.cause!!.javaClass.name})"))
+  }
+
   @Test
   fun `updateHeaders cancels the expiry wake of the entries it unparks`() {
     store.save(entry(id = "p", state = EntryState.AWAITING_AUTH, parkedGeneration = 0))

android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt

@@ -270,6 +270,15 @@ class QueueStoreTest {
     assertFalse(AtomicFiles.tmpFor(target).exists())
   }
 
+  @Test
+  fun `a failed rename says why and leaves no tmp file`() {
+    // A non-empty directory where the target goes: rename(2) refuses it.
+    val target = File(tmp.newFolder(), "entry.json").apply { mkdir(); File(this, "x").writeText("x") }
+    val e = assertThrows(IOException::class.java) { AtomicFiles.writeText(target, "new") }
+    assertTrue(e.javaClass.name, e is java.nio.file.FileSystemException)
+    assertFalse(AtomicFiles.tmpFor(target).exists())
+  }
+
   @Test
   fun `a save into an unwritable directory throws`() {
     val notADir = tmp.newFile()

android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt

@@ -0,0 +1,52 @@
+package ai.openspace.backgroundupload
+
+import android.system.ErrnoException
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import java.io.FileNotFoundException
+import java.io.IOException
+
+class StorageCauseTest {
+  private val defaultErrnoNameOf = StorageCause.errnoNameOf
+
+  @After
+  fun tearDown() {
+    StorageCause.errnoNameOf = defaultErrnoNameOf
+  }
+
+  @Test
+  fun `suffix names the errno when the cause chain holds an ErrnoException`() {
+    // On the JVM the android.jar stub leaves errno at 0 and cannot name it, so
+    // the test names it. On a device the default reads errno and OsConstants.
+    StorageCause.errnoNameOf = { if (it is ErrnoException) "EACCES" else null }
+    val e = IOException("could not create /q", ErrnoException("mkdir", 13))
+    assertEquals(" (cause: java.io.IOException; errno EACCES)", StorageCause.suffix(e, withMessage = false))
+    val wrapped = QueueException(QueueException.E_STORAGE, "could not save the entry", e)
+    assertEquals(" (cause: ai.openspace.backgroundupload.QueueException; errno EACCES)",
+      StorageCause.suffix(wrapped, withMessage = false))
+  }
+
+  @Test
+  fun `the default errno reader ignores a throwable that is not an ErrnoException`() {
+    assertEquals(null, StorageCause.errnoNameOf(IOException("x")))
+  }
+  @Test
+  fun `suffix names the class and the message`() {
+    val e = FileNotFoundException("/q/settings.json.tmp: open failed: ENOSPC (No space left on device)")
+    assertEquals(
+      " (cause: java.io.FileNotFoundException: /q/settings.json.tmp: open failed: ENOSPC (No space left on device))",
+      StorageCause.suffix(e),
+    )
+    assertEquals(" (cause: java.io.FileNotFoundException)", StorageCause.suffix(e, withMessage = false))
+  }
+
+  @Test
+  fun `reject message keeps the old text first`() {
+    assertEquals("disk (cause: java.io.IOException)", StorageCause.rejectMessage(IOException("disk")))
+    assertEquals(
+      "IllegalStateException (cause: java.lang.IllegalStateException)",
+      StorageCause.rejectMessage(IllegalStateException()),
+    )
+  }
+}