Repository navigation
fix(electron): make the packaged Windows build pass the #7592 cold-restart smoke - #11443
Conversation
|
Outstanding debugging write-up — the USERPROFILE-over-APPDATA userData chain and the electron-builder !/node_modules/ finding are exactly the kind of root causes worth documenting, and the Hard Rule #18 packaged-Windows validation record in the body is appreciated. Two blockers before this can move: (1) your branch carries ~82 unrelated commits cut from release/v3.8.50 that are not yet on release/v3.8.51 — merged as-is it would drag that whole line into v3.8.51 out-of-band; please rebase onto release/v3.8.51 carrying only the electron-smoke commit (verified: all touched files exist there unchanged, cherry-pick is clean). (2) The new USERPROFILE regression test fails on Linux — executed here, the suite is 8/9 because ensureSmokeEnvDirs branches on host os.platform(), not the injected currentPlatform, so the win32 tree is never created on a Linux CI host and test:unit goes red. Parameterize the platform (or derive it from the smoke env) so the guard is testable cross-platform, and re-run the suite. Minor: the body's base-red reference (#9985) and the 'targets release/v3.8.50' note are stale now that the PR sits on release/v3.8.51. |
|
@diegosouzapw |
9dc7711 to
70f7ef3
Compare
|
Both blockers are cleared on the pushed branch ( (1) Rebase — done exactly as you verified: cherry-pick of the electron-smoke commit onto (2) Linux-red USERPROFILE test — root cause confirmed as you diagnosed: (3) Stale body references — removed the old base-red note and the "targets release/v3.8.50" line; description now states the validation was executed on a v3.8.50-tip build while this PR targets @Xxx91n — thanks for confirming the packaged build resolves your black screen. Per the freeze (#11439) the fix lands via |
…7592 cold-restart smoke Five defects found while validating diegosouzapw#7592 on a real packaged Windows build: - optionalPackStaging: GNU tar reads the drive letter in an absolute -f C:\... archive path as a remote rsh host (Cannot connect to C:), failing optional-pack staging on Git-for-Windows machines. Pass a bare filename with cwd at the tarball directory; surface tar stderr. - electron/package.json: lib/loginHeaderCapture.js was missing from the asar files allowlist; loginManager.js requires it top-level, so the packaged main process crashed on launch. - electron-builder >=26 injects !**/node_modules/** into every extraResources pattern list and no positive filter can override it, silently dropping the staged runtime node_modules (including the better-sqlite3 N-API prebuild) from resources/app. Add an afterPack hook (scripts/build/afterpack-copy-node-modules.mjs) that restores it. - smoke harness: Electron resolves userData from %USERPROFILE%/AppData/Roaming/<name> (USERPROFILE wins over APPDATA) and the path service throws when it is missing, so requestSingleInstanceLock() returned false and the app exited(0) silently before whenReady. ensureSmokeEnvDirs now pre-creates the derived tree and is exported for tests. - core.ts: the diegosouzapw#7592 guard parses a [DB] Driver: ... line that only the unused openDatabaseAsync() emitted; getDbInstance() now logs the same line on its primary open so the assertion is reachable. Also makes the smoke env-allowlist unit test host-agnostic (it hardcoded POSIX paths) and adds regression tests for the USERPROFILE derived tree and tarPack under Windows-style absolute paths. Closes diegosouzapw#7592 (cherry picked from commit 9dc7711)
ensureSmokeEnvDirs branched its win32 USERPROFILE/APPDATA userData-tree creation on the HOST os.platform(), so the diegosouzapw#7592 regression guard could never pass on a Linux CI host (suite ran 8/9 there). Parameterize currentPlatform like stopApp/buildSmokeEnv already do (default stays host platform() so the real packaged-smoke run is unchanged) and inject 'win32' from the regression test. 9/9 locally.
CI on the retargeted head surfaced failures that exist on the .51 tip (a179ffe) independent of the smoke work: - glmCodingProviderConfig fixtures missing glm-5.3-max inventory + routed tier (registry gained it; vitest 2 failed -> 10/10 after fix) - DB migrations doc drift: code has 160, README/AGENTS/llm.txt + 42 i18n mirrors said 159 - config/quality/eslint-suppressions.json carried 10 stale entries -> lint:json exit 2; pruned against this exact tree
70f7ef3 to
a134280
Compare
|
Follow-up on the retarget: the
Branch now = tip |
Remaining reds on this PR — all inherited from the
|
| Failing job / test | Inherited cause at tip | Covered by |
|---|---|---|
| Unit 1/4 · agent.json/agent-card ×5 | getBaseUrl() dereferences request.nextUrl (src/lib/wellKnown.ts:10) when route handlers are invoked without a request |
#11450 |
Unit 1/4 · APIKEY_PROVIDERS … 231 entries |
Registry has 233; frozen gate says 231 | #11450 |
Unit 3/4 · CC create route ×2 (cc prefix 400 ≠ 403/201) |
"cc" reserved by claude registry alias (open-sse/config/providers/registry/claude/index.ts:16) rejects before flag check |
#11450 |
| Unit 3/4 · openapi operation floor | Tip added undocumented ops faster than spec (floor rebaseline needed vs current tip tree) | #11450 (re-measure needed — see note below) |
| Unit 3/4 · t06 volcengine Zod ×1 (+ Qwen Code JSON noise) | volcengine-plan/connect* routes call request.json() unvalidated |
#11450 |
| Unit 4/4 · antigravity postExchange retry ×1 | {done:true} onboardUser ack skips discovery retry (antigravity.ts) |
#11450 |
| No new ESLint warnings (exit 1 now, not 2 — prune worked) | Tip-wide unused-vars debt, e.g. 22 in open-sse/services/combo.ts alone landed with #11493; ~275 uncovered errors tree-wide |
NOT covered by #11450 (it fixed only 5 sites) — needs its own cleanup pass or ratchet baseline refresh at this tip |
| Build (advisory) | useLiveDashboard.ts:16 imports resolveLiveWsUrl/sanitizeLiveWsPort which stopped existing at tip commit d82b6827 (#11452) — broken on the base itself, advisory-only |
nobody yet |
Why I'm not porting all of #11450 into this PR: you explicitly scoped this one to "only the electron-smoke commit", and duplicating its fixes here would guarantee conflicts when both land. Cleanest sequence: land #11450 first (it is fully green against .51 as of its latest run), then this PR rebase-drops onto it and every unit row above disappears; the two items #11450 does not cover (tip-wide ESLint debt, advisory Build import break) deserve their own small PRs against .51.
Two measurement notes for #11450 before merge: (a) the provider count moved 352 → 353 with the new tip commits, so its docs-count claims must be re-measured at a179ffed5; (b) its openapi floor re-baseline (34.4 @ d82b6827) needs re-verification at the same tip. Happy to do either here if you'd rather consolidate.
9862f12
into
diegosouzapw:release/v3.8.51
…7592 cold-restart smoke (diegosouzapw#11443) Validated in a combined 3-PR batch worktree off release/v3.8.51 tip. - Focused test: electron-smoke-script.test.ts — part of batch's 165/165 node:test run - typecheck:core, file-size, changelog-integrity, complexity, cognitive-complexity, check:docs-counts-sync — all OK - Full-repo lint: 228 pre-existing dashboard react-hooks/* findings, unrelated to this diff Real hardware validation on a fresh packaged Windows build (Hard Rule diegosouzapw#18) surfacing and fixing 5 genuine defects (GNU-tar drive-letter parsing, missing asar file, electron-builder's node_modules extraResources drop, USERPROFILE-derived userData path, unreachable driver-log assertion) is exactly the kind of investigation this repo needs more of. Thank you.
Summary
#7592's root-cause fixes (#6605 Electron/ABI rebuild, #7353 hashed-external normalization, #6835 retry cap) and regression guard (#10921 cold-restart +
assertNativeDriverSelected) had never run against a real packaged Windows build — the issue stayed open for exactly that validation. This PR is that validation, plus the five defects it surfaced and fixes. Final result on a freshrelease/v3.8.50clone, fully built and packaged on Windows:Findings — what broke, what happened, what fixed it
1. Optional-pack staging failed on Windows (GNU tar remote-host parsing)
npm run build:windied withoptional-pack tar failed for optional-pack-ml-runtime.tar.gz (exit 2)— twice, so not transient. The thrown error discarded tar's stderr, hiding the cause.spawnSync("tar.exe", ["-czf", "C:\...tar.gz", ...])failed. Surfacing stderr:tar (child): Cannot connect to C: resolve failed— GNU tar (Git-for-Windows/usr/bin) parses the drive letter in an absolute-f C:\...path as an rsh remote host. bsdtar (System32, what CI runners resolve) tolerates colons, so CI never saw this.tarPack()(scripts/build/optionalPackStaging.mjs) now passes a bare filename withcwdat the tarball directory — portable across GNU tar and bsdtar — and the thrown error includes tar stderr.2. Packaged main process crashed at startup: missing
lib/loginHeaderCapture.jsCannot find module './lib/loginHeaderCapture', require stackloginManager.js←main.js.loginManager.js:15requires it top-level (fatal), but the asarfilesallowlist inelectron/package.jsonomitted it.lib/loginHeaderCapture.jstofiles.3. electron-builder ≥26 silently dropped
node_modulesfrom extraResources — the native driver never shippedresources/app/node_moduleswas empty while the staging tree (.build/electron-standalone/node_modules) held 103 modules includingbetter-sqlite3with itsprebuilds/win32-x64.node. A diff of staging vs packed output showed exactly one missing top-level entry:node_modules.app-builder-lib/out/fileMatcher.jsinjects!**/node_modules/**into everyextraResources/extraFilespattern list, and no later positive pattern can override it — proven with a minimal fixture on 26.15.3: builds clean, control files copied,node_modulesdropped under all three filter orderings (["**/*","node_modules/**/*"], reversed, and with the dir itself listed).scripts/build/afterpack-copy-node-modules.mjs, wired asafterPackinelectron/package.json, restoring the stagednode_modulesintoresources/apppost-pack (103 modules, verifiedprebuilds/win32-x64.nodepresent in the package).4. Smoke harness: redirected
USERPROFILEbroke Electron userData → single-instance lock false → silentexit(0)code=0~2s after launch, before serving; the smoke reportedexited before readiness. No captured logs — which turned out to be meaningless: a Windows GUI Electron app'sconsole.lognever reaches a captured stdout pipe, so "no logs" was never evidence. (First proven by an asar-instrumented run: the process was alive and logging to a file while the captured pipe stayed empty.)app.getPath("userData")→ throwsFailed to get 'userData' pathrequestSingleInstanceLock()(which resolves userData internally) →false→main.js:58-59doesapp.quit(); process.exit(0);— the only silent, pre-whenReadyexit path in the main process (confirmed by graph-wide grep of exit paths).APPDATA+ redirectedLOCALAPPDATA→ OK; redirectedLOCALAPPDATAonly → OK; redirectedUSERPROFILEalone → reproduces. Electron derives the Roaming profile from%USERPROFILE%\AppData\Roaming\<name>—USERPROFILEtakes precedence over theAPPDATAenv var — and the path service throws rather than creates the missing directory. The harness pre-createdAPPDATA\{omniroute-desktop,OmniRoute,omniroute}but never the USERPROFILE-derived tree.%USERPROFILE%\AppData\Roaming\omniroute-desktopflipsgotTheLock=trueandwhenReadyis reached.ensureSmokeEnvDirs(scripts/dev/smoke-electron-packaged.mjs) now also pre-creates the USERPROFILE-derived tree; exported for unit testing.5. The #7592 driver assertion was unreachable — no
[DB] Driver:line on the server's primary DB pathlogs contain no '[DB] Driver: ...' line.openDatabaseAsync()(driverFactory.ts:371), whose only production caller is the db-backups import route. Server startup opens the DB viagetDbInstance()(core.ts:1286→ logs[DB] SQLite database readywithout naming the driver). The guard asserted a line the product never prints on this path.getDbInstance()now logs[DB] Driver: ${db.driver} | file: ${sqliteFile}right after its primary open — same format the guard'sNATIVE_DRIVER_LOG_PATTERN/SQLJS_DRIVER_LOG_PATTERNparse, so a sql.js fallback now fails the assertion loudly.Environmental (documented, no repo change needed)
FATAL: GPU process isn't usable. Goodbye.→ abort. The smoke already passes--no-sandbox --disable-gpuwhenCIis set — run it withCI=1outside GitHub Actions too.ELECTRON_SMOKE_TIMEOUT_MS=180000covers it (cold-restart second launch is fast).Test plan
node --import tsx/esm --test tests/unit/electron-smoke-script.test.ts→ 9 pass / 0 fail on Windows (previously 6/7: the env-allowlist test hardcoded POSIX paths and could only pass on Linux/macOS — expectations are now built withpath.join; plus two new regression tests: USERPROFILE-derived Roaming tree pre-creation, andtarPackunder absolute Windows-style paths).Full packaged validation (Hard Rule fix(ci): add environment for npm token access #18 — real-environment record):
Notes for reviewers
release-freeze🔒 release-freeze: v3.8.50 in progress #11439 (v3.8.50) is active; per review this PR now targetsrelease/v3.8.51, carrying only the electron-smoke commit(s). The Windows packaged validation itself was executed on a v3.8.50-tip build (8bbe92c) — all seven touched files are unchanged between the two tips.electron/package-lock.jsonchurn from localnpm installis deliberately excluded.Review follow-ups (this branch)
release/v3.8.51(d82b68274) carrying only the electron-smoke work: cherry-pick of9dc7711dwas clean exactly as predicted, plus one follow-up commit.ensureSmokeEnvDirsbranched its win32USERPROFILE/APPDATAuserData-tree creation on the hostos.platform(), so the fix(startup): better-sqlite3 not unpacked from app.asar → native module load fails → sql.js fallback OOM crash #7592 regression test ran 8/9 on Linux CI. The function now takes{ currentPlatform }(same injection style asstopApp/buildSmokeEnv; default remains the host platform so real packaged-smoke runs are unchanged) and the regression test injects"win32". Suite: 9/9.