fix(cache): authorize a warm claim by its run, not by its job name - #305
Merged
Conversation
A promoted warm container keeps its provider instance name while the one-job JIT runner takes GitHub's runner name, so the broker proves the pair through the queue journal before handing over cache credentials. It did that with BindRunning, which must resolve exactly one intent because it writes to it, and which separates a run's sibling jobs by comparing the journal's display name against GITHUB_JOB. Those two agree for a bare job and for a reusable-workflow prefix, and disagree for every matrix job: `Analyze (python)` is not `analyze`, and `Backend pytest shard 4/4` is nothing like its id. This fleet is mostly matrix jobs. Measured over one live hour: 7 warm correlations bound, 19 refused. The claim is nonfatal, so those jobs still ran -- uncached, which is the slow path warm capacity exists to remove. Authorization does not need to identify the job. The credential is scoped to a repository, so the question is whether this scale set is serving that repository's run, and (scale set, owner, repository, workflow run id) is a tighter answer than any job-name comparison. AuthorizeRunning asks exactly that and writes nothing; the exact intent is still bound by BindRunning and its asynchronous retry once GitHub reports the job started. An intent admitted before JobAvailable carries no run id yet, and BindRunning already treats that as compatible, so authorization does too -- refusing it would deny warm capacity the head-of-queue case it is for. The refusal was also the only path in this handler that logged no identity at all: the one failure costing every warm runner its cache was the one that could not be read from its own log. It now carries instance, runner, repository, run, job and pool, and the bind error. A foreign repository is still refused and its one-job token is still not consumed; there is a test for that, and one that reproduces the matrix shape from the live journal.
The cache broker takes its version from this manifest -- CONTROLLER_VERSION reads derivative_version and stamps gha-fleet, gha-fleet-observer and gha-cache-broker with it -- so a broker behaviour change that ships under the previous version leaves the live identity unable to say which code is running. That is the divergence this manifest exists to prevent. Two local rebuilds with the workflow's own flags agree.
CodeQL flagged the new refusal log as built from user input, and it is right that the guarantee was not visible there. validateJobCorrelation already refuses CR, LF and NUL through boundedText, so nothing reaching this handler can forge a log line today -- but that invariant lives a hundred lines away in a handler that has grown, and neither a reader nor the analyser can see it from the call. logText makes it local. An ordinary value passes through untouched.
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.
19 warm cache claims refused against 7 bound, in one live hour. Every one of those jobs ran uncached — the slow path warm capacity exists to remove.
A promoted warm container keeps its provider instance name while the one-job JIT runner takes GitHub's runner name, so the broker proves the pair through the queue journal before handing over credentials. It did that with
BindRunning, which must resolve exactly one intent because it writes to it, and which separates a run's sibling jobs by comparing the journal's display name againstGITHUB_JOB.Those agree for a bare job and a reusable-workflow prefix, and disagree for every matrix job. From the live journal, taken while writing this:
The fix. Authorization does not need to identify the job. The credential is scoped to a repository, so the question is whether this scale set is serving that repository's run — and (scale set, owner, repository, workflow run id) is a tighter key than any job-name comparison.
AuthorizeRunningasks exactly that and writes nothing. The exact intent is still bound byBindRunningand its asynchronous retry once GitHub reports the job started.An intent admitted before
JobAvailablecarries no run id yet, andBindRunningalready treats that as compatible, so authorization does too — refusing it would deny warm capacity the head-of-queue case it is for.It is not a weakening.
TestWarmClaimForAnotherRepositoryIsStillRefused: a warm runner presenting a repository this scale set holds no active work for is refused, and its one-job token is not consumed.The refusal is now diagnosable. It was the only path in the handler that logged no identity at all — the one failure costing every warm runner its cache was the one that could not be read from its own log. It now carries instance, runner, repository, run, job, pool and the bind error.
TestWarmClaimIsAuthorizedForAMatrixJobDisplayNamereproduces the live shape with three siblingAnalyze (…)intents; it asserts the claim is authorized and thatqueue running correlation deferredis still logged — which is the proof that the old exact-resolution path genuinely could not resolve this fixture..95becauseCONTROLLER_VERSIONreadsderivative_versionand stampsgha-cache-brokerwith it; a broker change shipping under the previous version leaves the live identity unable to say which code is running.