Don't retry POST after a lost reply, timeout or 5xx

src/netbird/client.ts

@@ -9,7 +9,7 @@ export interface NetBirdClientOptions {
   timeoutMs: number;
   /** Injectable for tests. Defaults to global fetch. */
   fetchImpl?: typeof fetch;
-  /** Max retries on 429 / 5xx / network errors. */
+  /** Max retries on 429 / 5xx / network errors (POST: 429 only). */
   maxRetries?: number;
 }
 
@@ -62,7 +62,7 @@ export type TokenVerification = "ok" | "invalid" | "unknown";
 /**
  * Thin REST client for the NetBird public API. Handles auth, timeouts, a
  * client-side rate limit, and exponential backoff on 429/5xx (honoring
- * Retry-After). Transport-agnostic — one instance per AuthContext.
+ * Retry-After; POST only on 429). Transport-agnostic — one instance per AuthContext.
  */
 export class NetBirdClient {
   private readonly fetchImpl: typeof fetch;
@@ -187,22 +187,32 @@ export class NetBirdClient {
     path: string,
   ): Promise<RetryDecision<T>> {
     const canRetry = attempt < this.maxRetries;
+    // POST is not idempotent: after a lost reply, a timeout or a 5xx the server
+    // may already have created the object, and sending it again creates a second
+    // one. So a POST is retried only on 429 (rejected before processing). GET,
+    // PUT (full replacement) and DELETE leave the same state when repeated.
+    const idempotent = method !== "POST";
+    // When a POST is not retried, say so in the error, so a caller (or an agent)
+    // doesn't treat it as a definite failure and simply send it again.
+    const uncertain = idempotent
+      ? ""
+      : " (the request may have been processed; check before retrying)";
 
     if (outcome.kind === "error") {
-      if (canRetry) {
+      if (canRetry && idempotent) {
         await this.backoff(backoffMs(attempt), "netbird request failed, retrying", {
           method,
           path,
           attempt,
         });
         return { action: "retry" };
       }
-      const msg = `Network error calling NetBird ${method} ${path}: ${(outcome.error as Error).message}`;
+      const msg = `Network error calling NetBird ${method} ${path}: ${(outcome.error as Error).message}${uncertain}`;
       return { action: "throw", error: new NetBirdApiError(msg, 0) };
     }
 
     const { res } = outcome;
-    if ((res.status === 429 || res.status >= 500) && canRetry) {
+    if ((res.status === 429 || (res.status >= 500 && idempotent)) && canRetry) {
       const retryAfter = parseRetryAfter(res.headers.get("retry-after"));
       // Clamp the honored delay: never trust an upstream Retry-After beyond the cap.
       const delay = Math.min(retryAfter ?? backoffMs(attempt), MAX_BACKOFF_MS);
@@ -215,11 +225,28 @@ export class NetBirdClient {
       return { action: "retry" };
     }
 
-    const text = await res.text();
+    let text: string;
+    try {
+      text = await res.text();
+    } catch (err) {
+      // The headers arrived but the body was cut off. Idempotent methods retry like
+      // any other transport error; a POST may still have been processed, so it
+      // keeps the same warning instead.
+      if (canRetry && idempotent) {
+        await this.backoff(backoffMs(attempt), "netbird response cut off, retrying", {
+          method,
+          path,
+          attempt,
+        });
+        return { action: "retry" };
+      }
+      const msg = `Network error reading NetBird ${method} ${path} response: ${(err as Error).message}${uncertain}`;
+      return { action: "throw", error: new NetBirdApiError(msg, 0) };
+    }
     const parsed = text ? safeJson(text) : undefined;
     if (!res.ok) {
       const error = new NetBirdApiError(
-        `NetBird ${method} ${path} failed with ${res.status}`,
+        `NetBird ${method} ${path} failed with ${res.status}${res.status >= 500 ? uncertain : ""}`,
         res.status,
         parsed ?? text,
       );

test/client.test.ts

@@ -206,3 +206,120 @@ describe("NetBirdClient#verifyToken", () => {
     expect(await client.verifyToken()).toBe("unknown");
   });
 });
+
+describe("NetBirdClient retries and non-idempotent POSTs", () => {
+  // A stand-in for the NetBird API: every POST /api/setup-keys creates a key
+  // before the reply goes out. The first reply then goes wrong in one of the
+  // ways a client can't tell apart from "not created": the connection drops,
+  // the client times out, or a proxy answers 503.
+  function setupKeyServer(firstReply: "drop" | "timeout" | "503") {
+    const created: string[] = [];
+    const fetchImpl = vi.fn(async (_url, init?: RequestInit) => {
+      const id = `key-${created.length + 1}`;
+      created.push(id);
+      if (created.length === 1) {
+        if (firstReply === "drop") throw new TypeError("fetch failed: socket hang up");
+        if (firstReply === "503") {
+          return new Response("upstream error", { status: 503, headers: { "retry-after": "0" } });
+        }
+        await new Promise((_, reject) =>
+          init?.signal?.addEventListener("abort", () =>
+            reject(new DOMException("The operation was aborted.", "AbortError")),
+          ),
+        );
+      }
+      return jsonResponse({ id });
+    }) as unknown as typeof fetch;
+    return { created, fetchImpl };
+  }
+
+  for (const firstReply of ["drop", "timeout", "503"] as const) {
+    it(`one POST creates one object when the reply is lost (${firstReply})`, async () => {
+      const { created, fetchImpl } = setupKeyServer(firstReply);
+      const client = new NetBirdClient({
+        auth: { token: "test-pat", baseUrl: "https://api.netbird.io" },
+        logger: silentLogger,
+        rateLimiter: new RateLimiter(1000),
+        timeoutMs: 50,
+        fetchImpl,
+      });
+
+      const err = await client.post("/api/setup-keys", { name: "ci-runner" }).catch((e) => e);
+      expect(err).toBeInstanceOf(NetBirdApiError);
+      expect(err.message).toContain("may have been processed");
+      expect(fetchImpl).toHaveBeenCalledOnce();
+      expect(created).toEqual(["key-1"]);
+    });
+  }
+
+  it("keeps the POST warning when the reply body is cut off", async () => {
+    const fetchImpl = vi.fn(async () => {
+      const body = new ReadableStream({
+        start(controller) {
+          controller.error(new TypeError("terminated: other side closed"));
+        },
+      });
+      return new Response(body, { status: 200, headers: { "content-type": "application/json" } });
+    }) as unknown as typeof fetch;
+
+    const err = await makeClient(fetchImpl).post("/api/setup-keys", { name: "ci-runner" }).catch((e) => e);
+    expect(err).toBeInstanceOf(NetBirdApiError);
+    expect(err.message).toContain("may have been processed");
+    expect(fetchImpl).toHaveBeenCalledOnce();
+  });
+
+  it("retries a GET whose reply body is cut off", async () => {
+    let calls = 0;
+    const fetchImpl = vi.fn(async () => {
+      calls++;
+      if (calls === 1) {
+        const body = new ReadableStream({
+          start(controller) {
+            controller.error(new TypeError("terminated: other side closed"));
+          },
+        });
+        return new Response(body, { status: 200, headers: { "content-type": "application/json" } });
+      }
+      return jsonResponse([{ id: "p1" }]);
+    }) as unknown as typeof fetch;
+
+    const peers = await makeClient(fetchImpl).get("/api/peers");
+    expect(peers).toEqual([{ id: "p1" }]);
+    expect(calls).toBe(2);
+  });
+
+  it("still retries a POST on 429, which the server rejected before processing", async () => {
+    let calls = 0;
+    const fetchImpl = vi.fn(async () => {
+      calls++;
+      if (calls === 1) return new Response("", { status: 429, headers: { "retry-after": "0" } });
+      return jsonResponse({ id: "key-1" });
+    }) as unknown as typeof fetch;
+
+    const client = makeClient(fetchImpl);
+    expect(await client.post("/api/setup-keys", { name: "ci-runner" })).toEqual({ id: "key-1" });
+    expect(calls).toBe(2);
+  });
+
+  for (const method of ["get", "put", "delete"] as const) {
+    it(`still retries an idempotent ${method.toUpperCase()} on 503 and on a network error`, async () => {
+      let calls = 0;
+      const fetchImpl = vi.fn(async () => {
+        calls++;
+        if (calls === 1) throw new TypeError("fetch failed");
+        if (calls === 2) return new Response("", { status: 503, headers: { "retry-after": "0" } });
+        return jsonResponse({ ok: true });
+      }) as unknown as typeof fetch;
+
+      const client = makeClient(fetchImpl);
+      const result =
+        method === "get"
+          ? await client.get("/api/groups/g1")
+          : method === "put"
+            ? await client.put("/api/groups/g1", { name: "eng" })
+            : await client.delete("/api/groups/g1");
+      expect(result).toEqual({ ok: true });
+      expect(calls).toBe(3);
+    });
+  }
+});