AlinsRan commented on PR #13840:
URL: https://github.com/apache/apisix/pull/13840#issuecomment-5349797067
Thanks — I checked all four against the code before answering. Two are
already handled, one is a real gap and is now fixed, one is real but the
obvious fix does not close it.
**1. Request-path introspection I/O — confirmed, deliberate, staying.**
The fetch is lazy on purpose, and it is bounded rather than unbounded: it is
skipped entirely when the service carries no decorations, so a deployment not
using the cost model never makes the call; it is single-flighted per cache key
with `resty.lock` under an explicit lock timeout, so an unresponsive endpoint
parks one request rather than every request; the result is cached per worker
per service, and a failure is cached for 10s so a dead endpoint costs at most
one upstream request per worker per 10s.
Prefetching at `init_worker` is not available to it: the plugin does not
know at boot which services carry decorations — they arrive through the
`/services` watcher — the derived endpoint depends on the route's upstream, and
the upstream is frequently not reachable during boot. Moving it off the request
path would mean a services-watcher-driven registry plus a refresher timer,
fetching schemas for services that may never receive GraphQL traffic. That is a
reasonable thing to want, but it is a separate change; I would rather not add a
background fetcher to this PR. Tracking separately.
**2. Plugin reload coverage — valid gap, fixed.**
You are right that nothing asserted it. Rather than a `t/cli` lifecycle
script, I added an in-tree case that primes both introspection caches through
the shipped module's own upvalues (not a replica), calls `destroy()` the way
`apisix/plugin.lua` does on reload, and asserts both are empty. I verified it
is not vacuous: making `destroy()` a no-op reddens it.
**3. Backend error classification — already guarded; the residual case is
the operator's own choice.**
`graphql-limit-count.lua` already distinguishes them:
```lua
local code, msg = limit_count.rate_limit(conf, ctx, plugin_name, cost)
if code == 500 and conf.rejected_code ~= 500 then
return code, msg
end
```
A counter-backend failure returns a literal 500 from `limit-count`, a
rejection returns `conf.rejected_code`. The guard is written this way rather
than `code >= 500` precisely so the default `rejected_code` of 503 does not get
returned where the `max_cost` rejection should answer 403.
That leaves exactly one case: `rejected_code == 500`. There the two are
indistinguishable by construction, because the operator chose the rejection
code that collides with the internal one — the ambiguity exists for a plain
rejection too, with or without `max_cost`. And when `max_cost` is exceeded, 403
is the accurate answer regardless of whether the counter also failed: the query
really is over the limit, and that is a property of the query, not of the
counter. The backend failure is not lost either — `limit-count/init.lua` logs
it at error level before returning.
An explicit outcome marker from `rate_limit` would be cleaner, but
`rate_limit` lives in the shared `limit-count/init.lua` and is called by
`limit-count` as well, so changing its contract is not a change this plugin can
make on its own. Tracking separately.
**4. Service recreation race — confirmed real, and reordering does not close
it.**
The window is as you describe: `service_absent` → `base_put` →
`reclaim_stale_decorations`, and a decoration `PUT` landing between the last
two is deleted after returning 201.
I checked the obvious fix — reclaiming before `base_put` — and it does not
work. It only moves the window: `service_absent` says absent, another client
creates the service *and* a decoration, then our reclaim deletes it. The
decoration handler 404s when the parent service is missing, so the window is
empty at that instant, but the check-then-act gap is still there.
The correct fix is the one you point at: delete only the keys that predate
the new service, i.e. those whose `mod_revision` is below the create revision
`base_put` gets back from etcd. `core.etcd.rmdir` is an unconditional range
delete, so this needs a list followed by a per-key compare-and-delete, which is
a different shape from the current one-call reclaim.
Worth noting how narrow it is in practice: the reclaim only runs on a
**create** of a service id that was absent a moment earlier, so the racing
client has to be writing a decoration to a service id that is being created
concurrently by someone else. The failure mode is a lost decoration, not a
wrong cost — the cost model degrades to the node count.
Happy to implement the revision-conditioned reclaim in this PR if you would
rather not carry it; my inclination is to track it separately so this PR does
not also change the etcd write shape. Let me know which you prefer.
--
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]
