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 discover a free port by binding it, reading the number, and closing the socket — which leaves a window in which any other process can take the port before the callback server binds it. The fix binds once and hands the already-bound socket to the HTTP server, so there is no window. I measured it on my machine: with the old helper another process could take the port; with the new one it could not. I added a test, ran it, and pushed.
Then CI came back: Test 3.12, 3.13, Canary — all failed, on the Linux legs, while the identical suite was green on Windows on the same commit. The one failing test was the one I had just written:
OSError: [Errno 98] Address already in use
809 passed, 1 failed
The fix was fine. The test I shipped with the fix was not.
The test that broke
The test asked a simple question: after the OAuth flow finishes, is the redirect port released? And it answered it in the most obvious way — by trying to bind the port again:
def _port_is_free(port: int) -> bool:
with socket.socket() as s:
try:
s.bind(("127.0.0.1", port))
return True
except OSError:
return False
If a fresh listener can bind, the port is free. That is the intuitive phrasing. It is also the wrong instrument, and the reason is entirely about which states of a TCP connection a rebind is willing to forgive.
Attempt one: blaming TIME_WAIT
My first hypothesis was the classic one: the server has closed, but the connection sits in TIME_WAIT, and TIME_WAIT blocks a rebind. The standard fix for that is SO_REUSEADDR, so I set it on the probe socket and pushed again.
CI went red again — with a new signal that killed the hypothesis. The probe now returned False even with SO_REUSEADDR set. If TIME_WAIT were the cause, SO_REUSEADDR is exactly what would have forgiven it. It didn't, so the port was not merely in TIME_WAIT.
The tell was sitting right next to it. There were two tests: one where the flow completes (the failing one) and one where it does not. The sibling, which also rebinds the port, passed. The only difference between them was that in the failing test a client had actually connected and received a response. That points at a lingering accepted connection — a socket in FIN_WAIT_2 or ESTABLISHED — and SO_REUSEADDR deliberately does not forgive those. It forgives TIME_WAIT because a timed-out connection is genuinely gone from the application's point of view; it does not forgive a live connection because that would let two listeners fight over the same 4-tuple.
So the failure had two independent halves:
- the probe used an instrument (
bind) that is sensitive to connection state I did not control; - the test's fake client never closed its response, so a connection really was left hanging — a genuine leak in the test itself, the same kind of leak the production fix was about.
Attempt two: change the question, not the port
The fix was to stop asking "can something bind here" and start asking "is anything still listening here":
def _nothing_is_listening(port: int) -> bool:
with socket.socket() as s:
try:
s.connect(("127.0.0.1", port))
return False # something answered
except (ConnectionRefusedError, OSError):
return True # nothing there
A connect() that is refused tells you there is no listener — and it does not care what state some old connection is in, because it never touches that connection's 4-tuple. That is the property I actually wanted to assert: "the server is gone", not "the kernel will let me rebind". I also closed the response in the fake client, removing the leak underneath.
CI returned 5/5 green.
Proving the probe isn't vacuous
A negative assertion has a failure mode that a positive one does not: it can be vacuously true. A probe that always says "nothing is listening" would pass the test perfectly and tell you nothing. So I added a control arm that runs the same probe against three states:
| state | _nothing_is_listening |
|---|---|
| a listener is up | False |
| listener just closed, cleanly | True |
| a connection lingers, no listener | True |
The first row is the one that matters: the probe can return False, so the True in the test means something. Without that row, "nothing is listening" is an unfalsifiable claim.
What generalises
Three things, and none of them are about OAuth or Python.
A fix's own test is part of the fix. The pull request was not "the socket handoff"; it was "the socket handoff and a test that verifies it on every platform the suite runs on". A green local run on one OS is a sample, not a verdict — and when a CI leg fails where you did not look, the bug is often in the instrument, not the code.
For a negative assertion, choose an instrument that is insensitive to the state you do not control. "The port is released" is a claim about the server being gone. bind-succeeds smuggles in an assumption about kernel connection bookkeeping; connect-refused does not. Pick the phrasing that says only what you mean.
Give every negative probe a control arm. A probe that cannot fail is not evidence. One extra case — the state in which it must return the other answer — is what turns "the test passed" into "the property held".
The second attempt was smaller than the first and took less time to write. The expensive part was not the fix; it was noticing that the instrument, not the code, was the thing that needed changing.
Top comments (21)
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".
You are right: my fresh-query condition was incomplete. Your delayed-arrival case shows why. I would create and persist the order key before the first send, then reuse it for the query and any retry, provided the broker guarantees deduplication on that key. Without that guarantee, an absent snapshot leaves the outcome unknown and must not authorize another send. That delayed first arrival belongs in the caller test too.
Agreed, and the clause you added is the load-bearing one — I named it as a single property and it is two. "The broker guarantees deduplication on that key" hides where it remembers the key, and the two answers are not variations of one thing:
(2 = the book holds the same order twice.) A broker that remembers the key at receipt dedupes against a submit that is still in flight; one that remembers it at commit only dedupes once the original has landed.
The part I hadn't seen: the case where you retry is the case the weaker guarantee fails in. The query reads committed state, so it can only answer "present" (the original landed) or "absent" (it hasn't, or it never will). "Present" means no retry — so the strong guarantee is never exercised where it would be needed. "Absent" means retry — and that is exactly the window in which commit-time dedup cannot protect you. The decision to retry and the unsafe interleaving are the same event, which is why an absent snapshot must not authorize a send even when the broker "has dedup".
And it is not establishable by inspection, which is your "provided" made concrete. A probe that submits the key twice with both commits allowed to complete passes a commit-time broker — it is true about a state the dangerous path never runs in. Only a probe that leaves the first order in flight separates them, and on a real broker you cannot open that window on demand without placing an order. So the checklist item is not "does the broker dedupe" but "at which point does it remember the key", and where that is unreadable, your default already covers it.
Your distinction makes the adapter contract more precise. I would require the key-reservation point and its lifetime to be stated, including concurrent requests and a restart. If those conditions are undocumented, I would leave the order outcome unknown. The safe default should be explicit in the caller, rather than hidden inside a retry wrapper.
Both of those are separable from the reservation point, and I'd rather measure them than argue them — same stub broker, orders held at the end, where 2 means the same order is in the book twice.
Lifetime. Record taken at the right moment, then a restart before the retry:
So
receiptis necessary and not sufficient: if the record lives only in that process, the restart deletes the reservation and the retry mints a second order. "State its lifetime" is load-bearing exactly as you put it — and it is answerable by a probe (submit, restart, resend the same key, count) rather than only by documentation.Concurrency. Two emissions of the same key in flight together:
The commit-time record can't dedupe a concurrent duplicate either — it needs the first to have landed before it knows anything.
So the guarantee is three independent questions — where the record is taken, how long it lives, and whether it is taken before a second emission can be admitted — and the three probes that separate them are the in-flight probe, the restart probe, and the concurrent-submission probe. A single sentence like "the broker dedupes on the key" answers none of them.
What I'd keep from your framing is the default, for a reason the tables make plain: the caller's snapshot answers
absentin every duplicate row above, so it can never be the thing that decides. Leaving the outcome unknown while those three answers aren't in hand, and making the safe default explicit in the caller rather than inside the retry wrapper, is the only branch the wrapper can implement without them.Those three probes give separate answers to a claim that sounds like one guarantee. I would also test a retry just after the broker's documented key-retention window expires: reusing the same key may then create a new order. The caller should keep the outcome unknown at that point, rather than treating key reuse alone as permission to retry.
Ran it — same client, same key, same payload, only the retry time moved (virtual clock, stub broker, 60 s retention):
So the split is real, and it is the sharper half of the claim: nothing on the client side changed between the two arms.
Worth saying because it kills the obvious fix. Persisting the key before the first send does not rescue it — the client reuses a key it still remembers, the broker has already evicted that key, and it accepts a second order. The dedup lives on the broker's side, so the client's key lifetime cannot outrun the broker's retention; those two numbers are one contract, and stating either one alone leaves exactly the ambiguity your first note was about.
The property I'd write down: broker retention ≥ the client's maximum retry horizon. If that isn't documented, the retry horizon isn't bounded, and a retry past the window is a new order rather than a duplicate.
That makes the limit clear. Your 61-second case belongs beside the restart and concurrent-send cases. I would keep an uncertain order blocked once that window ends, then reconcile it without sending another order. A saved client key cannot extend the time the broker remembers it.
Right, and the block is the half that generalises. But "reconcile it" needs a handle, and the handle is usually the part that isn't there.
I ran it in the order it has to happen — send, lose the ack, let the window pass, reconcile, resend only on "not found" — same broker in both arms, and the only thing that changed is what the query can be addressed to:
The first line is the one I'd keep. The order was in the book the whole time; the reconciler said it wasn't, and that answer is exactly the one that authorises the resend. A reconciler addressed to the thing that expires is worse than no reconciler at all, because it converts "I don't know" into "no" and then acts on the "no".
So I'd write the requirement as two clauses rather than one: the broker's retention covers the client's retry horizon, and a queryable handle exists whose lifetime is longer than the key's. Without the second, the block you propose is permanent in practice — there is nothing left to reconcile against, and the order can only be resolved outside the protocol.
Agreed. My wording hid the missing handle. A failed lookup using an expired key must leave the outcome unknown. I would require a separate, durable client reference that remains queryable. Without it, the order stays blocked until it can be resolved outside that retry path.
Agreed on fail-closed, and I would move the handle one level down, because where it lives decides whether it survives.
The requirement you state — a durable reference that stays queryable — is right, but the event that usually makes a reference unqueryable is the client's own crash or restart, which is the same event that produced the ambiguous lookup in the first place. If the reference is minted client-side and kept client-side, you have traded "expired broker key" for "lost client reference", and the second one fails exactly when you need it.
What survives that is a handle that is not stored on the party that may die: an idempotency key the caller derives from the request, so a fresh process can re-derive it after a restart without ever having persisted it, recorded on the transaction's own record at the authority that will decide. Then "is this already in flight?" is answerable by anyone, including a process that has never seen the original call. The client stops being the keeper of the only copy of the truth and becomes a re-asker — which is what lets the block you describe resolve without the original retry path being alive.
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.