Conversation
11dc22a to
e5509b0
Compare
分支已 restack 到当前 main,请重评按维护者建议(#87 上「每个 PR 都基于 main 重新创建,PR 之间的修改不要重叠」),本分支已重建。 问题背景:本分支此前的 merge-base 停在 本次变更:仅把本分支自身的 commit cherry-pick 到当前 main。
有一处需要说明:原 commit 的父提交带有 #114(数据保留清理器)的代码,因此该 commit 的 diff 里混入了一段恢复 即 已验证: 请在新 head 上重评。 |
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明:本评审报告由 AI 自动生成,当前使用 Claude Opus 5 模型进行分析。
🔄 增量评审:上轮 → 本轮
本轮无新增内容改动 —— 分支被 rebase 到 #85 合入后的 main(dc2877b → 26c4301)。已用 git diff 11dc22a1 e5509b0f -- <本PR自身文件> 逐文件核对,结果为空,改动内容与上轮评审时逐字一致(HEAD 变化仅来自 rebase)。
新增改动的问题:无(纯 rebase)。
旧问题解决情况:
- ⏳ [原评审] 本轮 diff 由
-10/+11变为10 insertions(+),即http.Server超时配置从「改动现有字面量」变为「新增显式字段赋值」。语义等价,无新增问题。 - 💡 建议确认
ReadHeaderTimeout是否一并设置(仅设ReadTimeout时,慢速发 header 的连接仍会长时间占用 goroutine 直到 ReadTimeout 到期)。如已在别处处理可忽略。
🎯 结论:🔄 Request Changes — 内容未变,上轮问题状态同上
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @suantea 提交!
🤖 AI 自动审核声明。
🎯 结论
🔄 Request Changes — WriteTimeout=0 的权衡及 hijacked 连接读超时风险需验证
📖 概要
给 http.Server 增加读/空闲超时防止慢客户端资源耗尽(10行改动)
🚨 关键问题
P1:
- 💡
WriteTimeout=0是全局配置,对所有路由生效而非仅 SSE/WS 路由,普通 JSON API 的慢速读取客户端仍可无限期占住连接。建议改用 Go 1.20+ 的http.ResponseController(w).SetWriteDeadline(...),仅在 SSE/WS handler 里显式清除写超时,其余路由保留合理的全局 WriteTimeout。 - 💡
ReadTimeout=30s会在 handler 调用前基于此值设置 deadline,如果 WebSocket/终端升级的 handler 在Hijack()后没有显式清除该 deadline,长连接会在建立后 30 秒被意外断开,与本 PR"不掐断 WS/终端"的设计目标矛盾,需要验证。
P2:若存在大文件上传接口,ReadTimeout 覆盖整个请求体读取过程,慢网络下可能被意外中断。
🎯 结论:🔄 Request Changes — 方向正确(行业标准 Slowloris 防护实践),但需验证 WS/终端场景不受影响后合并
- ReadTimeout 30s / ReadHeaderTimeout 10s / IdleTimeout 120s / MaxHeaderBytes 1MB - WriteTimeout 保持 0:AI 对话与 CF 隧道日志走 SSE、终端走 WebSocket,写超时会掐断长连接
e5509b0 to
9ad41ee
Compare
概述
http.Server此前零超时配置,慢连接可长期占住 goroutine 与文件描述符,影响稳定性。改动
ReadTimeout: 30s/ReadHeaderTimeout: 10s/IdleTimeout: 120s/MaxHeaderBytes: 1MBWriteTimeout保持 0(有意为之):AI 对话与 CF 隧道日志走 SSE 流式响应、监控终端走 WebSocket 升级,任何写超时都会掐断长连接验证
go build/go vet/ 全量go test ./...通过