Repository navigation
Conversation
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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughOn 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 ChangesOwnership-Lease Diagnostics
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
|
macOS CI diagnosis for unchanged head 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.
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 Both attempts were last logging 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. |
|
@coderabbitai review Please perform one bounded source review of unchanged head 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. |
✅ Action performedReview finished.
|
|
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.
|
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 Carry head: |
Summary
nullbefore launching a process for this label.pslookup, recorded PID, liveness, lease age, busy error metadata, acquisition, reclamation, delegation and release behavior.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, thendevc30b5228d0bc5302e4033f0533babe100dacd751.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
3320395db4abefe6c18846c66523b71bef83d6e2, whose tree is identical to the published tree above. Its log explicitly passesWindows busy-lease diagnostics never search for an executableand completes all 111 primary batches.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 changingprocess.platform; this is not native Windows execution.Command, from the isolated integration worktree:
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.exelabel instead of null; realtasklistwas not launched. An unscoped staged merge whitespace check also reported an existing dev blank line at EOF indevlog/_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
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.aa913267with no actionable comments. It is not represented as a new review of the integration commit.Checklist
Summary by CodeRabbit