# Reviewer Report — M9-2 Go driver + firmware service

> 審查日期：2026-05-25
> 範圍：M9-2 Go driver UpgradeFirmware + firmware service + types/progress + M9-1 follow-up cleanup
> 統計：0 Critical / 2 Major / 5 Minor / 5 Suggestion

## TL;DR

M9-2 整體實作完整、規格對齊度高、TDD §8.6 graceful shutdown 介面齊備、bridge 端 4 個情境 stage 序列在 Go driver 端正確 forward；25 個 Go test + 36 個 Python test 覆蓋核心路徑。但**有 2 個 Major 與 driver mutex 範圍 + close-channel race 相關**，建議 backend 第 2 輪修；其餘 Minor / Suggestion 不阻擋 M9-3 啟動。**不升級 security agent**。

## 審查範圍

| # | 檔案 | 行數 | 性質 |
|---|------|------|------|
| 1 | `server/internal/firmware/types.go` | 123（新） | 列舉 / Schema |
| 2 | `server/internal/firmware/progress.go` | 141（新） | ProgressTracker |
| 3 | `server/internal/firmware/service.go` | 346（新） | 核心 service |
| 4 | `server/internal/firmware/service_test.go` | 517（新、11 tests） | 單元測試 |
| 5 | `server/internal/driver/kneron/kl720_driver.go` | 697→948 | 修改、+251 行 |
| 6 | `server/internal/driver/kneron/kl720_driver_test.go` | 177（新、8 tests） | 單元測試 |
| 7 | `server/scripts/test_kneron_bridge_firmware.py` | 清 m5/s5、+9/-2 | M9-1 follow-up |

---

## 🔴 Critical（必修、阻擋 merge）

**無**。

---

## 🟠 Major（強烈建議修、建議第 2 輪修）

### Major 1 — driver mutex hold 整段升級 + timeout deadlock
**檔案**：`kl720_driver.go:860-866 + 875`

**問題**：sendCommand goroutine 在 `d.mu.Lock() → sendCommand → d.mu.Unlock()` 整段持鎖、sendCommand 內部會 blocking 等 bridge stdout 回應 60-200s。期間：
- 其他 method（Info / IsConnected / Disconnect）被卡 60-200s
- M9-3 GetActiveTaskInfo / DeviceInfo 會打 `driver.Info()` → 被卡
- **timeout 路徑死鎖**：service ctx.Done → driver line 875 `d.mu.Lock()` 等 → 但 sendCommand goroutine 還持有 d.mu → sendCommand goroutine 永遠不回（bridge 卡住、沒人殺它）→ 死鎖

**建議**：sendCommand goroutine 改為只在 stdin write / stdout read 細粒度 lock、不整段持鎖；或為 firmware 升級開獨立 sub-mutex 避免阻塞 Info。

### Major 2 — fwProgressCh close-channel race
**檔案**：`kl720_driver.go:758-797 + service.go:197-203`

**問題**：tryRouteFirmwareEvent 取 ch 後 release fwMu 才 send、與 service goroutine close(intermediate) 之間有 race window。
- 視窗很窄（一個 LOCK/UNLOCK 之間）
- stderr 在 device disconnect / process die 的 inflight event 處理時最容易撞到
- **panic: send on closed channel** 可能發生

**建議修法（任選）**：
- A：tryRouteFirmwareEvent 用 `defer recover()` 包 send
- B：driver 自己擁有 stderr push ch、UpgradeFirmware return 前先 close own ch + drain 後再 close service intermediate
- C：用 atomic.Value 存 ch + close 前先 setFirmwareProgressCh(nil)（已做、但與 close 沒嚴格 happen-before）

production 跑 1 萬次升級會撞上、建議第 2 輪修。

---

## 🟡 Minor（建議修、不阻擋）

| # | 檔案:行 | 問題 | 建議 |
|---|--------|------|------|
| Minor 1 | service.go:36-39 | brickRiskReasons 集合少 `ReasonTimeout`（TDD §3.4 第 6 列「>180s KL720」標 BRICK_RISK）| 加註解或加入集合 |
| Minor 2 | service_test.go / kl720_driver_test.go | 缺 `ctx.Done` 後 sendCommand goroutine 釋放測試、缺多 device 平行 upgrade 測試 | 補 `TestUpgradeFirmware_CtxCancelReleasesBridge` + `TestUpgradeFirmware_MultiDeviceParallel` |
| Minor 3 | kl720_driver.go:830-840 | 早退路徑（pythonReady=false / unsupported chip）defer 仍把 needsReset 設 true、語意不準 | 加 `if !attemptedUpgrade { return }` guard |
| Minor 4 | service.go:189-196 | 「結論先寫、推論在前」順序、初讀者要多看一次 | 把結論移到段首 |
| Minor 5 | kl720_driver.go:758-797 | fwMu 取 ch 後 release 才 send 的 atomic 缺口（同 Major 2 同源、但跨 session lifecycle 風險）| 與 Major 2 一起處理 |

---

## 💡 Suggestion（純改善建議）

