Skip to content

fix(setup): query Windows process start times - #218

Merged
tt-a1i merged 1 commit into
openpi-dev:mainfrom
qwertvgty:fix-windows-test-portability
Aug 29, 2026
Merged

fix(setup): query Windows process start times#218
tt-a1i merged 1 commit into
openpi-dev:mainfrom
qwertvgty:fix-windows-test-portability

Conversation

@qwertvgty

Copy link
Copy Markdown
Contributor

Problem

Fixes #211.

Windows test runs exposed two platform assumptions:

  • setup config lock liveness could not distinguish a reused PID from the original lock owner on win32, because
    process start-time lookup always returned undefined;
  • capability guidance tests assumed POSIX / path separators, while Windows paths use \ and JSON stringification
    escapes them as \\.

As a result, the Windows reproduction commands failed even though package.json declares Node support without an OS
restriction.

Value

Windows contributors can trust the targeted setup/capabilities tests instead of being blocked by unrelated platform
failures.

More importantly, setup config lock recovery now exercises the same PID + process-start-time safety check on Windows
as on POSIX. A stale lock left by a crashed writer can be recovered when the owner identity is provably dead, while
uncertain ownership still fails closed.

Approach

Implemented a platform-specific process start-time query helper:

  • Windows uses powershell.exe -NoProfile -NonInteractive -Command with Get-Process -Id <pid> and emits Unix
    milliseconds from StartTime;
  • POSIX keeps the existing ps -o lstart= -p <pid> path with LC_ALL=C;
  • command failures, unavailable platform tools, timeouts, or unparsable output still return undefined, preserving
    the existing conservative unknown liveness fallback.

The lock protocol itself is unchanged.

For capabilities tests, replaced the hardcoded POSIX path regex with one that accepts /, \, and JSON-escaped \\.

Validation

Run on Windows with Bun 1.3.14:

  • bun install --frozen-lockfile — pass
  • node --test --experimental-strip-types tests/extensions/shared/setup-config.test.ts — pass, 17/17
  • node --test --experimental-strip-types tests/extensions/capabilities/index.test.ts — pass, 15/15
  • bun run lint — pass
  • bun run typecheck — pass, with existing Effect language-service warnings

Not fully green locally:

Impact

User-visible behavior: None.

Model-visible context/tools: None.

Runtime/lifecycle: Windows setup config lock recovery can now prove a lock owner with a reused PID is dead by
comparing process start time, matching the existing POSIX safety invariant.

Persisted config/data: None. Lock file format and setup config format are unchanged.

Compatibility or risk: Adds a Windows dependency on powershell.exe for the stronger liveness check. If PowerShell is
unavailable, restricted, times out, or returns unparsable output, behavior falls back to the existing conservative
unknown result.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed commit 09b8f54 against the current main.

The implementation itself looks sound. The Windows query keeps the existing fail-closed behavior: unavailable PowerShell, command failures, timeouts, and unparsable output all degrade to unknown ownership rather than authorizing lock recovery. The PID is numeric, the lock protocol and persisted format remain unchanged, and the capability-path assertions now cover POSIX, Windows, and JSON-escaped separators.

I also ran the focused setup-config and capabilities suites locally (32/32 passed), and bun run check passed with only the existing Effect diagnostics. The real Windows execution evidence remains the author's validation; I did not independently rerun it on Windows.

There are no code findings on this head. The current blocker is branch state: GitHub reports this PR as CONFLICTING, specifically because tests/extensions/shared/setup-config.test.ts now overlaps with the recently merged setup tests from PR #203. There are also no GitHub CI results for this head.

Please rebase onto the latest main, preserve both the post-edit command tests from #203 and the new process-start query coverage, then push the resolved head and let CI run. I can take another look once that is green.

@qwertvgty
qwertvgty force-pushed the fix-windows-test-portability branch from 09b8f54 to 3f15789 Compare August 28, 2026 06:24
@qwertvgty

Copy link
Copy Markdown
Contributor Author

Reviewed commit 09b8f54 against the current main.

