test: harden port-bind race in EPP integration harness#16
Open
chethanuk wants to merge 16 commits into
Open
Conversation
🤖 CodeAnt AI — Review Status
Updated in place by CodeAnt AI · last 5 reviews |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR propagates datastore registration failures, bounds and removes flow-control metric series, expands benchmark coverage, and improves EPP listener reservation, gRPC readiness, and manager cleanup. It also updates CI, documentation, and Go module versions. ChangesDatastore registration error propagation
Metric cardinality and flow cleanup
Flow-control benchmark suite
EPP listener reservation and readiness
Documentation and module updates
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant NewTestHarness
participant EPPManager
participant ExtProcServer
participant WaitExtProcReady
participant ExtProcClient
NewTestHarness->>EPPManager: start with reserved listener
EPPManager->>ExtProcServer: serve ext-proc on listener
NewTestHarness->>WaitExtProcReady: wait for connectivity Ready
WaitExtProcReady->>ExtProcServer: establish gRPC connection
ExtProcServer-->>WaitExtProcReady: readiness state
WaitExtProcReady-->>ExtProcClient: readiness result
NewTestHarness->>EPPManager: cancel and await shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
codeant-ai
Bot
added
the
size:L
![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 a retry mechanism for port binding in integration tests to resolve intermittent 'address already in use' failures. It adds helper functions such as isAddrInUse, RetryOnAddrInUse, and WaitExtProcReady, along with corresponding unit tests. The feedback points out an inconsistency in WaitExtProcReady where a nil error from mgrErr in the second select block is ignored, which could cause the harness to incorrectly report that the server is ready when the manager has actually terminated.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/integration/epp/harness.go (2)
223-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRationale duplicated across files — flagged for consolidation.
Restates the same TOCTOU/issue-#1066 explanation already documented on
isAddrInUseintest/integration/util.goand on theportBindMaxAttempts/portBindRetryDelayconstants above. See consolidated comment.🤖 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 `@test/integration/epp/harness.go` around lines 223 - 227, Remove the duplicated rationale comment near the manager restart logic in the harness, retaining only the implementation-relevant explanation already documented by isAddrInUse, portBindMaxAttempts, and portBindRetryDelay.Source: Coding guidelines
75-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRationale duplicated across files — flagged for consolidation.
Same issue-llm-d#1066/TOCTOU explanation as
util.go'sisAddrInUsedoc and theattemptclosure comment below. See consolidated comment.🤖 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 `@test/integration/epp/harness.go` around lines 75 - 80, The constants portBindMaxAttempts and portBindRetryDelay duplicate the issue-#1066 rationale documented elsewhere. Remove the repeated explanatory comment here and retain only the concise constant declarations, leaving the existing retry behavior unchanged.Source: Coding guidelines
test/integration/util.go (1)
647-661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRationale duplicated across files — flagged for consolidation.
This doc-comment restates the
GetFreePort/bind TOCTOU rationale (issue#1066) that is also spelled out intest/integration/epp/harness.go(constants doc and theattemptclosure comment). See consolidated comment below. As per coding guidelines, "State each fact once in its canonical location. Do not duplicate across struct docs, prose, tables, inline comments, and examples."🤖 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 `@test/integration/util.go` around lines 647 - 661, The comment above isAddrInUse duplicates the GetFreePort/bind TOCTOU rationale documented in the epp harness. Remove the duplicated rationale and retain only a concise description of what isAddrInUse detects, while preserving the function’s existing behavior and inline fallback comments.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 `@test/integration/epp/harness.go`:
- Around line 223-227: Remove the duplicated rationale comment near the manager
restart logic in the harness, retaining only the implementation-relevant
explanation already documented by isAddrInUse, portBindMaxAttempts, and
portBindRetryDelay.
- Around line 75-80: The constants portBindMaxAttempts and portBindRetryDelay
duplicate the issue-#1066 rationale documented elsewhere. Remove the repeated
explanatory comment here and retain only the concise constant declarations,
leaving the existing retry behavior unchanged.
In `@test/integration/util.go`:
- Around line 647-661: The comment above isAddrInUse duplicates the
GetFreePort/bind TOCTOU rationale documented in the epp harness. Remove the
duplicated rationale and retain only a concise description of what isAddrInUse
detects, while preserving the function’s existing behavior and inline fallback
comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fcd49c3-c5bd-418f-89aa-74190630ea33
📒 Files selected for processing (3)
test/integration/epp/harness.gotest/integration/util.gotest/integration/util_test.go
chethanuk
force-pushed
the
fix/issue-1066-port-bind-race
branch
from
July 21, 2026 14:04
21b251b to
f87269b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/util.go (1)
655-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo:
extPorcConnSetupPollInterval→extProcConnSetupPollInterval.Minor misspelling ("Porc") in the constant name; harmless but hurts searchability.
🤖 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 `@test/integration/util.go` at line 655, Correct the misspelled constant reference in the context timeout setup from extPorcConnSetupPollInterval to extProcConnSetupPollInterval, using the existing correctly named constant and leaving the surrounding waitCtx/cancel logic unchanged.internal/runnable/grpc.go (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant comments restate the same fact.
Lines 62-63 both say "shut down / terminate on context closed" — pick one. As per coding guidelines, comments should "state each fact once in its canonical location" and stay terse.
✏️ Trim to one line
- // Shutdown on context closed. - // Terminate the server on context closed. - // Make sure the goroutine does not leak. + // Terminate the server on context closed; doneCh prevents the goroutine from leaking.🤖 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 `@internal/runnable/grpc.go` around lines 62 - 64, Remove one of the duplicate shutdown comments in the context-closed handling near the runnable gRPC server, keeping a single terse comment that states the server terminates when the context closes; preserve the separate goroutine-leak note if it conveys distinct information.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 `@internal/runnable/grpc.go`:
- Around line 62-64: Remove one of the duplicate shutdown comments in the
context-closed handling near the runnable gRPC server, keeping a single terse
comment that states the server terminates when the context closes; preserve the
separate goroutine-leak note if it conveys distinct information.
In `@test/integration/util.go`:
- Line 655: Correct the misspelled constant reference in the context timeout
setup from extPorcConnSetupPollInterval to extProcConnSetupPollInterval, using
the existing correctly named constant and leaving the surrounding waitCtx/cancel
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c8223be-6ec0-48b1-8962-4cc46916dd60
📒 Files selected for processing (7)
cmd/epp/runner/test_runner.gointernal/runnable/grpc.gopkg/epp/server/runserver.gotest/integration/epp/harness.gotest/integration/util.gotest/integration/util_test.gotest/utils/network.go
GetFreePort returns a vacated port that the manager binds a moment later, so the OS can hand the port to another process in that window and the bind fails intermittently with 'address already in use' (issue llm-d#1066). Wrap the harness bring-up in a bounded retry that re-rolls the free ports and restarts the manager only on an EADDRINUSE bind failure, resetting metrics between attempts and failing loud once the budget is exhausted so a genuine bind bug still surfaces. GetFreePort's signature is unchanged. Signed-off-by: ChethanUK <chethanuk@outlook.com>
The per-attempt cancellable context reassigns the outer mgrCtx inside the retry closure, which fatcontext flags as a nested context. It is intentional (each attempt derives a fresh context from the stable parent and cancels the prior one), so suppress it with a justified nolint. Verified with golangci-lint on ./test/integration/... = 0 issues. Signed-off-by: ChethanUK <chethanuk@outlook.com>
WaitExtProcReady's pre-dial select already reports an error when the manager exits with a nil error, but the post-dial select fell through to return ready on the same condition. A nil there means the manager returned (context cancelled) and is shutting down, not that the ext-proc server is ready, so mirror the pre-dial block and fail loud instead of handing back a dying server. Signed-off-by: ChethanUK <chethanuk@outlook.com>
The hermetic harness picked three random ports with GetFreePort, which binds, closes, and returns the number, leaving a window for another process to take the port before the server binds it. That is the "address already in use" flake in issue llm-d#1066. Only the ext_proc port is ever used by a test. Let the kernel assign the metrics and health ports instead. A bind on port 0 cannot collide, so two thirds of the race surface goes away without any production change. The metrics server itself stays up, because the pprof and plugin-state debug handlers register through it. This also removes a self-collision: GetFreePort was called three times back to back, each closing its listener before the next, so two calls could return the same port. One call now remains. Refs: llm-d#1066 Signed-off-by: ChethanUK <chethanuk@outlook.com>
GetFreePort selects a port by binding, closing, and returning the number; the server binds it again a moment later. Between the close and the bind the OS can hand that port to another process, which is the "address already in use" flake in issue llm-d#1066. Hand the ext_proc server a listener that is already bound instead. Holding the listener open means the port is never released, so there is no window to lose. Verified: with a listener held, draining the entire ephemeral port range never reassigns that port, and a plain bind of it fails with EADDRINUSE. ExtProcServerRunner gains an optional GrpcListener that takes precedence over GrpcPort, following manager.Server's Listener field, which documents the same pattern for the same reason. It defaults to nil, so production keeps binding from the CLI-supplied port and options.go and the flags are untouched. The health runnable also stays on the port path, which keeps the net.Listen branch under test. Refs: llm-d#1066 Signed-off-by: ChethanUK <chethanuk@outlook.com>
chethanuk
force-pushed
the
fix/issue-1066-port-bind-race
branch
from
July 21, 2026 14:30
f87269b to
9cb8caa
Compare
A held listener cannot lose a port race, so RetryOnAddrInUse and isAddrInUse have nothing left to retry. Removing them returns the harness to straight-line startup: no per-attempt manager rebuild and no matching on the text of the aggregated manager error. The dial-based readiness check has to go with them, for a subtler reason. The listener is bound before the server starts, so the kernel completes the handshake out of the listen backlog and a dial succeeds while grpc.Serve has not run yet. Waiting for the client connection to reach connectivity.Ready requires the HTTP/2 handshake, which only completes once the server is serving, and it still selects on the manager's exit so a start failure fails fast instead of hanging. Both claims are gated by tests rather than asserted: TestReserveListenerDefendsPort holds a reservation, draws 2000 ephemeral ports and shows none of them is the reserved one, and shows an explicit bind of it gets EADDRINUSE; TestPreBoundListenerQueuesDials dials before Serve and shows the request is still served. Refs: llm-d#1066 Signed-off-by: ChethanUK <chethanuk@outlook.com>
chethanuk
force-pushed
the
fix/issue-1066-port-bind-race
branch
from
July 21, 2026 14:45
9cb8caa to
8c66909
Compare
…uite (llm-d#1638) * test(flowcontrol): harden and reorganize the flow-control benchmark suite Test-only changes to pkg/epp/flowcontrol/benchmark; no production code is touched. - Re-scope the full-path benchmark (FullPathStress -> FullPath) to what it uniquely measures: dispatch throughput and allocation overhead through the real producer -> concurrency detector -> controller path. Drop the off-layer producer leak-counter assertion (covered by the inflightload producer's own unit tests), report d/s like the other benchmarks, and fix a b.Fatalf called from inside RunParallel (invalid off the benchmark goroutine). - Fix the mock benchDetector so the PerformanceMatrix L sweep is meaningful: it accumulates in-flight up to the limit and reports saturated once exceeded (real W>L backpressure), while self-healing the empty-queue idle grants the processor's 1ms dispatch ticker would otherwise leak (which previously latched and hung at L=1). Distinguish leaked idle grants from genuine load saturation via a release counter. Add detector unit tests covering saturation-at-limit and self-heal. - Replace fixed bootstrap time.Sleep calls with a warmup request that blocks until dispatch -- deterministic readiness instead of arbitrary timing. - Reorganize into doc.go (two-family overview) + helpers_test.go + benchmark_test.go, and dedup the shared registry/policy setup. - Add a bench-smoke make target and CI step (go test -bench=. -benchtime=1x) so benchmark bodies execute on every PR; go test only compiles them otherwise, letting runtime breakage (panics, deadlocks) go undetected. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * test(flowcontrol): correct FullPath backpressure comment The inline comment claimed the FullPath benchmark sustains W>L queuing, but worker count rounds down to <= L and the real detector's in-flight load is released near-instantly, so dispatch is effectively free-flowing. Reword to match the function godoc: FullPath prices dispatch + the detector's per-cycle DynamicAttribute reads, not sustained backpressure. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * test(flowcontrol): address review feedback on benchmark suite - Use the llm-d Authors copyright header on the new files (doc.go, detector_test.go) per review. - Fail fast if the PerformanceMatrix warmup request is not dispatched, matching the full-path harness, instead of silently timing a dead dispatch loop. - Fail fast if a matrix coordinate dispatches zero requests, so a wedged dispatch path produces a red build instead of plausible zero rows. - Drop the matrix harness TTL from 5m to 30s: nothing in a throughput benchmark legitimately waits minutes, and a tight TTL bounds a wedged coordinate to seconds of CI time instead of minutes per cell. - Document the liveness bias of the benchDetector's stuck-read reclaim heuristic: true queueing still reports saturated while releases flow; a stalled release transiently over-admits rather than latching. Signed-off-by: Luke Van Drie <lukevandrie@google.com> --------- Signed-off-by: Luke Van Drie <lukevandrie@google.com>
… series (llm-d#2119) * fix(metrics): bound fairness_id label cardinality and prune GC'd flow series The fairness_id label on request and flow-control metrics is populated from a client request header (or agent-identity attribute), so its cardinality is not operator-bounded. Prometheus vectors never evict label combinations: every distinct fairness ID ever observed permanently allocated histogram series across both the deprecated and llm_d_epp metric families, growing pod RSS and scrape payloads without bound. Under flow-ID churn this was ~60% of remaining allocations in the full-path flow-control benchmark. Three changes, following the existing model-label precedent: - Bound fairness_id through a boundedLabel limiter (cap 1000, overflow to "other") in every record function that takes it, exactly as model names are already bounded via boundModels. - Bound model labels in RecordFlowControlRequestQueueDuration, which was missing the boundModels call its siblings have. - Prune a flow's flow-control series (both families) when the registry garbage-collects the flow, so the vectors track live flows. Pruning runs after cleanupFlowResources and outside the registry lock; DeletePartialMatch scans whole vectors and must not run under it. FullPath benchmark (registry/flow churn, M4 Pro, -benchtime=3s): 118k -> 213k d/s, 14.5KB -> 4.0KB per op, 264 -> 94 allocs per op. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * test(metrics): cover fairness_id bounding paths; document label cardinality - Verify the request-metric family bounds fairness_id like the flow control family, and that RecordFlowControlRequestQueueDuration bounds its model labels. - Verify flow GC prunes the collected flow's metric series end to end. - Document the client-derived label caps and GC pruning in docs/metrics.md. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * fix(metrics): guard overflow series from flow GC pruning A flow whose client-chosen fairness ID equals the overflow value must not delete the shared series that aggregates every capped-out tenant, so DeleteFlowControlFlowSeries now no-ops on the overflow value. Also document the revive-during-GC gauge race as an accepted limitation and clarify in docs/metrics.md that the model labels share one cap pool and that caps apply over the process lifetime. Signed-off-by: Luke Van Drie <lukevandrie@google.com> --------- Signed-off-by: Luke Van Drie <lukevandrie@google.com>
…tly untracking pods (llm-d#2092) * fix(datastore): surface dropped endpoint registrations and requeue pod reconciles An upsert that overlaps an in-flight delete (or whose collector fails to start) observes a nil endpoint from NewEndpoint with no surviving pods-map entry. Previously this was silently swallowed: the pod stayed untracked until the next pod event, which can leave the flow control dispatch cycle with an empty candidate list and saturation pinned at 1.0. upsertEndpoint now distinguishes the benign duplicate-start race (a concurrent upsert already stored the entry) from a dropped registration, which is returned as an error. PodReconciler propagates it so controller-runtime requeues with backoff; podResyncAll propagates it through PoolSet for the same effect on the pool reconciler; discovery sources log it. Related to llm-d#2060. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * fix(datastore): retry resync after registration drop and apply metadata on upsert races Close gaps in the dropped-registration handling: - PoolSet stores the pool before resyncing, so a PoolSet retried after a resync failure compared the incoming pool against the already-stored identical pool and skipped the resync entirely. A needsResync flag now forces the retried PoolSet to resync until one succeeds. - upsertEndpoint applies the caller's metadata through the update path when a concurrent upsert wins the registration race, instead of silently discarding it. - The dropped-registration error is a sentinel (errRegistrationDropped) so callers and tests can match it with errors.Is. - PodUpdateOrAddIfNotExist skips and logs at DEBUG when the pool is not synced instead of logging a misleading 'Pod already exists'; the added/exists logging lives in one place. - Unchecked calls of the now error-returning PodUpdateOrAddIfNotExist are handled or explicitly discarded to satisfy errcheck. - The bespoke test endpoint factories collapse into a configurable datalayer.FakeEndpointFactory. Related to llm-d#2060. Signed-off-by: Luke Van Drie <lukevandrie@google.com> * chore: use llm-d Authors copyright in fake_factory.go Signed-off-by: Luke Van Drie <lukevandrie@google.com> * chore: fix requeueing typo flagged by CI Signed-off-by: Luke Van Drie <lukevandrie@google.com> --------- Signed-off-by: Luke Van Drie <lukevandrie@google.com>
…d#2128) Signed-off-by: Luke Van Drie <lukevandrie@google.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.81.1 to 1.82.1. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.81.1...v1.82.1) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.82.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: llm-d-router-release-notes[bot] <287676111+llm-d-router-release-notes[bot]@users.noreply.github.com> Co-authored-by: llm-d-router-release-notes[bot] <287676111+llm-d-router-release-notes[bot]@users.noreply.github.com>
* docs: align architecture.md terminology with the data layer The architecture doc referred to a Scrapers component that no longer exists; the metrics subsystem is the Data Layer (data sources plus extractors). Rename the stale Scrapers references, describe the Source -> Extract -> Attribute lifecycle, and link the canonical upstream llm-d architecture docs the review comment pointed at. Refs: llm-d#1307 Signed-off-by: ChethanUK <chethanuk@outlook.com> * docs: rename Metric Scraping heading to include the data layer The section body describes the data layer's Source -> Extract -> Attribute lifecycle, but its heading and table-of-contents entry still read Metric Scraping alone, leaving the last stale reference after the Scrapers -> Data Layer rename. Update the heading and its TOC anchor to match. Signed-off-by: ChethanUK <chethanuk@outlook.com> * docs: data sources collect more than metrics Review feedback on llm-d#2084: sources also poll served models via /v1/models and include notification-based types, not only metric scrapers. Signed-off-by: ChethanUK <chethanuk@outlook.com> --------- Signed-off-by: ChethanUK <chethanuk@outlook.com>
* draft Signed-off-by: Xiyue Yu <xiyue@google.com> * added metadata Signed-off-by: Xiyue Yu <xiyue@google.com> * added video estimation support Signed-off-by: Xiyue Yu <xiyue@google.com> * rename variable Signed-off-by: Xiyue Yu <xiyue@google.com> * changed test Signed-off-by: Xiyue Yu <xiyue@google.com> * added gemma4 live test Signed-off-by: Xiyue Yu <xiyue@google.com> * fixed typo Signed-off-by: Xiyue Yu <xiyue@google.com> * adde logging info Signed-off-by: Xiyue Yu <xiyue@google.com> * changed config setting Signed-off-by: Xiyue Yu <xiyue@google.com> * renamed variable Signed-off-by: Xiyue Yu <xiyue@google.com> * updated readme Signed-off-by: Xiyue Yu <xiyue@google.com> * deleted debug logging Signed-off-by: Xiyue Yu <xiyue@google.com> * fixed linter Signed-off-by: Xiyue Yu <xiyue@google.com> * fixed linter Signed-off-by: Xiyue Yu <xiyue@google.com> --------- Signed-off-by: Xiyue Yu <xiyue@google.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/architecture.md (1)
321-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating canonical upstream links.
This section repeats the Request Scheduler, Configuration, and Data Layer links already maintained at Lines 61, 82, and 227. Keep one canonical index or the inline references, rather than maintaining both.
As per coding guidelines: “State each fact once in its canonical location. Do not duplicate across struct docs, prose, tables, inline comments, and examples.”
🤖 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 `@docs/architecture.md` around lines 321 - 330, Remove the duplicated Request Scheduler, Configuration, and Data Layer entries from the “Canonical llm-d architecture (upstream)” link list, preserving the existing canonical references elsewhere in the document. Keep the remaining unique upstream links unchanged.Source: Coding guidelines
pkg/epp/metrics/cardinality.go (1)
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove comments that only restate code.
pkg/epp/metrics/cardinality.go#L100-L109: retain only the client-controlled-label rationale; remove theboundFairnessIDname/behavior restatement.pkg/epp/metrics/metrics.go#L581-L584: remove or replace the comment with non-obvious rationale rather than paraphrasing the function name.As per coding guidelines, “Comments are terse and only present when the WHY is non-obvious” and “State each fact once in its canonical location.”
🤖 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/metrics/cardinality.go` around lines 100 - 109, Remove the comment lines in pkg/epp/metrics/cardinality.go:100-109 that merely restate boundFairnessID’s name or behavior, retaining only the non-obvious rationale about client-controlled fairness ID cardinality. In pkg/epp/metrics/metrics.go:581-584, remove the comment or replace it with rationale that is not a paraphrase of the associated function name; no other changes are needed.Source: Coding guidelines
pkg/epp/datastore/datastore.go (2)
95-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInterface doc overstates the failure granularity.
The comment says a dropped registration means "the pod is then not tracked by the datastore," but
podUpdateOrAddIfNotExisttracks per-endpoint: a multi-port pod can have some endpoints stored (added=true) while others are dropped and joined into the returned error. Worth tightening the wording so callers don't assume all-or-nothing semantics.✏️ Suggested doc tweak
- // PodUpdateOrAddIfNotExist stores or updates the endpoints for the given pod. It returns an - // error when an endpoint registration was dropped (see upsertEndpoint); the pod is then not - // tracked by the datastore and the caller must retry (e.g. by requeuing the reconcile). + // PodUpdateOrAddIfNotExist stores or updates the endpoints for the given pod. It returns an + // error when one or more of the pod's endpoint registrations were dropped (see upsertEndpoint); + // those endpoints are then not tracked by the datastore and the caller must retry (e.g. by + // requeuing the reconcile). PodUpdateOrAddIfNotExist(ctx context.Context, pod *corev1.Pod) 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/datastore/datastore.go` around lines 95 - 98, Update the PodUpdateOrAddIfNotExist interface documentation to describe per-endpoint failure semantics rather than stating that the entire pod is untracked when an endpoint registration is dropped. Clarify that some endpoints may remain stored while dropped registrations are reported in the returned error, and callers should retry the affected reconciliation as appropriate.
355-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
existingEpSetis populated but never read.
existingEpSet.Insert(...)is called for every endpoint of the pod, but the set is never consulted afterward — the "remove endpoints no longer active" loop below (lines 378-389) usesactivePorts/pool.TargetPortsinstead. This looks like leftover code from a refactor; either remove it or wire it into the cleanup logic if it was meant to replace theactivePorts.Has(port)check.🧹 Suggested cleanup
added := false var errs []error - existingEpSet := sets.Set[types.NamespacedName]{} for _, endpointMetadata := range pods { - existingEpSet.Insert(endpointMetadata.NamespacedName) created, err := ds.upsertEndpoint(ctx, endpointMetadata)Please confirm this wasn't intended to feed the cleanup loop below before removing it.
🤖 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/datastore/datastore.go` around lines 355 - 368, Remove the unused existingEpSet declaration and its existingEpSet.Insert call from the endpoint upsert loop; keep the cleanup logic based on activePorts and pool.TargetPorts unchanged, confirming the set is not required elsewhere in the surrounding function.
🤖 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 `@pkg/epp/flowcontrol/registry/registry.go`:
- Around line 482-484: Update cleanupFlowResources and the flow
revival/metric-recording path so pruning is coordinated atomically with
recreated flows; do not delete a metric series for any key whose flow was
revived after keysToClean was collected. Ensure revived-flow increments remain
intact, using synchronization or generation-specific metric series as
appropriate while preserving cleanup for genuinely stale keys.
---
Nitpick comments:
In `@docs/architecture.md`:
- Around line 321-330: Remove the duplicated Request Scheduler, Configuration,
and Data Layer entries from the “Canonical llm-d architecture (upstream)” link
list, preserving the existing canonical references elsewhere in the document.
Keep the remaining unique upstream links unchanged.
In `@pkg/epp/datastore/datastore.go`:
- Around line 95-98: Update the PodUpdateOrAddIfNotExist interface documentation
to describe per-endpoint failure semantics rather than stating that the entire
pod is untracked when an endpoint registration is dropped. Clarify that some
endpoints may remain stored while dropped registrations are reported in the
returned error, and callers should retry the affected reconciliation as
appropriate.
- Around line 355-368: Remove the unused existingEpSet declaration and its
existingEpSet.Insert call from the endpoint upsert loop; keep the cleanup logic
based on activePorts and pool.TargetPorts unchanged, confirming the set is not
required elsewhere in the surrounding function.
In `@pkg/epp/metrics/cardinality.go`:
- Around line 100-109: Remove the comment lines in
pkg/epp/metrics/cardinality.go:100-109 that merely restate boundFairnessID’s
name or behavior, retaining only the non-obvious rationale about
client-controlled fairness ID cardinality. In
pkg/epp/metrics/metrics.go:581-584, remove the comment or replace it with
rationale that is not a paraphrase of the associated function name; no other
changes are needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86ddbc0f-638e-431c-ae14-30392f3bee5e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/ci-pr-checks.yamlMakefilecmd/epp/runner/test_runner.godocs/architecture.mddocs/metrics.mdgo.modinternal/runnable/grpc.gopkg/epp/controller/pod_reconciler.gopkg/epp/controller/pod_reconciler_test.gopkg/epp/datalayer/fake_factory.gopkg/epp/datastore/datastore.gopkg/epp/datastore/datastore_test.gopkg/epp/flowcontrol/benchmark/benchmark_test.gopkg/epp/flowcontrol/benchmark/detector_test.gopkg/epp/flowcontrol/benchmark/doc.gopkg/epp/flowcontrol/benchmark/helpers_test.gopkg/epp/flowcontrol/registry/registry.gopkg/epp/flowcontrol/registry/registry_test.gopkg/epp/metrics/cardinality.gopkg/epp/metrics/cardinality_test.gopkg/epp/metrics/metrics.gopkg/epp/requestcontrol/director_test.gopkg/epp/server/runserver.gorelease-notes.d/unreleased/2119.mdtest/integration/epp/harness.gotest/integration/util.gotest/integration/util_test.gotest/utils/network.gotest/utils/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
- test/utils/network.go
- cmd/epp/runner/test_runner.go
- internal/runnable/grpc.go
- pkg/epp/server/runserver.go
- test/integration/util_test.go
- test/integration/util.go
- test/integration/epp/harness.go
Comment on lines
+482
to
+484
| for _, key := range keysToClean { | ||
| metrics.DeleteFlowControlFlowSeries(key.ID, strconv.Itoa(key.Priority)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make metric pruning atomic with flow revival.
cleanupFlowResources skips a key when its flow has been recreated, but this loop still deletes that key’s live metric series. A revived flow can therefore lose an increment and later recreate a queue gauge with a negative value. Coordinate flow creation/metric recording with pruning, or use generation-specific series, rather than pruning every originally collected key.
🤖 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/flowcontrol/registry/registry.go` around lines 482 - 484, Update
cleanupFlowResources and the flow revival/metric-recording path so pruning is
coordinated atomically with recreated flows; do not delete a metric series for
any key whose flow was revived after keysToClean was collected. Ensure
revived-flow increments remain intact, using synchronization or
generation-specific metric series as appropriate while preserving cleanup for
genuinely stale keys.
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
3 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.
User description
What type of PR is this?
/kind test
What this PR does / why we need it:
The EPP hermetic integration tests intermittently fail on CI with
Server failed to bind port 127.0.0.1:<port>.GetFreePortallocates a port bylistening on
:0, immediately closes the listener, and returns the bare int. Thecontroller-runtime manager binds that port a moment later (gRPC ext-proc, metrics,
health), so the OS can hand the freed port to another process in the window between
the close and the later bind, and the bind fails with
EADDRINUSE.This hardens the test harness only: the port-alloc -> manager-start -> readiness
sequence in
NewTestHarnessis wrapped in a bounded retry that re-rolls all threefree ports and restarts the manager only on an address-in-use bind failure
(bounded to 5 attempts, 500ms apart), resetting Prometheus metrics between attempts
and failing loud once the budget is exhausted so a genuine bind bug still surfaces.
The manager goroutine now surfaces
mgr.Start's error on a channel instead ofswallowing it.
GetFreePort's signature is unchanged.New table-driven tests cover the classifier (
isAddrInUse), the retry control flow(
RetryOnAddrInUse: re-roll, budget exhaustion, non-retryable pass-through), and thenon-fatal readiness wait (
WaitExtProcReady).Which issue(s) this PR fixes:
Fixes llm-d#1066
Release note (write
NONEif no user-facing change):CodeAnt-AI Description
Stabilize the EPP integration harness when ports are briefly taken
What Changed
Impact
✅ Fewer flaky CI failures in EPP integration tests✅ Fewer "address already in use" startup failures✅ Clearer failure reporting when the server never binds💡 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
fairness_idcardinality and prune fairness-specific series when flows are garbage-collected.bench-smoke) and a full-path flow-control benchmark.