Bound flush() retries so a persistently-retriable upload can't hang teardown

ypopovych · claude · ypopovych · commit a37f6915cb92 · 2026-07-09T13:20:52.000+01:00
The synchronous `flush()` on the shutdown path retried a `.retry` upload
result in an unbounded loop. A persistently retriable server (503/500/
408/429) or a downed network — both surface as `needsRetry` — would loop
forever, hanging process teardown.

Cap flush retries at 3 (1 initial attempt + 3 retries). On giving up,
report failure and leave the undelivered batch on disk for a later run.
The background worker's indefinite cross-tick retry behaviour is
unchanged; only the bounded shutdown flush is affected.

Adds a regression test that a persistently-retriable upload returns
failure instead of hanging, attempts exactly 4 uploads, and leaves the
batch on disk.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/Sources/EventsExporter/Upload/DataUploadWorker.swift b/Sources/EventsExporter/Upload/DataUploadWorker.swift
@@ -91,6 +91,13 @@ internal final class DataUploadWorker: DataUploadWorkerType {
         queue.sync { fileReader.update(dataFormat: dataFormat) }
     }
 
+    /// Maximum number of retries per batch during a synchronous `flush()`.
+    /// Unlike the background worker (which retries indefinitely across ticks),
+    /// `flush()` runs on the shutdown path and must be bounded: a persistently
+    /// retriable server (503/500/408/429) or a downed network — both map to
+    /// `needsRetry` — would otherwise loop forever, hanging process teardown.
+    private static let maxFlushRetries = 3
+
     /// Drains all pending batches synchronously. Holds the upload queue for
     /// the duration so the periodic worker can't interleave reads with the flush.
     func flush() throws -> Bool {
@@ -103,6 +110,7 @@ internal final class DataUploadWorker: DataUploadWorkerType {
                     break
                 }
                 var status: UploadResult
+                var retries = 0
                 repeat {
                     status = upload(data: batch.data)
                     switch status {
@@ -112,6 +120,15 @@ internal final class DataUploadWorker: DataUploadWorkerType {
                     case .failed:
                         result = false
                     case .retry:
+                        retries += 1
+                        guard retries <= Self.maxFlushRetries else {
+                            // Give up rather than hang teardown; leave the batch
+                            // on disk for a later run to pick up.
+                            log.print("[\(featureName)] flush giving up after \(Self.maxFlushRetries) retries")
+                            result = false
+                            status = .failed
+                            break
+                        }
                         Thread.sleep(forTimeInterval: delay.current)
                     }
                 } while status.isRetry
diff --git a/Tests/EventsExporter/Upload/DataUploadWorkerTests.swift b/Tests/EventsExporter/Upload/DataUploadWorkerTests.swift
@@ -304,6 +304,45 @@ class DataUploadWorkerTests: XCTestCase {
 
         worker.stop()
     }
+
+    func testFlushGivesUpAfterMaxRetriesInsteadOfHanging() throws {
+        // A server that always asks to retry (e.g. persistent 503) used to make
+        // `flush()` loop forever, hanging the synchronous shutdown flush.
+        let uploadCount = LockedInt()
+        var mockDataUploader = DataUploaderMock(uploadStatus: .mockWith(needsRetry: true))
+        mockDataUploader.onUpload = { uploadCount.increment() }
+
+        writer.write(value: ["k1": "v1"])
+        writer.queue.sync {}
+
+        let worker = DataUploadWorker(
+            fileReader: reader,
+            dataUploader: mockDataUploader,
+            delay: MockDelay(),
+            featureName: .mockAny(),
+            priority: .userInteractive,
+            log: Log()
+        )
+
+        // When: this must return (not hang) and report failure.
+        let flushed = try worker.flush()
+
+        // Then
+        XCTAssertFalse(flushed, "A persistently-retriable upload should end in failure, not success")
+        XCTAssertEqual(uploadCount.value, 4, "1 initial attempt + 3 retries, then give up")
+        XCTAssertEqual(try temporaryDirectory.files().count, 1, "Undelivered batch is left on disk for a later run")
+
+        worker.stop()
+    }
+}
+
+/// Thread-safe integer counter for asserting on callbacks made from the worker queue.
+private final class LockedInt: @unchecked Sendable {
+    private let lock = NSLock()
+    private var _value = 0
+
+    var value: Int { lock.withLock { _value } }
+    func increment() { lock.withLock { _value += 1 } }
 }
 
 struct MockDelay: Delay {
