test(windows): harden the release-verification harness and pin its contracts - #3327
Conversation
…ntracts Split out of apache#3265 at its reviewer's request so these fixes merge on their own evidence and that PR stands on the installer transaction alone. Contents: - Read the CDP port from the DevTools stderr announcement (waitForDevToolsPort) instead of pre-reserving one; widen renderer discovery to 90s with per-probe AbortSignal bounds and errno cause chains (four observed CI failures in this family). - waitForUsableRenderer: poll the renderer-usable state with the deadline as the sole authority — one stalled Runtime.evaluate used to fail the whole gate (run 32352924376); the WebSocket handshake now has its own bound so a port that accepts but never speaks fails the probe, not the lane. - Tolerate taskkill exit 128 when the relaunched instance already exited; the authoritative assertion remains waitForInstalledProcessesToExit. - Match the versioned uninstall DisplayName ('Maka 0.1.11'): the -eq 'Maka' filter matched nothing, deterministically, and every reader of the scan was blind. - Bound every PowerShell probe that runs under a polling deadline (the anti-pattern apache#3241 names), and let waits tolerate one failed probe: a failed enumeration is never treated as 'no processes'. - waitForUninstallRegistrationToClear: a detached uninstaller deletes its registry keys after waitUntilMissing sees the files disappear; wait for the registration to clear before the next install, with the one-registry-call residual window stated precisely. - directoryTreeManifest/diffTreeManifests shared exports for the rollback gate, now recording empty directories so their loss is visible; capture upgrade-state evidence on a relaunch version mismatch. - Commit the table-driven contract tests as scripts/verify-windows-harness.test.mjs and wire them into the CI planner test step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
The upgraded-app smoke pipes stderr but only waitForDevToolsPort's temporary listener ever read it: once removed, the paused stream lets Chromium's --enable-logging=stderr output fill the pipe and block the child, and the evidence the pipe exists to preserve is lost. Attach the same persistent collector every sibling smoke uses and append its tail to renderer-readiness failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
e9b64be to
8a7f9e6
Compare
…aunch Run 32378497920: taskkill /T /F on the force-run instance exceeded its 30s bound on a wedged runner and failed the gate, even though the authoritative assertion - waitForInstalledProcessesToExit, which fails with the live process list if anything from the install tree still runs - was one line below. Treat a kill that overran its bound like exit 128: the kill is the mechanism, the exit wait is the assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
All lanes are green at head d6f443f: the unified CI job (harness contract tests included, running pre-install as pinned by #3317's step contract) and the full |
|
@Astro-Han @M4n5ter PTAL |
Run 32392391944 failed at the packaged renderer smoke with the 30-second CDP deadline and no app stderr - the exact flake family apache#3327 fixes; this branch is based on main, which predates those harness repairs.
hqhq1025
left a comment
There was a problem hiding this comment.
[P1] scripts/verify-windows-installer-lifecycle.mjs:47 — Retry WMI failures inside the relaunch deadline. This new 30-second bound correctly turns a wedged Get-CimInstance into a rejection, but scripts/verify-windows-autoupdate.mjs:410 directly awaits listInstalledProcesses() inside a 120-second relaunch loop. Run 32340493254 failed on exactly this path after one transient WMI timeout even though the upgraded process may already have relaunched. Catch failed rounds, continue until the outer deadline, and report the last probe error only if no later enumeration succeeds; use a shorter or remaining-budget probe timeout and add a failure-then-success regression.
[P2] scripts/verify-windows-autoupdate.mjs:492 — Reuse this termination policy in cleanup. The main path now treats exit 128 and timed-out taskkill as mechanism failures before relying on waitForInstalledProcessesToExit, but the finally block at lines 607-613 still calls taskkill directly. Run 32378497920 recorded the same stale-PID exit 128 from cleanup. After a later smoke or verification failure, that exception skips the authoritative exit wait and the uninstaller, potentially leaving a stale uninstall registration on the runner. Extract one stop-and-verify helper for both paths and complete the uninstall/registration barrier during cleanup.
Codex-assisted review performed under the maintainer-approved review workflow.
|
A data point for the bounded-probe half of this PR, from #3225's lane.
That last one is the shape you describe in the PR body: with no bound the step has no upper limit at all, so the cost is a wedged runner rather than a fast red. Good to see Reference: https://github.com/apache/maka/actions/runs/32387807797 (job 96534477439). Nothing needed from you — just adding evidence in case it's useful for the review. |
…mination Review round on apache#3327 named the two loops that still predated the policy the rest of the harness already follows. The relaunch wait awaited listInstalledProcesses directly inside its 120-second loop, so one transient WMI failure rejected the gate even though the upgraded app may already have been running (run 32340493254). It is now waitForInstalledProcessAppearance in the lifecycle module: the mirror of the exit wait - probes are tolerated, the deadline is the authority, and the last probe error is evidence only when no later enumeration succeeds. Covered by a failure-then-success regression. The cleanup path still ran its own strict taskkill loop, so the same stale-PID exit 128 the main path tolerates (run 32378497920) could skip the authoritative exit wait and the uninstall/registration barrier, leaking a registration onto the runner. Both paths now share terminateInstalledProcesses: tolerant kill (exit 128 and an overrun bound are mechanism failures), then the exit wait as the assertion. Covered by mechanism-tolerance and rethrow tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
@liugddx — this subsumes #3241, so I am closing that one rather than making anyone review the same file twice. Two things from its review and its CI history that are worth carrying over, since neither is in this PR yet. The probe timeout is too large for the budget it sits in. Measured at the relaunch wait, same step, across two revisions of my branch:
Being precise about what that proves: the last row changed both the bound and the loop behaviour at once, so it does not isolate the constant — the argument for 10s over 30s is the arithmetic above, not this table. What the table does establish is that the stall is real, reproducible at that exact step, and that a tolerant loop gets through it. Two faults still ahead of you on that lane, neither mine to fix:
The uninstall |
|
@hqhq1025 @Joob1n Addressed at head
Regression coverage includes failure-then-success WMI enumeration, transient cleanup enumeration failure, tolerated kill failures followed by the exit proof, both uninstall barriers (including an already-missing detached uninstaller), a half-open WebSocket, and proof that a timed-out command's child no longer executes. Local verification: 29 passed, 1 symlink-privilege skip, 0 failed; Node syntax checks, Biome 2.5.6 formatting, and |
hqhq1025
left a comment
There was a problem hiding this comment.
The previous WMI appearance-loop P1 is fixed, and the cleanup paths now share the intended stop-and-prove helper. Exact-head harness and CI-planner tests passed 51/51; syntax, lint, format, diff, and merge-tree checks also passed. One cleanup-path P2 remains inline.
Codex-assisted review performed under the maintainer-approved review workflow.
|
Follow-up from the first Windows run at Head Local verification: 30 passed, 1 symlink-privilege skip, 0 failed; Biome 2.5.6, syntax checks, and |
|
@hqhq1025 PTAL |
hqhq1025
left a comment
There was a problem hiding this comment.
Approved at exact head d9fe443614bed143d84da4692f9eba3cc2dc9458.
No actionable findings remain. The two issues from my previous review are fixed at the shared ownership boundary:
- Process-appearance polling now tolerates transient WMI failures until the enclosing deadline.
- Main-path and cleanup termination both use one stop-and-prove helper. Every
taskkillerror is retained as mechanism evidence, but the authoritative installed-process exit check always runs; kill errors are ignored only when that proof confirms no process remains, and are aggregated with the exit failure when residue remains.
The later ProductVersion fix follows the same rule: each PowerShell read is capped to ten seconds or the remaining outer budget, transient probe failures retry for up to sixty seconds, and the version assertion remains strict. This improves observability and boundedness without converting a real old-version relaunch into success.
Problem definition and mechanism: the PR addresses verifier instability caused by unbounded or one-shot observations, not product failures. It removes the CDP port reservation race, bounds HTTP/WebSocket/CDP and PowerShell probes, retries transient observation failures under one outer deadline, waits for both uninstall files and registration state, and retains hard assertions on renderer usability, installed process exit, product version, directory contents, and uninstall cleanup.
First principles and Occam's razor: yes. Unreliable probes are mechanisms; durable state and final process/file/registry assertions are the verdict. The shared renderer wait, process lifecycle helpers, and uninstall barrier remove duplicated policies rather than adding fallback paths. I found no production code to delete because the PR touches verifier code only. The table-driven tests cover distinct contracts and failure sequences; I found no low-quality test that should be removed. No deeper refactor is required.
Verification:
- Harness and CI planner contracts passed 53/53 locally.
- Node syntax checks, Biome, and
git diff --checkpassed. - The merge tree against current
mainis clean; the current-main semantic delta remains the same five verifier/workflow files. - Live CI is green, including the full Windows release, installer, and end-to-end automatic-update job.
Merge verdict: ready to merge.
Residual risk: this hardens the verifier but does not claim to fix product-side Windows startup or update defects such as #3279. Those states should now fail within bounded time with enough evidence to attribute them, rather than hanging or being misclassified.
Summary
Windows release-verification harness hardening, split out of #3265 at its reviewer's request ("land the CI-stability fixes first, as their own PR") so these fixes merge on their own evidence and #3265 stands or falls on the installer transaction alone. All changes are under
scripts/plus one workflow test-step line; no packaged-product code is touched.What's fixed
CDP attach and renderer readiness (four observed failures in this family across #3161/#3240/#3265 lanes):
waitForDevToolsPortreads the port from the DevTools stderr announcement (--remote-debugging-port=0) instead of pre-reserving a port that Electron may race.AbortSignalbounds (a hung connect once turned a "90 seconds" failure into ~6 minutes), errno cause chains in the failure message.waitForUsableRenderer: polls the renderer-usable state with the deadline as the sole authority — a single stalledRuntime.evaluateused to fail the whole gate (run 32352924376,CDP renderer evaluation timed out.while the renderer was still booting). The WebSocket handshake now has its own bound, so a port that accepts TCP but never speaks fails one probe instead of hanging the lane (feat(win): Abort-path installer rollback with backup retention #3265 review finding 3821101776).Uninstall registration reads (the cause of the deterministic red in run 32363159920):
DisplayName -eq 'Maka', but electron-builder's defaultuninstallDisplayNameis"${productName} ${version}"(NsisTarget.js:473), so the registered name isMaka 0.1.11and the filter matched nothing — every reader was blind. Now matches the versioned name.waitForUninstallRegistrationToClear: an NSIS uninstaller launched without_?=detaches and deletes its registry keys tens of seconds after the files disappear; anything installing into that window gets its fresh registration deleted. The wait's comment states the residual window precisely (one registry call wide — the uninstall key at uninstaller.nsh:250, the install key at :254; finding 3820922176).Bounded probes under polling deadlines (findings 3820232602 / 3820922173 / 3821101780 — the anti-pattern #3241 names; kept as explicit per-site bounds rather than a
runCommanddefault, per the accepted argument that codesign/notarization have no honest upper bound):timeoutMs, including the evidence capture that runs on an already-wedged machine.Evidence and coverage (finding 3819778535):
directoryTreeManifest/diffTreeManifests(shared exports the feat(win): Abort-path installer rollback with backup retention #3265 gate consumes) now record empty directories so their loss is visible.scripts/verify-windows-harness.test.mjs(manifest shape/sorting/empty-dir/symlink-fail-closed, diff table, runCommand timeout kill, DevTools port announcement, usable-renderer deadline semantics, both waits' tolerate-and-deadline contracts) and run in the CI planner test step on every PR — 19 pass locally on Windows (1 skip: symlink creation needs privilege), and the step runs them on Linux too.Verification
node --checkon all touched files; biome clean;verify-windows-harness.test.mjs19 pass / 1 skip;ci-test-plan.test.mjs17 pass (step-name pin updated to the renamed step, contract unchanged: planner tests still run before dependency setup, unconditionally).testlane runs both test files.Relates to #3265 (review findings 3819778548, 3820232602, 3820922173, 3820922176, 3821101776, 3821101780, 3819778535) and #2142.