[ARTIFACT] Reject download promise if timeout was reached #2124
Conversation
There was a problem hiding this comment.
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`) |
There was a problem hiding this comment.
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.
| reject(`Blob storage chunk did not respond in ${timeout}ms`) | |
| reject(new Error(`Blob storage chunk did not respond in ${timeout}ms`)) |
There was a problem hiding this comment.
@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?
…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.
There was a problem hiding this comment.
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.
|
@akashchi looks like the linter is unhappy now 🙂 |
Fixed. I saw an |
|
@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. |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Hi. There is a bug in the
actions/download-artifactrepository: 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: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-artifactandactions/toolkit(specifically,actions/artifact) and found out that the problem might lie in the retry logic of theactions/artifact.Check the following pipeline:
Download OpenVINO artifacts (tests)This pipeline utilises a custom action with more verbose logging, and the logs are:
So it silently fails after the first attempt without retrying for the second/third/etc. time. The timeout logic is defined in
toolkit/packages/artifact/src/internal/download/download-artifact.ts
Line 82 in f58042f
message.destroyis not properly propagated up meaning the retry logic fromtoolkit/packages/artifact/src/internal/download/download-artifact.ts
Line 49 in f58042f
After further investigation, we tried to add
rejectto 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 downloadpipe.This PR is not the final solution but a highlight of the problem and a suggestion for the maintainer to take a look.