diff --git a/examples/customresources/demos/optionaldepsmodule/module.go b/examples/customresources/demos/optionaldepsmodule/module.go index 6100c05d453..2699c229cb8 100644 --- a/examples/customresources/demos/optionaldepsmodule/module.go +++ b/examples/customresources/demos/optionaldepsmodule/module.go @@ -8,6 +8,7 @@ import ( "context" "errors" "fmt" + "sync/atomic" "go.viam.com/rdk/components/generic" "go.viam.com/rdk/components/motor" @@ -18,8 +19,10 @@ import ( ) var ( - fooModel = resource.NewModel("acme", "demo", "foo") - mocModel = resource.NewModel("acme", "demo", "moc" /* "mutual optional child" */) + fooModel = resource.NewModel("acme", "demo", "foo") + mocModel = resource.NewModel("acme", "demo", "moc" /* "mutual optional child" */) + pointerTargetModel = resource.NewModel("acme", "demo", "pointer-target") + pointerHolderModel = resource.NewModel("acme", "demo", "pointer-holder") ) func main() { @@ -29,9 +32,17 @@ func main() { resource.RegisterComponent(generic.API, mocModel, resource.Registration[resource.Resource, *MutualOptionalChildConfig]{ Constructor: newMutualOptionalChild, }) + resource.RegisterComponent(generic.API, pointerTargetModel, resource.Registration[resource.Resource, *PointerTargetConfig]{ + Constructor: newPointerTarget, + }) + resource.RegisterComponent(generic.API, pointerHolderModel, resource.Registration[resource.Resource, *PointerHolderConfig]{ + Constructor: newPointerHolder, + }) module.ModularMain(resource.APIModel{generic.API, fooModel}, - resource.APIModel{generic.API, mocModel}) + resource.APIModel{generic.API, mocModel}, + resource.APIModel{generic.API, pointerTargetModel}, + resource.APIModel{generic.API, pointerHolderModel}) } // FooConfig contains a required and optional motor that the component will necessarily @@ -243,3 +254,98 @@ func (moc *mutualOptionalChild) DoCommand(ctx context.Context, req map[string]an // `moc` is notably missing a `Reconfigure` method. Modular resources with optional // dependencies should be able to leverage optional dependencies even as "always rebuild" // resources. + +// PointerTargetConfig configures a pointer-target: a module-served resource that carries +// an optional dependency, making it eligible for rebuild by the RDK's +// updateWeakAndOptionalDependents flow. Another module-internal resource (a pointer-holder) +// can capture a direct Go pointer to it and exercise stale-pointer scenarios when the +// target is rebuilt via the weak/optional path. +type PointerTargetConfig struct { + OptionalDep string `json:"optional_dep"` +} + +// Validate returns the optional dep so updateWeakAndOptionalDependents considers this +// resource for rebuilding. +func (c *PointerTargetConfig) Validate(path string) ([]string, []string, error) { + var optionalDeps []string + if c.OptionalDep != "" { + optionalDeps = append(optionalDeps, c.OptionalDep) + } + return nil, optionalDeps, nil +} + +// pointerTargetInstanceCounter gives each pointerTarget instance a unique ID so tests can +// verify whether a pointer-holder's captured Go pointer references the latest instance or +// a stale one. +var pointerTargetInstanceCounter atomic.Uint64 + +// pointerTarget tracks whether Close was called and carries a unique instance ID. +// DoCommand returns an error if called after Close — this is what lets a dependent detect +// a stale pointer to a closed instance. When alive, it returns its instance ID so callers +// can verify they are talking to the latest instance. +type pointerTarget struct { + resource.Named + resource.AlwaysRebuild + instanceID uint64 + closed atomic.Bool +} + +func newPointerTarget(_ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger) (resource.Resource, error) { + return &pointerTarget{ + Named: conf.ResourceName().AsNamed(), + instanceID: pointerTargetInstanceCounter.Add(1), + }, nil +} + +func (p *pointerTarget) Close(_ context.Context) error { + p.closed.Store(true) + return nil +} + +func (p *pointerTarget) DoCommand(_ context.Context, _ map[string]any) (map[string]any, error) { + if p.closed.Load() { + return nil, errors.New("pointerTarget is closed") + } + return map[string]any{"instance_id": p.instanceID}, nil +} + +// PointerHolderConfig configures a pointer-holder: a resource that explicitly depends on +// a pointer-target and captures a direct Go pointer to it at construction time. +type PointerHolderConfig struct { + Target string `json:"target"` +} + +// Validate declares target as a required dep so it gets resolved via `deps` at +// construction time and a Go pointer can be captured. +func (c *PointerHolderConfig) Validate(path string) ([]string, []string, error) { + if c.Target == "" { + return nil, nil, fmt.Errorf(`expected "target" attribute for pointer-holder %q`, path) + } + return []string{c.Target}, nil, nil +} + +// pointerHolder holds a direct Go pointer to a pointerTarget captured at construction time. +// Its DoCommand proxies through the stored pointer — this is the pattern that leaves the +// holder with a stale reference if the target is rebuilt without notifying the holder. +type pointerHolder struct { + resource.Named + resource.TriviallyCloseable + resource.AlwaysRebuild + target resource.Resource +} + +func newPointerHolder(_ context.Context, deps resource.Dependencies, conf resource.Config, _ logging.Logger) (resource.Resource, error) { + cfg, err := resource.NativeConfig[*PointerHolderConfig](conf) + if err != nil { + return nil, err + } + target, ok := deps[generic.Named(cfg.Target)] + if !ok { + return nil, fmt.Errorf("pointer-holder could not find target %q in dependencies", cfg.Target) + } + return &pointerHolder{Named: conf.ResourceName().AsNamed(), target: target}, nil +} + +func (p *pointerHolder) DoCommand(ctx context.Context, cmd map[string]any) (map[string]any, error) { + return p.target.DoCommand(ctx, cmd) +} diff --git a/module/module.go b/module/module.go index 70103d500fb..5f2844919b3 100644 --- a/module/module.go +++ b/module/module.go @@ -104,11 +104,11 @@ type Module struct { // registerMu protects the maps immediately below as resources/streams come in and out of existence registerMu sync.Mutex collections map[resource.API]resource.APIResourceCollection[resource.Resource] - // internalDeps is keyed by a "child" resource and its values are "internal" resources that - // depend on the child. We use a pointer for the value such that it's stable across map growth. - // Similarly, the slice of `resConfigureArgs` can grow, hence we must use pointers such that - // modifiying in place remains valid. - internalDeps map[resource.Resource][]resConfigureArgs + // internalDeps is keyed by a "child" resource name and its values are "internal" resources that + // depend on the child. Keying by name (rather than object pointer) ensures that the tracking + // survives remove+re-add cycles: when a resource is rebuilt, its name is stable even though + // the Go object pointer changes. + internalDeps map[resource.Name][]resConfigureArgs resLoggers map[resource.Resource]logging.Logger activeResourceStreams map[resource.Name]peerResourceState streamSourceByName map[resource.Name]rtppassthrough.Source @@ -186,7 +186,7 @@ func NewModule(ctx context.Context, address string, logger logging.Logger) (*Mod handlers: HandlerMap{}, collections: map[resource.API]resource.APIResourceCollection[resource.Resource]{}, resLoggers: map[resource.Resource]logging.Logger{}, - internalDeps: map[resource.Resource][]resConfigureArgs{}, + internalDeps: map[resource.Name][]resConfigureArgs{}, } if tracingEnabled { diff --git a/module/resources.go b/module/resources.go index cda7c7508b1..70220d6f483 100644 --- a/module/resources.go +++ b/module/resources.go @@ -373,7 +373,8 @@ func (m *Module) addResource( // Dan: We could call `m.getLocalResource(dep.Name())` but that's just a linear scan over // resLoggers. if _, exists := m.resLoggers[dep]; exists { - m.internalDeps[dep] = append(m.internalDeps[dep], resConfigureArgs{ + depName := dep.Name() + m.internalDeps[depName] = append(m.internalDeps[depName], resConfigureArgs{ toReconfig: res, conf: conf, depStrings: depStrings, @@ -381,6 +382,71 @@ func (m *Module) addResource( } } + // Cascade-rebuild any existing module-internal resources whose Go pointer to the newly-added + // resource is now stale. This handles the case where the RDK removes and re-adds a dependency + // where the dependent was built with an old Go pointer and must be rebuilt to hold a + // reference to the new parent object. + // + // The RDK's updateWeakAndOptionalDependents flow rebuilds a modular resource via + // RemoveResource + AddResource on the module but omits the markChildrenForUpdate call + // so the RDK never notifies dependents. If a module-internal dependent (e.g. a gripper + // that depends_on the arm) holds a direct Go pointer to the rebuilt resource, that + // pointer is left stale. This cascade closes the gap on the module side. + // + // If cascading would transitively circle back to the resource we just added (a mutual optional-dep + // cycle of any length), the recursion would keep rebuilding each member of the cycle without ever + // returning. We prevent that by seeding a `visited` set with the just-added resource and threading + // it through the rebuild recursion; any dependent whose name is already on the cascade stack is + // skipped. One side of the cycle ends up with a stale pointer. + newResName := conf.ResourceName() + if staleDependents, ok := m.internalDeps[newResName]; ok { + visited := map[resource.Name]struct{}{newResName: {}} + for _, args := range staleDependents { + depColl, collOk := m.collections[args.conf.API] + if !collOk { + continue + } + if _, err := depColl.Resource(args.conf.ResourceName().Name); err != nil { + // Dependent no longer registered in the collection; nothing to rebuild. + continue + } + if _, cycled := visited[args.conf.ResourceName()]; cycled { + m.logger.Warnw("skipping cascade rebuild due to mutual-dep cycle; dependent will hold a stale pointer until next reconfigure", + "changedResource", newResName, "dependent", args.conf.Name) + continue + } + freshDeps, err := m.getDependenciesForConstruction(ctx, args.depStrings) + if err != nil { + m.logger.Warnw("failed to get deps for cascade rebuild after resource re-add", + "changedResource", newResName, "dependent", args.conf.Name, "err", err) + continue + } + m.registerMu.Unlock() + newDependentRes, err := m.rebuildResourceWithVisited(ctx, freshDeps, args.conf, nil, visited) + m.registerMu.Lock() + if err != nil { + m.logger.Warnw("failed to cascade rebuild dependent after resource re-add", + "changedResource", newResName, "dependent", args.conf.Name, "err", err) + continue + } + // Record the new dependent pointer in our entry so internalDeps stays in sync + // with the collection. This keeps `toReconfig` accurate for any downstream code + // that reads it (e.g. removeResource's filter loop matches entries by pointer + // identity — if we left toReconfig at the pre-rebuild pointer, a subsequent + // remove of the newly-rebuilt dependent wouldn't clean up this entry). + if newDependentRes != nil { + dependentName := args.conf.ResourceName() + entries := m.internalDeps[newResName] + for j := range entries { + if entries[j].conf.ResourceName() == dependentName { + entries[j].toReconfig = newDependentRes + break + } + } + } + } + } + return nil } @@ -416,18 +482,21 @@ func (m *Module) removeResource(ctx context.Context, resName resource.Name) erro delete(m.activeResourceStreams, res.Name()) delete(m.resLoggers, res) - // The viam-server forbids removing a resource until dependents are first closed/removed. Hence - // it's safe to assume the value in the map for `res` is empty and simply remove the map entry.q - delete(m.internalDeps, res) - - for dep, chainReconfiguresPtr := range m.internalDeps { - chainReconfigures := chainReconfiguresPtr - for idx, chainRes := range chainReconfigures { - if res == chainRes.toReconfig { - // Clear the removed resource from any chain of reconfigures it appears in. - m.internalDeps[dep] = append(chainReconfigures[:idx], chainReconfigures[idx+1:]...) + // Clear the removed resource from any dependency chain it appears in as a dependent. We do NOT + // delete the name-keyed entry for `res` itself — if `res` is re-added later, the cascade step + // in addResource needs that entry to find stale dependents and rebuild them. + for depName, chainReconfigures := range m.internalDeps { + filtered := chainReconfigures[:0] + for _, chainRes := range chainReconfigures { + if chainRes.toReconfig != res { + filtered = append(filtered, chainRes) } } + if len(filtered) == 0 { + delete(m.internalDeps, depName) + } else { + m.internalDeps[depName] = filtered + } } return coll.Remove(resName) @@ -438,6 +507,21 @@ func (m *Module) removeResource(ctx context.Context, resName resource.Name) erro func (m *Module) rebuildResource( ctx context.Context, deps resource.Dependencies, conf *resource.Config, logLevel *logging.Level, ) (resource.Resource, error) { + return m.rebuildResourceWithVisited(ctx, deps, conf, logLevel, map[resource.Name]struct{}{}) +} + +// rebuildResourceWithVisited is the recursive body of rebuildResource. It tracks which +// resources are currently on the cascade stack (in `visited`) to prevent infinite recursion +// through mutual-dep cycles. The current resource is added to `visited` on entry and +// removed on exit (stack discipline) so that sibling branches of an outer cascade can be +// rebuilt independently. +func (m *Module) rebuildResourceWithVisited( + ctx context.Context, deps resource.Dependencies, conf *resource.Config, logLevel *logging.Level, + visited map[resource.Name]struct{}, +) (resource.Resource, error) { + visited[conf.ResourceName()] = struct{}{} + defer delete(visited, conf.ResourceName()) + m.registerMu.Lock() coll, ok := m.collections[conf.API] if !ok { @@ -493,10 +577,19 @@ func (m *Module) rebuildResource( m.streamSourceByName[res.Name()] = p } - depsToRebuild := m.internalDeps[res] - // Build up a new slice to map `m.internalDeps[newRes]` to. + resName := res.Name() + depsToRebuild := m.internalDeps[resName] + // Build up a new slice to map `m.internalDeps[resName]` to. newDepsToRebuild := make([]resConfigureArgs, 0, len(depsToRebuild)) for _, depToReconfig := range depsToRebuild { + if _, cycled := visited[depToReconfig.conf.ResourceName()]; cycled { + // This dependent is already on the cascade stack. Rebuilding would close + // a resource that an outer caller still holds. Keep the entry unchanged. + m.logger.Warnw("skipping cascade rebuild due to mutual-dep cycle; dependent will hold a stale pointer until next reconfigure", + "changedResource", resName, "dependent", depToReconfig.conf.Name) + newDepsToRebuild = append(newDepsToRebuild, depToReconfig) + continue + } // We are going to modify `toReconfig` at the end. Make sure changes to `dependentResConfigureArgs` // get reflected in the slice. deps, err := m.getDependenciesForConstruction(ctx, depToReconfig.depStrings) @@ -512,12 +605,12 @@ func (m *Module) rebuildResource( // We release the `registerMu` to let other resource query/acquisition methods make // progress. We do not assume `rebuildResource` is fast. // - // We also release the mutex as the recursive call to `rebuildResource` will reacquire - // it. And the mutex is not reentrant. + // We also release the mutex as the recursive call to `rebuildResourceWithVisited` + // will reacquire it. And the mutex is not reentrant. m.registerMu.Unlock() var nilLogLevel *logging.Level // pass in nil to avoid changing the log level - rebuiltRes, err := m.rebuildResource(ctx, deps, depToReconfig.conf, nilLogLevel) + rebuiltRes, err := m.rebuildResourceWithVisited(ctx, deps, depToReconfig.conf, nilLogLevel, visited) if err != nil { m.logger.Warn("Failed to cascade dependent reconfigure", "changedResource", conf.Name, @@ -533,8 +626,7 @@ func (m *Module) rebuildResource( }) } - m.internalDeps[newRes] = newDepsToRebuild - delete(m.internalDeps, res) + m.internalDeps[resName] = newDepsToRebuild m.registerMu.Unlock() return newRes, nil diff --git a/robot/impl/optional_dependencies_test.go b/robot/impl/optional_dependencies_test.go index 6f5cda1a15b..bc1e6a9ae6b 100644 --- a/robot/impl/optional_dependencies_test.go +++ b/robot/impl/optional_dependencies_test.go @@ -1352,6 +1352,18 @@ func TestModularOptionalDependenciesCycles(t *testing.T) { lr.Reconfigure(ctx, &cfg) { // Assertions + // Note on stale pointers in mutual-optional cycles: updateWeakAndOptionalDependents + // processes cfg.Components in order. The resource processed *last* ends up with + // `otherMOC` pointing at a now-closed prior instance of the first-processed side. + // The init loop inside its AddResource captures whatever is in the collection at + // that moment; its own cascade then rebuilds (and closes) that captured other. + // Swap the component order in the config and the stale side swaps too. + // + // The DoCommand assertions below pass anyway because `mutualOptionalChild` + // embeds `resource.TriviallyCloseable` — `Close` is a no-op, so the stale + // (closed) pointer still services method calls. "usable" here means "the + // pointer reaches a responsive moc", not "the pointer is fresh". + // Assert that the first 'moc' component is still accessible (did not fail to // reconstruct). mocRes, err := lr.ResourceByName(mocName) @@ -2250,3 +2262,313 @@ func TestModularOptionalDependencyModuleCrash(t *testing.T) { test.That(t, err, test.ShouldBeNil) test.That(t, doCommandResp, test.ShouldResemble, map[string]any{"optional_motor_state": "moving: false"}) } + +// TestModularStalePointerAfterWeakOptionalRebuild pins the regression behind the module-level +// cascade in module/resources.go. +// +// Setup: +// - pointer-target (modular) declares an optional dep. When that dep's availability +// changes, the RDK's updateWeakAndOptionalDependents flow rebuilds the target by +// sending RemoveResource + AddResource to the module. +// - pointer-holder (modular, same module) explicitly depends_on the target and captures +// a direct Go pointer to it at construction time. +// +// Each target instance carries a unique ID. After the target is rebuilt, the holder must +// be rebuilt too so its captured pointer references the current target instance — not the +// one that was just closed. The test probes both the target directly and the +// target-via-holder and asserts their instance IDs match. Without the cascade, the holder +// ends up pointing at a closed prior instance and its proxied DoCommand either errors or +// returns a stale ID. +func TestModularStalePointerAfterWeakOptionalRebuild(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + + lr := setupLocalRobot(t, ctx, &config.Config{}, logger, WithDisableCompleteConfigWorker()) + + optionalDepsModulePath := testutils.BuildTempModule(t, "examples/customresources/demos/optionaldepsmodule") + + targetModel := resource.NewModel("acme", "demo", "pointer-target") + holderModel := resource.NewModel("acme", "demo", "pointer-holder") + targetName := generic.Named("target1") + holderName := generic.Named("holder1") + + cfg := config.Config{ + Modules: []config.Module{ + { + Name: "optional-deps", + ExePath: optionalDepsModulePath, + }, + }, + Components: []resource.Config{ + { + // The optional-dep target — its presence causes pointer-target to be + // rebuilt by updateWeakAndOptionalDependents on every reconfigure cycle. + Name: "opt-dep", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }, + { + Name: targetName.Name, + API: generic.API, + Model: targetModel, + Attributes: rutils.AttributeMap{ + "optional_dep": "opt-dep", + }, + }, + { + // Explicit depends_on target1 via the config attribute; the module's + // Validate returns it as a required dep so the holder captures a Go + // pointer to target1. + Name: holderName.Name, + API: generic.API, + Model: holderModel, + Attributes: rutils.AttributeMap{ + "target": targetName.Name, + }, + }, + }, + } + // Ensure fills in ImplicitOptionalDependsOn from Validate — required for + // updateWeakAndOptionalDependents to consider pointer-target. + test.That(t, cfg.Ensure(false, logger), test.ShouldBeNil) + + // assertHolderPointsToCurrentTarget fetches the current target instance ID directly, + // then fetches the target instance ID via the holder (which proxies through its + // captured pointer), and asserts they match. A mismatch means the holder is holding + // a stale pointer to a previous target instance. + assertHolderPointsToCurrentTarget := func(label string) { + targetRes, err := lr.ResourceByName(targetName) + test.That(t, err, test.ShouldBeNil) + targetResp, err := targetRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + + holderRes, err := lr.ResourceByName(holderName) + test.That(t, err, test.ShouldBeNil) + holderResp, err := holderRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + + test.That(t, holderResp["instance_id"], test.ShouldEqual, targetResp["instance_id"]) + } + + // Initial Reconfigure — builds target_v1 and holder_v1, then + // updateWeakAndOptionalDependents rebuilds target (to target_v2) and the module's + // cascade rebuilds holder (to holder_v2) so it points at target_v2. + lr.Reconfigure(ctx, &cfg) + assertHolderPointsToCurrentTarget("initial") + + // Second Reconfigure with an unrelated change (adds another fake motor) advances the + // logical clock, so updateWeakAndOptionalDependents fires for target a second time. + // This is the regression case: the first cascade already rebuilt holder, and we need + // the second cascade to rebuild it again so its captured pointer tracks the latest + // target instance rather than a now-closed prior one. + cfg2 := cfg + cfg2.Components = append(append([]resource.Config{}, cfg.Components...), resource.Config{ + Name: "unrelated-motor", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }) + test.That(t, cfg2.Ensure(false, logger), test.ShouldBeNil) + lr.Reconfigure(ctx, &cfg2) + assertHolderPointsToCurrentTarget("after-unrelated-change") +} + +// TestModularStalePointerCascadeFanOut verifies that a single rebuild of a resource with +// multiple explicit dependents cascades to rebuild every one of them. +// +// Setup: +// - One pointer-target with an optional dep (triggers updateWeakAndOptionalDependents +// on every reconfigure). +// - Three pointer-holders, each depending on that target. +// +// After reconfigure, every holder should route DoCommand through the current target +// instance. If the cascade only rebuilds one holder (or rebuilds them against different +// target instances), some holder's instance_id would diverge from the others or from the +// target itself. +func TestModularStalePointerCascadeFanOut(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + + lr := setupLocalRobot(t, ctx, &config.Config{}, logger, WithDisableCompleteConfigWorker()) + + optionalDepsModulePath := testutils.BuildTempModule(t, "examples/customresources/demos/optionaldepsmodule") + + targetModel := resource.NewModel("acme", "demo", "pointer-target") + holderModel := resource.NewModel("acme", "demo", "pointer-holder") + targetName := generic.Named("target") + holderNames := []resource.Name{ + generic.Named("holder-a"), + generic.Named("holder-b"), + generic.Named("holder-c"), + } + + components := []resource.Config{ + { + Name: "opt-dep", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }, + { + Name: targetName.Name, + API: generic.API, + Model: targetModel, + Attributes: rutils.AttributeMap{ + "optional_dep": "opt-dep", + }, + }, + } + for _, hn := range holderNames { + components = append(components, resource.Config{ + Name: hn.Name, + API: generic.API, + Model: holderModel, + Attributes: rutils.AttributeMap{ + "target": targetName.Name, + }, + }) + } + + cfg := config.Config{ + Modules: []config.Module{ + {Name: "optional-deps", ExePath: optionalDepsModulePath}, + }, + Components: components, + } + test.That(t, cfg.Ensure(false, logger), test.ShouldBeNil) + + assertAllHoldersPointToCurrentTarget := func(label string) { + targetRes, err := lr.ResourceByName(targetName) + test.That(t, err, test.ShouldBeNil) + targetResp, err := targetRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + wantID := targetResp["instance_id"] + + for _, hn := range holderNames { + holderRes, err := lr.ResourceByName(hn) + test.That(t, err, test.ShouldBeNil) + holderResp, err := holderRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + test.That(t, holderResp["instance_id"], test.ShouldEqual, wantID) + } + } + + lr.Reconfigure(ctx, &cfg) + assertAllHoldersPointToCurrentTarget("initial") + + // Push a config change to advance the clock and re-trigger updateWeakAndOptionalDependents. + cfg2 := cfg + cfg2.Components = append(append([]resource.Config{}, cfg.Components...), resource.Config{ + Name: "unrelated-motor", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }) + test.That(t, cfg2.Ensure(false, logger), test.ShouldBeNil) + lr.Reconfigure(ctx, &cfg2) + assertAllHoldersPointToCurrentTarget("after-unrelated-change") +} + +// TestModularStalePointerCascadeChain verifies that cascades propagate transitively through +// multi-hop explicit-dep chains. This exercises the recursive cascade inside +// rebuildResourceWithVisited (invoked by the outer addResource cascade), not just the +// single-level cascade in addResource. +// +// Setup (chain): target → mid-holder (depends on target) → top-holder (depends on mid-holder) +// +// When target is rebuilt: +// 1. addResource(target_new) cascade finds mid-holder in internalDeps[target], rebuilds it. +// 2. rebuildResourceWithVisited(mid-holder) runs ITS cascade over internalDeps[mid-holder], +// rebuilds top-holder. +// 3. top-holder ends up freshly constructed, its captured pointer referencing the freshly +// constructed mid-holder, which references the new target. +// +// top-holder.DoCommand proxies → mid-holder.DoCommand → target.DoCommand → returns the +// current target's instance_id. Any break in the chain would either error or report a +// stale ID. +func TestModularStalePointerCascadeChain(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + + lr := setupLocalRobot(t, ctx, &config.Config{}, logger, WithDisableCompleteConfigWorker()) + + optionalDepsModulePath := testutils.BuildTempModule(t, "examples/customresources/demos/optionaldepsmodule") + + targetModel := resource.NewModel("acme", "demo", "pointer-target") + holderModel := resource.NewModel("acme", "demo", "pointer-holder") + targetName := generic.Named("target") + midName := generic.Named("mid-holder") + topName := generic.Named("top-holder") + + cfg := config.Config{ + Modules: []config.Module{ + {Name: "optional-deps", ExePath: optionalDepsModulePath}, + }, + Components: []resource.Config{ + { + Name: "opt-dep", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }, + { + Name: targetName.Name, + API: generic.API, + Model: targetModel, + Attributes: rutils.AttributeMap{ + "optional_dep": "opt-dep", + }, + }, + { + Name: midName.Name, + API: generic.API, + Model: holderModel, + Attributes: rutils.AttributeMap{ + "target": targetName.Name, + }, + }, + { + Name: topName.Name, + API: generic.API, + Model: holderModel, + Attributes: rutils.AttributeMap{ + "target": midName.Name, + }, + }, + }, + } + test.That(t, cfg.Ensure(false, logger), test.ShouldBeNil) + + // The top-holder proxies DoCommand through mid-holder, which proxies through target. + // If any link in the chain holds a stale pointer, the ID returned at the top won't + // match the target's current ID. + assertTopHolderReachesCurrentTarget := func(label string) { + targetRes, err := lr.ResourceByName(targetName) + test.That(t, err, test.ShouldBeNil) + targetResp, err := targetRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + + topRes, err := lr.ResourceByName(topName) + test.That(t, err, test.ShouldBeNil) + topResp, err := topRes.DoCommand(ctx, map[string]any{"probe": label}) + test.That(t, err, test.ShouldBeNil) + + test.That(t, topResp["instance_id"], test.ShouldEqual, targetResp["instance_id"]) + } + + lr.Reconfigure(ctx, &cfg) + assertTopHolderReachesCurrentTarget("initial") + + // Push a config change to advance the clock and re-trigger the cascade. + cfg2 := cfg + cfg2.Components = append(append([]resource.Config{}, cfg.Components...), resource.Config{ + Name: "unrelated-motor", + API: motor.API, + Model: fake.Model, + ConvertedAttributes: &fake.Config{}, + }) + test.That(t, cfg2.Ensure(false, logger), test.ShouldBeNil) + lr.Reconfigure(ctx, &cfg2) + assertTopHolderReachesCurrentTarget("after-unrelated-change") +}