fix(api): replay a gateway error only for reads, so a write is never sent twice

[dawsontoth]

Stacked on #1882 (claude/1283-freeform-operations). It reuses that PR's isReadOperation and skipGatewayRetry, so it targets that branch. Retarget it to stage once #1882 merges.

⊙ Problem

Every instance client replayed a 502, 503 or 504 up to three times (5s, 10s, 20s apart), whatever the request. None of those statuses means the request went unapplied, so a write whose answer a gateway lost could run again: an insert storing its records twice, a drop_table that worked and then reported "does not exist", a second deploy_component or restart. Closes #1879.

❓ Your call: Writes now get no automatic retry on any gateway status, 503 included. The issue suggested also replaying a write's 503 ("usually refused before processing"). I left that out: Harper itself answers 503 after it has handed an operation to a worker that then exits or times out (harper server/serverHelpers/registeredOperations.ts, The worker thread executing this operation exited / Timed out waiting for worker thread…). Adding a 503 exemption back would be a one-line change.

💡 Solution

The gateway retry now replays only requests the restart gate already treats as reads: a GET, or an operations-API body whose operation is a read by Harper's naming. A write is sent once, and its first 5xx reaches the caller the same way the last failed retry used to. The predicate moves out of the restart gate into isReadOperation.ts as isReadRequest, so "safe to send late" and "safe to send twice" are one rule with one list. Like the gate, it parses a body that axios has already serialized. That matters here: by the time a response fails, axios 1.20.0 has replaced config.data with its JSON string (lib/core/dispatchRequest.js:46). Anything it does not recognize counts as a write. skipGatewayRetry stays, and the operations console still sets it, because a component can register a get_… operation that writes.

Callers that lose the replay, checked one by one:

• Restarts (restart / restart_service): a 502/504 now fails the click straight away instead of sending a second restart 5s later. The tracker is still released in finally. The user_info probes in waitUntilAnsweringAfterRestart are reads and keep their retry.
• Rolling set_configuration: the user_info probe still retries. A 5xx on set_configuration or restart_service counts that member as failed, and the loop moves on to the existing "Only N of M restarted" toast. No member restarts twice.
• Deploys: the live (SSE) deploy uses fetch and is unaffected. A 504 on the buffered deploy_component now shows "Failed to import" instead of deploying a second time.
• Logouts: sign-out's logout client already drops every response interceptor (fix(auth): send the sign-out logout to the instance, and stop its failure reaching Error Tracking #1680). Disconnect swallows the error and clears local state anyway; it now finishes up to 35s sooner.
• Token minting (create_authentication_tokens, refresh_operation_token): neither is a read by name, on purpose (see the restart gate). A gateway status on the route guard's mint now sends the user to sign-in at once instead of holding navigation for up to 35s. A gateway status on a refresh falls back to re-minting through the proxy at once.
• Polls (get_job, get_status, get_deployment, get_analytics, cluster_status, GET /health, GET /api/openapi/rest): reads, so nothing changes.

❓ Your call: Token minting gives up its retry together with every other write. Both token operations would be safe to replay, as traced in harper security/tokenAuthentication.ts: create_authentication_tokens overwrites hdb_user.refresh_token, so the latest mint wins, and refresh_operation_token writes nothing. I kept the rule as plain "reads only" rather than adding a per-call opt-in, because these failures already fall back to something the user can act on. If a transient gateway blip on first visit should not land people on sign-in, the alternative is a gatewayRetry: true opt-in on those two calls. It is easy to add and easy to remove.

⚖️ Alternatives

• Set skipGatewayRetry on every write call site. Rejected: the replay stays the default, there are about 40 distinct write operations, and any new write that forgets the flag brings the bug back.
• Opt reads in at call sites. Rejected: reads are most of the roughly 80 files that use an instance client, and every read that was not touched would silently lose its retry. The body already names the operation, so the shared client can classify it, as the restart gate does.
• Remove the retry. Rejected: reads outside React Query (the route guard's user_info, the rolling-config probes, the restart probes) have no other retry.
• Use the console's isBuiltInRead (naming plus the role-editor catalog). Not needed: only the console sends operations Studio did not write itself, and it opts out entirely. Pulling the catalog into integrations/api would also make the integration layer depend on a feature.
• Server-side dedupe (idempotency keys). Neither Harper's operations API nor central manager's proxy has one.

The planning review (codex) returned Framing-Verdict: chosen-approach-sound.

🔧 Changes

• src/integrations/api/retryGatewayErrors.ts: the status check now requires isReadRequest(config). The doc comments say why a gateway status is not proof that a request went unapplied, and what skipGatewayRetry is still for.
• src/integrations/api/isReadOperation.ts: adds isReadRequest, the gate's former operationOf + isSafeToDelay, unchanged except that a non-string method reads as not-GET instead of throwing inside the interceptor.
• src/lib/restart/restartGate.ts: calls isReadRequest; the gate's behavior is unchanged.
• DESIGN.md: a new "Instance clients replay only reads" note, and the console's "At most one send" bullet now gives the reason that still holds.
• Comment-only: src/features/auth/describeAuthFailure.ts and src/integrations/api/sse/resolveInstanceConnection.ts described the old retry; both now describe the new one.

Filed while tracing callers: #1889. The live deploy's fallback sends deploy_component again after a gateway status or a dropped connection, which is the same double send through fetch, outside this interceptor. #1890. A write's gateway status still reads as a plain failure, which invites a manual retry. This PR stops the automatic replays and leaves that copy as it was. Residual, not filed: a read's retry on a proxied or cookie client re-sends with whatever credentials are current after the 5s–35s wait. Direct-connect clients already refuse that through the connection-generation stamp. This PR narrows that exposure to reads; it does not change it.

✅ Verification

• End to end, through the real interceptor chain: src/integrations/api/retryGatewayErrors.integration.test.ts builds a real getInstanceClient client whose adapter rejects with an AxiosError carrying 502/503/504, then counts adapter calls. A write (insert, drop_table, deploy_component, set_configuration, restart, create_authentication_tokens) is sent once. A read (describe_all) is sent 4 times, sleeping 5s/10s/20s. A recovering get_job resolves on its replay, a GET replays, and a read with skipGatewayRetry is sent once.
• Fails on base: on aaca36e38 (feat(instance): add an Operations console for sending any operations-API request #1882's head) the same file fails 8 of 14 tests, all of them write cases, with the bug's signature: expected [ …(4) ] to deeply equal [ { method: 'post', … } ].
• Mutation checks: dropping && isReadRequest(config) turns 13 tests red. Ignoring a serialized body turns 9 red, among them the end-to-end read replays and the restart gate's existing serialized-body test. Dropping the GET branch turns 4 red. Every mutation was reverted.
• Unit tests: retryGatewayErrors.test.ts (the retry tests now use a serialized read; new write cases cover object, serialized, token-mint, non-JSON and missing bodies across all three statuses) and isReadOperation.test.ts (isReadRequest).
• Gate: the pre-commit hook's full vitest run passed 402 files / 3,791 tests. npx tsc -b exit 0, npx oxlint --format stylish . exit 0, npx dprint check exit 0.
• Not verified in a browser: a gateway status cannot be produced on demand against a live instance, so the adapter-level end-to-end test is the route.
• Cross-model review: one full round, codex (graded) and gemini. Codex's one finding was the misleading failure copy, now A write's gateway status or timeout reads as a plain failure, though the write may have been applied #1890. Gemini's two were rejected on evidence. Its "backoff applies only to 503" blocker is contradicted by the passing end-to-end tests, which assert 5s/10s/20s for 502 and 504 too. Its "JSON.parse of a large failed write" cost runs only on a gateway status, and it is the same order of work as the JSON.stringify axios already did to send that body. The Cursor legs failed no-receipt (the 1Password SSH agent refused git fetch), and the Harper domain leg failed auth (expired OAuth session), so no adjudication ran. That is an infrastructure gap, not an open finding.

_{🤖 Generated with Claude Code (Claude Opus 5.5); posted via @dawsontoth.}

Related PRs: #1882 overlaps (stack base — this PR builds on its isReadOperation and skipGatewayRetry and must land after it)
Complexity: medium

_{Review-Coverage: authored=claude; ran=codex,gemini; blocked=cursor-grok(no-receipt),cursor-composer(no-receipt),domain(auth); declined=cursor-kimi,cursor-muse; rounds=1; full=1 @ 3688575}

_{Review-Attention: study ~11m (sensitive: describeAuthFailure.ts; raised: degraded review) @ 3688575}

[gemini-code-assist]

Code Review

This pull request refactors the gateway retry mechanism to ensure that only read requests (GET requests or POST requests with read operations) are replayed, and integrates this unified check into the restart gate. The reviewer feedback suggests making the isReadRequest utility more robust by defaulting undefined methods to 'get' and restricting operation-based read checks specifically to 'post' requests, as well as adding corresponding test cases to verify these edge cases.

[gemini-code-assist]

The current implementation of isReadRequest has two potential issues:

1. If method is omitted/undefined, it will not default to 'get', even though Axios defaults to 'get' for requests. This would cause default GET requests to be incorrectly classified as writes.
2. If the HTTP method is a write/mutation method other than POST (such as DELETE, PUT, or PATCH) but the request body happens to contain an operation field matching a read pattern, it will be incorrectly classified as a read request.

We can make this more robust by defaulting method to 'get' when it is undefined, and explicitly restricting the operation-based read check to POST requests.

Suggested change

	export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean {
	return (typeof method === 'string' && method.toLowerCase() === 'get') || isReadOperation(operationOf(data));
	}
	export function isReadRequest({ method, data }: { method?: unknown; data?: unknown }): boolean {
	const normalizedMethod = method === undefined ? 'get' : typeof method === 'string' ? method.toLowerCase() : '';
	if (normalizedMethod === 'get') {
	return true;
	}
	if (normalizedMethod === 'post') {
	return isReadOperation(operationOf(data));
	}
	return false;
	}

[gemini-code-assist]

Add a test case to verify that isReadRequest correctly defaults to 'get' when the method property is omitted/undefined.

Suggested change

	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) => {
	it.each([
	['a GET', { method: 'get' }],
	['an upper-case GET', { method: 'GET' }],
	['an omitted method (defaults to 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) => {

[gemini-code-assist]

Add a test case to verify that a non-POST/GET request (like DELETE) with a body containing a read operation is still correctly treated as a write request.

Suggested change

	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) => {
	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 DELETE with read body', { method: 'delete', data: { operation: 'describe_all' } }],
	['a method that is not a string', { method: 42 }],
	])('treats %s as able to change something', (_, config) => {

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	67.95%	10252 / 15087
🔵	Statements	68.09%	10933 / 16056
🔵	Functions	61.35%	2642 / 4306
🔵	Branches	62.99%	7824 / 12421

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/features/auth/describeAuthFailure.ts	100%	100%	100%	100%
src/integrations/api/isReadOperation.ts	100%	100%	100%	100%
src/integrations/api/retryGatewayErrors.ts	100%	100%	100%	100%
src/integrations/api/sse/resolveInstanceConnection.ts	100%	91.3%	100%	100%
src/lib/restart/restartGate.ts	97.56%	90.9%	100%	97.29%	94

Generated in workflow #2102 for commit 3688575 by the Vitest Coverage Report Action