Skip to content

fix: 故障转移后无条件更新 Session 绑定 #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ding113 merged 1 commit into from
Nov 27, 2025
Merged

fix: 故障转移后无条件更新 Session 绑定 #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/app/v1/_lib/proxy/forwarder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,13 @@ export class ProxyForwarder {

// ⭐ 成功后绑定 session 到供应商（智能绑定策略）
if (session.sessionId) {
// 使用智能绑定策略（主备模式 + 健康自动回迁）
// 使用智能绑定策略（故障转移优先 + 稳定性优化）
const result = await SessionManager.updateSessionBindingSmart(
session.sessionId,
currentProvider.id,
currentProvider.priority || 0,
totalProvidersAttempted === 1 && attemptCount === 1 // isFirstAttempt
totalProvidersAttempted === 1 && attemptCount === 1, // isFirstAttempt
totalProvidersAttempted > 1 // isFailoverSuccess: 切换过供应商
);

if (result.updated) {
Expand Down
39 changes: 22 additions & 17 deletions src/lib/session-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,29 +418,16 @@ export class SessionManager {
}

/**
* 智能更新 Session 绑定（主备模式 + 健康自动回迁）
* 智能更新 Session 绑定
Copy link
Owner Author

@ding113 ding113 Nov 27, 2025

Choose a reason for hiding this comment

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

🟢 Low: Documentation simplification may reduce maintainability

Why this is a problem: The original docstring contained detailed strategy documentation explaining the smart binding logic (primary-backup mode, health-based migration, priority comparison). The simplified docstring only states "策略：首次绑定用 SET NX；故障转移后无条件更新；其他情况按优先级和熔断状态决策" which loses the detailed explanation of rules A and B.

While the code itself is self-documenting, the original detailed comments helped maintainers understand the rationale behind each decision path.

Suggested fix: Consider preserving some of the original documentation, especially the rules explanation:

/**
 * 智能更新 Session 绑定
 *
 * 策略：
 * 1. 首次绑定：SET NX（避免并发竞争）
 * 2. 故障转移成功：无条件更新（isFailoverSuccess）
 * 3. 其他情况：
 *    a) 新供应商优先级更高 → 直接迁移
 *    b) 否则检查原供应商是否熔断 → 熔断则切换，健康则保持
 */

*
* ⚠️ 职责分离：不做分组权限检查（选择器已保证）
*
* 核心策略：
* 1. 首次绑定：直接绑定到成功的供应商（SET NX 避免并发竞争）
* 2. 重试成功：智能决策
* a) 新供应商优先级更高（数字更小）→ 直接更新（迁移到主供应商）
* b) 新供应商优先级相同或更低 → 检查原供应商健康状态：
* - 原供应商已熔断 → 更新到新供应商（备用供应商接管）
* - 原供应商健康 → 保持原绑定（优先使用主供应商）
*
* @param sessionId - Session ID
* @param newProviderId - 新供应商 ID
* @param newProviderPriority - 新供应商优先级
* @param isFirstAttempt - 是否首次尝试
* @returns { updated: 是否更新, reason: 原因说明, details: 详细说明 }
* 策略：首次绑定用 SET NX；故障转移后无条件更新；其他情况按优先级和熔断状态决策
*/
static async updateSessionBindingSmart(
sessionId: string,
newProviderId: number,
newProviderPriority: number,
isFirstAttempt: boolean = false
isFirstAttempt: boolean = false,
isFailoverSuccess: boolean = false
): Promise<{ updated: boolean; reason: string; details?: string }> {
const redis = getRedisClient();
if (!redis || redis.status !== "ready") {
Expand Down Expand Up @@ -477,6 +464,24 @@ export class SessionManager {

// ========== 情况 2：重试成功（需要智能决策）==========

// 2.0 故障转移成功：无条件更新绑定（减少缓存切换）
Copy link
Owner Author

@ding113 ding113 Nov 27, 2025

Choose a reason for hiding this comment

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

🟡 Medium: Consider adding a clarifying comment for the intentional design choice

Why this is a problem: The isFailoverSuccess code path uses redis.setex() directly which unconditionally overwrites any existing binding. This is inconsistent with the concurrency-safe pattern (SET NX) used in other paths of this function (isFirstAttempt at line 455 and no_previous_binding at line 485).

While the current implementation is correct for the intended behavior (failover success should override existing bindings), the inconsistency might confuse future maintainers.

Suggested fix: Add a clarifying comment:

// 2.0 故障转移成功：无条件更新绑定（减少缓存切换）
// 注意：此处故意使用 setex 而非 SET NX，因为故障转移成功后应覆盖任何现有绑定
if (isFailoverSuccess) {

if (isFailoverSuccess) {
const key = `session:${sessionId}:provider`;
await redis.setex(key, this.SESSION_TTL, newProviderId.toString());

logger.info("SessionManager: Updated binding after failover", {
sessionId,
newProviderId,
newPriority: newProviderPriority,
});

return {
updated: true,
reason: "failover_success",
details: `故障转移成功，绑定到供应商 ${newProviderId}`,
};
}

// 2.1 获取当前绑定的供应商 ID
const currentProviderIdStr = await redis.get(`session:${sessionId}:provider`);
if (!currentProviderIdStr) {
Expand Down
Loading