perf(android): move state persistence off UI thread (AN-1) + test hardening

android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java

@@ -0,0 +1,64 @@
+package cn.reactnative.modules.update;
+
+import android.util.Log;
+import androidx.annotation.Nullable;
+import com.facebook.react.bridge.Promise;
+
+import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+
+/**
+ * Runs state-persistence operations (switchVersion / markSuccess / setUuid /
+ * setLocalHashInfo) on a dedicated single background thread.
+ *
+ * These operations only read/modify SharedPreferences via a synchronous
+ * commit(); they were previously dispatched to the UI thread purely to
+ * serialize them. markSuccess in particular runs on every cold start, so doing
+ * its blocking disk write on the main thread caused jank/ANR on low-end
+ * devices. A single-thread executor preserves the same serialization guarantee
+ * while keeping the disk I/O off the UI thread.
+ *
+ * Note: reload/restart operations must still run on the UI thread and therefore
+ * keep using {@link UiThreadRunner}.
+ */
+final class StateSerialRunner {
+    interface Operation {
+        void run() throws Throwable;
+    }
+
+    // Single worker thread -> operations stay serialized in submission order,
+    // matching the previous UI-thread behavior. The thread is named so it is
+    // identifiable in thread dumps / ANR traces when diagnosing persistence.
+    private static final Executor EXECUTOR = Executors.newSingleThreadExecutor(
+        new ThreadFactory() {
+            @Override
+            public Thread newThread(Runnable r) {
+                return new Thread(r, "pushy-state-serial");
+            }
+        });
+
+    private StateSerialRunner() {
+    }
+
+    static void run(
+        @Nullable final Promise promise,
+        final String operationName,
+        final Operation operation
+    ) {
+        EXECUTOR.execute(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    operation.run();
+                } catch (Throwable error) {
+                    if (promise != null) {
+                        promise.reject(operationName + " failed", error);
+                    } else {
+                        Log.e(UpdateContext.TAG, operationName + " failed", error);
+                    }
+                }
+            }
+        });
+    }
+}

android/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.java

