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

[Joob1n]

Summary

The Windows upgrade-lifecycle check hung a CI job for 59 minutes past a 120-second deadline, and the runner cancelled it. The app it was waiting for had in fact relaunched — the runner terminated Maka-0.1.12-win-x64 as an orphan during cleanup — so the deadline should have fired and did not.

listInstalledProcesses runs Get-CimInstance Win32_Process through runCommand with no timeoutMs, and a poll loop only checks its deadline between probes. One stalled probe therefore makes the deadline unenforceable. The stall is not random: it happens while NSIS hands off and relaunches the upgraded app, with the process table in flux, and it reproduced on two consecutive runs at the same step.

Two changes, from one rule — a poll loop bounds its own attempts, and no single stalled attempt outranks the loop's deadline:

• The probe takes a timeoutMs (10 seconds by default), well under the 60-second and 120-second budgets above it. An earlier revision used 30 seconds; @likun666661 pointed out that leaves room for two attempts inside a 60-second poll, so one stall consumes the whole window — and a loop that can only try twice cannot survive a stall, which is the entire point of tolerating one.
• waitForInstalledProcessesToExit treats a failed probe as an unknown round and keeps polling. Its deadline stays the authority, and the message now distinguishes the two faults: never having read the process table is reported as unknown, separately from having observed processes that would not exit. Only one of those is about Maka.

Scope

This PR originally also replaced the reserve-then-release CDP port discovery. That half now belongs to #3265, which reads the port from Chromium's DevTools listening on … stderr line — fresh per launch, so it cannot go stale, which is the failure this branch had to guard against explicitly (the upgrade-lifecycle check reuses one user-data directory across two app versions with a kill in between). It wins on merit, and @liugddx and I split at file level so neither of us pays another flake-priced CI cycle. Both PRs independently arrived at the same per-attempt poll bound; that goes with the port half.

What is left touches only scripts/verify-windows-installer-lifecycle.mjs, which #3265 does not touch.

Verification

Measured on CI, across three revisions of this branch, at the same step:

Revision	Result at the relaunch wait
before	hung 59 minutes past a 120s deadline; runner cancelled the job
bounded probe only	failed in 74 seconds, naming the stalled query
bounded probe + tolerant loop	relaunch detected in 27 seconds

Locally, the loop was driven through all three of its paths with an injected probe: transient stalls followed by a clean exit return successfully after retrying; probes that never succeed report Could not read the process table within …, so whether installed Maka processes exited is unknown; observed processes that do not exit report themselves by name and pid.

lint, format:check — pass.

Known unrelated failure on this lane

The auto-update step can still fail after this fix, at the next stall: taskkill /PID … did not finish within 30000ms while stopping the relaunched instance. Same family — Windows process operations stalling around the NSIS relaunch — but it lives in verify-windows-autoupdate.mjs, which is #3265's half under the split. Data handed over there rather than patched here.

Separately, #3279 tracks an intermittent product-side fault on the same path: the upgraded install's Runtime Host stops responding during startup, which this lane surfaces as a renderer that never appears.

Breaking change

None. Release verifier internals only.

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — drafted the change and ran the verification above. Reviewed and submitted by the contributor of record. Generated-by: Claude Code is on the commits.

Checklist

• Tests cover the change and fail without it — the loop semantics are exercised through an injected probe as described above; the stall itself is only reproducible on a Windows runner mid-relaunch
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — a stalled Windows process probe no longer freezes or decides a release verification
• No

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c44367b6-7f8e-44cf-8169-0c7a8486b807

📥 Commits

Reviewing files that changed from the base of the PR and between 3ecb005 and 6532f7a.

📒 Files selected for processing (1)

• scripts/verify-packaged-app.mjs

Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Problem solved

The PR removes the CDP port race in smokePackagedRenderer. Chromium now selects the port with --remote-debugging-port=0 and writes it to DevToolsActivePort.

The verifier removes stale port data before startup. It reads and polls the port for the current process.

Timeouts report the port, elapsed time, and the failure stage. They distinguish missing port files, fetch failures, unexpected HTTP responses, and missing page targets. The verifier still fails immediately when the renderer process exits before readiness.

Design and scope

The PR extends the existing packaged-app verifier path. It does not create a parallel path or change exported declarations.

This is the smallest coherent solution. It removes manual port reservation and release and uses Chromium’s DevToolsActivePort as the source of truth. Stale-file removal and the added polling states are necessary for reused user-data directories and useful diagnostics.

No code or tests can be removed without weakening behavior or regression coverage based on the available evidence.

Complexity delta

• Authorities: Removes manual port allocation. Uses Chromium’s port file.
• States: Adds missing-file, connection-failure, unexpected-response, and missing-page-target states.
• Branches: Adds port-file polling and failure-stage reporting. Preserves the renderer-exited branch.
• Configuration: Uses --remote-debugging-port=0 and extends CDP and renderer-readiness deadlines to 120 seconds.
• Public surface: Adds no public or exported entities.
• Test maintenance: Removes race-prone port setup and retains stale-file and packaged-app coverage.

Total maintenance complexity decreases. The added branches and deadlines are necessary for reliable discovery on cold runners and for actionable failures.

Validation

Validation included packaged-app testing, stale-file testing with a pre-seeded DevToolsActivePort, lint, formatting, and affected checks. Required check results remain unverified from the available evidence.

Review-relevant risks

The current diff affects release-verifier internals. No apparent effect on user-visible behavior, public contracts, security, licensing, release artifacts, or governance was identified.

Any material change in a protected area requires independent human review under repository policy. The person performing the merge reviews the final diff, and a maintainer makes the final determination.

Walkthrough

The packaged app verifier now lets Chromium select the DevTools port, reads the assigned port from DevToolsActivePort, and reports detailed renderer readiness polling failures.

Changes

Packaged renderer verification

Layer / File(s)	Summary
DevTools readiness polling scripts/verify-packaged-app.mjs	The verifier reads DevToolsActivePort, polls /json/list for up to 120 seconds, and records missing-file, connection, HTTP-status, and missing-target states.
Dynamic port launch wiring scripts/verify-packaged-app.mjs	The verifier removes stale port state, starts the packaged renderer with --remote-debugging-port=0, passes the user-data directory to target discovery, and allows renderer usability polling for 120 seconds.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 6532f

The release smoke now discovers Chromium’s self-selected CDP port, but a stalled CDP request can still exceed the intended timeout, delay cleanup, and leave the verifier process running; non-OK responses also lack status details in diagnostics. Merge should wait for bounded requests and explicit handling of this follow-up risk.

Sequence Diagram(s)

sequenceDiagram
  participant Verifier
  participant PackagedRenderer
  participant Chromium
  participant DevToolsEndpoint

  Verifier->>PackagedRenderer: Remove stale port file and launch with --remote-debugging-port=0
  PackagedRenderer->>Chromium: Start renderer
  Chromium-->>Verifier: Write DevToolsActivePort
  Verifier->>DevToolsEndpoint: GET /json/list
  DevToolsEndpoint-->>Verifier: Return renderer targets or polling state

Loading

Possibly related PRs

• maka-agent/maka-agent#3240 — Both changes modify scripts/verify-packaged-app.mjs for packaged renderer CDP verification.

Suggested reviewers: zhiiw, liugddx

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: discovering Chromium's actual CDP port instead of reserving one.
Description check	✅ Passed	The description follows the template and documents the problem, solution, verification, AI use, checklist, behavior change, and linked issue.
Linked Issues check	✅ Passed	The changes address issue #3196 by removing the port race, reading DevToolsActivePort, improving diagnostics, and preserving early-exit handling.
Out of Scope Changes check	✅ Passed	The timeout extensions, stale-file removal, and diagnostic improvements directly support the linked issue and stated release-verifier scope.
Ai Use Disclosure	✅ Passed	The PR selects only substantive generative use, names Claude Code and its scope, and all three introduced commits contain matching standalone Generated-by: Claude Code trailers.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Eliminate CDP port race in packaged renderer smoke checks

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Let Chromium allocate CDP ports and discover them through DevToolsActivePort.
• Remove the reserve-and-release race from packaged renderer smoke checks.
• Classify CDP timeouts with elapsed time, discovered port, and last fetch error.

Diagram

graph TD
  A["Smoke verifier"] --> B["Launch port 0"] --> C{"Port file found?"}
  C -- "No" --> D{"Deadline reached?"}
  C -- "Yes" --> E["Poll CDP target"] --> F{"Target ready?"}
  F -- "Yes" --> G["Evaluate renderer"]
  F -- "No" --> D
  D -- "No" --> C
  D -- "Yes" --> H["Classified timeout"]

Loading

High-Level Assessment

The chosen approach is optimal because Chromium atomically selects an available port and publishes the authoritative value through its standard DevToolsActivePort mechanism. Parsing stderr would be less stable, while reserve-and-release or fixed-port strategies retain collision risk.

Files changed (1) +50 / -36

Bug fix (1) +50 / -36

verify-packaged-app.mjsDiscover Chromium's bound CDP port without pre-reservation +50/-36

Discover Chromium's bound CDP port without pre-reservation

• Removes the reserve-and-release TCP port helper and launches Electron with '--remote-debugging-port=0'. The smoke check reads Chromium's selected port from DevToolsActivePort, polls that endpoint, and reports cause-specific timeout diagnostics with elapsed time and fetch errors.

scripts/verify-packaged-app.mjs

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d614f76c-c566-4f54-b899-a5f062e7af47

📥 Commits

Reviewing files that changed from the base of the PR and between dc65776 and 7f61f1e.

📒 Files selected for processing (1)

• scripts/verify-packaged-app.mjs

Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline scripts/verify-packaged-app.mjs --view expanded
printf '%s\n' '--- relevant source ---'
sed -n '1,145p' scripts/verify-packaged-app.mjs
printf '%s\n' '--- package/runtime declarations ---'
rg -n --glob 'package.json' --glob '*.json' '"(engines|node|electron)"' . | head -80
printf '%s\n' '--- usages and tests ---'
rg -n 'findRendererTarget|DevToolsActivePort|verify-packaged-app|AbortSignal|fetch\\(' scripts test tests package.json .github 2>/dev/null | head -200

Repository: maka-agent/maka-agent

Length of output: 13651

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- runtime and workflow versions ---'
node --version
rg -n -C 3 'node-version|setup-node|verify-packaged-app|smokePackagedRenderer' .github scripts package.json 2>/dev/null | head -160
printf '%s\n' '--- launch and cleanup context ---'
sed -n '257,310p' scripts/verify-packaged-app.mjs
sed -n '186,245p' scripts/verify-packaged-app.mjs
printf '%s\n' '--- standalone cancellation probe ---'
node - <<'JS'
const events = [];
const signal = AbortSignal.timeout(30);
const stalledFetch = (_url, { signal }) =>
  new Promise((resolve, reject) => {
    const onAbort = () => {
      events.push(`aborted:${signal.reason?.name}`);
      reject(signal.reason);
    };
    if (signal.aborted) return onAbort();
    signal.addEventListener('abort', onAbort, { once: true });
  });

globalThis.fetch = stalledFetch;
const started = Date.now();
try {
  await fetch('http://127.0.0.1:1/json/list', { signal });
} catch (error) {
  console.log(JSON.stringify({
    apiAvailable: typeof AbortSignal.timeout === 'function',
    elapsedMs: Date.now() - started,
    errorName: error?.name,
    events,
  }));
}
JS

Repository: maka-agent/maka-agent

Length of output: 16284

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- package engine ---'
sed -n '1,18p' package.json
printf '%s\n' '--- smoke renderer callers and working-directory setup ---'
rg -n -C 5 'smokePackagedRenderer\\(|workingDirectory:|mkdtemp|temporaryDirectory|rm\\(' scripts .github | head -240
printf '%s\n' '--- deterministic abort probe ---'
node - <<'JS'
(async () => {
  const controller = new AbortController();
  const keepAlive = setTimeout(() => controller.abort(new DOMException('deadline', 'TimeoutError')), 30);
  const events = [];
  const stalledFetch = (_url, { signal }) =>
    new Promise((resolve, reject) => {
      const onAbort = () => {
        events.push(`fetch-aborted:${signal.reason?.name}`);
        reject(signal.reason);
      };
      if (signal.aborted) return onAbort();
      signal.addEventListener('abort', onAbort, { once: true });
    });

  globalThis.fetch = stalledFetch;
  const started = Date.now();
  try {
    await fetch('http://127.0.0.1:1/json/list', { signal: controller.signal });
  } catch (error) {
    console.log(JSON.stringify({
      apiAvailable: typeof AbortSignal.timeout === 'function',
      elapsedMs: Date.now() - started,
      errorName: error?.name,
      events,
    }));
  } finally {
    clearTimeout(keepAlive);
  }
})();
JS

Repository: maka-agent/maka-agent

Length of output: 911

---

Fix now: bound each CDP request by the remaining deadline.

Pass a deadline-based AbortSignal to fetch(); it also cancels response.json() body reads. Skip the 250 ms delay when the deadline expires so the timeout and finally cleanup run promptly.

Source: Path instructions

[Astro-Han]

Confirmed on exact head 090158d8d; I agree with this finding and calibrate it as P2. The outer 120-second loop does not bound an individual fetch() or response.json(). A peer that accepts the TCP connection and then never responds can therefore pin the verifier until the workflow timeout, bypassing the intended diagnostic deadline and delaying stopChild(). The smallest fix is to pass an AbortSignal based on the remaining deadline to the request/body read and add a hanging-server regression. I am not opening a duplicate inline thread.

中文说明

已在当前 head 复现并确认，校准为 P2。外层 120 秒循环并不能限制单次 fetch/response.json；如果对端建立连接后不响应，验证会一直挂到 workflow 超时，finally/stopChild 也无法及时执行。请用剩余 deadline 创建 AbortSignal，并补“连接成功但不响应”的测试。

[coderabbitai]

@Astro-Han, agreed. The finding remains valid and open.

The fix should derive an AbortSignal from the remaining findRendererTarget() deadline for both fetch() and response.json(). The regression should use a server that accepts the connection but does not send a response. It should verify that the verifier reaches its diagnostic timeout and executes cleanup without waiting for the workflow timeout.

_{You are interacting with an AI system.}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Stale CDP port stays cached ✓ Resolved 🐞 Bug ≡ Correctness

Description

findRendererTarget permanently caches the first valid DevToolsActivePort, even when it predates
the newly spawned process. The Windows upgrade-lifecycle check reuses the same user-data directory
for two sequential app launches, so its second smoke can poll the previous app's closed port for the
full timeout instead of discovering the new port.

Code

scripts/verify-packaged-app.mjs[108]

+    port ??= await readDevToolsPort(userDataDirectory);

Relevance

●●● Strong

Recent accepted release-smoke correctness findings support fixing deterministic stale state causing
sequential verification timeouts.

PR-#3185
PR-#3188

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The smoke uses the deterministic workingDirectory/user-data path and only creates it recursively,
leaving DevToolsActivePort behind after stopChild. The installer lifecycle passes the same
smokeDirectory first to the previous-version verification and then to the upgraded-version
verification; because line 108 only reads while port is null, the second invocation can cache the
first invocation's port before the new Chromium process overwrites the file and will never reread
it.

scripts/verify-packaged-app.mjs[99-108]
scripts/verify-packaged-app.mjs[257-271]
scripts/verify-packaged-app.mjs[303-307]
scripts/verify-windows-installer-lifecycle.mjs[121-155]
scripts/verify-windows-x64.mjs[208-209]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`findRendererTarget` can cache a stale `DevToolsActivePort` left by an earlier app launch. The Windows installer lifecycle reuses the smoke directory, causing the second verification to keep polling the previous port after Chromium writes a new one.

## Issue Context
The user-data directory is created recursively without being cleared, and `port ??=` prevents rereading the file once any valid value is found. Prefer deleting the unnecessary stale artifact before spawning Chromium; this is smaller than introducing new state or configuration.

## Fix Focus Areas
- scripts/verify-packaged-app.mjs[108-108]
- scripts/verify-packaged-app.mjs[257-271]
- scripts/verify-windows-installer-lifecycle.mjs[121-155]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced: This changes release smoke runtime behavior and process/port-discovery error handling; the risk is localized to one path but warrants a complete careful review.

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Astro-Han]

Thanks for removing the reserve-then-release race and making Chromium's DevToolsActivePort the source of truth—the direction and scope are both good.

