Skip to content

fix(release): bound the Windows process probe so its poll deadlines hold #3241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Joob1n wants to merge 11 commits into from
Closed

fix(release): bound the Windows process probe so its poll deadlines hold #3241

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7f61f1e
fix(release): read the CDP port Chromium bound instead of reserving one
Joob1n Aug 19, 2026
3ecb005
fix(release): delete the previous instance's DevToolsActivePort first
Joob1n Aug 19, 2026
6532f7a
fix(release): give CDP discovery the time a cold runner actually needs
Joob1n Aug 19, 2026
090158d
chore: restart CI — test_workspaces hung past three hours
Joob1n Aug 19, 2026
8a4a8d6
Merge remote-tracking branch 'origin/main' into fix/cdp-smoke-port-race
Joob1n Aug 20, 2026
119d9da
fix(release): bound each CDP poll attempt so the deadline stays honest
Joob1n Aug 20, 2026
55ce142
fix(release): bound the Windows process probe so its poll deadlines hold
Joob1n Aug 20, 2026
af023a7
fix(release): let the process-poll deadline outrank a stalled probe
Joob1n Aug 20, 2026
5543fba
Merge remote-tracking branch 'origin/main' into fix/cdp-smoke-port-race
Joob1n Aug 20, 2026
7eeb843
fix(release): size the process probe to its budget and say what was o…
Joob1n Aug 20, 2026
c611cfd
refactor(release): hand CDP port discovery to #3265
Joob1n Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 45 additions & 6 deletions scripts/verify-windows-installer-lifecycle.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ function delay(milliseconds) {
return new Promise((resolvePromise) => setTimeout(resolvePromise, milliseconds));
}

export async function listInstalledProcesses(installDirectory, { run = runCommand } = {}) {
export async function listInstalledProcesses(
installDirectory,
{ run = runCommand, timeoutMs = 10_000 } = {},
) {
const root = `${resolve(installDirectory)}${sep}`;
const script = String.raw`
$root = [IO.Path]::GetFullPath(${powerShellLiteral(root)})
Expand All @@ -37,7 +40,21 @@ $matches = @(
)
$matches | ConvertTo-Json -Compress
`;
const { stdout } = await run('powershell', ['-NoProfile', '-NonInteractive', '-Command', script]);
// Bounded because this probe runs inside deadline-bounded poll loops, and a
// loop only checks its deadline between probes: an unbounded one makes the
// deadline a lie. A `Get-CimInstance Win32_Process` query hung for the rest
// of the job during an auto-update relaunch, so the 120s relaunch deadline
// never fired and the runner cancelled the run an hour later.
//
// Well under the 60s and 120s budgets above it, so a stalled probe leaves

@Astro-Han Astro-Han Aug 20, 2026

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] Scope this claim, and fix or hand off the loop it does not hold for. "A stalled probe leaves room for several more attempts" is true of waitForInstalledProcessesToExit below, because you gave it a try/catch. It is not true of the other caller: scripts/verify-windows-autoupdate.mjs:405 awaits listInstalledProcesses(installDirectory) bare inside its 120s relaunch loop, with no try/catch anywhere in verifyWindowsAutoupdate, so one rejected probe propagates out and fails the whole auto-update verification on the first stall. That loop is the one the incident in lines 45–47 actually happened in. This PR still improves it — an hour-long hang becomes a 10s failure — but it converts a hang into a hard failure rather than into the tolerated retry this comment describes, and a healthy-but-slow probe that would previously have completed now fails a release verification instead. Confirmed by reading both files at this head; also confirmed that #3265 at 23e8a8bc9 does not touch lines 401–413, so neither PR currently owns it. Either wrap that probe in the same try/catch and let the 120s deadline decide, or narrow this comment to the function it describes and say who picks up the relaunch loop. Regression test: make listInstalledProcesses injectable into verifyWindowsAutoupdate, have it reject for the first N calls and then report the relaunched process, and assert the relaunch wait still succeeds.

// room for several more attempts rather than consuming the whole window: a
// loop that can only try twice cannot survive a stall, which is the entire
// point of tolerating one.
const { stdout } = await run(
'powershell',
['-NoProfile', '-NonInteractive', '-Command', script],
{ timeoutMs },
);
if (!stdout.trim()) return [];
const parsed = JSON.parse(stdout);
return Array.isArray(parsed) ? parsed : [parsed];
Expand All @@ -53,16 +70,38 @@ export async function waitForInstalledProcessesToExit(
} = {},
) {
const deadline = Date.now() + timeoutMs;
let processes = await listProcesses(installDirectory);
while (processes.length > 0) {
let processes = [];
let observed = false;
let probeError;
for (;;) {

@Astro-Han Astro-Han Aug 20, 2026

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] Land the injected-probe tests you describe in the PR body, or correct the checklist. There is no scripts/verify-windows-installer-lifecycle.test.mjs at this head, and waitForInstalledProcessesToExit is the rare release-script function that is trivially testable without Windows — listProcesses, sleep and timeoutMs are all injectable parameters, so the transient-failure, persistent-failure and never-exits paths are a few dozen lines of plain Node with no PowerShell involved. The PR checklist states tests cover the change and fail without it; at this head nothing in the tree does either. This matters more than usual here because the whole change is about which of three failure messages a future maintainer sees at 2am, and message selection is exactly what a test pins. Confirmed by listing scripts/ at c611cfd. One practical note when you add it: scripts/*.test.mjs is not collected by npm test, so it needs registering behind an explicit entry point the way check:release and windows:inventory are, or the test check will stay blind to it.

try {
processes = await listProcesses(installDirectory);

@Astro-Han Astro-Han Aug 20, 2026

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.

[P3] Derive the probe's bound from the time the loop has left, rather than letting the callee's default decide. This call passes no options, so each attempt is bounded only by listInstalledProcesses's own timeoutMs = 10_000 default, and the relationship between that number and this loop's deadline exists only in the comment above. No caller violates it today, but waitForInstalledProcessesToExit(dir, { timeoutMs: 5_000 }) would run for roughly 15 seconds while claiming a 5 second bound — reintroducing the precise defect this PR is fixing, one level up. Passing { timeoutMs: Math.min(10_000, deadline - Date.now()) } makes the deadline self-enforcing and also caps the overshoot at the poll interval. Inference: no such caller exists at this head, so this is about keeping the invariant true rather than a defect you can trigger today.

observed = true;
probeError = undefined;
if (processes.length === 0) return;
} catch (error) {
// A bounded probe that fails says nothing about the processes, so the
// loop's own deadline stays the authority — the Windows process query
// stalls while an installer is churning the process table, and one
// stalled probe must not decide a verification.
probeError = error;
}
if (Date.now() >= deadline) {
// Never having seen the process table is a different fault from seeing
// processes that would not exit, and only one of them is about Maka.
if (!observed) {

@Astro-Han Astro-Han Aug 20, 2026

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] Key this branch on whether the LAST probe succeeded, not on whether any probe ever did. observed is sticky: once any probe returns, it stays true for the rest of the loop, while processes keeps whatever that probe saw. So the sequence "first probe returns Maka.exe (1234), that process then exits, every later probe stalls" arrives at the deadline with observed === true and a processes array captured at t=0 — and falls through to the branch below, which reports Installed Maka processes did not exit within 60000ms: Maka.exe (1234). That is a definite claim about a process that may well have exited, derived from a snapshot up to a minute stale, and the The last probe also failed footnote does not undo the sentence in front of it. It defeats the distinction this PR exists to draw — the comment on line 90 says never having seen the table is a different fault from seeing processes that would not exit, and this is a third state, "saw them once and no longer knows", currently reported as the second. Confirmed by reading the loop at this head; the subagent reports reproducing it with an injected probe that succeeds once and then fails 15 times, yielding exactly that message. The fix is also a simplification: drop observed and processes in favour of one lastObservation set on success and cleared in the catch, then branch on whether it is undefined — three state variables become two and the semantics move from "ever" to "last". Regression test: inject a probe returning one process, then failing until the deadline, and assert the message reports unknown rather than "did not exit".

throw new Error(
`Could not read the process table within ${timeoutMs}ms, so whether installed Maka processes exited is unknown.\nLast process probe failed: ${probeError?.message}`,

@Astro-Han Astro-Han Aug 20, 2026

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.

[P3] Truncate the probe error before interpolating it. runCommand's timeout message is ${command} ${args.join(' ')} did not finish within ${timeoutMs}ms, and the last element of args is the whole multi-line Get-CimInstance script — so probeError?.message drops roughly 800 characters of embedded PowerShell, newlines included, into the CI failure output, both here and in the The last probe also failed branch below. The point of this change is that a failure should say which kind of fault occurred; the sentence that does that ends up buried under the script that caused it. No disclosure concern — the only interpolated value is the temporary install directory. Taking error.message.split('\n')[0], or eliding over-long args inside runCommand itself, keeps the diagnosis readable. Confirmed by reading verify-packaged-app.mjs at this head.

);
}
const summary = processes.map(({ processId, name }) => `${name} (${processId})`).join(', ');
throw new Error(
`Installed Maka processes did not exit within ${timeoutMs}ms: ${summary || '<unknown>'}.`,
`Installed Maka processes did not exit within ${timeoutMs}ms: ${summary || '<unknown>'}.${
probeError ? `\nThe last probe also failed: ${probeError.message}` : ''
}`,
);
}
await sleep(pollIntervalMs);
processes = await listProcesses(installDirectory);
}
}

Expand Down
Loading