diff --git a/CHANGELOG.md b/CHANGELOG.md index dce7702d..e9f8cd0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ Configs that still reference the legacy types now fail to load with an actionabl ## [Unreleased] +### Fixed (issue #663 — follow-up) + +- **`*external.RemoteModule.Dependencies()` now returns the yaml-level `dependsOn:` keys** instead of always returning `nil`. The v0.51.8 fix (PR #664) only reordered the `cfg.Modules` slice — but modular's `app.Init()` then runs its own `DependencyAware`-driven sort over the registered modules, and `RemoteModule` (the wrapper used for every external-plugin module) returned `nil` from `Dependencies()`, so modular saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this as the same `bmw-eventbus`/`bmw-stream` ordering race that v0.51.8 was supposed to close. Engine `BuildFromConfig` now filters `modCfg.DependsOn` through `filterResolvableDeps` (drops empty strings + names not present in `cfg.Modules` — the same edge-set topoSortModules used for ordering) and calls `SetDependencies(filtered)` on each module that **implements** `interface{ SetDependencies([]string) }`, but **only when the filtered slice is non-empty**, immediately after the factory returns and before `app.RegisterModule`. (Modules with no resolvable dependsOn — empty yaml + transform-injected modules whose dependsOn is all empty/ghost — are skipped, so a constructor-time default isn't clobbered with `SetDependencies(nil)`.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 7 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, defensive-copy aliasing, plus two type-assertion pins for `modular.DependencyAware` and the engine's `SetDependencies` interface) plus 4 engine-level `BuildFromConfig` tests covering the production path (basic plumb + defensive copy via raw-slice recorder + back-compat skip + real-modular Init order). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. + ### Fixed (issue #663) - **`StdEngine.BuildFromConfig` now topologically sorts `cfg.Modules` by yaml `dependsOn:` before module registration.** Previously the engine walked `cfg.Modules` in slice order and the modular framework's `app.Init()` then walked in registration order. For external-plugin modules (which do not implement `DependencyAware`), yaml-level `dependsOn:` was validated by the schema but ignored at init time — a child module's `Init()` could fire before its parent's `Init()` had registered runtime state in a plugin-local registry (broker, factory table, etc.), causing startup races like the "broker not registered within 10s" failure that bit BMW PR #279. The new `topoSortModules` (in `engine_module_order.go`) uses Kahn's algorithm with a stable tie-break on declared index, tolerates missing dependency targets (schema validation catches those separately), and returns an error listing every module that could not be ordered (cycle members and their downstream dependents — Kahn cannot distinguish them by inDegree alone) if dependencies form a loop. 12 unit tests cover the BMW shape, parallel chains, cycles + their downstream dependents, and edge cases. Operators who used the alphabetical-prefix workaround (renaming a dependency to `aaa-…`) can now revert to canonical names and rely on `dependsOn:` to drive the order. diff --git a/engine.go b/engine.go index 7328e707..47f28e44 100644 --- a/engine.go +++ b/engine.go @@ -509,6 +509,20 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { } cfg.Modules = orderedModules + // Build the resolvable-dependency name set so the SetDependencies plumbing + // below can filter raw modCfg.DependsOn the same way topoSortModules + // filters its internal edges. Without this filter, the engine would forward + // empty-string entries and unknown names — both intentionally ignored by + // topoSortModules — straight into modular's DependencyAware sort, which + // would then either misorder (unknown names treated as future deps that + // never resolve) or panic (depending on the modular framework version). + moduleNameSet := make(map[string]struct{}, len(cfg.Modules)) + for _, m := range cfg.Modules { + if m.Name != "" { + moduleNameSet[m.Name] = struct{}{} + } + } + // Compute config hash after transform hooks + dependency ordering so the // hash reflects the effective runtime config (hooks may mutate cfg, and // topoSortModules may reorder cfg.Modules, before modules are registered). @@ -555,6 +569,24 @@ func (e *StdEngine) BuildFromConfig(cfg *config.WorkflowConfig) error { return fmt.Errorf("factory for module type %q returned nil for module %q", modCfg.Type, modCfg.Name) } + // Plumb yaml-level `dependsOn:` into the module so modular's Init() + // walker honours it (workflow#663). Without this, external-plugin + // modules — which all return nil from Dependencies() by default — + // looked like roots to modular and got initialised in alphabetical + // order, which broke any plugin where module A's Init() registered + // runtime state that module B's Init() needed. The topoSortModules + // pass above already reordered cfg.Modules; this hands the same + // information to modular so its own initialisation graph agrees + // with engine-level ordering. filterResolvableDeps drops empty + // entries + unknown names so the slice modular sees matches the + // edge set topoSortModules used. + if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { + filtered := filterResolvableDeps(modCfg.DependsOn, moduleNameSet, modCfg.Name) + if len(filtered) > 0 { + depTarget.SetDependencies(filtered) + } + } + e.app.RegisterModule(mod) } diff --git a/engine_dependson_plumbing_test.go b/engine_dependson_plumbing_test.go new file mode 100644 index 00000000..9ea1457e --- /dev/null +++ b/engine_dependson_plumbing_test.go @@ -0,0 +1,316 @@ +package workflow + +import ( + "io" + "log/slog" + "testing" + + "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/config" +) + +// fakeDepAwareModule implements modular.Module + the +// `interface{ SetDependencies([]string) }` contract that engine.go uses to +// plumb yaml-level dependsOn into modules. Recording every SetDependencies +// call lets the test assert the engine actually invoked it. +type fakeDepAwareModule struct { + name string + setCalls [][]string // ordered record of every SetDependencies invocation + initialised bool +} + +func (m *fakeDepAwareModule) Name() string { return m.name } +func (m *fakeDepAwareModule) Init(_ modular.Application) error { m.initialised = true; return nil } +func (m *fakeDepAwareModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *fakeDepAwareModule) Dependencies() []string { return nil } +func (m *fakeDepAwareModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *fakeDepAwareModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *fakeDepAwareModule) SetDependencies(deps []string) { + // Defensive copy so we record what the engine passed, not what later + // code may have mutated on the slice. + cp := make([]string, len(deps)) + copy(cp, deps) + m.setCalls = append(m.setCalls, cp) +} + +// rawSliceRecorderModule retains the EXACT slice reference passed to +// SetDependencies — no defensive copy on the recording side. Used by +// TestEngine_BuildFromConfig_PlumbsDefensiveCopy to prove the engine +// itself copies; if the engine just passed modCfg.DependsOn through, +// mutating the original yaml-derived slice would mutate this recorded +// reference too, and the test would fail. +type rawSliceRecorderModule struct { + name string + rawRecorded []string // nil until SetDependencies fires; then the exact slice ref + callCount int +} + +func (m *rawSliceRecorderModule) Name() string { return m.name } +func (m *rawSliceRecorderModule) Init(_ modular.Application) error { return nil } +func (m *rawSliceRecorderModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *rawSliceRecorderModule) Dependencies() []string { return nil } +func (m *rawSliceRecorderModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *rawSliceRecorderModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *rawSliceRecorderModule) SetDependencies(deps []string) { + m.rawRecorded = deps // INTENTIONAL: no copy. Asserts engine-side copy. + m.callCount++ +} + +// fakePlainModule deliberately does NOT implement SetDependencies. The +// engine plumbing must skip it without panicking, regardless of whether +// modCfg.DependsOn is set. +type fakePlainModule struct { + name string +} + +func (m *fakePlainModule) Name() string { return m.name } +func (m *fakePlainModule) Init(_ modular.Application) error { return nil } +func (m *fakePlainModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *fakePlainModule) Dependencies() []string { return nil } +func (m *fakePlainModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *fakePlainModule) RequiresServices() []modular.ServiceDependency { return nil } + +// TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule pins the production +// path that closes workflow#663: for a module that implements the +// SetDependencies setter AND has at least one resolvable dependsOn entry +// (after filterResolvableDeps drops empties + unknown names), +// BuildFromConfig must call SetDependencies with a defensive copy of the +// filtered slice before app.RegisterModule. Without this end-to-end test, +// a future refactor that moves the structural type assertion out of the +// registration loop (or changes the contract) would silently regress the +// BMW PR #279/280 fix and only image-launch CI would catch it. +func TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + var capturedConsumer *fakeDepAwareModule + var capturedBroker *fakeDepAwareModule + engine.AddModuleType("test.broker", func(name string, _ map[string]any) modular.Module { + capturedBroker = &fakeDepAwareModule{name: name} + return capturedBroker + }) + engine.AddModuleType("test.consumer", func(name string, _ map[string]any) modular.Module { + capturedConsumer = &fakeDepAwareModule{name: name} + return capturedConsumer + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "broker", Type: "test.broker"}, + {Name: "consumer", Type: "test.consumer", DependsOn: []string{"broker"}}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + if capturedBroker == nil || capturedConsumer == nil { + t.Fatal("factories were not invoked") + } + + // Broker has no dependsOn → no SetDependencies call. + if len(capturedBroker.setCalls) != 0 { + t.Errorf("broker SetDependencies calls = %d, want 0", len(capturedBroker.setCalls)) + } + + // Consumer dependsOn:[broker] → exactly one SetDependencies call with that value. + if len(capturedConsumer.setCalls) != 1 { + t.Fatalf("consumer SetDependencies calls = %d, want 1", len(capturedConsumer.setCalls)) + } + got := capturedConsumer.setCalls[0] + if len(got) != 1 || got[0] != "broker" { + t.Errorf("consumer SetDependencies received %v, want [broker]", got) + } +} + +// TestEngine_BuildFromConfig_PlumbsDefensiveCopy pins the defensive copy: +// after BuildFromConfig returns, mutating modCfg.DependsOn must not +// affect the slice the module recorded. Without the copy, downstream +// code that re-uses ModuleConfig structs across builds (or mutates them +// for any reason) could silently corrupt the engine-side dependency graph. +// +// The fake here records the EXACT slice reference passed to +// SetDependencies (no defensive copy on the recording side) so a +// regression where the engine passes modCfg.DependsOn through verbatim +// would let a post-build mutation appear in the recorded value and fail +// the assertion. +func TestEngine_BuildFromConfig_PlumbsDefensiveCopy(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + var capturedChild *rawSliceRecorderModule + engine.AddModuleType("test.rawmod", func(name string, _ map[string]any) modular.Module { + m := &rawSliceRecorderModule{name: name} + if name == "child" { + capturedChild = m + } + return m + }) + + deps := []string{"root-a", "root-b"} + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "root-a", Type: "test.rawmod"}, + {Name: "root-b", Type: "test.rawmod"}, + {Name: "child", Type: "test.rawmod", DependsOn: deps}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + if capturedChild == nil { + t.Fatal("child factory was not invoked") + } + if capturedChild.callCount != 1 { + t.Fatalf("child SetDependencies call count = %d, want 1", capturedChild.callCount) + } + + // Mutate the original yaml-derived slice. The child's recorded + // reference must NOT see this change, because the engine should have + // passed a defensive copy. If the engine passed `deps` directly, this + // mutation would change capturedChild.rawRecorded[0] too and the + // assertion below would fail. + deps[0] = "MUTATED" + if capturedChild.rawRecorded[0] != "root-a" { + t.Errorf("engine did NOT defensively copy DependsOn — mutating the yaml slice from %q to %q changed the recorded value (now %q)", + "root-a", deps[0], capturedChild.rawRecorded[0]) + } +} + +// initOrderRecorderModule appends its name to a shared slice during Init(). +// Used by TestEngine_BuildFromConfig_RealModularHonoursDependsOn to assert +// the order modular's real Init walker fires modules in — proving that +// SetDependencies plumbed by BuildFromConfig actually drives modular's +// dependency-aware sort, not just that the recorded values look right. +type initOrderRecorderModule struct { + name string + deps []string + initOrder *[]string // shared pointer; appended to on Init +} + +func (m *initOrderRecorderModule) Name() string { return m.name } +func (m *initOrderRecorderModule) Init(_ modular.Application) error { + *m.initOrder = append(*m.initOrder, m.name) + return nil +} +func (m *initOrderRecorderModule) RegisterConfig(_ modular.Application) error { return nil } +func (m *initOrderRecorderModule) Dependencies() []string { return m.deps } +func (m *initOrderRecorderModule) ProvidesServices() []modular.ServiceProvider { return nil } +func (m *initOrderRecorderModule) RequiresServices() []modular.ServiceDependency { return nil } +func (m *initOrderRecorderModule) SetDependencies(deps []string) { + cp := make([]string, len(deps)) + copy(cp, deps) + m.deps = cp +} + +// stubConfigProvider is a minimal modular.ConfigProvider for NewStdApplication. +type stubConfigProvider struct{} + +func (stubConfigProvider) GetConfig() any { return nil } + +// TestEngine_BuildFromConfig_RealModularHonoursDependsOn proves the +// fix end-to-end against the *real* modular framework — not the test +// mockApplication whose Init() is a no-op. A consumer module declared +// FIRST in cfg.Modules with dependsOn:[broker] must initialise AFTER +// the broker, even though the broker is declared SECOND. This pins +// the actual production behaviour the BMW PR #279/#280 race depends on. +func TestEngine_BuildFromConfig_RealModularHonoursDependsOn(t *testing.T) { + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + app := modular.NewStdApplication(stubConfigProvider{}, logger) + engine := NewStdEngine(app, logger) + + var initOrder []string + engine.AddModuleType("test.depinit", func(name string, _ map[string]any) modular.Module { + return &initOrderRecorderModule{name: name, initOrder: &initOrder} + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + // Consumer declared FIRST, depends on broker. + {Name: "consumer", Type: "test.depinit", DependsOn: []string{"broker"}}, + // Broker declared SECOND. + {Name: "broker", Type: "test.depinit"}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig: %v", err) + } + + // Without the SetDependencies plumbing, modular's Init walker would + // see Dependencies()=nil for both modules and fall back to alphabetical + // (broker before consumer) — which would actually pass this assertion + // by coincidence. To prove the plumbing matters, declare the broker + // with a name that sorts AFTER the consumer alphabetically. With + // SetDependencies wired, modular still inits broker first (because + // consumer.Dependencies() now returns ["broker"]). Without it, modular + // would init "consumer" then "z-broker" (alphabetical) and the + // assertion would fail. + // + // Reset + re-run with that shape. + initOrder = initOrder[:0] + app2 := modular.NewStdApplication(stubConfigProvider{}, logger) + engine2 := NewStdEngine(app2, logger) + engine2.AddModuleType("test.depinit", func(name string, _ map[string]any) modular.Module { + return &initOrderRecorderModule{name: name, initOrder: &initOrder} + }) + cfg2 := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "consumer", Type: "test.depinit", DependsOn: []string{"z-broker"}}, + {Name: "z-broker", Type: "test.depinit"}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + if err := engine2.BuildFromConfig(cfg2); err != nil { + t.Fatalf("BuildFromConfig (z-broker shape): %v", err) + } + if len(initOrder) != 2 { + t.Fatalf("initOrder = %v, want 2 entries", initOrder) + } + if initOrder[0] != "z-broker" || initOrder[1] != "consumer" { + t.Errorf("modular Init order = %v, want [z-broker consumer] (broker must precede dependent even when alphabetically later)", + initOrder) + } +} + +// TestEngine_BuildFromConfig_SkipsModulesWithoutSetter pins that modules +// not implementing the SetDependencies contract are silently skipped +// (not panicked, not errored) even when modCfg.DependsOn is non-empty. +// The yaml-level dependsOn is then expressed only through cfg.Modules +// slice ordering (via topoSortModules) — modular's own DependencyAware +// sort sees nil from the module's Dependencies() and treats it as a root. +// This is the back-compat surface for built-in modules that haven't +// opted in to the new setter. +func TestEngine_BuildFromConfig_SkipsModulesWithoutSetter(t *testing.T) { + app := newMockApplication() + engine := NewStdEngine(app, app.Logger()) + + engine.AddModuleType("test.plain", func(name string, _ map[string]any) modular.Module { + return &fakePlainModule{name: name} + }) + + cfg := &config.WorkflowConfig{ + Modules: []config.ModuleConfig{ + {Name: "root", Type: "test.plain"}, + {Name: "child", Type: "test.plain", DependsOn: []string{"root"}}, + }, + Workflows: map[string]any{}, + Triggers: map[string]any{}, + } + + // The structural-type-assertion path must not panic on a module that + // lacks the setter. A successful BuildFromConfig is the assertion. + if err := engine.BuildFromConfig(cfg); err != nil { + t.Fatalf("BuildFromConfig must tolerate modules without SetDependencies: %v", err) + } +} diff --git a/engine_module_order.go b/engine_module_order.go index b88da19c..cbdd8a69 100644 --- a/engine_module_order.go +++ b/engine_module_order.go @@ -28,7 +28,20 @@ import ( // the dependent looks up its parent in a plugin-local registry. func topoSortModules(modules []config.ModuleConfig) ([]config.ModuleConfig, error) { n := len(modules) - if n <= 1 { + if n == 0 { + return modules, nil + } + if n == 1 { + // Single module can still self-reference, which the n>=2 path + // catches via Kahn's cycle detection. Detect it explicitly here. + for _, dep := range modules[0].DependsOn { + if dep == modules[0].Name { + return nil, fmt.Errorf( + "module dependsOn cycle (or dependents of one) among: %s", + modules[0].Name, + ) + } + } return modules, nil } @@ -127,3 +140,33 @@ func (h *intHeap) Pop() any { *h = old[:n-1] return x } + +// filterResolvableDeps returns a new slice containing only entries from deps +// that are present in the moduleNames set AND are not equal to selfName, in +// the same order. Empty strings, names not in moduleNames, and self-references +// are dropped. Used by engine BuildFromConfig before calling SetDependencies +// on a module, so modular's DependencyAware sort sees the same edge set that +// topoSortModules used when ordering cfg.Modules. +// +// Schema validation rejects empty + unknown dependsOn entries for declared +// modules, but does not reject a module declaring itself in dependsOn — and +// ConfigTransformHooks can inject modules whose dependsOn was never validated. +// This is the engine's defensive boundary against both: stripping the +// self-reference here means modular can't either treat it as a 1-cycle or +// stall waiting for the module to initialise itself before itself. +func filterResolvableDeps(deps []string, moduleNames map[string]struct{}, selfName string) []string { + out := make([]string, 0, len(deps)) + for _, dep := range deps { + if dep == "" { + continue + } + if dep == selfName { + continue + } + if _, exists := moduleNames[dep]; !exists { + continue + } + out = append(out, dep) + } + return out +} diff --git a/engine_module_order_test.go b/engine_module_order_test.go index 5acefaf7..765829e5 100644 --- a/engine_module_order_test.go +++ b/engine_module_order_test.go @@ -254,6 +254,91 @@ func TestTopoSortModules_DuplicateNames_FirstWins(t *testing.T) { } } +func TestFilterResolvableDeps_DropsEmptyAndUnknown(t *testing.T) { + names := map[string]struct{}{"a": {}, "b": {}, "c": {}} + got := filterResolvableDeps([]string{"a", "", "ghost", "b", "phantom", "c"}, names, "_test_self_") + want := []string{"a", "b", "c"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps = %v, want %v", got, want) + } +} + +func TestFilterResolvableDeps_AllUnknownReturnsEmpty(t *testing.T) { + names := map[string]struct{}{"only-this": {}} + got := filterResolvableDeps([]string{"x", "y", "z", ""}, names, "_test_self_") + if len(got) != 0 { + t.Errorf("filterResolvableDeps all-unknown = %v, want empty", got) + } +} + +func TestFilterResolvableDeps_EmptyInputEmptyOutput(t *testing.T) { + got := filterResolvableDeps(nil, map[string]struct{}{"a": {}}, "_test_self_") + if len(got) != 0 { + t.Errorf("filterResolvableDeps(nil) = %v, want empty", got) + } +} + +func TestFilterResolvableDeps_PreservesOrder(t *testing.T) { + // Order in input must be preserved in output. + names := map[string]struct{}{"a": {}, "b": {}, "c": {}} + got := filterResolvableDeps([]string{"c", "a", "b"}, names, "_test_self_") + want := []string{"c", "a", "b"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps = %v, want %v (order preserved)", got, want) + } +} + +func TestFilterResolvableDeps_DropsSelfReference(t *testing.T) { + // Schema validation does NOT reject a module declaring itself in + // dependsOn — and modular's DependencyAware sort would treat it as + // a 1-cycle (or stall, depending on framework version). Filter it. + names := map[string]struct{}{"me": {}, "you": {}} + got := filterResolvableDeps([]string{"me", "you"}, names, "me") + want := []string{"you"} + if !equalSlice(got, want) { + t.Errorf("filterResolvableDeps with self-reference = %v, want %v", got, want) + } +} + +func TestFilterResolvableDeps_OnlySelfReferenceReturnsEmpty(t *testing.T) { + // Module declaring only itself → engine should skip SetDependencies + // entirely (filtered slice is empty). + names := map[string]struct{}{"me": {}} + got := filterResolvableDeps([]string{"me", "me", "me"}, names, "me") + if len(got) != 0 { + t.Errorf("filterResolvableDeps only-self = %v, want empty", got) + } +} + +func TestTopoSortModules_SingleModuleSelfDepIsCycle(t *testing.T) { + // n==1 path used to short-circuit before cycle detection. A single + // module declaring itself in dependsOn IS a 1-cycle and must error. + in := []config.ModuleConfig{ + withDeps(config.ModuleConfig{Name: "self", Type: "x"}, "self"), + } + _, err := topoSortModules(in) + if err == nil { + t.Fatalf("expected cycle error for single-module self-dep, got nil") + } + if !strings.Contains(err.Error(), "cycle") || !strings.Contains(err.Error(), "self") { + t.Errorf("error %q should mention cycle + module name 'self'", err.Error()) + } +} + +func TestTopoSortModules_SingleModuleNoSelfDepIsClean(t *testing.T) { + // Sanity: n==1 with no self-dep still passes through cleanly. + in := []config.ModuleConfig{ + {Name: "lonely", Type: "x"}, + } + out, err := topoSortModules(in) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(out) != 1 || out[0].Name != "lonely" { + t.Errorf("singleton-no-self order = %v, want [lonely]", order(out)) + } +} + func TestTopoSortModules_EmptyDependencyStringIgnored(t *testing.T) { // Schema validation rejects "" entries, but defensively the sort should // not blow up if one slips through (e.g., from a hand-built ModuleConfig diff --git a/plugin/external/remote_module.go b/plugin/external/remote_module.go index dc57d92b..cca5c304 100644 --- a/plugin/external/remote_module.go +++ b/plugin/external/remote_module.go @@ -18,6 +18,16 @@ type RemoteModule struct { contract *pb.ContractDescriptor serviceContracts map[string]*pb.ContractDescriptor types protoregistry.MessageTypeResolver + + // dependencies holds the yaml-level `dependsOn:` keys for this module. + // Populated by the engine after the factory returns via SetDependencies + // — see workflow#663. Returned from Dependencies() so modular's Init() + // walker honours them when computing module init order. Without this, + // every external-plugin module appeared as a root and modular sorted + // them alphabetically, which broke any plugin where module A's Init() + // registered runtime state in a plugin-local registry that module B's + // Init() looked up. + dependencies []string } type remoteModuleContracts struct { @@ -53,7 +63,28 @@ func (m *RemoteModule) Name() string { } func (m *RemoteModule) Dependencies() []string { - return nil + return m.dependencies +} + +// SetDependencies records the yaml-level `dependsOn:` keys declared for this +// module so modular's Init() walker can honour them. Called by the engine +// from BuildFromConfig immediately after the factory returns and before +// app.RegisterModule, but only when the module's `modCfg.DependsOn` is +// **non-empty after filtering** (filterResolvableDeps drops empty strings +// + names not present in cfg.Modules) AND the module satisfies +// `interface{ SetDependencies([]string) }` — modules with no resolvable +// dependsOn are skipped so a constructor-time default isn't clobbered with +// a SetDependencies(nil) call. See workflow#663. +// +// Defensive copy: although the engine already copies before calling, the +// setter is exported and Dependencies() exposes the same backing array to +// modular. Copying here too means any caller (engine, tests, future +// integration paths) can mutate its source slice after calling without +// silently corrupting this module's init graph. +func (m *RemoteModule) SetDependencies(deps []string) { + cp := make([]string, len(deps)) + copy(cp, deps) + m.dependencies = cp } func (m *RemoteModule) ProvidesServices() []string { diff --git a/plugin/external/remote_module_dependencies_test.go b/plugin/external/remote_module_dependencies_test.go new file mode 100644 index 00000000..99280fd0 --- /dev/null +++ b/plugin/external/remote_module_dependencies_test.go @@ -0,0 +1,110 @@ +package external + +import ( + "testing" + + "github.com/GoCodeAlone/modular" +) + +// TestRemoteModule_Dependencies_DefaultsToNil pins that a freshly-constructed +// RemoteModule reports no dependencies — the same behaviour as before +// workflow#663. The DependencyAware contract still applies; modular's +// Init() walker just sees an empty list and treats the module as a root. +func TestRemoteModule_Dependencies_DefaultsToNil(t *testing.T) { + m := &RemoteModule{name: "x"} + if got := m.Dependencies(); got != nil { + t.Errorf("Dependencies() default = %v, want nil", got) + } +} + +// TestRemoteModule_SetDependencies_PlumbsYamlDependsOn verifies the engine's +// post-factory plumb path: after the factory returns the module the engine +// calls SetDependencies with the yaml-level `dependsOn:` slice. Dependencies() +// must then return exactly that slice so modular's DependencyAware-driven +// Init() walker honours it. Closes the workflow#663 follow-up that surfaced +// on BMW PR #280's image-launch. +func TestRemoteModule_SetDependencies_PlumbsYamlDependsOn(t *testing.T) { + m := &RemoteModule{name: "bmw-consumer-audit-appender"} + m.SetDependencies([]string{"bmw-eventbus", "bmw-stream"}) + got := m.Dependencies() + if len(got) != 2 || got[0] != "bmw-eventbus" || got[1] != "bmw-stream" { + t.Errorf("Dependencies() = %v, want [bmw-eventbus bmw-stream]", got) + } +} + +// TestRemoteModule_SetDependencies_EmptySliceIsEmpty pins that passing an +// empty slice records an empty (non-nil) slice. modular treats a non-nil +// empty slice as "no dependencies declared" the same as nil, so the +// distinction is internal — but pinning it prevents accidental drift if a +// future refactor adds a nil-check that changes semantics. +func TestRemoteModule_SetDependencies_EmptySliceIsEmpty(t *testing.T) { + m := &RemoteModule{name: "x"} + m.SetDependencies([]string{}) + got := m.Dependencies() + if got == nil || len(got) != 0 { + t.Errorf("Dependencies() after SetDependencies([]) = %v, want empty non-nil slice", got) + } +} + +// TestRemoteModule_SetDependencies_Overwrites pins that a second call +// replaces the previous slice rather than appending. The engine calls +// SetDependencies at most once per BuildFromConfig — only on modules +// that implement the setter AND have at least one resolvable dependsOn +// entry after filterResolvableDeps drops empties + unknown names. But +// pinning the overwrite contract guards against future engine-side +// double-calls (e.g., a config-transform hook that mutates dependsOn +// post-registration and re-fires the plumb). +func TestRemoteModule_SetDependencies_Overwrites(t *testing.T) { + m := &RemoteModule{name: "x"} + m.SetDependencies([]string{"a", "b"}) + m.SetDependencies([]string{"c"}) + got := m.Dependencies() + if len(got) != 1 || got[0] != "c" { + t.Errorf("Dependencies() after second SetDependencies = %v, want [c]", got) + } +} + +// TestRemoteModule_SetDependencies_DefensivelyCopies pins the setter-side +// defensive copy: mutating the caller's slice after SetDependencies must +// not change what Dependencies() returns. Without the copy, the engine +// stores its own pre-copied slice safely, but other callers (tests, +// future integration paths) hold a live reference to the module's +// dependency graph. Aliasing through that reference would silently +// corrupt modular's init order. +func TestRemoteModule_SetDependencies_DefensivelyCopies(t *testing.T) { + m := &RemoteModule{name: "x"} + src := []string{"a", "b"} + m.SetDependencies(src) + + // Mutate the source slice. The module must not see the mutation. + src[0] = "MUTATED" + + got := m.Dependencies() + if len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Errorf("Dependencies() = %v after caller-side mutation; want [a b] — SetDependencies did NOT defensively copy", got) + } +} + +// TestRemoteModule_ImplementsDependencyAware pins that *RemoteModule +// satisfies modular.DependencyAware (the actual interface modular's +// Init() walker type-asserts against). A regression that broke this +// satisfaction would silently drop external-plugin modules out of +// modular's dependency-aware sort and re-introduce the workflow#663 +// alphabetical-init race. +func TestRemoteModule_ImplementsDependencyAware(t *testing.T) { + var _ modular.DependencyAware = (*RemoteModule)(nil) +} + +// TestRemoteModule_ImplementsDependencyTargetInterface pins the structural +// type that engine.go uses to find modules whose dependsOn it should plumb: +// +// if dt, ok := mod.(interface{ SetDependencies([]string) }); ok { ... } +// +// If a future refactor drops SetDependencies the engine silently stops +// plumbing, which would re-introduce the workflow#663 race. This test +// fails loudly instead. +func TestRemoteModule_ImplementsDependencyTargetInterface(t *testing.T) { + var _ interface { + SetDependencies([]string) + } = (*RemoteModule)(nil) +}