fix(updater): ignore stale check results - #330
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ThunderTr77, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAppUpdateController adds per-request versioning so only the latest check for the active channel can commit completion/failure or persist ChangesStale Update Check Prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 `@apps/desktop/src/services/AppUpdateService/index.ts`:
- Around line 113-117: In setChannel (AppUpdateService), don't
increment/invalidate checkRequestVersion until after the channel write succeeds:
call await this.settings.updateAppUpdateChannel(channel) and await
this.settings.updateAppUpdateLastCheckedAt(null) first, and only then increment
this.checkRequestVersion so in-flight checks aren't incorrectly marked stale if
the persistence fails; adjust the order in the setChannel method accordingly
(references: setChannel, this.settings.updateAppUpdateChannel,
this.settings.updateAppUpdateLastCheckedAt, this.checkRequestVersion).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d72d99e-8170-4fea-b4a4-31163ff6b6fe
📒 Files selected for processing (2)
apps/desktop/src/services/AppUpdateService/index.tsapps/desktop/tests/services/AppUpdateService/service.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Quality
- GitHub Check: Frontend Tests
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
🔇 Additional comments (2)
apps/desktop/src/services/AppUpdateService/index.ts (1)
59-60: LGTM!Also applies to: 130-156, 185-187
apps/desktop/tests/services/AppUpdateService/service.test.ts (1)
173-199: LGTM!Also applies to: 278-306, 308-349
| async setChannel(channel: AppUpdateChannel): Promise<void> { | ||
| await this.initialize(); | ||
| await this.settings.updateAppUpdateChannel(channel); | ||
| this.checkRequestVersion += 1; |
There was a problem hiding this comment.
Potential issue: checkRequestVersion is advanced before updateAppUpdateLastCheckedAt(null) succeeds. If the channel write succeeds but clearing lastCheckedAt fails while a check is in flight, the old check is now treated as stale, while channel-updated is never committed. That can leave the controller stuck in the previous channel's checking state even though the persisted channel has already changed.
Could we cover the updateAppUpdateLastCheckedAt(null) rejection case and either move invalidation until the channel switch is fully committed, or explicitly handle that partial-failure path by committing/rolling back state so the in-flight check does not get stranded?
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/services/AppUpdateService/index.ts (1)
115-119:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHandle
setChannelpartial persistence failure as an atomic state transition.If
updateAppUpdateChannel(channel)succeeds butupdateAppUpdateLastCheckedAt(null)fails,setChannelrejects beforechannel-updatedcommit and before version invalidation. That can leave persisted channel/state out of sync and allow an in-flight pre-switch check to still persist a timestamp.Suggested direction
async setChannel(channel: AppUpdateChannel): Promise<void> { await this.initialize(); await this.settings.updateAppUpdateChannel(channel); - await this.settings.updateAppUpdateLastCheckedAt(null); - this.checkRequestVersion += 1; - this.commit({ type: 'channel-updated', channel }); + try { + await this.settings.updateAppUpdateLastCheckedAt(null); + } catch (error) { + // Channel persistence already succeeded; keep controller state aligned. + this.checkRequestVersion += 1; + this.commit({ type: 'channel-updated', channel }); + throw error; + } + this.checkRequestVersion += 1; + this.commit({ type: 'channel-updated', channel }); }🤖 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 `@apps/desktop/src/services/AppUpdateService/index.ts` around lines 115 - 119, The setChannel flow can leave persisted state inconsistent if updateAppUpdateChannel(channel) succeeds but updateAppUpdateLastCheckedAt(null) fails; make the transition atomic by ensuring both settings updates succeed before mutating in-memory state or committing. In setChannel, read and store the previous channel, perform both settings.updateAppUpdateChannel(channel) and settings.updateAppUpdateLastCheckedAt(null) (or perform last-checked update first), and only after both succeed increment this.checkRequestVersion and call this.commit({ type: 'channel-updated', channel }); if the second update fails, revert by calling settings.updateAppUpdateChannel(previousChannel) (catch/rethrow errors appropriately) so the persisted channel remains consistent.
🤖 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.
Duplicate comments:
In `@apps/desktop/src/services/AppUpdateService/index.ts`:
- Around line 115-119: The setChannel flow can leave persisted state
inconsistent if updateAppUpdateChannel(channel) succeeds but
updateAppUpdateLastCheckedAt(null) fails; make the transition atomic by ensuring
both settings updates succeed before mutating in-memory state or committing. In
setChannel, read and store the previous channel, perform both
settings.updateAppUpdateChannel(channel) and
settings.updateAppUpdateLastCheckedAt(null) (or perform last-checked update
first), and only after both succeed increment this.checkRequestVersion and call
this.commit({ type: 'channel-updated', channel }); if the second update fails,
revert by calling settings.updateAppUpdateChannel(previousChannel)
(catch/rethrow errors appropriately) so the persisted channel remains
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: feeca101-181d-499a-9241-f5d7d04593b0
📒 Files selected for processing (2)
apps/desktop/src/services/AppUpdateService/index.tsapps/desktop/tests/services/AppUpdateService/service.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: CodeQL (rust)
🔇 Additional comments (1)
apps/desktop/tests/services/AppUpdateService/service.test.ts (1)
43-53: LGTM!Also applies to: 75-83
|
This pull request has been inactive for 21 days. Please update it or leave a note if it is still in progress. |
|
Old checks can still commit while |
2 participants
Summary
Fixes a stale updater race where in-flight update checks could finish after the user changed channels or after a newer check completed.
AppUpdateControllerchecks.lastCheckedAtand stale failures from restoring old state.Related issue or RFC
AI assistance disclosure
Testing evidence
corepack pnpm --dir apps/desktop exec vitest run --configLoader runner tests/services/AppUpdateService/service.test.ts tests/services/AppUpdateService/state.test.tspassed: 2 files, 26 testscorepack pnpm --dir apps/desktop exec eslint src/services/AppUpdateService/index.ts tests/services/AppUpdateService/service.test.tspassedcorepack pnpm --dir apps/desktop exec prettier --check src/services/AppUpdateService/index.ts tests/services/AppUpdateService/service.test.tspassedcorepack pnpm --dir apps/desktop run test:typecheckpassedgit diff --checkpassedpnpm test:prwas not run locally; CI is covering the broader matrix.Risk notes
AgentService, runtime, MCP, or schema impact: none.Screenshots or recordings
No UI change; not applicable.
Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof.