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

Show file tree
Hide file tree
Changes from 8 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
113 changes: 74 additions & 39 deletions scripts/verify-packaged-app.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import { spawn } from 'node:child_process';
import { createHash } from 'node:crypto';
import { createReadStream } from 'node:fs';
import { access, mkdir } from 'node:fs/promises';
import { createServer } from 'node:net';
import { access, mkdir, readFile, rm } from 'node:fs/promises';
import { join } from 'node:path';

// `timeoutMs` is opt-in, for the commands that have actually hung: node-pty
Expand Down Expand Up @@ -74,54 +73,77 @@ export async function assertMissing(path) {
throw new Error(`Forbidden release resource exists: ${path}`);
}

export async function reserveTcpPort() {
const server = createServer();
await new Promise((resolvePromise, reject) => {
server.once('error', reject);
server.listen(0, '127.0.0.1', resolvePromise);
});
const address = server.address();
if (!address || typeof address === 'string') {
server.close();
throw new Error('Could not reserve a CDP port.');
}
await new Promise((resolvePromise, reject) => {
server.close((error) => (error ? reject(error) : resolvePromise()));
});
return address.port;
}

function delay(milliseconds) {
return new Promise((resolvePromise) => {
setTimeout(resolvePromise, milliseconds);
});
}

export async function findRendererTarget(port, child) {
const deadline = Date.now() + 30_000;
let lastError;
/**
* The port Chromium actually bound, read from the DevToolsActivePort file it
* writes into the user-data directory. The verifier used to reserve a port,
* release it, and hand the number to Electron — leaving a window in which
* anything else on the runner could take it, and a timeout log that could not
* say whether the app was even listening where the poll looked (issue #3196).
*/
async function readDevToolsPort(userDataDirectory) {
try {
const content = await readFile(join(userDataDirectory, 'DevToolsActivePort'), 'utf8');
const port = Number.parseInt(content.split('\n')[0] ?? '', 10);
return Number.isInteger(port) && port > 0 ? port : null;
} catch {
return null;
}
}

export async function findRendererTarget(userDataDirectory, child) {
const startedAt = Date.now();
// Generous on purpose: a cold Windows runner scans a first-run executable
// before letting it serve, and the port-file diagnostics showed a healthy
// app answering /json/list with no page target yet past the old 30-second
// line (the step itself had been observed taking 74 seconds). The child
// exiting still fails immediately; the workflow timeout is the outer bound
// for everything else.
const deadline = startedAt + 120_000;
let port = null;
let lastState = 'DevToolsActivePort was never written under the user-data directory';
while (Date.now() < deadline) {
if (child.exitCode !== null) {
throw new Error(`Packaged Maka exited before its renderer was ready.`);
}
try {
const response = await fetch(`http://127.0.0.1:${port}/json/list`);
if (response.ok) {
const targets = await response.json();
const page = targets.find(
(target) => target.type === 'page' && target.webSocketDebuggerUrl,
);
if (page) return page;
port ??= await readDevToolsPort(userDataDirectory);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Resolved

@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] Make findRendererTarget enforce the freshness it now depends on, rather than trusting each caller to have deleted the file. port ??= caches the first value read and never re-reads, so a DevToolsActivePort left by a previous instance is latched for the entire 120-second window and the run ends with port <dead> did not answer — which reads as a hung app rather than as a stale file, the precise misdiagnosis this PR exists to eliminate. All three current call sites do delete the file first, so nothing is broken today; what is fragile is that the requirement lives only in three separate comments, on an exported function whose signature gives no hint of it. The fourth caller will not know. Passing the spawn timestamp and ignoring a port file older than it, or having this function do the deletion as part of a startRendererTarget(userDataDirectory) that owns both halves, puts the invariant where it cannot be forgotten. Inference about the future caller; the caching behaviour itself is confirmed by reading the code at this head.

if (port !== null) {
try {
// Per-attempt timeout: undici's default headers timeout is 300 seconds,
// so a single hanging attempt against a bound-but-unresponsive endpoint
// (a main process stuck in startup) would otherwise blow straight
// through the loop's deadline — one run overshot it to 355 seconds.
const response = await fetch(`http://127.0.0.1:${port}/json/list`, {
signal: AbortSignal.timeout(2_000),
});
if (response.ok) {
const targets = await response.json();
const page = targets.find(
(target) => target.type === 'page' && target.webSocketDebuggerUrl,
);
if (page) return page;
lastState = `port ${port} answered with ${targets.length} targets but no debuggable page`;
} else {
lastState = `port ${port} answered HTTP ${response.status}`;
}
} catch (error) {
lastState = `port ${port} did not answer: ${error.message}`;
}
} catch (error) {
lastError = error;
}
await delay(250);
}
// Say exactly where discovery stalled, so classifying the failure does not
// take a re-run: no port file, a port that never answers, an unexpected
// HTTP status, and a healthy endpoint with no page target are four
// different faults.
const elapsedSeconds = Math.round((Date.now() - startedAt) / 1000);
throw new Error(
`Packaged Maka renderer did not expose CDP within 30 seconds${
lastError ? `: ${lastError.message}` : ''
}.`,
`Packaged Maka renderer did not expose CDP within ${elapsedSeconds} seconds: ${lastState}.`,
);
}

Expand Down Expand Up @@ -271,17 +293,28 @@ export function isolatedUserEnv(homeDirectory, { temporaryDirectory = homeDirect
}

export async function smokePackagedRenderer(executable, { workingDirectory } = {}) {
const port = await reserveTcpPort();
const home = join(workingDirectory, 'home');
const userData = join(workingDirectory, 'user-data');
const userEnv = isolatedUserEnv(home);
await mkdir(home, { recursive: true });
await mkdir(userData, { recursive: true });
// Chromium removes DevToolsActivePort only on a clean exit, and the
// upgrade-lifecycle check reuses one user-data directory across two app
// versions with a SIGKILL between them — so a file found here can belong to
// the previous instance, pointing the poll at a port nothing listens on
// anymore. Deleting it first means whatever appears was written by this
// child. This is what the first run of the port-file diagnostics caught:
// the poll read a bound-looking port and fetch failed for the full window.
await rm(join(userData, 'DevToolsActivePort'), { force: true });
await mkdir(userEnv.APPDATA, { recursive: true });
await mkdir(userEnv.LOCALAPPDATA, { recursive: true });
const child = spawn(
executable,
[`--remote-debugging-port=${port}`, `--user-data-dir=${userData}`, '--enable-logging=stderr'],
// Port 0: Chromium binds a free port itself and records it in the
// user-data directory's DevToolsActivePort file, which is where the poll
// reads it back — no reserve-then-release window for another process on
// the runner to take the number first.
['--remote-debugging-port=0', `--user-data-dir=${userData}`, '--enable-logging=stderr'],
{
cwd: workingDirectory,
env: {
Expand All @@ -299,8 +332,10 @@ export async function smokePackagedRenderer(executable, { workingDirectory } = {
});

try {
const target = await findRendererTarget(port, child);
const deadline = Date.now() + 30_000;
const target = await findRendererTarget(userData, child);
// Same headroom as target discovery: a cold runner that was slow to serve
// CDP is as slow to mount React, and a wrong deadline fails a good build.
const deadline = Date.now() + 120_000;
let rendererState;
while (Date.now() < deadline) {
rendererState = await evaluateRenderer(target.webSocketDebuggerUrl);
Expand Down
51 changes: 35 additions & 16 deletions scripts/verify-windows-autoupdate.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
isPackagedRendererUsable,
isolatedUserEnv,
RENDERER_STATE_EXPRESSION,
reserveTcpPort,
runCommand,
stopChild,
} from './verify-packaged-app.mjs';
Expand Down Expand Up @@ -230,14 +229,16 @@ export async function verifyWindowsAutoupdate(
await access(uninstaller);

step('launching the installed candidate against the loopback feed');
const cdpPort = await reserveTcpPort();
const home = join(temporaryDirectory, 'home');
const userData = join(temporaryDirectory, 'user-data');
const userEnv = isolatedUserEnv(home);
await mkdir(home, { recursive: true });
await mkdir(userData, { recursive: true });
await mkdir(userEnv.APPDATA, { recursive: true });
await mkdir(userEnv.LOCALAPPDATA, { recursive: true });
// A DevToolsActivePort left by an earlier instance would point the poll at
// a dead port; whatever appears after this belongs to the child below.
await rm(join(userData, 'DevToolsActivePort'), { force: true });
const childEnv = {
...process.env,
MAKA_SKIP_SHELL_ENV: '1',
Expand All @@ -250,11 +251,9 @@ export async function verifyWindowsAutoupdate(
delete childEnv.MAKA_UPDATE_MOCK_STATE;
child = spawn(
installedExecutable,
[
`--remote-debugging-port=${cdpPort}`,
`--user-data-dir=${userData}`,
'--enable-logging=stderr',
],
// Port 0: Chromium binds a free port and records it in DevToolsActivePort,
// which is where findRendererTarget reads it back.
['--remote-debugging-port=0', `--user-data-dir=${userData}`, '--enable-logging=stderr'],
{ cwd: temporaryDirectory, env: childEnv, stdio: ['ignore', 'ignore', 'pipe'] },
);
let stderr = '';
Expand All @@ -263,7 +262,7 @@ export async function verifyWindowsAutoupdate(
stderr = `${stderr}${chunk}`.slice(-16_384);
});

const target = await findRendererTarget(cdpPort, child);
const target = await findRendererTarget(userData, child);

@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] Apply the new headroom to the Windows renderer waits too, since that is the path the failures came from. smokePackagedRenderer raised both its CDP discovery and its renderer-state deadline to 120 seconds, with the reasoning that a cold runner slow to serve CDP is as slow to mount React. That reasoning applies unchanged to rendererDeadline on the next line, which stays at 30 seconds, and to the deadline in the nested smokeRenderer at line 487, which also stays at 30 seconds — and both of those are in the Windows auto-update verifier, which the description names as having hit this fault twice, once in the standalone smoke and once in the upgrade-lifecycle step. As it stands the PR concludes that 30 seconds is too tight for a cold Windows runner and then leaves two Windows renderer waits at 30 seconds, so the same timeout can still fail a good build with a message that says nothing about why. Either raise them with the same justification or say why the post-upgrade relaunch is expected to be faster than a cold start; a shared constant next to findRendererTarget's deadline would keep the answer in one place.

const rendererDeadline = Date.now() + 30_000;
for (;;) {
const state = await evaluateInRenderer(
Expand Down Expand Up @@ -401,13 +400,29 @@ export async function verifyWindowsAutoupdate(
step('waiting for the upgraded app to relaunch automatically');
const relaunchDeadline = Date.now() + 120_000;
let relaunched = [];
let probeError;
for (;;) {
relaunched = (await listInstalledProcesses(installDirectory)).filter(
(processInfo) => basename(processInfo.path).toLowerCase() === executableName.toLowerCase(),
);
if (relaunched.length > 0) break;
try {
relaunched = (await listInstalledProcesses(installDirectory)).filter(
(processInfo) =>
basename(processInfo.path).toLowerCase() === executableName.toLowerCase(),
);
probeError = undefined;
if (relaunched.length > 0) break;
} catch (error) {
// The Windows process query stalls exactly here — while NSIS is
// handing off and relaunching, the process table is in flux — so a
// bounded probe that gives up says nothing about the relaunch. This
// deadline stays the authority; the last probe failure is reported
// with it so a persistent stall is not mistaken for a missing app.
probeError = error;
}
if (Date.now() >= relaunchDeadline) {
throw new Error('The installer did not relaunch the upgraded app within 120s.');
throw new Error(
`The installer did not relaunch the upgraded app within 120s.${
probeError ? `\nLast process probe failed: ${probeError.message}` : ''
}`,
);
}
await delay(1_000);
}
Expand Down Expand Up @@ -442,18 +457,22 @@ export async function verifyWindowsAutoupdate(
requireWindowsSandbox: true,
requireDisclaimer: true,
smokeRenderer: async (executable, { workingDirectory }) => {
const smokePort = await reserveTcpPort();
const smokeHome = join(workingDirectory, 'home');
const smokeUserData = join(workingDirectory, 'user-data');
const smokeEnv = isolatedUserEnv(smokeHome);
await mkdir(smokeHome, { recursive: true });
await mkdir(smokeUserData, { recursive: true });
await mkdir(smokeEnv.APPDATA, { recursive: true });
await mkdir(smokeEnv.LOCALAPPDATA, { recursive: true });
// This directory is shared across the lifecycle's app versions, and
// Chromium removes DevToolsActivePort only on a clean exit — a stale
// file here pointed the poll at the previous instance's dead port.
await rm(join(smokeUserData, 'DevToolsActivePort'), { force: true });
const smokeChild = spawn(
executable,
// Port 0: the bound port comes back through DevToolsActivePort.
[
`--remote-debugging-port=${smokePort}`,
'--remote-debugging-port=0',
`--user-data-dir=${smokeUserData}`,
'--enable-logging=stderr',
],
Expand All @@ -464,7 +483,7 @@ export async function verifyWindowsAutoupdate(
},
);
try {
const smokeTarget = await findRendererTarget(smokePort, smokeChild);
const smokeTarget = await findRendererTarget(smokeUserData, smokeChild);
const deadline = Date.now() + 30_000;
for (;;) {
const state = await evaluateInRenderer(
Expand Down
34 changes: 29 additions & 5 deletions scripts/verify-windows-installer-lifecycle.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,18 @@ $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.
const { stdout } = await run(
'powershell',
['-NoProfile', '-NonInteractive', '-Command', script],
{
timeoutMs: 30_000,

@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] A 30-second probe bound does not fit inside a 60-second wait. waitForInstalledProcessesToExit defaults to timeoutMs = 60_000, so with this bound in place a loop that hits the stall this PR is fixing gets at most two observations before its deadline, and if both stall it throws Installed Maka processes did not exit within 60000ms: <unknown> — never having read the process table once. That is a more confusing failure than the hang it replaces, because the message names a condition the code never actually observed. The comment above is right that the loop's deadline should stay the authority, but that only holds when the probe bound is small relative to the budget; here it is half of it. Make the probe bound a fraction of the caller's timeout — a few seconds, matching the 1-second poll interval — or raise the wait's default, and have the message distinguish "still running" from "never successfully probed" rather than printing <unknown> for both.

},
);
if (!stdout.trim()) return [];
const parsed = JSON.parse(stdout);
return Array.isArray(parsed) ? parsed : [parsed];
Expand All @@ -53,16 +64,29 @@ export async function waitForInstalledProcessesToExit(
} = {},
) {
const deadline = Date.now() + timeoutMs;
let processes = await listProcesses(installDirectory);
while (processes.length > 0) {
let processes = [];
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.

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) {
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 ? `\nLast process probe failed: ${probeError.message}` : ''
}`,
);
}
await sleep(pollIntervalMs);
processes = await listProcesses(installDirectory);
}
}

Expand Down
Loading