test(updates): macOS auto-update E2E with real install + relaunch#2828
test(updates): macOS auto-update E2E with real install + relaunch#2828charlesvien wants to merge 11 commits into
Conversation
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
.github/workflows/code-update-e2e.yml:10-14
**Temporary push trigger not removed before merge**
The PR description explicitly states this push trigger "is meant to be removed before merge." It's still present. If the source branch (`test/macos-auto-update-e2e`) continues to exist in the repo after the merge, any future push to it will fire this workflow — which runs two full signed macOS builds and exercises the real install. That's expensive (and confusing) for what should be a nightly-only job.
### Issue 2 of 3
apps/code/tests/e2e/tests/update.spec.ts:63-130
**`app` and `updated` not closed in finally**
If `pollStatus` for check/download times out (phase 1 fails), the `app` Electron process is left running — only `feed.kill()` is in the `finally` block. Similarly, if the `expect(version).toBe(NEW_VERSION)` assertion at line 126 fails, `updated` is leaked. Playwright does not automatically close `electron.launch()` instances on test failure. Adding `app.close().catch(() => {})` / `updated.close().catch(() => {})` to the `finally` block (tracked via `let app` / `let updated` in outer scope) would prevent stale processes from interfering with a retry.
### Issue 3 of 3
apps/code/scripts/dev-update/serve.mjs:50-52
**Suffix-range requests (`bytes=-N`) compute wrong offsets**
When the range header is `bytes=-500` (last 500 bytes), the regex captures `match[1] = ""` and `match[2] = "500"`. Because `""` is falsy, `start` is set to `0` instead of `stat.size - 500`, so the response sends the wrong slice of the file. The electron-updater differential/blockmap code can emit suffix ranges during verification. In practice `electron-updater` on this path likely uses open-ended ranges (`bytes=0-`), but a defensive fix would be: `const start = match?.[1] ? Number(match[1]) : match?.[2] ? stat.size - Number(match[2]) : 0`.
Reviews (1): Last reviewed commit: "add macos auto-update e2e harness and ci" | Re-trigger Greptile |
| # Temporary: also run on this branch so the harness can be exercised on the PR. | ||
| # Remove this push trigger once merged; nightly + dispatch is the steady state. | ||
| push: | ||
| branches: | ||
| - test/macos-auto-update-e2e |
There was a problem hiding this comment.
Temporary push trigger not removed before merge
The PR description explicitly states this push trigger "is meant to be removed before merge." It's still present. If the source branch (test/macos-auto-update-e2e) continues to exist in the repo after the merge, any future push to it will fire this workflow — which runs two full signed macOS builds and exercises the real install. That's expensive (and confusing) for what should be a nightly-only job.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/code-update-e2e.yml
Line: 10-14
Comment:
**Temporary push trigger not removed before merge**
The PR description explicitly states this push trigger "is meant to be removed before merge." It's still present. If the source branch (`test/macos-auto-update-e2e`) continues to exist in the repo after the merge, any future push to it will fire this workflow — which runs two full signed macOS builds and exercises the real install. That's expensive (and confusing) for what should be a nightly-only job.
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| // Phase 1: drive the real download + install on the old build. | ||
| const app = await electron.launch({ | ||
| executablePath: RUN_APP_BIN, | ||
| args: [], | ||
| env: { | ||
| ...process.env, | ||
| ELECTRON_DISABLE_GPU: "1", | ||
| POSTHOG_E2E_UPDATE_FEED: FEED_URL, | ||
| }, | ||
| }); | ||
|
|
||
| await expect | ||
| .poll( | ||
| () => app.evaluate(() => typeof (globalThis as Hooked).__e2eUpdates), | ||
| { | ||
| timeout: 30_000, | ||
| message: "update hook was never installed", | ||
| }, | ||
| ) | ||
| .toBe("object"); | ||
|
|
||
| await app.evaluate(() => (globalThis as Hooked).__e2eUpdates.check()); | ||
| await pollStatus( | ||
| app, | ||
| (s) => s.available === true && s.availableVersion === NEW_VERSION, | ||
| "update never became available", | ||
| ); | ||
|
|
||
| await app.evaluate(() => (globalThis as Hooked).__e2eUpdates.download()); | ||
| await pollStatus( | ||
| app, | ||
| (s) => s.updateReady === true, | ||
| "update never finished downloading", | ||
| ); | ||
|
|
||
| const closed = app.waitForEvent("close"); | ||
| void app | ||
| .evaluate(() => { | ||
| void (globalThis as Hooked).__e2eUpdates.install(); | ||
| }) | ||
| .catch(() => undefined); | ||
| await closed; | ||
|
|
||
| // Phase 2: prove the bundle swapped and a fresh launch is the new version. | ||
| await waitUntil( | ||
| () => readBundleVersion(RUN_APP) === NEW_VERSION, | ||
| 120_000, | ||
| "bundle was not swapped to the new version", | ||
| ); | ||
| killApp(); | ||
| await waitUntil( | ||
| () => !isAppRunning(), | ||
| 30_000, | ||
| "relaunched instance did not exit", | ||
| ); | ||
|
|
||
| const updated = await electron.launch({ | ||
| executablePath: RUN_APP_BIN, | ||
| args: [], | ||
| env: { ...process.env, ELECTRON_DISABLE_GPU: "1" }, | ||
| }); | ||
| const version = await updated.evaluate(({ app: a }) => a.getVersion()); | ||
| expect(version).toBe(NEW_VERSION); | ||
| await updated.close(); | ||
| } finally { | ||
| feed.kill(); | ||
| } |
There was a problem hiding this comment.
app and updated not closed in finally
If pollStatus for check/download times out (phase 1 fails), the app Electron process is left running — only feed.kill() is in the finally block. Similarly, if the expect(version).toBe(NEW_VERSION) assertion at line 126 fails, updated is leaked. Playwright does not automatically close electron.launch() instances on test failure. Adding app.close().catch(() => {}) / updated.close().catch(() => {}) to the finally block (tracked via let app / let updated in outer scope) would prevent stale processes from interfering with a retry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/tests/e2e/tests/update.spec.ts
Line: 63-130
Comment:
**`app` and `updated` not closed in finally**
If `pollStatus` for check/download times out (phase 1 fails), the `app` Electron process is left running — only `feed.kill()` is in the `finally` block. Similarly, if the `expect(version).toBe(NEW_VERSION)` assertion at line 126 fails, `updated` is leaked. Playwright does not automatically close `electron.launch()` instances on test failure. Adding `app.close().catch(() => {})` / `updated.close().catch(() => {})` to the `finally` block (tracked via `let app` / `let updated` in outer scope) would prevent stale processes from interfering with a retry.
How can I resolve this? If you propose a fix, please make it concise.| const match = /bytes=(\d*)-(\d*)/.exec(range); | ||
| const start = match?.[1] ? Number(match[1]) : 0; | ||
| const end = match?.[2] ? Number(match[2]) : stat.size - 1; |
There was a problem hiding this comment.
Suffix-range requests (
bytes=-N) compute wrong offsets
When the range header is bytes=-500 (last 500 bytes), the regex captures match[1] = "" and match[2] = "500". Because "" is falsy, start is set to 0 instead of stat.size - 500, so the response sends the wrong slice of the file. The electron-updater differential/blockmap code can emit suffix ranges during verification. In practice electron-updater on this path likely uses open-ended ranges (bytes=0-), but a defensive fix would be: const start = match?.[1] ? Number(match[1]) : match?.[2] ? stat.size - Number(match[2]) : 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/scripts/dev-update/serve.mjs
Line: 50-52
Comment:
**Suffix-range requests (`bytes=-N`) compute wrong offsets**
When the range header is `bytes=-500` (last 500 bytes), the regex captures `match[1] = ""` and `match[2] = "500"`. Because `""` is falsy, `start` is set to `0` instead of `stat.size - 500`, so the response sends the wrong slice of the file. The electron-updater differential/blockmap code can emit suffix ranges during verification. In practice `electron-updater` on this path likely uses open-ended ranges (`bytes=0-`), but a defensive fix would be: `const start = match?.[1] ? Number(match[1]) : match?.[2] ? stat.size - Number(match[2]) : 0`.
How can I resolve this? If you propose a fix, please make it concise.97fb702 to
d07a09a
Compare
b6f48b2 to
afd5861
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Gates denied because this PR adds a new CI/CD workflow file (.github/workflows/code-update-e2e.yml), which is on the infra/CI deny-list and requires human review. Additionally, there are unresolved bot comments flagging a push trigger that was explicitly noted for removal before merge but wasn't removed, and a functional bug in the range-request handling in serve.mjs.
71f5705 to
817d137
Compare
817d137 to
8257a56
Compare
f2656c7 to
482f3bf
Compare

