connor4312/cockatiel

CountBreaker state exposes samples array by reference, causing shared state and negative counters

Github

Github

Back to all

Summary

  • Context: CountBreaker is a circuit breaker policy that tracks failures in a sliding window using an internal array of boolean samples, with success/failure counters that must stay synchronized with the array contents.

  • Bug: The state getter returns the internal samples array by reference instead of a copy, and the state setter uses Object.assign which copies the reference, causing multiple breaker instances to share the same underlying array.

  • Actual vs. expected: When state is copied between two CountBreaker instances without JSON serialization, they share the same samples array, causing modifications to one breaker to corrupt the other’s internal state (the samplesarray changes but counters don’t update). Expected behavior is that each breaker instance has independent state.

  • Impact: When one breaker modifies the shared array, the other breaker’s samples array is modified without updating its successes/failures counters, leading to negative counter values and incorrect circuit-breaking decisions.

Code with bug

public get state(): unknown {
  return {
    samples: this.samples,  // <-- BUG 🔴 Returns array by reference, not a copy
    currentSample: this.currentSample,
    failures: this.failures,
    successes: this.successes,
  } satisfies ICountBreakerState;
}
public set state(value: unknown) {
  Object.assign(this, value);
  // <-- BUG 🔴 Copies array reference, causing sharing
}

Failing test

import { expect } from 'chai';
import { CircuitState } from './CircuitBreakerPolicy';
import { CountBreaker } from './breaker/CountBreaker';

describe('CountBreaker samples array reference bug', () => {
  it('demonstrates that samples array is shared by reference, causing state corruption', () => {
    // Create breaker1 with some state
    const breaker1 = new CountBreaker({ threshold: 0.5, size: 5 });

    // Add 3 failures to breaker1
    breaker1.failure(CircuitState.Closed); // samples[0] = false, failures = 1
    breaker1.failure(CircuitState.Closed); // samples[1] = false, failures = 2
    breaker1.failure(CircuitState.Closed); // samples[2] = false, failures = 3

    // Verify breaker1's initial state
    expect((breaker1 as any).failures).to.equal(3);
    expect((breaker1 as any).successes).to.equal(0);
    expect((breaker1 as any).currentSample).to.equal(3);

    // Get state from breaker1
    const state = breaker1.state;

    // Create breaker2 with fresh state, then assign breaker1's state
    const breaker2 = new CountBreaker({ threshold: 0.5, size: 5 });
    breaker2.state = state;

    // Verify that breaker1 and breaker2 share the SAME samples array
    const breaker1Samples = (breaker1 as any).samples;
    const breaker2Samples = (breaker2 as any).samples;
    expect(breaker1Samples).to.equal(breaker2Samples); // They are the same object!

    // Now modify breaker2 by calling success
    breaker2.success(CircuitState.Closed); // samples[3] = true, successes = 1

    // Check breaker2's state - this should be correct
    expect((breaker2 as any).failures).to.equal(3);
    expect((breaker2 as any).successes).to.equal(1);
    expect((breaker2 as any).samples[3]).to.equal(true);
    expect((breaker2 as any).currentSample).to.equal(4);

    // Check breaker1's state - THIS REVEALS THE BUG
    expect((breaker1 as any).failures).to.equal(3);      // Still 3 (not updated)
    expect((breaker1 as any).successes).to.equal(0);     // Still 0 (not updated)
    expect((breaker1 as any).samples[3]).to.equal(true); // CHANGED! (bug)
    expect((breaker1 as any).currentSample).to.equal(3); // Still 3 (not updated)

    // When breaker1 is used next, it will try to replace samples[3]
    // It will decrement successes (making it -1!) because samples[3] is true
    const breaker1SuccessesBefore = (breaker1 as any).successes; // 0
    breaker1.failure(CircuitState.Closed); // Replace samples[3] with false
    const breaker1SuccessesAfter = (breaker1 as any).successes; // 0 - 1 = -1 !

    // The bug: successes is now negative!
    expect(breaker1SuccessesAfter).to.equal(-1); // This demonstrates the corruption
  });
});

Test output (key observations):

  • breaker1Samples === breaker2Samples -> true (same array reference)

  • After breaker1.failure(...), breaker1SuccessesAfter -> -1 (assertion passes)

Recommended fix

The state getter should return a copy of the samples array instead of a reference:

public get state(): unknown {
  return {
    samples: [...this.samples],  // <-- FIX 🟢 Return a shallow copy
    currentSample: this.currentSample,
    failures: this.failures,
    successes: this.successes,
  } satisfies ICountBreakerState;
}

Alternatively, the state setter could defensively clone the incoming array:

public set state(value: unknown) {
  const typed = value as ICountBreakerState;

  this.samples = [...typed.samples]; // <-- FIX 🟢 Clone the array
  this.currentSample = typed.currentSample;
  this.failures = typed.failures;
  this.successes = typed.successes;
}

The getter fix is preferred because:

  1. It prevents the issue at the source

  2. It maintains the invariant that the returned state is independent

  3. It’s consistent with principle of encapsulation

  4. It has minimal performance impact (shallow copy of a fixed-size array)