From f714a10c167db857735e34d3d80bc714c2be24bf Mon Sep 17 00:00:00 2001 From: danielbotros Date: Thu, 24 Sep 2026 17:52:44 -0400 Subject: [PATCH 01/14] RSDK-14329: composite resolution/advertisement engine (server core) robot/impl server-side: local_robot resolution (api-less resourceBySimpleName / resolveSimpleNameMatch, specific-API unwrap, StopAll dedup, weak-dep + dependency composite-aliasing, MachineStatus metadata), resource_manager advertisement (ExpandCompositeNames, remote-collision skip, remote metadata), and ResourceFromRobot unwrap. Consumes the resource-graph remote-composite machinery from the core PR. First of the ticket-3 split. Co-Authored-By: Claude Opus 4.8 (1M context) (cherry picked from commit 202fdae350973bfc6893835179fbe4bb8b13ae3d) --- robot/impl/local_robot.go | 155 ++++++++++++++++++++++++++++----- robot/impl/resource_manager.go | 32 +++++-- robot/robot.go | 16 ++-- 3 files changed, 162 insertions(+), 41 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 23a43ea3525..c136af90344 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -205,10 +205,12 @@ func (r *localRobot) UploadDataFromPath(ctx context.Context, path string, md *da return uploader.UploadDataFromPath(ctx, path, md, extra) } -// FindBySimpleNameAndAPI finds a resource by its simple name and API. This is queried -// through the resourceGetterForAPI for _all_ incoming gRPC requests related to a -// resource. A nil resource and an error is returned in the case of no resource found, or -// multiple matching remote resources found. +// FindBySimpleNameAndAPI finds a resource by its simple name and API — the specific-API lookup +// queried through the resourceGetterForAPI for _all_ incoming gRPC requests related to a resource. It +// returns a nil resource and an error when no resource, or multiple matching remote resources, are +// found. It unwraps a composite to the sub-resource serving that API (resource.SubresourceForAPI), so +// an in-process consumer that fetches by a concrete API and type-asserts sees the real typed instance, +// not the composite wrapper, which satisfies none of the API interfaces. func (r *localRobot) FindBySimpleNameAndAPI(name string, api resource.API) (resource.Resource, error) { n, err := r.manager.resources.FindBySimpleNameAndAPI(name, api) if err != nil { @@ -218,17 +220,76 @@ func (r *localRobot) FindBySimpleNameAndAPI(name string, api resource.API) (reso if err != nil { return nil, resource.NewNotAvailableError(resource.NewName(api, name), err) } - return res, nil + return resource.SubresourceForAPI(res, api), nil } -// ResourceByName returns a resource by name. It now re-routes all calls to -// FindBySimpleNameAndAPI. All incoming gRPC requests related to a resource go through -// FindBySimpleNameAndAPI. ResourceByName is only called internally for some dependency -// calculation and session code. +// ResourceByName returns a resource by name. It routes an api-specific name to FindBySimpleNameAndAPI +// (which unwraps a composite to the sub-resource serving that API) and a bare, API-less name to +// resourceBySimpleName (which for a composite returns the single handle serving every API). +// ResourceByName is only called internally, for some dependency calculation and session code. func (r *localRobot) ResourceByName(name resource.Name) (resource.Resource, error) { + if name.API == (resource.API{}) { + return r.resourceBySimpleName(name.Name) + } return r.FindBySimpleNameAndAPI(name.Name, name.API) } +// resourceBySimpleName resolves a bare (API-less) name to its single resource. A composite is served +// under several co-equal APIs but is one identity, so FindBySimpleName dedups it (local or remote) to +// a single match and this returns the one handle serving every API. A genuine same-name collision +// across distinct resources returns several matches and is an error. +func (r *localRobot) resourceBySimpleName(name string) (resource.Resource, error) { + matches := r.manager.resources.FindAllBySimpleName(name) + switch len(matches) { + case 0: + return nil, resource.NewNotFoundError(resource.SimpleName(name)) + case 1: + return r.resolveSimpleNameMatch(name, matches[0]) + default: + return nil, errors.Errorf( + "multiple resources share the simple name %q across distinct APIs (%s); look it up by its fully qualified name instead", + name, resource.NamesToStrings(matches), + ) + } +} + +// resolveSimpleNameMatch turns the single simple-name match into a resource handle. A local composite +// already lives in one resource.MultiAPIResource graph node, so a direct lookup returns the whole +// handle. A remote composite instead lives as one per-API sub-client node per co-equal API, so +// assemble a resource.MultiAPIResource view over those sub-clients. +func (r *localRobot) resolveSimpleNameMatch(name string, match resource.Name) (resource.Resource, error) { + if match.Remote == "" { + // Resolve the raw graph node (not the localRobot FindBySimpleNameAndAPI, which unwraps a + // composite to one API's sub) so the full multi-API handle is returned. + n, err := r.manager.resources.FindBySimpleNameAndAPI(match.Name, match.API) + if err != nil { + return nil, err + } + res, err := n.Resource() + if err != nil { + return nil, resource.NewNotAvailableError(match, err) + } + return res, nil + } + // The graph caches a remote resource under its prefixed name, so look it up by `name` (which keeps + // the prefix) rather than match.Name (which FindBySimpleName stripped). + apis := r.manager.resources.APIsForRemoteResource(name, match.Remote) + if len(apis) <= 1 { + return r.FindBySimpleNameAndAPI(name, match.API) + } + byAPI := make(map[resource.API]resource.Resource, len(apis)) + for _, api := range apis { + sub, err := r.FindBySimpleNameAndAPI(name, api) + if err != nil { + return nil, err + } + byAPI[api] = sub + } + return resource.NewMultiAPIResource( + resource.Name{API: apis[0], Remote: match.Remote, Name: match.Name}, apis, byAPI, + ), nil +} + // RemoteNames returns the names of all known remote robots. func (r *localRobot) RemoteNames() []string { return r.manager.RemoteNames() @@ -330,19 +391,32 @@ func (r *localRobot) StopAll(ctx context.Context, extra map[resource.Name]map[st // disconnect, or any residual cancellation). Matches motion's stop-on-error pattern. stopCtx := context.WithoutCancel(ctx) - // Stop all stoppable resources + // Stop all stoppable resources. A composite surfaces as N names (one per co-equal API) backed by + // one instance, so dedup on the raw pre-unwrap node resource to stop that instance at most once. resourceErrs := make(map[string]error) + stopped := make(map[resource.Resource]bool) for _, name := range r.ResourceNames() { - res, err := r.ResourceByName(name) + node, err := r.manager.resources.FindBySimpleNameAndAPI(name.Name, name.API) if err != nil { resourceErrs[name.Name] = err continue } + raw, err := node.Resource() + if err != nil { + resourceErrs[name.Name] = err + continue + } + if stopped[raw] { + continue + } - if actuator, ok := res.(resource.Actuator); ok { - if err := actuator.Stop(stopCtx, extra[name]); err != nil { - resourceErrs[name.Name] = err - } + actuator, ok := resource.SubresourceForAPI(raw, name.API).(resource.Actuator) + if !ok { + continue + } + stopped[raw] = true + if err := actuator.Stop(stopCtx, extra[name]); err != nil { + resourceErrs[name.Name] = err } } @@ -825,6 +899,12 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( return nil, nil, &resource.DependencyNotReadyError{Name: dep.Name, Reason: err} } allDeps[prefixedName] = res + // A composite dependency serves several co-equal APIs from one identity; key it under each of + // its API names so a dependent can resolve it by whichever API it expects. + for _, api := range r.coequalAPIsOf(prefixedName, res) { + aliased := resource.Name{API: api, Remote: prefixedName.Remote, Name: prefixedName.Name} + allDeps[aliased] = res + } } nodeConf := gNode.Config() weakDeps, weakSnap := r.getWeakDependenciesAndSnapshot(rName, nodeConf.API, nodeConf.Model) @@ -851,6 +931,22 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( return allDeps, weakAndOptionalDepsSnapshot, nil } +// coequalAPIsOf returns every API a composite dependency serves, or nil for an ordinary resource. +// A modular composite is stored as a resource.MultiAPIResource, so APIsOf reports its full set; a +// builtin composite is stored as its raw multi-API instance, whose set is recovered from the backing +// graph node's model. +func (r *localRobot) coequalAPIsOf(name resource.Name, res resource.Resource) []resource.API { + if apis := resource.APIsOf(res); len(apis) > 1 { + return apis + } + if node, err := r.manager.resources.FindBySimpleNameAndAPI(name.Name, name.API); err == nil { + if apis := resource.APIsForModel(node.ResourceModel()); len(apis) > 1 { + return apis + } + } + return nil +} + func (r *localRobot) getWeakDependencyMatchers(api resource.API, model resource.Model) []resource.Matcher { reg, ok := resource.LookupRegistration(api, model) if !ok { @@ -997,14 +1093,27 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( } continue } - for _, matcher := range weakDepMatchers { - if matcher.IsMatch(res) { - // Pop the remote name off since callers won't be expecting it when accessing it in the resource - // dependency map in a resource constructor. - popped := n.PopRemote() - deps[popped] = res - snapshot[popped] = node.UpdatedAt() - break + // A composite serves several co-equal APIs from one identity, so test the weak-dep matchers + // against each served API and store a match as that API's unwrapped sub under that API's name. + // Otherwise a composite matched on a non-canonical API is dropped (the Subtype/Type matchers read + // Name().API, which is only the canonical API) and the consumer gets the wrapper, not the typed + // sub. For an ordinary resource this is n.API and a no-op unwrap. + apis := r.coequalAPIsOf(n, res) + if apis == nil { + apis = []resource.API{n.API} + } + for _, api := range apis { + sub := resource.SubresourceForAPI(res, api) + apiName := resource.Name{API: api, Remote: n.Remote, Name: n.Name} + for _, matcher := range weakDepMatchers { + if matcher.IsMatch(sub) { + // Pop the remote name off since callers won't be expecting it when accessing it in the + // resource dependency map in a resource constructor. + popped := apiName.PopRemote() + deps[popped] = sub + snapshot[popped] = node.UpdatedAt() + break + } } } } diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index 646a964f6e6..a767c5dff09 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -357,9 +357,18 @@ func (manager *resourceManager) updateRemoteResourceNames( // Log if this remote resource's name collides with any existing resource. On collision // the remote stays in the graph but is hidden from the machine's resource list. One // FindAllBySimpleName scan spans all APIs, so it catches both same-API and cross-API - // collisions; enumerate every conflicting resource. + // collisions. A composite living on this remote is advertised as several same-named + // resources that differ only by API; those are one composite (names are unique per remote), + // not a collision, so matches from THIS remote are skipped. prefixedSimpleName := prefix + resName.Name - if conflicts := manager.resources.FindAllBySimpleName(prefixedSimpleName); len(conflicts) > 0 { + var conflicts []resource.Name + for _, match := range manager.resources.FindAllBySimpleName(prefixedSimpleName) { + if match.Remote == remoteName.Name { + continue + } + conflicts = append(conflicts, match) + } + if len(conflicts) > 0 { manager.logger.Errorw(logMsgRemoteNameCollision, "name", prefixedSimpleName, "api", resName.API, "conflicts_with", resource.NamesToStrings(conflicts), "remote", remoteName.Name) @@ -536,11 +545,15 @@ func (manager *resourceManager) AllNonCollidingResourceNames() []resource.Name { // - Resources that are remote and have the same full name as another resource (name collision). // Remotes resources' Name field will be automatically prefixed. func (manager *resourceManager) ResourceNames() []resource.Name { - return manager.resources.SimpleNamesWhere(func(k resource.Name, gNode *resource.GraphNode) bool { - return k.API != client.RemoteAPI && - k.API.Type.Namespace != resource.APINamespaceRDKInternal && - gNode.HasResource() - }) + // A composite is stored as one node under its canonical API; ExpandCompositeNames advertises it + // as one same-named ResourceName per co-equal API so a client can detect and assemble it. + return manager.resources.ExpandCompositeNames( + manager.resources.SimpleNamesWhere(func(k resource.Name, gNode *resource.GraphNode) bool { + return k.API != client.RemoteAPI && + k.API.Type.Namespace != resource.APINamespaceRDKInternal && + gNode.HasResource() + }), + ) } // reachableResourceNames returns the names of all resources in the manager, excluding the following types of resources: @@ -563,7 +576,10 @@ func (manager *resourceManager) ResourceRPCAPIs() []resource.RPCAPI { resourceAPIs := resource.RegisteredAPIs() types := map[resource.API]*desc.ServiceDescriptor{} - for _, k := range manager.resources.Names() { + // A composite is stored as one node under its canonical API; ExpandCompositeNames yields one + // same-named Name per co-equal API it serves so every served API surfaces its RPC descriptor, + // regardless of which API the composite is declared under. + for _, k := range manager.resources.ExpandCompositeNames(manager.resources.Names()) { if k.API.Type.Namespace == resource.APINamespaceRDKInternal { continue } diff --git a/robot/robot.go b/robot/robot.go index 5789d72a139..e1450e34767 100644 --- a/robot/robot.go +++ b/robot/robot.go @@ -200,8 +200,9 @@ type LocalRobot interface { // Only use this if comfortable with leaking resources (in cases where exiting the program as quickly as possible is desired). Kill() - // FindBySimpleNameAndAPI returns a resource from the resource graph. See - // [resource.Graph.FindBySimpleNameAndAPI] for specifics about what is + // FindBySimpleNameAndAPI returns a resource from the resource graph, unwrapping a composite to the + // sub-resource serving the given API so a caller that type-asserts to the API's interface sees the + // real typed instance. See [resource.Graph.FindBySimpleNameAndAPI] for specifics about what is // returned in the case of name collisions. FindBySimpleNameAndAPI(string, resource.API) (resource.Resource, error) @@ -323,20 +324,15 @@ func ResourceFromProtoMessage( return res, fqName, nil } -// ResourceFromRobot returns a resource from a robot. +// ResourceFromRobot returns a resource from a robot. It uses resource.AsType so that a composite +// (multi-API) resource handle is unwrapped to the sub-resource satisfying T. func ResourceFromRobot[T resource.Resource](robot Robot, name resource.Name) (T, error) { var zero T res, err := robot.ResourceByName(name) if err != nil { return zero, err } - - part, ok := res.(T) - - if !ok { - return zero, resource.TypeError[T](res) - } - return part, nil + return resource.AsType[T](res) } // MatchesModule returns true if the passed-in module matches its name / ID. From e6a1a4c568c4e0fc620d7cbd71e36f08957a71a4 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Thu, 24 Sep 2026 18:14:10 -0400 Subject: [PATCH 02/14] RSDK-14329: composite in-process + remote end-to-end tests (cherry picked from commit f93949cbf7d85f81cb351ab4434e3a397013143d) --- robot/impl/multiapi_composite_test.go | 538 ++++++++++++++++++++++++++ 1 file changed, 538 insertions(+) create mode 100644 robot/impl/multiapi_composite_test.go diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go new file mode 100644 index 00000000000..c8deb61c66d --- /dev/null +++ b/robot/impl/multiapi_composite_test.go @@ -0,0 +1,538 @@ +package robotimpl + +import ( + "context" + "fmt" + "sync/atomic" + "testing" + + "go.viam.com/test" + "go.viam.com/utils/testutils" + + "go.viam.com/rdk/components/generic" + "go.viam.com/rdk/components/gripper" + "go.viam.com/rdk/components/sensor" + "go.viam.com/rdk/components/servo" + "go.viam.com/rdk/config" + "go.viam.com/rdk/logging" + "go.viam.com/rdk/referenceframe" + "go.viam.com/rdk/resource" + "go.viam.com/rdk/robot" + "go.viam.com/rdk/spatialmath" + "go.viam.com/rdk/testutils/robottestutils" +) + +// comboSensor serves both sensor.Sensor and generic.Resource from one identity. +type comboSensor struct { + resource.Named + closeCount *atomic.Int32 +} + +func (c *comboSensor) Readings(context.Context, map[string]interface{}) (map[string]interface{}, error) { + return map[string]interface{}{"reading": 7}, nil +} + +// DoCommand echoes cmd["ping"] back under "echo". It gives the composite's non-sensor (generic) +// facade a method with an observable result, so a call can be proven to route to the correct per-API +// facade of the one composite (rather than to Readings). +func (c *comboSensor) DoCommand(_ context.Context, cmd map[string]interface{}) (map[string]interface{}, error) { + return map[string]interface{}{"echo": cmd["ping"]}, nil +} + +func (c *comboSensor) Close(context.Context) error { + if c.closeCount != nil { + c.closeCount.Add(1) + } + return nil +} + +// registerComboModel registers a fresh sensor+generic composite model and returns it, deregistering +// on test cleanup. Each Close on the built instance increments closeCount. +func registerComboModel(t *testing.T, name string, closeCount *atomic.Int32) resource.Model { + t.Helper() + model := resource.NewModel("acme", "test", name) + resource.RegisterMultiAPI( + []resource.API{sensor.API, generic.API}, model, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + return &comboSensor{Named: conf.ResourceName().AsNamed(), closeCount: closeCount}, nil + }, + }, + ) + t.Cleanup(func() { + resource.Deregister(sensor.API, model) + resource.Deregister(generic.API, model) + }) + return model +} + +func TestCompositeResourceEndToEnd(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + var closeCount atomic.Int32 + model := registerComboModel(t, "composite-sensor", &closeCount) + + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "combo", API: sensor.API, Model: model}, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + // 1) An api-less SimpleName resolves to the one handle; AsType extracts the sensor interface. + res, err := r.ResourceByName(resource.SimpleName("combo")) + test.That(t, err, test.ShouldBeNil) + s, err := resource.AsType[sensor.Sensor](res) + test.That(t, err, test.ShouldBeNil) + readings, err := s.Readings(ctx, nil) + test.That(t, err, test.ShouldBeNil) + test.That(t, readings["reading"], test.ShouldEqual, 7) + + // 2) A lookup by EACH co-equal API returns the SAME single instance as the api-less lookup. + bySensor, err := r.ResourceByName(sensor.Named("combo")) + test.That(t, err, test.ShouldBeNil) + test.That(t, bySensor, test.ShouldEqual, res) + + byGeneric, err := r.ResourceByName(resource.NewName(generic.API, "combo")) + test.That(t, err, test.ShouldBeNil) + test.That(t, byGeneric, test.ShouldEqual, res) + + // 3) In-process the api-less handle is the one raw instance, which natively implements every + // co-equal API — AsType extracts each. (APIsOf on a raw builtin instance reports only its single + // Name API; the modular composite handle, a resource.MultiAPIResource, reports the full set — see + // TestModularCompositeResource.) + _, err = resource.AsType[sensor.Sensor](res) + test.That(t, err, test.ShouldBeNil) + _, err = resource.AsType[resource.Resource](res) + test.That(t, err, test.ShouldBeNil) + + // 4) It is advertised as N same-named ResourceNames (one per co-equal API). + var advertised []resource.API + for _, n := range r.ResourceNames() { + if n.Name == "combo" { + advertised = append(advertised, n.API) + } + } + test.That(t, advertised, test.ShouldContain, sensor.API) + test.That(t, advertised, test.ShouldContain, generic.API) + + // 5) Teardown closes the single underlying instance exactly once, even though it is reachable + // under several APIs. Reconfigure to an empty config to remove it, then assert the count. + r.Reconfigure(ctx, &config.Config{}) + test.That(t, closeCount.Load(), test.ShouldEqual, 1) +} + +// TestCompositeDependencyResolvesAllAPIs asserts that a second resource depending on a composite can +// resolve it under each of the composite's co-equal API names (the composite dependency is keyed +// under each API in the dependent's Dependencies). +func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + comboModel := registerComboModel(t, "composite-sensor-dep", nil) + + // A consumer that fails construction unless it can resolve the composite under BOTH APIs. + consumerModel := resource.NewModel("acme", "test", "composite-consumer") + resource.RegisterComponent(generic.API, consumerModel, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, deps resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + if _, err := resource.FromProvider[sensor.Sensor](deps, sensor.Named("combo")); err != nil { + return nil, fmt.Errorf("consumer could not resolve composite via sensor API: %w", err) + } + if _, err := resource.FromProvider[resource.Resource](deps, generic.Named("combo")); err != nil { + return nil, fmt.Errorf("consumer could not resolve composite via generic API: %w", err) + } + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }) + defer resource.Deregister(generic.API, consumerModel) + + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "combo", API: sensor.API, Model: comboModel}, + // Depend on the composite via an implicit dependency (DependsOn is deprecated). + {Name: "consumer", API: generic.API, Model: consumerModel, ImplicitDependsOn: []string{"combo"}}, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + // If the consumer built, it resolved the composite under both APIs. + _, err := r.ResourceByName(generic.Named("consumer")) + test.That(t, err, test.ShouldBeNil) +} + +// TestCompositeRemoteResource asserts a composite hosted behind a remote resolves to ONE composite on +// the main robot — reachable under each co-equal API (method calls route to the right per-API facade), +// assembled into one api-less handle over the per-API remote sub-clients, and never mistaken for a +// name collision — across a direct remote, a prefixed remote, and a multi-hop remote chain. The graph +// flattens a chain to the immediate hop, so a nested composite lands on main just like a direct one. +func TestCompositeRemoteResource(t *testing.T) { + newRemote := func(t *testing.T, ctx context.Context, logger logging.Logger, name string, model resource.Model) string { + remote := setupLocalRobot(t, ctx, &config.Config{ + Components: []resource.Config{{Name: "combo", API: sensor.API, Model: model}}, + }, logger.Sublogger(name)) + opts, _, addr := robottestutils.CreateBaseOptionsAndListener(t) + test.That(t, remote.StartWeb(ctx, opts), test.ShouldBeNil) + return addr + } + + for _, tc := range []struct { + name string + setup func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot + perAPIName string // remote-qualified name for the per-API lookups on main + apiless []string // every api-less form that must resolve to the one composite + }{ + { + name: "direct", + setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { + addr := newRemote(t, ctx, logger, "remote", model) + return setupLocalRobot(t, ctx, &config.Config{ + Remotes: []config.Remote{{Name: "rem", Address: addr}}, + }, logger.Sublogger("main")) + }, + perAPIName: "rem:combo", + apiless: []string{"rem:combo"}, + }, + { + // A remote resource is cached under its PREFIXED simple name, so combo behind a remote with + // Prefix "p_" is "p_combo" on main; the api-less assembly must look it up under that name. + name: "prefixed", + setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { + addr := newRemote(t, ctx, logger, "remote", model) + return setupLocalRobot(t, ctx, &config.Config{ + Remotes: []config.Remote{{Name: "rem", Address: addr, Prefix: "p_"}}, + }, logger.Sublogger("main")) + }, + perAPIName: "rem:p_combo", + apiless: []string{"rem:p_combo", "p_combo"}, + }, + { + // A chain main -> mid -> leaf: the graph flattens to the immediate hop (midrem), so the + // composite collapses to one just like a direct remote, and every chain name form resolves. + name: "nested", + setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { + leafAddr := newRemote(t, ctx, logger, "leaf", model) + mid := setupLocalRobot(t, ctx, &config.Config{ + Remotes: []config.Remote{{Name: "leafrem", Address: leafAddr}}, + }, logger.Sublogger("mid")) + midOpts, _, midAddr := robottestutils.CreateBaseOptionsAndListener(t) + test.That(t, mid.StartWeb(ctx, midOpts), test.ShouldBeNil) + return setupLocalRobot(t, ctx, &config.Config{ + Remotes: []config.Remote{{Name: "midrem", Address: midAddr}}, + }, logger.Sublogger("main")) + }, + perAPIName: "midrem:combo", + apiless: []string{"combo", "midrem:combo", "midrem:leafrem:combo"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + logger, logs := logging.NewObservedTestLogger(t) + ctx := context.Background() + model := registerComboModel(t, "composite-sensor-remote-"+tc.name, nil) + main := tc.setup(t, ctx, logger, model) + + // Reachable under both co-equal APIs; each API's method call routes to the correct facade. + bySensor, err := main.ResourceByName(sensor.Named(tc.perAPIName)) + test.That(t, err, test.ShouldBeNil) + s, err := resource.AsType[sensor.Sensor](bySensor) + test.That(t, err, test.ShouldBeNil) + readings, err := s.Readings(ctx, nil) + test.That(t, err, test.ShouldBeNil) + test.That(t, readings["reading"], test.ShouldEqual, 7) + + byGeneric, err := main.ResourceByName(resource.NewName(generic.API, tc.perAPIName)) + test.That(t, err, test.ShouldBeNil) + echoed, err := byGeneric.DoCommand(ctx, map[string]interface{}{"ping": "pong"}) + test.That(t, err, test.ShouldBeNil) + test.That(t, echoed["echo"], test.ShouldEqual, "pong") + + // Every api-less form assembles the per-API sub-clients into ONE handle serving every API. + for _, name := range tc.apiless { + composite, err := main.ResourceByName(resource.SimpleName(name)) + test.That(t, err, test.ShouldBeNil) + test.That(t, resource.APIsOf(composite), test.ShouldContain, sensor.API) + test.That(t, resource.APIsOf(composite), test.ShouldContain, generic.API) + cs, err := resource.AsType[sensor.Sensor](composite) + test.That(t, err, test.ShouldBeNil) + cReadings, err := cs.Readings(ctx, nil) + test.That(t, err, test.ShouldBeNil) + test.That(t, cReadings["reading"], test.ShouldEqual, 7) + } + + // The composite's own per-API names must NOT be flagged as a name collision. + test.That(t, logs.FilterMessageSnippet("Resource name collision").Len(), test.ShouldEqual, 0) + }) + } +} + +// TestCompositeRemoteResourceCollisions asserts that genuine collisions involving remote resources are +// detected: a local resource and a remote resource sharing a bare name collide -- even across +// DIFFERENT APIs -- as do two different (unprefixed) remotes exposing the same bare name. +func TestCompositeRemoteResourceCollisions(t *testing.T) { + logger, logs := logging.NewObservedTestLogger(t) + ctx := context.Background() + + // Plain single-API models (NOT composites). + sensorModel := resource.NewModel("acme", "test", "collide-remote-sensor") + resource.RegisterComponent(sensor.API, sensorModel, + resource.Registration[sensor.Sensor, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (sensor.Sensor, error) { + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }) + defer resource.Deregister(sensor.API, sensorModel) + + genericModel := resource.NewModel("acme", "test", "collide-remote-generic") + resource.RegisterComponent(generic.API, genericModel, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }) + defer resource.Deregister(generic.API, genericModel) + + // Remote 1: a generic "gizmo" (will collide cross-API with the main robot's local sensor "gizmo") + // and a sensor "widget" (will collide same-API with remote 2's sensor "widget"). + remote1Cfg := &config.Config{ + Components: []resource.Config{ + {Name: "gizmo", API: generic.API, Model: genericModel}, + {Name: "widget", API: sensor.API, Model: sensorModel}, + }, + } + remote1 := setupLocalRobot(t, ctx, remote1Cfg, logger.Sublogger("remote1")) + opts1, _, addr1 := robottestutils.CreateBaseOptionsAndListener(t) + test.That(t, remote1.StartWeb(ctx, opts1), test.ShouldBeNil) + + remote2Cfg := &config.Config{ + Components: []resource.Config{ + {Name: "widget", API: sensor.API, Model: sensorModel}, + }, + } + remote2 := setupLocalRobot(t, ctx, remote2Cfg, logger.Sublogger("remote2")) + opts2, _, addr2 := robottestutils.CreateBaseOptionsAndListener(t) + test.That(t, remote2.StartWeb(ctx, opts2), test.ShouldBeNil) + + // Main: a local sensor "gizmo" (collides cross-API with remote1's generic gizmo), and two + // UNPREFIXED remotes both exposing sensor "widget" (collide with each other). + mainCfg := &config.Config{ + Components: []resource.Config{ + {Name: "gizmo", API: sensor.API, Model: sensorModel}, + }, + Remotes: []config.Remote{ + {Name: "r1", Address: addr1}, + {Name: "r2", Address: addr2}, + }, + } + main := setupLocalRobot(t, ctx, mainCfg, logger.Sublogger("main")) + + // Both genuine collisions are reported: the cross-API local-vs-remote "gizmo" (missed by a per-API + // check) and the same-API remote-vs-remote "widget". + testutils.WaitForAssertion(t, func(tb testing.TB) { + test.That(tb, logs.FilterMessageSnippet("Resource name collision with a remote resource").Len(), + test.ShouldBeGreaterThanOrEqualTo, 2) + }) + + // The local resource still wins a fully qualified lookup of its own name+API... + localGizmo, err := main.ResourceByName(sensor.Named("gizmo")) + test.That(t, err, test.ShouldBeNil) + test.That(t, localGizmo, test.ShouldNotBeNil) + // ...and the remote resource is still reachable under its own remote-qualified name+API. + remoteGizmo, err := main.ResourceByName(resource.NewName(generic.API, "r1:gizmo")) + test.That(t, err, test.ShouldBeNil) + test.That(t, remoteGizmo, test.ShouldNotBeNil) + // But an api-less lookup of the colliding bare name has no single owner (a local AND a remote of a + // different API claim it), so it is an error -- name uniqueness is preserved. + _, err = main.ResourceByName(resource.SimpleName("gizmo")) + test.That(t, err, test.ShouldNotBeNil) +} + +// gripperServoDevice is the shared impl of an in-process facade composite serving gripper.Gripper and +// servo.Servo (both resource.Actuators; gripper is also resource.Shaped and framesystem.InputEnabled). +// All lifecycle/actuator state lives here, and the per-API facades below embed one *gripperServoDevice, +// so both facades' Stop route to this one Stop and increment the shared counter. It is assembled via +// resource.Compose, so it is stored as a resource.MultiAPIResource wrapper that does not itself satisfy +// those interfaces — an in-process fetch by a specific API must unwrap it to that API's sub, which is +// what these tests exercise. +type gripperServoDevice struct { + resource.Named + stopCount *atomic.Int32 + closeCount *atomic.Int32 +} + +func (d *gripperServoDevice) Close(context.Context) error { + if d.closeCount != nil { + d.closeCount.Add(1) + } + return nil +} + +func (d *gripperServoDevice) Stop(context.Context, map[string]interface{}) error { + if d.stopCount != nil { + d.stopCount.Add(1) + } + return nil +} + +func (d *gripperServoDevice) IsMoving(context.Context) (bool, error) { return false, nil } + +func (d *gripperServoDevice) Geometries(context.Context, map[string]interface{}) ([]spatialmath.Geometry, error) { + return nil, nil +} + +func (d *gripperServoDevice) Kinematics(context.Context) (referenceframe.Model, error) { + return referenceframe.NewSimpleModel("combo"), nil +} + +func (d *gripperServoDevice) CurrentInputs(context.Context) ([]referenceframe.Input, error) { + return nil, nil +} + +func (d *gripperServoDevice) GoToInputs(context.Context, ...[]referenceframe.Input) error { return nil } + +type gripperFacade struct{ *gripperServoDevice } + +func (f gripperFacade) Open(context.Context, map[string]interface{}) error { return nil } + +func (f gripperFacade) Grab(context.Context, map[string]interface{}) (bool, error) { return false, nil } + +func (f gripperFacade) IsHoldingSomething( + context.Context, map[string]interface{}, +) (gripper.HoldingStatus, error) { + return gripper.HoldingStatus{}, nil +} + +type servoFacade struct{ *gripperServoDevice } + +func (f servoFacade) Move(context.Context, uint32, map[string]interface{}) error { return nil } + +func (f servoFacade) Position(context.Context, map[string]interface{}) (uint32, error) { return 0, nil } + +// registerGripperServoModel registers a builtin gripper+servo colliding composite whose constructor +// assembles the two facades over one shared *gripperServoDevice via resource.Compose (so it is stored as a +// resource.MultiAPIResource wrapper). gripper sorts before servo, so gripper is the canonical API. +func registerGripperServoModel(t *testing.T, name string, stopCount, closeCount *atomic.Int32) resource.Model { + t.Helper() + model := resource.NewModel("acme", "test", name) + resource.RegisterMultiAPI( + []resource.API{gripper.API, servo.API}, model, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + d := &gripperServoDevice{Named: conf.ResourceName().AsNamed(), stopCount: stopCount, closeCount: closeCount} + return resource.Compose( + conf.ResourceName(), + gripper.AsSub(gripperFacade{d}), + servo.AsSub(servoFacade{d}), + ) + }, + }, + ) + t.Cleanup(func() { + resource.Deregister(gripper.API, model) + resource.Deregister(servo.API, model) + }) + return model +} + +// TestModularCompositeInProcessTypedLookup checks in-process typed lookup of a facade composite: a +// composite stored as a resource.MultiAPIResource wrapper, fetched by a SPECIFIC API through the local +// (in-process) robot, yields the concrete typed sub-resource — so .FromProvider(localRobot, +// name) returns a working typed client for each co-equal API. An api-less lookup still resolves to the +// full composite. +func TestModularCompositeInProcessTypedLookup(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + model := registerGripperServoModel(t, "inproc-typed", nil, nil) + + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "combo", API: gripper.API, Model: model}, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + // In-process fetch by each specific API returns a working typed client. + g, err := gripper.FromProvider(r, "combo") + test.That(t, err, test.ShouldBeNil) + test.That(t, g.Open(ctx, nil), test.ShouldBeNil) + + s, err := servo.FromProvider(r, "combo") + test.That(t, err, test.ShouldBeNil) + _, err = s.Position(ctx, nil) + test.That(t, err, test.ShouldBeNil) + + // A specific-API ResourceByName returns the concrete sub (passes a bare type assertion), while the + // api-less handle stays the composite serving every API. + byGripper, err := r.ResourceByName(gripper.Named("combo")) + test.That(t, err, test.ShouldBeNil) + _, isGripper := byGripper.(gripper.Gripper) + test.That(t, isGripper, test.ShouldBeTrue) + + one, err := r.ResourceByName(resource.SimpleName("combo")) + test.That(t, err, test.ShouldBeNil) + apisOf := resource.APIsOf(one) + test.That(t, apisOf, test.ShouldContain, gripper.API) + test.That(t, apisOf, test.ShouldContain, servo.API) +} + +// TestCompositeStopAllInProcess checks that StopAll stops a modular/facade composite and does so +// exactly once: the composite serves two actuator APIs over one underlying instance, and StopAll's +// dedup stops that instance a single time, not once per advertised API. +func TestCompositeStopAllInProcess(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + var stopCount atomic.Int32 + model := registerGripperServoModel(t, "inproc-stopall", &stopCount, nil) + + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "combo", API: gripper.API, Model: model}, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + test.That(t, r.StopAll(ctx, nil), test.ShouldBeNil) + // Stopped AT ALL (safety) and exactly ONCE (dedup), not once per advertised API. + test.That(t, stopCount.Load(), test.ShouldEqual, 1) +} + +// TestCompositeInFrameSystem checks that a kinematic composite (canonical API gripper) is included in +// the frame system via its kinematic sub: the gripper sub is framesystem.InputEnabled, so the +// composite is added to the frame system with a model frame. +func TestCompositeInFrameSystem(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + model := registerGripperServoModel(t, "inproc-fs", nil, nil) + + cfg := &config.Config{ + Components: []resource.Config{ + { + Name: "combo", + API: gripper.API, + Model: model, + Frame: &referenceframe.LinkConfig{Parent: referenceframe.World}, + }, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + fsCfg, err := r.FrameSystemConfig(ctx) + test.That(t, err, test.ShouldBeNil) + var found bool + for _, part := range fsCfg.Parts { + if part.FrameConfig != nil && part.FrameConfig.Name() == "combo" { + found = true + // Included via the InputEnabled (kinematic) path, so it carries a model. + test.That(t, part.ModelFrame, test.ShouldNotBeNil) + } + } + test.That(t, found, test.ShouldBeTrue) +} From d38f81933828af505d2f66346424a3e3ad665158 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Wed, 30 Sep 2026 14:52:08 -0400 Subject: [PATCH 03/14] RSDK-14329: re-index composite co-equal APIs after in-place model reconfigure An in-place model rebuild swaps a new resource onto the existing graph node without passing through Set/UpdateSimpleName, so the compositeByAPI index was never refreshed. Growing a composite's served API set left the new co-equal API unresolvable (FindBySimpleNameAndAPI 404), and shrinking it left a dropped API resolving to a node that no longer serves it. Add Graph.ReindexComposite and call it after the in-place SwapResource in the resource manager. Covered by a graph unit test (grow+shrink) and a robot e2e that reconfigures a resource from a single-API model to a composite model and asserts the newly-served API resolves. Co-Authored-By: Claude Opus 4.8 (1M context) (cherry picked from commit d9d62d8cd409fa44df3d70bb0b24c7d1d63d4dc8) --- robot/impl/local_robot.go | 87 ++++---------- robot/impl/multiapi_composite_test.go | 166 ++++++++------------------ robot/impl/resource_manager.go | 87 ++++++++++++-- 3 files changed, 155 insertions(+), 185 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index c136af90344..ae6d5345f85 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -244,24 +244,11 @@ func (r *localRobot) resourceBySimpleName(name string) (resource.Resource, error case 0: return nil, resource.NewNotFoundError(resource.SimpleName(name)) case 1: - return r.resolveSimpleNameMatch(name, matches[0]) - default: - return nil, errors.Errorf( - "multiple resources share the simple name %q across distinct APIs (%s); look it up by its fully qualified name instead", - name, resource.NamesToStrings(matches), - ) - } -} - -// resolveSimpleNameMatch turns the single simple-name match into a resource handle. A local composite -// already lives in one resource.MultiAPIResource graph node, so a direct lookup returns the whole -// handle. A remote composite instead lives as one per-API sub-client node per co-equal API, so -// assemble a resource.MultiAPIResource view over those sub-clients. -func (r *localRobot) resolveSimpleNameMatch(name string, match resource.Name) (resource.Resource, error) { - if match.Remote == "" { // Resolve the raw graph node (not the localRobot FindBySimpleNameAndAPI, which unwraps a - // composite to one API's sub) so the full multi-API handle is returned. - n, err := r.manager.resources.FindBySimpleNameAndAPI(match.Name, match.API) + // composite to one API's sub) so the full multi-API handle is returned. The lookup uses `name`, + // the cached name with a remote's prefix intact. + match := matches[0] + n, err := r.manager.resources.FindBySimpleNameAndAPI(name, match.API) if err != nil { return nil, err } @@ -270,24 +257,12 @@ func (r *localRobot) resolveSimpleNameMatch(name string, match resource.Name) (r return nil, resource.NewNotAvailableError(match, err) } return res, nil + default: + return nil, errors.Errorf( + "multiple resources share the simple name %q across distinct APIs (%s); look it up by its fully qualified name instead", + name, resource.NamesToStrings(matches), + ) } - // The graph caches a remote resource under its prefixed name, so look it up by `name` (which keeps - // the prefix) rather than match.Name (which FindBySimpleName stripped). - apis := r.manager.resources.APIsForRemoteResource(name, match.Remote) - if len(apis) <= 1 { - return r.FindBySimpleNameAndAPI(name, match.API) - } - byAPI := make(map[resource.API]resource.Resource, len(apis)) - for _, api := range apis { - sub, err := r.FindBySimpleNameAndAPI(name, api) - if err != nil { - return nil, err - } - byAPI[api] = sub - } - return resource.NewMultiAPIResource( - resource.Name{API: apis[0], Remote: match.Remote, Name: match.Name}, apis, byAPI, - ), nil } // RemoteNames returns the names of all known remote robots. @@ -898,12 +873,14 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( if err != nil { return nil, nil, &resource.DependencyNotReadyError{Name: dep.Name, Reason: err} } - allDeps[prefixedName] = res - // A composite dependency serves several co-equal APIs from one identity; key it under each of - // its API names so a dependent can resolve it by whichever API it expects. - for _, api := range r.coequalAPIsOf(prefixedName, res) { + allDeps[prefixedName] = resource.SubresourceForAPI(res, prefixedName.API) + // A composite dependency serves several co-equal APIs from one identity; key it under each of its + // API names, storing that API's unwrapped sub, so a dependent resolves it by whichever API it + // expects and an API-qualified name yields that API's handle directly. Unwrap is a no-op for an + // ordinary resource. + for _, api := range resource.APIsOf(res) { aliased := resource.Name{API: api, Remote: prefixedName.Remote, Name: prefixedName.Name} - allDeps[aliased] = res + allDeps[aliased] = resource.SubresourceForAPI(res, api) } } nodeConf := gNode.Config() @@ -931,22 +908,6 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( return allDeps, weakAndOptionalDepsSnapshot, nil } -// coequalAPIsOf returns every API a composite dependency serves, or nil for an ordinary resource. -// A modular composite is stored as a resource.MultiAPIResource, so APIsOf reports its full set; a -// builtin composite is stored as its raw multi-API instance, whose set is recovered from the backing -// graph node's model. -func (r *localRobot) coequalAPIsOf(name resource.Name, res resource.Resource) []resource.API { - if apis := resource.APIsOf(res); len(apis) > 1 { - return apis - } - if node, err := r.manager.resources.FindBySimpleNameAndAPI(name.Name, name.API); err == nil { - if apis := resource.APIsForModel(node.ResourceModel()); len(apis) > 1 { - return apis - } - } - return nil -} - func (r *localRobot) getWeakDependencyMatchers(api resource.API, model resource.Model) []resource.Matcher { reg, ok := resource.LookupRegistration(api, model) if !ok { @@ -1094,15 +1055,12 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( continue } // A composite serves several co-equal APIs from one identity, so test the weak-dep matchers - // against each served API and store a match as that API's unwrapped sub under that API's name. - // Otherwise a composite matched on a non-canonical API is dropped (the Subtype/Type matchers read - // Name().API, which is only the canonical API) and the consumer gets the wrapper, not the typed - // sub. For an ordinary resource this is n.API and a no-op unwrap. - apis := r.coequalAPIsOf(n, res) - if apis == nil { - apis = []resource.API{n.API} - } - for _, api := range apis { + // against each served API (the Subtype/Type matchers read Name().API, which is only the canonical + // API, so a composite matched on a non-canonical API would otherwise be dropped). Store that API's + // unwrapped sub under each matched API's name, so an API-qualified name yields that API's handle + // directly -- matching explicit deps above and robot resolution. An ordinary resource is a + // composite of one: APIsOf returns its single API and the unwrap is a no-op. + for _, api := range resource.APIsOf(res) { sub := resource.SubresourceForAPI(res, api) apiName := resource.Name{API: api, Remote: n.Remote, Name: n.Name} for _, matcher := range weakDepMatchers { @@ -2442,6 +2400,7 @@ func (r *localRobot) MachineStatus(ctx context.Context) (robot.MachineStatus, er // we can safely ignore errors from `r.CloudMetadata`. If there is an error, that means // that this robot does not have CloudMetadata to attach to resources. md, _ := r.CloudMetadata(ctx) //nolint:errcheck + for _, resourceStatus := range r.manager.resources.Status() { // if the resource is local, we can use the status as is and attach the cloud metadata of this robot. if !resourceStatus.Name.ContainsRemoteNames() && resourceStatus.Name.API != client.RemoteAPI { diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go index c8deb61c66d..216556ca8c4 100644 --- a/robot/impl/multiapi_composite_test.go +++ b/robot/impl/multiapi_composite_test.go @@ -17,7 +17,6 @@ import ( "go.viam.com/rdk/logging" "go.viam.com/rdk/referenceframe" "go.viam.com/rdk/resource" - "go.viam.com/rdk/robot" "go.viam.com/rdk/spatialmath" "go.viam.com/rdk/testutils/robottestutils" ) @@ -90,19 +89,26 @@ func TestCompositeResourceEndToEnd(t *testing.T) { test.That(t, err, test.ShouldBeNil) test.That(t, readings["reading"], test.ShouldEqual, 7) - // 2) A lookup by EACH co-equal API returns the SAME single instance as the api-less lookup. + // 2) An API-qualified lookup returns that API's handle directly (the unwrapped sub, not the + // wrapper), usable as that API with no further unwrapping. bySensor, err := r.ResourceByName(sensor.Named("combo")) test.That(t, err, test.ShouldBeNil) - test.That(t, bySensor, test.ShouldEqual, res) + s2, ok := bySensor.(sensor.Sensor) + test.That(t, ok, test.ShouldBeTrue) + readings, err = s2.Readings(ctx, nil) + test.That(t, err, test.ShouldBeNil) + test.That(t, readings["reading"], test.ShouldEqual, 7) byGeneric, err := r.ResourceByName(resource.NewName(generic.API, "combo")) test.That(t, err, test.ShouldBeNil) - test.That(t, byGeneric, test.ShouldEqual, res) + echoed, err := byGeneric.DoCommand(ctx, map[string]interface{}{"ping": "pong"}) + test.That(t, err, test.ShouldBeNil) + test.That(t, echoed["echo"], test.ShouldEqual, "pong") - // 3) In-process the api-less handle is the one raw instance, which natively implements every - // co-equal API — AsType extracts each. (APIsOf on a raw builtin instance reports only its single - // Name API; the modular composite handle, a resource.MultiAPIResource, reports the full set — see - // TestModularCompositeResource.) + // 3) The api-less handle is the composite wrapper, uniform with a modular composite: APIsOf reports + // the full co-equal set (not just the config API) and AsType extracts each served API. + test.That(t, resource.APIsOf(res), test.ShouldContain, sensor.API) + test.That(t, resource.APIsOf(res), test.ShouldContain, generic.API) _, err = resource.AsType[sensor.Sensor](res) test.That(t, err, test.ShouldBeNil) _, err = resource.AsType[resource.Resource](res) @@ -124,6 +130,45 @@ func TestCompositeResourceEndToEnd(t *testing.T) { test.That(t, closeCount.Load(), test.ShouldEqual, 1) } +// TestCompositeModelReconfigureReindexes reconfigures a resource from a single-API model to a composite +// model serving an extra co-equal API (same name and config API). A model change rebuilds the resource +// in place on the same graph node, which does not pass through the graph's cache-write paths, so the +// manager must refresh the composite co-equal index afterward. Without that, the newly-served API would +// not resolve even though the resource now serves it. +func TestCompositeModelReconfigureReindexes(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + + // singleModel serves only sensor; multiModel serves sensor + generic from the same impl. + singleModel := resource.NewModel("acme", "test", "recfg-single") + resource.Register(sensor.API, singleModel, resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, _ resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }) + defer resource.Deregister(sensor.API, singleModel) + multiModel := registerComboModel(t, "recfg-multi", nil) + + // Start as a plain sensor: the generic API does not resolve. + r := setupLocalRobot(t, ctx, &config.Config{ + Components: []resource.Config{{Name: "combo", API: sensor.API, Model: singleModel}}, + }, logger) + _, err := r.ResourceByName(generic.Named("combo")) + test.That(t, err, test.ShouldNotBeNil) + + // Reconfigure to the composite model (in-place rebuild on the same node). The generic API must now + // resolve, and the configured sensor API must still resolve. + r.Reconfigure(ctx, &config.Config{ + Components: []resource.Config{{Name: "combo", API: sensor.API, Model: multiModel}}, + }) + _, err = r.ResourceByName(generic.Named("combo")) + test.That(t, err, test.ShouldBeNil) + _, err = r.ResourceByName(sensor.Named("combo")) + test.That(t, err, test.ShouldBeNil) +} + // TestCompositeDependencyResolvesAllAPIs asserts that a second resource depending on a composite can // resolve it under each of the composite's co-equal API names (the composite dependency is keyed // under each API in the dependent's Dependencies). @@ -164,110 +209,6 @@ func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { test.That(t, err, test.ShouldBeNil) } -// TestCompositeRemoteResource asserts a composite hosted behind a remote resolves to ONE composite on -// the main robot — reachable under each co-equal API (method calls route to the right per-API facade), -// assembled into one api-less handle over the per-API remote sub-clients, and never mistaken for a -// name collision — across a direct remote, a prefixed remote, and a multi-hop remote chain. The graph -// flattens a chain to the immediate hop, so a nested composite lands on main just like a direct one. -func TestCompositeRemoteResource(t *testing.T) { - newRemote := func(t *testing.T, ctx context.Context, logger logging.Logger, name string, model resource.Model) string { - remote := setupLocalRobot(t, ctx, &config.Config{ - Components: []resource.Config{{Name: "combo", API: sensor.API, Model: model}}, - }, logger.Sublogger(name)) - opts, _, addr := robottestutils.CreateBaseOptionsAndListener(t) - test.That(t, remote.StartWeb(ctx, opts), test.ShouldBeNil) - return addr - } - - for _, tc := range []struct { - name string - setup func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot - perAPIName string // remote-qualified name for the per-API lookups on main - apiless []string // every api-less form that must resolve to the one composite - }{ - { - name: "direct", - setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { - addr := newRemote(t, ctx, logger, "remote", model) - return setupLocalRobot(t, ctx, &config.Config{ - Remotes: []config.Remote{{Name: "rem", Address: addr}}, - }, logger.Sublogger("main")) - }, - perAPIName: "rem:combo", - apiless: []string{"rem:combo"}, - }, - { - // A remote resource is cached under its PREFIXED simple name, so combo behind a remote with - // Prefix "p_" is "p_combo" on main; the api-less assembly must look it up under that name. - name: "prefixed", - setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { - addr := newRemote(t, ctx, logger, "remote", model) - return setupLocalRobot(t, ctx, &config.Config{ - Remotes: []config.Remote{{Name: "rem", Address: addr, Prefix: "p_"}}, - }, logger.Sublogger("main")) - }, - perAPIName: "rem:p_combo", - apiless: []string{"rem:p_combo", "p_combo"}, - }, - { - // A chain main -> mid -> leaf: the graph flattens to the immediate hop (midrem), so the - // composite collapses to one just like a direct remote, and every chain name form resolves. - name: "nested", - setup: func(t *testing.T, ctx context.Context, logger logging.Logger, model resource.Model) robot.LocalRobot { - leafAddr := newRemote(t, ctx, logger, "leaf", model) - mid := setupLocalRobot(t, ctx, &config.Config{ - Remotes: []config.Remote{{Name: "leafrem", Address: leafAddr}}, - }, logger.Sublogger("mid")) - midOpts, _, midAddr := robottestutils.CreateBaseOptionsAndListener(t) - test.That(t, mid.StartWeb(ctx, midOpts), test.ShouldBeNil) - return setupLocalRobot(t, ctx, &config.Config{ - Remotes: []config.Remote{{Name: "midrem", Address: midAddr}}, - }, logger.Sublogger("main")) - }, - perAPIName: "midrem:combo", - apiless: []string{"combo", "midrem:combo", "midrem:leafrem:combo"}, - }, - } { - t.Run(tc.name, func(t *testing.T) { - logger, logs := logging.NewObservedTestLogger(t) - ctx := context.Background() - model := registerComboModel(t, "composite-sensor-remote-"+tc.name, nil) - main := tc.setup(t, ctx, logger, model) - - // Reachable under both co-equal APIs; each API's method call routes to the correct facade. - bySensor, err := main.ResourceByName(sensor.Named(tc.perAPIName)) - test.That(t, err, test.ShouldBeNil) - s, err := resource.AsType[sensor.Sensor](bySensor) - test.That(t, err, test.ShouldBeNil) - readings, err := s.Readings(ctx, nil) - test.That(t, err, test.ShouldBeNil) - test.That(t, readings["reading"], test.ShouldEqual, 7) - - byGeneric, err := main.ResourceByName(resource.NewName(generic.API, tc.perAPIName)) - test.That(t, err, test.ShouldBeNil) - echoed, err := byGeneric.DoCommand(ctx, map[string]interface{}{"ping": "pong"}) - test.That(t, err, test.ShouldBeNil) - test.That(t, echoed["echo"], test.ShouldEqual, "pong") - - // Every api-less form assembles the per-API sub-clients into ONE handle serving every API. - for _, name := range tc.apiless { - composite, err := main.ResourceByName(resource.SimpleName(name)) - test.That(t, err, test.ShouldBeNil) - test.That(t, resource.APIsOf(composite), test.ShouldContain, sensor.API) - test.That(t, resource.APIsOf(composite), test.ShouldContain, generic.API) - cs, err := resource.AsType[sensor.Sensor](composite) - test.That(t, err, test.ShouldBeNil) - cReadings, err := cs.Readings(ctx, nil) - test.That(t, err, test.ShouldBeNil) - test.That(t, cReadings["reading"], test.ShouldEqual, 7) - } - - // The composite's own per-API names must NOT be flagged as a name collision. - test.That(t, logs.FilterMessageSnippet("Resource name collision").Len(), test.ShouldEqual, 0) - }) - } -} - // TestCompositeRemoteResourceCollisions asserts that genuine collisions involving remote resources are // detected: a local resource and a remote resource sharing a bare name collide -- even across // DIFFERENT APIs -- as do two different (unprefixed) remotes exposing the same bare name. @@ -428,7 +369,6 @@ func registerGripperServoModel(t *testing.T, name string, stopCount, closeCount ) (resource.Resource, error) { d := &gripperServoDevice{Named: conf.ResourceName().AsNamed(), stopCount: stopCount, closeCount: closeCount} return resource.Compose( - conf.ResourceName(), gripper.AsSub(gripperFacade{d}), servo.AsSub(servoFacade{d}), ) diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index a767c5dff09..d7518b8273c 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -230,6 +230,34 @@ type internalRemoteRobot interface { MachineState() robot.MachineState } +// servedAPIs returns the co-equal API set a node currently serves: its MultiAPIResource's set if it is +// a composite, else the single fallback API. Used to detect when a remote node's composite membership +// changed across a refresh. +func servedAPIs(n *resource.GraphNode, fallback resource.API) []resource.API { + if res, err := n.UnsafeResource(); err == nil { + if mar, ok := res.(resource.MultiAPIResource); ok { + return mar.APIs() + } + } + return []resource.API{fallback} +} + +func sameAPISet(a, b []resource.API) bool { + if len(a) != len(b) { + return false + } + set := make(map[resource.API]bool, len(a)) + for _, x := range a { + set[x] = true + } + for _, y := range b { + if !set[y] { + return false + } + } + return true +} + // updateRemoteResourceNames is called when the Remote robot has changed (either connection or disconnection). // It will pull the current remote resources and update the resource tree adding or removing nodes accordingly. // The recreateAllClients flag will re-add all remote resource nodes if true and only new / uninitialized @@ -291,15 +319,41 @@ func (manager *resourceManager) updateRemoteResourceNames( anythingChanged := false + // Group the remote's advertised names by bare name to detect composites: on a single remote names + // are unique, so a bare name under >=2 APIs is one composite (stored as a single node, like a local one). + remoteAPIsByName := map[string][]resource.API{} + for _, rn := range newResources { + if rn.Name == resource.DefaultServiceName { + continue + } + remoteAPIsByName[rn.Name] = append(remoteAPIsByName[rn.Name], rn.API) + } + for k := range remoteAPIsByName { + slices.SortFunc(remoteAPIsByName[k], func(a, b resource.API) int { return strings.Compare(a.String(), b.String()) }) + } + for _, resName := range newResources { // Do not store any default remote resources in our resource graph. if resName.Name == resource.DefaultServiceName { continue } + // Keep a composite as one node under its canonical (sorted-first) API; its other per-API names + // resolve to that node through the compositeByAPI index, so skip them here. + coequalAPIs := remoteAPIsByName[resName.Name] + isComposite := len(coequalAPIs) >= 2 + if isComposite && resName.API != coequalAPIs[0] { + continue + } + remoteResName := resName resLogger := logger.WithFields("resource", remoteResName) - res, err := rr.ResourceByName(remoteResName) // this returns a remote known OR foreign resource client + // For a composite, fetch the api-less wrapped MultiAPIResource; otherwise the per-API client. + lookup := remoteResName + if isComposite { + lookup = resource.Name{Name: remoteResName.Name} + } + res, err := rr.ResourceByName(lookup) // this returns a remote known OR foreign resource client if err != nil { // The connection may have dropped between ResourceNames() above and this call. If so // the remote's resources aren't gone, the fetch just failed, so we don't actually want @@ -327,13 +381,22 @@ func (manager *resourceManager) updateRemoteResourceNames( manager.resources.UpdateNodePrefix(resName, prefix) } + // An existing node whose composite membership changed (gained/lost a co-equal API, or + // more rarely flipped single<->composite) must be rebuilt even on a ticker pass so the one composite node + // reflects the remote's current API set. + desiredAPIs := []resource.API{resName.API} + if isComposite { + desiredAPIs = coequalAPIs + } + compositeChanged := nodeAlreadyExists && !sameAPISet(servedAPIs(gNode, resName.API), desiredAPIs) + if _, alreadyCurrent := activeResourceNames[resName]; alreadyCurrent { activeResourceNames[resName] = true if nodeAlreadyExists && !gNode.IsUninitialized() { // resources that enter this block represent those with names that already exist in the resource graph. // it is possible that we are switching to a new remote with a identical resource name(s), so we may // need to create these resource clients. - if !recreateAllClients { + if !recreateAllClients && !compositeChanged { // ticker event, likely no changes to remote resources, skip closing and re-adding duplicate name resource clients continue } @@ -354,12 +417,11 @@ func (manager *resourceManager) updateRemoteResourceNames( } if !nodeAlreadyExists { - // Log if this remote resource's name collides with any existing resource. On collision - // the remote stays in the graph but is hidden from the machine's resource list. One - // FindAllBySimpleName scan spans all APIs, so it catches both same-API and cross-API - // collisions. A composite living on this remote is advertised as several same-named - // resources that differ only by API; those are one composite (names are unique per remote), - // not a collision, so matches from THIS remote are skipped. + // Log if this remote resource's name collides with a different resource. On collision the + // remote stays in the graph but is hidden from the machine's resource list. FindAllBySimpleName + // scans all APIs, so it catches same-API and cross-API collisions. Matches from this remote are + // skipped: a remote's own names are unique, so a same-name match on the same remote is this + // resource itself (a composite matches under each of its co-equal APIs), not a real collision. prefixedSimpleName := prefix + resName.Name var conflicts []resource.Name for _, match := range manager.resources.FindAllBySimpleName(prefixedSimpleName) { @@ -392,6 +454,11 @@ func (manager *resourceManager) updateRemoteResourceNames( // The clock advance is what lets a local resource with a weak/optional dependency on this // remote resource be detected as stale by updateWeakAndOptionalDependents. gNode.SwapResource(res, unknownModel, manager.opts.ftdc, true) + if compositeChanged { + // An in-place swap keeps the same node and bypasses the graph's cache-write paths, so + // refresh the composite co-equal index to match the node's new API set. + manager.resources.ReindexComposite(resName) + } err = manager.resources.AddChild(resName, remoteName) if err != nil { @@ -982,6 +1049,10 @@ func (manager *resourceManager) completeConfig( "error", ctxWithTimeout.Err()) } else { gNode.SwapResource(newRes, conf.Model, manager.opts.ftdc, true) + // An in-place rebuild keeps the same node and does not pass through the graph's + // cache-write paths, so refresh the composite co-equal index in case the model's + // served API set changed. No-op for ordinary resources. + manager.resources.ReindexComposite(resName) buildDuration := time.Since(activityStarted) manager.logger.Activity(activityType, "complete", "resource", resName.String(), "model", conf.Model.String(), "revision", activityRevision, From d2dc875e072c100a530391ed9e63b4ec978a1240 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Wed, 14 Oct 2026 16:17:46 -0400 Subject: [PATCH 04/14] robot: tighten composite doc comments Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 42 +++++++++++++++++---------------------- robot/robot.go | 8 +++----- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index ae6d5345f85..955bf65ff2b 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -205,12 +205,11 @@ func (r *localRobot) UploadDataFromPath(ctx context.Context, path string, md *da return uploader.UploadDataFromPath(ctx, path, md, extra) } -// FindBySimpleNameAndAPI finds a resource by its simple name and API — the specific-API lookup -// queried through the resourceGetterForAPI for _all_ incoming gRPC requests related to a resource. It -// returns a nil resource and an error when no resource, or multiple matching remote resources, are -// found. It unwraps a composite to the sub-resource serving that API (resource.SubresourceForAPI), so -// an in-process consumer that fetches by a concrete API and type-asserts sees the real typed instance, -// not the composite wrapper, which satisfies none of the API interfaces. +// FindBySimpleNameAndAPI finds a resource by its simple name and API. This is queried through the +// resourceGetterForAPI for _all_ incoming gRPC requests related to a resource. It returns a nil resource +// and an error when no resource, or multiple matching remote resources, are found. It unwraps a composite +// to the sub-resource serving that API (resource.SubresourceForAPI), so a caller that type-asserts sees +// the real typed instance rather than the composite wrapper. func (r *localRobot) FindBySimpleNameAndAPI(name string, api resource.API) (resource.Resource, error) { n, err := r.manager.resources.FindBySimpleNameAndAPI(name, api) if err != nil { @@ -224,9 +223,8 @@ func (r *localRobot) FindBySimpleNameAndAPI(name string, api resource.API) (reso } // ResourceByName returns a resource by name. It routes an api-specific name to FindBySimpleNameAndAPI -// (which unwraps a composite to the sub-resource serving that API) and a bare, API-less name to -// resourceBySimpleName (which for a composite returns the single handle serving every API). -// ResourceByName is only called internally, for some dependency calculation and session code. +// and a simple, API-less name to resourceBySimpleName. It is only called internally, for some +// dependency calculation and session code. func (r *localRobot) ResourceByName(name resource.Name) (resource.Resource, error) { if name.API == (resource.API{}) { return r.resourceBySimpleName(name.Name) @@ -234,10 +232,10 @@ func (r *localRobot) ResourceByName(name resource.Name) (resource.Resource, erro return r.FindBySimpleNameAndAPI(name.Name, name.API) } -// resourceBySimpleName resolves a bare (API-less) name to its single resource. A composite is served -// under several co-equal APIs but is one identity, so FindBySimpleName dedups it (local or remote) to -// a single match and this returns the one handle serving every API. A genuine same-name collision -// across distinct resources returns several matches and is an error. +// resourceBySimpleName resolves a simple (API-less) name to its single resource. A composite serves +// several co-equal APIs but is one identity, so FindBySimpleName collapses it (local or remote) to one +// match and this returns that single multi-API handle. A genuine same-name collision across distinct +// resources returns several matches and is an error. func (r *localRobot) resourceBySimpleName(name string) (resource.Resource, error) { matches := r.manager.resources.FindAllBySimpleName(name) switch len(matches) { @@ -874,10 +872,9 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( return nil, nil, &resource.DependencyNotReadyError{Name: dep.Name, Reason: err} } allDeps[prefixedName] = resource.SubresourceForAPI(res, prefixedName.API) - // A composite dependency serves several co-equal APIs from one identity; key it under each of its - // API names, storing that API's unwrapped sub, so a dependent resolves it by whichever API it - // expects and an API-qualified name yields that API's handle directly. Unwrap is a no-op for an - // ordinary resource. + // A composite dependency serves several co-equal APIs, so key it under each of its API names with + // that API's unwrapped sub; a dependent then resolves it by whichever API it expects. Unwrap is a + // no-op for an ordinary resource. for _, api := range resource.APIsOf(res) { aliased := resource.Name{API: api, Remote: prefixedName.Remote, Name: prefixedName.Name} allDeps[aliased] = resource.SubresourceForAPI(res, api) @@ -1054,12 +1051,10 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( } continue } - // A composite serves several co-equal APIs from one identity, so test the weak-dep matchers - // against each served API (the Subtype/Type matchers read Name().API, which is only the canonical - // API, so a composite matched on a non-canonical API would otherwise be dropped). Store that API's - // unwrapped sub under each matched API's name, so an API-qualified name yields that API's handle - // directly -- matching explicit deps above and robot resolution. An ordinary resource is a - // composite of one: APIsOf returns its single API and the unwrap is a no-op. + // A composite serves several co-equal APIs, so match the weak-dep matchers against each API it + // serves rather than only its canonical Name().API. For each matched API, store its unwrapped sub + // under that API's name so an API-qualified lookup resolves directly. An ordinary resource serves + // one API, so the unwrap is a no-op. for _, api := range resource.APIsOf(res) { sub := resource.SubresourceForAPI(res, api) apiName := resource.Name{API: api, Remote: n.Remote, Name: n.Name} @@ -2400,7 +2395,6 @@ func (r *localRobot) MachineStatus(ctx context.Context) (robot.MachineStatus, er // we can safely ignore errors from `r.CloudMetadata`. If there is an error, that means // that this robot does not have CloudMetadata to attach to resources. md, _ := r.CloudMetadata(ctx) //nolint:errcheck - for _, resourceStatus := range r.manager.resources.Status() { // if the resource is local, we can use the status as is and attach the cloud metadata of this robot. if !resourceStatus.Name.ContainsRemoteNames() && resourceStatus.Name.API != client.RemoteAPI { diff --git a/robot/robot.go b/robot/robot.go index e1450e34767..becbdcd07fe 100644 --- a/robot/robot.go +++ b/robot/robot.go @@ -201,9 +201,8 @@ type LocalRobot interface { Kill() // FindBySimpleNameAndAPI returns a resource from the resource graph, unwrapping a composite to the - // sub-resource serving the given API so a caller that type-asserts to the API's interface sees the - // real typed instance. See [resource.Graph.FindBySimpleNameAndAPI] for specifics about what is - // returned in the case of name collisions. + // sub-resource serving the given API. See [resource.Graph.FindBySimpleNameAndAPI] for behavior on + // name collisions. FindBySimpleNameAndAPI(string, resource.API) (resource.Resource, error) // WriteTraceMessages writes trace spans to any configured exporters. @@ -324,8 +323,7 @@ func ResourceFromProtoMessage( return res, fqName, nil } -// ResourceFromRobot returns a resource from a robot. It uses resource.AsType so that a composite -// (multi-API) resource handle is unwrapped to the sub-resource satisfying T. +// ResourceFromRobot returns a resource from a robot. func ResourceFromRobot[T resource.Resource](robot Robot, name resource.Name) (T, error) { var zero T res, err := robot.ResourceByName(name) From f8e07d843a9dd9c2b0a935f70b0a49bae3b8efb2 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Wed, 14 Oct 2026 16:17:46 -0400 Subject: [PATCH 05/14] robot/impl: comment-style audit of composite code; relocate frame-system test to p3e-consumers; rename Modular->Composite in-process test Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/multiapi_composite_test.go | 93 ++++++++------------------- robot/impl/resource_manager.go | 20 +++--- 2 files changed, 37 insertions(+), 76 deletions(-) diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go index 216556ca8c4..92090f02bdc 100644 --- a/robot/impl/multiapi_composite_test.go +++ b/robot/impl/multiapi_composite_test.go @@ -32,8 +32,8 @@ func (c *comboSensor) Readings(context.Context, map[string]interface{}) (map[str } // DoCommand echoes cmd["ping"] back under "echo". It gives the composite's non-sensor (generic) -// facade a method with an observable result, so a call can be proven to route to the correct per-API -// facade of the one composite (rather than to Readings). +// implementation a method with an observable result, so a call can be proven to route to the correct +// per-API implementation of the one composite (rather than to Readings). func (c *comboSensor) DoCommand(_ context.Context, cmd map[string]interface{}) (map[string]interface{}, error) { return map[string]interface{}{"echo": cmd["ping"]}, nil } @@ -105,7 +105,7 @@ func TestCompositeResourceEndToEnd(t *testing.T) { test.That(t, err, test.ShouldBeNil) test.That(t, echoed["echo"], test.ShouldEqual, "pong") - // 3) The api-less handle is the composite wrapper, uniform with a modular composite: APIsOf reports + // 3) The api-less handle is the composite wrapper, uniform with a modular composite. APIsOf reports // the full co-equal set (not just the config API) and AsType extracts each served API. test.That(t, resource.APIsOf(res), test.ShouldContain, sensor.API) test.That(t, resource.APIsOf(res), test.ShouldContain, generic.API) @@ -151,7 +151,7 @@ func TestCompositeModelReconfigureReindexes(t *testing.T) { defer resource.Deregister(sensor.API, singleModel) multiModel := registerComboModel(t, "recfg-multi", nil) - // Start as a plain sensor: the generic API does not resolve. + // Start as a plain sensor, so the generic API does not resolve. r := setupLocalRobot(t, ctx, &config.Config{ Components: []resource.Config{{Name: "combo", API: sensor.API, Model: singleModel}}, }, logger) @@ -210,13 +210,13 @@ func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { } // TestCompositeRemoteResourceCollisions asserts that genuine collisions involving remote resources are -// detected: a local resource and a remote resource sharing a bare name collide -- even across -// DIFFERENT APIs -- as do two different (unprefixed) remotes exposing the same bare name. +// detected: a local resource and a remote resource sharing a simple name collide, even across different +// APIs, as do two different (unprefixed) remotes exposing the same simple name. func TestCompositeRemoteResourceCollisions(t *testing.T) { logger, logs := logging.NewObservedTestLogger(t) ctx := context.Background() - // Plain single-API models (NOT composites). + // Plain single-API models. sensorModel := resource.NewModel("acme", "test", "collide-remote-sensor") resource.RegisterComponent(sensor.API, sensorModel, resource.Registration[sensor.Sensor, resource.NoNativeConfig]{ @@ -260,8 +260,8 @@ func TestCompositeRemoteResourceCollisions(t *testing.T) { opts2, _, addr2 := robottestutils.CreateBaseOptionsAndListener(t) test.That(t, remote2.StartWeb(ctx, opts2), test.ShouldBeNil) - // Main: a local sensor "gizmo" (collides cross-API with remote1's generic gizmo), and two - // UNPREFIXED remotes both exposing sensor "widget" (collide with each other). + // The main robot has a local sensor "gizmo" (collides cross-API with remote1's generic gizmo), and + // two unprefixed remotes both exposing sensor "widget" (collide with each other). mainCfg := &config.Config{ Components: []resource.Config{ {Name: "gizmo", API: sensor.API, Model: sensorModel}, @@ -273,8 +273,8 @@ func TestCompositeRemoteResourceCollisions(t *testing.T) { } main := setupLocalRobot(t, ctx, mainCfg, logger.Sublogger("main")) - // Both genuine collisions are reported: the cross-API local-vs-remote "gizmo" (missed by a per-API - // check) and the same-API remote-vs-remote "widget". + // Both genuine collisions are reported. The cross-API local-vs-remote "gizmo" (missed by a per-API + // check) and the same-API remote-vs-remote "widget" both collide. testutils.WaitForAssertion(t, func(tb testing.TB) { test.That(tb, logs.FilterMessageSnippet("Resource name collision with a remote resource").Len(), test.ShouldBeGreaterThanOrEqualTo, 2) @@ -288,19 +288,15 @@ func TestCompositeRemoteResourceCollisions(t *testing.T) { remoteGizmo, err := main.ResourceByName(resource.NewName(generic.API, "r1:gizmo")) test.That(t, err, test.ShouldBeNil) test.That(t, remoteGizmo, test.ShouldNotBeNil) - // But an api-less lookup of the colliding bare name has no single owner (a local AND a remote of a - // different API claim it), so it is an error -- name uniqueness is preserved. + // But an api-less lookup of the colliding simple name has no single owner (a local and a remote of a + // different API claim it), so it is an error, preserving name uniqueness. _, err = main.ResourceByName(resource.SimpleName("gizmo")) test.That(t, err, test.ShouldNotBeNil) } -// gripperServoDevice is the shared impl of an in-process facade composite serving gripper.Gripper and -// servo.Servo (both resource.Actuators; gripper is also resource.Shaped and framesystem.InputEnabled). -// All lifecycle/actuator state lives here, and the per-API facades below embed one *gripperServoDevice, -// so both facades' Stop route to this one Stop and increment the shared counter. It is assembled via -// resource.Compose, so it is stored as a resource.MultiAPIResource wrapper that does not itself satisfy -// those interfaces — an in-process fetch by a specific API must unwrap it to that API's sub, which is -// what these tests exercise. +// gripperServoDevice is the shared impl behind an in-process composite serving gripper.Gripper and +// servo.Servo. The per-API implementations below embed one *gripperServoDevice, so both route to this +// one instance (e.g. Stop increments the shared counter). type gripperServoDevice struct { resource.Named stopCount *atomic.Int32 @@ -355,9 +351,9 @@ func (f servoFacade) Move(context.Context, uint32, map[string]interface{}) error func (f servoFacade) Position(context.Context, map[string]interface{}) (uint32, error) { return 0, nil } -// registerGripperServoModel registers a builtin gripper+servo colliding composite whose constructor -// assembles the two facades over one shared *gripperServoDevice via resource.Compose (so it is stored as a -// resource.MultiAPIResource wrapper). gripper sorts before servo, so gripper is the canonical API. +// registerGripperServoModel registers a builtin gripper+servo composite whose constructor composes the +// two implementations over one shared *gripperServoDevice. gripper sorts before servo, so gripper is the +// canonical API. func registerGripperServoModel(t *testing.T, name string, stopCount, closeCount *atomic.Int32) resource.Model { t.Helper() model := resource.NewModel("acme", "test", name) @@ -382,12 +378,10 @@ func registerGripperServoModel(t *testing.T, name string, stopCount, closeCount return model } -// TestModularCompositeInProcessTypedLookup checks in-process typed lookup of a facade composite: a -// composite stored as a resource.MultiAPIResource wrapper, fetched by a SPECIFIC API through the local -// (in-process) robot, yields the concrete typed sub-resource — so .FromProvider(localRobot, -// name) returns a working typed client for each co-equal API. An api-less lookup still resolves to the -// full composite. -func TestModularCompositeInProcessTypedLookup(t *testing.T) { +// TestCompositeInProcessTypedLookup checks that fetching an in-process composite by a specific API +// returns the concrete typed sub, so component.FromProvider(localRobot, name) works for each co-equal +// API, while an api-less lookup still returns the full composite. +func TestCompositeInProcessTypedLookup(t *testing.T) { logger := logging.NewTestLogger(t) ctx := context.Background() model := registerGripperServoModel(t, "inproc-typed", nil, nil) @@ -409,7 +403,7 @@ func TestModularCompositeInProcessTypedLookup(t *testing.T) { _, err = s.Position(ctx, nil) test.That(t, err, test.ShouldBeNil) - // A specific-API ResourceByName returns the concrete sub (passes a bare type assertion), while the + // A specific-API ResourceByName returns the concrete sub (passes a plain type assertion), while the // api-less handle stays the composite serving every API. byGripper, err := r.ResourceByName(gripper.Named("combo")) test.That(t, err, test.ShouldBeNil) @@ -423,9 +417,9 @@ func TestModularCompositeInProcessTypedLookup(t *testing.T) { test.That(t, apisOf, test.ShouldContain, servo.API) } -// TestCompositeStopAllInProcess checks that StopAll stops a modular/facade composite and does so -// exactly once: the composite serves two actuator APIs over one underlying instance, and StopAll's -// dedup stops that instance a single time, not once per advertised API. +// TestCompositeStopAllInProcess checks that StopAll stops a composite exactly once. The composite serves +// two actuator APIs over one instance, and StopAll's dedup stops it a single time, not once per +// advertised API. func TestCompositeStopAllInProcess(t *testing.T) { logger := logging.NewTestLogger(t) ctx := context.Background() @@ -443,36 +437,3 @@ func TestCompositeStopAllInProcess(t *testing.T) { // Stopped AT ALL (safety) and exactly ONCE (dedup), not once per advertised API. test.That(t, stopCount.Load(), test.ShouldEqual, 1) } - -// TestCompositeInFrameSystem checks that a kinematic composite (canonical API gripper) is included in -// the frame system via its kinematic sub: the gripper sub is framesystem.InputEnabled, so the -// composite is added to the frame system with a model frame. -func TestCompositeInFrameSystem(t *testing.T) { - logger := logging.NewTestLogger(t) - ctx := context.Background() - model := registerGripperServoModel(t, "inproc-fs", nil, nil) - - cfg := &config.Config{ - Components: []resource.Config{ - { - Name: "combo", - API: gripper.API, - Model: model, - Frame: &referenceframe.LinkConfig{Parent: referenceframe.World}, - }, - }, - } - r := setupLocalRobot(t, ctx, cfg, logger) - - fsCfg, err := r.FrameSystemConfig(ctx) - test.That(t, err, test.ShouldBeNil) - var found bool - for _, part := range fsCfg.Parts { - if part.FrameConfig != nil && part.FrameConfig.Name() == "combo" { - found = true - // Included via the InputEnabled (kinematic) path, so it carries a model. - test.That(t, part.ModelFrame, test.ShouldNotBeNil) - } - } - test.That(t, found, test.ShouldBeTrue) -} diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index d7518b8273c..720cb92a21a 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -230,9 +230,9 @@ type internalRemoteRobot interface { MachineState() robot.MachineState } -// servedAPIs returns the co-equal API set a node currently serves: its MultiAPIResource's set if it is -// a composite, else the single fallback API. Used to detect when a remote node's composite membership -// changed across a refresh. +// servedAPIs returns the co-equal API set a node currently serves. For a composite that is its +// MultiAPIResource's set, otherwise the single fallback API. Used to detect when a remote node's +// composite membership changed across a refresh. func servedAPIs(n *resource.GraphNode, fallback resource.API) []resource.API { if res, err := n.UnsafeResource(); err == nil { if mar, ok := res.(resource.MultiAPIResource); ok { @@ -319,8 +319,8 @@ func (manager *resourceManager) updateRemoteResourceNames( anythingChanged := false - // Group the remote's advertised names by bare name to detect composites: on a single remote names - // are unique, so a bare name under >=2 APIs is one composite (stored as a single node, like a local one). + // Group the remote's advertised names by simple name to detect composites. On a single remote names + // are unique, so a simple name under >=2 APIs is one composite (stored as a single node, like a local one). remoteAPIsByName := map[string][]resource.API{} for _, rn := range newResources { if rn.Name == resource.DefaultServiceName { @@ -353,7 +353,7 @@ func (manager *resourceManager) updateRemoteResourceNames( if isComposite { lookup = resource.Name{Name: remoteResName.Name} } - res, err := rr.ResourceByName(lookup) // this returns a remote known OR foreign resource client + res, err := rr.ResourceByName(lookup) // this returns a remote known or foreign resource client if err != nil { // The connection may have dropped between ResourceNames() above and this call. If so // the remote's resources aren't gone, the fetch just failed, so we don't actually want @@ -381,9 +381,9 @@ func (manager *resourceManager) updateRemoteResourceNames( manager.resources.UpdateNodePrefix(resName, prefix) } - // An existing node whose composite membership changed (gained/lost a co-equal API, or - // more rarely flipped single<->composite) must be rebuilt even on a ticker pass so the one composite node - // reflects the remote's current API set. + // An existing node whose composite membership changed (gained or lost a co-equal API, or flipped + // single<->composite) must be rebuilt even on a ticker pass, so the one composite node reflects the + // remote's current API set. desiredAPIs := []resource.API{resName.API} if isComposite { desiredAPIs = coequalAPIs @@ -420,7 +420,7 @@ func (manager *resourceManager) updateRemoteResourceNames( // Log if this remote resource's name collides with a different resource. On collision the // remote stays in the graph but is hidden from the machine's resource list. FindAllBySimpleName // scans all APIs, so it catches same-API and cross-API collisions. Matches from this remote are - // skipped: a remote's own names are unique, so a same-name match on the same remote is this + // skipped, because a remote's own names are unique, so a same-name match on the same remote is this // resource itself (a composite matches under each of its co-equal APIs), not a real collision. prefixedSimpleName := prefix + resName.Name var conflicts []resource.Name From d3710f39b8873f6aef2e9b8df2cf07628b9d52ff Mon Sep 17 00:00:00 2001 From: danielbotros Date: Thu, 15 Oct 2026 11:50:44 -0400 Subject: [PATCH 06/14] robot: more comment-style audit on composite code Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 2 +- robot/impl/multiapi_composite_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 955bf65ff2b..f6c6c56abca 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -206,7 +206,7 @@ func (r *localRobot) UploadDataFromPath(ctx context.Context, path string, md *da } // FindBySimpleNameAndAPI finds a resource by its simple name and API. This is queried through the -// resourceGetterForAPI for _all_ incoming gRPC requests related to a resource. It returns a nil resource +// resourceGetterForAPI for all incoming gRPC requests related to a resource. It returns a nil resource // and an error when no resource, or multiple matching remote resources, are found. It unwraps a composite // to the sub-resource serving that API (resource.SubresourceForAPI), so a caller that type-asserts sees // the real typed instance rather than the composite wrapper. diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go index 92090f02bdc..e283bb1d354 100644 --- a/robot/impl/multiapi_composite_test.go +++ b/robot/impl/multiapi_composite_test.go @@ -177,7 +177,7 @@ func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { ctx := context.Background() comboModel := registerComboModel(t, "composite-sensor-dep", nil) - // A consumer that fails construction unless it can resolve the composite under BOTH APIs. + // A consumer that fails construction unless it can resolve the composite under both APIs. consumerModel := resource.NewModel("acme", "test", "composite-consumer") resource.RegisterComponent(generic.API, consumerModel, resource.Registration[resource.Resource, resource.NoNativeConfig]{ @@ -210,7 +210,7 @@ func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { } // TestCompositeRemoteResourceCollisions asserts that genuine collisions involving remote resources are -// detected: a local resource and a remote resource sharing a simple name collide, even across different +// detected. A local resource and a remote resource sharing a simple name collide, even across different // APIs, as do two different (unprefixed) remotes exposing the same simple name. func TestCompositeRemoteResourceCollisions(t *testing.T) { logger, logs := logging.NewObservedTestLogger(t) @@ -434,6 +434,6 @@ func TestCompositeStopAllInProcess(t *testing.T) { r := setupLocalRobot(t, ctx, cfg, logger) test.That(t, r.StopAll(ctx, nil), test.ShouldBeNil) - // Stopped AT ALL (safety) and exactly ONCE (dedup), not once per advertised API. + // Stopped at all (safety) and exactly once (dedup), not once per advertised API. test.That(t, stopCount.Load(), test.ShouldEqual, 1) } From 5f363fbffa1c6357ce607a9821393e572c39cfde Mon Sep 17 00:00:00 2001 From: danielbotros Date: Thu, 15 Oct 2026 12:19:55 -0400 Subject: [PATCH 07/14] robot/impl: dedup composite API/sub enumeration via resource helpers Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 14 +++++--------- robot/impl/resource_manager.go | 20 +++++++++----------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index f6c6c56abca..f6de82f20e8 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -871,13 +871,11 @@ func (r *localRobot) getDependenciesWithWeakOptionalSnapshot( if err != nil { return nil, nil, &resource.DependencyNotReadyError{Name: dep.Name, Reason: err} } - allDeps[prefixedName] = resource.SubresourceForAPI(res, prefixedName.API) // A composite dependency serves several co-equal APIs, so key it under each of its API names with - // that API's unwrapped sub; a dependent then resolves it by whichever API it expects. Unwrap is a - // no-op for an ordinary resource. - for _, api := range resource.APIsOf(res) { - aliased := resource.Name{API: api, Remote: prefixedName.Remote, Name: prefixedName.Name} - allDeps[aliased] = resource.SubresourceForAPI(res, api) + // that API's unwrapped sub; a dependent then resolves it by whichever API it expects. For an + // ordinary resource this is the single (name, res) pair. + for aliased, sub := range resource.SubsByName(prefixedName, res) { + allDeps[aliased] = sub } } nodeConf := gNode.Config() @@ -1055,9 +1053,7 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( // serves rather than only its canonical Name().API. For each matched API, store its unwrapped sub // under that API's name so an API-qualified lookup resolves directly. An ordinary resource serves // one API, so the unwrap is a no-op. - for _, api := range resource.APIsOf(res) { - sub := resource.SubresourceForAPI(res, api) - apiName := resource.Name{API: api, Remote: n.Remote, Name: n.Name} + for apiName, sub := range resource.SubsByName(n, res) { for _, matcher := range weakDepMatchers { if matcher.IsMatch(sub) { // Pop the remote name off since callers won't be expecting it when accessing it in the diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index 720cb92a21a..81f0fd14b37 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -230,23 +230,21 @@ type internalRemoteRobot interface { MachineState() robot.MachineState } -// servedAPIs returns the co-equal API set a node currently serves. For a composite that is its -// MultiAPIResource's set, otherwise the single fallback API. Used to detect when a remote node's -// composite membership changed across a refresh. -func servedAPIs(n *resource.GraphNode, fallback resource.API) []resource.API { +// servedAPIs returns the API set a node serves, from its live resource, or just nameAPI if it has no +// resource yet. +func servedAPIs(n *resource.GraphNode, nameAPI resource.API) []resource.API { if res, err := n.UnsafeResource(); err == nil { - if mar, ok := res.(resource.MultiAPIResource); ok { - return mar.APIs() - } + return resource.APIsOf(res) } - return []resource.API{fallback} + return []resource.API{nameAPI} } -func sameAPISet(a, b []resource.API) bool { +// sameSet reports whether a and b contain the same elements, ignoring order and assuming no duplicates. +func sameSet[T comparable](a, b []T) bool { if len(a) != len(b) { return false } - set := make(map[resource.API]bool, len(a)) + set := make(map[T]bool, len(a)) for _, x := range a { set[x] = true } @@ -388,7 +386,7 @@ func (manager *resourceManager) updateRemoteResourceNames( if isComposite { desiredAPIs = coequalAPIs } - compositeChanged := nodeAlreadyExists && !sameAPISet(servedAPIs(gNode, resName.API), desiredAPIs) + compositeChanged := nodeAlreadyExists && !sameSet(servedAPIs(gNode, resName.API), desiredAPIs) if _, alreadyCurrent := activeResourceNames[resName]; alreadyCurrent { activeResourceNames[resName] = true From 339e25862ad1c8cf4ce520642da11fd7175d0947 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Thu, 15 Oct 2026 13:07:44 -0400 Subject: [PATCH 08/14] robot: resolve free ResourceByName via the composite-aware method Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/robot.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/robot/robot.go b/robot/robot.go index becbdcd07fe..c2ba36f5b0b 100644 --- a/robot/robot.go +++ b/robot/robot.go @@ -229,14 +229,7 @@ type RestartModuleRequest struct { // ResourceByName looks up via short name, and will error if none or more than 1 exist. func ResourceByName(r Robot, name string) (resource.Resource, error) { - all := AllResourcesByName(r, name) - if len(all) == 0 { - return nil, fmt.Errorf("no resource named [%s]", name) - } - if len(all) > 1 { - return nil, fmt.Errorf("too many resources named [%s] %d", name, len(all)) - } - return all[0], nil + return r.ResourceByName(resource.SimpleName(name)) } // AllResourcesByName returns an array of all resources that have this short name. From 947e5972351522d15e67497c6076b3ff4565cb7f Mon Sep 17 00:00:00 2001 From: danielbotros Date: Fri, 16 Oct 2026 14:38:56 -0400 Subject: [PATCH 09/14] robot/impl: clarify composite weak-dependency comment The weak-dep matcher runs against each API a composite serves, not only the one its Name() carries. Reword the comment so it no longer implies that API is canonical. Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index f6de82f20e8..04214f65483 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1050,9 +1050,9 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( continue } // A composite serves several co-equal APIs, so match the weak-dep matchers against each API it - // serves rather than only its canonical Name().API. For each matched API, store its unwrapped sub - // under that API's name so an API-qualified lookup resolves directly. An ordinary resource serves - // one API, so the unwrap is a no-op. + // serves rather than only the one its Name() carries. For each matched API, store its unwrapped + // sub under that API's name so an API-qualified lookup resolves directly. An ordinary resource + // serves one API, so the unwrap is a no-op. for apiName, sub := range resource.SubsByName(n, res) { for _, matcher := range weakDepMatchers { if matcher.IsMatch(sub) { From 18a1b3566922af632d198e7c56fa05c58c5d7691 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Mon, 19 Oct 2026 11:47:35 -0400 Subject: [PATCH 10/14] robot/impl,config: label composite resource logger as composite/ Name a composite's node logger composite/ via resource.LogName so its logs are not filed under a single co-equal API, and derive the per-resource log-level pattern the same way so log_configuration still matches. Co-Authored-By: Claude Opus 4.8 (1M context) --- config/config.go | 4 ++-- robot/impl/resource_manager.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/config.go b/config/config.go index 7f36a75bfa2..43231989151 100644 --- a/config/config.go +++ b/config/config.go @@ -1296,7 +1296,7 @@ func UpdateLoggerRegistryFromConfig(registry *logging.Registry, cfg *Config, log for _, serv := range cfg.Services { if serv.LogConfiguration != nil { resLogCfg := logging.LoggerPatternConfig{ - Pattern: "rdk.resource_manager." + serv.ResourceName().String(), + Pattern: "rdk.resource_manager." + resource.LogName(serv.ResourceName(), resource.APIsForModel(serv.Model)), Level: serv.LogConfiguration.Level.String(), } combinedLogCfg = append(combinedLogCfg, resLogCfg) @@ -1305,7 +1305,7 @@ func UpdateLoggerRegistryFromConfig(registry *logging.Registry, cfg *Config, log for _, comp := range cfg.Components { if comp.LogConfiguration != nil { resLogCfg := logging.LoggerPatternConfig{ - Pattern: "rdk.resource_manager." + comp.ResourceName().String(), + Pattern: "rdk.resource_manager." + resource.LogName(comp.ResourceName(), resource.APIsForModel(comp.Model)), Level: comp.LogConfiguration.Level.String(), } combinedLogCfg = append(combinedLogCfg, resLogCfg) diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index 81f0fd14b37..d9e7a45e400 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -947,7 +947,7 @@ func (manager *resourceManager) completeConfig( conf := gNode.Config() if gNode.IsUninitialized() { gNode.InitializeLogger( - manager.logger, resName.String(), + manager.logger, resource.LogName(resName, resource.APIsForModel(conf.Model)), ) } else { verb = "rebuild" From dc8dd64b698e6f9bda3911453da59159baf267fb Mon Sep 17 00:00:00 2001 From: danielbotros Date: Mon, 19 Oct 2026 14:08:18 -0400 Subject: [PATCH 11/14] robot/impl: resolve each weak-dep node once for composites A composite is one graph node reached under each of its co-equal API names, so it appears in AllNonCollidingResourceNames once per API. SubsByName already fans every served sub in a single pass, so dedupe on the resolved node rather than re-fanning it per alias. Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 04214f65483..254c04d9681 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1027,6 +1027,9 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( allNames := r.manager.AllNonCollidingResourceNames() deps := make(resource.Dependencies, len(allNames)) snapshot := make(map[resource.Name]int64) + // A composite is one node reached under each of its co-equal API names, and SubsByName fans all its + // subs in a single pass, so handle each node once instead of re-fanning it per name. + seenNodes := make(map[*resource.GraphNode]bool) for _, n := range allNames { if !(n.API.IsComponent() || n.API.IsService()) || n == resName { continue @@ -1040,6 +1043,10 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( } continue } + if seenNodes[node] { + continue + } + seenNodes[node] = true res, err := node.Resource() if err != nil { // Wrap as NotAvailableError to match the prior ResourceByName behavior From e9389e008de57ccdb7ac408326c397b729d61ff3 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Tue, 20 Oct 2026 14:14:11 -0400 Subject: [PATCH 12/14] robot/impl: store the composite wrapper in weak deps, not the per-API sub A weak-dependency matcher wired a composite in as the already-unwrapped sub for each matched API, leaving a weak-dep consumer with a single-API resource. Store the wrapper instead; both weak-dep consumers (data capture and motion) unwrap with resource.SubresourceForAPI, and composite data capture needs the full co-equal set to resolve a method to its owning API. An ordinary resource is its own only sub. Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 254c04d9681..f44862c1bea 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -1057,16 +1057,16 @@ func (r *localRobot) getWeakDependenciesAndSnapshot( continue } // A composite serves several co-equal APIs, so match the weak-dep matchers against each API it - // serves rather than only the one its Name() carries. For each matched API, store its unwrapped - // sub under that API's name so an API-qualified lookup resolves directly. An ordinary resource - // serves one API, so the unwrap is a no-op. + // serves rather than only the one its Name() carries. Store the wrapper under each matched API + // name. A weak-dep consumer unwraps it to the API it wants with resource.SubresourceForAPI. An + // ordinary resource is its own only sub, so nothing changes. for apiName, sub := range resource.SubsByName(n, res) { for _, matcher := range weakDepMatchers { if matcher.IsMatch(sub) { // Pop the remote name off since callers won't be expecting it when accessing it in the // resource dependency map in a resource constructor. popped := apiName.PopRemote() - deps[popped] = sub + deps[popped] = res snapshot[popped] = node.UpdatedAt() break } From 3d08c054f233631b64c04bf4e38dd95c7fb98a67 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Fri, 23 Oct 2026 10:42:32 -0400 Subject: [PATCH 13/14] robot: composite review fixes StopAll unwraps and dedups on the sub so a shared impl stops once, remote composite lookup uses SimpleName, and ResourceByName's doc matches behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 15 ++++++++------- robot/impl/multiapi_composite_test.go | 5 +++-- robot/impl/resource_manager.go | 6 +++--- robot/robot.go | 3 ++- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index f44862c1bea..834b06971d4 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -364,8 +364,9 @@ func (r *localRobot) StopAll(ctx context.Context, extra map[resource.Name]map[st // disconnect, or any residual cancellation). Matches motion's stop-on-error pattern. stopCtx := context.WithoutCancel(ctx) - // Stop all stoppable resources. A composite surfaces as N names (one per co-equal API) backed by - // one instance, so dedup on the raw pre-unwrap node resource to stop that instance at most once. + // Stop all stoppable resources. A composite surfaces as N names (one per API); unwrap each to its sub + // and dedup on the sub, not the wrapper, so a shared impl stops once but separate per-API controllers + // each stop. resourceErrs := make(map[string]error) stopped := make(map[resource.Resource]bool) for _, name := range r.ResourceNames() { @@ -379,15 +380,15 @@ func (r *localRobot) StopAll(ctx context.Context, extra map[resource.Name]map[st resourceErrs[name.Name] = err continue } - if stopped[raw] { + sub := resource.SubresourceForAPI(raw, name.API) + actuator, ok := sub.(resource.Actuator) + if !ok { continue } - - actuator, ok := resource.SubresourceForAPI(raw, name.API).(resource.Actuator) - if !ok { + if stopped[sub] { continue } - stopped[raw] = true + stopped[sub] = true if err := actuator.Stop(stopCtx, extra[name]); err != nil { resourceErrs[name.Name] = err } diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go index e283bb1d354..2135323628a 100644 --- a/robot/impl/multiapi_composite_test.go +++ b/robot/impl/multiapi_composite_test.go @@ -434,6 +434,7 @@ func TestCompositeStopAllInProcess(t *testing.T) { r := setupLocalRobot(t, ctx, cfg, logger) test.That(t, r.StopAll(ctx, nil), test.ShouldBeNil) - // Stopped at all (safety) and exactly once (dedup), not once per advertised API. - test.That(t, stopCount.Load(), test.ShouldEqual, 1) + // Every actuator API is stopped (safety). The gripper and servo facades are distinct objects, so + // each is stopped; a repeat Stop on a shared core is safe because Stop must be idempotent. + test.That(t, stopCount.Load(), test.ShouldEqual, 2) } diff --git a/robot/impl/resource_manager.go b/robot/impl/resource_manager.go index d9e7a45e400..5819b05a432 100644 --- a/robot/impl/resource_manager.go +++ b/robot/impl/resource_manager.go @@ -327,7 +327,7 @@ func (manager *resourceManager) updateRemoteResourceNames( remoteAPIsByName[rn.Name] = append(remoteAPIsByName[rn.Name], rn.API) } for k := range remoteAPIsByName { - slices.SortFunc(remoteAPIsByName[k], func(a, b resource.API) int { return strings.Compare(a.String(), b.String()) }) + resource.SortAPIs(remoteAPIsByName[k]) } for _, resName := range newResources { @@ -337,7 +337,7 @@ func (manager *resourceManager) updateRemoteResourceNames( } // Keep a composite as one node under its canonical (sorted-first) API; its other per-API names - // resolve to that node through the compositeByAPI index, so skip them here. + // resolve to that node through the composite index, so skip them here. coequalAPIs := remoteAPIsByName[resName.Name] isComposite := len(coequalAPIs) >= 2 if isComposite && resName.API != coequalAPIs[0] { @@ -349,7 +349,7 @@ func (manager *resourceManager) updateRemoteResourceNames( // For a composite, fetch the api-less wrapped MultiAPIResource; otherwise the per-API client. lookup := remoteResName if isComposite { - lookup = resource.Name{Name: remoteResName.Name} + lookup = resource.SimpleName(remoteResName.Name) } res, err := rr.ResourceByName(lookup) // this returns a remote known or foreign resource client if err != nil { diff --git a/robot/robot.go b/robot/robot.go index c2ba36f5b0b..0a1dc2f93b5 100644 --- a/robot/robot.go +++ b/robot/robot.go @@ -227,7 +227,8 @@ type RestartModuleRequest struct { ModuleName string } -// ResourceByName looks up via short name, and will error if none or more than 1 exist. +// ResourceByName looks a resource up by its simple name. A composite's several APIs collapse to one +// handle; it errors if nothing matches or distinct resources collide on the name. func ResourceByName(r Robot, name string) (resource.Resource, error) { return r.ResourceByName(resource.SimpleName(name)) } From 0d6346451c35f8a2a06471000891967320953941 Mon Sep 17 00:00:00 2001 From: danielbotros Date: Fri, 23 Oct 2026 16:39:45 -0400 Subject: [PATCH 14/14] robot: composite job-manager dispatch and optional-dependency fan-out Dispatch a composite jobs method to the co-equal API that serves it, resolve a composite to its whole handle for the job manager, and fan an optional dependency out across a composite served APIs. Adds in-process composite dependency tests and names a few inject mocks the fan-out now reads. Co-Authored-By: Claude Opus 4.8 (1M context) --- robot/impl/local_robot.go | 13 +++- robot/impl/local_robot_test.go | 29 ++++--- robot/impl/multiapi_composite_test.go | 96 +++++++++++++++++++++++- robot/impl/optional_dependencies_test.go | 2 +- robot/jobmanager/jobmanager.go | 41 ++++++++-- 5 files changed, 150 insertions(+), 31 deletions(-) diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 834b06971d4..4a0e7ed6ca5 100644 --- a/robot/impl/local_robot.go +++ b/robot/impl/local_robot.go @@ -738,7 +738,8 @@ func newWithResources( if !found { return nil, errors.Errorf("could not find the resource for name %s", res) } - return r.ResourceByName(match) + // SimpleName so a composite resolves to its whole handle, not one API's sub, for the job manager to dispatch. + return r.ResourceByName(resource.SimpleName(match.Name)) } jobManager, err := jobmanager.New(ctx, logger, getResource, r.webSvc.ModuleAddresses()) @@ -980,9 +981,13 @@ func (r *localRobot) getOptionalDependenciesAndSnapshot( continue } - optDeps[resolvedOptionalDepName] = optionalDep - found = append(found, resolvedOptionalDepName) - snapshot[resolvedOptionalDepName] = node.UpdatedAt() + // A composite serves several co-equal APIs, so key it under each with that API's sub. For an + // ordinary resource this is the single (name, res) pair. + for aliased, sub := range resource.SubsByName(resolvedOptionalDepName, optionalDep) { + optDeps[aliased] = sub + found = append(found, aliased) + snapshot[aliased] = node.UpdatedAt() + } } if len(conf.ImplicitOptionalDependsOn) > 0 { r.logger.Infow( diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index dd9da1ad670..e2ebd2e2b62 100644 --- a/robot/impl/local_robot_test.go +++ b/robot/impl/local_robot_test.go @@ -5873,16 +5873,15 @@ func TestModuleNamePassing(t *testing.T) { // The channel must be buffered to such that the `ReadingsFunc` returns without waiting on a // reader of the channel. moduleNameCh := make(chan string, 1) - callbackSensor := &inject.Sensor{ - ReadingsFunc: func(ctx context.Context, extra map[string]any) (map[string]any, error) { - moduleNameCh <- rgrpc.GetModuleName(ctx) - return map[string]any{ - "reading": 42, - }, nil - }, - CloseFunc: func(ctx context.Context) error { - return nil - }, + callbackSensor := inject.NewSensor("foo") + callbackSensor.ReadingsFunc = func(ctx context.Context, extra map[string]any) (map[string]any, error) { + moduleNameCh <- rgrpc.GetModuleName(ctx) + return map[string]any{ + "reading": 42, + }, nil + } + callbackSensor.CloseFunc = func(ctx context.Context) error { + return nil } // The resource registry is a global. We must use unique model names to avoid unexpected @@ -5984,11 +5983,11 @@ func TestInternalPanicFromModuleDoesNotCrash(t *testing.T) { conf resource.Config, logger logging.Logger, ) (sensor.Sensor, error) { - return &inject.Sensor{ - ReadingsFunc: func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { - panic("oh no") - }, - }, nil + s := inject.NewSensor(conf.Name) + s.ReadingsFunc = func(ctx context.Context, extra map[string]interface{}) (map[string]interface{}, error) { + panic("oh no") + } + return s, nil }}, ) diff --git a/robot/impl/multiapi_composite_test.go b/robot/impl/multiapi_composite_test.go index 2135323628a..7e6285608e6 100644 --- a/robot/impl/multiapi_composite_test.go +++ b/robot/impl/multiapi_composite_test.go @@ -209,6 +209,97 @@ func TestCompositeDependencyResolvesAllAPIs(t *testing.T) { test.That(t, err, test.ShouldBeNil) } +// TestCompositeDependsOnComposite asserts a composite resource can itself depend on another composite, +// resolving it under each of its co-equal APIs, and is in turn advertised under all of its own. +func TestCompositeDependsOnComposite(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + + innerModel := registerComboModel(t, "inner-combo", nil) + + // An outer composite (sensor + generic) whose constructor fails unless it resolves the inner composite + // under both of its APIs. + outerModel := resource.NewModel("acme", "test", "outer-combo") + resource.RegisterMultiAPI( + []resource.API{sensor.API, generic.API}, outerModel, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, deps resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + if _, err := resource.FromProvider[sensor.Sensor](deps, sensor.Named("inner")); err != nil { + return nil, fmt.Errorf("outer could not resolve inner composite via sensor API: %w", err) + } + if _, err := resource.FromProvider[resource.Resource](deps, generic.Named("inner")); err != nil { + return nil, fmt.Errorf("outer could not resolve inner composite via generic API: %w", err) + } + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }, + ) + defer resource.Deregister(sensor.API, outerModel) + defer resource.Deregister(generic.API, outerModel) + + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "inner", API: sensor.API, Model: innerModel}, + {Name: "outer", API: sensor.API, Model: outerModel, ImplicitDependsOn: []string{"inner"}}, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + + // outer built (so it resolved inner under both APIs) and is itself reachable under both of its APIs. + _, err := r.ResourceByName(sensor.Named("outer")) + test.That(t, err, test.ShouldBeNil) + _, err = r.ResourceByName(generic.Named("outer")) + test.That(t, err, test.ShouldBeNil) +} + +// TestCompositeRemovalTearsDownDependent asserts removing a composite that a live resource depends on +// tears that dependent down (its required dep is gone) and drops the composite under every API. +func TestCompositeRemovalTearsDownDependent(t *testing.T) { + logger := logging.NewTestLogger(t) + ctx := context.Background() + comboModel := registerComboModel(t, "removal-combo", nil) + + consumerModel := resource.NewModel("acme", "test", "removal-consumer") + resource.RegisterComponent(generic.API, consumerModel, + resource.Registration[resource.Resource, resource.NoNativeConfig]{ + Constructor: func( + _ context.Context, deps resource.Dependencies, conf resource.Config, _ logging.Logger, + ) (resource.Resource, error) { + if _, err := resource.FromProvider[sensor.Sensor](deps, sensor.Named("combo")); err != nil { + return nil, fmt.Errorf("consumer could not resolve composite: %w", err) + } + return &comboSensor{Named: conf.ResourceName().AsNamed()}, nil + }, + }) + defer resource.Deregister(generic.API, consumerModel) + + consumerCfg := resource.Config{ + Name: "consumer", API: generic.API, Model: consumerModel, ImplicitDependsOn: []string{"combo"}, + } + cfg := &config.Config{ + Components: []resource.Config{ + {Name: "combo", API: sensor.API, Model: comboModel}, + consumerCfg, + }, + } + r := setupLocalRobot(t, ctx, cfg, logger) + _, err := r.ResourceByName(generic.Named("consumer")) + test.That(t, err, test.ShouldBeNil) + + // Remove the composite, keeping the dependent configured. + r.Reconfigure(ctx, &config.Config{Components: []resource.Config{consumerCfg}}) + + // The composite is gone under every API, and the dependent is no longer available. + _, err = r.ResourceByName(sensor.Named("combo")) + test.That(t, err, test.ShouldNotBeNil) + _, err = r.ResourceByName(generic.Named("combo")) + test.That(t, err, test.ShouldNotBeNil) + _, err = r.ResourceByName(generic.Named("consumer")) + test.That(t, err, test.ShouldNotBeNil) +} + // TestCompositeRemoteResourceCollisions asserts that genuine collisions involving remote resources are // detected. A local resource and a remote resource sharing a simple name collide, even across different // APIs, as do two different (unprefixed) remotes exposing the same simple name. @@ -417,9 +508,8 @@ func TestCompositeInProcessTypedLookup(t *testing.T) { test.That(t, apisOf, test.ShouldContain, servo.API) } -// TestCompositeStopAllInProcess checks that StopAll stops a composite exactly once. The composite serves -// two actuator APIs over one instance, and StopAll's dedup stops it a single time, not once per -// advertised API. +// TestCompositeStopAllInProcess checks that StopAll reaches every actuator API a composite serves, not +// just its canonical one. func TestCompositeStopAllInProcess(t *testing.T) { logger := logging.NewTestLogger(t) ctx := context.Background() diff --git a/robot/impl/optional_dependencies_test.go b/robot/impl/optional_dependencies_test.go index 6f865518797..6c2c79ddeb7 100644 --- a/robot/impl/optional_dependencies_test.go +++ b/robot/impl/optional_dependencies_test.go @@ -966,7 +966,7 @@ func TestOptionalDependencyLocalWinsOverRemote(t *testing.T) { lr := setupLocalRobot(t, ctx, &config.Config{}, logger, WithDisableCompleteConfigWorker()).(*localRobot) // A local arm and an (unprefixed) remote sensor both named "shared". - injectArm := &inject.Arm{} + injectArm := inject.NewArm("shared") localArmCfg := resource.Config{API: arm.API, Name: "shared"} err := lr.manager.resources.AddNode( localArmCfg.ResourceName(), diff --git a/robot/jobmanager/jobmanager.go b/robot/jobmanager/jobmanager.go index 1c51409b085..53b2d5863df 100644 --- a/robot/jobmanager/jobmanager.go +++ b/robot/jobmanager/jobmanager.go @@ -15,6 +15,7 @@ import ( "github.com/fullstorydev/grpcurl" "github.com/go-co-op/gocron/v2" "github.com/google/uuid" + "github.com/jhump/protoreflect/desc" "github.com/jhump/protoreflect/grpcreflect" "github.com/pkg/errors" "go.uber.org/multierr" @@ -180,27 +181,51 @@ func (jm *JobManager) createDescriptorSourceAndgRPCMethod( // refClient.AllowMissingFileDescriptors() reflSource := grpcurl.DescriptorSourceFromServer(jm.ctx, refClient) descSource := reflSource - resourceType := res.Name().API.SubtypeName - // some subtypes have an underscore in their name, like audio_in, audio_out, input_controller, - // or pose_tracker, while their APIs do not - so we have to remove the underscore. - resourceType = strings.ReplaceAll(resourceType, "_", "") services, err := descSource.ListServices() if err != nil { return nil, refClient, "", "", err } + // A composite serves several co-equal APIs, so the job's method may belong to any of them. Try each + // served API and route to the one whose service exposes the method; an ordinary resource has a + // single API and keeps its original behavior. + // Known limitation: if two co-equal APIs expose a method of the same name, this picks the + // canonical-first one. A job addresses a composite by simple name and can't target a specific API. + apis := resource.APIsOf(res) var grpcService string - for _, srv := range services { - if strings.Split(srv, ".")[componentServiceIndex] == resourceType { - grpcService = srv + for _, api := range apis { + // some subtypes have an underscore in their name, like audio_in, audio_out, input_controller, + // or pose_tracker, while their APIs do not - so we have to remove the underscore. + resourceType := strings.ReplaceAll(api.SubtypeName, "_", "") + for _, srv := range services { + if strings.Split(srv, ".")[componentServiceIndex] != resourceType { + continue + } + if len(apis) == 1 || serviceHasMethod(descSource, srv, method) { + grpcService = srv + } + break + } + if grpcService != "" { break } } if grpcService == "" { - return nil, refClient, "", "", errors.Errorf("could not find a service for type: %s", resourceType) + return nil, refClient, "", "", errors.Errorf("could not find a service exposing method %q for resource %s", method, res.Name()) } return descSource, refClient, grpcService, method, nil } +// serviceHasMethod reports whether the named gRPC service exposes the given method. It picks the right +// co-equal API when dispatching a job to a composite resource that serves several. +func serviceHasMethod(descSource grpcurl.DescriptorSource, service, method string) bool { + sym, err := descSource.FindSymbol(service) + if err != nil { + return false + } + sd, ok := sym.(*desc.ServiceDescriptor) + return ok && sd.FindMethodByName(method) != nil +} + // createJobFunction returns a function that the job scheduler puts on its queue. func (jm *JobManager) createJobFunction(jc config.JobConfig, continuous bool) func(ctx context.Context) error { jobLogger := jm.logger.Sublogger(jc.Name)