fix(discovery): preserve final rescue and bill ready losers#1353
Merged
ding113 merged 4 commits intoJul 23, 2026
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughChanges有界 Discovery 与协议判定
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/discovery-validity.ts (1)
203-204: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value可复用模块级
TextEncoder以减少流式热路径上的分配。每次
push处理字符串分片时都会new TextEncoder()，而在发现阶段竞速的流式路径上push可能被高频调用。解码器已是实例级字段，编码器同样是无状态的，可提升为模块级常量复用。♻️ 建议改动
+const sharedTextEncoder = new TextEncoder(); + export class DiscoveryValidityParser {this.bytesSeen += - typeof chunk === "string" ? new TextEncoder().encode(chunk).byteLength : chunk.byteLength; + typeof chunk === "string" ? sharedTextEncoder.encode(chunk).byteLength : chunk.byteLength;🤖 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/discovery-validity.ts` around lines 203 - 204, 在 discovery validity 模块中将 TextEncoder 提升为可复用的模块级常量，并更新处理字符串分片的 push 逻辑使用该实例；保留非字符串 chunk 的 byteLength 计算不变，避免每次调用 push 时重复创建编码器。
🤖 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 `@docs/streaming-discovery.md`:
- Around line 37-39: 更新 docs/streaming-discovery.md 中关于 Discovery losers
的描述：说明已产出有效响应前缀且已就绪的失败者会复用 legacy bill_hedge_losers 进行后台读取、drain
和计费；仅未满足该条件的失败者才会被取消并释放 readers、agents 和 provider-session 引用。保持文档与
messages/en/settings/config.json 的 billHedgeLosersDesc 语义一致。
In `@drizzle/meta/_journal.json`:
- Around line 766-774: 补齐 drizzle journal 中缺失的 idx 109 条目，使迁移序号从 108 连续到
110；使用对应的 0109_* 迁移标识及其正确的 version、when 和 breakpoints 元数据，并保持现有条目顺序不变。
In `@src/app/api/admin/system-config/route.ts`:
- Around line 64-73: 统一 UpdateSystemSettingsSchema.parse 的 ZodError
处理与此处手动校验的失败响应：当 superRefine 触发 discovery window 校验失败时，也返回稳定的
DISCOVERY_WINDOW_INVALID_ERROR_CODE，并保持与手动校验一致的响应结构。更新相关 ZodError
分支，避免完整更新和部分更新产生不同错误格式。
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/discovery-validity.ts`:
- Around line 203-204: 在 discovery validity 模块中将 TextEncoder
提升为可复用的模块级常量，并更新处理字符串分片的 push 逻辑使用该实例；保留非字符串 chunk 的 byteLength 计算不变，避免每次调用 push
时重复创建编码器。
🪄 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: d1978617-9f3b-4b09-b31b-ca17fa4a63d3
📒 Files selected for processing (120)
.env.exampledocs/streaming-discovery.mddrizzle/0110_daffy_rawhide_kid.sqldrizzle/0111_happy_mauler.sqldrizzle/meta/0110_snapshot.jsondrizzle/meta/0111_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/en/settings/config.jsonmessages/ja/dashboard.jsonmessages/ja/settings/config.jsonmessages/ru/dashboard.jsonmessages/ru/settings/config.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/settings/config.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/settings/config.jsonpackage.jsonsrc/actions/system-config.tssrc/actions/usage-logs.tssrc/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/DiscoveryTraceView.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/index.tssrc/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/types.tssrc/app/[locale]/dashboard/logs/_components/provider-chain-popover.test.tsxsrc/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/config/_components/system-settings-form.tsxsrc/app/[locale]/settings/config/page.tsxsrc/app/api/admin/system-config/route.tssrc/app/api/v1/resources/system/handlers.tssrc/app/api/v1/resources/system/router.tssrc/app/v1/_lib/proxy/discovery-coordinator.tssrc/app/v1/_lib/proxy/discovery-validity.tssrc/app/v1/_lib/proxy/error-handler.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session.tssrc/app/v1/_lib/proxy/stream-finalization.tssrc/drizzle/schema.tssrc/instrumentation.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/_shared/error-envelope.tssrc/lib/api/v1/_shared/request-body.tssrc/lib/api/v1/schemas/system-config.tssrc/lib/config/env.schema.tssrc/lib/config/system-settings-cache.tssrc/lib/ledger-backfill/trigger.sqlsrc/lib/lifecycle/shutdown.tssrc/lib/observability/discovery-metrics.tssrc/lib/redis/client.tssrc/lib/redis/live-chain-store.storage.test.tssrc/lib/redis/live-chain-store.test.tssrc/lib/redis/live-chain-store.tssrc/lib/redis/lua-scripts.tssrc/lib/redis/session-binding.tssrc/lib/session-manager.tssrc/lib/session-tracker.tssrc/lib/utils/provider-chain-display.test.tssrc/lib/utils/provider-chain-display.tssrc/lib/validation/discovery-settings.tssrc/lib/validation/schemas.tssrc/repository/_shared/transformers.tssrc/repository/message-write-buffer.tssrc/repository/message.tssrc/repository/routing-trace-outbox.tssrc/repository/routing-trace-persistence.tssrc/repository/system-config.tssrc/repository/usage-logs.tssrc/types/message.tssrc/types/routing-trace.tssrc/types/system-config.tstests/api/v1/system/system-config.test.tstests/configs/integration.config.tstests/configs/session-binding.config.tstests/integration/proxy-hedge-lifecycle.test.tstests/integration/session-binding-versioning-redis.test.tstests/unit/actions/system-config-save.test.tstests/unit/api/admin-system-config-route.test.tstests/unit/lib/redis/client.test.tstests/unit/lib/redis/live-chain-store.test.tstests/unit/lib/redis/session-binding.test.tstests/unit/lib/session-manager-binding-smart.test.tstests/unit/lib/session-manager-content-hash.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/lib/shutdown.test.tstests/unit/proxy/discovery-coordinator.test.tstests/unit/proxy/discovery-validity.test.tstests/unit/proxy/error-handler-durable-persistence.test.tstests/unit/proxy/hedge-winner-dedup.test.tstests/unit/proxy/provider-selector-cross-type-model.test.tstests/unit/proxy/provider-selector-group-priority.test.tstests/unit/proxy/provider-selector-model-mismatch-binding.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/proxy-forwarder-provider-session-release.test.tstests/unit/proxy/response-handler-abort-listener-cleanup.test.tstests/unit/proxy/response-handler-client-abort-drain.test.tstests/unit/proxy/response-handler-endpoint-circuit-isolation.test.tstests/unit/proxy/response-handler-exported-finalizers.test.tstests/unit/proxy/response-handler-gemini-stream-passthrough-timeouts.test.tstests/unit/proxy/routing-trace.test.tstests/unit/proxy/terminal-outcome-contract.test.tstests/unit/repository/message-terminal-write-apis.test.tstests/unit/repository/message-write-buffer.test.tstests/unit/repository/routing-trace-outbox.test.tstests/unit/repository/system-config-degradation-ladder.test.tstests/unit/repository/system-config-update-missing-columns.test.tstests/unit/types/routing-trace.test.tstests/unit/usage-ledger/trigger.test.tstests/unit/validation/system-settings-discovery.test.ts
Brisbanehuang
marked this pull request as ready for review
July 22, 2026 14:43
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: de15e9c320
ℹ️ 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".
Comment on lines
+769
to
+772
| "idx": 110, | ||
| "version": "7", | ||
| "when": 1784571513591, | ||
| "tag": "0110_daffy_rawhide_kid", |
There was a problem hiding this comment.
When this journal is applied on a database that has only migrations through 0108 (I checked this tree and drizzle/0109*.sql is absent), Drizzle Kit v0.x will record 0110's when as the newest applied migration; Drizzle's v0→v1 notes state that v0 only looked for local migrations newer than the last applied one, so the dependency's later-added 0109 migration would be silently skipped. Please include/rebase 0109 before these entries or regenerate 0110/0111 with later timestamps before merging.
Useful? React with 👍 / 👎.
Contributor
Author
ding113
changed the base branch from
dev
to
integration/discovery-stack-20260723
July 22, 2026 23:28
…0723' into agent/discovery-1353-on-integration # Conflicts: # messages/en/dashboard.json # messages/ja/dashboard.json # messages/ru/dashboard.json # messages/zh-CN/dashboard.json # messages/zh-TW/dashboard.json # src/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsx # src/app/v1/_lib/proxy/forwarder.ts # tests/unit/proxy/response-handler-endpoint-circuit-isolation.test.ts
…0723' into agent/discovery-1353-on-integration
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx (1)
1903-1995: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win修正不同 attempt 的 winner 成本断言
该用例声称旧 loser 不应关联到后续 winner，但
101:1的$0.006仍被用于将总成本$0.015推导为 winner 的$0.009。这与“按 provider ID 和 attempt number 匹配成本”的 PR 约束冲突。请改为使用与101:4匹配的 loser 数据，或断言不受101:1成本影响的 winner 结果，并明确costUsd的总成本语义。🤖 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/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx around lines 1903 - 1995, The test fixture in “does not attach an older loser cost to a later winner from the same provider” uses an unmatched loser cost while asserting a derived winner cost, conflicting with attempt-specific matching. Update hedgeLosers to include cost data matching provider 101 and attempt 4, or change the assertions to verify the winner result is unaffected by the 101:1 cost; ensure the test explicitly reflects that costUsd represents the total cost.
🤖 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.
Outside diff comments:
In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:
- Around line 1903-1995: The test fixture in “does not attach an older loser
cost to a later winner from the same provider” uses an unmatched loser cost
while asserting a derived winner cost, conflicting with attempt-specific
matching. Update hedgeLosers to include cost data matching provider 101 and
attempt 4, or change the assertions to verify the winner result is unaffected by
the 101:1 cost; ensure the test explicitly reflects that costUsd represents the
total cost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dc95a1b-4d56-46da-a912-647e77511a3e
📒 Files selected for processing (11)
drizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/v1/_lib/proxy/forwarder.tstests/integration/proxy-hedge-lifecycle.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- messages/en/dashboard.json
- src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx
- tests/unit/proxy/proxy-forwarder-hedge-first-byte.test.ts
- src/app/v1/_lib/proxy/forwarder.ts
ding113
merged commit Jul 23, 2026
671870b
into
ding113:integration/discovery-stack-20260723
3 checks passed
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Safety boundaries
Verification
The repository-wide placeholder audit still reports its existing Japanese and Traditional Chinese same-as-zh-CN baseline; this PR adds no entry to that failure list.
Dependency
This is a stacked PR and depends on #1351. Until #1351 merges, the dev comparison also contains its parent observability commits.
Refs #1340
Migration ordering
Migration
0109_nice_shriekis owned by #1336. That PR must merge before #1349, #1351, and this PR. After #1336 lands, rebase the Discovery stack in order and regenerate the0110/0111Drizzle metadata against0109before marking this PR ready. Deploying0110or0111first would cause the older0109migration to be skipped by Drizzle's timestamp ordering.Greptile Summary
This PR expands bounded Discovery recovery and accounting. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/int..." | Re-trigger Greptile