fix(coderd/x/chatd/chatdebug): restore request body after capture (#24784)

> Mux working on behalf of Mike. Debug recording could consume request bodies when a provider SDK returned the active body from `GetBody`, which left the upstream request with an empty body after capture. Reset the request body after debug capture and add coverage for shared `GetBody` readers so debug logging does not alter the bytes sent upstream.
2026-09-22 05:05:20 +08:00 · 2026-04-28 19:09:27 +02:00
parent 4a91656fe5
commit 1d8e29815e
2 changed files with 204 additions and 7 deletions
@@ -12,6 +12,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"golang.org/x/xerrors"
 )

 // attemptStatusCompleted is the status recorded when a response body
@@ -160,14 +162,22 @@ func captureRequestBody(req *http.Request) ([]byte, error) {
 	if req.GetBody != nil {
 		clone, err := req.GetBody()
 		if err == nil {
-			defer clone.Close()
-			limited, err := io.ReadAll(io.LimitReader(clone, maxRecordedRequestBodyBytes+1))
-			if err == nil {
-				if len(limited) > maxRecordedRequestBodyBytes {
-					return []byte("[TRUNCATED]"), nil
-				}
-				return RedactJSONSecrets(limited), nil
+			limited, readErr := io.ReadAll(io.LimitReader(clone, maxRecordedRequestBodyBytes+1))
+			_ = clone.Close()
+			// Some SDKs return the active body from GetBody instead of an
+			// independent reader. Restore the request body from GetBody so
+			// the upstream transport still receives the original bytes.
+			resetErr := resetRequestBody(req)
+			if resetErr != nil {
+				return nil, xerrors.Errorf("chatdebug: reset request body: %w", resetErr)
 			}
+			if readErr != nil {
+				return nil, nil
+			}
+			if len(limited) > maxRecordedRequestBodyBytes {
+				return []byte("[TRUNCATED]"), nil
+			}
+			return RedactJSONSecrets(limited), nil
 		}
 	}

@@ -178,6 +188,24 @@ func captureRequestBody(req *http.Request) ([]byte, error) {
 	return nil, nil
 }

+// resetRequestBody replaces req.Body with a fresh reader from req.GetBody.
+// It closes the previous request body before installing the replacement.
+// Callers must ensure req.GetBody is non-nil.
+func resetRequestBody(req *http.Request) error {
+	body, err := req.GetBody()
+	if err != nil {
+		return err
+	}
+	if req.Body != nil {
+		if err := req.Body.Close(); err != nil {
+			_ = body.Close()
+			return err
+		}
+	}
+	req.Body = body
+	return nil
+}
+
 type recordingBody struct {
 	inner         io.ReadCloser
 	contentLength int64
@@ -1,6 +1,7 @@
 package chatdebug //nolint:testpackage // Uses unexported recorder helpers.

 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -56,6 +57,17 @@ func (*scriptedReadCloser) Close() error {
 	return nil
 }

+type closeTrackingReadCloser struct {
+	*bytes.Reader
+	closed   bool
+	closeErr error
+}
+
+func (c *closeTrackingReadCloser) Close() error {
+	c.closed = true
+	return c.closeErr
+}
+
 func TestRecordingTransport_NoSink(t *testing.T) {
 	t.Parallel()

@@ -139,6 +151,163 @@ func TestRecordingTransport_CaptureRequest(t *testing.T) {
 	require.Equal(t, "Bearer top-secret", received.authorization)
 }

+func TestRecordingTransport_CaptureRequestRestoresSharedGetBody(t *testing.T) {
+	t.Parallel()
+
+	const requestBody = `{"message":"hello","api_key":"super-secret"}`
+
+	gotRequest := make(chan []byte, 1)
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		body, err := io.ReadAll(req.Body)
+		require.NoError(t, err)
+		gotRequest <- body
+		_, _ = rw.Write([]byte(`{"ok":true}`))
+	}))
+	defer server.Close()
+
+	ctx, sink := newTestSinkContext(t)
+	client := &http.Client{
+		Transport: &RecordingTransport{Base: server.Client().Transport},
+	}
+
+	reader := bytes.NewReader([]byte(requestBody))
+	originalBody := &closeTrackingReadCloser{Reader: reader}
+	req, err := http.NewRequestWithContext(
+		ctx,
+		http.MethodPost,
+		server.URL,
+		originalBody,
+	)
+	require.NoError(t, err)
+	req.ContentLength = int64(len(requestBody))
+	req.GetBody = func() (io.ReadCloser, error) {
+		_, err := reader.Seek(0, io.SeekStart)
+		if err != nil {
+			return nil, err
+		}
+		return io.NopCloser(reader), nil
+	}
+
+	resp, err := client.Do(req)
+	require.NoError(t, err)
+	_, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+
+	require.JSONEq(t, requestBody, string(<-gotRequest))
+	require.True(t, originalBody.closed)
+	attempts := sink.snapshot()
+	require.Len(t, attempts, 1)
+	require.Equal(t, attemptStatusCompleted, attempts[0].Status)
+	require.JSONEq(t, `{"message":"hello","api_key":"[REDACTED]"}`, string(attempts[0].RequestBody))
+}
+
+func TestRecordingTransport_CaptureRequestResetFailureFailsRequest(t *testing.T) {
+	t.Parallel()
+
+	const requestBody = `{"message":"hello"}`
+
+	gotRequest := make(chan struct{}, 1)
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		gotRequest <- struct{}{}
+		_, _ = rw.Write([]byte(`{"ok":true}`))
+	}))
+	defer server.Close()
+
+	ctx, sink := newTestSinkContext(t)
+	client := &http.Client{
+		Transport: &RecordingTransport{Base: server.Client().Transport},
+	}
+
+	reader := bytes.NewReader([]byte(requestBody))
+	req, err := http.NewRequestWithContext(
+		ctx,
+		http.MethodPost,
+		server.URL,
+		io.NopCloser(reader),
+	)
+	require.NoError(t, err)
+	req.ContentLength = int64(len(requestBody))
+	getBodyCalls := 0
+	req.GetBody = func() (io.ReadCloser, error) {
+		getBodyCalls++
+		if getBodyCalls == 2 {
+			return nil, xerrors.New("reset failed")
+		}
+		_, err := reader.Seek(0, io.SeekStart)
+		if err != nil {
+			return nil, err
+		}
+		return io.NopCloser(reader), nil
+	}
+
+	resp, err := client.Do(req)
+	if resp != nil {
+		require.NoError(t, resp.Body.Close())
+	}
+	require.ErrorContains(t, err, "chatdebug: reset request body: reset failed")
+	require.Nil(t, resp)
+	require.Empty(t, sink.snapshot())
+	select {
+	case <-gotRequest:
+		t.Fatal("request should not be sent with a drained body")
+	default:
+	}
+}
+
+func TestRecordingTransport_CaptureRequestBodyCloseFailureFailsRequest(t *testing.T) {
+	t.Parallel()
+
+	const requestBody = `{"message":"hello"}`
+
+	gotRequest := make(chan struct{}, 1)
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		gotRequest <- struct{}{}
+		_, _ = rw.Write([]byte(`{"ok":true}`))
+	}))
+	defer server.Close()
+
+	ctx, sink := newTestSinkContext(t)
+	client := &http.Client{
+		Transport: &RecordingTransport{Base: server.Client().Transport},
+	}
+
+	reader := bytes.NewReader([]byte(requestBody))
+	originalBody := &closeTrackingReadCloser{
+		Reader:   reader,
+		closeErr: xerrors.New("close failed"),
+	}
+	req, err := http.NewRequestWithContext(
+		ctx,
+		http.MethodPost,
+		server.URL,
+		originalBody,
+	)
+	require.NoError(t, err)
+	req.ContentLength = int64(len(requestBody))
+	req.GetBody = func() (io.ReadCloser, error) {
+		_, err := reader.Seek(0, io.SeekStart)
+		if err != nil {
+			return nil, err
+		}
+		return io.NopCloser(reader), nil
+	}
+
+	resp, err := client.Do(req)
+	if resp != nil {
+		require.NoError(t, resp.Body.Close())
+	}
+	require.ErrorContains(t, err, "chatdebug: reset request body: close failed")
+	require.Nil(t, resp)
+	require.True(t, originalBody.closed)
+	require.Empty(t, sink.snapshot())
+	select {
+	case <-gotRequest:
+		t.Fatal("request should not be sent when the captured body cannot be closed")
+	default:
+	}
+}
+
 func TestRecordingTransport_RedactsSensitiveQueryParameters(t *testing.T) {
 	t.Parallel()