test: cover cross-process credential writes - #280
Conversation
WalkthroughAdds cross-process credential write coverage using configurable child-process coordination, validating failure handling, concurrent profile preservation, and credentials lock-file cleanup. ChangesCross-process credential writes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 `@test/credentials-cross-process.test.ts`:
- Around line 57-93: Add focused tests for the failure branches of
runCredentialWriter alongside the existing success test: omit a required
credential environment variable and assert exit code 1, and configure an
uncreated CRED_START_PATH with a shortened timeout and assert exit code 2. Reuse
the existing temporary-directory setup and child-result assertions, and cover
the writeProfile error path with exit code 3 if the helper exposes a
controllable failure trigger.
- Around line 52-55: Set an explicit timeout on the beforeAll build hook in the
“credentials cross-process writes” suite, long enough for execNpm(['run',
'build'], ...) to complete reliably. Keep the existing build command and test
behavior unchanged.
🪄 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: 9e8ce548-81fa-42cc-abfe-dfe9b90d615d
📒 Files selected for processing (2)
test/credentials-cross-process.test.tstest/helpers/credentials-write-child.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/credentials-cross-process.test.ts (1)
1-50: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake process timing and barrier coordination injectable.
The test suite relies on hard-coded wall-clock waits: the child helper waits 5 seconds,
runCredentialWriterhas a fixed kill timeout, and the race test uses a short delay before creating the marker. This makes the suite slower/flakier under load, and the delay does not guarantee every child reached the barrier.Expose timeout/sleep controls through test dependencies and use an explicit child-readiness handshake before releasing the marker.
As per path instructions, tests must be deterministic and offline: no real network and no real timers — inject fetch/sleep via the test deps.
Also applies to: 75-96, 123-163
🤖 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 `@test/credentials-cross-process.test.ts` around lines 1 - 50, Update the cross-process credential tests and runCredentialWriter to inject sleep, timeout, and fetch behavior through test dependencies instead of using hard-coded delays, kill timers, or real network access. Replace the race test’s marker delay with an explicit readiness handshake from every child before releasing the marker, and make the child helper’s barrier wait use the injected timing controls. Preserve the existing credential assertions while ensuring all synchronization is deterministic and timer-free.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.
Outside diff comments:
In `@test/credentials-cross-process.test.ts`:
- Around line 1-50: Update the cross-process credential tests and
runCredentialWriter to inject sleep, timeout, and fetch behavior through test
dependencies instead of using hard-coded delays, kill timers, or real network
access. Replace the race test’s marker delay with an explicit readiness
handshake from every child before releasing the marker, and make the child
helper’s barrier wait use the injected timing controls. Preserve the existing
credential assertions while ensuring all synchronization is deterministic and
timer-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e51b8517-0b23-40f2-8e70-ce7a9eb4f7f5
📒 Files selected for processing (1)
test/credentials-cross-process.test.ts
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 `@test/credentials-cross-process.test.ts`:
- Around line 52-62: Update waitForFiles to accept injectable clock and sleep
dependencies instead of directly calling Date.now and setTimeout, while
preserving its polling and timeout behavior. Update the readiness and
timeout-path tests to provide controlled fake time and sleep advancement,
removing reliance on real timer delays and child-process scheduling.
🪄 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: 74e8ad9d-6b8c-4330-8839-8235f42961b6
📒 Files selected for processing (2)
test/credentials-cross-process.test.tstest/helpers/credentials-write-child.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/helpers/credentials-write-child.mjs
| async function waitForFiles(paths: string[], timeoutMs = 5_000): Promise<void> { | ||
| const deadline = Date.now() + timeoutMs; | ||
|
|
||
| while (true) { | ||
| const missing = paths.filter(path => !existsSync(path)); | ||
| if (missing.length === 0) return; | ||
| if (Date.now() >= deadline) { | ||
| throw new Error(`Timed out waiting for files: ${missing.join(', ')}`); | ||
| } | ||
| await new Promise(resolveDelay => setTimeout(resolveDelay, 5)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Inject time control instead of polling wall-clock time.
waitForFiles directly uses Date.now()/setTimeout, while the timeout-path test depends on real 1 ms/100 ms child timing. Inject clock/sleep dependencies so readiness and timeout cases can run under controlled time rather than CI scheduling.
As per path instructions, “Tests must be deterministic and offline: No real network and no real timers — inject fetch/sleep via the test deps.”
Also applies to: 97-99
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@test/credentials-cross-process.test.ts` around lines 52 - 62, Update
waitForFiles to accept injectable clock and sleep dependencies instead of
directly calling Date.now and setTimeout, while preserving its polling and
timeout behavior. Update the readiness and timeout-path tests to provide
controlled fake time and sleep advancement, removing reliance on real timer
delays and child-process scheduling.
Source: Path instructions
1 participant
Closes #279.
#272 put a lock around credential profile mutations. Nothing in the suite proves it works across processes — which is the only place the bug it fixes can happen. This adds that regression test.
What is actually being protected
writeProfileis read-modify-write on a shared file (src/lib/credentials.ts:178):mutateCredentialsFilereads the whole credentials file, hands it to the mutator, and writes the whole thing back (credentials.ts:265). Two processes interleaving that sequence is a classic lost update: both read the same starting state, each adds its own profile, and the secondrenameSyncoverwrites the first. The profile does not fail to write — it silently disappears, and the user finds out later when a command that needs it reportsAUTH_REQUIRED.This is not hypothetical usage. Two shells, a CI job overlapping a local
testsprite setup, or an agent running commands in parallel all hit it, and every one of those is a separate process — so no in-process mutex would have helped.The guard is an
O_EXCLlock file (credentials.ts:289):writeFileSync(lockPath, …, { flag: 'wx' })succeeds for exactly one process, everyone else spins untilCREDENTIALS_LOCK_WAIT_MS(5 s), with stale-lock reclaim and anassertHeld()token re-check immediately before the write in case a reclaim stole ownership mid-flight.Every one of those branches is cross-process by construction. A single-process test cannot reach any of them.
How the test creates real contention
test/credentials-cross-process.test.tsspawns 8 real child processes —spawn(process.execPath, [CHILD_PATH]), not workers or mockedfs— each writing a distinct profile to one shared credentials file.The hard part is that spawning 8 processes staggers them by tens of milliseconds, which is longer than the critical section. They would queue politely and the test would pass without ever exercising the lock. So the children are held at a starting gate:
<profile>.readymarker, then spin-waits onexistsSync(startPath).startfile, releasing all 8 into the critical section together.The child sleeps between polls with
Atomics.waiton a throwawaySharedArrayBufferrather than a timer, so the wait is a genuine blocking sleep with no event-loop scheduling in the path.Children import from
dist/lib/credentials.js, so the test exercises the built artifact users actually run.beforeAllrunsnpm run buildfor that reason.Assertions
0with empty stdout and empty stderr — a lock timeout or a failedassertHeld()would surface here as a non-zero exit.apiKey. This is the lost-update assertion: under the pre-fix(auth): lock credential profile mutations #272 behaviour, profiles go missing.${credentialsPath}.lockno longer exists — thefinallyrelease ran, so the next writer is not blocked for 5 s by an orphan lock.Three tests covering the harness itself
A concurrency test that silently stops testing anything is worse than no test, so the child contract is pinned by its exit codes:
12Timed out waiting for start marker: <path>writeProfilerejects (bad]profile name)3Invalid request.Without these, a typo in an env var name would make all 8 children exit early and the main test would pass green while asserting nothing.
Notes
src/changes, so nothing ships to users from this PR.mkdtempSyncroot and removes it infinally; nothing touches the real credentials path.Validation
All CI checks are currently green on this branch.
Summary by CodeRabbit