Skip to content

perf(harmony): run patch/cleanup off the UI thread via async NAPI (HM-1)#603

Merged
sunnylqm merged 2 commits into
masterfrom
review/hm1-harmony-offthread
Jul 4, 2026
Merged

perf(harmony): run patch/cleanup off the UI thread via async NAPI (HM-1)#603
sunnylqm merged 2 commits into
masterfrom
review/hm1-harmony-offthread

Conversation

@sunnylqm

@sunnylqm sunnylqm commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

背景

代码审计发现 HM-1:Pushy 的 HarmonyOS TurboModule 跑在 UI 线程,而 applyPatchFromFileSource / cleanupOldEntries同步 NAPI 调用——hdiff 打补丁和递归清理会把 UI 冻结数百 ms 到数秒。Android 端同样的工作本来就在后台线程,本 PR 补齐这一平台差距。

改动

  • C++harmony/pushy/src/main/cpp/pushy.cpp):把 ApplyPatchFromFileSource / CleanupOldEntriesnapi_create_async_work 包成异步、返回 Promise。参数在 JS 线程解析;hdiff/清理在 libuv worker 线程执行;Promise 在 JS 线程 resolve/reject。
  • ArkTS 绑定NativePatchCore.ts):两个方法返回类型改 Promise<void>DownloadTask.ts 全部调用点 await
  • UpdateContext.cleanUp():保持 void 签名(所有调用方本就是 fire-and-forget),内部改为发起 Promise + .catch 记日志,冷启动 syncStateWithBinaryVersion → cleanUp 不再阻塞在同步磁盘 I/O(HM-1 点③随之解决)。

验证说明

⚠️ CI 无 HarmonyOS runner,本地也无 Harmony 设备/模拟器,本 PR 未经运行时验证(按维护者指示直接实现)。已确认:共享 C++ patch_core 15 个单测仍全过、Android JNI glue 语法校验无误(改动隔离在 Harmony 层)。

待真机验证:完整"下载 → 打补丁 → 切版本 → 重启 → markSuccess"链路,重点确认打补丁期间 UI 不再冻结、async work 生命周期与错误传播正确。

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Patch application and cleanup are now asynchronous, returning Promises to improve responsiveness during update operations.
    • Cleanup runs in the background without blocking the caller.
  • Bug Fixes

    • Ensured patch and cleanup operations are properly awaited so subsequent steps run only after native work completes.
    • Added best-effort error handling for background cleanup (failures are logged without interrupting the main process).

The Pushy TurboModule runs on the UI thread, and applyPatchFromFileSource /
cleanupOldEntries were synchronous NAPI calls, so hdiff patching and recursive
cleanup froze the UI for hundreds of ms to seconds (Android already did this
work on a background thread — this closes the platform gap).

- C++: wrap both exports in napi_create_async_work returning a Promise. Args
  are parsed on the JS thread; the heavy hdiff/cleanup runs on a libuv worker
  thread; the Promise is settled back on the JS thread.
- ArkTS bindings: applyPatchFromFileSource / cleanupOldEntries now return
  Promise<void>; all call sites in DownloadTask await them.
- UpdateContext.cleanUp() keeps its void signature (all callers are
  fire-and-forget) but now launches the async cleanup with a .catch, so the
  cold-start syncStateWithBinaryVersion -> cleanUp path no longer blocks on
  synchronous disk I/O.

Note: there is no HarmonyOS runner in CI and no local device, so this is not
runtime-validated; needs an on-device pass of the full download -> patch ->
switch -> restart -> markSuccess flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cb995320-4a0c-458e-a59d-d0bdba291367

📥 Commits

Reviewing files that changed from the base of the PR and between 6570733 and 827b6fd.

📒 Files selected for processing (1)
  • harmony/pushy/src/main/cpp/pushy.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • harmony/pushy/src/main/cpp/pushy.cpp

📝 Walkthrough

Walkthrough

Converts the native applyPatchFromFileSource and cleanupOldEntries NAPI bindings from synchronous execution to Promise-based async work using napi_create_async_work. Updates corresponding TypeScript binding declarations to return Promise, and updates call sites to await these calls or handle them as background fire-and-forget operations with error logging.

Changes

Async native patch/cleanup migration

Layer / File(s) Summary
Native async work structs and Promise flows
harmony/pushy/src/main/cpp/pushy.cpp
Adds async-work state helpers and converts applyPatchFromFileSource and cleanupOldEntries to Promise-based napi_create_async_work flows with resolve, reject, and failure cleanup paths.
TS binding signature updates
harmony/pushy/src/main/ets/NativePatchCore.ts
Updates applyPatchFromFileSource and cleanupOldEntries to return Promise<void> instead of void.
Call site await and error handling updates
harmony/pushy/src/main/ets/DownloadTask.ts, harmony/pushy/src/main/ets/UpdateContext.ts
Adds await before native patch and cleanup calls in DownloadTask.ts, and changes UpdateContext.cleanUp() to invoke cleanupOldEntries as a background operation with failure logging.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DownloadTask as DownloadTask.ts
  participant Binding as NativePatchCore
  participant Napi as pushy.cpp
  participant Worker as Worker Thread

  DownloadTask->>Binding: await applyPatchFromFileSource(options)
  Binding->>Napi: applyPatchFromFileSource(options)
  Napi->>Worker: schedule ApplyPatchFromFileSource
  Worker-->>Napi: Status result
  Napi-->>Binding: resolve/reject Promise
  Binding-->>DownloadTask: completion or error
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: moving patch and cleanup work off the UI thread using async NAPI.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch review/hm1-harmony-offthread

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@harmony/pushy/src/main/cpp/pushy.cpp`:
- Around line 806-833: The async work setup in pushy.cpp is not checking
failures from napi_create_async_work or napi_queue_async_work, so ApplyPatchWork
can leak and the promise can remain pending if setup fails. Update the
async-work flow around the ApplyPatchWork creation to inspect both napi_status
return values, clean up work_data and any created work on error, and reject the
deferred promise or throw an appropriate JS error before returning so failure is
surfaced immediately.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b17ad8ad-d396-4d70-9086-272d89f41a0c

📥 Commits

Reviewing files that changed from the base of the PR and between 7d959ad and 6570733.

📒 Files selected for processing (4)
  • harmony/pushy/src/main/cpp/pushy.cpp
  • harmony/pushy/src/main/ets/DownloadTask.ts
  • harmony/pushy/src/main/ets/NativePatchCore.ts
  • harmony/pushy/src/main/ets/UpdateContext.ts

Comment thread harmony/pushy/src/main/cpp/pushy.cpp Outdated
napi_create_async_work / napi_queue_async_work return napi_status; if either
fails the completion callback never runs, leaving the Promise pending forever
and leaking the heap-allocated work data. Check both return values, and on
failure delete the work (when created), reject the deferred, and free the data.

Addresses CodeRabbit review on PR #603.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sunnylqm sunnylqm merged commit 80a1111 into master Jul 4, 2026
4 checks passed
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.

1 participant