fix(web-api): retry responses interrupted while reading the body

.changeset/web-api-retry-interrupted-response-body.md

@@ -0,0 +1,5 @@
+---
+"@slack/web-api": patch
+---
+
+fix(web-api): retry and error-wrap responses whose connection drops while the body is being read

packages/web-api/src/WebClient.test.ts

@@ -1,5 +1,6 @@
 import assert from 'node:assert/strict';
 import fs from 'node:fs';
+import http from 'node:http';
 import { afterEach, beforeEach, describe, it } from 'node:test';
 import zlib from 'node:zlib';
 import type { ContextActionsBlock } from '@slack/types';
@@ -515,6 +516,108 @@ describe('WebClient', () => {
       }
     });
 
+    describe('when the response body is interrupted', () => {
+      const servers: http.Server[] = [];
+      afterEach(() => {
+        for (const server of servers.splice(0)) {
+          server.closeAllConnections();
+          server.close();
+        }
+      });
+
+      // `fetch` resolves once the response headers arrive, so a connection dropped partway through
+      // the body cannot be simulated with nock. These use a local server that writes a 200 status
+      // line and part of the body before destroying the socket. `Content-Length` deliberately
+      // overstates the body so the client is still waiting on it when the socket goes away.
+      function startFlakyServer(succeedAfter: number): Promise<{ url: string; requests: () => number }> {
+        let requests = 0;
+        const server = http.createServer((req, res) => {
+          req.resume();
+          requests += 1;
+          if (requests > succeedAfter) {
+            res.end('{"ok":true}');
+            return;
+          }
+          res.writeHead(200, { 'Content-Type': 'application/json', 'Content-Length': '100' });
+          res.write('{"ok":');
+          setTimeout(() => res.destroy(), 10);
+        });
+        servers.push(server);
+        return new Promise((resolve) => {
+          server.listen(0, '127.0.0.1', () => {
+            const address = server.address();
+            assert.ok(address !== null && typeof address !== 'string');
+            resolve({ url: `http://127.0.0.1:${address.port}/`, requests: () => requests });
+          });
+        });
+      }
+
+      it('should retry the request when the connection drops while reading the body', async () => {
+        const { url, requests } = await startFlakyServer(1);
+        const client = new WebClient(token, {
+          slackApiUrl: url,
+          retryConfig: { retries: 1, minTimeout: 1, maxTimeout: 1 },
+        });
+
+        const result = await client.apiCall('method');
+
+        assert.strictEqual(result.ok, true);
+        assert.strictEqual(requests(), 2);
+      });
+
+      it('should retry when reading the body rejects, without depending on socket timing', async () => {
+        // A deterministic counterpart to the socket-level tests above: the body read is what fails,
+        // which is the exact condition #2738 describes, with no reliance on when the connection drops.
+        let calls = 0;
+        const fetchFn: FetchFunction = async () => {
+          calls += 1;
+          const failing = calls === 1;
+          const payload = JSON.stringify({ ok: true });
+          const readBody = async () => {
+            if (failing) throw new TypeError('terminated');
+            return payload;
+          };
+          return {
+            ok: true,
+            status: 200,
+            statusText: 'OK',
+            url: 'https://slack.com/api/method',
+            headers: { get: () => null, entries: () => [] },
+            arrayBuffer: async () => new TextEncoder().encode(await readBody()).buffer as ArrayBuffer,
+            text: readBody,
+            json: async () => JSON.parse(await readBody()),
+          };
+        };
+        const client = new WebClient(token, {
+          fetch: fetchFn,
+          retryConfig: { retries: 1, minTimeout: 1, maxTimeout: 1 },
+        });
+
+        const result = await client.apiCall('method');
+
+        assert.strictEqual(result.ok, true);
+        assert.strictEqual(calls, 2);
+      });
+
+      it('should fail with WebAPIRequestError once retries are exhausted', async () => {
+        const { url } = await startFlakyServer(Number.POSITIVE_INFINITY);
+        const client = new WebClient(token, {
+          slackApiUrl: url,
+          retryConfig: { retries: 1, minTimeout: 1, maxTimeout: 1 },
+        });
+
+        try {
+          await client.apiCall('method');
+          assert.fail('expected error to be thrown');
+        } catch (error) {
+          assert.ok(error instanceof WebAPIRequestError);
+          assert.ok(error instanceof SlackError);
+          assert.strictEqual(error.code, ErrorCode.RequestError);
+          assert.ok(error.original instanceof Error);
+        }
+      });
+    });
+
     it('should set error.body to the raw string when HTTP error response is not valid JSON', async () => {
       const htmlBody = '<html><body><h1>502 Bad Gateway</h1></body></html>';
       const scope = nock('https://slack.com').post(/api/).reply(502, htmlBody);

packages/web-api/src/WebClient.ts

@@ -659,7 +659,11 @@ export class WebClient extends Methods {
             );
           }
 
-          return response;
+          // Read the body here, while still inside the retried task, so that a connection dropped
+          // partway through the response is retried and error-wrapped like any other request
+          // failure. `fetch` resolves as soon as the response headers arrive, so a body read left
+          // to the caller would land outside both this retry loop and the `catch` below.
+          return await bufferResponseBody(response);
         } catch (error) {
           if (error instanceof AbortError) {
             throw error;
@@ -859,6 +863,25 @@ function paginationOptionsForNextPage(
   return undefined;
 }
 
+/**
+ * Read a response body in full and return a {@link FetchResponse} that replays those bytes, so that
+ * the body can be consumed again by callers without touching the network.
+ */
+async function bufferResponseBody(response: FetchResponse): Promise<FetchResponse> {
+  const body = await response.arrayBuffer();
+  const decode = () => new TextDecoder().decode(body);
+  return {
+    ok: response.ok,
+    status: response.status,
+    statusText: response.statusText,
+    url: response.url,
+    headers: response.headers,
+    arrayBuffer: async () => body,
+    text: async () => decode(),
+    json: async () => JSON.parse(decode()),
+  };
+}
+
 /**
  * Extract the amount of time (in seconds) the platform has recommended this client wait before sending another request
  * from a rate-limited HTTP response (statusCode = 429).