fix(skills): keep the disposal verdict when staging cleanup fails, and retry release-cut installs (#21366)

* fix(skills): keep the disposal verdict when staging cleanup fails

`begin()` ended with `await this.removeOwnershipIfDisposed()` inside its `finally`,
so when a caller raced `dispose()` the rejection it received was whatever that
opportunistic `rmdir` threw -- not `skill-upload-service-disposed`. A caller could
not tell "the service shut down" from "the filesystem broke", and the Windows
release gate saw it as `EPERM: operation not permitted, rmdir`.

Two causes, both fixed here:

- The EPERM itself: an in-flight operation and disposal each call
  `ownership.remove()`, so two `rm -rf` run concurrently against the same owner
  directory. On POSIX the loser reads ENOENT and `force: true` swallows it; on
  Windows the loser reads a delete-pending directory and gets EPERM.
  `SkillUploadStagingOwnership.remove()` now joins one removal and forgets it on
  failure so a later caller still retries.
- The masking: cleanup in a `finally` no longer replaces the outcome of the call
  it is cleaning up after. Disposal retries staging removal and reports its own
  failure, matching `removeUnpublished`/`retainFailedCleanup` in this class.

Both regressions are pinned platform-independently: one injects a failing
ownership removal and asserts the racing `begin` still rejects with
`skill-upload-service-disposed` while `dispose()` reports the cleanup failure; the
other models Windows delete-pending rmdir in the `node:fs/promises` mock, which
turns a second removal into EPERM on every platform.

* ci(release-cut): retry the installs that fetch node-gyp headers

`golden e2e windows` installs with lifecycle scripts enabled, so pnpm runs
node-gyp for the `native/windows-registry` workspace project, which downloads that
Node version's headers from nodejs.org. A single `read ECONNRESET` on that fetch
failed a blocking release gate, and the release build job one screen below already
wraps its install in `nick-fields/retry@v4` for exactly this class of failure.

Both remaining unretried installs in this workflow (the blocking platform golden and
the non-blocking rendering-evidence lane) now use the same wrapper, and a contract
test keeps every release-cut install retryable.

.github/workflows/release-cut.yml

@@ -871,8 +871,17 @@ jobs:
           npm install -g node-gyp@11.5.0
           echo "npm_config_node_gyp=$(npm root -g)/node-gyp/bin/node-gyp.js" >> "$GITHUB_ENV"
 
+      # Why: this install runs lifecycle scripts, so node-gyp rebuilds
+      # native/windows-registry and fetches that Node version's headers from
+      # nodejs.org. One `read ECONNRESET` there failed this blocking gate and the
+      # whole cut. Retry like the release build's install below.
       - name: Install dependencies
-        run: pnpm install --frozen-lockfile
+        uses: nick-fields/retry@v4
+        with:
+          timeout_minutes: 10
+          max_attempts: 3
+          retry_wait_seconds: 30
+          command: pnpm install --frozen-lockfile
 
       - name: Build Electron app for platform golden
         run: npx electron-vite build --mode e2e
@@ -1088,8 +1097,14 @@ jobs:
           npm install -g node-gyp@11.5.0
           echo "npm_config_node_gyp=$(npm root -g)/node-gyp/bin/node-gyp.js" >> "$GITHUB_ENV"
 
+      # Same node-gyp header fetch as the blocking golden gate above.
       - name: Install dependencies
-        run: pnpm install --frozen-lockfile
+        uses: nick-fields/retry@v4
+        with:
+          timeout_minutes: 10
+          max_attempts: 3
+          retry_wait_seconds: 30
+          command: pnpm install --frozen-lockfile
 
       - name: Build Electron app for terminal rendering evidence
         run: npx electron-vite build --mode e2e

config/scripts/ci-dependency-download-cache.test.mjs

@@ -32,25 +32,37 @@ describe('CI dependency download caches', () => {
 describe('release install targets', () => {
   const macCpuFlag = '--cpu=current,x64,arm64'
   // Both shapes: `run:` steps and steps wrapped in nick-fields/retry (`with.command`).
+  const installCommand = (step) => step.with?.command ?? step.run
   const installSteps = (name) =>
     Object.values(workflow(name).jobs)
       .flatMap((job) => job.steps ?? [])
-      .map((step) => step.with?.command ?? step.run)
-      .filter((command) => typeof command === 'string' && command.includes('pnpm install '))
+      .filter((step) => installCommand(step)?.includes('pnpm install '))
+  const installCommands = (name) => installSteps(name).map(installCommand)
 
   it.each(['adhoc-mac-build', 'daily-mac-build', 'hourly-mac-build', 'release-mac-build'])(
     '%s installs both mac CPU variants for the x64+arm64 package config',
     (name) => {
-      const installs = installSteps(name)
+      const installs = installCommands(name)
       expect(installs.length).toBeGreaterThan(0)
       expect(installs.some((command) => command.includes(macCpuFlag))).toBe(true)
     }
   )
 
+  // A transient `read ECONNRESET` fetching this Node version's headers for
+  // native/windows-registry's node-gyp rebuild failed a blocking golden gate and the cut.
+  it('retries every release-cut install so one transient download cannot fail a cut', () => {
+    const installs = installSteps('release-cut')
+    expect(installs.length).toBeGreaterThan(0)
+    for (const step of installs) {
+      expect(step.uses).toBe('nick-fields/retry@v4')
+      expect(step.with.max_attempts).toBeGreaterThan(1)
+    }
+  })
+
   it.each(['release-cut', 'dev-channel-win-build', 'windows-signing-rehearsal'])(
     '%s keeps installs scoped to the runner host',
     (name) => {
-      const installs = installSteps(name)
+      const installs = installCommands(name)
       expect(installs.length).toBeGreaterThan(0)
       for (const command of installs) {
         expect(command).not.toContain('--os=')

src/main/skills/skill-upload-session-admission-regression.test.ts

@@ -1,11 +1,12 @@
 import { createHash } from 'node:crypto'
-import { mkdtemp, readdir, rm } from 'node:fs/promises'
+import { mkdir, mkdtemp, readdir, rm } from 'node:fs/promises'
 import type * as NodeFsPromises from 'node:fs/promises'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { afterEach, describe, expect, it, vi } from 'vitest'
 import type { SkillUploadRetainedPaths } from './skill-upload-retained-paths'
 import { SkillUploadSessionService } from './skill-upload-session-service'
+import type { SkillUploadStagingOwnership } from './skill-upload-staging-ownership'
 
 const roots: string[] = []
 
@@ -14,6 +15,9 @@ const openGate = vi.hoisted(() => ({
   started: null as (() => void) | null
 }))
 
+// Models Windows delete-pending rmdir: the first removal wins and every later one gets EPERM.
+const deletePendingGate = vi.hoisted((): { removed: Set<string> | null } => ({ removed: null }))
+
 vi.mock('node:fs/promises', async (importOriginal) => {
   const actual = await importOriginal<typeof NodeFsPromises>()
   return {
@@ -27,6 +31,16 @@ vi.mock('node:fs/promises', async (importOriginal) => {
         await release
       }
       return handle
+    },
+    rm: async (path: string, options?: Parameters<typeof actual.rm>[1]) => {
+      const removed = deletePendingGate.removed
+      if (removed?.has(path)) {
+        throw Object.assign(new Error(`EPERM: operation not permitted, rmdir '${path}'`), {
+          code: 'EPERM'
+        })
+      }
+      removed?.add(path)
+      await actual.rm(path, options)
     }
   }
 })
@@ -35,6 +49,7 @@ afterEach(async () => {
   vi.useRealTimers()
   openGate.release = null
   openGate.started = null
+  deletePendingGate.removed = null
   await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true })))
 })
 
@@ -52,6 +67,30 @@ function retainedPathCleanup(service: SkillUploadSessionService): SkillUploadRet
   return service['retainedPaths']
 }
 
+function stagingOwnership(service: SkillUploadSessionService): SkillUploadStagingOwnership {
+  return service['ownership']
+}
+
+function initializationGate(uploads: string) {
+  let releaseInitialization!: () => void
+  const initializationReleased = new Promise<void>((resolve) => {
+    releaseInitialization = resolve
+  })
+  let markInitializationStarted!: () => void
+  const initializationStarted = new Promise<void>((resolve) => {
+    markInitializationStarted = resolve
+  })
+  return {
+    initializationStarted,
+    releaseInitialization,
+    initializeRoot: async () => {
+      await mkdir(uploads, { recursive: true })
+      markInitializationStarted()
+      await initializationReleased
+    }
+  }
+}
+
 async function stagedArchiveCount(uploads: string): Promise<number> {
   const owners = await readdir(uploads, { withFileTypes: true })
   const archives = await Promise.all(
@@ -146,6 +185,42 @@ describe('SkillUploadSessionService admission regressions', () => {
     await service.dispose()
   })
 
+  it('reports disposal, not the staging cleanup failure, to a begin racing disposal', async () => {
+    const root = await mkdtemp(join(tmpdir(), 'orca-skill-upload-admission-'))
+    roots.push(root)
+    const uploads = join(root, 'uploads')
+    const gate = initializationGate(uploads)
+    const service = new SkillUploadSessionService(uploads, { initializeRoot: gate.initializeRoot })
+    const cleanupFailure = new Error('injected-staging-rmdir-failure')
+    vi.spyOn(stagingOwnership(service), 'remove').mockRejectedValue(cleanupFailure)
+
+    const begin = service.begin({ package: identity(Buffer.from('closing package')) })
+    await gate.initializationStarted
+    const disposal = service.dispose()
+    gate.releaseInitialization()
+
+    await expect(begin).rejects.toThrow('skill-upload-service-disposed')
+    await expect(disposal).rejects.toBe(cleanupFailure)
+  })
+
+  it('removes disposed staging once when a begin and disposal race the same directory', async () => {
+    const root = await mkdtemp(join(tmpdir(), 'orca-skill-upload-admission-'))
+    roots.push(root)
+    const uploads = join(root, 'uploads')
+    const gate = initializationGate(uploads)
+    const service = new SkillUploadSessionService(uploads, { initializeRoot: gate.initializeRoot })
+    deletePendingGate.removed = new Set<string>()
+
+    const begin = service.begin({ package: identity(Buffer.from('closing package')) })
+    await gate.initializationStarted
+    const disposal = service.dispose()
+    gate.releaseInitialization()
+
+    await expect(begin).rejects.toThrow('skill-upload-service-disposed')
+    await disposal
+    expect(await readdir(uploads)).toEqual([])
+  })
+
   it('removes an unpublished archive when disposal starts during open', async () => {
     const root = await mkdtemp(join(tmpdir(), 'orca-skill-upload-admission-'))
     roots.push(root)

src/main/skills/skill-upload-session-service.ts

@@ -77,7 +77,8 @@ export class SkillUploadSessionService {
       return skillUploadBeginResult(session)
     } finally {
       leaveOperation()
-      await this.removeOwnershipIfDisposed()
+      // Opportunistic cleanup: disposal retries it, so its failure must not replace this outcome.
+      await this.removeOwnershipIfDisposed().catch(() => undefined)
     }
   }
 

src/main/skills/skill-upload-staging-ownership.ts

@@ -16,6 +16,7 @@ export type SkillUploadStagingOwnershipOptions = {
 export class SkillUploadStagingOwnership {
   readonly directory: string
   private readonly processIsAlive: (pid: number) => boolean
+  private removal: Promise<void> | null = null
 
   constructor(
     private readonly root: string,
@@ -35,8 +36,18 @@ export class SkillUploadStagingOwnership {
     await mkdir(this.directory, { recursive: true, mode: 0o700 })
   }
 
+  // Callers race this (an in-flight operation and disposal), and a second rmdir of a
+  // delete-pending directory fails with EPERM on Windows, so join one removal instead.
   async remove(): Promise<void> {
-    await rm(this.directory, { recursive: true, force: true })
+    const removal = (this.removal ??= rm(this.directory, { recursive: true, force: true }))
+    try {
+      await removal
+    } catch (error) {
+      if (this.removal === removal) {
+        this.removal = null
+      }
+      throw error
+    }
   }
 
   private async cleanupAbandonedOwners(): Promise<void> {