Skip to content

fix(update): use system proxy session for app update downloads - #86

Closed
yan-6 wants to merge 0 commit into
freestylefly:mainfrom
yan-6:fix/issue-73
Closed

yan-6 wants to merge 0 commit into
freestylefly:mainfrom
yan-6:fix/issue-73

Conversation

@yan-6

@yan-6 yan-6 commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Update downloads were stalling for users who route traffic through a local proxy (e.g. Clash Verge on macOS). The download function always used session.defaultSession.fetch(), which respects whatever proxy mode has been set on the default session. When the WeSight "use system proxy" preference is off (the default), the session is in direct mode — so users behind a proxy got stuck at the download step.

Root Cause

downloadUpdate in appUpdateInstaller.ts called session.defaultSession.fetch(url, ...). If the app-level proxy preference was not explicitly turned on, the default session ran in direct mode and the download bypassed any system proxy.

Fix

Create a dedicated partition session (persist:wesight-update-download) and set its proxy to system mode before each download. This:

  1. Isolates the proxy override — the default session and all other in-flight requests are unaffected.
  2. Always follows the OS-level proxy configuration for downloads, regardless of the app-level proxy toggle.
  3. Requires no new imports — session is already imported in the file.

Files Changed

  • src/main/libs/appUpdateInstaller.ts

Self-review

  • Change is minimal and self-contained; no logic outside downloadUpdate is touched.
  • session.fromPartition returns the same session object on repeated calls with the same name — safe to call before each download.
  • setProxy({ mode: 'system' }) on the partition session does not affect session.defaultSession.
  • TypeScript compilation clean (npx tsc --noEmit exits 0).
  • ESLint clean on changed file.
  • No existing tests cover the download path; behaviour is consistent with how the rest of the app uses session.defaultSession for other network calls.

Closes #73


Part 2 — the connect phase was still unguarded (2026-09-19)

The proxy session above fixes the routing half of #73. It does not fix the reported symptom. The issue title is "现在卡下载" — stuck downloading — and that state could still last forever.

Root cause

DOWNLOAD_INACTIVITY_TIMEOUT_MS (60s) is only armed after response.body exists:

const response = await updateSession.fetch(url, { signal: controller.signal });  // <- no timer running
// ...
resetInactivityTimer();   // first armed only here, after the body is obtained

A proxy that completes the TCP handshake and then black-holes the request never produces a response. session.fetch() stays pending, no timer is running, and the only escape is the user clicking cancel. That is exactly "卡下载" — and it is the failure mode a misconfigured local proxy produces most often, which makes it the likeliest thing the reporter actually hit.

Fix

  • New DOWNLOAD_RESPONSE_TIMEOUT_MS (60s), armed immediately before fetch and cleared in a finally around it, reusing the existing AbortController so the existing abort-reason handling maps it to the same timeout error message.
  • Clearing it in finally (not after) is what keeps a streaming download from being killed at T+60s — once a response arrives the timer is gone and the inactivity timer takes over.
  • Both timers are now also cleared in the function's outer finally, so no download path can leak a pending timer.

Verification

Harness built with the versions this repo pins (vitest 4.1.0 / typescript 5.7.3 / eslint 9.39.4 / @typescript-eslint 8.57.1 / electron 40.2.1), using the repo's own vitest.config.ts and eslint.config.mjs.

  • New appUpdateResponseTimeout.test.ts: 5 cases, all passing. Combined with the existing session test: 12 passed.
  • Reverse verification: restoring the pre-fix appUpdateInstaller.ts makes 2 of the 5 fail — and they fail by hanging to the 5s vitest timeout, which is the bug reproducing literally rather than an assertion tripping.
  • Two cases are deliberately anti-regression: a streaming download pushed past T+120s must still resolve, and vi.getTimerCount() must be 0 after success.
  • eslint on both changed files: 0 problems (includes simple-import-sort).
  • tsc --strict: appUpdateInstaller.ts clean. The test file reports 2 TS2305 errors for the electron mock's test-only exports — pre-existing convention in this repo, not introduced here: this PR's own appUpdateDownloadSession.test.ts reports 5 errors of the same kind under the identical check.

Self-review

  • The new timer cannot abort an in-flight stream: cleared in finally immediately after fetch settles, before any body reading.
  • Cancel semantics preserved — a user cancel during the response phase still surfaces as "Download cancelled", not as a timeout (covered by a test).
  • No new imports beyond the Session type already imported by Part 1.
  • Chose 60s to match the existing inactivity budget rather than introducing a second number to reason about.

One thing worth a maintainer's eye

Both phases now use 60s, declared as two separate constants. Collapsing them into one constant would be less code but would couple two independent budgets; I kept them separate deliberately. Say the word if you'd prefer one.

@vercel

vercel Bot commented Sep 9, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the canghe's projects Team on Vercel.

A member of the Team first needs to authorize it.

@yan-6

yan-6 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up self-review found two defects in the original commit of this PR. Both are fixed in 144be2e.

1. persist: partition wrote installer payloads to disk

The session was created with session.fromPartition('persist:wesight-update-download'). Per the Electron docs, a partition prefixed with persist: is backed by a persistent on-disk store under userData; without the prefix it is in-memory. Installer payloads here are hundreds of MB (.dmg / .exe), so every update download was creating persistent partition state under ~/Library/Application Support/WeSight/ that nothing in the codebase ever cleans up. The partition is now in-memory.

2. Cache was left enabled

fromPartition defaults to cache: true unless --disable-http-cache is set, which this app does not set. So the installer response could additionally be retained in the HTTP cache. Now passes { cache: false } explicitly — this is also why the partition name had to change, since fromPartition options only apply the first time a given partition is constructed.

3. setProxy failure aborted the whole download

await updateSession.setProxy(...) sat unguarded inside the try block, so a setProxy rejection surfaced as a download failure even though the request might still succeed over a direct route. It is now wrapped and logged, and the download proceeds.

Session construction is extracted into an exported prepareUpdateDownloadSession() so it can be unit-tested.

Root cause confirmation for issue #73

Worth recording why the dedicated session is needed at all rather than just relying on Electron's default proxy resolution. applyProxyPreference in src/main/main.ts:3420 calls session.defaultSession.setProxy({ mode: useSystemProxy ? 'system' : 'direct' }), and useSystemProxy defaults to false (src/renderer/config.ts:325). So on a default install the default session is explicitly pinned to direct, which is exactly why users whose only route to the release host is a local proxy (Clash Verge in the reporter's case) see the download hang rather than fall back.

One consequence to be aware of when reviewing: the download path now always uses system-proxy resolution, so it intentionally does not follow the "use system proxy" toggle. For this issue that is the desired behavior, but it does mean this one path is no longer governed by that preference.

Verification (run locally on macOS arm64)

  • New src/main/libs/appUpdateDownloadSession.test.ts — 7 tests, all passing. Confirmed as genuine regression tests: 3 of the 7 fail when reverted to the previous persist: / cached implementation.
  • Extended the shared src/test/mocks/electron.ts with session.fromPartition (the mock previously only exposed defaultSession), recording partition name, options and setProxy calls.
  • npx tsc --noEmit — exit 0
  • npx eslint on all three changed files — clean
  • Full npx vitest run — 523 passed. The 8 failing SQLite-backed files are a pre-existing local better-sqlite3 NODE_MODULE_VERSION mismatch, identical on pristine origin/main, unrelated to this change.

Note the update check path (api:fetch → session.defaultSession.fetch, src/main/main.ts:7563) still runs on the default session, so it remains subject to the same direct-mode pinning. Out of scope here since the reported symptom is the download stalling, but it is likely the same class of bug if anyone reports update checks failing behind a proxy.

@yan-6

yan-6 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 5fcafbf — this PR only fixed half of #73, and I'd rather flag that than let it merge as-is.

What was missing: the system-proxy session fixes routing. It does not fix the reported symptom ("现在卡下载"). DOWNLOAD_INACTIVITY_TIMEOUT_MS is armed only after response.body exists, so during the connect/response phase no timer is running at all. A proxy that accepts the TCP connection and then black-holes the request leaves session.fetch() pending forever, with manual cancel as the only exit. For a user whose proxy is misconfigured — the exact population this PR targets — that is the most likely thing they actually hit.

Fix: a DOWNLOAD_RESPONSE_TIMEOUT_MS (60s) armed right before fetch, reusing the existing AbortController so it maps onto the existing abort-reason handling and error message. Cleared in a finally around the fetch specifically so a streaming download is never killed at T+60s; both timers are also cleared in the outer finally.

Reverse verification (the part I care about): restoring the pre-fix file makes 2 of the 5 new cases fail, and they fail by hanging to vitest's 5s timeout rather than by tripping an assertion — the bug reproducing literally. Two of the other cases are anti-regression: a streaming download pushed past T+120s still resolves, and vi.getTimerCount() is 0 after success.

Harness pinned to the versions this repo declares (vitest 4.1.0 / typescript 5.7.3 / eslint 9.39.4 / @typescript-eslint 8.57.1 / electron 40.2.1) using the repo's own vitest.config.ts and eslint.config.mjs. 12 tests pass in total. eslint 0 problems. tsc --strict clean on appUpdateInstaller.ts; the test file's 2 TS2305 errors are the repo's existing electron-mock convention — this PR's own appUpdateDownloadSession.test.ts produces 5 of the same kind under an identical check.

One judgement call for you: both phases use 60s as two separate constants. Merging them into one would be less code but couples two independent budgets. I kept them separate; easy to change if you disagree.

@yan-6

yan-6 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Status update (2026-09-20): rebased onto the new main

main moved for the first time since 2026-08-24 — 8984cbe (tokendance integration, v1.0.7) and a8606e1 (macOS keychain fix). This branch was 2 behind and diverged; it is now updated and 0 behind.

This PR touches only src/main/libs/appUpdateInstaller.ts and its test, neither of which the new upstream commits modified, so the merge was clean with no overlap to reason about.

CI on the merge commit ae5699a is green: verify, lint, test, build-main, codeql, CodeQL, dependency-audit, secrets-scan, skills-audit, changed-files, label all SUCCESS, including the repo's own full test job. Vercel is the known fork-PR deploy-secret failure, unrelated to the code.

No code changes in this update. The two-phase timeout fix from the 09-19 review (connect-phase DOWNLOAD_RESPONSE_TIMEOUT_MS plus the existing inactivity timer) is unchanged, and the open question from that review still stands: both phases currently use 60s as two separate constants, and I kept them separate deliberately rather than collapsing them into one.

@yan-6

yan-6 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Synced with main (c913cd9)

Upstream c913cd9 (feat(models): add openlux provider for v1.0.9) landed on main. Merged it in and checked for file-level overlap with this PR: none — this branch and the upstream commit touch disjoint files, so there was no risk of two diffs landing in one function.

Verified after the sync that this PR's own diff is unchanged (same files, same +/- counts, nothing pulled in from the merge), it is now 0 behind main, and CI is green on the merge commit: test, verify, lint, build-main, CodeQL, dependency-audit, secrets-scan, skills-audit all pass. The only red check is Vercel (Authorization required to deploy) — fork PRs can't reach deploy secrets, environmental and unrelated to the code.

No code changes in this update; updated_at is also reset, which pushes back the 30-day stale-bot countdown (area:* labels are not in exempt-pr-labels). Still ready for review.

This branch was successfully deployed

1 active deployment
Production — c913cd94 Deployed Oct 1, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

请考虑让下载更新时试用系统当前使用的代理设置,现在卡下载

1 participant