Skip to content

fix(review): avoid repeated ancestor ACL checks during owner sweep - #2595

Closed
aiorellana502 wants to merge 1 commit into
Gentleman-Programming:mainfrom
aiorellana502:fix/2269-windows-owner-sweep
Closed

aiorellana502 wants to merge 1 commit into
Gentleman-Programming:mainfrom
aiorellana502:fix/2269-windows-owner-sweep

Conversation

@aiorellana502

Copy link
Copy Markdown
Contributor

Linked issue

Fixes #2269

PR type

  • Bug fix
  • Performance

Summary

readOwner re-ran assertCandidateOwnerParent for every ancestor on every marker during the startup orphan sweep, even though the same ancestor chain is shared by every marker in a single sweep pass. On Windows this meant ~31 powershell/icacls subprocess spawns, adding roughly 60s to startup.

Changes

  • lib/review-candidate-view-owner.ts: the sweep now validates each ancestor replacement boundary once per sweep pass instead of once per marker, while still validating every owner marker individually and still revalidating boundaries during destructive cleanup. The reuse is sweep-local only — no process-lifetime cache, no change to the per-marker trust decision.
  • tests/review-candidate-view.test.ts: two new regression tests — one confirming ancestors are validated once while every marker is still checked, one confirming rejected boundaries and owner markers are still preserved correctly after the reuse.

Test plan

  • Focused: the two new tests pass (node --experimental-strip-types --test --test-name-pattern="Windows orphan sweep" tests/review-candidate-view.test.ts) — 2/2.
  • Full tests/review-candidate-view.test.ts: not clean, independent of this change. The file has pre-existing failures/slowness from real Windows ACL (icacls/whoami) subprocess tests unrelated to this fix; three of those were confirmed to reproduce identically on upstream/main with no source changes, the rest were not individually classified. Posted on the issue before starting (Startup takes ~60 s on Windows: readOwner re-runs assertCandidateOwnerParent per owner, spawning ~31 powershell/icacls probes #2269 (comment)) and repeating that disclosure here rather than claiming full-suite verification.
  • Typecheck: 0 diagnostics.
  • End-to-end startup time improvement: not measured in this PR; the isolated real-ACL measurement on the issue showed owner checks dropping from 7 to 4 and subprocesses from 20 to 12 for one marker, with the marker preserved.

AI Assistance

  • Material assistance used — Pi / el Gentleman with subagents; investigation, implementation and tests.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · 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.

@Alan-TheGentleman

Copy link
Copy Markdown
Collaborator

Thanks for the work on this PR. After reviewing the open backlog, we're closing it because the change is already covered on main or by another PR (see #2343). If something is still missing on v4.5.1, please open an issue or comment here with the details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Startup takes ~60 s on Windows: readOwner re-runs assertCandidateOwnerParent per owner, spawning ~31 powershell/icacls probes

2 participants