From 845b6d9ae83e17358bfc1dd94ad1aab69ec63ab8 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 11:48:05 -0500 Subject: [PATCH 1/2] Skip unchanged setWifiOnly writes and put the cause in E_STORAGE setWifiOnly() wrote the settings file on every call, also when the value did not change. The Diana app calls it at every start, so on a nearly full disk the call rejected E_STORAGE for no reason. Now a call with the current value writes nothing and resolves, on both platforms. On Android the skip is in QueueSettingsStore.update, so pause, resume and the retry defaults also skip a write that changes nothing, as iOS pause and resume already did. Several E_STORAGE rejections did not say why the write failed, so the Sentry events could not be told apart. Each one now keeps its message text and adds " (cause: ...)": on iOS the NSError domain and code, the POSIX code when there is one, and the description when the message does not already hold it; on Android the exception class and the errno name when the cause chain holds an ErrnoException. QueueException now has a cause. The code stays E_STORAGE. To get an errno on iOS, FileIO.writeSynced and the multipart writer now create the file with POSIX open() instead of FileManager.createFile, and FileIO.rename throws an NSPOSIXErrorDomain error. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 12 ++ .../backgroundupload/QueueController.kt | 24 +++- .../backgroundupload/QueueSettings.kt | 10 +- .../openspace/backgroundupload/QueueStore.kt | 4 +- .../backgroundupload/StorageCause.kt | 34 ++++++ .../backgroundupload/UploaderModule.kt | 4 +- .../backgroundupload/QueueControllerTest.kt | 62 ++++++++++ .../backgroundupload/StorageCauseTest.kt | 27 +++++ ios/BodyStaging.swift | 23 ++-- ios/EventJournal.swift | 10 +- ios/FileIO.swift | 52 +++++++-- ios/QueueCoordinator+Enqueue.swift | 6 +- ios/QueueCoordinator+Outcomes.swift | 35 +++--- ios/QueueCoordinator.swift | 47 +++++--- ios/Tests/StorageCauseTests.swift | 106 ++++++++++++++++++ 15 files changed, 396 insertions(+), 60 deletions(-) create mode 100644 android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt create mode 100644 android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt create mode 100644 ios/Tests/StorageCauseTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 008df890..fe002c26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ ## Unreleased +`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. On Android, `pause()`, `resume()` and the retry +defaults of `configure()` also skip a write that changes nothing. + +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. The code stays `E_STORAGE`. + iOS no longer deletes a chunked upload's file when `enqueue` cannot save the entry, for example on a full disk. The library had already moved the caller's file into its queue, so this deleted the only copy. The file now diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt index b5e83e34..178b8b4b 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt @@ -139,7 +139,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. */ @@ -148,7 +152,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) @@ -158,7 +166,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, + ) } } @@ -242,7 +254,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 1adda7fe..15c2a2c1 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt @@ -77,10 +77,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 3db76532..1f4e9b15 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueStore.kt @@ -210,8 +210,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..379f2108 --- /dev/null +++ b/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt @@ -0,0 +1,34 @@ +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 { + /** [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? { + val errno = generateSequence(error) { it.cause }.take(8).filterIsInstance().firstOrNull() + ?: return null + return runCatching { OsConstants.errnoName(errno.errno) }.getOrNull() ?: errno.errno.toString() + } +} 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 1f70cfa2..dcbbc631 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt @@ -667,6 +667,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/StorageCauseTest.kt b/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt new file mode 100644 index 00000000..5da021e4 --- /dev/null +++ b/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt @@ -0,0 +1,27 @@ +package ai.openspace.backgroundupload + +import org.junit.Assert.assertEquals +import org.junit.Test +import java.io.FileNotFoundException +import java.io.IOException + +class StorageCauseTest { + @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..af35128a 100644 --- a/ios/FileIO.swift +++ b/ios/FileIO.swift @@ -23,22 +23,60 @@ enum FileIO { /// 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 2566222a..a998ae3a 100644 --- a/ios/QueueCoordinator+Enqueue.swift +++ b/ios/QueueCoordinator+Enqueue.swift @@ -172,7 +172,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)) } } @@ -185,7 +186,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 9684185f..4a6cb699 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) @@ -78,7 +86,6 @@ extension QueueCoordinator { } disarmExpiry(id) throttle.reset(id) - 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 dcc46db8..486ce660 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -132,7 +132,7 @@ final class QueueCoordinator { queue.async { var next = self.settings next.apply(configure: options) - _ = self.saveSettings(next) + try? self.saveSettings(next) } } @@ -146,7 +146,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) @@ -192,9 +192,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() @@ -243,14 +247,18 @@ final class QueueCoordinator { queue.async { guard let e = self.index.entry(id) else { 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() @@ -261,17 +269,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()) } @@ -299,8 +311,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 { @@ -560,14 +574,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..7a6aad37 --- /dev/null +++ b/ios/Tests/StorageCauseTests.swift @@ -0,0 +1,106 @@ +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) }) + } + + // 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) + } + + // 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))") + } +} From deccc4295778966bf62edd70abe1b2424e62f6a5 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 12:02:43 -0500 Subject: [PATCH 2/2] Skip unchanged configure writes and clean up failed tmp files Review follow-ups. iOS configure() wrote the settings file on every call. Now the same options write nothing, as on Android. iOS FileIO.writeAtomically left a partly written .tmp file when the write or fsync failed, for example on a full disk. That file held space that a retry needs. Now it is deleted, as Android already did. Android AtomicFiles used File.mkdirs and File.renameTo, which return only a Boolean, so a failure did not say why. It now uses java.nio Files.createDirectories and Files.move with ATOMIC_MOVE, whose exceptions carry the reason. android.system.Os would also give the errno, but it is a stub in the JVM unit tests, so every test that writes a file would fail. StorageCause gets a seam for the errno name, so a JVM test can check the "; errno EACCES" text. New iOS test for the multipart writer when it cannot create its file. README notes that the same setWifiOnly value resolves without a write, and that E_STORAGE messages are for logs only. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 15 ++++--- README.md | 6 ++- .../openspace/backgroundupload/AtomicFiles.kt | 18 +++++--- .../backgroundupload/StorageCause.kt | 16 ++++--- .../backgroundupload/QueueStoreTest.kt | 9 ++++ .../backgroundupload/StorageCauseTest.kt | 25 +++++++++++ ios/FileIO.swift | 11 ++++- ios/QueueCoordinator.swift | 1 + ios/Tests/StorageCauseTests.swift | 45 +++++++++++++++++++ 9 files changed, 127 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fe002c26..97b0fbbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,16 +1,21 @@ ## Unreleased `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. On Android, `pause()`, `resume()` and the retry -defaults of `configure()` also skip a write that changes nothing. +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. The code stays `E_STORAGE`. +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`. iOS no longer deletes a chunked upload's file when `enqueue` cannot save the entry, for example on a full disk. The library had already moved the diff --git a/README.md b/README.md index 2709ebf5..2317af6b 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, @@ -430,7 +431,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/StorageCause.kt b/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt index 379f2108..1959b2a0 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/StorageCause.kt @@ -14,6 +14,15 @@ import android.system.OsConstants * 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 @@ -26,9 +35,6 @@ internal object StorageCause { fun rejectMessage(error: Throwable): String = (error.message ?: error.javaClass.simpleName) + suffix(error, withMessage = false) - private fun errnoName(error: Throwable): String? { - val errno = generateSequence(error) { it.cause }.take(8).filterIsInstance().firstOrNull() - ?: return null - return runCatching { OsConstants.errnoName(errno.errno) }.getOrNull() ?: errno.errno.toString() - } + private fun errnoName(error: Throwable): String? = + generateSequence(error) { it.cause }.take(8).firstNotNullOfOrNull { errnoNameOf(it) } } diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt index 98d107ba..e3f28cd0 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueStoreTest.kt @@ -235,6 +235,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 index 5da021e4..bc9e4146 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/StorageCauseTest.kt @@ -1,11 +1,36 @@ 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)") diff --git a/ios/FileIO.swift b/ios/FileIO.swift index af35128a..8332c932 100644 --- a/ios/FileIO.swift +++ b/ios/FileIO.swift @@ -15,10 +15,17 @@ 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. diff --git a/ios/QueueCoordinator.swift b/ios/QueueCoordinator.swift index 486ce660..ff90f2e6 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -132,6 +132,7 @@ final class QueueCoordinator { queue.async { var next = self.settings next.apply(configure: options) + guard next != self.settings else { return } // the same options write nothing try? self.saveSettings(next) } } diff --git a/ios/Tests/StorageCauseTests.swift b/ios/Tests/StorageCauseTests.swift index 7a6aad37..5cbd3a4c 100644 --- a/ios/Tests/StorageCauseTests.swift +++ b/ios/Tests/StorageCauseTests.swift @@ -53,6 +53,39 @@ final class StorageCauseTests: XCTestCase { 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 { @@ -91,6 +124,18 @@ final class StorageCauseTests: XCTestCase { 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() {