feat: add tenant-safe versioned session bindings - #1347
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough新增 Redis 版本化 Session 绑定机制，使用 canonical/legacy 镜像、generation CAS、租约、租户安全清理与能力探测，并将 SessionManager、代理流程、刷新逻辑及相关测试迁移到新绑定接口。 ChangesSession 绑定版本化迁移
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tenant-scoped versioned session bindings to prevent unauthorized session hijacking and modifications. It implements atomic Redis operations via new Lua scripts (reconcile, CAS, clear, and terminate) and integrates them into SessionManager, ProxyForwarder, and related proxy handlers by passing the API key owner (keyId). It also updates the Redis client to handle configuration changes dynamically. The review feedback suggests attaching a dummy .catch() handler to the pending operation in withCapabilityProbeDeadline to avoid unhandled promise rejections when a timeout occurs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/v1/_lib/proxy/forwarder.ts (1)
5000-5010: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议：抽取
keyId解析为共享 helper，消除重复。
session.authState?.key?.id ?? session.messageContext?.key?.id ?? null在本文件（Line 5000、5009）及response-handler.ts（Line 1152、1497、1810、1981、2123、3409）、provider-selector.ts（Line 469）多处重复。建议在ProxySession上提供如getOwnerKeyId()的方法集中该逻辑，便于后续统一调整 fail-closed 策略并降低漂移风险。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 5000 - 5010, Extract the repeated key ID resolution into a shared ProxySession helper such as getOwnerKeyId(), preserving the authState key precedence, messageContext fallback, and null default. Update clearSessionProvider and clearSessionProviderBindings plus the corresponding usages in response-handler.ts and provider-selector.ts to call the helper instead of duplicating the optional-chain expression.src/lib/redis/session-binding.ts (1)
238-248: 🚀 Performance & Scalability | 🔵 Trivial集群部署下版本化绑定会被整体禁用，建议明确记录/监控。
canonical key 使用了 hash tag
{...}，而 legacy 镜像 key（session:<id>:provider、session:<id>:key）没有 hash tag。在 Redis Cluster 下，多 key 的EVAL会因不同 slot 触发CROSSSLOT，被isCapabilityError捕获后 capability 置为unavailable，从而始终回退到 legacy 路径——即版本化绑定在集群模式下永不生效。这是当前设计的预期行为，但建议：
- 在部署文档中说明该限制；
- 通过
getVersionedBindingCapabilityState()暴露指标/告警，便于运维发现版本化能力未启用。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/redis/session-binding.ts` around lines 238 - 248, 记录 Redis Cluster 下 canonical 与 legacy key 跨 slot 导致版本化绑定回退为 legacy 的限制，并在部署文档中明确说明。围绕 getVersionedBindingCapabilityState() 暴露可监控的 capability 状态，确保运维能够发现版本化绑定处于 unavailable 并配置指标或告警；不要改变 buildSessionBindingKeys() 的现有回退行为。Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 5000-5010: Extract the repeated key ID resolution into a shared
ProxySession helper such as getOwnerKeyId(), preserving the authState key
precedence, messageContext fallback, and null default. Update
clearSessionProvider and clearSessionProviderBindings plus the corresponding
usages in response-handler.ts and provider-selector.ts to call the helper
instead of duplicating the optional-chain expression.
In `@src/lib/redis/session-binding.ts`:
- Around line 238-248: 记录 Redis Cluster 下 canonical 与 legacy key 跨 slot
导致版本化绑定回退为 legacy 的限制，并在部署文档中明确说明。围绕 getVersionedBindingCapabilityState() 暴露可监控的
capability 状态，确保运维能够发现版本化绑定处于 unavailable 并配置指标或告警；不要改变
buildSessionBindingKeys() 的现有回退行为。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90eebfcc-e3ce-4c9e-a897-55d7cb1c6626
📒 Files selected for processing (23)
package.jsonsrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/lib/redis/client.tssrc/lib/redis/lua-scripts.tssrc/lib/redis/session-binding.tssrc/lib/session-manager.tssrc/lib/session-tracker.tstests/configs/integration.config.tstests/configs/session-binding.config.tstests/integration/session-binding-versioning-redis.test.tstests/unit/lib/redis/client.test.tstests/unit/lib/redis/session-binding.test.tstests/unit/lib/session-manager-binding-smart.test.tstests/unit/lib/session-manager-terminate-provider-sessions.test.tstests/unit/lib/session-manager-terminate-session.test.tstests/unit/lib/session-manager-versioned-binding.test.tstests/unit/lib/session-tracker-cleanup.test.tstests/unit/proxy/provider-selector-cross-type-model.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 626f804297
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces a tenant-scoped, versioned session binding compatibility layer with atomic Lua-based CAS operations, generation-aware bind/clear, null tombstones, and tenant-authorized termination. The design is sound: ownership is verified at every mutation point, the legacy fallback is properly gated behind a capability probe, and error paths return typed results rather than throwing. After a thorough multi-perspective review of all 24 changed files, no issues meeting the reporting threshold were identified.
PR Size: L
- Lines changed: 4,236 (3,934 additions, 302 deletions)
- Files changed: 24 (9 source, 13 test, 2 config)
Split suggestions (recommended for easier review/rollback):
- Lua scripts + capability gate (
lua-scripts.ts,session-binding.ts,redis/client.ts) — the foundational Redis layer and capability probing. Could be merged independently since it's not wired into the request path until the SessionManager adapter consumes it. - SessionManager adapter (
session-manager.ts,session-tracker.ts) — the versioned/legacy dual-write integration into existing bind/clear/terminate flows. - Proxy call-site keyId propagation (
forwarder.ts,provider-selector.ts,response-handler.ts) — threads the API key identity through selection, forwarding, and finalization. - Tests — the 931-line unit suite and 503-line Redis integration suite could travel with their respective source PRs.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness — CAS generation rotation, ABA protection, mirror reconciliation, and null-tombstone semantics verified against Lua scripts
- Security (OWASP Top 10) — tenant isolation enforced via key-scoped canonical keys, ownership verification in every Lua script, fail-closed on foreign/unproven owners
- Error handling — all catch blocks log and return typed results; no silent failures; capability errors vs. data errors vs. operation errors are correctly distinguished
- Type safety — discriminated union result types, no
anyusage, thorough input validation (isPositiveInteger,isValidIdentity) - Documentation accuracy — comments match code behavior; the forwarder
isActualHedgeWinchange correctly documents why the duplicate binding update is deferred to response-handler - Test coverage — 85%+ statements on
session-binding.ts; unit tests cover all mutation types, capability transitions, NOSCRIPT fallback, epoch invalidation; integration suite covers real Redis reconcile/CAS/clear/cooldown/termination - Code clarity — helper functions (
persistBinding,evalBindingScript,handleOperationError) reduce duplication; Lua scripts are well-documented with KEYS/ARGV contracts
Automated review by Claude AI
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 488bc8cd53
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45dac9476d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac3d996ad
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8adf0207d5
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95a94e80c4
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/redis/session-binding.ts`:
- Around line 1466-1474: 更新无作用域终止分支中处理 legacyProvider 和 legacyOwner 的逻辑：不要无条件删除
keys.legacyOwner；参考有作用域分支，在并发恢复出的版本化绑定场景下保留或恢复 owner，必要时按条件一并恢复 provider。确保
READ_OR_RECONCILE_SESSION_BINDING 在 canonical 存在但 legacy_owner 缺失时不会进入
mirror_missing 错误状态，并保留 rejectLegacyMutationAfterCanonicalAppeared 的冲突检查。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f910ed02-4ef5-4e45-9753-877d648c1813
📒 Files selected for processing (8)
src/lib/redis/lua-scripts.tssrc/lib/redis/session-binding.tssrc/lib/session-manager.tstests/integration/session-binding-versioning-redis.test.tstests/unit/lib/redis/session-binding.test.tstests/unit/lib/session-manager-binding-smart.test.tstests/unit/lib/session-manager-terminate-session.test.tstests/unit/lib/session-manager-versioned-binding.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/session-binding-versioning-redis.test.ts
- src/lib/session-manager.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81d9b4f446
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 684e334e5b
ℹ️ 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".
…0723' into agent/discovery-1347-on-integration # Conflicts: # tests/integration/proxy-hedge-lifecycle.test.ts # tests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25f0fa9519
ℹ️ 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".
| environment: "node", | ||
| testFiles: ["tests/unit/lib/redis/session-binding.test.ts"], | ||
| sourceFiles: ["src/lib/redis/session-binding.ts"], | ||
| thresholds: { lines: 80, functions: 80, branches: 75, statements: 80 }, |
There was a problem hiding this comment.
This dedicated coverage target for the new session-binding feature will pass with only 75% branch coverage, so a run of test:coverage:session-binding can report success even when the feature is below the repository's required 80% unit-test coverage. Please raise the branch threshold to 80 or add tests until it can meet the same minimum.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| const result = await redis.eval( | ||
| DELETE_LEGACY_PROVIDER_IF_VALUE, | ||
| 1, | ||
| providerKey, | ||
| providerId.toString() | ||
| ); |
There was a problem hiding this comment.
When the capability probe marks versioned bindings unavailable because EVAL itself is disabled or denied, the legacy fallback still reaches this redis.eval for clears and terminations, so Redis rejects the same command and the catch path returns operation_failed. In those deployments a timed-out sticky provider or admin termination cannot remove session:*:provider until TTL, even though the caller was told legacy fallback is allowed; use a non-script fallback for this case or stop allowing legacy fallback when scripts are unavailable.
Useful? React with 👍 / 👎.
| expectedProviderId: expectedProviderIds | ||
| ? (binding.snapshot.providerId ?? undefined) | ||
| : undefined, |
There was a problem hiding this comment.
For unscoped admin termination this call omits an expected provider, so the Lua termination can linearize after a concurrent failover moves the binding from provider P to provider Q. The generic cleanup below still uses the provider id captured before this call, removing P's active-session indexes while leaving Q's provider:*:active_sessions/refs until TTL; with provider concurrency limits that stale Q entry can falsely consume capacity. Have the versioned termination return the provider it actually cleared and use that for index cleanup.
Useful? React with 👍 / 👎.
4aae243
into
ding113:integration/discovery-stack-20260723
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
This is PR 1/3 for the bounded streaming Discovery work proposed in #1340. It adds the tenant-safe coordination foundation without enabling Discovery or changing the existing Hedge scheduler.
CROSSSLOTfalls back to the existing legacy path and keeps Discovery ineligible.Pcleanup cannot delete a concurrentP -> Qfailover.Rollout
This PR does not start provider fan-out. Existing routing remains authoritative until the follow-up scheduler and the final, default-off configuration switch are both merged and explicitly enabled.
Validation
bun run typecheckbun run format:checkREDIS_URLis configured.Stack
Refs #1340.
Greptile Summary
This PR introduces the tenant-safe, versioned session-binding foundation for the Discovery work. It adds generation-based CAS with ABA protection, null tombstones, cooldown markers, and a capability gate backed by a live Redis probe — without touching any Discovery fan-out logic. Existing routing stays authoritative; the scheduler and feature flag come in follow-up PRs.
lua-scripts.ts): six new multi-key Lua scripts (READ_OR_RECONCILE, CAS, TOUCH, CLEAR, TERMINATE, two lease scripts) plus two single-key conditional scripts for rolling-upgrade mirror safety. All operations validate ownership, mirror consistency, and generation in a single atomic step.session-binding.ts): module-level connection-lifecycle tracking resets capability state on Redis reconnect, funnels all versioned operations through areadyVersionedClientguard, and deduplicates concurrent probes via a cached Promise.TOUCH_SESSION_BINDINGand issues a final touch at stream boundary before side-effects are committed.Confidence Score: 5/5
Safe to merge — no P0 or P1 issues found after thorough analysis of all 25 changed files.
Lua scripts are atomic and fail-closed, ABA protection via generation UUIDs is correct, the capability gate correctly blocks all versioned operations when the probe fails, the hedge heartbeat has proper in-flight deduplication and a complete() barrier before side-effects are committed, and the tenant content-hash key change correctly scopes sessions to keyId. All previously raised concerns are correctly addressed.
No files require special attention. The two largest new files (session-binding.ts and lua-scripts.ts) are the most complex but are well-structured and internally consistent.
Important Files Changed
Reviews (11): Last reviewed commit: "test(discovery): update versioned cleanu..." | Re-trigger Greptile