Problem
The electron-builder + electron-updater migration gave us a real auto-update path (download, install, relaunch), but nothing tests it end to end.
Changes
Two tiny test-gated hooks in the main process, both inert unless
POSTHOG_E2E_UPDATE_FEEDis set (no production behavior change):electron-updater.tspoints the updater at a local feed viasetFeedURL.disableDifferentialDownload,autoDownload,autoInstallOnAppQuitandisSupportedare untouched.main/index.tsexposesglobalThis.__e2eUpdates(check/download/install/status) wired to the realUpdatesService.Harness:
scripts/dev-update/serve.mjs: dependency-free, range-capable static feed server.scripts/dev-update/build-pair.sh: builds a signed2.0.0feed and a runnable signed1.0.0app (SKIP_NOTARIZE=1).tests/e2e/fixtures/update.ts:dittorun-copy so a retry restarts from1.0.0, plist version read, pkill helpers.tests/e2e/tests/update.spec.ts: phase 1 drives download then install via the main-process hook; phase 2 waits for the on-disk bundle to swap, kills the auto-relaunched instance, then fresh-launches and assertsapp.getVersion()is2.0.0.CI: a dedicated
code-update-e2e.yml, macOS only, reusing the existingbuild-macossteps and Apple signing secrets. It is not on the PR gate (nightly +workflow_dispatch); a temporary push trigger on this branch lets it run on the PR and is meant to be removed before merge.The swap is signature-gated (Squirrel.Mac matches the designated requirement), which is why this runs in CI where the signing cert is present rather than as a local unit test. Notarization is skipped on purpose: it is a Gatekeeper concern, not what the in-place update verifies.
How did you test this?
Ran locally:
pnpm --filter code typecheckpasses with the hooks.node --check serve.mjsandbash -n build-pair.shpass.smoke.spec.ts).Not run locally: the full signed build plus end-to-end swap (two signed mac builds, needs the Developer ID cert). That runs in the new workflow on this branch via the temporary push trigger, which is the real validation.
Automatic notifications