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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent 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). (5)
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbitBug Fixes
WalkthroughAppUpdateController adds per-request versioning to ensure 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 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)
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?
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.