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 6 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.
Outdated
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Outdated

@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
25 changes: 14 additions & 11 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 @@ -442,18 +441,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 +467,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
Loading