I’m leaving this as COMMENT for the current head because the Windows installer lifecycle check reproduces a stale-port failure already captured in the existing CodeRabbit/Qodo thread on scripts/verify-packaged-app.mjs:108. The lifecycle reuses the smoke directory, the old DevToolsActivePort remains, and port ??= permanently caches that old value while the new process listens elsewhere. The smallest fix is to remove the stale file before spawning (or otherwise require a fresh value for this launch) and add a regression with a pre-existing port file. It would also be prudent to bound each CDP fetch() attempt with an abort deadline so one half-open response cannot outlive the outer poll deadline.

Once that existing thread is addressed and the Windows lifecycle lane is green, this should be straightforward to re-review.

AI-assisted review disclosure: Codex reviewed exact head 7f61f1e and verified the blocking path against the current failing Windows CI log; no duplicate inline comment was added.

中文说明

这个改动从 Chromium 实际写出的 DevToolsActivePort 读取端口，方向和范围都是对的。但当前 Windows installer lifecycle 已真实复现旧端口文件的问题：测试复用 smoke 目录，旧文件仍存在，而 port ??= 一旦读到旧端口就不会再刷新，最终一直轮询错误端口并超时。建议在启动前删除旧文件（或明确只接受本次启动产生的新值），并补一个预置旧端口文件的回归测试。现有行内线程已经覆盖，因此不重复发 inline comment。

[Joob1n]

