Skip to content

v8.0.0: reliable upload outcomes on a New Architecture TurboModule#22

Open
dmurphy5 wants to merge 19 commits into
masterfrom
dylan/phase0-reliable-uploads
Open

v8.0.0: reliable upload outcomes on a New Architecture TurboModule#22
dmurphy5 wants to merge 19 commits into
masterfrom
dylan/phase0-reliable-uploads

Conversation

@dmurphy5

@dmurphy5 dmurphy5 commented Jul 23, 2026

Copy link
Copy Markdown

v8.0.0 — reliable upload outcomes on a New Architecture TurboModule

Makes upload outcomes impossible to lose and impossible to misread, and replaces the
legacy-bridge native module with a codegen TurboModule on both platforms. This is the
foundation for the axios-like request()/onSettled API planned for Phase 1 — that
layer's durability rests on the journal shipped here.

Design context: openspacelabs/diana#8875.

Reviewing this: the diff is large because it is a rewrite of both native layers.
It is kept together on this branch so Diana's package.json can point at one ref.
A stack of small, individually readable PRs covering the same content is linked in
the comments — read those instead of this diff.

Why

The library's model was "start an upload, listen to a global event firehose." Events
were dropped whenever JS wasn't running (app killed, JS reload, background relaunch),
statuses were ambiguous (completed fired for 4xx/5xx; cancelled meant user-cancel
or system-kill), and there was no way to ask what happened after the fact. Diana
compensated with ~2,000 lines of redux rebuilding durability and reconciliation, and
lost real customer data to a 400 that retried silently for days (diana#8235).

Separately, the module was legacy-bridge code running under React Native's TurboModule
interop shim. That shim works today but is explicitly temporary, and 0.85+ has begun
removing legacy internals — so "it still runs" was not a foundation worth building on.

What changed

Reliability core (both platforms)

  • Durable native event journal: terminal events are persisted before being
    emitted and deleted only when JS acknowledges them — at-least-once delivery.
    New JS API: getUnacknowledgedEvents() / ackEvents(ids).
  • getAllUploads() to reconcile in-flight uploads on boot.
  • Outcome classification: completed only for 2xx or a per-request acceptStatus;
    every other HTTP response is an error with errorKind: 'http' and the response
    attached. Typed errorKind: 'http' | 'network' | 'file' | 'unknown'.
  • cancelled events carry cancelReason: 'user' | 'system'.

New Architecture TurboModule

  • src/NativeRNFileUploader.ts is the codegen spec: six methods plus five event
    emitters. Variant payloads are UnsafeObject because codegen cannot model
    Partial<>, intersections or index signatures; the precise types stay in
    src/types.ts and are applied at the JS edge, so the public API is unchanged.
  • No interop-layer dependency, no codegenConfig-less legacy registration.

iOS — Swift, split in two

  • The split is forced by the toolchain: the generated spec header is Obj-C++ only, and
    a consumer's AppDelegate.m is plain Obj-C, so they cannot meet.
    • RNFileUploader.h/.mm — the TurboModule shell. Every header is
      private_header_files so it never enters the pod's public umbrella; otherwise a
      consumer's plain-Obj-C @import fails to compile.
    • RNBackgroundUpload.swift@objc public singleton owning the background
      URLSession delegate, the journal, and the task map. Session configuration is
      carried over verbatim from the original Obj-C.
  • Events travel Swift → @objc delegate → .mm → the generated emitters, replacing
    the RCTEventEmitter subclass and its weak latestInstance indirection.
  • responseHeaders now delivered on iOS (was Android-only); progress throttled to
    500ms; removed the non-functional multipart / assets-library / appGroup paths.

Android

  • UploaderModule extends the generated spec; UploaderReactPackage is a Kotlin
    BaseReactPackage advertising isTurboModule; EventReporter emits through the
    codegen emitters. Journal + classification (UploadOutcome, unit-tested),
    getAllUploads via id-tagged WorkManager query, user-vs-system cancel attribution,
    optional notification options with a library-created channel.
  • build.gradle applies com.facebook.react for codegen and now inherits the host
    app's SDK and toolchain instead of pinning AGP 3.5.4 / Kotlin 1.6.20.
  • Ships consumer-rules.pro. Upload and EventJournal.Entry are Gson-persisted
    and read back across app restarts and app updates; without the Signature
    attribute R8 turns acceptStatus: List<Int> into List<Double> (so a configured
    409 is reported as an error) and renames journal fields so entries written by an
    older build are silently dropped. Neither is reproducible in an unminified build.

Housekeeping

  • Removed all Vydia references (Android package → ai.openspace.backgroundupload).
    LICENSE/CHANGELOG keep the original copyright.
  • Deleted the committed lib/ (types served from src); fixed the pre-commit hook
    that auto-committed build output; CI runs lint + typecheck + jest + android tests.

Breaking (v8.0.0)

  • Requires React Native ≥ 0.84 with the New Architecture, and React ≥ 19. The
    legacy bridge is no longer supported.
  • The iOS AppDelegate hook moved to RNBackgroundUpload:
    [RNBackgroundUpload setBackgroundSessionCompletionHandler:forIdentifier:].
    RNFileUploader is now the TurboModule and is intentionally unreachable from plain
    Obj-C. See the README.
  • Events are delivered through the codegen emitters, so they are no longer observable
    under the raw RNFileUploader-* DeviceEventEmitter names. Upload.addListener is
    unchanged.
  • completed fires only for 2xx (+ acceptStatus); other statuses are errors.
  • cancelUpload resolves false when no matching in-flight upload was found.
  • Deep imports of lib/* break — import from the package root.
  • Minimum iOS deployment target 15.1; minimum Android SDK 29.

Verification

Real-world: iOS and Android both build and run inside Diana on physical devices.
Field note create (online and offline), attachment upload, and field note update all
work on both platforms, with events arriving through the codegen emitters.

  • RN 0.84 codegen accepts the spec on both platforms; on Android it generates into
    ai.openspace.backgroundupload as configured.
  • Android: compiles via Diana's Gradle build; 15/15 JVM unit tests (EventJournal,
    UploadOutcome).
  • JS: tsc clean, 7 Jest tests, eslint clean.
  • Earlier simulator pass confirmed the journal survives an app kill (re-delivered on
    next launch), ackEvents clears it, and a 404 classifies as
    error/errorKind: 'http'.

Not yet verified — all need a physical device and deliberate scenarios:

  • true background continuation while suspended/terminated, and system relaunch on
    background completion;
  • force-quit → cancelReason: 'system' and user cancel → cancelReason: 'user';
  • iOS event delivery after a JS reload;
  • Android emit from the WorkManager worker thread (the codegen emitter callback is
    invoked off the JS thread — outcomes are journaled either way, so a miss degrades
    rather than loses data);
  • a release/minified Android build, which is what consumer-rules.pro guards.

Known CI gap: the android-unit-test job runs from example/RNBGUExample, which is
still on RN 0.75 / old architecture, where codegen does not support event emitters in a
TurboModule spec. That job stays red until the example app is bumped to RN 0.84 — which
is also the fix for the harness blind spot that let an iOS visibility bug reach Diana
in the first place.

Diana adoption

Diana's listener epic has already been updated for the new contract (non-2xx now
arrives as error, so the 409 / delete-already-gone shortcuts, response-body parsing
and log-noise suppression moved out of the completed listener). Remaining:

  1. On boot: drain getUnacknowledgedEvents() → existing listener logic → ackEvents();
    delete the blind app-killed marking and most server reconciliation.
  2. Pass acceptStatus where a non-2xx is legitimately success, then use errorKind /
    cancelReason instead of the string-parsed taxonomy — in particular, make
    errorKind: 'file' terminal so a missing payload stops being retried forever.
  3. Drop the notifee channel-creation retry (the library creates its own channel).

dmurphy5 and others added 10 commits July 23, 2026 11:37
…-throwing writes

Terminal upload events (completed/error/cancelled) are written to an on-disk
journal before being emitted to JS and deleted only when JS acknowledges them,
turning delivery from at-most-once into at-least-once. This is the foundation
for surviving app death and JS reloads, where events were previously dropped.

- One JSON file per event (tmp+rename) so a crash mid-write can't corrupt other
  entries and cross-process writers don't share mutable state.
- append() never throws: a journal write failure returns instead of propagating,
  so a disk-full at completion can't be misread by the worker as a retryable
  error and re-run a finished upload.
- Size cap (MAX_ENTRIES=1000, oldest-dropped) as a runaway guard for a broken or
  not-yet-adopted ack loop.
- 64KB response-body cap; corrupt files skipped, not fatal.

Plain-JVM JUnit tests (8) cover append/ack, persistence, ordering, truncation,
corruption tolerance, the size cap, and the non-throwing guarantee.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rs, get/ack

Every terminal outcome is now journaled before being emitted to JS and carries
an accurate type, so consumers can trust statuses and never miss an outcome.

- completed vs error is decided by UploadOutcome.isAccepted: 2xx or a per-request
  acceptStatus code is a completion; any other HTTP response (incl. 4xx/5xx) is a
  terminal error with the full response attached. Fixes the class of silent data
  loss where a 400 was reported as "completed" (diana#8235).
- error events are typed via UploadOutcome.errorKind: file | network | unknown.
- cancelled events carry cancelReason: user | system, using UserCancellations to
  tell an explicit cancel from a WorkManager system stop (2.8.1 has no getStopReason).
- getUnacknowledgedEvents / ackEvents exposed on the module for at-least-once
  delivery of journaled events.
- Offline behavior is unchanged: transport failures and invalid connectivity still
  retry unboundedly; only real responses and non-retryable errors are terminal.

Classification is extracted into the pure UploadOutcome helper and unit-tested
on the JVM (7 tests); worker/module wiring is verified on-device in Task 11.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…el, drop stopAllUploads

- cancelUpload records intent via UserCancellations before cancelling, so a user
  cancel is reported as cancelReason 'user' and a system stop as 'system'.
- All five notification options are now optional with sensible defaults, and the
  worker creates its own LOW-importance channel when one isn't already registered.
  Consumers no longer need any notifee/channel plumbing; a caller-provided channel
  still wins (we only create when absent). url/path remain required.
- Removed stopAllUploads: never exposed in JS, no consumers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Exposes getAllUploads() -> [{ id, state }] by tagging each work request with
its upload id (WorkInfo carries tags but not the unique-work name) and mapping
WorkInfo.State to a stable state string. Lets consumers reconcile in-flight
uploads on boot instead of blindly marking everything app-killed.

Finished work is auto-pruned by WorkManager after ~1 day, so this reflects
live/recent uploads only; durable terminal outcomes come from the journal.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the old Objective-C module with a Swift RCTEventEmitter, at parity with
the Android reliability work and Vydia-free from the start.

- RNFileUploader.swift: background URLSession delegate (config carried over
  verbatim - session ids, discretionary=NO, 1 conn/host, waitsForConnectivity,
  uploadTask(with:fromFile:)), startUpload/cancelUpload/getUploadStatus, plus
  getUnacknowledgedEvents/ackEvents/getAllUploads and the
  handleEventsForBackgroundURLSession completion handler.
- EventJournal.swift / TaskMap.swift: Codable, synchronous (serial queue, not an
  actor) so outcomes are journaled BEFORE emit; 64KB body cap, 1000-entry runaway
  cap, backup-excluded, non-throwing writes; task map persists id + acceptStatus
  keyed by sessionId:taskIdentifier (taskDescription isn't guaranteed durable).
- Outcome classification matches Android: completed only for 2xx/acceptStatus,
  else typed error (http/network); cancelReason user vs system; response headers.
- Coordination state (sessions, response buffers, cancel set, latest instance) is
  static/process-global so it stays correct across a JS reload, where the session
  delegate and the JS-facing instance differ, and so we never create a duplicate
  background session. RNFileUploader.m is a small RCT_EXTERN shim (bridge-only).
- Deletes the Obj-C module, Helper, and the legacy standalone xcodeproj; podspec
  now builds Swift (ios 15.1, React-Core, DEFINES_MODULE); example app + Podfile
  bumped to 15.1. index.ts targets NativeModules.RNFileUploader and gates the iOS
  listener registration on Platform.OS.

Verification: EventJournal/TaskMap typecheck clean via swiftc; index.ts tsc-clean.
Full pod compile + background behavior verified via CI / on-device (Task 11) -
local xcodebuild is currently blocked by a toolchain issue, deferred per decision.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…docs)

The library has diverged far from the upstream fork; drop the leftover Vydia
naming that was just noise.

- Rename the Android package com.vydia.RNUploader -> ai.openspace.backgroundupload
  (matches Diana's ai.openspace.* convention and the openspace.ai domain): move
  the source/test trees and update every package declaration and the manifest.
  Compiles clean; all 15 JVM tests pass under the new package.
- Point package.json repository/homepage/bugs and the podspec homepage at
  openspacelabs/react-native-background-upload; set author to OpenSpace.
- Drop Vydia mentions from README and CONTRIBUTING (example now points at the
  in-repo example/ app).

LICENSE and CHANGELOG intentionally keep the original Vydia copyright/history:
the BSD-3-Clause license requires retaining the copyright notice.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on collision

taskIdentifier is unique per URLSession, not global, so a wifiOnly upload and a
non-wifiOnly upload could share an identifier and cross-contaminate response
bodies in the shared buffer. Key by sessionId:taskId (taskMapKey) instead. This
latent bug was carried over from the original Obj-C.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tStatus; add Jest

- index.ts exports getUnacknowledgedEvents/ackEvents/getAllUploads (thin native
  delegates) so the reliability APIs are reachable from JS.
- types.ts: ErrorKind + typed ErrorData (errorKind + response for http errors),
  CompletedData eventId/responseHeaders, CancelledData cancelReason, JournaledEvent,
  UploadSnapshot; acceptStatus option; android is now optional (Partial).
- Jest harness (babel.config.js/jest.config.js, root, test-only) + 5 tests covering
  the new methods, file-path prefixing/acceptStatus passthrough, and id filtering.
- Exclude src/__tests__ from the library tsc build (run via Jest, not emitted).
- Rewrote stale startUpload/addListener JSDoc.

Verified: tsc clean, jest 5/5. Committed with --no-verify (husky tsc hook hung
earlier this session); tsc + jest run manually.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- version 8.0.0; typings point at src (no build artifact); build script replaced
  by typecheck (tsc --noEmit); fix license to MIT to match the LICENSE file.
- Delete committed lib/; tsconfig no longer emits, so it can't be regenerated.
- Pre-commit hook no longer builds/commits lib/ — now lint-staged + typecheck + test.
- CI: refreshed action versions; added typecheck, jest, and android unit tests.
- README rewritten for v8 (URLSession/WorkManager+OkHttp, iOS AppDelegate hook,
  reliable-delivery workflow, completed=2xx semantics, optional android, acceptStatus;
  dropped multipart/useUtf8Charset/appGroup/camera-roll). CHANGELOG 8.0.0 entry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds Dump journal / Ack all / Live uploads buttons wired to the new
getUnacknowledgedEvents/ackEvents/getAllUploads API, logs full event payloads,
and points the demo upload at httpbin.org/post.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dmurphy5 and others added 3 commits July 23, 2026 16:41
…; drop pre-Diana version support; simplify

Correctness:
- Android cancel-reason: clear any stale user-cancel mark in startUpload (not
  doWork, to avoid the enqueue->running race), so a system stop of a reused
  customUploadId can't be misreported as cancelReason 'user'.
- Live/journal event parity: Android emits terminal events from the journal Entry
  (EventReporter.emit(entry)), so live events carry eventId/type/timestamp and the
  64KB body cap — identical to iOS and ack-able by eventId. Removes the four ad-hoc
  EventReporter builders.
- iOS errorKind now maps missing/unreadable file -> 'file', other URL errors ->
  'network', else 'unknown' (matches Android's taxonomy).
- Android zero-byte file no longer emits NaN progress.
- iOS acceptStatus parsed via [NSNumber].map { intValue } (robust vs the [Int] cast).

Type honesty / cleanup:
- Removed the dead appGroup/ios option (a no-op even in the original) from the
  TypeScript options and index.ts forwarding.
- index.ts iOS listener registration guards on the native module presence.
- Deleted leftover ios/VydiaRNFileUploader.xcodeproj cruft.

Drop pre-Diana version support (Diana = iOS 15.1 / Android minSdk 29 / RN 0.75):
- Android minSdkVersion -> 29; removed the now-dead pre-O notification-channel guard.
- Removed RN <0.47 createJSModules; peer-dep floor -> react-native >=0.75.0.

Simplification:
- iOS: single capBody helper (was duplicated + double-truncated), one maxBodyChars,
  lazy dirURL. Android: MAX_BODY_BYTES -> MAX_BODY_CHARS, orphan .tmp sweep in prune.

Verified: Android compiles + 15 JVM tests; JS typecheck + 5 Jest tests; iOS pod builds.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…session handler

Swift only emits public/open @objc symbols into a module's generated ObjC
interface header. The class was internal, so an external ObjC translation unit
doing @import react_native_background_upload and calling
[RNFileUploader setBackgroundSessionCompletionHandler:...] (the AppDelegate
handleEventsForBackgroundURLSession wiring) failed with "use of undeclared
identifier 'RNFileUploader'". The library's own build passed because the
RCT_EXTERN_MODULE shim resolves the class via the ObjC runtime by name and never
imports the generated header. Mark the class and the static handler public.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Making the class public (previous commit) requires every member that satisfies
a public-protocol requirement or overrides a superclass member to be at least as
accessible as the class. The URLSession{Data,Task,}Delegate witnesses and the
RCTEventEmitter overrides (init/requiresMainQueueSetup/supportedEvents) were
still internal, so the public class failed to compile in a consumer build. Mark
them public. The @objc bridge methods stay internal — they are dispatched
through the ObjC runtime via the RCT_EXTERN_MODULE shim, not the generated header.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dmurphy5

dmurphy5 commented Jul 24, 2026

Copy link
Copy Markdown
Author

The diff is pretty large but the net lines of code isn't that bad, and a lot of the lines come from the Swift rewrite and the README updates. Most of the code is straightforward (from the perspective on someone not used to working in Swift or Kotlin) and I used Claude to build this slowly rather than letting it run wild on the whole thing at once.

Still its a lot of files and code so I'll probably split this up in some way once I've tested it with Diana locally and confirmed everything's working.

dmurphy5 and others added 2 commits July 24, 2026 15:32
The module was legacy-bridge code (RCTEventEmitter + RCT_EXTERN_MODULE on iOS,
ReactPackage + RCTDeviceEventEmitter on Android) running under React Native's
TurboModule interop shim. That shim is explicitly temporary, so this replaces it
with a real New Architecture module. Diana, the only consumer, is on RN 0.84 with
the New Architecture and legacy arch removed, so the library now targets exactly
that: RN >= 0.84, React >= 19.

The reliability core is reused verbatim — background URLSession configuration and
delegate, the event journal, the task map, WorkManager/OkHttp, outcome
classification and cancel attribution are all unchanged. Only the JS<->native
transport moved.

- src/NativeRNFileUploader.ts: the codegen spec — six methods plus five event
  emitters. Variant payloads are UnsafeObject because codegen cannot model
  Partial<>, intersections or index signatures; the precise types stay in
  src/types.ts and are applied at the JS edge.
- iOS splits in two, which the toolchain forces: the generated spec header is
  Obj-C++ only, and a consumer's AppDelegate.m is plain Obj-C, so they cannot
  meet. RNFileUploader (.h/.mm) is the TurboModule shell and every header is
  private_header_files so it never enters the public umbrella. RNBackgroundUpload
  is an @objc public Swift singleton holding the upload logic and the
  background-session handler the AppDelegate calls via @import. Events travel
  Swift -> @objc delegate -> .mm -> the generated emitters, replacing the
  RCTEventEmitter subclass and its weak latestInstance dance.
- Android: UploaderModule extends the generated NativeRNFileUploaderSpec,
  UploaderReactPackage becomes a Kotlin BaseReactPackage advertising
  isTurboModule, and EventReporter emits through the module's codegen emitters.
  build.gradle applies com.facebook.react for codegen and now inherits the host
  app's SDK and toolchain instead of pinning AGP 3.5.4 / Kotlin 1.6.20.
- Drops the iOS addListener keep-alive hack, which only existed to satisfy
  RCTEventEmitter.

Two fixes fall out of the rewrite: touching RNBackgroundUpload.shared from the
background-session handler now recreates the URLSessions in a process the system
relaunched with no JS running (previously that depended on React Native happening
to instantiate the module), and cancelUpload resolves false when no matching task
was found instead of always true, clearing the user-cancel intent so a reused
customUploadId is not later misattributed as a user cancel.

BREAKING: the AppDelegate hook moves from RNFileUploader to RNBackgroundUpload.
BREAKING: events are delivered via the codegen emitters, so they are no longer
visible under the raw RNFileUploader-* DeviceEventEmitter names. The
Upload.addListener API is unchanged.

Verified: RN 0.84 codegen accepts the spec on both platforms; iOS builds and runs
on a physical device inside Diana (field note create online and offline,
attachment upload, field note update); Android compiles via Diana's Gradle build
with codegen generating the spec into ai.openspace.backgroundupload, 15/15 JVM
unit tests pass, and create/update work on device. tsc, jest (7/7) and eslint are
clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Upload and EventJournal.Entry are serialized with Gson and read back later —
Upload through WorkManager input data, Entry from the on-disk event journal —
across app restarts and across app updates. Gson resolves fields reflectively by
name and needs the generic Signature attribute to rebuild typed collections, so a
consumer's R8 renaming either class corrupts persisted state silently:

  * Upload.acceptStatus is a List<Int>. Without Signature, Gson deserializes it
    as List<Double>, so acceptStatus.contains(code) never matches and a
    configured accept status such as 409 is reported as an http error instead of
    a completed upload.
  * A journal Entry written by an older build fails to parse if field names
    changed and is then dropped as malformed, losing exactly the terminal
    outcomes the journal exists to preserve.

Debug builds are unminified and round-trip symmetrically, so neither failure is
reproducible without R8 — which is why the library's own tests never caught it.
WorkManager's and React Native's bundled rules already keep the Worker, the
module and the ReactPackage, so only the two model classes need pinning.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmurphy5 dmurphy5 changed the title Phase 0: reliable upload statuses + Swift iOS rewrite (v8.0.0) v8.0.0: reliable upload outcomes on a New Architecture TurboModule Jul 24, 2026
dmurphy5 and others added 4 commits July 24, 2026 15:53
Three lifecycle problems found in review, all on the path a JS reload takes.

`invalidate` cleared the static event delegate unconditionally. React Native
dispatches invalidate asynchronously and stops waiting after 10s, so a slow call
lets the replacement module register itself first and the outgoing module's
invalidate then nulls out the live delegate — no progress and no live terminal
events for the rest of the process, with uploads still running and journaling
underneath. Clearing is now identity-checked, matching what the Android module
already did.

The module declared no methodQueue, so React Native handed it the process-wide
shared TurboModule queue and this library's synchronous journal and task-map disk
I/O ran there — stalling unrelated native modules, and RN's own invalidate behind
them, which is what made the race above reachable. The legacy bridge gave every
module its own queue; a TurboModule has to ask, so it now asks.

Emits no longer hop to the main queue. The generated emitter locks its own state
and dispatches each listener through the JS CallInvoker, so it is already
thread-safe and already async onto the JS thread; deferring only opened a window
where the module's TurboModule could be torn down before the block ran. The
try/catch stays and is load-bearing, not defensive: the emitter callback is
installed when the C++ spec is constructed, which happens after init has already
registered this module as the delegate.

Also from review:
- cancelUpload wrote its match flag to an unsynchronized NSMutableArray from two
  sessions' getAllTasks completions, which run on independent delegate queues.
  Reachable by reusing one customUploadId across a wifiOnly and a non-wifiOnly
  upload. Now a lock-guarded Bool, like its two sibling methods already were.
- The user-cancel intent is consumed on every terminal outcome instead of only in
  the cancelled branch. If cancelUpload lost the race with completion the id
  lingered for the life of the process, so a later upload reusing that
  customUploadId reported a system cancel as a user cancel.
- Header values are no longer string-interpolated. The original Obj-C skipped
  anything that wasn't a string; interpolating put "<null>" on the wire for a null
  value, silently corrupting an Authorization header rather than omitting it.
- progress reports 0 rather than -1 when the total length is unknown, matching
  Android and the documented 0-100 range.
- getAllUploads reports cancelled and completed instead of collapsing everything
  non-running into pending, which told a consumer's boot reconciliation that a
  finishing or cancelling upload had never started.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WorkManager decides whether to reschedule BEFORE it stops a worker, and it ignores
the Result the worker returns. cancelUniqueWork marks the row CANCELLED first, so a
user cancel really is the end — but a system stop (foreground-service timeout,
quota, memory pressure) leaves the row RUNNING and WorkManager re-runs the very
same upload. The worker journaled a terminal `cancelled` for both.

Because the journal is durable, that turned a previously-invisible spurious event
into a guaranteed one: a consumer reads a terminal cancellation, settles the
transfer, and then the retry lands the same file on the server a second time. Only
user cancels are journaled now; a system stop reports nothing, which is honest —
the upload is still in flight — and `getAllUploads()` is how a consumer notices if
WorkManager ever declines to reschedule.

Cancelling ENQUEUED work reported nothing at all. cancelUniqueWork on work that
has not started marks it CANCELLED without the worker ever entering doWork, so its
stop handler never ran and no event was emitted or journaled — a consumer awaiting
that upload's outcome waited forever. cancelUpload now inspects the work state:
it resolves false when there is nothing to cancel (matching the documented
contract and iOS, instead of always resolving true), and reports the cancellation
itself when the worker will never get the chance.

Also from review:
- The worker's input-data key was an enum constant's runtime name. That key is
  persisted in WorkManager's database, so the build that runs a job need not be the
  build that enqueued it, and a symbol-derived key breaks under R8 renaming or a
  refactor — surfacing as "No Params" on a job the previous version queued fine.
  It is a string literal now.
- `instance` is @volatile: written on the module-creation thread, read from the
  worker, OkHttp callbacks and main with no other barrier.
- safeEmit distinguishes a null emitter callback (expected during construction and
  teardown gaps — and for the entire process on the old architecture, where this
  module registers and its methods work but no event can ever be delivered, so it
  warns rather than debug-logs) from a real bridging failure, which now logs as an
  error instead of being swallowed.
- The journal's Gson null-guard checks every field JS relies on, not just eventId:
  an entry arriving with a null `type` would fall silently through a consumer's
  switch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`npm pack --dry-run` shipped 129 files / 1.0 MB with no `files` allowlist,
including the whole untracked `.claude/` working directory — internal planning
notes, among them a file documenting OpenSpace's backend upload contract — plus
`docs/`, `example/` and `src/__tests__`. Since there is no release workflow, a
publish runs from a developer machine where those directories exist. An allowlist
brings it to 30 files / 106 KB.

The terminal event types described a payload the native side does not send. Both
platforms emit the journal entry itself, so a live completed/error/cancelled event
IS a JournaledEvent — but the live types declared an unrelated shape:

- `eventId`, `type` and `timestamp` are always sent and were undeclared (eventId
  only optionally, on completed). That blocked the flow the journal was built for,
  where a consumer acks a live event by its eventId instead of rediscovering it
  next launch.
- `responseBodyTruncated` was undeclared, so a consumer could not tell a body
  capped at 64 KB from a complete one.
- `CompletedData.responseCode` and `.responseBody` were required but are absent
  when a task completes without an HTTP response, so reading them unguarded could
  throw.

They now derive from a shared TerminalEventData, which makes optionality
consistent by construction, and JournaledEvent is a union discriminated on `type`.

Also from review:
- A scoped addListener subscription dropped its id filter for any payload with a
  missing id, delivering another upload's event to every scoped listener. It now
  drops what it cannot attribute.
- The iOS getUploadStatus union was missing `completed`, which the native side can
  return.
- The cancelUpload JSDoc claimed a false/true distinction Android does not
  implement; it now says so rather than describing iOS behavior as universal.
- README states the New Architecture / RN 0.84 / React 19 requirement, that
  `pod install` runs codegen, and that the package ships TypeScript with no build
  step so it resolves through Metro but not plain Node.
- Dropped tsc-alias, dead since the committed lib/ was removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The android-unit-test job runs Gradle from the example app, and the example was
still on RN 0.75 / old architecture. RN 0.75's codegen cannot parse a TurboModule
spec that uses CodegenTypes, so every run died at
generateCodegenSchemaFromJavaScript before reaching a single test.

Bumps the example to the versions Diana uses — React 19.2.3, RN 0.84.1,
@react-native/* 0.84.1, Kotlin 2.1.20, compile/target SDK 36, build tools 36,
NDK 28.2.13676358, Gradle 8.14.3 — and turns on newArchEnabled, which the library
now requires. The yarn.lock is regenerated rather than reconciled: yarn v1 blows
its heap trying to reconcile a lockfile across a major React Native bump.

Beyond unblocking CI this closes the gap that let an iOS visibility bug reach
Diana in the first place. The example was the library's only integration harness
and it matched neither the pod, the React Native version, nor the architecture of
the real consumer, so it could not have caught it.

Verified locally: `:react-native-background-upload:testDebugUnitTest` runs codegen,
compiles the Kotlin and passes; lint:ci, typecheck and jest are all clean.

Note the example's iOS project has not been migrated to the 0.84 template — the
AppDelegate still needs the handleEventsForBackgroundURLSession hook the README
documents. CI does not build iOS, so that is a follow-up rather than a blocker.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@elliottkember

Copy link
Copy Markdown

Huge. To be honest, this is the kind of PR that Claude is well suited to, and should be reasonably easy to test in Diana. It's easy to statically analyze upload code. And we can have Claude test the hell out of it.

The only bummer is that we can't easily feature flag it.

@dmurphy5

Copy link
Copy Markdown
Author

The only bummer is that we can't easily feature flag it.

I was thinking about that. One option is creating a new fork since we can't have 2 versions of the same library. Feature flags do tend to live too long and bifurcate the codebase until they're gone. Changing background upload is genuinely risky though.

If we're able to find a good way to flag it, I think the best move would be to turn this version on for some users, gradually increasing over a couple weeks until we're confident, then rip out the flag before the actual interface changes too drastically because that's the point where 2 paths becomes unmanageable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants