Fix missing task cancellation and legacy keychain fingerprint mismatch#499
Open
nikhil8182 wants to merge 1 commit intosteipete:mainfrom
Open
Fix missing task cancellation and legacy keychain fingerprint mismatch#499nikhil8182 wants to merge 1 commit intosteipete:mainfrom
nikhil8182 wants to merge 1 commit intosteipete:mainfrom
Conversation
Cancel pathDebugRefreshTask in UsageStore deinit to prevent background work continuing after deallocation. Build legacy keychain fingerprints from the actual legacy candidate instead of calling currentClaudeKeychainFingerprintWithoutPrompt(), which prefers the newest candidate and may return a fingerprint for a different keychain item. This caused stale comparisons in background sync checks and unnecessary keychain re-reads. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ratulsarna
reviewed
Mar 9, 2026
Collaborator
ratulsarna
left a comment
ratulsarna
left a comment
There was a problem hiding this comment.
A few questions to sanity-check this change.
| { | ||
| // Same as above: store fingerprint after interactive read to avoid background "sync" reads. | ||
| self.saveClaudeKeychainFingerprint(self.currentClaudeKeychainFingerprintWithoutPrompt()) | ||
| // Build fingerprint from the legacy candidate directly rather than using |
Collaborator
There was a problem hiding this comment.
Could saving the legacy candidate's fingerprint here get out of sync with the fingerprint that later change detection treats as current when duplicate keychain entries exist?
| !creds.isExpired | ||
| { | ||
| self.saveClaudeKeychainFingerprint(self.currentClaudeKeychainFingerprintWithoutPrompt()) | ||
| // Build fingerprint from the legacy candidate directly, matching the candidate |
Collaborator
There was a problem hiding this comment.
Could this fallback path end up with the same mismatch too, where we save one item's fingerprint but later background checks compare against another?
| if let legacyCandidate = self.claudeKeychainLegacyCandidateWithoutPrompt( | ||
| promptMode: fallbackPromptMode) | ||
| { | ||
| let legacyFingerprint = ClaudeKeychainFingerprint( |
Collaborator
There was a problem hiding this comment.
Would it be worth pulling this fingerprint construction into a small helper? I'm wondering if that would also help with the file-length lint failure on this file.
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
2 participants
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
pathDebugRefreshTaskinUsageStore.deinit— this task was missing from the deinit cleanup, causing it to continue running after deallocation (the[weak self]prevents crashes but the task does unnecessary background work)ClaudeOAuthCredentials— two call sites (loadFromClaudeKeychainUsingSecurityFrameworkandsyncFromClaudeKeychainWithoutPrompt) usedcurrentClaudeKeychainFingerprintWithoutPrompt()after a successful legacy data read. This function prefers the newest candidate and may return a fingerprint for a different keychain item than the one actually read via the legacy service-level query. Now builds the fingerprint fromclaudeKeychainLegacyCandidateWithoutPrompt()directly, matching the pattern already used in the candidate pathContext
The fingerprint mismatch caused
syncWithClaudeKeychainIfChangedto see a "changed" fingerprint on subsequent checks, triggering unnecessary keychain re-reads. This is related to #340 — while not the root cause (which is Claude Code's delete+recreate ACL wipe), it reduced the effectiveness of the cache layer that mitigates the repeated prompts.Test plan
swift buildpasses cleanlypathDebugRefreshTaskis properly canceled on store deallocation🤖 Generated with Claude Code