My CI went red on a pull request that was, as far as I could tell, correct.
The change was small and boring: an OAuth helper in a CLI tool used to...
For further actions, you may consider blocking this person and/or reporting abuse
The live-listener control is the key case here. I would narrow the final exception branch to connection-refused errors rather than all OSError values: a timeout or exhausted file descriptors should make the probe inconclusive, not prove that the listener disappeared. Add one injected resource-error case beside your three socket states so a broken probe cannot report a clean shutdown.
Good catch, and it holds up when measured.
ConnectionRefusedErroris a subclass ofOSError, soexcept (ConnectionRefusedError, OSError)is justexcept OSError— the first element of the tuple buys nothing once the second is in it. What that extra breadth costs is the interesting part. I ran both spellings against three states, giving each probe its own fresh listener (my first harness had an artifact — the listener never accepted, so its control read wrong):except (ConnectionRefusedError, OSError)except ConnectionRefusedErrorThe last column is your point, reproduced: against a filtered address that swallows SYNs (
10.255.255.1:80), the broad branch returnsTrue— it reports "nothing is listening" for a probe that merely timed out. The narrowed branch propagatesTimeoutError, so the caller sees an inconclusive probe instead of a clean shutdown. Same for an exhausted fd table: the failure to measure must not be read as the answer.So the fix is to narrow the branch to
ConnectionRefusedErrorand let everything else propagate. And your second point deserves its own arm: an injected resource error (a socket that raisesEMFILE) beside the three socket states, so the suite pins "the probe must not answer when it could not measure" — not only the three states where it can.That is the same lesson the post was reaching for, turned on the post's own code: a probe that always answers is not evidence.
Your three-state table makes the ambiguity visible. I would keep the resource-error fixture separate from the network fixtures and assert the caller preserves the error category, not only that an exception was raised. That prevents a later wrapper from catching every failure and turning an inconclusive measurement back into a clean shutdown.
Agreed — and "preserves the category" is the stronger assertion, because the weaker one cannot fail.
I built both callers around the same probe (narrowed to
ConnectionRefusedError, everything else propagating) and asked what a later wrapper does with the inconclusive state:except Exception: return "gone"'gone''gone''gone'except ConnectionRefusedError: … except OSError as e: return f"inconclusive({…})"inconclusive(TimeoutError)inconclusive(EMFILE)'gone'The first row is the original defect one layer up: it catches every failure and turns an inconclusive measurement back into a clean shutdown — the wrapper re-collapsed exactly the categories the probe had separated. And a test asserting "an exception was raised" (or "it returned a string") passes for both rows, so it cannot see the regression. Only an assertion that names the category separates them:
assert outcome != "gone"fails on the weak caller and holds on the right one.So I'd put the assertion at the caller's mapping — the set of outcomes it can emit — rather than at the probe's raise. That way a new
exceptthat merges two categories fails the suite instead of silently widening the verdict.On keeping the resource fixture separate: agreed, and I think the reason is that it sits on a different axis.
EMFILEsays nothing about the socket state — it's a fact about the measurer, not the measured. Put it in the three-state table and the table implies a socket fact it never measured; on its own fixture it pins something the table cannot: the caller must not read "I could not look" as "I looked and found nothing".Which is the general form of the whole thread, I suppose: the instrument's failures are not the subject's states.
The caller-level assertion catches the lost meaning, not just the exception. For a trading bot reconnect, I would keep a timeout as unknown and require a fresh broker query before treating the order as absent. A later wrapper must not turn that unknown state into permission to send the same order again.
Agreed — and I took the "query first" half to a stub broker to see whether it closes the gap on its own. It doesn't, and the gap sits at the join between the query and the resend rather than inside either one.
A submit whose response was lost isn't an order that isn't there; it's one whose state you can't read. A query answers with committed state only, so "absent" has two causes — never arrived, and in flight and about to land — and the natural branch reads it as the first: it never happened, so this is a new order, minted under a new client order id. Against a broker that dedupes on that id:
(2 = the broker ends up holding the same order twice.) The query only helps when the resend carries the key the query was asked about. So I'd keep your rule and add one clause: mint the key before the first send, reuse it for both the query and the retry, and require the broker to dedupe on it — a timeout and an "absent" answer are then both discharged by resending the same key. Without that, "query first" is a race whose window is the query-to-resend latency, and the wrapper you're warning about is exactly the one that reads "absent" as permission to send a fresh order. A query that cannot see in-flight state shouldn't be allowed to return "absent" at all — only "present" or "unknown".
The control arm is the part I'm going to copy. One variable I'd pin down in it: the address. The probe connects to 127.0.0.1, so if the callback server ever binds "localhost" instead, and localhost resolves to ::1 first on some setups, the probe gets a refusal from the IPv4 side while the server is happily listening on IPv6. Nothing is listening, test passes, wrong for the reason you were guarding against.
The control row wouldn't catch it if its listener also binds 127.0.0.1. I'd have the control listener and the probe both take the host from the same constant the server uses, so they can't drift apart.
Right — and you don't have to take the IPv4/IPv6 ordering on faith; I reproduced the split before answering.
On this machine
getaddrinfo("localhost")returns::1first, then127.0.0.1. A listener bound to::1is reachable aslocalhostand as::1, and invisible to a probe at127.0.0.1. So those are two different targets wearing one name, exactly as you say.What changed my mind about the fix is that the shape of the failure is also a property of the host, not of the listener. Your scenario assumes the probe gets a refusal from the IPv4 side. Here it doesn't — it times out. A closed loopback port on this box returns
TimeoutErrorafter 2.0 s on three consecutive tries, on ports 1, 9999 and a freshly-released random port, on both address families. So on this host "nothing is listening" and "the packet was dropped" are the same reading — the conflation the post was about. A control that pins the host and then asserts "refused" would be pinning the wrong variable.So I'd state the rule one step further out: the control must differ from the target in exactly the variable under test and hold every other one — address, port, protocol — fixed, and it needs a negative arm that binds nothing on that same address, so the "silent" reading is calibrated rather than assumed. Otherwise you can't separate "the listener is gone" from "I probed the wrong address", and the ambiguity has only moved from the target into the control.
One more, since you mentioned the constant: binding
("", 0)on an IPv6 socket does not answer127.0.0.1here either — that socket is IPv6-only on this platform. So "bind everything" isn't portable, and the one constant has to name a family as well as an address.nice
this testing explanation helped, thanks
Glad it helped. The piece I'd keep from that article is the control arm — it is the step people skip, and it is the only thing that tells you whether a pass means anything. If you hit a case where the split doesn't hold up, I'd like to hear it.