diff --git a/src/commands.ts b/src/commands.ts index 8433a21..0fb9b79 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -7,7 +7,14 @@ import kleur from 'kleur'; import { parseUserRef, fetchProfile, readManifest } from './fetch.js'; import { plan, printPlan, isExecutable, applyProfile } from './plan.js'; import { restoreBackup, listBackups, restoreBackupToStamp, parseApplied } from './backup.js'; -import { recordInstall, readInstalled, list, isImmutableRef } from './state.js'; +import { + recordInstall, + readInstalled, + list, + isImmutableRef, + acquireLock, + releaseLock, +} from './state.js'; import { scanForSecrets, printAndGateFindings, type Finding } from './scanner.js'; import { walk, tildify, Dirs, DEFAULT_DIRS, cp, rootsFor } from './paths.js'; @@ -158,6 +165,8 @@ export async function update( opts?: InstallOpts, dirs: Dirs = DEFAULT_DIRS ): Promise { + const lockPath = path.join(dirs.state, '.lock'); + acquireLock(lockPath); try { const includeHooks = opts?.includeHooks ?? false; const yes = opts?.yes ?? false; @@ -248,10 +257,13 @@ export async function update( ); console.log(kleur.dim(` Undo: sharekit rollback ${user}\n`)); } finally { + releaseLock(lockPath); } } export async function install(user: string, opts?: InstallOpts): Promise { + const lockPath = path.join(DEFAULT_DIRS.state, '.lock'); + acquireLock(lockPath); try { const includeHooks = opts?.includeHooks ?? false; const { user: userName, ref: userRef } = parseUserRef(user); @@ -316,6 +328,7 @@ export async function install(user: string, opts?: InstallOpts): Promise { } console.log(); } finally { + releaseLock(lockPath); } } @@ -366,68 +379,74 @@ export async function inspect(user: string): Promise { export async function rollback(user: string, opts?: InstallOpts): Promise { const HOME = os.homedir(); const STATE = path.join(HOME, '.sharekit'); + const lockPath = path.join(STATE, '.lock'); - const yes = opts?.yes ?? false; - const dryRun = opts?.dryRun ?? false; + acquireLock(lockPath); + try { + const yes = opts?.yes ?? false; + const dryRun = opts?.dryRun ?? false; - const root = path.join(STATE, 'backups'); - const last = fs.existsSync(root) - ? fs - .readdirSync(root) - .filter((e) => e.startsWith(user + '-')) - .sort() - .pop() - : undefined; - if (!last) return void console.log(kleur.yellow(`No backup for ${user}.`)); + const root = path.join(STATE, 'backups'); + const last = fs.existsSync(root) + ? fs + .readdirSync(root) + .filter((e) => e.startsWith(user + '-')) + .sort() + .pop() + : undefined; + if (!last) return void console.log(kleur.yellow(`No backup for ${user}.`)); + + const dir = path.join(root, last); + + // Read and parse applied.json with safe error handling + let applied: { dest: string; status: string }[]; + const appliedPath = path.join(dir, 'applied.json'); + try { + const rawData = fs.readFileSync(appliedPath, 'utf8'); + applied = parseApplied(rawData); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + throw new Error(`Backup data is corrupt or unreadable: ${msg}`); + } - const dir = path.join(root, last); + let versionStr = ''; + const metadataPath = path.join(dir, 'metadata.json'); + if (fs.existsSync(metadataPath)) { + try { + const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf8')); + if (metadata.sourceVersion) versionStr = ` (v${metadata.sourceVersion})`; + } catch { + // If metadata can't be read, just continue without version info + } + } - // Read and parse applied.json with safe error handling - let applied: { dest: string; status: string }[]; - const appliedPath = path.join(dir, 'applied.json'); - try { - const rawData = fs.readFileSync(appliedPath, 'utf8'); - applied = parseApplied(rawData); - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - throw new Error(`Backup data is corrupt or unreadable: ${msg}`); - } + console.log(kleur.bold(`\n Rollback ${user}${versionStr} (${applied.length} file(s))\n`)); + if (!(await confirm('Restore?', yes))) return void console.log(kleur.dim('\n Aborted.\n')); - let versionStr = ''; - const metadataPath = path.join(dir, 'metadata.json'); - if (fs.existsSync(metadataPath)) { - try { - const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf8')); - if (metadata.sourceVersion) versionStr = ` (v${metadata.sourceVersion})`; - } catch { - // If metadata can't be read, just continue without version info + if (dryRun) { + console.log(kleur.cyan(`\n (dry-run — no files restored)`)); + console.log(kleur.green(`\n ✓ Would restore ${applied.length} file(s).`)); + console.log(); + return; } - } - console.log(kleur.bold(`\n Rollback ${user}${versionStr} (${applied.length} file(s))\n`)); - if (!(await confirm('Restore?', yes))) return void console.log(kleur.dim('\n Aborted.\n')); + const metadata = restoreBackup(user); + const summary = `${metadata.filesRestored} file(s) restored${ + metadata.filesRemoved > 0 ? `, ${metadata.filesRemoved} removed` : '' + }`; + // Handle null sourceCommit (offline cache case) + let versionSuffix = ''; + if (metadata.sourceCommit === null) { + versionSuffix = ' — from offline cache, exact version unknown'; + } else if (metadata.sourceVersion) { + versionSuffix = ` (reverted to v${metadata.sourceVersion})`; + } - if (dryRun) { - console.log(kleur.cyan(`\n (dry-run — no files restored)`)); - console.log(kleur.green(`\n ✓ Would restore ${applied.length} file(s).`)); + console.log(kleur.green(`\n ✓ ${summary}${versionSuffix}`)); console.log(); - return; - } - - const metadata = restoreBackup(user); - const summary = `${metadata.filesRestored} file(s) restored${ - metadata.filesRemoved > 0 ? `, ${metadata.filesRemoved} removed` : '' - }`; - // Handle null sourceCommit (offline cache case) - let versionSuffix = ''; - if (metadata.sourceCommit === null) { - versionSuffix = ' — from offline cache, exact version unknown'; - } else if (metadata.sourceVersion) { - versionSuffix = ` (reverted to v${metadata.sourceVersion})`; + } finally { + releaseLock(lockPath); } - - console.log(kleur.green(`\n ✓ ${summary}${versionSuffix}`)); - console.log(); } export async function uninstall( @@ -435,103 +454,111 @@ export async function uninstall( dirs: Dirs = DEFAULT_DIRS, force = false ): Promise { - const installed = readInstalled(dirs); - const record = installed[user]; + const lockPath = path.join(dirs.state, '.lock'); + acquireLock(lockPath); + try { + const installed = readInstalled(dirs); + const record = installed[user]; - if (!record) { - throw new Error(`${user} is not installed.`); - } + if (!record) { + throw new Error(`${user} is not installed.`); + } - // Find the latest backup for this user - const root = path.join(dirs.state, 'backups'); - const last = fs.existsSync(root) - ? fs - .readdirSync(root) - .filter((e) => e.startsWith(user + '-')) - .sort() - .pop() - : undefined; - - if (!last) { - throw new Error(`No backup found for ${user}. Cannot uninstall without restore information.`); - } + // Find the latest backup for this user + const root = path.join(dirs.state, 'backups'); + const last = fs.existsSync(root) + ? fs + .readdirSync(root) + .filter((e) => e.startsWith(user + '-')) + .sort() + .pop() + : undefined; + + if (!last) { + throw new Error(`No backup found for ${user}. Cannot uninstall without restore information.`); + } - const backupDir = path.join(root, last); + const backupDir = path.join(root, last); - // Read and parse applied.json with safe error handling - let applied: { dest: string; status: string }[]; - const appliedPath = path.join(backupDir, 'applied.json'); - try { - const rawData = fs.readFileSync(appliedPath, 'utf8'); - applied = parseApplied(rawData); - } catch (error) { - const msg = error instanceof Error ? error.message : String(error); - throw new Error(`Backup data is corrupt or unreadable: ${msg}`); - } + // Read and parse applied.json with safe error handling + let applied: { dest: string; status: string }[]; + const appliedPath = path.join(backupDir, 'applied.json'); + try { + const rawData = fs.readFileSync(appliedPath, 'utf8'); + applied = parseApplied(rawData); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + throw new Error(`Backup data is corrupt or unreadable: ${msg}`); + } - // Print what will be removed/restored - const toRemove = applied.filter((a) => a.status === 'new'); - const toRestore = applied.filter((a) => a.status === 'changed'); + // Print what will be removed/restored + const toRemove = applied.filter((a) => a.status === 'new'); + const toRestore = applied.filter((a) => a.status === 'changed'); - console.log(); - console.log(kleur.bold(` Uninstall ${user}${record.version ? ` (v${record.version})` : ''}\n`)); + console.log(); + console.log( + kleur.bold(` Uninstall ${user}${record.version ? ` (v${record.version})` : ''}\n`) + ); - if (toRemove.length > 0) { - console.log(kleur.red(` - remove (${toRemove.length})`)); - for (const a of toRemove) { - console.log(kleur.red(` ${tildify(a.dest)}`)); + if (toRemove.length > 0) { + console.log(kleur.red(` - remove (${toRemove.length})`)); + for (const a of toRemove) { + console.log(kleur.red(` ${tildify(a.dest)}`)); + } } - } - if (toRestore.length > 0) { - console.log(kleur.yellow(`\n ~ restore (${toRestore.length})`)); - for (const a of toRestore) { - console.log(kleur.yellow(` ${tildify(a.dest)}`)); + if (toRestore.length > 0) { + console.log(kleur.yellow(`\n ~ restore (${toRestore.length})`)); + for (const a of toRestore) { + console.log(kleur.yellow(` ${tildify(a.dest)}`)); + } } - } - console.log(); - if (!force && !(await confirm(`Remove ${user}?`))) { - return void console.log(kleur.dim('\n Aborted.\n')); - } + console.log(); + if (!force && !(await confirm(`Remove ${user}?`))) { + return void console.log(kleur.dim('\n Aborted.\n')); + } - // Resolve home directory once for bounds checking - const resolvedHome = path.resolve(dirs.home); + // Resolve home directory once for bounds checking + const resolvedHome = path.resolve(dirs.home); - // Execute the uninstall: reverse all changes - for (const a of applied) { - // Bounds-check: ensure dest is within dirs.home - const resolvedDest = path.resolve(a.dest); - if (!resolvedDest.startsWith(resolvedHome + path.sep)) { - console.warn(`Skipping out-of-bounds entry: ${a.dest}`); - continue; - } + // Execute the uninstall: reverse all changes + for (const a of applied) { + // Bounds-check: ensure dest is within dirs.home + const resolvedDest = path.resolve(a.dest); + if (!resolvedDest.startsWith(resolvedHome + path.sep)) { + console.warn(`Skipping out-of-bounds entry: ${a.dest}`); + continue; + } - if (a.status === 'new') { - // File was added by the profile — remove it - fs.rmSync(resolvedDest, { force: true }); - } else if (a.status === 'changed') { - // File was changed — restore from backup - const src = path.join(backupDir, path.relative(resolvedHome, resolvedDest)); - if (fs.existsSync(src)) { - fs.mkdirSync(path.dirname(resolvedDest), { recursive: true }); - cp(src, resolvedDest); + if (a.status === 'new') { + // File was added by the profile — remove it + fs.rmSync(resolvedDest, { force: true }); + } else if (a.status === 'changed') { + // File was changed — restore from backup + const src = path.join(backupDir, path.relative(resolvedHome, resolvedDest)); + if (fs.existsSync(src)) { + fs.mkdirSync(path.dirname(resolvedDest), { recursive: true }); + cp(src, resolvedDest); + } } } - } - // Remove user from installed.json atomically (#122) - delete installed[user]; - const stateFile = path.join(dirs.state, 'installed.json'); - const tmp = stateFile + '.tmp'; - fs.writeFileSync(tmp, JSON.stringify(installed, null, 2)); - fs.renameSync(tmp, stateFile); - - const summary = `${toRemove.length} file(s) removed${ - toRestore.length > 0 ? `, ${toRestore.length} restored` : '' - }`; - console.log(kleur.green(`\n ✓ Uninstalled ${user}. ${summary}`)); - console.log(); + // Remove user from installed.json atomically (#122) + delete installed[user]; + const stateFile = path.join(dirs.state, 'installed.json'); + const tmp = stateFile + '.tmp'; + fs.writeFileSync(tmp, JSON.stringify(installed, null, 2)); + fs.renameSync(tmp, stateFile); + + const summary = `${toRemove.length} file(s) removed${ + toRestore.length > 0 ? `, ${toRestore.length} restored` : '' + }`; + console.log(kleur.green(`\n ✓ Uninstalled ${user}. ${summary}`)); + console.log(); + } finally { + releaseLock(lockPath); + } } export async function scan(dir?: string, force = false): Promise { diff --git a/src/sharekit.ts b/src/sharekit.ts index bf69cde..c81c3ac 100644 --- a/src/sharekit.ts +++ b/src/sharekit.ts @@ -39,7 +39,14 @@ export { export type { BackupInfo, RestoreMetadata } from './backup.js'; // From state.ts -export { recordInstall, readInstalled, list, isImmutableRef } from './state.js'; +export { + recordInstall, + readInstalled, + list, + isImmutableRef, + acquireLock, + releaseLock, +} from './state.js'; export type { InstallRecord } from './state.js'; // From commands.ts diff --git a/src/state.ts b/src/state.ts index c757a2a..c361fd0 100644 --- a/src/state.ts +++ b/src/state.ts @@ -97,23 +97,45 @@ export function isImmutableRef(ref: string): boolean { const LOCK_FILE = path.join(DEFAULT_DIRS.state, '.lock'); +/** + * Portable cross-platform PID liveness check using process.kill(pid, 0). + * Signal 0 doesn't send a signal; it only checks if the process exists and is accessible. + * - EPERM: process exists but we don't own it (return true — alive) + * - ESRCH: process doesn't exist (return false — dead) + * - Other errors: treat as dead + */ +function isPidAlive(pidStr: string): boolean { + const pid = Number(pidStr); + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (e) { + const code = (e as NodeJS.ErrnoException).code; + return code === 'EPERM'; // EPERM = alive but not owned by us; ESRCH = dead + } +} + export function acquireLock(lockPath: string = LOCK_FILE): void { const pid = process.pid.toString(); if (fs.existsSync(lockPath)) { try { const existingPid = fs.readFileSync(lockPath, 'utf8').trim(); - try { - fs.statSync(`/proc/${existingPid}`); - console.error( - `Another sharekit operation is running (PID ${existingPid}). Try again in a moment.` + if (isPidAlive(existingPid)) { + throw new Error( + `another sharekit process is running (PID ${existingPid}) — retry after it finishes, or delete ${lockPath} if stale` ); - process.exit(1); - } catch { - try { - fs.unlinkSync(lockPath); - } catch {} } - } catch { + // Process is dead — clean up stale lock + try { + fs.unlinkSync(lockPath); + } catch {} + } catch (e) { + // If checking liveness or reading the lock file fails, check if it's our error + if (e instanceof Error && e.message.includes('another sharekit process')) { + throw e; // Re-throw the "process still running" error + } + // Otherwise try to clean up the lock file try { fs.unlinkSync(lockPath); } catch {} diff --git a/test/lock.test.ts b/test/lock.test.ts new file mode 100644 index 0000000..ae52488 --- /dev/null +++ b/test/lock.test.ts @@ -0,0 +1,135 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { acquireLock, releaseLock } from '../src/state.js'; + +// Test 1: acquireLock throws when lock is held by a live process (portable across macOS/Linux) +test('acquireLock throws when another process holds the lock', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, '.lock'); + + // Write the current process's PID to the lock file (this process is definitely alive) + fs.mkdirSync(path.dirname(lockPath), { recursive: true }); + fs.writeFileSync(lockPath, process.pid.toString()); + + // Try to acquire the same lock — should fail + let errorThrown = false; + let errorMessage = ''; + try { + acquireLock(lockPath); + } catch (e) { + errorThrown = true; + errorMessage = e instanceof Error ? e.message : String(e); + } + + assert.ok(errorThrown, 'acquireLock should throw when lock is held by live process'); + assert.ok( + errorMessage.includes('another sharekit process is running'), + 'error should mention another process is running' + ); + assert.ok(errorMessage.includes(process.pid.toString()), 'error should include the PID'); + assert.ok(errorMessage.includes('retry'), 'error should suggest retrying'); + assert.ok(errorMessage.includes('delete'), 'error should suggest deleting the lock file'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +// Test 2: stale lock (dead PID) is cleaned and acquisition succeeds +test('acquireLock cleans up stale lock and acquires successfully', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, '.lock'); + + // Write a dead PID (very unlikely to exist) to the lock file + const deadPid = '999999999'; + fs.mkdirSync(path.dirname(lockPath), { recursive: true }); + fs.writeFileSync(lockPath, deadPid); + + // Try to acquire the lock — should succeed (cleaning up stale lock) + acquireLock(lockPath); + + // Verify that the lock file now contains the current process PID + assert.ok(fs.existsSync(lockPath), 'lock file should exist'); + const content = fs.readFileSync(lockPath, 'utf8').trim(); + assert.equal(content, process.pid.toString(), 'lock file should contain current PID'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +// Test 3: release lock removes the lock file +test('releaseLock removes the lock file', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, '.lock'); + + // Create a lock file + fs.mkdirSync(path.dirname(lockPath), { recursive: true }); + fs.writeFileSync(lockPath, process.pid.toString()); + assert.ok(fs.existsSync(lockPath), 'lock file should exist before release'); + + // Release the lock + releaseLock(lockPath); + + // Verify that the lock file is gone + assert.ok(!fs.existsSync(lockPath), 'lock file should be removed after release'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +// Test 4: releaseLock silently succeeds even if lock file doesn't exist +test('releaseLock succeeds even if lock file does not exist', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, '.lock'); + + // Don't create the lock file, just try to release + assert.doesNotThrow(() => { + releaseLock(lockPath); + }, 'releaseLock should not throw if lock file does not exist'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +// Test 5: acquireLock creates lock directory if it doesn't exist +test('acquireLock creates the lock directory if needed', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, 'subdir', '.lock'); + + // Don't create the directory beforehand + assert.ok(!fs.existsSync(path.dirname(lockPath)), 'lock directory should not exist yet'); + + // Acquire the lock + acquireLock(lockPath); + + // Verify that the directory and file were created + assert.ok(fs.existsSync(lockPath), 'lock file should be created'); + const content = fs.readFileSync(lockPath, 'utf8').trim(); + assert.equal(content, process.pid.toString(), 'lock file should contain current PID'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +// Test 6: acquireLock treats non-numeric PID as stale and cleans up +test('acquireLock treats non-numeric lock content as stale', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'sk-lock-')); + const lockPath = path.join(tmp, '.lock'); + + // Write non-numeric content (corrupted lock file) + fs.mkdirSync(path.dirname(lockPath), { recursive: true }); + fs.writeFileSync(lockPath, 'corrupted-pid-data'); + + // Try to acquire the lock — should succeed (cleaning up corrupted lock) + acquireLock(lockPath); + + // Verify that the lock file now contains the current process PID + assert.ok(fs.existsSync(lockPath), 'lock file should exist'); + const content = fs.readFileSync(lockPath, 'utf8').trim(); + assert.equal(content, process.pid.toString(), 'lock file should contain current PID'); + + // Cleanup + fs.rmSync(tmp, { recursive: true, force: true }); +});