Skipped PR build remains a pending check in GitHub UI

[hugovk]

Details

• Read the Docs project URL: https://readthedocs.org/projects/cpython-previews/
• Build URL (if applicable): https://readthedocs.org/projects/cpython-previews/builds/20835824/
• PR URL: RtD docs previews: Cancel building PRs if no changes in Doc dir python/cpython#104100

Expected Result

When skipping the build with:

if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && [ "$(git diff --quiet origin/main -- Doc/ .readthedocs.yml; echo $?)" -eq 0 ];
then
  echo "No changes to Doc/ - exiting the build.";
  exit 183;
fi

It should either show the GitHub check as a grey skipped result (or possibly green success, but skip would be more accurate).

Actual Result

The build was cancelled as expected:

Details

https://readthedocs.org/projects/cpython-previews/builds/20835824/

However, the status check on GitHub still shows as pending:

Details

And also shows in the tab:

Details

[benjaoming]

Thanks for reporting this 👍 This is likely a regression from #10085.

[benjaoming]

Note: Also reported here - #10335

[benjaoming]

I needed to just understand this very very basic thing that had gotten lost somehow:

• If a build succeeds, we send back a URL to the Pull Request preview!
• If a build is skipped, we also send back a "success" status. But that status should NOT contain the PR preview URL, since the build was skipped 🙃

It could be proposed to clear the whole status (after all, a build never succeeded) - but I think it's nice to keep it, including a URL to the build. That way, it's explicit when a build was skipped. IMHO, we need a more integrated skipping mechanism if we're gonna skip builds and delete the "pending" status.

[benjaoming]

Currently, this is a bit of a mystery since my changes here are skipped and "pending" is overwritten correctly with "success": readthedocs/test-builds#2100

[benjaoming]

This theoretical scenario is AFAICT possible, but I haven't been able to look at data to see if it's actually what happens.

But it could be the sequence of events:

1. Incoming webhook event for push notification: Two separate tasks are added to the Celery queue:
   a) a separate call to send_external_build_status with "pending" (queue: web)
   b) Build is triggered (queue: build)
2. Build encounters the exit code 183 and celery calls UpdateDocsTask.on_failure
3. UpdateDocsTask.on_failure spawns another task that should call send_external_build_status with status "success" (queue: web).

In this scenario, it could be possible that the "success" status is overwritten by "pending" later.

It seems we are having the issue no matter if the exit 183 is added in post_checkout or commands section.

Edit: Updated with queue names

[benjaoming]

Ah! Just noticing.. the cpython build is very slow because the checkout takes a long time. "Build took 89 seconds". So the "pending" status should almost definitely have arrived before the "success" status is sent.

[benjaoming]

(deleted)

[benjaoming]

There is definitely something weird about the difference between these two PRs:

First commit should not be skipped and has a "success", subsequent commits all get a success, even if they are skipped:
readthedocs/test-builds#2100

The first commit should be skipped but gets stuck at "pending" and never receives "success":
readthedocs/test-builds#2102

[benjaoming]

We found the issue ✔️

readthedocs.org/readthedocs/builds/models.py

Lines 379 to 393 in 0957dac

	def get_absolute_url(self):
	"""Get absolute url to the docs of the version."""
	if not self.built and not self.uploaded:
	return reverse(
	'project_version_detail',
	kwargs={
	'project_slug': self.project.slug,
	'version_slug': self.slug,
	},
	)
	external = self.type == EXTERNAL
	return self.project.get_docs_url(
	version_slug=self.slug,
	external=external,
	)

In this method, we return an absolute path for successful builds. But if the build isn't successful, we return a relative path. This is of course an invalid target_url for GitHub's API, so we need to prepend our PRODUCTION_DOMAIN in a solid fashion.

I'll create a PR, just need to check how this get_absolute_url is used to make sure we're not doing something in for instance email templates with a different assumption.

[benjaoming]

@hugovk thanks for taking time to provide good feedback here! The issue is now fixed and will be included in the next deployment (probably next Tuesday).

[hugovk]

And thank you very much for the investigation and fix!

[benjaoming]

I'm still seeing issues here - for example #10393.

Will reopen and investigate...

[benjaoming]

It works => #10410

I was looking at a PR that was opened moments before the fix was deployed.

[hugovk]

Hmm, looking at python/cpython#105510 which has no Doc/ .readthedocs.yml changes, but it still built the docs instead of exiting:

https://readthedocs.org/projects/cpython-previews/builds/20956452/

if [ "$READTHEDOCS_VERSION_TYPE" = "external" ] && [ "$(git diff --quiet origin/main -- Doc/ .readthedocs.yml; echo $?)" -eq 0 ];
then
  echo "No changes to Doc/ - exiting the build.";
  exit 183;
fi

[benjaoming]

Hmm, 2 potential explanations:

1. Something about the command - but I guess you tested this one
2. I wonder if origin/main is from the external PR fork or python/cpython