diff --git a/CHANGELOG.md b/CHANGELOG.md index 4811236e..1289cb4b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 6e84bf0d..8011ca01 100644 --- a/README.md +++ b/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` Merges the patch into the headers of every entry not yet forgotten and diff --git a/android/src/main/java/ai/openspace/backgroundupload/AtomicFiles.kt b/android/src/main/java/ai/openspace/backgroundupload/AtomicFiles.kt index bf291b53..38a9c997 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/AtomicFiles.kt +++ b/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,12 +19,17 @@ 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 -> @@ -30,7 +37,8 @@ internal object AtomicFiles { 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 diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt index 812b93c1..deb43a2d 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt +++ b/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 diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt index 8d86e490..58e6d7d9 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt +++ b/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 diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt index 7dedf9e6..83ac73af 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt +++ b/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" diff --git a/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt b/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt new file mode 100644 index 00000000..1959b2a0 --- /dev/null +++ b/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: : ; errno )`. 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) } +} diff --git a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt b/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt index 106f9518..aeed9aa4 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt +++ b/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) } } } diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt index 20244942..d08852a7 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt +++ b/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)) diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt index 5096a56b..f120889d 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt +++ b/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() diff --git a/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt b/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt new file mode 100644 index 00000000..bc9e4146 --- /dev/null +++ b/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()), + ) + } +} diff --git a/ios/BodyStaging.swift b/ios/BodyStaging.swift index 73656426..0ed9c6b5 100644 --- a/ios/BodyStaging.swift +++ b/ios/BodyStaging.swift @@ -38,7 +38,7 @@ enum BodyStaging { do { try fm.createDirectory(at: dir, withIntermediateDirectories: true) } catch { - throw StagingError.io("cannot create the entry directory: \(error.localizedDescription)") + throw ioError("cannot create the entry directory", error) } switch body { case .none: @@ -75,7 +75,7 @@ enum BodyStaging { throw e } catch { try? fm.removeItem(at: tmp) - throw StagingError.io("cannot write the form body: \(error.localizedDescription)") + throw ioError("cannot write the form body", error) } case .file(let path): @@ -93,7 +93,7 @@ enum BodyStaging { try? fm.removeItem(at: tmp) // The source vanished between the check and the copy. if !fm.fileExists(atPath: src.path) { throw StagingError.fileMissing(path) } - throw StagingError.io("cannot copy the file body: \(error.localizedDescription)") + throw ioError("cannot copy the file body", error) } return StagedBody(kind: .file, relativePath: name, contentType: nil, forceContentType: false, totalBytes: FileIO.size(dest) ?? 0, adopted: false) @@ -108,7 +108,7 @@ enum BodyStaging { // An O(1) rename on the same volume. Across volumes FileManager copies. try fm.moveItem(at: src, to: dir.appendingPathComponent(name)) } catch { - throw StagingError.io("cannot move the chunked file: \(error.localizedDescription)") + throw ioError("cannot move the chunked file", error) } return StagedBody(kind: .parts, relativePath: name, contentType: nil, forceContentType: false, totalBytes: size, adopted: false) @@ -127,10 +127,12 @@ enum BodyStaging { static func writeMultipart(_ fields: [ParsedEnqueue.FormField], boundary: String, to url: URL, fm: FileManager = .default) throws -> Int64 { try? fm.removeItem(at: url) - guard fm.createFile(atPath: url.path, contents: nil) else { - throw StagingError.io("cannot create the form body") + let out: FileHandle + do { + out = try FileIO.create(url) + } catch { + throw StagingError.io("cannot create the form body" + FileIO.cause(error)) } - let out = try FileHandle(forWritingTo: url) defer { try? out.close() } var total: Int64 = 0 func put(_ data: Data) throws { @@ -192,11 +194,16 @@ enum BodyStaging { .replacingOccurrences(of: "\n", with: "%0A") } + /// `: (cause: ; POSIX )`. + private static func ioError(_ step: String, _ error: Error) -> StagingError { + .io("\(step): \(error.localizedDescription)" + FileIO.cause(error, description: false)) + } + private static func write(_ data: Data, _ url: URL) throws { do { try FileIO.writeAtomically(data, to: url) } catch { - throw StagingError.io("cannot write the body: \(error.localizedDescription)") + throw ioError("cannot write the body", error) } } } diff --git a/ios/EventJournal.swift b/ios/EventJournal.swift index 46a0af89..c038c83b 100644 --- a/ios/EventJournal.swift +++ b/ios/EventJournal.swift @@ -125,15 +125,19 @@ final class EventJournal { /// outcome file is gone cannot be acked. @discardableResult func append(_ event: JournaledEvent, keeping: Set = []) -> Bool { - queue.sync { + (try? appendOrThrow(event, keeping: keeping)) != nil + } + + /// `append`, but a failed write throws its error. + func appendOrThrow(_ event: JournaledEvent, keeping: Set = []) throws { + try queue.sync { do { try write(event) } catch { NSLog("[RNFileUploader] journal append failed: \(error.localizedDescription)") - return false + throw error } pruneToMax(keeping: keeping.union([event.eventId])) - return true } } diff --git a/ios/FileIO.swift b/ios/FileIO.swift index e41690ec..8332c932 100644 --- a/ios/FileIO.swift +++ b/ios/FileIO.swift @@ -15,30 +15,75 @@ enum FileIO { } /// Writes `data` to `url.tmp`, flushes it to disk, then renames it onto `url`. + /// On a failure the tmp file is deleted, so a partly written file does not + /// hold space that a retry needs. static func writeAtomically(_ data: Data, to url: URL) throws { let tmp = tmpURL(for: url) - try writeSynced(data, to: tmp) - try rename(tmp, onto: url) + do { + try writeSynced(data, to: tmp) + try rename(tmp, onto: url) + } catch { + try? FileManager.default.removeItem(at: tmp) + throw error + } } /// Writes and fsyncs a file in place. Callers rename it afterwards. static func writeSynced(_ data: Data, to url: URL) throws { - let fm = FileManager.default - try? fm.removeItem(at: url) - guard fm.createFile(atPath: url.path, contents: nil) else { - throw IOError(message: "cannot create \(url.lastPathComponent)") - } - let handle = try FileHandle(forWritingTo: url) + try? FileManager.default.removeItem(at: url) + let handle = try create(url) defer { try? handle.close() } try handle.write(contentsOf: data) try handle.synchronize() } + /// Creates (or truncates) a file and opens it for writing. POSIX `open`, + /// not `FileManager.createFile`, so a failure carries its errno. + static func create(_ url: URL) throws -> FileHandle { + let fd = open(url.path, O_WRONLY | O_CREAT | O_TRUNC, 0o644) + guard fd >= 0 else { throw posixError(errno, "cannot create \(url.lastPathComponent)") } + return FileHandle(fileDescriptor: fd, closeOnDealloc: true) + } + /// POSIX rename: atomic, and it replaces an existing destination. static func rename(_ from: URL, onto to: URL) throws { guard Foundation.rename(from.path, to.path) == 0 else { - throw IOError(message: "rename \(from.lastPathComponent) -> \(to.lastPathComponent) failed: errno \(errno)") + throw posixError(errno, "rename \(from.lastPathComponent) -> \(to.lastPathComponent) failed") + } + } + + /// An NSPOSIXErrorDomain error whose description names the step and the errno. + static func posixError(_ code: Int32, _ step: String) -> NSError { + NSError(domain: NSPOSIXErrorDomain, code: Int(code), userInfo: [ + NSLocalizedDescriptionKey: "\(step): errno \(code) (\(String(cString: strerror(code))))", + ]) + } + + /// The underlying error of an E_STORAGE rejection, as a suffix for its + /// message: ` (cause: ; POSIX ; )`. The + /// POSIX code comes from the NSUnderlyingErrorKey chain and is left out + /// when the error is itself POSIX or the chain holds none. Pass + /// `description: false` when the message already holds the description. + /// + /// A later option: also pass the same fields as a structured + /// `userInfo.cause` on the rejection. Not done: the module rejects with a + /// nil NSError, and it is not checked that RN brings `userInfo` to JS. + static func cause(_ error: Error, description: Bool = true) -> String { + let ns = error as NSError + var fields = ["\(ns.domain) \(ns.code)"] + if ns.domain != NSPOSIXErrorDomain, let posix = underlyingPOSIXCode(ns) { fields.append("POSIX \(posix)") } + if description { fields.append(ns.localizedDescription) } + return " (cause: \(fields.joined(separator: "; ")))" + } + + private static func underlyingPOSIXCode(_ error: NSError) -> Int? { + var next = error.userInfo[NSUnderlyingErrorKey] as? NSError + for _ in 0..<8 { + guard let e = next else { return nil } + if e.domain == NSPOSIXErrorDomain { return e.code } + next = e.userInfo[NSUnderlyingErrorKey] as? NSError } + return nil } static func size(_ url: URL) -> Int64? { diff --git a/ios/QueueCoordinator+Enqueue.swift b/ios/QueueCoordinator+Enqueue.swift index a5027f3b..36ef434a 100644 --- a/ios/QueueCoordinator+Enqueue.swift +++ b/ios/QueueCoordinator+Enqueue.swift @@ -176,7 +176,8 @@ extension QueueCoordinator { do { try store.save(e) } catch { - throw EnqueueError.storage("enqueue: cannot save '\(e.id)': \(error.localizedDescription)") + throw EnqueueError.storage( + "enqueue: cannot save '\(e.id)': \(error.localizedDescription)" + FileIO.cause(error, description: false)) } } @@ -189,7 +190,8 @@ extension QueueCoordinator { try store.save(e) } catch { if staged.kind != .parts { try? FileManager.default.removeItem(at: store.fileURL(e.id, staged.relativePath)) } - throw EnqueueError.storage("enqueue: cannot save '\(e.id)': \(error.localizedDescription)") + throw EnqueueError.storage( + "enqueue: cannot save '\(e.id)': \(error.localizedDescription)" + FileIO.cause(error, description: false)) } } diff --git a/ios/QueueCoordinator+Outcomes.swift b/ios/QueueCoordinator+Outcomes.swift index 8374ac8f..1e141a21 100644 --- a/ios/QueueCoordinator+Outcomes.swift +++ b/ios/QueueCoordinator+Outcomes.swift @@ -14,17 +14,19 @@ extension QueueCoordinator { /// exists. With no listener the outcome stays at deliveries 0 for the /// drain. /// - /// `requireJournal` (a user cancel): a failed journal write returns false - /// and changes nothing. No task stops, no row moves, nothing is emitted, - /// and nothing is kept in memory, so the caller can reject and JS can call - /// again. - /// - /// Otherwise a failed journal write still settles and emits: the request - /// already ran, so a retry would send it twice. The event stays in memory, - /// a timer retries the write, and ack finds the row by its settledEventId. - @discardableResult - func settle(_ id: String, _ outcome: Outcome, requireJournal: Bool = false) -> Bool { - guard var e = index.entry(id) else { return true } + /// A failed journal write still settles and emits: the request already + /// ran, so a retry would send it twice. The event stays in memory, a timer + /// retries the write, and ack finds the row by its settledEventId. + func settle(_ id: String, _ outcome: Outcome) { + try? settle(id, outcome, requireJournal: false) + } + + /// `requireJournal` (a user cancel): a failed journal write throws its + /// error and changes nothing. No task stops, no row moves, nothing is + /// emitted, and nothing is kept in memory, so the caller can reject and JS + /// can call again. With `requireJournal` false it never throws. + func settle(_ id: String, _ outcome: Outcome, requireJournal: Bool) throws { + guard var e = index.entry(id) else { return } switch outcome { case .completed: e.bytesSent = e.totalBytes // A simple entry keeps the live bytes of its last attempt. @@ -52,8 +54,14 @@ extension QueueCoordinator { event.partIndex = nil e.state = .cancelled } - let journaled = journal.append(event, keeping: referencedEventIds().union([event.eventId])) - if !journaled && requireJournal { return false } + let journaled: Bool + do { + try journal.appendOrThrow(event, keeping: referencedEventIds().union([event.eventId])) + journaled = true + } catch { + if requireJournal { throw error } + journaled = false + } cancelTasks(id, purpose: .superseded) chunked.stop(id) emitProgress(id, sent: e.bytesSent, total: e.totalBytes) @@ -79,7 +87,6 @@ extension QueueCoordinator { disarmExpiry(id) throttle.reset(id) attemptStarts[id] = nil - return true } /// A settle whose journal write landed but whose entry.json save failed diff --git a/ios/QueueCoordinator.swift b/ios/QueueCoordinator.swift index b1bd7491..262cbe3b 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -177,7 +177,8 @@ final class QueueCoordinator { queue.async { var next = self.settings next.apply(configure: options) - _ = self.saveSettings(next) + guard next != self.settings else { return } // the same options write nothing + try? self.saveSettings(next) } } @@ -191,7 +192,7 @@ final class QueueCoordinator { reject(e.code, e.message) return } catch { - reject("E_STORAGE", error.localizedDescription) + reject("E_STORAGE", error.localizedDescription + FileIO.cause(error, description: false)) return } resolve(result.id) @@ -237,9 +238,13 @@ final class QueueCoordinator { } var next = self.settings change(&next, keys) - guard next == self.settings || self.saveSettings(next) else { - reject("E_STORAGE", "\(method): cannot save the queue settings") - return + if next != self.settings { + do { + try self.saveSettings(next) + } catch { + reject("E_STORAGE", "\(method): cannot save the queue settings" + FileIO.cause(error)) + return + } } self.applyPauseGates() resolve() @@ -294,14 +299,18 @@ final class QueueCoordinator { return resolve() } if e.isLive && !e.legacy { - guard self.settle(id, .cancelled(reason: "user"), requireJournal: true) else { - return reject("E_STORAGE", "cancel: cannot journal the outcome of '\(id)'; nothing changed") + do { + try self.settle(id, .cancelled(reason: "user"), requireJournal: true) + } catch { + return reject("E_STORAGE", "cancel: cannot journal the outcome of '\(id)'; nothing changed" + + FileIO.cause(error)) } } else { do { try self.forgetWithEvents(id) } catch { - return reject("E_STORAGE", "cancel: cannot delete '\(id)': \(error.localizedDescription)") + return reject("E_STORAGE", "cancel: cannot delete '\(id)': \(error.localizedDescription)" + + FileIO.cause(error, description: false)) } } resolve() @@ -312,17 +321,21 @@ final class QueueCoordinator { /// the session it picks. Such a queued entry whose retry waits in the /// daemon moves to the new session; a running task finishes where it /// started (session config is fixed). An entry that pins wifiOnly stays. + /// The value it already has writes nothing and resolves, so a call at + /// every app start does not fail on a full disk. func setWifiOnly(_ enabled: Bool, resolve: @escaping () -> Void, reject: @escaping (String, String) -> Void) { queue.async { + guard self.settings.wifiOnly != enabled else { return resolve() } var next = self.settings - let changed = next.wifiOnly != enabled next.wifiOnly = enabled - guard self.saveSettings(next) else { - reject("E_STORAGE", "setWifiOnly: cannot save the queue settings") + do { + try self.saveSettings(next) + } catch { + reject("E_STORAGE", "setWifiOnly: cannot save the queue settings" + FileIO.cause(error)) return } - if changed && self.ready { + if self.ready { for e in self.index.entries() where e.state == .queued && !e.isChunked && !e.legacy && e.wifiOnly == nil { let remaining = e.nextAttemptAt.map { Int($0 - self.now()) } @@ -350,8 +363,10 @@ final class QueueCoordinator { } var next = self.settings next.headerGeneration += 1 - guard self.saveSettings(next) else { - reject("E_STORAGE", "updateHeaders: cannot save the queue settings") + do { + try self.saveSettings(next) + } catch { + reject("E_STORAGE", "updateHeaders: cannot save the queue settings" + FileIO.cause(error)) return } for e in self.index.entries() where !e.legacy { @@ -611,14 +626,13 @@ final class QueueCoordinator { } // Assigns only when the write landed. - func saveSettings(_ next: QueueSettings) -> Bool { + func saveSettings(_ next: QueueSettings) throws { do { try store.saveSettings(next) settings = next - return true } catch { NSLog("[RNFileUploader] cannot save settings: \(error.localizedDescription)") - return false + throw error } } } diff --git a/ios/Tests/StorageCauseTests.swift b/ios/Tests/StorageCauseTests.swift new file mode 100644 index 00000000..5cbd3a4c --- /dev/null +++ b/ios/Tests/StorageCauseTests.swift @@ -0,0 +1,151 @@ +import XCTest +@testable import RNBGUCore + +/// setWifiOnly skips an unchanged write, and every E_STORAGE rejection +/// carries the underlying error after its message. +final class StorageCauseTests: XCTestCase { + private var h: Harness! + + override func setUp() { + h = Harness() + h.boot() + } + + override func tearDown() { + setReadOnly(h.store.root, false) + setReadOnly(h.journal.root, false) + try? FileManager.default.removeItem(at: h.root) + } + + private var settingsFile: URL { h.store.root.appendingPathComponent("settings.json") } + + /// Runs one module method and returns its rejection, or nil when it resolved. + private func rejection(_ call: (@escaping () -> Void, @escaping (String, String) -> Void) -> Void) -> EnqueueError? { + var rejected: EnqueueError? + call({}, { rejected = EnqueueError(code: $0, message: $1) }) + h.drain() + return rejected + } + + // MARK: - setWifiOnly with an unchanged value + + func testSetWifiOnlyWithTheSameValueWritesOnce() throws { + h.setWifiOnly(true) + XCTAssertTrue(FileIO.exists(settingsFile)) + // A second write would create the file again. + try FileManager.default.removeItem(at: settingsFile) + h.setWifiOnly(true) + XCTAssertFalse(FileIO.exists(settingsFile), "the same value writes nothing") + XCTAssertTrue(h.coordinator.settings.wifiOnly) + } + + func testSetWifiOnlyWithAChangedValueStillWrites() { + h.setWifiOnly(true) + h.setWifiOnly(false) + XCTAssertFalse(h.store.loadSettings().wifiOnly) + h.setWifiOnly(true) + XCTAssertTrue(h.store.loadSettings().wifiOnly) + } + + func testSetWifiOnlyWithTheSameValueResolvesWhenTheSettingsCannotBeSaved() { + h.setWifiOnly(true) + setReadOnly(h.store.root, true) + XCTAssertNil(rejection { self.h.coordinator.setWifiOnly(true, resolve: $0, reject: $1) }) + } + + func testConfigureWithTheSameOptionsWritesOnce() throws { + let options: [String: Any] = ["retry": ["backoff": ["baseMs": 500]]] + h.coordinator.configure(options) + h.drain() + XCTAssertTrue(FileIO.exists(settingsFile)) + try FileManager.default.removeItem(at: settingsFile) + h.coordinator.configure(options) + h.drain() + XCTAssertFalse(FileIO.exists(settingsFile), "the same options write nothing") + XCTAssertEqual(h.coordinator.settings.retry?.baseMs, 500) + } + + // MARK: - A failed write leaves no tmp file + + func testFailedWriteDeletesThePartlyWrittenTmpFile() throws { + let target = h.root.appendingPathComponent("f.json") + // A file size limit makes write(2) fail with EFBIG after the first bytes, + // as ENOSPC would. SIGXFSZ is ignored so the process is not killed. + let handler = signal(SIGXFSZ, SIG_IGN) + var saved = rlimit() + getrlimit(RLIMIT_FSIZE, &saved) + var small = saved + small.rlim_cur = 16 + setrlimit(RLIMIT_FSIZE, &small) + var thrown: Error? + do { try FileIO.writeAtomically(Data(count: 4096), to: target) } catch { thrown = error } + setrlimit(RLIMIT_FSIZE, &saved) + signal(SIGXFSZ, handler) + XCTAssertNotNil(thrown, "the write fails") + XCTAssertFalse(FileIO.exists(FileIO.tmpURL(for: target)), "the tmp file is deleted") + XCTAssertFalse(FileIO.exists(target)) + } + + // MARK: - The cause in the message + + func testFailedSettingsSaveRejectsWithTheCause() throws { + h.pause(keys: ["capture"]) // so the resume below changes the settings + setReadOnly(h.store.root, true) + let calls: [(String, EnqueueError?)] = [ + ("setWifiOnly", rejection { self.h.coordinator.setWifiOnly(true, resolve: $0, reject: $1) }), + ("updateHeaders", rejection { self.h.coordinator.updateHeaders(["A": "b"], resolve: $0, reject: $1) }), + ("pause", rejection { self.h.coordinator.pause([:], resolve: $0, reject: $1) }), + ("resume", rejection { self.h.coordinator.resume(["keys": ["capture"]], resolve: $0, reject: $1) }), + ] + for (method, rejected) in calls { + let r = try XCTUnwrap(rejected, "\(method) resolved") + XCTAssertEqual(r.code, "E_STORAGE") + // EACCES: the read-only directory refuses settings.json.tmp. + XCTAssertTrue(r.message.hasPrefix( + "\(method): cannot save the queue settings (cause: NSPOSIXErrorDomain 13; cannot create settings.json.tmp"), + r.message) + } + } + + func testCancelWhoseJournalWriteFailsRejectsWithTheCause() throws { + _ = try h.enqueue(h.dataRaw(id: "a")).get() + setReadOnly(h.journal.root, true) + let r = try XCTUnwrap(h.cancel("a")) + XCTAssertEqual(r.code, "E_STORAGE") + XCTAssertTrue(r.message.hasPrefix("cancel: cannot journal the outcome of 'a'; nothing changed (cause: "), r.message) + XCTAssertTrue(r.message.contains("NSPOSIXErrorDomain 13"), r.message) + } + + func testFailedEntrySaveKeepsTheDescriptionAndAddsTheCode() throws { + setReadOnly(h.store.root, true) // the entry directory cannot be made + guard case .failure(let r) = h.enqueue(h.dataRaw(id: "a")) else { return XCTFail("enqueue resolved") } + XCTAssertEqual(r.code, "E_STORAGE") + XCTAssertTrue(r.message.hasPrefix("enqueue: cannot create the entry directory: "), r.message) + XCTAssertTrue(r.message.hasSuffix("(cause: NSCocoaErrorDomain 513; POSIX 13)"), r.message) + } + + func testFormBodyThatCannotBeCreatedRejectsWithTheCause() throws { + let dir = h.store.dir("f") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + setReadOnly(dir, true) // the multipart writer cannot create its tmp file + defer { setReadOnly(dir, false) } + guard case .failure(let r) = h.enqueue(h.raw(id: "f", descriptor: [ + "url": "https://a.test", "form": [["name": "a", "contentType": "text/plain", "string": "s"]]])) else { return XCTFail("enqueue resolved") } + XCTAssertEqual(r.code, "E_STORAGE") + XCTAssertTrue(r.message.hasPrefix("enqueue: cannot create the form body (cause: NSPOSIXErrorDomain 13; "), + r.message) + } + + // MARK: - The formatter + + func testCauseNamesTheDomainCodeUnderlyingPOSIXCodeAndDescription() { + let full = NSError(domain: NSCocoaErrorDomain, code: 640, userInfo: [ + NSLocalizedDescriptionKey: "no space", + NSUnderlyingErrorKey: NSError(domain: NSPOSIXErrorDomain, code: 28), + ]) + XCTAssertEqual(FileIO.cause(full), " (cause: NSCocoaErrorDomain 640; POSIX 28; no space)") + XCTAssertEqual(FileIO.cause(full, description: false), " (cause: NSCocoaErrorDomain 640; POSIX 28)") + XCTAssertEqual(FileIO.cause(FileIO.posixError(28, "cannot create x")), + " (cause: NSPOSIXErrorDomain 28; cannot create x: errno 28 (No space left on device))") + } +}