diff --git a/package-lock.json b/package-lock.json index 3773c89..506f9db 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 7f7bca6..0832c64 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 3d67e21..26edee1 100644 --- a/src/commands/branch/create.test.ts +++ b/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 ').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('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 73f0940..d5b6be2 100644 --- a/src/commands/branch/create.ts +++ b/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,6 +294,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 +311,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 +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})`); } diff --git a/src/commands/branch/poll.ts b/src/commands/branch/poll.ts new file mode 100644 index 0000000..688f1bd --- /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 fbf5e0e..85d7065 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 { describe, it, expect, vi, beforeEach, type Mock } 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,127 @@ 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('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. + // + // 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; + 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(); + 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 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; + 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 12718d7..cd90ec9 100644 --- a/src/commands/branch/reset.ts +++ b/src/commands/branch/reset.ts @@ -1,11 +1,12 @@ 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'; 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 }); @@ -83,17 +87,41 @@ 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, + name: string, apiUrl: string | undefined, showProgress: boolean, startingState: string, ): 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 +138,32 @@ 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 …”. Retried, + // because this read decides the verdict and one unlucky 502 should not cost + // a reset that had just landed. + // + // 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 '${name}': 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-poll.test.ts b/src/commands/deployments/deploy-poll.test.ts index ad5d8ef..92b98ef 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 4d0758e..60a10ad 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', @@ -323,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. - const isTerminal = - err instanceof CLIError && - (err.statusCode === undefined || - (err.statusCode < 500 && !TRANSIENT_4XX_STATUSES.has(err.statusCode))); - if (isTerminal) { + // 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 9563136..5de8b90 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 9d8cc02..8326425 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 { diff --git a/src/lib/api/platform.ts b/src/lib/api/platform.ts index 56ffe9a..61b7e6e 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 4c92cc0..f0b50c0 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,44 @@ 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 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 64e8972..0c95e87 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,35 @@ export class PermissionError extends CLIError { } } +/** + * 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 + * 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` — 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 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); +} + /** * Format a Node fetch error with actionable context. *