chore: add CODE_REVIEW.md
This commit is contained in:
tao.chen
+202
@@ -0,0 +1,202 @@
|
|||||||
|
# 代码审查报告
|
||||||
|
|
||||||
|
- 审查日期：2026-08-14
|
||||||
|
- 审查范围：backend / common / runtime / schedule / frontend / migrations / 部署配置（约 1.5 万行 Python + 50 个前端文件）
|
||||||
|
- 审查方式：Ruff 静态扫描 + 逐模块人工逻辑审查
|
||||||
|
- 严重度约定：🔴 高（功能错误 / 安全风险，应尽快修复）｜🟡 中（特定条件下出错或资源泄漏）｜🔵 低（健壮性 / 可维护性问题）
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# 第一部分：静态扫描结果
|
||||||
|
|
||||||
|
## 总体健康状况
|
||||||
|
|
||||||
|
| 检查项 | 结果 |
|
||||||
|
| --- | --- |
|
||||||
|
| `python -m compileall`（4 个包） | 通过 |
|
||||||
|
| `pytest backend/tests` | 37 passed（含 2026-08-14 数据资源链路新增用例） |
|
||||||
|
| 前端 `tsc --noEmit` | 零错误 |
|
||||||
|
| Ruff | 391 条（198 条可 `--fix` 自动修复） |
|
||||||
|
| 危险模式（eval / shell=True / dangerouslySetInnerHTML / SQL 拼接） | 未发现 |
|
||||||
|
| `.env` 入库 | 未入库（.gitignore 已覆盖） |
|
||||||
|
|
||||||
|
## Ruff 规则分布（Top）
|
||||||
|
|
||||||
|
| 规则 | 数量 | 说明 |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| B008 | 147 | FastAPI `Depends()`/`Query()` 惯用法误报，建议配置忽略或 `extend-immutable-calls` |
|
||||||
|
| UP045 | 108 | `Optional[X]` → `X \| None`，可自动修复 |
|
||||||
|
| I001 | 38 | import 排序，可自动修复 |
|
||||||
|
| BLE001 | 19 | 盲捕 `Exception`（runtime/process.py 占 11 处），多数为兜底、可接受 |
|
||||||
|
| F401 | 16 | 未使用 import，可自动修复 |
|
||||||
|
|
||||||
|
其余：F811 ×3（storage_api.py 本地 def 覆盖顶部同名 import）、DTZ003 ×4（platform.py 使用已弃用的 `datetime.utcnow()`）、F841 ×2（scripts.py 赋值未使用）等。
|
||||||
|
|
||||||
|
## 静态扫描安全发现
|
||||||
|
|
||||||
|
1. 🔴 **docker-compose 端口映射与"仅 Nginx 对外"架构矛盾**：`backend`（8891）和 `runtime`（8892）实际映射了宿主机端口，但注释声称 "No host port"。后果：
|
||||||
|
- backend `/internal/v1/*` 存储 API 无 JWT 认证，直接信任请求体中的 `user_id`/`workspace_id`（services/storage.py:215）；
|
||||||
|
- runtime `POST /api/v1/jupyter` 无认证，可对任意 workspace 启动/停止 Jupyter。
|
||||||
|
建议：删除 `ports` 映射或绑定 `127.0.0.1`；为 `/internal/*` 增加 service token。
|
||||||
|
2. 🟡 `INITIAL_ADMIN_PASSWORD` 默认 `admin12345`，且当前 `.env` 实际使用该值。
|
||||||
|
3. 🟡 `/api/v1/auth/login` 无速率限制 / 账户锁定，可在线爆破（401 统一文案防枚举做得对）。
|
||||||
|
4. 🔵 `jwt_secret` 存在 `"dev-only-not-for-production"` 兜底默认值（当前 .env 已配置真随机值），建议生产启动时 fail-fast 校验。
|
||||||
|
|
||||||
|
## 测试覆盖
|
||||||
|
|
||||||
|
backend 14.4k 行代码仅 3 个测试文件 / 30 个用例；runtime、schedule、common 无测试。CLAUDE.md 中记录的 platform 员工管理 guard 顺序等 load-bearing 逻辑无回归测试兜底。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# 第二部分：逐模块逻辑审查
|
||||||
|
|
||||||
|
## 1. backend — 存储层（storage_api.py / services/storage.py）
|
||||||
|
|
||||||
|
**🔴 B1. 上传失败/过期状态永远不落库**
|
||||||
|
`services/storage.py` `upload_bytes_to_session` 中 `upload.upload_status = "expired" / "failed"` 三处赋值后紧跟 `raise HTTPException`，而 `session_scope`（common/db/session.py）遇异常即回滚——这些状态迁移全部被撤销。失败的上传会话在库里永远停在 `created/uploading`。IntegrityError 路径更显眼：先置 `failed` 再显式 `rollback()`，把自己的赋值也回滚了。
|
||||||
|
> 2026-08-14 部分处理：IntegrityError 分支的无效赋值与显式 `rollback()` 已删除（该分支现靠命名锁前置避免，见下方"数据资源链路复审"第 3 项）；"failed/expired 状态被 session_scope 回滚"的根因仍在，待修。
|
||||||
|
|
||||||
|
**🟡 B2. 软删除大对象整体读入内存**
|
||||||
|
`soft_delete_object` 用 `get()`（返回 `bytes`）把对象全量加载后复制到 trash 桶。nginx 允许 100G 上传，删除大文件会 OOM。S3 应走 server-side copy，local 后端已有 `copy()`。
|
||||||
|
|
||||||
|
**🔵 B3. `USAGE_TYPE_TO_PURPOSE[item.usage_type]` 直接索引**
|
||||||
|
未知 usage_type 抛 KeyError → 500。同文件 `_resolve_bucket_for_usage` 是优雅降级的，两处策略不一致。
|
||||||
|
> ✅ 2026-08-14 已修复：改为 `.get(item.usage_type, "workspace")` 兜底。
|
||||||
|
|
||||||
|
**🔵 B4. 上传 IntegrityError 后 S3 对象成孤儿**
|
||||||
|
先 `put` 后 `flush`，flush 失败只回滚 DB，已写入的对象字节无补偿删除（scripts.py 的 Jupyter 文件有补偿清理，这里没有，不对称）。
|
||||||
|
|
||||||
|
## 2. backend — 脚本模块（scripts.py，1552 行）
|
||||||
|
|
||||||
|
**🔴 B5. `update_script` 不回写 DB 元数据**
|
||||||
|
编辑保存后只更新 `script.updated_at`，新的 `content_hash`/`size_bytes` 只拼在响应 dict 里——`StorageObjects` 行永远保留创建时的旧哈希和大小。且响应里 `relative_path` 用了 `jupyter_path` 格式（无 `workspace/` 前缀），与库中存储格式不一致。
|
||||||
|
|
||||||
|
**🟡 B6. `delete_script` 不删 Jupyter 侧实际文件**
|
||||||
|
只软删 DB 行并把 S3 字节移入 trash，工作区挂载里的文件保留。创建路径的注释明确说 "rclone VFS 会把文件复制回对象存储"——残留文件存在被回写"复活"的风险，且用户在 Jupyter 里仍能看到/编辑已删除的脚本。
|
||||||
|
|
||||||
|
**🔵 B7. 日志泄露内容**
|
||||||
|
`logger.debug(notebook)` 把整本 notebook JSON 写进日志；`logger.debug(jupyter_resp)` 同理。`log_level` 默认即为 DEBUG。
|
||||||
|
|
||||||
|
**🔵 B8. `_derive_jupyter_path` 的 fallback 可疑**
|
||||||
|
无 StorageObjects 行时用 `script_id` 当文件名（`<ULID>.ipynb`），与创建时按用户文件名命名的规则不匹配——"jupyter-only" 脚本的读写会打到不存在的文件上。若为 legacy 数据准备，需写清来源。
|
||||||
|
|
||||||
|
## 3. backend — 其余（auth / dependencies / jupyter / platform / admin / schedule_runs）
|
||||||
|
|
||||||
|
- ✅ JWT 验签固定 HS256、防 alg 混淆、`compare_digest`；登录 401 统一文案防枚举；`request_context` 强制显式 `workspace_id`；platform.py 的 last-admin 守卫顺序与 CLAUDE.md 约定一致。
|
||||||
|
- 🔵 `schedule_runs.py` 手动触发接口允许 `payload.reason == "cron"` 把 `trigger_type` 伪装成 cron，审计字段失真。
|
||||||
|
- 🔵 手动 `run_schedule_now` 不校验 `schedule.enabled`，已禁用的调度仍可手动跑（需确认是否有意）。
|
||||||
|
- 🔵 nginx 对 `/jupyter/` 响应 `add_header X-Debug-Full-Url ... always`，向浏览器暴露内部 upstream 地址，调试头应下线。
|
||||||
|
|
||||||
|
## 4. common（config / auth / db / eventing / storage / scheduler trigger）
|
||||||
|
|
||||||
|
- ✅ jwt.py、passwords.py、local 后端 `_resolve` 路径穿越防护、eventing.py 均无问题。
|
||||||
|
- 🟡 C1. `trigger.py` 模块 docstring 声称 "executor 会在执行前 re-verify `Users.status='active'`"，但 worker.py 全文没有任何 Users 校验——**被停用/删除用户的既有调度仍会照常执行**。文档与实现二选一。
|
||||||
|
- 🔵 C2. bcrypt 72 字节上限：schema 限 `max_length=72` 字符，但按 UTF-8 字节算 24 个汉字就超限，`hash_password` 抛 ValueError → 注册 500。应按字节校验或截断。
|
||||||
|
- 🔵 C3. local 后端 `get_url` 返回 `file://` URI，浏览器无法使用——local 模式下"下载链接"功能实际不可用。
|
||||||
|
- 🔵 C4. 敏感配置均有默认值兜底，生产漏配会静默使用弱密钥（加固建议：默认值时启动 fail-fast）。
|
||||||
|
|
||||||
|
## 5. runtime（main / mount / process）
|
||||||
|
|
||||||
|
**🟡 R1. reaper 清理死进程无锁，可误删新记录**
|
||||||
|
`_reap_loop` 基于循环开头的快照判断 `poll() != None` 后直接 `del JUPYTER_PROCESSES[ws_id]`。若此间 `start_workspace`（持锁）已删掉旧记录、启动新进程并写入新记录，reaper 这一 `del` 删掉的是**新记录**——新 Jupyter 脱离注册表，永不回收（端口/进程泄漏）。
|
||||||
|
|
||||||
|
**🟡 R2. `_drop_workspace_lock` 破坏互斥**
|
||||||
|
stop 后移除锁：正在 await 旧锁的协程仍持有旧锁对象，新调用者创建新锁——同一 workspace 出现两把锁，start 可并发。建议不删锁或引用计数。
|
||||||
|
|
||||||
|
**🟡 R3. rclone RC 无认证监听 0.0.0.0:5572**
|
||||||
|
`--rc --rc-no-auth`——Docker 网络内任何容器可读写整个 workspace bucket。建议绑定 127.0.0.1 或加 rc-user/rc-pass。
|
||||||
|
|
||||||
|
- 🔵 R4. `stop_workspace` 在事件循环里同步 `process.wait(timeout=3)`，逐 workspace 串行阻塞事件循环。
|
||||||
|
- 🔵 R5. start 复用路径对超 `MAX_LIFETIME` 的进程只 warn 仍复用，而 reaper 会在 60s 内按 max-lifetime 杀掉它——两处策略矛盾。
|
||||||
|
- 🔵 R6. `get_free_port()` 到 Jupyter bind 之间 TOCTOU；`JupyterProcessRecord` TypedDict 漏声明实际写入的 `workspace_path` 键。
|
||||||
|
- 🔵 R7. `/tmp/rclone-mount.log` 追加写入无轮转。
|
||||||
|
|
||||||
|
## 6. schedule（service / scheduler / orchestrator / worker / execution）
|
||||||
|
|
||||||
|
**🔴 S1. worker 下载产物忽略 bucket，与发布路径矛盾**
|
||||||
|
`_download_artifact` 只用 `object_key`，`build_object_store()` 固定指向 version bucket；但发布路径 `_resolve_bucket_for_usage` 尊重 `workspace.artifact_bucket` 覆盖。**凡是配置了 artifact_bucket 的 workspace，其调度运行必然找不到产物**。service.py 中 "regardless of the artifact's workspace 是正确解析" 的注释在该场景下是错的。
|
||||||
|
|
||||||
|
**🟡 S2. 重试耗尽后 run 永久卡死**
|
||||||
|
outbox 事件重试 5 次后标记 `failed`，但对应的 `node_run` 永远停在 `queued/running`，`_advance_run` 再无机会触发——run 永远 `running`，无 run 级超时或 janitor 兜底。
|
||||||
|
|
||||||
|
**🟡 S3. 单个坏 cron 表达式毒化整个同步循环**
|
||||||
|
`_sync_once` 遍历中直接 `CronTrigger.from_crontab(...)` / `ZoneInfo(item.timezone)`，任一 schedule 抛异常即中断整轮同步，所有 cron 调度停止 reconcile（外层 5s 重试同样失败）。需要 per-schedule try/except 隔离。
|
||||||
|
|
||||||
|
- 🔵 S4. 每 5 秒对所有 job 无条件 `reschedule_job`，churn 无谓；可比对表达式后再 reschedule。
|
||||||
|
- 🔵 S5. `process_pending_events` 的 select 未加 `skip_locked`——单副本靠 `dispatch_lock` 没问题，多副本部署会重复认领（inbox 幂等兜底，但浪费执行）。
|
||||||
|
- 🔵 S6. `notebook_runner.py` 用 `"timeout" in type(exc).__name__.lower()` 判定超时映射 exit 124，依赖 nbclient 异常类名，依赖升级会静默失效。
|
||||||
|
- 🔵 S7. scheduler.py / service.py 模块 docstring 仍写 "cron tick posts back to Backend"，实际早已直写 DB——过时注释。
|
||||||
|
- ✅ orchestrator 的 DAG 推进逻辑（重试次数语义、skip 条件、failure_policy、崩溃恢复、bootstrap 去重 flush）逐条推演未发现错误；租约绑定 `timeout_seconds + slack` 的设计正确。
|
||||||
|
|
||||||
|
## 7. frontend
|
||||||
|
|
||||||
|
- ✅ `tsc` 零错误；无 `dangerouslySetInnerHTML`/`eval`；zustand 绑定模式按 CLAUDE.md 约定实现。
|
||||||
|
- 🟡 F1. `api.ts` 的 `heartbeatFileLock`/`releaseFileLock` 是纯前端 stub，不触达任何后端接口——"编辑锁"实际只存在于当前标签页内存，刷新即失效。注释自称 compatibility mode，需确认是预期终态还是临时方案。
|
||||||
|
- 🔵 F2. `waitForJupyterReady` 400ms×10 ≈ 4s 总超时，对冷启动 Jupyter（runtime 侧允许 30s）明显偏短，用户会偶发看到"启动超时"。
|
||||||
|
|
||||||
|
## 8. 部署与迁移
|
||||||
|
|
||||||
|
- 🔴 docker-compose 端口暴露（见第一部分安全发现 1），与 R3 叠加后攻击面从"内网"扩到"宿主机可达"，为全库最高优先项。
|
||||||
|
- 🟡 `INITIAL_ADMIN_PASSWORD=admin12345` 同时出现在 compose 默认值和当前 `.env`。
|
||||||
|
- ✅ migrations 单 baseline 文件结构干净；`.env` 未入库；JWT_SECRET 已是真随机值。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# 附：2026-08-14 数据资源上传/绑定/删除链路复审（已全部修复）
|
||||||
|
|
||||||
|
针对 `POST /uploads` → `PUT /uploads/{id}` → `POST /uploads/{id}/bind` → `DELETE /{resource_id}`
|
||||||
|
链路的专项复审。起因：个人根目录已存在文件 `a` 时，子目录上传同名文件报
|
||||||
|
"a data resource with this name already exists in this workspace"——原查重按
|
||||||
|
workspace 全局 `resource_name` 唯一，不区分目录。本轮修复含语义调整与 10 个附带问题，
|
||||||
|
改动文件：`backend/resources.py`、`backend/services/storage.py`、`backend/storage_api.py`、
|
||||||
|
`common/db/models/storage.py`（仅注释）、`backend/tests/test_resources.py`。
|
||||||
|
|
||||||
|
**语义调整（需求确认）**
|
||||||
|
|
||||||
|
- 同名查重维度：`workspace + resource_name` → `owner + 目录 + resource_name`。
|
||||||
|
目录从绑定的 `StorageObjects.object_key`（`{ws}/{user}/{target_path}/{file_name}`）
|
||||||
|
解析（`resources.py::resource_directory`），同目录同名才 409；不同目录、不同用户均可重名。
|
||||||
|
- 回收站防覆盖：`trash_key` 由 `{purpose}/{object_key}` 改为
|
||||||
|
`{purpose}/{object_key}-{storage_object_id}`，同名文件多次删除不再互相覆盖；
|
||||||
|
restore 时剥掉 id 后缀拷回原 key（`endswith` 判断，旧格式数据天然兼容）。
|
||||||
|
|
||||||
|
**修复清单（按审查时严重度排序）**
|
||||||
|
|
||||||
|
1. 🔴 bind 不幂等：成功后的重试必被同名查重误伤 409。→ storage object 复用检查前置，
|
||||||
|
已有 active 绑定行直接返回（`reused: true`）。
|
||||||
|
2. 🔴 删除后重新 bind 复用 `status="deleted"` 的"尸体"行（无 status 过滤），200 返回已删除数据。
|
||||||
|
→ 复用查询加 `status == "active"`，旧删除行走新建流程。
|
||||||
|
3. 🔴 PUT 先于 INSERT，同 key 并发上传互相覆盖字节，赢家的 `content_hash` 与实际字节不符。
|
||||||
|
→ `upload_bytes_to_session` 的「PUT + INSERT」临界区按 object_key 加 MySQL 命名锁
|
||||||
|
（新增 `acquire_named_lock`/`release_named_lock`，锁名 sha256 压缩到 64 字符内，
|
||||||
|
finally 释放）；锁内复查占用并换 ULID 后缀新 key。
|
||||||
|
4. 🟡 `delete_resource` 不检查共享引用，直接软删底层对象导致其他 active 资源行失效。
|
||||||
|
→ 删除前统计同 object 的其他 active 行，有引用则只删 DataResources 行。
|
||||||
|
5. 🟡 `soft_delete_object` 未知 usage_type KeyError → 500。→ `.get` 兜底（即原 B3）。
|
||||||
|
6. 🟡 幂等键元数据比对漏 file_name/target_path，同键不同路径会静默复用旧会话。
|
||||||
|
→ 比对加路径维度；已有会话的 key 先经 `_strip_uniqueness_suffix` 去唯一化后缀再比。
|
||||||
|
7. 🔵 bind 不校验 `usage_type`，任意用途的 upload session 可 bind 成数据资源。
|
||||||
|
→ 非 `data_resource` 报 409。
|
||||||
|
8. 🔵 bind 查重无并发保护。→ 查重 + 建行按 `(owner, 目录, 名称)` 加命名锁。
|
||||||
|
9. 🔵 回收站 key 碰撞 + 移入回收站后 `object_key_hash` 过期。→ 见上方语义调整；
|
||||||
|
hash 随 trash key 同步更新。
|
||||||
|
10. 🔵 IntegrityError 分支 `upload_status="failed"` 紧接着 `rollback()` 自我撤销。
|
||||||
|
→ 删除无效赋值与多余 rollback（锁内复查后该分支仅剩理论可能）。
|
||||||
|
|
||||||
|
**遗留说明**
|
||||||
|
|
||||||
|
- 命名锁依赖 MySQL `GET_LOCK`/`RELEASE_LOCK`（锁绑定会话连接，连接归还连接池不自动释放，
|
||||||
|
故必须 finally 显式释放）；更换数据库时需重审。
|
||||||
|
- 测试 37 passed；新增用例覆盖：同目录同名 409、跨目录/跨 owner 同名放行、幂等重 bind、
|
||||||
|
非 data_resource 上传 bind 拒绝、目录/后缀解析等纯函数。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# 修复优先级建议
|
||||||
|
|
||||||
|
1. 端口暴露（第一部分安全发现 1）+ R3（rclone RC 无认证）
|
||||||
|
2. B5（update_script 元数据不落库）——数据正确性，每日发生
|
||||||
|
3. S1（artifact_bucket 覆盖导致调度必然失败）——潜在功能断裂
|
||||||
|
4. S2 + R1/R2（run 卡死、Jupyter 注册表竞态）——长跑后的资源/状态泄漏
|
||||||
|
5. B1（根因仍在，IntegrityError 分支已随数据资源链路复审清理）/ B2 / C1 / F1（状态机失真、大文件 OOM、停用用户仍执行、锁 stub）
|
||||||
|
6. 其余低优项可并入一次 `ruff --fix` + 清理提交
|
||||||
Reference in New Issue
Block a user