Skip to content

fix(admin): reconcile plugins reload instead of trusting the broadcast#13714

Merged
AlinsRan merged 2 commits into
apache:masterfrom
AlinsRan:fix/plugins-reload-reconcile
Jul 22, 2026
Merged

fix(admin): reconcile plugins reload instead of trusting the broadcast#13714
AlinsRan merged 2 commits into
apache:masterfrom
AlinsRan:fix/plugins-reload-reconcile

Conversation

@AlinsRan

Copy link
Copy Markdown
Contributor

Description

Fixes #13537

PUT /apisix/admin/plugins/reload returns done as soon as events: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-status shared dict:

  • post_reload_plugins() increments it before broadcasting, so even a completely lost event still leaves a record that a reload was requested
  • every process that registers the reload handler compares the shared version against the one it applied, once a second, and calls reload_plugins() when they differ

The 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.lua already uses, for the same underlying reason — its comment spells it out.

On the details worth checking:

  • internal-status is a safe choice. It is declared unconditionally for the http subsystem in ngx_tpl.lua and 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.every rather than timers.lua. The background timer runs all registered entries through thread_spawn/thread_wait, so one slow entry would drag reconciliation with it; and admin.init_worker() runs before timers.init_worker(), so a self-contained timer is simpler.
  • Idempotence. plugin.load() unloads everything and re-inits, and log-rotate's destroy()/init() register and unregister its timer symmetrically, so a redundant round costs nothing. Steady-state cost is one shdict read per process per second.
  • No spurious reloads. Cold start: the dict is empty, both sides are 0. HUP reload: the dict survives, and a fresh worker adopts the current version at init_worker, so it does not reload just for being new.
  • Concurrent reloads. The version is sampled before 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 broken config.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:post still 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.lua acknowledges the same limitation), and propagation between instances still goes through etcd /plugins.

Tests

New t/admin/plugins-reload-reconcile.t:

  • TEST 1 — a reload request bumps the shared version
  • TEST 2 — a process whose applied version is behind reloads within the reconciliation interval, reproducing the "broadcast never arrived" state directly rather than through the events layer
  • TEST 3 — an unchanged version never triggers a reload

Verified discriminating: without the change TEST 1 reports bumped=false and TEST 2 sees no reload; TEST 3 passes either way. Ran the file five times to confirm it is not timing-flaky.

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
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 20, 2026
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.

@membphis membphis 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.

LGTM

@AlinsRan
AlinsRan merged commit c9eaca6 into apache:master Jul 22, 2026
16 checks passed
@AlinsRan
AlinsRan deleted the fix/plugins-reload-reconcile branch July 22, 2026 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: plugins reload event may never reach the privileged agent, leaving timers of removed plugins running

4 participants