Skip to content

ci: count only runner groups this repository can actually use - #338

Merged
Eli Pinkerton (wallstop) merged 3 commits into
masterfrom
dev/wallstop/watchdog-runner-group-visibility
Aug 2, 2026
Merged

ci: count only runner groups this repository can actually use#338
Eli Pinkerton (wallstop) merged 3 commits into
masterfrom
dev/wallstop/watchdog-runner-group-visibility

Conversation

@wallstop

@wallstop Eli Pinkerton (wallstop) commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #331, correcting a claim I made in it.

What was wrong

#331 removed the visible_to_repository filter from the watchdog's runner-group
walk, on the stated grounds that "the organization registers a single group
today, so the filter selects exactly what the unfiltered call already returns."

The first production run after that merge disproved it:

Organization runner groups: Default, ambiguous-interactive-organization-builds, Default

Two distinct groups, one repeated by --paginate. The premise was simply wrong,
and the risk I filed as hypothetical in #335 was live: runners in a group this
repository may not be allowed to use were being counted as available capacity.

Why that matters. Inflated capacity is what turns a legitimately queued run
into a wrongful cancel — the audit sees an idle runner carrying the required
labels, concludes the dispatcher is stuck, and cancels a run that was correctly
waiting for a runner it can actually use. No wrongful cancel has occurred: every
post-merge run so far reported No dispatcher-stuck run found. The exposure was
real but unrealised.

The fix

Visibility is resolved through documented shapes rather than the ambiguous
query parameter that started all this:

  • each group's own visibility field — all needs no further call;
  • selected groups are resolved against the group's repositories endpoint and
    compared with github.repository_id.

Numeric ids on both sides, nothing to guess. This is what #335 asked for, minus
the admin:org probe, because the group object already carries visibility.

Groups that exclude us are named in the step summary, and a walk ending with
no visible group fails closed — capacity we cannot read is not capacity that
is zero.

Also dedupes the group list by id. The runner dedupe already made the repeat
harmless for the counts, so this is not a correctness fix; it removes a redundant
API call per duplicate and stops the summary printing Default, …, Default,
which is exactly the kind of line that makes an operator distrust the report.

A harness bug that made the first version of these tests worthless

watchdog_runners numbered runner ids from 1 on every call, and the
inventory is deduplicated by runner id — so two groups' runners collided and the
second group's silently vanished. My multi-group fixture therefore proved
nothing: the case asserting "an invisible group's runner is excluded" passed
identically whether or not the exclusion worked. Ids are now distinct per group,
and only then did the mutations start failing as they should.

Verification

Four mutations, each against a verified-green baseline:

mutation result
visibility filtering removed (the pre-fix behaviour) killed
an invisible group treated as visible killed
group dedupe removed killed
an empty visible set tolerated instead of failing closed killed

New rows: an idle runner in a group we cannot use is not capacity · an idle
runner in a group we can use is capacity · no visible group fails closed · a
group repeated by pagination is walked once.

Refs #335.


Note

High Risk
Changes how the watchdog decides dispatcher-stuck vs legitimately queued runs; over-counting capacity was the path to cancelling runs that were correctly waiting, and this PR is the corrective logic on that automation.

Overview
The stuck-job watchdog no longer treats every organization runner group as capacity for this repo. It filters groups by documented visibility, allows_public_repositories, and (for selected groups) whether github.repository_id appears in the group’s repository list—so idle runners in groups this public repo cannot use are not counted and cannot trigger a wrongful cancel.

Runner groups are deduped by id after pagination (fewer redundant API calls and cleaner summaries). Excluded groups are named in the step summary; if no group is usable, the audit fails closed instead of reporting zero capacity. Inventory logging now says capacity is from groups visible to ${REPO}.

REPO_ID is passed into the audit step. The unity PR policy harness adds REPO_ID, fixes watchdog_runners so multi-group fixtures use distinct runner ids, and adds regression cases for invisible groups, public-repo refusal, private visibility, fail-closed when nothing is visible, and pagination duplicates.

Reviewed by Cursor Bugbot for commit 05c2967. Bugbot is set up for automated code reviews on this repo. Configure here.

#331 removed the `visible_to_repository` filter from the watchdog's inventory
walk, justified by the claim that the organization registered a single runner
group, so filtering selected exactly what the unfiltered call returned. The
first production run after that merge disproved it:

    Organization runner groups: Default, ambiguous-interactive-organization-builds, Default

Two distinct groups, one of them repeated by `--paginate`. The premise was
simply wrong, and the risk written up as hypothetical in #335 was live: runners
in a group this repository may not use were being counted as capacity. Inflated
capacity is what turns a legitimately queued run into a WRONGFUL CANCEL -- the
audit sees an idle runner carrying the right labels, concludes the dispatcher is
stuck, and cancels a run that was correctly waiting.

Visibility is now resolved through documented shapes instead of the ambiguous
query parameter: each group's own `visibility` field, and for `selected` groups
the group's repositories endpoint, compared against `github.repository_id`.
Numeric ids on both sides, nothing to guess. Groups that exclude us are named in
the step summary, and a walk that ends with NO visible group fails closed --
capacity we cannot read is not the same as capacity that is zero.

Also dedupes the group list by id. The runner dedupe already made the repeat
harmless for the COUNTS, so this is not a correctness fix; it removes a
redundant API call per duplicate and stops the summary printing
`Default, ..., Default`, which is exactly the kind of line that makes an
operator distrust the rest of the report.

One harness bug found while writing the tests, and it mattered: `watchdog_runners`
numbered runner ids from 1 on EVERY call, and the inventory is deduplicated by
id, so two groups' runners collided and the second group's silently vanished. A
multi-group fixture proved nothing -- the case asserting an invisible group's
runner is excluded passed identically whether or not the exclusion worked. Ids
are now distinct per group.

Mutations killed: visibility filtering removed (the pre-fix behaviour), an
invisible group treated as visible, group dedupe removed, and an empty visible
set tolerated.

Refs #335.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 1, 2026 22:34
@wallstop

Copy link
Copy Markdown
Collaborator Author

bugbot run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens the stuck-job watchdog’s notion of available self-hosted runner capacity by counting only organization runner groups that are actually usable by this repository (based on runner-group visibility and repository membership), and updates the policy-validation harness to correctly model multi-group runner inventories.

Changes:

  • Add REPO_ID (github.repository_id) to the watchdog environment and use it to filter selected/restricted runner groups via each group’s repositories endpoint.
  • Dedupe paginated runner-group listings by group id to avoid redundant walks and confusing summaries.
  • Fix the test harness’s runner-fixture generator so multi-group scenarios don’t collide on runner ids during dedupe, and add coverage for invisible/visible groups and pagination duplicates.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
scripts/validate-unity-pr-policy.py Adds REPO_ID to the watchdog harness context and fixes runner-fixture id collisions with a first_id offset to make multi-group tests meaningful.
.github/workflows/stuck-job-watchdog.yml Filters runner capacity to groups visible to this repo, dedupes paginated groups by id, and updates step summary wording accordingly.

Comment thread .github/workflows/stuck-job-watchdog.yml Outdated
Comment thread .github/workflows/stuck-job-watchdog.yml
Comment thread .github/workflows/stuck-job-watchdog.yml
Three review findings, all real.

**`allows_public_repositories` was ignored (Cursor Bugbot).** A group can be
`visibility: all` and still refuse PUBLIC repositories, and this repository is
public -- so its runners are unusable capacity, which is precisely the
over-count that cancels a live run. Excluded unconditionally rather than only
when we know we are public: that would need another API call and another failure
mode, and the cost of being wrong is asymmetric. For a private repository this
under-counts, and under-counting can only produce a starvation report, never a
cancel.

Writing the fixture for it found the check inverted. `jq`'s `//` treats an
explicit `false` as absent:

    {"allows_public_repositories": false} | // true   -> true
    {"allows_public_repositories": false} | != false  -> false
    {}                                    | != false  -> true

So `.allows_public_repositories // true` read a group that genuinely refuses
public repositories as allowing them -- the exact inversion of the check, in the
idiom a reader would expect to be correct.

**`--argjson` could abort without failing closed (GitHub Copilot).** An empty or
non-numeric `REPO_ID` makes `--argjson` fail to parse, and under `set -e` that
kills the audit without reaching `fail_closed` -- a job that dies instead of
reporting. Both sides are compared as strings now.

**The excluded-group list did not join with ", " (GitHub Copilot).** For a quoted
`"${arr[*]}"` bash joins with only the FIRST character of IFS, so `IFS=', '`
rendered `a,b`. Measured, then replaced with an explicit join.

Mutations killed: public-repo gate removed, `// true` restored, visibility
filtering removed, an invisible group treated as visible, group dedupe removed,
an empty visible set tolerated, and `--argjson` restored.
Copilot AI review requested due to automatic review settings August 1, 2026 23:13
@wallstop

Copy link
Copy Markdown
Collaborator Author

All three findings were real; fixed in 3d27513e

Bugbot — allows_public_repositories ignored. Confirmed and material: this repository is public (visibility=public), so a group that refuses public repositories cannot serve it, and counting its idle runner is exactly the over-count that cancels a live run.

Excluded unconditionally rather than only when we know we're public — that would need another API call and another failure mode, and the cost of being wrong is asymmetric: for a private repo this under-counts, and under-counting can only produce a starvation report, never a cancel.

Writing the fixture for it found my check inverted. jq's // treats an explicit false as absent:

{"allows_public_repositories": false} | // true   -> true
{"allows_public_repositories": false} | != false  -> false
{}                                    | != false  -> true

So .allows_public_repositories // true read a group that genuinely refuses public repositories as allowing them — the exact inversion of the check, written in the idiom a reader would assume is correct. The test failed on the first run and that is the only reason it was caught.

Copilot — --argjson can abort without failing closed. Right. An empty or non-numeric REPO_ID makes --argjson fail to parse, and under set -e that kills the audit without reaching fail_closed — a job that dies rather than reports. Both sides are compared as strings now.

Copilot — the IFS=', ' join. Right, and measured:

a=(one two three); IFS=", "; echo "${a[*]}"   ->  one,two,three

Quoted ${arr[*]} joins with only the FIRST character of IFS. Replaced with an explicit join.


Mutations killed, each against a verified-green baseline: public-repo gate removed · // true restored · visibility filtering removed · invisible group treated as visible · group dedupe removed · empty visible set tolerated · --argjson restored.

@wallstop

Copy link
Copy Markdown
Collaborator Author

bugbot run

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3d27513. Configure here.

Comment thread .github/workflows/stuck-job-watchdog.yml
…pository

`visibility: private` means "every PRIVATE repository in the organization" --
not a selected list. Only `all` short-circuited, so a `private` group fell
through to the repositories-endpoint branch, which answers a question that does
not apply to it and could fail the whole audit closed on an error it never
needed to make. This repository is public, so such a group can never serve it.
(Cursor Bugbot.)

Excluded directly, the same shape as the `allows_public_repositories` refusal
and for the same asymmetry: under-counting capacity can only produce a
starvation report, while over-counting is what cancels a live run.

Mutation killed: `private` treated as `selected` again.
@wallstop

Copy link
Copy Markdown
Collaborator Author

Bugbot round 2 — right again, fixed

private visibility treated as selected. Confirmed. visibility: private means every private repository in the organization, not a selected list — so only all short-circuiting meant a private group fell through to the repositories-endpoint branch, which answers a question that doesn't apply to it and could fail the whole audit closed on an error it never needed to make. This repository is public, so such a group can never serve it.

Excluded directly now, same shape as the allows_public_repositories refusal and for the same asymmetry: under-counting capacity can only produce a starvation report; over-counting is what cancels a live run.

Mutation killed: private treated as selected again. Copilot reported no new comments on this head.

Copilot AI review requested due to automatic review settings August 2, 2026 00:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/workflows/stuck-job-watchdog.yml:334

  • The comment says both selected and private groups are resolved via the repositories endpoint, but the code below intentionally excludes visibility: private without calling the endpoint. This makes the inline documentation misleading for future edits/debugging.
          # `selected` (and `private`) are resolved against the group's own
          # repositories endpoint, which returns repository objects with numeric
          # ids -- an unambiguous comparison against `github.repository_id`.

@wallstop
Eli Pinkerton (wallstop) enabled auto-merge (squash) August 2, 2026 00:28
@wallstop
Eli Pinkerton (wallstop) merged commit e0cfc4a into master Aug 2, 2026
43 of 61 checks passed
@wallstop
Eli Pinkerton (wallstop) deleted the dev/wallstop/watchdog-runner-group-visibility branch August 2, 2026 01:12
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.

2 participants