1. **kl720_driver.go:925-937**：sendCommand 成功後補 done event 容錯、會與 stderr 推的 done event 重複。M9-3 wire 到 WS 後前端可能跑兩次 cleanup。建議 service.runUpgrade forward loop 對 `StageDone` 去重、或前端 idempotent guard。
2. 補 `TestUpgradeFirmware_MultiDeviceParallel` 3 device 同時 upgrade、驗 ProgressTracker key 不誤匹配。
3. `service.go:303-324` ListBundledVersions 加 `os.Stat(chipDir)` lightly check、區分 chip 不支援 vs firmware 漏 build。
4. `bridgeFirmwareEvent` struct 與 `firmware.FirmwareProgress` 共用 struct + 雙 JSON tag、避免欄位增刪維護兩處。
5. `progress.go:25` `Task.cancel func()` field 目前 service 寫入但沒 reader、加 TODO 或 godoc 註解說明預留 SIGTERM force-cancel。

---

## 4 個情境 stage 序列在 Go driver 端的對應

| 情境 | bridge 行為（M9-1）| Go driver 處理 | 結果 |
|------|---------|--------------|------|
| KL520 KDP1 + loader | 5 stage：preparing → loading → flashing → verifying → done | stderr scanner 偵 5 line JSON → tryRouteFirmwareEvent → forward intermediate → runUpgrade 補欄位 → task.ProgressCh | ✅ TestUpgradeFirmware_SuccessFiveStages 驗證 |
| KL520 KDP2 short-circuit | 4 stage：preparing → flashing → verifying → done（無 loading） | 同上、4 event forward；driver 端對 stage 順序無 hardcode 校驗 | ✅ 不誤判 |
| KL720 legacy（無 loader） | 4 stage：preparing → flashing → verifying → done | 同上、4 event forward | ✅ TestKL720KDPLegacy 等效驗證 |
| 已 KDP2 / no-op | bridge 端仍走 verify rescan 確認 | done event 推到 progressCh + sendCommand 回 success 後 line 928 補 done event 容錯（雙保險、可能 done 出現 2 次、Suggestion 1）| ✅ 雙保險（但需去重） |

---

## TDD 對齊度評估

| TDD 章節 | 對應實作 | 對齊狀態 |
|---------|---------|---------|
| §4.2 FirmwareProgress 9 欄位 | types.go:34-53 | ✅ 完全對齊 |
| §4.3 Stage 列舉 6 個 | types.go:56-63 | ✅ |
| §5.1 A 階段流程 | service.go + driver.UpgradeFirmware | ✅ |
| §5.2 B2 降版流程 | 留 B2、本期 only Upgrade | ✅ Direction enum 兩值都保留 |
| §6.1 8 reason enum | types.go:73-82 | ✅ |
| §6.1 handler 回傳格式 | bridgeFirmwareEvent struct kl720_driver.go:731-744 | ✅ JSON tag snake_case 對齊 bridge.py |
| §8.6.1 HasActiveTask / RequestShutdown / WaitForActiveTasks | service.go:258-293 | ✅ |
| §8.6.2 GetActiveTaskInfo（含 EtaSeconds） | service.go:266-268 + progress.go:108-141 | ✅ |
| §8.6.3 SIGTERM handler 整合 main.go | 延後 M9-3 | ✅ service 端介面齊備 |

---

## Concurrency 評估（重點關注）

整體 goroutine / mutex / channel 拓樸：

```
service.UpgradeFirmware（caller 同步）
  └─ taskWg.Add(1)
  └─ go runUpgrade
        ├─ go drv.UpgradeFirmware(ctx, chip, intermediate)
        │     └─ go func() sendCommand → resCh
        │     └─ select ctx.Done / resCh → 推 event 到 intermediate
        │     └─ return → defer setFirmwareProgressCh(nil)
        ├─ for ev := range intermediate { forward + 補欄位 → task.ProgressCh }
        ├─ <-driverDone（err）
        ├─ if err && lastStage != Done/Error → 補 error event
        └─ defer close(task.ProgressCh) → markDone → taskWg.Done

外部 stderr goroutine（在 driver.startPython 內、process-level）
  └─ scanner.Scan() loop
       └─ if firmware_progress prefix → tryRouteFirmwareEvent → 取 fwProgressCh → write
```

**Race / deadlock 詳細分析見上方 Major 1 / Major 2。**

---

## M9-1 follow-up 清理驗證

| 項目 | 預期 | 實際 | 狀態 |
|------|------|------|------|
| m5 行 616 死碼刪除 | 砍 `bridge._firmware_upgrade_start_ts` | line 615-617 改為只設 `_firmware_upgrade_in_progress = False` + Reviewer 註解 | ✅ |
| m5 行 632 死碼刪除 | 同上 | line 631-636 改為 `start_ts = time.monotonic()` 本地變數 + 註解明示 closure capture | ✅ |
| s5 註解 | 第二次 register 的「刻意設計」說明 | line 684-688 註解明確說明 idempotent shape + retest install→unregister | ✅ |

**正面評價**：M9-1 reviewer 留下的問題清單在 M9-2 確實被執行。

---

## 結論

- ✅ **通過 with 2 Major + 5 Minor + 5 Suggestion**
- ✅ **不阻擋 M9-3 啟動**——Major 1 / 2 是 driver 內部 concurrency 不影響 M9-3（HTTP handler + WebSocket）介面對齊；M9-3 可平行起手、Major 1 / 2 第 2 輪修
- ✅ **不需要升級給 security agent**——security 軸無 OWASP / authentication / authorization / injection 等深審需求
- ⚠️ **建議 backend 第 2 輪修 Major 1 + Major 2**：mutex 範圍 + close-channel race 是 production 跑萬次後會踩雷的問題、修起來成本不高、值得在 M9-2 收尾前修完