Skip to content

fix(mcp): cap delegated token cache TTL so a disconnect is enforced (#380) - #381

Merged
initializ-mk merged 3 commits into
mainfrom
fix/delegated-token-cache-cap
Aug 1, 2026
Merged

fix(mcp): cap delegated token cache TTL so a disconnect is enforced (#380)#381
initializ-mk merged 3 commits into
mainfrom
fix/delegated-token-cache-cap

Conversation

@initializ-mk

Copy link
Copy Markdown
Contributor

Addresses #380 (bounds the window; instant-revoke signal is the documented follow-up there).

Problem

After a user disconnects a delegated MCP account, the agent keeps acting as that user for up to the access-token lifetime (~1h for Atlassian). A new execution kept returning Jira results well after the account was disconnected (field report).

Root cause

The managed delegated path caches the per-subject access token in an internal SubjectTokenStore with ttl = expires_in (~1h). A platform disconnect only deletes the vault grant (agent-builder DeleteUserGrant) — it has no channel to forge's in-memory cache, Evict is never called on this path, and the provider token is still valid at the provider, so nothing downstream fails. The token keeps working until its TTL lapses.

Forge fetches from the platform token endpoint, which is the grant authority (403/404 after disconnect) — it just wasn't re-asking often enough.

Fix

Cap the delegated per-subject cache at delegatedTokenMaxTTL (5m). Forge re-validates against the platform within that window, so a disconnected grant yields ErrNoToken — and with the call-time auth gate (#377) the call parks/denies — in ≤5m instead of ~1h. Self-contained; no cross-repo change; transparent to connected users (a cheap platform token re-fetch at most every ~5m per active user-mode subject). Agent-principal (platform-mode) tokens are not capped — that grant is org registration, not a per-user connection someone disconnects.

Follow-up (in #380)

Instant revocation via a platform→forge POST /mcp/revoke {subject, server} on disconnect (evict token cache + pooled connection; fan-out mirrors signalConsent), plus downstream 401/403 eviction for provider-side revocation. That's a larger cross-repo change (the managed source's store needs to be externally reachable); this PR is the self-contained bound.

Tests

TestDelegatedTokenSource_CachesCappedTTL injects a spy SubjectTokenStore and asserts the stub's 1h expires_in is clamped to the cap. go test ./mcp/... green, gofmt + golangci-lint clean.

The delegated-consent auth gate (#330) was consulted only for
resolveClient (connection-establish) errors. But per-request-auth
transports (transport_http attaches the bearer on every Send,
including tools/call) 403 at CALL time, not at initialize — so a
type=user server whose user has no grant surfaced ErrNoToken from
client.CallTool, which the gate never inspected. The call failed hard
with reason=no_token, no mcp_auth_required event fired, and the
platform consent flow had nothing to trigger on (field: Atlassian
agent, forge#376).

Execute now wraps the whole resolve→call sequence in the gate: an
ErrNoToken from either half parks via authGate.Await and, on a granted
resume, retries resolve→call. Same bounded, one-shot semantics as
before; the diagnostic phase prefix is irrelevant now since both paths
are covered.

Tests: call-time no-token parks + retries to success; gate give-up
still fails as no_token; a non-auth CallTool error never parks; nil
gate still surfaces ErrNoToken.
…covery (closes #378)

The recovered-session dedup skipped the incoming user message when it
matched the last user message ANYWHERE in history. A verbatim re-run of
an already-answered request (the session ends in an assistant turn)
therefore matched the earlier user turn and was dropped — the loop
replayed the poisoned transcript and repeated its last reply without
re-attempting tools. Field: a delegated MCP (Jira) call that failed
"not connected" never recovered when the user connected the account and
re-sent the identical request.

Dedup is now trailing-only: skip only when the recovered session's LAST
message is an identical user turn (a genuine premature-loop-exit
duplicate — persisted but never answered). If it ends in an assistant
turn, an identical incoming message is a legitimate re-run and is
appended so the loop re-enters and re-attempts tools.

Tests: a re-run after an answered session appends the turn (regression);
a trailing-user duplicate is still skipped (no regression).
A managed delegated (type=user) access token was cached for the
platform's full expires_in (Atlassian ~1h). A platform-side disconnect
deletes the vault grant but can't reach forge's in-memory cache, so the
agent kept acting as the revoked user until the token lapsed (field
report).

Cap the per-subject delegated cache at delegatedTokenMaxTTL (5m):
forge re-validates against the platform — the grant authority — within
that window, so a disconnected grant yields ErrNoToken (and with the
call-time auth gate, parks) in ≤5m instead of ~1h. Agent-principal
(platform-mode) tokens are not capped — that grant isn't a per-user
connection. Instant revocation via a platform→forge /mcp/revoke signal
is the documented follow-up (#380).

Test injects a spy SubjectTokenStore and asserts the 1h expires_in is
clamped to the cap.

@initializ-mk initializ-mk left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed against the branch source — correct, well-scoped, and the mechanism delivers what it claims. LGTM.

Scope — third in the stack

Commits confirm #381 sits on #377#379#381. mcp_tool.go (#377) and loop.go (#379) are byte-identical to those already-reviewed branches — net-new here is just platform_token.go (the cap) + platform_delegated_test.go. Same merge ordering caveat: landing #381 subsumes #377 and #379.

The cap enforces the window — traced end-to-end

Not just the clamp — the whole path:

  • TokenForSubject clamps ttl to delegatedTokenMaxTTL (5m) before store.Put, so expiresAt = now + ≤5m.
  • memSubjectTokenStore.Get returns the token only while now < expiresAt - skew (skew 30s), then eagerly deletes it. Effective cache lifetime ~4m30s; the next call re-fetches from the platform token endpoint (the grant authority), which 403/404s a disconnected subject → ErrNoToken → the call parks/denies via the #377 gate. The "≤5m instead of ~1h" claim holds (≈4.5m worst case).

The uncapped agent-principal claim checks out

The cap lives only on delegatedTokenSource.TokenForSubject. The agent-principal platformTokenSource is a separate struct with its own inline p.token/p.expiresAt cache — it never touches SubjectTokenStore and is genuinely uncapped. Right call: org-registration grant, not a per-user connection.

Clamp direction is safe

if ttl > delegatedTokenMaxTTL only shrinks — a short expires_in (< 5m) is left alone, and since defaultPlatformTokenTTL is also 5m the omitted-expires_in path is a no-op.

Observations (non-blocking)

  1. Platform token-endpoint fetch amplification (low). Capping raises the delegated re-fetch rate for a long-lived active subject from ~1/hour to ~1 per 4.5 min (~12×). Fine if the platform token endpoint serves from its own cache; if it instead does a full upstream OAuth refresh-token exchange with the provider on every call, this multiplies upstream refresh load and could rotate refresh tokens or brush provider rate limits. Worth a one-line confirmation that the platform endpoint caches rather than round-tripping the provider each time — the one assumption the 5m value rests on.
  2. Test covers only the clamp-down (nit). CachesCappedTTL asserts 3600s → 5m via a spy store — a solid regression test; it just doesn't also assert the expires_in < 5m pass-through (an obvious no-op branch).

Scoping is honest

The PR is explicit that this bounds the exposure window and that instant revocation (platform→forge POST /mcp/revoke + downstream 401/403 eviction) is the documented #380 follow-up. Appropriate to land the self-contained bound now. CI fully green.


// Cap the cache lifetime so a platform-side disconnect is enforced within
// delegatedTokenMaxTTL, not the provider token's full lifetime (#380).
if ttl > delegatedTokenMaxTTL {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the fix and it's correct. Clamping before store.Put means memSubjectTokenStore sets expiresAt = now + ≤5m, and its skew-aware Get (now < expiresAt - 30s, then eager delete) forces a re-fetch within ~4m30s — at which point the platform endpoint 403/404s a disconnected subject into ErrNoToken and the #377 gate denies/parks. End-to-end the exposure window drops from ~1h to ≤5m as claimed.

Clamp direction is safe (only shrinks; a sub-5m expires_in passes through, and defaultPlatformTokenTTL == 5m makes the omitted-expires_in path a no-op), and the cap is correctly scoped to the delegated per-subject source only — the agent-principal platformTokenSource keeps its own uncapped cache.

@initializ-mk
initializ-mk merged commit 7c2503a into main Aug 1, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant