From 6a3ec3ae845830491857bcfdc2d6b062cb435a42 Mon Sep 17 00:00:00 2001 From: SahilRakhaiya05 Date: Thu, 25 Jun 2026 12:20:10 +0530 Subject: [PATCH 1/2] fix(credentials): serialize profile writes with cross-process file locking writeProfile and deleteProfile read-modify-write the credentials file; without a lock, concurrent CLI processes can each read the same snapshot and the last atomic rename wins, silently dropping the other update. Acquire an exclusive lock file ({path}.lock) before read-modify-write, with stale-lock reclamation and a bounded retry loop. Add subprocess regression tests proving concurrent writes to different profiles both survive. --- src/lib/credentials.test.ts | 65 ++++++++++++- src/lib/credentials.ts | 119 ++++++++++++++++++++--- test/helpers/credentials-write-child.mjs | 12 +++ 3 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 test/helpers/credentials-write-child.mjs diff --git a/src/lib/credentials.test.ts b/src/lib/credentials.test.ts index 8be03af..d67d73f 100644 --- a/src/lib/credentials.test.ts +++ b/src/lib/credentials.test.ts @@ -1,7 +1,11 @@ +import { execSync } from 'node:child_process'; +import type { ChildProcess } from 'node:child_process'; +import { spawn } from 'node:child_process'; import { mkdtempSync, statSync, readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { fileURLToPath } from 'node:url'; +import { afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { DEFAULT_PROFILE, defaultCredentialsPath, @@ -168,3 +172,62 @@ describe('defaultCredentialsPath', () => { expect(defaultCredentialsPath().endsWith('/.testsprite/credentials')).toBe(true); }); }); + +const projectRoot = fileURLToPath(new URL('../..', import.meta.url)); +const credentialsWriteChild = join(projectRoot, 'test/helpers/credentials-write-child.mjs'); + +function waitForChild(child: ChildProcess): Promise { + return new Promise((resolve, reject) => { + let stderr = ''; + child.stderr?.on('data', chunk => { + stderr += String(chunk); + }); + child.on('error', reject); + child.on('close', code => { + if (code === 0) resolve(); + else reject(new Error(`child exited with code ${code}: ${stderr}`)); + }); + }); +} + +function spawnCredentialsWriter(profile: string, apiKey: string, path: string): ChildProcess { + return spawn(process.execPath, [credentialsWriteChild], { + env: { + ...process.env, + CRED_PROFILE: profile, + CRED_PATH: path, + CRED_API_KEY: apiKey, + }, + stdio: ['ignore', 'ignore', 'pipe'], + }); +} + +describe('credentials write lock', () => { + beforeAll(() => { + execSync('npm run build', { cwd: projectRoot, stdio: 'pipe' }); + }); + + it('serializes cross-process writes so concurrent profile updates are not lost', async () => { + const children = [ + spawnCredentialsWriter('dev', 'sk-dev', credentialsPath), + spawnCredentialsWriter('staging', 'sk-staging', credentialsPath), + ]; + await Promise.all(children.map(waitForChild)); + + const file = readCredentialsFile({ path: credentialsPath }); + expect(file.dev).toEqual({ apiKey: 'sk-dev' }); + expect(file.staging).toEqual({ apiKey: 'sk-staging' }); + }); + + it('removes the lock file after writeProfile completes', () => { + writeProfile('default', { apiKey: 'sk-lock-cleanup' }, { path: credentialsPath }); + expect(existsSync(`${credentialsPath}.lock`)).toBe(false); + }); + + it('removes the lock file after deleteProfile completes', () => { + writeProfile('default', { apiKey: 'sk-d' }, { path: credentialsPath }); + writeProfile('dev', { apiKey: 'sk-dev' }, { path: credentialsPath }); + expect(deleteProfile('dev', { path: credentialsPath })).toBe(true); + expect(existsSync(`${credentialsPath}.lock`)).toBe(false); + }); +}); diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index c59de98..661d3bd 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -1,10 +1,13 @@ import { chmodSync, + closeSync, existsSync, mkdirSync, + openSync, readFileSync, renameSync, statSync, + unlinkSync, writeFileSync, } from 'node:fs'; import { homedir } from 'node:os'; @@ -109,22 +112,26 @@ export function writeProfile( options: CredentialsOptions = {}, ): void { const path = resolvePath(options); - const file = readCredentialsFile(options); - file[profile] = { ...file[profile], ...entry }; - writeCredentialsAtomic(path, file); + withCredentialsLock(path, () => { + const file = readCredentialsFile(options); + file[profile] = { ...file[profile], ...entry }; + writeCredentialsAtomic(path, file); + }); } export function deleteProfile(profile: string, options: CredentialsOptions = {}): boolean { const path = resolvePath(options); - const file = readCredentialsFile(options); - if (!(profile in file)) return false; - delete file[profile]; - if (Object.keys(file).length === 0) { - writeCredentialsAtomic(path, {}); - } else { - writeCredentialsAtomic(path, file); - } - return true; + return withCredentialsLock(path, () => { + const file = readCredentialsFile(options); + if (!(profile in file)) return false; + delete file[profile]; + if (Object.keys(file).length === 0) { + writeCredentialsAtomic(path, {}); + } else { + writeCredentialsAtomic(path, file); + } + return true; + }); } export function ensureRestrictiveMode(path: string): void { @@ -144,3 +151,91 @@ function writeCredentialsAtomic(path: string, file: CredentialsFile): void { renameSync(tmp, path); ensureRestrictiveMode(path); } + +/** Max wall-clock wait when another process holds the credentials lock. */ +const CREDENTIALS_LOCK_MAX_WAIT_MS = 10_000; +/** Back-off between lock attempts. */ +const CREDENTIALS_LOCK_RETRY_MS = 25; +/** Reclaim a lock file when the holder pid is gone or the file is older than this. */ +const CREDENTIALS_LOCK_STALE_MS = 30_000; + +function credentialsLockPath(credentialsPath: string): string { + return `${credentialsPath}.lock`; +} + +/** + * Serialize read-modify-write on the credentials file across processes. + * `writeCredentialsAtomic` only makes the final rename atomic; without this + * lock, concurrent `writeProfile` / `deleteProfile` calls can each read the + * same snapshot and the last rename wins — silently dropping the other update. + */ +function withCredentialsLock(credentialsPath: string, fn: () => T): T { + acquireCredentialsLock(credentialsPath); + try { + return fn(); + } finally { + releaseCredentialsLock(credentialsPath); + } +} + +function acquireCredentialsLock(credentialsPath: string): void { + const lockPath = credentialsLockPath(credentialsPath); + const deadline = Date.now() + CREDENTIALS_LOCK_MAX_WAIT_MS; + while (Date.now() < deadline) { + try { + const fd = openSync(lockPath, 'wx'); + try { + writeFileSync(fd, `${process.pid}\n${Date.now()}\n`, 'utf8'); + } finally { + closeSync(fd); + } + return; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== 'EEXIST') throw err; + if (isStaleCredentialsLock(lockPath)) { + try { + unlinkSync(lockPath); + } catch { + // Another waiter may have claimed or released the lock. + } + continue; + } + syncSleep(CREDENTIALS_LOCK_RETRY_MS); + } + } + throw new Error(`Timed out acquiring credentials lock: ${lockPath}`); +} + +function releaseCredentialsLock(credentialsPath: string): void { + try { + unlinkSync(credentialsLockPath(credentialsPath)); + } catch { + // Lock already released or never acquired — teardown must not mask errors. + } +} + +function isStaleCredentialsLock(lockPath: string): boolean { + try { + const stat = statSync(lockPath); + if (Date.now() - stat.mtimeMs > CREDENTIALS_LOCK_STALE_MS) return true; + const firstLine = readFileSync(lockPath, 'utf8').split('\n')[0] ?? ''; + const pid = Number.parseInt(firstLine, 10); + if (!Number.isFinite(pid) || pid <= 0) return true; + try { + process.kill(pid, 0); + return false; + } catch { + return true; + } + } catch { + return false; + } +} + +function syncSleep(ms: number): void { + const until = Date.now() + ms; + while (Date.now() < until) { + // Busy-wait: credentials I/O is sync-only; sub-ms precision is unnecessary. + } +} diff --git a/test/helpers/credentials-write-child.mjs b/test/helpers/credentials-write-child.mjs new file mode 100644 index 0000000..c142a9b --- /dev/null +++ b/test/helpers/credentials-write-child.mjs @@ -0,0 +1,12 @@ +import { writeProfile } from '../../dist/lib/credentials.js'; + +const profile = process.env.CRED_PROFILE; +const credentialsPath = process.env.CRED_PATH; +const apiKey = process.env.CRED_API_KEY; + +if (!profile || !credentialsPath || !apiKey) { + console.error('CRED_PROFILE, CRED_PATH, and CRED_API_KEY are required'); + process.exit(1); +} + +writeProfile(profile, { apiKey }, { path: credentialsPath }); \ No newline at end of file From f607eaf11e93076a3ffccf4fdb3d1d3d420e4c62 Mon Sep 17 00:00:00 2001 From: SahilRakhaiya05 Date: Fri, 26 Jun 2026 15:34:35 +0530 Subject: [PATCH 2/2] fix(credentials): mkdir before lock file; satisfy Prettier acquireCredentialsLock now creates the credentials directory before openSync on the lock path, so first-time setup on a fresh HOME no longer throws ENOENT (fixes subprocess setup/auth tests). Add trailing newline to credentials-write-child.mjs for format:check. --- src/lib/credentials.ts | 3 +++ test/helpers/credentials-write-child.mjs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/lib/credentials.ts b/src/lib/credentials.ts index 07ec4b2..07db962 100644 --- a/src/lib/credentials.ts +++ b/src/lib/credentials.ts @@ -217,6 +217,9 @@ function withCredentialsLock(credentialsPath: string, fn: () => T): T { function acquireCredentialsLock(credentialsPath: string): void { const lockPath = credentialsLockPath(credentialsPath); + // Ensure the credentials directory exists before creating the lock file. + // writeCredentialsAtomic also mkdirs, but only after the lock is held. + mkdirSync(dirname(lockPath), { recursive: true, mode: 0o700 }); const deadline = Date.now() + CREDENTIALS_LOCK_MAX_WAIT_MS; while (Date.now() < deadline) { try { diff --git a/test/helpers/credentials-write-child.mjs b/test/helpers/credentials-write-child.mjs index c142a9b..c5b561b 100644 --- a/test/helpers/credentials-write-child.mjs +++ b/test/helpers/credentials-write-child.mjs @@ -9,4 +9,4 @@ if (!profile || !credentialsPath || !apiKey) { process.exit(1); } -writeProfile(profile, { apiKey }, { path: credentialsPath }); \ No newline at end of file +writeProfile(profile, { apiKey }, { path: credentialsPath });