fix(client): say which network failure happened, not just "unreachable" - #133
Merged
Conversation
`JobdClient._request` caught four distinct httpx transport failures and
collapsed them into one `BrokerUnreachable` whose only payload was a
formatted string. Every consumer therefore rendered identical text for two
situations with opposite remedies:
refused -> something answered and said no. The path is fine;
the broker process is not running.
timeout -> nothing answered at all. The broker is probably fine;
the path is not (firewall, ACL, routing).
This is not hypothetical. On 2026-09-02 the gt76 broker was healthy —
container up, listening, freshly auto-deployed to 0.5.43 — and a Tailscale
ACL was dropping packets because the machine had re-registered as a second,
untagged node. `job ping` said `health: unreachable`, and that was read as
"the broker is down and the fleet never upgraded" and reported as such
before the host was checked.
Classify once, where the exception is caught, so every consumer gets it
without re-deriving it from a message string: `BrokerUnreachable` now carries
`kind` (dns / refused / timeout / read_timeout / tls / network) and an
operator-facing `hint`. `job ping` and the `main()` error boundary render
both; the boundary previously advised "is the broker running?", which is
actively wrong for a dropped packet.
Also stop calling a broker that ANSWERED "unreachable". A 401, a 5xx, or a
non-ok body now render as "reachable, but not healthy" (exit code stays 2),
and `--json` gains `kind`/`hint` while `reachable` comes to mean "the broker
sent us bytes" rather than "nothing went wrong". One existing assertion
required the word "unreachable" for any non-ok body; its comment described
the defect rather than the intent, and it is updated here.
Tests drive real sockets rather than fixtures. Note the loopback probe: the
obvious ECONNREFUSED source — a closed port on 127.0.0.1 — is not portable.
Under WSL2 `networkingMode=mirrored`, IPv4 loopback to an unbound port is
silently DROPPED while IPv6 `::1` still returns RST, so a test assuming
refusal there passes on normal Linux and fails on this repo's dev machine.
The tests ask the kernel which family does what and skip with a stated
reason, which also makes the probe a positive control: it returns an address
only after observing the errno the assertion depends on.
Mutation-verified: forcing the ConnectTimeout branch to return "refused"
reddens only the two timeout tests and leaves the refused test green, so the
two paths are discriminated independently rather than keyed off one branch;
reverting `reachable` to `error is None` reddens only the 401 test.
`tests/test_changelog_fragments.py` requires every fragment to start with `- ` (changelog.d/README.md: "The first line must start with `- `"), because scripts/roll-changelog.py splices fragments straight into a category's bullet list at release time — a prose paragraph would land mid-list unindented. Caught by CI rather than locally, and worth naming why: the local full-suite run was already in flight when the fragment was written, so the one file the run did not cover was the one file added after it started. Validate, then add a file, and the validation no longer describes the tree.
The MCP transport-error path returned `jobd transport error: {e}` — the raw
exception string, with none of the `kind`/`hint` the client now carries. So
the surface where the diagnosis matters MOST was the one still withholding it.
An agent cannot ssh to the host to check whether the broker is actually down,
which makes it more dependent on the steer than a human operator, not less.
From a bare "timed out" the obvious inference is "the broker is down" — wrong
for a dropped packet, and an agent acting on it might restart a service that
was fine. On 2026-09-02 a human made exactly that inference about a broker
that was healthy and freshly upgraded, with more evidence available than an
agent would have had.
An agent now receives:
jobd transport error (timeout): ConnectTimeout: timed out (JOBD_URL=...)
hint: the connection was dropped rather than refused, which points at a
firewall, a tailnet ACL, or routing — not at the broker. [...]
The JSONL call log also records `transport_<kind>` instead of a flat
`transport`, so the telemetry can distinguish an ACL drop from a dead broker
after the fact.
The new protocol test asserts the parenthesised kind rather than a bare
substring: `"timeout" in text` would also match the exception's own class
name and pass without the fix. Positive control in the same test, matching
its sibling.
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 client caught four distinct httpx transport failures and collapsed them into one
BrokerUnreachablecarrying only a formatted string, so every consumer rendered identical text for two situations with opposite remedies:This came out of a live incident. On 2026-09-02 the gt76 broker was healthy (container up, listening, freshly auto-deployed to 0.5.43) while a Tailscale ACL dropped packets, because the machine had re-registered as a second, untagged node.
job pingreportedhealth: unreachable, and that was read as "the broker is down and the fleet never upgraded" — and reported that way before the host was checked.What changed
BrokerUnreachablenow carrieskind(dns/refused/timeout/read_timeout/tls/network) and an operator-facinghint, classified once where the exception is caught so no consumer has to re-derive it from a message string.job pingand themain()boundary render both — the boundary previously advised "is the broker running?", which is actively wrong for a dropped packet.job pingalso stops calling a broker that answered "unreachable": a 401, a 5xx, or a non-ok body now render as "reachable, but not healthy" (exit code still 2).--jsongainskind/hint, andreachablenow means "the broker sent us bytes".Real output for the incident shape:
On the tests
They drive real sockets. The obvious ECONNREFUSED source — a closed port on
127.0.0.1— is not portable: under WSL2networkingMode=mirrored, IPv4 loopback to an unbound port is silently dropped while IPv6::1still returns RST. A test assuming refusal there passes on normal Linux and fails on this repo's dev machine. The tests ask the kernel which family does what and skip with a stated reason, which doubles as a positive control — the probe returns an address only after observing the errno the assertion depends on.One existing assertion required the word "unreachable" for any non-ok body; its comment described the defect rather than the intent, and is updated.
Verification
ConnectTimeoutbranch to return"refused"reddens only the two timeout tests and leaves the refused test green, proving the paths are discriminated independently rather than keyed off one branch. Revertingreachabletoerror is Nonereddens only the 401 test.