From 36885759a461552f19648a849f03cfb804e16118 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Sun, 11 Oct 2026 04:22:22 -0400 Subject: [PATCH 1/3] fix(api): replay a gateway error only for reads, so a write is never sent twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every instance client replayed a 502/503/504 up to three times whatever the request, and none of those statuses means the request went unapplied: a proxy answers for an upstream that may already have it, and Harper itself answers 503 after handing an operation to a worker that exits or overruns. So an insert could store its records twice, a drop could succeed and then report "does not exist", and a deploy or restart could run again. The retry now replays only what the restart gate already treats as a read — a GET, or a body whose operation is a read by Harper's naming. That predicate moves out of the gate into isReadOperation.ts as isReadRequest, so the two share one rule. A write is sent once and its first 5xx is final. skipGatewayRetry stays for the operations console, whose component operations can carry a read-looking name. Closes #1879 Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 6 +- src/features/auth/describeAuthFailure.ts | 6 +- src/integrations/api/isReadOperation.test.ts | 25 ++++- src/integrations/api/isReadOperation.ts | 28 ++++- .../retryGatewayErrors.integration.test.ts | 101 ++++++++++++++++++ .../api/retryGatewayErrors.test.ts | 53 +++++++-- src/integrations/api/retryGatewayErrors.ts | 13 ++- .../api/sse/resolveInstanceConnection.ts | 5 +- src/lib/restart/restartGate.ts | 21 +--- 9 files changed, 215 insertions(+), 43 deletions(-) create mode 100644 src/integrations/api/retryGatewayErrors.integration.test.ts diff --git a/DESIGN.md b/DESIGN.md index 622b6af34..ed56bc6ae 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -29,7 +29,7 @@ The `/apis` route is a **custom, in-house API explorer** ([`src/features/instanc [`src/features/instance/operations/`](src/features/instance/operations/) sends one freeform operations-API body through the entity's own `getInstanceClient` client (direct, basic, or proxied — whatever every other page uses). The rules, each pinned by a test there: - **The typed text is the request body.** It is parsed only to validate it and read `operation`; re-serializing the parse would round integers past 2^53 (a primary key) before Harper saw them. Axios sends a string body that parses as JSON as-is, only trimmed. -- **At most one send.** It sets `skipGatewayRetry`, because the shared client otherwise replays a 502/503/504 up to three times and a gateway can answer that after Harper applied the operation. Only a request Studio refused before sending counts as unsent. Anything that reached Harper marks data stale, an error answer included, because an arbitrary operation can change something before it fails; and a request abandoned mid-flight (the page unmounted) is reported as possibly applied. +- **At most one send.** It sets `skipGatewayRetry`, because the shared client replays a 502/503/504 for any body whose `operation` reads by name, and a component's `get_…` operation is no proof that it only reads. Only a request Studio refused before sending counts as unsent. Anything that reached Harper marks data stale, an error answer included, because an arbitrary operation can change something before it fails; and a request abandoned mid-flight (the page unmounted) is reported as possibly applied. - **Only Harper's own reads skip the confirmation.** [`isBuiltInRead`](src/features/instance/operations/operationKinds.ts) is the restart gate's naming rule ([`isReadOperation`](src/integrations/api/isReadOperation.ts)) _and_ membership in the role editor's catalog of Harper's operations, because a component can register a mutating `get_…`. Everything else — unknown names, every `sql` — confirms, showing the exact body. It guards against a mistyped paste; Harper's `verifyPerms` is what authorizes each operation. - **Operations Studio has to know about are not sent blind.** A `restart` is tracked with `markRestarting` exactly like the Restart button's (released at once on a 4xx; a 5xx can be a gateway losing the answer to a restart that ran), so the page's reads are held (an instance) or their connectivity errors muted (a cluster, which restarts one member) until `waitUntilAnsweringAfterRestart` sees the target again. `logout` is refused: it would end the session the rest of Studio is still using. Any other operation with such a side effect needs the same treatment rather than a raw send. - **Nothing leaves the page.** Failures render inline and never pass through `console.error` or the global React Query handlers (RUM reports the former, and bodies carry passwords and secrets). The editor buffer is not persisted, and the console remounts per entity so a confirmation written for one instance can never be sent to another. After a possible write, entity queries are marked stale with `refetchType: 'none'`, never refetched into whatever the operation just did. The download is abandoned once progress passes 5 MB (XHR reports progress about every 50 ms, so this bounds, not caps, what arrives), and display truncates like the API explorer. @@ -50,6 +50,10 @@ The wide hero artwork spans both columns beneath the form card in three independ - **Only query errors are muted, and only connectivity-class ones.** Mutations still toast; a 403 or a Harper 500 during a restart still toasts. - **Detection is per tab.** A page learns of a container op started elsewhere only from a cluster-record fetch, and the in-cluster studio pages don't poll the cluster, so they don't notice one started in another tab. +## Instance clients replay only reads + +Every `getInstanceClient` client replays a 502/503/504 up to three times (5s/10s/20s), but only for a request [`isReadRequest`](src/integrations/api/isReadOperation.ts) accepts: a GET, or a body whose `operation` is a read by Harper's naming — the restart gate's rule, one predicate for both ([`retryGatewayErrors.ts`](src/integrations/api/retryGatewayErrors.ts)). A write is sent once and its first 5xx is final, because no gateway status proves the write was not applied: Harper itself answers 503 after handing an operation to a worker that exits or overruns. Do not add an exemption for a "usually safe" status or a write that looks idempotent; a caller whose write is safe to repeat retries it itself, where it can say why. + ## Cluster upgrade picker — which versions an existing cluster is offered [`buildUpgradeVersionOptions`](src/features/clusters/upsert/lib/buildUpgradeVersionOptions.ts) is the one place that decides, and its test file is the spec. Two rules, and they differ on purpose: diff --git a/src/features/auth/describeAuthFailure.ts b/src/features/auth/describeAuthFailure.ts index c866db7ba..3c285e2e5 100644 --- a/src/features/auth/describeAuthFailure.ts +++ b/src/features/auth/describeAuthFailure.ts @@ -15,9 +15,9 @@ export const OUTCOME_UNKNOWN_MESSAGE = "We didn't get an answer from the server, so we can't tell whether that went through."; // 503 alone: the server is declining requests, so it very likely never processed this one and a -// plain retry is honest. (`curryRetryGatewayErrors` also classes 502/504 as retryable, but it is -// installed on instance clients only, never on `apiClient` — nothing retries these calls -// automatically, and the retry this copy invites is the user's own.) +// plain retry is honest. (`curryRetryGatewayErrors` replays 502/503/504, but only for reads and +// only on instance clients, never on `apiClient` — nothing retries these calls automatically, and +// the retry this copy invites is the user's own.) const TRANSIENT_STATUSES: ReadonlySet = new Set([503]); // Every form using this submits a non-idempotent POST, and each of these means the request had // already been handed upstream, so the write may have applied (RFC 9110 §9.2.2 — a retry is only diff --git a/src/integrations/api/isReadOperation.test.ts b/src/integrations/api/isReadOperation.test.ts index 9ce03821d..26510cb37 100644 --- a/src/integrations/api/isReadOperation.test.ts +++ b/src/integrations/api/isReadOperation.test.ts @@ -1,4 +1,4 @@ -import { isReadOperation } from '@/integrations/api/isReadOperation'; +import { isReadOperation, isReadRequest } from '@/integrations/api/isReadOperation'; import { describe, expect, it } from 'vitest'; describe('isReadOperation', () => { @@ -43,3 +43,26 @@ describe('isReadOperation', () => { }, ); }); + +describe('isReadRequest', () => { + it.each([ + ['a GET', { method: 'get' }], + ['an upper-case GET', { method: 'GET' }], + ['a read body', { method: 'post', data: { operation: 'describe_all' } }], + ['a read body axios already serialized', { method: 'post', data: JSON.stringify({ operation: 'get_job' }) }], + ])('reads %s as a read', (_, config) => { + expect(isReadRequest(config)).toBe(true); + }); + + it.each([ + ['a write body', { method: 'post', data: { operation: 'insert' } }], + ['a serialized write body', { method: 'post', data: JSON.stringify({ operation: 'drop_table' }) }], + ['a body that is not JSON', { method: 'post', data: 'describe_all' }], + ['a JSON null body', { method: 'post', data: 'null' }], + ['no body', { method: 'post' }], + ['a DELETE', { method: 'delete' }], + ['a method that is not a string', { method: 42 }], + ])('treats %s as able to change something', (_, config) => { + expect(isReadRequest(config)).toBe(false); + }); +}); diff --git a/src/integrations/api/isReadOperation.ts b/src/integrations/api/isReadOperation.ts index 84ff5c56b..1d1e10b0b 100644 --- a/src/integrations/api/isReadOperation.ts +++ b/src/integrations/api/isReadOperation.ts @@ -1,8 +1,9 @@ /** * Reads, by Harper's operation naming: the restart gate holds only these (anything else is refused - * while the entity restarts), and the operations console asks before sending anything else. Token - * minting is deliberately not one — held, it would stall a route guard's Fabric Connect attempt behind - * a notice that cannot mount yet; refused, the guard falls through to sign-in at once. + * while the entity restarts), the gateway retry replays only these, and the operations console asks + * before sending anything else. Token minting is deliberately not one — held, it would stall a route + * guard's Fabric Connect attempt behind a notice that cannot mount yet; refused, the guard falls + * through to sign-in at once. */ const READ_OPERATION = /^(?:get_|search_|describe_|list_|read_|user_info$|system_information$|cluster_status$|registration_info$)/; @@ -10,3 +11,24 @@ const READ_OPERATION = export function isReadOperation(operation: unknown): boolean { return typeof operation === 'string' && READ_OPERATION.test(operation); } + +function operationOf(data: unknown): unknown { + if (typeof data !== 'string') { + return (data as { operation?: unknown } | undefined)?.operation; + } + // By the time a request has been sent, axios has serialized `config.data` in place; the operations + // console also sends raw text. + try { + return (JSON.parse(data) as { operation?: unknown } | null)?.operation; + } catch { + return undefined; + } +} + +/** + * A GET, or an operations-API body naming a read. Only these are safe to send late (the restart gate) + * or twice (the gateway retry); anything unrecognized counts as a write. + */ +export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean { + return (typeof method === 'string' && method.toLowerCase() === 'get') || isReadOperation(operationOf(data)); +} diff --git a/src/integrations/api/retryGatewayErrors.integration.test.ts b/src/integrations/api/retryGatewayErrors.integration.test.ts new file mode 100644 index 000000000..c7c14b9eb --- /dev/null +++ b/src/integrations/api/retryGatewayErrors.integration.test.ts @@ -0,0 +1,101 @@ +/** @vitest-environment jsdom */ +import { getInstanceClient } from '@/config/getInstanceClient'; +import * as sleepModule from '@/lib/sleep'; +import { AxiosError, type InternalAxiosRequestConfig } from 'axios'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('@/lib/sleep', () => ({ + sleep: vi.fn().mockResolvedValue(true), +})); + +const OPERATIONS_URL = 'https://ins-123.example.com:9925/'; + +/** + * A target behind a gateway that answers `statuses` in turn, then 200. Axios applies + * `validateStatus` inside its own adapters, so this one rejects a non-2xx itself. + */ +function gateway(...statuses: number[]) { + const sent: Array<{ method?: string; body: unknown }> = []; + const client = getInstanceClient({ id: 'ins-123', operationsUrl: OPERATIONS_URL }); + client.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + sent.push({ method: config.method, body: typeof config.data === 'string' ? JSON.parse(config.data) : config.data }); + const status = statuses.shift() ?? 200; + const response = { status, statusText: '', data: { status }, headers: {}, config }; + if (status >= 400) { + throw new AxiosError( + `Request failed with status code ${status}`, + AxiosError.ERR_BAD_RESPONSE, + config, + undefined, + response, + ); + } + return response; + }; + return { client, sent }; +} + +function statusOf(error: unknown): number | undefined { + return (error as AxiosError).response?.status; +} + +describe('the gateway retry on a real instance client', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it.each([502, 503, 504])('sends a write once and surfaces its %i', async (status) => { + const { client, sent } = gateway(status, status, status, status); + + const error = await client.post('/', { operation: 'insert', records: [{ name: 'a' }] }).catch((e: unknown) => e); + + expect(statusOf(error)).toBe(status); + expect(sent).toEqual([{ method: 'post', body: { operation: 'insert', records: [{ name: 'a' }] } }]); + expect(sleepModule.sleep).not.toHaveBeenCalled(); + }); + + it.each(['drop_table', 'deploy_component', 'set_configuration', 'restart', 'create_authentication_tokens'])( + 'never replays %s', + async (operation) => { + const { client, sent } = gateway(504); + + await expect(client.post('/', { operation })).rejects.toBeInstanceOf(AxiosError); + expect(sent).toHaveLength(1); + }, + ); + + it.each([502, 503, 504])('replays a read three times on a %i, 5s/10s/20s apart, as before', async (status) => { + const { client, sent } = gateway(status, status, status, status); + + const error = await client.post('/', { operation: 'describe_all' }).catch((e: unknown) => e); + + expect(statusOf(error)).toBe(status); + expect(sent).toHaveLength(4); + expect(sent.every(({ body }) => (body as { operation: string }).operation === 'describe_all')).toBe(true); + expect(vi.mocked(sleepModule.sleep).mock.calls).toEqual([[5_000], [10_000], [20_000]]); + }); + + it('returns the answer to a read that recovers on its replay', async () => { + const { client, sent } = gateway(503); + + const { data } = await client.post('/', { operation: 'get_job', id: 'job-1' }); + + expect(data).toEqual({ status: 200 }); + expect(sent).toHaveLength(2); + }); + + it('replays a GET', async () => { + const { client, sent } = gateway(502, 502); + + await expect(client.get('/health')).resolves.toMatchObject({ status: 200 }); + expect(sent).toHaveLength(3); + }); + + it('sends a read that opted out once', async () => { + const { client, sent } = gateway(503); + + await expect(client.post('/', { operation: 'describe_all' }, { skipGatewayRetry: true })).rejects + .toBeInstanceOf(AxiosError); + expect(sent).toHaveLength(1); + }); +}); diff --git a/src/integrations/api/retryGatewayErrors.test.ts b/src/integrations/api/retryGatewayErrors.test.ts index 3d1a8c827..f2f7b8fc5 100644 --- a/src/integrations/api/retryGatewayErrors.test.ts +++ b/src/integrations/api/retryGatewayErrors.test.ts @@ -7,6 +7,9 @@ vi.mock('@/lib/sleep', () => ({ sleep: vi.fn().mockResolvedValue(true), })); +/** A read as the interceptor sees it: axios has serialized `config.data` by the time it fails. */ +const READ = { method: 'post', data: JSON.stringify({ operation: 'describe_all' }) }; + describe('curryRetryGatewayErrors', () => { let client: Pick; @@ -32,9 +35,9 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).not.toHaveBeenCalled(); }); - it('rejects and does not retry for non-gateway status codes', async () => { + it('rejects and does not retry a read for non-gateway status codes', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = {}; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ }; const err = { response: { status: 500 }, config }; await expect(handler(err)).rejects.toBe(err); @@ -44,9 +47,9 @@ describe('curryRetryGatewayErrors', () => { expect(config.__retryCount).toBeUndefined(); }); - it('retries on 502 and increments __retryCount with initial delay on first retry', async () => { + it('retries a read on 502 and increments __retryCount with initial delay on first retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = {}; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ }; const err = { response: { status: 502 }, config }; const result = await handler(err); @@ -59,9 +62,9 @@ describe('curryRetryGatewayErrors', () => { expect(result).toEqual({ ok: true }); }); - it('retries on 503 with exponential backoff: more backoff on second retry', async () => { + it('retries a read on 503 with exponential backoff: more backoff on second retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 1 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 1 }; const err = { response: { status: 503 }, config }; await handler(err); @@ -72,9 +75,9 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).toHaveBeenCalledTimes(1); }); - it('retries on 504 with the largest backoff on third retry', async () => { + it('retries a read on 504 with the largest backoff on third retry', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 2 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 2 }; const err = { response: { status: 504 }, config }; await handler(err); @@ -85,9 +88,37 @@ describe('curryRetryGatewayErrors', () => { expect(client.request).toHaveBeenCalledTimes(1); }); - it.each([502, 503, 504])('never replays a %i for a request that opted out', async (status) => { + it.each([ + ['an object body', { operation: 'insert', records: [] }], + ['a serialized body', JSON.stringify({ operation: 'drop_table', table: 'dog' })], + ['token minting', JSON.stringify({ operation: 'create_authentication_tokens' })], + ['a body that is not JSON', 'operation=describe_all'], + ['no body', undefined], + ])('never replays a write on any gateway status: %s', async (_, data) => { + const handler = curryRetryGatewayErrors(client); + for (const status of [502, 503, 504]) { + const config: AxiosRequestConfig & { __retryCount?: number } = { method: 'post', data }; + const err = { response: { status }, config }; + + await expect(handler(err)).rejects.toBe(err); + expect(config.__retryCount).toBeUndefined(); + } + expect(sleepModule.sleep).not.toHaveBeenCalled(); + expect(client.request).not.toHaveBeenCalled(); + }); + + it('replays a GET, which carries no operation', async () => { + const handler = curryRetryGatewayErrors(client); + const config: AxiosRequestConfig & { __retryCount?: number } = { method: 'get' }; + + await handler({ response: { status: 503 }, config }); + + expect(client.request).toHaveBeenCalledWith(config); + }); + + it.each([502, 503, 504])('never replays a %i for a read that opted out', async (status) => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { skipGatewayRetry: true }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, skipGatewayRetry: true }; const err = { response: { status }, config }; await expect(handler(err)).rejects.toBe(err); @@ -98,7 +129,7 @@ describe('curryRetryGatewayErrors', () => { it('stops retrying when max retries reached (>= 3)', async () => { const handler = curryRetryGatewayErrors(client); - const config: AxiosRequestConfig & { __retryCount?: number } = { __retryCount: 3 }; + const config: AxiosRequestConfig & { __retryCount?: number } = { ...READ, __retryCount: 3 }; const err = { response: { status: 502 }, config }; await expect(handler(err)).rejects.toBe(err); diff --git a/src/integrations/api/retryGatewayErrors.ts b/src/integrations/api/retryGatewayErrors.ts index 306c0e8d0..766c2838d 100644 --- a/src/integrations/api/retryGatewayErrors.ts +++ b/src/integrations/api/retryGatewayErrors.ts @@ -1,16 +1,23 @@ +import { isReadRequest } from '@/integrations/api/isReadOperation'; import { sleep } from '@/lib/sleep'; import { AxiosInstance } from 'axios'; declare module 'axios' { interface AxiosRequestConfig { /** - * Send at most once. A 502/504 can arrive after Harper has applied the operation, so a request - * whose idempotence is unknown (the operations console's freeform body) must not be replayed. + * Send at most once, even a read. The operations console sets it: its freeform body can name a + * component's operation, and a `get_` name from a component is no proof that it only reads. */ skipGatewayRetry?: boolean; } } +/** + * Replays a 502/503/504 up to three times, for reads only. None of the three means the request went + * unapplied — a proxy answers for an upstream that may already have it, and Harper itself answers 503 + * when the worker running an operation exits or overruns — so a write is sent once and its first + * answer is final. + */ export function curryRetryGatewayErrors(client: Pick) { // eslint-disable-next-line @typescript-eslint/no-explicit-any return async (error: any) => { @@ -20,7 +27,7 @@ export function curryRetryGatewayErrors(client: Pick) return Promise.reject(error); } - const shouldRetry = status === 502 || status === 503 || status === 504; + const shouldRetry = (status === 502 || status === 503 || status === 504) && isReadRequest(config); if (!shouldRetry) { return Promise.reject(error); } diff --git a/src/integrations/api/sse/resolveInstanceConnection.ts b/src/integrations/api/sse/resolveInstanceConnection.ts index 44bce0e9b..6ccadbd76 100644 --- a/src/integrations/api/sse/resolveInstanceConnection.ts +++ b/src/integrations/api/sse/resolveInstanceConnection.ts @@ -32,8 +32,9 @@ export interface ResolvedInstanceConnection { * connection/auth resolution in `src/config/getInstanceClient.ts` one-way: `getInstanceClient` * is deliberately left untouched so its battle-tested axios path — including the * `rejectReplicationFailures` / gateway-retry interceptors that this fetch path intentionally - * does NOT carry — keeps working for every non-streaming caller. Callers recover gateway/ - * cold-start resilience by falling back to the axios path when the stream can't be established. + * does NOT carry — keeps working for every non-streaming caller. Callers fall back to the axios + * path when the stream can't be established; a read regains its gateway retry there, a deploy does + * not, because that retry replays reads only. * * Fabric Connect now resolves to a direct Bearer (operation token) connection when it can * (see authStore.establishFabricConnectAuth); we prefer that so SSE streams straight from diff --git a/src/lib/restart/restartGate.ts b/src/lib/restart/restartGate.ts index 81a7fbcb6..b9a2537cb 100644 --- a/src/lib/restart/restartGate.ts +++ b/src/lib/restart/restartGate.ts @@ -1,4 +1,4 @@ -import { isReadOperation } from '@/integrations/api/isReadOperation'; +import { isReadRequest } from '@/integrations/api/isReadOperation'; import { getRestartState, holdsRequests, isRestarting, waitUntilReachable } from '@/lib/restart/restartTracker'; import { AxiosError, type AxiosInstance, CanceledError, type InternalAxiosRequestConfig } from 'axios'; @@ -15,23 +15,6 @@ declare module 'axios' { export const RESTARTING_ERROR_CODE = 'ERR_TARGET_RESTARTING'; -function operationOf(data: unknown): unknown { - if (typeof data !== 'string') { - return (data as { operation?: unknown } | undefined)?.operation; - } - // A gateway retry re-sends the config axios already serialized; the operations console sends raw text. - try { - return (JSON.parse(data) as { operation?: unknown } | null)?.operation; - } catch { - return undefined; - } -} - -function isSafeToDelay(config: InternalAxiosRequestConfig): boolean { - if (config.method?.toLowerCase() === 'get') { return true; } - return isReadOperation(operationOf(config.data)); -} - /** * While the tracker says this kind of client cannot reach `entityId`, hold its reads and send them * once it can — a held read is a query still fetching, where a failed one is an error state, a retry @@ -55,7 +38,7 @@ export function installRestartGate( ) { client.interceptors.request.use( (config: InternalAxiosRequestConfig) => { - if (!isSafeToDelay(config)) { + if (!isReadRequest(config)) { const target = entityId.startsWith('clu-') ? 'This cluster' : 'This instance'; throw new AxiosError( `${target} is restarting. Try again once it's back online.`, From c2dc659329070416ed88fd292e1e66f79a6d847b Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:25:56 -0400 Subject: [PATCH 2/3] fix(api): read a request's operation only from a POST body isReadRequest took the operation name from any request's body, so a DELETE, PUT or PATCH whose body happened to carry a read-named operation field counted as a read: the restart gate would hold it and the gateway retry would replay it. Harper takes an operation only from POST / (server/operationsServer.ts) and central manager's proxy forwards one only from a POST body (HDBInstance.js post), so on any other method that field is data and the request is a write. A missing or non-string method stays a write too; axios always sets a lowercase method before interceptors run (lib/core/Axios.js), so no real request reaches the predicate without one. Co-Authored-By: Claude Opus 5.5 --- DESIGN.md | 2 +- src/integrations/api/isReadOperation.test.ts | 6 ++++++ src/integrations/api/isReadOperation.ts | 9 ++++++--- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index ed56bc6ae..95933fc8d 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -52,7 +52,7 @@ The wide hero artwork spans both columns beneath the form card in three independ ## Instance clients replay only reads -Every `getInstanceClient` client replays a 502/503/504 up to three times (5s/10s/20s), but only for a request [`isReadRequest`](src/integrations/api/isReadOperation.ts) accepts: a GET, or a body whose `operation` is a read by Harper's naming — the restart gate's rule, one predicate for both ([`retryGatewayErrors.ts`](src/integrations/api/retryGatewayErrors.ts)). A write is sent once and its first 5xx is final, because no gateway status proves the write was not applied: Harper itself answers 503 after handing an operation to a worker that exits or overruns. Do not add an exemption for a "usually safe" status or a write that looks idempotent; a caller whose write is safe to repeat retries it itself, where it can say why. +Every `getInstanceClient` client replays a 502/503/504 up to three times (5s/10s/20s), but only for a request [`isReadRequest`](src/integrations/api/isReadOperation.ts) accepts: a GET, or a POST whose body's `operation` is a read by Harper's naming — the restart gate's rule, one predicate for both ([`retryGatewayErrors.ts`](src/integrations/api/retryGatewayErrors.ts)). A write is sent once and its first 5xx is final, because no gateway status proves the write was not applied: Harper itself answers 503 after handing an operation to a worker that exits or overruns. Do not add an exemption for a "usually safe" status or a write that looks idempotent; a caller whose write is safe to repeat retries it itself, where it can say why. ## Cluster upgrade picker — which versions an existing cluster is offered diff --git a/src/integrations/api/isReadOperation.test.ts b/src/integrations/api/isReadOperation.test.ts index 26510cb37..79f87b361 100644 --- a/src/integrations/api/isReadOperation.test.ts +++ b/src/integrations/api/isReadOperation.test.ts @@ -49,6 +49,7 @@ describe('isReadRequest', () => { ['a GET', { method: 'get' }], ['an upper-case GET', { method: 'GET' }], ['a read body', { method: 'post', data: { operation: 'describe_all' } }], + ['a read body under an upper-case POST', { method: 'POST', data: { operation: 'describe_all' } }], ['a read body axios already serialized', { method: 'post', data: JSON.stringify({ operation: 'get_job' }) }], ])('reads %s as a read', (_, config) => { expect(isReadRequest(config)).toBe(true); @@ -61,7 +62,12 @@ describe('isReadRequest', () => { ['a JSON null body', { method: 'post', data: 'null' }], ['no body', { method: 'post' }], ['a DELETE', { method: 'delete' }], + ['a DELETE with a read body', { method: 'delete', data: { operation: 'describe_all' } }], + ['a PUT with a serialized read body', { method: 'put', data: JSON.stringify({ operation: 'get_job' }) }], + ['a PATCH with a read body', { method: 'patch', data: { operation: 'search_by_value' } }], + ['a read body with no method', { data: { operation: 'describe_all' } }], ['a method that is not a string', { method: 42 }], + ['a read body under a method that is not a string', { method: 42, data: { operation: 'describe_all' } }], ])('treats %s as able to change something', (_, config) => { expect(isReadRequest(config)).toBe(false); }); diff --git a/src/integrations/api/isReadOperation.ts b/src/integrations/api/isReadOperation.ts index 1d1e10b0b..236ce51c4 100644 --- a/src/integrations/api/isReadOperation.ts +++ b/src/integrations/api/isReadOperation.ts @@ -26,9 +26,12 @@ function operationOf(data: unknown): unknown { } /** - * A GET, or an operations-API body naming a read. Only these are safe to send late (the restart gate) - * or twice (the gateway retry); anything unrecognized counts as a write. + * A GET, or a POST whose operations-API body names a read. Only these are safe to send late (the + * restart gate) or twice (the gateway retry); anything unrecognized counts as a write. Harper and + * central manager's proxy take an operation only from a POST body, so on any other method an + * `operation` field is just data. */ export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean { - return (typeof method === 'string' && method.toLowerCase() === 'get') || isReadOperation(operationOf(data)); + const verb = typeof method === 'string' ? method.toLowerCase() : undefined; + return verb === 'get' || (verb === 'post' && isReadOperation(operationOf(data))); } From 77ef3b02e5657ef1706ad35bbf6e102c3a1622e4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Mon, 12 Oct 2026 11:37:08 -0400 Subject: [PATCH 3/3] docs(api): say why isReadRequest's rule holds, not what it checks The cross-model round flagged the JSDoc's first sentence as restating the predicate's two branches. Keep only what the code cannot say: what the answer is used for, the fail-closed default, and why only a POST body names an operation. Co-Authored-By: Claude Opus 5.5 --- src/integrations/api/isReadOperation.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/integrations/api/isReadOperation.ts b/src/integrations/api/isReadOperation.ts index 236ce51c4..274dda618 100644 --- a/src/integrations/api/isReadOperation.ts +++ b/src/integrations/api/isReadOperation.ts @@ -26,10 +26,9 @@ function operationOf(data: unknown): unknown { } /** - * A GET, or a POST whose operations-API body names a read. Only these are safe to send late (the - * restart gate) or twice (the gateway retry); anything unrecognized counts as a write. Harper and - * central manager's proxy take an operation only from a POST body, so on any other method an - * `operation` field is just data. + * Whether a request is safe to send late (the restart gate) or twice (the gateway retry); anything + * unrecognized counts as a write. Harper and central manager's proxy take an operation only from a + * POST body, so on any other method an `operation` field is just data. */ export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean { const verb = typeof method === 'string' ? method.toLowerCase() : undefined;