fix(main): cancel unread fetch response bodies so a peer socket close cannot crash the app (#9142)
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
23368ee9da
commit
d0b090b0ff
|
|
@ -1,5 +1,6 @@
|
|||
import { Buffer } from 'node:buffer'
|
||||
import type { AzureDevOpsRepoRef } from './repository-ref'
|
||||
import { cancelUnreadResponseBody } from '../lib/unread-response-body'
|
||||
|
||||
const REQUEST_TIMEOUT_MS = 5000
|
||||
|
||||
|
|
@ -84,6 +85,7 @@ export async function requestAzureDevOpsJsonAtBase<T>(
|
|||
signal: AbortSignal.timeout(options.timeoutMs ?? REQUEST_TIMEOUT_MS)
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
return null
|
||||
}
|
||||
return (await response.json()) as T
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { cancelTrackingResponse } from '../lib/unread-response-body.test-fixtures'
|
||||
import {
|
||||
getAzureDevOpsAuthStatus,
|
||||
getAzureDevOpsPullRequestForBranch,
|
||||
|
|
@ -176,4 +177,22 @@ describe('Azure DevOps client', () => {
|
|||
headSha: 'newsha'
|
||||
})
|
||||
})
|
||||
|
||||
it('cancels unread error-response bodies so bundled undici cannot crash on socket close', async () => {
|
||||
gitExecFileAsyncMock.mockResolvedValue({
|
||||
stdout: 'https://dev.azure.com/acme/Project/_git/repo\n'
|
||||
})
|
||||
let cancelledBodies = 0
|
||||
const fetchMock = vi.fn(async () =>
|
||||
cancelTrackingResponse(502, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
globalThis.fetch = fetchMock as never
|
||||
|
||||
await getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/feature/azure')
|
||||
|
||||
expect(fetchMock).toHaveBeenCalled()
|
||||
expect(cancelledBodies).toBe(fetchMock.mock.calls.length)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { cancelTrackingResponse } from '../lib/unread-response-body.test-fixtures'
|
||||
|
||||
const { gitExecFileAsyncMock } = vi.hoisted(() => ({
|
||||
gitExecFileAsyncMock: vi.fn()
|
||||
|
|
@ -122,4 +123,19 @@ describe('Bitbucket client', () => {
|
|||
account: 'bitbucket-user'
|
||||
})
|
||||
})
|
||||
|
||||
it('cancels unread error-response bodies so bundled undici cannot crash on socket close', async () => {
|
||||
let cancelledBodies = 0
|
||||
const fetchMock = vi.fn(async () =>
|
||||
cancelTrackingResponse(502, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
vi.stubGlobal('fetch', fetchMock)
|
||||
|
||||
await getBitbucketPullRequestForBranch('/repo', 'refs/heads/feature/bitbucket')
|
||||
|
||||
expect(fetchMock).toHaveBeenCalled()
|
||||
expect(cancelledBodies).toBe(fetchMock.mock.calls.length)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ import {
|
|||
getHostedReviewLocalGitOptions,
|
||||
type HostedReviewExecutionOptions
|
||||
} from '../source-control/hosted-review-git-options'
|
||||
import { cancelUnreadResponseBody } from '../lib/unread-response-body'
|
||||
|
||||
const DEFAULT_API_BASE_URL = 'https://api.bitbucket.org/2.0'
|
||||
const REQUEST_TIMEOUT_MS = 5000
|
||||
|
|
@ -97,6 +98,7 @@ async function requestJson<T>(path: string, options: RequestOptions = {}): Promi
|
|||
signal: AbortSignal.timeout(options.timeoutMs ?? REQUEST_TIMEOUT_MS)
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
return null
|
||||
}
|
||||
return (await response.json()) as T
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { cancelTrackingResponse } from '../lib/unread-response-body.test-fixtures'
|
||||
|
||||
const { gitExecFileAsyncMock } = vi.hoisted(() => ({
|
||||
gitExecFileAsyncMock: vi.fn()
|
||||
|
|
@ -301,4 +302,19 @@ describe('Gitea client', () => {
|
|||
})
|
||||
expect(String(fetchMock.mock.calls[0]?.[0])).toBe('https://git.example.com/api/v1/user')
|
||||
})
|
||||
|
||||
it('cancels unread error-response bodies so bundled undici cannot crash on socket close', async () => {
|
||||
let cancelledBodies = 0
|
||||
const fetchMock = vi.fn(async () =>
|
||||
cancelTrackingResponse(502, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
vi.stubGlobal('fetch', fetchMock)
|
||||
|
||||
await getGiteaPullRequestForBranch('/repo', 'refs/heads/feature/gitea')
|
||||
|
||||
expect(fetchMock).toHaveBeenCalled()
|
||||
expect(cancelledBodies).toBe(fetchMock.mock.calls.length)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import {
|
|||
getHostedReviewLocalGitOptions,
|
||||
type HostedReviewExecutionOptions
|
||||
} from '../source-control/hosted-review-git-options'
|
||||
import { cancelUnreadResponseBody } from '../lib/unread-response-body'
|
||||
|
||||
const REQUEST_TIMEOUT_MS = 5000
|
||||
// Why: self-hosted Forgejo can take ~5s to serve one /pulls page (it loads
|
||||
|
|
@ -89,6 +90,7 @@ async function requestJsonAtBase<T>(
|
|||
signal: AbortSignal.timeout(options.timeoutMs ?? REQUEST_TIMEOUT_MS)
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
return null
|
||||
}
|
||||
return (await response.json()) as T
|
||||
|
|
|
|||
|
|
@ -0,0 +1,97 @@
|
|||
import { readdirSync, readFileSync } from 'node:fs'
|
||||
import { join, relative, sep } from 'node:path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
// Global fetch (bare or via globalThis/global — unlike Electron's net.fetch)
|
||||
// goes through Node's bundled undici, which can crash the whole process when
|
||||
// an unread response body pauses the HTTP/1 parser and the peer closes the
|
||||
// socket (nodejs/undici#5360, orca#8695). This applies to every Node process
|
||||
// we ship: Electron main, the CLI, and the SSH relay.
|
||||
//
|
||||
// Each entry below maps an audited file to its expected number of matching
|
||||
// lines. Real call sites must consume or cancel the body on every path,
|
||||
// including !response.ok (see main/lib/unread-response-body.ts). A count
|
||||
// change means a call site was added, removed, or moved: re-audit the file
|
||||
// and update the count.
|
||||
const AUDITED_GLOBAL_FETCH_LINES = new Map<string, number>([
|
||||
// HTTP call sites — body consumed or cancelled on every path, including !ok
|
||||
['main/azure-devops/azure-devops-api-request.ts', 1],
|
||||
['main/bitbucket/client.ts', 1],
|
||||
['main/gitea/client.ts', 1],
|
||||
['main/orca-profiles/profile-cloud-client.ts', 1],
|
||||
['main/orca-profiles/profile-cloud-org-members-client.ts', 1],
|
||||
['main/rate-limits/codex-fetcher.ts', 3],
|
||||
['main/runtime/relay/relay-http-client.ts', 2],
|
||||
['main/source-control/hosted-review-api-request.ts', 1],
|
||||
['main/speech/openai-transcription-client.ts', 1],
|
||||
// fetch appears only inside injected-page script source strings, not as a
|
||||
// call this process makes
|
||||
['main/amp/hook-service.ts', 1],
|
||||
['main/opencode/hook-service.ts', 1],
|
||||
['main/pi/agent-status-extension-source.ts', 1],
|
||||
// local identifiers named `fetch` (git fetch), not HTTP
|
||||
['main/ipc/worktree-remote.ts', 2],
|
||||
['relay/git-handler.ts', 1],
|
||||
// fetch mentioned only in a comment
|
||||
['main/ipc/feedback.ts', 1]
|
||||
])
|
||||
|
||||
// A line is a hit when it calls bare `fetch(` or touches `globalThis.fetch` /
|
||||
// `global.fetch` in any way (call, alias, fallback like `input.fetch ??
|
||||
// globalThis.fetch`). `typeof globalThis.fetch` type annotations are exempt.
|
||||
const GLOBAL_FETCH_LINE = /(^|[^.\w])fetch\(|(?<!typeof )\bglobal(This)?\.fetch\b/
|
||||
|
||||
const SCANNED_ROOTS = ['main', 'cli', 'relay']
|
||||
|
||||
function globalFetchLineCounts(srcRoot: string): Map<string, number> {
|
||||
const counts = new Map<string, number>()
|
||||
for (const root of SCANNED_ROOTS) {
|
||||
for (const entry of readdirSync(join(srcRoot, root), {
|
||||
recursive: true,
|
||||
withFileTypes: true
|
||||
})) {
|
||||
if (!entry.isFile() || !entry.name.endsWith('.ts')) {
|
||||
continue
|
||||
}
|
||||
if (
|
||||
entry.name.endsWith('.test.ts') ||
|
||||
entry.name.endsWith('.test-fixtures.ts') ||
|
||||
entry.name.endsWith('.d.ts')
|
||||
) {
|
||||
continue
|
||||
}
|
||||
const filePath = join(entry.parentPath, entry.name)
|
||||
const content = readFileSync(filePath, 'utf8')
|
||||
if (!GLOBAL_FETCH_LINE.test(content)) {
|
||||
continue
|
||||
}
|
||||
const hits = content.split('\n').filter((line) => GLOBAL_FETCH_LINE.test(line)).length
|
||||
if (hits > 0) {
|
||||
counts.set(relative(srcRoot, filePath).split(sep).join('/'), hits)
|
||||
}
|
||||
}
|
||||
}
|
||||
return counts
|
||||
}
|
||||
|
||||
describe('global fetch call-site audit (main, cli, relay)', () => {
|
||||
it('keeps every global-fetch line audited with its expected count', () => {
|
||||
const found = globalFetchLineCounts(join(__dirname, '..'))
|
||||
|
||||
const drifted = [...found]
|
||||
.filter(([file, count]) => AUDITED_GLOBAL_FETCH_LINES.get(file) !== count)
|
||||
.map(([file, count]) => `${file}: found ${count} line(s)`)
|
||||
.sort()
|
||||
expect(
|
||||
drifted,
|
||||
'Global fetch (bare, globalThis.fetch, or global.fetch) uses undici, ' +
|
||||
'where an unread response body can crash the whole process (orca#8695). ' +
|
||||
'New or moved call sites must either use Electron net.fetch or consume/' +
|
||||
'cancel the response body on ALL paths (cancelUnreadResponseBody in ' +
|
||||
'main/lib/unread-response-body.ts), then update AUDITED_GLOBAL_FETCH_LINES.'
|
||||
).toEqual([])
|
||||
|
||||
const stale = [...AUDITED_GLOBAL_FETCH_LINES.keys()].filter((file) => !found.has(file)).sort()
|
||||
expect(stale, 'Remove audited entries whose global-fetch lines are gone.').toEqual([])
|
||||
})
|
||||
})
|
||||
|
|
@ -0,0 +1,13 @@
|
|||
/** Response whose body reports cancellation, for asserting that error paths
|
||||
* cancel unread bodies (see unread-response-body.ts). */
|
||||
export function cancelTrackingResponse(status: number, onCancel: () => void): Response {
|
||||
const body = new ReadableStream<Uint8Array>({
|
||||
start(controller) {
|
||||
controller.enqueue(new TextEncoder().encode('<html>unread error body</html>'))
|
||||
},
|
||||
cancel() {
|
||||
onCancel()
|
||||
}
|
||||
})
|
||||
return new Response(body, { status })
|
||||
}
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { cancelUnreadResponseBody } from './unread-response-body'
|
||||
import { cancelTrackingResponse } from './unread-response-body.test-fixtures'
|
||||
|
||||
describe('cancelUnreadResponseBody', () => {
|
||||
it('cancels an unread body stream', async () => {
|
||||
let cancelled = false
|
||||
await cancelUnreadResponseBody(
|
||||
cancelTrackingResponse(500, () => {
|
||||
cancelled = true
|
||||
})
|
||||
)
|
||||
expect(cancelled).toBe(true)
|
||||
})
|
||||
|
||||
it('no-ops on a body-less response', async () => {
|
||||
await expect(
|
||||
cancelUnreadResponseBody(new Response(null, { status: 500 }))
|
||||
).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
it('swallows cancellation failures on a locked stream', async () => {
|
||||
const response = cancelTrackingResponse(500, () => {})
|
||||
response.body?.getReader()
|
||||
await expect(cancelUnreadResponseBody(response)).resolves.toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
|
@ -0,0 +1,12 @@
|
|||
/**
|
||||
* Cancel a fetch Response body that no code path will read. Why: leaving it
|
||||
* unread can crash the whole process from inside Node's bundled undici
|
||||
* (nodejs/undici#5360, orca#8695); see global-fetch-call-site-audit.test.ts.
|
||||
*/
|
||||
export async function cancelUnreadResponseBody(response: Response): Promise<void> {
|
||||
try {
|
||||
await response.body?.cancel()
|
||||
} catch {
|
||||
// Cancelling an already-errored, locked, or closed stream is harmless.
|
||||
}
|
||||
}
|
||||
|
|
@ -1,4 +1,5 @@
|
|||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { cancelTrackingResponse } from '../lib/unread-response-body.test-fixtures'
|
||||
import type { OrcaCloudAuthConfig } from './profile-cloud-auth-config'
|
||||
import type { OrcaCloudSession } from './profile-cloud-session-store'
|
||||
import {
|
||||
|
|
@ -248,4 +249,16 @@ describe('Orca cloud client', () => {
|
|||
}
|
||||
})
|
||||
})
|
||||
|
||||
it('cancels the unread error-response body so bundled undici cannot crash on socket close', async () => {
|
||||
let cancelledBodies = 0
|
||||
fetchMock.mockResolvedValue(
|
||||
cancelTrackingResponse(502, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
|
||||
await expect(refreshOrcaCloudCapabilities(config, session)).rejects.toThrow()
|
||||
expect(cancelledBodies).toBe(1)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import type {
|
|||
import type { OrcaCloudAuthConfig } from './profile-cloud-auth-config'
|
||||
import type { OrcaCloudSession } from './profile-cloud-session-store'
|
||||
import type { OrcaCloudSessionExchangeResponse } from './profile-cloud-session-exchange'
|
||||
import { cancelUnreadResponseBody } from '../lib/unread-response-body'
|
||||
|
||||
type ExchangeCodeArgs = {
|
||||
code: string
|
||||
|
|
@ -173,6 +174,7 @@ async function postJson<T>(url: string, body: unknown, accessToken?: string): Pr
|
|||
signal: AbortSignal.timeout(CLOUD_REQUEST_TIMEOUT_MS)
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
throw new OrcaCloudRequestError(response.status)
|
||||
}
|
||||
return (await response.json()) as T
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { join } from 'node:path'
|
||||
import { cancelTrackingResponse } from '../lib/unread-response-body.test-fixtures'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const { childSpawnMock, readFileMock, ptySpawnMock } = vi.hoisted(() => ({
|
||||
|
|
@ -193,4 +194,26 @@ describe('Codex backend rate-limit requests', () => {
|
|||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('cancels the unread error-response body so bundled undici cannot crash on socket close', async () => {
|
||||
readFileMock.mockResolvedValue(
|
||||
JSON.stringify({
|
||||
tokens: { access_token: 'access-token', account_id: 'account-id' }
|
||||
})
|
||||
)
|
||||
let cancelledBodies = 0
|
||||
vi.mocked(fetch).mockResolvedValue(
|
||||
cancelTrackingResponse(429, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
|
||||
await expect(
|
||||
consumeCodexRateLimitResetCredit({
|
||||
codexHomePath: '/managed/codex-home',
|
||||
idempotencyKey: 'redeem-429'
|
||||
})
|
||||
).rejects.toThrow('Codex reset failed: HTTP 429')
|
||||
expect(cancelledBodies).toBe(1)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ import type {
|
|||
import { spawn } from 'node:child_process'
|
||||
import { readFile } from 'node:fs/promises'
|
||||
import { homedir } from 'node:os'
|
||||
import { cancelUnreadResponseBody } from '../lib/unread-response-body'
|
||||
import { join } from 'node:path'
|
||||
import { probeCodexAuthPresence } from './codex-auth-presence'
|
||||
import { resolveCodexCommand } from '../codex-cli/command'
|
||||
|
|
@ -381,6 +382,7 @@ async function fetchBackendRateLimitResetCredits(
|
|||
signal
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
return null
|
||||
}
|
||||
const payload = (await response.json()) as BackendRateLimitResetCreditsResponse
|
||||
|
|
@ -448,6 +450,7 @@ export async function consumeCodexRateLimitResetCredit(options: {
|
|||
}
|
||||
)
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
throw new Error(`Codex reset failed: HTTP ${response.status}`)
|
||||
}
|
||||
const payload = (await response.json()) as BackendConsumeRateLimitResetCreditResponse
|
||||
|
|
@ -531,6 +534,7 @@ async function fetchViaBackend(
|
|||
signal
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
return null
|
||||
}
|
||||
const payload = (await response.json()) as BackendUsageResponse
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { describe, expect, it, vi } from 'vitest'
|
||||
import nacl from 'tweetnacl'
|
||||
import { cancelTrackingResponse } from '../../lib/unread-response-body.test-fixtures'
|
||||
import { exchangeRelayAuthorization, requestRelayAssignment } from './relay-http-client'
|
||||
|
||||
describe('relay HTTP client', () => {
|
||||
|
|
@ -72,4 +73,35 @@ describe('relay HTTP client', () => {
|
|||
})
|
||||
).rejects.toThrow('relay_assignment_failed_502')
|
||||
})
|
||||
|
||||
it('cancels unread error-response bodies so bundled undici cannot crash on socket close', async () => {
|
||||
const keypair = nacl.box.keyPair()
|
||||
let cancelledBodies = 0
|
||||
const fetch = vi.fn<typeof globalThis.fetch>(async () =>
|
||||
cancelTrackingResponse(503, () => {
|
||||
cancelledBodies += 1
|
||||
})
|
||||
)
|
||||
|
||||
await expect(
|
||||
exchangeRelayAuthorization({
|
||||
endpoint: 'https://auth.example/v1/desktop/auth/relay-token',
|
||||
accessToken: 'ordinary-access-token',
|
||||
keypair: {
|
||||
...keypair,
|
||||
publicKeyB64: Buffer.from(keypair.publicKey).toString('base64')
|
||||
},
|
||||
fetch
|
||||
})
|
||||
).rejects.toThrow()
|
||||
await expect(
|
||||
requestRelayAssignment({
|
||||
directorUrl: 'https://relay.example',
|
||||
relayToken: 'scoped-token',
|
||||
relayHostId: 'AbCdEf0123_-xyZ9',
|
||||
fetch
|
||||
})
|
||||
).rejects.toThrow()
|
||||
expect(cancelledBodies).toBe(2)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { createHash } from 'node:crypto'
|
||||
import { z } from 'zod'
|
||||
import type { E2EEKeypair } from '../e2ee-keypair'
|
||||
import { cancelUnreadResponseBody } from '../../lib/unread-response-body'
|
||||
|
||||
const RelayTokenResponseSchema = z
|
||||
.object({
|
||||
|
|
@ -69,6 +70,7 @@ export async function exchangeRelayAuthorization(input: {
|
|||
body: JSON.stringify({ relayHostId, hostPublicKeyB64: input.keypair.publicKeyB64 })
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
throw new RelayHttpError('token-exchange', response.status)
|
||||
}
|
||||
const parsed = RelayTokenResponseSchema.safeParse(await response.json())
|
||||
|
|
@ -96,6 +98,7 @@ export async function requestRelayAssignment(input: {
|
|||
body: JSON.stringify({ v: 1, relayHostId: input.relayHostId })
|
||||
})
|
||||
if (!response.ok) {
|
||||
await cancelUnreadResponseBody(response)
|
||||
throw new RelayHttpError('assignment', response.status)
|
||||
}
|
||||
const parsed = AssignmentResponseSchema.safeParse(await response.json())
|
||||
|
|
|
|||
Loading…
Reference in New Issue