Skip to content

test(windows): harden the release-verification harness and pin its contracts - #3327

Open
liugddx wants to merge 7 commits into
apache:mainfrom
liugddx:fix/windows-verify-harness
Open

test(windows): harden the release-verification harness and pin its contracts#3327
liugddx wants to merge 7 commits into
apache:mainfrom
liugddx:fix/windows-verify-harness

Conversation

@liugddx

@liugddx liugddx commented Aug 20, 2026

Copy link
Copy Markdown
Member

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):

  • waitForDevToolsPort reads the port from the DevTools stderr announcement (--remote-debugging-port=0) instead of pre-reserving a port that Electron may race.
  • Renderer discovery: 90s deadline, per-probe AbortSignal bounds (a hung connect once turned a "90 seconds" failure into ~6 minutes), errno cause chains in the failure message.
  • New shared waitForUsableRenderer: polls the renderer-usable state with the deadline as the sole authority — a single stalled Runtime.evaluate used 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):

  • The scan matched DisplayName -eq 'Maka', but electron-builder's default uninstallDisplayName is "${productName} ${version}" (NsisTarget.js:473), so the registered name is Maka 0.1.11 and 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 runCommand default, per the accepted argument that codesign/notarization have no honest upper bound):

  • Every PowerShell probe that runs inside a wait loop now passes timeoutMs, including the evidence capture that runs on an already-wedged machine.
  • Waits tolerate a failed probe and retry until their deadline; a failed enumeration is never treated as "no processes".

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.
  • The table-driven contract tests exist in the repository as 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.
  • On a relaunch version mismatch, the autoupdate harness captures process command lines, backup-directory presence, uninstall registrations and executable timestamps before failing, so the next occurrence of the nondeterministic "relaunched but still old" state is attributable.

Verification

  • L1/L2 (this machine): node --check on all touched files; biome clean; verify-windows-harness.test.mjs 19 pass / 1 skip; ci-test-plan.test.mjs 17 pass (step-name pin updated to the renamed step, contract unchanged: planner tests still run before dependency setup, unconditionally).
  • L3 (CI): the Windows lanes execute these helpers for real; this PR's own test lane runs both test files.

Relates to #3265 (review findings 3819778548, 3820232602, 3820922173, 3820922176, 3821101776, 3821101780, 3819778535) and #2142.

liugddx and others added 2 commits August 20, 2026 22:01
…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
@liugddx
liugddx force-pushed the fix/windows-verify-harness branch from e9b64be to 8a7f9e6 Compare August 20, 2026 14:09
…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
@liugddx

liugddx commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

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 release-windows-check Windows lane (18m14s, through the end-to-end autoupdate gate — run 32380790549). This is the split requested in #3265's review so the harness fixes merge on their own evidence; #3265 is stacked on it and rebases cleanly. Ready for review/merge whenever convenient.

@liugddx

liugddx commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

@Astro-Han @M4n5ter PTAL

liugddx added a commit to liugddx/maka-agent that referenced this pull request Aug 20, 2026
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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@Astro-Han

Copy link
Copy Markdown
Contributor

A data point for the bounded-probe half of this PR, from #3225's lane.

Verify automatic update end to end went red there four times: once did not expose CDP within 30 seconds: fetch failed, twice taskkill /PID … /T /F failed with exit code 128, and on the last rerun it just stopped — 47 minutes on that single step, no output, heading for the 75-minute job timeout. I cancelled it.

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 timeoutMs pushed down to every probe inside a wait loop.

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
@Joob1n

Joob1n commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@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. listInstalledProcesses here takes timeoutMs: 30_000, and waitForInstalledProcessesToExit's default budget is 60 seconds — so a stalled probe leaves room for exactly one more attempt. @likun666661 made this point on #3241 (where I had the same 30 seconds) and it is the sharpest review either of these PRs has had: a loop that can only try twice cannot survive a stall, which is the entire reason to retry. #3241 ended at 10 seconds, giving about six attempts inside a 60-second wait and twelve inside the 120-second relaunch wait, still far above a healthy WMI query. The retry logic you have written is right; the constant is what decides whether it can do anything.

