Two comments, one function, no agreement
Our Play release adapter has one function whose job is to merge an incoming release into a track that already has some. It has two rules, and they are next to each other in the source.
The first rule is about completed releases. A track may not carry two of them, so a newer completed release supersedes the older one. The comment above it explains why this matters even though it sounds obvious:
// ...androidpublisher rejects a
// tracks.update carrying two with 400 releasesTooManyCompletedReleases —
// which classifies as retryable, so every promotion after a product's first
// would burn its whole River budget on a condition that never clears.
The second rule is about drafts, added a few weeks later after a real build died. It explains the same kind of failure:
// Play answers 400 INVALID_ARGUMENT "Only one draft release is allowed.",
// which classifies PERMANENT — and with auto_submit
// off an unpromoted draft is the normal resting state, so every automatic
// build after the first fails for good rather than retrying into success.
One says Play's 400 classifies as retryable. The other says Play's 400 classifies as permanent. They are sixteen lines apart in the same function, about the same vendor, about the same HTTP status.
The second one is wrong, and the wrongness is not subtle.
What our own code does with a 400
Permanence is not a property of the error. In this system it is a decision, made by a predicate, and the predicate is right there:
func (e *HTTPError) IsPermanent() bool {
if e.IsEditUnavailable() {
return false
}
return e.StatusCode >= 400 && e.StatusCode < 500 &&
e.StatusCode != http.StatusBadRequest && // 400, explicitly
e.StatusCode != http.StatusUnauthorized &&
e.StatusCode != http.StatusForbidden &&
...
}
400 is excluded by name. So are 401, 403, 408, 409 and 429. Everything else in the 4xx range is permanent; those six are not. A test in the same package says it plainly: permanence forgives 400 because several of them clear on their own.
And that is not a bug in the predicate. The comment above it gives the whole argument, and the argument is good:
// 400/401/403 are excluded because permanence here is not free: the publish
// handler turns it into FailUpload, and a terminally failed candidate needs
// a whole new build. Google returns 403 for rateLimitExceeded/quotaExceeded
// as readily as for a genuine permission problem, and the token endpoint
// answers 400 invalid_grant for host clock skew and for a key that has not
// finished propagating — all of which clear on their own in minutes.
The reasoning: marking an error permanent is not a label, it is a decision to throw away the build. You cannot make that decision for every 400, because some 400s are transient, and mistaking a transient 400 for a fatal one costs you a rebuild and a version number. So 400 gets forgiveness by default, and the cost of being wrong is spent on retry budget rather than on the artifact.
That is a considered trade-off, made for a good reason, written down where the decision lives.
What the error actually did
So the draft release did not classify as permanent. It classified as retryable, which is worse in one specific way and better in none.
Worse: a client-state violation cannot clear. Retrying a request that will always be refused is not resilience, it is a loop that spends the budget and then fails anyway. The completed-release comment says exactly this, and it is right: every promotion after a product's first one would burn its whole retry budget on a condition that never clears. The draft case has the same shape. Not permanently broken, but forever broken.
Not better in any way: the forgiveness for 400 does not help here. It helps when a 400 is transient. This one is not transient. Forgiveness here just converts an instant failure into a slow one.
The actual fix
The fix does not touch the predicate, and that is the right call. Reclassifying 400 as permanent would be worse: it would break the cases the forgiveness exists for, clock skew and key propagation, in order to fix a case that should never have been emitted in the first place.
Instead, the merge now enforces the draft rule the way it enforced the completed rule:
- The function reads the track, merges the incoming release into it, and writes the whole track back. It never created a second draft and never deleted anything; it read, merged, and wrote a track that had ended up carrying two drafts because the merge step had no rule for that case.
- The rule: when the incoming release is a draft and an existing release is also a draft, the newer supersedes the older. A draft serves nobody, so nothing ships or un-ships either way, and what remains on the track is the build a human would actually want to promote.
- Grouped drafts travel with their target. If Play grouped the draft with other version codes for device coverage, those siblings are carried over rather than left behind as a second draft, and rather than dropped, which would silently shrink the device coverage CI had built.
- The same reasoning the completed rule uses for its siblings applies here, for the same reason: detaching a target and discarding the rest is not the same as superseding it.
Two tests were added, and the third was corrected. The corrected one is the interesting one.
The test that was wrong
An existing test asserted what happened when a grouped release was split by an incoming draft: that the leftover version codes ended up as their own release on the track. That is, the test asserted that after the operation there were two drafts.
That is the exact state Play refuses with a 400. The test encoded an illegal state as the expected outcome, and it had been green the entire time, because it was faithfully testing what the code did. The test did not lie; the code did, and the test agreed with it.
This is what a fake does when it models the wrong thing. The fake allowed any set of releases to be written to a track. Play does not. So every test that used the fake was testing a system more permissive than the real one, and the tests were correct about a program that cannot exist.
The corrected expectation is not a small edit. It asserts the length of the resulting release list and the contents of the target's version codes, which is a different claim from the one before it. The old test was about detachment. The new one is about the track ending in a state Play accepts.
The comment that outlived the fix
There is a detail in that commit that belongs in a story about lying comments, because it is the same species.
The function's own doc comment, at the top, still reads:
// mergeTrackRelease upserts targetVersionCode, keeping the staged, halted and
// draft releases already on the track — but never a second completed one.
After the fix, that is false. Draft releases are not kept; the newest one supersedes the others. The comment describes the pre-fix behaviour, the fix edited the body of the function, and the doc comment was left describing the bug.
The file has not been touched since. The comment is still there.
A comment is not verified by anything. It compiles in every build, it survives every refactor, and it is read by exactly one person: whoever is debugging at the time.
There is no test that can catch a stale comment, no linter in the default set that will fail on it, and no review step that reliably does. The only defence is writing the comment as a claim that someone will notice is false, and the only mechanism that helps is proximity: a comment about a rule should be next to the rule, not in a header written once and never revisited.
What this is really about
Two things went wrong, and they are worth separating because they have different fixes.
A fact about the world got into a comment and was never checked. Play's 400 classification is not a property of the code; it is a property of the error-handling policy, which lives in a different function, a different file, and was written for different reasons. The comment reported a conclusion without a pointer to the thing it was a conclusion about. A comment that says "this classifies as permanent" is only as good as the reader's willingness to go check, and it reads as settled fact.
A comment described one case as if it were all cases. The false statement in the draft rule was not invented; it was the completed rule's phrasing, reused for a different case. "Drafts coexist legally" was lifted from a context where the desired release was completed, and it does not hold when the desired release is itself a draft. Copying a comment from a neighbouring block is how a wrong claim gets into a file with a confident voice.
The general shape: a comment is a claim about a fact that lives somewhere else, with nothing connecting it to that fact, and it has the authority of source code while having none of its checking. The fix is not to write fewer comments. It is to make the claim point at its owner: name the function, name the predicate, name the file. A reader who can follow the pointer can check it. A reader who can only read the sentence has to trust it.
Takeaways
In one function, two comments classified the same vendor's 400 in opposite ways, sixteen lines apart. One was correct and one was not, and the way to know which is to read the predicate that actually decides, which is in a different file and was written for a different reason.
Excluding 400 from permanence is a trade, not an oversight: marking an error permanent means discarding the build, and some 400s really are transient. The trade is sound. The comment that reports the consequence as the opposite is not.
The consequence is still a bad outcome. A state violation that can never clear, retried until the budget is spent, is a slow failure rather than a fast one. The fix stops emitting the state instead of classifying the error better, which is stronger: a rule that never produces the condition does not depend on the classifier being right.
A test can encode an illegal state as its expected value and stay green for as long as the code agrees with it. The test here asserted that a track would end up carrying two drafts, which is precisely what Play refuses. It was faithfully testing the code, and the code was wrong.
And a doc comment at the top of a function can be wrong while the fix that made it wrong sits fifteen lines below it. Nothing compiles that fact, nothing tests it, and nothing will ever notice. Comments are claims about facts that live elsewhere, and the only thing that makes one checkable is a pointer to where the fact is.