Skip to content

feat(subscriber): read engine load via GetLoads gRPC (CAC-235)#160

Open
EdHasNoLife wants to merge 9 commits into
mainfrom
feat/subscriber-grpc-loads
Open

feat(subscriber): read engine load via GetLoads gRPC (CAC-235)#160
EdHasNoLife wants to merge 9 commits into
mainfrom
feat/subscriber-grpc-loads

Conversation

@EdHasNoLife

@EdHasNoLife EdHasNoLife commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Gives the kvevent-subscriber a gRPC-native load source: read the engine's GetLoads RPC and project SchedulerLoad into ReplicaStats, as an alternative to scraping HTTP /metrics.

Why: a vLLM engine served over gRPC (vllm.entrypoints.grpc_server) exposes no HTTP /metrics — live load is available only over GetLoads. Today the subscriber only scrapes HTTP, so IC gets no load and the ranker routes on residency alone. This is the clean long-term fix: structured data, no text round-trip, no extra container.

Design: GRPCLoadsScraper implements the existing statsScraper interface, so StatsReporter is unchanged. Selected by a new --engine-loads-grpc <host:port> flag (empty = HTTP scrape, unchanged default). Mapping mirrors the HTTP scraper: request counts to pressure = clamp01((running+waiting)/ceiling), token_usage to cache_memory_bytes (via --engine-cache-size-bytes), cache_hit_rate to hit_rate; across DP ranks counts SUM, usage MAX, hit-rate MEAN (finite values only).

Vendored proto: a minimal GetLoads-only subset of the vLLM engine gRPC contract (pkg/adapters/engine/vllmengine/), package kept vllm.grpc.engine for wire compat, deliberately outside proto/ so it isn't subject to IC's buf-lint / drift checks (it's a vendored external contract, not IC's API). Regenerable via make proto-gen-vendored; CI drift-checks it.

Caveat: GetLoads carries no external-tier (T2/LMCache) token counters, so those ReplicaStats fields stay HTTP-/metrics-only in this mode; the load signals the ranker uses (pressure, cache-usage, hit-rate) are all provided.

Tests: mapping (single/multi-rank, zero-config, sanitization, overflow, error) + a bufconn end-to-end through the generated stubs + load-source control-flow selection. Full pkg/adapters/engine suite passes. Live end-to-end validation is pending a GetLoads-capable (vLLM >= 0.19) engine.

CAC-235.

@linear-code

linear-code Bot commented Jul 22, 2026

Copy link
Copy Markdown

CAC-235

CAC-249

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • cmd/
    • kvevent-subscriber/main.go [reviewed]
  • pkg/adapters/engine/
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

  • cmd/kvevent-subscriber/main.go:47 — The new load source is not plumbed into the auto-injected sidecar — RenderSubscriberSidecar supplies neither --engine-loads-grpc nor an operator-facing way to configure it, so deployed SMG engines retain the HTTP default and still report no load.
  • cmd/kvevent-subscriber/main.go:47 — The accepted subscriber wiring design was not updated — docs/design/kvevent-subscriber-wiring.md still describes an HTTP-only stats path and treats subscriber tuning as deferred, leaving the documented deployment contract inconsistent with this flag.
  • cmd/kvevent-subscriber/main.go:99 — Load-source selection lacks a control-flow test — scraper mapping and wire behavior are covered, but no test verifies that a nonempty flag selects gRPC and an empty flag preserves HTTP behavior.
  • pkg/adapters/engine/grpc_loads_scraper.go:95 — External load values are not sanitized — out-of-range or non-finite token_usage can produce negative/overflowed cache bytes, while cache_hit_rate can escape its expected [0,1] range before reporting; clamp or reject these values and test the boundary cases.
  • pkg/adapters/engine/vllmengine/vllm_engine.proto:13 — The vendored contract has no reproducible generation or drift-check path — existing make proto-gen only scans proto/, so contributors cannot reliably regenerate or verify the two committed stubs after an upstream contract update.

Nit

  • cmd/kvevent-subscriber/main.go:9 — The command documentation still says the stats path is always an HTTP GET; mention the selectable GetLoads source.
  • cmd/kvevent-subscriber/main.go:162 — Startup logging always emits engine_metrics_url and never the selected gRPC address, making gRPC-mode diagnostics misleading.
  • pkg/adapters/engine/grpc_loads_scraper.go:15 — Exported Go names use Grpc rather than the conventional initialism GRPC (GRPCLoadsScraper, GRPCLoadsScraperConfig).
  • pkg/adapters/engine/grpc_loads_scraper_test.go:119 — The bufconn server’s Serve error is discarded; report unexpected termination through the test instead of swallowing it.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: cmd/kvevent-subscriber/main.go:47
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: cmd/kvevent-subscriber/main.go:9, cmd/kvevent-subscriber/main.go:47, cmd/kvevent-subscriber/main.go:99, cmd/kvevent-subscriber/main.go:162, pkg/adapters/engine/grpc_loads_scraper.go:15, pkg/adapters/engine/grpc_loads_scraper.go:95, pkg/adapters/engine/grpc_loads_scraper_test.go:119, pkg/adapters/engine/vllmengine/vllm_engine.proto:13

Verdict

changes-requested

…ucible vendored proto

- Clamp external token_usage/cache_hit_rate to finite [0,1] so garbage from the
  engine can't overflow cache_memory_bytes or poison hit_rate (NaN-safe).
- Extract buildStatsScraper so the gRPC-vs-HTTP load-source selection is unit
  tested; add the control-flow test.
- Rename GRPCLoadsScraper* to the Go initialism; log the selected load source.
- Add proto-gen-vendored (run by proto-gen) + go:generate + CI drift check so the
  vendored engine stubs are reproducible and can't silently drift.
- Document the selectable gRPC stats path and the deferred sidecar auto-inject of
  --engine-loads-grpc (gated on the engine GetLoads floor) in the wiring doc.
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • [reviewed] .github/workflows/ci.yml
  • /
    • [reviewed] Makefile
  • cmd/kvevent-subscriber/
    • [reviewed] main.go
    • [reviewed] main_test.go
  • docs/design/
    • [reviewed] kvevent-subscriber-wiring.md
  • pkg/adapters/engine/
    • [reviewed] grpc_loads_scraper.go
    • [reviewed] grpc_loads_scraper_test.go
  • pkg/adapters/engine/vllmengine/
    • [reviewed] generate.go
    • [skipped — generated] vllm_engine.pb.go
    • [reviewed] vllm_engine.proto
    • [skipped — generated] vllm_engine_grpc.pb.go

Findings

