mirror of
https://github.com/stablyai/orca.git
synced 2026-10-06 16:02:25 +00:00
test(ci): retry Windows teardown EPERM and restart evaluate misses (#17780)
Restart-survival polls treated a recycled renderer as a hard failure. Wrap those evaluates so "Execution context was destroyed" is a pending miss. Windows package-lane teardowns after a force-kill used rmSync with force:true only, which does not absorb EPERM; put them on the shared maxRetries:8 policy.
This commit is contained in:
Neil
@@ -1,4 +1,5 @@
|
||||
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { removeTreeSync } from '../windows-transient-lock-removal'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
|
||||
@@ -37,7 +38,7 @@ describeOnWindows('Windows .cmd argument round-trip', () => {
|
||||
})
|
||||
|
||||
afterAll(() => {
|
||||
rmSync(dir, { recursive: true, force: true })
|
||||
removeTreeSync(dir)
|
||||
})
|
||||
|
||||
function decode(stdout: string): string[] {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { chmodSync, mkdtempSync, writeFileSync } from 'node:fs'
|
||||
import { removeTreeSync } from './windows-transient-lock-removal'
|
||||
import type * as NodeFs from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
@@ -34,7 +35,7 @@ const createdPaths: string[] = []
|
||||
afterEach(() => {
|
||||
openedPaths.length = 0
|
||||
for (const path of createdPaths.splice(0)) {
|
||||
rmSync(path, { recursive: true, force: true })
|
||||
removeTreeSync(path)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -0,0 +1,171 @@
|
||||
import { readFileSync } from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
/**
|
||||
* The Windows CI lane runs a fixed list of specs on `windows-2022`, and every one of them removes
|
||||
* a temporary tree when it is done. On Windows those removals race a handle the OS has not
|
||||
* released yet — a just-exited child, an indexer, a dlopen'd native module — so a raw
|
||||
* `rmSync(dir, { recursive: true, force: true })` throws EPERM after the test's assertions have
|
||||
* all passed, and the lane reports a green test as a failure.
|
||||
*
|
||||
* `removeTree`/`removeTreeSync` carry the repo's `maxRetries: 8` policy. This keeps the lane on
|
||||
* them: a new spec that hand-rolls the removal fails here rather than intermittently on Windows.
|
||||
*/
|
||||
const REPO_ROOT = join(__dirname, '..', '..')
|
||||
const WORKFLOW_PATH = join(REPO_ROOT, '.github', 'workflows', 'pr.yml')
|
||||
const WINDOWS_STEP_NAME = 'Test Windows-specific boundaries'
|
||||
|
||||
/** The spec paths the `package (windows)` job passes to vitest, read from the workflow itself. */
|
||||
function readWindowsLaneSpecs(): string[] {
|
||||
const workflow = readFileSync(WORKFLOW_PATH, 'utf8')
|
||||
const stepIndex = workflow.indexOf(`- name: ${WINDOWS_STEP_NAME}`)
|
||||
expect(stepIndex, `${WORKFLOW_PATH} no longer has a "${WINDOWS_STEP_NAME}" step`).toBeGreaterThan(
|
||||
-1
|
||||
)
|
||||
const nextStepIndex = workflow.indexOf('\n - name:', stepIndex + 1)
|
||||
const step = workflow.slice(stepIndex, nextStepIndex === -1 ? undefined : nextStepIndex)
|
||||
return step
|
||||
.split('\n')
|
||||
.map((line) => line.trim())
|
||||
.filter((line) => /^(src|tests|config)\/.+\.(test|spec)\.(ts|tsx|mjs)$/.test(line))
|
||||
}
|
||||
|
||||
/** `node:fs` and `node:fs/promises`, spelled with or without the `node:` prefix. */
|
||||
const FS_SPECIFIER = String.raw`['"](?:node:)?fs(?:/promises)?['"]`
|
||||
/** The `{ … }` clause of an fs import or require, which is where a rename would be declared. */
|
||||
const FS_BINDING_CLAUSE = new RegExp(
|
||||
String.raw`\{([^}]*)\}\s*(?:from\s*${FS_SPECIFIER}|=\s*(?:await\s+import|require)\(\s*${FS_SPECIFIER})`,
|
||||
'g'
|
||||
)
|
||||
/** `rm as removeDir` or `rmSync: dropTree` — the two ways a binding gets a local name. */
|
||||
const RENAMED_REMOVAL = /\brm(?:Sync)?\s*(?:as|:)\s*([A-Za-z0-9_$]+)/g
|
||||
|
||||
/**
|
||||
* The local names a recursive removal can be called by in `source`.
|
||||
*
|
||||
* Namespaced spellings are covered by the optional `<identifier>.` prefix in the matcher rather
|
||||
* than by listing names, so `fsp.rm` and `fsPromises.rm` are caught without anyone having to teach
|
||||
* the rule that spelling first. Renames are the one form that prefix cannot see, so they are read
|
||||
* out of the import clause.
|
||||
*/
|
||||
function collectRemovalNames(source: string): string[] {
|
||||
const names = new Set(['rmSync', 'rm'])
|
||||
for (const clause of source.matchAll(FS_BINDING_CLAUSE)) {
|
||||
for (const rename of clause[1].matchAll(RENAMED_REMOVAL)) {
|
||||
names.add(rename[1])
|
||||
}
|
||||
}
|
||||
return [...names]
|
||||
}
|
||||
|
||||
/** Every recursive removal that does not go through the retrying helper. */
|
||||
function findRawRecursiveRemovals(source: string): number[] {
|
||||
const offenders: number[] = []
|
||||
const call = new RegExp(
|
||||
String.raw`(?<![\w$.])(?:[\w$]+\.)?(?:${collectRemovalNames(source).join('|')})\s*\(`,
|
||||
'g'
|
||||
)
|
||||
let match: RegExpExecArray | null
|
||||
while ((match = call.exec(source)) !== null) {
|
||||
// Read to the call's closing paren so multi-line option objects are covered.
|
||||
let depth = 0
|
||||
let end = match.index + match[0].length - 1
|
||||
for (; end < source.length; end += 1) {
|
||||
if (source[end] === '(') {
|
||||
depth += 1
|
||||
} else if (source[end] === ')') {
|
||||
depth -= 1
|
||||
if (depth === 0) {
|
||||
break
|
||||
}
|
||||
}
|
||||
}
|
||||
const args = source.slice(match.index, end + 1)
|
||||
if (!args.includes('recursive')) {
|
||||
continue
|
||||
}
|
||||
if (args.includes('maxRetries')) {
|
||||
continue
|
||||
}
|
||||
offenders.push(source.slice(0, match.index).split('\n').length)
|
||||
}
|
||||
return offenders
|
||||
}
|
||||
|
||||
describe('windows lane tree removal', () => {
|
||||
const specs = readWindowsLaneSpecs()
|
||||
|
||||
it('reads a non-trivial spec list out of the workflow', () => {
|
||||
// A parser that silently matched nothing would make every assertion below vacuous.
|
||||
expect(specs.length).toBeGreaterThan(10)
|
||||
expect(specs).toContain('config/scripts/rebuild-native-deps.test.mjs')
|
||||
expect(specs).toContain('src/main/windows/windows-host-job.win32.test.ts')
|
||||
})
|
||||
|
||||
it('actually detects a raw recursive removal', () => {
|
||||
// Without this the scan below passes for any reason at all, including not scanning.
|
||||
expect(findRawRecursiveRemovals('rmSync(dir, { recursive: true, force: true })')).toEqual([1])
|
||||
expect(
|
||||
findRawRecursiveRemovals(
|
||||
'await rm(dir, {\n recursive: true,\n force: true,\n maxRetries: 8\n})'
|
||||
)
|
||||
).toEqual([])
|
||||
// A single-file removal is not this rule's business.
|
||||
expect(findRawRecursiveRemovals('rmSync(file, { force: true })')).toEqual([])
|
||||
})
|
||||
|
||||
it('detects the removal whatever the import spelled it', () => {
|
||||
// Why: a rule that only reads one import style stops catching violations the moment someone
|
||||
// writes the next one differently — and the guard goes on reporting zero offenders.
|
||||
const spellings: [string, string][] = [
|
||||
['bare named import', "import { rmSync } from 'node:fs'\nrmSync(DIR"],
|
||||
['fs namespace', "import * as fs from 'node:fs'\nfs.rmSync(DIR"],
|
||||
['fsp namespace', "import * as fsp from 'node:fs/promises'\nawait fsp.rm(DIR"],
|
||||
[
|
||||
'fsPromises namespace',
|
||||
"import * as fsPromises from 'node:fs/promises'\nawait fsPromises.rm(DIR"
|
||||
],
|
||||
['nodeFs namespace', "import * as nodeFs from 'node:fs'\nnodeFs.rmSync(DIR"],
|
||||
['unprefixed fs specifier', "import * as fs from 'fs'\nfs.rmSync(DIR"],
|
||||
[
|
||||
'renamed named import',
|
||||
"import { rm as removeDir } from 'node:fs/promises'\nawait removeDir(DIR"
|
||||
],
|
||||
['renamed require', "const { rmSync: dropTree } = require('node:fs')\ndropTree(DIR"]
|
||||
]
|
||||
|
||||
for (const [label, prelude] of spellings) {
|
||||
const source = `${prelude}, { recursive: true, force: true })`
|
||||
expect(findRawRecursiveRemovals(source), `${label} slipped past the scan`).toEqual([2])
|
||||
}
|
||||
})
|
||||
|
||||
it('still exempts the retrying spellings and single-file removals', () => {
|
||||
// The widened matcher must not start reporting the calls the rule is asking people to write.
|
||||
expect(
|
||||
findRawRecursiveRemovals(
|
||||
"import * as fsp from 'node:fs/promises'\nawait fsp.rm(dir, { recursive: true, maxRetries: 8 })"
|
||||
)
|
||||
).toEqual([])
|
||||
expect(
|
||||
findRawRecursiveRemovals(
|
||||
"import { rm as removeDir } from 'node:fs/promises'\nawait removeDir(file, { force: true })"
|
||||
)
|
||||
).toEqual([])
|
||||
// `rm` inside a longer identifier is not a removal call.
|
||||
expect(findRawRecursiveRemovals('confirmRemoval(dir, { recursive: true })')).toEqual([])
|
||||
})
|
||||
|
||||
it('removes trees through the retrying helper, never a raw recursive rm', () => {
|
||||
const offenders = specs.flatMap((spec) => {
|
||||
const source = readFileSync(join(REPO_ROOT, spec), 'utf8')
|
||||
return findRawRecursiveRemovals(source).map((line) => `${spec}:${line}`)
|
||||
})
|
||||
|
||||
expect(
|
||||
offenders,
|
||||
'these teardowns can throw EPERM on Windows after their assertions have passed; use removeTree/removeTreeSync from src/shared/windows-transient-lock-removal.ts'
|
||||
).toEqual([])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,117 @@
|
||||
import type * as NodeFs from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { scanSourceTree, stripComments } from './source-scan/source-tree-scan'
|
||||
import {
|
||||
WINDOWS_RM_MAX_RETRIES,
|
||||
WINDOWS_RM_RETRY_DELAY_MS,
|
||||
removeTreeSync,
|
||||
transientLockRemovalOptions
|
||||
} from './windows-transient-lock-removal'
|
||||
|
||||
const { rmSyncMock } = vi.hoisted(() => ({
|
||||
rmSyncMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('node:fs', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof NodeFs>()
|
||||
return { ...actual, rmSync: rmSyncMock }
|
||||
})
|
||||
|
||||
function withPlatform(platform: NodeJS.Platform): void {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: platform })
|
||||
}
|
||||
|
||||
const SOURCE_ROOT = join(__dirname, '..')
|
||||
const OWNING_MODULE = 'shared/windows-transient-lock-removal.ts'
|
||||
/** A `const WINDOWS_RM_… =` line, i.e. a file stating the policy rather than importing it. */
|
||||
const POLICY_DECLARATION =
|
||||
/^\s*(?:export\s+)?const\s+WINDOWS_RM_(?:MAX_RETRIES|RETRY_DELAY_MS)\s*=/m
|
||||
|
||||
/** Every file that declares the retry policy instead of importing it. */
|
||||
function findPolicyDeclarations(): string[] {
|
||||
return scanSourceTree(SOURCE_ROOT, { includeTests: true })
|
||||
.filter(
|
||||
(file) =>
|
||||
POLICY_DECLARATION.test(file.source) && POLICY_DECLARATION.test(stripComments(file.source))
|
||||
)
|
||||
.map((file) => file.relativePath)
|
||||
.sort()
|
||||
}
|
||||
|
||||
describe('transient lock removal options', () => {
|
||||
const originalPlatform = process.platform
|
||||
|
||||
afterEach(() => {
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
|
||||
rmSyncMock.mockReset()
|
||||
})
|
||||
|
||||
it('retries on Windows, where a late handle release is the whole problem', () => {
|
||||
withPlatform('win32')
|
||||
|
||||
expect(transientLockRemovalOptions()).toEqual({
|
||||
recursive: true,
|
||||
force: true,
|
||||
maxRetries: WINDOWS_RM_MAX_RETRIES,
|
||||
retryDelay: WINDOWS_RM_RETRY_DELAY_MS
|
||||
})
|
||||
})
|
||||
|
||||
it('matches the repo policy of eight attempts', () => {
|
||||
expect(WINDOWS_RM_MAX_RETRIES).toBe(8)
|
||||
})
|
||||
|
||||
it('asks for no retries where removal is not raced by the OS', () => {
|
||||
for (const platform of ['darwin', 'linux'] as const) {
|
||||
withPlatform(platform)
|
||||
expect(transientLockRemovalOptions()).toEqual({ recursive: true, force: true })
|
||||
Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
|
||||
}
|
||||
})
|
||||
|
||||
it('retries a transient EPERM instead of treating force: true as enough', () => {
|
||||
withPlatform('win32')
|
||||
const eperm = Object.assign(new Error('EPERM: operation not permitted, unlink'), {
|
||||
code: 'EPERM'
|
||||
})
|
||||
rmSyncMock.mockImplementationOnce(() => {
|
||||
throw eperm
|
||||
})
|
||||
rmSyncMock.mockImplementationOnce(() => undefined)
|
||||
|
||||
expect(() => removeTreeSync('C:\\temp\\orca-host-job')).not.toThrow()
|
||||
expect(rmSyncMock).toHaveBeenCalledTimes(2)
|
||||
expect(rmSyncMock.mock.calls[0]?.[1]).toEqual(
|
||||
expect.objectContaining({ recursive: true, force: true, maxRetries: WINDOWS_RM_MAX_RETRIES })
|
||||
)
|
||||
})
|
||||
|
||||
it('does not hide a non-lock removal failure', () => {
|
||||
withPlatform('win32')
|
||||
rmSyncMock.mockImplementation(() => {
|
||||
throw Object.assign(new Error('EIO: i/o error'), { code: 'EIO' })
|
||||
})
|
||||
|
||||
expect(() => removeTreeSync('C:\\temp\\orca-host-job')).toThrow('EIO')
|
||||
expect(rmSyncMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('actually detects a file that states the policy', () => {
|
||||
// Without this the scan below passes for any reason at all, including not scanning.
|
||||
expect(POLICY_DECLARATION.test('const WINDOWS_RM_MAX_RETRIES = 8')).toBe(true)
|
||||
expect(POLICY_DECLARATION.test(' export const WINDOWS_RM_RETRY_DELAY_MS = 150')).toBe(true)
|
||||
// Importing the policy is the thing this rule is asking for, not a violation of it.
|
||||
expect(POLICY_DECLARATION.test('import { WINDOWS_RM_MAX_RETRIES } from x')).toBe(false)
|
||||
expect(POLICY_DECLARATION.test(' retryDelay: WINDOWS_RM_RETRY_DELAY_MS')).toBe(false)
|
||||
})
|
||||
|
||||
it('is the only file that states the policy', () => {
|
||||
// Why a ratchet: a second copy is how "8 attempts" becomes 8 in one file and 4 in another,
|
||||
// and nothing fails until a Windows lane goes red for a reason nobody can place.
|
||||
expect(
|
||||
findPolicyDeclarations(),
|
||||
'declare the retry policy once, in src/shared/windows-transient-lock-removal.ts, and import it'
|
||||
).toEqual([OWNING_MODULE])
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,64 @@
|
||||
// Why: Windows releases handles late. Antivirus, the search indexer, a just-exited child and a
|
||||
// freshly dlopen'd DLL all keep a tree Node has just emptied locked for a few milliseconds, which
|
||||
// surfaces as EBUSY/ENOTEMPTY/EPERM. Node's own `maxRetries` absorbs exactly that, and the repo
|
||||
// already settled on 8 attempts — but only product code was using it, so test teardown kept
|
||||
// failing tests whose assertions had already passed.
|
||||
|
||||
import type { RmOptions } from 'node:fs'
|
||||
import { rmSync } from 'node:fs'
|
||||
import { rm } from 'node:fs/promises'
|
||||
|
||||
export const WINDOWS_RM_MAX_RETRIES = 8
|
||||
export const WINDOWS_RM_RETRY_DELAY_MS = 150
|
||||
|
||||
/** `rm`/`rmSync` options for a recursive removal that must survive a late handle release. */
|
||||
export function transientLockRemovalOptions(): RmOptions {
|
||||
const base = { recursive: true, force: true }
|
||||
if (process.platform !== 'win32') {
|
||||
return base
|
||||
}
|
||||
return { ...base, maxRetries: WINDOWS_RM_MAX_RETRIES, retryDelay: WINDOWS_RM_RETRY_DELAY_MS }
|
||||
}
|
||||
|
||||
function isTransientWindowsLockError(error: unknown): boolean {
|
||||
if (process.platform !== 'win32' || typeof error !== 'object' || error === null) {
|
||||
return false
|
||||
}
|
||||
const code = 'code' in error && typeof error.code === 'string' ? error.code : undefined
|
||||
if (code && ['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(code)) {
|
||||
return true
|
||||
}
|
||||
const message = 'message' in error && typeof error.message === 'string' ? error.message : ''
|
||||
return /directory not empty|resource busy|operation not permitted/i.test(message)
|
||||
}
|
||||
|
||||
function sleepSync(ms: number): void {
|
||||
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms)
|
||||
}
|
||||
|
||||
/** Recursively remove a directory, retrying the transient Windows locks. */
|
||||
export function removeTreeSync(targetPath: string): void {
|
||||
const options = transientLockRemovalOptions()
|
||||
const extraAttempts = process.platform === 'win32' ? WINDOWS_RM_MAX_RETRIES : 0
|
||||
let attempt = 0
|
||||
for (;;) {
|
||||
try {
|
||||
rmSync(targetPath, options)
|
||||
return
|
||||
} catch (error) {
|
||||
// Why the outer loop: Node's `maxRetries` only runs inside a real `rmSync`. A mock, or a
|
||||
// handle that outlives those inner attempts, still surfaces EPERM. `force: true` only
|
||||
// suppresses ENOENT.
|
||||
if (attempt >= extraAttempts || !isTransientWindowsLockError(error)) {
|
||||
throw error
|
||||
}
|
||||
sleepSync(WINDOWS_RM_RETRY_DELAY_MS)
|
||||
attempt += 1
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/** Recursively remove a directory, retrying the transient Windows locks. */
|
||||
export async function removeTree(targetPath: string): Promise<void> {
|
||||
await rm(targetPath, transientLockRemovalOptions())
|
||||
}
|
||||
Reference in New Issue
Block a user