Force pushing risks losing data

[rossjrw]

It looks like this action implements its changes by force-pushing to the target branch:

github-pages-deploy-action/src/git.ts

Lines 220 to 221 in 226f2c4

	await execute(
	`git push --force ${action.repositoryPath} ${temporaryDeploymentBranch}:${action.branch}`,

I believe this works for the vast majority of use cases, but not all of them. In the case where two deployments to different locations on the same branch are initiated at the same time (or perhaps just very close to each other), changes could be destroyed.

Details about my specific use case

Consider a use case where this action is being used to create multiple deployments in different directories in a single branch - for example, a main gh-pages deployment and a preview deployment for a pull request:

branch: gh-pages
/
├─ pr-preview/
│  └─ pr-8/
│     └─ <preview deployment>
└─ <main deployment>

This repository would have one workflow that creates the main deployment on push to the main branch:

# .github/workflows/deploy.yml
name: build
on:
  push:
    branches: [main]
jobs:
  deploy:
    steps:
      - uses: actions/checkout@v2
      - run: npm install && npm run build
      - uses: JamesIves/github-pages-deploy-action@v4
        with:
          branch: gh-pages
          folder: dist
          clean-exclude: pr-preview/

And another that handles preview deployments for pull requests - both setting them up for opened/closed/synchronize events, and removing them on closed events:

# .github/workflows/preview.yml
on:
  pull_request:
    types:
      - opened
      - reopened
      - synchronize
      - closed
jobs:
  deploy-preview:
    if: github.event.pull_request.action != 'closed'
    steps:
      - uses: actions/checkout@v2
      - run: npm install && npm run build
      - uses: JamesIves/github-pages-deploy-action@v4
        with:
          branch: gh-pages
          folder: dist
          target-folder: pr-preview/pr-${{ github.event.pull_request.number }}
  remove-preview:
    if: github.event.pull_request.action == 'closed'
    steps:
      - run: echo "empty_dir=$(mktemp -d)" >> $GITHUB_ENV
      - uses: JamesIves/github-pages-deploy-action@v4
        with:
          branch: gh-pages
          folder: ${{ env.empty_dir }}
          target-folder: pr-preview/pr-${{ github.event.pull_request.number }}

This is where the action's current assumption that all deployments will neatly overwrite the previous deployment breaks down. When a pull request is merged to the main branch, the pull request's closed event and the main branch's push event will fire at the same time. This causes two simultaneous deployments.

Both deployments will pull and secure a reference to the current SHA of the gh-pages branch. They'll prepare their commit, and then push. Normally, one would be successful and the other would fail, but in this case both force-push their changes and one of the commits will be destroyed.

In my case, I merged a pull request, the preview for the pull request was successfully removed, and the main deployment was lost.

I believe this could be resolved by changing how the action pushes its changes. Rather than force-push the deployment, I think it should attempt a regular push then, if that fails, pull the remote and push again just like a human would.

An implementation that does this forever until it pushes successfully could look something like this:

let rejected
do {
  let pushResult = await execute(
    `git push --porcelain ${action.repositoryPath} ${temporaryDeploymentBranch}:${action.branch}`
  )
  rejected = pushResult.includes(`[rejected]`)
  if (rejected) await execute(
    `git pull --rebase --no-edit ${action.repositoryPath} ${action.branch}:${temporaryDeploymentBranch}`
  )
} while (rejected)

Do you think this would work / is a good idea? Have I misdiagnosed the issue; or is there a better way of resolving it?

I'm happy to work on this as a PR - I need this functionality, so I'll be mucking about in a fork regardless. Please do let me know if you have any thoughts.

[JamesIves]

Something similar to this issue was raised in #1000. Would adding a concurrency param to the workflow be an appropriate workaround for this? If not I'd be happy to review a pull request and provide guidance if you'd like. I think this would be a good thing to figure out at the project level if it can be made fail safe.

[rossjrw]

You're right, it looks like this and #1000 refer to the same root issue.

concurrency works great when this action is only being used to handle a single deployment, so it's a good solution for #1000 and for the vast majority of use cases. But concurrency can only handle a queue of length 1 - there can only be a single 'pending' workflow at a time, and any new workflow initiation just cancels an existing pending workflow. If the 3 workflows in that scenario (ongoing, pending and incoming) all target different deployments, one of those deployments will be lost.

In fact if a user knows that they're using this action for more than 2 deployments, they should avoid the concurrency parameter. (Or, better: include some deployment identifier in the concurrency group name, e.g. in my case the PR number)

concurrency docs for reference: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency

I also discussed this here: https://github.community/t/avoiding-conflicts-when-two-workflows-push-to-the-same-branch/233774/3?u=rossjrw

I'll raise a PR in the near future, likely gated behind a flag for now.

[JamesIves]

Awesome. Thanks for the context! I will keep an eye out for the pull request.