Repository navigation
🤺 fix: Prefix MCP Tool Cache Write Fences - #17458
Conversation
|
Head: c4f04b1. Both MCP tool-cache ownership fences now use rawKey(); 10 new regression cases cover prefix/separator handling and Redis Cluster slot compatibility. Focused lock suite: 26 passed. |
|
Head: c1ff264. Prefixes both MCP ownership fences; test-only lint issues corrected. Focused lock suite: 26 passed. Real Redis 7 ACL smoke: 24 assertions passed across single-node and three-master cluster configurations, including the original NOPERM failures as negative controls. |
|
Head: 1efd6e1. Both ownership fences carry the configured namespace and retain their guarded cluster slots, including literal-brace prefixes. Fixed independent-review finding R1 (P2). Lock suite: 34 passed. Real Redis ACL smoke: 56 assertions passed. API typecheck and scoped static checks passed. |
|
Independent review complete for head 1efd6e1: no findings. R1 (P2, literal-brace prefix slot regression) verified fixed. Local verification: 34 lock tests, 14 MCP catalog integration tests, 56 real Redis ACL assertions, API build/typecheck, and scoped static checks passed. CI static checks, package builds, and typechecks are green. |
|
@codex review the latest head, final review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1efd6e1b47
ℹ️ 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".
| const globalCacheRawKey = rawKey(ToolCacheKeys.GLOBAL); | ||
| /** Hashes to the same Redis Cluster slot as the unchanged legacy global catalog key. */ | ||
| const globalFenceKey = `tools:global:write-fence:{${redisHashTag(globalCacheRawKey)}}`; | ||
| const globalFenceKey = ownershipFenceKey('tools:global:write-fence', globalCacheRawKey); |
There was a problem hiding this comment.
Preserve fencing across rolling upgrades
During a rolling upgrade, old instances still use the legacy fence key while this head uses the newly namespaced key. If an old writer's distributed lock is lost early (for example, through independent Redis eviction or deletion) while its fence remains, a new instance can claim its different fence and complete a write, after which the delayed old writer can still validate against the legacy fence and overwrite the newer catalog. Keep the legacy and new versions on a shared fencing token/key, or add a migration protocol that checks both, so mixed-version writers continue to supersede one another.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1c9c70b. New writers atomically claim the prefixed and exact legacy fence with one token/deadline, then validate the legacy fence when it is usable. Prefix-only ACLs fall back only after a pre-execution denial; other errors/conflicts abort. Added real Redis old→new and new→old tests with early lock deletion, stale-writer rejection, cleanup, and prefix-restricted ACL coverage. All 9 tests pass on both single-node Redis and a three-master cluster.
|
Head: 1c9c70b. Fixes the mixed-version fence finding with a dual-fence claim and legacy-authoritative guarded writes. Local checks: 57 lock regressions, 14 MCP SDK integration tests, 9 real Redis tests per single-node/cluster configuration, API build/typecheck, and scoped static checks passed. |
|
@codex review the latest head, final review |
|
Independent review complete for head 1c9c70b: no findings. R1 (P2, literal-brace slots) and C1 (P2, mixed-version fences) verified fixed. Reviewer modeled 96 prefix/scope/ACL/topology cases and 5 ambiguous-error cases. Local committed tests and CI separately verify real Redis. Compatibility prerequisite remains: drain old replicas before restricting Redis credentials while old credentials still have access to the legacy fence. Mixed-ACL rolling upgrades are not protected; this prerequisite is operational, not enforced. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c9c70b632
ℹ️ 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".
| }); | ||
| const redis = deps.keyvRedisClient!; | ||
| const args = [token, String(leaseExpiresAt), String(LOCK_FENCE_SAFETY_MS)]; | ||
| let keys = [fenceKey, legacyFenceKey]; |
There was a problem hiding this comment.
Keep the legacy user fence isolated by deployment
When multiple LibreChat deployments share unrestricted Redis with different REDIS_KEY_PREFIX values but the same user/server identifiers, both now claim the same legacy user fence: redisHashTag(generationRawKey) extracts only {userId:serverName}, discarding the deployment prefix. Their actual lock and catalog keys remain separately prefixed, so the deployments can acquire their locks concurrently, but one claim here returns 0 and fails the cache operation as “superseded.” Because every new replica continues claiming the legacy key indefinitely, this permanently reintroduces the cross-deployment interference the namespaced fence is intended to remove; constrain the compatibility bridge to a migration period or otherwise avoid using the unscoped legacy fence between current replicas.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b346a83. Current replicas validate their prefixed ownership fence. The legacy compatibility record holds independent expiring owner entries, so two deployments with the same user/server IDs can proceed concurrently. Claims, guarded writes, and cleanup validate the caller’s entry atomically; cleanup cannot remove a peer’s protection. Old UUID fences still block new writers, and old writers remain excluded while any current owner is active. Added real Redis concurrency, lease/cleanup, and stale-generation/migration tests. All 13 tests pass on single-node Redis and a three-master cluster; the deployment-isolation regression fails on the previous head.
|
Head: b346a83. Fixes cross-deployment contention without dropping rolling-upgrade fencing. Local verification: 57 lock tests, 14 MCP SDK integration tests, 13 real Redis tests per single-node/cluster configuration, API build/typecheck, and scoped static checks passed. Deployment-isolation negative control fails on the previous head. |
|
Independent review complete for b346a83: no findings. R1 (P2, brace-prefix slots), C1 (P2, rolling-version fencing), and C2 (P2, deployment isolation) verified fixed. Local: 57 lock tests, 14 MCP integration tests, 13 real Redis tests per single-node/cluster configuration, API build/typecheck, and scoped static checks passed. CI builds, typechecks, static checks, Redis integration, and Lighthouse passed. |
|
@codex review the latest head, final review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Fixes #17288. MCP tool-cache fences bypassed
REDIS_KEY_PREFIX, causing startup or user-catalogNOPERMfailures with prefix-restricted Redis ACLs.Ownership now stays in a prefixed fence. On unrestricted Redis, the legacy key holds independent expiring compatibility entries rather than one exclusive deployment token. Current deployments sharing user/server IDs can proceed concurrently while old writers remain excluded during active leases. Claims, guarded mutations, and per-owner cleanup are atomic and same-slot. Only pre-execution ACL or incompatible-slot rejections permit prefixed-only operation.
Type of change
Testing
tsc --noEmit, and scoped static checks passed.Risk / compatibility
Drain old replicas before restricting Redis credentials while old credentials still access the legacy fence. Mixed-ACL rolling upgrades are not protected. Empty
{}prefix tags retain a pre-existing Redis Cluster slot-calculation limitation. Older replicas retain their existing cross-deployment contention while they hold legacy fences.Screenshots / recordings
No user-facing change.