fix(plugin): make the hot reload of plugins transactional - #13720
Open
AlinsRan wants to merge 12 commits into
Open
fix(plugin): make the hot reload of plugins transactional#13720AlinsRan wants to merge 12 commits into
AlinsRan wants to merge 12 commits into
Conversation
`plugin.load()` used to tear the live plugin tables down before rebuilding them: it destroyed every old plugin, cleared `local_plugins` / `local_plugins_hash` in place, and then re-required and re-`init()`ed the plugins one by one, with no protection and no rollback. An unprotected `init()` therefore leaves the worker permanently broken: the plugins that had already been loaded stay in the array (`load_plugin` inserted before calling `init()`, so even the failing plugin stays), the `local_plugins_hash` rebuild never runs so the hash stays empty and the Admin API rejects every plugin with `unknown plugin [...]`, and which plugins survive depends on the `pairs()` order, so it differs per worker. The error is swallowed by the worker-events pcall while the reload endpoint has already answered `200 done`. Rework `load()` / `load_stream()` into three phases: 1. build the new plugin set in a local table, so the tables read by the request path are untouched while modules are required; 2. destroy the old instances, then run the `init()` / `workflow_handler()` hooks of the new ones under pcall. On failure, destroy the new instances, restore the `package.loaded` snapshot and re-init the old instances, then return the error; 3. on success, repopulate the live tables in place. The old instances are destroyed before the new ones are initialized (rather than the other way around) because plugins such as server-info and log-rotate register timers globally by name, so an overlap would let the old `destroy()` unregister the timer the new instance just registered. The live tables are repopulated in place rather than swapped, because `_M.plugins` and the `ipairs(plugin_mod.plugins)` readers in api_router / control router alias them; there is no yield point between the clear and the end of the loop, so no request can observe a partially updated set. That also closes the second part of the issue: modules are required before the commit, not during it. The Admin `/apisix/admin/plugins/reload` and Control `/v1/plugins/reload` endpoints now load on the serving worker first and only broadcast the event if that succeeds, so a plugin set that cannot be loaded is reported as `500` instead of an unconditional `200 done`. The event handlers skip the originating worker id to avoid loading twice. Fixes apache#13087
test() returns (status, body, headers) on an error, so `code, _, body` put the headers table into body and ngx.say aborted on it — the whole block produced empty output. Take the body as the second return value. Also declare the expected init() error so the default no_error_log guard does not flag it, and tolerate the create-route status (200 on EE, 201 upstream) plus the trailing newline in the error body.
load()/load_stream() cleared package.loaded for the whole target plugin set before requiring, so the very first load (init_worker) re-required every plugin and threw away the module instances initialized in init_by_lua. A plugin's destroy() registered there was lost (t/node/plugin.t), and any module-level init done in init_by_lua ran twice. Match master: drop only the currently-loaded set (empty on first load, so those modules are reused), which still re-reads code from disk on a reload.
The rollback of an aborted reload destroyed every instance of the new plugin set, including the ones sitting after the failing plugin, whose init() had never run. destroy() of an uninitialized instance publishes its uninitialized state: gm and ocsp-stapling assign their nil upvalue back to radixtree_sni.set_cert_and_key, which breaks the SSL handshake path and is then saved as the "original" function by the rollback, so the damage survives the reload. Both plugins have a lower priority than almost everything else, so any failing init() reaches them. Destroy only the instances whose init() completed, and do it in the reverse order of init(), so that plugins wrapping a shared function unwind their chain innermost first. The old instances are destroyed in reverse order too, for the same reason.
The reverse-order destroy had no coverage: reverting it alone left the suite green. TEST 4 asserts the hook sequence of two probe plugins across an aborted reload, which pins both the LIFO unwind of the old set and the fact that a reload failing on the very first plugin (inited = 0) destroys nothing of the new set. The comment claimed the innermost wrapper is removed first, which is the opposite of what the code does: gm installs first and is the inner layer, so it is ocsp-stapling, the outermost, that has to go first.
exit_worker() iterated the hash, i.e. in an arbitrary order, which is the one remaining path that does not respect the unwind order the reload now guarantees. Impact is near zero since the process is going away, but leaving it inconsistent invites the next reader to copy the wrong pattern.
…ransaction # Conflicts: # apisix/admin/init.lua # apisix/control/v1.lua
pkg_snapshot only records the plugins that were live before the reload, so a plugin the reload introduces has no entry there. Restoring the snapshot therefore left its module in package.loaded, and phase 1 of the next reload only drops the modules of the live set — which does not include it. require() kept handing back the stale module even after the operator fixed the file on disk, and only a restart cleared it, which contradicts the point of re-reading plugin code on reload. Clear the modules of the whole new set before restoring the snapshot: the plugins present in both sets are put back by the snapshot, and the ones this reload introduced stay gone.
AlinsRan
marked this pull request as ready for review
August 4, 2026 06:33
dosubot
Bot
added
size:XL
The serving worker loads before the version is bumped, so returning 503 on an incr() failure left it on the new plugin set while every other worker kept the old one — and with no version change the reconciliation timer had nothing to notice, so the split never healed. Post the event first and report the failure afterwards: the workers end up in sync through the broadcast, and the operator still learns that a process which missed it has no version to reconcile against.
`require()` populates package.loaded before the field checks run, so a plugin that loads but fails validation — a new plugin missing `priority`, say — left its module cached. Nothing drops it afterwards: the commit phase only unloads plugins that were live and are not anymore, and phase 1 of the next reload only drops the modules of the live set, neither of which includes a plugin that never made it in. The operator fixes the file, reloads, and `require()` hands back the same rejected module; only a restart clears it. This is the same failure the rollback path was fixed for, reached through validation instead of through an aborted reload. Route every rejection through a `reject()` helper that drops the cached module. Also make the 503 from a failed `events:post()` use the `error_msg` object the rest of these handlers return, and spell out in the reconciliation comment what recording the version after a failed load costs: that worker stays on its old set until some later reload bumps the version again.
dosubot
Bot
added
size:XXL
utils/check-lua-code-style.sh rejects reaching for a Lua global, and v1.lua had no local for tostring. Caught by CI lint, which runs that script on top of luacheck - luacheck alone is clean.
membphis
reviewed
Aug 4, 2026
membphis
left a comment
membphis
left a comment
Member
There was a problem hiding this comment.
[P1] Add a real-process Shell regression for the plugin lifecycle
This change rewrites global HTTP/stream plugin load, hot-reload, and unload behavior, but the new coverage is limited to Perl .t tests. That does not exercise the actual gateway lifecycle through the required CI path.
Please add a repository-native t/cli/test_*.sh regression that starts the gateway, covers successful and failed hot reload while the old plugin set remains serving, checks LIFO destroy/init ordering and reverse unload, and uses bounded polling for convergence.
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
2 participants
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 #13087.
plugin.load()destroys every old plugin and clearslocal_plugins/local_plugins_hashbefore re-requiring and re-init()ing them, with nopcalland no rollback. If oneinit()throws, the hash rebuild never runs: the hash stays empty, the Admin API starts rejecting every plugin withunknown plugin [...], the surviving array depends onpairs()order so it differs per worker, and the error is swallowed by the worker-eventspcallwhile the endpoint has already answered200 done. Only a restart or a later successful reload clears it.Solution
load()/load_stream()become a three-phase transaction:init()the new ones, all underpcall. On failure everything is rolled back and the previous set keeps serving.Three details that are not the obvious choice, each explained in a comment at the call site:
server-info,log-rotateanderror-log-loggerregister timers globally by name — initializing first would let the olddestroy()unregister the timer the new instance just registered.destroy()runs in the reverse order ofinit(), and the rollback only destroys instances whoseinit()actually ran.gmandocsp-staplingwrap the same SSL function, so unwinding in the wrong order leaks a stale wrapper, and destroying an uninitialized instance restores anilupvalue overradixtree_sni.set_cert_and_key.package.loaded.require()caches before the checks run, and nothing else would ever drop it, so the next reload would keep handing back the rejected version even after the file on disk is fixed._M.pluginsis an alias other modules hold.Behaviour change (please read)
PUT /apisix/admin/plugins/reloadandPUT /v1/plugins/reloadnow load synchronously on the serving worker first and only broadcast if that succeeded. On failure they return500with the error instead of200 done, and nothing is broadcast. The success path is unchanged (200 done), but is slower, since the request now does a full load instead of only posting an event.The current
200 doneis the worse behaviour — the operator is told the reload succeeded while the plugin table is corrupt — but automation asserting "reload always returns 200" will notice, so this deserves discussion rather than a silent merge.Tests
New
t/admin/plugins-reload-transaction.twith three test-only fixtures undert/apisix/plugins/(a plugin whoseinit()throws, plus two lifecycle probes). Four cases: the aborted reload keeps the previous set live and the route working; a failed reload leaves no sticky state; the rollback destroys only what it initialized; and the hook order is the reverse ofinit(). Each assertion was verified to fail without the corresponding half of the fix.Checklist