Skip to content

Skip the external PDS suite when those services are unreachable (wave 29) - #139

Closed
aaronstevenwhite wants to merge 2 commits into
fix/trending-batching-wave-28from
fix/external-deps-wave-29
Closed

Skip the external PDS suite when those services are unreachable (wave 29)#139
aaronstevenwhite wants to merge 2 commits into
fix/trending-batching-wave-28from
fix/external-deps-wave-29

Conversation

@aaronstevenwhite

Copy link
Copy Markdown
Contributor

Summary

Wave 29 closes CI-7: a third-party outage red-lines this repository.

tests/pre-deployment/script-execution.test.ts talks to a real user PDS and the governance PDS, and the Pre-Deployment Verification job that runs it is a required check on both staging and main. So while bsky.social is unreachable, no pull request can merge — however unrelated the change.

This is not hypothetical. The GitHub Actions outage on 2026-08-26 blocked this repository's entire queue for roughly two hours, and the same shape applies to any provider the pre-deployment suite depends on.

The file's own docblock stated the intent — "These tests MUST pass. They do NOT skip" — so this was a deliberate choice, and changing it is a change of policy rather than a bug fix.

The distinction drawn

Unreachable versus wrong:

  • DNS failure, refused connection, timeout, or a 5xx — nothing to learn, so the suite skips and merges are not gated on someone else's availability.
  • A service that responds but responds incorrectly — a genuine integration failure, so the tests run and fail exactly as before.

The skip is deliberately loud. It names the endpoint that failed and prints "This is not a pass: nothing about PDS integration was checked in this run." A skipped run must never be mistakable for a verified one.

Related Issues

Backlog 0.8.0: CI-7. Stacked on #138 (wave 28).

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update

How Has This Been Tested?

9 tests pinning the policy: both endpoints are probed, a 5xx counts as unreachable, the probe is bounded by a timeout so a hang cannot stall the job, the suite is gated on the probe, the skip prints its reason, it states that nothing was verified, and the docblock no longer claims the suite never skips.

I verified describe.skipIf actually behaves as expected in Vitest 4 with a throwaway probe test — skipping the group while the file still passes — rather than assuming the API does what its name suggests.

Full unit suite: 205 files, 4,468 tests passing. Typecheck and lint clean.

Checklist

General

  • I have performed a self-review of my code
  • Code follows style guide (npm run lint passes)
  • Tests added/updated for changes
  • All new and existing tests pass (npm test)
  • Documentation updated (if applicable)

ATProto Compliance (required for data flow changes)

  • N/A — test policy only
  • No writes to user PDSes
  • BlobRef storage only (never blob data)
  • Indexes can be rebuilt from firehose
  • PDS source is tracked for staleness detection

Breaking Changes

  • N/A — no breaking changes
  • Migration path documented

Note for review

This weakens a guarantee, and that is the point of the trade — but it is worth being explicit. A deploy can now proceed having verified nothing about PDS integration, if the PDSes happened to be unreachable when CI ran. The mitigation is that the skip is loud in the job log rather than silent, so the state is discoverable. If that trade is unacceptable, the alternative is to keep the job blocking and accept that third-party availability gates merges; I do not think that is the right default, but it is a legitimate position.

…wave-29

# Conflicts:
#	.github/workflows/security.yml
#	tests/unit/api/handlers/xrpc/trending-paging.test.ts
#	tests/unit/config/security-scanning.test.ts
@aaronstevenwhite

Copy link
Copy Markdown
Contributor Author

Superseded by #140, which carried waves 16-30 to staging in one piece after I discovered these had been merging into each other's feature branches rather than into staging. The content of this PR is included there and is now on staging; closing to avoid implying it landed separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant