Neymar-10-JR commented on PR #7848:
URL: https://github.com/apache/shenyu/pull/7848#issuecomment-6297038994
> ## Review — opt-in token budget reservations
> I read the production diff (`RedisTokenBudgetStore`, `TokenBudgetWindow`,
`TokenBudgetRequest`, the four Lua scripts, `AiTokenLimiterPlugin`, the admin
validator and the metrics observer). This is unusually disciplined work for a
change of this size, and I found nothing blocking. Posting as a comment rather
than an approval because **CI has never run on this branch** — see the end.
>
> ### What I checked and liked
> **The Lua scripts are the real contract, and they are written
defensively.**
>
> * `ai_token_reserve.lua` validates every argument with an exact-integer
guard, checks `used > limit or reserved > limit - used or amount > limit - used
- reserved` so the comparison itself cannot overflow, and validates the whole
state _before_ writing anything. Returning `6` (window not current) without
touching state is the right primitive for a fixed window selected with `TIME`.
> * `ai_token_settle.lua` refuses to recreate a missing window, requires a
repeat settlement to carry the _same_ usage, and deliberately allows `actual`
to exceed the original budget instead of rejecting a truthful measurement.
> * `ai_token_complete_without_usage.lua` distinguishes "provably not
forwarded" from "usage unknown", and refuses to downgrade a recorded
`USAGE_UNKNOWN` into a release. That is the exact failure mode a reservation
scheme usually gets wrong.
> * `string.format('%.0f', ...)` everywhere before `HSET` avoids Lua's
scientific-notation serialisation for large integers. Small detail, but it is
the kind of thing that silently corrupts budgets.
> * `ai_token_time.lua` passes a hash-tagged anchor key it never touches,
and `scopePrefix` SHA-256s the scope so a caller-supplied `{}` cannot move the
slot. Both correct for Redis Cluster.
>
> **The reactive lifecycle integration is race-safe.** `beginForwarding` is
a `NOT_FORWARDED → FORWARDED` CAS, and `finish` does `getAndSet(CLOSED)`, so a
cleanup that wins the race prevents forwarding from starting and a request that
already forwarded is never reported as released. `Mono.usingWhen` covers
complete, error and cancel, and `finishReservation` bounds cleanup at 5s so a
slow Redis cannot hold the response open indefinitely. Holding unknown usage
until window expiry, rather than guessing zero, is the conservative choice and
it is documented in the class javadoc.
>
> **Usage extraction distinguishes "zero" from "absent".** Using
`MAPPER.reader().with(FAIL_ON_TRAILING_TOKENS)` in reservation mode means a
partially flushed SSE chunk cannot be parsed into a wrong usage number, and
`isKnownUsage` rejects non-integral, negative and out-of-range values so they
fall through to `USAGE_UNKNOWN` instead of being coerced to `0`. Skipping the
legacy `tokensRecorder` path when `reservationMode` is on avoids
double-counting.
>
> **Validation is applied at both boundaries.**
`AiTokenLimiterRuleHandleValidator` (admin, before persist/publish),
`handlerRule` (gateway cache), and
`AiTokenLimiterHandle.validateReservationSettings()` share one contract, and
absent legacy handles still pass. The SQL additions cover fresh installs plus
all five dialects' upgrade scripts.
>
> ### Non-blocking nits
> 1. `executeWithReservation` constructs `new
RedisTokenBudgetStore(redisTemplate)` per request. It is a thin wrapper so the
cost is small, but it is a per-request allocation in the hot path and could be
a field or a lazily initialised singleton.
> 2. `MAX_EXACT_INTEGER` is declared twice — `private` in
`RedisTokenBudgetStore`, package-private in `TokenBudgetWindow`. One of them
should reference the other.
> 3. `RedisTokenBudgetStore.isValidInteger` does not read instance state;
make it `static`.
>
> ### ⚠️ CI has not run on this branch
> There are no checks on head `ed4c1b5a`, and the reason is not a failure:
all eight workflow runs for `feat/7702-ai-token-reservation` are in
`action_required`, i.e. GitHub is waiting for a maintainer to approve workflow
execution for this fork. Until that happens nothing here is compiled or tested,
including the new Redis-backed job this PR adds to `ci.yml` — which is
precisely the job that should validate the Lua scripts.
>
> Please ask a maintainer to approve the pending runs (or push after one has
been approved), and I will approve once `pr_build` and the new Redis job are
green. Given the size and the fact that this changes admission control on the
data plane, I do not want to approve on code reading alone.
Hi @Aias00, could you please re-run the failed jobs in it-k8s?
The gRPC job failed during “Install k8s”, before the Maven build
or integration tests started:
go install sigs.k8s.io/[email protected]
stream error: stream ID 5; INTERNAL_ERROR; received from peer
Run: https://github.com/apache/shenyu/actions/runs/39399648280
This appears to be a download/network failure. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
