Skip to content

test(windows): retry one transient CIM enumeration timeout - #801

Merged
Wibias merged 3 commits into
lidge-jun:devfrom
luvs01:agent/windows-powershell-enumeration-retry
Jul 31, 2026
Merged

test(windows): retry one transient CIM enumeration timeout#801
Wibias merged 3 commits into
lidge-jun:devfrom
luvs01:agent/windows-powershell-enumeration-retry

Conversation

@luvs01

@luvs01 luvs01 commented Jul 31, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Retry the Windows real-PowerShell process-enumeration probe once when the first CIM enumeration returns no matching process.
  • Replace the test-only Atomics.wait settle with Bun.sleepSync and keep the probe alive long enough for two bounded 12-second enumerations.
  • Leave the production listWindowsSnapshots() timeout and fail-closed empty-result behavior unchanged.

Root cause

The Windows job on PR #750 failed this integration test after 12.70 seconds, which matches the production enumerator's 12-second execFileSync deadline. The catch path correctly returned an empty snapshot list, but the test treated one transient PowerShell/CIM stall as a product regression.

The exact PR base and current dev both passed the same test in about 0.85 seconds, so extending the production timeout would weaken a deliberate safety bound without addressing the test flake. This change instead permits one test-only retry and still fails after two consecutive stalls or any real owner/command-line matching regression.

Evidence:

Verification

  • Bun 1.3.14: tests/codex-app-server-processes.test.ts --rerun-each 10 — 180 passed, 0 failed.
  • Bun 1.4.0-canary.1: the same repeated run — 180 passed, 0 failed.
  • bun run typecheck — passed.
  • bun run privacy:scan — passed.
  • git diff --check — passed.
  • Full repository test suite — inconclusive locally: the Windows run exceeded the 15-minute harness limit without returning a test result; its owned Bun process tree exited after the timeout. Cross-platform CI remains the authoritative full-suite check.

CI follow-up

The changed Windows process-enumeration test passed in the initial CI. Two unrelated 5-second integration probes timed out elsewhere in that full-suite run:

The source branch does not touch either test. Current head 1b532c4e is rebased onto current dev (81f3f689), which triggered a fresh cross-platform run. Following the maintainer's latest direction, this PR is ready for review while the new checks complete.

Summary by CodeRabbit

  • Tests
    • Improved Windows integration test reliability by allowing more time for process startup and adding a retry when the child process is not immediately detected.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2eb46a29-b9c4-4d2f-8854-eca780700dbb

📥 Commits

Reviewing files that changed from the base of the PR and between 81f3f68 and 1b532c4.

📒 Files selected for processing (1)
  • tests/codex-app-server-processes.test.ts
📝 Walkthrough

Walkthrough

The Windows integration probe now runs longer, uses Bun.sleepSync, retries process enumeration after a 250 ms delay, and allows 35 seconds for the test.

Changes

Windows probe timing

Layer / File(s) Summary
Probe retry and timeout handling
tests/codex-app-server-processes.test.ts:362-378, tests/codex-app-server-processes.test.ts:391		 The probe lifetime increases from 25 to 45 seconds. The test replaces Atomics.wait with Bun.sleepSync, retries listWindowsSnapshots() once after 250 ms when the child is absent, and increases the timeout from 30 to 35 seconds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding one retry for a transient Windows CIM enumeration timeout in the integration test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Jul 31, 2026
lidge-jun added a commit that referenced this pull request Jul 31, 2026
@lidge-jun
test: stop two suites from waiting on the Windows runner
a18acd6 
Three CI runs failed only on windows-latest, each on a different test,
all passing locally. Two of them share a cause: the test waits on
something the runner controls, and Bun's 5s budget expires first.

Sidebar routes spawned the user's real gh. 0af17fb already fixed the
Windows .cmd shim resolution, and the tests still timed out after it,
because resolving the binary correctly does not make an external
process fast on a loaded runner. star-state.ts already had a StarDeps
seam with an injectable runGh; setStarDepsForTests() selects it. The
route tests now assert what they actually claim — reachability, shape,
and that no gh output, token, or account identifier is ever serialized
— against a deterministic fake, in 0.25ms instead of 5s. Production
keeps the real runner.

Server auth built four proxy/upstream harnesses inside one test. On
Windows the startup cost alone exhausted the budget, and the last
request was still in flight when the next test began, racing it through
global fetch. One harness now serves all four malformed-detail cases;
each still proves its exact original 400 and a single acct-pool-a
dispatch.

No timeout was raised, no assertion deleted, no test skipped. Those
would have traded a reliability signal for silence.

