perf(android): move state persistence off UI thread (AN-1) + test hardening#602
Conversation
switchVersion/markSuccess/setUuid/setLocalHashInfo only read/modify SharedPreferences via a synchronous commit(); they were dispatched to the UI thread purely to serialize them. markSuccess runs on every cold start, so its blocking disk write on the main thread caused jank/ANR on low-end devices. Add StateSerialRunner (a dedicated single-thread executor) that preserves the same serialization + commit() persistence guarantees while keeping the disk I/O off the UI thread. reloadUpdate/restartApp remain on UiThreadRunner since reload is a genuine UI operation. Also adds CODE_AUDIT.md documenting the full P0-P2 audit and remediation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an Android ChangesAndroid background state serialization
C++ patch-core test infrastructure
Client download/install concurrency tests
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/__tests__/client.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java (2)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming the executor's thread for easier debugging.
Executors.newSingleThreadExecutor()produces an unnamed thread (e.g.,pool-1-thread-1), which makes it harder to identify in thread dumps/ANR traces when diagnosing persistence-related issues.♻️ Optional: use a named ThreadFactory
- private static final Executor EXECUTOR = Executors.newSingleThreadExecutor(); + private static final Executor EXECUTOR = Executors.newSingleThreadExecutor( + r -> new Thread(r, "StateSerialRunner") + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java` at line 31, The executor in StateSerialRunner currently uses Executors.newSingleThreadExecutor() with a default unnamed thread, making debugging harder. Update the EXECUTOR initialization to use a ThreadFactory that assigns a clear, stable thread name for this runner, and keep the change localized to the StateSerialRunner class so thread dumps and ANR traces are easier to identify.
25-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a unit test for the ordering/error-handling contract.
This class introduces new threading semantics (serialized execution, promise-reject-vs-log branching) that other code now depends on. A small JVM-level test (submit N operations, assert execution order; submit a throwing operation with promise=null and assert
Log.epath, etc.) would guard against regressions, complementing the test hardening already done in this PR for the C++ side.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java` around lines 25 - 56, Add a JVM-level test for StateSerialRunner to lock in its new contract: verify that run() executes submitted Operation instances in submission order on the single-thread EXECUTOR, and verify the error-handling branch by running a throwing Operation with a null Promise so the Log.e path is exercised. Also cover the promise-reject path by passing a Promise and confirming promise.reject is invoked from the catch block, using StateSerialRunner.run, Operation.run, and the UpdateContext.TAG logging branch as the key entry points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/client.test.ts`:
- Around line 100-155: Reset the Android test setup after each case in
client.test.ts: setupAndroidApkMocks mutates globalThis.__DEV__ and installs
mock.module overrides for react-native, ../core, ../permissions, and ../i18n
without cleanup, so later tests can inherit the Android/DEV-off state. Add
teardown in the test suite (for example via afterEach with mock.restore or
equivalent) to restore the mocked modules and reset any globals changed by
setupAndroidApkMocks.
---
Nitpick comments:
In `@android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.java`:
- Line 31: The executor in StateSerialRunner currently uses
Executors.newSingleThreadExecutor() with a default unnamed thread, making
debugging harder. Update the EXECUTOR initialization to use a ThreadFactory that
assigns a clear, stable thread name for this runner, and keep the change
localized to the StateSerialRunner class so thread dumps and ANR traces are
easier to identify.
- Around line 25-56: Add a JVM-level test for StateSerialRunner to lock in its
new contract: verify that run() executes submitted Operation instances in
submission order on the single-thread EXECUTOR, and verify the error-handling
branch by running a throwing Operation with a null Promise so the Log.e path is
exercised. Also cover the promise-reject path by passing a Promise and
confirming promise.reject is invoked from the catch block, using
StateSerialRunner.run, Operation.run, and the UpdateContext.TAG logging branch
as the key entry points.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bee95f0-1c99-481e-acee-b8372caee6b4
📒 Files selected for processing (5)
android/src/main/java/cn/reactnative/modules/update/StateSerialRunner.javaandroid/src/main/java/cn/reactnative/modules/update/UpdateModuleImpl.javacpp/patch_core/tests/patch_core_test.cppscripts/test-patch-core.shsrc/__tests__/client.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Executors.newSingleThreadExecutor() produces an unnamed thread
(pool-1-thread-1), which is hard to spot in thread dumps / ANR traces. Give it
a named ThreadFactory ("pushy-state-serial") so persistence-related issues are
easier to diagnose.
Addresses CodeRabbit review on PR #602.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
背景
代码审计（见
CODE_AUDIT.md）路线图的收尾项。P0/P1 的绝大部分已在7d959ad落地（已在 master）。本 PR 包含两个后续提交：perf(android): 状态持久化移出 UI 线程（AN-1）switchVersion/markSuccess/setUuid/setLocalHashInfo本质只是 SharedPreferences 的同步commit()，之前 dispatch 到 UI 线程纯粹为串行化。markSuccess每次冷启动必经，其阻塞磁盘写在主线程是低端机卡顿/ANR 来源。StateSerialRunner（专用单线程 executor），保留串行化 +commit()持久性，把磁盘 I/O 移出 UI 线程。reloadUpdate/restartApp仍走UiThreadRunner（reload 是真 UI 操作）。improvement 2: 测试加固test-patch-core.sh增加SANITIZE=1开关跑 ASan+UBSan。验证
bun lint通过、73 个 JS 单测全绿、16 个 C++ patch_core 测试默认与 sanitizer 双模式全通过。Example/e2etest/e2e/local-merge.test.ts，覆盖新架构 + RN0.77 旧架构双 job）回归；沙盒环境无法跑模拟器/Gradle，故开此 PR 让 CI 验证。已知盲区（记录，不在本 PR 处理）
串行 e2e 测不到并发竞态窗口（
switchVersionLater后台线程 vsreloadUpdate→switchVersionUI 线程对同一 SP 的交叉写）——该窗口与改动前风险等价，AN-1 未扩大它。详见CODE_AUDIT.md。🤖 Generated with Claude Code
Summary by CodeRabbit