Repository navigation
fix(updates): 回退先重挂页面、不可取消、落盘失败不抛 - #39
Merged
Merged
Conversation
- UpdatesPage 先切到 embedded 再调 rollBack：check() 下载期间持有包锁， 之前页面要空白等到锁释放 - rollBack 立即把 current 切回 embedded，锁内的拉黑与落盘放进 NonCancellable， 页面离开不再中断回退 - saveState 失败发 Failed(STORAGE) 而不是抛进 LaunchedEffect - 先落盘再删 installed 目录，两步之间被杀不会让 check() 把坏版本装回来 - 同步 docs/updates.md §4.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR hardens E2/E6 rollback by remounting embedded content before lock acquisition, making cleanup non-cancellable, persisting the blacklist before deleting the bad package, and reporting storage failures instead of throwing; documentation and focused tests cover the new behavior. Sequence diagram for non-cancellable update rollbacksequenceDiagram
participant Page as UpdatesPage
participant Updates as Updates
participant Check as check()
participant Lock as PackageLock
participant FS as Filesystem
participant Events as UpdateEvents
Page->>Page: bundle = embedded(pkg)
Page->>Updates: rollBack(pkg, bundle, page, failure)
Updates->>Updates: current = embedded
Updates->>Updates: withContext(NonCancellable)
Updates->>Lock: withLock
Note over Check,Lock: check() may hold the lock during download
Updates->>Updates: failed += version
Updates->>Updates: installed = null
Updates->>FS: saveState()
alt saveState() fails
FS-->>Updates: IOException
Updates->>Events: Failed(pkg, version, STORAGE, ...)
end
Updates->>FS: deleteRecursively(installed/version)
Updates->>Events: RolledBack(pkg, version, page, ...)
Updates-->>Page: rollback completes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
![codeseerbot[bot]](https://avatars.githubusercontent.com/in/4845701?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeSeer 审查
结论：批准（附 1 条建议）
摘要
本次改动将页面重挂提前，并把回退的落盘、删除和事件上报放入 NonCancellable，同时补充了锁竞争和落盘失败的测试。包锁已被占用后回退不再会因页面协程取消而中断，落盘失败也能上报并继续删除坏包。主要残余风险是回退任务仍由页面生命周期绑定的 LaunchedEffect 创建，在协程实际开始前页面离开时，整个回退仍可能完全不执行。
整个 PR 至 62b5aa2 · gpt-5.6-terra
| if ((failure.kind == "E2" || failure.kind == "E6") && page.bundle !== updates.embedded(pkg)) { | ||
| LaunchedEffect(failure) { | ||
| updates.rollBack(pkg, page.bundle, page.module.name, failure) | ||
| // remount first: the rollback can wait for a check() downloading under the package lock |
There was a problem hiding this comment.
[建议] rollBack 虽然在进入函数后会切到 NonCancellable，但它仍由页面生命周期绑定的 LaunchedEffect 启动；如果页面在该 effect 的协程实际开始前离开组合，effect 会被取消，bundle = ... 和 rollBack(...) 都不会执行。这样仍会保留坏包为 current，也不会拉黑或删除它，未完全满足“页面离开不取消回退”的目标。应将回退调度到生命周期独立、由 Updates 或宿主长期持有的 scope，或在能保证任务已提交到该 scope 后再允许页面 effect 结束。
Member
Author
There was a problem hiding this comment.
不改。effect 从进入组合到协程开始执行只隔一次调度，通常不到一帧，用户来不及在这期间离开页面。即使发生，后果也能自己恢复：坏版本没被拉黑，下次打开页面会再报 E2/E6、再走一次回退。要彻底消除这个窗口，就得让 Updates 持有一个长期存在的协程作用域，还要定义它的生命周期和取消语义，代价超过收益。
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="updates/src/commonMain/kotlin/app/tinyui/updates/Updates.kt" line_range="135" />
<code_context>
+ } catch (e: IOException) {
+ onEvent(UpdateEvent.Failed(pkg, version, FailStage.STORAGE, "rollback not saved: ${e.message ?: e}"))
+ }
+ if (drop) runCatching { fs.deleteRecursively(p.root / INSTALLED / version) }
}
- p.saveState()
</code_context>
<issue_to_address>
**Rollback leaves stale installed state**
When rollback state saving fails and the app later restarts, `rollBack` still deletes the installed directory while the previous state file can still name that version without its failed-version blacklist. With storage still unavailable, `selectAtStartup()` calls `dropInstalled()`, whose `saveState()` failure aborts construction; if storage recovers, startup clears the stale pointer without blacklisting the version, so `check()` can install the same bad version again.
Keep the installed directory until the rollback state, including the failed-version blacklist, is durably saved, and delete it only after that save succeeds.
</issue_to_address>
### Comment 2
<location path="updates/src/commonMain/kotlin/app/tinyui/updates/Updates.kt" line_range="125" />
<code_context>
internal suspend fun rollBack(pkg: String, bundle: Bundle, page: String, failure: PageFailure) {
val p = pkg(pkg)
val version = bundle.manifest.version
- p.lock.withLock {
- if (version !in p.failed) p.failed += version
- p.current = p.embedded
</code_context>
<issue_to_address>
**Rollback never reaches blacklist**
When a host-provided fetch stays suspended indefinitely while `check()` holds the package lock, `p.lock.withLock` waits for the active check, and `NonCancellable` keeps that wait alive even after its caller is gone. If the host-provided `fetch` never completes, the package stays embedded only in this process; the failed version is never persisted and `RolledBack` is never emitted.
Require or enforce a bounded check/fetch duration and define how rollback completes if that bound is exceeded.
</issue_to_address>
### Comment 3
<location path="updates/src/commonMain/kotlin/app/tinyui/updates/Updates.kt" line_range="133" />
<code_context>
+ try {
+ p.saveState()
+ } catch (e: IOException) {
+ onEvent(UpdateEvent.Failed(pkg, version, FailStage.STORAGE, "rollback not saved: ${e.message ?: e}"))
+ }
+ if (drop) runCatching { fs.deleteRecursively(p.root / INSTALLED / version) }
</code_context>
<issue_to_address>
**Rollback failure event is undocumented**
`rollBack` emits `Failed(STORAGE)`, but the event table describes `Failed` as occurring during `check()` only. Hosts following that contract can misclassify or omit rollback storage failures from their event handling and telemetry.
Update the event contract to include rollback persistence failures as a source of `Failed(STORAGE)`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and a rollback can persist a version blacklist and clear the installed-version marker, so those effects remain after a revert; a failed save is reported while the process stays on the embedded page. The impact is bounded to that package version and can be repaired by installing a newer version or restoring package state.
Blocking findings: updates/src/commonMain/kotlin/app/tinyui/updates/Updates.kt:135, updates/src/commonMain/kotlin/app/tinyui/updates/Updates.kt:125
- dropInstalled 的落盘与删目录失败不抛：回退落盘失败后目录已删，磁盘仍满时 下次启动会走到这里，之前会让 Updates 构造抛出 - docs §4.1 写明 fetch 必须有超时；§4.2 事件表补回退落盘失败的 Failed(STORAGE)； §4.5 说明落盘失败仍删目录的取舍 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
CodeSeer 审查
结论：批准（附 1 条建议）
上轮意见：1 条，已处理 0 条，待处理 1 条
updates/src/commonMain/kotlin/app/tinyui/updates/UpdatesPage.kt:63[建议]rollBack虽然在进入函数后会切到NonCancellable，但它仍由页面生命周期绑定的LaunchedEffect…
摘要
本次改动将回退时的当前包切换前置，并用 NonCancellable 保证等待包锁后的黑名单落盘和回退事件不会因页面销毁而中断。回退状态改为先持久化后清理安装目录，且对回退和启动阶段的状态写入失败进行了兜底；测试覆盖了检查持锁和状态文件写入失败的场景。未发现本次增量引入的可确认问题。
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
修复 #38 审查中 Sourcery 指出的三个问题。它们都在原有的 E2/E6 回退流程里（
Updates.rollBack和UpdatesPage），来自 #15，不是 #38 引入的。这条路径只在线上发了坏包时才会走到，一走到就是批量用户同时受影响。rollBack先删 installed 目录、后saveState()check()会把坏版本重新装回来，多失败一次LaunchedEffect里执行，而且要等包锁check()在整个下载期间都持有包锁，弱网下页面会空白等十几秒；用户这时离开页面，回退就被取消saveState()失败（例如磁盘写满）时异常往外抛LaunchedEffect，页面切不到内置包；按 Compose 的通常行为，在 Android 上可能导致崩溃（推断，未复现）改动
UpdatesPage：先把页面切到 embedded 重新挂载，再调用rollBackrollBack：current切回 embedded，之后新打开的页面不再拿到坏包RolledBack都放在NonCancellable里执行，不会被取消saveState()失败时发Failed(STORAGE)，不往外抛；坏包目录照样删除docs/updates.md§4.5测试
:updates:testAndroidHostTest、:updates:iosSimulatorArm64Test共 20 个用例全部通过。新增 2 个，在旧的rollBack上都会失败：check()持锁期间发起回退：current立即变成 embedded；回退的协程被取消后，拉黑和RolledBack仍然完成state.json换成目录，模拟落盘失败：不抛异常，依次上报Failed(STORAGE)和RolledBack，坏包目录被删除UpdatesPage的调用顺序调整没有 Compose 测试；#1 的崩溃时机没法在单测里模拟，靠代码顺序保证。审查后补充（18143d6）
dropInstalled()的存盘和删目录失败都不抛。回退存盘失败后目录已经删了，如果磁盘仍然满，下次启动会走到这里；之前会让Updates构造函数抛异常docs/updates.md：§4.1 写明fetch必须有超时；§4.2 事件表补上回退存盘失败时的Failed(STORAGE)；§4.5 说明存盘失败仍删目录的取舍🤖 Generated with Claude Code
Summary by Sourcery
Harden failed update rollback so users immediately return to the embedded page and faulty packages cannot be retried after interrupted or unsuccessful cleanup.
Bug Fixes:
Enhancements:
Documentation:
Tests: