PR review companion seems flaky?

[schalkneethling]

On this build the PR review companion keep failing with the following stack:

Run actions/github-script@v5
  with:
    script: var artifacts = await github.rest.actions.listWorkflowRunArtifacts({
     owner: context.repo.owner,
     repo: context.repo.repo,
     run_id: 1480759121,
  });
  var matchArtifacts = artifacts.data.artifacts.filter((artifact) => {
    return artifact.name == "build"
  });
  if (matchArtifacts.length === 0) {
    console.warn(
      'No artifacts to download probably just means nothing ' +
      'was built in the "PR test" workflow. That\'s OK. ' +
      'This is actually not a genuine CI error.'
    );
    throw new Error(
      'No matched build artifacts. ' +
      'Perhaps nothing built in the "PR test" workflow'
    );
  }
  var matchArtifact = matchArtifacts[0];
  var download = await github.rest.actions.downloadArtifact({
      owner: context.repo.owner,
      repo: context.repo.repo,
      artifact_id: matchArtifact.id,
      archive_format: 'zip',
  });
  var fs = require('fs');
  fs.writeFileSync('/home/runner/work/content/content/build.zip', Buffer.from(download.data));
  
    github-token: ***
    debug: false
    user-agent: actions/github-script
    result-encoding: json
(node:1562) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
No artifacts to download probably just means nothing was built in the "PR test" workflow. That's OK. This is actually not a genuine CI error.
Error: No matched build artifacts. Perhaps nothing built in the "PR test" workflow
    at eval (eval at callAsyncFunction (/home/runner/work/_actions/actions/github-script/v5/dist/index.js:4942:56), <anonymous>:17:9)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async main (/home/runner/work/_actions/actions/github-script/v5/dist/index.js:4997:20)
Error: Unhandled error: Error: No matched build artifacts. Perhaps nothing built in the "PR test" workflow

We recently changes github.actions.listWorkflowRunArtifacts to github.rest.actions.listWorkflowRunArtifacts because of the breaking change in the underlying GitHub action. The same change was made by @SphinxKnight on translated-content. It looks like the error is not related to the change, but based on the fact that there was no artefacts. However, there is this console.warn in the code:

console.warn(
      'No artifacts to download probably just means nothing ' +
      'was built in the "PR test" workflow. That\'s OK. ' +
      'This is actually not a genuine CI error.'
    );

There is also this run that did not error. We will need to have a look at what the actual cause is and resolve the issue.

[wbamberg]

FWIW I'm also not seeing built pages for mdn/content#10646 (the first time it worked, but after review the author made some changes, and those I am not seeing).

[schalkneethling]

I have not kept an eye on this one. @wbamberg, @SphinxKnight, or @ddbeck, is this still behaving inconsistently?

[SphinxKnight]

From https://github.com/mdn/translated-content/actions/workflows/pr-review-companion.yml?query=is%3Afailure I can see https://github.com/mdn/translated-content/actions/runs/1831126327 / https://github.com/mdn/translated-content/runs/5171601783?check_suite_focus=true and https://github.com/mdn/translated-content/runs/5176772124?check_suite_focus=true "recently".
Most of the time it goes without issues though.

I'll be back later with more time to have a deeper look at those failures.

Edit: seems indeed pretty frequent for mdn/content as well : https://github.com/mdn/content/actions/workflows/pr-review-companion.yml?query=is%3Afailure

[ddbeck]

Yes, I think the PR review companion workflow is prone to failure. But because of the way the PR companion is wired up, it's not very obvious when it has failed, why it has failed, or what PR it failed for.

For reasons unknown to me, it's triggered by the completion of the PR Test workflow completion, instead of being a second (dependent) job in the same workflow as the PR Test job. This creates a few problems:

• Companion reviews are invisible. Since it's triggered by another workflow, the companion doesn't show up in the "Checks" tab for any particular PR. Here's an example, where the companion ran, but you have no indication of such in the "Checks" tab; you only get a comment on the PR.
• Failures are difficult to investigate. There's no way to see, in the list of Actions runs, which PR a companion run is associated with (the # is the workflow run count, not a PR number). Sometimes I have received emails about companion failures long enough after opening a PR that I have no idea which PR or commit caused it or why.
• Failures are difficult to recover from. For example, if the PR companion gets stuck, you can't re-run it from the "Checks" tab. I think the only way to re-run the PR companion is to push a new commit, though I've never investigated this closely.

I think the first step would be to combine the PR Test and PR review companion workflows, with two dependent jobs. That will probably give us some better information on why the companion is misbehaving.

(There's also some other suspect stuff in those workflows, but I've been reluctant to mess with them since I'd have no way of knowing if or why they'd fail.)

[awxiaoxian2020]

I still have this problem. See https://github.com/mdn/translated-content/actions/runs/2896760814

[caugner]

I quickly looked into this and the "PR review companion" workflow seems to fail if the "PR Test" did not upload any artifact, especially ...

1. if the "PR Test" workflow failed, or
2. if the "PR Test" ran on a PR that didn't change any page.

Example for 1:

• Among the recent failures of the PR review companion workflow is this failed job.
• In the "Download artifact" step, it mentions run 3471794992, which is in the context of adding docs for WebTransport content#22310.
• That job failed on commit 45c419a (merge commit simulated by GitHub), because The document's slug (...) doesn't match its disk folder name (...).
• Because the "PR Test" workflow failed, the "PR review companion" workflow couldn't found the artifact.

Example for 2:

• The occurrence mentioned by @awxiaoxian2020, i.e. "PR review companion" run 2896760814 mentions run 2896757290.
• That run succeeded BUT does not (seem to) have any artifact.
• 💡 Probably the reason is that it didn't change any index.md, so it didn't build any page.

[caugner]

To fix case 1, we could run the "PR review companion" workflow conditionally only if the "PR Test" workflow run concluded with "success":

if: ${{ github.event.workflow_run.conclusion == 'success' }}
Source: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-based-on-the-conclusion-of-another-workflow

🤔 However, it should already do that, see: https://github.com/mdn/content/blame/d987faaf1b65fbc11323327d82ce13a004cb96d4/.github/workflows/pr-review-companion.yml#L18-L20

To fix case 2, we could modify the "PR Test" workflow to ...

• conditionally run only if anything inside files/ was changed, and/or
• always upload an artifact (and optionally suppress the warning).

[caugner]

Decreased priority, because it's just annoying, not really a problem that the workflow fails.

[bsmth]

I think it's safe to close this for now as there's been multiple refactoring efforts completed on these workflows; a significant one being the following that's landed in content and translated content:

• refactor(ci/pr-review): use workflow_call instead of workflow_run event (v2) content#30064
• refactor(ci/pr-review): use workflow_call instead of workflow_run event translated-content#16891

A few things that add stability:

• Replace actions/github-script@v5 that handled artifacts with hand-written logic with actions/download-artifact@v3
• Make PR companion dependent on Tests: https://github.com/mdn/content/blob/a320ca3ba41cde9b385f249a1519194f20c35d5c/.github/workflows/pr-test.yml#L148-L152

And there are a number of other small but important fixes:

• fix(ci): pr-test.yml throws error for non .md files content#30286
• fix(ci): do not paginate for fetching file changes content#22898

Related issues:

• [ci] PR Review Companion should be called iff there is diffed content from PR Test content#28579

[caugner]

Closing as suggested by Brian in #4972 (comment).