feat(witan-core): ask for offline_access at login, so a session outlives its task - #240
Merged
Conversation
…ves its task The device grant requested `openid` alone, tying the refresh token to the interactive SSO session — about five minutes here. A login went stale in the middle of the work it was started for, and retrying did not help because a fresh login died at the same point. The concurrency probe made it concrete: it needs ~8 minutes across two phases, pinned a token for phase A, and got invalid_grant pinning phase B four minutes after a successful login. That blocks the deployment's phase-exit measurement. Keycloak advertises an offline refresh token as refresh_expires_in: 0, which _store_token already reads as "never" rather than "already expired", so nothing downstream changes. The trade: the cached refresh token stops expiring on its own, so a stolen cache is good until revoked at the IdP. Accepted because the cache is created 0o600 by os.open, and because the alternative — raising the realm's SSO timeouts — would lengthen every session for every client instead of just this credential. Degrades rather than failing: a realm that refuses the scope answers invalid_scope and gets retried with plain openid, since raising there would turn a convenience into a total login outage. Any other failure propagates and is deliberately not retried, so a 500 is diagnosed rather than hidden behind a second identical attempt. Releases witan-core 0.21.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Witan’s OIDC device login to request longer-lived offline sessions while retaining compatibility with realms that reject the scope.
Changes:
- Requests
openid offline_access, with anopenidfallback. - Adds tests for scope handling and retry behavior.
- Releases
witan-core0.21.0.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/witan-core/witan_core/remote/oidc.py |
Implements offline-access requests and fallback. |
packages/witan-core/tests/test_remote_oidc.py |
Tests scope and fallback behavior. |
packages/witan-core/pyproject.toml |
Bumps the package version. |
packages/witan-core/CHANGELOG.md |
Documents the release and security tradeoff. |
uv.lock |
Synchronizes the workspace version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… on PyPI
Reported from a real `uv tool install`:
ImportError: cannot import name 'SessionLife' from 'witan_core.remote.oidc'
witan-council 0.13.0 imports SessionLife at module scope but declared
witan-core>=0.20, which does not export it. The symbol and its caller landed
together in #239 without bumping either the library version or this floor.
This is the fifth time this exact failure has shipped and the first to reach a
user. The workspace resolves witan-core by path, so the full suite passes while
a published install resolves a pair that cannot import each other — nothing in
CI can catch it. The pin comment now says that at the point where the next
person will be adding a symbol, rather than only in the tally.
Releases witan-council 0.14.0. The 0.21 floor is satisfied by the witan-core
release in this same branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5
… saying so Review caught that the fallback matched `invalid_scope` in the response body regardless of status, so a 500 carrying that string would trigger a second device-authorization request — contradicting the no-retry guarantee stated two paragraphs above it and burying the original outage behind an identical second failure. RFC 6749 §5.2 puts `invalid_scope` at 400, so the status is now part of the test. The existing non-scope test used a 500 with `server_error` and could never have caught this; a new one sends 500 with `invalid_scope` and asserts the endpoint is hit exactly once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5
blarghmatey
added a commit
that referenced
this pull request
Aug 16, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5
blarghmatey
added a commit
that referenced
this pull request
Aug 16, 2026
* fix(witan): stop dropping in-flight requests 2s into a shutdown FastMCP builds its uvicorn config with a hardcoded timeout_graceful_shutdown of 2 seconds, so on SIGTERM the server stopped accepting connections, gave running requests two seconds, and dropped the rest. A witan write has been measured at 27s under load, so every one in flight was severed by a deploy, an eviction or a node drain — and a severed write is exactly the indeterminate outcome the caller cannot safely retry. serve() now passes uvicorn_config through, defaulting to 120s to match the request budget the deployment enforces at its gateway, and settable via --shutdown-grace-seconds / WITAN_MCP_SHUTDOWN_GRACE_SECONDS. This could not be fixed from the deployment side. ol-infrastructure sets terminationGracePeriodSeconds: 150 so the kubelet waits, but uvicorn declined to use it — the pod-side setting bought time nothing spent, while looking like the whole fix. Found while investigating FastMCP's concurrency knobs, not from a failure report. Verified against the real library rather than the test double: with uvicorn.Config instrumented, 120.0 arrives where fastmcp's 2 would have been. stdio runs pass no uvicorn config at all and are unaffected. Releases witan-council 0.14.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5 * chore(witan): release 0.15.0 — 0.14.0 was taken by #240 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5 * docs(witan): add --shutdown-grace-seconds to the CLI reference Review caught that the serve flag table enumerates every public option and env var, and this PR added one without updating it. The table is the canonical reference, so a missing row is a stale doc rather than an omission nobody sees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
What are the relevant tickets?
Addresses
tk-a-witan-login-lasts-5-minutes-of-token-and-dies--550dd4. Unblockstk-re-measure-indeterminate-writes-in-qa-once-witan-100d95, which cannot run today.Description (What does it do?)
witan loginrequestsopenid offline_accessinstead ofopenidalone.openidwhen the realm refuses the scope.witan-core0.21.0.The device grant tied the refresh token to the interactive SSO session, which on
this deployment is about five minutes. A login went stale in the middle of the
work it was started for, and retrying did not help — a fresh login died at the
same point.
Measured, not inferred. The concurrency probe needs ~8 minutes across two
phases (
lead 20 + WORKER_TIMEOUT_S 90 + _PHASE_MARGIN_S 120= 230s each). On2026-08-16 it pinned a token for phase A with 249s of life, then got
invalid_grantpinning phase B — four minutes after a successful login:--leadcannot rescue it: 90 + 120 dominates, so even--lead 1leaves 211sper phase. That blocks the witan deployment's phase-exit measurement outright.
Keycloak advertises an offline refresh token as
refresh_expires_in: 0, which_store_tokenalready interprets as "never expires" rather than "alreadyexpired", so nothing downstream needed changing.
★ The trade, stated plainly
The cached refresh token stops expiring on its own, so a stolen cache file is
usable until the grant is revoked at the IdP rather than until the session
lapses. That is a real widening of exposure.
It is accepted because:
os.open(..., 0o600)— restrictive from creation,never even briefly group- or world-readable (there is an existing test
asserting the mode);
would lengthen every session for every client rather than just this
credential.
Revocation now belongs to the IdP; there is no longer a short clock doing it
implicitly. Worth a look if that is not an acceptable posture for this realm —
the realm-config route is the other option and this PR does not foreclose it.
How can this be tested?
Three new tests, each verified to bind by reverting
_LOGIN_SCOPEto"openid"—test_login_asks_for_offline_accessandtest_login_falls_back_when_the_realm_refuses_offline_accessboth fail, whiletest_a_non_scope_failure_at_the_device_endpoint_is_not_retriedcorrectly staysgreen.
End-to-end confirmation is a real
witan login --target qafollowed by a proberun that survives both phases. I have not run that — it needs an interactive
device flow — so the live behaviour of this change is unverified and the
realm may yet refuse the scope. If it does, the fallback keeps login working and
we take the realm-config route instead.
Additional Context
Why the fallback is not optional. A realm that does not grant
offline_accessto this client answers the device-authorization endpoint withinvalid_scope. Raising there would convert a convenience into "nobody can login at all" — strictly worse than the short session being fixed. So that specific
refusal retries with plain
openid.Why only that refusal. Any other failure propagates and is deliberately not
retried. A 500 is not a scope problem, and a second identical attempt would bury
the diagnosis rather than surface it. A test asserts the endpoint is hit exactly
once in that case.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JYb9sMjetD9Nxjf24Aw3m5