diff --git a/cli/src/services/skill-target-lock.ts b/cli/src/services/skill-target-lock.ts index d1440180..f449def1 100644 --- a/cli/src/services/skill-target-lock.ts +++ b/cli/src/services/skill-target-lock.ts @@ -1,53 +1,94 @@ -import { open, readFile, rm } from 'node:fs/promises' -import { join } from 'node:path' +import { createHash, randomUUID } from 'node:crypto' +import { open, readFile, rename, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { ensureDir } from '../platform/paths' import { CliError } from '../shared/errors' 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 = join(rootDir, `.${slug}.skillhub-install.lock`) + 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, createdAt: new Date().toISOString() })) + await handle.writeFile(JSON.stringify({ + pid: process.pid, + ownershipToken, + createdAt: new Date().toISOString() + })) return handle } catch (error) { await handle.close().catch(() => {}) - await rm(lockPath, { force: true }).catch(() => {}) + await removeOwnedLock(lockPath, ownershipToken) throw error } } - let handle: Awaited> - try { - handle = await createLock() - } catch (error) { - if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error - - let ownerIsRunning = true + let handle: Awaited> | null = null + for (let attempt = 0; attempt < 3 && !handle; attempt++) { 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 - } - } + handle = await createLock() + break + } catch (error) { + if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error - if (ownerIsRunning) { - throw new CliError(`install target is busy: ${join(rootDir, slug)}`, EXIT.filesystem, { - path: join(rootDir, slug), - next: 'wait for the other SkillHub CLI process to finish and retry' - }) + 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(() => {}) } - await rm(lockPath, { force: true }) - handle = await createLock() } + if (!handle) throw targetBusyError(rootDir, slug) + const ownedHandle = handle return async () => { - await handle.close().catch(() => {}) - await rm(lockPath, { force: true }).catch(() => {}) + await ownedHandle.close().catch(() => {}) + await removeOwnedLock(lockPath, ownershipToken) + } +} + +export async function skillTargetLockPath(rootDir: string, slug: string): Promise { + const target = resolve(rootDir, slug) + const digest = createHash('sha256').update(target).digest('hex') + const uid = typeof process.getuid === 'function' ? process.getuid() : 'user' + const lockDir = join(tmpdir(), `skillhub-cli-target-locks-${uid}`) + await ensureDir(lockDir) + return join(lockDir, `${digest}.lock`) +} + +function targetBusyError(rootDir: string, slug: string): CliError { + return new CliError(`install target is busy: ${join(rootDir, slug)}`, EXIT.filesystem, { + path: join(rootDir, slug), + 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/integration/version-upgrade-flow.test.ts b/cli/test/integration/version-upgrade-flow.test.ts index 75cc761e..56705f61 100644 --- a/cli/test/integration/version-upgrade-flow.test.ts +++ b/cli/test/integration/version-upgrade-flow.test.ts @@ -37,23 +37,25 @@ describe('version upgrade flow', () => { // ------------------------------------------------------------------------- // VU1 — full upgrade lifecycle: // 1. Registry serves pdf-parser@1.0.0 → install → metadata=v1, content=v1 - // 2. Stop registry, start a new one serving pdf-parser@2.0.0 - // 3. Install --force using the new registry URL + // 2. The same registry serves pdf-parser@2.0.0 + // 3. Install --force from the same source identity // 4. metadata.json, inventory.json AND on-disk SKILL.md all reflect v2 // ------------------------------------------------------------------------- test('VU1 install v1 then upgrade to v2 with --force replaces metadata, inventory, and content', async () => { const env = await createTempHome() // --- Stage 1: install v1 ---------------------------------------------- + const skill = { + namespace: 'global', + slug: 'pdf-parser', + version: '1.0.0', + versionId: 1, + zipBytes: makeSkillZipWithBody('# pdf-parser v1\n\nVersion one body.') + } registry = await startFakeRegistry({ token: 'sk_ok', user: { handle: 'u1', displayName: 'User One' }, - skills: [{ - namespace: 'global', - slug: 'pdf-parser', - version: '1.0.0', - zipBytes: makeSkillZipWithBody('# pdf-parser v1\n\nVersion one body.') - }] + skills: [skill] }) await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) @@ -77,22 +79,10 @@ describe('version upgrade flow', () => { expect(body).toContain('Version one body.') } - // --- Stage 2: swap registry to v2 ------------------------------------- - registry.stop() - registry = await startFakeRegistry({ - token: 'sk_ok', - user: { handle: 'u1', displayName: 'User One' }, - skills: [{ - namespace: 'global', - slug: 'pdf-parser', - version: '2.0.0', - zipBytes: makeSkillZipWithBody('# pdf-parser v2\n\nVersion two body.') - }] - }) - - // Re-login against the new registry (URL changed, so credentials are - // keyed differently). - await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home }) + // --- Stage 2: publish v2 from the same source -------------------------- + skill.version = '2.0.0' + skill.versionId = 2 + skill.zipBytes = makeSkillZipWithBody('# pdf-parser v2\n\nVersion two body.') const r2 = await runCli( ['install', 'pdf-parser', '--dir', installDir, '--registry', registry.url, '--token', 'sk_ok', '--force'], diff --git a/cli/test/unit/services/install-service.test.ts b/cli/test/unit/services/install-service.test.ts index 66904e19..9cc257d9 100644 --- a/cli/test/unit/services/install-service.test.ts +++ b/cli/test/unit/services/install-service.test.ts @@ -4,6 +4,7 @@ import { join } from 'node:path' import { afterEach, describe, expect, test } from 'bun:test' import { zipSync } from 'fflate' import { installSkill } from '../../../src/services/install-service' +import { skillTargetLockPath } from '../../../src/services/skill-target-lock' const originalFetch = globalThis.fetch @@ -327,7 +328,7 @@ describe('installSkill', () => { const metadata = JSON.parse(await readFile(join(skillDir, '.skillhub', 'metadata.json'), 'utf-8')) expect(metadata.registry).toBe('http://other-registry.test') expect((await readdir(rootDir)).some(name => name.includes('skillhub-backup'))).toBe(false) - expect(await exists(join(rootDir, '.demo.skillhub-install.lock'))).toBe(false) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) expect(await exists(join(home, '.skillhub', 'inventory.json'))).toBe(false) }) @@ -397,7 +398,7 @@ describe('installSkill', () => { for (const rootDir of [firstRoot, secondRoot]) { const entries = await readdir(rootDir) expect(entries.some(name => name.includes('skillhub-backup'))).toBe(false) - expect(entries.some(name => name.includes('skillhub-install.lock'))).toBe(false) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) } }) @@ -405,7 +406,7 @@ describe('installSkill', () => { globalThis.fetch = installFetch({ 'SKILL.md': '# New' }) const home = await mkdtemp(join(tmpdir(), 'skillhub-install-home-')) const rootDir = await mkdtemp(join(tmpdir(), 'skillhub-install-root-')) - const lockPath = join(rootDir, '.demo.skillhub-install.lock') + const lockPath = await skillTargetLockPath(rootDir, 'demo') await writeFile(lockPath, JSON.stringify({ pid: process.pid })) await expect(installSkill({ diff --git a/cli/test/unit/services/remove-service.test.ts b/cli/test/unit/services/remove-service.test.ts index 3eeec099..6885eb31 100644 --- a/cli/test/unit/services/remove-service.test.ts +++ b/cli/test/unit/services/remove-service.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, test } from 'bun:test' import { removeLocalSkill } from '../../../src/services/remove-service' -import { acquireSkillTargetLock } from '../../../src/services/skill-target-lock' +import { acquireSkillTargetLock, skillTargetLockPath } from '../../../src/services/skill-target-lock' import { InventoryStore } from '../../../src/stores/inventory-store' async function exists(path: string): Promise { @@ -123,7 +123,31 @@ describe('removeLocalSkill', () => { expect(removed.removed).toHaveLength(1) expect(await exists(skillDir)).toBe(false) expect((await store.read()).items).toEqual([]) - expect(await exists(join(rootDir, '.demo.skillhub-install.lock'))).toBe(false) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) + }) + + test('removes a stale inventory target when the recorded root directory is missing', async () => { + const home = await mkdtemp(join(tmpdir(), 'skillhub-remove-stale-home-')) + const parent = await mkdtemp(join(tmpdir(), 'skillhub-remove-stale-parent-')) + const rootDir = join(parent, 'missing-root') + const skillDir = join(rootDir, 'demo') + const store = new InventoryStore(home) + await store.write({ + items: [{ + registry: 'https://skill.xfyun.cn', + namespace: 'global', + slug: 'demo', + version: '1.0.0', + targets: [{ agent: 'codex', rootDir, installDir: skillDir, installedAt: '2026-09-01T00:00:00Z' }] + }] + }) + + const result = await removeLocalSkill({ registry: 'https://skill.xfyun.cn', slug: 'demo', home }) + + expect(result.removed).toEqual([{ namespace: 'global', agent: 'codex', dir: skillDir, existed: false }]) + expect(await exists(rootDir)).toBe(false) + expect((await store.read()).items).toEqual([]) + expect(await exists(await skillTargetLockPath(rootDir, 'demo'))).toBe(false) }) test('throws on path traversal in installDir', async () => { diff --git a/cli/test/unit/services/skill-target-lock.test.ts b/cli/test/unit/services/skill-target-lock.test.ts new file mode 100644 index 00000000..3f677f79 --- /dev/null +++ b/cli/test/unit/services/skill-target-lock.test.ts @@ -0,0 +1,38 @@ +import { access, writeFile } from 'node:fs/promises' +import { mkdtemp } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, test } from 'bun:test' +import { acquireSkillTargetLock, skillTargetLockPath } from '../../../src/services/skill-target-lock' + +async function exists(path: string): Promise { + try { + await access(path) + return true + } catch { + return false + } +} + +describe('skill target lifecycle lock', () => { + test('simultaneous stale recovery admits exactly one owner', 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' })) + + 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') + + expect(winners).toHaveLength(1) + expect(losers).toHaveLength(1) + expect(await exists(lockPath)).toBe(true) + await winners[0]!.value() + expect(await exists(lockPath)).toBe(false) + }) +})