fix(proxy): prioritize TTFB when stream gate is not enforced - #1467
Conversation
Make off, shadow, and high-concurrency routing paths TTFB-first across serial forwarder, legacy hedge, and Discovery attempts. Previously, replay owners, forced Codex response handling, and Discovery validity parsing buffered upstream chunks until the first valid content frame even when stream gating was disabled or running in shadow mode, delaying initial byte delivery to downstream clients. Introduce isStreamGatePrecommitActive so precommit buffering only engages when streamGateMode is enforce and high-concurrency mode is disabled. Replay owners and headerless responses now stream the first upstream byte immediately in non-enforce modes, leaving malformed stream aborts to downstream response observers.
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough本次更新统一流内容预提交门控判定：仅在 Changes流内容门控行为
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The streaming paths consistently apply immediate first-byte forwarding outside enforce mode while retaining enforce-mode gating behavior. No merge-blocking risk is established. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (6 skipped: 5 unsupported, 1 too large.) ✨ Finishing Touches📝 Generate docstrings
🧪 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}
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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
No significant issues identified in this PR. The change centralizes precommit-gate activation into a single predicate (isStreamGatePrecommitActive) and consistently applies it across the serial SSE, legacy hedge, and Discovery paths in the forwarder. Full-context verification confirmed the documented safety net: for off/shadow/high-concurrency modes, the response-handler's protocol observation aborts the replay spool on post-commit bad frames (response-handler.ts observeChunk -> replaySpool.abort), TTFB/TTFT metrics remain correct via the documented recordTtft fallback, and health attribution stays guarded by the authoritative first-byte observation in the response handler.
PR Size: L
- Lines changed: 718 (568 additions / 150 deletions)
- Files changed: 12
Size is dominated by test rework (~538 lines) and i18n/OpenAPI description text (~30 lines); actual production logic is ~40 lines. If splitting were desired, the natural seams would be (1) core predicate + forwarder changes, (2) test suite reorganization, (3) locale/OpenAPI description updates - though given the coupling between behavior and its pinning tests, keeping them together is also defensible.
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 |
Notable candidates investigated and discarded during validation (for transparency):
- Discovery loser-billing eligibility shifting to first-byte readiness: symmetric with the winner's first-byte commit and identical to the legacy hedge first-chunk semantics; opt-in via
billHedgeLosers. Intentional. - 100ms timing windows in new TTFB tests: resolution is microtask-based after the first chunk; pattern matches existing file conventions. Below reporting threshold.
- Discovery off-mode coverage: exercised by existing integration tests that run under the default
offmode (drain/billing lifecycle tests); enforce-mode Discovery pinned explicitly by this PR.
Review Coverage
- Logic and correctness - Clean (all three forwarder paths verified in full context; null-safety of the
validityrewrite confirmed equivalent to prior behavior when the gate is active) - Security (OWASP Top 10) - Clean (no new input handling, secrets, or injection surface; transparency policy change is admin-configured and documented)
- Error handling - Clean (no new catch blocks; intentional non-throwing under inactive gate is compensated by post-hoc protocol observation and replay-entry suppression)
- Type safety - Clean (no
anyintroduced;DiscoveryValidity | nullhandled via correct short-circuit optional chaining) - Documentation accuracy - Clean (settings text updated consistently in all 5 locales, OpenAPI schema and regenerated types match; code comments match new behavior)
- Test coverage - Adequate (predicate truth table, enforce interception, off/shadow passthrough, high-concurrency TTFB, replay-owner TTFB across serial and hedge paths)
- Code clarity - Good (predicate centralization reduces previously scattered conditions)
CLAUDE.md compliance: no emoji in changed strings, i18n used for all user-facing text, PR targets dev.
Automated review by Claude AI
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Users could still see delayed first-byte delivery after disabling the streaming output gate, selecting shadow mode, or enabling high-concurrency mode. The delay came from precommit content gating in multiple forwarder paths, including forced headerless Codex Responses handling and Discovery validity parsing.
This change makes precommit buffering active only for
STREAM_GATE_MODE=enforcewhen high-concurrency mode is disabled. Off, shadow, replay-owner, high-concurrency, and headerless Codex streams now hand the first upstream byte to the client immediately. Discovery remains enabled, but uses first-byte readiness when precommit gating is inactive. Enforce mode keeps the existing error/malformed/empty-stream failover behavior.Problem
forceCodexResponsesStreamand replay-owner state independently enabledrunStreamContentGate, sooffandshadowwere not actually TTFB-first. Discovery ran before the legacy serial/hedge paths and always waited forDiscoveryValidityParserto observe valid content, bypassing the configured gate policy.Related Issues / PRs:
stream_gate_mode=offfor replay owners. fix(proxy): recognize terminal Responses compaction streams (#1410) #1411 fixed the falseempty_streamrejections but explicitly kept the replay-owner precommit gate; this PR removes it outside enforce mode.Solution
Precommit gate activation is centralized in a single predicate,
isStreamGatePrecommitActive(highConcurrencyMode): active only whenSTREAM_GATE_MODE=enforceand high-concurrency mode is disabled. The serial SSE, legacy hedge, and Discovery paths in the forwarder all consult this predicate instead of independently enabling the gate via replay-owner or headerless-Codex conditions.For off/shadow modes, bad streams are no longer traded for zero-byte failover: the response-handler's
StreamProtocolObserveraborts the stream after the fact and the replay entry is not published.Changes
Core Changes
src/app/v1/_lib/proxy/stream-gate/stream-content-gate.ts: newisStreamGatePrecommitActive()helpersrc/app/v1/_lib/proxy/forwarder.ts:DiscoveryValidityParser.push()only runs when precommit is active; otherwise the first upstream byte marks the attempt ready (validity checks become null-safe)Supporting Changes
streamGateModeDescsettings text updated in all 5 locales (en/ja/ru/zh-CN/zh-TW)system-config.ts) plus regeneratedopenapi-types.gen.ts; enum values unchangedstream-gate-mode-resolution.test.tsadds the predicate truth table (+18);stream-gate-forwarder-integration.test.ts(+505/-119) moves enforce-mode interception coverage up and adds off/shadow first-byte passthrough, high-concurrency TTFB, and replay-owner TTFB cases;proxy-hedge-lifecycle.test.ts(+6/-2) pins the Discovery test to enforce mode and dispatches downstream before content completesBehavioral Changes (no API or migration impact)
off/shadow+ replay owneroff/shadow+ headerless Codex Responsesenforce+ high-concurrency modeoff: TTFB-firstenforce(default), no high-concurrencyValidation
bun run typecheck: passed.bun run lintandbun run lint:fix: passed; existing Biome schema-version info only.bun run build: passed; existing Edge-runtime warnings only.bun run openapi:check: passed.bun run openapi:lint: passed.bun run i18n:audit-messages-no-emoji:fail: passed.git diff --check: passed.Notes
The existing settings placeholder audit reports unrelated same-as-zh-CN translation entries in other locale files; the changed messages pass the no-emoji audit.
Description enhanced by Claude AI