-
Notifications
You must be signed in to change notification settings - Fork 490
Preserve HTTP errors from streaming bundle downloads #4138
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,13 @@ import { ActionState } from "./action-common"; | |||||||||||||||||||||
| import { ActionsEnvVars, getEnv, ReadOnlyEnv } from "./environment"; | ||||||||||||||||||||||
| import { formatDuration, Logger } from "./logging"; | ||||||||||||||||||||||
| import * as tar from "./tar"; | ||||||||||||||||||||||
| import { cleanUpPath, getErrorMessage, getRequiredEnvParam } from "./util"; | ||||||||||||||||||||||
| import { | ||||||||||||||||||||||
| asHTTPError, | ||||||||||||||||||||||
| cleanUpPath, | ||||||||||||||||||||||
| getErrorMessage, | ||||||||||||||||||||||
| getRequiredEnvParam, | ||||||||||||||||||||||
| HTTPError, | ||||||||||||||||||||||
| } from "./util"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * High watermark to use when streaming the download and extraction of the CodeQL tools. | ||||||||||||||||||||||
|
|
@@ -88,14 +94,20 @@ export async function downloadAndExtract( | |||||||||||||||||||||
| return { totalDurationMs }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } catch (e) { | ||||||||||||||||||||||
| // If we failed during processing, we want to clean up the destination directory | ||||||||||||||||||||||
| // before we either try again or give up. | ||||||||||||||||||||||
| await cleanUpPath(dest, "CodeQL bundle", logger); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Retrying a 404 is pointless: the asset does not exist, so downloading it a different way | ||||||||||||||||||||||
| // will fail in the same way. | ||||||||||||||||||||||
| if (asHTTPError(e)?.status === 404) { | ||||||||||||||||||||||
| throw e; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| core.warning( | ||||||||||||||||||||||
| `Failed to download and extract CodeQL bundle using streaming with error: ${getErrorMessage(e)}`, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| core.warning(`Falling back to downloading the bundle before extracting.`); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // If we failed during processing, we want to clean up the destination directory | ||||||||||||||||||||||
| // before we try again. | ||||||||||||||||||||||
| await cleanUpPath(dest, "CodeQL bundle", logger); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const toolsDownloadStart = performance.now(); | ||||||||||||||||||||||
|
|
@@ -188,12 +200,15 @@ async function downloadAndExtractZstdWithStreaming( | |||||||||||||||||||||
| }); | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (response.statusCode !== 200) { | ||||||||||||||||||||||
| const statusCode = response.statusCode ?? 0; | ||||||||||||||||||||||
| if (statusCode !== 200) { | ||||||||||||||||||||||
| // Discard the response body so that the connection can be released. | ||||||||||||||||||||||
| response.resume(); | ||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||
| `Failed to download CodeQL bundle from ${codeqlURL}. HTTP status code: ${response.statusCode}.`, | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| let message = `Failed to download CodeQL bundle from ${codeqlURL}.`; | ||||||||||||||||||||||
| if (statusCode !== 0) { | ||||||||||||||||||||||
| message += ` HTTP status code: ${statusCode}.`; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| throw new HTTPError(message, statusCode); | ||||||||||||||||||||||
|
Comment on lines
+207
to
+211
Member
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. Minor: The
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| await tar.extractTarZst(response, dest, tarVersion, logger); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I have seen the previous review comment from Copilot.)
Why default to
0here? Based on the review comment from Copilot, it seems that the justification is so that we don't end up withundefinedin theHTTPErrorbelow, but why even throw aHTTPErrorat all in that case? Could we throw a non-HTTPErrorif we don't have a status code instead?If the
HTTPErroris needed, e.g. because some upstream handler uses it to distinguish between different scenarios, then it would be worth documenting that here (e.g. "We throw aHTTPErroreven if we don't have a status code, because ...") or possibly refactoring so that we can throw a different error type here and still get the desired upstream effect.