Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 41 additions & 22 deletions Sources/QuotaCore/ClaudeCredentialSource.swift
Original file line number Diff line number Diff line change
Expand Up @@ -296,40 +296,59 @@ public enum ClaudeCredentialSource {
case .unknown:
return .temporarilyUnavailable
case .present:
if let data = rememberedCredential() {
// The item was authorized and read from Keychain into memory.
// Even if the token inside is expired, macOS Keychain access was
// already granted. Return .authorized(data) so LocalQuotaReader
// and ClaudeLoginState report it as signedOut/idle, not
// needsPermission.
// Memory is a permission fact, not a freshness fact. It records

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Performance medium

Throttle or memoize the failed security re-read in resolveSilently so stale remembered grants do not spawn a security child process on every refresh cycle while the activeRead semaphore is held.

Throttle re-read attempts or memoize the failure to avoid spawning the security CLI on every cycle for stale remembered grants.
Prompt for LLM

File Sources/QuotaCore/ClaudeCredentialSource.swift:

Line 299:

Throttle or memoize the failed security re-read in resolveSilently so stale remembered grants do not spawn a security child process on every refresh cycle while the activeRead semaphore is held.

Suggested Code:

Throttle re-read attempts or memoize the failure to avoid spawning the security CLI on every cycle for stale remembered grants.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

// that macOS already granted `/usr/bin/security` this item, which
// is what stops the consent row from re-arming on every launch.
// It does NOT record the current token: Claude Code rewrites this
// same item every time it renews its own login, so a payload
// captured at launch goes stale within the hour. Answering from it
// forever (which is what shipped in #156) pinned the row to
// "login idle" while Claude Code sat right there signed in — the
// Keychain item held a fresh token the app simply never re-read.
// So: serve memory only while its own access token is unexpired.
let remembered = rememberedCredential()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kody code-review Performance medium

The silent re-read has no throttle or failure memo, so once the remembered payload is stale — the normal state whenever Claude Code is idle or not running — every provider refresh tick forks a /usr/bin/security child on the serial com.jays.usage-monitor.claude-keychain queue while holding the activeRead semaphore, forever. neither outcome lets the state recover: on .denied/.timedOut the stale memory is preserved by if let remembered { return .authorized(remembered) }, and on .found the same expired bytes are re-memorialized by remember(data), so rememberedGrantIsFresh is still false on the next tick; each spawn blocks that queue up to cliAttemptDeadline (12s) and makes a concurrent boundedAccess fail semaphore.wait(timeout: .now()) in access(), which LocalQuotaReader maps to the "Claude quota is temporarily unavailable." issue. Before this PR a remembered grant short-circuited after one child per process lifetime. memoize the last re-read attempt (timestamp) and only fork again after a minimum interval, or skip the re-read unless the remembered record is renewable/expiring soon.

// Re-read at most once per interval: a stale remembered payload
            // never becomes fresh on its own, so an unthrottled retry forks
            // /usr/bin/security on every refresh tick and blocks the serial
            // worker queue.
            if lastSilentReadAt.map({ Date().timeIntervalSince($0) < silentReadRetryInterval }) ?? false {
                if let remembered { return .authorized(remembered) }
                return .temporarilyUnavailable
            }
            lastSilentReadAt = Date()
            let outcome = probe.readViaSecurityCLI(attemptDeadline, { isAbandoned() })
            log.notice("claude keychain silent security CLI: \(outcome.logLabel, privacy: .public)")
            if case .found(let data) = outcome {
                remember(data)
                return .authorized(data)
            }
            if let remembered { return .authorized(remembered) }
Prompt for LLM

File Sources/QuotaCore/ClaudeCredentialSource.swift:

Line 309:

The silent re-read has no throttle or failure memo, so once the remembered payload is stale — the normal state whenever Claude Code is idle or not running — every provider refresh tick forks a `/usr/bin/security` child on the serial `com.jays.usage-monitor.claude-keychain` queue while holding the `activeRead` semaphore, forever. neither outcome lets the state recover: on `.denied`/`.timedOut` the stale memory is preserved by `if let remembered { return .authorized(remembered) }`, and on `.found` the same expired bytes are re-memorialized by `remember(data)`, so `rememberedGrantIsFresh` is still false on the next tick; each spawn blocks that queue up to `cliAttemptDeadline` (12s) and makes a concurrent `boundedAccess` fail `semaphore.wait(timeout: .now())` in `access()`, which LocalQuotaReader maps to the "Claude quota is temporarily unavailable." issue. Before this PR a remembered grant short-circuited after one child per process lifetime. memoize the last re-read attempt (timestamp) and only fork again after a minimum interval, or skip the re-read unless the remembered record is renewable/expiring soon.

Suggested Code:

// Re-read at most once per interval: a stale remembered payload
            // never becomes fresh on its own, so an unthrottled retry forks
            // /usr/bin/security on every refresh tick and blocks the serial
            // worker queue.
            if lastSilentReadAt.map({ Date().timeIntervalSince($0) < silentReadRetryInterval }) ?? false {
                if let remembered { return .authorized(remembered) }
                return .temporarilyUnavailable
            }
            lastSilentReadAt = Date()
            let outcome = probe.readViaSecurityCLI(attemptDeadline, { isAbandoned() })
            log.notice("claude keychain silent security CLI: \(outcome.logLabel, privacy: .public)")
            if case .found(let data) = outcome {
                remember(data)
                return .authorized(data)
            }
            if let remembered { return .authorized(remembered) }

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

if let data = remembered, rememberedGrantIsFresh(data) {
return .authorized(data)
}
// Nothing in memory, but the item is here. Before this was a bare
// `.unauthorized`, which re-armed the "needs permission" row on
// every single launch and made the owner re-click Allow Access
// forever — the grant was process-memory only, so it never
// survived a relaunch.
//
// Try the one read that can still succeed quietly. Once the owner
// has answered Always Allow, `/usr/bin/security` is on the item's
// access list and this returns the bytes without a panel, so the
// app reads itself for the rest of the session with no further
// prompting. The panel only reappears if the ACL is revoked, which
// is precisely when the owner should be asked again.
//
// A refusal here is not a failure: it is the first run on a fresh
// install that has never been granted, and the caller's
// `.unauthorized` is the correct answer for that case.
// Past that point, or with nothing remembered at all, read the
// bytes again. This is the one read that can succeed quietly:
// once the owner has answered Always Allow, `/usr/bin/security` is
// on the item's access list, so it returns without a panel and the
// app refreshes itself with no prompting. The panel only reappears
// if the ACL is revoked, which is precisely when the owner should
// be asked again. It is also what lets a renewal that Claude Code
// performed on our behalf actually be seen.
let outcome = probe.readViaSecurityCLI(attemptDeadline, { isAbandoned() })
log.notice("claude keychain silent security CLI: \(outcome.logLabel, privacy: .public)")
if case .found(let data) = outcome {
remember(data)
return .authorized(data)
}
// The read produced nothing. With a grant already remembered that
// is not a permission problem — it is a slow or refused read on a
// loaded Mac, and reporting `.unauthorized` here is exactly the
// consent loop #156 closed. Serve what we already hold and let the
// row report signedOut/idle, which is the honest state.
if let remembered { return .authorized(remembered) }
// Nothing remembered and nothing readable: a fresh install that has
// never been granted, so `.unauthorized` is the correct answer.
return .unauthorized
}
}