Measured at the relaunch wait, same step, across two revisions of my branch:

probe bound loop on probe failure result
none hung 59 minutes past a 120s deadline; runner cancelled the job
30s aborts the wait failed in 74 seconds naming the stalled query
10s retries until the deadline relaunch detected in 27 seconds

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:

  • With the relaunch wait fixed, my last run died at the next step: taskkill /PID … did not finish within 30000ms while stopping the relaunched instance, plus a second taskkill … exit code 255 during cleanup. Bounded, so it fails cleanly — but the same process-table churn, one step later.
  • runtime-host: intermittent 'stopped responding during startup' after a Windows upgrade over an existing profile #3279 tracks a product-side intermittent: the upgraded install's own Runtime Host stops responding during startup, which this lane shows as a renderer that never appears. Worth recognising so it does not get filed as a harness bug.

The uninstall DisplayName finding is a good catch — Maka 0.1.11 versus a bare Maka would make every reader silently blind, and that class of bug survives exactly because the check still "passes".

@liugddx

liugddx commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@hqhq1025 @Joob1n Addressed at head 22ad8be62 within the verifier-only scope.

  • WMI/registry polling rounds are capped at 10 seconds and additionally capped to the enclosing wait's remaining budget.
  • The upgraded-process appearance wait now retries transient enumeration failures until its 120-second deadline and reports the last probe failure only if no later round succeeds.
  • Main-path and finally cleanup now share one terminateInstalledProcesses stop-and-prove policy. Exit 128 and an overrun taskkill remain mechanism failures only; the authoritative process-exit wait still runs.
  • Both normal and cleanup uninstall paths share the same completion barrier: installation directory absent, then uninstall registration absent.
  • Renderer CDP evaluation now receives the outer wait's remaining budget, so a half-open WebSocket cannot overrun the declared deadline.

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 git diff --check all pass.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
} catch (error) {
const message = String(error?.message);
if (!/exit code 128/.test(message) && !/did not finish within/.test(message)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Always run the authoritative exit proof before classifying taskkill failures. This branch states that the kill is only the mechanism, but any error other than exit 128 or a timeout is rethrown here before waitForExit; the live PR discussion records exit 255 on this same cleanup lane. If the process exited between enumeration and kill, cleanup still sets exited = false and skips the uninstaller plus both directory and registration barriers, recreating the stale-runner state this fix is meant to prevent. Record kill errors, run waitForExit unconditionally, and let that proof decide: ignore mechanism errors when no process remains, or report them together with a failed exit proof. The test that currently expects exit 1 to bypass the proof should be updated accordingly.

@liugddx liugddx Aug 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in d9fe44361. terminateInstalledProcesses now records every taskkill error and always runs the authoritative exit proof. If the proof finds no installed process, mechanism errors including exit 255 do not block uninstall/cleanup. If residue remains, an AggregateError reports the exit-proof failure together with all recorded kill diagnostics.

The prior test that expected exit 1 to bypass the proof was replaced with two contract cases: arbitrary kill failure + successful proof, and kill failure + failed proof with both errors retained. Local harness result: 31 passed, 1 symlink-privilege skip, 0 failed.

@liugddx

liugddx commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

Follow-up from the first Windows run at 22ad8be62: the relaunch and cleanup fixes worked far enough to expose the next transient probe, a one-shot PowerShell ProductVersion read that timed out after 30 seconds.

Head 913fbb99d keeps this narrowly in the verifier: the installed-version read now owns a 60-second outer budget, caps each PowerShell round at 10 seconds (or the remaining budget), and retries failures before reporting the last error at the deadline. Added a failure-then-success regression.

Local verification: 30 passed, 1 symlink-privilege skip, 0 failed; Biome 2.5.6, syntax checks, and git diff --check pass. The new CI run is the authoritative Windows evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants