fix(crx): 序列化 update 标记写入/清除，修复 reload 竞态与双触发 - #313
Conversation
Addresses race conditions the first fix left open: - onInstalled now awaits the marker removal before boot, so replayPendingUpdateIfAny cannot read a stale marker mid-clear. - onUpdateAvailable persists the marker before consuming it; previously the fire-and-forget write could land after clearPendingUpdate's remove and survive the reload, re-firing the loop next boot. - tryApplyUpdate sets updateReloadInFlight synchronously, so the idle callback and the onUpdateAvailable async closure cannot both reload. - Use removeStorageValues instead of chrome.storage.local.remove to avoid @types/chrome's enum-literal narrowing. Tests: onUpdateAvailable persistence now routes through the storage fake; agent-idle test asserts exactly one reload after the async closure settles.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesUpdate reload coordination
Native-host connection logging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceWorker
participant UpdateOperationQueue
participant Storage
participant Runtime
ServiceWorker->>UpdateOperationQueue: Queue update handling
UpdateOperationQueue->>Storage: Persist update marker
Storage-->>UpdateOperationQueue: Confirm write
UpdateOperationQueue->>Runtime: Request reload
Runtime-->>ServiceWorker: Report reload setup result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![duckpr[bot]](https://avatars.githubusercontent.com/in/4166808?s=60&v=4){kind=small}
| updateReloadInFlight = true; | ||
| await clearPendingUpdate(); | ||
| chrome.runtime.reload(); |
There was a problem hiding this comment.
updateReloadInFlight 设为 true 后调用 chrome.runtime.reload()，但如果 reload 因任何原因未能真正执行（例如被浏览器策略阻止、或抛出同步异常），该标志将永远保持 true，导致后续所有 tryApplyUpdate() 调用被短路，扩展无法再应用更新。建议在 reload 失败时重置标志，例如用 try/catch 包裹并在 catch 中将 updateReloadInFlight 置回 false。
建议修改:
| updateReloadInFlight = true; | |
| await clearPendingUpdate(); | |
| chrome.runtime.reload(); | |
| updateReloadInFlight = true; | |
| try { | |
| await clearPendingUpdate(); | |
| chrome.runtime.reload(); | |
| } catch { | |
| updateReloadInFlight = false; | |
| throw err; | |
| } |
There was a problem hiding this comment.
已部分处理：新增的 try/catch 在 clearPendingUpdate() 失败时重置 updateReloadInFlight，覆盖了最常见的发生异常的路径（storage 写入失败）。reload 本身失败的更窄场景在 review 4861108495 单独跟踪。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 b757c6c 中处理：chrome.runtime.reload() 之后新增 setTimeout(() => { updateReloadInFlight = false; }, 5000) 兜底，SW 未被终止时护栏会自动重置。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 b757c6c 中完全处理：tryApplyUpdate 的 try/catch + 5s setTimeout 兜底重置覆盖了 clearPendingUpdate 失败与 reload 未生效两条路径。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Resolved — 已在 b757c6c 中完全处理。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 1985611 / b757c6c 实现：try/catch 包裹 clearPendingUpdate() 与 chrome.runtime.reload()，catch 中重置 guard 并 console.warn 记录；另在 reload 后追加 setTimeout(…, 5000) 兜底覆盖静默失败场景。
anthropic/glm-5.2 ｜ 𝕏
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to prevent double-triggering extension reloads during updates by adding an updateReloadInFlight guard and ensuring that update markers are persisted to storage before being consumed. However, a critical race condition was identified in the onUpdateAvailable listener: if an idle callback triggers tryApplyUpdate while the async storage writes are still in progress, the writes could execute after the storage is cleared, leading to a stale marker and an infinite reload loop on the next boot. It is recommended to check the updateReloadInFlight guard before each storage write in the async closure.
| void (async () => { | ||
| // Persist the marker before consuming it: tryApplyUpdate clears storage, | ||
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | ||
| await tryApplyUpdate(); | ||
| })(); |
There was a problem hiding this comment.
潜在的竞态条件可能导致无限重载循环
虽然引入了 updateReloadInFlight 变量来防止 tryApplyUpdate 被多次触发，但在 onUpdateAvailable 的异步闭包中，向 storage 写入标记的操作（setStorageValue）是异步的，且没有受到 updateReloadInFlight 的保护。
考虑以下场景：
onUpdateAvailable触发，异步闭包开始执行，并开始await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true)。- 在该写入操作未完成时，Agent 变为空闲，触发了
setOnAgentBecameIdle回调。 - 空闲回调调用
tryApplyUpdate()，此时updateReloadInFlight为false，因此它将updateReloadInFlight设为true，并调用clearPendingUpdate()（删除 storage 中的PENDING_UPDATE_VERSION），然后调用chrome.runtime.reload()。 - 此时，异步闭包中的第一个
setStorageValue完成，继续执行await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version)。 - 这个写入操作发生在
clearPendingUpdate()的删除操作之后。 - 浏览器执行重载。
- 下次启动时，
replayPendingUpdateIfAny从 storage 中读取到了刚刚被写入的PENDING_UPDATE_VERSION，从而再次触发重载，导致无限重载循环。
解决方案：
在异步闭包中的每次写入 storage 之前，先检查 updateReloadInFlight。如果已经有重载在进行中，则立即退出，避免在清除操作之后再次写入残留标记。
void (async () => {
// Persist the marker before consuming it: tryApplyUpdate clears storage,
// so an un-awaited write could land after the clear and survive the
// reload, re-firing the loop on the next boot.
if (updateReloadInFlight) return;
await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true);
if (updateReloadInFlight) return;
await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version);
await tryApplyUpdate();
})();![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929aa1185c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); |
There was a problem hiding this comment.
When onUpdateAvailable fires while the agent is active, pendingUpdateVersion is set synchronously, so the idle callback can call tryApplyUpdate() before this awaited persistence reaches the pending-version write. In that ordering, the idle path clears storage and reloads, then this line can write pendingUpdateVersion after the clear; because updateReloadInFlight is already true, the async closure's later tryApplyUpdate() is a no-op, leaving a stale marker that replays on the next service-worker boot and re-triggers the reload loop.
Useful? React with 👍 / 👎.
…ails If clearPendingUpdate rejects (storage error), the guard stayed true forever and every later update was silently skipped. Reset it and log; a new onUpdateAvailable can then retry. Adds a regression test.
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 本 PR 修复了 service worker update/reload 流程中三处竞态（onInstalled 清理、onUpdateAvailable 持久化顺序、tryApplyUpdate 双触发），并清理了一处类型 cast。改动小而聚焦，测试覆盖到位。
onInstalled标记清理改为 await 且先于 boot — 把 fire-and-forget 的remove换成await removeStorageValues([...])，置于ensureServiceWorkerBooted()之前，避免 boot 内replayPendingUpdateIfAny读到残留pendingUpdateVersion。onUpdateAvailable先持久化后消费 — 两次setStorageValue+tryApplyUpdate包进 async IIFE 依次 await，杜绝 un-awaited 写入晚于clearPendingUpdate删除而残留。tryApplyUpdate新增updateReloadInFlight护栏 — idle 回调与 async 闭包并发到达时保证只 reload 一次；flag 在 SW 生命周期内不重置（reload 会终止 SW，语义正确）。- 改用
removeStorageValues帮助函数 — 去掉StorageArea.remove的 enum 字面量窄化所需的as stringcast。
验证
bun run test -- service-worker.boot.test.ts 全部 11/11 通过，包括新增的 idle 异步闭包落定断言。
anthropic/glm-5.2 ｜ 𝕏
|
已修复 ✅ commit 1985611： |
| updateReloadInFlight = true; | ||
| try { | ||
| await clearPendingUpdate(); | ||
| chrome.runtime.reload(); | ||
| } catch (err) { | ||
| // Clear failed — reset the guard so a later update can still be applied. | ||
| updateReloadInFlight = false; | ||
| console.warn('[superduck] failed to apply pending update', err); | ||
| } |
There was a problem hiding this comment.
chrome.runtime.reload() 是 fire-and-forget 的——它不返回 Promise，且扩展的实际重载是异步发生的（SW 终止 → 重新启动）。当 reload 成功时，SW 被终止，catch 块永远不会执行，updateReloadInFlight 保持 true 是正确的（因为新 SW 实例会重新初始化该变量为 false）。
但问题在于：如果 chrome.runtime.reload() 调用后 SW 没有被终止（例如 Chrome 内部异常、reload 在某些边缘场景下未生效），updateReloadInFlight 将永远保持 true，导致当前 SW 生命周期内后续所有的 tryApplyUpdate() 调用都被短路跳过（包括 setOnAgentBecameIdle 回调和 replayPendingUpdateIfAny），更新将永远无法应用。
建议：为 reload 设置一个超时回退机制，或改用带 callback 的形式确认 reload 已触发后再标记，例如使用 setTimeout 在一定时间后重置 guard 作为兜底。
建议修改:
| updateReloadInFlight = true; | |
| try { | |
| await clearPendingUpdate(); | |
| chrome.runtime.reload(); | |
| } catch (err) { | |
| // Clear failed — reset the guard so a later update can still be applied. | |
| updateReloadInFlight = false; | |
| console.warn('[superduck] failed to apply pending update', err); | |
| } | |
| updateReloadInFlight = true; | |
| try { | |
| await clearPendingUpdate(); | |
| chrome.runtime.reload(); | |
| // Fallback: if the SW is still alive after a grace period, the reload | |
| // likely didn't take effect — reset the guard so retry remains possible. | |
| setTimeout(() => { updateReloadInFlight = false; }, 5000); | |
| } catch (err) { | |
| updateReloadInFlight = false; | |
| console.warn('[superduck] failed to apply pending update', err); | |
| } |
There was a problem hiding this comment.
已处理：新增 commit 在 chrome.runtime.reload() 之后加了 5000ms setTimeout 兜底重置 updateReloadInFlight（service-worker.ts:220-222），SW 未被终止时护栏不会永久卡死。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 b757c6c 中按此建议处理：reload() 后挂 5 秒 setTimeout 兜底重置 updateReloadInFlight。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已按此建议处理（b757c6c）：reload() 后挂 5s setTimeout 兜底重置 updateReloadInFlight。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Resolved — 已在 b757c6c 中处理。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 b757c6c 字面落实：chrome.runtime.reload() 后调度 setTimeout(() => { updateReloadInFlight = false; }, 5000) 兜底，覆盖 SW 静默未终止场景。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
已审查的更改 — PR #313 修复了 #312 遗留的 3 个 update/reload 竞态问题。通过 await 序列化 marker 写入/清除顺序、并引入 updateReloadInFlight 原子护栏防止双触发，所有改动都有对应的回归测试覆盖。
onInstalled先清除 marker 再 boot — 将原先 fire-and-forget 的chrome.storage.local.remove(...)改为await removeStorageValues([UPDATE_AVAILABLE, PENDING_UPDATE_VERSION])，且位于ensureServiceWorkerBooted()之前。这样 boot 内的replayPendingUpdateIfAny不会读到残留的pendingUpdateVersion而误触发 reload。同时用StorageKeys.UPDATE_AVAILABLE替代字面量"updateAvailable"，去掉as string强转。onUpdateAvailable持久化先于消费 — 把两次setStorageValue与tryApplyUpdate包进 async IIFE 依次await。pre-fix 的void setStorageValue(...)是 fire-and-forget，写入可能在clearPendingUpdate的删除之后落盘，导致 marker 残留并触发下次 boot 的 reload 循环。tryApplyUpdate增加updateReloadInFlight护栏 — idle 回调与onUpdateAvailableasync 闭包并发到达时只 reload 一次。由于chrome.runtime.reload()紧随其后调用、SW 会被终止重启，标志位在单次 SW 生命周期内无需重置。- 测试对齐真实写入-消费顺序 —
setStorageValuemock 改为真实写入storageLocalMock，新增removeStorageValuesmock；新增 idle 与闭包双到达的"只 reload 一次"断言（含 20ms settle 等待）；保留onInstalled清除 stale marker 的契约测试。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Pull request overview
此 PR 聚焦于 chrome-crx 扩展在 MV3 Service Worker 生命周期下的更新应用流程，主要通过“先持久化再消费”与并发护栏来修复 pendingUpdateVersion / updateAvailable 标记在 storage 与内存之间的竞态，从而避免 reload 循环、重复触发与 cold-boot 重放异常。
Changes:
onInstalled在 boot 前 await 清理更新相关 storage 标记，避免 boot 重放读到残留标记。onUpdateAvailable改为在异步闭包中 await 持久化标记后再调用tryApplyUpdate()，并在tryApplyUpdate增加updateReloadInFlight护栏避免双触发。- 测试侧将
setStorageValuemock 调整为真实写入 storage fake，并新增用例验证并发路径下只 reload 一次。
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| chrome-crx/src/service-worker.ts | 调整 update 标记写入/清除顺序并新增 reload 并发护栏，降低 reload 循环与双触发风险 |
| chrome-crx/src/service-worker.boot.test.ts | 更新 extensionServices mock 以更贴近真实 storage 行为，并补充“并发到达仅 reload 一次”的回归断言 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist the marker before consuming it: tryApplyUpdate clears storage, | ||
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); |
| @@ -484,6 +493,9 @@ describe('service worker cold-start boot', () => { | |||
| await vi.waitFor(() => { | |||
| expect(chromeMock.runtime.reload).toHaveBeenCalledTimes(1); | |||
| }); | |||
| // Give the async closure time to settle; it must not re-fire reload. | |||
| await new Promise((resolve) => setTimeout(resolve, 20)); | |||
| expect(chromeMock.runtime.reload).toHaveBeenCalledTimes(1); | |||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1985611719
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); |
There was a problem hiding this comment.
When an update arrives while a tool is active and that tool finishes before this awaited PENDING_UPDATE_VERSION write has run, the idle callback still calls tryApplyUpdate() using the in-memory pendingUpdateVersion. That path removes the key and calls reload; then this unawaited closure can write pendingUpdateVersion after the remove and skip its own retry because updateReloadInFlight is already true. Fresh evidence in this revision is that persistence was moved into this async closure, but the idle callback was not gated on that persistence, so the stale marker can still be replayed on the next service-worker boot and cause another reload loop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ No new issues found (incremental).
Reviewed changes — 增量提交 1985611 给原 updateReloadInFlight 护栏补了失败路径：当 clearPendingUpdate 或 reload 抛错时重置护栏，避免一次性 storage 故障把后续所有 update 都锁死。
tryApplyUpdate包裹 try/catch — 在chrome-crx/src/service-worker.ts:214-221，捕获clearPendingUpdate/reload异常后重置updateReloadInFlight = false并console.warn记录。这样 storage 偶发故障后，下一次onUpdateAvailable或 SW boot 的replayPendingUpdateIfAny仍能正常应用 update。- 新增 storage 失败恢复测试 —
chrome-crx/src/service-worker.boot.test.ts:501-519：第一次removemock 拒绝时断言 reload 未发生；第二次 update 触发时断言 reload 成功，证明护栏确实已重置、没有被卡住。覆盖路径合理（pendingUpdateVersion在clearPendingUpdate内已同步置 null，即使 await 拒绝也不会留下脏状态）。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 新增 commit 为 tryApplyUpdate 的 reload 路径加上存储清除失败恢复：clearPendingUpdate() reject 时重置 updateReloadInFlight 并 console.warn，保证后续 onUpdateAvailable 仍能应用更新。
tryApplyUpdate增加 try/catch 恢复路径 —clearPendingUpdate()失败时重置updateReloadInFlight = false，避免当前 SW 生命周期内护栏被永久卡住；同时console.warn记录失败。- 新增回归测试
recovers when clearing the marker fails— 用storageLocalMock.remove.mockRejectedValueOnce模拟首次清除失败，断言不 reload；第二次onUpdateAvailable（storage 恢复）正常 reload，验证护栏已重置。
验证：bun run test -- service-worker.boot.test.ts 12/12 通过（含新增恢复路径测试），stderr 中按预期出现 [superduck] failed to apply pending update Error: storage boom。
注：先前 review 4861108495 提出的 chrome.runtime.reload() fire-and-forget 场景（建议 setTimeout 兜底）仍未处理，但该场景属 Chrome 内部异常边缘情况、非本 commit 引入，可在后续单独评估。
anthropic/glm-5.2 ｜ 𝕏
Gemini Code Assist / ChatGPT Codex / DuckPR flagged a residual race: onUpdateAvailable's async closure awaits setStorageValue while the idle callback can clear the marker and reload; the late write then resurrects the cleared marker and the next boot re-reloads forever. - Check updateReloadInFlight before/after each marker write in the closure and bail if a reload already fired. - Reset the in-flight guard 5s after reload() as a fallback in case the reload silently fails and the SW survives. Regression test gates the first storage write, fires the idle path, then releases the write and asserts the marker stays gone. 726/726 pass.
|
三个 review（Gemini / ChatGPT Codex / DuckPR）指向同一竞态： 已修复 ✅ commit b757c6c：
新增回归测试：门控首次 storage 写入 → 触发 idle 路径 reload → 释放写入 → 断言标记不再出现。全量 726/726 通过。 |
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | ||
| await tryApplyUpdate(); |
There was a problem hiding this comment.
tryApplyUpdate 可能在 IIFE 之外被触发（如 setOnAgentBecameIdle 或 replayPendingUpdateIfAny），此时 updateReloadInFlight 会被置为 true 并执行 clearPendingUpdate()。但 IIFE 中的第二个 if (updateReloadInFlight) return 检查之后，PENDING_UPDATE_VERSION 的写入已被跳过，而 UPDATE_AVAILABLE 已经写入。这会导致 UPDATE_AVAILABLE 被持久化但 PENDING_UPDATE_VERSION 被清除的不一致状态。虽然不会造成 reload 循环（因为 replay 逻辑只检查 PENDING_UPDATE_VERSION），但 UPDATE_AVAILABLE 标记会残留，建议在跳过时也做清理，或在两步写入前一次性原子检查。
There was a problem hiding this comment.
已处理：新增 commit 在 IIFE 内对每次 setStorageValue 之前都检查 updateReloadInFlight（service-worker.ts:265,267），idle 抢先 reload 后闭包会短路返回，不会再残留 PENDING_UPDATE_VERSION。残留的 UPDATE_AVAILABLE 属良性（更新确已应用），且 reload 触发的 onInstalled 会清除它。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已确认：每次 setStorageValue 前都检查护栏，reload 抢先后闭包短路返回。残留的 UPDATE_AVAILABLE 属良性（已应用的更新确实可用），且 reload 后的 onInstalled 会清除它。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 4d7eaa2 中处理（按"文档化有意保留"方向）：注释明确 UPDATE_AVAILABLE 属于 PRESERVED_KEYS、是 UI 标记，由 onInstalled 在版本实际安装后清除；并删掉了无效的 slow-mock 测试，给保留的测试补了 SW 重启后不再重复 reload 的端到端断言。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Resolved — 已在 4d7eaa2 中按"文档化有意保留"方向处理。
anthropic/glm-5.2 ｜ 𝕏
The earlier RACE test patched the mocked setStorageValue after import, which vitest mocks ignore — it never exercised the race. Remove it and extend the effective storage-gated test with the full closure Copilot asked for: after the idle reload and a simulated SW restart, reload is still called exactly once and no stale marker exists.
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); |
There was a problem hiding this comment.
UPDATE_AVAILABLE 的写入没有被 clearPendingUpdate() 清除，也没有在 clearAllStorage() 中被移除（它在 PRESERVED_KEYS 中）。如果 reload 成功，该标记会残留到下次启动。建议确认这是否是预期行为，或者在 clearPendingUpdate 中一并清除 UPDATE_AVAILABLE。
建议修改:
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | |
| if (updateReloadInFlight) return; | |
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | |
| // 如果 UPDATE_AVAILABLE 不需要在 reload 后保留，建议在 clearPendingUpdate 中一并清除 | |
| // 或在此处确认其生命周期设计 |
There was a problem hiding this comment.
确认为预期行为：UPDATE_AVAILABLE 标记 reload 后残留无实际影响（replay 逻辑只读 PENDING_UPDATE_VERSION），且下一次 onInstalled 会一并清除 UPDATE_AVAILABLE（service-worker.ts:136）。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
残留属预期：UPDATE_AVAILABLE 不参与 replayPendingUpdateIfAny 的判定（只读 PENDING_UPDATE_VERSION），且 reload 后 onInstalled 会清除它（service-worker.ts:136）。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 新提交 1985611 在 tryApplyUpdate 中加入 try/catch，覆盖 clearPendingUpdate() 拒绝时重置 updateReloadInFlight，并新增回归测试 recovers when clearing the marker fails 锁定恢复路径。
tryApplyUpdate加入 try/catch 重置 guard — 当clearPendingUpdate()因 storage 错误 reject 时，catch 块将updateReloadInFlight重置为false并通过console.warn('[superduck] failed to apply pending update', err)记录，避免后续tryApplyUpdate()调用被永久短路。- 新增回归测试
recovers when clearing the marker fails—storageLocalMock.remove.mockRejectedValueOnce让首次清理失败，断言 reload 未触发；第二次onUpdateAvailable时 storage 恢复健康，断言 reload 触发一次，证明 guard 已正确重置且后续更新仍能应用。
注：chrome.runtime.reload() 静默失败（fire-and-forget，SW 未实际终止）导致 guard 卡住的更广隐患仍在 thread 4861108495 中追踪，本次提交未覆盖该场景。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题（增量）。
Reviewed changes — 增量提交 b757c6c 同时解决了上一轮 DuckPR 的两条行内反馈：tryApplyUpdate 的 reload 兜底（review 4861108495）与 IIFE 闭包的 stale-marker 写入（review 4861125801），并配套新增两个竞态回归测试。
- IIFE 内每次写入前检查
updateReloadInFlight—chrome-crx/src/service-worker.ts:265,267：把原先只挡第二次写入的护栏前移到两次setStorageValue之前。idle 回调抢先触发 reload 后，闭包会短路返回，杜绝PENDING_UPDATE_VERSION被 late write 复活。 tryApplyUpdate加 5000mssetTimeout重置护栏 —chrome-crx/src/service-worker.ts:220-222：chrome.runtime.reload()是 fire-and-forget，若 SW 未被终止，护栏会卡死阻塞后续所有 update；grace period 后兜底重置，与 catch 路径的updateReloadInFlight = false形成互补。- 新增两个 stale-marker 竞态测试 —
chrome-crx/src/service-worker.boot.test.ts:501-567：分别用slowSet全量挂起与 gate 第二次storage.set的方式，覆盖「idle 抢先 reload 后闭包写入不得复活 marker」的两种到达顺序。
anthropic/glm-5.2 ｜ 𝕏
|
关于 |
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 增量未发现新问题。
Reviewed changes — 增量提交 b757c6c 给 tryApplyUpdate 的 reload 路径补了"SW 存活"兜底，并给 onUpdateAvailable 异步闭包加了双检查护栏，防止 idle 抢先 reload 之后晚到的写入把已清除的 marker 复活。两个先前关于 chrome.runtime.reload() fire-and-forget 的关注点（review 4861095406 / 4861108495）已被本提交收尾。
tryApplyUpdate在reload()之后挂 5 秒setTimeout兜底重置updateReloadInFlight—chrome.runtime.reload()是 fire-and-forget，正常路径下 SW 被终止、setTimeout永不触发；只有 SW 未被终止的边缘场景下，5 秒后护栏自动回到false，后续onUpdateAvailable或replayPendingUpdateIfAny仍能再次尝试。onUpdateAvailable异步闭包两次setStorageValue之前各加if (updateReloadInFlight) return检查 — 防止 idle 回调抢先触发 reload、clearPendingUpdate已清掉 storage 之后，晚到的PENDING_UPDATE_VERSION写入再把 marker 复活（否则下次 boot 又会 reload）。- 新增两条 RACE 回归测试 —
RACE: idle reload must not be followed by a stale marker write用 slowsetStorageValuemock 把闭包卡在第一次写入中途；does not persist the marker after an idle reload already fired用storageLocalMock.set写入门控只阻塞pendingUpdateVersion。两条都断言 idle reload 之后pendingUpdateVersion不会被残留写入。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题（增量）。
Reviewed changes — 增量提交 7412971 是 test-only 调整：删除与 does not persist the marker... 测试覆盖重叠的 RACE: 测试，并在保留的测试末尾补一个 SW-restart 断言，验证 stale marker 没有落盘时 boot 不会再次触发 reload。
- 删除冗余
RACE:测试 — 原slowSetmock 把两次setStorageValue一并挂起、忽略 key 参数，与更贴近真实行为的does not persist the marker...测试（仅 gatependingUpdateVersion写入）覆盖重叠，删除合理。 does not persist the marker...增加 SW-restart 闭环断言 —chrome-crx/src/service-worker.boot.test.ts:538-543：vi.resetModules()后重新 import，断言 reload 仍只发生 1 次，端到端证明 stale marker 未落盘→boot loop 不会发生。该断言把「单次写入跳过」与「跨 SW 生命周期的最终效果」连成一条证据链，比原测试更有说服力。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 增量未发现新问题。
Reviewed changes — 增量提交 7412971 + 4d7eaa2 收尾了 review 4861125801 关于 UPDATE_AVAILABLE 残留的关注点：作者选择"文档化有意保留"方向，并清理了一条无效的竞态测试、给保留的测试补了端到端断言。
onUpdateAvailable异步闭包增加注释说明UPDATE_AVAILABLE有意保留 — 明确该 key 属于PRESERVED_KEYS、是 UI 标记（useInitialization.ts据此显示更新提示），由onInstalled在新版本实际安装后清除，而非每次 SW reload 都清。回应了 review 4861125801 的"不一致状态"疑问。- 删除
RACE: idle reload must not be followed by a stale marker write测试 — 原 slowsetStorageValuemock 通过(svMock as any).setStorageValue = slowSet在 import 后替换，对 ES module 静态 import 的覆盖并不可靠；保留does not persist the marker after an idle reload already fired（直接 gatestorageLocalMock.set）更贴近真实写入路径。 - 保留的测试补了 SW 重启端到端断言 — 在
releaseWrite()之后用vi.resetModules()+ 重新import('./service-worker')模拟 SW 重启，断言chrome.runtime.reload仍只被调用 1 次，验证"晚到的写入被拦下、下次 boot 不会再次 reload"的完整闭环。
anthropic/glm-5.2 ｜ 𝕏
| void (async () => { | ||
| // Persist the marker before consuming it: tryApplyUpdate clears storage, | ||
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. Guard each write: if a | ||
| // reload already fired (e.g. idle retry raced ahead while we were | ||
| // awaiting), stop — a late write would resurrect the cleared marker. | ||
| // UPDATE_AVAILABLE is intentionally preserved (PRESERVED_KEYS): it is a | ||
| // UI flag cleared by onInstalled after the reload. | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | ||
| await tryApplyUpdate(); | ||
| })(); |
There was a problem hiding this comment.
这个 async IIFE 没有错误处理。文件中其他 void (async () => { ... })() 调用都使用了 .catch()（如第 81 行 runAfterServiceWorkerBoot）。如果 setStorageValue 抛出异常（如 storage 配额超限、SW 被终止），将产生未捕获的 Promise rejection。建议加上 .catch() 以保持与文件中其他异步 IIFE 的错误处理模式一致。
建议修改:
| void (async () => { | |
| // Persist the marker before consuming it: tryApplyUpdate clears storage, | |
| // so an un-awaited write could land after the clear and survive the | |
| // reload, re-firing the loop on the next boot. Guard each write: if a | |
| // reload already fired (e.g. idle retry raced ahead while we were | |
| // awaiting), stop — a late write would resurrect the cleared marker. | |
| // UPDATE_AVAILABLE is intentionally preserved (PRESERVED_KEYS): it is a | |
| // UI flag cleared by onInstalled after the reload. | |
| if (updateReloadInFlight) return; | |
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | |
| if (updateReloadInFlight) return; | |
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | |
| await tryApplyUpdate(); | |
| })(); | |
| void (async () => { | |
| // ... | |
| if (updateReloadInFlight) return; | |
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | |
| if (updateReloadInFlight) return; | |
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | |
| await tryApplyUpdate(); | |
| })().catch((err) => { | |
| console.warn('[superduck] failed to persist pending update marker', err); | |
| }); |
The async closure's setStorageValue can reject (storage error); with a bare void the rejection would surface as an unhandled rejection in the SW. Attach a catch that logs instead. Self-review catch, not from bots.
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 无新问题——仅一条遗留的小建议。
Reviewed changes — 增量提交 4d7eaa2 给 onUpdateAvailable 异步闭包补了一条注释，说明 UPDATE_AVAILABLE 属于 PRESERVED_KEYS、是 UI 标记，由 onInstalled 在 reload 后清除。这回应了前一轮关于「UPDATE_AVAILABLE 残留」的两条行内反馈。
onUpdateAvailableIIFE 补注释说明UPDATE_AVAILABLE有意保留 — 明确该 key 不会被clearPendingUpdate/clearAllStorage清除，而是由新版本安装后的onInstalled统一清理。注释把「为什么残留是良性的」固化在代码里，避免后续维护者重复质疑。- 删除冗余
RACE:测试、补 SW 重启端到端断言（commit7412971，本轮回顾确认）— 删掉了用(svMock as any).setStorageValue = slowSet在 import 后替换的不可靠 mock，保留的does not persist the marker after an idle reload already fired用vi.resetModules()+ 重新 import 断言 reload 仍只发生 1 次，闭环更扎实。
anthropic/glm-5.2 ｜ 𝕏
| void (async () => { | ||
| // Persist the marker before consuming it: tryApplyUpdate clears storage, | ||
| // so an un-awaited write could land after the clear and survive the | ||
| // reload, re-firing the loop on the next boot. Guard each write: if a | ||
| // reload already fired (e.g. idle retry raced ahead while we were | ||
| // awaiting), stop — a late write would resurrect the cleared marker. | ||
| // UPDATE_AVAILABLE is intentionally preserved (PRESERVED_KEYS): it is a | ||
| // UI flag cleared by onInstalled after the reload. | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); | ||
| await tryApplyUpdate(); | ||
| })(); |
There was a problem hiding this comment.
ℹ️ 这个 async IIFE 没有 .catch()——若 setStorageValue reject（storage 配额超限、SW 被 Chrome 终止），会产生未捕获的 Promise rejection。文件内 runAfterServiceWorkerBoot（:81）和 setPanelBehavior（:97）都用 .catch() 兜底，建议对齐该模式。
Technical details
# IIFE 缺少 .catch() 兜底
## Affected sites
- `chrome-crx/src/service-worker.ts:259-272` — `onUpdateAvailable` 内的 `void (async () => { ... })()` 无 `.catch()`，两次 `await setStorageValue` 与 `await tryApplyUpdate` 的 rejection 无处着陆。
## Required outcome
- 捕获 IIFE 内任意 await 抛出的异常，避免未捕获 rejection；与文件内 `runAfterServiceWorkerBoot`（:78-83）的 `.catch((err) => console.warn(...))` 模式一致。
## Suggested approach
```ts
void (async () => {
// ...existing guards and writes...
await tryApplyUpdate();
})().catch((err) => {
console.warn('[superduck] failed to persist pending update marker', err);
});
```
## Open questions for the human
- 同文件 alarm handler（:314-319）同样无 `.catch()`，是否要一并补齐？代码库当前模式不完全统一。There was a problem hiding this comment.
已在 8d0cc53 补上 .catch()，与 runAfterServiceWorkerBoot 的兜底模式对齐。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
已在 8d0cc53 补上 .catch((err) => console.warn('[superduck] failed to persist pending update', err))，与 runAfterServiceWorkerBoot 的兜底模式对齐。
anthropic/glm-5.2 ｜ 𝕏
| setTimeout(() => { | ||
| updateReloadInFlight = false; | ||
| }, 5000); |
There was a problem hiding this comment.
在 chrome.runtime.reload() 之后设置 setTimeout 存在时序隐患：如果 reload 成功，Service Worker 会被终止，setTimeout 回调不会执行（但此时内存变量也随 SW 销毁，无影响）；如果 reload 未生效且 SW 存活，5 秒后重置是合理的。
但更关键的问题是：reload() 调用后到 SW 实际终止之间可能存在一个时间窗口，在此窗口内 onUpdateAvailable 监听器的 async IIFE 仍可能在运行。由于 updateReloadInFlight 为 true，IIFE 会在 guard check 处提前返回，不会写入 storage——这是预期行为，逻辑正确。
建议：可以考虑将 5000ms 提取为具名常量（如 RELOAD_GRACE_PERIOD_MS），便于后续维护和理解。
建议修改:
| setTimeout(() => { | |
| updateReloadInFlight = false; | |
| }, 5000); | |
| const RELOAD_GRACE_PERIOD_MS = 5000; | |
| setTimeout(() => { | |
| updateReloadInFlight = false; | |
| }, RELOAD_GRACE_PERIOD_MS); |
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 新提交 b757c6c 与 7412971 收紧 residual race：onUpdateAvailable 异步闭包在每次 marker 写入前增加 updateReloadInFlight 守卫；tryApplyUpdate 在 chrome.runtime.reload() 后追加 5s setTimeout 兜底以覆盖 SW 静默未终止场景；测试剔除无效的 RACE mock 并扩展 storage-gated 测试覆盖完整 SW 重启闭包。
onUpdateAvailable闭包逐写入检查 guard — 两次setStorageValue调用前各检查updateReloadInFlight，若 idle 回调已抢先触发 reload 则中止后续写入，杜绝迟到写入复活已清除 marker 的链路。tryApplyUpdatesetTimeout 兜底 —chrome.runtime.reload()后调度setTimeout(() => { updateReloadInFlight = false; }, 5000)，覆盖 reload 静默失败导致 guard 永久卡住的场景；正常 reload 时 SW 终止会自动清理定时器。- 测试收紧 — 移除无效 RACE 测试（post-import 改 mock 被 vitest 忽略），扩展 storage-gated 测试断言 SW 重启后 reload 仍为单次且无 stale marker。
注：prior threads 4861095406 / 4861108495 的建议已在 1985611 / b757c6c 字面落实，已在对应 thread 回复；thread resolve 因 integration 权限受限未自动完成，需要人工点击 Resolve。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 增量未发现新问题。
Reviewed changes — 增量提交 8d0cc53 给 onUpdateAvailable 异步 IIFE 补了 .catch()，吞掉两次 setStorageValue / tryApplyUpdate 的 rejection，与文件内 runAfterServiceWorkerBoot（:78-83）的兜底模式对齐。这关闭了上一轮 review 4861153773 的行内反馈。
onUpdateAvailableIIFE 末尾追加.catch((err) => console.warn('[superduck] failed to persist pending update', err))—chrome-crx/src/service-worker.ts:272-274。storage 配额超限或 SW 终止导致的 reject 不再产生未捕获 Promise rejection；console.warn前缀与消息格式和文件内既有用法一致。
｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 未发现新问题——仅一处测试可读性建议。
Reviewed changes — 增量提交 7412971 是纯测试调整：删除了用 vi.fn 覆盖 setStorageValue 的 RACE 测试（作者判定无效），并将 does not persist the marker after an idle reload already fired 扩展为"full closure"——释放 gated write 后用 vi.resetModules() + 重新 import 模拟 SW 重启，断言 reload 仍只触发一次。
- 删除
RACE测试 — 该测试直接覆写setStorageValue为慢 mock，绕过了真实的 storage 写入路径，无法验证写入-清除顺序。 - 新增 full-closure 断言 — 在 gated write 释放 + 20ms settle 后执行
vi.resetModules()+await import('./service-worker')模拟 SW 重启，断言replayPendingUpdateIfAny不会因残留 marker 再次 reload。覆盖了端到端语义。
验证：bun run test -- service-worker.boot.test.ts 13/13 通过，bun run typecheck / bun run lint 0 错误。
anthropic/glm-5.2 ｜ 𝕏
| // the reload decision — otherwise the next boot re-reloads. | ||
| releaseWrite!(); | ||
| await new Promise((resolve) => setTimeout(resolve, 20)); | ||
| expect(storageLocalMock._store['pendingUpdateVersion']).toBeUndefined(); |
There was a problem hiding this comment.
这里的 writeGate/releaseWrite 脚手架实际是死代码：onUpdateAvailable 与 idleCallback!() 之间没有 await，IIFE 在第一次 await setStorageValue(UPDATE_AVAILABLE, true) 处挂起时 idle 回调已同步执行并把 updateReloadInFlight 置为 true。因此 IIFE 恢复后第二个 if (updateReloadInFlight) return（service-worker.ts:267）直接短路，gated 的 PENDING_UPDATE_VERSION 写入从未启动——releaseWrite() 实际没有可释放的东西。
测试本身仍然有效（验证了护栏 + 重启不 re-fire），但 "Release the closure's write. It must NOT resurrect the marker" 这段注释描述的场景并未发生。若要真正覆盖"写入确实 in-flight 时 idle 抢先"的竞态，需要在 onUpdateAvailable 后、idleCallback 前插入一个 await（如 await new Promise(r => setTimeout(r, 0))）让 IIFE 推进到 gated 写入处。
Technical details
# Gated write scaffolding is unreachable
## Affected sites
- `chrome-crx/src/service-worker.boot.test.ts:505-517` — `writeGate`/`releaseWrite` 设置，gate 仅在 `values['pendingUpdateVersion'] !== undefined` 时触发
- `chrome-crx/src/service-worker.boot.test.ts:532-536` — "Release the closure's write" 注释 + 断言；实际无可释放的写入
## Required outcome
- 要么在 `onUpdateAvailable` 触发后插入一个微任务/宏任务 yield（如 `await new Promise(r => setTimeout(r, 0))`），让 IIFE 推进到 `await setStorageValue(PENDING_UPDATE_VERSION, ...)` 并在 gate 上阻塞，从而使 `releaseWrite()` 真正释放一个 in-flight 写入
- 要么删除 gate 脚手架与误导性注释，明确该测试覆盖的是"护栏在 IIFE 恢复时短路"而非"gated 写入释放后不残留"
## Suggested approach
在 `fixtures.onUpdateAvailable.listeners[0]({ version: '1.1.0' })` 之后、`fixtures.isAgentActive.mockReturnValue(false)` 之前插入：
```ts
await new Promise((r) => setTimeout(r, 0));
```
这样 IIFE 会推进到 gated 写入并阻塞，`releaseWrite()` 才会真正释放一个 in-flight 的 `PENDING_UPDATE_VERSION` 写入。注意：释放后该写入会落盘，此时需要依赖 `tryApplyUpdate()` 内的护栏阻止第二次 reload——这正是 full-closure 断言（537-543）要验证的。
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 新提交 4d7eaa2 与 8d0cc53 为小幅打磨：补充 UPDATE_AVAILABLE 故意保留（PRESERVED_KEYS）的注释，并为 onUpdateAvailable async 闭包追加 .catch 把 marker 持久化失败转为 console.warn 而非 unhandled rejection。
onUpdateAvailable闭包追加.catch— IIFE 链上})().catch((err) => console.warn('[superduck] failed to persist pending update', err))，让setStorageValuereject 等场景可见且不外溢为 unhandled promise rejection；tryApplyUpdate内部已有自己的 try/catch，不会由此路径抛出。- 注释补充
UPDATE_AVAILABLE的保留语义 — 在闭包注释中标注该 key 在PRESERVED_KEYS中故clearAllStorage不清除，由onInstalled在 reload 后清理，避免后续维护者误改。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 增量提交 7412971 是纯测试重构：移除一个 ineffective 的 RACE 测试，把"SW 重启后不再触发 reload"的端到端断言合并进现有 race 测试。生产代码未变更。
- 移除 ineffective 的
RACE: idle reload must not be followed by a stale marker write测试 — 该用例在service-worker.ts已捕获setStorageValue引用之后，才动态替换extensionServices.setStorageValue，对闭包已绑定的本地引用无效，无法真正触发 race 场景。 - 为现有 race 测试新增 SW 重启断言 —
does not persist the marker after an idle reload already fired在 release 写入门控后追加vi.resetModules()+ 重新import('./service-worker')，模拟 SW 重启并断言chrome.runtime.reload仍只被调用 1 次（marker 未复活 →replayPendingUpdateIfAny不会再次触发 reload），把修复的端到端契约锁死。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 增量提交 4d7eaa2 + 8d0cc53：前者给 IIFE 补注释说明 UPDATE_AVAILABLE 为何保留（PRESERVED_KEYS + onInstalled 清除），后者把 void (async () => {...})() 改为 .catch(err => console.warn(...)) 吞掉标记持久化的 rejected promise。
UPDATE_AVAILABLE生命周期注释 — 回应了先前 review 对该标记残留的疑问；纯文档，无行为变化。- IIFE
.catch兜底 —setStorageValue失败时原本会变成 unhandled rejection（被void修饰），现在改为console.warn记录。tryApplyUpdate自身已有 try/catch 不会抛出，故.catch只覆盖持久化失败路径；行为合理——存储失败时 update 本就无法应用，记日志后由后续onUpdateAvailable重试。
验证：bun run test -- service-worker.boot.test.ts 13/13 通过。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 未发现新问题。
Reviewed changes — 两个新提交响应了上轮对 UPDATE_AVAILABLE 残留的疑问，并为 onUpdateAvailable 闭包补上 unhandled rejection 兜底。
service-worker.ts:265-266新增UPDATE_AVAILABLE生命周期注释（4d7eaa2） — 标注该键在PRESERVED_KEYS中、作为 UI 标记由onInstalled在 reload 后清除；与extensionServices/core.ts:116（PRESERVED_KEYS包含updateAvailable）和sidepanel/hooks/useInitialization.ts:122（hasUpdateUI 状态）的实际用法一致。service-worker.ts:272-274为 IIFE 添加.catch()兜底（8d0cc53） —setStorageValue失败时不再变成 unhandled promise rejection，改为console.warn('[superduck] failed to persist pending update', err)。tryApplyUpdate已自带 try/catch 不会冒泡到这里，所以 catch 实际只覆盖setStorageValue失败路径；失败语义合理：marker 没持久化，下次 boot 的replayPendingUpdateIfAny读不到 marker 直接跳过，不会卡 reload 循环。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: claude
Model: anthropic/glm-5.2
✅ 增量未发现新问题。
Reviewed changes — 增量提交 8d0cc53 给 onUpdateAvailable 的 async IIFE 补上 .catch() 兜底，落地了上一轮两条同源反馈（review 4861153773 / 4861146111）。
onUpdateAvailableIIFE 加.catch()兜底 — 把void (async () => { ... })()改为void (async () => { ... })().catch((err) => console.warn('[superduck] failed to persist pending update', err))，与文件内runAfterServiceWorkerBoot（service-worker.ts:81）和tryApplyUpdatecatch 块（service-worker.ts:226）的错误处理模式对齐。两次await setStorageValue与await tryApplyUpdate的 rejection 不再变成未捕获的 Promise rejection；tryApplyUpdate自身已有 try/catch，这里主要兜底 storage 写入失败这一类环境异常。
｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
DuckPR's review exposed that the in-flight guard alone cannot stop the race: when onUpdateAvailable's PENDING_UPDATE_VERSION write is already in flight, an idle reload's clear can finish first and the late write resurrects the marker on the next boot. Serialize all marker operations (onUpdateAvailable persistence, idle retry) through a promise chain so a write can never interleave with a clear+reload. The guard still bails out of queued work once a reload has fired. Race test now gates the marker write, yields so the closure blocks in-flight, then asserts the write lands before the single reload and nothing survives a simulated restart. Test isolation: beforeEach restores the storage fake's plain set/remove implementations.
|
好眼力 ✅ 已按建议修正（commit 5759047）。 按你建议插入 最终修复改为操作队列串行化（ 测试现在验证：写入 in-flight → idle 触发 → 写入先落盘（不残留）→ reload 恰好 1 次 → 重启后不再 reload。725/725 通过。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5759047e25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.UPDATE_AVAILABLE, true); | ||
| if (updateReloadInFlight) return; | ||
| await setStorageValue(StorageKeys.PENDING_UPDATE_VERSION, details.version); |
There was a problem hiding this comment.
When an onUpdateAvailable event arrives while cold-start boot is still before replayPendingUpdateIfAny(), this queued write can be in flight while boot calls tryApplyUpdate() directly outside updateOperationChain; boot can clear/remove the marker and call reload(), then this PENDING_UPDATE_VERSION write lands afterward, leaving a stale marker that the next service-worker boot replays into another reload. Fresh evidence in this revision is that idle retries were moved onto the queue, but the boot replay path remains unqueued, so the same write-after-clear race still exists for cold-start updates.
Useful? React with 👍 / 👎.
![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
🤖 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 `@chrome-crx/src/service-worker.ts`:
- Around line 217-220: Update clearPendingUpdate so pendingUpdateVersion is set
to null only after removeStorageValues succeeds, preserving the marker when
removal rejects. Extend the failure-recovery test for
clearPendingUpdate/tryApplyUpdate to invoke the idle callback after the one-time
removal failure and verify the retry occurs.
🪄 Autofix
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 Plus
Run ID: 2a39eb69-d916-4d27-b0fa-327988781cdc
📒 Files selected for processing (2)
chrome-crx/src/service-worker.boot.test.tschrome-crx/src/service-worker.ts
| async function clearPendingUpdate(): Promise<void> { | ||
| pendingUpdateVersion = null; | ||
| await chrome.storage.local.remove(StorageKeys.PENDING_UPDATE_VERSION as string); | ||
| await removeStorageValues(StorageKeys.PENDING_UPDATE_VERSION); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve pendingUpdateVersion when marker removal fails.
Line 218 clears the in-memory version before removeStorageValues completes. If removal rejects, tryApplyUpdate resets the reload guard, but a later idle callback returns because pendingUpdateVersion is null. The persisted marker then retries only after a new update event or a service-worker restart.
Clear the in-memory version only after successful removal. Extend the failure-recovery test to invoke the idle callback after the one-time removal failure.
Proposed fix
async function clearPendingUpdate(): Promise<void> {
- pendingUpdateVersion = null;
await removeStorageValues(StorageKeys.PENDING_UPDATE_VERSION);
+ pendingUpdateVersion = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function clearPendingUpdate(): Promise<void> { | |
| pendingUpdateVersion = null; | |
| await chrome.storage.local.remove(StorageKeys.PENDING_UPDATE_VERSION as string); | |
| await removeStorageValues(StorageKeys.PENDING_UPDATE_VERSION); | |
| } | |
| async function clearPendingUpdate(): Promise<void> { | |
| await removeStorageValues(StorageKeys.PENDING_UPDATE_VERSION); | |
| pendingUpdateVersion = null; | |
| } |
🤖 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 `@chrome-crx/src/service-worker.ts` around lines 217 - 220, Update
clearPendingUpdate so pendingUpdateVersion is set to null only after
removeStorageValues succeeds, preserving the marker when removal rejects. Extend
the failure-recovery test for clearPendingUpdate/tryApplyUpdate to invoke the
idle callback after the one-time removal failure and verify the retry occurs.
… log The persisted swBootCount + timestamp makes SW restarts visible in the panel, distinguishing normal 30s idle recycling from a reload/crash loop (verified stable in the field: boot stays at #1 during active use). nativeHost 'connected' was logged at warn level — it is a normal event, demote to log.
背景
PR #312 修复了
pendingUpdateVersion只写不消费导致的 reload 循环（已合并）。本 PR 处理 #312 遗留的 3 个竞态问题，由 CodeRabbit / Copilot / Gemini Code Assist / DuckPR 的 review 反馈驱动。修复内容
onInstalled移除标记改为 await 且先于 boot：原先 fire-and-forget 的remove与ensureServiceWorkerBooted并发，boot 内的replayPendingUpdateIfAny可能读到尚未删除的残留标记。onUpdateAvailable先持久化后消费：原先setStorageValue未 await 就调用tryApplyUpdate，写入可能晚于clearPendingUpdate的删除而残留在 storage，下次 boot 再次触发循环。tryApplyUpdate增加updateReloadInFlight原子护栏：idle 回调与onUpdateAvailableasync 闭包并发到达时，保证只 reload 一次（有回归测试锁定）。removeStorageValues帮助函数：规避@types/chrome对StorageArea.remove参数的 enum 字面量窄化，去掉冗余as string。测试
setStorageValuemock 改为真实写入 storage fake，覆盖写入-消费的真实顺序。关联 issue：#311
Summary by CodeRabbit
Bug Fixes
Diagnostics
Logging