Skip to content

RSDK-9718: fix tagger.proto go_package so its descriptor registers correctly#847

Draft
NickPPC wants to merge 1 commit intomainfrom
fix-rsdk-9718
Draft

RSDK-9718: fix tagger.proto go_package so its descriptor registers correctly#847
NickPPC wants to merge 1 commit intomainfrom
fix-rsdk-9718

Conversation

@NickPPC
Copy link
Copy Markdown
Member

@NickPPC NickPPC commented May 5, 2026

Summary

One-line proto change (plus regenerated .pb.go) that fixes a long-standing gRPC reflection failure. After this PR, any service whose descriptor chain reaches tagger/v1/tagger.proto — notably viam.service.discovery.v1.DiscoveryService — can be reflected end-to-end. This unblocks RDK-side cleanup tracked in RSDK-9718 and silences a debug log line that fires on every RobotClient refresh cycle (~once every 5 s) for every robot configured with a discovery service.

The problem

A debug line fires on every RobotClient refresh:

debug … client/client.go:835 failed to find symbol for resource API
  api rdk:service:discovery
  error Symbol not found: viam.service.discovery.v1.DiscoveryService
        caused by: File not found: tagger/v1/tagger.proto

It is the visible tip of a deeper bug. The same root cause forced three other workarounds in RDK:

  • module/server.go — special-cased the discovery API to skip reflection lookup.
  • robot/jobmanager/jobmanager.go — TODO suggesting refClient.AllowMissingFileDescriptors() as a workaround.
  • robot/impl/jobmanager_test.go — discovery job test commented out (Discovery Service is currently excluded … it will be available after a change in the API repo).

Root cause

proto/viam/tagger/v1/tagger.proto declared:

option go_package = "github.com/srikrsna/protoc-gen-gotag/tagger;tagger";

That go_package option points to the upstream protoc-gen-gotag Go package, not the regenerated copy that lives at go.viam.com/api/tagger/v1. The option go_package value is what protoc-gen-go writes into every dependent .pb.go as the import path for the tagger types. So the generated app/v1/robot.pb.go, app/v1/app.pb.go, and app/mltraining/v1/ml_training.pb.go each had:

_ "github.com/srikrsna/protoc-gen-gotag/tagger"

The upstream package's init() registers a different file in protoregistry.GlobalFilestagger/tagger.proto (root path, package tagger) — not tagger/v1/tagger.proto. Meanwhile the regenerated go.viam.com/api/tagger/v1/tagger.pb.go package, which would register tagger/v1/tagger.proto correctly, was effectively dead code: it shipped in the module but nothing imported it, so its init() never ran.

The descriptor chain encoded into the dependents' .pb.go files comes from the proto import (import "tagger/v1/tagger.proto"), not the Go-side blank import. So at runtime the FileDescriptor for app/v1/robot.proto legitimately lists tagger/v1/tagger.proto as a dependency — but no init in the binary ever registers a file by that name. When a gRPC reflection client walks the dep chain of discovery.v1.DiscoveryServiceapp/v1/robot.prototagger/v1/tagger.proto, the server returns NotFound.

This is why the bug only surfaces at reflection time and only for service descriptors that transitively reach into app/v1/*. Wire-format calls to those services have always worked — they don't need the file descriptor.

The fix

Change the go_package option to point at the regenerated package:

option go_package = "go.viam.com/api/tagger/v1;tagger";

After regenerating, dependents now blank-import _ "go.viam.com/api/tagger/v1" instead of the upstream package, so the regenerated tagger's init() runs and registers tagger/v1/tagger.proto under the path consumers actually look it up by.

No proto path or wire-format changes — only the Go import path of dependents (which is generator-managed, not user code).

Verification

  • mise r buf-go produced exactly 5 file diffs: the proto change, the regenerated tagger package (raw FileDescriptor bytes encode the new go_package), and the three dependents flipping their blank import. Diff is +8/-9.

  • go build ./... clean; existing tests unaffected.

  • Spot-check on a tiny program that blank-imports go.viam.com/api/service/discovery/v1 and queries the registry:

    fd, err := protoregistry.GlobalFiles.FindFileByPath("tagger/v1/tagger.proto")

    Returns NotFound on main. After this change, returns tagger/v1/tagger.proto (package=tagger.v1).

  • End-to-end on RDK (with a local replace pointing at this branch): the previously-disabled discovery-service job in robot/impl/jobmanager_test.go::TestJobManagerConfigChanges now passes — the job hits FindSymbol on viam.service.discovery.v1.DiscoveryService, succeeds, and the gRPC method invocation goes through cleanly. The failed to find symbol for resource API debug line no longer fires for the discovery API.

Notes

  • app/model/v1/model.pb.go still references the upstream tagger package, but its source proto (proto/viam/app/model/v1/model.proto) was deleted in commit 87932114 (DATA-1524 cleanup). It's an orphaned generated file and not regenerated by buf-go, so it's untouched here. Out of scope; can be removed in a follow-up if desired.
  • The github.com/srikrsna/protoc-gen-gotag Go module dep stays in go.mod because that orphan file still imports it. The protoc-gen-gotag codegen binary is unaffected (installed via mise.toml, separate from the runtime module dep).
  • A follow-up RDK PR will drop the three workarounds (discovery skip in module/server.go, TODO line in robot/jobmanager/jobmanager.go, commented-out test entry in robot/impl/jobmanager_test.go) and bump go.viam.com/api to the version cut from this PR.

Test plan

  • CI green on this branch.
  • Reviewer confirms no proto wire-format implications (it's a Go-side import path change only).
  • After release, RDK PR (linked when posted) verifies the end-to-end fix and removes the workarounds.

…proto path

The tagger.proto's go_package pointed at the upstream protoc-gen-gotag
package, which registers a different file path (tagger/tagger.proto)
than what consuming protos import (tagger/v1/tagger.proto). As a result,
generated descriptors for app/v1/robot.proto, app/v1/app.proto, and
app/mltraining/v1/ml_training.proto have a transitive dependency on
tagger/v1/tagger.proto that is never registered in the running process,
breaking gRPC reflection for any service whose descriptor chain reaches
this file (notably viam.service.discovery.v1.DiscoveryService).

Switch go_package to the regenerated go.viam.com/api/tagger/v1 package
so its init() runs and registers tagger/v1/tagger.proto under the path
its consumers expect. Regenerate dependents via mise r buf-go.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the safe to test committer is a member of this org label May 5, 2026
@NickPPC NickPPC added the ready-for-protos add this when you want protos to compile on every commit label May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

protos-compiled ready-for-protos add this when you want protos to compile on every commit safe to test committer is a member of this org

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant