Skip to content

fix(service): avoid Windows executable lookup in lease diagnostics - #6655

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/windows-lease-diagnostic-upstream-20261006
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:codex/windows-lease-diagnostic-upstream-20261006

Conversation

@luvs01

@luvs01 luvs01 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Omit the optional executable image name from Windows ownership-mutation lease diagnostics. The shared module runs in both Bun and the Node npm/pnpm launcher; it now returns null before launching a process for this label.
  • Retain the POSIX ps lookup, recorded PID, liveness, lease age, busy error metadata, acquisition, reclamation, delegation and release behavior.
  • Add regression coverage for Windows inspection, status and busy acquisition, and update the declaration, service/runtime contracts and English/Korean lifecycle documentation.

This change hardens the local executable lookup used for the optional Windows diagnostic label.

Verification

Current integration — 2026-10-07

Published HEAD: 1d11485adab1ffcce2db954297ff6a701d1fdb5f.
Tree: ebb986cffdd29b45c79c94aaf73a3d358a39e73c.
Ordered parents: prior PR HEAD aa913267d8ac9a91645a90707bbc7188f30f8957, then dev c30b5228d0bc5302e4033f0533babe100dacd751.

This normal, non-force merge incorporates current dev. The PR remains seven files, +39/−11. The diagnostic module, declaration, regression test and Korean lifecycle page are byte-identical to the prior PR head; the owning/English documents retain the incoming dev additions. The module SHA-256 remains ea74e7f325066bf579f4622c348899063123b1509c0a1f09e8d12953b34cb3ba. Workflow, runner scripts, package and lock files match integrated dev; no extra timeout, assertion or workflow change was authored in this refresh.

Current-head hosted result

  • Cross-platform CI 37576849423 completed success: 32 successful jobs, four skipped, none failed or cancelled.
  • All four Linux shards, all nine Windows shards, both macOS shards, gates/typecheck, structure, documentation, storage/API, Docker, selected keyring/npm-global jobs, macOS widget/bundle and desktop shell passed.
  • Skips were the separate macOS full-membership control, setup-action matrix, remote-helper matrix and separate privacy-gate job. The privacy scan ran successfully under gates; skipped jobs are not executed coverage.
  • Service lifecycle 37576849510 passed on all three selected OSes. React Doctor 37576849422 passed.
  • Actual macOS 1/2 job checkout is merge ref 3320395db4abefe6c18846c66523b71bef83d6e2, whose tree is identical to the published tree above. Its log explicitly passes Windows busy-lease diagnostics never search for an executable and completes all 111 primary batches.
  • Both macOS shards completed under the existing 20-minute job limit. This closes the new head's incomplete-CI gate; it does not establish the root cause of the older cancellations or prove that this refresh alone caused the timing improvement.

Current integration's focused local evidence

Node v24.19.0 / Linux passed 64 assertions using the actual shared module and a real child process holding a private temporary lease. Liveness was not stubbed. The harness covered inspection/status/busy refusal, owner-file identity and byte preservation, seven instrumented child-process APIs with zero Windows diagnostic calls, actual POSIX ps, explicit image injection, nested acquisition/release, dead-holder reclamation and successor-owner preservation. The Windows branch was simulated by changing process.platform; this is not native Windows execution.

Command, from the isolated integration worktree:

timeout 30s node /workspace/scratch/lease705/focused-lease-check.mjs src/service/ownership-mutation-lease.mjs --simulate-win32
node --check src/service/ownership-mutation-lease.mjs
git diff --cached c30b5228d0bc5302e4033f0533babe100dacd751 --check
git diff c30b5228d0bc5302e4033f0533babe100dacd751 HEAD --check
bash -n scripts/ci/run-bun-test-batches.sh scripts/ci/windows-sensitive-diff.sh
node /workspace/scratch/lease705/refresh-20261007/node-file-size-check.mjs
git ls-files -u

These checks passed; the last command found no unresolved entries. The file-size check executed the repository's exported ratchet logic through Node: 11,600 tracked files, 5,653 inspected, 51 baselines, zero violations. It is not a Bun-suite result. Additional index/source checks were recorded as passing, but their full inline command was not preserved, so no reconstructed command is presented as executed evidence.

The original implementation's negative control failed as expected when the simulated Windows diagnostic returned the intercepted fixture-tasklist.exe label instead of null; real tasklist was not launched. An unscoped staged merge whitespace check also reported an existing dev blank line at EOF in devlog/_plan/261006_lane_c_provider_compat/000_roadmap.md:16; that upstream file was preserved. The dev-relative seven-file checks above passed.

Bun, tsc and repository node_modules were unavailable locally, so the full local Bun suite, test:changed, typecheck and docs build were not run. No dependency installation or previously denied registry retry was attempted. The focused validation scope exception leaves broader execution to the completed current-head hosted checks above.

Historical evidence and review hold

  • The earlier native Windows 10.0.26300.0 / Node v24.16.0 run passed 15 assertions on the same module SHA-256, covering a real live PID, inspection/status/busy refusal, owner-file preservation and zero diagnostic subprocess calls. It remains a separate historical native-Node result.
  • The previous HEAD aa913267's Cross-platform CI 37410307004 was cancelled on both attempts. The two-attempt diagnosis remains a record of time-budget exhaustion with an unproven CPU/I/O/Bun cause. Those old cancellations are not relabeled as successful and no third retry of that head was requested.
  • CodeRabbit's completed source review covers the prior seven-file head aa913267 with no actionable comments. It is not represented as a new review of the integration commit.
  • Maintainer editing is enabled, resolving the edit-access obstacle mentioned in the maintainer sweep note. Draft and the explicit maintainer security-review hold remain. Passing CI is not security approval or merge authorization.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs and declaration describe omission of the optional Windows image label.
  • Security-sensitive changes received the required upstream review.

Summary by CodeRabbit

  • Bug Fixes
    • On Windows, busy-lease diagnostics no longer launch a separate process to identify the executable. They continue to report the recorded PID, whether it is alive, lease age, and recovery guidance; the executable name is shown only when available.
    • Windows lease acquisition continues to reject an occupied lease immediately.

Omit the optional Windows image label in the shared Node/Bun lease helper.
Preserve POSIX lookup and lease ownership behavior, with regression coverage
and matching declaration and lifecycle documentation.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 6becdb41-1069-4752-b976-096ded7b50f1
📥 Commits

Reviewing files that changed from the base of the PR and between f924820 and aa91326.

📒 Files selected for processing (7)
  • docs-site/src/content/docs/ko/reference/cli/lifecycle.md
  • docs-site/src/content/docs/reference/cli/lifecycle.md
  • src/service/ownership-mutation-lease.d.mts
  • src/service/ownership-mutation-lease.mjs
  • structure/ops/service-and-sidecars.md
  • structure/runtime.md
  • tests/service/service-ownership-state.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

On Windows, ownership-lease diagnostics no longer launch a process to look up an executable name. They retain PID, liveness, lease-age, and recovery information. POSIX systems continue to use ps for the optional process image.

Changes

Ownership-Lease Diagnostics

Layer / File(s) Summary
Windows process-image behavior
src/service/ownership-mutation-lease.mjs, src/service/ownership-mutation-lease.d.mts
On Windows, processImage(pid) returns null without running tasklist. Other platforms continue to use ps.
Diagnostic validation and documentation
tests/service/service-ownership-state.test.ts, docs-site/src/content/docs/reference/cli/lifecycle.md, docs-site/src/content/docs/ko/reference/cli/lifecycle.md, structure/ops/service-and-sidecars.md, structure/runtime.md
The Windows test checks that diagnostics retain the recorded PID and liveness, mark the image as unknown, and do not spawn a process during failed acquisition. The documentation describes the omitted Windows image lookup and the remaining diagnostic information.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to aa913

Windows lease diagnostics omit the optional executable name while retaining holder and recovery information, and POSIX lookup remains. No concrete merge-blocking regression is established here, although exact-head cross-platform validation is incomplete.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (4 skipped: 4 …
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: Windows lease diagnostics no longer launch a process to look up the executable name. It is concise and specific.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 6, 2026

luvs01 commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

macOS CI diagnosis for unchanged head aa913267d8ac9a91645a90707bbc7188f30f8957, compared with base f92482065217bae891a88e27fd6c00012f212042.

Both macOS 1/2 attempts completed 102/109 primary batches and were cancelled during batch 103 under the same 20-minute job budget. The workflow, fresh-process runner and batch membership are unchanged from base.

macOS 1/2 job Test-step elapsed Wall time for the same first 102 batches Result
Base 12m42s 693.335s All 109 batches completed
PR attempt 1 19m27s 1124.192s Cancelled in batch 103
PR attempt 2 19m13s 1124.229s Cancelled in batch 103

The base run's macOS job succeeded; its aggregate failed on Windows.

Before the new test ran in batch 93, batches 1–92 already took 676.430s on base versus 1098.641s / 1098.774s on this head. The new Windows busy-lease diagnostics never search for an executable regression passed in 1.16ms / 2.70ms. No assertion failure, batch deadline or Bun crash failure was observed before cancellation. The largest gaps between timestamped test-log lines were 15.190s / 15.146s.

Both attempts were last logging tests/update/update-restart-lease.test.ts. On retry, the last PASS was EOF releases the real lease with acknowledgment and a reaped child at 04:39:15.036852 UTC; cancellation followed at 04:39:20.530474 UTC, 24.879s into batch 103.

This supports budget exhaustion while tests were progressing. It does not establish the CPU/I/O/Bun cause or determine the patch's performance impact, and does not establish the same hang reported in #4956.

The cancelled shard's coverage remains incomplete. This PR stays Draft, and no additional retry is planned.

luvs01 commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Please perform one bounded source review of unchanged head aa913267d8ac9a91645a90707bbc7188f30f8957; Draft currently prevents automatic review.

Current CI state (2026-10-06): Service lifecycle 37410306941 and React Doctor 37410306873 completed successfully. Cross-platform CI 37410307004 is completed with conclusion cancelled, not pending or passing.

The existing two-attempt macOS diagnosis and its attribution limits remain authoritative. No third retry, increased timeout, skipped assertion or workflow change was made. The incomplete macOS coverage remains a readiness blocker.

Please inspect the narrow Windows diagnostic-subprocess removal and preservation of POSIX lookup and lease acquisition/reclamation/release behavior. This source review request does not waive the CI blocker, native coverage limits or required upstream security review. Draft is retained.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer sweep note (lane D, compat hardening sweep): this PR is left for maintainer security review rather than landed in the sweep.

No change was requested here; the PR stays in draft.

Integrate dev at c30b522 while preserving
the focused Windows image omission, POSIX lookup and lease ownership behavior.
@lidge-jun

Copy link
Copy Markdown
Owner

Carried by #6715: #6715

The carry preserves the optional Windows image-label omission and applicable declaration/docs/structure updates, with a sibling regression file covering zero diagnostic subprocesses, busy refusal and owner preservation, stale reclamation, unchanged POSIX lookup, and explicit image injection. Your authorship is preserved as Co-authored-by: Epinephrine <luvs01@hanmail.net>.

Carry head: 1c68a50e5c20ea1e16bf8dc396fe524148ac8264. Focused tests passed 50/0; layout/ratchet checks passed 27/0; typecheck, structure, privacy, and docs build passed. Full local suite was not run. Coordinator platform CI and independent maintainer security review remain pending; the original PR remains open.

lidge-jun added a commit that referenced this pull request Oct 7, 2026
…arry #6655) (#6715)

Carry the optional Windows image-label omission and contract documentation from contributor PR #6655, with focused diagnostic and ownership regression coverage.

Co-authored-by: Epinephrine <luvs01@hanmail.net>
@lidge-jun

Copy link
Copy Markdown
Owner

Carried and merged by #6715 (3bffca3) with Co-authored-by credit. Thank you.

@lidge-jun lidge-jun closed this Oct 7, 2026
@lidge-jun lidge-jun mentioned this pull request Oct 8, 2026
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants