From 6023896bb1a7e7d4020f6f713d02b97411094c84 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 13:29:42 -0500 Subject: [PATCH 1/5] iOS: apply settings in memory and save best-effort setWifiOnly, updateHeaders, pause, resume and configure no longer reject E_STORAGE when settings.json cannot be written. The new value applies at once, the failed save is logged with its cause, and the save is tried again at the next settings call and at the next app activation. updateHeaders now bumps the header generation above every entry's value too, so a relaunch that read an older settings.json does not reuse a generation. Co-Authored-By: Claude Fable 5.2 --- ios/QueueCoordinator.swift | 86 +++++++++++------- ios/Tests/SettingsSaveTests.swift | 146 ++++++++++++++++++++++++++++++ ios/Tests/StorageCauseTests.swift | 19 ---- 3 files changed, 197 insertions(+), 54 deletions(-) create mode 100644 ios/Tests/SettingsSaveTests.swift diff --git a/ios/QueueCoordinator.swift b/ios/QueueCoordinator.swift index ff90f2e6..dcf822b1 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -82,6 +82,10 @@ final class QueueCoordinator { let schedule: (_ delayMs: Int, _ block: @escaping () -> Void) -> Void var settings: QueueSettings + /// true while settings.json holds an older value than `settings`, because + /// a save failed. The next settings call and the next app activation try + /// the save again. + var settingsBehind = false /// false until the first reconcile has matched the daemon's tasks to the /// entries. Until then nothing issues: a task made now could duplicate one /// the daemon already holds. Reconcile issues whatever waited. @@ -132,8 +136,9 @@ 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) + // The same options write nothing, unless an earlier save failed. + guard next != self.settings else { return self.retrySettingsSave() } + self.applySettings(next) } } @@ -177,8 +182,9 @@ final class QueueCoordinator { } } - /// Parses the scope, saves the changed settings, then moves every entry + /// Parses the scope, applies the changed settings, then moves every entry /// whose paused state changed. `change` gets nil keys for the whole queue. + /// A failed save does not reject: see applySettings. private func changeGate(_ scope: [String: Any], method: String, resolve: @escaping () -> Void, reject: @escaping (String, String) -> Void, change: @escaping (inout QueueSettings, Set?) -> Void) { @@ -193,14 +199,7 @@ final class QueueCoordinator { } var next = self.settings change(&next, keys) - if next != self.settings { - do { - try self.saveSettings(next) - } catch { - reject("E_STORAGE", "\(method): cannot save the queue settings" + FileIO.cause(error)) - return - } - } + if next != self.settings { self.applySettings(next) } else { self.retrySettingsSave() } self.applyPauseGates() resolve() } @@ -271,19 +270,18 @@ final class QueueCoordinator { /// 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. + /// every app start does not fail on a full disk. A failed save does not + /// reject either: see applySettings. func setWifiOnly(_ enabled: Bool, resolve: @escaping () -> Void, reject: @escaping (String, String) -> Void) { queue.async { - guard self.settings.wifiOnly != enabled else { return resolve() } + guard self.settings.wifiOnly != enabled else { + self.retrySettingsSave() + return resolve() + } var next = self.settings next.wifiOnly = enabled - do { - try self.saveSettings(next) - } catch { - reject("E_STORAGE", "setWifiOnly: cannot save the queue settings" + FileIO.cause(error)) - return - } + self.applySettings(next) if self.ready { for e in self.index.entries() where e.state == .queued && !e.isChunked && !e.legacy && e.wifiOnly == nil { @@ -299,7 +297,8 @@ final class QueueCoordinator { /// Merges the patch into every entry not yet forgotten (names match /// without regard to case) and bumps the header generation. Parked entries - /// go back to queued and issue. Resolves after the loop. + /// go back to queued and issue. Resolves after the loop. Only an invalid + /// header rejects; a failed save does not (see applySettings). func updateHeaders(_ patch: [String: Any], resolve: @escaping () -> Void, reject: @escaping (String, String) -> Void) { queue.async { @@ -311,13 +310,12 @@ final class QueueCoordinator { return } var next = self.settings - next.headerGeneration += 1 - do { - try self.saveSettings(next) - } catch { - reject("E_STORAGE", "updateHeaders: cannot save the queue settings" + FileIO.cause(error)) - return - } + // Above every entry's value too. After a failed save and a relaunch, + // settings.json can hold an older generation than the entries, and a + // 401 under the newest one must still see newer headers. + next.headerGeneration = max(next.headerGeneration, + self.index.entries().map(\.headerGeneration).max() ?? 0) + 1 + self.applySettings(next) for e in self.index.entries() where !e.legacy { var n = e n.headers = HeaderMerge.merge(e.headers, headers) @@ -342,10 +340,13 @@ final class QueueCoordinator { /// The app became active. Tasks created from now on are foreground tasks, /// and the background tasks that have not started are replaced after - /// replaceDelayMs. + /// replaceDelayMs. A settings save that failed earlier is tried again. func appBecameActive() { activity.isActive = true - queue.async { self.scheduleReplace() } + queue.async { + self.retrySettingsSave() + self.scheduleReplace() + } } /// Runs replaceBackgroundTasks after replaceDelayMs, while the app is @@ -574,14 +575,29 @@ final class QueueCoordinator { expiryTokens[id] = nil } - // Assigns only when the write landed. - func saveSettings(_ next: QueueSettings) throws { + /// Applies `next` in memory, then saves it. The caller's value never + /// depends on the disk: a failed save is logged, settings.json stays + /// behind, and retrySettingsSave writes it later. Until then, a relaunch + /// with no JS reads the older saved value. + func applySettings(_ next: QueueSettings) { + settings = next + persistSettings() + } + + /// Saves `settings` when an earlier save failed. Runs at every settings + /// call and at app activation; no timer. + func retrySettingsSave() { + if settingsBehind { persistSettings() } + } + + private func persistSettings() { do { - try store.saveSettings(next) - settings = next + try store.saveSettings(settings) + settingsBehind = false } catch { - NSLog("[RNFileUploader] cannot save settings: \(error.localizedDescription)") - throw error + NSLog("[RNFileUploader] cannot save the queue settings; this session uses the new value, and the next " + + "settings call or app activation tries again" + FileIO.cause(error)) + settingsBehind = true } } } diff --git a/ios/Tests/SettingsSaveTests.swift b/ios/Tests/SettingsSaveTests.swift new file mode 100644 index 00000000..fc69d859 --- /dev/null +++ b/ios/Tests/SettingsSaveTests.swift @@ -0,0 +1,146 @@ +import XCTest +@testable import RNBGUCore + +/// A failed settings save does not fail the caller. The value applies in +/// memory at once, and the save is tried again at the next settings call and +/// at the next app activation. +final class SettingsSaveTests: XCTestCase { + private var h: Harness! + + override func setUp() { + h = Harness() + h.boot() + } + + override func tearDown() { + setReadOnly(h.store.root, false) + try? FileManager.default.removeItem(at: h.root) + } + + private var settingsFile: URL { h.store.root.appendingPathComponent("settings.json") } + private var saved: QueueSettings { h.store.loadSettings() } + + /// 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 + } + + private func onlyTask(file: StaticString = #filePath, line: UInt = #line) -> FakeTask { + XCTAssertEqual(h.transport.live.count, 1, file: file, line: line) + return h.transport.live.last! + } + + /// The settings file cannot be written. Entry directories that already + /// exist stay writable. + private func failSettingsSaves() { setReadOnly(h.store.root, true) } + private func allowSettingsSaves() { setReadOnly(h.store.root, false) } + + // MARK: - The caller does not fail + + func testSetWifiOnlyResolvesAndAppliesWhenTheSaveFails() throws { + _ = try h.enqueue(h.dataRaw(id: "a")).get() + h.complete(onlyTask(), status: 503) + let waiting = onlyTask() + failSettingsSaves() + XCTAssertNil(rejection { self.h.coordinator.setWifiOnly(true, resolve: $0, reject: $1) }) + XCTAssertTrue(h.coordinator.settings.wifiOnly, "this session uses the new value") + XCTAssertTrue(waiting.cancelled, "the waiting retry moves to the Wi-Fi session") + XCTAssertTrue(onlyTask().wifiOnly) + XCTAssertFalse(saved.wifiOnly, "the saved copy is behind") + } + + func testUpdateHeadersResolvesAndResumesParkedEntriesWhenTheSaveFails() throws { + _ = try h.enqueue(h.dataRaw(id: "a", headers: ["Authorization": "old"])).get() + h.complete(onlyTask(), status: 401) + XCTAssertEqual(h.entry("a")?.state, .awaitingAuth) + failSettingsSaves() + XCTAssertNil(rejection { self.h.coordinator.updateHeaders(["Authorization": "new"], resolve: $0, reject: $1) }) + XCTAssertEqual(h.coordinator.settings.headerGeneration, 1) + XCTAssertEqual(h.entry("a")?.headers["Authorization"], "new") + XCTAssertEqual(onlyTask().header("Authorization"), "new", "the parked entry sends again with the new headers") + XCTAssertEqual(saved.headerGeneration, 0, "the saved copy is behind") + } + + func testPauseAndResumeResolveAndMoveEntriesWhenTheSaveFails() throws { + _ = try h.enqueue(h.dataRaw(id: "a")).get() + failSettingsSaves() + XCTAssertNil(rejection { self.h.coordinator.pause([:], resolve: $0, reject: $1) }) + XCTAssertEqual(h.entry("a")?.state, .paused) + XCTAssertTrue(h.transport.live.isEmpty) + XCTAssertNil(rejection { self.h.coordinator.resume([:], resolve: $0, reject: $1) }) + XCTAssertEqual(h.entry("a")?.state, .running) + } + + func testConfigureAppliesWhenTheSaveFails() { + failSettingsSaves() + h.coordinator.configure(["retry": ["backoff": ["baseMs": 500]]]) + h.drain() + XCTAssertEqual(h.coordinator.settings.retry?.baseMs, 500) + XCTAssertNil(saved.retry) + } + + func testAnInvalidHeaderStillRejectsAndChangesNothing() throws { + _ = try h.enqueue(h.dataRaw(id: "a", headers: ["Authorization": "old"])).get() + failSettingsSaves() + let r = try XCTUnwrap(rejection { self.h.coordinator.updateHeaders(["Authorization": "new\r\n"], resolve: $0, reject: $1) }) + XCTAssertEqual(r.code, "E_INVALID") + XCTAssertEqual(h.coordinator.settings.headerGeneration, 0) + XCTAssertEqual(h.entry("a")?.headers["Authorization"], "old") + } + + // MARK: - The save is tried again + + func testTheNextSettingsCallSavesTheValueThatWasBehind() { + failSettingsSaves() + h.updateHeaders(["Authorization": "new"]) + allowSettingsSaves() + h.setWifiOnly(false) // unchanged, but the saved copy is behind + XCTAssertEqual(saved.headerGeneration, 1) + } + + func testTheNextActivationSavesTheValueThatWasBehind() { + failSettingsSaves() + h.setWifiOnly(true) + allowSettingsSaves() + h.becomeActive() + XCTAssertTrue(saved.wifiOnly) + } + + func testARetryThatFailsAgainKeepsTheValueAndTriesLater() { + failSettingsSaves() + h.setWifiOnly(true) + h.becomeActive() + XCTAssertTrue(h.coordinator.settings.wifiOnly) + XCTAssertFalse(saved.wifiOnly) + allowSettingsSaves() + h.becomeActive() + XCTAssertTrue(saved.wifiOnly) + } + + func testActivationWritesNothingWhenTheSavedCopyIsCurrent() throws { + h.setWifiOnly(true) + try FileManager.default.removeItem(at: settingsFile) + h.becomeActive() + XCTAssertFalse(FileIO.exists(settingsFile), "nothing was behind, so nothing is written") + } + + // MARK: - After a relaunch + + func testTheHeaderGenerationDoesNotGoBackAfterARelaunch() throws { + _ = try h.enqueue(h.dataRaw(id: "a")).get() + failSettingsSaves() + h.updateHeaders(["Authorization": "t1"]) // entry a saves generation 1; settings.json keeps 0 + allowSettingsSaves() + XCTAssertEqual(h.store.load("a")?.headerGeneration, 1) + h.relaunch() + h.boot() + XCTAssertEqual(h.coordinator.settings.headerGeneration, 0, "the relaunch reads the older saved value") + h.updateHeaders(["Authorization": "t2"]) + // A task sent under generation 1 (t1) that gets a 401 must see newer headers. + XCTAssertEqual(h.coordinator.settings.headerGeneration, 2) + XCTAssertEqual(h.entry("a")?.headerGeneration, 2) + } +} diff --git a/ios/Tests/StorageCauseTests.swift b/ios/Tests/StorageCauseTests.swift index 5cbd3a4c..310add99 100644 --- a/ios/Tests/StorageCauseTests.swift +++ b/ios/Tests/StorageCauseTests.swift @@ -88,25 +88,6 @@ final class StorageCauseTests: XCTestCase { // 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) From bd11e7d2cfb3e6589343fafecbc814868d2d4e77 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 13:31:30 -0500 Subject: [PATCH 2/5] Android: apply settings in memory and save best-effort QueueSettingsStore.update no longer throws when settings.json cannot be written. The cache takes the new value at once, Diag.warn records the failure with its cause, and the store remembers that the file is behind. The next update (even one that changes nothing) and the next app foreground (onHostResume) write it again. So setWifiOnly, updateHeaders, pause and resume no longer reject E_STORAGE for a failed settings save. updateHeaders now bumps the header generation above every entry's value too, as on iOS. Co-Authored-By: Claude Fable 5.2 --- .../backgroundupload/QueueController.kt | 11 ++- .../backgroundupload/QueueSettings.kt | 50 ++++++++++-- .../backgroundupload/UploaderModule.kt | 15 +++- .../backgroundupload/QueueControllerTest.kt | 66 ++++++++++++++-- .../backgroundupload/QueueSettingsTest.kt | 76 +++++++++++++++++-- 5 files changed, 198 insertions(+), 20 deletions(-) diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt index 178b8b4b..9b3a263e 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt @@ -307,6 +307,11 @@ class QueueController( generation = e.generation, ) + /** + * The settings calls (this one, [pause], [resume], [configureRetry], + * [updateHeaders]) never throw for a failed settings write. See + * [QueueSettingsStore.update]. + */ fun setWifiOnly(enabled: Boolean) { settings.update { it.copy(wifiOnly = enabled) } } @@ -323,7 +328,11 @@ class QueueController( * entry and re-issues, or parks first and is requeued here. */ fun updateHeaders(patch: Map) { - val s = settings.update { it.copy(headerGeneration = it.headerGeneration + 1) } + // Above every entry's value too. After a failed settings write and a + // relaunch, the file can hold an older generation than the entries, and + // a 401 under the newest one must still see newer headers. + val entriesMax = store.all().maxOfOrNull { it.headerGeneration } ?: 0 + val s = settings.update { it.copy(headerGeneration = maxOf(it.headerGeneration, entriesMax) + 1) } val now = clock() val unparkedIds = mutableSetOf() val changed = transformAll { e -> diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt index 15c2a2c1..238c01fd 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt @@ -68,6 +68,14 @@ class QueueSettingsStore(private val file: File) { private var cached: QueueSettings? = null + /** + * True while the file holds an older value than the cache, because a + * write failed. The next [update] and [saveIfBehind] try again. + */ + @get:Synchronized + var isBehind = false + private set + @Synchronized fun load(): QueueSettings { cached?.let { return it } @@ -78,20 +86,52 @@ class QueueSettingsStore(private val file: File) { } /** - * 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. + * Applies [transform] to the cache at once, then writes the file. A failed + * write does not throw: this process uses the new value, a warning is + * recorded, and the file stays behind until a later write lands. Until + * then, a new process (a background relaunch with no JS) reads the older + * value. + * + * A [transform] that changes nothing writes nothing, unless the file is + * behind. So a call such as setWifiOnly() at every app start does not + * write on a full disk. */ @Synchronized fun update(transform: (QueueSettings) -> QueueSettings): QueueSettings { val current = load() val next = transform(current) - if (next == current) return current - AtomicFiles.writeText(file, gson.toJson(next)) + if (next == current) { + saveIfBehind() + return current + } cached = next + write(next) return next } + /** Writes the cache when an earlier write failed. The module calls it when the app comes to the foreground. */ + @Synchronized + fun saveIfBehind() { + if (isBehind) write(load()) + } + + private fun write(s: QueueSettings) { + try { + AtomicFiles.writeText(file, gson.toJson(s)) + isBehind = false + } catch (e: Exception) { + // Only the first failure in a row is persisted, so retries on a full + // disk do not crowd the diagnostic log. + Diag.warn( + "could not save the queue settings; this session uses the new value, and the next settings " + + "call or app foreground tries again" + StorageCause.suffix(e), + e, + persist = !isBehind, + ) + isBehind = true + } + } + // Gson does not run constructors, so absent fields read as null or 0. @Suppress("SENSELESS_COMPARISON", "USELESS_ELVIS", "UNNECESSARY_SAFE_CALL") private fun validated(s: QueueSettings?): QueueSettings { diff --git a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt b/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt index aeed9aa4..1c47c2d1 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt @@ -1,6 +1,7 @@ package ai.openspace.backgroundupload import com.facebook.react.bridge.Arguments +import com.facebook.react.bridge.LifecycleEventListener import com.facebook.react.bridge.Promise import com.facebook.react.bridge.ReactApplicationContext import com.facebook.react.bridge.ReadableArray @@ -24,7 +25,7 @@ import java.util.concurrent.TimeUnit * getRequests() is synchronous and reads the in-memory index only. */ class UploaderModule(context: ReactApplicationContext) : - NativeRNFileUploaderSpec(context) { + NativeRNFileUploaderSpec(context), LifecycleEventListener { companion object { const val NAME = "RNFileUploader" @@ -73,8 +74,19 @@ class UploaderModule(context: ReactApplicationContext) : } runCatching { controller.sweep() }.onFailure { Diag.error("boot sweep failed", it) } } + context.addLifecycleEventListener(this) } + // A settings write that failed earlier is tried again each time the app + // comes to the foreground. Off the UI thread, on the queue executor. + override fun onHostResume() { + queueExecutor.execute { settings.saveIfBehind() } + } + + override fun onHostPause() = Unit + + override fun onHostDestroy() = Unit + // Set by invalidate(). A drain still queued on the executor then does not // make this dead module the journal's listener. @Volatile @@ -86,6 +98,7 @@ class UploaderModule(context: ReactApplicationContext) : // The flag goes first: the drain reads it under the journal lock. invalidated = true journal.stopListening(this) + reactApplicationContext.removeLifecycleEventListener(this) if (instance === this) instance = null super.invalidate() } diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt index dcbbc631..06e62a5c 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt @@ -699,18 +699,68 @@ class QueueControllerTest { } } - @Test - fun `a failed settings write rejects with the cause in the message`() { + /** Runs [block] while the settings file cannot be written. Entry directories that already exist stay writable. */ + private fun withSettingsUnwritable(block: () -> Unit) { root.setWritable(false) - val e = try { - assertThrows(java.io.IOException::class.java) { controller.setWifiOnly(true) } + try { + block() } 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)")) + } + + private fun savedSettings() = QueueSettingsStore(File(root, "settings.json")).load() + + @Test + fun `setWifiOnly does not throw when the settings can not be written, and this session uses the value`() { + withSettingsUnwritable { controller.setWifiOnly(true) } + assertTrue(settings.load().wifiOnly) + assertFalse("the saved copy is behind", savedSettings().wifiOnly) + } + + @Test + fun `updateHeaders patches and requeues when the settings can not be written`() { + store.save(entry(id = "p", state = EntryState.AWAITING_AUTH, parkedGeneration = 0)) + withSettingsUnwritable { controller.updateHeaders(mapOf("Authorization" to "Bearer new")) } + assertEquals(1, settings.load().headerGeneration) + val p = store.load("p")!! + assertEquals(EntryState.QUEUED, p.state) + assertEquals("Bearer new", p.descriptor!!.headers["Authorization"]) + assertEquals(1, p.headerGeneration) + assertEquals(listOf("p"), scheduler.scheduled) + assertEquals(0, savedSettings().headerGeneration) + } + + @Test + fun `pause and resume move rows when the settings can not be written`() { + store.save(entry(id = "q", state = EntryState.QUEUED)) + withSettingsUnwritable { controller.pause() } + assertEquals(EntryState.PAUSED, store.load("q")!!.state) + assertTrue(settings.load().paused) + withSettingsUnwritable { controller.resume() } + assertEquals(EntryState.QUEUED, store.load("q")!!.state) + assertFalse(settings.load().paused) + } + + @Test + fun `the next settings call saves a value that is behind`() { + withSettingsUnwritable { controller.setWifiOnly(true) } + controller.setWifiOnly(true) // the same value + assertTrue(savedSettings().wifiOnly) + } + + @Test + fun `the header generation does not go back after a relaunch that read an older settings file`() { + store.save(entry(id = "q", state = EntryState.QUEUED)) + withSettingsUnwritable { controller.updateHeaders(mapOf("Authorization" to "t1")) } + assertEquals(1, store.load("q")!!.headerGeneration) + // A new process reads settings.json, which still holds 0. + val relaunched = QueueSettingsStore(File(root, "settings.json")) + val c = QueueController(store, journal, relaunched, events, scheduler, { it in running }, { now }) + c.updateHeaders(mapOf("Authorization" to "t2")) + // A worker that sent t1 under generation 1 and gets a 401 must see newer headers. + assertEquals(2, relaunched.load().headerGeneration) + assertEquals(2, store.load("q")!!.headerGeneration) } @Test diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt index d21668ee..f66fa96a 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt @@ -86,12 +86,78 @@ class QueueSettingsTest { } @Test - fun `a failed write keeps the old value`() { - // A regular file where the directory should be: the write fails. - val notADir = tmp.newFile() - val store = QueueSettingsStore(File(notADir, "settings.json")) - runCatching { store.update { it.copy(paused = true) } } + fun `a failed write keeps the new value in memory and the old one on disk`() { + val dir = tmp.newFolder() + val file = File(dir, "settings.json") + val store = QueueSettingsStore(file) + dir.setWritable(false) + try { + assertTrue(store.update { it.copy(paused = true) }.paused) + } finally { + dir.setWritable(true) + } + assertTrue(store.load().paused) + assertTrue(store.isBehind) + assertFalse(QueueSettingsStore(file).load().paused) + } + + @Test + fun `a failed write records a warning with the cause`() { + val dir = tmp.newFolder() + Diag.log = DiagnosticLog(File(tmp.newFolder(), "diagnostics.json")) + val store = QueueSettingsStore(File(dir, "settings.json")) + dir.setWritable(false) + try { + store.update { it.copy(wifiOnly = true) } + } finally { + dir.setWritable(true) + val records = Diag.log!!.drain() + Diag.log = null + assertEquals(listOf(DiagnosticLog.WARN), records.map { it.level }) + val message = records.single().message + assertTrue(message, message.startsWith("could not save the queue settings; this session uses the new value")) + assertTrue(message, message.contains(" (cause: java.io.FileNotFoundException")) + } + } + + @Test + fun `the next update saves a value that is behind, even when it changes nothing`() { + val dir = tmp.newFolder() + val file = File(dir, "settings.json") + val store = QueueSettingsStore(file) + dir.setWritable(false) + try { + store.update { it.copy(wifiOnly = true) } + } finally { + dir.setWritable(true) + } + store.update { it.copy(wifiOnly = true) } // the same value + assertTrue(QueueSettingsStore(file).load().wifiOnly) + assertFalse(store.isBehind) + } + + @Test + fun `saveIfBehind writes only after a failed save`() { + val dir = tmp.newFolder() + val file = File(dir, "settings.json") + val store = QueueSettingsStore(file) + store.update { it.copy(paused = true) } + file.delete() + store.saveIfBehind() + assertFalse("nothing was behind, so nothing is written", file.exists()) + + dir.setWritable(false) + try { + store.update { it.copy(paused = false) } + store.saveIfBehind() // fails again and keeps the value + } finally { + dir.setWritable(true) + } assertFalse(store.load().paused) + assertTrue(store.isBehind) + store.saveIfBehind() + assertFalse(store.isBehind) + assertFalse(QueueSettingsStore(file).load().paused) } @Test From 735071dbd4bde8e2e4f229eec6121b955d0dc0a7 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 13:32:26 -0500 Subject: [PATCH 3/5] Document that settings calls do not reject for a failed save README: a "Settings on a full disk" section with the one cost (a background relaunch with no JS reads the older saved value until a later save lands), and links from setWifiOnly, updateHeaders, pause and resume. CHANGELOG under Unreleased. The JSDoc and the native spec comments say the same. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 17 +++++++++++++++++ README.md | 26 +++++++++++++++++++++++++- src/NativeRNFileUploader.ts | 9 ++++++++- src/index.ts | 6 ++++-- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97b0fbbc..ce10c39e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,22 @@ ## Unreleased +`setWifiOnly()`, `updateHeaders()`, `pause()` and `resume()` no longer +reject `E_STORAGE` when the settings file cannot be saved, on both +platforms. The new value applies in memory at once, so this session uses +it. The failed save is logged with its cause (on Android in +`takeDiagnostics()`), and the library saves again at the next settings call +and each time the app becomes active. Before, one failed save at startup +kept the old value for the whole session, and an app that stopped on the +rejection sent nothing. `configure()` also applies its retry defaults when +the save fails; before, they were dropped. Until a save lands, a relaunch +in the background with no JS reads the older saved value. See "Settings on +a full disk" in the README. `updateHeaders()` with an invalid header still +rejects `E_INVALID` and changes nothing. + +`updateHeaders()` now raises the header generation above every entry's +value as well, so the generation does not go back after a relaunch that +read an older settings file. + `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 diff --git a/README.md b/README.md index 2317af6b..ea214b18 100644 --- a/README.md +++ b/README.md @@ -415,6 +415,9 @@ event (`paused`, then `queued`). No outcome is produced and the bytes are kept. A paused entry past `expiresAt` settles `error` with `errorKind: 'expired'` when it is resumed. +Neither call rejects because the settings cannot be saved. See +[Settings on a full disk](#settings-on-a-full-disk). + ### `cancel(id): Promise` A live entry settles `cancelled` with reason `user` and is forgotten after its ack. A settled entry is forgotten now: row, bytes, and its @@ -434,6 +437,9 @@ its own value. Both are checked before each attempt, so a toggle moves the queued entries that follow the setting. A call with the current value resolves without a write. +It does not reject. The new value applies at once, also when the disk is +full. See [Settings on a full disk](#settings-on-a-full-disk). + ### `updateHeaders(patch): Promise` Merges the patch into the headers of every entry not yet forgotten and resumes the entries parked on `awaiting-auth`. The patch also replaces @@ -441,7 +447,25 @@ same-named headers a part carries. This is how a fresh token reaches requests that stalled on 401. Each call bumps a header generation: a 401 or 403 from an attempt issued under an older generation re-issues at once instead of parking. Parking emits one `state` event per entry. A header name -or value the platform HTTP client cannot send rejects with `E_INVALID`. +or value the platform HTTP client cannot send rejects with `E_INVALID` and +changes nothing. That is the only rejection: a full disk does not reject. +See [Settings on a full disk](#settings-on-a-full-disk). + +### Settings on a full disk +`setWifiOnly()`, `updateHeaders()`, `pause()`, `resume()` and `configure()` +change queue settings that native saves to a file. They apply the new value +in memory first, so this session uses it at once. Then they save the file. +If the save fails, for example on a full disk, the call still resolves. The +library records a warning (on Android, in `takeDiagnostics()`) and tries the +save again at the next one of these calls and each time the app becomes +active. + +The cost: until a save lands, a relaunch in the background with no JS reads +the older saved value. For `updateHeaders()`, a request can then send the +older headers, get a 401, and wait in `awaiting-auth` for the next +`updateHeaders()`. For `setWifiOnly()`, the earlier Wi-Fi setting applies +during that relaunch. For `pause()` and `resume()`, the earlier pause state +applies, so a paused entry can run or a resumed one can wait. ### `getRequests(filter?): RequestRow[]` Synchronous, from native's in-memory index, so it works offline. Returns diff --git a/src/NativeRNFileUploader.ts b/src/NativeRNFileUploader.ts index d9895858..37393ee5 100644 --- a/src/NativeRNFileUploader.ts +++ b/src/NativeRNFileUploader.ts @@ -65,6 +65,11 @@ export interface Spec extends TurboModule { // entry. A running attempt stops. No outcome is produced and no attempt // event is emitted. A paused entry past expiresAt settles error/expired // when it stops being paused. + // + // pause, resume, setWifiOnly, updateHeaders and configure apply the new + // settings in memory, then save them best-effort. A failed save does not + // reject: native logs it and saves again at the next settings call and at + // the next app activation. pause(scope: CodegenTypes.UnsafeObject): Promise; resume(scope: CodegenTypes.UnsafeObject): Promise; // Live entry: journal 'cancelled' (user), forget after its ack. The @@ -79,11 +84,13 @@ export interface Spec extends TurboModule { // future entries whose descriptor has no wifiOnly. An entry with // descriptor.wifiOnly set (true or false) ignores it. Both are evaluated // per attempt, so a toggle moves queued entries that follow the setting. + // Never rejects. setWifiOnly(enabled: boolean): Promise; // Merges the patch into every entry not yet forgotten and bumps a header // generation. The patch also replaces same-named headers a part carries. A 401/403 from an attempt issued under an older generation // re-issues at once instead of parking. Parking emits one 'state' event per - // entry. + // entry. Rejects E_INVALID for a header native cannot send, and nothing + // else. updateHeaders(patch: CodegenTypes.UnsafeObject): Promise; // Synchronous. Serialized from the in-memory index that native keeps // current on every state change, never from disk. Returns every entry not diff --git a/src/index.ts b/src/index.ts index d68c4d2d..dfc53464 100644 --- a/src/index.ts +++ b/src/index.ts @@ -185,7 +185,8 @@ export const createUploadClient = ({ /** * Persisted natively. Applies to queued and future entries whose - * descriptor does not set `wifiOnly`. + * descriptor does not set `wifiOnly`. Does not reject: a failed save + * still applies the value for this session. */ const setWifiOnly = (enabled: boolean): Promise => native.setWifiOnly(enabled); @@ -196,7 +197,8 @@ export const createUploadClient = ({ * same-named headers a part carries. This is how a fresh token * reaches requests that stalled on 401. Native bumps a header generation, so * a 401 from an attempt issued under the old headers re-issues at once - * instead of parking. + * instead of parking. Rejects only for a header native cannot send + * (`E_INVALID`); a failed save still applies the patch. */ const updateHeaders = (patch: Record): Promise => native.updateHeaders(patch); From f1e55fcb7f8ed4f1a42097e9774658037d3c608e Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 13:42:23 -0500 Subject: [PATCH 4/5] Retry the settings save on leaving the foreground; fix the iOS generation at launch - Also retry a settings save that is behind when the app leaves the foreground (iOS appResignedActive, Android onHostPause). It is the last chance before a relaunch with no JS reads the file. - iOS raises the header generation at launch above every saved entry and every task-map tag, since taskWillBegin re-tags a task without saving its entry. updateHeaders goes back to a plain +1. - iOS reports a failed settings save from one function, so routing it into takeDiagnostics() later is a one-line change. - README and CHANGELOG: the retries run only while the process lives; a new process uses the last value on disk, so an app should send its settings at each start. The updateHeaders cost is the counter, not the headers. iOS logs the failure to the device log only for now. - Tests: the foreground-leave retry, the task-map tag at launch, unchanged configure and resume while behind (iOS), and that a second failure in a row is not persisted (Android). Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 23 ++++++----- README.md | 34 +++++++++++----- .../backgroundupload/QueueSettings.kt | 8 ++-- .../backgroundupload/UploaderModule.kt | 8 +++- .../backgroundupload/QueueSettingsTest.kt | 30 ++++++++++++++ ios/QueueCoordinator.swift | 33 ++++++++++----- ios/TaskMap.swift | 5 +++ ios/Tests/SettingsSaveTests.swift | 40 ++++++++++++++++++- src/NativeRNFileUploader.ts | 5 ++- 9 files changed, 145 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce10c39e..6162b74c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,19 +3,22 @@ `setWifiOnly()`, `updateHeaders()`, `pause()` and `resume()` no longer reject `E_STORAGE` when the settings file cannot be saved, on both platforms. The new value applies in memory at once, so this session uses -it. The failed save is logged with its cause (on Android in -`takeDiagnostics()`), and the library saves again at the next settings call -and each time the app becomes active. Before, one failed save at startup -kept the old value for the whole session, and an app that stopped on the -rejection sent nothing. `configure()` also applies its retry defaults when -the save fails; before, they were dropped. Until a save lands, a relaunch -in the background with no JS reads the older saved value. See "Settings on +it. The failed save is logged with its cause. On Android, +`takeDiagnostics()` returns it. On iOS, it goes to the device log only for +now. The library saves again at the next settings call, when the app +becomes active, and when it leaves the foreground. These retries happen +only while the process lives. A new process uses the last value that +reached the disk, so an app should send its settings again at each start. +A call with an unchanged value writes nothing. Before, one failed save at +startup kept the old value for the whole session, and an app that stopped +on the rejection sent nothing. `configure()` also applies its retry +defaults when the save fails; before, they were dropped. See "Settings on a full disk" in the README. `updateHeaders()` with an invalid header still rejects `E_INVALID` and changes nothing. -`updateHeaders()` now raises the header generation above every entry's -value as well, so the generation does not go back after a relaunch that -read an older settings file. +The header generation no longer goes back after a relaunch that read an +older settings file. iOS raises it at launch above every entry and task +record. Android raises it above every entry at each `updateHeaders()`. `setWifiOnly()` with the value the queue already has no longer writes the settings file, on both platforms. It still resolves. Before, an app that diff --git a/README.md b/README.md index ea214b18..08f9d609 100644 --- a/README.md +++ b/README.md @@ -455,17 +455,29 @@ See [Settings on a full disk](#settings-on-a-full-disk). `setWifiOnly()`, `updateHeaders()`, `pause()`, `resume()` and `configure()` change queue settings that native saves to a file. They apply the new value in memory first, so this session uses it at once. Then they save the file. -If the save fails, for example on a full disk, the call still resolves. The -library records a warning (on Android, in `takeDiagnostics()`) and tries the -save again at the next one of these calls and each time the app becomes -active. - -The cost: until a save lands, a relaunch in the background with no JS reads -the older saved value. For `updateHeaders()`, a request can then send the -older headers, get a 401, and wait in `awaiting-auth` for the next -`updateHeaders()`. For `setWifiOnly()`, the earlier Wi-Fi setting applies -during that relaunch. For `pause()` and `resume()`, the earlier pause state -applies, so a paused entry can run or a resumed one can wait. +If the save fails, for example on a full disk, the call still resolves. + +The library records a warning. On Android, `takeDiagnostics()` returns it. +On iOS, the warning goes to the device log only for now. It will go to +`takeDiagnostics()` when iOS has a diagnostics list. + +The library tries the save again at the next one of these calls, when the +app becomes active, and when the app leaves the foreground. These retries +happen only while the process lives. A new process, for example a relaunch +in the background with no JS, uses the last value that reached the disk: + +- `setWifiOnly()`: the earlier Wi-Fi setting applies. +- `pause()` and `resume()`: the earlier pause state applies. At launch, + native moves the entries to match it. So an entry that was paused can run, + and an entry that was resumed can wait. +- `updateHeaders()`: the headers are saved with each entry, not in the + settings file, so they are not lost. Only a counter of header updates is. + Native raises that counter again above the entries' values, so a 401 still + re-issues as described above. + +So send your settings again at each app start: call `setWifiOnly()`, and +`pause()` or `resume()`, with the values you want. A call with the value +native already has writes nothing. ### `getRequests(filter?): RequestRow[]` Synchronous, from native's in-memory index, so it works offline. Returns diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt index 238c01fd..481f9866 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueSettings.kt @@ -88,9 +88,9 @@ class QueueSettingsStore(private val file: File) { /** * Applies [transform] to the cache at once, then writes the file. A failed * write does not throw: this process uses the new value, a warning is - * recorded, and the file stays behind until a later write lands. Until - * then, a new process (a background relaunch with no JS) reads the older - * value. + * recorded, and the file stays behind until a later write in this process + * lands. A new process (a background relaunch with no JS) reads the last + * value that reached the file. * * A [transform] that changes nothing writes nothing, unless the file is * behind. So a call such as setWifiOnly() at every app start does not @@ -109,7 +109,7 @@ class QueueSettingsStore(private val file: File) { return next } - /** Writes the cache when an earlier write failed. The module calls it when the app comes to the foreground. */ + /** Writes the cache when an earlier write failed. The module calls it when the app enters and leaves the foreground. */ @Synchronized fun saveIfBehind() { if (isBehind) write(load()) diff --git a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt b/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt index 1c47c2d1..349215bf 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/UploaderModule.kt @@ -78,12 +78,16 @@ class UploaderModule(context: ReactApplicationContext) : } // A settings write that failed earlier is tried again each time the app - // comes to the foreground. Off the UI thread, on the queue executor. + // comes to the foreground, and when it leaves: the last chance before a + // relaunch with no JS reads the file. Off the UI thread, on the queue + // executor. override fun onHostResume() { queueExecutor.execute { settings.saveIfBehind() } } - override fun onHostPause() = Unit + override fun onHostPause() { + queueExecutor.execute { settings.saveIfBehind() } + } override fun onHostDestroy() = Unit diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt index f66fa96a..a2d494a7 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueSettingsTest.kt @@ -120,6 +120,36 @@ class QueueSettingsTest { } } + @Test + fun `a second failure in a row is not persisted to the diagnostic log`() { + val dir = tmp.newFolder() + Diag.log = DiagnosticLog(File(tmp.newFolder(), "diagnostics.json")) + val store = QueueSettingsStore(File(dir, "settings.json")) + dir.setWritable(false) + try { + store.update { it.copy(wifiOnly = true) } + store.update { it.copy(paused = true) } + store.saveIfBehind() + } finally { + dir.setWritable(true) + val records = Diag.log!!.drain() + Diag.log = null + assertEquals("only the first failure", 1, records.size) + } + // After a write lands, the next failure is persisted again. + store.saveIfBehind() + Diag.log = DiagnosticLog(File(tmp.newFolder(), "diagnostics.json")) + dir.setWritable(false) + try { + store.update { it.copy(paused = false) } + } finally { + dir.setWritable(true) + val records = Diag.log!!.drain() + Diag.log = null + assertEquals(1, records.size) + } + } + @Test fun `the next update saves a value that is behind, even when it changes nothing`() { val dir = tmp.newFolder() diff --git a/ios/QueueCoordinator.swift b/ios/QueueCoordinator.swift index dcf822b1..567927fb 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -128,6 +128,13 @@ final class QueueCoordinator { // the time JS can call it, legacy rows included. index.load(store.all()) importLegacyIfNeeded() + // After a settings save that never landed, settings.json can hold an + // older header generation than an entry or a task tag (taskWillBegin + // re-tags a task without saving its entry). The next updateHeaders must + // go above all of them, or a 401 under the newest headers parks. + settings.headerGeneration = max(settings.headerGeneration, + index.entries().map(\.headerGeneration).max() ?? 0, + taskMap.maxHeaderGeneration() ?? 0) } // MARK: - Module methods @@ -310,11 +317,7 @@ final class QueueCoordinator { return } var next = self.settings - // Above every entry's value too. After a failed save and a relaunch, - // settings.json can hold an older generation than the entries, and a - // 401 under the newest one must still see newer headers. - next.headerGeneration = max(next.headerGeneration, - self.index.entries().map(\.headerGeneration).max() ?? 0) + 1 + next.headerGeneration += 1 // init raised it above every saved tag self.applySettings(next) for e in self.index.entries() where !e.legacy { var n = e @@ -363,9 +366,11 @@ final class QueueCoordinator { } /// The app is about to leave the foreground. Tasks created from now on are - /// background tasks. + /// background tasks. A settings save that failed earlier is tried again: + /// this is the last chance before a relaunch with no JS reads the file. func appResignedActive() { activity.isActive = false + queue.async { self.retrySettingsSave() } } /// Replaces every live task that was created while the app was not active @@ -577,15 +582,15 @@ final class QueueCoordinator { /// Applies `next` in memory, then saves it. The caller's value never /// depends on the disk: a failed save is logged, settings.json stays - /// behind, and retrySettingsSave writes it later. Until then, a relaunch - /// with no JS reads the older saved value. + /// behind, and retrySettingsSave writes it later in this process. A new + /// process reads the last value that reached the file. func applySettings(_ next: QueueSettings) { settings = next persistSettings() } /// Saves `settings` when an earlier save failed. Runs at every settings - /// call and at app activation; no timer. + /// call and when the app becomes or resigns active; no timer. func retrySettingsSave() { if settingsBehind { persistSettings() } } @@ -595,9 +600,15 @@ final class QueueCoordinator { try store.saveSettings(settings) settingsBehind = false } catch { - NSLog("[RNFileUploader] cannot save the queue settings; this session uses the new value, and the next " - + "settings call or app activation tries again" + FileIO.cause(error)) + reportSettingsSaveFailure(error) settingsBehind = true } } + + /// The device log only for now. iOS has no diagnostics list yet; once it + /// has one, record this there too, so takeDiagnostics() returns it. + private func reportSettingsSaveFailure(_ error: Error) { + NSLog("[RNFileUploader] cannot save the queue settings; this session uses the new value, and the next " + + "settings call or app activation tries again" + FileIO.cause(error)) + } } diff --git a/ios/TaskMap.swift b/ios/TaskMap.swift index 91e264d6..e471a412 100644 --- a/ios/TaskMap.swift +++ b/ios/TaskMap.swift @@ -132,6 +132,11 @@ final class TaskMap { } } + /// The highest header generation a task was tagged with, or nil. + func maxHeaderGeneration() -> Int? { + queue.sync { cache.values.compactMap(\.headerGeneration).max() } + } + func keys(where predicate: (Meta) -> Bool) -> [String] { queue.sync { cache.filter { predicate($0.value) }.map(\.key) } } diff --git a/ios/Tests/SettingsSaveTests.swift b/ios/Tests/SettingsSaveTests.swift index fc69d859..b1a952be 100644 --- a/ios/Tests/SettingsSaveTests.swift +++ b/ios/Tests/SettingsSaveTests.swift @@ -109,6 +109,32 @@ final class SettingsSaveTests: XCTestCase { XCTAssertTrue(saved.wifiOnly) } + func testLeavingTheForegroundSavesTheValueThatWasBehind() { + failSettingsSaves() + h.setWifiOnly(true) + allowSettingsSaves() + h.resignActive() + h.drain() + XCTAssertTrue(saved.wifiOnly, "the last chance before a relaunch with no JS") + } + + func testConfigureWithUnchangedOptionsSavesTheValueThatWasBehind() { + failSettingsSaves() + h.setWifiOnly(true) + allowSettingsSaves() + h.coordinator.configure([:]) // no retry, as now + h.drain() + XCTAssertTrue(saved.wifiOnly) + } + + func testPauseOrResumeThatChangesNothingSavesTheValueThatWasBehind() { + failSettingsSaves() + h.setWifiOnly(true) + allowSettingsSaves() + h.resume() // nothing is paused + XCTAssertTrue(saved.wifiOnly) + } + func testARetryThatFailsAgainKeepsTheValueAndTriesLater() { failSettingsSaves() h.setWifiOnly(true) @@ -135,12 +161,24 @@ final class SettingsSaveTests: XCTestCase { h.updateHeaders(["Authorization": "t1"]) // entry a saves generation 1; settings.json keeps 0 allowSettingsSaves() XCTAssertEqual(h.store.load("a")?.headerGeneration, 1) + XCTAssertEqual(saved.headerGeneration, 0) h.relaunch() h.boot() - XCTAssertEqual(h.coordinator.settings.headerGeneration, 0, "the relaunch reads the older saved value") + XCTAssertEqual(h.coordinator.settings.headerGeneration, 1, "raised to the entry's value at launch") h.updateHeaders(["Authorization": "t2"]) // A task sent under generation 1 (t1) that gets a 401 must see newer headers. XCTAssertEqual(h.coordinator.settings.headerGeneration, 2) XCTAssertEqual(h.entry("a")?.headerGeneration, 2) } + + func testTheHeaderGenerationAtLaunchIsAboveEveryTaskTag() { + // taskWillBegin re-tags a task without saving its entry, so a tag can be + // above every saved entry and above settings.json. + h.map.set(TaskMap.Meta(id: "a", headerGeneration: 5), forKey: "any:99") + h.relaunch() + XCTAssertEqual(h.coordinator.settings.headerGeneration, 5) + h.boot() + h.updateHeaders(["Authorization": "t"]) + XCTAssertEqual(h.coordinator.settings.headerGeneration, 6) + } } diff --git a/src/NativeRNFileUploader.ts b/src/NativeRNFileUploader.ts index 37393ee5..9067a924 100644 --- a/src/NativeRNFileUploader.ts +++ b/src/NativeRNFileUploader.ts @@ -68,8 +68,9 @@ export interface Spec extends TurboModule { // // pause, resume, setWifiOnly, updateHeaders and configure apply the new // settings in memory, then save them best-effort. A failed save does not - // reject: native logs it and saves again at the next settings call and at - // the next app activation. + // reject: native logs it and saves again at the next settings call and + // when the app becomes active or leaves the foreground, while the process + // lives. A new process reads the last value that reached the disk. pause(scope: CodegenTypes.UnsafeObject): Promise; resume(scope: CodegenTypes.UnsafeObject): Promise; // Live entry: journal 'cancelled' (user), forget after its ack. The From a555a2887910d3afc093e7eb0cdcb9297cf298a5 Mon Sep 17 00:00:00 2001 From: Dylan Murphy Date: Tue, 10 Nov 2026 15:05:29 -0500 Subject: [PATCH 5/5] Android: keep going past a failed entry save; compare parked entries with their own generation - pause, resume and updateHeaders on Android read entries from disk, so an entry cannot change in memory only. transformAll now goes on past an entry whose save fails. Each call stops or schedules every entry it did save, then rejects E_STORAGE with the count and the cause. Before, the first failure stopped the loop, and entries already saved as queued got no work. - toResumed compares parkedGeneration with the entry's own headerGeneration, not the settings value, which can be stale after a settings save that never landed. - iOS logs only the first settings save failure in a row. - README, JSDoc, native spec comments and CHANGELOG say that on Android these calls can still reject E_STORAGE for an entry file, and what state a failed entry keeps. Co-Authored-By: Claude Fable 5.2 --- CHANGELOG.md | 12 ++++ README.md | 23 +++++-- .../backgroundupload/EntryTransitions.kt | 11 ++- .../backgroundupload/QueueController.kt | 69 +++++++++++++++---- .../backgroundupload/EntryTransitionsTest.kt | 11 +-- .../backgroundupload/QueueControllerTest.kt | 69 +++++++++++++++++++ ios/QueueCoordinator.swift | 4 +- src/NativeRNFileUploader.ts | 10 +-- src/index.ts | 6 +- 9 files changed, 182 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6162b74c..4e10bfed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,18 @@ defaults when the save fails; before, they were dropped. See "Settings on a full disk" in the README. `updateHeaders()` with an invalid header still rejects `E_INVALID` and changes nothing. +On Android, `updateHeaders()`, `pause()` and `resume()` still reject +`E_STORAGE` when an entry file cannot be saved, because Android reads the +entries from disk. They now change and schedule every other entry first. +Before, the first failed entry stopped the loop, and entries already saved +as `queued` got no work until the next launch. An entry that failed keeps +its old state. + +On Android, `resume()` and the launch sweep now compare a parked entry with +its own header generation, not the one in the settings file. Before, after +a settings save that never landed, a resumed entry that already had new +headers could go back to `awaiting-auth`. + The header generation no longer goes back after a relaunch that read an older settings file. iOS raises it at launch above every entry and task record. Android raises it above every entry at each `updateHeaders()`. diff --git a/README.md b/README.md index 08f9d609..bfe9a693 100644 --- a/README.md +++ b/README.md @@ -415,8 +415,9 @@ event (`paused`, then `queued`). No outcome is produced and the bytes are kept. A paused entry past `expiresAt` settles `error` with `errorKind: 'expired'` when it is resumed. -Neither call rejects because the settings cannot be saved. See -[Settings on a full disk](#settings-on-a-full-disk). +Neither call rejects because the settings cannot be saved. On Android, +either call can still reject `E_STORAGE` when an entry file cannot be saved. +See [Settings on a full disk](#settings-on-a-full-disk). ### `cancel(id): Promise` A live entry settles `cancelled` with reason `user` and is forgotten after @@ -448,8 +449,9 @@ reaches requests that stalled on 401. Each call bumps a header generation: a 401 or 403 from an attempt issued under an older generation re-issues at once instead of parking. Parking emits one `state` event per entry. A header name or value the platform HTTP client cannot send rejects with `E_INVALID` and -changes nothing. That is the only rejection: a full disk does not reject. -See [Settings on a full disk](#settings-on-a-full-disk). +changes nothing. A settings file that cannot be saved does not reject. On +Android, an entry file that cannot be saved does reject `E_STORAGE`. See +[Settings on a full disk](#settings-on-a-full-disk). ### Settings on a full disk `setWifiOnly()`, `updateHeaders()`, `pause()`, `resume()` and `configure()` @@ -479,6 +481,19 @@ So send your settings again at each app start: call `setWifiOnly()`, and `pause()` or `resume()`, with the values you want. A call with the value native already has writes nothing. +`updateHeaders()`, `pause()` and `resume()` also save each entry they +change. On iOS, a failed entry save is logged, the entry changes in memory, +and the next save of that entry writes it. On Android, the entries are read +from disk, so they cannot change in memory only. There, the call changes and +schedules every entry it can save, then rejects `E_STORAGE` when one or more +entries could not be saved. Each entry that failed keeps its old state: + +- `updateHeaders()`: it keeps its old headers and stays in `awaiting-auth` + if it was there. Call `updateHeaders()` again. +- `pause()`: it keeps its state, but its worker sees the pause before its + next attempt. +- `resume()`: it stays `paused`. Call `resume()` again. + ### `getRequests(filter?): RequestRow[]` Synchronous, from native's in-memory index, so it works offline. Returns every entry native has not yet forgotten: `queued`, `running`, diff --git a/android/src/main/java/ai/openspace/backgroundupload/EntryTransitions.kt b/android/src/main/java/ai/openspace/backgroundupload/EntryTransitions.kt index d099d8df..7b1784c5 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/EntryTransitions.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/EntryTransitions.kt @@ -58,9 +58,14 @@ object EntryTransitions { fun toPaused(e: QueueEntry, now: Long) = e.copy(state = EntryState.PAUSED, nextAttemptAt = null, updatedAt = now) - /** resume(): back to awaiting-auth only when no updateHeaders() came in between. */ - fun toResumed(e: QueueEntry, headerGeneration: Int, now: Long): QueueEntry = - if (e.parkedGeneration != null && e.parkedGeneration == headerGeneration) { + /** + * resume(): back to awaiting-auth only when no updateHeaders() came in + * between. It compares the entry's own headerGeneration, which + * updateHeaders() saves with the new headers. The settings value can be + * older after a settings write that never landed. + */ + fun toResumed(e: QueueEntry, now: Long): QueueEntry = + if (e.parkedGeneration != null && e.parkedGeneration == e.headerGeneration) { e.copy(state = EntryState.AWAITING_AUTH, updatedAt = now) } else { e.copy(state = EntryState.QUEUED, parkedGeneration = null, updatedAt = now) diff --git a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt index 9b3a263e..958a934c 100644 --- a/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt +++ b/android/src/main/java/ai/openspace/backgroundupload/QueueController.kt @@ -181,6 +181,11 @@ class QueueController( * paused set, and an empty list changes nothing. Each live row that is now * paused (gate on, or its key in the set) moves to paused and its work * stops. No outcome. + * + * A failed settings write does not throw (see [QueueSettingsStore.update]). + * A failed entry save throws E_STORAGE after every other entry is moved + * and its work stopped. The entry that failed keeps its old row; its + * worker sees the paused gate before its next attempt. */ fun pause(keys: List? = null) { if (keys != null && keys.isEmpty()) return @@ -188,17 +193,21 @@ class QueueController( if (keys == null) it.copy(paused = true) else it.copy(pausedKeys = it.pausedKeys + keys) } val now = clock() - val paused = transformAll { e -> + val result = transformAll { e -> if (e.isLive && e.state != EntryState.PAUSED && s.isPaused(e.key)) EntryTransitions.toPaused(e, now) else e } - paused.forEach { scheduler.cancel(it.id) } - paused.forEach { events.state(it.toRow()) } + result.changed.forEach { scheduler.cancel(it.id) } + result.changed.forEach { events.state(it.toRow()) } + result.throwIfFailed("pause") } /** * Undoes the pause of the same scope: [keys] null turns the gate off, a * list removes those keys from the set. A row comes back only when no * scope still pauses it, so the gate on + its key resumed stays paused. + * + * A failed entry save throws E_STORAGE after every other entry is moved + * and every queued entry is scheduled. The entry that failed stays paused. */ fun resume(keys: List? = null) { if (keys != null && keys.isEmpty()) return @@ -206,12 +215,13 @@ class QueueController( if (keys == null) it.copy(paused = false) else it.copy(pausedKeys = it.pausedKeys - keys.toSet()) } val now = clock() - val resumed = transformAll { e -> - if (e.state == EntryState.PAUSED && !s.isPaused(e.key)) EntryTransitions.toResumed(e, s.headerGeneration, now) else e + val result = transformAll { e -> + if (e.state == EntryState.PAUSED && !s.isPaused(e.key)) EntryTransitions.toResumed(e, now) else e } - resumed.forEach { events.state(it.toRow()) } + result.changed.forEach { events.state(it.toRow()) } // Every queued entry, not only the resumed ones: a run is idempotent. store.all().forEach { scheduleRun(it) } + result.throwIfFailed("resume") } /** @@ -326,6 +336,10 @@ class QueueController( * own headerGeneration, which changes here under the store lock together * with its headers. So a worker that gets a 401 either sees the patched * entry and re-issues, or parks first and is requeued here. + * + * A failed entry save throws E_STORAGE after every other entry is patched + * and the unparked ones are scheduled. The entry that failed keeps its old + * headers on disk and stays parked if it was. A later call patches it. */ fun updateHeaders(patch: Map) { // Above every entry's value too. After a failed settings write and a @@ -335,16 +349,17 @@ class QueueController( val s = settings.update { it.copy(headerGeneration = maxOf(it.headerGeneration, entriesMax) + 1) } val now = clock() val unparkedIds = mutableSetOf() - val changed = transformAll { e -> + val result = transformAll { e -> val patched = e.withHeadersPatched(patch, s.headerGeneration) if (patched.state != EntryState.AWAITING_AUTH) return@transformAll patched unparkedIds += e.id EntryTransitions.toUnparked(patched, s.isPaused(patched.key), now) } - changed.filter { it.id in unparkedIds }.forEach { entry -> + result.changed.filter { it.id in unparkedIds }.forEach { entry -> scheduleRun(entry) events.state(entry.toRow()) } + result.throwIfFailed("updateHeaders") } // MARK: - journal @@ -443,7 +458,7 @@ class QueueController( if (paused && cur.state != EntryState.PAUSED) { cur = EntryTransitions.toPaused(cur, now) } else if (!paused && cur.state == EntryState.PAUSED) { - cur = EntryTransitions.toResumed(cur, s.headerGeneration, now) + cur = EntryTransitions.toResumed(cur, now) } if (cur !== e) { if (trySave(cur)) changed += cur else continue @@ -517,16 +532,42 @@ class QueueController( if (!isWorkerRunning(id)) scheduler.cancelWake(id) } - /** Applies [transform] to every non-legacy entry under the store lock. Returns the ones it changed. */ - private fun transformAll(transform: (QueueEntry) -> QueueEntry): List { + /** The entries [transformAll] saved, and the saves that failed. */ + private class Transformed(val changed: List, val failures: Int, val firstFailure: IOException?) { + /** Throws E_STORAGE when an entry save failed. Call it after the work for [changed] is done. */ + fun throwIfFailed(method: String) { + val e = firstFailure ?: return + throw QueueException( + QueueException.E_STORAGE, + "$method: could not save $failures entries: ${e.message}" + StorageCause.suffix(e, withMessage = false), + e, + ) + } + } + + /** + * Applies [transform] to every non-legacy entry under the store lock. An + * entry whose save fails keeps its old row, and the loop goes on, so one + * full-disk failure does not leave the other entries unchanged. + */ + private fun transformAll(transform: (QueueEntry) -> QueueEntry): Transformed { val changed = mutableListOf() + var failures = 0 + var first: IOException? = null store.locked { store.all().filter { !it.legacy }.forEach { e -> - store.compute(e.id) { cur -> - if (cur == null) null else transform(cur).also { if (it !== cur) changed += it } + try { + var next: QueueEntry? = null + store.compute(e.id) { cur -> + if (cur == null) null else transform(cur).also { if (it !== cur) next = it } + } + next?.let { changed += it } // only once the save landed + } catch (error: IOException) { + failures++ + if (first == null) first = error } } } - return changed + return Transformed(changed, failures, first) } } diff --git a/android/src/test/java/ai/openspace/backgroundupload/EntryTransitionsTest.kt b/android/src/test/java/ai/openspace/backgroundupload/EntryTransitionsTest.kt index e6121262..58ad3bbf 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/EntryTransitionsTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/EntryTransitionsTest.kt @@ -42,13 +42,14 @@ class EntryTransitionsTest { } @Test - fun `resume returns to awaiting-auth only when the parked generation is current`() { - val paused = entry(state = EntryState.PAUSED, parkedGeneration = 3) - assertEquals(EntryState.AWAITING_AUTH, EntryTransitions.toResumed(paused, headerGeneration = 3, now = 9).state) - val stale = EntryTransitions.toResumed(paused, headerGeneration = 4, now = 9) + fun `resume returns to awaiting-auth only when the parked generation is the entry's own`() { + val paused = entry(state = EntryState.PAUSED, parkedGeneration = 3).copy(headerGeneration = 3) + assertEquals(EntryState.AWAITING_AUTH, EntryTransitions.toResumed(paused, now = 9).state) + // updateHeaders() saved newer headers with the entry. + val stale = EntryTransitions.toResumed(paused.copy(headerGeneration = 4), now = 9) assertEquals(EntryState.QUEUED, stale.state) assertNull(stale.parkedGeneration) - assertEquals(EntryState.QUEUED, EntryTransitions.toResumed(entry(state = EntryState.PAUSED), 0, 9).state) + assertEquals(EntryState.QUEUED, EntryTransitions.toResumed(entry(state = EntryState.PAUSED), 9).state) } @Test diff --git a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt index 06e62a5c..ce998445 100644 --- a/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt +++ b/android/src/test/java/ai/openspace/backgroundupload/QueueControllerTest.kt @@ -742,6 +742,75 @@ class QueueControllerTest { assertFalse(settings.load().paused) } + /** Runs [block] while the entry file of [id] cannot be written. */ + private fun withEntryUnwritable(id: String, block: () -> T): T { + val dir = store.entryDir(id) + dir.setWritable(false) + try { + return block() + } finally { + dir.setWritable(true) + } + } + + @Test + fun `updateHeaders with an entry that can not be saved patches and schedules the rest, then rejects`() { + store.save(entry(id = "p", state = EntryState.AWAITING_AUTH, parkedGeneration = 0)) + store.save(entry(id = "x", state = EntryState.AWAITING_AUTH, parkedGeneration = 0)) + val e = withEntryUnwritable("x") { + assertThrows(QueueException::class.java) { controller.updateHeaders(mapOf("Authorization" to "Bearer new")) } + } + assertEquals(QueueException.E_STORAGE, e.code) + assertTrue(e.message, e.message!!.startsWith("updateHeaders: could not save 1 entries: ")) + assertEquals(EntryState.QUEUED, store.load("p")!!.state) + assertEquals("Bearer new", store.load("p")!!.descriptor!!.headers["Authorization"]) + assertEquals(listOf("p"), scheduler.scheduled) + assertEquals(EntryState.AWAITING_AUTH, store.load("x")!!.state) + assertEquals(1, settings.load().headerGeneration) + } + + @Test + fun `resume with an entry that can not be saved resumes and schedules the rest, then rejects`() { + store.save(entry(id = "q", state = EntryState.QUEUED)) + store.save(entry(id = "x", state = EntryState.QUEUED)) + controller.pause() + val e = withEntryUnwritable("x") { + assertThrows(QueueException::class.java) { controller.resume() } + } + assertEquals(QueueException.E_STORAGE, e.code) + assertEquals(EntryState.QUEUED, store.load("q")!!.state) + assertTrue("q" in scheduler.scheduled) + assertEquals(EntryState.PAUSED, store.load("x")!!.state) + assertFalse(settings.load().paused) + } + + @Test + fun `pause with an entry that can not be saved pauses and stops the rest, then rejects`() { + store.save(entry(id = "q", state = EntryState.QUEUED)) + store.save(entry(id = "x", state = EntryState.QUEUED)) + val e = withEntryUnwritable("x") { + assertThrows(QueueException::class.java) { controller.pause() } + } + assertEquals(QueueException.E_STORAGE, e.code) + assertEquals(EntryState.PAUSED, store.load("q")!!.state) + assertTrue("q" in scheduler.cancelled) + assertEquals(EntryState.QUEUED, store.load("x")!!.state) + assertTrue("the gate is on, so the worker stops before its next attempt", settings.load().paused) + } + + @Test + fun `a paused parked entry with newer headers resumes to queued after a relaunch that read an older settings file`() { + store.save(entry(id = "a", state = EntryState.AWAITING_AUTH, parkedGeneration = 0)) + controller.pause() + withSettingsUnwritable { controller.updateHeaders(mapOf("Authorization" to "Bearer new")) } + assertEquals(1, store.load("a")!!.headerGeneration) + assertEquals(0, savedSettings().headerGeneration) + // A new process reads settings.json, which still holds generation 0. + val relaunched = QueueSettingsStore(File(root, "settings.json")) + QueueController(store, journal, relaunched, events, scheduler, { it in running }, { now }).resume() + assertEquals("it holds new headers it never tried", EntryState.QUEUED, store.load("a")!!.state) + } + @Test fun `the next settings call saves a value that is behind`() { withSettingsUnwritable { controller.setWifiOnly(true) } diff --git a/ios/QueueCoordinator.swift b/ios/QueueCoordinator.swift index 567927fb..a88dd8a8 100644 --- a/ios/QueueCoordinator.swift +++ b/ios/QueueCoordinator.swift @@ -600,7 +600,9 @@ final class QueueCoordinator { try store.saveSettings(settings) settingsBehind = false } catch { - reportSettingsSaveFailure(error) + // Only the first failure in a row, so retries on a full disk do not + // repeat it at every activation, as on Android. + if !settingsBehind { reportSettingsSaveFailure(error) } settingsBehind = true } } diff --git a/src/NativeRNFileUploader.ts b/src/NativeRNFileUploader.ts index 9067a924..ae3f1316 100644 --- a/src/NativeRNFileUploader.ts +++ b/src/NativeRNFileUploader.ts @@ -67,8 +67,9 @@ export interface Spec extends TurboModule { // when it stops being paused. // // pause, resume, setWifiOnly, updateHeaders and configure apply the new - // settings in memory, then save them best-effort. A failed save does not - // reject: native logs it and saves again at the next settings call and + // settings in memory, then save them best-effort. A failed settings save + // does not reject (on Android, a failed entry save in pause, resume or + // updateHeaders still rejects E_STORAGE): native logs it and saves again at the next settings call and // when the app becomes active or leaves the foreground, while the process // lives. A new process reads the last value that reached the disk. pause(scope: CodegenTypes.UnsafeObject): Promise; @@ -90,8 +91,9 @@ export interface Spec extends TurboModule { // Merges the patch into every entry not yet forgotten and bumps a header // generation. The patch also replaces same-named headers a part carries. A 401/403 from an attempt issued under an older generation // re-issues at once instead of parking. Parking emits one 'state' event per - // entry. Rejects E_INVALID for a header native cannot send, and nothing - // else. + // entry. Rejects E_INVALID for a header native cannot send. On Android, + // also rejects E_STORAGE when an entry file cannot be saved, after every + // other entry is patched and scheduled. updateHeaders(patch: CodegenTypes.UnsafeObject): Promise; // Synchronous. Serialized from the in-memory index that native keeps // current on every state change, never from disk. Returns every entry not diff --git a/src/index.ts b/src/index.ts index dfc53464..9e3de45e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -197,8 +197,10 @@ export const createUploadClient = ({ * same-named headers a part carries. This is how a fresh token * reaches requests that stalled on 401. Native bumps a header generation, so * a 401 from an attempt issued under the old headers re-issues at once - * instead of parking. Rejects only for a header native cannot send - * (`E_INVALID`); a failed save still applies the patch. + * instead of parking. Rejects `E_INVALID` for a header native cannot + * send. A failed settings save does not reject. On Android, an entry file + * that cannot be saved rejects `E_STORAGE` after the other entries are + * patched. */ const updateHeaders = (patch: Record): Promise => native.updateHeaders(patch);