fix(credentials): serialize profile writes with cross-process file locking#32
fix(credentials): serialize profile writes with cross-process file locking#32SahilRakhaiya05 wants to merge 8 commits into
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds inter-process locking for credential file writes and makes credential persistence awaited across commands and tests. CI and Vitest are updated to build before tests and avoid parallel file-worker races. ChangesCredentials Write Lock
Awaited Credential Persistence
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)
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
🤖 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 226-244: Fresh locks are being treated as stale in the lock
acquisition flow, which can let a second process reclaim a just-created lock
before its contents are fully written. Update the lock handling in the
credentials locking logic around the openSync/writeFileSync path and
isStaleCredentialsLock so only a lock that is definitively owned by a dead
process can be reclaimed; do not unlink or reuse a lock with missing or
incomplete PID/timestamp contents. Make the stale-lock check ownership-safe in
the retry loop that calls syncSleep, using the existing lockPath and
isStaleCredentialsLock helpers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8321e47c-d2e2-41b8-9a49-8664fd8edbea
📒 Files selected for processing (3)
src/lib/credentials.test.tssrc/lib/credentials.tstest/helpers/credentials-write-child.mjs
|
Thanks @SahilRakhaiya05 — strong work here. The cross-process test that spawns real child processes is exactly the rigor we want to see. 🙌 One thing to close before merge: the lock has a create-then-write TOCTOU. Once that's in, happy to merge. Thanks! 🚀 |
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.
SahilRakhaiya05
requested review from
ruili-testsprite and
zeshi-du
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 223-235: The lock acquisition in credentials.ts is using the wrong
primitive in the temp-file swap path around renameSync, which can overwrite an
existing lock and break mutual exclusion. Update the lock creation logic to use
an exclusive-create approach instead, such as openSync with the wx flag or
linkSync, and keep the temp-file cleanup and EEXIST handling in the same
lock-acquisition flow. Use the existing lockPath temp-file sequence in the
credentials lock helper to make the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a1e39dd-e983-4c03-b57e-a3e0b8033f66
📒 Files selected for processing (1)
src/lib/credentials.ts
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.
|
@zeshi-du Thanks for the detailed review I have updated all the requested changes. |
|
Substance looks right — the await new Promise(resolve => setTimeout(resolve, LOCK_RETRY_DELAY_MS));If there's a sync-context constraint I'm missing, |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/credentials.test.ts (1)
212-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the concurrency regression force overlap.
These children are spawned back-to-back, but one can still finish before the other reads the credentials file, so the test can pass without exercising the lost-update race. Add a small start barrier in
test/helpers/credentials-write-child.mjsbeforewriteProfile, then release both children together.🤖 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.test.ts` around lines 212 - 217, The concurrency regression test is not guaranteed to overlap the two writers, so it can pass without hitting the lost-update path. Update the credential write child helper, test/helpers/credentials-write-child.mjs, to wait on a small start barrier before calling writeProfile, then have src/lib/credentials.test.ts release both spawned children together in the serializes cross-process writes test so the concurrent profile updates actually race.src/lib/credentials.ts (1)
210-214: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAwait async callbacks before releasing the credentials lock.
withCredentialsLockshould acceptT | Promise<T>andreturn await fn()so an async caller can’t release the lock before its work finishes.🤖 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 210 - 214, withCredentialsLock currently releases the credentials lock before an async callback finishes because it returns fn() directly; update withCredentialsLock to accept T | Promise<T> and use return await fn() inside the try block so the lock is held until the callback completes. Keep the fix centered on withCredentialsLock and any callers that rely on it for serialized credentials access.
🤖 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.
Nitpick comments:
In `@src/lib/credentials.test.ts`:
- Around line 212-217: The concurrency regression test is not guaranteed to
overlap the two writers, so it can pass without hitting the lost-update path.
Update the credential write child helper,
test/helpers/credentials-write-child.mjs, to wait on a small start barrier
before calling writeProfile, then have src/lib/credentials.test.ts release both
spawned children together in the serializes cross-process writes test so the
concurrent profile updates actually race.
In `@src/lib/credentials.ts`:
- Around line 210-214: withCredentialsLock currently releases the credentials
lock before an async callback finishes because it returns fn() directly; update
withCredentialsLock to accept T | Promise<T> and use return await fn() inside
the try block so the lock is held until the callback completes. Keep the fix
centered on withCredentialsLock and any callers that rely on it for serialized
credentials access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d5a3c3a-0121-49ef-86ce-d38f2169eb2e
📒 Files selected for processing (7)
src/commands/auth.test.tssrc/commands/auth.tssrc/commands/usage.test.tssrc/lib/config.test.tssrc/lib/credentials.test.tssrc/lib/credentials.tstest/helpers/credentials-write-child.mjs
✅ Files skipped from review due to trivial changes (1)
- src/lib/config.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/helpers/credentials-write-child.mjs
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
Summary
Bug Fixes
configureandlogoutflows to ensure credential persistence/removal completes before reporting results.Tests
Chores
Solving Issue #109