From dbfaa83fc618798214f487e7671f3e3fe708a859 Mon Sep 17 00:00:00 2001 From: HipsterBrown Date: Fri, 10 Jul 2026 22:38:52 -0400 Subject: [PATCH] fix(registry): key shared controller by canonical device path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared-controller registry keyed on the raw `port` string, so an arm configured as "/dev/ttyUSB0" and a gripper configured via a "/dev/serial/by-id/usb-..." symlink to the same device were treated as two different ports — opening two feetech buses that silently contend for one UART (garbled frames, intermittent timeouts). This is footgun #1 from the shared-controller architecture review (#25). Resolve the port to a stable device identity (filepath.EvalSymlinks, best-effort with fallback to the raw string) and use that as the registry map key. Only the key is normalized — the caller's config is left untouched because it can be read concurrently via GetControllerStatus (mutating it tripped the race detector) — so configsEqual and trackCaller canonicalize port strings at comparison time instead. Result: different spellings of one device collapse to a single shared bus. Tests (no hardware): canonicalPortKey resolves a symlink to its target and falls back for unresolvable paths; a consumer acquiring via a symlink reuses the entry created via the real path (one entry, refcount 2) rather than opening a second bus. Full package passes under -race, including the existing TestConcurrentRegistryAccess. Refs #25. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JNvy6dGUT6wBHLNBv8YPYv --- canonical_port_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++ manager.go | 2 +- registry.go | 32 +++++++++++++++++- 3 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 canonical_port_test.go diff --git a/canonical_port_test.go b/canonical_port_test.go new file mode 100644 index 0000000..1d282db --- /dev/null +++ b/canonical_port_test.go @@ -0,0 +1,73 @@ +package so_arm + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCanonicalPortKey(t *testing.T) { + dir := t.TempDir() + device := filepath.Join(dir, "ttyUSB0") + if err := os.WriteFile(device, nil, 0o600); err != nil { + t.Fatalf("create fake device: %v", err) + } + link := filepath.Join(dir, "by-id-usb-serial") + if err := os.Symlink(device, link); err != nil { + t.Fatalf("symlink: %v", err) + } + + // A symlink and its target resolve to the same canonical key. + if got, want := canonicalPortKey(link), canonicalPortKey(device); got != want { + t.Errorf("symlink and target should canonicalize equal: %q vs %q", got, want) + } + + // An unresolvable path falls back to the original string (best-effort). + missing := filepath.Join(dir, "does-not-exist") + if got := canonicalPortKey(missing); got != missing { + t.Errorf("unresolvable path should fall back to original: got %q, want %q", got, missing) + } +} + +// TestRegistry_SymlinkPortSharesController verifies that a consumer acquiring a +// port via a /dev/serial/by-id-style symlink reuses the controller a prior +// consumer created via the real device path, instead of opening a second bus +// on the same UART. Regression guard for the silent double-open footgun. +func TestRegistry_SymlinkPortSharesController(t *testing.T) { + dir := t.TempDir() + device := filepath.Join(dir, "ttyUSB0") + if err := os.WriteFile(device, nil, 0o600); err != nil { + t.Fatalf("create fake device: %v", err) + } + link := filepath.Join(dir, "by-id-usb-serial") + if err := os.Symlink(device, link); err != nil { + t.Fatalf("symlink: %v", err) + } + + realKey := canonicalPortKey(device) + + registry := NewControllerRegistry() + // Pre-inject an entry keyed by the canonical device path, as if the arm + // acquired first via the real path. No live bus is needed here: + // getExistingController touches no bus method on the shared-acquire path. + registry.entries[realKey] = &ControllerEntry{ + controller: &SafeSoArmController{}, + config: testConfig(realKey), + calibration: DefaultSO101FullCalibration, + refCount: 1, + } + + // A second consumer acquires the same device via the symlink spelling. + if _, err := registry.GetController(link, testConfig(link), DefaultSO101FullCalibration, false); err != nil { + t.Fatalf("acquire via symlink: %v", err) + } + + // It must reuse the existing entry rather than create a second one... + if n := len(registry.entries); n != 1 { + t.Errorf("expected 1 shared registry entry, got %d", n) + } + // ...bumping the shared refcount instead of opening a second bus. + if rc := registry.entries[realKey].refCount; rc != 2 { + t.Errorf("expected refCount 2 after shared acquire, got %d", rc) + } +} diff --git a/manager.go b/manager.go index 298f5d2..f30e948 100644 --- a/manager.go +++ b/manager.go @@ -349,7 +349,7 @@ func configsEqual(a, b *SoArm101Config) bool { if a == nil || b == nil { return false } - return a.Port == b.Port && + return canonicalPortKey(a.Port) == canonicalPortKey(b.Port) && a.Baudrate == b.Baudrate && a.Timeout == b.Timeout } diff --git a/registry.go b/registry.go index 2493377..cf31ecf 100644 --- a/registry.go +++ b/registry.go @@ -3,6 +3,7 @@ package so_arm import ( "context" "fmt" + "path/filepath" "runtime" "strings" "sync" @@ -12,6 +13,24 @@ import ( "github.com/hipsterbrown/feetech-servo/feetech" ) +// canonicalPortKey resolves a serial-port path to a stable device identity so +// that two spellings of the same physical port — e.g. "/dev/ttyUSB0" and a +// "/dev/serial/by-id/usb-..." symlink pointing at it — map to a single shared +// controller instead of two feetech buses contending for one UART. +// +// It is best-effort: filepath.EvalSymlinks resolves symlinks and cleans the +// path, but if the path can't be resolved (device not present yet, or a +// platform without symlink semantics) the original string is returned so the +// caller still gets a usable key. The result is used as the registry map key +// and for port-equality comparisons; the bus is still opened with the caller's +// original config.Port, so an unresolved fallback preserves today's behavior. +func canonicalPortKey(port string) string { + if resolved, err := filepath.EvalSymlinks(port); err == nil { + return resolved + } + return port +} + type ControllerEntry struct { controller *SafeSoArmController config *SoArm101Config @@ -38,6 +57,13 @@ func NewControllerRegistry() *ControllerRegistry { } func (r *ControllerRegistry) GetController(portPath string, config *SoArm101Config, calibration SO101FullCalibration, fromFile bool) (*SafeSoArmController, error) { + // Collapse different spellings of the same device (e.g. /dev/ttyUSB0 and a + // /dev/serial/by-id symlink) to one registry key so both consumers share a + // single bus instead of contending. Only the key is normalized -- the caller's + // config is left untouched (it may be read concurrently via GetControllerStatus), + // so configsEqual and trackCaller canonicalize port strings at comparison time. + portPath = canonicalPortKey(portPath) + r.mu.RLock() entry, exists := r.entries[portPath] r.mu.RUnlock() @@ -93,7 +119,9 @@ func (r *ControllerRegistry) getExistingController(entry *ControllerEntry, confi } atomic.AddInt64(&entry.refCount, 1) - r.trackCaller(entry.config.Port) + // Track against the canonical key so releaseFromCaller can find the entry + // even when this consumer spelled the port differently than the creator. + r.trackCaller(canonicalPortKey(entry.config.Port)) return &SafeSoArmController{ bus: entry.controller.bus, @@ -316,6 +344,8 @@ func (r *ControllerRegistry) GetControllerStatus(portPath string) (int64, bool, } func (r *ControllerRegistry) GetCurrentCalibration(portPath string) SO101FullCalibration { + portPath = canonicalPortKey(portPath) + r.mu.RLock() entry, exists := r.entries[portPath] r.mu.RUnlock()