Skip to content

Keep the OIDC callback listener alive when an LLM proxy client disconnects - #6229

Merged
jhrozek merged 4 commits into
mainfrom
llm-proxy-login-ctx-minimal
Aug 6, 2026
Merged

Keep the OIDC callback listener alive when an LLM proxy client disconnects#6229
jhrozek merged 4 commits into
mainfrom
llm-proxy-login-ctx-minimal

Conversation

@jhrozek

@jhrozek jhrozek commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • On a cold start, thv llm proxy opens the browser for the OIDC login but the callback listener is gone before the user can finish authenticating, so thv llm setup --lazy cannot complete a login through the proxy. The token fetch was rooted in the inbound request's context, which bounds the login's lifetime by the calling client's patience — and no HTTP client waits the 30-90s a person needs at an IdP. When the client gives up, net/http cancels r.Context(), oauth.Flow.Start returns at flow.go:255, and its deferred Shutdown tears down the callback server mid-login.
  • Root the token fetch in the proxy-lifetime context passed to Start instead. A client disconnect can no longer abort a login in progress; Ctrl+C (which cancels Start's ctx) still can.
  • This also removes a second failure mode: previously a retrying client would start a fresh flow on the same pinned callback port with a new state, so a completed login on the user's original tab was rejected as invalid state parameter. With the flow no longer dying on disconnect, there is no successor flow to invalidate the open tab.
  • Corrects two doc comments that no longer matched behaviour: the interactive parameter on llm.NewTokenSource (runLLMToken has passed true since lazy setup landed), and tokenFetchTimeout, which now explains what a queued request experiences during the initial login.

Closes #6227

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests — go test -race ./pkg/llm/... green (the full task test suite has not been run on this branch)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing

Manual: reproduced the original failure with thv llm setup --lazy + thv llm proxy against Okta — browser opened, the calling tool timed out, and the callback landed on a dead listener. With this change the login completes after the client has already disconnected, and the client's retry is served from the cached token.

Two new unit tests pin both halves of the contract:

  • TestHandler_TokenFetchSurvivesClientDisconnect — cancelling the request ctx does not cancel the fetch ctx.
  • TestHandler_TokenFetchCancelsWithStartContext — cancelling Start's ctx does.

Changes

File Change
pkg/llm/proxy/proxy.go handler takes the proxy-lifetime ctx; token fetch derives from it instead of r.Context(). Expanded the tokenFetchTimeout comment.
pkg/llm/proxy/proxy_test.go Two tests for the disconnect/lifetime contract; existing tests updated for the handler signature.
pkg/llm/tokensource.go Doc comment only — describe what interactive actually gates.

Does this introduce a user-facing change?

Yes. Interactive OIDC login through thv llm proxy now works when the calling client times out mid-login, which is the normal case for thv llm setup --lazy. Previously the login could not be completed at all.

Special notes for reviewers

The behavioural trade is deliberate and worth a look: a handler goroutine's lifetime is no longer bounded by its client's connection, so during the login window a disconnected client's handler stays parked until the token arrives or tokenFetchTimeout (3 min) elapses. That is what makes the login survivable. The token source serializes Token() internally, so concurrent requests queue behind the one login rather than each starting their own browser flow — one flow, however many requests. This is documented at the tokenFetchTimeout declaration.

Follow-ups I deliberately left out of scope:

  1. thv llm token invoked as an apiKeyHelper shows the same symptom from a different cause — the client kills the child process, so the listener dies with it. No amount of context rooting fixes that; it needs either an eager login or a process-independent listener.
  2. oauth.Flow binds :port rather than 127.0.0.1:port (flow.go:202) and treats a foreign state as fatal instead of returning 400 and continuing to wait (flow.go:322-327). Together that is a LAN-reachable way to kill someone's in-progress login. Worth its own PR.
  3. Priming the token once in Start before Serve (warn-and-continue on failure) would move the cold-start browser tab to the moment the user typed the command, out of the concurrent HTTP path entirely, without breaking what --lazy promises.

Generated with Claude Code

jhrozek added 4 commits August 6, 2026 11:17
The token fetch was rooted in the inbound client's request context, so a
client disconnect (e.g. an LLM client's ~11s timeout) tore down the OAuth
callback server mid-interactive-login and the completed sign-in was
discarded. Thread Start's proxy-lifetime context into the handler instead:
an impatient client can no longer abort a login, while Ctrl+C still can.

Closes #6227
Pin the context contract: cancelling the inbound request's context must not
cancel the in-flight token fetch, while cancelling Start's lifetime context
must. Guards the fix against re-rooting in r.Context().
The doc claimed interactive is false for thv llm token, but runLLMToken has
passed true since lazy setup landed: a cache miss there is expected to sign
the user in transparently. Describe what the flag actually gates.
The reparented fetch changes what a queued request experiences: it now waits
on a login that outlives its own client. State that the queue collapses onto a
single browser flow so the behaviour is not mistaken for a stall.
@jhrozek
jhrozek requested a review from JAORMX as a code owner August 6, 2026 09:18
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Aug 6, 2026
@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.46%. Comparing base (12ffd7a) to head (87fc715).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6229   +/-   ##
=======================================
  Coverage   72.46%   72.46%           
=======================================
  Files         739      739           
  Lines       76728    76767   +39     
=======================================
+ Hits        55598    55631   +33     
- Misses      17163    17172    +9     
+ Partials     3967     3964    -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek
jhrozek merged commit f3a2818 into main Aug 6, 2026
48 checks passed
@jhrozek
jhrozek deleted the llm-proxy-login-ctx-minimal branch August 6, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

thv llm proxy: browser login fails because a client disconnect kills the OAuth callback listener

2 participants