Skip to content

perf(consumer): incremental consumer plugin tree rebuild instead of O… - #13707

Open
jlaupa wants to merge 4 commits into
apache:masterfrom
jlaupa:feat/consumer-incremental-rebuild
Open

jlaupa wants to merge 4 commits into
apache:masterfrom
jlaupa:feat/consumer-incremental-rebuild

Conversation

@jlaupa

@jlaupa jlaupa commented Jul 17, 2026 •

Copy link
Copy Markdown

Description

Problem

apisix/consumer.lua's _M.plugin() rebuilds the entire consumer plugin tree on every
consumers.conf_version change:

function _M.plugin(plugin_name)
    local plugin_conf = core.lrucache.global("/consumers",
                            consumers.conf_version, plugin_consumer)
    return plugin_conf[plugin_name]
end

plugin_consumer() iterates all consumers (and credentials) and reconstructs the per–auth-plugin
node arrays. Because the lru-cache key is consumers.conf_version, any consumer create / update /
delete invalidates the whole cache, and the next request that needs consumer data pays an O(N) full
rebuild on the request (access) path
.

With a small consumer base this is invisible. With tens of thousands of consumers and steady consumer
churn (registrations, API-key rotations, deletions) it is not: each change forces a full rebuild, the
rebuilds run on request threads, they stack up under load, request latency collapses and CPU saturates.
The benchmark below reproduces exactly this in a controlled lab setup.

The lru-cache capacity knob (consumers_count_for_lrucache) does not fix this — it only affects how
much of the per-consumer object cache fits; the _M.plugin() path still rebuilds the whole tree on
every conf_version change regardless of that value.

Change

Make the rebuild incremental:

  • Bootstrap: the first request builds the full tree once (unchanged cost, one time).
  • Incremental apply: the existing /consumers etcd filter callback records changed consumers in a
    pending set; on the next conf_version change _M.plugin() applies only those changes — O(changed),
    not O(N). Auth-plugin node arrays are maintained with an index (plugin\0id -> position) and O(1)
    swap-with-last removal.
  • Delete detection: when #consumers.values drops, the changed set is reconciled against the current
    ids and stale entries are removed.
  • Background consistency check (every 30s): a lightweight check — process any pending changes, then
    compare tracked count and scan ids; it only falls back to a full rebuild if it finds an actual
    discrepancy (this covers the rare simultaneous create+delete where the count is unchanged but the set
    differs). It never reconstructs consumer data on the happy path, so steady state is a no-op.

consumers_count_for_lrucache is left at its default (4096) — with incremental rebuild the LRU only
affects fill cost, not stability, so no tuning change is bundled here.

Benchmark

Same harness, same workload, run sequentially on the same host (APISIX container limited to 2 CPUs),
3.17.0 base, 50,000 pre-loaded key-auth consumers, ~600 rps of authenticated traffic rotating across 300
distinct keys, then a 5-minute churn phase of 500 consumer creates (@concurrency 10) + 100 deletes via
the Admin API
under that load. The patched column is this PR's code with the default
consumers_count_for_lrucache = 4096.

Phase Metric vanilla 3.17.0 this PR (incremental)
baseline throughput 544 rps 576 rps
baseline p50 / p99 66 ms / 710 ms 8.5 ms / 303 ms
churn throughput 8.3 rps 568 rps
churn p50 17,243 ms 5.7 ms
churn p99 / max 21,905 ms / 21,910 ms 339 ms / 10,042 ms
churn data-plane timeouts 85% of requests 0
churn Admin API creates 2 / 500 succeeded (rest 503 / timeout) 500 / 500 succeeded
churn container CPU (avg) ~208% ~58%
cooldown p50 32 ms 5.7 ms
— new-key activation not measurable (control plane down) 73 ms

Under churn, vanilla's throughput collapses by ~98% (544 → 8 rps), p50 latency rises from 66 ms to
17 seconds, 85% of requests time out, and the Admin API itself falls over (only 2 of 500 creates
succeed — the rebuild storm starves the control plane too). With the patch, throughput and latency are
essentially flat through churn and the Admin API stays healthy.

Note on the patched column's non-2xx responses: they are 401s equal to the share of keys I deliberately
deleted during churn (100 of the 300 rotated keys) — i.e. deletions propagate immediately and
correctly
(measured per deleted key: time-to-401 p50 87 ms / max 503 ms across all 99 deletes); there are zero timeouts. CPU is reported as docker stats percentage for a container
hard-capped at 2 CPUs (values read high because they are scaled against the host core count); the
meaningful figure is the ratio.

Reproduction

Minimal stack: etcd + apisix (image under test, cpus: 2.0) + an nginx echo upstream, on one host.

APISIX config: etcd config provider, key-auth + prometheus only, admin API open, and
data_encryption.enable_encrypt_fields: false (so 50k consumers can be pre-loaded straight into etcd as
plaintext key-auth for speed; identical for both runs, so it does not bias the comparison — if anything
it understates vanilla's cost by removing per-consumer decryption from the rebuild).

Steps, run once per image (apache/apisix:3.17.0-debian, then a build of this PR), fresh etcd each time:

  1. Create one key-auth route to the echo upstream.
  2. Bulk-load 50,000 consumers via the etcd v3 HTTP gateway
    (POST /v3/kv/put of /apisix/consumers/<user> = {"username","plugins":{"key-auth":{"key"}}}).
  3. Drive ~600 rps closed-loop GET with apikey rotating over 300 keys (multi-process generator),
    logging per-request latency + status.
  4. Timeline under load: 3 min baseline → 5 min churn (500 Admin-API creates @c10 + 100 deletes, spread
    evenly) → 2 min cooldown. Sample container CPU (docker stats) every 1s.
  5. Report latency percentiles, error/timeout counts, achieved rps and CPU per phase; time how long a
    freshly created key takes to authenticate.

(The scripts used — compose file, minimal config.yaml, the etcd bulk loader, the load generator and the
churn driver — are ~200 lines of Python/YAML total and can be attached on request.)

Reliability

A fair objection to incremental state (raised on the route-side PR #12826) is that it can drift and become
less reliable than a full rebuild over long-term operation. This design is built to not trust
incrementality blindly:

  • Deletes are first-class in the incremental path. The etcd watch delivers a value-less event on
    removal; the config filter callback flags it, and the next request reconciles the incremental index
    against the current id set (an O(N) id scan with no consumer-data construction — the cheap part) and
    drops the removed entries. A deleted consumer therefore stops authenticating on the next request, in the
    same latency class as a create — which matters for revocations/bans (security-relevant).
  • Self-healing background check (every 30s). A timer processes any pending changes, then compares the
    tracked count against #consumers.values and scans the incremental index's ids against the current ids.
    On any discrepancy it performs a full rebuild. It is a true anomaly net — not the delete mechanism —
    so worst-case drift is bounded to ≤30s and heals automatically; the system cannot get "stuck" in a wrong
    state the way silent drift would imply.
  • I adversarially benchmarked this failure mode. An earlier revision detected deletes by watching for
    #consumers.values to decrease. Under net-positive churn (many creates interleaved with a few
    deletes) the count never drops, so deletes were missed on the fast path and only caught by the 30s full
    rebuild — which, at the stock consumers_count_for_lrucache, is itself expensive. My own benchmark
    surfaced exactly this (deleted keys kept authenticating; CPU spiked on the periodic full rebuilds), so
    this version makes deletes first-class as above and keeps the background check purely as a safety net.
    The regression test TEST 13–16 in the suite reproduces the edge (delete a consumer while creating
    others, net count rising → the deleted key is rejected on the next request).
  • Production mileage. This implementation has been running since February 2026 in a production
    deployment I operate — tens of thousands of consumers, sustained high traffic with steady consumer
    churn — without issues.
  • Covered by tests. The added Test::Nginx suite exercises create → auth, key update (old key
    rejected / new key accepted), delete → rejected, delete-under-net-positive-churn, credential-based
    consumers, and consumer-group plugin merge, so the incremental paths are validated in CI.

If a maintainer prefers, the background check interval is a one-line constant and could be exposed or
shortened; the full-rebuild fallback guarantees correctness regardless of its value.

Backward compatibility

No configuration changes, no Admin-API or schema changes, and no change to observable behavior: the same
consumers resolve to the same auth data. consumers_count_for_lrucache is untouched. The public surface
of apisix/consumer.lua (_M.plugin, _M.consumers_conf, filter_consumers_list, init_worker, …) is
unchanged; only the internal rebuild strategy behind _M.plugin() differs, plus one background timer.

Prior art

This is the consumer-side counterpart of the route-side idea in
#12826 (incremental radixtree router build), which a
maintainer showed interest in and which was closed by the stale bot rather than rejected. Related consumer
performance reports: #9692,
#9140. An earlier iteration of this approach has been
running since February 2026 in a production deployment I operate — tens of thousands of consumers under
sustained high traffic with steady consumer churn — without issues; the submitted version additionally
hardens delete handling (see Reliability above).

Which issue(s) this PR fixes:

Related: #9692, #9140 (and #12826, the route-side counterpart of the same idea).

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

…(N) full rebuild per change

_M.plugin() rebuilt the entire consumer plugin tree via core.lrucache.global on
every consumers.conf_version change, which is O(N) on the request path. With a
large consumer base and steady consumer churn, rebuilds take seconds and stack
up: request latency collapses and CPU saturates.

Replace the full rebuild with an incremental one: a pending-set fed by the etcd
filter callback applies only changed consumers on conf_version change; a full
rebuild happens only on bootstrap (first request); a lightweight 30s background
consistency check (count compare + id scan) triggers a full rebuild only if it
detects a discrepancy (covers the simultaneous create+delete edge). No config
or API changes; consumers_count_for_lrucache is unchanged.
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. performance generate flamegraph for the current PR labels Jul 17, 2026
@jlaupa

jlaupa commented Jul 20, 2026

Copy link
Copy Markdown
Author

@membphis I think this could be a great improvement 🙏🏼 can we add reviews?

@juzhiyuan

Copy link
Copy Markdown
Member

Thanks for working on this—avoiding an O(N) rebuild on every consumer change is valuable for large deployments. However, I found several consistency issues that should be addressed before merging:

  • Removing all auth plugins from a consumer is not queued by filter(), so the old cached credential can remain valid indefinitely.
  • Credential IDs are inconsistent: a full rebuild indexes credentials by the leaf credential_id, while incremental events use IDs such as jack/credentials/cred-1. This can leave an old key valid after rotation, cause collisions across consumers, and trigger repeated full rebuilds in the background check.
  • Updating a parent consumer does not rebuild its credential-derived entries, so changes to group_id, labels, or custom_id may never reach requests authenticated through credentials.
  • The new per-plugin kv map has no TTL, so secret-backed credentials may stop refreshing when the external secret changes.
  • The current Code Lint check also fails because the old lrucache variable is now unused.

Please add regression coverage for removing auth plugins, updating a credential that existed before bootstrap, duplicate credential leaf IDs under different consumers, parent consumer group/label updates, and secret rotation. Attaching the benchmark harness would also make the performance results independently reproducible. The optimization goal is worthwhile, but authentication correctness needs to be preserved first.

@juzhiyuan juzhiyuan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline comments for the consistency issues from my earlier summary.

Comment thread apisix/consumer.lua Outdated
Comment thread apisix/consumer.lua Outdated
Comment thread apisix/consumer.lua
Comment thread apisix/consumer.lua
…evoke on empty plugins, TTL-bound secret refresh

Follow-up to @juzhiyuan's review:

- Normalize entity ids across both paths: the full build now keys nodes by the
  same "/consumers"-relative id the incremental events use (stamped in
  plugin_consumer), so bootstrapped credentials can be updated/removed and the
  consistency timer no longer flags them as stale. Fixes cross-consumer
  collisions between credentials that share a leaf id.
- Enqueue an entity on change even when its new plugin set is empty, so removing
  all of a consumer's auth plugins removes its old auth entries.
- On a parent consumer change, refresh all of its child credential entries
  (they clone the parent's group_id/labels/custom_id), so credential-auth'd
  requests do not keep stale group policy/headers.
- Give the per-plugin key map a TTL (KV_TTL, matching the previous global
  lru-cache 300s bound) so secret-backed auth values are re-derived even though
  /secrets changes do not bump conf_version.
- Remove the now-unused module-level lrucache (lint).

Tests: add remove-all-plugins, duplicate credential leaf ids across consumers,
credential rotation, and parent-group-change-propagates-to-credential cases.
@jlaupa
jlaupa force-pushed the feat/consumer-incremental-rebuild branch from 60cec0b to d9317ba Compare July 21, 2026 09:39
The admin-setup blocks used a --- config/location /t stanza with no
--- request line, so Test::Nginx had no request to send and aborted the
whole file at the first such block (Request line should be non-empty).
Add --- request / GET /t to each config-only block so the setup requests
actually fire and the suite runs.
@jlaupa
jlaupa force-pushed the feat/consumer-incremental-rebuild branch from bdc925e to b6ef18c Compare July 21, 2026 11:23
@jlaupa
jlaupa requested a review from juzhiyuan July 22, 2026 07:29
@juzhiyuan

Copy link
Copy Markdown
Member

Hi @nic-6443 @membphis kindly review this PR and let @jlaupa know if anything else needed.

Context: https://the-asf.slack.com/archives/CUC5MN17A/p1784285591039329?thread_ts=1748519766.694379&cid=CUC5MN17A

@membphis

membphis commented Aug 28, 2026 •

Copy link
Copy Markdown
Member

Thank you for PR. Confirmed that the problem does exist, and the overall direction of your proposed changes looks reasonable.

However, this PR introduces substantial changes to the core code path and may affect all users. Therefore, we need to hold it to a high quality bar. In addition to demonstrating the performance improvements, the PR should include comprehensive unit and E2E test coverage to verify correctness and prevent regressions.

@bzp2010 I think you can help to improve this PR, work with @jlaupa together

@juzhiyuan

Copy link
Copy Markdown
Member

Updates: @bzp2010 will review this.

@bzp2010 bzp2010 self-assigned this Sep 10, 2026
@membphis

Copy link
Copy Markdown
Member

I found three issues that should be addressed before merging.

Reviewed head: b6ef18c7d85f. Comparison baseline: 5dccf0e04977.

Validation scope: targeted LuaJIT probes loaded the unmodified consumer.lua from each commit. Configuration callbacks, cache storage, and secret responses were stubbed; a coroutine suspension simulated a yielding remote-secret read. These results are module-level reproductions, not full gateway/Vault integration tests.

[P1] An older secret lookup can restore a superseded authentication key

Trigger: two consumer updates overlap while the first request is resolving a remote secret.

Reproduction:

  1. Warm the consumer key map with an initial key.
  2. Apply version 2, whose key resolves from a remote secret to new1.
  3. Start request A and suspend it inside secret resolution.
  4. Apply version 3 with new2; let request B finish updating the index and authenticate with new2.
  5. Resume request A, then try new1 again.
  6. Invoke the background consistency callback and retry new1.

Result:

Lookup after version 3 is applied Baseline PR
Superseded key new1 Rejected Accepted
Superseded key after the consistency callback Rejected Still accepted
Current key new2 Accepted Accepted

The shared nodes/index have already been modified when kv_upsert() calls secret.fetch_secrets(..., false). A newer update can finish while that call yields. The older coroutine then writes its result into the same pd.kv without checking the entity revision. The final version bookkeeping can mark this mixed state as current. The consistency callback checks IDs, so it does not detect the orphaned key; it can remain accepted until the key map is rebuilt on TTL expiry.

Suggested fix: resolve secrets before publishing shared state, validate the captured entity/config revision before committing the result, and prevent concurrent incremental applications from publishing stale results or consuming each other's pending changes. Add a regression with this coroutine interleaving.

[P1] Snapshot replacement leaves deleted consumers authenticating until the background check

Trigger: the index is already initialized, then a YAML/JSON replacement or an etcd full resynchronization removes a consumer without delivering an individual value-less deletion callback.

Reproduction:

  1. Warm the map and verify that Alice's key authenticates.
  2. Replace the configuration snapshot with one that omits Alice and advance conf_version.
  3. Deliver filters only for surviving records, as the snapshot-loading paths do.
  4. Authenticate with Alice's deleted key before running the consistency callback.
  5. Run the callback and retry.

Result:

Deleted-key lookup Baseline PR
First lookup after the snapshot/version change Rejected Accepted
After the consistency callback Rejected Rejected

Deletion reconciliation runs only when pending_delete is set. That flag depends on a value-less filter event. YAML/JSON replacement removes old records without that event, and etcd full loading filters the records present in the new snapshot. Advancing the version therefore does not necessarily remove stale auth entries. The 30-second timer eventually repairs this, but immediate revocation has regressed.

Suggested fix: supply deletion notifications or an explicit reconciliation signal for snapshot replacement, and complete the removal before serving authentication against the new version. Add snapshot-deletion coverage, including a replacement where other consumers survive.

[P2] A request now performs remote-secret reads for unrelated authentication plugins

Trigger: both key-auth and basic-auth maps have been warmed; a basic-auth consumer's Vault-backed password changes; the next request uses key-auth.

Reproduction:

  1. Create a plaintext key-auth consumer and one or two basic-auth consumers, each with one remote-secret password.
  2. Warm both plugin maps, then reset the secret-resolution counter.
  3. Update only the basic-auth passwords and deliver their configuration callbacks.
  4. Make one lookup for the unchanged key-auth consumer.
  5. Count secret resolutions performed by that key-auth lookup.

Result:

Pending basic-auth updates Baseline secret resolutions PR secret resolutions
1 0 1
2 0 2

These are counted resolver invocations in the probe. For Vault, each uncached reference follows vault.get() -> make_request_to_vault() -> httpc:request_uri(), so the additional resolutions introduce outbound HTTP calls on an otherwise unaffected request.

apply_incremental() processes all pending entities, and add_consumer_entry() eagerly calls kv_upsert() for any plugin whose map is already populated. This couples key-auth traffic to unrelated basic-auth updates and remote-secret latency/availability.

Suggested fix: resolve and cache remote secrets off the request path. At minimum, avoid refreshing another plugin's secret-backed map from the current plugin's request, and retain suitable caching for unchanged entries. Add a regression that asserts the unrelated key-auth request performs no additional secret reads.

The ordinary watch-based key update and deletion control probes passed for both versions; the failures above exercise the snapshot, concurrency, and cross-plugin paths specifically.

@guoqqqi guoqqqi assigned LiteSun and unassigned bzp2010 Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance generate flamegraph for the current PR size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants