fix: Refactoring the download process for StaticSiteClient

[shibayan]

Fixed an issue where ETXTBSY and EBUSY errors would occur when executing processes using spawn after the download process ran when StaticSiteClient did not exist.

In this PR, we simplified the close processing and error handling after downloading the file by using the Streams Promises API, and stabilized the download process.

I wrote the following workflow and checked the failure rate of the swa deploy command in the branches before and after the improvement.

jobs:
  deploy:
    runs-on: windows-latest
    steps:
    - uses: actions/checkout@v4
    
    - uses: actions/checkout@v4
      with:
        repository: shibayan/static-web-apps-cli
        ref: deploy-client-download
        path: static-web-apps-cli

    - name: Build SWA CLI
      run: |
        npm install
        npm run build
      working-directory: ./static-web-apps-cli

    - name: Deploy to Static Web App
      run: node ./static-web-apps-cli/dist/cli/bin.js deploy ./dist/ --env production

deploy-new.yml is the workflow that uses the fixes in this PR. I can see that the deployment stability has improved significantly, as the failure rate of the workflow after the fixes is 0%, whereas the failure rate of the workflow before the fixes was over 60%.

Fixes #721
Fixes #799
Fixes #916

[shibayan]

@Timothyw0 @cjk7989 I consider this to be a fairly critical issue, so it would be very helpful if you could review it 🙏 Thanks!

[mohamedfarees]

Based on test result, LGTM, It will be great if this fix goes to live

[dvasdekis]

@Timothyw0 and @cjk7989 are you able to review this pull request? We are an Azure Enterprise customer and this is creating issues for us. Happy to include our account manager if that would expedite it

[cjk7989]

Hi @shibayan, I appreciate your great fix of this issue. Can you share more details on why using

await stp.pipeline(response.body, writableStream);

instead of

const bodyStream = response?.body?.pipe(new PassThrough());
const writableStream = fs.createWriteStream(outputFile, { mode: isPosix ? 0o755 : undefined });
bodyStream?.pipe(writableStream);
writableStream.on("finish", async () => {
...
})

can improve the stability of downloading?

As I understand, stream.pipeline ensures that all stream operations are completed before resolving the Promise. This means that the data is fully written to disk, not just to the system cache, before proceeding. In contrast, manually managing the streams and relying on the finish event might lead to situations where the data is still in the system cache and not fully written to disk, causing issues like ETXTBSY when trying to execute the binary immediately after download.

Thank you!

[cjk7989]

The PR has passed testing in the Linux environment.

[shibayan]

@cjk7989 As you understand it, the main difference is that stp.piepline, instead of using a callback, will certainly do the closing and other processing when the stream copy is complete.

Node's WritableStream is introduced as being closed automatically, but in fact it seems that it must be closed explicitly and committed reliably.

In the case of callbacks, there is a difference between end and finish, and it was not clear at which timing the stream should be closed, but with stp.pipeline, it can be written in an easy-to-understand Promise-based, and since the closing is definitely done at the right time It is clear.

Perhaps this could have been solved by simply adding an explicit stream close process, but we decided that this method was more reliable and simpler to code.

[mohamedfarees]

Hi @cjk7989
Can you release this change let me know when tenatively we are planning to release, as issue still persist adn thhis fix will helpful