Skip to content

lua: run __shutdown__ with every Lua state quiesced - #13556

Draft
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:ts-lua-shutdown-barrier
Draft

lua: run __shutdown__ with every Lua state quiesced#13556
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:ts-lua-shutdown-barrier

Conversation

@bneradt

@bneradt bneradt commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

In draft until @shukitchan is available to review.


A shutdown function usually releases process global resources, and
often does so through FFI into a native library. The shutdown handler
locked only the state whose shutdown it was invoking and released
that lock before moving to the next state, so the callback running in
state 0 could tear those resources down while a request callback was
still using them in state 1. Production has crashed this way during
restart, inside a global read-request callback rather than in any
shutdown path.

This patch hands the callbacks to a thread of its own, which acquires
every main Lua state mutex, global and remap, before invoking any
shutdown function and keeps them for the rest of the process
lifetime, so nothing queued behind one of them can enter Lua after the
resources it uses are gone. A separate thread is required because
ProxyMutex is recursive per event thread: the lifecycle thread would
still enter Lua itself when it returns to its event loop for a final
iteration. Acquisition is bounded, and the callbacks are skipped with an
error rather than run against a state that never went idle. Remap
instance teardown declines to wait on the retained mutexes, so it cannot
deadlock the rest of shutdown. A single continuation now invokes every
script's shutdown, so the states are quiesced exactly once no matter
how many scripts define one.

@bneradt bneradt added this to the 11.0.0 milestone Aug 17, 2026
Copilot AI lite review requested due to automatic review settings August 17, 2026 19:34
@bneradt bneradt self-assigned this Aug 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bneradt

bneradt commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

I think there is still a remap-state re-entry window at unlockStates(remap_states).

The lifecycle hook returns before AutoStopCont calls shutdown_url_rewrite() and TSSystemState::shut_down_event_system(). An existing HttpSM retains a shared_ptr lease on the remap configuration, so a remap callback already blocked on one of these mutexes can acquire it as soon as the barrier releases it and enter Lua after the global __shutdown__ has freed shared process/FFI resources. That preserves the crash race when proxy.config.plugin.dynamic_reload_mode=0. The new AuTest drives shutdown load only through the global hook; the remap Lua script is exercised once before shutdown, so it does not detect this re-entry path.

My suggested fix is to add a per-state execution-stop flag, for example ts_lua_main_ctx::is_shutting_down, protected by that state's mutex:

  1. After the barrier has acquired every global and remap state mutex, but before invoking any __shutdown__, set is_shutting_down = true on every state.
  2. In every runtime Lua entry/resume path, check the flag immediately after acquiring the state mutex. If it is set, skip Lua and perform the appropriate ATS-side reenable/cleanup. This includes the global/remap/vconn handlers and coroutine, transform, intercept, and fetch resume paths.
  3. Keep explicit teardown such as ts_lua_del_module() exempt from the runtime gate, so TSRemapDeleteInstance can still acquire the released remap mutexes and invoke __clean__.
  4. The global locks can remain held as this patch already does; the remap locks can then be released safely because queued callbacks will acquire them, observe the flag, and decline to enter Lua.

Because the flag is set only while the barrier owns all state mutexes and is read only after acquiring the corresponding mutex, it does not need to be atomic: callbacks that were already executing drain before the barrier completes, while callbacks queued behind it see the stopped state.

I would also extend the AuTest with a remap Lua callback held/queued across SIGTERM and assert that it never logs execution after the global shutdown marker.

A __shutdown__ function usually releases process global resources, and
often does so through FFI into a native library. The shutdown handler
locked only the state whose __shutdown__ it was invoking and released
that lock before moving to the next state, so the callback running in
state 0 could tear those resources down while a request callback was
still using them in state 1. Production has crashed this way during
restart, inside a global read-request callback rather than in any
shutdown path.

This patch hands the callbacks to a thread of its own, which acquires
every main Lua state mutex, global and remap, before invoking any
__shutdown__ function and keeps them for the rest of the process
lifetime, so nothing queued behind one of them can enter Lua after the
resources it uses are gone. A separate thread is required because
ProxyMutex is recursive per event thread: the lifecycle thread would
still enter Lua itself when it returns to its event loop for a final
iteration. Acquisition is bounded, and the callbacks are skipped with an
error rather than run against a state that never went idle. Remap
instance teardown declines to wait on the retained mutexes, so it cannot
deadlock the rest of shutdown. A single continuation now invokes every
script's __shutdown__, so the states are quiesced exactly once no matter
how many scripts define one.
@bneradt
bneradt force-pushed the ts-lua-shutdown-barrier branch from aa043d4 to c20efbf Compare August 17, 2026 20:55
Copilot AI review requested due to automatic review settings August 17, 2026 20:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bryancall
bryancall requested a review from shukitchan August 17, 2026 21:54
@bneradt
bneradt marked this pull request as draft August 17, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants