Recovered: fix(credentials): serialize profile writes with cross-process file locking (#32 by @SahilRakhaiya05)#244
Conversation
…cking
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.
Resolve conflict in credentials.test.ts by keeping both the cross-process write-lock regression tests and the profile-name validation tests added in TestSprite#21.
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.
Write PID and timestamp to a temp file and renameSync into place, matching writeCredentialsAtomic. Prevents a contender from reclaiming an empty/partial lock during the old open-then-write window. Only reclaim locks with complete PID+timestamp content; treat incomplete locks as in-flight acquisition.
renameSync atomically replaces an existing lock file on POSIX, so two contending processes can overwrite lockPath and both enter the critical section. writeFileSync with flag 'wx' creates the lock with full PID and timestamp content in one syscall, preserving mutual exclusion without the open-then-write TOCTOU window.
Multiple suites (cli.subprocess, help.snapshot, credentials lock) each run npm run build in beforeAll. With fileParallelism enabled, workers can corrupt dist/ mid-run and subprocess smoke tests flake with exit 1. Run build once in CI before npm test and disable vitest file parallelism.
WalkthroughCredential persistence is now asynchronous and protected by an inter-process lock. Callers and tests await writes and deletions, while test infrastructure performs one conditional build before workers run. ChangesCredentials and test infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: lockfile failed supply-chain policy check. Run Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
49-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin workflow actions to immutable commit SHAs.
actions/checkout@v5andactions/setup-node@v5are mutable references. Replace them with reviewed full 40-character commit SHAs before merging.As per path instructions, all third-party actions must be pinned to a full 40-char commit SHA.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 49 - 58, Replace the mutable v5 references in the checkout and Setup Node.js workflow steps with reviewed, full 40-character immutable commit SHAs. Keep the existing action inputs and workflow behavior unchanged, and ensure both actions/checkout and actions/setup-node are pinned.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/credentials.ts`:
- Around line 219-252: Update acquireCredentialsLock and releaseCredentialsLock
to track the lock identity established by each successful acquisition, such as
its unique token or ownership metadata. Before unlinking in
releaseCredentialsLock, verify the lock still exists and matches this
acquisition’s ownership; skip removal when it was reclaimed or replaced by
another owner, while preserving teardown’s non-throwing behavior.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 49-58: Replace the mutable v5 references in the checkout and Setup
Node.js workflow steps with reviewed, full 40-character immutable commit SHAs.
Keep the existing action inputs and workflow behavior unchanged, and ensure both
actions/checkout and actions/setup-node are pinned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4851e85e-0cfc-49b6-96b3-29efd16fb62e
📒 Files selected for processing (12)
.github/workflows/ci.ymlsrc/commands/auth.test.tssrc/commands/auth.tssrc/commands/usage.test.tssrc/lib/config.test.tssrc/lib/credentials.test.tssrc/lib/credentials.tstest/cli.subprocess.test.tstest/global-setup.mjstest/help.snapshot.test.tstest/helpers/credentials-write-child.mjsvitest.config.ts
| async function acquireCredentialsLock(credentialsPath: string): Promise<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 { | ||
| writeFileSync(lockPath, `${process.pid}\n${Date.now()}\n`, { flag: 'wx', encoding: 'utf8' }); | ||
| 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; | ||
| } | ||
| await new Promise(resolve => setTimeout(resolve, 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. | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the locking helpers around the cited lines.
ast-grep outline src/lib/credentials.ts --view expanded || true
echo '---'
nl -ba src/lib/credentials.ts | sed -n '150,320p'
echo '---'
# Find all references to the lock helpers and stale-lock detection.
rg -n "acquireCredentialsLock|releaseCredentialsLock|isStaleCredentialsLock|credentialsLockPath|CREDENTIALS_LOCK_" src/lib/credentials.ts src -g '!**/dist/**' -g '!**/build/**'Repository: TestSprite/testsprite-cli
Length of output: 1859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,280p' src/lib/credentials.ts | cat -nRepository: TestSprite/testsprite-cli
Length of output: 3396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "withCredentialsLock\\(" src/lib/credentials.ts src -g '!**/dist/**' -g '!**/build/**'
echo '---'
sed -n '1,220p' src/lib/credentials.ts | cat -nRepository: TestSprite/testsprite-cli
Length of output: 9218
Guard lock release with ownership checks. isStaleCredentialsLock() can reclaim a lock on age alone, so a long-running holder can be preempted and later releaseCredentialsLock() may unlink a newer owner’s lock. Track ownership per acquisition and only unlink if this process still owns the lock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/credentials.ts` around lines 219 - 252, Update acquireCredentialsLock
and releaseCredentialsLock to track the lock identity established by each
successful acquisition, such as its unique token or ownership metadata. Before
unlinking in releaseCredentialsLock, verify the lock still exists and matches
this acquisition’s ownership; skip removal when it was reclaimed or replaced by
another owner, while preserving teardown’s non-throwing behavior.
|
Hi @SahilRakhaiya05 👋 — this is the recovery of your PR #32 , which was auto-closed by an error in our 2026-07-09 release process. Your commits are intact — thank you for the contribution! It currently shows merge conflicts because Option A — merge (no force-push): git checkout BRANCH
git fetch upstream # your remote for TestSprite/testsprite-cli (may be "origin")
git merge upstream/main # resolve conflicts, commit
git pushOption B — rebase (cleaner history, needs force): git checkout BRANCH
git fetch upstream
git rebase upstream/main # resolve conflicts
git push --force-with-leaseOnce you push, the conflicts here resolve automatically and we'll take it into review. Sorry again for the extra step! |
2 participants
Summary
writeProfile and deleteProfile perform read-modify-write on
~/.testsprite/credentials. The finalrenameis atomic, but the read and write are not one operation. Two concurrent CLI processes (e.g. two terminals runningtestsprite setupfor different profiles, orsetup+auth remove) can each read the same snapshot; the last rename wins and silently drops the other profile update - real credentials data loss.The fix
writeProfile/deleteProfileinwithCredentialsLock(){credentialsPath}.lockviaopenSync(..., 'wx')Tests
dev+stagingprofiles - both survivewriteProfile/deleteProfilecompleteNotes
npm teston Windows still has ~15 pre-existing failures covered by PR fix(test): restore full test suite on Windows #4; new credentials tests pass locallySummary by CodeRabbit
Bug Fixes
Tests
Chores
Solving Issue #109