tao.chen/model-platform
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
cdcfcb2e4313f3bbf640bca241cb0edcac5c661b
model-platform/CODE_REVIEW.md
T
tao.chen cbfbee7fe0 chore: add CODE_REVIEW.md
2026-08-14 20:55:59 +08:00

15 KiB
Raw Blame History

代码审查报告

  • 审查日期：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 View Git Blame Copy Permalink