From 12b1fe3bad3deff56883c297a8d9d0a50c4aeb62 Mon Sep 17 00:00:00 2001 From: jim800121chen Date: Fri, 10 Jul 2026 09:44:47 +0800 Subject: [PATCH] =?UTF-8?q?fix(local-agent):=20Rescan=20=E5=BA=8F=E8=99=9F?= =?UTF-8?q?=E8=BA=AB=E5=88=86=E6=AF=94=E5=B0=8D=E6=9B=BF=E6=8F=9B=20stale?= =?UTF-8?q?=20session=20+=20=E9=8E=96=E5=A4=96=20Disconnect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - serialIdentity() 三態比對（空/假序號 0x00000000 正規化為無身分、對齊 ADR-018 R2）：Rescan 對既存合成 ID 比對身分——同一顆保留 live session、 換位/拔插以新 info 替換 session，serial 索引重建即正確 - stale drivers 改釋放鎖後 Disconnect（不再阻塞 GetDevice 路由）+ Disconnect 錯誤改 WARNING log - 測試 seam：detectFn/newDriverFn 注入 + newSessionDriverLocked 統一建構 （Start 行為零變動）；新 4 支行為級測試（拔除位移/換位 rebind/ 同序號保 session/無身分回歸） - evidence：build/vet/test 全綠 + -race ok；review 通過 0C/0M/0Mi/4Sug （wp0-serial-routing-review.md「Minor #1 修復審查」章節） - WP-C 阻擋項解除；觀察項：多裝置實測留意 changed identity log （偵測順序抖動時解法在 detector 端排序） Co-Authored-By: Claude Fable 5 --- local-agent/server/internal/device/manager.go | 102 +++++++++-- .../server/internal/device/manager_test.go | 162 +++++++++++++++++- 2 files changed, 243 insertions(+), 21 deletions(-) diff --git a/local-agent/server/internal/device/manager.go b/local-agent/server/internal/device/manager.go index 4ceb000..aa36ff2 100644 --- a/local-agent/server/internal/device/manager.go +++ b/local-agent/server/internal/device/manager.go @@ -34,6 +34,13 @@ type Manager struct { scriptPath string logBroadcaster *logger.Broadcaster mu sync.RWMutex + + // detectFn / newDriverFn are seams so Rescan behavior (unplug / slot + // swap of positional synthetic IDs) can be tested without spawning the + // Python bridge. Production wiring (set in NewManager) uses the kneron + // implementations. + detectFn func(scriptPath string) []driver.DeviceInfo + newDriverFn func(info driver.DeviceInfo, scriptPath string) driver.DeviceDriver } func NewManager(registry *DriverRegistry, scriptPath string) *Manager { @@ -43,6 +50,10 @@ func NewManager(registry *DriverRegistry, scriptPath string) *Manager { serialToLocalID: make(map[string]string), eventBus: make(chan DeviceEvent, 100), scriptPath: scriptPath, + detectFn: kneron.DetectDevices, + newDriverFn: func(info driver.DeviceInfo, scriptPath string) driver.DeviceDriver { + return kneron.NewKneronDriver(info, scriptPath) + }, } } @@ -60,9 +71,23 @@ func (m *Manager) SetLogBroadcaster(b *logger.Broadcaster) { } } +// newSessionDriverLocked builds a driver for a detected device and attaches +// the log broadcaster when the concrete driver supports it. Caller must hold +// m.mu (write lock) — it only reads m.logBroadcaster/scriptPath, but is kept +// locked-only for consistency with its callers. +func (m *Manager) newSessionDriverLocked(info driver.DeviceInfo) driver.DeviceDriver { + d := m.newDriverFn(info, m.scriptPath) + if m.logBroadcaster != nil { + if kd, ok := d.(*kneron.KneronDriver); ok { + kd.SetLogBroadcaster(m.logBroadcaster) + } + } + return d +} + func (m *Manager) Start() { // Detect real Kneron devices (KL520, KL720, etc.) via Python bridge. - devices := kneron.DetectDevices(m.scriptPath) + devices := m.detectFn(m.scriptPath) if len(devices) == 0 { log.Println("No Kneron devices detected") return @@ -71,20 +96,36 @@ func (m *Manager) Start() { m.mu.Lock() defer m.mu.Unlock() for _, info := range devices { - d := kneron.NewKneronDriver(info, m.scriptPath) - if m.logBroadcaster != nil { - d.SetLogBroadcaster(m.logBroadcaster) - } - m.sessions[info.ID] = NewSession(d) + m.sessions[info.ID] = NewSession(m.newSessionDriverLocked(info)) log.Printf("Registered Kneron device: %s (%s, type=%s)", info.Name, info.ID, info.Type) } m.rebuildSerialIndexLocked() } -// Rescan re-detects connected Kneron devices. New devices are registered, -// removed devices are cleaned up, and existing devices are left untouched. +// serialIdentity normalizes a serial for identity comparison: empty and the +// fake pyusb-fallback serial both mean "no identity" (ADR-018 R2), so they +// compare equal to each other and never equal to a real kn_number. +func serialIdentity(serial string) string { + if serial == fakeSerialNumber { + return "" + } + return serial +} + +// Rescan re-detects connected Kneron devices. New devices are registered and +// removed devices are cleaned up. For an existing synthetic ID the session is +// kept only when the detected serial identity matches the session's; on a +// mismatch the session is replaced with a fresh driver built from the +// detected info. +// +// Why the identity check (WP-0 review Minor #1): synthetic IDs are +// positional ("kl520-0" = first KL520 found). With multiple same-chip +// dongles, unplugging or re-ordering shifts which physical device occupies a +// slot. Keeping the old session would make rebuildSerialIndexLocked() index +// a stale serial → the serial route would hit a removed/relocated device. +// The index must always reflect what detection currently sees. func (m *Manager) Rescan() []driver.DeviceInfo { - detected := kneron.DetectDevices(m.scriptPath) + detected := m.detectFn(m.scriptPath) // Build a set of detected device IDs. detectedIDs := make(map[string]driver.DeviceInfo, len(detected)) @@ -92,37 +133,60 @@ func (m *Manager) Rescan() []driver.DeviceInfo { detectedIDs[info.ID] = info } + // Drivers to disconnect after releasing the lock: a real KneronDriver + // Disconnect can take seconds, and holding the write lock would block + // every GetDevice (inference / flash / camera routing) meanwhile. + type staleDriver struct { + id string + drv driver.DeviceDriver + } + var stale []staleDriver + m.mu.Lock() - defer m.mu.Unlock() // Remove devices that are no longer present. for id, s := range m.sessions { if _, exists := detectedIDs[id]; !exists { log.Printf("Device removed: %s", id) - s.Driver.Disconnect() + stale = append(stale, staleDriver{id: id, drv: s.Driver}) delete(m.sessions, id) } } - // Add newly detected devices. + // Register newly detected devices and replace sessions whose slot is now + // occupied by a different physical device. for _, info := range detected { - if _, exists := m.sessions[info.ID]; !exists { - d := kneron.NewKneronDriver(info, m.scriptPath) - if m.logBroadcaster != nil { - d.SetLogBroadcaster(m.logBroadcaster) + if s, exists := m.sessions[info.ID]; exists { + old := s.Driver.Info().SerialNumber + if serialIdentity(old) == serialIdentity(info.SerialNumber) { + // Same physical device (or no identity on either side, e.g. + // the single-dongle no-SDK demo) — keep the live session and + // its connection state untouched. + continue } - m.sessions[info.ID] = NewSession(d) - log.Printf("Registered Kneron device: %s (%s, type=%s)", info.Name, info.ID, info.Type) + log.Printf("Device %s changed identity (serial %q -> %q); replacing session", info.ID, old, info.SerialNumber) + stale = append(stale, staleDriver{id: info.ID, drv: s.Driver}) + delete(m.sessions, info.ID) } + m.sessions[info.ID] = NewSession(m.newSessionDriverLocked(info)) + log.Printf("Registered Kneron device: %s (%s, type=%s)", info.Name, info.ID, info.Type) } m.rebuildSerialIndexLocked() - // Return current list. + // Snapshot current list while still holding the lock. devices := make([]driver.DeviceInfo, 0, len(m.sessions)) for _, s := range m.sessions { devices = append(devices, s.Driver.Info()) } + m.mu.Unlock() + + for _, sd := range stale { + if err := sd.drv.Disconnect(); err != nil { + log.Printf("WARNING: disconnect of stale device %s failed: %v", sd.id, err) + } + } + return devices } diff --git a/local-agent/server/internal/device/manager_test.go b/local-agent/server/internal/device/manager_test.go index ca6eb9b..2c358c8 100644 --- a/local-agent/server/internal/device/manager_test.go +++ b/local-agent/server/internal/device/manager_test.go @@ -7,8 +7,9 @@ import ( ) type testDriver struct { - info driver.DeviceInfo - connected bool + info driver.DeviceInfo + connected bool + disconnects int } func (d *testDriver) Info() driver.DeviceInfo { return d.info } @@ -19,6 +20,7 @@ func (d *testDriver) Connect() error { } func (d *testDriver) Disconnect() error { d.connected = false + d.disconnects++ d.info.Status = driver.StatusDisconnected return nil } @@ -197,6 +199,162 @@ func TestManager_SerialIndex_SkipsEmptyAndFakeSerial(t *testing.T) { } } +// ========================================================================== +// Rescan identity check (WP-0 review Minor #1): positional synthetic IDs vs +// serial identity — the serial index must reflect current detection. +// ========================================================================== + +// newRescanManager returns a Manager whose detection and driver factory are +// stubbed: Rescan sees whatever *detected currently holds and builds +// testDrivers instead of spawning the Python bridge. +func newRescanManager(detected *[]driver.DeviceInfo) *Manager { + mgr := NewManager(NewRegistry(), "") + mgr.detectFn = func(string) []driver.DeviceInfo { return *detected } + mgr.newDriverFn = func(info driver.DeviceInfo, _ string) driver.DeviceDriver { + return &testDriver{info: info} + } + return mgr +} + +func TestManager_Rescan_UnplugShiftsPositionalID(t *testing.T) { + // Two same-chip dongles: kl520-0 = serial A, kl520-1 = serial B. + detected := []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: "0xAAAA0001"}, + {ID: "kl520-1", Type: "KL520", SerialNumber: "0xBBBB0002"}, + } + mgr := newRescanManager(&detected) + mgr.Rescan() + + oldSession, err := mgr.GetDevice("kl520-0") + if err != nil { + t.Fatalf("GetDevice(kl520-0) error = %v", err) + } + oldDriver := oldSession.Driver.(*testDriver) + + // Unplug A: detection now sees a single device, and the positional ID + // kl520-0 is occupied by the dongle with serial B. + detected = []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: "0xBBBB0002"}, + } + mgr.Rescan() + + // Removed serial A must no longer be routable. + if _, err := mgr.GetDevice("0xAAAA0001"); err == nil { + t.Error("GetDevice(0xAAAA0001) expected error after unplug, got nil (stale serial route)") + } + // Serial B must route to its new positional slot kl520-0. + s, err := mgr.GetDevice("0xBBBB0002") + if err != nil { + t.Fatalf("GetDevice(0xBBBB0002) error = %v", err) + } + if got := s.Driver.Info().ID; got != "kl520-0" { + t.Errorf("serial 0xBBBB0002 resolved to %q, want kl520-0", got) + } + if got := s.Driver.Info().SerialNumber; got != "0xBBBB0002" { + t.Errorf("session kl520-0 serial = %q, want 0xBBBB0002 (Device Info not refreshed)", got) + } + // The vacated positional slot kl520-1 must be gone. + if _, err := mgr.GetDevice("kl520-1"); err == nil { + t.Error("GetDevice(kl520-1) expected error after unplug, got nil") + } + // The replaced stale driver (old occupant of kl520-0) was disconnected. + if oldDriver.disconnects == 0 { + t.Error("stale driver for old kl520-0 occupant was not disconnected") + } +} + +func TestManager_Rescan_SwapRebindsSerials(t *testing.T) { + detected := []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: "0xAAAA0001"}, + {ID: "kl520-1", Type: "KL520", SerialNumber: "0xBBBB0002"}, + } + mgr := newRescanManager(&detected) + mgr.Rescan() + + // Replug in the opposite order: slots swap occupants. + detected = []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: "0xBBBB0002"}, + {ID: "kl520-1", Type: "KL520", SerialNumber: "0xAAAA0001"}, + } + mgr.Rescan() + + sA, err := mgr.GetDevice("0xAAAA0001") + if err != nil { + t.Fatalf("GetDevice(0xAAAA0001) error = %v", err) + } + if got := sA.Driver.Info().ID; got != "kl520-1" { + t.Errorf("serial 0xAAAA0001 resolved to %q, want kl520-1 (swap not reflected)", got) + } + sB, err := mgr.GetDevice("0xBBBB0002") + if err != nil { + t.Fatalf("GetDevice(0xBBBB0002) error = %v", err) + } + if got := sB.Driver.Info().ID; got != "kl520-0" { + t.Errorf("serial 0xBBBB0002 resolved to %q, want kl520-0 (swap not reflected)", got) + } +} + +func TestManager_Rescan_SameDeviceKeepsSession(t *testing.T) { + // Single-dongle regression: same serial across rescans → session (and + // its connection state) must be left untouched. + detected := []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: "0x1A2B3C4D"}, + } + mgr := newRescanManager(&detected) + mgr.Rescan() + + go func() { + for range mgr.Events() { + } + }() + if err := mgr.Connect("kl520-0"); err != nil { + t.Fatalf("Connect() error = %v", err) + } + before, _ := mgr.GetDevice("kl520-0") + + mgr.Rescan() + + after, err := mgr.GetDevice("kl520-0") + if err != nil { + t.Fatalf("GetDevice(kl520-0) after rescan error = %v", err) + } + if before != after { + t.Error("session was replaced on rescan although the device did not change") + } + if td := after.Driver.(*testDriver); !td.connected { + t.Error("connection state lost across rescan of an unchanged device") + } + if s, err := mgr.GetDevice("0x1A2B3C4D"); err != nil || s != after { + t.Errorf("serial route after rescan = (%v, %v), want same session", s, err) + } +} + +func TestManager_Rescan_NoSerialSingleDeviceUntouched(t *testing.T) { + // No-SDK demo regression: empty/fake serials carry no identity, so the + // existing session must be kept (previous "left untouched" behavior). + for _, serials := range [][2]string{{"", ""}, {fakeSerialNumber, fakeSerialNumber}, {"", fakeSerialNumber}} { + detected := []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: serials[0]}, + } + mgr := newRescanManager(&detected) + mgr.Rescan() + before, _ := mgr.GetDevice("kl520-0") + + detected = []driver.DeviceInfo{ + {ID: "kl520-0", Type: "KL520", SerialNumber: serials[1]}, + } + mgr.Rescan() + + after, err := mgr.GetDevice("kl520-0") + if err != nil { + t.Fatalf("serials %v: GetDevice(kl520-0) error = %v", serials, err) + } + if before != after { + t.Errorf("serials %v: session replaced although neither serial carries identity", serials) + } + } +} + func TestManager_SerialIndex_RemovedDeviceUnroutable(t *testing.T) { mgr := NewManager(NewRegistry(), "") seedDevice(mgr, driver.DeviceInfo{ID: "kl520-0", SerialNumber: "0x1A2B3C4D"})