feat(flowcontrol): expose round-robin fairness identity via DumpState#5
feat(flowcontrol): expose round-robin fairness identity via DumpState#5chethanuk wants to merge 1 commit into
Conversation
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughAdds ChangesroundRobin StateDumper
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| When `addEstimatedOutputTokens` is true, the estimated output added per request is | ||
| `min(round(inputTokens * outputRatio), clientMaxOutputTokens?, maxEstimatedOutputTokens?)`. | ||
| The client cap is the request's own output limit (`max_tokens` / | ||
| `max_completion_tokens` / `max_output_tokens` depending on the API), applied only | ||
| when the client specified one. This keeps estimates realistic for high-input / |
There was a problem hiding this comment.
Suggestion: Consolidate this estimation formula detail into the parameter table (or keep it only here) so the same factual rule is documented in only one canonical place. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The README already states the same factual rule in the parameter table: outputRatio describes the estimated output formula, and maxEstimatedOutputTokens describes the upper bound. Repeating that logic again in prose duplicates factual content across table and prose, which matches the custom rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/README.md
**Line:** 34:38
**Comment:**
*Custom Rule: Consolidate this estimation formula detail into the parameter table (or keep it only here) so the same factual rule is documented in only one canonical place.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // extractRequestBody extracts the InferenceRequestBody from the given raw body | ||
| // for the already-resolved API type. |
There was a problem hiding this comment.
Suggestion: Replace this comment with a brief rationale-focused note (or remove it) since it currently just repeats what the function signature already makes obvious. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment mostly repeats what the function name and parameters already convey, and it does not explain any non-obvious intent or rationale. That matches the custom rule against comments that merely restate the code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requesthandling/parsers/openai/openai.go
**Line:** 205:206
**Comment:**
*Custom Rule: Replace this comment with a brief rationale-focused note (or remove it) since it currently just repeats what the function signature already makes obvious.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| Generate: &generate, | ||
| Payload: fwkrh.PayloadMap(bodyMap), | ||
| } | ||
| // max_tokens lives under sampling_params in the generate wire format. |
There was a problem hiding this comment.
Suggestion: Rewrite or remove this inline comment so it captures the non-obvious rationale for the special-case handling rather than restating the field access performed by the next line. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment is a direct statement of the field path used by the next line, so it restates visible code behavior rather than adding non-obvious intent or rationale. That matches the custom rule against comments that merely paraphrase the code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requesthandling/parsers/vllmhttp/vllmhttp.go
**Line:** 125:125
**Comment:**
*Custom Rule: Rewrite or remove this inline comment so it captures the non-obvious rationale for the special-case handling rather than restating the field access performed by the next line.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // MaxOutputTokens is the client-requested cap on generated output tokens, | ||
| // normalized across APIs (OpenAI max_tokens / max_completion_tokens, Anthropic | ||
| // max_tokens, Responses max_output_tokens, vLLM SamplingParams.max_tokens). | ||
| // It is nil when the client did not specify a cap. Consumers such as output | ||
| // token estimators use it as an upper bound. Derived, not round-tripped. |
There was a problem hiding this comment.
Suggestion: De-duplicate repeated factual details by keeping key-resolution semantics in one canonical doc location and shortening this field comment to only its purpose. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The comment repeats factual details that are also documented in the new function comment below, especially the cross-API key semantics and interpretation of the cap. This matches the duplicated-documentation rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/interface/requesthandling/types.go
**Line:** 106:110
**Comment:**
*Custom Rule: De-duplicate repeated factual details by keeping key-resolution semantics in one canonical doc location and shortening this field comment to only its purpose.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| default: | ||
| continue | ||
| } | ||
| // Skip negative or non-integral values as malformed and try the next key. |
There was a problem hiding this comment.
Suggestion: Remove or rewrite this inline comment so it explains non-obvious rationale only, rather than restating the condition that is immediately visible in code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment simply restates the immediately following condition and loop behavior. The code already makes that control flow obvious, so the comment is redundant under the repository’s comment-style rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/interface/requesthandling/types.go
**Line:** 141:141
**Comment:**
*Custom Rule: Remove or rewrite this inline comment so it explains non-obvious rationale only, rather than restating the condition that is immediately visible in code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| func TestSimpleTokenEstimator_Estimate_HonorsClientCap(t *testing.T) { | ||
| estimator := NewSimpleTokenEstimator() // ratio 1.5, no operator cap | ||
|
|
||
| // 10 input tokens, client caps output at 3 -> 10 + min(15, 3) = 13. |
There was a problem hiding this comment.
Suggestion: Remove this inline arithmetic narration or replace it with a brief rationale that explains why this scenario is important to validate, since the current text just mirrors the assertion inputs and expected value. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment merely repeats the arithmetic already visible in the test setup and assertion, rather than explaining why the capped-output case matters. That matches the rule against comments that restate code instead of conveying non-obvious intent.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator_test.go
**Line:** 178:178
**Comment:**
*Custom Rule: Remove this inline arithmetic narration or replace it with a brief rationale that explains why this scenario is important to validate, since the current text just mirrors the assertion inputs and expected value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| req.Body.MaxOutputTokens = ptr.To(int64(3)) | ||
| require.Equal(t, int64(13), estimator.Estimate(req)) | ||
|
|
||
| // Without a client cap, the full ratio applies -> 10 + 15 = 25. |
There was a problem hiding this comment.
Suggestion: Remove this descriptive calculation comment or rewrite it to capture non-obvious intent, because it currently repeats what the test call and expected assertion already show. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment is a direct restatement of the expected result and calculation already shown by the test call. It does not add rationale or non-obvious intent, so it fits the custom rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator_test.go
**Line:** 183:183
**Comment:**
*Custom Rule: Remove this descriptive calculation comment or rewrite it to capture non-obvious intent, because it currently repeats what the test call and expected assertion already show.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // roundRobinState is the debug snapshot for the round-robin fairness policy. | ||
| // The policy is a stateless singleton: its per-priority-band cursors live in the | ||
| // flow-control registry (PriorityBandAccessor.PolicyState), not on the plugin, so | ||
| // no live scheduling state is reachable from here. This snapshot reports only | ||
| // static identity plus an explicit marker so operators are not misled into | ||
| // thinking the cursor position is shown. |
There was a problem hiding this comment.
Suggestion: Remove or significantly shorten this struct doc block and keep the detailed state-location explanation in only one canonical place to avoid repeating the same fact multiple times. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The file repeats the same factual point about cursor state living in the flow-control registry rather than on the plugin in multiple places, including this struct doc, the DumpState comment, and the JSON note field. That matches the rule against duplicated factual content across docs/comments/examples, so the suggestion is verified.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go
**Line:** 132:137
**Comment:**
*Custom Rule: Remove or significantly shorten this struct doc block and keep the detailed state-location explanation in only one canonical place to avoid repeating the same fact multiple times.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // mutable runtime state on the plugin; the per-band cursors are owned by the flow | ||
| // registry. The payload is therefore a fixed, bounded struct independent of band | ||
| // or flow cardinality. | ||
| func (p *roundRobin) DumpState() (json.RawMessage, error) { |
There was a problem hiding this comment.
Suggestion: Condense this method comment to only non-obvious intent and avoid repeating details already stated in nearby docs and payload text. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment largely repeats the same explanation already given in the surrounding struct doc and the Note field, rather than adding distinct non-obvious rationale. That fits the rule against comments that merely restate code or duplicate nearby documentation, so it is verified.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go
**Line:** 145:148
**Comment:**
*Custom Rule: Condense this method comment to only non-obvious intent and avoid repeating details already stated in nearby docs and payload text.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert.Equal(t, RoundRobinFairnessPolicyType, s.Policy) | ||
| assert.False(t, s.Stateful, "policy holds no reachable mutable state") | ||
| assert.NotEmpty(t, s.Note, "note must explain where cursors actually live") | ||
| // Exactly the three sanitized fields, nothing else (no name, no flow IDs). |
There was a problem hiding this comment.
Suggestion: Consolidate this repeated fact into one canonical place (either the key set/assertion message or the comment) to avoid duplicating the same payload-field contract in multiple forms. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The comment repeats the same payload-shape contract that is already encoded immediately below in wantKeys and the assert.Len/key assertions. That is duplicated factual content in nearby prose and code, so it fits the duplication rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin_test.go
**Line:** 307:307
**Comment:**
*Custom Rule: Consolidate this repeated fact into one canonical place (either the key set/assertion message or the comment) to avoid duplicating the same payload-field contract in multiple forms.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if tt.name == "default name" { | ||
| defaultBytes = payload | ||
| } else { | ||
| // Name is not emitted: a differently-named policy yields identical bytes. |
There was a problem hiding this comment.
Suggestion: Remove this comment or replace it with non-obvious rationale, since it currently just narrates what the immediately following assertion already shows. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The comment simply paraphrases the immediately following assertion that compares the bytes for two differently named policies. It does not add non-obvious rationale, so it matches the rule against comments that restate code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin_test.go
**Line:** 333:333
**Comment:**
*Custom Rule: Remove this comment or replace it with non-obvious rationale, since it currently just narrates what the immediately following assertion already shows.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // EstimateOutput returns the estimated output token count given the input token | ||
| // count, bounded by the client-requested cap (maxOutputTokens, nil if unset) | ||
| // and the estimator's configured operator cap. |
There was a problem hiding this comment.
Suggestion: Avoid repeating the same cap semantics in multiple doc locations; keep this fact in one canonical place (for example on the concrete method) and shorten this interface comment accordingly. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This comment repeats cap semantics that are also documented elsewhere in the same file, including the concrete method comment and the struct field comment. That matches the rule against duplicated factual content across documentation locations.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator.go
**Line:** 31:33
**Comment:**
*Custom Rule: Avoid repeating the same cap semantics in multiple doc locations; keep this fact in one canonical place (for example on the concrete method) and shorten this interface comment accordingly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // NewSimpleTokenEstimator returns a SimpleTokenEstimator with the default output | ||
| // ratio and no operator cap. |
There was a problem hiding this comment.
Suggestion: Trim this constructor comment to avoid repeating details already conveyed by the function body and related config docs. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This constructor comment just restates the default configuration used by the function body and the delegated helper, without adding non-obvious rationale. That is the kind of repetitive documentation the rule flags.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator.go
**Line:** 51:52
**Comment:**
*Custom Rule: Trim this constructor comment to avoid repeating details already conveyed by the function body and related config docs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // NewSimpleTokenEstimatorWithRatio returns a SimpleTokenEstimator that estimates | ||
| // output tokens as round(inputTokens * ratio). | ||
| // output tokens as round(inputTokens * ratio), with no operator cap. |
There was a problem hiding this comment.
Suggestion: Rewrite this comment to explain intent rather than mirroring the exact formula already evident in the implementation. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The comment mostly restates the implementation formula and constructor behavior already evident from the function body and name. It does not explain additional intent or rationale, so it fits the rule against comments that merely mirror code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator.go
**Line:** 57:58
**Comment:**
*Custom Rule: Rewrite this comment to explain intent rather than mirroring the exact formula already evident in the implementation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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 introduces client-requested output token limit normalization (MaxOutputTokens) across OpenAI, Anthropic, and vLLM APIs, integrating it alongside a new operator-defined cap (MaxEstimatedOutputTokens) into the in-flight load token estimator to refine concurrency tracking. It also implements state dumping for the round-robin fairness policy. The review feedback identifies a missing encoding/json import in roundrobin.go that will cause a compilation error, and points out a potential integer overflow vulnerability when parsing extremely large float values in MaxOutputTokensFromPayload, recommending an upper-bound check and an associated test case.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // mutable runtime state on the plugin; the per-band cursors are owned by the flow | ||
| // registry. The payload is therefore a fixed, bounded struct independent of band | ||
| // or flow cardinality. | ||
| func (p *roundRobin) DumpState() (json.RawMessage, error) { |
There was a problem hiding this comment.
The DumpState method uses json.RawMessage and json.Marshal, but encoding/json is not imported in this file. This will cause a compilation error. Please ensure "encoding/json" is added to the import block of roundrobin.go.
| // Skip negative or non-integral values as malformed and try the next key. | ||
| if f < 0 || f != math.Trunc(f) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
If f is extremely large (e.g., 1e20), converting it to int64 can overflow and result in math.MinInt64 (which is negative). Since f < 0 is checked before conversion, an overflowed negative value would bypass this check. To prevent this, we should also check if f exceeds float64(math.MaxInt64).
| // Skip negative or non-integral values as malformed and try the next key. | |
| if f < 0 || f != math.Trunc(f) { | |
| continue | |
| } | |
| // Skip negative, overflow, or non-integral values as malformed and try the next key. | |
| if f < 0 || f > float64(math.MaxInt64) || f != math.Trunc(f) { | |
| continue | |
| } |
| {name: "negative ignored", m: PayloadMap{"max_tokens": float64(-1)}, keys: []string{"max_tokens"}, want: nil}, | ||
| {name: "non-integral ignored", m: PayloadMap{"max_tokens": float64(1.5)}, keys: []string{"max_tokens"}, want: nil}, | ||
| {name: "wrong type ignored", m: PayloadMap{"max_tokens": "64"}, keys: []string{"max_tokens"}, want: nil}, |
There was a problem hiding this comment.
Add a test case to verify that extremely large float values that would overflow int64 are correctly ignored.
{name: "negative ignored", m: PayloadMap{"max_tokens": float64(-1)}, keys: []string{"max_tokens"}, want: nil},
{name: "overflow ignored", m: PayloadMap{"max_tokens": float64(1e20)}, keys: []string{"max_tokens"}, want: nil},
{name: "non-integral ignored", m: PayloadMap{"max_tokens": float64(1.5)}, keys: []string{"max_tokens"}, want: nil},
{name: "wrong type ignored", m: PayloadMap{"max_tokens": "64"}, keys: []string{"max_tokens"}, want: nil},
| if f < 0 || f != math.Trunc(f) { | ||
| continue | ||
| } | ||
| out := int64(f) | ||
| return &out |
There was a problem hiding this comment.
Suggestion: Converting f to int64 without an upper-bound check can overflow for very large numeric inputs (for example values above math.MaxInt64), producing an implementation-dependent wrapped value. Validate that the parsed number is <= math.MaxInt64 before casting, otherwise treat it as invalid/absent. [type error]
Severity Level: Major ⚠️
- ❌ OpenAI HTTP parser miscomputes caps for huge inputs.
- ⚠️ Token estimator uses overflowed MaxOutputTokens bounds.
- ⚠️ vLLM HTTP generate parser shares unsafe helper.Steps of Reproduction ✅
1. A client sends an OpenAI-compatible HTTP request (e.g. `POST /v1/chat/completions`)
parsed by `OpenAIParser.ParseRequest` in
`pkg/epp/framework/plugins/requesthandling/parsers/openai/openai.go:1-16`, with a JSON
body that includes a very large numeric cap such as `"max_tokens": 1e20` or
`"max_output_tokens": 1e20`.
2. `OpenAIParser.ParseRequest` unmarshals the body into `bodyMap` using `json.Unmarshal`
(openai.go:2-4), producing `map[string]any` where the cap field is a `float64`, then calls
`maxOutputTokensForAPI` (openai.go:19-32), which in turn calls
`fwkrh.MaxOutputTokensFromPayload(bodyMap, "max_tokens")` or the appropriate field name.
3. Inside `MaxOutputTokensFromPayload` in
`pkg/epp/framework/interface/requesthandling/types.go:43-70`, the large `float64` value is
assigned to `f`, passes the non-negative and integral check at lines 62-64, and then
executes `out := int64(f)` and `return &out` (lines 66-67). Because `f` exceeds
`math.MaxInt64`, the float-to-int64 conversion is out-of-range and the resulting `int64`
value is implementation-dependent, not the original cap.
4. The resulting (potentially wrapped or saturated) pointer is stored in
`InferenceRequestBody.MaxOutputTokens` and later read by the token estimator in
`pkg/epp/framework/plugins/requestcontrol/dataproducer/inflightload/token_estimator.go:80-85`,
where `Estimate` retrieves `request.Body.MaxOutputTokens` and `EstimateOutput` (lines
100-107) uses it to bound estimated output tokens, so the overflowed value can cause
incorrect or unpredictable token limits for that request.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/epp/framework/interface/requesthandling/types.go
**Line:** 142:146
**Comment:**
*Type Error: Converting `f` to `int64` without an upper-bound check can overflow for very large numeric inputs (for example values above `math.MaxInt64`), producing an implementation-dependent wrapped value. Validate that the parsed number is `<= math.MaxInt64` before casting, otherwise treat it as invalid/absent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
Implement plugin.StateDumper for the round-robin fairness policy so it participates in the /debug/plugins/state endpoint alongside the other plugins. The policy is a stateless singleton: its per-priority-band cursors live in the flow-control registry, not on the plugin, so no live scheduling state is reachable here. DumpState therefore returns a fixed, bounded struct (policy name plus an explicit stateful=false marker and a note) rather than implying a cursor position it cannot see. The payload size is independent of band or flow cardinality. Signed-off-by: ChethanUK <chethanuk@outlook.com>
42fe09e to
0c09105
Compare
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
|
CodeAnt AI Incremental review completed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go (1)
131-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame fact is stated in three places.
The "per-band cursors live in the flow-control registry, not on the plugin" fact is repeated across the struct doc (Lines 133-134), the
DumpStatemethod doc (Lines 145-146), and theNotepayload string (Line 152). TheNoteis operator-facing output and must stay; consider trimming the method doc so the canonical explanation lives only on the struct.As per coding guidelines: "State each fact once in its canonical location. Do not duplicate across struct docs, prose, tables, inline comments, and examples".
♻️ Proposed trim of the redundant method doc
-// DumpState implements [fwkplugin.StateDumper]. The round-robin policy holds no -// mutable runtime state on the plugin; the per-band cursors are owned by the flow -// registry. The payload is therefore a fixed, bounded struct independent of band -// or flow cardinality. +// DumpState implements [fwkplugin.StateDumper]. The payload is a fixed, bounded +// struct independent of band or flow cardinality. func (p *roundRobin) DumpState() (json.RawMessage, error) {🤖 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 `@pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go` around lines 131 - 153, The round-robin cursor-location explanation is duplicated in the `roundRobinState` struct doc, the `DumpState` method doc, and the `Note` field payload; keep the operator-facing `Note` in `DumpState`, but trim the `roundRobin.DumpState` comment so the canonical explanation lives only in the `roundRobinState` doc. Update the comments around `roundRobinState` and `DumpState` to avoid repeating the same fact while preserving the intent that the plugin is stateless and the registry owns per-band cursors.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.go`:
- Around line 131-153: The round-robin cursor-location explanation is duplicated
in the `roundRobinState` struct doc, the `DumpState` method doc, and the `Note`
field payload; keep the operator-facing `Note` in `DumpState`, but trim the
`roundRobin.DumpState` comment so the canonical explanation lives only in the
`roundRobinState` doc. Update the comments around `roundRobinState` and
`DumpState` to avoid repeating the same fact while preserving the intent that
the plugin is stateless and the registry owns per-band cursors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 315217b8-b8f4-4220-adc8-cd4317d9c074
📒 Files selected for processing (2)
pkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin.gopkg/epp/framework/plugins/flowcontrol/fairness/roundrobin/roundrobin_test.go
1 participant
User description
Problem
The
/debug/plugins/stateendpoint discovers any plugin that implementsplugin.StateDumper. The round-robin fairness policy did not implement it, so theendpoint reported it as "plugin does not support state collection" — an entry that
reads as an oversight rather than a deliberate statement about the plugin.
The plugin genuinely holds no reachable mutable runtime state:
roundRobinis astateless singleton whose only field is its immutable name. Its per-priority-band
round-robin cursors are created by
NewStateand owned by the flow-controlregistry (
PriorityBandAccessor.PolicyState); the plugin keeps no reference tothem and cannot enumerate them.
Change
Implement
plugin.StateDumperfor interface-contract completeness, with thesmallest honest payload:
policy— the policy type;stateful: false;note— pointing at where the cursors actually live (the flow-control registry,per priority band).
This is deliberately not an attempt to surface cursor positions: none are reachable
from the plugin, and the dump makes that explicit so operators are not misled. The
payload is a fixed three-field struct independent of band or flow cardinality, so
no bounding/
truncatedflag is needed and nothing sensitive (flow IDs, requestdata, even the instance name) is emitted. No registration change is required
(the plugin is already registered; the debug handler discovers
StateDumperbytype assertion). No lock is added — there is no state to guard.
Tests
Table-driven coverage for default-name and custom-name (identical bytes, proving
the name is not emitted), a raw-key/no-leak assertion (only
policy,stateful,note), and an invariant-after-Pickcase: advancing a band cursor throughPickleaves
DumpStatebyte-identical, proving the dump cannot and does not reflectcursor movement.
CodeAnt-AI Description
Show a clear round-robin fairness state entry in debug output
What Changed
/debug/plugins/statewith a small, fixed state entry instead of “state collection not supported”stateful: falsemarker, and a note explaining that cursor positions live in the flow-control registryPickcallImpact
✅ Clearer plugin state pages✅ Fewer misleading debug messages✅ Safer round-robin troubleshooting💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Tests