fix(engine,#663): plumb yaml dependsOn into RemoteModule.Dependencies (PR #664 follow-up)#665
Merged
Conversation
Follow-up to PR #664 (v0.51.8). The v0.51.8 fix reordered cfg.Modules by yaml dependsOn before app.RegisterModule, 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 still saw every external-plugin module as a root and sorted alphabetically. BMW PR #280 image-launch surfaced this on 53283b2: with the aaa-/aab- prefix workaround removed and v0.51.8 in place, the engine still initialised the 6 bmw-consumer-* modules before bmw-eventbus and bmw-stream, hit the broker-not-registered 10s race, and never served /healthz. The fix has two parts: 1. plugin/external/remote_module.go: add a dependencies field on RemoteModule + a SetDependencies setter. Dependencies() now returns the stored slice instead of always returning nil. 2. engine.go: after the factory returns the module, call SetDependencies(modCfg.DependsOn) via a structural type assertion: if dt, ok := mod.(interface{ SetDependencies([]string) }); ok { dt.SetDependencies(deps) } Built-in modules opt in by implementing the same setter; modules that don't implement it see no behaviour change. The yaml slice is defensively copied before storing. 6 unit tests in remote_module_dependencies_test.go cover the contract: default-nil, plumb-from-yaml, empty-slice tolerance, overwrite-replaces, and two structural type-assertion pins so a future refactor that drops SetDependencies fails loudly instead of silently re-introducing the workflow#663 race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the #663 init-order fix by ensuring external-plugin modules expose YAML dependsOn declarations through RemoteModule.Dependencies(), so modular’s own dependency-aware initialization sort matches the engine’s config ordering.
Changes:
- Adds dependency storage and
SetDependenciessupport toRemoteModule. - Plumbs
modCfg.DependsOnfromStdEngine.BuildFromConfiginto modules that support the setter. - Adds RemoteModule dependency contract tests and an Unreleased changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
engine.go |
Copies YAML module dependencies into modules that implement SetDependencies. |
plugin/external/remote_module.go |
Stores and returns RemoteModule dependencies for modular init ordering. |
plugin/external/remote_module_dependencies_test.go |
Adds unit tests for RemoteModule dependency behavior and structural interfaces. |
CHANGELOG.md |
Documents the follow-up fix for external-plugin module init ordering. |
Comment on lines
+568
to
+571
| if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { | ||
| deps := make([]string, len(modCfg.DependsOn)) | ||
| copy(deps, modCfg.DependsOn) | ||
| depTarget.SetDependencies(deps) |
Contributor
Author
There was a problem hiding this comment.
Addressed in f0a6599: added 3 BuildFromConfig-level tests covering the new engine plumbing path. TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule pins the production wiring (broker + consumer config drives exactly one SetDependencies call on the consumer with the broker name); TestEngine_BuildFromConfig_PlumbsDefensiveCopy proves the engine copies the yaml slice before passing through; TestEngine_BuildFromConfig_SkipsModulesWithoutSetter pins back-compat for modules that don't opt in. All 3 use a custom ModuleFactory registered via AddModuleType + a fakeDepAwareModule that records every SetDependencies call.
Copilot R1 on PR #665 flagged: the new engine.go plumbing path that calls SetDependencies via structural type assertion is not covered by an end-to-end test. The added RemoteModule unit tests prove the storage works in isolation, but a future refactor that moves or removes the assertion in engine.go would silently regress the workflow#663 fix and only image-launch CI would catch it. This commit adds 3 BuildFromConfig-level tests using a custom ModuleFactory that registers a fake module implementing the SetDependencies setter: - TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule: a 2-module config (broker + consumer where consumer DependsOn:[broker]) must drive exactly one SetDependencies call on the consumer with the value [broker], and zero calls on the broker. - TestEngine_BuildFromConfig_PlumbsDefensiveCopy: mutates the yaml-derived dependsOn slice after BuildFromConfig returns and verifies the recorded value didn't change. Proves the engine copies the slice before passing it through. - TestEngine_BuildFromConfig_SkipsModulesWithoutSetter: registers a module whose type does NOT implement SetDependencies and confirms BuildFromConfig succeeds (doesn't panic, doesn't error). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Comment on lines
+27
to
+31
| // 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) |
Contributor
Author
There was a problem hiding this comment.
Addressed in 925fafe: split off rawSliceRecorderModule (separate from fakeDepAwareModule) that retains the EXACT slice reference SetDependencies receives — no defensive copy on the recording side. TestEngine_BuildFromConfig_PlumbsDefensiveCopy now uses that fake. Sanity-checked: temporarily replaced the engine's depTarget.SetDependencies(deps) with depTarget.SetDependencies(modCfg.DependsOn) and the test correctly FAILS with 'engine did NOT defensively copy'. Restoring the canonical engine code makes it pass again.
Copilot R2 caught it: the original fakeDepAwareModule.SetDependencies defensively copied the slice on the recording side, which masked whether the engine itself copies — even if BuildFromConfig passed modCfg.DependsOn through verbatim, mutating the yaml slice afterward would not change the recorded value (the fake had its own copy). This commit splits the recorder into a second fake type (rawSliceRecorderModule) that retains the EXACT slice reference passed to SetDependencies. TestEngine_BuildFromConfig_PlumbsDefensiveCopy now uses that fake and proves the engine itself copies: mutating the yaml-derived `deps` slice after BuildFromConfig must not affect the child's recorded reference. Sanity-checked locally by temporarily replacing the engine's `depTarget.SetDependencies(deps)` with `depTarget.SetDependencies(modCfg.DependsOn)` — the test correctly FAILS with "engine did NOT defensively copy". After restoring the engine the test passes again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+73
to
+74
| func (m *RemoteModule) SetDependencies(deps []string) { | ||
| m.dependencies = deps |
Contributor
Author
There was a problem hiding this comment.
Addressed in 04ac269: SetDependencies now defensively copies the slice. The engine path already copies before calling, but copying inside the setter too means external callers (tests, future integration paths) can mutate their source slice without silently corrupting this module's init graph.
|
|
||
| ### 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 calls `SetDependencies(modCfg.DependsOn)` on each module that implements `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. `RemoteModule` implements that setter and stores the slice on the struct; modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins). Built-in modules can opt in by implementing the same setter; existing behaviour is unchanged for modules that don't. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 04ac269: CHANGELOG now states 'each module that declares dependsOn: in yaml AND implements the setter' and explains why the len(modCfg.DependsOn) > 0 guard exists (to avoid clobbering constructor-time defaults with SetDependencies(nil)).
Comment on lines
+79
to
+80
| app := newMockApplication() | ||
| engine := NewStdEngine(app, app.Logger()) |
Contributor
Author
There was a problem hiding this comment.
Addressed in 04ac269: added TestEngine_BuildFromConfig_RealModularHonoursDependsOn which uses modular.NewStdApplication (real framework, not the no-op mockApplication.Init). The test names the broker 'z-broker' — alphabetically AFTER its dependent 'consumer' — so the only way the assertion passes is if SetDependencies plumbs into modular's actual DependencyAware sort. A regression where deps are recorded but modular still misorders would fail with order=[consumer z-broker] instead of [z-broker consumer].
…NGELOG Addresses Copilot R3 on PR #665: 1. **plugin/external/remote_module.go**: SetDependencies now defensively copies the slice. The engine path already copies before calling, but the setter is exported and Dependencies() exposes the same backing array to modular — copying inside the setter too means any external caller (tests, future integration paths) can mutate its source slice without silently corrupting this module's init graph. 2. **CHANGELOG.md**: clarified that BuildFromConfig calls SetDependencies only when modCfg.DependsOn is non-empty (the production code's `len(modCfg.DependsOn) > 0` guard exists so calling SetDependencies(nil) doesn't clobber a custom module's constructor-time defaults). Also bumped the test count from 6 to 10 to reflect the engine-level coverage added in #f0a6599b + #925fafe8 + this commit. 3. **engine_dependson_plumbing_test.go**: added TestEngine_BuildFromConfig_RealModularHonoursDependsOn which uses modular.NewStdApplication (NOT the test-package mockApplication whose Init() is a no-op). The test names the broker module "z-broker" — alphabetically AFTER its dependent "consumer" — so the only way the assertion passes is if SetDependencies actually plumbs into modular's DependencyAware-driven Init walker. A regression where SetDependencies is called but modular still misorders would FAIL with order=[consumer z-broker] instead of the asserted [z-broker consumer]. Adds 2 helpers (initOrderRecorderModule + stubConfigProvider) so this remains self-contained inside the workflow package. Sanity checked locally that the new test passes against the real modular framework + against the canonical engine code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+79
to
+82
| func (m *RemoteModule) SetDependencies(deps []string) { | ||
| cp := make([]string, len(deps)) | ||
| copy(cp, deps) | ||
| m.dependencies = cp |
Contributor
Author
There was a problem hiding this comment.
Addressed in fb9d4dd: added TestRemoteModule_SetDependencies_DefensivelyCopies — mutates the caller's source slice after SetDependencies and asserts Dependencies() still returns the original values. Sanity-checked: temporarily removing the cp := make()/copy block in remote_module.go made the test FAIL with 'Dependencies() = [MUTATED b]'. Restoring made it pass.
Comment on lines
+63
to
+66
| // satisfies a structurally-typed interface matching modular.DependencyAware. | ||
| // We can't import modular's interface here without a circular dep, but | ||
| // the structural check is sufficient — modular itself does the same | ||
| // type assertion at init time. |
Contributor
Author
There was a problem hiding this comment.
Addressed in fb9d4dd: TestRemoteModule_ImplementsDependencyAware now asserts *RemoteModule satisfies modular.DependencyAware directly. The original 'circular dep' rationale was misleading — plugin/external already imports modular for modular.Module + modular.Application.
Addresses Copilot R4 on PR #665: 1. Added TestRemoteModule_SetDependencies_DefensivelyCopies — mutates the caller's source slice after SetDependencies and asserts Dependencies() still returns the original values. Sanity-checked locally: temporarily removed the cp := make()/copy block in remote_module.go and the test correctly FAILS with "Dependencies() = [MUTATED b]". Restoring the copy makes it pass. This closes the regression window where the engine's pre-copy masks a missing setter-side copy from R3's review. 2. TestRemoteModule_ImplementsDependencyAware now asserts *RemoteModule satisfies modular.DependencyAware directly, not a structural-typed interface. plugin/external already imports modular (remote_module.go imports it for modular.Module + modular.Application), so the original "circular dep" rationale was misleading — Copilot caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
||
| ### 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 calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `RemoteModule` implements that setter, defensively copies the slice, and modular's Init() walker then reads it via the existing `Dependencies()` contract. 6 unit tests cover the `RemoteModule` contract (default-nil, plumb, empty-slice, overwrite, two structural type-assertion pins) 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. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 448463c: CHANGELOG count bumped 6 → 7 and descriptor updated to mention the new defensive-copy aliasing test + clarify that one of the type-assertion pins is now against the real modular.DependencyAware (not a structural type).
Copilot R5 caught: CHANGELOG said 6 RemoteModule unit tests but the file now contains 7 after R4 added TestRemoteModule_SetDependencies_DefensivelyCopies. Updated the count + the descriptor to mention the new aliasing test and reflect that one of the type-assertion pins is now against the real modular.DependencyAware interface rather than a structural one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+70
to
+72
| // module so modular's Init() walker can honour them. Called by the engine | ||
| // from BuildFromConfig once per module, immediately after the factory | ||
| // returns and before app.RegisterModule. See workflow#663. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 212ffa0: docstring now spells out both conditions (non-empty modCfg.DependsOn AND setter satisfied) and the rationale (constructor-time defaults must not be clobbered by SetDependencies(nil)).
Copilot R6 caught: the SetDependencies docstring said the engine calls it once per module, but BuildFromConfig only calls it when modCfg.DependsOn is non-empty AND the module satisfies the setter interface. Updated the docstring to spell out both conditions and the rationale (constructor-time defaults must not be clobbered by SetDependencies(nil)). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+569
to
+571
| deps := make([]string, len(modCfg.DependsOn)) | ||
| copy(deps, modCfg.DependsOn) | ||
| depTarget.SetDependencies(deps) |
Contributor
Author
There was a problem hiding this comment.
Addressed in a096a67: extracted filterResolvableDeps helper into engine_module_order.go and call it from engine.go before each SetDependencies. The helper drops empty strings + names not in cfg.Modules, preserving input order. If everything is filtered out, SetDependencies is skipped entirely (same back-compat as a module declared with no dependsOn). 4 helper-level unit tests cover drops-empty/all-unknown/empty-input/order-preserved. Going through BuildFromConfig end-to-end isn't possible without registering a config transform hook (schema rejects the bad inputs upstream).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Copilot R7 caught: the engine forwarded the raw modCfg.DependsOn slice into SetDependencies even though topoSortModules intentionally ignored empty strings and dependency names not present in cfg.Modules. modular's DependencyAware sort would then see unresolvable entries — either mis-ordering modules or panicking depending on modular version. Schema validation rejects empty + unknown deps for declared modules, but ConfigTransformHooks can inject modules whose dependsOn was never schema-checked — this is the engine's defensive boundary against that. Two-part fix: 1. engine_module_order.go: new filterResolvableDeps helper that drops empty strings + names not in moduleNames, preserving input order. Same filter as topoSortModules' internal edge construction. 2. engine.go: BuildFromConfig builds a moduleNameSet once after topoSortModules + uses filterResolvableDeps before each SetDependencies call. If the filtered slice is empty (everything was unresolvable or empty), the setter is skipped entirely — same back-compat surface as a module declared with no dependsOn. 4 unit tests covering filterResolvableDeps directly (drops-empty, all-unknown, empty-input, order-preserved). Going through BuildFromConfig isn't possible without registering a config transform hook (schema rejects the bad inputs upstream); the helper-level tests are the practical surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
||
| ### 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 calls `SetDependencies(modCfg.DependsOn)` on each module that **declares `dependsOn:` in yaml AND implements** `interface{ SetDependencies([]string) }` immediately after the factory returns, before `app.RegisterModule`. (Modules with empty `dependsOn:` are skipped — calling `SetDependencies(nil)` would clear any defaults a custom module set up in its constructor.) `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. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 78d7718: comment now reflects the post-R7 filter behaviour (calls SetDependencies only when filterResolvableDeps leaves at least one resolvable name).
Comment on lines
+72
to
+75
| // app.RegisterModule, but only when the module's `modCfg.DependsOn` is | ||
| // non-empty AND the module satisfies `interface{ SetDependencies([]string) }` | ||
| // — modules with no declared dependsOn are skipped so a constructor-time | ||
| // default isn't clobbered with a SetDependencies(nil) call. See workflow#663. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 78d7718: comment now reflects the post-R7 filter behaviour (calls SetDependencies only when filterResolvableDeps leaves at least one resolvable name).
Comment on lines
+50
to
+53
| // replaces the previous slice rather than appending. The engine calls | ||
| // SetDependencies exactly once per module per BuildFromConfig, but | ||
| // pinning the contract guards against future engine-side double-calls | ||
| // (e.g., a config-transform hook that mutates dependsOn post-registration). |
Contributor
Author
There was a problem hiding this comment.
Addressed in 78d7718: comment now reflects the post-R7 filter behaviour (calls SetDependencies only when filterResolvableDeps leaves at least one resolvable name).
Comment on lines
+75
to
+79
| // on each module that implements it, with a defensive copy of | ||
| // modCfg.DependsOn, 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. |
Contributor
Author
There was a problem hiding this comment.
Addressed in 78d7718: comment now reflects the post-R7 filter behaviour (calls SetDependencies only when filterResolvableDeps leaves at least one resolvable name).
Copilot R8 caught 4 wording-drift sites where comments still described
the pre-R7 plumb behaviour ("calls SetDependencies on each module that
implements it") instead of the post-R7 reality ("calls only when
filterResolvableDeps leaves at least one resolvable name"):
- CHANGELOG.md: spell out filterResolvableDeps role + the
non-empty-after-filter precondition.
- plugin/external/remote_module.go: SetDependencies docstring now
mentions filterResolvableDeps + the no-resolvable-dep skip path.
- plugin/external/remote_module_dependencies_test.go:
TestRemoteModule_SetDependencies_Overwrites comment says "at most
once per BuildFromConfig" + spells out the filter precondition.
- engine_dependson_plumbing_test.go:
TestEngine_BuildFromConfig_PlumbsDependsOnIntoModule comment narrows
to the setter-implementing + has-resolvable-dep case.
No code changes — only doc accuracy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+582
to
+587
| // edge set topoSortModules used. | ||
| if depTarget, ok := mod.(interface{ SetDependencies([]string) }); ok { | ||
| filtered := filterResolvableDeps(modCfg.DependsOn, moduleNameSet) | ||
| if len(filtered) > 0 { | ||
| depTarget.SetDependencies(filtered) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Addressed in a55b13b: filterResolvableDeps now takes a selfName arg and drops self-references. topoSortModules' n==1 path explicitly checks for a self-dep and errors with the same cycle message format. 3 new tests cover the self-dep paths (drops-self-reference, only-self-returns-empty, single-module-self-dep-is-cycle); existing filter tests updated for the new signature.
…ycle
Copilot R9 caught the self-dependency edge case:
1. Schema validation does NOT reject a module declaring itself in
dependsOn (only checks the dep name exists, which "self" trivially
does).
2. topoSortModules' n<=1 early return short-circuited before cycle
detection, so a single-module config with a self-dep slipped past
too.
3. filterResolvableDeps preserved the self-name in the filtered slice
(the name resolves to itself), and the engine handed it to modular's
DependencyAware sort which would either treat it as a 1-cycle or
stall.
Three-part fix:
- engine_module_order.go: filterResolvableDeps now takes a `selfName`
parameter and drops any dep matching it. Comment explains why the
schema gap matters.
- engine_module_order.go: topoSortModules splits the n<=1 path —
n==0 still short-circuits, n==1 explicitly checks for a self-dep
and returns the same cycle error format Kahn produces for n>=2.
- engine.go: passes modCfg.Name as selfName to filterResolvableDeps.
3 new unit tests:
- TestFilterResolvableDeps_DropsSelfReference — module "me" depending
on ["me", "you"] filters to ["you"].
- TestFilterResolvableDeps_OnlySelfReferenceReturnsEmpty — module
"me" depending only on itself returns empty (engine then skips
SetDependencies, preserving constructor defaults).
- TestTopoSortModules_SingleModuleSelfDepIsCycle — single module
declaring itself errors with the cycle message.
- TestTopoSortModules_SingleModuleNoSelfDepIsClean — sanity that
n==1 without self-dep still passes through.
Existing 4 filterResolvableDeps tests updated to pass the new
selfName arg ("_test_self_" — non-matching sentinel).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment on lines
+304
to
+305
| // Module declaring only itself → engine should skip SetDependencies | ||
| // entirely (filtered slice is empty). |
Comment on lines
+154
to
+156
| // 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. |
|
|
||
| ### 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. |
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
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.
Summary
Follow-up to #664 (v0.51.8). The v0.51.8 fix reordered
cfg.Modulesby yamldependsOn:beforeapp.RegisterModule, but modular'sapp.Init()then runs its ownDependencyAware-driven sort over the registered modules — andRemoteModule(the wrapper used for every external-plugin module) returnednilfromDependencies(). So modular still saw every external-plugin module as a root and sorted alphabetically.BMW PR #280 image-launch confirmed the gap on commit
53283b2: with theaaa-/aab-prefix workaround removed and v0.51.8 in place, the engine still initialised the 6bmw-consumer-*modules beforebmw-eventbusandbmw-stream, hit the broker-not-registered 10s race, and never served/healthz. The modular init-order log shows the same alphabetical order v0.51.8 was meant to break.Fix
Two-part:
1.
plugin/external/remote_module.go— add adependenciesfield onRemoteModule+ aSetDependenciessetter.Dependencies()now returns the stored slice instead of always returningnil.2.
engine.go— after the factory returns the module, callSetDependencies(modCfg.DependsOn)via a structural type assertion:Built-in modules opt in by implementing the same setter; modules that don't implement it see no behaviour change. The yaml slice is defensively copied before storing so a downstream mutator can't corrupt the engine-side dependency graph.
Tests
6 new unit tests in
plugin/external/remote_module_dependencies_test.go:TestRemoteModule_Dependencies_DefaultsToNil— fresh module reports no deps.TestRemoteModule_SetDependencies_PlumbsYamlDependsOn— explicit BMW shape (bmw-eventbus,bmw-stream→ consumer).TestRemoteModule_SetDependencies_EmptySliceIsEmpty— empty slice stays empty (not nil).TestRemoteModule_SetDependencies_Overwrites— second call replaces.TestRemoteModule_ImplementsDependencyAware— structural pin matching modular's interface.TestRemoteModule_ImplementsDependencyTargetInterface— structural pin on the setter so a future refactor that dropsSetDependenciesfails loudly.Follow-up
BMW PR #280 will re-validate end-to-end against
v0.51.9after this lands. Once green there, theaaa-/aab-prefix workaround inbuymywishlist/app.yamlfinally goes away for real.🤖 Generated with Claude Code