The first CI run of this PR paid for itself: the package job failed again, but this time the log said polled port 51921 from DevToolsActivePort; last error: fetch failed — which falsifies the port-collision hypothesis (#3196's suspected cause) and names the real one. Chromium removes DevToolsActivePort only on a clean exit, and the upgrade-lifecycle step reuses one user-data directory across two app versions with a kill in between, so the poll was reading the previous instance's file and waiting on a port nothing listened on. 3ecb005 deletes the file before spawning, so whatever appears was written by the child under test. Verified against a real packaged app with a stale file pre-seeded naming a dead port — the smoke passes. This also explains why the historical flake clustered in the lifecycle step: the standalone smoke uses a fresh directory and never had a stale file to read.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

scripts/verify-packaged-app.mjs (1)

111-119: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Record non-OK /json/list responses.

When fetch() returns ok === false, set lastError with the HTTP status before retrying. Otherwise, the timeout reports only the port and does not distinguish an unhealthy endpoint from a missing port file.

Source: Path instructions

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5f514c1a-da4f-4e57-b98e-3700d365b648

📥 Commits

Reviewing files that changed from the base of the PR and between 7f61f1e and 3ecb005.

📒 Files selected for processing (1)

• scripts/verify-packaged-app.mjs

Included review availability: Your plan provides up to 3 included reviews per hour; 1 remains after this review.

[Joob1n]

The second diagnostics run separated one more cause, again in a way the old log could not: this time the standalone smoke (fresh directory, so the port file was the child's own after 3ecb005) reported polled port 54625 from DevToolsActivePort. with no fetch error — meaning /json/list answered throughout and no debuggable page target appeared within the old 30-second line. That is a healthy app on a cold Windows runner that scans a first-run executable, consistent with #3196's original observation of the step taking 74 seconds against the 30-second deadline. 6532f7a applies this file's own stated rule — a wrong deadline fails a good release — to the two polls: target discovery and the renderer-usable loop get 120 seconds, the child exiting still fails immediately, and the workflow timeout stays the outer bound. The timeout message now states where discovery stalled: no port file, a port that never answers, an unexpected HTTP status, and a healthy endpoint with no page target are four different faults. So the flake decomposed into three real causes, each now handled: the stale port file from the lifecycle's reused profile (deleted before spawn), the too-tight deadline for cold runners (widened), and the log that could not tell them apart (it now can).

[Astro-Han]

The stale DevToolsActivePort issue is fixed on this head: the verifier removes the old file before spawn and now waits longer for the packaged renderer. The authority remains Chromium's actual port, which is the right design.

I’m keeping COMMENT because the existing unresolved CodeRabbit thread at scripts/verify-packaged-app.mjs:117-119 still applies: neither fetch() nor response.json() is bounded by the remaining deadline. A half-open CDP endpoint can therefore outlive the 120-second outer loop and leave cleanup to the workflow timeout. Please pass an AbortSignal derived from the remaining attempt/deadline budget and cover a server that accepts but never answers.

The new Windows workflow is still queued; it must pass before this can be considered merge-ready. No duplicate inline comment was added.

AI-assisted review disclosure: Codex re-reviewed current head 6532f7a, the two intervening deltas, current Windows runs, and existing thread state.

中文说明

当前 head 已修复旧 DevToolsActivePort 文件问题，并延长 packaged renderer 等待时间，方向正确。仍需处理现有 CodeRabbit 线程：fetch() 和 response.json() 没有受剩余 deadline 的 AbortSignal 约束，半开连接仍可能让 verifier 无限等待到 workflow timeout。建议按剩余时间传入 abort signal，并补“连接建立但永不响应”的回归测试。新的 Windows workflow 仍在排队。

[Astro-Han]

Thanks for replacing the reserve-then-release race with Chromium's own DevToolsActivePort record. The stale-file cleanup and more specific diagnostics are sound, and this release-verifier-only change does not require product UI screenshots. The PR body and all four commits have complete Claude Code disclosure/trailers.

One P2 remains in the existing CodeRabbit thread, which I confirmed and replied to rather than duplicating: an individual fetch() or body read is not bounded by the 120-second loop, so a peer that accepts the connection and never responds can hang the job until the workflow timeout. Please add a remaining-deadline AbortSignal and a hanging-server regression.

The required Release Windows check and Storybook jobs are also still running. Please resolve the P2 and let the exact-head checks complete before rereview.

Reviewed with Codex as an AI-assisted code review. I verified the exact-head diff, deadline behavior, existing threads, CI, UI scope, and commit provenance; no external model output was used.

中文说明

用 DevToolsActivePort 消除端口竞态的方向正确，stale file 清理与诊断也合理；这是 release verifier 内部改动，不需要产品截图，AI 说明和四个 commit trailer 都完整。当前仍有一个已存在的 CodeRabbit P2：单次 fetch/response.json 没有被 120 秒 deadline 约束，连接后不响应会挂到 workflow 超时。我已在原 thread 回复确认，没有重复开 inline。修复并等 required checks 完成后再 review。

[Joob1n]

One more data point while this waits for review: #3148's merge-CI just hit the same class again — Packaged Maka renderer did not expose CDP within 30 seconds: fetch failed — this time inside the auto-update verification #3240 added yesterday, which is another consumer of the pre-fix smoke. That is four occurrences across two PRs in two days, each needing a full retrigger. The three fixes here (stale DevToolsActivePort deleted before spawn, cold-runner deadlines, cause-classifying timeout messages) apply to every smoke call site through the unchanged smokePackagedRenderer signature.

[Joob1n]

The approved run's one failure is worth reading rather than retrying blindly — the new diagnostics attributed it precisely, and it is not the port plumbing this PR fixes: the upgraded 0.1.11 install's own stderr shows [runtime-host] fatal: Error: Runtime Host stopped responding during startup, so the main process never served CDP at the port it had bound. The 0.1.9 baseline and the standalone 0.1.11 smoke both passed in the same job; only the upgrade-over-existing-profile path hung. That is a product-side question now tracked as #3279. Two verifier notes from the same log, both addressed in 119d9da: the loop reported 355s against a 120s deadline because an attempt against a bound-but-unresponsive endpoint hangs for undici's 300s headers timeout — each poll attempt now carries a 2s abort; and the app's fatal stderr riding along in the failure message is what made this attribution possible at all. Could someone approve the runs for the new head? This PR cannot make a hung Runtime Host pass — nor should it — but on a healthy app the port-file discovery is exercised end to end.

[Astro-Han]

[P2] Bound the WebSocket handshake as well as the CDP HTTP poll

The previous unbounded fetch() / response-body issue is fixed on exact head 119d9da: the per-attempt AbortSignal also aborts a stalled response.json() read.

However, evaluateInRenderer() still awaits the WebSocket open or error event before starting its timeout. A peer that accepts the TCP connection but never completes the WebSocket handshake therefore leaves the verifier in CONNECTING until the workflow-level timeout. I reproduced this locally with a TCP server that accepts the connection without completing the upgrade; neither event fired and the promise remained unsettled.

This is relevant to the current exact-head Release Windows run: all earlier package, standalone-smoke, and upgrade-lifecycle steps passed, while Verify automatic update end to end has remained in progress for over 30 minutes.

Please start the timeout before opening the WebSocket, cover both the handshake and the Runtime.evaluate response, close the socket when the deadline expires, and add a regression using a server that accepts the connection without completing the WebSocket handshake. The current required run should also reach a terminal state and its log should be inspected before rereview.

Reviewed by Astro-Han with Codex assisting as an AI-assisted code review. Astro-Han made the review decision; Codex verified the exact-head diff, reproduced the hanging handshake, and monitored the required Windows run.

中文说明

最新提交已经修复原来的 fetch/body-read 超时问题，但 WebSocket 建连阶段仍没有超时：TCP 建立后如果不完成 WebSocket 握手，验证会一直卡到 workflow timeout。本地已复现，当前 Windows exact-head run 的自动更新步骤也已长时间不结束。请让同一个 deadline 覆盖握手和 Runtime.evaluate、超时后关闭 socket，并补 hanging-handshake 回归测试。

[Joob1n]

Status on the approved run, since it reads worse than it is: the fix worked, and the run died of something else.

On 119d9da the Windows installer lifecycle passed end to end — installing previous version 0.1.9 → verifying the previous installed application → upgrading to 0.1.11 → verifying the installed application → uninstalling → lifecycle verified. The Runtime Host startup hang from the previous head did not reproduce, which is consistent with #3279 being intermittent. Every other check on that head is green: CI, Windows baseline, Windows recovery, Windows sandbox W0.

The job then went on to the auto-update verification, got as far as waiting for the upgraded app to relaunch automatically, and hung there for 59 minutes until the runner cancelled it. Two facts settle what happened: that loop has a 120-second deadline, and at cleanup the runner reported Terminate orphan process: pid (5576) (Maka-0.1.12-win-x64) — so the app had relaunched and the deadline should have fired. What hung is the probe: listInstalledProcesses runs Get-CimInstance Win32_Process through runCommand with no timeoutMs, and a poll loop only checks its deadline between probes, so one stuck probe makes the deadline unenforceable.

That is the same defect class this PR already fixed one layer up (119d9da bounded each CDP poll attempt after a hung fetch reported 355s against a 120s deadline). 55ce142 bounds the process probe at 30 seconds — well above a normal WMI query, well below the 60s and 120s deadlines that sit above it — so a stuck query fails naming itself instead of freezing the job.

Both hangs were invisible before: the old code could only report "did not expose CDP within 30 seconds" or nothing at all. Worth stating plainly, though: I have not reproduced the WMI stall locally, so the 30-second bound is a guard derived from the observed failure, not from a root cause I can demonstrate. If the probe starts failing at 30s on healthy runners, that is a signal worth reading rather than raising the number.

Could someone approve the runs for 55ce142? The fork-PR approval requirement came with the move into apache, so every push needs a click — sorry for the traffic.

[Joob1n]

The probe bound in 55ce142 did its job and, in doing so, showed it was not the whole fix.

That run reached waiting for the upgraded app to relaunch automatically at 06:59:28 and failed at 07:00:42 with powershell … did not finish within 30000ms — 74 seconds instead of the previous run's 59-minute hang. So the Windows process query does not stall randomly: it stalls at one specific moment, while NSIS hands off and relaunches the upgraded app and the process table is in flux. Two runs, same place.

That makes the bound alone wrong in a way worth naming: a probe that gives up says nothing about the processes, but it was aborting a verification whose own 120-second deadline had barely started — about a relaunch that had in fact happened (the earlier run's cleanup terminated the relaunched Maka-0.1.12-win-x64 as an orphan). af023a7 completes it: the poll loops treat a failed probe as an unknown round and keep polling, so their deadline is the authority, and the last probe failure is reported with the deadline message so a persistent stall stays visible instead of being mistaken for a missing app.

Where that leaves this PR's scope: it started as "read the CDP port Chromium bound rather than reserving one" and has become one coherent rule for the release verifier — every poll loop bounds its own attempts, and no single stalled attempt outranks the loop's deadline. Three instances now: the CDP fetch (undici's 300s default blew a 120s deadline to 355s), the process probe (unbounded, hung a job for an hour), and the loops around it. I would rather state that plainly than pretend the last two commits are about port discovery.

Not verified locally: I cannot reproduce the WMI stall on macOS, so the retry behaviour is reasoned from the two observed runs, not demonstrated. If a stall ever outlasts a full 120-second window, the new message will say so, and that is a genuine finding rather than this loop's problem.

Runs on af023a7 need approval when someone has a moment.

[Joob1n]

Coordination note: @liugddx's #3265 fixes the same CDP fault independently, reading the port from Chromium's DevTools listening on … stderr line rather than the DevToolsActivePort file, with a 90-second deadline. Their mechanism is structurally immune to the staleness this PR has to guard against explicitly, so I have offered there to drop my port-discovery half in favour of theirs. What I would keep either way, because it is independent of how the port is found: the per-attempt fetch bound (undici's 300s default let one run report 355 seconds against a 120-second deadline) and the Windows process-probe bound plus poll-loop tolerance (an unbounded Get-CimInstance Win32_Process hung a job for 59 minutes past a 120-second deadline). Details in #3265 (comment). Happy to reshape this PR whichever way the maintainers prefer.

[Astro-Han]

Thanks — reading the port back from DevToolsActivePort is the right fix, and removing the race beats reporting it. Reviewed exact head af023a7ff95d12f44496dd2c9a87c71aac1af977.

The core change is sound and is what the browser-automation tools do for exactly this reason: --remote-debugging-port=0 plus the port file is the only discovery method with no window between choosing a number and binding it. Deleting a stale port file before spawn is the necessary companion — Chromium removes it only on a clean exit, and the upgrade-lifecycle path SIGKILLs between versions in one user-data directory, so without that rm the poll would read the dead instance's port. Bounding the fetch against undici's 300-second header timeout, and bounding the PowerShell probe that runs inside deadline-bounded loops, are both real fixes to the same underlying mistake: a deadline you only check between unbounded operations is not a deadline. I also checked that rm is already imported in verify-windows-autoupdate.mjs and that both call sites there were updated to the new findRendererTarget signature.

Two P2s and a P3 inline, all about where the new bounds were and were not applied. Also worth noting: no CI checks are reported at this head, so the end-to-end macOS run in the description is currently the only evidence — and since these scripts only execute in the release workflow, that is the evidence that matters most.

Review disclosure: this review was prepared with Claude Code, which read the diff at this head, checked the import and call-site consistency described above, and enumerated the surviving timeout constants across all three scripts. Nothing here was reproduced by running the verifiers. The human contributor reviewed this before posting.

[Astro-Han]

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

[Astro-Han]

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

[Astro-Han]

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

[Astro-Han]

Re-reviewed at exact head 5543fba031f0f1bb372e0f6305691747f18a6293. The head moved only by a merge of origin/main: all three changed files are byte-identical to the head I reviewed before (af023a7ff), so I am not re-filing the findings as new inline comments — the existing threads still apply verbatim and remain accurate at this commit.

Standing, unchanged:

• [P2] verify-windows-installer-lifecycle.mjs — a 30-second probe bound inside a 60-second waitForInstalledProcessesToExit default gives the loop at most two observations, and two stalls produce did not exit within 60000ms: <unknown> for a condition never actually observed.
• [P2] verify-windows-autoupdate.mjs — the new 120-second headroom was applied in smokePackagedRenderer but not to rendererDeadline or the nested smokeRenderer deadline, both still 30 seconds, on the Windows path the description names as having hit this fault twice.
• [P3] verify-packaged-app.mjs — findRendererTarget latches the first port it reads and never re-reads, so its freshness precondition lives only in three call-site comments rather than in the exported signature.

The core change is still right and I still like it: --remote-debugging-port=0 plus the port file is the only discovery method with no window between choosing a number and binding it, and deleting a stale port file before spawn is the necessary companion. One coordination note that has become relevant since: #3265 is fixing the same CDP-port discovery race in this same file, and its branch is carrying its own copy of a waitForDevToolsPort helper plus a 30s→90s discovery timeout. Whichever lands second will conflict here — this PR is the narrower and better-scoped of the two, so my suggestion is that #3265 rebase onto this rather than the reverse.

No CI checks are reported at this head.

Review disclosure: this review was prepared with Claude Code, which compared the three changed files between the previous head and this one by content hash and re-read the surrounding timeout constants. The human contributor reviewed this before posting.

[likun666661]

Reviewed exact head 5543fba031f0f1bb372e0f6305691747f18a6293.

The core fix is sound: using --remote-debugging-port=0 and Chromium's DevToolsActivePort removes the reserve-then-release race, and the stale-file cleanup plus improved diagnostics are good changes.

I do not think this is merge-ready yet. The existing unresolved inline threads still apply:

• [P2] listInstalledProcesses allows a 30-second probe inside waitForInstalledProcessesToExit's 60-second total budget. Two stalled probes can consume the entire deadline without one successful process-table observation, yet the resulting error reports that processes did not exit. The per-probe timeout should be much smaller (or derived from the outer remaining budget), and the diagnostic should distinguish "still running" from "never successfully probed".
• [P2] The Windows auto-update path still has two 30-second renderer-readiness deadlines while the general packaged smoke was raised to 120 seconds specifically for slow cold Windows runners. Please apply the same headroom consistently, preferably through a shared constant, or document why those two launches are guaranteed to be faster.
• [P3] findRendererTarget permanently latches the first port it reads via port ??=; freshness therefore depends on every caller remembering to remove DevToolsActivePort before spawn. The current callers do, but the exported helper does not encode or enforce that invariant. Re-reading when the file changes or centralizing cleanup/spawn/discovery would make this robust.

Also, the exact-head GitHub Actions runs currently show action_required with no jobs, so there is no passing CI evidence for this commit yet. Please address the P2s and let the Release Windows lane complete successfully before merge.

AI-assisted review disclosure: Codex inspected the exact-head diff, current unresolved review threads, and exact-head workflow/check state. No code or PR metadata was changed.

[Joob1n]

@likun666661 — all three addressed in 7eeb843, and the first one was the sharpest catch on this PR so far.

P2 — probe budget. You are right, and it makes my own change nearly pointless as written: a 30-second probe inside a 60-second poll leaves room for two attempts, so one stall consumes the window — and a loop that can only try twice cannot survive a stall, which is the entire reason to tolerate one. The probe is now 10 seconds and takes timeoutMs as a parameter, so 60s allows about six attempts and 120s twelve.

The diagnostic is separated the way you asked, in both loops. Never having read the process table is now its own message — "Could not read the process table within 60000ms, so whether installed Maka processes exited is unknown" — distinct from having observed processes that would not exit. Only one of those two faults is about Maka, and the old wording sent the reader after the wrong one.

P2 — deadline consistency. Fixed through a shared constant, as you preferred: RENDERER_READY_TIMEOUT_MS is exported from verify-packaged-app.mjs and used by target discovery, the packaged smoke, and both auto-update renderer waits. The drift you found is exactly what a literal in four places produces. Two 30-second values remain in that file deliberately — a single checkForUpdates() evaluation and a taskkill — because they bound one call rather than wait for readiness; say the word if you would rather they moved too.

P3 — the latch. Also right, and worth stating what the fix does and does not buy. findRendererTarget now re-reads the file each round rather than keeping the first value, so a caller that left a predecessor's file behind converges as soon as the child overwrites it, instead of polling a dead port for the full deadline. That is a real improvement, but it is not enforcement: a stale file that is never overwritten still cannot be told from a fresh one, so callers removing it before spawn remains a convention the helper documents rather than guarantees. The mechanism that removes the invariant entirely is reading the port from Chromium's stderr line, which is what @liugddx's #3265 does — I have offered there to drop this half in favour of it, and this PR's remaining findings (per-attempt bounds, probe sizing, the diagnostics) apply either way.

Verified against a real packaged app, including the stale-port-file case. On CI evidence: you are right that there is none for the exact head — the runs sit in action_required because fork PRs need a maintainer to start them since the move into apache. Nothing I can do from this side.

[Astro-Han]

Re-reviewed at 7eeb843e. All three standing findings are fixed, and the fixes are better than what I asked for. No new findings.

• Duplicated renderer budget — resolved by extracting RENDERER_READY_TIMEOUT_MS and using it at all four wait sites. The two 30_000 renderer deadlines in verify-windows-autoupdate.mjs that previously contradicted the 120s one are now the same constant, so the "cold runner is slow to serve CDP and equally slow to mount React" reasoning is stated once and applied everywhere. I checked the remaining 30_000 literals in that file: they are per-command and per-evaluate timeouts, not renderer readiness, so leaving them is correct.
• Latched DevToolsActivePort — resolved twice over. The poll now re-reads instead of ??=-latching, so it converges once Chromium overwrites the file; and every call site removes the file before spawning (verify-packaged-app.mjs:317, verify-windows-autoupdate.mjs:242 and :478). The comment is honest about the residual case it cannot detect — a stale file that is never overwritten — which is exactly the right thing to say, since removal at the call sites is what actually closes it.
• Probe failure masked as a Maka failure — resolved in both loops. observed distinguishes "never read the process table" from "read it and the processes were still there / never came back", and the two messages now send the reader after different things. That was the substance of the finding: the old message asserted something about the installer that the run had no evidence for.

The unasked-for fourth change is the best one: dropping listInstalledProcesses's probe timeout from 30s to a configurable 10s means a stalled probe leaves room for several more attempts inside the 60s and 120s budgets, instead of a loop that can only try twice. That is the difference between tolerating a stall and merely surviving one. Worth watching in practice that 10s stays comfortably above a healthy Get-CimInstance on a loaded runner — if it does not, every probe times out and the run fails with the new "unknown" message. That message is at least honest about what happened, so I am noting it rather than filing it.

Reviewed with Claude Opus as an analysis assistant. Every disposition above was verified by reading the source at this head and diffing it against the head I previously reviewed; the Windows paths were not executed.

Not marking this approved only because my standing authorization in this review campaign is comment-only; nothing here blocks it from my side.

[Astro-Han]

Approving at 7eeb843e. The three standing findings are fixed at their mechanisms: one shared RENDERER_READY_TIMEOUT_MS replaces the contradicting deadlines, the DevToolsActivePort poll re-reads and every call site clears a stale file before spawning, and probe failures are no longer reported as installer failures. The probe-timeout change is a real improvement on top. Detail in the previous review; nothing outstanding from my side.

[Astro-Han]

The central argument is right and worth saying so plainly: an unbounded probe inside a deadline-bounded poll loop makes the deadline a lie, and the comment at lines 43–47 names the exact incident that proves it. Tolerating a single stalled probe instead of failing on it is also the right call — the loop's deadline should be the authority, not one flaky WMI query. I have no objection to the shape of the fix.

What I am raising is that the change is not yet complete on its own terms, in three places, and none of them is visible from this diff alone.

Architecturally there is one thing worth deciding rather than fixing: listInstalledProcesses has two callers, and only one of them was taught to tolerate a failed probe. The rule this PR establishes — a bounded probe that fails means unknown, and only the loop's deadline decides — is stated in a comment inside the function, but enforced in the caller. That works while there are two callers and one file; it will not survive a third. Either the tolerant loop becomes the shared helper both callers use, or the rule needs to live somewhere a new caller cannot miss.

None of the findings below blocks a release. I am leaving this as COMMENT rather than approving because the checklist claims test coverage that does not exist in the tree, and that is the kind of thing a second reviewer should not have to discover.

AI disclosure: this review was produced with Claude Code (Opus 5), with a subagent covering security, correctness, integration and simplification in parallel. I independently re-derived every finding published here by reading verify-windows-installer-lifecycle.mjs, verify-windows-autoupdate.mjs and verify-packaged-app.mjs at c611cfd, and confirmed the absence of a test file at this head. The subagent additionally reports executing the injected-probe scenarios; I did not re-run those myself, so treat the specific timings as its evidence rather than mine. Per AGENTS.md this is not independent human review.

[Astro-Han]

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

[Astro-Han]

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

[Astro-Han]

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

[Astro-Han]

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

[Astro-Han]

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

[liugddx]

Thanks for following up on the Windows installer-verifier regression exposed by the merged #2650/#2658 lifecycle work. Reviewed at exact head c611cfd7b1708a2a051047676d1b726ae69ae37c.

The central fix is sound: a deadline-bounded poll cannot contain an unbounded Get-CimInstance Win32_Process call, and bounding each probe restores authority to the outer deadline. Treating a transient WMI failure as an unknown round instead of letting one probe decide the lifecycle check is also the right direction.

I read the existing reviews and exact-head inline threads before finalizing this pass. Astro-Han has already captured the remaining concerns inline, including the sticky observed state, the direct auto-update caller that does not tolerate a rejected probe, the missing committed regression tests, the probe-budget coupling, and the verbose timeout diagnostic. I agree with those threads and am not duplicating them with another set of inline comments.

The exact-head syntax check, diff check, repository lint, and workspace typecheck pass. The GitHub test check is green; the package check currently fails in the automatic-update path. I am leaving this as COMMENT rather than approval until the existing P2 threads are resolved and the required release evidence reaches an accepted terminal state.

No additional finding beyond the existing exact-head threads came out of this pass.

AI-assisted review disclosure: Codex read the exact-head diff, all existing review bodies and inline threads, checked the relevant callers, and ran the static/type checks above. The reviewer made the final scope and disposition decisions.

[Joob1n]

Closing in favour of #3327, which covers this file's change and considerably more.

The story of this PR, for anyone who lands here from a CI log: it began as a fix for the reserve-then-release CDP port race, and every run of it surfaced a deeper cause underneath — an unbounded fetch that let a 120-second deadline report 355 seconds, a 30-second deadline too tight for a cold Windows runner, and finally an unbounded Get-CimInstance Win32_Process probe inside a poll loop, which hung a job for 59 minutes past its deadline while the app it was waiting for had already relaunched.

The port half went to #3265 earlier: reading Chromium's DevTools listening on … stderr line is fresh per launch, so it cannot go stale, which is a failure mode the DevToolsActivePort file has to guard against explicitly. #3327 now carries the probe half too, split out of #3265 at its reviewer's request. Both findings and the measurements behind them are handed over in #3327 (comment) — including the one thing #3327 has not picked up yet, that a 30-second probe inside a 60-second wait leaves room for two attempts, which @likun666661 identified here and which is what took this branch to 10 seconds.

Nothing here is abandoned; it is all in a PR that reviewers asked for and that covers more ground. Thanks @likun666661 for the review that made the remaining piece correct, and @liugddx for proposing the split before a merge conflict decided it for us.

Still open and unowned from this line of work: the taskkill stall one step past the relaunch wait, and #3279, where the upgraded install's Runtime Host stops responding during startup.