fix(planner): match runner labels case-insensitively#243
Merged
Conversation
GitHub injects architecture/OS labels (e.g. "X64", "Linux") with casing that differs from how flavors are defined, but flavor matching used case-sensitive array containment. Jobs requesting "X64" never matched a flavor labelled "x64", so they were left unassigned and contributed no pressure, causing the autoscaler to under-count demand (the jobs still ran on capacity provisioned for other flavors via GitHub's own case-insensitive matching). Normalize labels to lowercase as the canonical stored form: job labels in stripDefaultLabels (which also fixes case-sensitive stripping of the implicit "Linux" label) and flavor labels in AddFlavor. Make the self-hosted eligibility check case-insensitive too. Add migration 0002 to lowercase existing flavor and incomplete-job labels and re-run flavor assignment, recovering the backlog of jobs left unassigned purely because of label casing.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a scaling-accuracy bug by making planner/database runner-label matching effectively case-insensitive via write-time normalization (lowercasing) for both jobs and flavors, plus a data migration to normalize existing rows and re-run flavor assignment for impacted in-flight jobs.
Changes:
- Normalize job labels to lowercase during insertion and strip GitHub implicit labels case-insensitively.
- Normalize flavor labels to lowercase during insertion to keep
@>containment queries/indexes unchanged. - Add migration
0002to lowercase existing labels and re-run assignment for previously-unmatched incomplete jobs; add unit + integration tests covering the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/planner/consumer.go | Makes self-hosted eligibility check case-insensitive (but see review comment about Contains). |
| internal/planner/consumer_test.go | Adds coverage for mixed-case Self-Hosted eligibility. |
| internal/database/database.go | Lowercases job labels on insert via stripDefaultLabels; lowercases flavor labels on insert via lowercaseLabels. |
| internal/database/database_test.go | Adds integration tests validating case-insensitive flavor matching and lowercase flavor label storage. |
| internal/database/labels_test.go | Adds unit tests for stripDefaultLabels lowercasing + case-insensitive stripping. |
| internal/database/migrations/0002_lowercase_labels.up.sql | Normalizes stored labels and re-runs assignment for incomplete jobs (but see review comment: needs to strip implicit labels too). |
| internal/database/migrations/0002_lowercase_labels.down.sql | Documents irreversible normalization as a no-op down migration. |
| docs/changelog.md | Documents the user-facing behavior change and migration. |
The 0002 migration lowercased existing incomplete-job labels but did not strip GitHub's implicit labels (self-hosted, linux). Jobs that stored uppercased implicit labels (e.g. "Linux") under the old case-sensitive strip would keep them after lowercasing and still fail flavor matching, so they were not recovered. Mirror ingestion: lowercase and strip the implicit labels.
golang-migrate runs up-only migrations fine, so the no-op 0002 down file added nothing. Also corrects the 2026 copyright year on the files newly added in this PR.
Adds an integration test that seeds mixed-case flavor and job rows via raw SQL (reproducing how the old case-sensitive code stored them) and runs the exact embedded 0002 migration. Asserts flavor labels are lowercased, incomplete jobs are lowercased/stripped and reassigned, and completed jobs are left untouched as historical data. Closes the gap where the SQL-only backfill had no automated coverage.
The job-label rewrite touched every incomplete job unconditionally. Guard it like the flavor update so it only rewrites rows that have uppercase characters or a still-present implicit label, avoiding needless locks/WAL. Reassignment is a separate statement, so clean rows are still picked up; covered by an added already-normalized test case.
Restores a 0002 down file, but instead of a silent no-op it raises a clear exception explaining the label normalization is irreversible. This makes a down-direction run fail predictably with a meaningful reason rather than an opaque file-not-found, while keeping the pairing symmetric with 0001.
The job-label update intentionally normalizes all incomplete jobs, not
only unassigned ones: a later flavor disable/delete/update resets jobs to
unassigned and re-runs assignment, which re-reads j.labels, so assigned
jobs left mixed-case would re-introduce the casing mismatch. The previous
comment ('jobs still needing a runner') was misleading. Adds a test
covering an assigned, in-progress job.
cbartz
requested review from
florentianayuwono,
javierdelapuente,
weiiwang01,
yanksyoon and
yhaliaw
as code owners
florentianayuwono
approved these changes
Jun 19, 2026
florentianayuwono
left a comment
florentianayuwono
left a comment
Collaborator
There was a problem hiding this comment.
lgtm thank you chris!
12 tasks
cbartz
added a commit
that referenced
this pull request
Jun 25, 2026
* fix(cmd): count only owned jobs in waiting-p80-report Restrict the P80 report to jobs we own by filtering on assigned_flavor IS NOT NULL. The planner ingests webhook events for jobs served by runners we don't manage (e.g. third-party self-hosted 'spread-enabled' runners on repositories we receive webhooks from); those never match a configured flavor, so including them skewed the reported waiting-time P80. Document the pre-2026-06-19 caveat: the case-insensitive label matching fix (#243) only backfilled in-progress jobs, so completed jobs left unassigned due to label casing remain excluded for older date ranges. * docs(cmd): refine waiting-p80-report wording per review Soften the 'exactly when' claim about assigned_flavor (it can be NULL for historical jobs and is cleared on flavor disable/delete), align the changelog to the non-NULL filter, and replace Latin abbreviations per the docs style guide. * docs(cmd): fix waiting-p80-report test/README inconsistencies per review Clarify the e2e seed comment so the 92 figure is clearly the P80 of the 5 base jobs alone (the asserted day-1 P80 is 40 once the clamped negative-wait job joins), and align the README caveat date to the 2026-06-18 changelog entry for the case-insensitive label-matching change. * docs(changelog): note pre-2026-06-18 NULL-flavor caveat for waiting-p80-report Add the same caveat the README carries so a lower sample_count for older date ranges isn't misread as data loss. * docs: fix changelog spell-check failures in waiting-p80-report entry Replace 'repos' with 'repositories' and drop 'undercount' (both flagged by the docs spell check); mirror the wording in the command README for consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes planner flavor matching case-insensitive by storing labels in canonical lowercase:
stripDefaultLabelslowercases job labels and strips GitHub's implicit labels case-insensitively (fixes aLinuxstrip hole).AddFlavorlowercases flavor labels on insert;isSelfHostedcheck is now case-insensitive.0002lowercases existing flavor and incomplete-job labels and re-runs flavor assignment.Why we need it
GitHub injects OS/arch labels with arbitrary casing (e.g.
X64,Linux). Matching used case-sensitive Postgres array containment, so a job requestingX64never matched a flavor labelledx64. Those jobs ran (GitHub's own matching is case-insensitive) but contributed no pressure, so the autoscaler under-counted demand — a scaling-accuracy bug.Test plan
TDD: failing unit + integration tests added first. Full
./internal/...suite passes with-tags=integration -raceagainst PostgreSQL 16.14;gofmt/go vetclean.Review focus / breaking changes
Labels are normalized at the write boundary instead of in hot-path SQL, leaving matching queries and the GIN index unchanged. Flavor labels are now stored (and returned) lowercase — safe since GitHub matching is case-insensitive. Migration
0002is forward-only; its down migration deliberately raises a clear error because the normalization is irreversible.Checklist
CONTRIBUTING.mdhas been updated upon changes to the contribution/development process — n/adocs/changelog.mdwith user-relevant changesterraform fmtpasses andtflintreports no errors — n/a