The tray failure is NOT fixed. It launches PowerShell, then Bun, then
rebinds the same port, and proving the child does not inherit the listen
socket is the whole point of the test. Making it deterministic needs a
process-launch seam in src/tray/windows.ts, and faking it only in the
test would delete the proof. Left alone deliberately; PR #801 targets a
neighbouring Windows enumeration flake.
@Wibias Wibias mentioned this pull request Jul 31, 2026
Merged
@luvs01
luvs01 force-pushed the agent/windows-powershell-enumeration-retry branch from 8bd15c4 to 1b532c4 Compare July 31, 2026 08:55
@luvs01
luvs01 marked this pull request as ready for review July 31, 2026 08:55
@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

[shipping-github] Verdict: approve-comment

PR: #801 — test(windows): retry one transient CIM enumeration timeout
Head: 84601d62 on dev (mergeStateStatus: CLEAN)
Linked: none

Usefulness

Real CI flake fix. Windows job on PR #750 failed listWindowsSnapshots after ~12.7s (production execFileSync deadline), treating one transient PowerShell/CIM stall as a product regression. Exact-base and then-current dev both passed the same probe in ~0.85s. This PR keeps production behavior unchanged and hardens only the integration test.

Bugs / correctness

  • Checked: tests/codex-app-server-processes.test.ts vs src/codex/app-server-processes.ts (timeout: 12_000, GetOwner fail-closed, current-user scoping)
  • Findings: none blocking
  • Own bugbot: no bugs
  • Local: bun test tests/codex-app-server-processes.test.ts — 18 pass (incl. real PowerShell enumeration ~1.1s)
  • CI Windows log on tip: same probe pass at 875.57ms

Security

  • Scope reviewed: production listWindowsSnapshots() owner/timeout/fail-closed path vs test-only retry
  • Findings: none medium+
  • Production 12s timeout, GetOwner ReturnValue fail-closed, and current-user filter unchanged; retry lives only in the test

Spec / standards

  • Spec source: PR body (no linked issue / PRD)
  • Gaps: none — test-only retry + longer-lived probe child; production timeout untouched; matches claimed root cause
  • Standards: scoped to one test file; no secret logging; aligns with AGENTS.md regression-test practice

Reviews

  • Owners/maintainers: none open
  • Bots (CodeRabbit/Codex/Bugbot): CodeRabbit reported no actionable comments; 0 unresolved review threads

Base / CI

  • Behind/conflicts: synced onto upstream dev (84601d62); mergeable / CLEAN
  • Required checks: green on 84601d62 — ubuntu-latest, windows-latest, macos-latest, npm-global matrix, enforce-target, react-doctor, label, CodeRabbit
  • Flake note: one Windows GUI-suite mass failure on intermediate SHA bf5bd345 (133 unrelated GUI fails; this PR’s CIM probe still passed); --failed rerun recovered; tip SHA green first-try after resync that also picked up upstream Windows budget hardening
  • Local tip compile/tests: focused suite green on tip

Gate

none

Bottom line

Ship it. Narrow, evidence-backed test reliability fix that does not weaken production fail-closed enumeration. Ready for merge when you want it.

@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

[shipping-github] Merge ready

PR: #801 — test(windows): retry one transient CIM enumeration timeout
Head: 84601d62 → dev (mergeStateStatus: CLEAN)
Linked issues: none

Reviews

  • Humans (owners first): none open
  • Bots: 0 unresolved useful threads; CodeRabbit had no actionable findings
  • Own bug + security + spec/standards: done on tip; blockers fixed: none

Tip freshness

  • Updated from dev: yes (84601d62 merge commits onto upstream tip)
  • Compiles/tests against tip: bun test tests/codex-app-server-processes.test.ts — 18 pass
  • Conflicts: none

Checks

  • Local/CLI: focused process-enumeration suite green
  • Required CI: green on 84601d62 (flake retries: 1 — Windows GUI mass-fail on bf5bd345, unrelated; rerun recovered; tip green)
  • Policy: code-owner enforcement / stale approvals / merge-queue: clear / n/a

Residual

none / out of scope: broader Windows GUI / Bun stability work tracked on dev, not this PR

Ready to merge.

@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Thanks @luvs01 — merging this.

Why it helps: Windows CI was failing listWindowsSnapshots when one PowerShell/CIM enumeration hit the production 12s deadline under load, even though the product fail-closed path was correct. The change in tests/codex-app-server-processes.test.ts retries once in the test only and keeps the probe alive long enough for two bounded enumerations, without weakening production timeout or owner filtering.

Ship it.

@Wibias
Wibias merged commit 33caf33 into lidge-jun:dev Jul 31, 2026
10 checks passed
Wibias added a commit to luvs01/opencodex that referenced this pull request Jul 31, 2026
@Wibias
merge(dev): refresh pool-plan branch onto tip after lidge-jun#801
7b22a8b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidge-jun lidge-jun Awaiting requested review from lidge-jun lidge-jun is a code owner
@Ingwannu Ingwannu Awaiting requested review from Ingwannu Ingwannu is a code owner
@Wibias Wibias Awaiting requested review from Wibias Wibias is a code owner

Assignees

No one assigned

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@luvs01 @Wibias