-
Notifications
You must be signed in to change notification settings - Fork 734
fix(featureDev): Prevent crash on large repos or unsupported encoding during file collection #4888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "type": "Bug Fix", | ||
| "description": "Fixed a crash when trying to use Q /dev on large projects or projects containing files with unsupported encoding." | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { Uri } from 'vscode' | |
| import { GitIgnoreFilter } from './gitignore' | ||
|
|
||
| import AdmZip from 'adm-zip' | ||
| import { PrepareRepoFailedError } from '../errors' | ||
| import { ContentLengthError, PrepareRepoFailedError } from '../errors' | ||
| import { getLogger } from '../../shared/logger/logger' | ||
| import { maxFileSizeBytes } from '../limits' | ||
| import { createHash } from 'crypto' | ||
|
|
@@ -21,6 +21,7 @@ import { ToolkitError } from '../../shared/errors' | |
| import { AmazonqCreateUpload, Metric } from '../../shared/telemetry/telemetry' | ||
| import { TelemetryHelper } from './telemetryHelper' | ||
| import { sanitizeFilename } from '../../shared/utilities/textUtilities' | ||
| import { maxRepoSizeBytes } from '../constants' | ||
|
|
||
| export function getExcludePattern(additionalPatterns: string[] = []) { | ||
| const globAlwaysExcludedDirs = getGlobDirExcludedPatterns().map(pattern => `**/${pattern}/*`) | ||
|
|
@@ -94,6 +95,7 @@ export async function collectFiles( | |
| return prefix === '' ? path : `${prefix}/${path}` | ||
| } | ||
|
|
||
| let totalSizeBytes = 0 | ||
| for (const rootPath of sourcePaths) { | ||
| const allFiles = await vscode.workspace.findFiles( | ||
| new vscode.RelativePattern(rootPath, '**'), | ||
|
|
@@ -102,29 +104,48 @@ export async function collectFiles( | |
| const files = respectGitIgnore ? await filterOutGitignoredFiles(rootPath, allFiles) : allFiles | ||
|
|
||
| for (const file of files) { | ||
| try { | ||
| const fileContent = await SystemUtilities.readFile(file, new TextDecoder('utf8', { fatal: true })) | ||
| const relativePath = getWorkspaceRelativePath(file.fsPath, { workspaceFolders }) | ||
| const relativePath = getWorkspaceRelativePath(file.fsPath, { workspaceFolders }) | ||
| if (!relativePath) { | ||
| continue | ||
| } | ||
|
|
||
| if (relativePath) { | ||
| storage.push({ | ||
| workspaceFolder: relativePath.workspaceFolder, | ||
| relativeFilePath: relativePath.relativePath, | ||
| fileUri: file, | ||
| fileContent: fileContent, | ||
| zipFilePath: prefixWithFolderPrefix(relativePath.workspaceFolder, relativePath.relativePath), | ||
| }) | ||
| } | ||
| } catch (error) { | ||
| getLogger().debug( | ||
| `featureDev: Failed to read file ${file.fsPath} when collecting repository: ${error}. Skipping the file` | ||
| ) | ||
| const fileStat = await vscode.workspace.fs.stat(file) | ||
| if (totalSizeBytes + fileStat.size > maxRepoSizeBytes) { | ||
| throw new ContentLengthError() | ||
| } | ||
|
|
||
| const fileContent = await readFile(file) | ||
| if (fileContent === undefined) { | ||
| continue | ||
| } | ||
|
|
||
| // Now that we've read the file, increase our usage | ||
| totalSizeBytes += fileStat.size | ||
| storage.push({ | ||
| workspaceFolder: relativePath.workspaceFolder, | ||
| relativeFilePath: relativePath.relativePath, | ||
| fileUri: file, | ||
| fileContent: fileContent, | ||
| zipFilePath: prefixWithFolderPrefix(relativePath.workspaceFolder, relativePath.relativePath), | ||
| }) | ||
| } | ||
| } | ||
| return storage | ||
| } | ||
|
|
||
| const readFile = async (file: vscode.Uri) => { | ||
| try { | ||
| const fileContent = await SystemUtilities.readFile(file, new TextDecoder('utf8', { fatal: false })) | ||
| return fileContent | ||
| } catch (error) { | ||
| getLogger().debug( | ||
| `featureDev: Failed to read file ${file.fsPath} when collecting repository. Skipping the file` | ||
| ) | ||
| } | ||
|
|
||
| return undefined | ||
| } | ||
|
|
||
| const getSha256 = (file: Buffer) => createHash('sha256').update(file).digest('base64') | ||
|
|
||
| /** | ||
|
|
@@ -137,9 +158,9 @@ export async function prepareRepoData( | |
| span: Metric<AmazonqCreateUpload> | ||
| ) { | ||
| try { | ||
| const files = await collectFiles(repoRootPaths, workspaceFolders, true) | ||
| const zip = new AdmZip() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the current approach used to zip files does not "stream" the zip creation so will also run into memory constraints. please look at #4769 , can you rewrite this feature on top of that (after it lands)?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the ETA of this being ready? Presume we want both these in the same wave of the toolkit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I'm happy to change our implementation to use the stream after it's merged.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The zip streaming won't be ready today, but hopefully next week. @ctlai95 |
||
|
|
||
| const files = await collectFiles(repoRootPaths, workspaceFolders, true) | ||
| let totalBytes = 0 | ||
| for (const file of files) { | ||
| const fileSize = (await vscode.workspace.fs.stat(file.fileUri)).size | ||
|
|
@@ -163,6 +184,9 @@ export async function prepareRepoData( | |
| } | ||
| } catch (error) { | ||
| getLogger().debug(`featureDev: Failed to prepare repo: ${error}`) | ||
| if (error instanceof ContentLengthError) { | ||
| throw error | ||
| } | ||
| throw new PrepareRepoFailedError() | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.