From d6918c7a1d3603d64b4c99b81631d2d64f2fd612 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 28 Oct 2026 02:01:16 -0700 Subject: [PATCH] fix(remote-files): cap search replies, say when a diff is too large, and measure a 100 MB read on WSL (#34882) Follow-ups from the large-file audit (E7): - files.search had no byte budget: a result past the remote reply cap failed outright with reply_too_large. It now keeps the whole files that fit and marks the result truncated, as files.listAll and searchPaths already do. Local callers stay uncapped. - A diff side past the 10 MiB read cap came back as binary, so the diff said "Binary file changed" for a large text file. Blob reads mark it `tooLarge`, the binary diff result carries an optional `tooLarge`, and both diff views say "File too large to diff". Older clients ignore the field and keep the binary view. buildDiffResult builds its binary sides without a cast. - The real-WSL lane pages a 100 MB file over the plain distro's shared control socket in the editor's 384 KiB chunks and records the time, chunk count and orcad's VmHWM. Record-only: it asserts nothing, and a failure is recorded in the receipt. --- src/main/git/source-control/branch-diff.ts | 3 +- src/main/git/source-control/commit-diff.ts | 7 +- src/main/git/source-control/diff-result.ts | 15 +++-- .../diff-too-large-real-git.test.ts | 64 +++++++++++++++++++ src/main/git/source-control/file-diff.ts | 6 +- src/main/git/source-control/git-blob-read.ts | 15 ++++- src/main/git/source-control/submodule-diff.ts | 5 +- src/main/runtime/rpc/methods/files.ts | 30 +++++---- .../wsl/wsl-orcad-real-distro-lane.test.ts | 53 +++++++++++++++ .../src/components/editor/DiffSectionBody.tsx | 5 +- .../components/editor/DiffTooLargeNotice.tsx | 21 ++++++ .../editor/EditorDiffFileSurface.tsx | 4 ++ src/renderer/src/i18n/locales/en.json | 4 ++ src/shared/git-diff-compare-types.ts | 2 + .../search-result-transport-budget.test.ts | 37 +++++++++++ src/shared/search-result-transport-budget.ts | 31 +++++++++ 16 files changed, 274 insertions(+), 28 deletions(-) create mode 100644 src/main/git/source-control/diff-too-large-real-git.test.ts create mode 100644 src/renderer/src/components/editor/DiffTooLargeNotice.tsx create mode 100644 src/shared/search-result-transport-budget.test.ts create mode 100644 src/shared/search-result-transport-budget.ts diff --git a/src/main/git/source-control/branch-diff.ts b/src/main/git/source-control/branch-diff.ts index 65c866d42ad..b9a5e25aa02 100644 --- a/src/main/git/source-control/branch-diff.ts +++ b/src/main/git/source-control/branch-diff.ts @@ -53,7 +53,8 @@ async function loadBranchDiff( rightBlob.content, leftBlob.isBinary, rightBlob.isBinary, - args.filePath + args.filePath, + leftBlob.tooLarge === true || rightBlob.tooLarge === true ) } catch { return { diff --git a/src/main/git/source-control/commit-diff.ts b/src/main/git/source-control/commit-diff.ts index 32d60fdb5b6..15e9bec66c7 100644 --- a/src/main/git/source-control/commit-diff.ts +++ b/src/main/git/source-control/commit-diff.ts @@ -4,7 +4,7 @@ import type { GitRuntimeOptions } from '../git-runtime-options' import { gitRuntimeOptionsKey } from './git-runtime-options-cache-key' import { gitDiffReadDedupe } from './git-read-cache-invalidation' import { buildDiffResult } from './diff-result' -import { readGitBlobAtOidPath } from './git-blob-read' +import { readGitBlobAtOidPath, type GitBlobReadResult } from './git-blob-read' export async function getCommitDiff( worktreePath: string, @@ -46,7 +46,7 @@ async function loadCommitDiff( const [leftBlob, rightBlob] = await Promise.all([ args.parentOid ? readGitBlobAtOidPath(worktreePath, args.parentOid, leftPath, options) - : Promise.resolve({ content: '', isBinary: false }), + : Promise.resolve({ content: '', isBinary: false, exists: false }), readGitBlobAtOidPath(worktreePath, args.commitOid, args.filePath, options) ]) @@ -55,7 +55,8 @@ async function loadCommitDiff( rightBlob.content, leftBlob.isBinary, rightBlob.isBinary, - args.filePath + args.filePath, + leftBlob.tooLarge === true || rightBlob.tooLarge === true ) } catch { return { diff --git a/src/main/git/source-control/diff-result.ts b/src/main/git/source-control/diff-result.ts index 745ed277a72..7e74335db6e 100644 --- a/src/main/git/source-control/diff-result.ts +++ b/src/main/git/source-control/diff-result.ts @@ -8,21 +8,26 @@ export function buildDiffResult( modifiedContent: string, originalIsBinary: boolean, modifiedIsBinary: boolean, - filePath?: string + filePath?: string, + /** A side was over the read cap rather than proven binary. */ + tooLarge = false ): GitDiffResult { if (originalIsBinary || modifiedIsBinary) { const mimeType = filePath ? PREVIEWABLE_BINARY_MIME_TYPES[path.extname(filePath).toLowerCase()] : undefined + const sides = originalIsBinary + ? { originalIsBinary: true as const, modifiedIsBinary } + : { originalIsBinary: false, modifiedIsBinary: true as const } return { kind: 'binary', originalContent, modifiedContent, - originalIsBinary, - modifiedIsBinary, + ...sides, // Why: renderer still checks legacy `isImage` before previewing, so set it for PDFs too until the contract is renamed. - ...(mimeType ? { isImage: true, mimeType } : {}) - } as GitDiffResult + ...(mimeType ? { isImage: true, mimeType } : {}), + ...(tooLarge ? { tooLarge: true } : {}) + } } // Why: over the render limit, return metadata instead of huge text so the renderer can show fallback UI. diff --git a/src/main/git/source-control/diff-too-large-real-git.test.ts b/src/main/git/source-control/diff-too-large-real-git.test.ts new file mode 100644 index 00000000000..346ec67d76a --- /dev/null +++ b/src/main/git/source-control/diff-too-large-real-git.test.ts @@ -0,0 +1,64 @@ +import { rm, writeFile } from 'node:fs/promises' +import { execFileSync } from 'node:child_process' +import { mkdtempSync } from 'node:fs' +import { tmpdir } from 'node:os' +import * as path from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { getDiff } from './file-diff' +import { MAX_GIT_SHOW_BYTES } from './git-show-max-bytes' +import { invalidateGitReadCaches } from './git-read-cache-invalidation' + +function git(dir: string, args: string[]): void { + execFileSync('git', args, { cwd: dir, stdio: 'pipe' }) +} + +// Text, not binary: only its size keeps it out of a text diff. +const largeText = (): string => 'a line of text\n'.repeat(Math.ceil(MAX_GIT_SHOW_BYTES / 15) + 1) + +describe('a diff side past the read cap', () => { + let repo: string + + beforeEach(() => { + repo = mkdtempSync(path.join(tmpdir(), 'orca-diff-too-large-')) + git(repo, ['init']) + git(repo, ['config', 'user.email', 'test@example.com']) + git(repo, ['config', 'user.name', 'Test User']) + invalidateGitReadCaches() + }) + + afterEach(async () => { + invalidateGitReadCaches() + await rm(repo, { recursive: true, force: true }) + }) + + it('is marked too large, still binary for older readers, from the working tree and from Git', async () => { + await writeFile(path.join(repo, 'big.txt'), 'small\n') + git(repo, ['add', '.']) + git(repo, ['commit', '-m', 'initial']) + await writeFile(path.join(repo, 'big.txt'), largeText()) + + expect(await getDiff(repo, 'big.txt', false)).toMatchObject({ + kind: 'binary', + modifiedIsBinary: true, + tooLarge: true + }) + git(repo, ['add', 'big.txt']) + invalidateGitReadCaches() + expect(await getDiff(repo, 'big.txt', true)).toMatchObject({ + kind: 'binary', + modifiedIsBinary: true, + tooLarge: true + }) + }) + + it('leaves a real binary unmarked', async () => { + await writeFile(path.join(repo, 'blob.bin'), Buffer.from([1, 0, 2])) + git(repo, ['add', '.']) + git(repo, ['commit', '-m', 'initial']) + await writeFile(path.join(repo, 'blob.bin'), Buffer.from([3, 0, 4])) + + const result = await getDiff(repo, 'blob.bin', false) + expect(result).toMatchObject({ kind: 'binary', modifiedIsBinary: true }) + expect(result).not.toHaveProperty('tooLarge') + }) +}) diff --git a/src/main/git/source-control/file-diff.ts b/src/main/git/source-control/file-diff.ts index b28a6fc4f5a..846dc964be1 100644 --- a/src/main/git/source-control/file-diff.ts +++ b/src/main/git/source-control/file-diff.ts @@ -141,6 +141,7 @@ async function loadDiff( let modifiedIsBinary = false let modifiedDeleted = false let readFailed = false + let tooLarge = false try { if (staged) { @@ -155,6 +156,7 @@ async function loadDiff( modifiedIsBinary = rightBlob.isBinary modifiedDeleted = !rightBlob.exists && !rightBlob.failed readFailed = leftBlob.failed === true || rightBlob.failed === true + tooLarge = leftBlob.tooLarge === true || rightBlob.tooLarge === true } else { // The index-to-HEAD fallback is sequential internally, but independent of the working-tree read. // WSL Git can use a Linux cwd while Node needs its Win32 spelling. @@ -173,6 +175,7 @@ async function loadDiff( modifiedIsBinary = workingTreeBlob.isBinary modifiedDeleted = !workingTreeBlob.exists readFailed = leftBlob.failed === true || workingTreeBlob.failed === true + tooLarge = leftBlob.tooLarge === true || workingTreeBlob.tooLarge === true } } catch { readFailed = true @@ -183,7 +186,8 @@ async function loadDiff( modifiedContent, originalIsBinary, modifiedIsBinary, - filePath + filePath, + tooLarge ) // Mark proven deletions so previewers can distinguish them from failed empty reads. if (result.kind === 'binary' && modifiedDeleted) { diff --git a/src/main/git/source-control/git-blob-read.ts b/src/main/git/source-control/git-blob-read.ts index 5d3d158e47a..488c655ab41 100644 --- a/src/main/git/source-control/git-blob-read.ts +++ b/src/main/git/source-control/git-blob-read.ts @@ -15,6 +15,15 @@ export type GitBlobReadResult = { exists: boolean /** Unknown presence: never cache a diff whose empty side came from an incomplete read. */ failed?: boolean + /** Past MAX_GIT_SHOW_BYTES: left unread and reported binary, so older diff readers stay safe. */ + tooLarge?: boolean +} + +const TOO_LARGE_BLOB: GitBlobReadResult = { + content: '', + isBinary: true, + exists: true, + tooLarge: true } export async function readUnstagedLeftBlob( @@ -46,7 +55,7 @@ export async function readGitBlobAtIndexPath( return { ...bufferToBlob(stdout, filePath), exists: true } } catch (error) { if (isMaxBufferOverflowError(error)) { - return { content: '', isBinary: true, exists: true } + return TOO_LARGE_BLOB } return { content: '', @@ -77,7 +86,7 @@ export async function readGitBlobAtOidPath( return { ...bufferToBlob(stdout, filePath), exists: true } } catch (error) { if (isMaxBufferOverflowError(error)) { - return { content: '', isBinary: true, exists: true } + return TOO_LARGE_BLOB } return { content: '', @@ -102,7 +111,7 @@ export async function readWorkingTreeFile(filePath: string): Promise MAX_GIT_SHOW_BYTES) { // Mirror the Git buffer cap so filesystem reads cannot load huge assets. - return { content: '', isBinary: true, exists: true } + return TOO_LARGE_BLOB } try { const buffer = await readFile(filePath) diff --git a/src/main/git/source-control/submodule-diff.ts b/src/main/git/source-control/submodule-diff.ts index d016cbe748e..cead8baaf06 100644 --- a/src/main/git/source-control/submodule-diff.ts +++ b/src/main/git/source-control/submodule-diff.ts @@ -54,6 +54,7 @@ export async function buildSubmoduleInnerCommitRangeDiff( let modifiedContent = '' let originalIsBinary = false let modifiedIsBinary = false + let tooLarge = false try { const left = await readGitBlobAtOidPath(submoduleWorktreePath, fromOid, innerPath, options) originalContent = left.content @@ -61,12 +62,14 @@ export async function buildSubmoduleInnerCommitRangeDiff( const right = await readGitBlobAtOidPath(submoduleWorktreePath, toOid, innerPath, options) modifiedContent = right.content modifiedIsBinary = right.isBinary + tooLarge = left.tooLarge === true || right.tooLarge === true } catch {} return buildDiffResult( originalContent, modifiedContent, originalIsBinary, modifiedIsBinary, - innerPath + innerPath, + tooLarge ) } diff --git a/src/main/runtime/rpc/methods/files.ts b/src/main/runtime/rpc/methods/files.ts index 50636a347ca..347d0aaa606 100644 --- a/src/main/runtime/rpc/methods/files.ts +++ b/src/main/runtime/rpc/methods/files.ts @@ -26,6 +26,7 @@ import { ResolveTerminalPath, ServerDirectoryBrowse } from '../../../../shared/rpc-contract/files-params' +import { limitSearchResultToContentBudget } from '../../../../shared/search-result-transport-budget' let filesWatchSubscriptionSeq = 0 @@ -174,19 +175,22 @@ export const FILE_METHODS = [ name: 'files.search', permission: 'workspace', params: FileSearch, - handler: async (params, { runtime, signal }) => - runtime.searchRuntimeFiles( - params.worktree, - { - query: params.query, - caseSensitive: params.caseSensitive, - wholeWord: params.wholeWord, - useRegex: params.useRegex, - includePattern: params.includePattern, - excludePattern: params.excludePattern, - maxResults: params.maxResults - }, - { signal } + handler: async (params, { runtime, clientKind, requestId, signal }) => + limitSearchResultToContentBudget( + await runtime.searchRuntimeFiles( + params.worktree, + { + query: params.query, + caseSensitive: params.caseSensitive, + wholeWord: params.wholeWord, + useRegex: params.useRegex, + includePattern: params.includePattern, + excludePattern: params.excludePattern, + maxResults: params.maxResults + }, + { signal } + ), + remoteFileContentBudget(clientKind, requestId) ) }), defineMethod({ diff --git a/src/main/wsl/wsl-orcad-real-distro-lane.test.ts b/src/main/wsl/wsl-orcad-real-distro-lane.test.ts index 4aedfef174b..d12c46da62c 100644 --- a/src/main/wsl/wsl-orcad-real-distro-lane.test.ts +++ b/src/main/wsl/wsl-orcad-real-distro-lane.test.ts @@ -486,6 +486,56 @@ async function updatePhase(distro: string): Promise { return result } +const LARGE_FILE_BYTES = 100 * 1024 * 1024 +// The editor's chunk size (runtime-paged-file-read.ts). +const LARGE_FILE_CHUNK_BYTES = 384 * 1024 + +/** + * Record-only (E7): a 100 MB file paged over the host the way the editor pages one, on the shared + * control socket the editor uses, with orcad's peak RSS. Failures are recorded, never asserted. + */ +async function measureLargeFileRead( + control: RemoteRuntimeSharedControlConnection, + orcadPid: number | null, + distro: string +): Promise { + if (orcadPid === null) { + return { error: 'no orcad pid' } + } + try { + const repo = ( + await guest( + distro, + `set -e; d="${LANE_REPO}"; head -c ${LARGE_FILE_BYTES} /dev/zero | tr '\\0' a > "$d/big.txt"; ` + + 'printf %s "$d"' + ) + ).trim() + // Reset the peak, so VmHWM is this read's. + await asRoot(distro, `echo 5 > /proc/${orcadPid}/clear_refs`) + const started = performance.now() + let chunks = 0 + for (let offset = 0; offset < LARGE_FILE_BYTES; offset += LARGE_FILE_CHUNK_BYTES) { + await control.request( + 'files.readChunk', + { + worktree: `path:${repo}`, + relativePath: 'big.txt', + offset, + length: LARGE_FILE_CHUNK_BYTES + }, + 60_000 + ) + chunks += 1 + } + const readMs = Math.round(performance.now() - started) + const peak = (await guest(distro, `grep VmHWM /proc/${orcadPid}/status`)).trim() + await guest(distro, `rm -f "${repo}/big.txt"`) + return { bytes: LARGE_FILE_BYTES, chunks, readMs, orcadVmHWM: peak } + } catch (error) { + return { error: error instanceof Error ? error.message : String(error) } + } +} + async function launchPhase(distro: string, systemd: boolean): Promise { const result: Receipt = {} visibleWork = true @@ -524,6 +574,9 @@ async function launchPhase(distro: string, systemd: boolean): Promise { clientCapabilities: ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES }) await control.request('status.get', undefined, 30_000) + if (!systemd) { + result.largeFileRead = await measureLargeFileRead(control, connection.pid, distro) + } result.watchFiresOnNormalCloses = await countWatchFiresAcrossRpcs(connection, 10) expect(result.watchFiresOnNormalCloses, 'normal socket closes must not read as orcad exit').toBe( 0 diff --git a/src/renderer/src/components/editor/DiffSectionBody.tsx b/src/renderer/src/components/editor/DiffSectionBody.tsx index dbab300f3c4..b4cb33f9d9b 100644 --- a/src/renderer/src/components/editor/DiffSectionBody.tsx +++ b/src/renderer/src/components/editor/DiffSectionBody.tsx @@ -1,3 +1,4 @@ +import { DiffTooLargeNotice } from './DiffTooLargeNotice' import { useEffect, useLayoutEffect, useRef } from 'react' import type { editor } from 'monaco-editor' import { lazyWithRetry as lazy } from '@/lib/lazy-with-retry' @@ -152,7 +153,9 @@ export function DiffSectionBody({ ) : section.diffResult?.kind === 'binary' ? ( - section.diffResult.isImage ? ( + section.diffResult.tooLarge ? ( + + ) : section.diffResult.isImage ? ( +
+
+ {translate('editor.diffTooLarge.title', 'File too large to diff')} +
+
+ {translate( + 'editor.diffTooLarge.detail', + 'One version of this file is over 10 MB, so Orca doesn’t load it for a text diff.' + )} +
+
+ + ) +} diff --git a/src/renderer/src/components/editor/EditorDiffFileSurface.tsx b/src/renderer/src/components/editor/EditorDiffFileSurface.tsx index 7200285636d..d42331a6782 100644 --- a/src/renderer/src/components/editor/EditorDiffFileSurface.tsx +++ b/src/renderer/src/components/editor/EditorDiffFileSurface.tsx @@ -1,3 +1,4 @@ +import { DiffTooLargeNotice } from './DiffTooLargeNotice' import { translate } from '@/i18n/i18n' import type { OpenFile } from '@/store/slices/editor' import type { GitDiffResult } from '../../../../shared/git-diff-compare-types' @@ -54,6 +55,9 @@ export function EditorDiffFileSurface({ const isEditable = activeFile.diffSource === 'unstaged' if (diffContent.kind === 'binary') { + if (diffContent.tooLarge) { + return + } if (diffContent.isImage) { return ( { + it('leaves a result that fits, or an uncapped local one, as it is', () => { + const small = result([file(1, 10)]) + expect(limitSearchResultToContentBudget(small, 64 * 1024)).toBe(small) + const large = result(Array.from({ length: 200 }, (_, index) => file(index, 400))) + expect(limitSearchResultToContentBudget(large, undefined)).toBe(large) + }) + + it('keeps whole files that fit the reply and marks the rest truncated', () => { + const large = result(Array.from({ length: 200 }, (_, index) => file(index, 400))) + const maxBytes = 16 * 1024 + const limited = limitSearchResultToContentBudget(large, maxBytes) + + expect(limited.truncated).toBe(true) + expect(limited.files.length).toBeGreaterThan(0) + expect(limited.files.length).toBeLessThan(200) + expect(limited.files).toEqual(large.files.slice(0, limited.files.length)) + expect(limited.totalMatches).toBe(limited.files.length) + expect(Buffer.byteLength(JSON.stringify(limited), 'utf8')).toBeLessThanOrEqual(maxBytes) + }) +}) diff --git a/src/shared/search-result-transport-budget.ts b/src/shared/search-result-transport-budget.ts new file mode 100644 index 00000000000..8f2d79b69bc --- /dev/null +++ b/src/shared/search-result-transport-budget.ts @@ -0,0 +1,31 @@ +import type { SearchFileResult, SearchResult } from './code-search-types' +import { remoteRpcResultExceedsContentBudget } from './remote-rpc-content-budget' + +/** + * `result` cut to whole files that fit one remote reply, marked truncated: a search past the reply + * cap otherwise fails outright. `maxBytes === undefined` means uncapped (local callers). + */ +export function limitSearchResultToContentBudget( + result: SearchResult, + maxBytes: number | undefined +): SearchResult { + if (maxBytes === undefined || !remoteRpcResultExceedsContentBudget(result, maxBytes)) { + return result + } + const files: SearchFileResult[] = [] + // `{"files":[],"totalMatches":N,"truncated":true}` with room for any count. + let bytes = 64 + for (const file of result.files) { + const fileBytes = Buffer.byteLength(JSON.stringify(file), 'utf8') + 1 + if (bytes + fileBytes > maxBytes) { + break + } + files.push(file) + bytes += fileBytes + } + const totalMatches = files.reduce( + (sum, file) => sum + (file.matchCount ?? file.matches.length), + 0 + ) + return { files, totalMatches, truncated: true } +}