TT-5486: Fix session cache access rights race#8320
Conversation
Store cloned session states in the local session cache so cached entries do not share AccessRights maps with request-local sessions that policy application mutates. Add a regression test proving cached sessions remain isolated, plus an opt-in crash reproducer that can be run with TYK_RUN_SESSION_CACHE_RACE_REPRO=1 to demonstrate the old concurrent map failure.
|
This PR addresses a critical race condition causing gateway panics ( The fix is to store a deep copy of the session object in the cache by using Files Changed AnalysisThe changes are focused and well-tested:
Architecture & Impact Assessment
Data Flow: Before vs. AfterThe diagrams below illustrate how the fix isolates the cached data from live request data. Before (Shared Map Reference): graph TD
A[Request] --> B{CheckSessionAndIdentityForValidKey};
B --> C[Load SessionState];
C --|Shallow Copy|--> D[Request-Local Session];
C --|Shallow Copy|--> E[Local Session Cache];
D --|Mutates|--> F(Shared AccessRights Map);
E --|Another request reads or clones|--> F;
After (Isolated Map via Clone): graph TD
A[Request] --> B{CheckSessionAndIdentityForValidKey};
B --> C[Load SessionState];
C --> C_Clone(Clone SessionState);
C -- Original object --> D[Request-Local Session];
C_Clone -- Cloned object --> E[Local Session Cache];
D -- Mutates --> F(Original AccessRights Map);
E -- Another request reads --> G(Isolated Cached AccessRights Map);
Scope Discovery & Context Expansion
References
Metadata
Powered by Visor from Probelabs Last updated: 2026-06-17T12:11:50.846Z | Triggered by: pr_updated | Commit: 9726025 💡 TIP: You can chat with Visor using |
🎯 Recommended Merge TargetsBased on JIRA ticket TT-5486: panic: fatal error: concurrent map iteration and map write Fix Version: Tyk 5.8.15Required:
Fix Version: Tyk 5.13.1Required:
Fix Version: Tyk 5.14.0
Required:
📋 Workflow
|
✅ Security Check PassedNo security issues found – changes LGTM. ✅ Architecture Check PassedNo architecture issues found – changes LGTM. Performance Issues (1)
✅ Security Check PassedNo security issues found – changes LGTM. \n\n✅ Architecture Check PassedNo architecture issues found – changes LGTM. \n\n \n\nQuality Issues (1)
Powered by Visor from Probelabs Last updated: 2026-06-17T12:10:21.005Z | Triggered by: pr_updated | Commit: 9726025 💡 TIP: You can chat with Visor using |
bec76d9 to
5ac2519
Compare
|
SentinelOne CNS Hardcoded Secret Detector SentinelOne CNS is a cloud-agnostic, agentless CSPM & CWPP solution that continuously detects and prevents vulnerabilities that have the highest probability of being exploited in Azure, AWS, Google Cloud, and Kubernetes. |
🚨 Jira Linter FailedCommit: The Jira linter failed to validate your PR. Please check the error details below: 🔍 Click to view error detailsNext Steps
This comment will be automatically deleted once the linter passes. |
|
|
/release to release-5.8 |
|
/release to release-5.13 |
|
✅ Cherry-pick successful. A PR was created: #8329 |
|
✅ Cherry-pick successful. A PR was created: #8330 |
3 participants
Summary
This fixes the gateway panic seen under high authentication load:
The fix stores cloned
SessionStatevalues in the local session cache, so cached entries do not shareAccessRightsmap internals with request-local sessions that policy application mutates.Changes:
session.Clone()inSessionCache.Set(...)on both session-store and auth-store cache write paths.AccessRightswith the request-local session.Root Cause
SessionStateis a struct, but several of its fields are reference types. In particular,AccessRightsis a map.Before this PR, the gateway could cache a
SessionStatevalue before policy application finished:That stores a struct copy, but not a deep copy of the maps. As a result, the cached session and the request-local session could share the same
AccessRightsmap.The request-local session is then mutated by policy application and MCP normalization. At the same time, another request can load the cached value and clone it. That means one goroutine can iterate/clone
AccessRightswhile another goroutine writes to the same underlying map, which Go treats as a fatal runtime error.Request And Cache Lifecycle
For each authenticated request, the gateway looks up a session from either:
The session is cloned into a request-local
SessionState. That local copy can then be safely modified for the duration of that request by policy application, endpoint normalization, key hash setup,Touch(), and other request-scoped work.The important boundary is ownership:
This PR keeps that model by cloning before storing in the cache.
Why The Local Cache Is Still Needed
The local session cache avoids repeatedly hitting the backing session/auth stores on every request. That matters on the hot authentication path, especially under high request concurrency.
The cache is still valid here; the bug was not the existence of the cache. The bug was that the cached value and request-local value were accidentally sharing mutable map internals.
Stale Data Considerations
There is always some existing stale-data behavior with a local cache: cached session fields can remain in memory until cache expiry or explicit invalidation.
This PR does not increase that stale-data window. It preserves the existing cache-hit behavior: cache hits still clone the cached session and apply policies during request validation.
That means policy changes are still evaluated on the request-local copy, while the cached raw session remains isolated from request mutations.
Why This Fix Is Narrow
An alternative approach is to add locking inside
SessionState, but that is a larger ownership change and is risky becauseSessionStateis copied by value in many places.Putting a
sync.RWMutexdirectly insideSessionStatemakesClone()and other value copies vulnerable to copying a live lock. That can introduce deadlocks andgo vet -copylocksfailures unless the entire type ownership model is redesigned.For this bug, we do not need to make all
SessionStatemap access globally synchronized. We only need to ensure that the local cache never stores maps that are still owned by an active request.Reproduction
The opt-in test intentionally recreates the old shallow-copy behavior:
AccessRightsmap.SessionCache.Set(cacheKey, session, ...)behavior.It is skipped by default and only runs when explicitly requested:
Observed output includes:
Validation
Normal focused regression path:
Result:
TestCheckSessionAndIdentityForValidKey_CachesSessionWithoutSharingAccessRights: passed.TestOnDemandReproduceSessionCacheAccessRightsConcurrentMapCrash: skipped by default.Opt-in reproducer:
Result:
Pre-push hook: