fix(admin): reconcile plugins reload instead of trusting the broadcast#13714
Open
AlinsRan wants to merge 2 commits into
Open
fix(admin): reconcile plugins reload instead of trusting the broadcast#13714AlinsRan wants to merge 2 commits into
AlinsRan wants to merge 2 commits into
Conversation
events:post has no delivery guarantee. A process that is connecting or reconnecting to the events broker when a reload is accepted loses that event for good, and nothing ever brings it back in line: plugin.load() has only two call sites and neither of them reconciles. The visible symptom is the privileged agent still running timers of plugins that were removed — log-rotate keeps rotating long after it was disabled. Bump a version counter in the internal-status shared dict before broadcasting, and have every process that registers the reload handler compare it once a second, loading the plugins again when it differs. The broadcast stays as the sub-second fast path; the counter is the slow path that actually guarantees convergence. This mirrors what admin/standalone.lua already does, for the same reason. plugin.load() is idempotent, so a redundant round is harmless. The version is sampled before loading, so a reload accepted while load() runs leaves the versions unequal and gets applied on the next tick. A fresh worker adopts the current version at init_worker, which keeps a HUP reload from triggering a spurious load. Two gaps are left untouched: the stream subsystem shares neither the shared dict nor the events socket with http, and propagation between instances still goes through etcd. Both are pre-existing. Fixes apache#13537
If the shared-dict incr fails the reconciliation timer never sees a change, so a worker that misses the broadcast stays stale forever. Return 503 instead of logging and reporting success. Also check the incr in the reconcile test so a setup failure surfaces directly.
nic-6443
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #13537
PUT /apisix/admin/plugins/reloadreturnsdoneas soon asevents:post()accepts the event, but that post carries no delivery guarantee. A process that is connecting or reconnecting to the events broker at that moment loses the event permanently, and nothing ever brings it back in line —plugin.load()has exactly two call sites and neither reconciles.The privileged agent is where this hurts. It hosts the timers registered by plugins, so a lost reload leaves it running the timers of plugins that were just removed. In #13537 that shows up as log-rotate continuing to rotate long after it was disabled, with no way out except sending the reload again.
This adds a version counter in the
internal-statusshared dict:post_reload_plugins()increments it before broadcasting, so even a completely lost event still leaves a record that a reload was requestedreload_plugins()when they differThe broadcast stays as the sub-second fast path; the counter is the slow path that actually guarantees convergence (≤1s). This is the same shape
admin/standalone.luaalready uses, for the same underlying reason — its comment spells it out.On the details worth checking:
internal-statusis a safe choice. It is declared unconditionally for the http subsystem inngx_tpl.luaand is already a hard dependency of the server-info plugin. The privileged agent is an http process and sees it. The key is its own, so there is no collision with server-info.ngx.timer.everyrather thantimers.lua. The background timer runs all registered entries throughthread_spawn/thread_wait, so one slow entry would drag reconciliation with it; andadmin.init_worker()runs beforetimers.init_worker(), so a self-contained timer is simpler.plugin.load()unloads everything and re-inits, and log-rotate'sdestroy()/init()register and unregister its timer symmetrically, so a redundant round costs nothing. Steady-state cost is one shdict read per process per second.init_worker, so it does not reload just for being new.plugin.load(), so a reload accepted while load is running leaves the versions unequal and gets applied on the next tick rather than being swallowed.plugin.load()throwing (say a brokenconfig.yaml) aborts the timer callback without recording the version, so it retries next tick and keeps logging. Deliberately not swallowed — a transient failure should not permanently skip a reload.One API nuance to note: a failed
events:poststill returns 503, which is left as is. Since the increment already happened, that 503 now means "broadcast failed but the reload will still take effect within a second" rather than "nothing happened". Retrying the PUT is idempotent either way.Two gaps are deliberately left open, both pre-existing: the stream subsystem shares neither the shared dict nor the events socket with http (
admin/standalone.luaacknowledges the same limitation), and propagation between instances still goes through etcd/plugins.Tests
New
t/admin/plugins-reload-reconcile.t:Verified discriminating: without the change TEST 1 reports
bumped=falseand TEST 2 sees no reload; TEST 3 passes either way. Ran the file five times to confirm it is not timing-flaky.