From 1bec99a2f899f22dee49608c76fe03ec929a58c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Mon, 23 Jun 2025 11:51:30 -0400 Subject: [PATCH] fix(plugins): prevent concurrent WASM compilation race condition (#4253) * fix: eliminate race condition in plugin system Added compilation waiting mechanism to prevent WASM plugins from being instantiated before their background compilation completes. This fixes the intermittent error 'source module must be compiled before instantiation' that occurred when tests or plugin usage happened before asynchronous compilation finished. Changes include: - Added manager reference to wasmBasePlugin for compilation synchronization - Modified all plugin adapter constructors to accept manager parameter - Updated getInstance() to wait for compilation before loading instances - Fixed runtime test to handle manually created plugins appropriately The race condition was caused by plugins trying to compile WASM modules synchronously during Load() calls while background compilation was still in progress. This change ensures proper coordination between the compilation and instantiation phases. * fix: add plugin-clean target to Makefile for easier plugin cleanup Signed-off-by: Deluan * refactor: reorder plugin constructor parameters and add nil safety Moved manager parameter to third position in pluginConstructor signature for\nbetter parameter ordering consistency.\n\nAlso added nil check for adapter creation to prevent registration of failed\nplugin adapters, which could lead to nil-pointer dereferences. Plugin\ncreation failures are now logged with context and gracefully skipped.\n\nChanges:\n- Reordered pluginConstructor parameters: manager moved before runtime\n- Updated all 4 adapter constructor signatures to match new order\n- Added nil safety check in registerPlugin to skip failed adapters\n- Updated runtime test to use new parameter order\n\nThis improves both code consistency and runtime safety by preventing\nnil adapters from being registered in the plugin manager. * fix: prevent concurrent WASM compilation race condition * refactor: remove unnecessary manager parameter from plugin constructors * fix: update parameter name in newWasmSchedulerCallback for consistency Signed-off-by: Deluan --------- Signed-off-by: Deluan --- Makefile | 5 +++++ plugins/adapter_scheduler_callback.go | 6 +++--- plugins/manager.go | 4 ++++ plugins/runtime.go | 17 ++++++++++++++++- plugins/wasm_base_plugin.go | 1 + 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 3935fe8fd..3b52212db 100644 --- a/Makefile +++ b/Makefile @@ -230,6 +230,11 @@ plugin-examples: check_go_env ##@Development Build all example plugins $(MAKE) -C plugins/examples clean all .PHONY: plugin-examples +plugin-clean: check_go_env ##@Development Clean all plugins + $(MAKE) -C plugins/examples clean + $(MAKE) -C plugins/testdata clean +.PHONY: plugin-clean + plugin-tests: check_go_env ##@Development Build all test plugins $(MAKE) -C plugins/testdata clean all .PHONY: plugin-tests diff --git a/plugins/adapter_scheduler_callback.go b/plugins/adapter_scheduler_callback.go index 72cd2aa07..1e1b73c85 100644 --- a/plugins/adapter_scheduler_callback.go +++ b/plugins/adapter_scheduler_callback.go @@ -9,16 +9,16 @@ import ( ) // newWasmSchedulerCallback creates a new adapter for a SchedulerCallback plugin -func newWasmSchedulerCallback(wasmPath, pluginName string, runtime api.WazeroNewRuntime, mc wazero.ModuleConfig) WasmPlugin { +func newWasmSchedulerCallback(wasmPath, pluginID string, runtime api.WazeroNewRuntime, mc wazero.ModuleConfig) WasmPlugin { loader, err := api.NewSchedulerCallbackPlugin(context.Background(), api.WazeroRuntime(runtime), api.WazeroModuleConfig(mc)) if err != nil { - log.Error("Error creating scheduler callback plugin", "plugin", pluginName, "path", wasmPath, err) + log.Error("Error creating scheduler callback plugin", "plugin", pluginID, "path", wasmPath, err) return nil } return &wasmSchedulerCallback{ wasmBasePlugin: &wasmBasePlugin[api.SchedulerCallback, *api.SchedulerCallbackPlugin]{ wasmPath: wasmPath, - id: pluginName, + id: pluginID, capability: CapabilitySchedulerCallback, loader: loader, loadFunc: func(ctx context.Context, l *api.SchedulerCallbackPlugin, path string) (api.SchedulerCallback, error) { diff --git a/plugins/manager.go b/plugins/manager.go index a9976bda2..b8a79fe63 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -161,6 +161,10 @@ func (m *Manager) registerPlugin(pluginID, pluginDir, wasmPath string, manifest continue } adapter := constructor(wasmPath, pluginID, customRuntime, mc) + if adapter == nil { + log.Error("Failed to create plugin adapter", "plugin", pluginID, "capability", capabilityStr, "path", wasmPath) + continue + } m.adapters[pluginID+"_"+capabilityStr] = adapter } diff --git a/plugins/runtime.go b/plugins/runtime.go index 05f8b56ec..a5cc736a4 100644 --- a/plugins/runtime.go +++ b/plugins/runtime.go @@ -520,6 +520,9 @@ type cachingRuntime struct { // poolInitOnce ensures the pool is initialized only once poolInitOnce sync.Once + + // compilationMu ensures only one compilation happens at a time per runtime + compilationMu sync.Mutex } func newCachingRuntime(runtime wazero.Runtime, pluginID string) *cachingRuntime { @@ -580,7 +583,7 @@ func (r *cachingRuntime) setCachedModule(module wazero.CompiledModule, wasmBytes func (r *cachingRuntime) CompileModule(ctx context.Context, wasmBytes []byte) (wazero.CompiledModule, error) { incomingHash := md5.Sum(wasmBytes) - // Try to get from cache + // Try to get from cache first (without lock for performance) if cached := r.cachedModule.Load(); cached != nil { if module := cached.get(incomingHash); module != nil { log.Trace(ctx, "cachingRuntime: using cached compiled module", "plugin", r.pluginID) @@ -588,6 +591,18 @@ func (r *cachingRuntime) CompileModule(ctx context.Context, wasmBytes []byte) (w } } + // Synchronize compilation to prevent concurrent compilation issues + r.compilationMu.Lock() + defer r.compilationMu.Unlock() + + // Double-check cache after acquiring lock (another goroutine might have compiled it) + if cached := r.cachedModule.Load(); cached != nil { + if module := cached.get(incomingHash); module != nil { + log.Trace(ctx, "cachingRuntime: using cached compiled module (after lock)", "plugin", r.pluginID) + return module, nil + } + } + // Fall back to normal compilation for different bytes log.Trace(ctx, "cachingRuntime: hash doesn't match cache, compiling normally", "plugin", r.pluginID) module, err := r.Runtime.CompileModule(ctx, wasmBytes) diff --git a/plugins/wasm_base_plugin.go b/plugins/wasm_base_plugin.go index 4010f3918..9b101aa24 100644 --- a/plugins/wasm_base_plugin.go +++ b/plugins/wasm_base_plugin.go @@ -39,6 +39,7 @@ func (w *wasmBasePlugin[S, P]) getInstance(ctx context.Context, methodName strin start := time.Now() // Add context metadata for tracing ctx = log.NewContext(ctx, "capability", w.serviceName(), "method", methodName) + inst, err := w.loadFunc(ctx, w.loader, w.wasmPath) if err != nil { var zero S