[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]

Claude finished @mzieniukbw's task —— View job

---

Code Review Summary

I've analyzed the PR that fixes a race condition where auto keys weren't being stored due to vault timeout being null during read operations. The solution correctly addresses the issue by filtering out null values from the observable stream.

---

PR Documentation Quality

The PR documentation is excellent:

• Title: Clear and specific with Jira reference
• Objective: Well-explained - describes both the problem (race condition) and the solution (filter for non-nullability)
• Technical details: Good explanation of the race condition mechanism and why it happens
• Test coverage: Mentions comprehensive testing across multiple scenarios

---

Findings

Finding 1: The core fix correctly addresses the race condition by filtering null vault timeout values

Details: The change in key.service.ts:692-694 adds a filter((timeout) => timeout != null) operator to the observable stream. This is the correct approach because:

• getVaultTimeoutByUserId$ writes the default vault timeout as a side effect when it's null
• The observable is "hot", so firstValueFrom could capture null before the write completes
• Filtering ensures we wait for the actual non-null value
• Since vault timeout always has a non-null default for all clients (as stated in PR description), this won't cause indefinite waiting

Reference: libs/key-management/src/key.service.ts:691-695

---

Finding 2: The tap to concatMap change improves async handling but could use clarification

Details: In vault-timeout-settings.service.ts:139-145, the code changed from tap() to concatMap() to properly await the state write operation. This is a good change because:

• tap() doesn't wait for async operations to complete
• concatMap() properly handles the async state write and ensures it completes before the next value is emitted
• The explicit return vaultTimeout at the end clarifies the emitted value

However, the comment on line 140 says "As a side effect" but concatMap is designed for handling async operations that affect the stream - it's not really a "side effect" anymore. Consider updating the comment to reflect that this is now part of the main observable chain.

Suggestion: Update comment from "As a side effect, set..." to "Set the new value determined by determineVaultTimeout into state if it's different from the current"

Reference: libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts:139-145

---

Finding 3: Test improvements appropriately initialize vault timeout state

Details: Both test files now properly initialize VAULT_TIMEOUT state in beforeEach:

• key.service.spec.ts sets it to VaultTimeoutStringType.Never (line 77)
• electron-key.service.spec.ts sets it to 10 (line 50)

These changes ensure tests don't encounter the same race condition the PR is fixing. The values chosen are appropriate for each context:

• Never for the main key service tests (testing auto key storage)
• 10 minutes for the electron tests (testing non-auto scenarios)

Minor: The electron test cleanup removed nested describe blocks and redundant tests, which improves test clarity and removes duplication.

References:

• libs/key-management/src/key.service.spec.ts:77
• apps/desktop/src/key-management/electron-key.service.spec.ts:50

---

Finding 4: Export of VAULT_TIMEOUT is appropriately documented but exposes internal state

Details: The PR exports VAULT_TIMEOUT from the vault-timeout index file with a clear comment explaining it's "Only used by desktop's electron-key.service.spec.ts test" (line 12).

While the comment is helpful, exposing internal state definitions for tests is generally not ideal. Consider whether the test could use a different approach (e.g., mocking or a test utility) rather than importing internal state definitions.

However, this is a minor architectural concern and doesn't impact the correctness of the fix.

Reference: libs/common/src/key-management/vault-timeout/index.ts:12-13

---

Security Considerations

✅ No security concerns identified

• The PR correctly handles vault timeout state without exposing sensitive data
• No vault data is logged or exposed
• The filter operation doesn't introduce any security vulnerabilities
• Changes align with the CLAUDE.md security requirements

---

Architecture & Best Practices

✅ Observable pattern correctly implemented

• The filter() operator is the right RxJS approach for this scenario
• concatMap() properly handles async operations in the observable chain
• No explicit subscriptions were added (maintains ADR-0003 compliance)

✅ Test coverage maintained

• Tests properly initialize required state
• Test cleanup removes redundant nested describe blocks

---

Summary

This is a well-executed fix for a race condition bug. The core solution (filtering null values) is correct and addresses the root cause. The PR includes appropriate test updates and good documentation. The only minor suggestion is to update the comment in vault-timeout-settings.service.ts to better reflect that the operation is now part of the main observable chain rather than a "side effect."

Overall assessment: Approve with minor comment improvement suggestion

---

[github-actions]

Checkmarx One – Scan Summary & Details – fcfc4a9b-ea89-48e4-b4a7-10f29e365b3a

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