Uh oh!
There was an error while loading. Please reload this page.
fix: port ensemble streaming to the post-#607 MultiReservation API (unbreaks main) - #608
Conversation
#606 (ensemble streaming) was written against the pre-#607 reservation API and merged onto a main that already had #607's cluster-Redis change, which dropped MultiReservation's generics/lifetime, made commit_tokens async, and made into_stream_hold arg-less. The semantic merge conflict broke main's build (no textual conflict, so it merged CLEAN). Port the streaming-ensemble reservation code to the new API: - MultiReservation<'a, SystemClock> -> MultiReservation. - into_stream_hold(arg) -> into_stream_hold(). - The error-exit billing helper was a sync closure calling the now-async commit_tokens with no .await, so the commit was a dropped future and panel tokens were never billed on streaming error exits. Split into a sync survivor_total + emit_panel_then_fail helper and .await commit_tokens inline at each of the 6 exits (mirroring the non-streaming path). One commit per request, every survivor billed once -- now actually executed. Verified: cargo check + clippy -D warnings clean, 263 core + 440 proxy tests green (incl. both streaming e2e), schema-drift gate clean. Refs #601.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesStreaming Ensemble Async Token Commit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
Uh oh!
There was an error while loading. Please reload this page.
🔴 Unbreaks
main— build is currently redmaindoes not compile right now. #606 (ensemble streaming) was written against the pre-#607 reservation API and merged onto amainthat already contained #607 ("cluster-level rate limiting via shared Redis"), which changedaisix_ratelimit::MultiReservation(dropped its generics/lifetime, madecommit_tokensasync, madeinto_stream_holdarg-less). There was no textual conflict (the two PRs touch different lines), so #606 mergedCLEAN— but it's a semantic merge conflict: the merged tree doesn't build, andmain's post-merge CI for #606 is failing. (My miss — I merged #606 without rebasing onto #607; filing this immediately.)The fix
Port the streaming-ensemble reservation code in
dispatch_ensembleto the new API (only the streaming arm; the single-upstream path,build_sse_stream,aisix-ratelimit, andaisix-coreare untouched):MultiReservation<'a, SystemClock>→MultiReservation.into_stream_hold(Arc::clone(&state.limiter))→into_stream_hold().bill_panel_and_failwas a sync closure calling the now-asynccommit_tokenswith no.await— so post-feat(ratelimit): cluster-level rate limiting via shared Redis #607 the commit was a dropped future and the surviving panel tokens were never billed on streaming error exits. Split into a syncsurvivor_total+emit_panel_then_failand.await reservation.commit_tokens(...)inline at each of the 6 error exits (mirroring the non-streaming ensemble path). One commit per request, every survivor billed exactly once — now actually executed.Verification (local, against this branch = current main + the fix)
cargo check -p aisix-proxy— clean (main compiles again).cargo clippy -p aisix-core -p aisix-proxy --all-targets -- -D warnings— zero warnings.cargo test -p aisix-core— 263 pass;cargo test -p aisix-proxy— 440 pass (incl.ensemble_streams_judge_synthesis,ensemble_streaming_judge_connect_failure_still_bills_panel, and the rest of the 20 ensemble tests).dump-schema+git diff --exit-code schemas/) — clean.36/36-line change, isolated to
chat.rs's streaming-ensemble arm. Please merge once CI is green to restoremain.Refs #601.
Summary by CodeRabbit