From 181e0ba076db25033cb2edad3a24e63a3c791819 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:07:49 +0800 Subject: [PATCH] fix(cli): use proven cross-process target locks Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> --- cli/bun.lock | 14 ++++ cli/package.json | 2 + cli/src/services/skill-target-lock.ts | 80 ++++--------------- cli/test/helpers/target-lock-worker.ts | 19 +++++ .../unit/services/install-service.test.ts | 9 ++- .../unit/services/skill-target-lock.test.ts | 38 +++++---- 6 files changed, 76 insertions(+), 86 deletions(-) create mode 100644 cli/test/helpers/target-lock-worker.ts diff --git a/cli/bun.lock b/cli/bun.lock index 39760c2d..107bcc74 100644 --- a/cli/bun.lock +++ b/cli/bun.lock @@ -8,12 +8,14 @@ "cac": "^6.7.14", "fflate": "^0.8.2", "prompts": "^2.4.2", + "proper-lockfile": "4.1.2", "semver": "^7.6.3", "zod": "^3.24.1", }, "devDependencies": { "@types/bun": "^1.3.13", "@types/prompts": "^2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "^7.5.8", "@typescript-eslint/eslint-plugin": "^7.18.0", "@typescript-eslint/parser": "^7.18.0", @@ -49,6 +51,10 @@ "@types/prompts": ["@types/prompts@2.4.9", "https://registry.npmmirror.com/@types/prompts/-/prompts-2.4.9.tgz", { "dependencies": { "@types/node": "*", "kleur": "^3.0.3" } }, "sha512-qTxFi6Buiu8+50/+3DGIWLHM6QuWsEKugJnnP6iv2Mc4ncxE4A/OJkjuVOA+5X0X1S/nq5VJRa8Lu+nwcvbrKA=="], + "@types/proper-lockfile": ["@types/proper-lockfile@4.1.4", "https://registry.npmmirror.com/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz", { "dependencies": { "@types/retry": "*" } }, "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ=="], + + "@types/retry": ["@types/retry@0.12.5", "https://registry.npmmirror.com/@types/retry/-/retry-0.12.5.tgz", {}, "sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw=="], + "@types/semver": ["@types/semver@7.7.1", "https://registry.npmmirror.com/@types/semver/-/semver-7.7.1.tgz", {}, "sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA=="], "@typescript-eslint/eslint-plugin": ["@typescript-eslint/eslint-plugin@7.18.0", "https://registry.npmmirror.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-7.18.0.tgz", { "dependencies": { "@eslint-community/regexpp": "^4.10.0", "@typescript-eslint/scope-manager": "7.18.0", "@typescript-eslint/type-utils": "7.18.0", "@typescript-eslint/utils": "7.18.0", "@typescript-eslint/visitor-keys": "7.18.0", "graphemer": "^1.4.0", "ignore": "^5.3.1", "natural-compare": "^1.4.0", "ts-api-utils": "^1.3.0" }, "peerDependencies": { "@typescript-eslint/parser": "^7.0.0", "eslint": "^8.56.0" } }, "sha512-94EQTWZ40mzBc42ATNIBimBEDltSJ9RQHCC8vc/PDbxi4k8dVwUAv4o98dk50M1zB+JGFxp43FP7f8+FP8R6Sw=="], @@ -163,6 +169,8 @@ "globby": ["globby@11.1.0", "https://registry.npmmirror.com/globby/-/globby-11.1.0.tgz", { "dependencies": { "array-union": "^2.1.0", "dir-glob": "^3.0.1", "fast-glob": "^3.2.9", "ignore": "^5.2.0", "merge2": "^1.4.1", "slash": "^3.0.0" } }, "sha512-jhIXaOzy1sb8IyocaruWSn1TjmnBVs8Ayhcy83rmxNJ8q2uWKCAj3CnJY+KpGSXCueAPc0i05kVvVKtP1t9S3g=="], + "graceful-fs": ["graceful-fs@4.2.11", "https://registry.npmmirror.com/graceful-fs/-/graceful-fs-4.2.11.tgz", {}, "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ=="], + "graphemer": ["graphemer@1.4.0", "https://registry.npmmirror.com/graphemer/-/graphemer-1.4.0.tgz", {}, "sha512-EtKwoO6kxCL9WO5xipiHTZlSzBm7WLT627TqC/uVRd0HKmq8NXyebnNYxDoBi7wt8eTWrUrKXCOVaFq9x1kgag=="], "has-flag": ["has-flag@4.0.0", "https://registry.npmmirror.com/has-flag/-/has-flag-4.0.0.tgz", {}, "sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ=="], @@ -239,12 +247,16 @@ "prompts": ["prompts@2.4.2", "https://registry.npmmirror.com/prompts/-/prompts-2.4.2.tgz", { "dependencies": { "kleur": "^3.0.3", "sisteransi": "^1.0.5" } }, "sha512-NxNv/kLguCA7p3jE8oL2aEBsrJWgAakBpgmgK6lpPWV+WuOmY6r2/zbAVnP+T8bQlA0nzHXSJSJW0Hq7ylaD2Q=="], + "proper-lockfile": ["proper-lockfile@4.1.2", "https://registry.npmmirror.com/proper-lockfile/-/proper-lockfile-4.1.2.tgz", { "dependencies": { "graceful-fs": "^4.2.4", "retry": "^0.12.0", "signal-exit": "^3.0.2" } }, "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA=="], + "punycode": ["punycode@2.3.1", "https://registry.npmmirror.com/punycode/-/punycode-2.3.1.tgz", {}, "sha512-vYt7UD1U9Wg6138shLtLOvdAu+8DsC/ilFtEVHcH+wydcSpNE20AfSOduf6MkRFahL5FY7X1oU7nKVZFtfq8Fg=="], "queue-microtask": ["queue-microtask@1.2.3", "https://registry.npmmirror.com/queue-microtask/-/queue-microtask-1.2.3.tgz", {}, "sha512-NuaNSa6flKT5JaSYQzJok04JzTL1CA6aGhv5rfLW3PgqA+M2ChpZQnAC8h8i4ZFkBS8X5RqkDBHA7r4hej3K9A=="], "resolve-from": ["resolve-from@4.0.0", "https://registry.npmmirror.com/resolve-from/-/resolve-from-4.0.0.tgz", {}, "sha512-pb/MYmXstAkysRFx8piNI1tGFNQIFA3vkE3Gq4EuA1dF6gHp/+vgZqsCGJapvy8N3Q+4o7FwvquPJcnZ7RYy4g=="], + "retry": ["retry@0.12.0", "https://registry.npmmirror.com/retry/-/retry-0.12.0.tgz", {}, "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow=="], + "reusify": ["reusify@1.1.0", "https://registry.npmmirror.com/reusify/-/reusify-1.1.0.tgz", {}, "sha512-g6QUff04oZpHs0eG5p83rFLhHeV00ug/Yf9nZM6fLeUrPguBTkTQOdpAWWspMh55TZfVQDPaN3NQJfbVRAxdIw=="], "rimraf": ["rimraf@3.0.2", "https://registry.npmmirror.com/rimraf/-/rimraf-3.0.2.tgz", { "dependencies": { "glob": "^7.1.3" }, "bin": { "rimraf": "bin.js" } }, "sha512-JZkJMZkAGFFPP2YqXZXPbMlMBgsxzE8ILs4lMIX/2o0L9UBw9O/Y3o6wFw/i9YLapcUJWwqbi3kdxIPdC62TIA=="], @@ -257,6 +269,8 @@ "shebang-regex": ["shebang-regex@3.0.0", "https://registry.npmmirror.com/shebang-regex/-/shebang-regex-3.0.0.tgz", {}, "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A=="], + "signal-exit": ["signal-exit@3.0.7", "https://registry.npmmirror.com/signal-exit/-/signal-exit-3.0.7.tgz", {}, "sha512-wnD2ZE+l+SPC/uoS0vXeE9L1+0wuaMqKlfz9AMUo38JsyLSBWSFcHR1Rri62LZc12vLr1gb3jl7iwQhgwpAbGQ=="], + "sisteransi": ["sisteransi@1.0.5", "https://registry.npmmirror.com/sisteransi/-/sisteransi-1.0.5.tgz", {}, "sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg=="], "slash": ["slash@3.0.0", "https://registry.npmmirror.com/slash/-/slash-3.0.0.tgz", {}, "sha512-g9Q1haeby36OSStwb4ntCGGGaKsaVSjQ68fBxoQcutl5fS1vuY18H3wSt3jFyFtrkx+Kz0V1G85A4MyAdDMi2Q=="], diff --git a/cli/package.json b/cli/package.json index 8e4105a3..d9f8b650 100644 --- a/cli/package.json +++ b/cli/package.json @@ -48,12 +48,14 @@ "cac": "^6.7.14", "fflate": "^0.8.2", "prompts": "^2.4.2", + "proper-lockfile": "4.1.2", "semver": "^7.6.3", "zod": "^3.24.1" }, "devDependencies": { "@types/bun": "^1.3.13", "@types/prompts": "^2.4.9", + "@types/proper-lockfile": "4.1.4", "@types/semver": "^7.5.8", "@typescript-eslint/eslint-plugin": "^7.18.0", "@typescript-eslint/parser": "^7.18.0", diff --git a/cli/src/services/skill-target-lock.ts b/cli/src/services/skill-target-lock.ts index f449def1..540202b2 100644 --- a/cli/src/services/skill-target-lock.ts +++ b/cli/src/services/skill-target-lock.ts @@ -1,7 +1,7 @@ -import { createHash, randomUUID } from 'node:crypto' -import { open, readFile, rename, rm } from 'node:fs/promises' +import { createHash } from 'node:crypto' import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' +import { lock } from 'proper-lockfile' import { ensureDir } from '../platform/paths' import { CliError } from '../shared/errors' import { EXIT } from '../shared/constants' @@ -9,61 +9,19 @@ import { EXIT } from '../shared/constants' /** Serializes every local lifecycle mutation for one Skill target directory. */ export async function acquireSkillTargetLock(rootDir: string, slug: string): Promise<() => Promise> { const lockPath = await skillTargetLockPath(rootDir, slug) - const ownershipToken = randomUUID() - - const createLock = async () => { - const handle = await open(lockPath, 'wx') - try { - await handle.writeFile(JSON.stringify({ - pid: process.pid, - ownershipToken, - createdAt: new Date().toISOString() - })) - return handle - } catch (error) { - await handle.close().catch(() => {}) - await removeOwnedLock(lockPath, ownershipToken) - throw error + try { + return await lock(lockPath, { + lockfilePath: lockPath, + realpath: false, + stale: 10_000, + update: 3_000, + retries: 0 + }) + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ELOCKED') { + throw targetBusyError(rootDir, slug) } - } - - let handle: Awaited> | null = null - for (let attempt = 0; attempt < 3 && !handle; attempt++) { - try { - handle = await createLock() - break - } catch (error) { - if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error - - let ownerIsRunning = true - try { - const lock = JSON.parse(await readFile(lockPath, 'utf-8')) as { pid?: unknown } - if (typeof lock.pid !== 'number') throw new Error('invalid Skill target lock') - process.kill(lock.pid, 0) - } catch (lockError) { - if (lockError instanceof Error && 'code' in lockError && lockError.code === 'ESRCH') { - ownerIsRunning = false - } - } - - if (ownerIsRunning) throw targetBusyError(rootDir, slug) - - const quarantinedPath = `${lockPath}.stale-${ownershipToken}` - try { - await rename(lockPath, quarantinedPath) - } catch (reclaimError) { - if (reclaimError instanceof Error && 'code' in reclaimError && reclaimError.code === 'ENOENT') continue - throw reclaimError - } - await rm(quarantinedPath, { force: true }).catch(() => {}) - } - } - if (!handle) throw targetBusyError(rootDir, slug) - const ownedHandle = handle - - return async () => { - await ownedHandle.close().catch(() => {}) - await removeOwnedLock(lockPath, ownershipToken) + throw error } } @@ -82,13 +40,3 @@ function targetBusyError(rootDir: string, slug: string): CliError { next: 'wait for the other SkillHub CLI process to finish and retry' }) } - -async function removeOwnedLock(lockPath: string, ownershipToken: string): Promise { - try { - const lock = JSON.parse(await readFile(lockPath, 'utf-8')) as { ownershipToken?: unknown } - if (lock.ownershipToken !== ownershipToken) return - await rm(lockPath, { force: true }) - } catch { - // Missing or replaced locks are no longer owned by this caller. - } -} diff --git a/cli/test/helpers/target-lock-worker.ts b/cli/test/helpers/target-lock-worker.ts new file mode 100644 index 00000000..911863f5 --- /dev/null +++ b/cli/test/helpers/target-lock-worker.ts @@ -0,0 +1,19 @@ +import { acquireSkillTargetLock } from '../../src/services/skill-target-lock' +import { CliError } from '../../src/shared/errors' + +const [rootDir, slug, holdMilliseconds] = process.argv.slice(2) +if (!rootDir || !slug || !holdMilliseconds) process.exit(5) + +try { + const release = await acquireSkillTargetLock(rootDir, slug) + process.stdout.write('acquired\n') + await new Promise(resolve => setTimeout(resolve, Number(holdMilliseconds))) + await release() + process.exit(0) +} catch (error) { + if (error instanceof CliError) { + process.stderr.write(`${error.message}\n`) + process.exit(error.exitCode) + } + throw error +} diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 9cc257d9..8c4816a7 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -1,4 +1,4 @@ -import { access, mkdir, mkdtemp, readFile, readdir, rm, symlink, writeFile } from 'node:fs/promises' +import { access, mkdir, mkdtemp, readFile, readdir, rm, symlink, utimes, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' @@ -407,7 +407,7 @@ describe('installSkill', () => { const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) const lockPath = await skillTargetLockPath(rootDir, 'demo') - await writeFile(lockPath, JSON.stringify({ pid: process.pid })) + await mkdir(lockPath) await expect(installSkill({ registry: 'http://registry.test', namespace: 'global', slug: 'demo', @@ -416,7 +416,10 @@ describe('installSkill', () => { })).rejects.toThrow('install target is busy') expect(await exists(join(rootDir, 'demo'))).toBe(false) - await writeFile(lockPath, JSON.stringify({ pid: 999999 })) + await rm(lockPath, { recursive: true }) + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) await installSkill({ registry: 'http://registry.test', namespace: 'global', slug: 'demo', targets: [{ agent: 'codex', rootDir, scope: 'project', source: 'explicit' }], diff --git a/cli/test/unit/services/skill-target-lock.test.ts b/cli/test/unit/services/skill-target-lock.test.ts index 3f677f79..558d7141 100644 --- a/cli/test/unit/services/skill-target-lock.test.ts +++ b/cli/test/unit/services/skill-target-lock.test.ts @@ -1,9 +1,9 @@ -import { access, writeFile } from 'node:fs/promises' -import { mkdtemp } from 'node:fs/promises' +import { access, mkdir, mkdtemp, utimes } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { fileURLToPath } from 'node:url' import { describe, expect, test } from 'bun:test' -import { acquireSkillTargetLock, skillTargetLockPath } from '../../../src/services/skill-target-lock' +import { skillTargetLockPath } from '../../../src/services/skill-target-lock' async function exists(path: string): Promise { try { @@ -15,24 +15,28 @@ async function exists(path: string): Promise { } describe('skill target lifecycle lock', () => { - test('simultaneous stale recovery admits exactly one owner', async () => { + test('simultaneous stale recovery admits exactly one owner across processes', async () => { const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-target-lock-root-')) const lockPath = await skillTargetLockPath(rootDir, 'demo') - await writeFile(lockPath, JSON.stringify({ pid: 2_147_483_647, createdAt: '2026-09-01T00:00:00Z' })) + await mkdir(lockPath) + const staleTime = new Date(Date.now() - 60_000) + await utimes(lockPath, staleTime, staleTime) + const worker = fileURLToPath(new URL('../../helpers/target-lock-worker.ts', import.meta.url)) + const bunPath = (await Bun.which('bun')) ?? process.execPath - const outcomes = await Promise.allSettled([ - acquireSkillTargetLock(rootDir, 'demo'), - acquireSkillTargetLock(rootDir, 'demo') - ]) - const winners = outcomes.filter( - (outcome): outcome is PromiseFulfilledResult<() => Promise> => outcome.status === 'fulfilled' - ) - const losers = outcomes.filter(outcome => outcome.status === 'rejected') + const processes = [0, 1].map(() => Bun.spawn({ + cmd: [bunPath, worker, rootDir, 'demo', '1000'], + stdout: 'pipe', + stderr: 'pipe' + })) + const results = await Promise.all(processes.map(async process => ({ + exitCode: await process.exited, + stdout: (await new Response(process.stdout).text()).trim(), + stderr: (await new Response(process.stderr).text()).trim() + }))) - expect(winners).toHaveLength(1) - expect(losers).toHaveLength(1) - expect(await exists(lockPath)).toBe(true) - await winners[0]!.value() + expect(results.map(result => result.exitCode).sort()).toEqual([0, 4]) + expect(results.filter(result => result.stdout === 'acquired')).toHaveLength(1) expect(await exists(lockPath)).toBe(false) }) })