fix(test): classify() must survive a probe that answers with a status code - #348
Closed
codeslake wants to merge 1 commit into
Closed
fix(test): classify() must survive a probe that answers with a status code#348codeslake wants to merge 1 commit into
codeslake wants to merge 1 commit into
Conversation
… code
CI went red on Node 22 with `body.startsWith is not a function`, inside "refuses
nothing when the proxy under it dies". Six of this file's seven probes resolve
`ERR:${e.code}` — a string with the prefix classify() tests for. The seventh, at
the forced-kill case, resolves a bare `r.statusCode` on success and a bare
`e.code` on error. Its caller filters out 200 and hands everything else to
classify(), so a 502 arrives as a Number and the case dies where the answer is
simply "that was a reply, not an outage".
The path only opens when a non-200 is actually observed, which is why it survived
every local run and three CI matrices before this one. A unit case pins it now:
a status code classifies as null, an ERR: string still classifies, and a bare
ECONNRESET with no prefix stays null.
Measured: removing the type guard reds that case with the exact CI message.
Not caused by the branch that surfaced it — nothing in that branch touches this
file. Sent separately so it can land on its own.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Folded into #345 as 392369b rather than landing separately. Same five-line guard and the same unit case; keeping it apart would have left #345's CI red on a crash it does not cause, needed a comment to explain that, and fixed the order the two had to merge in. Nothing here is abandoned — the fix is in #345. — Proxy Builder 🤖 Generated with Claude Code |
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 failure
CI red on Node 22, in
refuses nothing when the proxy under it dies:The cause
test/proxy-held-port.test.mjshas seven inline health probes. Six resolve`ERR:${e.code}`— a string carrying the prefixclassify()tests for:The seventh, at the forced-kill case, resolves a bare status code on success and a bare error code on failure:
Its caller filters out 200 and hands everything else straight to
classify():So a 502 arrives as a
Numberandclassify()dies on.startsWith, in a place where the correct answer is simply that was a reply, not an outage.Why it took this long to surface
The path only opens when a non-200 is actually observed during that forced kill. Every local run and three CI matrices had none. That is also why it must be pinned by a unit case rather than left to the integration case that happens to reach it.
The change
plus a unit case fixing the four answers:
502null— a status code is a reply, not an outage200null"ERR:ECONNREFUSED"OUTAGE.REFUSED"ECONNRESET"null— noERR:prefix, not ours to classifyMeasured: removing the type guard reds that unit case with the exact CI message,
body.startsWith is not a function. Full file 35/35 with the guard.Scope
Not caused by the branch that surfaced it — that branch touches neither this file nor
classify(). Sent on its own so it can land independently.Left alone deliberately: the seventh probe still resolves a different shape from the other six. Normalising it to
`ERR:${e.code}`would change what the forced-kill case'sseen.filter((c) => c !== 200)compares against, and that comparison is load-bearing for the assertion below it. The guard fixes the crash without moving that.— Proxy Builder
🤖 Generated with Claude Code