test(skills): synchronize Windows rename contention (#31237)

.github/workflows/pr.yml

@@ -1435,6 +1435,9 @@ jobs:
         run: node config/scripts/build-windows-cli-launcher.mjs
 
       - name: Test Windows-specific boundaries
+        env:
+          ORCA_BACKGROUND_LAUNCH: '1'
+          ORCA_REAL_WINDOWS_SKILL_TEST: '1'
         run: >-
           pnpm exec vitest run --config config/vitest.config.ts
           src/main/zcode/zcode-plan-api-key-store-native-windows.test.ts
@@ -1511,6 +1514,7 @@ jobs:
           src/main/ipc/preflight-provider-command-selection.test.ts
           src/main/ipc/preflight-runnable-local-cli.test.ts
           src/main/managed-server/install/remote-node-runtime-store-windows.test.ts
+          src/main/skills/skill-windows-rename-contention.integration.test.ts
 
       # Why the :parallel variant: identical to build:release except the three
       # electron-vite targets overlap instead of running back to back. The Linux package

config/scripts/pr-package-test-paths.mjs

@@ -79,5 +79,6 @@ export const WINDOWS_PACKAGE_TESTS = [
   'src/main/ipc/pty-spawn-env-codex-resume-provenance.test.ts',
   'src/main/ipc/preflight-provider-command-selection.test.ts',
   'src/main/ipc/preflight-runnable-local-cli.test.ts',
-  'src/main/managed-server/install/remote-node-runtime-store-windows.test.ts'
+  'src/main/managed-server/install/remote-node-runtime-store-windows.test.ts',
+  'src/main/skills/skill-windows-rename-contention.integration.test.ts'
 ]

config/scripts/win32-test-lane-registration.test.mjs

@@ -109,9 +109,7 @@ const MANUAL_OPT_IN = [
   'src/main/git/runner-wsl-linked-gitdir-windows.test.ts',
   // `runRealWsl = … && ORCA_REAL_WSL_BANNER_TEST === '1'`; needs a real distro.
   'src/main/local-worktree-filesystem-wsl-banner.wsl.test.ts',
-  // `RUN_REAL_WINDOWS = platform === 'win32' && ORCA_REAL_WINDOWS_SKILL_TEST === '1'`.
-  'src/main/skills/skill-windows-rename-contention.integration.test.ts',
-  // Same flag; installs into a real Windows workspace.
+  // `ORCA_REAL_WINDOWS_SKILL_TEST === '1'`; installs into a real Windows workspace.
   'src/main/skills/skill-windows-workspace.integration.test.ts',
   // `RUN_REAL_WSL = … && ORCA_REAL_WSL_SKILL_TEST === '1'`; real distro filesystem.
   'src/main/skills/skill-wsl-delete.integration.test.ts',
@@ -129,7 +127,7 @@ const MANUAL_OPT_IN = [
 
 /** Caps so growing either list is two deliberate edits, not one. */
 const UNREGISTERED_MAX = 8
-const MANUAL_OPT_IN_MAX = 10
+const MANUAL_OPT_IN_MAX = 9
 
 /**
  * Floor for the Windows-gated population, so a broken walk or a regex that
@@ -350,6 +348,18 @@ function sourceOf(path) {
 }
 
 describe('Windows-gated test files are registered in the Windows CI lane', () => {
+  it('enables real Windows rename contention tests in the registered step', () => {
+    const workflow = parse(readFileSync(join(projectDir, '.github/workflows/pr.yml'), 'utf8'))
+    const step = workflow.jobs.package_windows.steps.find(
+      (candidate) => candidate.name === 'Test Windows-specific boundaries'
+    )
+    expect(step.env.ORCA_REAL_WINDOWS_SKILL_TEST).toBe('1')
+    expect(step.env.ORCA_BACKGROUND_LAUNCH).toBe('1')
+    expect(step.run).toContain(
+      'src/main/skills/skill-windows-rename-contention.integration.test.ts'
+    )
+  })
+
   it('scans a plausible number of test files', () => {
     // A broken root or extension filter would make every assertion below vacuous.
     expect(scannedTestFiles.length).toBeGreaterThan(5000)

src/main/skills/skill-windows-rename-contention.integration.test.ts

@@ -1,37 +1,55 @@
-import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'
-import { mkdir, mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises'
+import type { ChildProcessWithoutNullStreams } from 'node:child_process'
+import type * as FsPromises from 'node:fs/promises'
+import { mkdir, mkdtemp, readFile, rename, rm, stat, writeFile } from 'node:fs/promises'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
-import { afterEach, describe, expect, it } from 'vitest'
+import { spawnProcess } from '@orca/process-host'
+import { afterEach, describe, expect, it, vi } from 'vitest'
 import { renameSkillPathWithWindowsRetry } from './skill-filesystem-retry'
 
 const RUN_REAL_WINDOWS =
   process.platform === 'win32' && process.env.ORCA_REAL_WINDOWS_SKILL_TEST === '1'
 const roots: string[] = []
-const lockers: ChildProcessWithoutNullStreams[] = []
+const lockers: {
+  child: ChildProcessWithoutNullStreams
+  closed: Promise<number | null>
+}[] = []
+let onRenameError: ((error: unknown) => Promise<void>) | undefined
+
+vi.mock('node:fs/promises', async (importOriginal) => {
+  const actual = await importOriginal<typeof FsPromises>()
+  return {
+    ...actual,
+    rename: vi.fn(async (...args: Parameters<typeof actual.rename>) => {
+      try {
+        await actual.rename(...args)
+      } catch (error) {
+        await onRenameError?.(error)
+        throw error
+      }
+    })
+  }
+})
 
 const LOCK_SCRIPT = [
   '$stream = [System.IO.File]::Open($env:ORCA_SKILL_LOCK_PATH, [System.IO.FileMode]::Open,',
   '  [System.IO.FileAccess]::Read, [System.IO.FileShare]::None)',
   "[Console]::Out.WriteLine('LOCKED')",
-  'Start-Sleep -Milliseconds ([int]$env:ORCA_SKILL_LOCK_DURATION_MS)',
+  '$null = [Console]::In.ReadLine()',
   '$stream.Dispose()'
 ].join('\n')
 
-function holdFile(path: string, durationMs: number) {
-  const child = spawn(
-    'powershell.exe',
-    ['-NoProfile', '-NonInteractive', '-Command', LOCK_SCRIPT],
-    {
-      windowsHide: true,
-      env: {
-        ...process.env,
-        ORCA_SKILL_LOCK_PATH: path,
-        ORCA_SKILL_LOCK_DURATION_MS: String(durationMs)
-      }
+function holdFile(path: string) {
+  const child = spawnProcess({
+    program: 'powershell.exe',
+    args: ['-NoProfile', '-NonInteractive', '-Command', LOCK_SCRIPT],
+    env: {
+      ...process.env,
+      ORCA_SKILL_LOCK_PATH: path
     }
-  )
-  lockers.push(child)
+  })
+  const closed = new Promise<number | null>((resolve) => child.once('close', resolve))
+  lockers.push({ child, closed })
   let ready = false
   const locked = new Promise<void>((resolve, reject) => {
     child.once('error', reject)
@@ -47,19 +65,15 @@ function holdFile(path: string, durationMs: number) {
       }
     })
   })
-  const released = new Promise<void>((resolve, reject) => {
-    child.once('error', reject)
-    child.once('exit', (code) => {
-      if (code === 0) {
-        resolve()
-      } else if (ready) {
-        reject(new Error(`windows-locker-exited-${code ?? 'unknown'}`))
-      } else {
-        resolve()
+  return {
+    locked,
+    release: async () => {
+      if (child.exitCode === null && child.signalCode === null) {
+        child.stdin.end('\n')
       }
-    })
-  })
-  return { locked, released }
+      expect(await closed).toBe(0)
+    }
+  }
 }
 
 async function fixture(label: string) {
@@ -74,29 +88,40 @@ async function fixture(label: string) {
 }
 
 afterEach(async () => {
-  for (const child of lockers.splice(0)) {
-    if (child.exitCode === null) {
-      child.kill()
-    }
-  }
+  onRenameError = undefined
+  vi.clearAllMocks()
+  await Promise.all(
+    lockers.splice(0).map(async ({ child, closed }) => {
+      if (child.exitCode === null && child.signalCode === null) {
+        child.kill()
+      }
+      await closed
+    })
+  )
   await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true })))
 })
 
 describe.runIf(RUN_REAL_WINDOWS)('real Windows rename contention', () => {
   it('retries a transient exclusive file lock', async () => {
     const value = await fixture('transient')
-    const lock = holdFile(value.file, 300)
+    const lock = holdFile(value.file)
     await lock.locked
 
+    // Release only after a real sharing violation, outside the retry timer budget.
+    onRenameError = async (error) => {
+      expect(error).toMatchObject({ code: expect.stringMatching(/^(EACCES|EBUSY|EPERM)$/) })
+      onRenameError = undefined
+      await lock.release()
+    }
     await renameSkillPathWithWindowsRetry(value.source, value.target)
-    await lock.released
 
+    expect(vi.mocked(rename).mock.calls.length).toBeGreaterThanOrEqual(2)
     expect(await readFile(join(value.target, 'SKILL.md'), 'utf8')).toBe('private skill')
   })
 
   it('preserves the source after bounded retry exhaustion', async () => {
     const value = await fixture('exhaustion')
-    const lock = holdFile(value.file, 2_000)
+    const lock = holdFile(value.file)
     await lock.locked
 
     await expect(renameSkillPathWithWindowsRetry(value.source, value.target)).rejects.toMatchObject(
@@ -105,7 +130,8 @@ describe.runIf(RUN_REAL_WINDOWS)('real Windows rename contention', () => {
       }
     )
     expect((await stat(value.source)).isDirectory()).toBe(true)
-    await lock.released
+    expect(rename).toHaveBeenCalledTimes(6)
+    await lock.release()
     expect(await readFile(value.file, 'utf8')).toBe('private skill')
 
     await expect(