httpclient: 记录 RFC 9110 redirect gap, deferred test 锁住当前行为

代码 review 发现 DoWithRedirect 有两个 RFC 9110 §15.4 标准违反,
但当前不修:

  1. 303 See Other 应该把 method 切到 GET, 当前保持原 method (例如
     POST → 307 → 继续 POST). YARN/SHS 当前不用 303, gap 未触发.
  2. 307 Temporary Redirect / 308 Permanent Redirect 收到 POST 时
     应该原 method + body 重发, 当前 body 强制置 nil. YARN amContainerLogs
     是 GET 触发 307, gap 未触发.

不修原因: 没有真实用户报告. 修 307 body 重发要 buffer 起来 + 重写
Content-Length, 容易出 subtle bug (multipart/trailer). 修复成本 vs
收益不成比例.

新增 TestDoWithRedirect_DeferredRFC9110Gaps (2 subtest) 锁定当前
行为, 未来要修必须主动改测试 → 强制 review 行为变更. 同时给
redirect.go 加 TODO(redirect) 注释详细说明两个 gap 和修法方向.

subtest 修正一个错位: 303 的 body 丢失不是 gap (RFC 规定 303 必须
丢 body, 跟 method 错没错无关), gap 只是 method 保留 vs 切 GET.
已对应调整注释和断言.

未提交: .env (gitignored 内容之外有意未提交)

Co-Authored-By: Claude <noreply@anthropic.com>

internal/httpclient/redirect.go

@@ -56,6 +56,15 @@ func (c *Client) DoWithRedirect(ctx context.Context, req *http.Request, maxRedir
 		}
 		// **保留** header (Authorization 等)
 		newReq.Header = current.Header.Clone()
+		// TODO(redirect): per RFC 9110 §15.4, 303 See Other must switch the
+		// method to GET and drop the body; 307 Temporary Redirect and
+		// 308 Permanent Redirect must preserve both method AND body.  This
+		// implementation always preserves the method and discards the body,
+		// which is correct for 307/308 GET (YARN amContainerLogs) but
+		// non-compliant for 303 and any 307/308 POST.  Current YARN / SHS
+		// API surface only triggers 307 on GET, so the gap is unfired.
+		// TestDoWithRedirect_DeferredRFC9110Gaps locks the current behavior
+		// so a future fix must update that test deliberately.
 		newReq.Body = nil
 		current = newReq
 	}

internal/httpclient/redirect_test.go

@@ -140,3 +140,87 @@ func TestDoWithRedirect_ZeroDisallows(t *testing.T) {
 		t.Fatalf("status=%d, want 302", resp.StatusCode)
 	}
 }
+
+// TestDoWithRedirect_DeferredRFC9110Gaps documents two known
+// non-conformances with RFC 9110 §15.4 that we have NOT fixed because
+// the current upstream YARN / SHS API surface only triggers 307 on
+// GET amContainerLogs, where neither gap is fired.
+//
+// The assertions below verify the CURRENT (gap) behavior.  Any future
+// fix must update these assertions deliberately — a silent flip would
+// mean the gap fix passed without anyone thinking about 303 method
+// switch or 307-POST body resend.  See the TODO comment in
+// redirect.go for the full discussion.
+//
+//	(1) 303 See Other: we keep the original method (RFC requires switch
+//	    to GET).  The body IS correctly dropped — that's RFC-compliant
+//	    even with the wrong method.
+//	(2) 307 Temporary Redirect / 308 Permanent Redirect on a POST: we
+//	    keep the method (correct) but drop the body (RFC requires
+//	    resend with the same method).
+func TestDoWithRedirect_DeferredRFC9110Gaps(t *testing.T) {
+	t.Run("303 keeps method, should switch to GET", func(t *testing.T) {
+		var hitMethod, hitBody string
+		target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			hitMethod = r.Method
+			b, _ := io.ReadAll(r.Body)
+			hitBody = string(b)
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer target.Close()
+
+		origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Location", target.URL+"/see-other")
+			w.WriteHeader(http.StatusSeeOther)
+		}))
+		defer origin.Close()
+
+		c := New(Config{Timeout: 5 * time.Second, MaxResponseBytes: 1024})
+		req, _ := http.NewRequest(http.MethodPost, origin.URL+"/start", strings.NewReader("payload-303"))
+		if _, err := c.DoWithRedirect(context.Background(), req, 5, "127.0.0.1"); err != nil {
+			t.Fatalf("DoWithRedirect: %v", err)
+		}
+
+		// CURRENT (gap) behavior.  RFC-correct behavior: hitMethod == "GET".
+		if hitMethod != http.MethodPost {
+			t.Errorf("303 arrived as %s, want POST (current gap; RFC says GET)", hitMethod)
+		}
+		// Body must be empty — that part is RFC-compliant (303 always
+		// drops body, even when the method is wrong).
+		if hitBody != "" {
+			t.Errorf("303 body=%q, want empty (RFC requires drop regardless of method)", hitBody)
+		}
+	})
+
+	t.Run("307 on POST drops body, should resend", func(t *testing.T) {
+		var hitMethod, hitBody string
+		target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			hitMethod = r.Method
+			b, _ := io.ReadAll(r.Body)
+			hitBody = string(b)
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer target.Close()
+
+		origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Header().Set("Location", target.URL+"/temp")
+			w.WriteHeader(http.StatusTemporaryRedirect)
+		}))
+		defer origin.Close()
+
+		c := New(Config{Timeout: 5 * time.Second, MaxResponseBytes: 1024})
+		req, _ := http.NewRequest(http.MethodPost, origin.URL+"/start", strings.NewReader("payload-307"))
+		if _, err := c.DoWithRedirect(context.Background(), req, 5, "127.0.0.1"); err != nil {
+			t.Fatalf("DoWithRedirect: %v", err)
+		}
+
+		// Method preserved (correct).
+		if hitMethod != http.MethodPost {
+			t.Errorf("307 arrived as %s, want POST", hitMethod)
+		}
+		// CURRENT (gap) behavior.  RFC-correct behavior: hitBody == "payload-307".
+		if hitBody != "" {
+			t.Errorf("307 body=%q, want empty (current gap; RFC says resend payload-307)", hitBody)
+		}
+	})
+}