[PM-26049] Auto key not stored due to vault timeout write vs read race condition for cli

[mzieniukbw]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26049

📔 Objective

Auto (user) key is (sometimes) not stored due to vault timeout being null. This happens due to race condition on the storage write vs read. The read is hot observable, so getting value via firstValueFrom might return null. Since vault timeout is not supposed to be null (since we always have a non-null default for all clients), adding a filter for non-nullability is enough to make sure the actual value is returned, before deciding whether to store the auto key.

Writing the vault timeout (default when not set) happens as a "side effect" of getVaultTimeoutByUserId$, which is not great, but since we require it (firstValueFrom) just before identity token request (before parsing it and setting user keys to state), the vault timeout is reliably stored in state for the first time when not set, during bw login and always before the auto key storage.

Tested login, unlock, sync, changing vault timeout in settings from and to Never on all clients, restarting app to see if it starts unlocked.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude[bot]]

Claude finished @mzieniukbw's task —— View job

---

Code Review Complete ✅

Summary

This PR effectively fixes a race condition where the auto (user) key was not being stored due to vault timeout being null when read via a hot observable. The solution is clean and well-tested.

---

Key Changes Reviewed

Core Fix (libs/key-management/src/key.service.ts:695-696)

• ✅ Added filter((timeout) => timeout != null) before firstValueFrom
• This ensures the observable waits for a non-null vault timeout value instead of immediately returning null from the hot observable
• This is the critical fix that prevents the race condition

Side Effect Improvement (vault-timeout-settings.service.ts:154)

• ✅ Changed from tap to concatMap with proper async/await
• Ensures the state write operation is properly awaited
• Maintains serialized execution order
• More correct than the previous tap usage which didn't guarantee the promise would be awaited

Test Updates

• ✅ Both test files properly initialize VAULT_TIMEOUT in beforeEach
• ✅ Simplified electron-key.service.spec.ts by removing unnecessary nested test structure
• ✅ Tests now pass consistently with the new non-null requirement

Export Addition (index.ts:14)

• ℹ️ Exports VAULT_TIMEOUT for test setup purposes
• Minor concern: Exposes internal state key, but acceptable for testing

---

Architecture Assessment

Why the fix works:

1. Hot observables emit their current value immediately to new subscribers
2. If vault timeout hasn't been written yet, that current value is null
3. firstValueFrom without filtering would immediately return that null
4. Adding filter((timeout) => timeout != null) makes firstValueFrom wait for the actual value
5. The vault timeout gets written as a "side effect" of getVaultTimeoutByUserId$ before the auto key storage decision

Code quality:

• ✅ Follows Observable Data Services pattern (ADR-0003)
• ✅ Proper RxJS operator usage
• ✅ Well-tested with appropriate setup
• ✅ No security concerns

---

Potential Improvements (Non-blocking)

1. Comment clarity: The comment in index.ts:14 could be more specific: "Only used by desktop's electron-key.service.spec.ts and libs/key-management/src/key.service.spec.ts"

2. Future refactoring: As noted in the code comments, the vault timeout being written as a "side effect" is not ideal. The TODO comment references PM-7082 for Auto Key Service work that should address this architectural concern.

---

Recommendation

Approve ✅

This is a solid fix for a race condition that was causing intermittent issues with auto key storage. The solution is minimal, focused, and properly tested. The change from tap to concatMap is a good improvement, and the addition of filter((timeout) => timeout != null) correctly addresses the root cause.

---

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 503ed571-5c46-42f5-9272-9b372f0c6847

Great job! No new security vulnerabilities introduced in this pull request

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 41.84%. Comparing base (7183d77) to head (d5267a0). :warning: Report is 24 commits behind head on main. :white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17707      +/-   ##
==========================================
- Coverage   41.84%   41.84%   -0.01%     
==========================================
  Files        3589     3589              
  Lines      104181   104185       +4     
  Branches    15712    15712              
==========================================
- Hits        43596    43592       -4     
- Misses      58737    58744       +7     
- Partials     1848     1849       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.