jiaozhiwang
·
2026-03-26
DISCUSS-CODE-REVIEW.md
1# DISCUSS: 代码审查 — Bug、风险与优化建议
2
3日期：2026-03-26
4来源：Claude 审查 conductor-daemon、codexd、browser-request-policy、firefox-bridge、firefox-ws 全部 TypeScript 源码
5
6---
7
8## 进度评估
9
10代码成熟度相当高。T-S001~T-S020 的质量在代码层面是可验证的：
11
12- conductor-daemon 的 leader lease / heartbeat / scheduler 三件套完整，单节点自选举可用
13- codexd 的 app-server 模式（stdio transport）完整，session/turn 生命周期管理到位
14- firefox-bridge 的 SSE stream session 有 open/idle timeout、buffer overflow 保护、seq tracking，设计远超 MVP
15- browser-request-policy 的限流/抖动/退避/熔断全部实现且已参数化
16- 手写 WebSocket 帧编解码（firefox-ws.ts）避免了 ws 库依赖，轻量且正确
17
18总体评价：**可上线质量的 MVP**，以下发现的问题多数是边缘场景。
19
20---
21
22## Bug（需修复）
23
24### BUG-A: writeHttpResponse drain handler 可能永远不触发
25
26**文件**：`apps/conductor-daemon/src/index.ts`，`writeHttpResponse()` 函数
27
28**问题**：
29
30```typescript
31if (!writableResponse.write(payload.body)) {
32 await new Promise<void>((resolve) => {
33 writableResponse.on?.("drain", resolve);
34 });
35}
36```
37
38如果连接在 `write` 返回 false 之后、drain 触发之前被关闭（客户端断开），这个 Promise 永远不 resolve。结果：
39
40- 请求处理协程挂死
41- 对应的 HTTP server connection 对象无法被 GC
42- 如果并发请求多，会逐渐耗尽内存
43
44同样的模式在 streamBody 循环里也出现了一次。
45
46**修复建议**：
47
48```typescript
49await new Promise<void>((resolve) => {
50 const onDrain = () => { cleanup(); resolve(); };
51 const onClose = () => { cleanup(); resolve(); };
52 const cleanup = () => {
53 writableResponse.off?.("drain", onDrain);
54 writableResponse.off?.("close", onClose);
55 };
56 writableResponse.on?.("drain", onDrain);
57 writableResponse.on?.("close", onClose);
58});
59```
60
61**严重度**：Medium-High（生产环境慢客户端/代理断连时会触发）
62
63---
64
65### BUG-B: browser-request-policy waiter 无超时，可永久挂起
66
67**文件**：`apps/conductor-daemon/src/browser-request-policy.ts`
68
69**问题**：
70
71```typescript
72private async acquireTargetSlot(state: BrowserRequestTargetState): Promise<void> {
73 if (state.inFlight < this.config.concurrency.maxInFlightPerClientPlatform
74 && state.waiters.length === 0) {
75 state.inFlight += 1;
76 return;
77 }
78 await new Promise<void>((resolve) => {
79 state.waiters.push(resolve);
80 });
81}
82```
83
84如果持有 slot 的请求因为 Firefox 断连或 conductor 内部错误而没有调用 `lease.complete()`，这个 waiter Promise 永远不被 resolve。后续所有对同一 target 的请求都会卡死。
85
86`acquirePlatformAdmission` 有同样的问题。
87
88**修复建议**：
89
901. 给 waiter 加超时（例如 120s），超时后 reject 并从 waiters 数组中移除
912. 或者在 `BrowserRequestPolicyLeaseImpl` 的析构/finalize 路径中确保 `complete()` 一定被调用
923. 在 `completeRequest` 的 `releaseTargetSlot` 中加防御：如果 `inFlight` 已经是 0 但 waiters 非空，说明 slot 泄漏，应主动释放一个 waiter
93
94**严重度**：Medium（当前 maxInFlightPerClientPlatform=1，一次泄漏就会永久卡死该 target）
95
96---
97
98### BUG-C: stream session openTimer/idleTimer 在 closed 后可能触发
99
100**文件**：`apps/conductor-daemon/src/firefox-bridge.ts`，`FirefoxBridgeApiStreamSession`
101
102**问题**：
103
104`resetOpenTimer` 和 `resetIdleTimer` 设置了 setTimeout，但 `finishWithEvent`（关闭 session）只把 `closed = true`，没有清除 openTimer 和 idleTimer。如果关闭后 timer 触发，会调用 `fail()` → `markError()` → 检查 `closed` 返回 false，看起来安全。但 `fail` 内部会再调 `markError` → `finishWithEvent`，其中 `onClose(requestId)` 会被重复调用（如果 timer 在 close 之后的事件循环 tick 中触发）。
105
106**实际影响**：较小，因为 `closed` flag 做了保护。但 timer 没清会导致 session 对象无法被 GC 直到 timer 触发。
107
108**修复建议**：在 `finishWithEvent` 中加：
109
110```typescript
111if (this.openTimer) { this.clearTimeoutImpl(this.openTimer); this.openTimer = null; }
112if (this.idleTimer) { this.clearTimeoutImpl(this.idleTimer); this.idleTimer = null; }
113```
114
115**严重度**：Low（有 closed 保护，影响仅是短暂内存延迟释放）
116
117---
118
119## 风险（非 Bug 但需关注）
120
121### RISK-1: local-api.ts 5434 行，单文件过大
122
123**问题**：
124
125一个文件承载了全部 HTTP 路由处理（describe、browser、codex、host-ops、tasks、status），任何改动都需要在 5000+ 行中定位，merge conflict 概率高。
126
127**建议**：按 route group 拆分为：
128
129| 文件 | 职责 | 预估行数 |
130|---|---|---|
131| `local-api.ts` | 路由分发 + 共享工具 | ~500 |
132| `routes/describe.ts` | describe 读面 | ~400 |
133| `routes/browser.ts` | browser 系列 | ~1500 |
134| `routes/codex.ts` | codex 系列 | ~300 |
135| `routes/host-ops.ts` | exec/files | ~200 |
136| `routes/tasks.ts` | tasks/runs | ~300 |
137| `routes/system.ts` | system state/pause/resume | ~200 |
138
139**优先级**：Medium（不影响功能，但影响开发效率和 codex 任务分配）
140
141---
142
143### RISK-2: normalizeOptionalString 重复 7 次
144
145**文件**：分布在 conductor-daemon (4处)、codexd (1处)、status-api (1处)、codex-exec (1处)
146
147**问题**：
148
149同一个函数被复制了 7 次，且签名不完全一致：
150- 4 处接受 `string | null | undefined`
151- 2 处接受 `unknown`
152- 1 处返回 failure 对象
153
154**建议**：提取到 `packages/shared-utils/` 或直接放在已有的 `packages/logging/` 中导出。
155
156**优先级**：Low（不影响功能，但增加维护负担）
157
158---
159
160### RISK-3: @ts-ignore 导入 status-api 构建产物
161
162**文件**：`apps/conductor-daemon/src/local-api.ts` 顶部
163
164```typescript
165// @ts-ignore conductor reuses the built status-api snapshot normalizer directly.
166import { createStatusSnapshotFromControlApiPayload } from "../../status-api/dist/...";
167// @ts-ignore conductor reuses the built status-api HTML renderer directly.
168import { renderStatusPage } from "../../status-api/dist/...";
169```
170
171**问题**：
172
173- `@ts-ignore` 跳过了类型检查，如果 status-api 的导出接口变了，编译不会报错，运行时才崩
174- conductor-daemon 构建依赖 status-api 已完成构建，增加了构建顺序耦合
175- 这是 STATUS_SUMMARY 里提到的已知技术债，但还没有跟踪卡
176
177**建议**：
178
1791. 近期：把 `createStatusSnapshotFromControlApiPayload` 和 `renderStatusPage` 提取到 `packages/status-renderer/`
1802. 远期：status-api 瘦身后直接合入 conductor-daemon
181
182**优先级**：Medium（已在 STATUS_SUMMARY 低优先级 backlog 中，但缺正式任务卡）
183
184---
185
186### RISK-4: codexd ensureAppServerClient 并发初始化窗口
187
188**文件**：`apps/codexd/src/daemon.ts`，`ensureAppServerClient()`
189
190**问题**：
191
192初始化失败后 `finally` 中设 `appServerInitializationPromise = null`，如果多个 caller 同时等待，失败后它们都会看到 null 并各自发起新的初始化。虽然功能上是 retry，但可能导致并发创建多个 transport 连接。
193
194**实际影响**：较小（当前 codexd 是串行使用的），但 task scheduler 上线后可能并发调用。
195
196**建议**：加一个 `initializationInProgress` lock 或 cool-down 计时器。
197
198**优先级**：Low（task scheduler 上线前不会触发）
199
200---
201
202### RISK-5: writeHttpResponse streamBody 循环无 abort 检查
203
204**文件**：`apps/conductor-daemon/src/index.ts`，`writeHttpResponse()`
205
206**问题**：
207
208```typescript
209for await (const chunk of payload.streamBody) {
210 if (writableResponse.destroyed === true) { break; }
211 ...
212}
213```
214
215只检查了 `destroyed`，没检查传入的 `request.signal`。如果客户端取消请求但 socket 还没 destroy（比如 HTTP/2 RST_STREAM），async generator 会继续迭代直到自然结束。
216
217**实际影响**：较小（当前 SSE stream 有 idle timeout 保护）。
218
219**优先级**：Low
220
221---
222
223## 优化建议
224
225### OPT-1: browser-request-policy 的 Map 永远不清理
226
227`platforms` 和 `targets` 两个 Map 只增不删。如果长时间运行且 Firefox 重连时 clientId 变化，旧的 targetState 会累积。
228
229建议加一个定期清理（比如每小时清除 lastSuccessAt 超过 24h 且 inFlight=0 且 waiters=[] 的 target）。
230
231### OPT-2: conductor-daemon index.ts 也有 1300+ 行
232
233`ConductorDaemon` 和 `ConductorRuntime` 类 + CLI 解析 + 辅助函数都在一个文件里。可以拆分为：
234
235- `daemon.ts` — ConductorDaemon 类
236- `runtime.ts` — ConductorRuntime 类
237- `cli.ts` — CLI 解析和 main 入口
238- `config.ts` — 配置解析和校验
239
240### OPT-3: 手写 WebSocket 帧编解码没有处理分片帧
241
242`firefox-ws.ts` 的帧解析只处理 FIN=1 的完整帧。如果 Firefox 插件发送分片帧（FIN=0），会被当作无效帧丢弃。当前可能不会触发（浏览器扩展通常不分片），但不符合 RFC 6455。
243
244---
245
246## 测试覆盖评估
247
248- conductor-daemon 有 `index.test.js`，覆盖了 daemon 生命周期和 fixture 清理
249- codexd 有 `index.test.js`，覆盖了 session/turn 流程
250- browser-request-policy 的限流/熔断逻辑**没有专门测试文件**
251- firefox-bridge 的 stream session（最复杂的组件之一）**没有专门测试文件**
252- firefox-ws 的帧编解码**没有专门测试文件**
253
254建议优先补 browser-request-policy 和 firefox-bridge 的单元测试。
255
256---
257
258## 总结
259
260| # | 类型 | 问题 | 严重度 | 行动 |
261|---|---|---|---|---|
262| BUG-A | Bug | drain handler 永久挂起 | Medium-High | 加 close 监听 |
263| BUG-B | Bug | policy waiter 无超时 | Medium | 加超时或保证 complete |
264| BUG-C | Bug | stream timer 未清除 | Low | finishWithEvent 里清 timer |
265| RISK-1 | 风险 | local-api.ts 5434 行 | Medium | 拆分路由模块 |
266| RISK-2 | 风险 | normalizeOptionalString x7 | Low | 提共享包 |
267| RISK-3 | 风险 | @ts-ignore 导入 | Medium | 提共享包 |
268| RISK-4 | 风险 | ensureAppServerClient 并发 | Low | 加 lock |
269| RISK-5 | 风险 | stream 无 abort 检查 | Low | 检查 signal |
270| OPT-1 | 优化 | policy Map 不清理 | Low | 定期清理 |
271| OPT-2 | 优化 | index.ts 1300+ 行 | Low | 拆文件 |
272| OPT-3 | 优化 | WS 不处理分片帧 | Low | 按需处理 |
273| TEST | 测试 | policy/bridge/ws 缺测试 | Medium | 补单元测试 |