fix: stop background Ollama refresh from spuriously requiring re-sign-in - #2814
Merged
steipete merged 4 commits intoAug 13, 2026
Merged
Conversation
Fixes steipete#2072. ollama.com rotates its session cookie independently of the user's signed-in state. When a background refresh (5-minute timer) sees the cached cookie fail with an auth error, fetchAutomatic cleared the cache and unconditionally fell through to a browser cookie read to recover — but BrowserCookieAccessGate denies that read in a background context (no interactive Keychain prompt outside a user-initiated action). The browser read then returned no candidates, surfacing as the misleading 'No Ollama session cookie found. Please sign in...' error, even though the user was still signed in and the cache had simply been cleared a moment earlier by this same call. The cache stayed empty, so every subsequent background tick repeated the same failure until the next manual refresh repopulated it — the reported flicker. fetchAutomatic now re-throws the original auth error instead of falling through to a browser read when the interaction context is .background, deferring recovery to the next user-initiated refresh (which already has the explicit-retry path from steipete#2249/steipete#2595). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8928a4bdb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // outside a user-initiated action), so falling through to `fetchBrowser` here would only | ||
| // trade this accurate auth error for a misleading "no session cookie" one. Defer recovery | ||
| // to the next user-initiated refresh, which already has the explicit-retry path. | ||
| guard ProviderInteractionContext.current == .userInitiated else { throw error } |
There was a problem hiding this comment.
When the cached Ollama cookie expires but the browser contains its rotated replacement, this guard now forces the refresh to fail instead of importing the fresh cookie. Background browser reads are not categorically denied: BrowserCookieAccessGate.shouldAttemptInBackground permits Chromium reads after a no-UI Keychain preflight, and non-Keychain sources can also be read safely; moreover, CLIUsageCommand deliberately performs normal usage fetches under .background, so the first CLI invocation after cookie rotation now reports invalidCredentials and only a second invocation can recover after the cache was cleared. Attempt fetchBrowser here and rethrow the cached authentication error only if that prompt-free recovery fails.
Useful? React with 👍 / 👎.
…cked Round-1 review (Codex adversarial-review) found that the prior fix's blanket 'skip browser recovery in the background' guard was wrong: BrowserCookieAccessGate does not always deny a background browser read — Safari never needs Keychain decryption at all, and a Chromium browser with a prior 'Always Allow' Keychain grant is also read without a prompt. The unconditional re-throw discarded those working recovery paths, turning a real self-heal into a permanent background failure whenever the cached-cookie clear also silently failed (clearIfCurrent's boolean result is already discarded upstream). fetchAutomatic now still attempts fetchBrowser() after an auth failure in the background, exactly as it already does for the parse-failure recovery branch below it, relying on BrowserCookieAccessGate's own no-UI preflight to naturally deny a prompt-requiring attempt. Only when that attempt itself fails does it surface the original, accurate auth error instead of a misleading 'no session cookie' one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 11:58 AM ET / 15:58 UTC. ClawSweeper reviewWhat this changesThis PR lets automatic Ollama usage refresh retry browser-cookie import after a cached cookie fails, while preserving specific browser-access errors. Merge readiness⛔ Blocked until stronger real behavior proof is added - 5 items remain Keep open: the changed recovery path still converts a deliberately suppressed background browser import into a “sign in again” error, and the submitted live trace does not exercise the stale cached-cookie branch. Priority: P1 Review scores
Verification
How this fits togetherCodexBar refreshes Ollama usage with a cached browser session cookie and falls back to browser-cookie import when that cache fails. The browser access gate permits only prompt-free background imports, then the resulting usage or error is presented by the provider UI. flowchart LR
A[Scheduled Ollama refresh] --> B[Cached browser cookie]
B --> C{Cached request succeeds?}
C -->|yes| D[Usage snapshot]
C -->|auth failure| E[Clear stale cache]
E --> F[Prompt-free browser access gate]
F --> G[Browser cookie recovery]
G --> H[Usage snapshot or recovery guidance]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Return a distinct background stale-cache recovery diagnosis that directs the user to refresh the provider, preserve actionable browser-access errors, and attach a redacted packaged-app trace of the repaired branch. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction of the changed branch is supplied: the PR explicitly says stale-but-present cached-cookie recovery was not live-exercised. The failure is high-confidence source-reproducible through the background access gate and error mapping. Is this the best way to solve the issue? No. Retaining the cached-auth error still instructs a signed-in user to sign in again; the recovery path needs a distinct manual-refresh diagnosis before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c4ed34d0e44a. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
Round-2 review (built-in /review) flagged that round 1's fix made the auth-failure branch's recovery logic byte-for-byte identical to the adjacent shouldTryBrowserCandidates branch. Extract the shared 'attempt fetchBrowser, surface the original error if that also fails' pattern into fetchBrowserOrRethrow so both branches call one helper instead of duplicating the do/catch. No behavior change; swift test --filter OllamaUsageFetcherRetryMappingTests stays 30/30 green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
Contributor
Author
🔍 Local review (cycle 3/3, final) — round 9284d2d6-d87e-4e30-a172-67877e4caf36Reviewed locally (
No FIX or UNVERIFIED findings this round. Cycle complete. |
Contributor
Author
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 4 SKIP (pre-existing architectural properties, out of scope for this issue), 0 UNVERIFIED. Review complete after 3 cycles — no FIX or UNVERIFIED findings remain. Per the review-only-on-merge policy, merging is a manual step for the PR author. The SKIPped findings (cross-account cookie binding, unconditional cache overwrite race, background cooldown lockout) describe a real, broader architectural gap in the automatic cookie-resolution system worth a follow-up issue, independent of this PR. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
ClawSweeper (P1) and Codex both flagged that fetchBrowserOrRethrow swallows every fetchBrowser failure — including the specific, actionable diagnoses (safariCookieAccessDenied, browserCookieDecryptionDenied, browserCookieDecryptionDisabled) — and always substitutes the generic stale cached-cookie auth error. On a user-initiated refresh this replaces a fixable diagnosis (grant Full Disk Access, retry a declined Keychain prompt) with a misleading 'please sign in again', undoing the explicit-retry guidance shipped in steipete#2249/steipete#2595. fetchBrowserOrRethrow now re-throws an actionable browser-access error as-is instead of substituting the original cached error; a non- actionable recovery failure (no candidates found, network error, etc.) still falls back to the original error as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
axisrow
added a commit
to axisrow/CodexBar
that referenced
this pull request
Aug 9, 2026
ClaudeWebAPIFetcher.fetchUsageSerialized cleared a stale cached session cookie on an auth failure and unconditionally fell through to a browser cookie read to recover, exactly the pattern fixed for Ollama in PR steipete#2814. In a background context BrowserCookieAccessGate typically denies that read (no interactive Keychain prompt outside a user-initiated action), so extractSessionKeyInfo finds no session key and the generic 'no session key found' error replaces the original, more informative cached-auth error (e.g. the OAuth-absence message) — surfacing as the same intermittent flicker already reported for Claude. Still attempt browser-cookie recovery after clearing the stale cache, even in a background context — BrowserCookieAccessGate already gates that read on its own no-UI preflight (Safari never needs Keychain decryption, and a Chromium browser with a prior 'Always Allow' Keychain grant is read without a prompt) — so a background attempt is not unconditionally blocked. Only when that attempt also fails do we now surface the original cached-auth error instead of the misleading generic one. Adds ClaudeWebBackgroundRecoveryTests covering: background recovery that finds nothing (original error surfaces), background recovery that succeeds without a prompt (still works), and the equivalent user-initiated case.
5 tasks
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
Closes #2072.
ollama.comrotates its session cookie independently of the user's signed-in state. When a background refresh (the 5-minute timer) sees the cached cookie fail with an auth error,OllamaStatusFetchStrategy.fetchAutomaticcleared the cache and unconditionally fell through to a browser cookie read to recover — butBrowserCookieAccessGatedenies that read in a background context (no interactive Keychain prompt outside a user-initiated action). The browser read then returned no candidates, surfacing as the misleading "No Ollama session cookie found. Please sign in at https://ollama.com/signin in your browser." error, even though the user was still signed in and the cache had simply been cleared a moment earlier by this same call. The cache stayed empty, so every subsequent background tick repeated the same failure until the next manual refresh repopulated it — this is the reported flicker (also matchesSuperSalsa20's repro in the issue thread: background fails → interactive refresh recovers → next background refresh fails again).fetchAutomaticstill attempts browser-cookie recovery after clearing the stale cache, even in a background context —BrowserCookieAccessGatealready gates that read on its own no-UI preflight (Safari never needs Keychain decryption, and a Chromium browser with a prior "Always Allow" Keychain grant is read without a prompt), so a background attempt is not unconditionally blocked. Only if that recovery attempt itself fails does the strategy fall back to surfacing an error: a generic, non-actionable browser failure (e.g. no candidates found) is swapped for the original, more informative cached-auth error, while a specific actionable browser-access diagnosis (Safari needs Full Disk Access, a Chromium Keychain prompt was declined/disabled) is preserved and re-thrown as-is, since that is always more useful than the stale cached error it would otherwise replace.Root-cause writeup with the full execution chain: #2072 (comment)
Note:
ClaudeWebAPIFetcher.fetchUsageSerializeduses a similar cache-invalidate-then-fallback-to-browser pattern; not touched in this PR to keep the change scoped to the reported Ollama issue, but worth a follow-up if an equivalent gap is independently confirmed there.Test plan
swift test --filter OllamaUsageFetcherRetryMappingTests(33/33 passing) — covers background recovery after cache invalidation, preserving actionable browser-access errors, and the pre-existing user-initiated cached-cookie-replacement behavior.make lint(0 violations)swift testsuite passingVerification
Live-verified against this repo's own signed-in
ollama.comsession, run through a locally packaged, adhoc-signedCodexBar.appbuilt from this branch (Scripts/package_app.sh release) so the CLI gets real Keychain persistence (an unsigned/unbundledswift buildbinary intentionally falls back to an in-memory-only cache — seeKeychainCacheStore.swift, "Unbundled processes ... must never touch the shared cache item"). Full trace kept locally, redacted excerpts below (cookie names only ever appear in these logs — never values; usernames/emails scrubbed).1. Cold background fetch after a forced cache miss — the background path honors its no-UI boundary: with no prior "Always Allow" Keychain grant for Chrome/Chromium on this machine, it correctly declines to prompt and surfaces the actionable sign-in message rather than silently hanging or crashing:
2. User-initiated recovery (
cookie refresh --provider ollama --allow-keychain-prompt) — real browser-cookie import against the live session, cache populated:3. Warm-cache reuse in a separate process invocation — proves the cookie from (2) persisted to the real Keychain (not just in-process memory) and is reused without re-hitting the browser:
fetchAutomatic's recovery and error-classification logic does not readProviderInteractionContext— onlyBrowserCookieAccessGate's no-UI preflight does — so the background and user-initiated code paths exercised above run the identical classification code covered by the stale-cached-cookie and actionable-browser-error unit tests. The one branch not live-exercised here is recovery from a stale-but-present cached cookie (vs. the cache-miss case (1) actually hit): reproducing that live would require writing a crafted expired value into this machine's real login-Keychain entry, which isn't a step I'm willing to script against real credentials — it's covered instead byOllamaUsageFetcherRetryMappingTests' injected-failure tests, which exercise the exact same code path with a controlled input.🤖 Generated with Claude Code