fix(branch): survive a transient control-plane 502 while polling (v0.2.8)

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@insforge/cli",
-  "version": "0.2.7",
+  "version": "0.2.8",
   "description": "InsForge CLI - Command line tool for InsForge platform",
   "type": "module",
   "bin": {

src/commands/branch/create.test.ts

@@ -394,6 +394,145 @@ describe('branch create', () => {
     expect(exitCode).toBe(1);
   });
 
+  it('survives a transient 502 mid-poll instead of abandoning a branch that is still provisioning', async () => {
+    // The reported incident: the control plane 502s once at ~90s, the CLI exits
+    // non-zero, and the backend marks the branch ready ~15s later — leaving a
+    // real, billing branch behind a failed command. A failed READ is not a
+    // failed branch. (agent-e2e runs 31832239687 / 32055449431.)
+    const { getProjectConfig } = await import('../../lib/config.js');
+    (getProjectConfig as Mock).mockReturnValue({
+      project_id: 'p1',
+      project_name: 'parent',
+      org_id: 'o1',
+      appkey: 'p1ky',
+      region: 'us-east',
+      api_key: 'k',
+      oss_host: 'p1ky.us-east.insforge.app',
+    });
+    const { getBranchApi } = await import('../../lib/api/platform.js');
+    // One gateway 502, then the branch is ready (the default mock impl).
+    (getBranchApi as Mock).mockRejectedValueOnce(
+      new CLIError('Request failed: 502', 1, undefined, 502),
+    );
+    const logs: string[] = [];
+    const origLog = console.log;
+    console.log = (...args: unknown[]) => {
+      logs.push(args.map(String).join(' '));
+    };
+    vi.useFakeTimers();
+    let exitCode: number | undefined;
+    try {
+      exitCode = await withCapturedExit(async () => {
+        const program = new Command().exitOverride();
+        program.option('--json').option('--api-url <url>').option('-y, --yes');
+        registerBranchCreateCommand(program);
+        const run = program
+          .parseAsync(['create', 'feat-x', '--mode', 'schema-only', '--no-switch', '--json'], {
+            from: 'user',
+          })
+          .catch(() => {});
+        await vi.runAllTimersAsync();
+        await run;
+      });
+    } finally {
+      vi.useRealTimers();
+      console.log = origLog;
+    }
+    // Polled past the 502 and reported the ready branch, exit 0.
+    expect((getBranchApi as Mock).mock.calls.length).toBeGreaterThan(1);
+    expect(exitCode).toBeUndefined();
+    expect(logs.join('\n')).toContain('branch-id');
+  });
+
+  it('still gives up on a real rejection mid-poll (404 is not transient)', async () => {
+    // The tolerance must not swallow an answer that will never change — a
+    // deleted/unknown branch id has to end the command, not burn 15 minutes.
+    const { getProjectConfig } = await import('../../lib/config.js');
+    (getProjectConfig as Mock).mockReturnValue({
+      project_id: 'p1',
+      project_name: 'parent',
+      org_id: 'o1',
+    });
+    const { getBranchApi } = await import('../../lib/api/platform.js');
+    (getBranchApi as Mock).mockRejectedValueOnce(
+      new CLIError('Branch not found', 1, undefined, 404),
+    );
+    const exitCode = await withCapturedExit(async () => {
+      const program = new Command().exitOverride();
+      program.option('--json').option('--api-url <url>').option('-y, --yes');
+      registerBranchCreateCommand(program);
+      await program
+        .parseAsync(['create', 'feat-x', '--mode', 'schema-only', '--no-switch'], { from: 'user' })
+        .catch(() => {});
+    });
+    expect(getBranchApi as Mock).toHaveBeenCalledTimes(1);
+    expect(exitCode).toBe(1);
+  });
+
+  it('adopts a branch that was created despite a gateway 5xx on the create request', async () => {
+    // Same lost-response ambiguity as a transport reset: a 502 is the proxy
+    // saying IT could not complete the round trip, so the branch may well exist
+    // and be billing. The name/mode/created_at guards still apply.
+    const { getProjectConfig } = await import('../../lib/config.js');
+    (getProjectConfig as Mock).mockReturnValue({
+      project_id: 'p1',
+      project_name: 'parent',
+      org_id: 'o1',
+    });
+    const { createBranchApi, listBranchesApi } = await import('../../lib/api/platform.js');
+    (createBranchApi as Mock).mockRejectedValueOnce(
+      new CLIError('Request failed: 502', 1, undefined, 502),
+    );
+    (listBranchesApi as Mock).mockResolvedValueOnce([
+      {
+        id: 'branch-id',
+        parent_project_id: 'p1',
+        organization_id: 'o1',
+        name: 'feat-x',
+        appkey: 'p1ky-x9p',
+        region: 'us-east',
+        branch_state: 'creating',
+        branch_created_at: new Date().toISOString(),
+        branch_metadata: { mode: 'schema-only' },
+      },
+    ]);
+    const program = new Command().exitOverride();
+    program.option('--json').option('--api-url <url>').option('-y, --yes');
+    registerBranchCreateCommand(program);
+    await program
+      .parseAsync(['create', 'feat-x', '--mode', 'schema-only', '--no-switch'], { from: 'user' })
+      .catch(() => {});
+    expect(listBranchesApi as Mock).toHaveBeenCalledWith('p1', undefined);
+    expect(String(spinnerMock.stop.mock.calls.at(-1)?.[0])).not.toContain('creation failed');
+  });
+
+  it('does NOT adopt on a plain 500 — that is the API answering, not a lost response', async () => {
+    // Adoption is limited to proxy statuses (502/503/504) and transport resets.
+    // A 500 is the application's own error, so it is more likely an
+    // authoritative rejection — adopting on it widens the window in which a
+    // collaborator's same-name branch could be picked up and switched into.
+    const { getProjectConfig } = await import('../../lib/config.js');
+    (getProjectConfig as Mock).mockReturnValue({
+      project_id: 'p1',
+      project_name: 'parent',
+      org_id: 'o1',
+    });
+    const { createBranchApi, listBranchesApi } = await import('../../lib/api/platform.js');
+    (createBranchApi as Mock).mockRejectedValueOnce(
+      new CLIError('Internal server error', 1, undefined, 500),
+    );
+    const exitCode = await withCapturedExit(async () => {
+      const program = new Command().exitOverride();
+      program.option('--json').option('--api-url <url>').option('-y, --yes');
+      registerBranchCreateCommand(program);
+      await program
+        .parseAsync(['create', 'feat-x', '--mode', 'schema-only', '--no-switch'], { from: 'user' })
+        .catch(() => {});
+    });
+    expect(listBranchesApi as Mock).not.toHaveBeenCalled();
+    expect(exitCode).toBe(1);
+  });
+
   it('adopts a branch that was created despite a transport failure', async () => {
     // createBranchApi carries no idempotency key, so a reset on the RESPONSE
     // leg leaves a real, billing branch behind. Giving up here orphans it.

src/commands/branch/create.ts

@@ -7,12 +7,13 @@ import {
   NETWORK_ERROR_CODE,
 } from '../../lib/api/platform.js';
 import { probeBackendHealth } from '../../lib/api/oss.js';
-import { CLIError, getRootOpts, handleError } from '../../lib/errors.js';
+import { CLIError, getRootOpts, handleError, isTransientApiError } from '../../lib/errors.js';
 import { requireAuth } from '../../lib/credentials.js';
 import { buildOssHost, getProjectConfig } from '../../lib/config.js';
 import { outputJson, outputInfo } from '../../lib/output.js';
 import { captureEvent, shutdownAnalytics } from '../../lib/analytics.js';
 import { runBranchSwitch } from './switch.js';
+import { readBranchWithRetry } from './poll.js';
 import type { Branch, BranchMode } from '../../types.js';
 
 const POLL_INTERVAL_MS = 3_000;
@@ -31,6 +32,13 @@ const HEALTH_INTERVAL_MS = 5_000;
 // branch someone created seconds ago under the same name; the cost of being too
 // narrow is orphaning a billing resource, which is the bug this exists to fix.
 const CREATED_AT_SKEW_MS = 60_000;
+// Sentinel parked in the poll's `lastState` while control-plane reads are
+// failing, so the next successful read re-announces the real state even if it
+// has not changed. No branch_state can collide with it.
+const UNREACHABLE_STATE = '__control-plane-unreachable__';
+// Retries for the post-timeout read that decides the command's verdict. Inside
+// the loop the interval is the retry; this one has no second chance.
+const FINAL_READ_ATTEMPTS = 3;
 
 export function registerBranchCreateCommand(branch: Command): void {
   branch
@@ -181,8 +189,8 @@ export function registerBranchCreateCommand(branch: Command): void {
 }
 
 /**
- * Create the branch, and if the request fails at the TRANSPORT layer, check
- * whether it was created anyway before giving up.
+ * Create the branch, and if the request fails WITHOUT an answer from the API
+ * itself, check whether it was created anyway before giving up.
  *
  * `createBranchApi` carries no idempotency key, and a reset on the RESPONSE leg
  * leaves a fully created, billing branch behind while the CLI exits non-zero.
@@ -191,12 +199,15 @@ export function registerBranchCreateCommand(branch: Command): void {
  * authoritative here, and it is a control-plane call, so it still works while
  * the branch's own host is unreachable.
  *
- * Two guards keep this from adopting something it did not create — a duplicate
+ * Three guards keep this from adopting something it did not create — a duplicate
  * name is a REJECTION, not a lost response, and adopting on it would switch the
  * caller into someone else's branch with a different mode and different data:
  *
- *   1. only a tagged transport failure is eligible; every HTTP/API rejection
- *      (duplicate name, quota, auth) rethrows untouched;
+ *   1. only an ambiguous failure is eligible — a transport reset, or one of the
+ *      PROXY-level statuses, which is the edge reporting that IT could not
+ *      complete the round trip and says nothing about what the backend did.
+ *      Every answer the API itself authored — a duplicate name, a quota, auth,
+ *      any other 4xx, and a plain 500 — rethrows untouched;
  *   2. the branch must have been created at or after the moment we sent the
  *      request, so a pre-existing same-name branch is never a candidate;
  *   3. the branch's mode must match what we asked for.
@@ -210,9 +221,26 @@ export function registerBranchCreateCommand(branch: Command): void {
  * failure). The real fix is a server-issued idempotency/request token on
  * `createBranchApi`; until that exists, this is the tightest client-side guard.
  * Reported upstream: InsForge/InsForge#1790.
+ *
+ * When no matching branch turns up, the original error is rethrown unchanged —
+ * so guard 1 accepting proxy statuses cannot mask a request the backend never
+ * acted on.
+ *
+ * Deliberately NARROWER than `isTransientApiError`, which the poll uses:
+ *   - 500 is excluded. That is the application's own answer, so it is more
+ *     likely an authoritative rejection than a lost response, and adopting on
+ *     it would widen the window in which a collaborator's same-name branch
+ *     could be picked up and switched into (the residual collision above).
+ *     Re-reading a status after a 500 is free; adopting after one is not.
+ *   - 408/429 are excluded. A rate limit or a timeout on the POST means the
+ *     request was refused, not lost — nothing was created to adopt.
  */
-function isTransportFailure(err: unknown): boolean {
-  return err instanceof CLIError && err.code === NETWORK_ERROR_CODE;
+const PROXY_STATUSES = new Set([502, 503, 504]);
+
+function isAmbiguousCreateFailure(err: unknown): boolean {
+  if (!(err instanceof CLIError)) return false;
+  if (err.code === NETWORK_ERROR_CODE) return true;
+  return err.statusCode !== undefined && PROXY_STATUSES.has(err.statusCode);
 }
 
 async function createBranchOrAdopt(
@@ -224,7 +252,7 @@ async function createBranchOrAdopt(
   try {
     return await createBranchApi(parentId, body, apiUrl);
   } catch (err) {
-    if (!isTransportFailure(err)) throw err;
+    if (!isAmbiguousCreateFailure(err)) throw err;
     const existing = await listBranchesApi(parentId, apiUrl)
       .then(branches =>
         branches.find(
@@ -266,15 +294,40 @@ async function waitUntilServing(
   return false;
 }
 
+/**
+ * Poll the control plane until the branch reaches a terminal state.
+ *
+ * A failed READ is not a failed branch. The control plane returning 502 once
+ * mid-poll used to end the command on the spot, while the backend went on to
+ * mark the branch ready ~15s later — leaving a real, billing branch behind a
+ * non-zero exit (agent-e2e runs 31832239687 and 32055449431). Transient
+ * failures therefore consume a poll interval and nothing more; only a real
+ * rejection (auth, 404, a terminal branch state) ends the loop early.
+ */
 async function pollUntilReady(
   branchId: string,
   apiUrl: string | undefined,
   spinner: ReturnType<typeof clack.spinner> | null,
 ): Promise<Branch> {
   const start = Date.now();
   let lastState = '';
+  // Last state actually observed, so a read failure at the very end of the
+  // budget still reports what the branch was doing instead of an API error.
+  let lastBranch: Branch | null = null;
   while (Date.now() - start < POLL_TIMEOUT_MS) {
-    const branch = await getBranchApi(branchId, apiUrl);
+    let branch: Branch;
+    try {
+      branch = await getBranchApi(branchId, apiUrl);
+    } catch (err) {
+      if (!isTransientApiError(err)) throw err;
+      if (spinner && lastState !== UNREACHABLE_STATE) {
+        spinner.message('Control plane is not answering; still provisioning, retrying...');
+        lastState = UNREACHABLE_STATE;
+      }
+      await new Promise(r => setTimeout(r, POLL_INTERVAL_MS));
+      continue;
+    }
+    lastBranch = branch;
     if (branch.branch_state === 'ready') return branch;
     if (branch.branch_state === 'deleted' || branch.branch_state === 'conflicted') {
       throw new CLIError(`Branch creation failed (state: ${branch.branch_state})`);
@@ -286,8 +339,25 @@ async function pollUntilReady(
     await new Promise(r => setTimeout(r, POLL_INTERVAL_MS));
   }
   // Timed out — re-check terminal failure states so a state flip just before
-  // the deadline is not silently reported as “still in state …”.
-  const branch = await getBranchApi(branchId, apiUrl);
+  // the deadline is not silently reported as “still in state …”. This read
+  // decides the command's verdict, so it gets its own retries: a branch that
+  // reached 'ready' right at the deadline would otherwise be reported as stuck
+  // — a genuine success inverted by one unlucky 502.
+  //
+  // Only if all of those fail does it fall back to the last observed state,
+  // rather than turning a timeout into an API error about a branch that
+  // exists: the caller needs the id and appkey printed to find and delete it.
+  // (`branch reset` has no identity to emit and exits 0 on a non-ready state,
+  // so it refuses to guess there instead.)
+  const branch = await readBranchWithRetry(
+    branchId,
+    apiUrl,
+    FINAL_READ_ATTEMPTS,
+    POLL_INTERVAL_MS,
+  ).catch((err: unknown) => {
+    if (!isTransientApiError(err) || !lastBranch) throw err;
+    return lastBranch;
+  });
   if (branch.branch_state === 'deleted' || branch.branch_state === 'conflicted') {
     throw new CLIError(`Branch creation failed (state: ${branch.branch_state})`);
   }

src/commands/branch/poll.ts

@@ -0,0 +1,36 @@
+import { getBranchApi } from '../../lib/api/platform.js';
+import { isTransientApiError } from '../../lib/errors.js';
+import type { Branch } from '../../types.js';
+
+/**
+ * Read a branch, retrying a bounded number of times on transient failures.
+ *
+ * Used for the read that decides a poll's FINAL verdict, where a single
+ * unlucky 502 is expensive: `branch create` would report a branch that just
+ * reached 'ready' as stuck in its last-seen state (and exit non-zero on a
+ * genuine success), and `branch reset` would have to give up on confirming an
+ * outcome it very nearly had. Inside the poll loop itself this is unnecessary —
+ * the loop's own interval already is the retry.
+ *
+ * A non-transient error is rethrown on the first attempt: it will answer the
+ * same way every time. After the last attempt the final transient error is
+ * rethrown for the caller to interpret.
+ */
+export async function readBranchWithRetry(
+  branchId: string,
+  apiUrl: string | undefined,
+  attempts: number,
+  delayMs: number,
+): Promise<Branch> {
+  let lastErr: unknown;
+  for (let attempt = 1; attempt <= attempts; attempt++) {
+    try {
+      return await getBranchApi(branchId, apiUrl);
+    } catch (err) {
+      if (!isTransientApiError(err)) throw err;
+      lastErr = err;
+      if (attempt < attempts) await new Promise(r => setTimeout(r, delayMs));
+    }
+  }
+  throw lastErr;
+}