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,9 +187,14 @@ 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";
 
     if (outcome.kind === "error") {
-      if (canRetry) {
+      if (canRetry && idempotent) {
         await this.backoff(backoffMs(attempt), "netbird request failed, retrying", {
           method,
           path,
@@ -202,7 +207,7 @@ export class NetBirdClient {
     }
 
     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);

test/client.test.ts

@@ -206,3 +206,84 @@ 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,
+      });
+
+      await expect(client.post("/api/setup-keys", { name: "ci-runner" })).rejects.toBeInstanceOf(
+        NetBirdApiError,
+      );
+      expect(fetchImpl).toHaveBeenCalledOnce();
+      expect(created).toEqual(["key-1"]);
+    });
+  }
+
+  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);
+    });
+  }
+});