Aias00 commented on PR #7848:
URL: https://github.com/apache/shenyu/pull/7848#issuecomment-6282517186
## 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.
--
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]
