Skip to content

fix(windows): avoid execute access when renaming files - #44601

Open
steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/windows-rename-no-execute
Open

steipete wants to merge 1 commit into
oven-sh:mainfrom
steipete:claude/windows-rename-no-execute

Conversation

@steipete

@steipete steipete commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Windows internal renames open the source with FILE_TRAVERSE. For a regular file, that bit is FILE_EXECUTE. Reopening freshly written compile-cache entries with it makes Defender scan them synchronously before granting access: a native trace recorded 2,001 OnOpen scans totaling 23.703 seconds during a 28.093-second flush.

Use the existing non-executable fallback rights for the first and only open. Keep SYNCHRONIZE | GENERIC_WRITE | DELETE, the sharing defaults, and the open/rename options. Delete the redundant retry. Remaining FILE_TRAVERSE call sites explicitly open directories.

Validation:

  • Interleaved Windows API controls over 2,000 unique cache-like files: reopen calls took 24.532/24.509 seconds with execute access and 0.119/0.116 seconds without it. Defender stayed enabled.
  • Actual cold-cache flush: 29.545 seconds before and 3.821 seconds after, with all 2,001 entries published. The original module regression and surrounding filesystem checks pass with the change.
  • Windows runtime measurements use openclaw/bun#130, whose entire changed Windows helper file is byte-identical to this port. That fork also passed all 12 Rust target checks and both native CI lanes; this upstream commit passes cargo check -p bun_sys for x86_64-pc-windows-msvc and aarch64-pc-windows-msvc.
  • Independent review through P2 is clean. No cache policy or test deadline changes.
Standalone explicit-flush reproduction

Run this script with a current Bun executable on Windows with Defender enabled. The optional argument selects the child executable; fixture creation is outside the measured flush. The same script was also verified with Node 24.21.0 on macOS.

import { spawnSync } from "node:child_process";
import fs from "node:fs";
import os from "node:os";
import path from "node:path";

const binary = process.argv[2] ?? process.execPath;
const root = fs.mkdtempSync(path.join(os.tmpdir(), "compile-cache-repro-"));
const count = 2_000;
const functions = Array.from({ length: 64 }, (_, i) => `function f${i}(x) { return x + ${i}; }`).join("\n");
try {
  for (let i = 0; i < count; i++) {
    fs.writeFileSync(path.join(root, `mod${i}.cjs`), `${functions}\nmodule.exports = f63(${i});`);
  }
  fs.writeFileSync(path.join(root, "main.cjs"), `
    const fs = require("node:fs");
    const moduleApi = require("node:module");
    const loadStarted = performance.now();
    for (let i = 0; i < ${count}; i++) require("./mod" + i + ".cjs");
    const loadMs = performance.now() - loadStarted;
    const started = performance.now();
    const cpu = process.cpuUsage();
    moduleApi.flushCompileCache();
    console.log(JSON.stringify({
      revision: globalThis.Bun?.revision ?? process.version,
      platform: process.platform,
      loadMs,
      flushMs: performance.now() - started,
      cpu: process.cpuUsage(cpu),
      entries: fs.readdirSync(moduleApi.getCompileCacheDir()).length,
    }));
  `);
  const env = { ...process.env, NODE_COMPILE_CACHE: path.join(root, "cache"), NODE_DEBUG_NATIVE: "", BUN_ENABLE_CRASH_REPORTING: "0", BUN_RUNTIME_TRANSPILER_CACHE_PATH: "0" };
  delete env.NODE_DISABLE_COMPILE_CACHE;
  const child = spawnSync(binary, [path.join(root, "main.cjs")], { env, encoding: "utf8" });
  process.stdout.write(child.stdout ?? "");
  process.stderr.write(child.stderr ?? "");
  if (child.error) throw child.error;
  if (child.status !== 0) throw new Error(`Child exited with ${child.status}, signal ${child.signal}`);
} finally {
  fs.rmSync(root, { recursive: true, force: true });
}

FILE_TRAVERSE aliases FILE_EXECUTE for regular files. Requesting it while reopening freshly written cache entries forces synchronous antivirus scans before the rename.

Use the existing fallback rights directly, preserving synchronization, write and delete access as well as the open and rename options. Remove the redundant retry.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: b3d0b771-c656-446e-b6eb-45ee461f600d
📥 Commits

