Skip to content

[ARTIFACT] Reject download promise if timeout was reached - #2124

Merged
danwkennedy merged 4 commits into
actions:mainfrom
akashchi:reject-on-download-failure
Sep 25, 2025
Merged

danwkennedy merged 4 commits into
actions:mainfrom
akashchi:reject-on-download-failure

Conversation

@akashchi

@akashchi akashchi commented Sep 1, 2025

Copy link
Copy Markdown
Contributor

Hi. There is a bug in the actions/download-artifact repository: actions/download-artifact#396. The gist is that the artefact downloading is reported as successful on the download step, but further down, it is revealed that the artefact is corrupt/not present/not fully downloaded. And the downloading step does not report any logs/issues, the only logs are:

Downloading single artifact
Preparing to download the following artifacts:
- fmod-project-build-16795056208 [redacted]
Redirecting to blob download url: [redacted]
Starting download of artifact to: /Users/effortstarbuild/runner/_work/[redacted]
(node:19985) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

We have faced this bug in the openvinotoolkit/openvino repository multiple times, just a couple recent ones:

To investigate, we have added logs to actions/download-artifact and actions/toolkit (specifically, actions/artifact) and found out that the problem might lie in the retry logic of the actions/artifact.

Check the following pipeline:

Attempt 1/5 to download and extract artifact
Initializing HTTP client for artifact download
Making HTTP GET request to blob storage
HTTP response received with status: 200
Creating hash stream for SHA256 verification
Setting up stream pipeline for download and extraction
Piping response message to passThrough stream
Piping passThrough stream to unzip.Extract
(node:17804) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Received 10 data chunks (chunk size: 16384 bytes)
Received 20 data chunks (chunk size: 16384 bytes)
Received 30 data chunks (chunk size: 16384 bytes)
Received 40 data chunks (chunk size: 16384 bytes)
Received 50 data chunks (chunk size: 16384 bytes)
Received 60 data chunks (chunk size: 16384 bytes)
Received 70 data chunks (chunk size: 16384 bytes)
Received 80 data chunks (chunk size: 16384 bytes)
Received 90 data chunks (chunk size: 16384 bytes)
Received 100 data chunks (chunk size: 16384 bytes)
Received 110 data chunks (chunk size: 16384 bytes)
Received 120 data chunks (chunk size: 16384 bytes)
Received 130 data chunks (chunk size: 16384 bytes)
Received 140 data chunks (chunk size: 16384 bytes)
Received 150 data chunks (chunk size: 16384 bytes)
Received 160 data chunks (chunk size: 16384 bytes)
Received 170 data chunks (chunk size: 16384 bytes)
Received 180 data chunks (chunk size: 16384 bytes)
Received 190 data chunks (chunk size: 16384 bytes)
Received 200 data chunks (chunk size: 16384 bytes)
Received 210 data chunks (chunk size: 16384 bytes)
Received 220 data chunks (chunk size: 16384 bytes)
Received 230 data chunks (chunk size: 16384 bytes)
Received 240 data chunks (chunk size: 16384 bytes)
Received 250 data chunks (chunk size: 16384 bytes)
Timeout reached: Blob storage chunk did not respond in 30000ms

So it silently fails after the first attempt without retrying for the second/third/etc. time. The timeout logic is defined in

and it seems like an error from message.destroy is not properly propagated up meaning the retry logic from is not used.

After further investigation, we tried to add reject to the timeout logic suggested in this PR and it seems to be correctly retrying the download if it fails, check the following job and step:

The first attempt fails but the download retries and completes successfully.

The problem is that the first download is running in the background https://github.com/openvinotoolkit/openvino/actions/runs/17206828551/job/48813115664#step:3:2512, so it seems to be the problem with the message.destroy/the download pipe.

This PR is not the final solution but a highlight of the problem and a suggestion for the maintainer to take a look.

Copilot AI review requested due to automatic review settings September 1, 2025 10:33
@akashchi
akashchi requested a review from a team as a code owner September 1, 2025 10:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a bug in the artifact download functionality where timeout errors are not properly propagated, causing the retry logic to fail silently. The change adds a reject call when a timeout occurs during blob storage downloads to ensure the promise is properly rejected and retry mechanisms can function as intended.

Key changes:

  • Added explicit promise rejection when download timeout is reached
  • Ensures timeout errors trigger the retry logic instead of silently failing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

response.message.destroy(
new Error(`Blob storage chunk did not respond in ${timeout}ms`)
)
reject(`Blob storage chunk did not respond in ${timeout}ms`)

Copilot AI Sep 1, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reject call should use an Error object instead of a string for consistency with the error passed to response.message.destroy() on the previous line. Consider using reject(new Error(Blob storage chunk did not respond in ${timeout}ms)) to maintain consistent error handling patterns.

Suggested change
reject(`Blob storage chunk did not respond in ${timeout}ms`)
reject(new Error(`Blob storage chunk did not respond in ${timeout}ms`))

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akashchi this is a worthwhile suggestion. Could you pull the new Error(...) into a variable and pass that to both response.message.destroy and reject?

github-merge-queue Bot pushed a commit to openvinotoolkit/openvino.genai that referenced this pull request Sep 3, 2025
…ed retries logic (#2692)

The gen.ai repo suffers from the same problem as the openvino repo did:
actions/download-artifact#396. My forked
action contains a workaround that enables the retry logic. It was merged
into the OV repo:
openvinotoolkit/openvino#31821, and it seems to
be working.
I've created a PR in the `actions/toolkit` repo:
actions/toolkit#2124, hopefully they will
address it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@danwkennedy

Copy link
Copy Markdown
Contributor

@akashchi looks like the linter is unhappy now 🙂

@akashchi

Copy link
Copy Markdown
Contributor Author

@akashchi looks like the linter is unhappy now 🙂

Fixed. I saw an npm audit failure, do I need to apply the fix?

@danwkennedy

Copy link
Copy Markdown
Contributor

@akashchi no, it's non-blocking and I've pushed a fix to those in a previous commit. Running the CI and then I'll hit approve and merge this.

@danwkennedy danwkennedy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@danwkennedy
danwkennedy merged commit 714f93a into actions:main Sep 25, 2025
17 checks passed
klutchell added a commit to balena-os/balena-yocto-scripts that referenced this pull request Oct 29, 2025
The original issue where promises were not rejected when a download
timeout was reached was fixed by actions/toolkit#2124

Change-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>
klutchell added a commit to balena-os/balena-yocto-scripts that referenced this pull request Oct 30, 2025
The original issue where promises were not rejected when a download
timeout was reached was fixed by actions/toolkit#2124

Change-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>
klutchell added a commit to balena-os/balena-yocto-scripts that referenced this pull request Oct 31, 2025
The original issue where promises were not rejected when a download
timeout was reached was fixed by actions/toolkit#2124

Change-type: patch
Signed-off-by: Kyle Harding <kyle@balena.io>
JohnLeFeng pushed a commit to JohnLeFeng/openvino.genai that referenced this pull request Aug 31, 2026
…ed retries logic (openvinotoolkit#2692)

The gen.ai repo suffers from the same problem as the openvino repo did:
actions/download-artifact#396. My forked
action contains a workaround that enables the retry logic. It was merged
into the OV repo:
openvinotoolkit/openvino#31821, and it seems to
be working.
I've created a PR in the `actions/toolkit` repo:
actions/toolkit#2124, hopefully they will
address it.
giorgiobot pushed a commit to giorgiobot/openperouter that referenced this pull request Sep 21, 2026
The kind node image is a 747MB artifact, and downloading it is the
slowest and most fragile step of every e2e job. When that download
stalls, v4 leaves a truncated tar on disk and still reports the step as
successful, so the failure only surfaces at the next step as a bare

    unexpected EOF

from docker load. The job then dies in setup, before the cluster is
ever deployed, and reads as an e2e failure in the run summary although
no test ran.

It happened in the 2026-09-20 nightly, on the hostmode-boot job. The
download printed "Starting download of artifact to: /tmp" and then
nothing further: no digest line, no completion line, no error and no
warning, yet the step ended after 37s with outcome=success. The seven
sibling jobs in the same run pulled the same artifact and loaded it
fine, and the runner had 103GB free, so the artifact was intact and
only this one transfer stalled.

That silence is the bug fixed in @actions/artifact 4.0.0:

    Fix: reject the promise on timeout
    actions/toolkit#2124

Before it, the download's inactivity timer tore down the response
without settling the promise, so a stalled transfer ended the step
quietly instead of failing it. download-artifact picked the fix up in
v6.0.0, which moved from @actions/artifact ^2.3.2 to ^4.0.0, so v4
never had it. A stall now rejects, is retried, and fails the step if
the retries do not help. v8 additionally defaults digest-mismatch to
'error', which covers an artifact that arrives complete but corrupt.

This composite was the last v4 user left; ci.yaml and nightly.yaml were
already on v8, so this also puts every artifact download on the same
version.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015wWv45Zgtd2yui2WrKDJSc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants