The patient reconnect gives up on permanent failures (issue 058 review)

The retry loop treated everything but a cert-pin mismatch as transient,
so a refused login (revoked/mis-sealed credential) or a malformed broker
URL retried for ever logging 'not reachable yet' — the silent
non-progress the fix set out to remove, and a contradiction of its own
docstring. fatalBrokerReason now classifies those three as fatal and
everything else (connection refused, timeout, DNS) as retryable, with a
unit test covering the split — the honest proof the bed cannot give,
since it only ever starts the consumer after the broker is up.

The pin case is now a typed PinMismatchError caught by instanceof, not a
prose substring a reword could silently downgrade to an infinite retry
against an impostor. Added a little jitter so modules do not stampede a
recovering broker in lockstep.
This commit is contained in:
2026-09-20 13:30:52 +02:00
parent 0ea3db3b20
commit 5b111da4a9
3 changed files with 97 additions and 8 deletions
+46
View File
@@ -0,0 +1,46 @@
import { test } from "node:test";
import assert from "node:assert/strict";
import { fatalBrokerReason, PinMismatchError } from "../src/broker-amqp.ts";
// novox/hq issue 058 (and its review): serve mode retries a broker that is not up yet, but must
// give up at once on a failure waiting cannot fix — otherwise a permanent fault loops for ever
// disguised as "not reachable". This is the classifier that draws the line; the bed cannot test it
// (it starts the consumer only after the broker is up), so it is proven here.
test("a broker that is not up yet is retryable, not fatal", () => {
for (const err of [
Object.assign(new Error("connect ECONNREFUSED 10.42.0.1:5671"), { code: "ECONNREFUSED" }),
Object.assign(new Error("connect ETIMEDOUT"), { code: "ETIMEDOUT" }),
Object.assign(new Error("getaddrinfo EAI_AGAIN anchor.internal"), { code: "EAI_AGAIN" }),
new Error("timed out fetching the broker's certificate"),
]) {
assert.equal(fatalBrokerReason(err), null, `should retry: ${(err as Error).message}`);
}
});
test("a certificate that does not match the pin is fatal, by type not by message", () => {
// Typed, so rewording the message cannot turn an impostor back into an infinite retry.
assert.notEqual(fatalBrokerReason(new PinMismatchError("anything at all")), null);
// A plain Error with pin-ish words is NOT treated as the pin case — only the type is.
assert.equal(fatalBrokerReason(new Error("the pinned value was fine")), null);
});
test("a malformed broker URL is fatal — it never parses on the next try", () => {
assert.notEqual(fatalBrokerReason(Object.assign(new Error("Invalid URL"), { code: "ERR_INVALID_URL" })), null);
assert.notEqual(fatalBrokerReason(new Error("Invalid URL: not-a-url")), null);
});
test("a refused login is fatal — a wrong or revoked credential, not an absent broker", () => {
for (const msg of [
"Handshake terminated by server: 403 (ACCESS-REFUSED) with message \"ACCESS_REFUSED - Login was refused\"",
"Login was refused using authentication mechanism PLAIN",
"ACCESS_REFUSED",
]) {
assert.notEqual(fatalBrokerReason(new Error(msg)), null, `should be fatal: ${msg}`);
}
});
test("a non-Error value does not crash the classifier", () => {
assert.equal(fatalBrokerReason("just a string"), null);
assert.equal(fatalBrokerReason(undefined), null);
});