diff --git a/.github/workflows/release-cut.yml b/.github/workflows/release-cut.yml index ac2e7f904e3..eb80d20af72 100644 --- a/.github/workflows/release-cut.yml +++ b/.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 diff --git a/config/scripts/ci-dependency-download-cache.test.mjs b/config/scripts/ci-dependency-download-cache.test.mjs index d2111a230bb..0860b53ebca 100644 --- a/config/scripts/ci-dependency-download-cache.test.mjs +++ b/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=') diff --git a/src/main/skills/skill-upload-session-admission-regression.test.ts b/src/main/skills/skill-upload-session-admission-regression.test.ts index 9bbdb71ef68..3d13a587889 100644 --- a/src/main/skills/skill-upload-session-admission-regression.test.ts +++ b/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 | null } => ({ removed: null })) + vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal() return { @@ -27,6 +31,16 @@ vi.mock('node:fs/promises', async (importOriginal) => { await release } return handle + }, + rm: async (path: string, options?: Parameters[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((resolve) => { + releaseInitialization = resolve + }) + let markInitializationStarted!: () => void + const initializationStarted = new Promise((resolve) => { + markInitializationStarted = resolve + }) + return { + initializationStarted, + releaseInitialization, + initializeRoot: async () => { + await mkdir(uploads, { recursive: true }) + markInitializationStarted() + await initializationReleased + } + } +} + async function stagedArchiveCount(uploads: string): Promise { 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() + + 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) diff --git a/src/main/skills/skill-upload-session-service.ts b/src/main/skills/skill-upload-session-service.ts index c06e628dcff..e2bba665aa2 100644 --- a/src/main/skills/skill-upload-session-service.ts +++ b/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) } } diff --git a/src/main/skills/skill-upload-staging-ownership.ts b/src/main/skills/skill-upload-staging-ownership.ts index 1f9c01518f5..cc630b004e0 100644 --- a/src/main/skills/skill-upload-staging-ownership.ts +++ b/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 | 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 { - 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 {