mirror of
https://github.com/pgsty/minio.git
synced 2026-10-06 17:55:59 +03:00
fix(http): honor configured request header deadlines
Signed-off-by: Feng Ruohang <rh@vonng.com> (cherry picked from commit 0d48d32d7e038ae1ea5966f3d7e0cb86780a6311) Signed-off-by: Feng Ruohang <rh@vonng.com>
This commit is contained in:
Feng Ruohang
@@ -0,0 +1,149 @@
|
||||
diff --git a/cmd/common-main.go b/cmd/common-main.go
|
||||
index 393ab17d0..c127759fc 100644
|
||||
--- a/cmd/common-main.go
|
||||
+++ b/cmd/common-main.go
|
||||
@@ -445,6 +445,7 @@ func buildServerCtxt(ctx *cli.Context, ctxt *serverCtxt) (err error) {
|
||||
ctxt.SendBufSize = ctx.Int("send-buf-size")
|
||||
ctxt.RecvBufSize = ctx.Int("recv-buf-size")
|
||||
ctxt.IdleTimeout = ctx.Duration("idle-timeout")
|
||||
+ ctxt.ReadHeaderTimeout = ctx.Duration("read-header-timeout")
|
||||
ctxt.UserTimeout = ctx.Duration("conn-user-timeout")
|
||||
|
||||
if conf := ctx.String("config"); len(conf) > 0 {
|
||||
diff --git a/cmd/server-main.go b/cmd/server-main.go
|
||||
index 48ed0f87d..c8a3a19ca 100644
|
||||
--- a/cmd/server-main.go
|
||||
+++ b/cmd/server-main.go
|
||||
@@ -901,6 +901,8 @@ func serverMain(ctx *cli.Context) {
|
||||
close(globalGridStart)
|
||||
close(globalLockGridStart)
|
||||
|
||||
+ // The HTTP/1 listener preserves absolute header deadlines and renews the
|
||||
+ // body read/write idle limits, so transfers may outlast IdleTimeout.
|
||||
httpServer := xhttp.NewServer(getServerListenAddrs()).
|
||||
UseHandler(setCriticalErrorHandler(corsHandler(handler))).
|
||||
UseTLSConfig(newTLSConfig(getCert)).
|
||||
diff --git a/internal/deadlineconn/deadlineconn.go b/internal/deadlineconn/deadlineconn.go
|
||||
index 95bb43eff..5fa5a1403 100644
|
||||
--- a/internal/deadlineconn/deadlineconn.go
|
||||
+++ b/internal/deadlineconn/deadlineconn.go
|
||||
@@ -34,6 +34,8 @@ type DeadlineConn struct {
|
||||
net.Conn
|
||||
readDeadline time.Duration // sets the read deadline on a connection.
|
||||
readSetAt time.Time
|
||||
+ readExplicit time.Time // last deadline requested by the caller.
|
||||
+ readDeadlineStrict bool // idle renewal must not extend readExplicit.
|
||||
writeDeadline time.Duration // sets the write deadline on a connection.
|
||||
writeSetAt time.Time
|
||||
abortReads, abortWrites atomic.Bool // A deadline was set to indicate caller wanted the conn to time out.
|
||||
@@ -59,17 +61,31 @@ func (c *DeadlineConn) setReadDeadline() {
|
||||
|
||||
c.mu.Lock()
|
||||
defer c.mu.Unlock()
|
||||
- if c.abortReads.Load() {
|
||||
+ if c.abortReads.Load() || c.infReads.Load() {
|
||||
return
|
||||
}
|
||||
|
||||
now := time.Now()
|
||||
if now.Sub(c.readSetAt) > updateInterval {
|
||||
- c.Conn.SetReadDeadline(now.Add(c.readDeadline + updateInterval))
|
||||
+ deadline := now.Add(c.readDeadline + updateInterval)
|
||||
+ if c.readDeadlineStrict && !c.readExplicit.IsZero() && c.readExplicit.Before(deadline) {
|
||||
+ deadline = c.readExplicit
|
||||
+ }
|
||||
+ c.Conn.SetReadDeadline(deadline)
|
||||
c.readSetAt = now
|
||||
}
|
||||
}
|
||||
|
||||
+// SetReadDeadlineStrict controls whether idle renewal may extend a deadline set
|
||||
+// by SetReadDeadline or SetDeadline. The default is false. Explicit zero and
|
||||
+// past deadlines retain their disable/cancel semantics in either mode.
|
||||
+func (c *DeadlineConn) SetReadDeadlineStrict(strict bool) {
|
||||
+ c.mu.Lock()
|
||||
+ defer c.mu.Unlock()
|
||||
+ c.readDeadlineStrict = strict
|
||||
+ c.readSetAt = time.Time{}
|
||||
+}
|
||||
+
|
||||
func (c *DeadlineConn) setWriteDeadline() {
|
||||
// Do not set a Write deadline, if upstream wants to cancel all reads.
|
||||
if c.writeDeadline <= 0 || c.abortWrites.Load() || c.infWrites.Load() {
|
||||
@@ -115,6 +131,7 @@ func (c *DeadlineConn) SetDeadline(t time.Time) error {
|
||||
defer c.mu.Unlock()
|
||||
|
||||
c.readSetAt = time.Time{}
|
||||
+ c.readExplicit = t
|
||||
c.writeSetAt = time.Time{}
|
||||
c.abortReads.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
c.abortWrites.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
@@ -132,6 +149,7 @@ func (c *DeadlineConn) SetReadDeadline(t time.Time) error {
|
||||
c.abortReads.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
c.infReads.Store(t.IsZero())
|
||||
c.readSetAt = time.Time{}
|
||||
+ c.readExplicit = t
|
||||
return c.Conn.SetReadDeadline(t)
|
||||
}
|
||||
|
||||
diff --git a/internal/http/listener.go b/internal/http/listener.go
|
||||
index bc6de3af9..14d34f6ea 100644
|
||||
--- a/internal/http/listener.go
|
||||
+++ b/internal/http/listener.go
|
||||
@@ -70,7 +70,10 @@ func (listener *httpListener) Accept() (conn net.Conn, err error) {
|
||||
if result.err != nil {
|
||||
return nil, result.err
|
||||
}
|
||||
- return deadlineconn.New(result.conn).WithReadDeadline(listener.opts.IdleTimeout).WithWriteDeadline(listener.opts.IdleTimeout), result.err
|
||||
+ conn := deadlineconn.New(result.conn).WithReadDeadline(listener.opts.IdleTimeout).WithWriteDeadline(listener.opts.IdleTimeout)
|
||||
+ // Server.Init switches to rolling reads only after HTTP/1 headers are read.
|
||||
+ conn.SetReadDeadlineStrict(true)
|
||||
+ return conn, nil
|
||||
case <-listener.ctxDoneCh:
|
||||
}
|
||||
return nil, syscall.EINVAL
|
||||
diff --git a/internal/http/server.go b/internal/http/server.go
|
||||
index 2934fda6c..d9c19a33f 100644
|
||||
--- a/internal/http/server.go
|
||||
+++ b/internal/http/server.go
|
||||
@@ -29,6 +29,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/dustin/go-humanize"
|
||||
+ "github.com/minio/minio/internal/deadlineconn"
|
||||
)
|
||||
|
||||
var (
|
||||
@@ -123,6 +124,32 @@ func (srv *Server) Init(listenCtx context.Context, listenErrCallback func(listen
|
||||
srv.listener = listener
|
||||
srv.listenerMutex.Unlock()
|
||||
|
||||
+ connState := srv.ConnState
|
||||
+ srv.ConnState = func(conn net.Conn, state http.ConnState) {
|
||||
+ raw := conn
|
||||
+ if tlsConn, ok := raw.(*tls.Conn); ok {
|
||||
+ if tlsConn.ConnectionState().NegotiatedProtocol == "h2" {
|
||||
+ // HTTP/2 owns its stream deadlines; do not change the connection.
|
||||
+ raw = nil
|
||||
+ } else {
|
||||
+ raw = tlsConn.NetConn()
|
||||
+ }
|
||||
+ }
|
||||
+ if dc, ok := raw.(*deadlineconn.DeadlineConn); ok {
|
||||
+ switch state {
|
||||
+ case http.StateNew, http.StateIdle:
|
||||
+ dc.SetReadDeadlineStrict(true)
|
||||
+ case http.StateActive:
|
||||
+ // net/http has finished reading the headers, including buffered
|
||||
+ // requests. Keep ReadTimeout as a rolling idle limit for uploads.
|
||||
+ dc.SetReadDeadlineStrict(false)
|
||||
+ }
|
||||
+ }
|
||||
+ if connState != nil {
|
||||
+ connState(conn, state)
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
var l net.Listener = listener
|
||||
if tlsConfig != nil {
|
||||
l = tls.NewListener(listener, tlsConfig)
|
||||
@@ -0,0 +1,16 @@
|
||||
# V1 implementation-review dispositions (carried into v2)
|
||||
|
||||
The real Opus implementation review returned REQUEST_CHANGES with one test/dependency blocker and no production-behavior defect. This is not recorded as implementation approval.
|
||||
|
||||
- B1 accepted: the test's direct x/net/http2 import would change the indirect annotation during tidy. Removed that import and use the Go1.27 standard-library HTTP/2 client with an explicit Protocols set containing only HTTP/2. Keep the server's HTTP/1-first ALPN order, and assert actual h2 negotiation and HTTP/2.0 response. No go.mod/go.sum change. Revalidation and dependency hygiene follow.
|
||||
- N1 accepted: add repeated-renewal recording-connection test across three actual 300ms intervals, without resetting the cap.
|
||||
- N2 noted: the existing concurrent test is a race probe; semantic assertions are supplied by separate deadline and HTTP tests, not inferred from that probe.
|
||||
- N3 accepted for h2: give the fixture a 3s server keep-alive idle period while retaining 400ms ReadTimeout and isolating the native read timer. Small scaled streaming tests retain their prior margin, separately backed by 33s production-default runs.
|
||||
- N4 retained: local conn shadowing is legal, no behavior concern; avoid unrelated cleanup.
|
||||
- N5 retained: redundant deadline calls are low-cost and intentional; no performance redesign.
|
||||
- N6 retained: explicit h2 exclusion documents the multiplexed boundary and is safe; zero deadlines remain the fundamental compatibility mechanism.
|
||||
- N7/N8 accepted: release notes distinguish header/handshake-read tightening from unchanged h2 stream and TLS handshake-write limitations.
|
||||
- N9 resolved with direct final macOS/Linux race logs, then recheck modified fixtures after this review's changes.
|
||||
- N10 accepted: record explicit command and exit status for vet and final checks, not only empty stdout.
|
||||
|
||||
The new common-main.go binding is still unmodified and awaits complete-plan v2 agreement. V1 approval cannot cover it. V2 review must also verify B1's chosen no-new-dependency resolution.
|
||||
@@ -0,0 +1,9 @@
|
||||
{
|
||||
"internal/deadlineconn/deadlineconn.go": "b9272ef640f1d4403b3d0af6cdbaba9186c51ad9a0226dfe449e8ef738e1ec4b",
|
||||
"internal/http/listener.go": "49628575367f6ab9b6986caf594726d74d370f7d2ac4eed582903600b6eb3fa2",
|
||||
"internal/http/server.go": "b7b0355f2781f8c5f7c77bc910cd4180cd3e5f22a87de41bd35ef119d36b4cdf",
|
||||
"cmd/server-main.go": "04c265de211412ba0297396928096d7f2d971244a957a3126154846035263514",
|
||||
"internal/deadlineconn/deadlineconn_strict_test.go": "fec37273f338d3cb6f56c71eb6d8d677f8d5e4c2a7c55bc5ab2e64c56ad4a50b",
|
||||
"internal/http/server_deadline_test.go": "c56460fd8258a7fe8cf25d88fb2030ecfee0e142e0559fad0eb1a05d43d27c0f",
|
||||
"internal/http/dial_deadline_linux_test.go": "0939d05b72a09760d53fcdf249775989e3f89bca824b9961d0b2a657ebfdf41e"
|
||||
}
|
||||
@@ -0,0 +1,78 @@
|
||||
I'll start by reading the review artifacts and the plan documents.
|
||||
|
||||
已完成只读复核。我独立读取了 Go 1.27.1 的 `net/http` 源码、全部 7 个当前实现文件、既有测试与全部 evidence 日志。
|
||||
|
||||
---
|
||||
|
||||
# R8 实现评审 — Opus 5.0（只读，实测复核）
|
||||
|
||||
## 结论：**REQUEST_CHANGES**
|
||||
|
||||
生产代码（`deadlineconn.go` / `listener.go` / `server.go` / `server-main.go`）我**未发现任何行为性缺陷**，与已同意的 v1（SHA256 `7cb6…6366`）逐条吻合。唯一阻塞项在**测试侧引入的依赖，会确定性地打红仓库自身的 CI 质量门**（`make check-gen`），一行 `go.mod` 即可修复，不需要改动任何生产语义。
|
||||
|
||||
> 诚实性声明：① 我**没有执行任何测试、`go vet`、`gofmt`、`go mod tidy` 或哈希计算**（本会话仅 Read/Grep/Glob）；所有 SHA256（plan、production diff、implementation-files.json）均沿用你提供的值，**未独立重算**。② 我逐字节比对了 `review/implementation.patch` 与四个生产文件的当前内容，二者一致。③ 下文所有"已核验"均指**我亲自读过的源码**（含 `/opt/homebrew/Cellar/go/1.27.1/libexec/src/net/http/`）；所有"日志显示"均指**我读日志得出的转述，非我运行**。④ 未使用子代理。
|
||||
|
||||
---
|
||||
|
||||
## 一、对 v1 的实现符合性（逐条，全部达成）
|
||||
|
||||
| v1 条款 | 实现位置 | 结论 |
|
||||
|---|---|---|
|
||||
| mutex 保护的 explicit 时间 + `readDeadlineStrict`（默认 false） | `deadlineconn.go:37-38,43` | ✅ |
|
||||
| `SetReadDeadline`/`SetDeadline` 记录 explicit，保留 zero/abort 与立即转发 | `deadlineconn.go:134,152` + `136-139,149-150` | ✅ |
|
||||
| `SetReadDeadlineStrict(bool)`，同锁、重置 `readSetAt`、带文档 | `deadlineconn.go:79-87` | ✅ |
|
||||
| 锁内复检 abort/inf；保留节流与 250ms 松弛；strict 下取 `min(idle, explicit)`；绝对上限不加松弛 | `deadlineconn.go:64-66,69-76`（`deadline = c.readExplicit`，未 `Add`） | ✅ |
|
||||
| 写侧完全不动 | `deadlineconn.go:89-105,161-168` 与基线一致 | ✅ |
|
||||
| listener 保留具体类型/读写 idle/Unwrap 兼容，返回前开启 strict | `listener.go:73-76` | ✅ |
|
||||
| Init **组合而非丢弃** 调用方 ConnState；先切模式再调用方 hook | `server.go:127,138-151`（`connState(conn,state)` 在 switch 之后） | ✅ |
|
||||
| 一层 `*tls.Conn` 用 `NetConn()` 解包；h2 跳过；其他状态忽略 | `server.go:130-137,139-147` | ✅ |
|
||||
| 不删 ReadTimeout/WriteTimeout、不改 flag；生产超时处加说明注释 | `cmd/server-main.go:904-905`（其余 906-915 未变） | ✅ |
|
||||
|
||||
验证矩阵 1–7 项的**内容**也已全部落地（含 N2 pipelined、N5 真 h2 断言、N6 >30s 下载三项补充）。
|
||||
|
||||
---
|
||||
|
||||
## 二、你点名的 8 项独立核验（结果）
|
||||
|
||||
1. **Go 1.27.1 相位序（实读源码）**：`setState(c.rwc,StateNew,runHooks)` 在 `go c.serve()` **之前**、accept 循环内同步执行（`server.go:3580-3581`、hook 同步调用见 `1881-1883`）；首个 header deadline 在 serve loop 之前（`2038-2040`）；`StateActive` 在 `readRequest` **返回之后**（`2054-2059`）；整请求 deadline **无条件**下发（`1103`，`ReadTimeout<=0` 时下发 zero）；`StateIdle`(`2152`) → idle deadline(`2163-2167`) → `Peek(4)`(`2173`) → 第二个 header deadline(`2177-2181`) **全部落在 strict 窗口内**。结论：v1 的相位切分正确。
|
||||
2. **mutex/atomic 交互**：`readExplicit`/`readDeadlineStrict`/`readSetAt` 三者只在 `mu` 下读写；`abortReads`/`infReads` 原子量在锁内**复检**（`deadlineconn.go:64`），恰好堵住"`Read` 已过外层门 → 并发 `SetReadDeadline(zero)` → 滚动 deadline 覆盖 net/http 背景读的零值"这一 TOCTOU；锁内无阻塞 I/O（`SetReadDeadline` 只是 runtime 定时器调整），因此 accept 循环里的 hook 不会被卡住。未见数据竞争面。
|
||||
3. **>250ms 更新不得续期 header 上限**：`deadlineconn.go:69-76` 每次到期重算都会再次 clamp 回同一个绝对 `readExplicit`，绝不外推。推演 trickle 用例（header 650ms、100ms 一字节）：t=0/300/600ms 三次落入更新分支，每次都 clamp 到 t0+650ms，650ms 必断。**不变量成立**。
|
||||
4. **nonzero/zero/past**：nonzero → `min()`；zero → `infReads` 在外层与锁内双重短路（`58`/`64`），背景读、hijack、h2 全部维持"永不超时"；past → `abortReads` 使 `Read` 直接返回 `context.DeadlineExceeded`（实现 `net.Error.Timeout()`）。strict 下**已过期的未来时间保持过期**（clamp 出一个过去时刻）。
|
||||
5. **H1 body 滚动**：`1103` 在 `2058` 之前就把 socket deadline 改成 `t0+ReadTimeout` 并同步刷新 `readExplicit`，二者之间**不存在任何读**；`StateActive` 关 strict 且清 `readSetAt`，首个 body 读立刻续期 → 长上传不被硬顶。`ReadTimeout<=0` 时退化为 `infReads`，与补丁前一致。
|
||||
6. **buffered/pipelined 与 keep-alive**：`StateActive` 的触发条件 `c.r.remain != initialReadLimitSize()` 在**成功路径上恒成立**——`readRequest` 在 `1067` 调 `setInfiniteReadLimit()`（`remain=maxInt64`），故纯缓冲的流水线第二个请求同样会关 strict（opus-v1 N2 得到证实）。keep-alive 侧 `2163` 会立刻覆盖掉上一请求遗留的 `readExplicit`，中间窗口无读，无陈旧值风险。
|
||||
7. **TLS/h2**：握手读上限 = `min(正 RHT,RT,WT)`（`server.go:969-983,1962-1968`），成功后两侧清零（`1988-1992`）→ 再进入新的 header 上限。协商 h2 时 net/http 走 `setState(...,skipHooks)`（`2002`）且在 `ServeConn` 前把两侧 deadline 清零（`http2.go:100-101`）；h2 自身只上报 Active/Idle（`internal/http2/server.go:568-572,796-800`），我们的 h2 分支跳过它们，即便不跳过也被 `infReads` 短路。h2 的 per-stream `ReadTimeout` 是 `time.AfterFunc` 计时器（`1970-1972` → `onReadTimeout` `1836-1841`，返回包装后的 `os.ErrDeadlineExceeded`，满足 `net.Error`），**未新增任何连接级读超时**。
|
||||
8. **grid/hijack 与默认调用方**：`hijackLocked` 先 `abortPendingRead` 再 `rwc.SetDeadline(zero)`，之后才 `StateHijacked`（`server.go:322-326,336`）；grid 随后 `deadlineconn.Unwrap(conn)` 取回裸 `*net.TCPConn`（`internal/grid/manager.go:193`），strict 根本触达不到 grid。内节点 dialer 走默认 false（`internal/http/dial_linux.go:126-131`），且 `DriveOPTimeout` 在生产仍被注释（`cmd/server-main.go:421-422`），生产内节点连接压根不经过 DeadlineConn。
|
||||
|
||||
---
|
||||
|
||||
## 三、阻塞项（1 项，不涉及生产语义）
|
||||
|
||||
**B1 — 新测试的直接依赖会打红 `make check-gen` / CI `quality` job**
|
||||
- `internal/http/server_deadline_test.go:35` 直接 `import "golang.org/x/net/http2"`，而 `go.mod:297` 为 `golang.org/x/net v0.59.0 // indirect`；全仓库（Grep 确认）**只有这一个文件**直接导入 `golang.org/x/net/*`。
|
||||
- `Makefile:49-58` 的 `check-gen` 会执行 `go mod tidy -compat=1.27`，随后 `git diff --name-only -- … go.mod go.sum` 非空即 `exit 1`；`.github/workflows/go.yml:75-76` 把它作为必跑步骤。`go mod tidy` 会因"主模块的测试直接导入"而把该行提升为直接依赖（去掉 `// indirect`）→ **go.mod 产生 diff → CI 红**。
|
||||
- 注意这**不是构建失败**：`go.sum:765` 已有完整 `h1:` 哈希，所以本地 `go test` 能过（日志也显示过了），git status 里 go.mod/go.sum 也确实未变——问题只在 tidy 门禁。
|
||||
- 两种修法任选其一：(a) 把 `golang.org/x/net v0.59.0` 移入直接 require 块（不改版本、不引新模块，零风险）；(b) 去掉该依赖，用标准库 h2 客户端（`stdhttp.Transport` + `TLSClientConfig.NextProtos=[]string{"h2"}`）——被测服务端本就是 net/http 内置 http2，(b) 反而更贴切。
|
||||
- 声明：我**无法执行** `go mod tidy` 验证，此结论由 `go.mod:297` + `Makefile:49-58` + `go.yml:75-76` + Grep 结果推得。
|
||||
|
||||
---
|
||||
|
||||
## 四、非阻塞项
|
||||
|
||||
- **N1（最值得补的测试缺口）** `deadlineconn_strict_test.go:40-100` 中每次 `Read` 之前都有 `SetReadDeadline*`，而它们会把 `readSetAt` 清零（`deadlineconn.go:86,151`），因此**只覆盖了"第一次更新"的 clamp，从未覆盖"跨 250ms 的第二/第三次更新仍 clamp"**——而这正是本缺陷的核心不变量。目前它只由真实 socket 的 trickle 用例（`server_deadline_test.go:145-151`）间接覆盖。建议加一个确定性用例：strict + 设上限 → `Read` → `sleep(300ms)` → `Read` → 断言 `raw.read` 仍等于上限。
|
||||
- **N2** `TestConcurrentStrictReadDeadline`（`:140-162`）无断言，且 setter 每轮立刻把 deadline 归零，**从未让 `Read` 与"正在生效的 clamp"并发**；作为 race 探针可以，但别把它当作语义回归。
|
||||
- **N3（flake 风险）** `TestServerHTTP2Deadlines`（`:376-478`）里 `IdleTimeout=400ms` 会被 net/http 映射为 h2 连接级 idle（`net/http/http2.go:57-61`）；两次初始 GET 之后、以及 `<-done` 到最后一次 GET 之间，连接处于 idle，若 runner 抖动 >400ms，连接会被 h2 自身关闭 → `connections.Load()==1` 假失败。同理 `TestServerContinuousUpload` 每块只有 550ms 余量。建议 h2 fixture 单独用更大的 idle。（fixture 清写定时器的修法本身是对的：`onWriteTimeout` 产生的是 `StreamError/INTERNAL_ERROR`（`internal/http2/server.go:1846-1852`），不满足 `net.Error`，正是初版失败的原因。）
|
||||
- **N4** `listener.go:73` 的局部 `conn` 遮蔽了具名返回值 `conn net.Conn`（且 `err` 也不再使用）；合法但可读性差，`dc := …` 更干净。
|
||||
- **N5（微小开销）** clamp 命中时 `deadlineconn.go:74` 会把 socket 已持有的同一时刻再写一遍；加上每次相位切换清 `readSetAt`（`server.go:141,145`），每个 H1 请求多出约 2 次 deadline 重算。相对 net/http 自身每请求 3 次 `SetReadDeadline` 可忽略，无需改。
|
||||
- **N6** `server.go:131` 每次状态转换都调 `tlsConn.ConnectionState()`（持 `handshakeMutex`）。正确且安全（StateNew 发生在 `go c.serve()` 之前），但如 opus-v1 N3 所述该分支严格冗余（`http2.go:100-101` 已清零，`infReads` 必然短路）。保留可作纵深防御，建议注释点明"仅为防御，非必要条件"。
|
||||
- **N7（发布说明，已在 consensus N7 登记）** 两处用户可见收紧：TLS 握手读被 `min(RHT,RT,WT)`（生产 30s）硬顶；请求头被 ReadHeaderTimeout 硬顶（即便字节持续到达）。
|
||||
- **N8（已知边界，非本轮）** 生产 `NextProtos{"http/1.1","h2"}`（`cmd/utils.go:970`）下，只宣告 h2 的客户端仍受 h2 **绝对** per-stream `ReadTimeout`（30s）约束；plan 第 24 行/consensus N3 已声明不修，交付说明请重述。
|
||||
- **N9（证据对齐，已更正）** 我核对了日志里的 `t.Logf` 归属行（注意 `runContinuousUpload/Download` 调了 `t.Helper()`，报的是**调用点**）：`implementation-focused.log:85` 的 `:259`、`default-30s-transfers.log:24` 的 `:274` 与当前文件 **完全一致**；下载区整体偏移 +36/+37 行，与"仅 H2 fixture 被重写（当前 `:376-478`，初版失败点 `:435` → 现 `:463`）+ 长用例补了 `t.Parallel()`"完全吻合。**结论：现存日志并非整体过期，H2 用例之前的部分与当前源码同版；H2 用例及其后的部分尚无对应通过记录**——与你说的"最终 race 复跑进行中"一致，我不将其计为已通过。
|
||||
- **N10** `evidence/vet.log` 为 **0 字节**。无输出多半就是干净，但空文件不自证；建议在其中记录命令行与退出码。
|
||||
|
||||
---
|
||||
|
||||
## 五、未由我执行的部分（请勿当作我背书的通过）
|
||||
|
||||
`implementation-focused.log` / `darwin-race.log` / `linux-race.log` / `grid.log`（仅 `TestDisconnect`+`TestSingleRoundtrip`）/ `default-30s-transfers.log`（33.03s 明文与 TLS 上传/下载）/ `vet.log` 均为**我阅读的记录**，非我运行。Linux 的 `dial_deadline_linux_test.go`（`//go:build linux`）在本机 Darwin 上不会运行，我只静态核验了它确实断言了默认滚动语义的三种情形（50ms 显式被续期、zero 禁用、1min 显式仍被 idle 截断）。最终全量 race 复跑结果待你回填。
|
||||
|
||||
修掉 B1（或明确判定 tidy 门禁不适用）后，我这边即可转 APPROVE。
|
||||
@@ -0,0 +1,5 @@
|
||||
Read-only implementation review of SILO R8 on baseline 9ebe81c1b3611f9cc73e676b5b741c2be62c467a. You previously agreed plan v1 SHA256 7cb609e37e3199ecd93c992f08d123968ba8082682a108f20490e0644d735366; this is a fresh actual Opus 5.0 review. Read docs/investigations/r8/plan-v1.md, consensus.md, review/opus-v1.md, review/implementation.patch (production diff SHA256 88716d9af433b55527308e09949b0adaab6980aca3bf1ad38b677e9c1c0ce083), and every current file in review/implementation-files.json. These are actual edits, including new untracked tests not represented in git diff. Do not edit or try to write a plan file; deliver findings in the response. Allowed tools are Read/Grep/Glob only.
|
||||
|
||||
Independent checks: exact phase ordering in current Go1.27.1 net/http, mutex/atomic interaction, no header cap renewal across >250ms updates, nonzero/zero/past handling, H1 body rolling, buffered/pipelined and keep-alive transitions, TLS/h2 and grid/hijack default-caller compatibility. Detect concrete bugs and weak tests, avoid hypothetical unrelated expansions. Also see evidence/implementation-focused.log, evidence/default-30s-transfers.log (>33s plaintext and TLS H1 uploads/downloads passed), evidence/grid.log, and evidence/linux-race.log. The first Darwin race run failed only because the H2 fixture's equal read/write timers raced to supply different timeout error types (not a data race); final fixture clears the per-stream write timer to isolate native ReadTimeout and checks healthy multiplexed requests and connection reuse. Final full race reruns are in progress; do not treat pending checks as passed.
|
||||
|
||||
Give Chinese verdict APPROVE or REQUEST_CHANGES, concrete file/line findings separated into blocking and nonblocking, and explicit implementation conformance to accepted v1. Clearly distinguish independently inspected source from tests you did not personally execute. If no blockers, say so.
|
||||
@@ -0,0 +1,22 @@
|
||||
{
|
||||
"requested_model": "claude-opus-5",
|
||||
"effort": "max",
|
||||
"cli_version": "2.1.270",
|
||||
"baseline_sha": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a",
|
||||
"plan_sha256": "7cb609e37e3199ecd93c992f08d123968ba8082682a108f20490e0644d735366",
|
||||
"production_diff_sha256": "88716d9af433b55527308e09949b0adaab6980aca3bf1ad38b677e9c1c0ce083",
|
||||
"status": "completed",
|
||||
"created_at_utc": "2026-09-15T15:54:25.631091+00:00",
|
||||
"actual_assistant_models": [
|
||||
"claude-opus-5"
|
||||
],
|
||||
"result_subtype": "success",
|
||||
"is_error": false,
|
||||
"session_id": "541429b8-dc15-4c36-8bf9-f6117e086ae3",
|
||||
"duration_ms": 382141,
|
||||
"raw_path": "/Users/vonng/tmp/silo-r8-01a0a5b9/opus-implementation.jsonl",
|
||||
"raw_sha256": "d5ec088b66ceed9024afc74fc5df40340dc6d208e73d99a8638beb1fde906952",
|
||||
"review_sha256": "e5861d0da930fe590606722563109d2b87b03b1497abc4a758b7846f446e1eba",
|
||||
"completed_at_utc": "2026-09-15T16:01:49.467166+00:00",
|
||||
"scope_note": "Covers v1 connection implementation only; newly discovered config binding requires v2 agreement."
|
||||
}
|
||||
@@ -0,0 +1,137 @@
|
||||
diff --git a/cmd/server-main.go b/cmd/server-main.go
|
||||
index 48ed0f87d..c8a3a19ca 100644
|
||||
--- a/cmd/server-main.go
|
||||
+++ b/cmd/server-main.go
|
||||
@@ -901,6 +901,8 @@ func serverMain(ctx *cli.Context) {
|
||||
close(globalGridStart)
|
||||
close(globalLockGridStart)
|
||||
|
||||
+ // The HTTP/1 listener preserves absolute header deadlines and renews the
|
||||
+ // body read/write idle limits, so transfers may outlast IdleTimeout.
|
||||
httpServer := xhttp.NewServer(getServerListenAddrs()).
|
||||
UseHandler(setCriticalErrorHandler(corsHandler(handler))).
|
||||
UseTLSConfig(newTLSConfig(getCert)).
|
||||
diff --git a/internal/deadlineconn/deadlineconn.go b/internal/deadlineconn/deadlineconn.go
|
||||
index 95bb43eff..5fa5a1403 100644
|
||||
--- a/internal/deadlineconn/deadlineconn.go
|
||||
+++ b/internal/deadlineconn/deadlineconn.go
|
||||
@@ -34,6 +34,8 @@ type DeadlineConn struct {
|
||||
net.Conn
|
||||
readDeadline time.Duration // sets the read deadline on a connection.
|
||||
readSetAt time.Time
|
||||
+ readExplicit time.Time // last deadline requested by the caller.
|
||||
+ readDeadlineStrict bool // idle renewal must not extend readExplicit.
|
||||
writeDeadline time.Duration // sets the write deadline on a connection.
|
||||
writeSetAt time.Time
|
||||
abortReads, abortWrites atomic.Bool // A deadline was set to indicate caller wanted the conn to time out.
|
||||
@@ -59,17 +61,31 @@ func (c *DeadlineConn) setReadDeadline() {
|
||||
|
||||
c.mu.Lock()
|
||||
defer c.mu.Unlock()
|
||||
- if c.abortReads.Load() {
|
||||
+ if c.abortReads.Load() || c.infReads.Load() {
|
||||
return
|
||||
}
|
||||
|
||||
now := time.Now()
|
||||
if now.Sub(c.readSetAt) > updateInterval {
|
||||
- c.Conn.SetReadDeadline(now.Add(c.readDeadline + updateInterval))
|
||||
+ deadline := now.Add(c.readDeadline + updateInterval)
|
||||
+ if c.readDeadlineStrict && !c.readExplicit.IsZero() && c.readExplicit.Before(deadline) {
|
||||
+ deadline = c.readExplicit
|
||||
+ }
|
||||
+ c.Conn.SetReadDeadline(deadline)
|
||||
c.readSetAt = now
|
||||
}
|
||||
}
|
||||
|
||||
+// SetReadDeadlineStrict controls whether idle renewal may extend a deadline set
|
||||
+// by SetReadDeadline or SetDeadline. The default is false. Explicit zero and
|
||||
+// past deadlines retain their disable/cancel semantics in either mode.
|
||||
+func (c *DeadlineConn) SetReadDeadlineStrict(strict bool) {
|
||||
+ c.mu.Lock()
|
||||
+ defer c.mu.Unlock()
|
||||
+ c.readDeadlineStrict = strict
|
||||
+ c.readSetAt = time.Time{}
|
||||
+}
|
||||
+
|
||||
func (c *DeadlineConn) setWriteDeadline() {
|
||||
// Do not set a Write deadline, if upstream wants to cancel all reads.
|
||||
if c.writeDeadline <= 0 || c.abortWrites.Load() || c.infWrites.Load() {
|
||||
@@ -115,6 +131,7 @@ func (c *DeadlineConn) SetDeadline(t time.Time) error {
|
||||
defer c.mu.Unlock()
|
||||
|
||||
c.readSetAt = time.Time{}
|
||||
+ c.readExplicit = t
|
||||
c.writeSetAt = time.Time{}
|
||||
c.abortReads.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
c.abortWrites.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
@@ -132,6 +149,7 @@ func (c *DeadlineConn) SetReadDeadline(t time.Time) error {
|
||||
c.abortReads.Store(!t.IsZero() && time.Until(t) < 0)
|
||||
c.infReads.Store(t.IsZero())
|
||||
c.readSetAt = time.Time{}
|
||||
+ c.readExplicit = t
|
||||
return c.Conn.SetReadDeadline(t)
|
||||
}
|
||||
|
||||
diff --git a/internal/http/listener.go b/internal/http/listener.go
|
||||
index bc6de3af9..14d34f6ea 100644
|
||||
--- a/internal/http/listener.go
|
||||
+++ b/internal/http/listener.go
|
||||
@@ -70,7 +70,10 @@ func (listener *httpListener) Accept() (conn net.Conn, err error) {
|
||||
if result.err != nil {
|
||||
return nil, result.err
|
||||
}
|
||||
- return deadlineconn.New(result.conn).WithReadDeadline(listener.opts.IdleTimeout).WithWriteDeadline(listener.opts.IdleTimeout), result.err
|
||||
+ conn := deadlineconn.New(result.conn).WithReadDeadline(listener.opts.IdleTimeout).WithWriteDeadline(listener.opts.IdleTimeout)
|
||||
+ // Server.Init switches to rolling reads only after HTTP/1 headers are read.
|
||||
+ conn.SetReadDeadlineStrict(true)
|
||||
+ return conn, nil
|
||||
case <-listener.ctxDoneCh:
|
||||
}
|
||||
return nil, syscall.EINVAL
|
||||
diff --git a/internal/http/server.go b/internal/http/server.go
|
||||
index 2934fda6c..d9c19a33f 100644
|
||||
--- a/internal/http/server.go
|
||||
+++ b/internal/http/server.go
|
||||
@@ -29,6 +29,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/dustin/go-humanize"
|
||||
+ "github.com/minio/minio/internal/deadlineconn"
|
||||
)
|
||||
|
||||
var (
|
||||
@@ -123,6 +124,32 @@ func (srv *Server) Init(listenCtx context.Context, listenErrCallback func(listen
|
||||
srv.listener = listener
|
||||
srv.listenerMutex.Unlock()
|
||||
|
||||
+ connState := srv.ConnState
|
||||
+ srv.ConnState = func(conn net.Conn, state http.ConnState) {
|
||||
+ raw := conn
|
||||
+ if tlsConn, ok := raw.(*tls.Conn); ok {
|
||||
+ if tlsConn.ConnectionState().NegotiatedProtocol == "h2" {
|
||||
+ // HTTP/2 owns its stream deadlines; do not change the connection.
|
||||
+ raw = nil
|
||||
+ } else {
|
||||
+ raw = tlsConn.NetConn()
|
||||
+ }
|
||||
+ }
|
||||
+ if dc, ok := raw.(*deadlineconn.DeadlineConn); ok {
|
||||
+ switch state {
|
||||
+ case http.StateNew, http.StateIdle:
|
||||
+ dc.SetReadDeadlineStrict(true)
|
||||
+ case http.StateActive:
|
||||
+ // net/http has finished reading the headers, including buffered
|
||||
+ // requests. Keep ReadTimeout as a rolling idle limit for uploads.
|
||||
+ dc.SetReadDeadlineStrict(false)
|
||||
+ }
|
||||
+ }
|
||||
+ if connState != nil {
|
||||
+ connState(conn, state)
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
var l net.Listener = listener
|
||||
if tlsConfig != nil {
|
||||
l = tls.NewListener(listener, tlsConfig)
|
||||
@@ -0,0 +1,70 @@
|
||||
I'll start by reading the required materials to ground my review.
|
||||
|
||||
Now let me verify the actual Go 1.27.1 `net/http` behavior rather than trusting the plan's summary.
|
||||
|
||||
Verification complete. Writing my review record to the plan file.
|
||||
|
||||
Write 工具在本会话被禁用（确为只读评审），故评审记录直接以回复形式交付。
|
||||
|
||||
---
|
||||
|
||||
# R8 独立评审 — Opus 5.0 reviewer
|
||||
|
||||
**结论：CONSENSUS —— 同意 `docs/investigations/r8/plan-v1.md`（SHA256 `7cb609e37e3199ecd93c992f08d123968ba8082682a108f20490e0644d735366`），无遗留阻塞性分歧。** 共 9 条非阻塞意见。
|
||||
|
||||
> 诚实性声明（先行）：① SHA256 采用你给出的值——本会话只读工具集不含哈希命令，我**未独立重算**；② `evidence/baseline.log` 对应的临时测试文件已不在 worktree（`git status` 仅 `docs/investigations/r8/` 未跟踪），我**未重跑**该日志，只是从 Go 源码独立推导出的机制与该日志完全吻合；③ 未使用子代理、未模拟工具、未声称运行过任何测试。
|
||||
|
||||
## 一、逐条核验（Go 1.27.1 源码，`/opt/homebrew/Cellar/go/1.27.1/libexec/src/net/http/`）
|
||||
|
||||
| 核验项 | 事实 | 位置 |
|
||||
|---|---|---|
|
||||
| ConnState 相位序 | `setState(c.rwc, StateNew, runHooks)` 在 `go c.serve()` **之前**，hook 同步执行、传入 `c.rwc` | `server.go:3580`, `1865-1884` |
|
||||
| 首个 header deadline | 在 serve loop **之前**设置，此时 state 仍为 StateNew | `server.go:2038-2040` |
|
||||
| StateActive 时机 | `readRequest` **返回之后**才触发；成功路径必然触发（`setInfiniteReadLimit()` 把 `remain` 置 `maxInt64`） | `2054-2059`, `1067` |
|
||||
| whole-request deadline | Go 1.27 **无条件** `SetReadDeadline(t0+ReadTimeout)`（旧版的 `!hdrDeadline.Equal(...)` 条件已删） | `1103`, `1038-1041` |
|
||||
| TLS 握手 | 读写 deadline = `now + min(正RHT,RT,WT)` = SILO 30s；成功后两侧清零 | `1962-1968`, `964-984`, `1988-1992` |
|
||||
| zero / background EOF | 有 body → EOF 回调启动 background read；无 body → 立即启动，且 `SetReadDeadline(zero)` | `2123-2127`, `731-743` |
|
||||
| abort / finishRequest | `abortPendingRead` 设 `aLongTimeAgo` → 等待 → 清零；由 `finishRequest` 调用 | `785-797`, `1694-1707` |
|
||||
| keep-alive 第二个 header | StateIdle hook → `SetReadDeadline(now+idle)` → `Peek(4)` → `SetReadDeadline(now+RHT)`，**全部落在 strict 窗口内** | `2152`, `2163-2181` |
|
||||
| Hijack（grid/ws） | `abortPendingRead` + `rwc.SetDeadline(zero)` + StateHijacked | `318-337` |
|
||||
| 写侧 | 每请求由 `readRequest` 的 defer 重新武装，请求结束清零 | `1042-1046`, `2145` |
|
||||
| H2 是否启用 | SILO 设了 `TLSConfig` 且 `NextProtos` 含 `"h2"` → `shouldConfigureHTTP2ForServe()` 为真 → `s.h2` 配置，走内置 http2 | `3469-3489`, `http2.go:82` |
|
||||
| H2 与 hook | h2 **会**调用用户 hook（带 nil 保护）；进入 ServeConn 前把两侧 deadline 清零；除 per-stream 定时器外无任何 conn 级 `SetReadDeadline` | `http2.go:100-101,188-198`; `internal/http2/server.go:571-572, 799-800, 1567, 2100, 1970-1972` |
|
||||
|
||||
SILO 侧：`cmd/server-main.go:907-913`（RT=WT=Idle=30s，RHT=30s）、`internal/http/listener.go:73`、`dial_linux.go:126-132`、`grid/manager.go:193`、`cmd/utils.go:970`。全仓库 `*.go` **无任何 `ConnState` 使用**；现有 `internal/deadlineconn`、`internal/http` 测试均不设显式 deadline，故不受 strict 影响。
|
||||
|
||||
## 二、相位推导：option 4 为何正确
|
||||
|
||||
- **strict ON 覆盖**：accept/StateNew → TLS 握手读 → 首个 header（`2038`）；以及 `2152` StateIdle 到下一轮 `2058` StateActive 之间的 idle 等待（`2163`）**与第二个 header**（`2177`）。
|
||||
- **strict OFF 覆盖**：`2058` 之后到 `2152` 之前，即 body 读 + handler 全程 → 保留 rolling，长上传不被硬顶。
|
||||
- **`1103` 与 `2058` 之间**虽仍 strict 且 explicit 已变为 `t0+ReadTimeout`，但该区间**不存在任何读**（只有 `2106` 的 header 检查与 `2118-2127` 的登记）——无副作用。
|
||||
- **zero 优先于 strict**（`infReads` 提前 return）→ background read、hijack、h2 全部保持原语义；长 handler（如 `mc admin trace` 这类流式 GET）不会被误杀。
|
||||
- 节流与 `readSetAt` 重置的组合可保证：strict 上限一旦写入 socket 就不会被后续 `Read` 重新拉长；模式切换重置 `readSetAt` 使下一次读立即按新模式重算。
|
||||
|
||||
**对 baseline 的验证**：strict 下 header 上限 = `min(now+2s+250ms, now+100ms)` = `now+100ms` → 400ms 完成的 header 必被拒，与标准 `net/http` 一致。
|
||||
|
||||
## 三、非阻塞意见（N1–N9）
|
||||
|
||||
1. **N1 措辞订正**：计划 20 行说 header deadline 在 `readRequest` 内设；实际在 `2038`/`2177`。且 Go 1.27 的 `1103` 是**无条件**的——这反而让 option 1 的否决理由更硬：RT=30s 会硬顶「header+body」整请求。
|
||||
2. **N2 不变量要写死**：StateActive 的触发依据应记为 `1067` 的 `setInfiniteReadLimit()`，而非「读到字节」。pipelined 请求即使 header 全来自 `bufio` 缓冲、零 socket 读也必然触发。建议补一个 pipelined 用例。
|
||||
3. **N3 可简化**：h2 特判可省——`http2.go:100-101` 已清零两侧 deadline，strict 分支恒被 `infReads` 短路。保留亦正确，只多一次 `ConnectionState()` 加锁；若保留，注意 StateNew 时握手尚未发生，不能依赖其返回值。
|
||||
4. **N4 可简化**：`Accept` 里开 strict 是冗余的（`3580` 严格 happens-before 任何读）。保留可作纵深防御，但会让「未装 hook 的 httpListener 使用者」隐式获得 strict body 语义（今天不存在，`httpListener` 未导出、仅 `Server.Init` 构造）。两者皆可，请写明这层耦合。
|
||||
5. **N5 测试有效性（最重要）**：`cmd/utils.go:970` 顺序为 `{"http/1.1","h2"}`，Go 的 ALPN 以**服务端顺序**优先 → 普通 Go HTTP/2 客户端会落回 http/1.1。H2 smoke 必须用只声明 `NextProtos: []string{"h2"}` 的 `tls.Client`，并断言 `NegotiatedProtocol == "h2"` 与 `resp.Proto == "HTTP/2.0"`，否则是假阳性。
|
||||
6. **N6 回归护栏**：写侧 rolling 是大对象 GET 的命门（`1042-1046` 每请求 `now+30s`，全靠每 250ms 续期）。计划正确地不动写侧；建议在 `setWriteDeadline` 旁注明「不可对称 strict」，并在矩阵补一个 >30s 的持续**下载**用例。
|
||||
7. **N7 发布说明**：两处用户可见收紧——TLS 握手读侧被 `min(RHT,RT,WT)=30s` 硬顶；请求头被 `ReadHeaderTimeout` 硬顶（即便字节持续到达）。这正是 stock `net/http` 语义，但极慢链路客户端可能受影响，需文档化。
|
||||
8. **N8 影响面比计划更小**：`cmd/server-main.go:421-422` 的 `DriveOPTimeout` 仍被 FIXME 注释掉 → `dial_linux.go:126` 分支不成立，**生产内节点连接根本不经过 DeadlineConn**。计划结论正确，建议据实补记。
|
||||
9. **N9 已知项**：握手写侧仍会被续期，慢读 ServerHello 的客户端仍能拉长写侧占用。既有缺陷、本轮不修，建议显式登记而非静默。
|
||||
|
||||
## 四、替代方案对比（是否存在更简单的正确解：无）
|
||||
|
||||
- **全局 clamp**：`1103` 无条件设 `wholeReqDeadline`，RT=30s 直接破功大对象上传。❌
|
||||
- **去 ReadTimeout + 把 zero 解释成 rolling**：与 `731-743`（background read）、`326`（hijack）冲突，两处都用 zero 表达「永不超时」。❌
|
||||
- **body/ResponseController wrapper**：引入 drain/EOF/chunked 记账 + h2 专门处理，范围远超本缺陷。❌
|
||||
- **由 hook 自行推算 cap（`now+RHT` 等）**：等于在 SILO 侧复刻 stdlib 的 deadline 算术，会随 Go 版本漂移（1.27 相对旧版已改两处）。计划「记住 stdlib 实际要求的值」更稳健。❌
|
||||
- **按「显式值是否短于 idle 窗口」自动判定**：生产默认 RHT = RT = Idle = 30s 三者相等，无法区分相位，仅在 baseline 的 100ms/2s 配置下看似有效，**不是通用正确解**。❌
|
||||
|
||||
所需信息（deadline 由哪一层请求 + 当前 HTTP 相位）只在 `DeadlineConn` API 边界与 `ConnState` 两处可得，**option 4 是最小正确形状**。
|
||||
|
||||
## 五、后续（实现仍需实际执行验证，不得以推断代替）
|
||||
|
||||
同意计划的 7 项矩阵，并请纳入 N2（pipelined）、N5（真·h2 断言）、N6（>30s 持续下载）三项补充；`-race` 下必须覆盖 `Read` / `SetReadDeadline` / `SetReadDeadlineStrict` 三者并发。Linux 部分若只有编译而无运行时验证，请分别如实报告。
|
||||
@@ -0,0 +1,24 @@
|
||||
{
|
||||
"baseline_sha": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a",
|
||||
"plan": "plan-v1.md",
|
||||
"plan_sha256": "7cb609e37e3199ecd93c992f08d123968ba8082682a108f20490e0644d735366",
|
||||
"requested_model": "claude-opus-5",
|
||||
"effort": "max",
|
||||
"cli_version": "2.1.270",
|
||||
"status": "completed",
|
||||
"created_at_utc": "2026-09-15T15:44:56.500168+00:00",
|
||||
"actual_assistant_models": [
|
||||
"claude-opus-5"
|
||||
],
|
||||
"result_subtype": "success",
|
||||
"is_error": false,
|
||||
"session_id": "7e51fd30-3842-47e9-9d19-47604d87b45b",
|
||||
"duration_ms": 292649,
|
||||
"raw_path": "/Users/vonng/tmp/silo-r8-01a0a5b9/opus-v1.jsonl",
|
||||
"stderr_path": "/Users/vonng/tmp/silo-r8-01a0a5b9/opus-v1.stderr.log",
|
||||
"raw_sha256": "7922230a171b041a3b736fa9b162282ce1d38614b57d0cf0c42f2f5eade566b7",
|
||||
"review_sha256": "a6e7c616fe3a158f73e90af7414569f699eedc93e259289dcd0b5c94d75f2dbf",
|
||||
"completed_at_utc": "2026-09-15T15:50:14.799630+00:00",
|
||||
"verdict": "CONSENSUS",
|
||||
"blocking_disagreements": 0
|
||||
}
|
||||
@@ -0,0 +1,70 @@
|
||||
I'll begin by reading the plan and its supporting evidence directly — as an independent reviewer I won't delegate verification.
|
||||
|
||||
Now the actual production code — the core of what I must verify independently.
|
||||
|
||||
Now the Go 1.27.1 source, to verify zero/negative semantics of `ReadHeaderTimeout` myself.
|
||||
|
||||
已完成独立只读复核。我亲自读取了 Go 1.27.1 `net/http` 源码、`minio/cli@v1.24.2` 源码、全部相关生产文件与新测试、以及 r8 目录下的计划/共识/证据文档。
|
||||
|
||||
---
|
||||
|
||||
# R8 完整方案 v2 评审 — Opus 5.0（独立、只读）
|
||||
|
||||
## 结论：**CONSENSUS**
|
||||
|
||||
我对 **`docs/investigations/r8/plan-v2.md`（v2，SHA256 `426127ed9fb08aeddf8259ebdc4b1c24ebec8cda751a970ed99338a44b065f4c`，基线 `9ebe81c1b3611f9cc73e676b5b741c2be62c467a`）的完整范围**（v1 连接层设计 + 新增配置绑定）表示同意，**零条阻塞性分歧**。
|
||||
|
||||
**据此授权在该方案范围内实施所提议的配置绑定**：在 `cmd/common-main.go` 的 `buildServerCtxt` 中于 `IdleTimeout` 旁新增 `ctxt.ReadHeaderTimeout = ctx.Duration("read-header-timeout")`。实现与测试验收仍是独立环节，不在本次同意范围内；合并、发布、部署同样不在内。
|
||||
|
||||
**B1 判定：已解决。** 全仓库 `.go` 文件 Grep `golang.org/x/net` **零命中**；`internal/http/server_deadline_test.go:20-35` 只引入标准库与 `internal/deadlineconn`。h2 改由标准库 `stdhttp.Protocols` + `SetHTTP2(true)`（`:409-411`）强制，断言 `NegotiatedProtocol=="h2" && Proto=="HTTP/2.0"`（`:420`）与 `ProtoMajor!=2` 保护（`:383-384`）均保留——即使协议回落也会响亮失败而非静默降级。由于主模块无任何包（含测试）直接导入 `golang.org/x/net/*`，`go mod tidy` 没有理由把 `go.mod:297` 的 `// indirect` 提升为直接依赖，`Makefile:49-58` 的 `check-gen` 门禁不会被打红，也不会因此丢失该 require（它仍被其他模块间接需要）。N1 亦已落实：`deadlineconn_strict_test.go:165-178` 跨 3 个真实 300ms 周期、全程不重设上限地断言 clamp 不外推。N3 仅对 h2 夹具放宽 keep-alive idle（`server_deadline_test.go:406` `srv.IdleTimeout = 3 * time.Second`），未触及其他用例。
|
||||
|
||||
> **诚实性声明**：① 本会话仅有 Read/Grep/Glob，**无 shell**。因此我**没有重算 v2 的 SHA256**，也没有运行任何 `go test`/`go vet`/`go mod tidy`/`git`；我核验的是该路径下实际文件的**内容**（116 行，与下文逐条引用一致）。② 所有"日志显示"均为我**阅读记录**的转述，非我运行。③ 未使用子代理。④ 未验证当前 HEAD 是否等于所述基线。
|
||||
|
||||
---
|
||||
|
||||
## 一、最小充分性：我独立确认的闭链（C1–C8）
|
||||
|
||||
| # | 结论 | 我核验的依据 |
|
||||
|---|---|---|
|
||||
| C1 | 一行赋值**充分** | 全库仅 `cmd/server-main.go:912` 读 `globalServerCtxt.ReadHeaderTimeout`；唯一 HTTP server 构造点也仅 `server-main.go:906`（`xhttp.NewServer` 全库两处命中，另一处在 patch 文档里） |
|
||||
| C2 | 链路闭合 | `buildServerCtxt`（`common-main.go:370`）是 serverCtxt 唯一填充函数；`serverMain` 在 `server-main.go:799` 用它填 `globalServerCtxt` |
|
||||
| C3 | **默认行为零变化** | `DefaultIdleTimeout == DefaultReadHeaderTimeout == 30s`（`internal/http/server.go:44-48`）→ `readHeaderTimeout()`（Go `server.go:3752-3757`）绑定前后都返回 30s；`tlsHandshakeTimeout()`（`:969-983`）也都是 30s。plan 第 17 行"两值相等时默认值恰好看起来正常"我独立证实 |
|
||||
| C4 | YAML 不可能覆盖 | `ServerConfigCommon`/`Opts`（`internal/config/server.go:20-45`）**根本没有超时字段**；`configCommonToSrvCtx`（`server-main.go:274-302`）只处理 RootUser/Pwd/Addr/ConsoleAddr/CertsDir/FTP/SFTP。即便 merge 发生在赋值之后（`common-main.go:450-454`）也无字段可覆盖 |
|
||||
| C5 | 优先级/错误面**完全不变** | `minio/cli@v1.24.2/flag.go:569-594`：env 作为 flag 默认值、命令行再覆盖 → flag > env > 静态默认；"存在但为空"的 env 在 `time.ParseDuration("")` 处报错，**发生在 flag 解析阶段，与本绑定无关、绑定前已如此**（`config-baseline-env-fixture-failure.log` 正是该现象）。新行不引入任何新错误路径 |
|
||||
| C6 | h2 不受影响 | `net/http/http2.go:199-211` 只把 `ReadTimeout`/`WriteTimeout`/`IdleTimeout` 交给 h2，`ReadHeaderTimeout` **完全不进入** h2 配置。plan 第 34/71 行正确 |
|
||||
| C7 | 共享调用方不受影响 | `deadlineconn.New` 仅 `listener.go:73` 与 `dial_linux.go:130`（后者不开 strict）；`SetReadDeadlineStrict` 生产调用仅 `listener.go:75`、`server.go:141/145` |
|
||||
| C8 | v1 在 v2 中**逐字保留**，实现与之一致 | plan-v1 `13-24/26-31/35-55/57-66/70-76/78-80` ↔ plan-v2 `23-34/36-41/52-72/74-83/94-100/102-104`；当前生产文件 `deadlineconn.go:37-38,56-87,129-154`、`listener.go:73-76`、`server.go:127-151`、`server-main.go:904-915` 与之吻合。v2 没有夹带修改已同意的 v1 语义 |
|
||||
|
||||
关于**证据可信度**：我不采信"已执行"的口头声明，但 `config_baseline_test.go.txt:26,36` 的设计本身具备自证力——同一夹具用 `MINIO_IDLE_TIMEOUT=2s` 并断言 `IdleTimeout==2s` 在全部 7 个子用例成立，这是一个**对照组**，排除了"夹具坏了"的解释；而 `ReadHeaderTimeout` 恒为 0。再叠加我上面对 `flag.go:569-594` 的独立源码核验，`config-baseline.log` 的每一行（30s/100ms/170ms/80ms/-1s）都可从源码推导出来。
|
||||
|
||||
**阻塞性发现：0 条。**
|
||||
|
||||
---
|
||||
|
||||
## 二、非阻塞发现（N1–N9）
|
||||
|
||||
1. **N1（新耦合，建议写入交付说明）** 绑定后 RHT 首次参与 TLS 握手窗口：`tlsHandshakeTimeout()`（Go `server.go:964-983`）取 RHT/RT/WT 中**正值的最小者**，并在 `:1962-1968` 同时下发读写 deadline。绑定前 RHT 恒为 0 → 窗口 = idle；绑定后 `--read-header-timeout=1s` 会把 TLS 握手（含 h2 的握手阶段）一起压到 1s。默认下无变化（30s）。
|
||||
|
||||
2. **N2（唯一"变松"的组合，最值得单列）** 相对**当前已实现的 v1**：用户只设 `--idle-timeout=2s`、不设 RHT 时，请求头上限从 2s 变为默认 30s。相对**真实基线 v0**（滚动续期、滴入可无限延长）仍是收紧。这是两个独立旋钮的正确语义，但它是全部组合中唯一"看起来放松"的一种，交付说明应明确点名，避免被误读为回归。
|
||||
|
||||
3. **N3（负值语义在明文/TLS 下不对称）** `--read-header-timeout=-1s` → `readHeaderTimeout()` 返回 -1s（`:3752-3757`），首请求处 `server.go:2038` 的 `d>0` 不成立 → **不下发任何 deadline**：明文连接因 `readExplicit` 为零值而在 `deadlineconn.go:71` 短路 clamp，回落到滚动 idle；TLS 连接则已在 `:1990-1991` 被清零 → `infReads=true` → 首个请求头**真正无上限**。keep-alive 第二个请求处 `:2179-2180` 显式下发零值，两者统一为无上限。这是 Go 文档化的 "negative = no timeout"（`:3072-3078`）且需用户显式选择，不要求改设计；建议测试断言并在说明中写明这一差异。
|
||||
|
||||
4. **N4（正面变化，但仍是可见变化）** `--idle-timeout=0`/负值时：绑定前 RT≤0 且 RHT=0 → `readHeaderTimeout()` 返回 0 → **完全没有请求头上限**，`tlsHandshakeTimeout()` 也为 0；绑定后默认 30s 生效。这是修复带来的安全性改善，建议同样纳入说明并补一条用例。
|
||||
|
||||
5. **N5（验证矩阵缺口）** v2 矩阵未提及 `buildscripts/test-timeout.sh`（`Makefile:177-179`），而它是仓库现存唯一端到端超时测试且直接使用 `--read-header-timeout 5s --idle-timeout 5s`。我推演结论不变：两值相等 → 绑定前后 `readHeaderTimeout()` 都是 5s，三个用例（20s 慢头 / 40s 慢 body / 1s+1s 正常）判定与 `:69` 的 `<= 11s` 时限均满足。建议验收时实跑并记录。顺带值得写进报告：该脚本用的是"一次长睡眠"而非"持续滴入"（`:44-61`），这正是 R8 缺陷长期未被它捕获的原因。
|
||||
|
||||
6. **N6（强烈建议）** 把配置用例固化为仓库内**常驻**回归测试，而非仅留 `evidence/*.txt`。缺陷本质是"结构体少复制一行"，只有常驻断言能防止再次静默回归；夹具可直接落地（`cmd/testdata/config/1.yaml` 存在）。注意 `zero-fallback` 子用例期望值为 0，**绑定前后都通过、不具判别力**，应保留但标注。
|
||||
|
||||
7. **N7（备案，无需改动）** 第二调用方 `cmd/fmt-gen.go:80` 也执行 `buildServerCtxt`，而 `fmtGenFlags`（`:30-45`）未注册该 flag。我核验 `minio/cli@v1.24.2/flag_generated.go:141-151`：`lookupDuration` 对未注册 flag 返回 0 且不 panic；现存 `ctx.Duration("idle-timeout")` 已在该路径长期运行，证明模式安全，且 fmt-gen 不启动 HTTP server。
|
||||
|
||||
8. **N8（勿顺手清理）** `common-main.go:444` 与 `:448` 重复赋值 `ctxt.UserTimeout`，属既有无害冗余。新行紧邻该处，**请不要在本次改动中一并清理**，以保持 diff 最小。
|
||||
|
||||
9. **N9（证据闭环）** `vet.log` 仍为 0 字节（v1 评审 N10 已接受尚未回填）；v2 两条新链建议统一记录命令行、退出码与二进制 SHA256。`runtime-v1.json` 只是**修复前**证据（`expected_rejection=false`，两例均 `HTTP/1.1 200 OK`），必须补一份 `expected_rejection=true` 的同探针输出才算闭环。我另行核验了探针判据 `rejected = not data`（`runtime_probe.py:59`）**正确**：读头超时后 Go 走 `isCommonNetReadError`（`server.go:1915-1926`，`net.Error.Timeout()` 为真）→ `:2090-2091` `return // don't reply`，确实不写任何响应字节，不会出现 408 误判。
|
||||
|
||||
---
|
||||
|
||||
## 三、边界（不由本次修复覆盖，请在交付说明中重述）
|
||||
|
||||
- HTTP/2 既有的**绝对** per-stream `ReadTimeout` 不在 R8 范围（`http2.go:203` 直接透传 `ReadTimeout`）。
|
||||
- TLS 握手**写**侧仍为滚动截止，属既有边界。
|
||||
- 我未执行任何测试；`darwin-race.log` / `linux-race.log` / `grid.log` / `default-30s-transfers.log` 等均为我阅读的记录，不构成我的背书。修改过的 h2 与 strict 夹具需在本轮变更后重跑并回填。
|
||||
@@ -0,0 +1,24 @@
|
||||
{
|
||||
"baseline_sha": "9ebe81c1b3611f9cc73e676b5b741c2be62c467a",
|
||||
"plan": "plan-v2.md",
|
||||
"plan_sha256": "426127ed9fb08aeddf8259ebdc4b1c24ebec8cda751a970ed99338a44b065f4c",
|
||||
"requested_model": "claude-opus-5",
|
||||
"effort": "max",
|
||||
"cli_version": "2.1.270",
|
||||
"status": "completed",
|
||||
"created_at_utc": "2026-09-15T15:59:43.188547+00:00",
|
||||
"actual_assistant_models": [
|
||||
"claude-opus-5"
|
||||
],
|
||||
"result_subtype": "success",
|
||||
"is_error": false,
|
||||
"session_id": "a1e9e8a2-3417-440b-93b6-c749cbca204f",
|
||||
"duration_ms": 211549,
|
||||
"raw_path": "/Users/vonng/tmp/silo-r8-01a0a5b9/opus-v2.jsonl",
|
||||
"stderr_path": "/Users/vonng/tmp/silo-r8-01a0a5b9/opus-v2.stderr.log",
|
||||
"raw_sha256": "28a1266b05b8813295168530d9b9dcc792a379a798e74419ee1f06aad364defd",
|
||||
"review_sha256": "da31df576da632377c963da2ad32e96a9bf4d5f747fb8e2493573b32a899a4ed",
|
||||
"completed_at_utc": "2026-09-15T16:07:10.728735+00:00",
|
||||
"verdict": "CONSENSUS",
|
||||
"blocking_disagreements": 0
|
||||
}
|
||||
@@ -0,0 +1,3 @@
|
||||
You are the independent actual Opus 5.0 reviewer for SILO R8. Read-only review; no code changes. Read AGENTS.md, docs/investigations/r8/plan-v1.md (SHA256 7cb609e37e3199ecd93c992f08d123968ba8082682a108f20490e0644d735366), docs/investigations/r8/evidence/baseline.log, internal/deadlineconn/deadlineconn.go, internal/http/listener.go, internal/http/server.go, cmd/server-main.go (timeout configuration), internal/http/dial_linux.go, internal/grid/manager.go (unwrap), cmd/utils.go (TLS NextProtos). Inspect actual Go1.27.1 net/http code as needed using allowed Read/Grep tools; baseline is 9ebe81c1b3611f9cc73e676b5b741c2be62c467a. No production patch exists yet.
|
||||
|
||||
The user requires real Opus discussion and explicit same-plan agreement before implementation. Independently challenge correctness and minimal compatibility. Verify exact ConnState phase ordering, TLS handshake, zero deadline/background EOF behavior, keep-alive second header, long uploads, strict mode concurrency, H2 preservation and Linux internode/grid callers. State blocking vs nonblocking findings with concrete source reasoning; compare alternatives if there is a simpler correct approach. Respond in Chinese (technical identifiers preserved), with verdict CONSENSUS or REVISE. CONSENSUS must explicitly name plan v1 and SHA256 and mean there are no remaining blocking disagreements; test implementation still follows. If any blocker exists do not give consensus. Do not simulate tools or pretend tests were run.
|
||||
@@ -0,0 +1,11 @@
|
||||
You are the actual independent Opus 5.0 reviewer. Read-only; do not attempt to write any file. The user requires explicit same-version agreement for the COMPLETE R8 scope before the new configuration change is implemented.
|
||||
|
||||
Review plan v2 at docs/investigations/r8/plan-v2.md, SHA256 426127ed9fb08aeddf8259ebdc4b1c24ebec8cda751a970ed99338a44b065f4c, on baseline 9ebe81c1b3611f9cc73e676b5b741c2be62c467a. V1 connection repair is already implemented after actual v1 CONSENSUS; see consensus.md and review/opus-v1.md. V1 does not cover the newly found CLI binding omission. The only new proposed production edit is ctxt.ReadHeaderTimeout = ctx.Duration("read-header-timeout") beside IdleTimeout in cmd/common-main.go. It has NOT been applied.
|
||||
|
||||
Evidence: evidence/config-baseline.log and config_baseline_test.go.txt exercise the REAL cli.App with serverCmd.Flags and buildServerCtxt. CLI parses correct default/flag/env/precedence/YAML/negative durations, but context is 0. Explicit zero remains 0. Also read evidence/runtime-v1.json and runtime_probe.py: a compiled v1 SILO binary was launched on disposable localhost-only storage; both flag and env set 100ms, idle 2s, but a 400ms health request header still returns HTTP 200 because missing binding leaves ReadHeaderTimeout=0 and Go falls back to idle. These are real tests by Codex, not claims that you ran them.
|
||||
|
||||
Read actual cmd/common-main.go buildServerCtxt; cmd/server-main.go ServerFlags, configCommonToSrvCtx, server setup; current DeadlineConn/listener/server files and new tests; review/implementation-opus.md if it now exists (v1 implementation review only); v2 validation matrix and compatibility notes. Independently confirm minimal sufficiency and side effects of binding defaults and custom values, zero/negative semantics, env precedence, YAML retention, header vs idle behavior, keepalive, TLS, h2, body/download renewal and shared callers. Do not trust a patch/plan claim as execution. Inspect Go1.27.1 source as needed.
|
||||
|
||||
Return Chinese verdict CONSENSUS or REVISE for COMPLETE plan v2, explicitly naming v2 and its SHA256. If consensus, say there are zero blocking disagreements and authorize the proposed configuration binding within that plan, with implementation/testing still separate. List numbered blocking/nonblocking findings with concrete evidence. Any substantive required design change must use REVISE. No rate-limit or missing reply counts as agreement.
|
||||
|
||||
Additional completed v1 implementation review: review/implementation-opus.md returned REQUEST_CHANGES for one TEST dependency issue (new direct x/net/http2 import while go.mod marked module indirect), explicitly no production behavior defect. Read review/implementation-dispositions.md. The test now uses the standard Go HTTP/2-only Protocols setting, so no external x/net import or go.mod change is needed; h2 assertions remain. Also added repeated-clamp unit coverage and widened only h2 keep-alive idle. Independently decide whether B1 is resolved and whether COMPLETE v2 has any remaining blockers.
|
||||
@@ -0,0 +1,10 @@
|
||||
{
|
||||
"internal/deadlineconn/deadlineconn.go": "b9272ef640f1d4403b3d0af6cdbaba9186c51ad9a0226dfe449e8ef738e1ec4b",
|
||||
"internal/http/listener.go": "49628575367f6ab9b6986caf594726d74d370f7d2ac4eed582903600b6eb3fa2",
|
||||
"internal/http/server.go": "b7b0355f2781f8c5f7c77bc910cd4180cd3e5f22a87de41bd35ef119d36b4cdf",
|
||||
"cmd/server-main.go": "04c265de211412ba0297396928096d7f2d971244a957a3126154846035263514",
|
||||
"cmd/common-main.go": "c06df6c3051ceb264f7ddbdddea33ebe38a59e0c113b5c4c44906c1c2e08a01b",
|
||||
"internal/deadlineconn/deadlineconn_strict_test.go": "f405690c9ff044595f48323d68f4a9b33ce695b3ad6820f54f17151067bfae5e",
|
||||
"internal/http/server_deadline_test.go": "36103c155795ab24a7b8f3101f2091c793f9cbe1f2459b5bea51a8a2dc192a52",
|
||||
"internal/http/dial_deadline_linux_test.go": "0939d05b72a09760d53fcdf249775989e3f89bca824b9961d0b2a657ebfdf41e"
|
||||
}
|
||||
Reference in New Issue
Block a user