fix: close race in Avo.boot that corrupts the plugin registry#4623
Open
Paul-Bob wants to merge 5 commits into
Open
fix: close race in Avo.boot that corrupts the plugin registry#4623Paul-Bob wants to merge 5 commits into
Paul-Bob wants to merge 5 commits into
Conversation
Avo.boot resets and repopulates Avo.plugin_manager's registry on every code reload (config.to_prepare fires on every reload thread), with no synchronization. Two threads booting concurrently -- routine in the Avo admin, where a single page load fires many parallel turbo-frame requests -- can interleave and leave duplicate engine entries, which crash the next route redraw with "Invalid route name, already in use" and wedge the dev server until restart. Serialize Avo.boot with a Mutex, and give PluginManager a staging buffer that's only published to the live @plugins/@engines via a single atomic reassignment once a boot's hooks finish. This closes the duplicate-registration race and also ensures a concurrent reader (e.g. mount_avo's route-drawing loop, which runs independently of Avo.boot) never observes a partially rebuilt registry mid-boot.
Fires concurrent Avo.boot calls against a stubbed pro-gem engine, then draws mount_avo into an isolated RouteSet and asserts no ArgumentError. Includes a sanity check proving the assertion is a real regression guard: mount_avo does raise the reported "Invalid route name, already in use" error when the registry actually holds a duplicate.
- Move eager_load_actions outside the boot mutex -- it doesn't touch the plugin registry state the lock protects, so it no longer blocks concurrent reload threads' registry rebuilds on unrelated class loading. - Extract the duplicated "isolate the global plugin manager" around-hook into a shared_context, and the duplicated draw_mount_avo test helper into spec/support/helpers.rb (also de-duplicating it out of the pre-existing mount_avo_spec.rb). - Tighten PluginManager#engines to attr_reader (was attr_accessor), closing a public writer that could bypass the new atomic-publish registry and reintroduce a torn read. - Document why #reset and mount_engine's per-class dedup still exist now that begin_reload/commit_reload own the boot lifecycle. While extracting the shared plugin-manager isolation hook, found and fixed a real bug in the original around-hook: reassigning Avo.plugin_manager back to the same object after the example doesn't undo Avo.boot's in-place mutation of that object's @plugins/@engines, so the "isolation" was a no-op. This was leaking fake test engines into the global registry and broke the pre-existing mount_avo_spec.rb when run after these specs. Fixed by swapping in a throwaway PluginManager for the example's duration instead.
Independent code review (correctness, testing, maintainability, and adversarial reviewers, plus a separate cross-model pass) converged on the same P0: PluginManager#initialize never set @building_plugins/ @building_engines, and #commit_reload nil'd them out after publishing. gems/avo-permissions calls Avo.plugin_manager.register directly inside a Rails initializer (not wrapped in on_load(:avo_boot) like every other pro gem), which always runs before Avo.boot's first begin_reload -- so every app with avo-permissions installed would crash on boot with NoMethodError. Initialize the staging buffers to [] and reset them to [] (not nil) in commit_reload, so register/mount_engine are safe to call at any time -- matching the pre-atomic-publish behavior where they never crashed regardless of timing. A call outside an active reload cycle is now simply discarded by the next begin_reload instead of raising. Also, per the same review: - Remove PluginManager#reset: confirmed dead (zero real callers), and it wrote directly to @plugins/@engines bypassing the mutex and staging buffer entirely -- an unguarded second door back into the exact torn-read hazard this fix exists to close. - Tighten commit_reload's docstring: the two publish reassignments are atomic individually, not jointly -- a reader combining .plugins and .engines in one operation could observe different reload generations. No current reader does this, but the prior wording implied a stronger guarantee than the code provides. - Reframe the "does not deadlock" spec (it only tested two sequential, non-nested calls, which were never at risk) and add a real reentrancy test proving genuine same-thread nesting raises ThreadError rather than hanging. - Add a test proving a hook exception mid-boot leaves the previously published registry intact and still releases the mutex. - Add a bounded timeout to the reader-consistency spec's rendezvous so a future regression fails clearly instead of hanging CI.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Draws mount_avo into a fresh, isolated RouteSet so specs exercising route | ||
| # drawing (e.g. mount_lookbook config, plugin-engine mounting) never touch | ||
| # the dummy app's real routes. | ||
| def draw_mount_avo(**options) |
Contributor
There was a problem hiding this comment.
[rubocop] reported by reviewdog 🐶
[Corrected] Style/ArgumentsForwarding: Use anonymous keyword arguments forwarding (**).
| # the dummy app's real routes. | ||
| def draw_mount_avo(**options) | ||
| routes = ActionDispatch::Routing::RouteSet.new | ||
| routes.draw { mount_avo(**options) } |
Contributor
There was a problem hiding this comment.
[rubocop] reported by reviewdog 🐶
[Corrected] Style/ArgumentsForwarding: Use anonymous keyword arguments forwarding (**).
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
1 participant
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.
In development, a code change followed by a few concurrent requests could wedge the whole dev server: it would crash on the next request with
ArgumentError: Invalid route name, already in use: 'avo_<some_pro_gem>', and stay crashed until a manual restart, because the failed reload never reached the code that would have fixed it. This is common in the Avo admin, where a single page load fires many parallel turbo-frame requests, each of which can trigger its own reload.What changed
Avo.bootruns on every reload (config.to_prepare) and once at startup. It reset the plugin/engine registry and then repopulated it from every pro gem'son_load(:avo_boot)hook, with no synchronization. Two threads reloading at once could interleave that reset-then-repopulate sequence and leave each pro gem's engine registered twice, which is whatmount_avothen choked on while drawing routes.The fix has two parts:
Avo.bootis now wrapped in aMutex, so concurrent reloads fully serialize instead of interleaving.PluginManagerbuilds registrations into a staging buffer during a boot and publishes them to the live, publicly-read state via one atomic reassignment only once every hook has finished. This closes the original duplicate-registration bug, and also a second, related race: without it, a route redraw running on another thread while a boot was mid-flight could still observe a half-rebuilt registry and silently mount only some of the installed pro gems.A plain mutex around the old reset-in-place code would have closed the first race but not the second — a redraw racing an in-flight boot could still see a torn registry, just without a crash to signal it. The staging-buffer/atomic-publish design closes both with the same mechanism.
We also considered building into a fresh
PluginManagerinstance and swapping the global reference, but every pro gem registers by calling the globalAvo.plugin_manageraccessor directly from its ownon_load(:avo_boot)hook — swapping the instance mid-boot would just relocate the race to whichever manager happened to be current at that moment.Also fixed: a boot-time crash the above change would have introduced
Review caught that
avo-permissionsregisters itself directly inside a Rails initializer, not insideon_load(:avo_boot)like every other pro gem — so it always runs beforeAvo.boot's first reload cycle. The initial version of this fix would have made that call raiseNoMethodErroron every boot of any app withavo-permissionsinstalled.PluginManager#register/#mount_engineare now safe to call at any time again, matching the pre-fix behavior where an out-of-cycle call was silently discarded rather than crashing.Test plan
Three new spec files reproduce both races directly and prove the fix holds:
spec/lib/avo/boot_thread_safety_spec.rb— concurrentAvo.bootcalls leave no duplicate registrations; a reader mid-boot never observes a partially rebuilt registry; a hook exception mid-boot leaves the last-good registry intact and still releases the mutex; genuine same-thread reentrancy raisesThreadErrorrather than hanging.spec/lib/avo/plugin_manager_spec.rb—PluginManager's staging-buffer/publish behavior in isolation, includingregister/mount_enginecalled outside a boot cycle.spec/lib/avo/plugin_manager_boot_race_spec.rb— the exact end-to-end reported symptom: concurrent boots followed by a real route redraw, with a sanity check proving the same setup does raise the reported error against a deliberately corrupted registry.Before finalizing, I verified the new specs actually catch the regressions they claim to by reverting the fix piece by piece and confirming each test fails for the right reason (the concurrency test fails without the mutex; the reader-consistency test fails against a naive mutex-only design that doesn't also fix the staging buffer). Full
external/avosuite passes; the two Ruby files touched arestandardrb-clean.Known trade-off, left as-is
Wrapping the whole boot body in one mutex means concurrent reloads that previously ran in parallel (racily) now queue behind each other, including whatever work each pro gem's
on_load(:avo_boot)hook does. This is accepted: the prior "parallelism" was never a supported guarantee, and a wedged dev server is worse than added reload latency. Not addressed here, and out of scope: a separate, pre-existing leak whereActiveSupport.run_load_hooks(:avo_boot, ...)grows unboundedly across reloads, causing late-registeredon_load(:avo_boot)blocks to fire once per historical boot.