fix(cli): make stale target lock recovery ownership-safe

Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>

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<void>> {
-  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<ReturnType<typeof open>>
-  try {
-    handle = await createLock()
-  } catch (error) {
-    if (!(error instanceof Error && 'code' in error && error.code === 'EEXIST')) throw error
-
-    let ownerIsRunning = true
+  let handle: Awaited<ReturnType<typeof open>> | 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<string> {
+  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<void> {
+  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.
   }
 }

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'],

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({

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<boolean> {
@@ -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 () => {

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<boolean> {
+  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<void>> => 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)
+  })
+})