Blocking

  • Makefile:289 — The new generated pkg/adapters/engine/vllmengine/*.pb.go files are not added to COVER_EXCLUDE. The coverage gate explicitly excludes generated code, but these stubs will now dilute hand-written coverage and can fail the 90% CI gate.

Should-fix

  • pkg/adapters/engine/vllmengine/vllm_engine.proto:3 — Verify: the vendored contract identifies only a mutable repository path, with no upstream commit, tag, or descriptor fixture proving compatibility with the targeted engine version. The bufconn test uses the same locally generated schema on both ends, so it cannot catch an incorrect RPC path, field number, or field type relative to SMG.
  • docs/design/kvevent-subscriber-wiring.md:115 — The document references a StatsReporter “stale-escalation path,” but no such path exists: scrape failures merely log and skip the tick. This gives operators an inaccurate account of failure behavior.
  • pkg/adapters/engine/grpc_loads_scraper.go:117 — Converting usage * float64(CacheSizeBytes) can produce a negative result for sufficiently large valid int64 capacities because MaxInt64 rounds to 2^63 as a float64 before conversion. Validate the capacity or use an overflow-safe conversion so cache_memory_bytes cannot become negative.

Nit

  • pkg/adapters/engine/grpc_loads_scraper.go:97 — Running and waiting counts are trusted even though the adjacent ratio fields are explicitly treated as untrusted external input. Negative rank counts can cancel legitimate load and under-report pressure; clamp each count to zero before aggregation.
  • pkg/adapters/engine/grpc_loads_scraper.go:92 — The comment says hit rate is averaged only across ranks “that report one,” but line 107 counts every load entry, including zero, invalid, or non-finite values mapped to zero. Either clarify that all ranks participate or exclude unavailable/invalid samples.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/engine/vllmengine/vllm_engine.proto:3, docs/design/kvevent-subscriber-wiring.md:115
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: Makefile:289, pkg/adapters/engine/grpc_loads_scraper.go:92, pkg/adapters/engine/grpc_loads_scraper.go:97, pkg/adapters/engine/grpc_loads_scraper.go:117

Verdict

changes-requested

Mapping a non-finite cache_hit_rate to 0 and still counting it dragged the
mean down and contradicted the 'ranks that report one' comment. Exclude
non-finite ranks from the average instead; finite out-of-range values are
still clamped and kept. token_usage keeps finite01 (0 never wins its max).
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • cmd/kvevent-subscriber/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • docs/design/
    • kvevent-subscriber-wiring.md [reviewed]
  • pkg/adapters/engine/
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • generate.go [reviewed]
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

  • Makefile:289 — The coverage exclusion does not include pkg/adapters/engine/vllmengine/*.pb.go. The new generated stubs will be counted as hand-written logic by make cover-check, contrary to its documented policy, potentially failing or materially distorting the 90% gate.
  • pkg/adapters/engine/vllmengine/vllm_engine.proto:4 — Verify the vendored contract against an immutable upstream revision and record that revision. The current repository/path attribution cannot establish that field numbers, types, and the RPC path match the intended SMG release, while the bufconn test only proves compatibility with this same local copy.
  • pkg/adapters/engine/grpc_loads_scraper.go:85 — Verify that an empty GetLoadsRequest returns every DP rank and all required metrics on the target engine. The contract exposes dp_rank and include selectors, but tests use only the locally generated mock and cannot catch upstream empty-selector semantics.

Nit

  • pkg/adapters/engine/grpc_loads_scraper_test.go:119 — Add explicit +Inf and -Inf cases. The implementation contains dedicated infinity handling, but the sanitization test exercises only NaN and leaves those branches unverified.
  • pkg/adapters/engine/grpc_loads_scraper_test.go:123 — Add a cancellation or deadline test for Scrape. Per-call timeout behavior is new control flow and is currently untested, including propagation to GetLoads.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/engine/vllmengine/vllm_engine.proto:4, pkg/adapters/engine/grpc_loads_scraper.go:85
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: Makefile:289, pkg/adapters/engine/grpc_loads_scraper_test.go:119, pkg/adapters/engine/grpc_loads_scraper_test.go:123

Verdict

changes-requested

…o provenance

The generated vllmengine/*.pb.go were counted as hand-written by cover-check;
add the package to COVER_EXCLUDE (it is entirely generated). Also strengthen the
proto provenance note: the fields are copied verbatim from the SMG source and a
re-sync must pin the upstream revision — the bufconn test proves self-consistency
with the generated stubs, not a match to the deployed engine.
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • Root
    • [reviewed] Makefile
  • .github/workflows/
    • [reviewed] ci.yml
  • cmd/kvevent-subscriber/
    • [reviewed] main.go
    • [reviewed] main_test.go
  • docs/design/
    • [reviewed] kvevent-subscriber-wiring.md
  • pkg/adapters/engine/
    • [reviewed] grpc_loads_scraper.go
    • [reviewed] grpc_loads_scraper_test.go
  • pkg/adapters/engine/vllmengine/
    • [reviewed] generate.go
    • [reviewed] vllm_engine.proto
    • [skipped — generated] vllm_engine.pb.go
    • [skipped — generated] vllm_engine_grpc.pb.go

Findings

Blocking

None.

Should-fix

  • pkg/adapters/engine/vllmengine/vllm_engine.proto:18 — Verify: record the exact upstream repository revision from which this external contract was copied. The file itself requires a pinned repo and commit, but supplies neither; the self-hosted bufconn test cannot detect incompatibility with the deployed engine.
  • Makefile:554 — Add pkg/adapters/engine/vllmengine to the pre-pr generated-drift paths. pre-pr now regenerates these stubs through proto-gen but does not inspect them, so its advertised local drift gate can pass with stale vendored generated code.

Nit

  • docs/design/kvevent-subscriber-wiring.md:75 — Remove or clarify the warning about passing flags the binary does not recognize. Every stats flag enumerated immediately before it is now recognized, leaving this inherited explanation misleading.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/engine/vllmengine/vllm_engine.proto:18, docs/design/kvevent-subscriber-wiring.md:75
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: Makefile:554

Verdict

changes-requested

… stale doc note

- Record the exact machxai/smg commit (6d63ec8, 2026-07-09) the vendored proto
  was copied from, verified field-for-field against upstream.
- Add pkg/adapters/engine/vllmengine to the pre-pr generated-drift paths so the
  local gate inspects the stubs it now regenerates (matches the CI check).
- Reword the wiring-doc note: the enumerated stats flags are recognised by the
  binary; they are simply not set by the sidecar yet.
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • cmd/kvevent-subscriber/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • docs/design/
    • kvevent-subscriber-wiring.md [reviewed]
  • pkg/adapters/engine/
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • generate.go [reviewed]
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

None.

Nit

  • cmd/kvevent-subscriber/main.go:159 — The newly added scraper connection’s Close error is discarded — Log a non-nil close error so connection-cleanup failures are not silently swallowed.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: cmd/kvevent-subscriber/main.go:159

Verdict

approve-with-nits

@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • cmd/kvevent-subscriber/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • docs/design/
    • kvevent-subscriber-wiring.md [reviewed]
  • pkg/adapters/engine/
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • generate.go [reviewed]
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

  • docs/design/kvevent-subscriber-wiring.md:111 — The new load source is not configurable through the existing auto-injected subscriber sidecar — Kubernetes users cannot exercise this Kubernetes-native feature without replacing or manually launching the container. Either wire the engine address through the runtime adapter or document a concrete supported deployment/configuration path.
  • pkg/adapters/engine/vllmengine/vllm_engine.proto:16 — Verify: wire compatibility is asserted only against a pinned external source, while the added bufconn test uses stubs generated from this same file and cannot detect an incorrect service path, field number, or field meaning. Add an authoritative upstream-contract comparison/fixture or validate against a GetLoads-capable engine before relying on this adapter.
  • pkg/adapters/engine/grpc_loads_scraper.go:122 — Converting usage * float64(CacheSizeBytes) can produce a negative int64 for valid large positive capacities because MaxInt64 rounds beyond the representable boundary in float64. Bound the conversion or reject capacities that cannot be converted safely.

Nit

  • pkg/adapters/engine/grpc_loads_scraper.go:37 — Adding a second statsScraper implementation leaves the package documentation and StatsReporter comments describing the source exclusively as HTTP /metrics. Update those existing comments so package-level documentation matches the new behavior.
  • cmd/kvevent-subscriber/main.go:117--cache-tier is validated even when the selected gRPC scraper never uses it. HTTP-only options should either be ignored in gRPC mode or clearly documented as still validated.
  • pkg/adapters/engine/grpc_loads_scraper_test.go:125 — Error coverage checks only that some error and non-nil stats are returned; it does not verify wrapping or cancellation/deadline behavior. Assert the underlying error with errors.Is and cover an in-flight RPC cancellation.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: docs/design/kvevent-subscriber-wiring.md:111, pkg/adapters/engine/vllmengine/vllm_engine.proto:16, pkg/adapters/engine/grpc_loads_scraper.go:37
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/adapters/engine/grpc_loads_scraper.go:122, cmd/kvevent-subscriber/main.go:117, pkg/adapters/engine/grpc_loads_scraper_test.go:125

Verdict

changes-requested

float64(CacheSizeBytes) can round above the int64 range at extreme capacities,
so int64(usage*float64(cap)) could overflow to a negative. Clamp in float space
before the conversion and return the exact int64 capacity at the top end. Adds a
MaxInt64 boundary test.
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

.github/workflows/

  • .github/workflows/ci.yml — [reviewed]

Repository root

  • Makefile — [reviewed]

cmd/kvevent-subscriber/

  • main.go — [reviewed]
  • main_test.go — [reviewed]

docs/design/

  • kvevent-subscriber-wiring.md — [reviewed]

pkg/adapters/engine/

  • grpc_loads_scraper.go — [reviewed]
  • grpc_loads_scraper_test.go — [reviewed]

pkg/adapters/engine/vllmengine/

  • generate.go — [reviewed]
  • vllm_engine.proto — [reviewed]
  • vllm_engine.pb.go — [skipped — generated]
  • vllm_engine_grpc.pb.go — [skipped — generated]

Findings

Blocking

None.

Should-fix

None.

Nit

pkg/adapters/engine/grpc_loads_scraper.go:33 — The new load source leaves the engine package documentation stale: pkg/adapters/engine/doc.go and stats_reporter.go still describe the stats path as HTTP /metrics-only. Update those comments to describe the scraper-neutral reporter and both supported sources.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: pkg/adapters/engine/grpc_loads_scraper.go:33
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): Clean

Verdict

approve-with-nits

@EdHasNoLife

Copy link
Copy Markdown
Collaborator Author

Re: the Codex finding that --engine-loads-grpc isn't reachable through the auto-injected sidecar (RenderSubscriberSidecar leaves it on the HTTP /metrics default), so K8s-deployed gRPC engines still report no load.

Tracked as CAC-249 — deferred deliberately, not wired here:

  • Premature vs. the engine floor: GetLoads needs SMG servicer ≥ 0.5.2 / vLLM ≥ 0.19; the deployed engine is 0.18 / 0.5.1. Auto-wiring the flag now points every injected sidecar at an unimplemented RPC → each flips to the stale-load escalation from fix(subscriber): surface load-stats loss loudly (CAC-236) #156 fleet-wide — strictly worse than the HTTP default.
  • Needs a config surface: selecting gRPC vs HTTP + the engine gRPC address is a new CacheBackend.spec field + renderer plumbing (a CRD API change, already a documented non-decision in docs/design/kvevent-subscriber-wiring.md).

Ships once the engine floor moves. The subscriber capability lands in this PR; the operator wiring follows in the ticket.

Ticket: https://linear.app/cachebox/issue/CAC-249

… stats-path docs source-neutral

The overflow test's 'got > math.MaxInt64' is always false for an int64 (SA4003,
failing ci-lint) — the meaningful guard is got < 0 (overflow wraps negative), and
the next assertion already pins the exact MaxInt64 value. Also update doc.go +
StatsReporter doc to describe the selectable load source (HTTP /metrics or GetLoads
gRPC), not HTTP-only (review nit).
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • cmd/kvevent-subscriber/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • docs/design/
    • kvevent-subscriber-wiring.md [reviewed]
  • pkg/adapters/engine/
    • doc.go [reviewed]
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
    • stats_reporter.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • generate.go [reviewed]
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

  • pkg/adapters/engine/grpc_loads_scraper.go:103 — Empty or incomplete loads responses are accepted as successful zero-load samples — A response with no ranks, or fewer entries than dp_rank_count, publishes zero/underreported pressure and cache usage instead of preserving the previous sample through the reporter’s stale path. Validate response completeness and return a wrapped error when the snapshot is unusable.

Nit

  • cmd/kvevent-subscriber/main.go:97 — The shared cache-size flag still says it is multiplied by usage_perc — In gRPC mode it is multiplied by SchedulerLoad.token_usage; describe this generically as the selected source’s cache-utilization ratio.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: Clean
  3. gRPC fail-open semantics: findings above at: pkg/adapters/engine/grpc_loads_scraper.go:103
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/adapters/engine/grpc_loads_scraper.go:103, cmd/kvevent-subscriber/main.go:97

Verdict

changes-requested

…ntract (drop SMG references)

inference-cache is a general repo, so its comments shouldn't name our downstream
gateway fork. Reframe the GetLoads load source + vendored proto as the vLLM engine
gRPC contract (vllm.grpc.engine.VllmEngine / vllm.entrypoints.grpc_server); drop
the machxai/smg provenance + the SMG-servicer version floor (keep the GetLoads +
vLLM >= 0.19 requirement). Regenerated the vendored stubs from the updated header.
@github-actions

Copy link
Copy Markdown

Codex review

Files reviewed

  • .github/workflows/
    • .github/workflows/ci.yml [reviewed]
  • Repository root
    • Makefile [reviewed]
  • cmd/kvevent-subscriber/
    • main.go [reviewed]
    • main_test.go [reviewed]
  • docs/design/
    • kvevent-subscriber-wiring.md [reviewed]
  • pkg/adapters/engine/
    • doc.go [reviewed]
    • grpc_loads_scraper.go [reviewed]
    • grpc_loads_scraper_test.go [reviewed]
    • stats_reporter.go [reviewed]
  • pkg/adapters/engine/vllmengine/
    • generate.go [reviewed]
    • vllm_engine.proto [reviewed]
    • vllm_engine.pb.go [skipped — generated]
    • vllm_engine_grpc.pb.go [skipped — generated]

Findings

Blocking

None.

Should-fix

  • docs/design/kvevent-subscriber-wiring.md:115 — The documented “StatsReporter stale-escalation path” does not exist at this revision. StatsReporter only warns and skips failed ticks, so the design currently promises an operational signal that the implementation never emits.
  • pkg/adapters/engine/vllmengine/vllm_engine.proto:16 — Verify: record the exact upstream source revision and applicable license for this hand-copied vendored contract. The self-referential bufconn test cannot detect upstream wire drift, and “verified against it” is not reproducible without a pinned source URL or commit.

Nit

  • pkg/adapters/engine/grpc_loads_scraper_test.go:169 — The over-wire test exercises pressure and cache-byte mapping but does not assert cache_hit_rate, despite hit rate being one of the three advertised signals. Adding that assertion would keep the complete generated-stub mapping covered.
  • pkg/adapters/engine/grpc_loads_scraper_test.go:146 — The error test covers an immediate RPC error but not timeout or caller cancellation. A deadline test would pin the new scraper’s most important failure-control path.

Per-category coverage

  1. Vendor-neutral naming: Clean
  2. Contract & spec / docs sync: findings above at: docs/design/kvevent-subscriber-wiring.md:115, pkg/adapters/engine/vllmengine/vllm_engine.proto:16
  3. gRPC fail-open semantics: Clean
  4. Quality (tests / error wrapping / package placement / generated drift): findings above at: pkg/adapters/engine/vllmengine/vllm_engine.proto:16, pkg/adapters/engine/grpc_loads_scraper_test.go:146, pkg/adapters/engine/grpc_loads_scraper_test.go:169

Verdict

changes-requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant