fix: run reactive rectifiers during streaming hedge#945
Conversation
📝 WalkthroughWalkthrough在代理转发器中添加了面向Anthropic提供商的“响应式整流器（ReactiveRectifier）”重试流程：新增内部重试状态与结果类型、整流应用函数与持久化辅助、对冲（hedge）尝试数据扩展及重构的尝试生命周期管理；相关测试补充了思维署名与思维预算整流+重试场景。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the proxy forwarder by integrating Anthropic reactive rectifier logic into streaming hedge attempts. This allows the system to intelligently modify problematic requests and retry them with the same provider, improving success rates for streaming requests that encounter specific Anthropic errors. The changes also ensure that all rectification actions are properly audited, providing better visibility into the system's retry mechanisms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
bug
![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 effectively refactors the reactive rectifier logic for Anthropic providers and extends its functionality to streaming hedge attempts. The code is now more modular and less repetitive, especially with the introduction of helper functions like tryApplyReactiveAnthropicRectifier and buildRetryFailedChainEntry. My main feedback is a suggestion to further reduce duplication within the new tryApplyReactiveAnthropicRectifier function for improved maintainability.
| async function tryApplyReactiveAnthropicRectifier(params: { | ||
| provider: Provider; | ||
| requestSession: ProxySession; | ||
| persistSession: ProxySession; | ||
| errorMessage: string; | ||
| attemptNumber: number; | ||
| retryAttemptNumber: number; | ||
| retryState: ReactiveRectifierRetryState; | ||
| }): Promise<ReactiveRectifierResult> { | ||
| const { | ||
| provider, | ||
| requestSession, | ||
| persistSession, | ||
| errorMessage, | ||
| attemptNumber, | ||
| retryAttemptNumber, | ||
| } = params; | ||
| const isAnthropicProvider = | ||
| provider.providerType === "claude" || provider.providerType === "claude-auth"; | ||
|
|
||
| if (!isAnthropicProvider) { | ||
| return { matched: false }; | ||
| } | ||
|
|
||
| const signatureTrigger = detectThinkingSignatureRectifierTrigger(errorMessage); | ||
| if (signatureTrigger) { | ||
| const settings = await getCachedSystemSettings(); | ||
| const enabled = settings.enableThinkingSignatureRectifier ?? true; | ||
|
|
||
| if (!enabled) { | ||
| return { matched: false }; | ||
| } | ||
|
|
||
| if (params.retryState.thinkingSignatureRetried) { | ||
| return { | ||
| matched: true, | ||
| applied: false, | ||
| reason: "already_retried", | ||
| rectifierType: "thinking_signature_rectifier", | ||
| trigger: signatureTrigger, | ||
| }; | ||
| } | ||
|
|
||
| const requestDetailsBeforeRectify = buildRequestDetails(requestSession); | ||
| const rectified = rectifyAnthropicRequestMessage( | ||
| requestSession.request.message as Record<string, unknown> | ||
| ); | ||
|
|
||
| addSpecialSettingForPersistence(requestSession, persistSession, { | ||
| type: "thinking_signature_rectifier", | ||
| scope: "request", | ||
| hit: rectified.applied, | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| trigger: signatureTrigger, | ||
| attemptNumber, | ||
| retryAttemptNumber, | ||
| removedThinkingBlocks: rectified.removedThinkingBlocks, | ||
| removedRedactedThinkingBlocks: rectified.removedRedactedThinkingBlocks, | ||
| removedSignatureFields: rectified.removedSignatureFields, | ||
| }); | ||
| await persistSpecialSettings(persistSession); | ||
|
|
||
| if (!rectified.applied) { | ||
| return { | ||
| matched: true, | ||
| applied: false, | ||
| reason: "not_applicable", | ||
| rectifierType: "thinking_signature_rectifier", | ||
| trigger: signatureTrigger, | ||
| }; | ||
| } | ||
|
|
||
| params.retryState.thinkingSignatureRetried = true; | ||
| return { | ||
| matched: true, | ||
| applied: true, | ||
| rectifierType: "thinking_signature_rectifier", | ||
| trigger: signatureTrigger, | ||
| requestDetailsBeforeRectify, | ||
| }; | ||
| } | ||
|
|
||
| const budgetTrigger = detectThinkingBudgetRectifierTrigger(errorMessage); | ||
| if (!budgetTrigger) { | ||
| return { matched: false }; | ||
| } | ||
|
|
||
| const settings = await getCachedSystemSettings(); | ||
| const enabled = settings.enableThinkingBudgetRectifier ?? true; | ||
|
|
||
| if (!enabled) { | ||
| return { matched: false }; | ||
| } | ||
|
|
||
| if (params.retryState.thinkingBudgetRetried) { | ||
| return { | ||
| matched: true, | ||
| applied: false, | ||
| reason: "already_retried", | ||
| rectifierType: "thinking_budget_rectifier", | ||
| trigger: budgetTrigger, | ||
| }; | ||
| } | ||
|
|
||
| const requestDetailsBeforeRectify = buildRequestDetails(requestSession); | ||
| const rectified = rectifyThinkingBudget( | ||
| requestSession.request.message as Record<string, unknown> | ||
| ); | ||
|
|
||
| addSpecialSettingForPersistence(requestSession, persistSession, { | ||
| type: "thinking_budget_rectifier", | ||
| scope: "request", | ||
| hit: rectified.applied, | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| trigger: budgetTrigger, | ||
| attemptNumber, | ||
| retryAttemptNumber, | ||
| before: rectified.before, | ||
| after: rectified.after, | ||
| }); | ||
| await persistSpecialSettings(persistSession); | ||
|
|
||
| if (!rectified.applied) { | ||
| return { | ||
| matched: true, | ||
| applied: false, | ||
| reason: "not_applicable", | ||
| rectifierType: "thinking_budget_rectifier", | ||
| trigger: budgetTrigger, | ||
| }; | ||
| } | ||
|
|
||
| params.retryState.thinkingBudgetRetried = true; | ||
| return { | ||
| matched: true, | ||
| applied: true, | ||
| rectifierType: "thinking_budget_rectifier", | ||
| trigger: budgetTrigger, | ||
| requestDetailsBeforeRectify, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This function is a great step towards centralizing the rectifier logic. However, there's significant code duplication between the handling of signatureTrigger and budgetTrigger. The logic for checking settings, retries, applying rectification, and persisting audit entries is nearly identical in both blocks.
To improve maintainability and reduce code duplication, consider refactoring this into a more generic helper function. This helper could take rectifier-specific logic as arguments, such as:
- The trigger detection function (
detectThinkingSignatureRectifierTriggerordetectThinkingBudgetRectifierTrigger) - The setting key to check for enablement (
enableThinkingSignatureRectifierorenableThinkingBudgetRectifier) - The retry state property to check and update (
thinkingSignatureRetriedorthinkingBudgetRetried) - The rectification function (
rectifyAnthropicRequestMessageorrectifyThinkingBudget) - A function to build the specific
SpecialSettingobject for auditing.
This would make tryApplyReactiveAnthropicRectifier much more concise and easier to extend with new rectifiers in the future.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3224-3254: When a reactive rectifier triggers a retry the code
increments attempt.requestAttemptCount and calls runAttempt(attempt) then
returns, but it skips clearing the threshold timer so the original
thresholdTimer can still fire and incorrectly trigger hedging; before calling
runAttempt(attempt) (inside the reactive rectifier branch handling in
ProxyForwarder) clear/reset attempt.thresholdTimer (e.g. clearTimeout and set to
null) so the timer cannot fire during the retry, then call runAttempt(attempt)
and return; reference symbols: attempt, attempt.requestAttemptCount,
attempt.thresholdTimer, runAttempt.
🪄 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: f0200a78-b82b-4748-8643-0fd0e950562a
📒 Files selected for processing (2)
src/app/v1/_lib/proxy/forwarder.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| attempt.requestAttemptCount += 1; | ||
| runAttempt(attempt); | ||
| return; |
There was a problem hiding this comment.
When tryApplyReactiveAnthropicRectifier returns applied: true, the function calls runAttempt(attempt) and returns early — but attempt.thresholdTimer is never cleared. The hedge threshold timer was started at startAttempt time and fires at launchTime + firstByteTimeoutMs, regardless of any mid-flight rectifier retry.
This causes two problems:
-
Misleading audit entry: When the timer fires during the retry, it still adds a
"hedge_triggered"chain entry (reason: "hedge_triggered") even though the provider never actually timed out — it returned a 400 error that was rectified and retried. This is confirmed by the test: in the signature-error test, provider2's timer fires att = 200ms(100ms after p2 launched) while the rectifier retry is in progress (t = 150ms → 330ms), producing a spurious"hedge_triggered"audit record. -
Unnecessary
launchAlternative()call: The timer fires and callslaunchAlternative(). In the test this is masked because the mock is exhausted and returnsnull(settingnoMoreProviders = true), but in production with a real provider pool a third provider would be launched unnecessarily while the rectifier retry is already in progress.
The fix should clear (and optionally re-arm) the threshold timer before launching the retry:
// Clear the stale threshold timer before re-launching
if (attempt.thresholdTimer) {
clearTimeout(attempt.thresholdTimer);
attempt.thresholdTimer = null;
}
// Optionally re-arm a fresh timer for the retry here
attempt.requestAttemptCount += 1;
runAttempt(attempt);
return;Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3251-3253
Comment:
**Threshold timer not cleared before rectifier retry**
When `tryApplyReactiveAnthropicRectifier` returns `applied: true`, the function calls `runAttempt(attempt)` and returns early — but `attempt.thresholdTimer` is never cleared. The hedge threshold timer was started at `startAttempt` time and fires at `launchTime + firstByteTimeoutMs`, regardless of any mid-flight rectifier retry.
This causes two problems:
1. **Misleading audit entry**: When the timer fires during the retry, it still adds a `"hedge_triggered"` chain entry (`reason: "hedge_triggered"`) even though the provider never actually timed out — it returned a 400 error that was rectified and retried. This is confirmed by the test: in the signature-error test, provider2's timer fires at `t = 200ms` (100ms after p2 launched) while the rectifier retry is in progress (`t = 150ms → 330ms`), producing a spurious `"hedge_triggered"` audit record.
2. **Unnecessary `launchAlternative()` call**: The timer fires and calls `launchAlternative()`. In the test this is masked because the mock is exhausted and returns `null` (setting `noMoreProviders = true`), but in production with a real provider pool a third provider would be launched unnecessarily while the rectifier retry is already in progress.
The fix should clear (and optionally re-arm) the threshold timer before launching the retry:
```typescript
// Clear the stale threshold timer before re-launching
if (attempt.thresholdTimer) {
clearTimeout(attempt.thresholdTimer);
attempt.thresholdTimer = null;
}
// Optionally re-arm a fresh timer for the retry here
attempt.requestAttemptCount += 1;
runAttempt(attempt);
return;
```
How can I resolve this? If you propose a fix, please make it concise.| .then(async (response) => { | ||
| if (settled || winnerCommitted) { |
There was a problem hiding this comment.
attempt.settled check in early-cancellation guard
The guard at the top of the .then() block checks settled || winnerCommitted but not attempt.settled. In contrast, commitWinner (line 3293) checks all three: settled || winnerCommitted || attempt.settled.
If attempt.settled becomes true between when runAttempt was called (for a rectifier retry) and when the response arrives (e.g., the threshold timer fires and drives the attempt through a separate handleAttemptFailure path that sets attempt.settled = true), the response body will not be cancelled in this guard. The code will proceed to set attempt.response, acquire a reader, and only then be blocked by commitWinner's guard — leaving the response body open unnecessarily.
| .then(async (response) => { | |
| if (settled || winnerCommitted) { | |
| if (settled || winnerCommitted || attempt.settled) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3094-3095
Comment:
**Missing `attempt.settled` check in early-cancellation guard**
The guard at the top of the `.then()` block checks `settled || winnerCommitted` but not `attempt.settled`. In contrast, `commitWinner` (line 3293) checks all three: `settled || winnerCommitted || attempt.settled`.
If `attempt.settled` becomes `true` between when `runAttempt` was called (for a rectifier retry) and when the response arrives (e.g., the threshold timer fires and drives the attempt through a separate `handleAttemptFailure` path that sets `attempt.settled = true`), the response body will not be cancelled in this guard. The code will proceed to set `attempt.response`, acquire a reader, and only then be blocked by `commitWinner`'s guard — leaving the response body open unnecessarily.
```suggestion
if (settled || winnerCommitted || attempt.settled) {
```
How can I resolve this? If you propose a fix, please make it concise.| buildRetryFailedChainEntry( | ||
| attempt.provider, | ||
| attempt.endpointAudit, | ||
| attempt.sequence, | ||
| error, | ||
| errorMessage, | ||
| reactiveRectifierResult.requestDetailsBeforeRectify | ||
| ) |
There was a problem hiding this comment.
attempt.sequence used as attemptNumber in retry chain entry
buildRetryFailedChainEntry is called with attempt.sequence (the hedge participant index: 1, 2, 3, …) as attemptNumber. In the standard (non-hedge) retry path the same helper is called with attemptCount (also 1, 2, 3, … but measuring how many times this provider has been tried). These two counters mean different things: sequence reflects the hedge slot, while the standard path's attemptCount reflects the retry iteration.
For the hedge rectifier case the chain entry will always show attemptNumber = attempt.sequence (e.g. 2 for the second hedge participant), regardless of whether this is the first or a subsequent rectifier retry. This is inconsistent with the standard path and could make audit data harder to interpret. Consider using attempt.requestAttemptCount here, which tracks per-attempt retries and is already incremented just below.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3241-3248
Comment:
**`attempt.sequence` used as `attemptNumber` in retry chain entry**
`buildRetryFailedChainEntry` is called with `attempt.sequence` (the hedge participant index: 1, 2, 3, …) as `attemptNumber`. In the standard (non-hedge) retry path the same helper is called with `attemptCount` (also 1, 2, 3, … but measuring how many times this provider has been tried). These two counters mean different things: `sequence` reflects the hedge slot, while the standard path's `attemptCount` reflects the retry iteration.
For the hedge rectifier case the chain entry will always show `attemptNumber = attempt.sequence` (e.g. `2` for the second hedge participant), regardless of whether this is the first or a subsequent rectifier retry. This is inconsistent with the standard path and could make audit data harder to interpret. Consider using `attempt.requestAttemptCount` here, which tracks per-attempt retries and is already incremented just below.
How can I resolve this? If you propose a fix, please make it concise.![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 refactors reactive rectifier logic for streaming hedge scenarios. While the overall refactoring is sound, there is a critical threshold-timer lifetime bug that can produce incorrect audit data and trigger unnecessary provider launches in production.
PR Size: L
- Lines changed: 1028 (687 additions, 341 deletions)
- Files changed: 2
Note: The PR is labeled size/XL but with only 2 changed files, this is a focused change that doesn't require splitting.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 1 | 1 |
| 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 |
Critical Issues (Must Fix)
[LOGIC-BUG] Threshold timer not cleared before rectifier retry
When tryApplyReactiveAnthropicRectifier returns applied: true, the function calls runAttempt(attempt) and returns early — but attempt.thresholdTimer is never cleared. The hedge threshold timer was started at startAttempt time and fires at launchTime + firstByteTimeoutMs, regardless of any mid-flight rectifier retry.
This causes two problems:
- Misleading audit entry: When the timer fires during the retry, it adds a spurious
"hedge_triggered"chain entry even though the provider returned a rectifiable error (not a timeout). - Unnecessary provider launch: The timer calls
launchAlternative(), which can launch a third provider unnecessarily while the rectifier retry is already in progress.
Location: src/app/v1/_lib/proxy/forwarder.ts lines 3251-3253
Suggested fix:
// Clear the stale threshold timer before re-launching
if (attempt.thresholdTimer) {
clearTimeout(attempt.thresholdTimer);
attempt.thresholdTimer = null;
}
attempt.requestAttemptCount += 1;
runAttempt(attempt);
return;Medium Priority Issues (Should Fix)
[LOGIC-BUG] Missing attempt.settled check in early-cancellation guard
The guard at the top of the .then() block (line 3095) checks settled || winnerCommitted but not attempt.settled. In contrast, commitWinner (line 3293) checks all three: settled || winnerCommitted || attempt.settled.
If attempt.settled becomes true between when runAttempt was called and when the response arrives (e.g., the threshold timer fires and drives the attempt through a separate handleAttemptFailure path), the response body will not be cancelled in this guard. The code will proceed to set attempt.response, acquire a reader, and only then be blocked by commitWinner's guard — leaving the response body open unnecessarily.
Suggested fix:
if (settled || winnerCommitted || attempt.settled) {Low Priority Issues (Author's Discretion)
[NAMING-POOR] attempt.sequence used as attemptNumber in retry chain entry
buildRetryFailedChainEntry is called with attempt.sequence (the hedge participant index: 1, 2, 3...) as attemptNumber. In the standard (non-hedge) retry path, the same helper is called with attemptCount (retry iteration). These mean different things.
For hedge rectifier retries, the chain entry will always show attemptNumber = attempt.sequence regardless of whether this is the first or a subsequent rectifier retry. Consider using attempt.requestAttemptCount which tracks per-attempt retries.
Location: Line 3244
Review Coverage
- Logic and correctness - Issues identified (see above)
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (2 new tests for rectifier retry scenarios)
- Code clarity - Good
Notes
- The Greptile summary correctly identified the threshold-timer issue
- Test coverage for the new rectifier retry scenarios is present but doesn't assert on the spurious "hedge_triggered" audit entry
- The extraction of helper functions (
addSpecialSettingForPersistence,buildRetryFailedChainEntry,getReactiveRectifierDisplayName) improves code maintainability
Automated review by Claude AI
| ): void { | ||
| ownerSession.addSpecialSetting(setting); | ||
| if (persistSession !== ownerSession) { | ||
| persistSession.addSpecialSetting(setting); |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] specialSettings can be dropped when a different shadow session wins the hedge
Why this is a problem: addSpecialSettingForPersistence() writes rectifier audit entries into the main session when persistSession !== ownerSession (see src/app/v1/_lib/proxy/forwarder.ts:388-390: persistSession.addSpecialSetting(setting);). But if a different shadow attempt later wins, syncWinningAttemptSession() overwrites the target session’s specialSettings from the winner snapshot (see src/app/v1/_lib/proxy/forwarder.ts:3673: targetState.specialSettings = [...sourceState.specialSettings];), which can discard those previously-persisted audit entries. The streaming finalization path persists specialSettings again from the winning session (src/app/v1/_lib/proxy/response-handler.ts:2136), so this can silently erase rectifier audit data.
Suggested fix:
// src/app/v1/_lib/proxy/forwarder.ts (syncWinningAttemptSession)
const existingSpecialSettings = new Set(
targetState.specialSettings.map((setting) => JSON.stringify(setting))
);
const mergedSpecialSettings = [...targetState.specialSettings];
for (const setting of sourceState.specialSettings) {
const key = JSON.stringify(setting);
if (existingSpecialSettings.has(key)) continue;
existingSpecialSettings.add(key);
mergedSpecialSettings.push(setting);
}
targetState.specialSettings = mergedSpecialSettings;
There was a problem hiding this comment.
Code Review Summary
This PR makes a large refactor in src/app/v1/_lib/proxy/forwarder.ts to reuse reactive rectifier logic for both standard retries and streaming first-byte hedge attempts. One high-risk correctness issue remains: rectifier audit entries added to the main session can be dropped when a different shadow session wins the hedge, causing persisted special_settings to be incomplete.
PR Size: XL
- Lines changed: 1028
- Files changed: 2
- Split suggestion (XL): Split into (1) pure refactor/extraction of reactive-rectifier helpers and chain-entry builders, and (2) streaming-hedge behavior changes + tests, to reduce review surface area and isolate behavior changes.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
src/app/v1/_lib/proxy/forwarder.ts:388/src/app/v1/_lib/proxy/forwarder.ts:3673- Hedge winner sync overwritesspecialSettings, which can drop rectifier audit entries previously added to the main session from other (losing) hedge attempts. Confidence: 85.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#945(“fix: run reactive rectifiers during streaming hedge”); changed files:src/app/v1/_lib/proxy/forwarder.ts,tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts. - PR size computed as XL (687 additions + 341 deletions = 1028 lines; 2 files) and applied label
size/XL. - Posted 1 inline comment ([HIGH] [LOGIC-BUG]) on
src/app/v1/_lib/proxy/forwarder.ts:389aboutspecialSettingsbeing overwritten when a different shadow session wins the hedge, potentially dropping rectifier audit entries (discussion:https://github.com/ding113/claude-code-hub/pull/945#discussion_r2954323197). - Submitted the required review summary via
gh pr review.
- Add attempt.settled guard in .then() callback to prevent stale response processing - Clear thresholdTimer before rectifier retry to avoid spurious hedge triggers - Use requestAttemptCount instead of sequence for accurate retry chain entries - Merge specialSettings on hedge winner sync to preserve rectifier audit data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| const reactiveRectifierResult = await tryApplyReactiveAnthropicRectifier({ | ||
| provider: attempt.provider, | ||
| requestSession: attempt.session, | ||
| persistSession: session, | ||
| errorMessage, | ||
| attemptNumber: attempt.requestAttemptCount, | ||
| retryAttemptNumber: attempt.requestAttemptCount + 1, | ||
| retryState: attempt.reactiveRectifierRetryState, | ||
| }); | ||
|
|
||
| if (reactiveRectifierResult.matched) { | ||
| if (!reactiveRectifierResult.applied) { | ||
| if (reactiveRectifierResult.reason === "not_applicable") { | ||
| logger.info( | ||
| `ProxyForwarder: ${getReactiveRectifierDisplayName( | ||
| reactiveRectifierResult.rectifierType | ||
| )} not applicable in hedge, skipping retry`, | ||
| { | ||
| providerId: attempt.provider.id, | ||
| providerName: attempt.provider.name, | ||
| trigger: reactiveRectifierResult.trigger, | ||
| participantSequence: attempt.sequence, | ||
| attemptNumber: attempt.requestAttemptCount, | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR; | ||
| lastErrorCategory = errorCategory; | ||
| } else { | ||
| logger.info( | ||
| `ProxyForwarder: ${getReactiveRectifierDisplayName( | ||
| reactiveRectifierResult.rectifierType | ||
| )} applied in hedge, retrying same provider`, | ||
| { | ||
| providerId: attempt.provider.id, | ||
| providerName: attempt.provider.name, | ||
| trigger: reactiveRectifierResult.trigger, | ||
| participantSequence: attempt.sequence, | ||
| attemptNumber: attempt.requestAttemptCount, | ||
| willRetryAttemptNumber: attempt.requestAttemptCount + 1, | ||
| } | ||
| ); | ||
|
|
||
| session.addProviderToChain( | ||
| attempt.provider, | ||
| buildRetryFailedChainEntry( | ||
| attempt.provider, | ||
| attempt.endpointAudit, | ||
| attempt.requestAttemptCount, | ||
| error, | ||
| errorMessage, | ||
| reactiveRectifierResult.requestDetailsBeforeRectify | ||
| ) | ||
| ); | ||
|
|
||
| if (attempt.thresholdTimer) { | ||
| clearTimeout(attempt.thresholdTimer); | ||
| attempt.thresholdTimer = null; | ||
| } | ||
| attempt.requestAttemptCount += 1; | ||
| runAttempt(attempt); |
There was a problem hiding this comment.
runAttempt after two async awaits
handleAttemptFailure re-checks settled || winnerCommitted || attempt.settled only at the very top (line 3154). However, two await calls follow before runAttempt(attempt) is reached — categorizeErrorAsync (line 3158) and tryApplyReactiveAnthropicRectifier (line 3195, which itself awaits getCachedSystemSettings). During either of these async gaps another hedge attempt could win and set winnerCommitted = true.
As a result, runAttempt(attempt) at line 3256 is called even though a winner has already been committed. The new provider request cannot be cancelled until the response arrives (when the .then() guard at line 3095 finally catches it). This wastes a full round-trip to the upstream provider.
A re-guard immediately before the retry branch would prevent the redundant call:
| const reactiveRectifierResult = await tryApplyReactiveAnthropicRectifier({ | |
| provider: attempt.provider, | |
| requestSession: attempt.session, | |
| persistSession: session, | |
| errorMessage, | |
| attemptNumber: attempt.requestAttemptCount, | |
| retryAttemptNumber: attempt.requestAttemptCount + 1, | |
| retryState: attempt.reactiveRectifierRetryState, | |
| }); | |
| if (reactiveRectifierResult.matched) { | |
| if (!reactiveRectifierResult.applied) { | |
| if (reactiveRectifierResult.reason === "not_applicable") { | |
| logger.info( | |
| `ProxyForwarder: ${getReactiveRectifierDisplayName( | |
| reactiveRectifierResult.rectifierType | |
| )} not applicable in hedge, skipping retry`, | |
| { | |
| providerId: attempt.provider.id, | |
| providerName: attempt.provider.name, | |
| trigger: reactiveRectifierResult.trigger, | |
| participantSequence: attempt.sequence, | |
| attemptNumber: attempt.requestAttemptCount, | |
| } | |
| ); | |
| } | |
| errorCategory = ErrorCategory.NON_RETRYABLE_CLIENT_ERROR; | |
| lastErrorCategory = errorCategory; | |
| } else { | |
| logger.info( | |
| `ProxyForwarder: ${getReactiveRectifierDisplayName( | |
| reactiveRectifierResult.rectifierType | |
| )} applied in hedge, retrying same provider`, | |
| { | |
| providerId: attempt.provider.id, | |
| providerName: attempt.provider.name, | |
| trigger: reactiveRectifierResult.trigger, | |
| participantSequence: attempt.sequence, | |
| attemptNumber: attempt.requestAttemptCount, | |
| willRetryAttemptNumber: attempt.requestAttemptCount + 1, | |
| } | |
| ); | |
| session.addProviderToChain( | |
| attempt.provider, | |
| buildRetryFailedChainEntry( | |
| attempt.provider, | |
| attempt.endpointAudit, | |
| attempt.requestAttemptCount, | |
| error, | |
| errorMessage, | |
| reactiveRectifierResult.requestDetailsBeforeRectify | |
| ) | |
| ); | |
| if (attempt.thresholdTimer) { | |
| clearTimeout(attempt.thresholdTimer); | |
| attempt.thresholdTimer = null; | |
| } | |
| attempt.requestAttemptCount += 1; | |
| runAttempt(attempt); | |
| if (settled || winnerCommitted || attempt.settled) { | |
| return; | |
| } | |
| if (attempt.thresholdTimer) { | |
| clearTimeout(attempt.thresholdTimer); | |
| attempt.thresholdTimer = null; | |
| } | |
| attempt.requestAttemptCount += 1; | |
| runAttempt(attempt); | |
| return; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/forwarder.ts
Line: 3195-3256
Comment:
**Stale guard before `runAttempt` after two async awaits**
`handleAttemptFailure` re-checks `settled || winnerCommitted || attempt.settled` only at the very top (line 3154). However, two `await` calls follow before `runAttempt(attempt)` is reached — `categorizeErrorAsync` (line 3158) and `tryApplyReactiveAnthropicRectifier` (line 3195, which itself awaits `getCachedSystemSettings`). During either of these async gaps another hedge attempt could win and set `winnerCommitted = true`.
As a result, `runAttempt(attempt)` at line 3256 is called even though a winner has already been committed. The new provider request cannot be cancelled until the response arrives (when the `.then()` guard at line 3095 finally catches it). This wastes a full round-trip to the upstream provider.
A re-guard immediately before the retry branch would prevent the redundant call:
```suggestion
if (settled || winnerCommitted || attempt.settled) {
return;
}
if (attempt.thresholdTimer) {
clearTimeout(attempt.thresholdTimer);
attempt.thresholdTimer = null;
}
attempt.requestAttemptCount += 1;
runAttempt(attempt);
return;
```
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/v1/_lib/proxy/forwarder.ts`:
- Around line 3082-3085: The retry path is clearing first-byte timeout by
setting attempt.provider.firstByteTimeoutStreamingMs to 0 (providerForRequest)
and then clearing thresholdTimer before calling runAttempt(attempt), which can
allow retries to hang indefinitely; preserve or restore the original first-byte
timeout and ensure the threshold timer is (re)initialized before invoking
runAttempt. Specifically, keep the original firstByteTimeoutStreamingMs value on
the attempt (or attach it as e.g. attempt._originalFirstByteTimeout), avoid
replacing it with 0 in providerForRequest (or restore it on attempt.provider
before retry), and restart the thresholdTimer prior to calling
runAttempt(attempt) so the same timeout protection applies on retries.
🪄 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: 9295ee04-de86-42cc-84de-162e361df2ea
📒 Files selected for processing (1)
src/app/v1/_lib/proxy/forwarder.ts
| const providerForRequest = | ||
| attempt.provider.firstByteTimeoutStreamingMs > 0 | ||
| ? { ...attempt.provider, firstByteTimeoutStreamingMs: 0 } | ||
| : attempt.provider; |
There was a problem hiding this comment.
Rectifier 重试路径可能失去超时保护并导致请求悬挂
在 Line [3083-3085] 里将 firstByteTimeoutStreamingMs 置为 0，而在 Line [3251-3257] 里重试前清掉 thresholdTimer 后直接 runAttempt(attempt)，未重置阈值计时器。两者叠加后，若重试请求在首字节前卡住且没有可用备选 provider，可能长期不返回。
🔧 建议修复（示例）
- const providerForRequest =
- attempt.provider.firstByteTimeoutStreamingMs > 0
- ? { ...attempt.provider, firstByteTimeoutStreamingMs: 0 }
- : attempt.provider;
+ const providerForRequest = attempt.provider;
...
if (attempt.thresholdTimer) {
clearTimeout(attempt.thresholdTimer);
attempt.thresholdTimer = null;
}
+ attempt.thresholdTriggered = false;
+ if (attempt.firstByteTimeoutMs > 0) {
+ attempt.thresholdTimer = setTimeout(() => {
+ if (settled || attempt.settled || attempt.thresholdTriggered) return;
+ attempt.thresholdTriggered = true;
+ attempt.session.addProviderToChain(attempt.provider, {
+ ...attempt.endpointAudit,
+ reason: "hedge_triggered",
+ attemptNumber: attempt.sequence,
+ circuitState: getCircuitState(attempt.provider.id),
+ });
+ void launchAlternative();
+ }, attempt.firstByteTimeoutMs);
+ }
attempt.requestAttemptCount += 1;
runAttempt(attempt);
return;Also applies to: 3251-3257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/v1/_lib/proxy/forwarder.ts` around lines 3082 - 3085, The retry path
is clearing first-byte timeout by setting
attempt.provider.firstByteTimeoutStreamingMs to 0 (providerForRequest) and then
clearing thresholdTimer before calling runAttempt(attempt), which can allow
retries to hang indefinitely; preserve or restore the original first-byte
timeout and ensure the threshold timer is (re)initialized before invoking
runAttempt. Specifically, keep the original firstByteTimeoutStreamingMs value on
the attempt (or attach it as e.g. attempt._originalFirstByteTimeout), avoid
replacing it with 0 in providerForRequest (or restore it on attempt.provider
before retry), and restart the thresholdTimer prior to calling
runAttempt(attempt) so the same timeout protection applies on retries.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Problem
When streaming requests used the hedge-based provider race (introduced in #894), reactive rectifiers (thinking signature rectifier and thinking budget rectifier from #576) were not being applied. If a hedge attempt encountered a rectifiable error (e.g., invalid thinking signature or budget_tokens < 1024), the attempt would simply fail instead of applying the rectifier and retrying with the same provider.
Related PRs:
Solution
Extract the reactive rectifier logic into a reusable
tryApplyReactiveAnthropicRectifierfunction that can be called from both:handleAttemptFailurehandlerKey design decisions:
reactiveRectifierRetryStateto prevent infinite retry loopspersistSessionparameter)addSpecialSettingForPersistence,buildRetryFailedChainEntry,getReactiveRectifierDisplayNameChanges
Core Changes (
src/app/v1/_lib/proxy/forwarder.ts)ReactiveRectifierRetryStateandReactiveRectifierResulttypes for tracking rectifier statetryApplyReactiveAnthropicRectifier()function handling both signature and budget rectifiersStreamingHedgeAttemptwithbaseUrl,requestAttemptCount, andreactiveRectifierRetryStatefieldshandleAttemptFailure()in streaming hedge pathrunAttempt()helper for re-executing hedge attempts after rectificationaddSpecialSettingForPersistence()andbuildRetryFailedChainEntry()helpersTest Coverage (
tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts)Testing
bun run buildbun run typecheckbun run testbun run lint(fails on pre-existing repository issues: Biome schema mismatch and existing unrelated lint findings)Description enhanced by Claude AI
Greptile Summary
This PR extends the streaming hedge provider race (from #894) to run reactive Anthropic rectifiers (thinking-signature and thinking-budget rectifiers from #576) when a hedge attempt encounters a rectifiable 4xx error, instead of simply failing. The core approach — extracting
tryApplyReactiveAnthropicRectifierinto a shared helper called from both the standard retry loop andhandleAttemptFailure, giving each hedge attempt its ownreactiveRectifierRetryState, and persisting audit settings to the main session from a shadow session — is well-designed and consistent with the rest of the forwarder.Key changes and observations:
tryApplyReactiveAnthropicRectifiercorrectly guards against infinite loops via per-attemptthinkingSignatureRetried/thinkingBudgetRetriedflags.runAttemptis called for a rectifier retry, preventing spurioushedge_triggeredaudit entries (the scenario raised in a previous thread is addressed in this code).mergeHedgeAttemptIntoSessionnow deduplicatesspecialSettingsbyJSON.stringifykey rather than overwriting, so audit entries pre-persisted to the main session are not lost.handleAttemptFailureinvokes twoawaitcalls (categorizeErrorAsyncandtryApplyReactiveAnthropicRectifier) before reachingrunAttempt. There is no re-check ofsettled || winnerCommitted || attempt.settledat that point, meaning a winner committed during either async gap will still trigger a redundant upstream provider request..catch()handler insiderunAttemptis still missingattempt.settledin its early-return guard (previously flagged thread), allowinghandleAttemptFailureto be entered for an already-settled attempt ifsettled/winnerCommittedhappen to be false at that moment — thoughhandleAttemptFailure's own entry guard will catch it.Confidence Score: 3/5
runAttemptafter two async awaits can cause redundant upstream requests when another hedge attempt wins concurrently, the.catch()guard inrunAttemptstill omitsattempt.settled(flagged in a previous thread), and theattemptNumberfield in the non-rectifier failure chain entry still usesattempt.sequencerather thanattempt.requestAttemptCount(also previously flagged). These three issues together hold the score below 4.src/app/v1/_lib/proxy/forwarder.ts— specificallyhandleAttemptFailure(missing re-guard beforerunAttempt) andrunAttempt's.catch()handler (missingattempt.settledcheck).Important Files Changed
ReactiveRectifierRetryState/ReactiveRectifierResulttypes,tryApplyReactiveAnthropicRectifierhelper,runAttemptrefactor, andhandleAttemptFailureextension. The timer-clearing and dedup logic look correct, buthandleAttemptFailureis missing a re-guard before therunAttemptcall after two async awaits, and the.catch()handler inrunAttemptis still missingattempt.settledin its early-return guard (flagged in previous threads).doForwardcall counts, rectified message verification, andstoreSessionSpecialSettingsaudit assertions are all well-structured and align with the expected timing model.Sequence Diagram
sequenceDiagram participant C as Client participant PF as ProxyForwarder participant P1 as Provider 1 (primary) participant P2 as Provider 2 (hedge) C->>PF: send(session) PF->>P1: runAttempt(attempt1) [doForward] Note over PF,P1: firstByteTimeoutMs timer starts Note over PF: t = firstByteTimeoutMs<br/>threshold timer fires PF->>P2: launchAlternative → runAttempt(attempt2) [doForward] P2-->>PF: ERROR: thinking signature / budget error (4xx) PF->>PF: handleAttemptFailure(attempt2, error) Note over PF: await categorizeErrorAsync() Note over PF: await tryApplyReactiveAnthropicRectifier()<br/>↳ mutates attempt2.session.request.message<br/>↳ persists audit to main session<br/>↳ sets reactiveRectifierRetryState flag PF->>PF: clearThresholdTimer(attempt2) Note over PF: ⚠️ no re-guard for settled/winnerCommitted here PF->>P2: runAttempt(attempt2) [retry, requestAttemptCount=2] par P1 still running P1-->>PF: (slow / first-byte delayed) and P2 rectified retry P2-->>PF: SUCCESS (first chunk) end PF->>PF: commitWinner(attempt2) PF->>PF: syncWinningAttemptSession → mergeSpecialSettings (dedup) PF->>PF: abortAllAttempts (cancels P1) PF-->>C: streaming response from P2 (rectified)Prompt To Fix All With AI
Last reviewed commit: "fix: address hedge r..."