Reviewing files that changed from the base of the PR and between 13a98b0 and bbee63a.

📒 Files selected for processing (1)
  • src/sys/windows/mod.rs

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


Walkthrough

rename_at_w now opens the source with SYNCHRONIZE | GENERIC_WRITE | DELETE in one attempt. If the open fails, it returns the error.

Changes

Windows rename

Layer / File(s) Summary
Open the rename source
src/sys/windows/mod.rs
rename_at_w removes the open attempt with FILE_TRAVERSE and its retry. It opens the source once with `SYNCHRONIZE

Suggested reviewers: dylan-conway

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to bbee6

No merge-blocking issue is identified; the Windows rename change is ready for normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to bbee6

The change requests fewer permissions using a fallback already supported by the previous implementation. Rename permissions, handle-based file identity, and cleanup remain intact. No material security regression was identified in the reviewed path.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected authority remains the Windows filesystem paths and directory handles supplied by existing rename callers, subject to native access checks. The change neither adds a caller nor introduces a new credential or privilege mechanism in the inspected path.

Security Findings and Attack Paths

  • inferred — The removed execute-access request was not a mandatory security gate: the base already retried every initial open error using the head's reduced rights. This counterevidence does not support an introduced permission-bypass attack path.

Trust Boundaries and Controls

  • observed — Native source-open authorization, relative-directory resolution, and handle-based rename authority remain in place. FILE_OPEN_REPARSE_POINT is preserved; this is not a claim that all path components are sandboxed or that absolute paths are prohibited.

Resilience and Maintainability Implications

  • inferred — The change preserves source-handle ownership and cleanup on normal return and unwinding, including rename failure. Repetition and concurrency use the same single handle-based rename operation as before; no new partially committed multi-step transition is introduced. Process termination and adversarial filesystem races were not runtime-verified.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: avoiding execute access when renaming files on Windows.
Description check ✅ Passed The description explains the change and its rationale, and provides detailed validation results. It covers both required template sections, although it uses “Validation” instead of the exact heading “…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

steipete added a commit to openclaw/bun that referenced this pull request Oct 5, 2026
Cold Node compile-cache persistence on Windows can exceed the existing 30-second regression deadlines while the idle loop keeps waking and cache entries keep appearing. The Windows rename helper reopened every temporary file with `FILE_TRAVERSE`, which is [`FILE_EXECUTE` for a regular file](https://learn.microsoft.com/en-us/windows/win32/fileio/file-access-rights-constants). Microsoft Defender synchronously scanned these files before granting that unnecessary access.

Use the existing non-executable fallback access mask for the first and only open, removing the redundant retry. Keep synchronization, write, and delete rights, sharing defaults, and atomic rename. The compile-cache workload, test deadlines, one-second/90% idle policy, and 250 ms signal-exit budget are unchanged. The watchdog only gains entry-count and child-exit diagnostics. The CI notes distinguish this Windows timeout from the Linux idle-accounting bug already fixed by #53.

The original Windows binary at `d5ada8e6d0c62a51ead1eaccee40cc8388ad3ede` fails the unchanged idle test in five of five isolated runs. Independent observations show continuous progress and completion around 32 seconds. Interleaved Windows API controls over 2,000 unique files spend 24.532/24.509 seconds reopening with execute access, versus 0.119/0.116 seconds without it. Defender's trace attributes 2,001 `OnOpen` scans to cache temporary files, totaling 23.703 seconds during a 28.093-second flush. Defender remained enabled throughout.

With the fix at `918fe702a958256698b429e6c0f4940b1f5d4aef`, three same-host pairs turn unchanged 30.089/30.085/30.085-second timeout failures into 10.714/10.172/10.250-second passes. The standalone explicit-flush reproduction drops from 29.545 seconds to 3.821 seconds while retaining all 2,001 entries. The full Windows module and filesystem suites pass. The filesystem run uses the repository runner's existing 90-second per-test policy; no configured deadline was changed.

Final head: `d708b0f50d8e4dfde3d56f8ddde6719c1830243a`.

- [Native CI](https://github.com/openclaw/bun/actions/runs/37377495588): Linux 15/15 and Darwin 12/12, including the full module file. The tested merge tree exactly matches the PR head.
- [Windows qualification](https://github.com/openclaw/bun/actions/runs/37377645374): x64 and ARM64 each pass 31/31 compatibility rows, both native smoke tests, and manifest assembly. Frozen workflow `5b3999cd3dbc0ff3d4b32c52865674e6b46bbbd2` builds/tests the exact source above. Its Windows jobs and test policy are unchanged; Linux scheduling uses existing Blacksmith runners, and signing/publication are unreachable. Native smoke verifies the full source revision and engine. Manifest/archive/executable/profile hashes and both PE architectures are independently verified.
- Local macOS: 953 passing module/filesystem/filesystem-promises tests, zero failures. All 12 Rust targets pass without skips.
- Isolated OpenClaw consumer checks: infrastructure compile-cache 7/7 and entry compile-cache 22/22, before and after the exact final binary.
- Format, source/JavaScript lint, Clippy, Miri, and cargo tests pass. P2 landing review with the unchanged temporary-file and compile-cache callers is scoped-clean.

The final native and Windows runtime qualification required no test retries or exceptions. Mordant's explicitly advisory resolver style warning is unchanged from main; no suppression was added.

The same helper exists upstream. The source-only port is open as [oven-sh#44601](oven-sh#44601), with passing Windows x64/ARM64 crate checks and source/Rust/format checks.
steipete added a commit to openclaw/bun that referenced this pull request Oct 6, 2026
…133)

The test runner can kill an unlimited test’s live child when a completed test’s stale deadline fires. Its process-reaping check treats the no-deadline/EPOCH timestamp as expired. Exclude that sentinel, matching the existing entry timeout check, and cover the completed-deadline/live-child sequence with a deterministic regression.

The deferred compile-cache idle test now preserves its ten-second persistence-progress guard and bounds module loading, pipe flushes, child exit, and cleanup individually. The complete 2,000-module workload, late-module handshake, 2,002-file assertion, idle-generation window, and signal-exit budget remain unchanged.

The exact Windows binary from [run 37386790583](https://github.com/openclaw/bun/actions/runs/37386790583) reproduces the old 30-second aggregate timeout 3/3 under a controlled CPU quota while files keep appearing. An independent driver completes all 2,002 entries in 64.7 seconds; the proposed test passes under that quota in 52.6/52.2 seconds. Deliberately stalled persistence, startup, and exit still fail at their ten-second guards. That hosted failure retained no progress timeline, so its specific runner-level trigger remains unknown. This is distinct from #53’s Linux idle-accounting fix and #130’s Windows execute-access scan fix.

The child-lifetime regression fails 3/3 on old Mac and Windows x64 fork binaries and official upstream Bun `13a98b0d`. Exact-source Windows pairs fail 3/3 before and pass 3/3 with the owner fix. Two stale hook-error column expectations were corrected from 15 to 11 after the pre-change fork reproduced all five failures and Node 24 confirmed column 11; exact source-position and error-attribution checks remain intact.

Final validation applies to `29cbbbf595` on main `10b749212a`:

- P2 Codex autoreview and the final pre-merge review are scoped-clean.
- Local release build passes, with 25 test-runner tests and 214 module tests passing; all 12 Rust targets pass.
- [Native CI](https://github.com/openclaw/bun/actions/runs/37402121441): Linux 16/16 and Darwin 13/13, first attempt. Its merge tree exactly matches the reviewed candidate.
- [Windows CI](https://github.com/openclaw/bun/actions/runs/37402131975): x64 and ARM64 each 31/31, both native smoke jobs and manifest pass on the first attempt. The immutable workflow is separate from runtime source; native revisions, WebKit identity, archive/executable/profile hashes, and PE architectures are verified for this exact source commit.
- OpenClaw consumer tests pass 7 + 22 before and after this exact commit, with the before binary’s tree matching current main and isolated HOME/state/configuration/temp directories.

Format, JavaScript/source lint, Clippy, Miri, and lol-html tests pass. Mordant reports the existing `bare_bool_args` finding in `src/resolver/package_json.rs`; its source, baseline, configuration, toolchain, and explicitly advisory workflow setting are unchanged from main. The initial issue-finder run lacked authentication; no credentials or CI settings were changed.

The shared owner fix is also proposed in oven-sh#44613 with a passing upstream-specific build and full test-runner suite. The separate Windows rename fix remains in oven-sh#44601.

This branch has not been deployed

No deployments
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.

1 participant