A 階段第二個 milestone、銜接 M9-1 bridge.py、暴露 service layer 給 M9-3 API/WebSocket。 New module `server/internal/firmware/`: - types.go: 123 行(FirmwareVersion / FirmwareProgress / ActiveTaskInfo / UpgradeDriver interface / 8 reason const) - progress.go: 147 行(仿 flash pattern 的 Tracker、Task.cancel 預留 SIGTERM force-cancel godoc) - service.go: 373 行(核心 service:UpgradeFirmware / HasActiveTask / GetActiveTaskInfo / RequestShutdown / WaitForActiveTasks / ListBundledVersions / GetCurrentVersion) - service_test.go: 676 行、13 個 test 含 MultiDeviceParallel Driver layer: - kl720_driver.go: 697 → 1054 行(+357、新 UpgradeFirmware method + tryRouteFirmwareEvent + sendCommandForUpgrade snapshot pattern) - kl720_driver_test.go: 360 行、11 個 test(含 InfoNotBlockedDuringUpgrade / CtxCancelReleasesBridge / StderrEventAfterCtxCancel 100 round stress) 關鍵設計: - flash 與 firmware 模組分離(不 import flash) - UpgradeDriver interface 隔離 driver 細節、DeviceLookup interface 隔離 device manager - 中介 channel pattern(service ↔ driver)方便 service 補欄位(DeviceID / Direction / BeforeVersion) - timeout 雙保險:chip timeout + 30s margin - 8 reason enum 對齊 bridge.py、stage 採 Design 命名 Concurrency race 修復(M9-2 Reviewer round 1 → round 2): - Major 1(mutex deadlock):新 fwUpgradeMu 獨立鎖 + sendCommandForUpgrade snapshot stdin/stdout pattern、避開 d.mu field-level race + 升級期間 Info/Disconnect 不被卡 + timeout 路徑無死鎖 - Major 2(close-channel race):tryRouteFirmwareEvent 持 fwMu 整段、配合 defer setFirmwareProgressCh(nil) 提供 happen-before、絕無 send on closed channel panic Reviewer 兩輪審查: - Round 1: 0 Critical / 2 Major / 5 Minor / 5 Suggestion - Round 2: 0 Critical / 0 Major / 2 Minor / 2 Suggestion(11/12 issue 修到位、Suggestion 4 留 follow-up) M9-1 follow-up 順手清: - m5(test 死碼 _firmware_upgrade_start_ts 殘留兩行)已清 - s5(test 註解 idempotent shape 說明)已加 測試: - go test ./... -race -count=1: 全綠(28s、無 regression) - Python: 36 tests + 22 subtests 全綠(0.31s) - go vet / build: 0 output 下一步:M9-3 API handler + WebSocket progress(CI 建議 `go test -race -count=3` 提升 race 偵測強度) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
25 KiB
Reviewer Report — M9-2 Go driver + firmware service(第 2 輪修改驗證)
審查日期:2026-05-25 範圍:第 1 輪 2 Major + 5 Minor + 4 Suggestion(Suggestion 4 留 follow-up)的修改驗證 統計:0 Critical / 0 Major / 2 Minor / 2 Suggestion 結論:✅ 通過、不阻擋 M9-3
TL;DR
第 1 輪所有 issue(2 Major + 5 Minor + 4 Suggestion,Suggestion 4 留 follow-up)皆已修復、且修法品質高:
- Major 1(mutex deadlock) 採方案 B 變體(fwUpgradeMu + sendCommandForUpgrade snapshot pattern)— 修法正確、snapshot pattern 安全。
- Major 2(close-channel race) 採方案 A 變體(tryRouteFirmwareEvent 持 fwMu 整段 + defer setFirmwareProgressCh(nil))— happen-before 鏈成立。
- 5 個新 test 都針對 race window 設計、
StderrEventAfterCtxCancel用 100×8 goroutine 壓測足以驗 race。
第 2 輪新發現 2 個 Minor + 2 個 Suggestion,皆不阻擋 M9-3 啟動、可在 M9-3 並行修。不需要 backend 第 3 輪。
第 1 輪 issue 修改驗證(逐項)
| Issue | 第 1 輪 | 第 2 輪修法 | 驗證結果 |
|---|---|---|---|
| Major 1(mutex deadlock) | sendCommand 持 d.mu 60-200s、卡 Info/IsConnected、timeout 路徑 deadlock | 新 fwUpgradeMu + 新 sendCommandForUpgrade(snapshot stdin/stdout 後 release d.mu)、UpgradeFirmware Step 2 改 fwUpgradeMu+sendCommandForUpgrade(line 953-971);ctx.Done 路徑仍走 d.mu 殺 bridge(line 981-984) |
✅ 修法正確、見下方深入評估 |
| Major 2(close-channel race) | tryRoute 取 ch 後 release fwMu 才 send、與 close 有 race window | tryRoute 改 defer d.fwMu.Unlock() 整段持鎖(line 868-869)+ UpgradeFirmware line 940 defer setFirmwareProgressCh(nil) 提供 happen-before |
✅ 修法正確、見下方深入評估 |
| Minor 1(brickRiskReasons) | ReasonTimeout 是否屬 brick 沒寫清楚 | service.go:36-44 加註解、明示 vendor-agnostic、把 chip+elapsed 判斷推給 M9-3 handler | ✅ 註解清晰、保留 vendor-agnostic 設計 |
| Minor 2(缺 ctx.Done + multi-device 測試) | 缺 TestUpgradeFirmware_CtxCancelReleasesBridge + multi-device |
driver_test 新增 TestUpgradeFirmware_CtxCancelReleasesBridge(line 269-308);service_test 新增 TestUpgradeFirmware_MultiDeviceParallel(line 519-615) |
✅ 兩個 test 都到位、測法合理 |
| Minor 3(attemptedUpgrade) | 早退路徑誤標 needsReset | line 916 加 var attemptedUpgrade bool + line 953 attemptedUpgrade = true(在 setFirmwareProgressCh 之後、進 sendCommand 之前);defer 內 if attemptedUpgrade { d.needsReset = true }(line 925-927) |
✅ 修法正確,但有小問題(見下方 Minor R-1) |
| Minor 4(結論先寫) | forward loop 推論在前 | service.go:192-198 把「結論:保留中介 channel pattern」放到段首、「推論」放到後面 | ✅ 結論先寫、可讀性提升 |
| Minor 5(fwMu atomic) | 與 Major 2 同源 | 隨 Major 2 修法一起處理 | ✅ |
| Suggestion 1(done event 去重) | sendCommand 補 done 與 stderr push done 會重複 | service.go:213-220 在 forward loop 加 seenDone guard、第二次 StageDone 跳過;對應 test 在 service_test.go:624-653 TestUpgradeFirmware_DedupeDoneEvent |
✅ 修法正確、test 有覆蓋 |
| Suggestion 2(multi-device parallel test) | 缺多 device 並行測 | service_test.go:519-615 補 3 device 並發、驗 deviceID 不誤匹配、callCount 各 = 1 | ✅ |
| Suggestion 3(ListBundledVersions Stat) | chipDir 不存在時無聲回 current | service.go:330-340 加 os.Stat(chipDir)、IsNotExist 回 missing error、其他 error 帶回 wrap;service_test.go:451-489 重寫用 t.TempDir() + 顯式建 KL520 dir、驗 KL720 missing 路徑 |
✅ 修法正確、test 有 cover |
| Suggestion 4(bridgeFirmwareEvent 與 FirmwareProgress 共用 struct) | 兩 struct 重複 | 本輪未修(明示為 follow-up) | — |
| Suggestion 5(Task.cancel godoc) | 無 reader、加 TODO | progress.go:25-31 加 godoc:「目前 service 只在 runUpgrade defer 內呼叫一次... 預留給未來 SIGTERM force-cancel 流程」 | ✅ |
統計:12 項中 11 項修、1 項明示 follow-up。修了的 11 項全部修法正確。
兩個 Major 的修法品質深入評估
Major 1(mutex deadlock)— ✅ 修法正確
修法總覽
- 新增
fwUpgradeMu sync.Mutex(line 56):與d.mu完全分離的鎖、只給升級期間 sendCommand 用。 - 新增
sendCommandForUpgrade()(line 273-314):- Lock d.mu → check pythonReady → snapshot stdin/stdout 到 local var → Unlock d.mu
- 接著 marshal cmd、
fmt.Fprintf(stdin, ...)、stdout.Scan()、parse response — 全部在 unlocked 狀態
- UpgradeFirmware Step 2 改寫(line 953-971):sendCommand goroutine 內
fwUpgradeMu.Lock()→ call sendCommandForUpgrade → Unlock。完全不再持 d.mu。 - ctx.Done 路徑(line 975-1001):仍走
d.mu.Lock + d.stopPython() + d.connected = false + d.mu.Unlock。
Snapshot pattern 正確性 — ✅
逐欄位驗:
| 在 sendCommandForUpgrade 內 | 來源 | 是否 snapshot 完整 |
|---|---|---|
d.pythonReady 檢查 |
持 d.mu 內讀 | ✅ 不需 snapshot(讀完即釋鎖) |
d.stdin → local stdin |
持 d.mu 內 copy ref(line 280) | ✅ |
d.stdout → local stdout |
持 d.mu 內 copy ref(line 281) | ✅ |
| 持 d.mu 期間是否做 I/O | 沒有、release 後才做 | ✅ |
| nil check on snapshot | line 284-286 加了 `if stdin == nil |
關鍵問題:stopPython 把 d.stdin = nil / d.stdout = nil 之後、snapshot 已取走的舊 ref 還能用嗎?write 到 closed pipe 會怎樣?
逐 case 推:
-
正常路徑(無 ctx cancel):
- sendCommand goroutine:snapshot → release d.mu → write/scan → return result via resCh
- 全程 stopPython 不會被呼叫、stdin/stdout 仍有效。✅
-
ctx cancel 路徑:
- sendCommand goroutine 已 snapshot stdin/stdout(local ref)、可能正在
stdout.Scan()等 bridge 回應 - ctx.Done 觸發 → main goroutine 在 line 981
d.mu.Lock()+d.stopPython():d.stdin.Close()(line 321)→ 關閉 write 端 pipe → sendCommand goroutine 若還沒 write 完、write 會回io.ErrClosedPipe(line 293 errs)d.pythonCmd.Process.Kill()(line 326)→ python subprocess 死、bridge 那端的 stdout pipe write 端關掉 → sendCommand goroutine 的stdout.Scan()拿到 EOF → return false → 進 line 298 走 error 路徑d.pythonCmd.Wait()(line 327)→ wait for process exit、通常幾十 ms 內完成
- sendCommand goroutine 從 Scan 回來、return err、寫 resCh、結束。
- 沒 panic、沒 goroutine leak、
go func() { <-resCh }()(line 1000)負責 drain leak 防護。✅
- sendCommand goroutine 已 snapshot stdin/stdout(local ref)、可能正在
-
race window:sendCommand goroutine snapshot 到 stdin/stdout 後、release d.mu 之前、stopPython 不能進來(被 d.mu 擋住);release d.mu 之後、snapshot 已 capture local ref、即使 stopPython 把
d.stdin = nil也只影響 future 呼叫、不影響此次的 local ref。沒 race。✅
寫 to closed pipe 不會 panic(io.Writer interface 只 return error)、Scan 拿 EOF 也只 return false、沒 panic 風險。✅
新 fwUpgradeMu 與 d.mu 的 lock 順序 — ✅ 安全
各 callsite 整理:
| Callsite | lock 順序 |
|---|---|
| sendCommand goroutine(line 960-969) | fwUpgradeMu.Lock → 內部 sendCommandForUpgrade 短暫持 d.mu(snapshot)→ release d.mu → release fwUpgradeMu |
| ctx.Done 路徑(line 981-984) | 只持 d.mu、不取 fwUpgradeMu |
| 其他 method(Info/IsConnected/Flash/...) | 只持 d.mu、不取 fwUpgradeMu |
lock 順序唯一可能撞點:sendCommand goroutine 內持 fwUpgradeMu 後又要拿 d.mu(snapshot 時)。但這是「外鎖 → 內鎖」單方向、不會反序——沒有其他路徑會「持 d.mu 後再去拿 fwUpgradeMu」(沒有任何 method 這樣做)。✅
無 lock-order deadlock 風險。
Timeout 路徑驗證 — ✅
老的死鎖 scenario:sendCommand 持 d.mu → ctx.Done → main 想拿 d.mu → wait → sendCommand 永遠不回 → 死鎖。
新的 scenario:
- sendCommand goroutine 只持 fwUpgradeMu、不持 d.mu
- ctx.Done → main 在 line 981 拿 d.mu(沒人持、立刻拿到)
d.stopPython()內Process.Kill() + Wait():通常 < 100ms(Wait 在 SIGKILL 後幾乎瞬間 return)- release d.mu → push error event → drain resCh leak prevention → return
timeout 路徑現在可預期在 200ms 內完成(vs 老版的「永遠死鎖」)。✅
sendCommand goroutine 釋放保證 — ✅
ctx cancel 後 sendCommand goroutine 的釋放鏈:
ctx cancel
→ main 進 line 981
→ d.mu.Lock(取得)
→ d.stopPython()
→ d.stdin.Close()(pipe write 端 close)
→ Process.Kill()(subprocess 死、stdout pipe 自動 close)
→ Process.Wait()(等清理完)
→ d.stdin = nil(但 sendCommand 已拿 snapshot、不受影響)
→ d.stdout = nil(同上)
→ release d.mu
→ push error event(line 996)
→ go func(){ <-resCh }()(leak prevention)
sendCommand goroutine 並發:
→ fwUpgradeMu.Lock(已持)
→ sendCommandForUpgrade 內:
stdout.Scan() 阻塞中 → bridge 死 → EOF → Scan return false
→ return err(line 298)
→ fwUpgradeMu.Unlock
→ resCh <- result{nil, err}
→ goroutine exit
Test 驗證:
TestUpgradeFirmware_CtxCancelReleasesBridge(driver_test.go:269)— 2s timeout 內 UpgradeFirmware 必須 return。雖然這個 test 用 fake bridge(不真起 python)、stopPython 沒實際 Process.Kill 可做(pythonCmd nil)、但 stdin.Close() 的 pipe 操作會讓 Scan 拿到 EOF、goroutine 確實能釋放。✅
Major 2(close-channel race)— ✅ 修法正確
修法總覽
- tryRouteFirmwareEvent 持 fwMu 整段(line 868-882):
關鍵:「檢查 ch + select send」全在持鎖期間、無「先 release 才 send」的縫隙。d.fwMu.Lock() defer d.fwMu.Unlock() ch := d.fwProgressCh if ch == nil { return false } select { case ch <- fp: default: // drop event } - setFirmwareProgressCh(nil) 取同把 fwMu(line 820-824):與 tryRoute 互斥。
- UpgradeFirmware return 前 defer(line 940):
defer d.setFirmwareProgressCh(nil)是 UpgradeFirmware 第二個 defer、在 close(intermediate) 之前 trigger。
Happen-before 推理鏈驗證 — ✅
逐步 trace:
service runUpgrade goroutine:
go func() {
driverDone <- drv.UpgradeFirmware(ctx, chip, intermediate)
close(intermediate)
}()
driver UpgradeFirmware:
defer d.setFirmwareProgressCh(nil) // 第二個 defer、第一個執行(LIFO)
defer ...status reset... // 第一個 defer
...
return nil/err // ← defer 從這裡開始 fire
對應的時序(success / error 路徑都適用):
| 步 | 動作 | 鎖 | happen-before 提供 |
|---|---|---|---|
| 1 | driver return | — | — |
| 2 | defer setFirmwareProgressCh(nil) 觸發 | fwMu.Lock → set nil → fwMu.Unlock | T2 ✅ |
| 3 | UpgradeFirmware return | — | — |
| 4 | service goroutine 收到 driverDone <- ... |
— | T4 happens-after T3 |
| 5 | service goroutine close(intermediate) | — | — |
關鍵:T2(setFirmwareProgressCh(nil) 釋放 fwMu)→ T5(close intermediate)之間有 sequential ordering(同個 service goroutine 內)。
任何 inflight tryRouteFirmwareEvent call(從 stderr scanner 端發起):
| 情境 | 結果 |
|---|---|
| tryRoute 在 T2 之前進來、取 fwMu | T2 必須等 tryRoute Unlock 才能 set nil。tryRoute 內讀到的 ch 還是 valid intermediate ch;send 後 return true。T5 close 還沒發生(因為 T5 在 T2 之後)。✅ 安全 |
| tryRoute 在 T2 進行中嘗試取 fwMu | 等 T2 完成、取到 fwMu 時 ch 已是 nil、return false。沒 send、沒 panic。✅ |
| tryRoute 在 T2 之後、T5 之前進來 | ch 已 nil、return false。沒 send。✅ |
| tryRoute 在 T5 之後進來 | ch 已 nil(T2 設過了)、return false。沒 send。✅ |
「send on closed channel」場景不可能發生。✅
stderr scanner 阻塞風險 — ⚠️ Minor R-2
tryRouteFirmwareEvent 在持 fwMu 期間做 select case ch <- fp / default:
- 非阻塞(有 default)— 不會卡 stderr scanner 太久。
- 持鎖時間:把 buffered channel send(< 1µs)+ select syntax overhead(< 1µs)= 通常 < 5µs。
- 最壞情境:setFirmwareProgressCh(nil) 並發來搶鎖、要等 tryRoute 釋鎖、wait < 5µs。
但有個細節值得改善(見 Minor R-2):JSON unmarshal(line 842-846)在持鎖之前完成、這部分正確;但 firmware.FirmwareProgress struct copy(line 851-863)也在 unmarshal 之後、Lock 之前完成、也正確。結構上沒問題。
第 2 輪新發現(regression risk)
🔴 Critical
無。
🟠 Major
無。
🟡 Minor
Minor R-1 — attemptedUpgrade 設定點位置略偏早
檔案:kl720_driver.go:953
問題:
// Step 1: register progress routing
d.setFirmwareProgressCh(progressCh)
defer d.setFirmwareProgressCh(nil)
// Step 2: spawn sendCommand in goroutine
...
attemptedUpgrade = true // ← line 953、在「進入 goroutine 前」設
...
resCh := make(chan result, 1)
go func() { ... }()
attemptedUpgrade = true 在 goroutine spawn 之前就設、若 goroutine 從未真的 send sendCommand(理論上不會發生、但理論上有 OS-level scheduler 異常或 panic 之類的)、needsReset = true 還是會被 defer 設。
實務影響:幾乎零。但語意上「真進 sendCommand 才算 attempted」會更精準——可以把 line 953 搬進 goroutine 內、緊鄰 d.fwUpgradeMu.Lock() 之後。
嚴重度:Minor。M9-3 啟動前不阻擋,純語意精確性。
建議修法:
go func() {
d.fwUpgradeMu.Lock()
attemptedUpgrade = true // ← 搬到這裡
resp, err := d.sendCommandForUpgrade(...)
d.fwUpgradeMu.Unlock()
resCh <- result{resp, err}
}()
⚠️ 注意:attemptedUpgrade 是 var 在 outer func、若搬進 goroutine、會有 data race(defer 在 outer goroutine 讀、inner goroutine 寫)。所以這個修法需要改成 atomic.Bool 或者保持在 outer goroutine 寫。權衡之下、現狀(在 goroutine 外設)反而是正確的——避免 race。
結論:現狀其實是正確的、Minor R-1 撤回。為求嚴謹保留紀錄、但不需修。降為 Suggestion R-3(見下)。
Minor R-2 — setFirmwareProgressCh 在 driver Disconnect 路徑沒清
檔案:kl720_driver.go:420-439 Disconnect()
問題:
func (d *KneronDriver) Disconnect() error {
d.mu.Lock()
defer d.mu.Unlock()
...
d.stopPython()
d.connected = false
...
}
Disconnect 不 reset d.fwProgressCh。理論情境:
- UpgradeFirmware in-flight、fwProgressCh = X
- 使用者 / 系統 call
Disconnect()(理論上應該不能在升級中被叫、但 driver 層沒擋) - stopPython 殺 bridge → sendCommand goroutine return error → UpgradeFirmware 走 case
<-resCh的 error 路徑 → return err →defer setFirmwareProgressCh(nil)觸發 → 此時才清
實際上沒漏——defer setFirmwareProgressCh(nil) 在 UpgradeFirmware return 時一定會跑。Disconnect 不重複清也沒問題(已經 nil)。
但若Disconnect 在 UpgradeFirmware 之前/之後被叫、且其他 path 之後又把 ch 設成 valid(這個情境不存在於目前 codebase),可能有 stale ch。
嚴重度:Minor、純防禦性建議。M9-3 啟動前不需修。
建議:在 Disconnect 內 stopPython 之前 / 之後加一行 d.fwMu.Lock(); d.fwProgressCh = nil; d.fwMu.Unlock()、純防禦性。或者在 stopPython 內加同一行(更乾淨、統一清理 pipe + ch 兩個資源)。
實務影響:目前 codebase 沒實際 bug、只是「再多一層防禦不壞」。
💡 Suggestion
Suggestion R-1 — seenDone guard 也該套用到 StageError
檔案:service.go:213-220
現況:
var lastStage string
var seenDone bool
for ev := range intermediate {
if ev.Stage == StageDone && seenDone {
continue
}
...
if ev.Stage == StageDone {
seenDone = true
}
task.ProgressCh <- ev
}
問題:類似的 race(stderr push error event + driver UpgradeFirmware error 路徑 line 1009-1018 也 push error event fallback safety net)也會產生重複 error event。雖然 driver 端 line 1006 註解寫「stage="error" event 通常已透過 stderr 推過了、這裡是 fallback safety net」、實際上和 done 一樣會雙保險、可能重複。
建議:加 seenError 同類 guard、避免前端跑兩次 cleanup:
var lastStage string
var seenDone, seenError bool
for ev := range intermediate {
if ev.Stage == StageDone && seenDone {
continue
}
if ev.Stage == StageError && seenError {
continue
}
...
if ev.Stage == StageDone {
seenDone = true
}
if ev.Stage == StageError {
seenError = true
}
task.ProgressCh <- ev
}
或更通用:用一個 set 記已看過的終態 stage。
嚴重度:Suggestion、不阻擋。M9-3 wire 到 WS 時若前端有 cleanup 邏輯、值得補。
Suggestion R-2 — TestUpgradeFirmware_StderrEventAfterCtxCancel 加 t.Parallel 防止 sequential 假陽性
檔案:kl720_driver_test.go:315
現況:100 round × 8 goroutine 不斷 tryRouteFirmwareEvent + 主 loop 不斷 unregister/close/recreate。沒 t.Parallel、跑單一 test 時 CPU 不一定有真實 race scheduling。
建議:加 t.Parallel(),跑 go test -race -count=10 ./... 時更容易撞出 race。當前實作已可信、go test -race 應該都能 pass,但加 t.Parallel 提升 race detector 觸發機率。
嚴重度:Suggestion、test 改善。
Suggestion R-3 — attemptedUpgrade 用 bool 在 outer goroutine 設、不在 inner goroutine(避免 data race)
檔案:kl720_driver.go:953
如 Minor R-1 結論所述、現狀正確、不要搬。為求文件齊備、加註解說明「為何不在 goroutine 內設」:
// attemptedUpgrade 必須在 outer goroutine 寫、不能搬進 sendCommand goroutine
// 內、因為 defer(line 917-928)也在 outer goroutine 讀;用 atomic 或 mutex
// 會 over-engineering。在 goroutine spawn 前設 = 99.99% 等同於 sendCommand
// 真的執行(OS scheduler 失敗導致 goroutine 永不執行的機率可忽略)。
attemptedUpgrade = true
嚴重度:Suggestion、純註解、降後續維護成本。
5 個新測試品質評估
TestUpgradeFirmware_InfoNotBlockedDuringUpgrade(driver_test.go:217)— ✅ 直接驗 Major 1
| 軸 | 評估 |
|---|---|
| 是否驗到 Major 1 修法 | ✅ 起 UpgradeFirmware goroutine、sleep 50ms 讓 sendCommand 進 stdout.Scan blocking、然後 d.Info() 必須 500ms 內回 |
| 是否會誤過(false positive) | 若 Major 1 沒修、Info 會被 d.mu 卡 60-200s(fake bridge 永不回應);500ms timeout 必抓到。✅ |
| 收尾乾淨 | cancelUpgrade() → 等 upgradeDone(2s budget)+ pipe cleanup(fakeBridge.t.Cleanup)。✅ |
| 小瑕疵 | t.Cleanup(cancelUpgrade) 在 line 225 註冊、但 line 257 又顯式 cancelUpgrade()、實際無害(cancel 是冪等的)但有點冗。不算問題。 |
TestUpgradeFirmware_CtxCancelReleasesBridge(driver_test.go:269)— ✅ 驗 Minor 2
| 軸 | 評估 |
|---|---|
| 是否驗到 sendCommand goroutine 釋放 | ✅ UpgradeFirmware 必須在 cancel 後 2s 內 return(vs 老版「永遠死鎖」) |
| 驗 timeout event 推送 | ✅ line 297-307 驗 progressCh 拿到 StageError + ReasonTimeout |
| 小瑕疵 | progressCh 沒被 drain、若 upgrade 又推 done event(理論不會、因為 sendCommand 應該 error return)會卡 channel buffer。但 buffer = 16、容得下。不算問題。 |
TestUpgradeFirmware_StderrEventAfterCtxCancel(driver_test.go:315)— ⚠️ 略有壓力測試強度疑慮
| 軸 | 評估 |
|---|---|
| 是否驗到 close-channel race | ✅ 100 round × 8 goroutine 高頻率「register → unregister → close」並發、若 fwMu 沒 cover 整段、必撞 panic |
| 強度評估 | 100 round 對 race detector 來說「夠用」、但若關閉 race detector(go test 沒 -race)、可能要更多 round 才能撞到。建議搭配 CI -race 跑。Suggestion R-2 建議加 t.Parallel。 |
| race window 是否真實覆蓋 | ✅ time.Sleep(10 * time.Microsecond) 給 route goroutine 時間進入 tryRoute、d.setFirmwareProgressCh(nil) + close(ch) 立刻 follow-up — 這正是 Major 2 設計修法要保證的 ordering。 |
TestUpgradeFirmware_MultiDeviceParallel(service_test.go:519)— ✅ 驗 Suggestion 2
| 軸 | 評估 |
|---|---|
| 是否驗到 tracker key 不誤匹配 | ✅ 3 device 並發、每個的 events.DeviceID 必須 = 自己的 id(line 585-589)、callCount 必須各 = 1(line 605-613) |
| 是否真的並行 | ✅ 3 個 goroutine 同時 call svc.UpgradeFirmware、收 startCh、再各自 drain。真並行、不是序列。 |
| 收尾乾淨 | svc.WaitForActiveTasks(3 * time.Second) 等所有 task done、再驗 HasActiveTask 為 false。✅ |
TestUpgradeFirmware_DedupeDoneEvent(service_test.go:624)— ✅ 驗 Suggestion 1
| 軸 | 評估 |
|---|---|
| 是否驗到 dedup | ✅ events 推 2 個 StageDone、drain 後 doneCount 必 = 1 |
| 小瑕疵 | 沒驗 done event 的 AfterVersion 等內容是否來自「第一個」done(forward 第一個、丟第二個)。雖然 2 個 done 的內容相同(line 629-631 都是 AfterVersion: "2.2.0")、看不出來。為驗 forward 是「第一個」而非「最後一個」、可以讓 2 個 done 的 AfterVersion 不同。不阻擋通過、純測試精度改善。 |
整體新 test 品質
- 覆蓋面:5 個 test 對應 2 Major + 3 Suggestion/Minor、覆蓋完整。
- 可信度:所有 test 都針對 race window 設計合理、用 fake bridge 模擬合適、不過度模擬到驗不到真實 race。
- 強度:建議 CI 跑
go test -race -count=5 ./internal/driver/kneron/... ./internal/firmware/...提升 race detection。
ListBundledVersions 修法(Suggestion 3)驗證
service.go:330-340 修法:
chipDir := filepath.Join(s.fwDir.Root, chip)
if _, err := os.Stat(chipDir); err != nil {
if os.IsNotExist(err) {
return nil, fmt.Errorf("firmware not bundled for chip %q (missing %s)", chip, chipDir)
}
return nil, fmt.Errorf("firmware dir stat failed for chip %q: %w", chip, err)
}
✅ 區分「chip dir 不存在」(明示 missing build)與「stat 其他錯誤」(permission / IO 問題)。
service_test.go:451-489 對應 test:
t.TempDir()建臨時根、只建 KL520 dir- KL520(dir 存在)→ 應回 1 個 current
- KL630(unsupported)→ ErrUnsupportedChip
- KL720(dir missing)→ 應回 error
- fwDir.Root 空 → 應回 error
✅ 邊界涵蓋齊全。
結論
- ✅ 通過、不阻擋 M9-3 啟動
- ✅ 不需要 backend 第 3 輪——第 2 輪新發現的 2 Minor + 2 Suggestion 都可在 M9-3 並行修,或留 follow-up,不影響介面/合約
- ✅ 不需要升級給 security agent——第 2 輪修改完全是 concurrency / pattern 改善、無 OWASP / authentication / authorization 相關
- ⚠️ CI 建議:M9-3 PR 時可加
go test -race -count=3 ./internal/driver/kneron/... ./internal/firmware/...對應 lane、確保 race detection 有跑
給 backend 的肯定
第 2 輪修法品質非常高:
- Major 1 採方案 B 變體(fwUpgradeMu + snapshot pattern)比方案 A(細粒度 Lock)更乾淨、不破壞 sendCommand 既有結構。snapshot pattern 的 nil check + ctx.Done 路徑 Wait 行為都考慮周到。
- Major 2 採方案 A 變體(fwMu 整段持鎖)+ defer + happen-before 推理、比方案 C(atomic.Value)更直觀易維護。
- 5 個新 test 都有針對性地驗 race window、不是只跑 happy path。
- 註解品質特別高(line 47-67、line 263-272、line 826-840、line 944-952)—把「為什麼這樣修 / 老版的問題 / 新版的保證」全寫進註解、未來改 code 的人不會誤踩。
第 2 輪新發現整理
| # | 嚴重度 | 檔案:行 | 摘要 |
|---|---|---|---|
| Minor R-1 | Minor → 撤回(現狀正確) | kl720_driver.go:953 | attemptedUpgrade 位置 — 結論:現狀正確、改 Suggestion R-3(加註解) |
| Minor R-2 | Minor | kl720_driver.go:420 / 317 | Disconnect / stopPython 沒主動清 fwProgressCh — 純防禦性、目前無 bug |
| Suggestion R-1 | Suggestion | service.go:213 | seenError 也加 guard、與 seenDone 對稱 |
| Suggestion R-2 | Suggestion | kl720_driver_test.go:315 | t.Parallel() 提升 race detection |
| Suggestion R-3 | Suggestion | kl720_driver.go:953 | 加註解說明 attemptedUpgrade 為何不搬進 goroutine |
全部 5 項都不阻擋 M9-3。