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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCredential writes and deletions now complete asynchronously under a filesystem lock. Command and test callers await these operations, credential concurrency coverage is expanded, Vitest uses conditional global build setup, and TTY ticker tests force ANSI output. ChangesCredentials and test infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuthCommand
participant writeProfile
participant withCredentialsLock
participant CredentialsFile
AuthCommand->>writeProfile: persist profile
writeProfile->>withCredentialsLock: execute locked update
withCredentialsLock->>CredentialsFile: create lock and update credentials
CredentialsFile-->>withCredentialsLock: completed write
withCredentialsLock-->>writeProfile: resolve promise
writeProfile-->>AuthCommand: continue after persistence
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
|
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! |
Resolve conflicts and align with latest main: - vitest.config: keep globalSetup + hermetic setupFiles + fileParallelism - credentials: guard lock release with ownership token check - cli.subprocess: drop unused execFileSync import after globalSetup Closes TestSprite#109
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/config.test.ts`:
- Around line 89-109: Mark both tests covering whitespace TESTSPRITE_API_URL and
TESTSPRITE_API_KEY as async, and await writeProfile before calling loadConfig.
Preserve the existing environment inputs and assertions.
🪄 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: 3341ffbb-96ab-49ee-be0d-baa3a8fced37
📒 Files selected for processing (8)
src/commands/auth.test.tssrc/commands/auth.tssrc/lib/config.test.tssrc/lib/credentials.test.tssrc/lib/credentials.tstest/cli.subprocess.test.tstest/help.snapshot.test.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- vitest.config.ts
- test/help.snapshot.test.ts
- src/commands/auth.ts
- test/cli.subprocess.test.ts
- src/lib/credentials.ts
- src/commands/auth.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/config.test.ts`:
- Around line 89-109: Mark both tests covering whitespace TESTSPRITE_API_URL and
TESTSPRITE_API_KEY as async, and await writeProfile before calling loadConfig.
Preserve the existing environment inputs and assertions.
🪄 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: 3341ffbb-96ab-49ee-be0d-baa3a8fced37
📒 Files selected for processing (8)
src/commands/auth.test.tssrc/commands/auth.tssrc/lib/config.test.tssrc/lib/credentials.test.tssrc/lib/credentials.tstest/cli.subprocess.test.tstest/help.snapshot.test.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- vitest.config.ts
- test/help.snapshot.test.ts
- src/commands/auth.ts
- test/cli.subprocess.test.ts
- src/lib/credentials.ts
- src/commands/auth.test.ts
🛑 Comments failed to post (1)
src/lib/config.test.ts (1)
89-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Awaiting
writeProfileis required for asynchronous profile writes.The
writeProfilefunction is now asynchronous. Because the tests at lines 89 and 102 are not markedasyncand do notawait writeProfile(...), the configuration loading executes before the credentials file is actually written, which causes the assertions to fail (as seen in the pipeline failures).💚 Proposed fix to await asynchronous profile writes
- it('treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)', () => { - writeProfile( + it('treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)', async () => { + await writeProfile( 'default', { apiKey: 'sk-file', apiUrl: 'https://api.example.com:8443' }, { path: credentialsPath }, ); const config = loadConfig({ env: { TESTSPRITE_API_URL: ' ' }, credentialsPath, }); expect(config.apiUrl).toBe('https://api.example.com:8443'); }); - it('treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)', () => { - writeProfile('default', { apiKey: 'sk-file' }, { path: credentialsPath }); + it('treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)', async () => { + await writeProfile('default', { apiKey: 'sk-file' }, { path: credentialsPath }); const config = loadConfig({ env: { TESTSPRITE_API_KEY: '' }, credentialsPath, }); expect(config.apiKey).toBe('sk-file'); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.it('treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)', async () => { await writeProfile( 'default', { apiKey: 'sk-file', apiUrl: 'https://api.example.com:8443' }, { path: credentialsPath }, ); const config = loadConfig({ env: { TESTSPRITE_API_URL: ' ' }, credentialsPath, }); expect(config.apiUrl).toBe('https://api.example.com:8443'); }); it('treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)', async () => { await writeProfile('default', { apiKey: 'sk-file' }, { path: credentialsPath }); const config = loadConfig({ env: { TESTSPRITE_API_KEY: '' }, credentialsPath, }); expect(config.apiKey).toBe('sk-file'); });🧰 Tools
🪛 GitHub Actions: CI / 4_Unit Tests (Node 20).txt
[error] 99-99: loadConfig: expected TESTSPRITE_API_URL containing only whitespace to be treated as unset; apiUrl should be 'https://api.example.com:8443', but was 'https://api.testsprite.com'.
[error] 108-108: loadConfig: expected TESTSPRITE_API_KEY containing only whitespace to be treated as unset; apiKey should be 'sk-file', but was undefined.
🪛 GitHub Actions: CI / Unit Tests (Node 20)
[error] 99-99: loadConfig: treats empty/whitespace TESTSPRITE_API_URL as unset; expected apiUrl to be "https://api.example.com:8443" but received "https://api.testsprite.com"
[error] 108-108: loadConfig: treats empty/whitespace TESTSPRITE_API_KEY as unset; expected apiKey to be "sk-file" but received undefined
🪛 GitHub Actions: Test Coverage / 0_Coverage (= 80%).txt
[error] 99-99: loadConfig: treats empty/whitespace TESTSPRITE_API_URL as unset; expected apiUrl "https://api.example.com:8443" but got "https://api.testsprite.com".
[error] 108-108: loadConfig: treats empty/whitespace TESTSPRITE_API_KEY as unset; expected apiKey "sk-file" but got undefined.
🪛 GitHub Actions: Test Coverage / Coverage (= 80%)
[error] 99-99: loadConfig: expected empty/whitespace TESTSPRITE_API_URL to be treated as unset and fall through to profile value "https://api.example.com:8443", but received "https://api.testsprite.com".
[error] 108-108: loadConfig: expected empty/whitespace TESTSPRITE_API_KEY to be treated as unset and fall through to profile value "sk-file", but received undefined.
🪛 GitHub Check: Coverage (>= 80%)
[failure] 108-108: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)
AssertionError: expected undefined to be 'sk-file' // Object.is equality
- Expected:
"sk-file"
- Received:
undefined❯ src/lib/config.test.ts:108:27
[failure] 99-99: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)
AssertionError: expected 'https://api.testsprite.com' to be 'https://api.example.com:8443' // Object.is equalityExpected: "https://api.example.com:8443"
Received: "https://api.testsprite.com"❯ src/lib/config.test.ts:99:27
🪛 GitHub Check: Unit Tests (Node 20)
[failure] 108-108: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)
AssertionError: expected undefined to be 'sk-file' // Object.is equality
- Expected:
"sk-file"
- Received:
undefined❯ src/lib/config.test.ts:108:27
[failure] 99-99: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)
AssertionError: expected 'https://api.testsprite.com' to be 'https://api.example.com:8443' // Object.is equalityExpected: "https://api.example.com:8443"
Received: "https://api.testsprite.com"❯ src/lib/config.test.ts:99:27
🪛 GitHub Check: Unit Tests (Node 22)
[failure] 108-108: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_KEY as unset (falls through to profile)
AssertionError: expected undefined to be 'sk-file' // Object.is equality
- Expected:
"sk-file"
- Received:
undefined❯ src/lib/config.test.ts:108:27
[failure] 99-99: src/lib/config.test.ts > loadConfig > treats empty / whitespace TESTSPRITE_API_URL as unset (falls through to profile)
AssertionError: expected 'https://api.testsprite.com' to be 'https://api.example.com:8443' // Object.is equalityExpected: "https://api.example.com:8443"
Received: "https://api.testsprite.com"❯ src/lib/config.test.ts:99:27
🤖 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/config.test.ts` around lines 89 - 109, Mark both tests covering whitespace TESTSPRITE_API_URL and TESTSPRITE_API_KEY as async, and await writeProfile before calling loadConfig. Preserve the existing environment inputs and assertions.Source: Pipeline failures
- await writeProfile in config and doctor unit tests - pass noColor=false in ticker TTY tests (CI sets NO_COLOR=1) Closes TestSprite#77
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
Solving Issue #109