Repository navigation
fix(updater): pin stable fallback before downloading - #34076
Conversation
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The updater implementation, fixtures, and updater tests support [#34034]. The PR also adds unrelated worktree behavior tests in src/main/ipc/worktrees-create-label-only-first-work.test.ts and `src/m… |
Remove the unrelated worktree and native-chat changes from this PR, or move them to a separate PR. Keep the updater implementation, updater tests, and updater fixtures. |
| Docstring Coverage | |
Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 15 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change: pinning the updater’s stable fallback to a concrete release before downloading. Linked Issues check Issue [ #34034] reports a mismatch between the release tag and installer filename.UpdaterReleaseFeed.pinDefaultReleaseFeedresolvesfetchLatestStableReleaseTag()before the fallback feed and then …Description check The pull request description is complete and follows the repository template. It includes the user impact, implementation details, rationale, linked issue, visual-proof explanation, testing evidence, … Full details: Linked Issues check
-
Explanation
Issue [
#34034] reports a mismatch between the release tag and installer filename.UpdaterReleaseFeed.pinDefaultReleaseFeedresolvesfetchLatestStableReleaseTag()before the fallback feed and then usesgetReleaseDownloadUrl(stableTag).fetchLatestStableReleaseTagvalidates the GitHub release redirect and checks the platform manifest and assets. The updater tests cover stable fallback, unavailable fallback, and expired checks.
Full details: Description check
-
Explanation
The pull request description is complete and follows the repository template. It includes the user impact, implementation details, rationale, linked issue, visual-proof explanation, testing evidence, review notes, scope considerations, and checklist status. The AI Disclosure section is empty, which is acceptable for an internal contributor and does not affect the completeness of the description.
-
Full details: Out of Scope Changes check
-
Explanation
The updater implementation, fixtures, and updater tests support [
#34034]. The PR also adds unrelated worktree behavior tests insrc/main/ipc/worktrees-create-label-only-first-work.test.tsandsrc/main/runtime/runtime-local-worktree-create-pr-conflicts.test.ts, removessrc/main/ipc/worktrees-create-metadata-persistence.test.tsandsrc/main/runtime/runtime-local-worktree-create.test.ts, and adds the unrelatedStructuredChatPaneMenuItemsrenderer component. These changes do not support updater release selection or download consistency.Resolution
Remove the unrelated worktree and native-chat changes from this PR, or move them to a separate PR. Keep the updater implementation, updater tests, and updater fixtures.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ff0f6a1a-d11b-48a2-a400-230776b5f7cb
-
src/main/updater-latest-stable-feed.test.tssrc/main/updater-net-request.fixture.tssrc/main/updater-prerelease-feed.tssrc/main/updater-test-harness.tssrc/main/updater.build-channel-selection.test.tssrc/main/updater.check-failure.test.tssrc/main/updater.feed-attempt-lifetime.test.tssrc/main/updater.install-failure-cause.test.tssrc/main/updater.mac-install.test.tssrc/main/updater.publishing-window-feed.test.tssrc/main/updater/updater-release-feed.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
-
ELI5
Orca can remember one release's ZIP filename and later follow “latest” into a newer release where that file does not exist. Resolve “latest” once, then keep the manifest and download on that concrete release.
What Changed
/releases/latestredirect.net.requestHEAD handling and platform manifest/asset readiness probes.net.fetchcancels manual redirects in real Electron, so it is not used for redirect discovery.Why
The normal path already pins a tag, but Atom outages and no-newer results still installed a
/releases/latest/downloadprovider. That provider retains relative filenames while GitHub's redirect can move between checking and downloading. Correct release manifests alone cannot prevent the mismatch.Credit to @brennanb2025 for the original tag-pinning fix in #1797, and @nwparker for the attempt-ownership guard in #23160, which this change extends across the fallback lookup. Also reviewed @sechan-netizen's #30952 and its review thread: its release-manifest validation is a complementary safeguard because this race reproduces with correct manifests. No commits from that PR are copied here. That related proposal is now closed during cleanup, with an invitation to reopen for a concrete remaining manifest-integrity case.
This PR adopts @brennanb2025's audited #34075 lint fix (splitting three files that already exceeded main's line limits), with Brennan retained as a co-author. Independent byte-level audits preserve the moved runtime bodies and test groups; the follow-up replaces five test-only assertions with typed calls and emits identical JavaScript. The standalone PR is now closed as adopted; its earlier tree had repeated daemon-test CI/order sensitivity, while the combined tree here passed all CI, including that exact daemon test file (16 tests) in run 40398604134, job 121839803430.
Linked Issue
Fixes #34034
Related reports #15921 and #22621 are also closed. All three reports have a fix link and a request to reopen if the failure recurs after updating to a release/build containing the fix. The controlled race matches their URL signature and retry behavior; the reporter's original feed selection cannot be established without its logs.
Visual Proof
N/A — updater feed selection only; no presentation change. Verified the HTTP behavior using the production fallback resolver and electron-updater 6.8.10 with unmodified v1.4.240/v1.4.241 manifests and a controlled local release server:
v1.4.241/Orca-1.4.240-arm64-mac.zip: 404No installer was executed and no app window was opened.
Testing
ORCA_BACKGROUND_LAUNCH=1 pnpm check:changed --maxWorkers=2: all 26 checks pass on the combined branch, including formatting, lint, typechecks, affected tests, and ratchets. The earlier updater-only run also passed all 20 selected checks.The controller regression run against the original implementation produces five failures with passing controls. The fixed implementation passes. Resolver tests exercise macOS arm64, Windows x64, Linux x64/arm64 manifest selection, invalid redirects, missing assets, and recovery after network failure; these platform variants use mocked transport.
A separate background Electron 43.8.1 smoke on macOS arm64 uses the production resolver and real Electron networking against GitHub, resolving the ready
v1.4.241tag. The controlled before/after reproduction also exercises the production resolver and actual GenericProvider. Neither proof exercises native installation.All non-skipped GitHub CI checks pass for combined head
c04a6ca95604e064fb4f91adb1a57ea3dcf9adf0: all five full unit shards, relay integration, Linux/Windows package checks, macOS updater regression tests, static analysis/typechecks, and the required aggregate check. Final independent review recommends merging that exact head as low risk with no code blockers.AI Disclosure
Review
Independent sub-agent review found no actionable issues and rated the updater change low risk; 57 relevant tests passed independently. The prerequisite received two independent refactor audits, with identical moved runtime/test bodies and a merge-preservation gate reporting zero unexplained deletions and zero unrelated files. The five typed mock changes produce identical JavaScript. Also self-reviewed the feed-selection paths, timeout ownership, transport cleanup, and release-channel filtering. The added lookup runs only on the existing fallback path, uses HEAD for redirect discovery, retains five-second request deadlines, and reuses manifest/asset probes. No new polling, subprocess, persistent cache, or per-workspace work is introduced. Native Windows/Linux networking and installer execution were not exercised locally.
Review disposition for the DMG-only manifest comment: an independent executable comparison of the original and fixed implementations returned the same
readyAtom result for a DMG-only manifest; the old fallback already handed that manifest directly to MacUpdater, which cannot select its required ZIP. This is an existing release-integrity limitation, not introduced or worsened by tag pinning. It remains outside this race fix; a complete integrity change must validate the native files list and architecture selection rather than merely finding any ZIP-like name.Agent skill upstream boundary
Notes
Desktop updater implementation is shared with headless serve. No IPC/RPC contract, remote version requirement, mobile client code, filesystem path, or workspace-kind assumption changes. Existing RC/perf selection remains in place. If fallback discovery also fails, checks use the existing retry/error flow instead of launching a moving feed.
Checklist