CircuitBreaker: failureCount never resets during normal (closed) operation, causing spurious trips on long-running instances

[KrisBuytaert]

Bug: PuppetDB circuit breaker's failure counter never resets during normal operation, causing spurious trips on long-running instances

Version: v1.4.0 (313adcc8ed07965d6605e8a92a1a3ed4f32aa718)

Summary

CircuitBreaker (backend/src/integrations/puppetdb/CircuitBreaker.ts - onSuccess() at line 160, onFailure() at line 174, reset() at line 259) accumulates failureCount for the entire process lifetime while the circuit is closed. It only resets to 0 when a half-open probe succeeds (line 166) or the breaker is force-reset (line 324). There is no decay, no rolling time window, and no reset on ordinary success while closed.

The practical effect: on a long-running Pabawi instance talking to a perfectly healthy PuppetDB, any 5 failures — however sparse, however unrelated, however far apart in time — will eventually sum past failureThreshold (default 5) and open the circuit, even if thousands of successful requests happened in between. This reads to an operator as "PuppetDB is having intermittent problems" when in fact PuppetDB may never have been unhealthy at all.

Where it happens

onSuccess():

onSuccess() {
    this.successCount++;
    this.lastSuccessTime = Date.now();
    if (this.state === "half-open") {
        this.reset();                 // only resets here
        this.transitionTo("closed");
    }
}

onFailure():

onFailure() {
    this.failureCount++;              // never decremented while closed
    this.lastFailureTime = Date.now();
    if (this.state === "half-open") {
        this.transitionTo("open");
        this.openedAt = Date.now();
    }
    else if (this.state === "closed") {
        if (this.failureCount >= this.config.failureThreshold) {
            this.transitionTo("open");
            ...
        }
    }
}

A success while state === "closed" does nothing to failureCount. It only ever goes down via reset(), which is only called from a successful half-open probe (i.e., only after the circuit has already tripped once).

Observed in the wild

Homelab instance, PuppetDB confirmed healthy throughout (service never restarted, load average ~0.05-0.15, no errors in PuppetDB's own logs) for the full window below. Pabawi process uptime: ~5h17m at time of trip.

15:02:05  WARN  Failed to query nodes for OS family grouping     (bare catch, no error detail logged)
16:10:55  WARN  Failed to query nodes for OS family grouping     (bare catch, no error detail logged)
16:34:11  ERROR Failed to fetch events for report '...': PuppetDB API error: Server Error   (only occurrence with a real logged cause)
20:08:43  ERROR [PuppetDB] Circuit breaker opened after 5 failures {"failureCount":5}
20:08:43  WARN  Failed to query nodes for OS family grouping
20:11:08  INFO  State transition: open -> half-open
20:11:08  INFO  State transition: half-open -> closed          (recovered on first probe)

Only one of the five accumulated failures has any diagnostic detail at all — the rest are swallowed by bare catch {} blocks (see below), so their actual cause is unknowable from the logs. Whatever they were, they were sparse (roughly one every 1-5 hours) and clearly not symptomatic of a sustained outage, since PuppetDB served large numbers of concurrent successful queries throughout the same window.

Once opened, the circuit recovered on the very first half-open probe two minutes later — consistent with the underlying service having been fine the whole time.

Secondary issue noticed along the way: swallowed errors

PuppetDBService.ts, around the OS-family-grouping query (line 665-677):

const osResult = await this.executeWithResilience(async () => {
  return await client.query(
    "pdb/query/v4/nodes",
    JSON.stringify(["extract", ["certname", ["fact", "os.family"]]])
  );
});

if (Array.isArray(osResult)) {
  const osGroups = this.createOSFamilyGroups(osResult as { certname: string; "os.family": string }[]);
  groups.push(...osGroups);
}
} catch {
  this.log("Failed to query nodes for OS family grouping", "warn");
}

The catch block doesn't bind or log the caught error at all, so there's no way to tell from the logs whether a given failure was a timeout, a connection error, a malformed response, or something else. This made root-causing the circuit trip much harder than it should have been - recommend at minimum logging error.message (and ideally error.name/status code where available) alongside the existing warning text. This pattern repeats at several other call sites in the same file.

Suggested fix

Any of the following would resolve the false-positive-trip issue (not mutually exclusive):

1. Reset (or decrement) failureCount on success while closed, not just on a successful half-open probe. E.g. onSuccess() could zero failureCount any time state === "closed", so only consecutive failures (not lifetime-cumulative ones) can trip the breaker.
2. Time-windowed failure counting - only count failures within a rolling window (e.g. last 60s), matching how most circuit breaker implementations (e.g. Polly, opossum) actually work, instead of an unbounded lifetime count.
3. At minimum, log the actual error in the swallowed catch blocks so operators aren't left guessing what the 5 accumulated failures actually were.

Happy to open this as a real upstream issue/PR if useful — this write-up is just the internal draft for review first.