Serialize OAuth profile-store read-modify-write updates

[TheGreatAxios]

Summary

• Serialize concurrent read-modify-write operations in createAuthStore with an exclusive file lock, preventing concurrent CLI sessions from dropping profiles or clobbering fresher tokens
• Apply a stale-lock threshold (10s) below the lock timeout (15s) so crash-recovered lock files are reclaimed without first hitting a timeout
• Same lock covers OAuth, xAI, and Codex stores since they all delegate to createAuthStore

Verification

• bun run typecheck, bun run build, and bun run test pass (5901 tests, 0 failures)
• Regression tests cover concurrent saveProfile, concurrent updateTokens, concurrent removeProfile, stale-lock recovery, and cross-home independence

Fixes CL-6713

[linear-code]

CL-6713

[TheGreatAxios]

Review · Request changes

OAuth profile mutations use a filesystem lock to serialize read-modify-write updates.

Findings

• src/auth/oauth/store.ts:140 — stale recovery deletes a lock solely from its age, so a live owner that runs longer than 10 seconds loses mutual exclusion. A second writer can enter, and the first owner then unconditionally removes the replacement lock at src/auth/oauth/store.ts:162, admitting further writers and allowing profiles or refreshed tokens to be overwritten.

Notes

• bun run check exits 0; lint, typecheck, build, and the full test suite complete successfully.
• GitHub prevents the authenticated PR author from submitting a formal request-changes review, so this verdict is posted as a PR comment.

[TheGreatAxios]

Critic · Comment

The new tests exercise common concurrent profile-store operations and stale-lock recovery.

Findings

• src/auth/oauth/oauth.test.ts:200 — the token concurrency test checks only profile names and token field types, so it still passes if saveProfile("a") overwrites the newer b1 tokens with seeded b0; could this assert that profile b retains b1?
• src/auth/oauth/oauth.test.ts:301 — awaiting saves to two homes proves both finish but does not prove they avoid contention, because a single global lock would also pass; could this use a barrier or timing-independent held-lock assertion to demonstrate isolation?

[TheGreatAxios]

Implementation update

The lock implementation now uses a per-home exclusive descriptor held through each read-modify-write operation and times out after 15 seconds without reclaiming stale or ambiguous lock files. This removes stale-owner, replacement-lock, recovery-claim, and metadata-write cleanup races.

Findings addressed

• Exact newer b1 token preservation is asserted.
• Home isolation holds home A’s lock while home B writes.
• Critical-section failure cleanup exercises the real store lock.
• Old-lock exclusivity is tested with bounded timing.
• Lock release checks inode and owner token before unlinking.

Verification

• bun test src/auth/oauth/oauth.test.ts — 26 passed, 0 failed.
• bun run typecheck — exit 0.
• bun run check — exit 0; 5903 tests passed, 0 failed across 418 files.
• Final commit: 3be5864.

[TheGreatAxios]

Review · Blocking

The OAuth profile store serializes read-modify-write operations with a filesystem lock.

Findings

• src/auth/oauth/store.ts:140 — A lock is removed solely because its mtime exceeds 10 seconds. If a live holder is suspended or delayed past that threshold, another process deletes its lock and enters the critical section; the original holder can then delete the replacement lock during cleanup. Could we use ownership-safe stale-lock recovery so a live holder cannot be displaced?
• src/auth/oauth/oauth.test.ts:200 — The concurrent token-update assertion checks only field types, so restoring seeded token values still passes. Could the regression assert the expected refreshed token values?

GitHub CI is green, but the live-lock displacement can reintroduce overlapping writes, so this should be resolved before merge.

[TheGreatAxios]

Review · Blocking

The current owner-token fix handles ordinary replacement cleanup, but stale reclamation still has a compare/unlink race.

• src/auth/oauth/store.ts:95 — Two reclaimers can both validate the old inode/content. One unlinks and acquires a replacement, then the second performs its already-approved pathname unlink and removes that replacement. Both can subsequently enter the profile-store critical section. The test at src/auth/oauth/oauth.test.ts:335 replaces the lock before validation, so it does not exercise this interleaving.

Could we use an atomic lock shape whose stale cleanup cannot remove a recreated owner—for example an owner file inside an exclusively-created lock directory, where cleanup unlinks the owner and rmdir cannot remove a replacement’s non-empty directory—and add a coordinated two-reclaimer regression?

[TheGreatAxios]

Closing this implementation rather than continuing the lock-protocol patch cycle. CL-6713 remains a valid concurrency problem, but this branch's user-space stale-lock recovery has accumulated crash and reclamation races and is no longer a safe or reviewable direction. No additional dependency will be added. The issue is returning to backlog for a fresh design from the read-modify-write invariant.