fix(#647): the guard counted its own in-flight run - #648
Merged
Conversation
`runConsumedQuota` treats any non-completed run as having spent quota. That is right in general - an in-flight run really may be testing right now - but the guard's OWN run is `in_progress` while its budget job executes, and nothing excluded it. So the guard always counted itself, and the daily cap of 10 behaved as 9: at limit-1 it adds itself, reaches the limit exactly, and refuses. Observed today, with nothing else running in between: 15:59Z 9/10 OK manual --dry-run 16:00Z 10/10 DAY_EXCEEDED the budget job on the run created at 16:00Z 16:20Z 9/10 OK manual, after that run finished all-skipped Job 93123780234 on run 31265873093 logged the 10/10 directly. WHAT IT COST. #644 merged with no E2E behind it. The merge order had been changed specifically so each PR would land covered once the meter was honest (#640/#645); the cycle window was fixed and the day window read 9/10 OK immediately before the merge - then the run blocked itself. The one thing the ordering existed to guarantee is the thing that did not happen. Thread GITHUB_RUN_ID through countRuns and skip that run in both windows. Compare as STRINGS. GITHUB_RUN_ID arrives as a string and the API returns id as a number, so a strict === between them silently excludes nothing and restores the bug. Tightening the comparison fails two tests, on purpose. Absent GITHUB_RUN_ID - local invocation, --dry-run - nothing is excluded, which is why the manual readings were right all along and CI's was not. Third defect in this counter, all the same shape: counting something that is not evidence of quota spent. #640 counted runs the breaker had blocked and a deleted project's runs against the live one; #639 called a blocked run covered; this counts itself. 4 new tests (32 in the file), each mutation-proven: removing the exclusion fails 3, tightening to a strict === fails 2. Closes #647
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.
The finding is in #647. This is the fix.
runConsumedQuotatreats any non-completedrun as having spent quota — right in general,since an in-flight run really may be testing right now. But the guard's own run is
in_progresswhile its budget job executes, and nothing excluded it.So it always counted itself, and the daily cap of 10 behaved as 9: at
limit - 1itadds itself, hits the limit exactly, and refuses.
Observed, with nothing else running in between
--dry-runJob
93123780234on run31265873093logged the 10/10 directly.What it cost
#644 merged with no E2E behind it. The merge order was changed specifically so each PR
would land covered once the meter was honest (#640/#645). The cycle window was fixed and the
day window read 9/10
OKimmediately before the merge — then the run blocked itself. The onething the ordering existed to guarantee is the thing that did not happen.
The fix
Thread
GITHUB_RUN_IDthroughcountRuns, skip that run in both windows.Compare as strings.
GITHUB_RUN_IDarrives as a string, the API returnsidas anumber; a strict
===between them silently excludes nothing and restores the bug. There isa test for exactly that, and tightening the comparison fails two.
Absent
GITHUB_RUN_ID— local invocation,--dry-run— nothing is excluded. That is whythe manual readings above were right all along and CI's was not.
Verification
test:scripts), each mutation-proven:removing the exclusion fails 3, tightening to
===fails 2.total_countand all 4 tests failed onlistRunsbefore reachingthe exclusion. Fixed the fixture, not the code.
Third defect in this counter, all the same shape
Counting something that is not evidence of quota spent:
against the live one
Closes #647