The implementation itself looks sound. The Windows query keeps the existing fail-closed behavior: unavailable PowerShell, command failures, timeouts, and unparsable output all degrade to unknown ownership rather than authorizing lock recovery. The PID is numeric, the lock protocol and persisted format remain unchanged, and the capability-path assertions now cover POSIX, Windows, and JSON-escaped separators.

I also ran the focused setup-config and capabilities suites locally (32/32 passed), and bun run check passed with only the existing Effect diagnostics. The real Windows execution evidence remains the author's validation; I did not independently rerun it on Windows.

There are no code findings on this head. The current blocker is branch state: GitHub reports this PR as CONFLICTING, specifically because tests/extensions/shared/setup-config.test.ts now overlaps with the recently merged setup tests from PR #203. There are also no GitHub CI results for this head.

Please rebase onto the latest main, preserve both the post-edit command tests from #203 and the new process-start query coverage, then push the resolved head and let CI run. I can take another look once that is green.

Rebased onto latest main and resolved the conflict in tests/extensions/shared/setup-config.test.ts.

The resolution preserves both:

Pushed updated head: 3f15789.

Re-ran focused validation locally on Windows:

  • node --test --experimental-strip-types tests/extensions/shared/setup-config.test.ts — 19/19 passed
  • node --test --experimental-strip-types tests/extensions/capabilities/index.test.ts — 15/15 passed
  • bun run typecheck — passed, with existing Effect diagnostics