/// Whether the remembered payload still carries an access token that has
/// not expired. Only then may the refresh loop answer from memory.
///
/// This is deliberately stricter than `rememberedGrantStillUsable`, which
/// also accepts an expired-but-renewable record. That looser rule is right
/// for deciding whether to keep trusting the *grant*, and wrong for
/// deciding whether to keep sending the *bytes*: an expired token answers
/// 401 no matter how renewable it is.
static func rememberedGrantIsFresh(_ data: Data, now: Date = Date()) -> Bool {
guard let root = ClaudeOAuthParser.parse(data) else { return false }
return ClaudeOAuthParser.validOAuth(in: root, now: now) != nil
}

/// A remembered payload is usable while its access token is still
/// unexpired, or while it holds an expired access token that can be renewed
/// via its refresh token. Anything else, including a rotated or unreadable
Expand Down
53 changes: 50 additions & 3 deletions Tests/QuotaCoreTests/ClaudeConsentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -353,18 +353,65 @@ final class ClaudeConsentTests: XCTestCase {
XCTAssertNil(ClaudeCredentialSource.rememberedCredential())
}

func testExpiredRememberedGrantRemainsAuthorizedWithoutAskingForConsent() {
func testExpiredRememberedGrantIsReReadButStillNeverAsksForConsent() {
// An expired remembered payload is no longer served as the answer: it
// is stale, and Claude Code rewrites the item when it renews its own
// login. So the loop reads the bytes again. That read must NOT turn a
// read failure into `.unauthorized` — a grant already remembered proves
// macOS allows this item, and re-arming consent is the loop #156 closed.
let expired = Data(#"{"claudeAiOauth":{"accessToken":"fixture","expiresAt":1}}"#.utf8)
ClaudeCredentialSource.remember(expired)
let log = ProbeLog()
let access = ClaudeCredentialSource.resolveSilently(
probe: makeProbe(log: log, cli: [.found(expired)], presence: .present),
probe: makeProbe(log: log, cli: [.denied], presence: .present),
isAbandoned: { false })
XCTAssertEqual(access, .authorized(expired))
XCTAssertEqual(log.calls, ["lookup"])
XCTAssertEqual(log.calls, ["lookup", "cli"], "an expired payload must be re-read")
XCTAssertEqual(ClaudeLoginState.resolve(hasUsableCredential: false, access: access),
.signedOut, "and it must not become a consent prompt")
XCTAssertFalse(ClaudeCredentialSource.rememberedGrantStillUsable(expired, now: Date(timeIntervalSince1970: 10)))
}

func testExpiredRememberedGrantPicksUpTheTokenClaudeCodeRenewedUnderIt() {
// The regression, end to end through the silent read. Launch reads an
// expired payload and remembers it; Claude Code then renews and rewrites
// the same Keychain item; the next refresh must return the NEW bytes.
// Serving the frozen copy forever is what pinned the row to "login idle"
// while Claude Code sat right there signed in (board dd5f6702).
let stale = Data(#"{"claudeAiOauth":{"accessToken":"old","expiresAt":1}}"#.utf8)
let renewed = Data(#"{"claudeAiOauth":{"accessToken":"new","expiresAt":4102444800000}}"#.utf8)
ClaudeCredentialSource.remember(stale)
let log = ProbeLog()
let access = ClaudeCredentialSource.resolveSilently(
probe: makeProbe(log: log, cli: [.found(renewed)], presence: .present))
XCTAssertEqual(access, .authorized(renewed))
XCTAssertEqual(log.calls, ["lookup", "cli"])
XCTAssertEqual(ClaudeCredentialSource.rememberedCredential(), renewed)
// And the renewed token is now what the reader sends, so the row is
// connected rather than idle.
XCTAssertEqual(ClaudeLoginState.resolve(hasUsableCredential: true, access: access), .connected)
// Being fresh, it is served from memory again on the next cycle.
let next = ProbeLog()
XCTAssertEqual(ClaudeCredentialSource.resolveSilently(
probe: makeProbe(log: next, cli: [.denied], presence: .present)),
.authorized(renewed))
XCTAssertEqual(next.calls, ["lookup"], "a fresh grant is still served without a child")
ClaudeCredentialSource.resetRememberedCredential()
}

func testRememberedGrantIsFreshOnlyWhileItsAccessTokenIsUnexpired() {
let fresh = Data(#"{"claudeAiOauth":{"accessToken":"fixture","expiresAt":4102444800000}}"#.utf8)
let expired = Data(#"{"claudeAiOauth":{"accessToken":"fixture","expiresAt":1}}"#.utf8)
let now = Date(timeIntervalSince1970: 1_700_000_000)
XCTAssertTrue(ClaudeCredentialSource.rememberedGrantIsFresh(fresh, now: now))
XCTAssertFalse(ClaudeCredentialSource.rememberedGrantIsFresh(expired, now: now))
// Stricter than the grant test on purpose: renewable-but-expired is
// still an expired token, and an expired token answers 401.
let renewableExpired = Data(#"{"claudeAiOauth":{"accessToken":"old","refreshToken":"r","expiresAt":1}}"#.utf8)
XCTAssertTrue(ClaudeCredentialSource.rememberedGrantStillUsable(renewableExpired, now: now))
XCTAssertFalse(ClaudeCredentialSource.rememberedGrantIsFresh(renewableExpired, now: now))
}

func testUnexpiredRememberedGrantStaysUsable() {
let payload = Data(#"{"claudeAiOauth":{"accessToken":"fixture","expiresAt":4102444800000}}"#.utf8)
XCTAssertTrue(ClaudeCredentialSource.rememberedGrantStillUsable(payload, now: Date(timeIntervalSince1970: 1_700_000_000)))
Expand Down
Loading