@@ -149,7 +149,7 @@ public static void setNeedUpdate(
         final Promise promise
     ) {
         final String hash = options.getString("hash");
-        UiThreadRunner.run(promise, "switchVersionLater", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(promise, "switchVersionLater", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setNeedUpdateInternal(updateContext, hash);
@@ -160,7 +160,7 @@ public void run() {
 
     public static void setNeedUpdate(final UpdateContext updateContext, final ReadableMap options) {
         final String hash = options.getString("hash");
-        UiThreadRunner.run(null, "switchVersionLater", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(null, "switchVersionLater", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setNeedUpdateInternal(updateContext, hash);
@@ -173,7 +173,7 @@ private static void markSuccessInternal(UpdateContext updateContext) {
     }
 
     public static void markSuccess(final UpdateContext updateContext, final Promise promise) {
-        UiThreadRunner.run(promise, "markSuccess", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(promise, "markSuccess", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 markSuccessInternal(updateContext);
@@ -183,7 +183,7 @@ public void run() {
     }
 
     public static void markSuccess(final UpdateContext updateContext) {
-        UiThreadRunner.run(null, "markSuccess", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(null, "markSuccess", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 markSuccessInternal(updateContext);
@@ -200,7 +200,7 @@ public static void setUuid(
         final String uuid,
         final Promise promise
     ) {
-        UiThreadRunner.run(promise, "setUuid", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(promise, "setUuid", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setUuidInternal(updateContext, uuid);
@@ -210,7 +210,7 @@ public void run() {
     }
 
     public static void setUuid(final UpdateContext updateContext, final String uuid) {
-        UiThreadRunner.run(null, "setUuid", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(null, "setUuid", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setUuidInternal(updateContext, uuid);
@@ -235,7 +235,7 @@ public static void setLocalHashInfo(
         final String info,
         final Promise promise
     ) {
-        UiThreadRunner.run(promise, "setLocalHashInfo", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(promise, "setLocalHashInfo", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setLocalHashInfoInternal(updateContext, hash, info);
@@ -249,7 +249,7 @@ public static void setLocalHashInfo(
         final String hash,
         final String info
     ) {
-        UiThreadRunner.run(null, "setLocalHashInfo", new UiThreadRunner.Operation() {
+        StateSerialRunner.run(null, "setLocalHashInfo", new StateSerialRunner.Operation() {
             @Override
             public void run() {
                 setLocalHashInfoInternal(updateContext, hash, info);

cpp/patch_core/tests/patch_core_test.cpp

@@ -554,15 +554,22 @@ int main() {
       {"StateCoreSwitchToSameVersion", TestStateCoreSwitchToSameVersion},
   };
 
+  int failures = 0;
   for (const auto& test : tests) {
     try {
       test.second();
       std::fprintf(stdout, "[PASS] %s\n", test.first.c_str());
     } catch (const std::exception& error) {
       std::fprintf(stderr, "[FAIL] %s: %s\n", test.first.c_str(), error.what());
-      return 1;
+      ++failures;
     }
   }
 
-  return 0;
+  std::fprintf(
+      stdout,
+      "\n%zu tests, %d passed, %d failed\n",
+      tests.size(),
+      static_cast<int>(tests.size()) - failures,
+      failures);
+  return failures == 0 ? 0 : 1;
 }

scripts/test-patch-core.sh

@@ -6,6 +6,15 @@ BUILD_DIR="$ROOT_DIR/.tmp/patch-core-tests"
 
 mkdir -p "$BUILD_DIR"
 
+# Opt-in AddressSanitizer + UndefinedBehaviorSanitizer build. The patch core
+# processes untrusted (downloaded) patch data, so running the tests under
+# sanitizers catches memory/UB regressions. Enable with: SANITIZE=1 npm run test:patch-core
+SANITIZE_FLAGS=""
+if [ "${SANITIZE:-0}" = "1" ]; then
+  SANITIZE_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g"
+  echo "Building patch core tests with AddressSanitizer + UBSan"
+fi
+
 COMMON_INCLUDES="
   -I$ROOT_DIR/cpp/patch_core
   -I$ROOT_DIR/android/jni
@@ -14,16 +23,17 @@ COMMON_INCLUDES="
   -I$ROOT_DIR/android/jni/lzma/C
 "
 
-cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/hpatch.c" -o "$BUILD_DIR/hpatch.o"
-cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/libHDiffPatch/HPatch/patch.c" -o "$BUILD_DIR/patch.o"
-cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/file_for_patch.c" -o "$BUILD_DIR/file_for_patch.o"
-cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/LzmaDec.c" -o "$BUILD_DIR/LzmaDec.o"
-cc -Wall -Wextra $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/Lzma2Dec.c" -o "$BUILD_DIR/Lzma2Dec.o"
+cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/hpatch.c" -o "$BUILD_DIR/hpatch.o"
+cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/libHDiffPatch/HPatch/patch.c" -o "$BUILD_DIR/patch.o"
+cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/HDiffPatch/file_for_patch.c" -o "$BUILD_DIR/file_for_patch.o"
+cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/LzmaDec.c" -o "$BUILD_DIR/LzmaDec.o"
+cc -Wall -Wextra $SANITIZE_FLAGS $COMMON_INCLUDES -c "$ROOT_DIR/android/jni/lzma/C/Lzma2Dec.c" -o "$BUILD_DIR/Lzma2Dec.o"
 
 c++ \
   -std=c++17 \
   -Wall \
   -Wextra \
+  $SANITIZE_FLAGS \
   $COMMON_INCLUDES \
   "$ROOT_DIR/cpp/patch_core/tests/patch_core_test.cpp" \
   "$ROOT_DIR/cpp/patch_core/archive_patch_core.cpp" \

src/__tests__/client.test.ts

@@ -2,6 +2,13 @@ import { afterEach, describe, expect, mock, test } from 'bun:test';
 
 const importFreshClient = (cacheKey: string) => import(`../client?${cacheKey}`);
 
+const originalDev = (globalThis as any).__DEV__;
+
+afterEach(() => {
+  mock.restore();
+  (globalThis as any).__DEV__ = originalDev;
+});
+
 const createJsonResponse = (payload: unknown) =>
   ({
     ok: true,
@@ -97,6 +104,63 @@ const setupClientMocks = ({
   }));
 };
 
+const setupAndroidApkMocks = (
+  downloadAndInstallApk: ReturnType<typeof mock>,
+) => {
+  (globalThis as any).__DEV__ = false;
+
+  mock.module('react-native', () => ({
+    Platform: {
+      OS: 'android',
+      Version: 30,
+    },
+    DeviceEventEmitter: {
+      addListener: mock(() => ({ remove: mock(() => {}) })),
+    },
+    NativeEventEmitter: class {
+      addListener = mock(() => ({ remove: mock(() => {}) }));
+      removeAllListeners = mock(() => {});
+    },
+  }));
+
+  mock.module('../core', () => ({
+    PushyModule: {
+      markSuccess: mock(() => {}),
+      reloadUpdate: mock(() => Promise.resolve()),
+      setNeedUpdate: mock(() => Promise.resolve()),
+      downloadPatchFromPpk: mock(() => Promise.resolve()),
+      downloadPatchFromPackage: mock(() => Promise.resolve()),
+      downloadFullUpdate: mock(() => Promise.resolve()),
+      downloadAndInstallApk,
+      restartApp: mock(() => Promise.resolve()),
+    },
+    buildTime: '2023-01-01',
+    cInfo: { rnu: '10.0.0', rn: '0.73.0', os: 'android', uuid: 'uuid' },
+    currentVersion: 'hash',
+    currentVersionInfo: {},
+    isFirstTime: false,
+    isRolledBack: false,
+    packageVersion: '1.0.0',
+    pushyNativeEventEmitter: {
+      addListener: mock(() => ({ remove: mock(() => {}) })),
+    },
+    rolledBackVersion: '',
+    setLocalHashInfo: mock(() => {}),
+  }));
+
+  mock.module('../permissions', () => ({
+    PermissionsAndroid: {
+      request: mock(() => Promise.resolve('granted')),
+      PERMISSIONS: { WRITE_EXTERNAL_STORAGE: 'WRITE_EXTERNAL_STORAGE' },
+      RESULTS: { GRANTED: 'granted' },
+    },
+  }));
+
+  mock.module('../i18n', () => ({
+    default: { t: (key: string) => key, setLocale: mock(() => {}) },
+  }));
+};
+
 describe('Pushy server config', () => {
   test('uses preset main endpoints directly as configured endpoints', async () => {
     setupClientMocks();
@@ -601,6 +665,45 @@ describe('downloadUpdate fallback chain', () => {
       'error_full_patch_failed',
     );
   });
+
+  test('deduplicates concurrent downloads of the same hash (JS-8)', async () => {
+    // A slow native download keeps the first call in-flight while the second
+    // starts, so the dedup must reuse the same promise instead of triggering a
+    // second native download.
+    let resolveDownload: (() => void) | undefined;
+    const downloadFullUpdate = mock(
+      () =>
+        new Promise<void>(resolve => {
+          resolveDownload = resolve;
+        }),
+    );
+    setupDownloadMocks({ downloadFullUpdate });
+    const { Pushy, sharedState } = await importFreshClient('dl-concurrent-dedup');
+    sharedState.downloadedHash = undefined;
+    // Only a full URL so the full strategy is the one exercised.
+    const fullOnlyInfo = {
+      update: true as const,
+      hash: 'new-hash',
+      full: 'full.ppk',
+      paths: ['https://cdn.example.com'],
+      name: 'v2.0',
+      description: 'test update',
+    };
+    // No onDownloadProgress passed — old code only deduped when a progress
+    // handler was registered, so this exercises the new promise-table dedup.
+    const client = new Pushy({ appKey: 'demo-app' });
+
+    const first = client.downloadUpdate(fullOnlyInfo);
+    const second = client.downloadUpdate(fullOnlyInfo);
+    // Let both calls reach the (single) native download, then let it finish.
+    await new Promise(r => setTimeout(r, 10));
+    resolveDownload?.();
+    const [firstHash, secondHash] = await Promise.all([first, second]);
+
+    expect(firstHash).toBe('new-hash');
+    expect(secondHash).toBe('new-hash');
+    expect(downloadFullUpdate).toHaveBeenCalledTimes(1);
+  });
 });
 
 describe('Cresc class', () => {
@@ -681,3 +784,31 @@ describe('Cresc class', () => {
     ]);
   });
 });
+
+describe('downloadAndInstallApk apkStatus (JS-3)', () => {
+  test('resets apkStatus after a failed download so retry is possible', async () => {
+    // First download fails, second succeeds. The old code unconditionally set
+    // apkStatus='downloaded' after a caught failure, permanently blocking retry.
+    let callCount = 0;
+    const downloadAndInstallApk = mock(() => {
+      callCount++;
+      if (callCount === 1) {
+        return Promise.reject(Error('download fail'));
+      }
+      return Promise.resolve();
+    });
+    setupAndroidApkMocks(downloadAndInstallApk);
+    const { Pushy, sharedState } = await importFreshClient('apk-retry');
+    sharedState.apkStatus = null;
+    const client = new Pushy({ appKey: 'demo-app' });
+
+    await client.downloadAndInstallApk('https://example.com/app.apk');
+    // Failure must leave apkStatus reset (null), not stuck at 'downloaded'.
+    expect(sharedState.apkStatus).toBe(null);
+
+    // A retry should now proceed and reach the native module a second time.
+    await client.downloadAndInstallApk('https://example.com/app.apk');
+    expect(downloadAndInstallApk).toHaveBeenCalledTimes(2);
+    expect(sharedState.apkStatus).toBe('downloaded');
+  });
+});