Skip to content

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

Open
AlinsRan wants to merge 2 commits into
apache:masterfrom
AlinsRan:fix/plugins-reload-reconcile
Open

fix(admin): reconcile plugins reload instead of trusting the broadcast#13714
AlinsRan wants to merge 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

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

3 participants