From 5f965bb7e776b282b4609bf42ca86a3797786e85 Mon Sep 17 00:00:00 2001 From: Lyu Date: Mon, 17 Aug 2026 15:51:20 -0700 Subject: [PATCH 1/3] fix(branch): survive a transient control-plane 502 while polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `insforge branch create` polls GET /projects/v1/branches/:id every 3s for up to 15 minutes, and a SINGLE 502 anywhere in that window ended the command on the spot — while the backend went on to mark the branch ready seconds later. The branch exists and bills; the CLI exited non-zero with no id in the output. Seen twice in agent-e2e (runs 31832239687 and 32055449431: CLI out at ~90s / ~97s, branch ready at ~108s / ~113s; a successful run took ~113s, so the duration itself is normal). A failed READ is not a failed branch: - add `isTransientApiError` (lib/errors.ts) — gateway 5xx, 408/429 and tagged network failures are transient; every other 4xx and any locally raised CLIError (a terminal branch state) stays terminal. deployments already carried an inline copy of this rule; it now shares the helper. - `platformFetch` attaches the HTTP status to the CLIError it throws, so a poller can tell a 502 from a 404 at all. CLIError.statusCode already existed and was already read by command telemetry — only ossFetch had been populating it. - both branch pollers (create, reset) wrap only the fetch: a transient failure costs one poll interval and nothing more. The post-timeout re-check falls back to the last observed state instead of turning a timeout into an API error about a branch that exists. Also close the same hole on the create POST, where it is worse — a 502 there orphans a billing branch with no id anywhere in the output. A gateway 5xx is the proxy reporting that IT could not complete the round trip, so it joins transport resets as an ambiguous failure eligible for the existing adopt path. The guards are unchanged (name + mode + created_at >= request time) and an unmatched search still rethrows the original error, so this cannot mask a request the backend never acted on. Bump 0.2.7 -> 0.2.8. Co-Authored-By: Claude Opus 5 --- package-lock.json | 4 +- package.json | 2 +- src/commands/branch/create.test.ts | 112 +++++++++++++++++++++++++++++ src/commands/branch/create.ts | 66 +++++++++++++---- src/commands/branch/reset.test.ts | 32 +++++++++ src/commands/branch/reset.ts | 35 +++++++-- src/commands/deployments/deploy.ts | 14 +--- src/lib/api/platform.ts | 20 ++++-- src/lib/errors.test.ts | 45 +++++++++++- src/lib/errors.ts | 34 +++++++++ 10 files changed, 327 insertions(+), 37 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3773c891..506f9dbd 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@insforge/cli", - "version": "0.2.7", + "version": "0.2.8", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@insforge/cli", - "version": "0.2.7", + "version": "0.2.8", "license": "Apache-2.0", "dependencies": { "@clack/prompts": "^0.9.1", diff --git a/package.json b/package.json index 7f7bca6c..0832c64f 100644 --- a/package.json +++ b/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": { diff --git a/src/commands/branch/create.test.ts b/src/commands/branch/create.test.ts index 3d67e211..deed09a3 100644 --- a/src/commands/branch/create.test.ts +++ b/src/commands/branch/create.test.ts @@ -394,6 +394,118 @@ 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 ').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 ').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 ').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('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. diff --git a/src/commands/branch/create.ts b/src/commands/branch/create.ts index 73f0940e..c2a30749 100644 --- a/src/commands/branch/create.ts +++ b/src/commands/branch/create.ts @@ -7,7 +7,7 @@ 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'; @@ -31,6 +31,10 @@ 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__'; export function registerBranchCreateCommand(branch: Command): void { branch @@ -181,8 +185,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 +195,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 a gateway + * 5xx, which is the proxy reporting that IT could not complete the round + * trip and says nothing about what the backend did. Every API-level + * rejection (duplicate name, quota, auth, any other 4xx) 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 +217,14 @@ 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 widening guard 1 to 5xx cannot mask a request the backend never acted on. */ -function isTransportFailure(err: unknown): boolean { - return err instanceof CLIError && err.code === NETWORK_ERROR_CODE; +function isAmbiguousCreateFailure(err: unknown): boolean { + if (!(err instanceof CLIError)) return false; + if (err.code === NETWORK_ERROR_CODE) return true; + return err.statusCode !== undefined && err.statusCode >= 500; } async function createBranchOrAdopt( @@ -224,7 +236,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,6 +278,16 @@ 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, @@ -273,8 +295,23 @@ async function pollUntilReady( ): Promise { 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 +323,13 @@ 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 …”. If even this + // read fails transiently, report the last state we did observe rather than + // turning a timeout into an API error about a branch that exists. + const branch = await getBranchApi(branchId, apiUrl).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})`); } diff --git a/src/commands/branch/reset.test.ts b/src/commands/branch/reset.test.ts index fbf5e0e4..2375ab45 100644 --- a/src/commands/branch/reset.test.ts +++ b/src/commands/branch/reset.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { Command } from 'commander'; import { registerBranchResetCommand } from './reset.js'; +import { CLIError } from '../../lib/errors.js'; vi.mock('../../lib/api/platform.js', () => ({ listBranchesApi: vi.fn(async () => [ @@ -146,6 +147,37 @@ describe('branch reset', () => { expect(resetBranchApi).not.toHaveBeenCalled(); }); + it('survives a transient 502 mid-poll instead of reporting a reset that is still running as failed', async () => { + // Same failure the create poll had: a gateway 502 on the status read says + // nothing about the reset job, which keeps running server-side. + const platformModule = await import('../../lib/api/platform.js'); + const getBranchApi = platformModule.getBranchApi as ReturnType; + getBranchApi.mockRejectedValueOnce(new CLIError('Request failed: 502', 1, undefined, 502)); + const program = makeProgram(); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.useFakeTimers(); + try { + const run = program.parseAsync(['reset', 'feat-x', '--yes', '--json'], { from: 'user' }); + await vi.runAllTimersAsync(); + await run; + } finally { + vi.useRealTimers(); + logSpy.mockRestore(); + } + expect(getBranchApi.mock.calls.length).toBeGreaterThan(1); + }); + + it('gives up on a real rejection mid-poll (404 is not transient)', async () => { + const platformModule = await import('../../lib/api/platform.js'); + const getBranchApi = platformModule.getBranchApi as ReturnType; + getBranchApi.mockRejectedValueOnce(new CLIError('Branch not found', 1, undefined, 404)); + const program = makeProgram(); + await expect( + program.parseAsync(['reset', 'feat-x', '--yes', '--json'], { from: 'user' }), + ).rejects.toThrow(); + expect(getBranchApi).toHaveBeenCalledTimes(1); + }); + it('throws when polling sees a terminal failure state (deleted)', async () => { const platformModule = await import('../../lib/api/platform.js'); (platformModule.getBranchApi as ReturnType).mockResolvedValueOnce({ diff --git a/src/commands/branch/reset.ts b/src/commands/branch/reset.ts index 12718d7e..836bbd2b 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -1,7 +1,7 @@ import type { Command } from 'commander'; import * as clack from '@clack/prompts'; import { listBranchesApi, resetBranchApi, getBranchApi } from '../../lib/api/platform.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 { getProjectConfig } from '../../lib/config.js'; import { outputJson, outputSuccess, outputInfo } from '../../lib/output.js'; @@ -83,6 +83,14 @@ export function registerBranchResetCommand(branch: Command): void { }); } +/** + * Poll the control plane until the reset lands. + * + * Same shape — and the same transient-failure tolerance — as the create poll: + * a 502 from the gateway is a failed READ, not a failed reset, and giving up on + * it leaves the caller believing a reset that is still running has failed. See + * the note on `pollUntilReady` in create.ts. + */ async function pollUntilReady( branchId: string, apiUrl: string | undefined, @@ -91,9 +99,24 @@ async function pollUntilReady( ): Promise { const start = Date.now(); let lastState = startingState; + let lastBranch: Branch | null = null; + let announcedUnreachable = false; if (showProgress) outputInfo(` state: ${startingState}…`); 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 (showProgress && !announcedUnreachable) { + outputInfo(' control plane not answering; reset still running, retrying…'); + announcedUnreachable = true; + } + await new Promise(r => setTimeout(r, POLL_INTERVAL_MS)); + continue; + } + lastBranch = branch; + announcedUnreachable = false; // Reset always lands at ready (even when entry was merged) — see // backend BranchQueue.processResetFinalize. A bounce back to ready // OR merged is the rollback path; treat both as terminal so the user @@ -110,8 +133,12 @@ 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 …”. A transient + // failure on that last read falls back to the last state we observed. + const branch = await getBranchApi(branchId, apiUrl).catch((err: unknown) => { + if (!isTransientApiError(err) || !lastBranch) throw err; + return lastBranch; + }); if (branch.branch_state === 'deleted' || branch.branch_state === 'conflicted') { throw new CLIError(`Branch reset failed (state: ${branch.branch_state})`); } diff --git a/src/commands/deployments/deploy.ts b/src/commands/deployments/deploy.ts index 4d0758e7..0a914376 100644 --- a/src/commands/deployments/deploy.ts +++ b/src/commands/deployments/deploy.ts @@ -8,7 +8,7 @@ import archiver from 'archiver'; import { ossFetch } from '../../lib/api/oss.js'; import { getProjectConfig } from '../../lib/config.js'; import { requireAuth } from '../../lib/credentials.js'; -import { handleError, getRootOpts, CLIError, ProjectNotLinkedError, getDeploymentError, formatFetchError } from '../../lib/errors.js'; +import { handleError, getRootOpts, CLIError, ProjectNotLinkedError, getDeploymentError, formatFetchError, isTransientApiError } from '../../lib/errors.js'; import { outputJson } from '../../lib/output.js'; import type { CreateDeploymentResponse, @@ -27,12 +27,6 @@ export const POLL_INTERVAL_MS = 5_000; export const POLL_TIMEOUT_MS = 300_000; const DIRECT_UPLOAD_CONCURRENCY = 8; -// 4xx statuses that are retryable while a deployment is in flight. A rate limit -// or request timeout on the status endpoint says nothing about the deployment, -// which keeps running server-side — and polling every 5s is exactly the shape -// that trips a rate limit. Every other 4xx stays terminal. -const TRANSIENT_4XX_STATUSES = new Set([408, 429]); - const EXCLUDE_PATTERNS = [ 'node_modules', '.git', @@ -324,11 +318,7 @@ export async function pollDeployment( // responses other than the retryable ones are terminal. Gateway 5xx // responses on the status endpoint are transient — the deployment itself // may still succeed — so keep polling, same as network-level fetch errors. - const isTerminal = - err instanceof CLIError && - (err.statusCode === undefined || - (err.statusCode < 500 && !TRANSIENT_4XX_STATUSES.has(err.statusCode))); - if (isTerminal) { + if (!isTransientApiError(err)) { throw err; } // Transient: keep polling, but remember why the read failed so that an diff --git a/src/lib/api/platform.ts b/src/lib/api/platform.ts index 56ffe9a9..61b7e6e8 100644 --- a/src/lib/api/platform.ts +++ b/src/lib/api/platform.ts @@ -1,6 +1,6 @@ import { getAccessToken, getCredentials, getPlatformApiUrl } from '../config.js'; import { refreshAccessToken } from '../credentials.js'; -import { AuthError, CLIError, formatFetchError } from '../errors.js'; +import { AuthError, CLIError, NETWORK_ERROR_CODE, formatFetchError } from '../errors.js'; import type { ApiKeyResponse, Backup, @@ -31,8 +31,10 @@ import type { } from '../../types.js'; // Marks a CLIError that came from a failed fetch rather than an HTTP response: -// the request may still have been received and acted on by the server. -export const NETWORK_ERROR_CODE = 'NETWORK_ERROR'; +// the request may still have been received and acted on by the server. Defined +// in lib/errors.ts (so `isTransientApiError` can reference it without importing +// this module) and re-exported here, where callers already import it from. +export { NETWORK_ERROR_CODE } from '../errors.js'; export interface PlatformFetchOptions extends RequestInit { /** @@ -119,7 +121,12 @@ export async function platformFetch( } if (!retryRes.ok) { const err = await retryRes.json().catch(() => ({})) as { error?: string }; - throw new CLIError(err.error ?? `Request failed: ${retryRes.status}`, retryRes.status === 403 ? 5 : 1); + throw new CLIError( + err.error ?? `Request failed: ${retryRes.status}`, + retryRes.status === 403 ? 5 : 1, + undefined, + retryRes.status, + ); } return retryRes; } @@ -131,7 +138,10 @@ export async function platformFetch( if (!res.ok) { const err = await res.json().catch(() => ({})) as { error?: string; message?: string }; const msg = err.message ? `${err.error ?? res.status}: ${err.message}` : (err.error ?? `Request failed: ${res.status}`); - throw new CLIError(msg, res.status === 403 ? 5 : 1); + // Carry the HTTP status: callers that poll need it to tell a transient + // gateway 5xx from a real rejection (see `isTransientApiError`), and + // command telemetry already reports it. + throw new CLIError(msg, res.status === 403 ? 5 : 1, undefined, res.status); } return res; diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 4c92cc0b..fb7c743d 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { formatFetchError } from './errors.js'; +import { + AuthError, + CLIError, + NETWORK_ERROR_CODE, + formatFetchError, + isTransientApiError, +} from './errors.js'; function fetchError(causeCode?: string, causeMessage?: string): Error { const err = new Error('fetch failed'); @@ -77,3 +83,40 @@ describe('formatFetchError', () => { expect(msg).toContain('broken-host'); }); }); + +describe('isTransientApiError', () => { + it('treats gateway 5xx as transient — the poll must survive one 502', () => { + for (const status of [500, 502, 503, 504]) { + expect(isTransientApiError(new CLIError('Request failed', 1, undefined, status))).toBe(true); + } + }); + + it('treats a tagged network failure as transient', () => { + expect( + isTransientApiError(new CLIError('Connection reset', 1, NETWORK_ERROR_CODE)), + ).toBe(true); + }); + + it('treats rate limiting and request timeout as transient', () => { + expect(isTransientApiError(new CLIError('Too many requests', 1, undefined, 429))).toBe(true); + expect(isTransientApiError(new CLIError('Request timeout', 1, undefined, 408))).toBe(true); + }); + + it('treats real API rejections as terminal', () => { + for (const status of [400, 401, 403, 404, 409, 422]) { + expect(isTransientApiError(new CLIError('Nope', 1, undefined, status))).toBe(false); + } + expect(isTransientApiError(new AuthError())).toBe(false); + }); + + it('treats a locally raised CLIError (no status) as terminal', () => { + // e.g. "Branch creation failed (state: deleted)" — retrying only burns the + // poll budget on an answer that will not change. + expect(isTransientApiError(new CLIError('Branch creation failed (state: deleted)'))).toBe(false); + }); + + it('treats a raw non-CLIError throw as transient', () => { + // ossFetch does not wrap fetch rejections, so these arrive as plain errors. + expect(isTransientApiError(new TypeError('fetch failed'))).toBe(true); + }); +}); diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 64e89726..fea29ff5 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -1,5 +1,16 @@ import type { Command } from 'commander'; +// Marks a CLIError that came from a failed fetch rather than an HTTP response: +// the request may still have been received and acted on by the server. +// Re-exported from `lib/api/platform.js`, which is where most callers import it. +export const NETWORK_ERROR_CODE = 'NETWORK_ERROR'; + +// 4xx statuses that say nothing about the operation a caller is polling for. A +// rate limit or a request timeout on a status endpoint is about THIS read, not +// about the job running server-side — and polling every few seconds is exactly +// the shape that trips a rate limit. Every other 4xx stays terminal. +const TRANSIENT_4XX_STATUSES = new Set([408, 429]); + export class CLIError extends Error { constructor( message: string, @@ -36,6 +47,29 @@ export class PermissionError extends CLIError { } } +/** + * True when an API failure says nothing about the operation being polled, so a + * caller in a poll loop should read again rather than give up. + * + * Gateway 5xx is the case this exists for: `insforge branch create` polls the + * control plane every 3s for up to 15 minutes, and a SINGLE 502 anywhere in + * that window used to abort a branch that the backend went on to finish + * creating seconds later (observed twice in agent-e2e: CLI out at ~90s and + * ~97s, branch ready at ~108s and ~113s). The branch still exists and still + * bills, so exiting is both wrong and expensive. + * + * Terminal by design: any CLIError with no `statusCode` — that is a locally + * raised error (a failed job, a bad state) rather than a transport hiccup, and + * retrying it just burns the budget. Non-CLIError throws (a raw fetch + * rejection, e.g. from `ossFetch`, which does not wrap them) are transient. + */ +export function isTransientApiError(err: unknown): boolean { + if (!(err instanceof CLIError)) return true; + if (err.code === NETWORK_ERROR_CODE) return true; + if (err.statusCode === undefined) return false; + return err.statusCode >= 500 || TRANSIENT_4XX_STATUSES.has(err.statusCode); +} + /** * Format a Node fetch error with actionable context. * From 85e31c7ccba335f68cbc9aad68af38c29e2a5fe8 Mon Sep 17 00:00:00 2001 From: Lyu Date: Mon, 17 Aug 2026 16:02:27 -0700 Subject: [PATCH 2/3] fix(branch): tighten transient classification after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, two addressed in code: 1. Greptile P1 — uncorrelated branch adoption. Accepting ANY 5xx on the create POST widened the window in which a collaborator's same-name, same-mode branch inside the skew window could be adopted, and with the default --switch that writes their identity into local config. Narrow adoption to the PROXY statuses (502/503/504) plus transport resets. A plain 500 is the application's own answer — more likely authoritative than lost — and 408/429 mean the POST was refused, so nothing was created to adopt. Re-reading a status after a 500 is free; adopting after one is not, so the poll predicate stays wider than this one. 2. Greptile P1 — stale reset state returns success. `branch reset` exits 0 on any non-ready state, so substituting the last polled state after an unreadable final check could report a branch that went deleted/conflicted as "still resetting" and exit successfully. It now raises a "could not confirm" error naming the last observed state. create keeps its fallback: it must still print the branch id/appkey so an orphan can be found, and every non-'ready' state there exits non-zero, so a stale label cannot manufacture success. 3. CodeRabbit — unclassified throws were transient. Valid in the branch pollers, which had zero tolerance before: a res.json() parse failure or a plain bug would now be retried for the full 15-minute budget. `isTransientApiError` no longer classifies non-CLIError throws as transient. Their proposed remedy (wrap ossFetch rejections) is not taken — it would flip deployment polling's raw fetch rejections to terminal, reintroducing this very bug class there — so deploy keeps its own explicit tolerance for unwrapped throws at the call site. Co-Authored-By: Claude Opus 5 --- src/commands/branch/create.test.ts | 27 ++++++++++++++++++++++ src/commands/branch/create.ts | 33 +++++++++++++++++++------- src/commands/branch/reset.test.ts | 37 ++++++++++++++++++++++++++++++ src/commands/branch/reset.ts | 21 +++++++++++++---- src/commands/deployments/deploy.ts | 6 ++++- src/lib/errors.test.ts | 10 +++++--- src/lib/errors.ts | 20 ++++++++++------ 7 files changed, 131 insertions(+), 23 deletions(-) diff --git a/src/commands/branch/create.test.ts b/src/commands/branch/create.test.ts index deed09a3..26edee1d 100644 --- a/src/commands/branch/create.test.ts +++ b/src/commands/branch/create.test.ts @@ -506,6 +506,33 @@ describe('branch create', () => { 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 ').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. diff --git a/src/commands/branch/create.ts b/src/commands/branch/create.ts index c2a30749..0a95bed4 100644 --- a/src/commands/branch/create.ts +++ b/src/commands/branch/create.ts @@ -199,11 +199,11 @@ export function registerBranchCreateCommand(branch: Command): void { * 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 an ambiguous failure is eligible — a transport reset, or a gateway - * 5xx, which is the proxy reporting that IT could not complete the round - * trip and says nothing about what the backend did. Every API-level - * rejection (duplicate name, quota, auth, any other 4xx) 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. @@ -219,12 +219,24 @@ export function registerBranchCreateCommand(branch: Command): void { * Reported upstream: InsForge/InsForge#1790. * * When no matching branch turns up, the original error is rethrown unchanged — - * so widening guard 1 to 5xx cannot mask a request the backend never acted on. + * 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. */ +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 && err.statusCode >= 500; + return err.statusCode !== undefined && PROXY_STATUSES.has(err.statusCode); } async function createBranchOrAdopt( @@ -325,7 +337,12 @@ async function pollUntilReady( // Timed out — re-check terminal failure states so a state flip just before // the deadline is not silently reported as “still in state …”. If even this // read fails transiently, report the last state we did observe rather than - // turning a timeout into an API error about a branch that exists. + // 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. The reported state + // may be stale, but it cannot manufacture success — every non-'ready' state + // exits non-zero, so the worst case is a stale label on a failure that was + // already going to be a failure. (`branch reset` has no identity to emit and + // exits 0 on a non-ready state, so it fails loudly there instead.) const branch = await getBranchApi(branchId, apiUrl).catch((err: unknown) => { if (!isTransientApiError(err) || !lastBranch) throw err; return lastBranch; diff --git a/src/commands/branch/reset.test.ts b/src/commands/branch/reset.test.ts index 2375ab45..1454475b 100644 --- a/src/commands/branch/reset.test.ts +++ b/src/commands/branch/reset.test.ts @@ -167,6 +167,43 @@ describe('branch reset', () => { expect(getBranchApi.mock.calls.length).toBeGreaterThan(1); }); + it('fails loudly when the final state could never be confirmed, instead of reporting a stale one', async () => { + // This command exits 0 on any non-ready state, so substituting the last + // polled state after an unreadable final check would let a reset that went + // 'deleted'/'conflicted' exit successfully. + const platformModule = await import('../../lib/api/platform.js'); + const getBranchApi = platformModule.getBranchApi as ReturnType; + getBranchApi.mockRejectedValue(new CLIError('Request failed: 502', 1, undefined, 502)); + const program = makeProgram(); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.useFakeTimers(); + let reads: number; + try { + // Attach the rejection handler BEFORE advancing timers, or the failure + // surfaces as an unhandled rejection while the fake clock runs. + const run = expect( + program.parseAsync(['reset', 'feat-x', '--yes', '--json'], { from: 'user' }), + ).rejects.toThrow(); + await vi.runAllTimersAsync(); + await run; + reads = getBranchApi.mock.calls.length; + } finally { + vi.useRealTimers(); + logSpy.mockRestore(); + // mockRejectedValue survives clearAllMocks — restore the shared impl or + // every later test polls a 502 for the full budget. + getBranchApi.mockReset(); + getBranchApi.mockResolvedValue({ + id: 'b1', name: 'feat-x', branch_state: 'ready', + organization_id: 'o1', parent_project_id: 'p1', appkey: 'k', region: 'us-east', + branch_created_at: '2026', + }); + } + // It kept polling for the whole budget rather than aborting on the first + // 502, and then refused to guess the outcome. + expect(reads).toBeGreaterThan(10); + }); + it('gives up on a real rejection mid-poll (404 is not transient)', async () => { const platformModule = await import('../../lib/api/platform.js'); const getBranchApi = platformModule.getBranchApi as ReturnType; diff --git a/src/commands/branch/reset.ts b/src/commands/branch/reset.ts index 836bbd2b..f0d044ff 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -133,11 +133,24 @@ 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 …”. A transient - // failure on that last read falls back to the last state we observed. + // the deadline is not silently reported as “still in state …”. + // + // If that last read fails too, the final state is UNKNOWN and the command + // must say so. Substituting the last polled state here would let a branch + // that went 'deleted'/'conflicted' after the last successful read be + // reported as "still resetting" — and this command exits 0 on a non-ready + // state, so a failed reset would exit successfully. Unlike create, there is + // no identity to emit that the caller does not already have (they named the + // branch), so failing loudly costs nothing. const branch = await getBranchApi(branchId, apiUrl).catch((err: unknown) => { - if (!isTransientApiError(err) || !lastBranch) throw err; - return lastBranch; + if (!isTransientApiError(err)) throw err; + throw new CLIError( + `Could not confirm the reset of branch ${branchId}: the control plane is not answering (${ + err instanceof CLIError ? err.message : String(err) + }).` + + (lastBranch ? ` Last observed state: '${lastBranch.branch_state}'.` : '') + + ' Run `insforge branch list` to check.', + ); }); if (branch.branch_state === 'deleted' || branch.branch_state === 'conflicted') { throw new CLIError(`Branch reset failed (state: ${branch.branch_state})`); diff --git a/src/commands/deployments/deploy.ts b/src/commands/deployments/deploy.ts index 0a914376..367e0d35 100644 --- a/src/commands/deployments/deploy.ts +++ b/src/commands/deployments/deploy.ts @@ -318,7 +318,11 @@ export async function pollDeployment( // responses other than the retryable ones are terminal. Gateway 5xx // responses on the status endpoint are transient — the deployment itself // may still succeed — so keep polling, same as network-level fetch errors. - if (!isTransientApiError(err)) { + // `ossFetch` does not wrap raw fetch rejections into CLIError the way + // `platformFetch` does, so a non-CLIError throw here IS the network-level + // case and stays retryable — that is why this is not a bare + // `!isTransientApiError(err)`. + if (err instanceof CLIError && !isTransientApiError(err)) { throw err; } // Transient: keep polling, but remember why the read failed so that an diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index fb7c743d..f0b50c0d 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -115,8 +115,12 @@ describe('isTransientApiError', () => { expect(isTransientApiError(new CLIError('Branch creation failed (state: deleted)'))).toBe(false); }); - it('treats a raw non-CLIError throw as transient', () => { - // ossFetch does not wrap fetch rejections, so these arrive as plain errors. - expect(isTransientApiError(new TypeError('fetch failed'))).toBe(true); + it('treats an unclassified throw as terminal', () => { + // A res.json() parse failure or a plain bug must not spin a poll loop for + // its whole budget. platformFetch wraps every transport/HTTP failure into a + // CLIError, so nothing real is misclassified here; ossFetch callers that + // need raw fetch rejections retried handle that themselves. + expect(isTransientApiError(new TypeError('fetch failed'))).toBe(false); + expect(isTransientApiError(new SyntaxError('Unexpected end of JSON input'))).toBe(false); }); }); diff --git a/src/lib/errors.ts b/src/lib/errors.ts index fea29ff5..0c95e87b 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -48,8 +48,8 @@ export class PermissionError extends CLIError { } /** - * True when an API failure says nothing about the operation being polled, so a - * caller in a poll loop should read again rather than give up. + * True when a CLASSIFIED API failure says nothing about the operation being + * polled, so a caller in a poll loop should read again rather than give up. * * Gateway 5xx is the case this exists for: `insforge branch create` polls the * control plane every 3s for up to 15 minutes, and a SINGLE 502 anywhere in @@ -58,13 +58,19 @@ export class PermissionError extends CLIError { * ~97s, branch ready at ~108s and ~113s). The branch still exists and still * bills, so exiting is both wrong and expensive. * - * Terminal by design: any CLIError with no `statusCode` — that is a locally - * raised error (a failed job, a bad state) rather than a transport hiccup, and - * retrying it just burns the budget. Non-CLIError throws (a raw fetch - * rejection, e.g. from `ossFetch`, which does not wrap them) are transient. + * Terminal by design: + * - any CLIError with no `statusCode` — a locally raised error (a failed job, + * a bad state) rather than a transport hiccup, so retrying only burns the + * budget; + * - anything that is not a CLIError at all. That is an UNCLASSIFIED throw — + * a `res.json()` parse failure on a mangled body, or a plain bug — and a + * poll loop must not spin on it for its whole budget. `platformFetch` + * wraps every transport and HTTP failure into a CLIError, so nothing real + * is lost. `ossFetch` does NOT wrap raw fetch rejections, so its callers + * handle that case themselves (see the deployment poller). */ export function isTransientApiError(err: unknown): boolean { - if (!(err instanceof CLIError)) return true; + if (!(err instanceof CLIError)) return false; if (err.code === NETWORK_ERROR_CODE) return true; if (err.statusCode === undefined) return false; return err.statusCode >= 500 || TRANSIENT_4XX_STATUSES.has(err.statusCode); From fbffb747845227154ea2b6b058dded41e5cc68e4 Mon Sep 17 00:00:00 2001 From: Lyu Date: Mon, 17 Aug 2026 16:28:19 -0700 Subject: [PATCH 3/3] fix(api): tag ossFetch transport failures; retry the verdict read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round — cubic P2/P2/P3 plus jwfing's suggestion. 1. cubic P2 (deploy.ts) — unclassified throws were retried for the whole poll window. The real cause is that `ossFetch` was the only client that let Node's bare "fetch failed" through, so its callers had to treat EVERY unclassified throw as possibly-the-network. It now tags transport failures as CLIError(NETWORK_ERROR_CODE) with a formatted message, exactly like `platformFetch` and `submitFeedback` already do. The deployment poller drops its special case as a result: a malformed status body now fails on the first read instead of being re-read for five minutes and then throwing a contextless TypeError at the timeout. Users also stop seeing "fetch failed" for DNS/TLS/proxy problems on every self-hosted command. 2. cubic P2 (reset.test.ts) — correct: the stale-state test rejected every read, so `lastBranch` stayed null and the substitution path it was named for was never entered; it passed under both the correct and the buggy implementation. It now polls 'resetting' successfully three times before the reads start failing, so there IS a stale state to substitute and the test fails if one is. 3. jwfing suggestion (create.ts) — the post-timeout read decides the verdict, and a branch that reached 'ready' right at the deadline was reported as stuck if that single read 502'd, inverting a genuine success. Both pollers now retry it (3 attempts, one poll interval apart) via a shared `readBranchWithRetry`, and the create comment no longer claims a stale label can only mislabel an already-failing run. 4. cubic P3 (reset.ts) — the "could not confirm" diagnostic printed the internal branch id; it now names the branch the user asked for. Co-Authored-By: Claude Opus 5 --- src/commands/branch/create.ts | 29 +++++++--- src/commands/branch/poll.ts | 36 ++++++++++++ src/commands/branch/reset.test.ts | 59 +++++++++++++++++++- src/commands/branch/reset.ts | 34 +++++++---- src/commands/deployments/deploy-poll.test.ts | 30 +++++++++- src/commands/deployments/deploy.ts | 12 ++-- src/lib/api/oss.test.ts | 28 ++++++++++ src/lib/api/oss.ts | 16 +++++- 8 files changed, 210 insertions(+), 34 deletions(-) create mode 100644 src/commands/branch/poll.ts diff --git a/src/commands/branch/create.ts b/src/commands/branch/create.ts index 0a95bed4..d5b6be2e 100644 --- a/src/commands/branch/create.ts +++ b/src/commands/branch/create.ts @@ -13,6 +13,7 @@ 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; @@ -35,6 +36,9 @@ const CREATED_AT_SKEW_MS = 60_000; // 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 @@ -335,15 +339,22 @@ 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 …”. If even this - // read fails transiently, report the last state we did observe 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. The reported state - // may be stale, but it cannot manufacture success — every non-'ready' state - // exits non-zero, so the worst case is a stale label on a failure that was - // already going to be a failure. (`branch reset` has no identity to emit and - // exits 0 on a non-ready state, so it fails loudly there instead.) - const branch = await getBranchApi(branchId, apiUrl).catch((err: unknown) => { + // 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; }); diff --git a/src/commands/branch/poll.ts b/src/commands/branch/poll.ts new file mode 100644 index 00000000..688f1bd9 --- /dev/null +++ b/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 { + 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; +} diff --git a/src/commands/branch/reset.test.ts b/src/commands/branch/reset.test.ts index 1454475b..85d70657 100644 --- a/src/commands/branch/reset.test.ts +++ b/src/commands/branch/reset.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; import { Command } from 'commander'; import { registerBranchResetCommand } from './reset.js'; import { CLIError } from '../../lib/errors.js'; @@ -171,9 +171,23 @@ describe('branch reset', () => { // This command exits 0 on any non-ready state, so substituting the last // polled state after an unreadable final check would let a reset that went // 'deleted'/'conflicted' exit successfully. + // + // The successful 'resetting' reads first are what makes this a real test: + // they populate the last-observed state, which is exactly what a stale + // fallback would substitute. Rejecting every read from the start would + // leave nothing to substitute and the test would pass either way. const platformModule = await import('../../lib/api/platform.js'); const getBranchApi = platformModule.getBranchApi as ReturnType; - getBranchApi.mockRejectedValue(new CLIError('Request failed: 502', 1, undefined, 502)); + const resetting = { + id: 'b1', name: 'feat-x', branch_state: 'resetting', + organization_id: 'o1', parent_project_id: 'p1', appkey: 'k', region: 'us-east', + branch_created_at: '2026', + }; + getBranchApi + .mockResolvedValueOnce(resetting) + .mockResolvedValueOnce(resetting) + .mockResolvedValueOnce(resetting) + .mockRejectedValue(new CLIError('Request failed: 502', 1, undefined, 502)); const program = makeProgram(); const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.useFakeTimers(); @@ -200,10 +214,49 @@ describe('branch reset', () => { }); } // It kept polling for the whole budget rather than aborting on the first - // 502, and then refused to guess the outcome. + // 502, and then refused to guess the outcome from the 'resetting' state it + // had in hand. expect(reads).toBeGreaterThan(10); }); + it('retries the verdict read so one 502 at the deadline does not lose a landed reset', async () => { + // The final re-check decides the outcome. A branch that reached 'ready' + // right at the deadline must not be reported as unconfirmable because a + // single read failed. + const platformModule = await import('../../lib/api/platform.js'); + const getBranchApi = platformModule.getBranchApi as Mock; + const ready = { + id: 'b1', name: 'feat-x', branch_state: 'ready', + organization_id: 'o1', parent_project_id: 'p1', appkey: 'k', region: 'us-east', + branch_created_at: '2026', + }; + getBranchApi.mockImplementation(async () => { + // Non-terminal for the whole poll window, then one 502 on the verdict + // read, then 'ready' — the state flipped just as the budget ran out. + const call = getBranchApi.mock.calls.length; + if (call <= 100) return { ...ready, branch_state: 'resetting' }; + if (call === 101) throw new CLIError('Request failed: 502', 1, undefined, 502); + return ready; + }); + const program = makeProgram(); + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.useFakeTimers(); + let printed: string; + try { + const run = program.parseAsync(['reset', 'feat-x', '--yes', '--json'], { from: 'user' }); + await vi.runAllTimersAsync(); + await run; + printed = logSpy.mock.calls.map(args => args.map(String).join(' ')).join('\n'); + } finally { + vi.useRealTimers(); + logSpy.mockRestore(); + getBranchApi.mockReset(); + getBranchApi.mockResolvedValue(ready); + } + // Resolved with the real final state rather than "could not confirm". + expect(printed).toContain('ready'); + }); + it('gives up on a real rejection mid-poll (404 is not transient)', async () => { const platformModule = await import('../../lib/api/platform.js'); const getBranchApi = platformModule.getBranchApi as ReturnType; diff --git a/src/commands/branch/reset.ts b/src/commands/branch/reset.ts index f0d044ff..cd90ec92 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -6,6 +6,7 @@ import { requireAuth } from '../../lib/credentials.js'; import { getProjectConfig } from '../../lib/config.js'; import { outputJson, outputSuccess, outputInfo } from '../../lib/output.js'; import { captureEvent, shutdownAnalytics } from '../../lib/analytics.js'; +import { readBranchWithRetry } from './poll.js'; import type { Branch } from '../../types.js'; const POLL_INTERVAL_MS = 3_000; @@ -13,6 +14,9 @@ const POLL_INTERVAL_MS = 3_000; // finalize. Same order of magnitude as create — minutes for a small DB, // longer for a populated one. Match create's 5-min budget. const POLL_TIMEOUT_MS = 5 * 60 * 1_000; +// Retries for the post-timeout read that decides the verdict. Inside the loop +// the interval is the retry; this one has no second chance. +const FINAL_READ_ATTEMPTS = 3; export function registerBranchResetCommand(branch: Command): void { branch @@ -63,7 +67,7 @@ export function registerBranchResetCommand(branch: Command): void { outputSuccess(`Reset enqueued for branch '${name}'. Restoring T0…`); } - const final = await pollUntilReady(target.id, apiUrl, !json, initial.branch_state); + const final = await pollUntilReady(target.id, name, apiUrl, !json, initial.branch_state); if (json) { outputJson({ branch: final }); @@ -93,6 +97,7 @@ export function registerBranchResetCommand(branch: Command): void { */ async function pollUntilReady( branchId: string, + name: string, apiUrl: string | undefined, showProgress: boolean, startingState: string, @@ -133,19 +138,26 @@ 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 …”. + // the deadline is not silently reported as “still in state …”. Retried, + // because this read decides the verdict and one unlucky 502 should not cost + // a reset that had just landed. // - // If that last read fails too, the final state is UNKNOWN and the command - // must say so. Substituting the last polled state here would let a branch - // that went 'deleted'/'conflicted' after the last successful read be - // reported as "still resetting" — and this command exits 0 on a non-ready - // state, so a failed reset would exit successfully. Unlike create, there is - // no identity to emit that the caller does not already have (they named the - // branch), so failing loudly costs nothing. - const branch = await getBranchApi(branchId, apiUrl).catch((err: unknown) => { + // If every attempt fails, the final state is UNKNOWN and the command must + // say so. Substituting the last polled state here would let a branch that + // went 'deleted'/'conflicted' after the last successful read be reported as + // "still resetting" — and this command exits 0 on a non-ready state, so a + // failed reset would exit successfully. Unlike create, there is no identity + // to emit that the caller does not already have (they named the branch), so + // failing loudly costs nothing. + const branch = await readBranchWithRetry( + branchId, + apiUrl, + FINAL_READ_ATTEMPTS, + POLL_INTERVAL_MS, + ).catch((err: unknown) => { if (!isTransientApiError(err)) throw err; throw new CLIError( - `Could not confirm the reset of branch ${branchId}: the control plane is not answering (${ + `Could not confirm the reset of branch '${name}': the control plane is not answering (${ err instanceof CLIError ? err.message : String(err) }).` + (lastBranch ? ` Last observed state: '${lastBranch.branch_state}'.` : '') + diff --git a/src/commands/deployments/deploy-poll.test.ts b/src/commands/deployments/deploy-poll.test.ts index ad5d8efd..92b98ef5 100644 --- a/src/commands/deployments/deploy-poll.test.ts +++ b/src/commands/deployments/deploy-poll.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { CLIError } from '../../lib/errors.js'; +import { CLIError, NETWORK_ERROR_CODE } from '../../lib/errors.js'; const ossMock = vi.hoisted(() => ({ ossFetch: vi.fn(), @@ -98,14 +98,38 @@ describe('pollDeployment', () => { }); it('reports a network-level failure that persists for the whole window', async () => { - ossMock.ossFetch.mockRejectedValue(new TypeError('fetch failed')); + // The shape a real connection failure now reaches the poller in: ossFetch + // tags transport failures the way platformFetch does, instead of letting + // Node's bare TypeError through. + ossMock.ossFetch.mockRejectedValue( + new CLIError('Connection to app.insforge.app was reset.', 1, NETWORK_ERROR_CODE), + ); const promise = pollDeployment('dep_1', null, false); await vi.advanceTimersByTimeAsync(POLL_TIMEOUT_MS + POLL_INTERVAL_MS); const result = await promise; expect(result.isReady).toBe(false); - expect(result.lastError).toContain('the deployment API'); + expect(result.lastError).toContain('was reset'); + }); + + it('fails fast on a malformed status body instead of retrying it for the whole window', async () => { + // An unparseable body answers the same way on every retry, so spinning on + // it only delays the real error by the length of the poll window. Only + // classified transport/gateway failures are worth re-reading. + ossMock.ossFetch.mockResolvedValue( + new Response('502 Bad Gateway', { + status: 200, + headers: { 'Content-Type': 'text/html' }, + }), + ); + + const promise = pollDeployment('dep_1', null, false); + const assertion = expect(promise).rejects.toThrow(SyntaxError); + await vi.advanceTimersByTimeAsync(POLL_INTERVAL_MS * 2); + await assertion; + // One read, not a window's worth. + expect(ossMock.ossFetch).toHaveBeenCalledTimes(1); }); it('leaves lastError null when the final read succeeded and the build was just slow', async () => { diff --git a/src/commands/deployments/deploy.ts b/src/commands/deployments/deploy.ts index 367e0d35..60a10ad9 100644 --- a/src/commands/deployments/deploy.ts +++ b/src/commands/deployments/deploy.ts @@ -317,12 +317,12 @@ export async function pollDeployment( // Deployment-failure errors (thrown above, no statusCode) and 4xx // responses other than the retryable ones are terminal. Gateway 5xx // responses on the status endpoint are transient — the deployment itself - // may still succeed — so keep polling, same as network-level fetch errors. - // `ossFetch` does not wrap raw fetch rejections into CLIError the way - // `platformFetch` does, so a non-CLIError throw here IS the network-level - // case and stays retryable — that is why this is not a bare - // `!isTransientApiError(err)`. - if (err instanceof CLIError && !isTransientApiError(err)) { + // may still succeed — so keep polling, same as network-level fetch + // failures, which `ossFetch` now tags as such. Anything left unclassified + // (a malformed status body failing `.json()`, a missing `status` field) + // is a bug, not an outage: it would fail identically on every retry, so + // it surfaces now instead of at the end of the poll window. + if (!isTransientApiError(err)) { throw err; } // Transient: keep polling, but remember why the read failed so that an diff --git a/src/lib/api/oss.test.ts b/src/lib/api/oss.test.ts index 9563136b..5de8b909 100644 --- a/src/lib/api/oss.test.ts +++ b/src/lib/api/oss.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import * as config from '../config.js'; +import { CLIError, NETWORK_ERROR_CODE } from '../errors.js'; import { isMaskedDatabasePassword, ossFetch, spliceDatabasePassword } from './oss.js'; import type { ProjectConfig } from '../../types.js'; @@ -101,4 +102,31 @@ describe('ossFetch', () => { /Upgrade your InsForge instance.*insforge webscraper apify connect --token/s, ); }); + + it('tags a transport failure as a network CLIError with an actionable message', async () => { + // Node's fetch throws a bare "fetch failed" for every transport problem. + // Callers need to tell that apart from a response they could not parse — + // a poll loop retries the first and must not retry the second — and users + // need to know it was DNS rather than the server. + vi.spyOn(config, 'getProjectConfig').mockReturnValue({ + project_id: 'p1', + project_name: 'demo', + org_id: 'o1', + appkey: 'app', + region: 'us-east', + api_key: 'ik_test', + oss_host: 'https://app.us-east.insforge.app', + } satisfies ProjectConfig); + const failure = new Error('fetch failed'); + (failure as { cause?: unknown }).cause = Object.assign(new Error('getaddrinfo ENOTFOUND'), { + code: 'ENOTFOUND', + }); + vi.spyOn(globalThis, 'fetch').mockRejectedValueOnce(failure); + + const err = await ossFetch('/api/metadata').catch((e: unknown) => e); + expect(err).toBeInstanceOf(CLIError); + expect((err as CLIError).code).toBe(NETWORK_ERROR_CODE); + expect((err as CLIError).message).toContain('app.us-east.insforge.app'); + expect((err as CLIError).message).toContain('DNS'); + }); }); diff --git a/src/lib/api/oss.ts b/src/lib/api/oss.ts index 9d8cc02d..83264258 100644 --- a/src/lib/api/oss.ts +++ b/src/lib/api/oss.ts @@ -1,5 +1,5 @@ import { getProjectConfig } from '../config.js'; -import { CLIError, formatFetchError, ProjectNotLinkedError } from '../errors.js'; +import { CLIError, NETWORK_ERROR_CODE, formatFetchError, ProjectNotLinkedError } from '../errors.js'; import type { AdvisorSuppression, AdvisorSuppressionReason, @@ -243,7 +243,19 @@ export async function ossFetch( ...(options.headers as Record ?? {}), }; - const res = await fetch(`${config.oss_host}${path}`, { ...options, headers }); + const url = `${config.oss_host}${path}`; + let res: Response; + try { + res = await fetch(url, { ...options, headers }); + } catch (err) { + // Tag transport failures the way `platformFetch` does. Two reasons: + // callers get an actionable message ("Cannot resolve host…") instead of + // Node's bare "fetch failed", and a poll loop can tell a lost connection + // apart from a response it could not parse — otherwise every unclassified + // throw, including a JSON parse error, has to be retried on the chance + // that it was the network. + throw new CLIError(formatFetchError(err, url), 1, NETWORK_ERROR_CODE); + } if (!res.ok) { const err = await res.json().catch(() => ({})) as {