fix: auth credential storage and transient error handling#81
Merged
Conversation
…lback Both stores now follow the same pattern: write to system keychain first, only write to file when keychain is unavailable, never proactively delete existing file backups. config-store: removed deleteFile() calls from saveConfig() and getConfig() migration path that caused environment configs to vanish after addon rebuilds or keychain access failures. credential-store: stopped always writing to file alongside keychain. File is now only written as fallback when keychain write fails.
Prevents race condition where two concurrent CLI processes both attempt to refresh the same token, causing one to get invalid_grant when the server rotates the refresh token. New file-lock utility uses atomic file creation (O_EXCL) with stale lock detection (30s timeout). New refreshAccessTokenSafe() wraps the existing refresh with lock-acquire-then-recheck pattern: after acquiring the lock, re-reads credentials in case another process already refreshed. Falls back to unlocked refresh if lock acquisition fails (degraded but functional). All callers (ensure-auth, background-refresh, credential-proxy) now use the safe variant.
This reverts commit 98095ab.
Only clear credentials on invalid_grant (refresh token permanently dead). Network and server errors are transient — preserve credentials so the user can retry without being forced to re-login. Previously, any refresh failure wiped the keychain, destroying a potentially valid refresh token over a temporary server hiccup.
- ensure-auth: collapse hasCredentials() + getCredentials() two-step into single getCredentials() call, eliminating double keyring read and fixing semantic mismatch (hasCredentials returns true for corrupt files, getCredentials returns null) - credential-store/config-store: add migrationAttempted flag to prevent repeated failed keyring writes on every read when keyring is unavailable (e.g., headless Linux with no keyring daemon)
The "(Session refreshed)" message is implementation detail noise for users. Moved to debug log, keeping only "Already logged in as ..." in user-facing output.
This was referenced Mar 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
config-store.tswas deleting~/.workos/config.jsonafter successful keychain migration, causing environment configs to vanish when keychain became temporarily inaccessible (e.g., native addon rebuild). File is now preserved as a fallback.credential-store.tswas always writing to file alongside keychain. Now only writes file when keychain fails, matching the intended keychain-primary pattern.ensure-auth.tswas callingclearCredentials()on network/server errors during token refresh, destroying a valid refresh token over temporary hiccups. Now only clears oninvalid_grant(permanently dead refresh token).Test plan
saveConfigwrites to keyring only when keyring available (no file created)saveConfigfalls back to file when keyring unavailablesaveCredentialssame keychain-primary behaviorgetConfigmigration to keyring preserves file backupinvalid_grantstill clears credentials (correct behavior)