@qwertvgty
qwertvgty requested a review from tt-a1i August 28, 2026 07:42

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested for one acceptance-evidence gap. The implementation and Windows success path look sound: the focused suites pass, bun run check passes, and Node 22/24 CI is green. However, Issue #211 explicitly requires the stronger Windows query to remain conservative when PowerShell is unavailable, restricted, errors, or times out. The current test covers command construction and output parsing only, so the fail-closed execution path is not protected against regression. Please add an injected or mocked executor test that reaches the liveness decision and proves no lock recovery is authorized when the query result is unknown.

]);
assert.match(windowsQuery.args.at(-1) ?? "", /Get-Process -Id 123/);
assert.equal(windowsQuery.parseOutput("1700000000000"), 1_700_000_000_000);
assert.equal(windowsQuery.parseOutput("not a timestamp"), undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test stops at the query descriptor and parser. It never exercises execFile failure or timeout, nor verifies the required undefined → processLiveness("unknown") → do not recover the lock chain. Because this new branch can authorize stale-lock deletion when it proves a reused PID, please inject/mock the executor and add error/timeout coverage for the fail-closed path required by Issue #211.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed the requested acceptance-evidence gap in 40fa311.

I added a focused test that starts a fresh Node process, mocks node:child_process.execFile before importing setup- config.ts, and verifies the full fail-closed path:

execFile failure → process start query returns undefined → liveness remains unknown → updateSetupConfig times
out without running the mutator or modifying the lock.

Re-ran locally on Windows:

  • node --test --experimental-strip-types tests/extensions/shared/setup-config.test.ts — 20/20 passed
  • node --test --experimental-strip-types tests/extensions/capabilities/index.test.ts — 15/15 passed
  • bun run lint — passed
  • bun run typecheck — passed, with existing Effect diagnostics

@qwertvgty
qwertvgty force-pushed the fix-windows-test-portability branch from 3f15789 to 40fa311 Compare August 28, 2026 08:23
@qwertvgty
qwertvgty requested a review from tt-a1i August 28, 2026 08:54

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

解决什么问题:修复 Windows 上 setup config 锁存活判断无法识别 PID 复用,以及 capabilities 测试硬编码 POSIX 路径分隔符的问题。

价值是什么:Windows contributor 可以信任 focused setup/capabilities 测试;更重要的是,锁恢复在 Windows 上也遵守 PID + process start time 的安全判据,查询不确定时继续 fail closed。

方法是什么:win32 通过 powershell.exe 查询 Get-Process StartTime 并转换为 Unix 毫秒;POSIX 保留 ps;失败、超时、工具不可用或输出不可解析统一返回 unknown。路径断言同时接受 /、\ 和 JSON 转义形式。

Review 结论:Changes Requested。实现本身和 Spec 均通过,上一轮要求的 unknown-liveness 回归已补齐;当前阻塞是分支历史。

Standards:1 个 P1。当前分支围绕一个实质提交包含 4 个 merge-main 提交(f2f915c、0f1460a、bc71945、9f6e3fa),违反 docs/contributing/linear-git-history.md 的“rebase onto latest main”规范。请把实质改动 rebase 到当前 main,并按文档用显式 expected-SHA lease 更新远端。另有一个 P3 非阻塞重复代码建议,见 inline comment。

Spec:0 finding。PowerShell query 的 error/timeout/unavailable/unparsable 路径均保守落到 unknown;测试已到达 liveness 决策并证明 mutator 不运行、锁不被修改、最终超时。

本地验证(head 9f6e3fa):

  • focused setup/capabilities:35 passed,0 failed
  • bun run check:通过
  • bun run test:Node 960 passed、1 个平台预期 skip;Vitest 30 passed
  • git diff --check:通过

远端:当前 head 尚无 GitHub checks。

process.stdout.write("preserved\\n");
`;

const scenarioAgentDir = mkdtempSync(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P3] 这里的 spawn/env/stdout/stderr/exit/cleanup harness 与下方现有 scenario harness(约第 555 行)大段重复。建议提取一个小的 runSetupConfigChildScenario helper,接收 source、额外 env 和 failure label,避免两套 Windows 清理与错误处理以后漂移。该项不阻塞本次修复。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

已按建议完成修改:

  1. 按 docs/contributing/linear-git-history.md 将分支 rebase 到最新 origin/main,移除了分支中的 merge-main 提交;当前仅保留一个实质提交。
  2. 处理了 P3:提取 runSetupConfigChildScenario helper,统一子进程启动、额外环境变量、stdout/stderr 收集、退出检查及临时目录清理,避免两套 scenario harness 后续漂移。
  3. 已使用显式 expected-SHA lease 更新远端分支。
    当前 head:1c63280
    本地验证:
  • bun run check:通过
  • focused setup/capabilities:35 passed,0 failed
  • git diff --check origin/main...HEAD:通过
  • bun run test 中遇到 Windows background-terminals 基线失败;同一失败可在未包含本 PR 修改的 origin/main 上复现,与本次变更无关。

@qwertvgty
qwertvgty force-pushed the fix-windows-test-portability branch from 9f6e3fa to 1c63280 Compare August 28, 2026 21:09
@qwertvgty
qwertvgty requested a review from tt-a1i August 29, 2026 01:58
@qwertvgty
qwertvgty force-pushed the fix-windows-test-portability branch 2 times, most recently from fe6f5bf to f1dd49c Compare August 29, 2026 13:36
@yxr-2025

Copy link
Copy Markdown
Contributor

这个问题阻塞了我的两个 PR,希望尽快解决😣

@tt-a1i
tt-a1i force-pushed the fix-windows-test-portability branch from f1dd49c to f49f358 Compare August 29, 2026 16:38
@github-actions github-actions Bot added the area:setup OpenPI setup, configuration, or setup documentation label Aug 29, 2026

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

复核通过,基于精确 head f49f358 和 current main 62a4a9a。解决的问题:Windows setup config 锁恢复现在可以用 PID + process start time 区分 PID 复用,capabilities 测试也不再硬编码 POSIX 路径。价值:Windows 上的锁恢复保持与 POSIX 相同的安全判据,并在 PowerShell 不可用、报错、超时或输出不可解析时继续 fail closed。方法:win32 使用 powershell.exe/Get-Process 查询 Unix 毫秒,POSIX 保留 ps;锁协议未改变。Standards:0 finding,分支已线性 rebase 到最新 main,range-diff patch-equivalent。Spec:0 finding,unknown-liveness 回归证明 mutator 不运行、锁不修改并最终超时。验证:merge-state focused 35/35、bun run check、完整 bun run test、diff check 均通过;新 head Node 22、Node 24、Windows、labeler CI 全绿。

@tt-a1i
tt-a1i merged commit 865f66e into openpi-dev:main Aug 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:setup OpenPI setup, configuration, or setup documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows 上测试套件不绿:setup-config 死锁回收从未在 win32 验证 + capabilities 测试硬编码正斜杠路径

3 participants